GBE: Fix a assert in CleanLlvmResource.

Submitted by Yang, Rong R on July 24, 2017, 9:09 a.m.

Details

Message ID 1500887363-11782-1-git-send-email-rong.r.yang@intel.com
State New
Headers show
Series "GBE: Fix a assert in CleanLlvmResource." ( rev: 1 ) in Beignet

Not browsing as part of any series.

Commit Message

Yang, Rong R July 24, 2017, 9:09 a.m.
From: Zhu Bingbing <bingbingx.zhu@intel.com>

After llvm3.9, the global context is a static variable. When call
CleanLlvmResource, this global context may has been deleted. When
delete the context, the module been deleted. So check the context
before delete module.

Signed-off-by: Yang Rong <rong.r.yang@intel.com>
---
 backend/src/backend/gen_program.cpp |  8 +++++++-
 backend/src/backend/program.cpp     |  4 ++--
 backend/src/llvm/llvm_to_gen.cpp    | 15 +++++++++++++--
 backend/src/llvm/llvm_to_gen.hpp    |  2 +-
 4 files changed, 23 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/backend/src/backend/gen_program.cpp b/backend/src/backend/gen_program.cpp
index bb1d22f..e7a88b4 100644
--- a/backend/src/backend/gen_program.cpp
+++ b/backend/src/backend/gen_program.cpp
@@ -126,6 +126,12 @@  namespace gbe {
 
   void GenProgram::CleanLlvmResource(void){
 #ifdef GBE_COMPILER_AVAILABLE
+#if LLVM_VERSION_MAJOR * 10 + LLVM_VERSION_MINOR >= 39
+    llvm::LLVMContext* c = GBEGetLLVMContext();
+    //context has been deleted, the module instantiated in the context
+    //also been deleted when delete context.
+    if (c == NULL) return;
+#endif
     if(module){
       delete (llvm::Module*)module;
       module = NULL;
@@ -353,7 +359,7 @@  namespace gbe {
     binary_content.assign(binary+1, size-1);
     llvm::StringRef llvm_bin_str(binary_content);
 #if LLVM_VERSION_MAJOR * 10 + LLVM_VERSION_MINOR >= 39
-    llvm::LLVMContext& c = GBEGetLLVMContext();
+    llvm::LLVMContext& c = *GBEGetLLVMContext();
 #else
     llvm::LLVMContext& c = llvm::getGlobalContext();
 #endif
diff --git a/backend/src/backend/program.cpp b/backend/src/backend/program.cpp
index c06ae5a..fda2b33 100644
--- a/backend/src/backend/program.cpp
+++ b/backend/src/backend/program.cpp
@@ -1104,7 +1104,7 @@  EXTEND_QUOTE:
       return NULL;
 
 #if LLVM_VERSION_MAJOR * 10 + LLVM_VERSION_MINOR >= 39
-    llvm::LLVMContext& c = GBEGetLLVMContext();
+    llvm::LLVMContext& c = *GBEGetLLVMContext();
 #else
     llvm::LLVMContext& c = llvm::getGlobalContext();
 #endif
@@ -1156,7 +1156,7 @@  EXTEND_QUOTE:
     //for some functions, so we use global context now, need switch to new context later.
     llvm::Module * out_module;
 #if LLVM_VERSION_MAJOR * 10 + LLVM_VERSION_MINOR >= 39
-    llvm::LLVMContext* llvm_ctx = &GBEGetLLVMContext();
+    llvm::LLVMContext* llvm_ctx = GBEGetLLVMContext();
 #else
     llvm::LLVMContext* llvm_ctx = &llvm::getGlobalContext();
 #endif
diff --git a/backend/src/llvm/llvm_to_gen.cpp b/backend/src/llvm/llvm_to_gen.cpp
index 8546f73..a155459 100644
--- a/backend/src/llvm/llvm_to_gen.cpp
+++ b/backend/src/llvm/llvm_to_gen.cpp
@@ -47,8 +47,19 @@  namespace gbe
   using namespace llvm;
 
 #if LLVM_VERSION_MAJOR * 10 + LLVM_VERSION_MINOR >= 39
-  llvm::LLVMContext& GBEGetLLVMContext() {
-    static llvm::LLVMContext GBEContext;
+  llvm::LLVMContext *GBEContext = NULL;
+  bool initialized = false;
+
+  void destroyLLVMContext (void) {
+    delete GBEContext;
+    GBEContext = NULL;
+  }
+  llvm::LLVMContext* GBEGetLLVMContext() {
+    if (initialized == false) {
+      GBEContext = new llvm::LLVMContext();
+      atexit(destroyLLVMContext);
+      initialized = true;
+    }
     return GBEContext;
   }
 #endif
diff --git a/backend/src/llvm/llvm_to_gen.hpp b/backend/src/llvm/llvm_to_gen.hpp
index 73e8819..56b8619 100644
--- a/backend/src/llvm/llvm_to_gen.hpp
+++ b/backend/src/llvm/llvm_to_gen.hpp
@@ -38,7 +38,7 @@  namespace gbe {
   bool llvmToGen(ir::Unit &unit, const void* module,
                  int optLevel, bool strictMath, int profiling, std::string &errors);
 #if LLVM_VERSION_MAJOR * 10 + LLVM_VERSION_MINOR >= 39
-  extern llvm::LLVMContext& GBEGetLLVMContext();
+  extern llvm::LLVMContext* GBEGetLLVMContext();
 #endif
 
 } /* namespace gbe */

Comments

I think using atexit() in a dlopen library may be unsafe.
Take a look at this.
https://stackoverflow.com/questions/17350308/atexit-considered-harmful
"Interaction of atexit with dlopen or other methods of loading libraries dynamically is not defined.
A library which has registered atexit handlers cannot safely be unloaded, and the ways different implementations deal with this situation can vary."
It may cause different behavior on different implementations. I am not sure if anybody else has more comments on this?

My suggestion is to avoid using global LLVM context.
The reason we use global context is for linking two modules.
Here is some suggestion for linking modules from different context:
http://lists.llvm.org/pipermail/llvm-dev/2015-June/086296.html
although it will introduce some more linking time. But I think things becomes more clear. And we don't need to acquire/release global context lock.

Thanks!
Ruiling

> -----Original Message-----

> From: Beignet [mailto:beignet-bounces@lists.freedesktop.org] On Behalf Of

> Yang Rong

> Sent: Monday, July 24, 2017 5:09 PM

> To: beignet@lists.freedesktop.org

> Cc: Yang, Rong R <rong.r.yang@intel.com>; Zhu Bingbing

> <bingbingx.zhu@intel.com>

> Subject: [Beignet] [PATCH] GBE: Fix a assert in CleanLlvmResource.

> 

> From: Zhu Bingbing <bingbingx.zhu@intel.com>

> 

> After llvm3.9, the global context is a static variable. When call

> CleanLlvmResource, this global context may has been deleted. When

> delete the context, the module been deleted. So check the context

> before delete module.

> 

> Signed-off-by: Yang Rong <rong.r.yang@intel.com>

> ---

>  backend/src/backend/gen_program.cpp |  8 +++++++-

>  backend/src/backend/program.cpp     |  4 ++--

>  backend/src/llvm/llvm_to_gen.cpp    | 15 +++++++++++++--

>  backend/src/llvm/llvm_to_gen.hpp    |  2 +-

>  4 files changed, 23 insertions(+), 6 deletions(-)

> 

> diff --git a/backend/src/backend/gen_program.cpp

> b/backend/src/backend/gen_program.cpp

> index bb1d22f..e7a88b4 100644

> --- a/backend/src/backend/gen_program.cpp

> +++ b/backend/src/backend/gen_program.cpp

> @@ -126,6 +126,12 @@ namespace gbe {

> 

>    void GenProgram::CleanLlvmResource(void){

>  #ifdef GBE_COMPILER_AVAILABLE

> +#if LLVM_VERSION_MAJOR * 10 + LLVM_VERSION_MINOR >= 39

> +    llvm::LLVMContext* c = GBEGetLLVMContext();

> +    //context has been deleted, the module instantiated in the context

> +    //also been deleted when delete context.

> +    if (c == NULL) return;

> +#endif

>      if(module){

>        delete (llvm::Module*)module;

>        module = NULL;

> @@ -353,7 +359,7 @@ namespace gbe {

>      binary_content.assign(binary+1, size-1);

>      llvm::StringRef llvm_bin_str(binary_content);

>  #if LLVM_VERSION_MAJOR * 10 + LLVM_VERSION_MINOR >= 39

> -    llvm::LLVMContext& c = GBEGetLLVMContext();

> +    llvm::LLVMContext& c = *GBEGetLLVMContext();

>  #else

>      llvm::LLVMContext& c = llvm::getGlobalContext();

>  #endif

> diff --git a/backend/src/backend/program.cpp

> b/backend/src/backend/program.cpp

> index c06ae5a..fda2b33 100644

> --- a/backend/src/backend/program.cpp

> +++ b/backend/src/backend/program.cpp

> @@ -1104,7 +1104,7 @@ EXTEND_QUOTE:

>        return NULL;

> 

>  #if LLVM_VERSION_MAJOR * 10 + LLVM_VERSION_MINOR >= 39

> -    llvm::LLVMContext& c = GBEGetLLVMContext();

> +    llvm::LLVMContext& c = *GBEGetLLVMContext();

>  #else

>      llvm::LLVMContext& c = llvm::getGlobalContext();

>  #endif

> @@ -1156,7 +1156,7 @@ EXTEND_QUOTE:

>      //for some functions, so we use global context now, need switch to new

> context later.

>      llvm::Module * out_module;

>  #if LLVM_VERSION_MAJOR * 10 + LLVM_VERSION_MINOR >= 39

> -    llvm::LLVMContext* llvm_ctx = &GBEGetLLVMContext();

> +    llvm::LLVMContext* llvm_ctx = GBEGetLLVMContext();

>  #else

>      llvm::LLVMContext* llvm_ctx = &llvm::getGlobalContext();

>  #endif

> diff --git a/backend/src/llvm/llvm_to_gen.cpp

> b/backend/src/llvm/llvm_to_gen.cpp

> index 8546f73..a155459 100644

> --- a/backend/src/llvm/llvm_to_gen.cpp

> +++ b/backend/src/llvm/llvm_to_gen.cpp

> @@ -47,8 +47,19 @@ namespace gbe

>    using namespace llvm;

> 

>  #if LLVM_VERSION_MAJOR * 10 + LLVM_VERSION_MINOR >= 39

> -  llvm::LLVMContext& GBEGetLLVMContext() {

> -    static llvm::LLVMContext GBEContext;

> +  llvm::LLVMContext *GBEContext = NULL;

> +  bool initialized = false;

> +

> +  void destroyLLVMContext (void) {

> +    delete GBEContext;

> +    GBEContext = NULL;

> +  }

> +  llvm::LLVMContext* GBEGetLLVMContext() {

> +    if (initialized == false) {

> +      GBEContext = new llvm::LLVMContext();

> +      atexit(destroyLLVMContext);

> +      initialized = true;

> +    }

>      return GBEContext;

>    }

>  #endif

> diff --git a/backend/src/llvm/llvm_to_gen.hpp

> b/backend/src/llvm/llvm_to_gen.hpp

> index 73e8819..56b8619 100644

> --- a/backend/src/llvm/llvm_to_gen.hpp

> +++ b/backend/src/llvm/llvm_to_gen.hpp

> @@ -38,7 +38,7 @@ namespace gbe {

>    bool llvmToGen(ir::Unit &unit, const void* module,

>                   int optLevel, bool strictMath, int profiling, std::string &errors);

>  #if LLVM_VERSION_MAJOR * 10 + LLVM_VERSION_MINOR >= 39

> -  extern llvm::LLVMContext& GBEGetLLVMContext();

> +  extern llvm::LLVMContext* GBEGetLLVMContext();

>  #endif

> 

>  } /* namespace gbe */

> --

> 2.1.4

> 

> _______________________________________________

> Beignet mailing list

> Beignet@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/beignet