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

[Song, Ruiling]

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 at lists.freedesktop.org] On Behalf Of
> Yang Rong
> Sent: Monday, July 24, 2017 5:09 PM
> To: beignet at lists.freedesktop.org
> Cc: Yang, Rong R <rong.r.yang at intel.com>; Zhu Bingbing
> <bingbingx.zhu at intel.com>
> Subject: [Beignet] [PATCH] GBE: Fix a assert in CleanLlvmResource.
> 
> From: Zhu Bingbing <bingbingx.zhu at 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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/beignet