> -----Original Message----- > From: Luo, Xionghu > Sent: Thursday, September 17, 2015 15:49 > To: Yang, Rong R; [email protected] > 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:[email protected]] > Sent: Monday, August 24, 2015 12:00 PM > To: [email protected] > 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 <[email protected]> > --- > 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 > [email protected] > http://lists.freedesktop.org/mailman/listinfo/beignet _______________________________________________ Beignet mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/beignet
