[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