[Beignet] [PATCH] Fix piglit clLinkProgram fail.
Yang, Rong R
rong.r.yang at intel.com
Thu Sep 17 19:50:01 PDT 2015
> -----Original Message-----
> From: Luo, Xionghu
> Sent: Thursday, September 17, 2015 15:49
> To: Yang, Rong R; beignet at lists.freedesktop.org
> Cc: Yang, Rong R
> Subject: RE: [Beignet] [PATCH] Fix piglit clLinkProgram fail.
>
> One potential memory leak needs confirmation.
> Since this case is passed, the error message of invalid value is confusing, we
> may remove this " error in /beignet/src/cl_api.c line 1014 Invalid value "
>
OK, I will remove the messages and push it.
> Other parts of this patch LGTM.
>
> Luo Xionghu
> Best Regards
>
> -----Original Message-----
> From: Yang Rong [mailto:rong.r.yang at intel.com]
> Sent: Monday, August 24, 2015 12:00 PM
> To: beignet at lists.freedesktop.org
> Cc: Yang, Rong R
> Subject: [Beignet] [PATCH] Fix piglit clLinkProgram fail.
>
> 1. return CL_INVALID_LINKER_OPTIONS when invalid options, using clang to
> check the options.
> 2. return CL_INVALID_OPERATION when the binary type is not same.
> 3. When link fail, will not return CL_LINK_PROGRAM_FAILURE, fix it.
> 4. Should not delete program in genProgramBuildFromLLVM, the program is
> new and delete from runtime.
>
> Signed-off-by: Yang Rong <rong.r.yang at intel.com>
> ---
> backend/src/backend/gen_program.cpp | 5 ++--
> backend/src/backend/program.cpp | 46
> +++++++++++++++++++++++++++++++++++--
> backend/src/backend/program.h | 10 ++++++--
> src/cl_api.c | 1 +
> src/cl_gbe_loader.cpp | 5 ++++
> src/cl_gbe_loader.h | 1 +
> src/cl_program.c | 20 +++++++++++++---
> 7 files changed, 79 insertions(+), 9 deletions(-)
>
> diff --git a/backend/src/backend/gen_program.cpp
> b/backend/src/backend/gen_program.cpp
> index 3c4983e..04da692 100644
> --- a/backend/src/backend/gen_program.cpp
> +++ b/backend/src/backend/gen_program.cpp
> @@ -386,7 +386,7 @@ namespace gbe {
> return (gbe_program) program;
> }
>
> - static void genProgramLinkFromLLVM(gbe_program dst_program,
> + static bool genProgramLinkFromLLVM(gbe_program dst_program,
> gbe_program src_program,
> size_t stringSize,
> char * err,
> @@ -408,10 +408,12 @@ namespace gbe {
> err[stringSize-1] = '\0';
> *errSize = strlen(err);
> }
> + return true;
> }
> }
> // Everything run fine
> #endif
> + return false;
> }
>
> static void genProgramBuildFromLLVM(gbe_program program, @@ -444,7
> +446,6 @@ namespace gbe {
> std::memcpy(err, error.c_str(), msgSize);
> *errSize = error.size();
> }
> - GBE_DELETE(p);
> }
> releaseLLVMContextLock();
> #endif
> diff --git a/backend/src/backend/program.cpp
> b/backend/src/backend/program.cpp index c02096f..9808e9e 100644
> --- a/backend/src/backend/program.cpp
> +++ b/backend/src/backend/program.cpp
> @@ -913,23 +913,63 @@ namespace gbe {
> #endif
>
> #ifdef GBE_COMPILER_AVAILABLE
> - static void programLinkProgram(gbe_program dst_program,
> + static bool programLinkProgram(gbe_program dst_program,
> gbe_program src_program,
> size_t stringSize,
> char * err,
> size_t * errSize)
> {
> + bool ret = 0;
> acquireLLVMContextLock();
>
> - gbe_program_link_from_llvm(dst_program, src_program, stringSize, err,
> errSize);
> + ret = gbe_program_link_from_llvm(dst_program, src_program,
> + stringSize, err, errSize);
>
> releaseLLVMContextLock();
>
> if (OCL_OUTPUT_BUILD_LOG && err)
> llvm::errs() << err;
> + return ret;
> }
> #endif
>
> +#ifdef GBE_COMPILER_AVAILABLE
> + static bool programCheckOption(const char * option)
> + {
> + vector<const char *> args;
> + std::string s(option);
> + size_t pos = s.find("-create-library");
> + //clang don't accept -create-library and -enable-link-options, erase them
> + if(pos != std::string::npos) {
> + s.erase(pos, strlen("-create-library"));
> + }
> + pos = s.find("-enable-link-options");
> + if(pos != std::string::npos) {
> + s.erase(pos, strlen("-enable-link-options"));
> + }
> + args.push_back(s.c_str());
> +
> + // The compiler invocation needs a DiagnosticsEngine so it can report
> problems
> + std::string ErrorString;
> + llvm::raw_string_ostream ErrorInfo(ErrorString);
> + llvm::IntrusiveRefCntPtr<clang::DiagnosticOptions> DiagOpts = new
> clang::DiagnosticOptions();
> + DiagOpts->ShowCarets = false;
> + DiagOpts->ShowPresumedLoc = true;
> +
> + clang::TextDiagnosticPrinter *DiagClient =
> + new
> + clang::TextDiagnosticPrinter(ErrorInfo, &*DiagOpts);
> Xionghu: seems this variable is not deleted.
Diags will take ownership of DiagClient, will release DiagClient in Diags destructor.
>
>
>
> + llvm::IntrusiveRefCntPtr<clang::DiagnosticIDs> DiagID(new
> clang::DiagnosticIDs());
> + clang::DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagClient);
> +
> + // Create the compiler invocation
> + std::unique_ptr<clang::CompilerInvocation> CI(new
> clang::CompilerInvocation);
> + return clang::CompilerInvocation::CreateFromArgs(*CI,
> + &args[0],
> + &args[0] + args.size(),
> + Diags);
> + }
> +#endif
> +
> +
> static size_t programGetGlobalConstantSize(gbe_program gbeProgram) {
> if (gbeProgram == NULL) return 0;
> const gbe::Program *program = (const gbe::Program*) gbeProgram; @@ -
> 1174,6 +1214,7 @@ void releaseLLVMContextLock() GBE_EXPORT_SYMBOL
> gbe_program_new_from_source_cb *gbe_program_new_from_source =
> NULL; GBE_EXPORT_SYMBOL gbe_program_compile_from_source_cb
> *gbe_program_compile_from_source = NULL; GBE_EXPORT_SYMBOL
> gbe_program_link_program_cb *gbe_program_link_program = NULL;
> +GBE_EXPORT_SYMBOL gbe_program_check_opt_cb
> *gbe_program_check_opt =
> +NULL;
> GBE_EXPORT_SYMBOL gbe_program_new_from_binary_cb
> *gbe_program_new_from_binary = NULL; GBE_EXPORT_SYMBOL
> gbe_program_new_from_llvm_binary_cb
> *gbe_program_new_from_llvm_binary = NULL; GBE_EXPORT_SYMBOL
> gbe_program_serialize_to_binary_cb *gbe_program_serialize_to_binary =
> NULL; @@ -1229,6 +1270,7 @@ namespace gbe
> gbe_program_new_from_source = gbe::programNewFromSource;
> gbe_program_compile_from_source = gbe::programCompileFromSource;
> gbe_program_link_program = gbe::programLinkProgram;
> + gbe_program_check_opt = gbe::programCheckOption;
> gbe_program_get_global_constant_size =
> gbe::programGetGlobalConstantSize;
> gbe_program_get_global_constant_data =
> gbe::programGetGlobalConstantData;
> gbe_program_clean_llvm_resource = gbe::programCleanLlvmResource;
> diff --git a/backend/src/backend/program.h
> b/backend/src/backend/program.h index fa75052..67d9db0 100644
> --- a/backend/src/backend/program.h
> +++ b/backend/src/backend/program.h
> @@ -30,6 +30,7 @@
>
> #include <stdint.h>
> #include <stdlib.h>
> +#include <stdbool.h>
>
> #ifdef __cplusplus
> extern "C" {
> @@ -180,14 +181,19 @@ typedef gbe_program
> (gbe_program_compile_from_source_cb)(uint32_t deviceID,
> char *err,
> size_t *err_size); extern
> gbe_program_compile_from_source_cb
> *gbe_program_compile_from_source;
> +
> /*! link the programs. */
> -typedef void (gbe_program_link_program_cb)(gbe_program
> dst_program,
> +typedef bool (gbe_program_link_program_cb)(gbe_program
> dst_program,
> gbe_program src_program,
> size_t stringSize,
> char * err,
> size_t * errSize);
> extern gbe_program_link_program_cb *gbe_program_link_program;
>
> +/*! check link option. */
> +typedef bool (gbe_program_check_opt_cb)(const char *option); extern
> +gbe_program_check_opt_cb *gbe_program_check_opt;
> +
> /*! create s new genprogram for link. */ typedef gbe_program
> (gbe_program_new_gen_program_cb)(uint32_t deviceID,
> const void *module, @@ -219,7 +225,7 @@
> typedef gbe_program (gbe_program_new_from_llvm_cb)(uint32_t
> deviceID, extern gbe_program_new_from_llvm_cb
> *gbe_program_new_from_llvm;
>
> /*! link the programs from llvm level. */ -typedef void
> (gbe_program_link_from_llvm_cb)(gbe_program dst_program,
> +typedef bool (gbe_program_link_from_llvm_cb)(gbe_program
> dst_program,
> gbe_program src_program,
> size_t stringSize,
> char * err,
> diff --git a/src/cl_api.c b/src/cl_api.c index 5c9b250..d0aebbd 100644
> --- a/src/cl_api.c
> +++ b/src/cl_api.c
> @@ -1015,6 +1015,7 @@ clLinkProgram(cl_context context,
> INVALID_VALUE_IF (pfn_notify == 0 && user_data != NULL);
> INVALID_VALUE_IF (num_input_programs == 0 && input_programs !=
> NULL);
> INVALID_VALUE_IF (num_input_programs != 0 && input_programs ==
> NULL);
> + INVALID_VALUE_IF (num_input_programs == 0 && input_programs ==
> NULL);
>
> program = cl_program_link(context, num_input_programs,
> input_programs, options, &err);
>
> diff --git a/src/cl_gbe_loader.cpp b/src/cl_gbe_loader.cpp index
> c3454e8..e832a53 100644
> --- a/src/cl_gbe_loader.cpp
> +++ b/src/cl_gbe_loader.cpp
> @@ -27,6 +27,7 @@ gbe_program_new_from_source_cb
> *compiler_program_new_from_source = NULL;
> gbe_program_compile_from_source_cb
> *compiler_program_compile_from_source = NULL;
> gbe_program_new_gen_program_cb
> *compiler_program_new_gen_program = NULL;
> gbe_program_link_program_cb *compiler_program_link_program = NULL;
> +gbe_program_check_opt_cb *compiler_program_check_opt = NULL;
> gbe_program_build_from_llvm_cb *compiler_program_build_from_llvm =
> NULL; gbe_program_new_from_llvm_binary_cb
> *compiler_program_new_from_llvm_binary = NULL;
> gbe_program_serialize_to_binary_cb
> *compiler_program_serialize_to_binary = NULL; @@ -279,6 +280,10 @@
> struct GbeLoaderInitializer
> if (compiler_program_link_program == NULL)
> return;
>
> + compiler_program_check_opt = *(gbe_program_check_opt_cb
> **)dlsym(dlhCompiler, "gbe_program_check_opt");
> + if (compiler_program_check_opt == NULL)
> + return;
> +
> compiler_program_build_from_llvm =
> *(gbe_program_build_from_llvm_cb **)dlsym(dlhCompiler,
> "gbe_program_build_from_llvm");
> if (compiler_program_build_from_llvm == NULL)
> return;
> diff --git a/src/cl_gbe_loader.h b/src/cl_gbe_loader.h index 6fa4c98..de91c85
> 100644
> --- a/src/cl_gbe_loader.h
> +++ b/src/cl_gbe_loader.h
> @@ -28,6 +28,7 @@ extern gbe_program_new_from_source_cb
> *compiler_program_new_from_source;
> extern gbe_program_compile_from_source_cb
> *compiler_program_compile_from_source;
> extern gbe_program_new_gen_program_cb
> *compiler_program_new_gen_program;
> extern gbe_program_link_program_cb *compiler_program_link_program;
> +extern gbe_program_check_opt_cb *compiler_program_check_opt;
> extern gbe_program_build_from_llvm_cb
> *compiler_program_build_from_llvm;
> extern gbe_program_new_from_llvm_binary_cb
> *compiler_program_new_from_llvm_binary;
> extern gbe_program_serialize_to_binary_cb
> *compiler_program_serialize_to_binary;
> diff --git a/src/cl_program.c b/src/cl_program.c index 4870d5d..ee5b8b1
> 100644
> --- a/src/cl_program.c
> +++ b/src/cl_program.c
> @@ -605,20 +605,34 @@ cl_program_link(cl_context context,
> cl_int i = 0;
> int copyed = 0;
> p = cl_program_new(context);
> + cl_bool ret = 0;
>
> if (!check_cl_version_option(p, options)) {
> err = CL_BUILD_PROGRAM_FAILURE;
> goto error;
> }
>
> + //Although we don't use options, but still need check options
> + if(!compiler_program_check_opt(options)) {
> + err = CL_INVALID_LINKER_OPTIONS;
> + goto error;
> + }
> +
> p->opaque = compiler_program_new_gen_program(context->device-
> >device_id, NULL, NULL);
>
> + for(i = 1; i < num_input_programs; i++) {
> + //num_input_programs >0 and input_programs MUST not NULL, so
> compare with input_programs[0] directly.
> + if(input_programs[i]->binary_type != input_programs[0]->binary_type) {
> + err = CL_INVALID_OPERATION;
> + goto error;
> + }
> + }
> for(i = 0; i < num_input_programs; i++) {
> // if program create with llvm binary, need deserilize first to get module.
> if(input_programs[i])
> - compiler_program_link_program(p->opaque, input_programs[i]-
> >opaque,
> - p->build_log_max_sz, p->build_log, &p->build_log_sz);
> - if (UNLIKELY(p->opaque == NULL)) {
> + ret = compiler_program_link_program(p->opaque, input_programs[i]-
> >opaque,
> + p->build_log_max_sz, p->build_log, &p->build_log_sz);
> + if (UNLIKELY(ret)) {
> err = CL_LINK_PROGRAM_FAILURE;
> goto error;
> }
> --
> 1.9.1
>
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet
More information about the Beignet
mailing list