[Beignet] [PATCH] Fix piglit clLinkProgram fail.
Luo, Xionghu
xionghu.luo at intel.com
Thu Sep 17 00:49:05 PDT 2015
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 "
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.
+ 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