You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by db...@apache.org on 2023/07/13 16:52:59 UTC
[impala] branch master updated: IMPALA-12260: Crash if '--asm_module_dir' is set
This is an automated email from the ASF dual-hosted git repository.
dbecker pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
The following commit(s) were added to refs/heads/master by this push:
new c0feea2c9 IMPALA-12260: Crash if '--asm_module_dir' is set
c0feea2c9 is described below
commit c0feea2c9f44ae4ee88c234581f7888a74d60658
Author: Daniel Becker <da...@cloudera.com>
AuthorDate: Thu Jun 29 17:55:23 2023 +0200
IMPALA-12260: Crash if '--asm_module_dir' is set
If Impala is started with the --asm_module_dir flag set and codegen
cache is used, Impala crashes.
The problem is with the lifetime of 'LlvmCodeGen::symbol_emitter_'. It
is registered as an event listener with the current
'llvm::ExecutionEngine'. Then the engine is cached but the
'LlvmCodeGen' object, which owns the symbol emitter, is destroyed at the
end of the query. When the cached execution engine is destroyed later,
it tries to notify the symbol emitter, but it has already been destroyed
so its pointer is invalid.
This change solves the problem by wrapping the execution engine and the
symbol emitter together in a wrapper class, LlvmExecutionEngineWrapper,
that is responsible for managing their lifetimes. The LlvmCodeGen and
the CodeGenCache classes now hold shared pointers to this wrapper class.
If we add other objects in the future whose lifetimes are tied to the
execution engine (but are not owned by it), they should be put into the
wrapper class.
Testing:
- added regression tests in tests/custom_cluster/test_codegen_cache.py
that fail without this change.
Change-Id: I23f871abb962ad317f9c0075ca303c09dd56bcd9
Reviewed-on: http://gerrit.cloudera.org:8080/20155
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
be/src/codegen/llvm-codegen-cache-test.cc | 29 +++++-----
be/src/codegen/llvm-codegen-cache.cc | 21 +++----
be/src/codegen/llvm-codegen-cache.h | 25 +++++----
be/src/codegen/llvm-codegen.cc | 78 +++++++++++++++-----------
be/src/codegen/llvm-codegen.h | 32 ++++++-----
be/src/codegen/llvm-execution-engine-wrapper.h | 59 +++++++++++++++++++
tests/custom_cluster/test_codegen_cache.py | 49 +++++++++++++++-
7 files changed, 208 insertions(+), 85 deletions(-)
diff --git a/be/src/codegen/llvm-codegen-cache-test.cc b/be/src/codegen/llvm-codegen-cache-test.cc
index 1146c3564..2d3fbf7db 100644
--- a/be/src/codegen/llvm-codegen-cache-test.cc
+++ b/be/src/codegen/llvm-codegen-cache-test.cc
@@ -78,8 +78,11 @@ class LlvmCodeGenCacheTest : public testing::Test {
}
static void CheckResult(LlvmCodeGen* codegen, bool is_double = false) {
- ASSERT_TRUE(codegen->execution_engine_cached_ != nullptr);
- CheckResult(codegen->execution_engine_cached_.get(), is_double);
+ ASSERT_TRUE(codegen->execution_engine_wrapper_cached_ != nullptr);
+ llvm::ExecutionEngine* cached_execution_engine =
+ codegen->execution_engine_wrapper_cached_->execution_engine();
+ ASSERT_TRUE(cached_execution_engine != nullptr);
+ CheckResult(cached_execution_engine, is_double);
}
static void CheckResult(llvm::ExecutionEngine* engine, bool is_double = false) {
@@ -131,7 +134,7 @@ class LlvmCodeGenCacheTest : public testing::Test {
RuntimeProfile* profile_;
scoped_ptr<TestEnv> test_env_;
scoped_ptr<CodeGenCache> codegen_cache_;
- shared_ptr<llvm::ExecutionEngine> exec_engine_;
+ shared_ptr<LlvmExecutionEngineWrapper> exec_engine_wrapper_;
TQueryOptions query_options_;
};
@@ -214,18 +217,18 @@ void LlvmCodeGenCacheTest::TestBasicFunction(TCodeGenCacheMode::type mode) {
EXPECT_OK(codegen_cache_->Store(cache_key, codegen.get(), mode));
CheckInUseMetrics(
codegen_cache_.get(), 1 /*num_entry_in_use*/, mem_charge /*bytes_in_use*/);
- EXPECT_OK(codegen_cache_->Lookup(cache_key, mode, &entry, &exec_engine_));
+ EXPECT_OK(codegen_cache_->Lookup(cache_key, mode, &entry, &exec_engine_wrapper_));
CheckResult(entry);
codegen->Close();
// Close the LlvmCodeGen, but should not affect the stored cache.
- EXPECT_OK(codegen_cache_->Lookup(cache_key, mode, &entry, &exec_engine_));
+ EXPECT_OK(codegen_cache_->Lookup(cache_key, mode, &entry, &exec_engine_wrapper_));
CheckResult(entry);
// Override the entry with a different function, should be able to find the new
// function from the new entry.
EXPECT_OK(codegen_cache_->Store(cache_key, codegen_double.get(), mode));
CheckInUseMetrics(
codegen_cache_.get(), 1 /*num_entry_in_use*/, mem_charge_double /*bytes_in_use*/);
- EXPECT_OK(codegen_cache_->Lookup(cache_key, mode, &entry, &exec_engine_));
+ EXPECT_OK(codegen_cache_->Lookup(cache_key, mode, &entry, &exec_engine_wrapper_));
CheckResult(entry, true /*is_double*/);
EXPECT_EQ(codegen_cache_->codegen_cache_entries_evicted_->GetValue(), 1);
codegen_double->Close();
@@ -411,12 +414,12 @@ TEST_F(LlvmCodeGenCacheTest, ModeAnalyzer) {
EXPECT_FALSE(CodeGenCacheModeAnalyzer::is_optimal(TCodeGenCacheMode::NORMAL_DEBUG));
}
-// Check the number of execution engine stored in the cache.
-// Because the shared pointer of the execution engine needs to be stored in the codegen
-// cache while the entry using the execution engine is stored in the cache.
+// Check the number of execution engine wrappers stored in the cache. Because the shared
+// pointer of the execution engine wrapper needs to be stored in the codegen cache while
+// the entry using the execution engine is stored in the cache.
void LlvmCodeGenCacheTest::CheckEngineCount(LlvmCodeGen* codegen, int expect_count) {
lock_guard<mutex> lock(codegen_cache_->cached_engines_lock_);
- auto engine_it = codegen_cache_->cached_engines_.find(codegen->execution_engine_.get());
+ auto engine_it = codegen_cache_->cached_engines_.find(codegen->execution_engine());
EXPECT_TRUE(engine_it != codegen_cache_->cached_engines_.end());
EXPECT_EQ(codegen_cache_->cached_engines_.size(), expect_count);
}
@@ -431,7 +434,7 @@ bool LlvmCodeGenCacheTest::CheckKeyExist(TCodeGenCacheMode::type mode, string ke
CodeGenCacheKey cache_key;
CodeGenCacheEntry entry;
CodeGenCacheKeyConstructor::construct(key, &cache_key);
- EXPECT_OK(codegen_cache_->Lookup(cache_key, mode, &entry, &exec_engine_));
+ EXPECT_OK(codegen_cache_->Lookup(cache_key, mode, &entry, &exec_engine_wrapper_));
return !entry.Empty();
}
@@ -470,9 +473,9 @@ void LlvmCodeGenCacheTest::TestSwitchModeHelper(TCodeGenCacheMode::type mode, st
if (expect_engine_num != -1) {
CheckEngineCount(codegen.get(), expect_engine_num);
}
- EXPECT_OK(codegen_cache_->Lookup(cache_key, mode, &entry, &exec_engine_));
+ EXPECT_OK(codegen_cache_->Lookup(cache_key, mode, &entry, &exec_engine_wrapper_));
CheckResult(entry);
- if (engine) *engine = codegen->execution_engine_.get();
+ if (engine) *engine = codegen->execution_engine();
codegen->Close();
}
diff --git a/be/src/codegen/llvm-codegen-cache.cc b/be/src/codegen/llvm-codegen-cache.cc
index bf1690699..3199e0ad4 100644
--- a/be/src/codegen/llvm-codegen-cache.cc
+++ b/be/src/codegen/llvm-codegen-cache.cc
@@ -105,7 +105,7 @@ void CodeGenCache::ReleaseResources() {
Status CodeGenCache::Lookup(const CodeGenCacheKey& cache_key,
const TCodeGenCacheMode::type& mode, CodeGenCacheEntry* entry,
- shared_ptr<llvm::ExecutionEngine>* execution_engine) {
+ shared_ptr<LlvmExecutionEngineWrapper>* execution_engine) {
DCHECK(!is_closed_);
DCHECK(cache_ != nullptr);
DCHECK(entry != nullptr);
@@ -139,7 +139,7 @@ Status CodeGenCache::Lookup(const CodeGenCacheKey& cache_key,
}
Status CodeGenCache::StoreInternal(const CodeGenCacheKey& cache_key,
- const LlvmCodeGen* codegen, const TCodeGenCacheMode::type& mode) {
+ LlvmCodeGen* codegen, const TCodeGenCacheMode::type& mode) {
// In normal mode, we will store the whole key content to the cache.
// Otherwise, in optimal mode, we will only store the hash code and length of the key.
Slice key = cache_key.optimal_key_slice();
@@ -158,14 +158,14 @@ Status CodeGenCache::StoreInternal(const CodeGenCacheKey& cache_key,
}
CodeGenCacheEntry* cache_entry =
reinterpret_cast<CodeGenCacheEntry*>(cache_->MutableValue(&pending_handle));
- cache_entry->Reset(codegen->execution_engine_.get(), codegen->num_functions_->value(),
+ cache_entry->Reset(codegen->execution_engine(), codegen->num_functions_->value(),
codegen->num_instructions_->value(), codegen->function_names_hashcode_, mem_charge);
StoreEngine(codegen);
/// It is thread-safe, but could override the existing entry with the same key.
Cache::UniqueHandle cache_handle =
cache_->Insert(move(pending_handle), evict_callback_.get());
if (cache_handle == nullptr) {
- RemoveEngine(codegen->execution_engine_.get());
+ RemoveEngine(codegen->execution_engine());
return Status(Substitute("Couldn't insert codegen cache entry,"
" hash code:'$0', size: '$1'",
cache_key.hash_code().str(), mem_charge));
@@ -176,7 +176,7 @@ Status CodeGenCache::StoreInternal(const CodeGenCacheKey& cache_key,
return Status::OK();
}
-Status CodeGenCache::Store(const CodeGenCacheKey& cache_key, const LlvmCodeGen* codegen,
+Status CodeGenCache::Store(const CodeGenCacheKey& cache_key, LlvmCodeGen* codegen,
const TCodeGenCacheMode::type& mode) {
DCHECK(!is_closed_);
DCHECK(cache_ != nullptr);
@@ -209,20 +209,21 @@ Status CodeGenCache::Store(const CodeGenCacheKey& cache_key, const LlvmCodeGen*
return status;
}
-void CodeGenCache::StoreEngine(const LlvmCodeGen* codegen) {
+void CodeGenCache::StoreEngine(LlvmCodeGen* codegen) {
DCHECK(codegen != nullptr);
lock_guard<mutex> lock(cached_engines_lock_);
- cached_engines_.emplace(codegen->execution_engine_.get(), codegen->execution_engine_);
+ cached_engines_.emplace(codegen->execution_engine(),
+ codegen->execution_engine_wrapper_);
}
bool CodeGenCache::LookupEngine(const llvm::ExecutionEngine* engine,
- shared_ptr<llvm::ExecutionEngine>* execution_engine) {
+ shared_ptr<LlvmExecutionEngineWrapper>* execution_engine_wrapper) {
DCHECK(engine != nullptr);
- DCHECK(execution_engine != nullptr);
+ DCHECK(execution_engine_wrapper != nullptr);
lock_guard<mutex> lock(cached_engines_lock_);
auto engine_it = cached_engines_.find(engine);
if (engine_it == cached_engines_.end()) return false;
- *execution_engine = engine_it->second;
+ *execution_engine_wrapper = engine_it->second;
return true;
}
diff --git a/be/src/codegen/llvm-codegen-cache.h b/be/src/codegen/llvm-codegen-cache.h
index 9242118ba..051c7e523 100644
--- a/be/src/codegen/llvm-codegen-cache.h
+++ b/be/src/codegen/llvm-codegen-cache.h
@@ -203,20 +203,21 @@ class CodeGenCache {
/// entry will be reset to empty.
/// Return Status::Okay unless there is any internal error to throw.
Status Lookup(const CodeGenCacheKey& key, const TCodeGenCacheMode::type& mode,
- CodeGenCacheEntry* entry, std::shared_ptr<llvm::ExecutionEngine>* execution_engine);
+ CodeGenCacheEntry* entry,
+ std::shared_ptr<LlvmExecutionEngineWrapper>* execution_engine);
/// Store the cache entry with the specific cache key.
- Status Store(const CodeGenCacheKey& key, const LlvmCodeGen* codegen,
+ Status Store(const CodeGenCacheKey& key, LlvmCodeGen* codegen,
const TCodeGenCacheMode::type& mode);
/// Store the shared pointer of llvm execution engine to the cache to keep all the
/// jitted functions in that engine alive.
- void StoreEngine(const LlvmCodeGen* codegen);
+ void StoreEngine(LlvmCodeGen* codegen);
- /// Look up the shared pointer of llvm execution engine by its raw pointer address.
- /// If found, return true. Ohterwise, return false.
+ /// Look up the shared pointer of the LlvmExecutionEngineWrapper by the raw pointer of
+ /// its execution engine field. If found, return true. Ohterwise, return false.
bool LookupEngine(const llvm::ExecutionEngine* engine,
- std::shared_ptr<llvm::ExecutionEngine>* execution_engine);
+ std::shared_ptr<LlvmExecutionEngineWrapper>* execution_engine_wrapper);
/// Remove the shared pointer of llvm execution engine from the cache by its raw
/// pointer address.
@@ -255,7 +256,7 @@ class CodeGenCache {
friend class LlvmCodeGenCacheTest;
/// Helper function to store the entry to the cache.
- Status StoreInternal(const CodeGenCacheKey& key, const LlvmCodeGen* codegen,
+ Status StoreInternal(const CodeGenCacheKey& key, LlvmCodeGen* codegen,
const TCodeGenCacheMode::type& mode);
/// Indicate if the cache is closed. If is closed, no call is allowed for any functions
@@ -289,10 +290,10 @@ class CodeGenCache {
/// Protects to the map of cached engines.
std::mutex cached_engines_lock_;
- /// Stores the llvm execution engine shared pointer to keep the jitted functions alive.
- /// The shared pointer entry could be removed when cache entry evicted or the whole
- /// cache destructed.
- std::unordered_map<const llvm::ExecutionEngine*, std::shared_ptr<llvm::ExecutionEngine>>
- cached_engines_;
+ /// Stores shared pointers to LlvmExecutionEngineWrappers to keep the jitted functions
+ /// alive. The shared pointer entries could be removed when the cache entry is evicted
+ /// or when the whole cache is destructed.
+ std::unordered_map<const llvm::ExecutionEngine*,
+ std::shared_ptr<LlvmExecutionEngineWrapper>> cached_engines_;
};
} // namespace impala
diff --git a/be/src/codegen/llvm-codegen.cc b/be/src/codegen/llvm-codegen.cc
index 8bc449602..89fa64933 100644
--- a/be/src/codegen/llvm-codegen.cc
+++ b/be/src/codegen/llvm-codegen.cc
@@ -363,7 +363,7 @@ Status LlvmCodeGen::LinkModuleFromLocalFs(const string& file) {
RETURN_IF_ERROR(LoadModuleFromFile(file, &new_module));
// The module data layout must match the one selected by the execution engine.
- new_module->setDataLayout(execution_engine_->getDataLayout());
+ new_module->setDataLayout(execution_engine()->getDataLayout());
// Parse all functions' names from the new module and find those which also exist in
// the main module. They are declarations in the new module or duplicated definitions
@@ -449,7 +449,7 @@ Status LlvmCodeGen::CreateImpalaCodegen(FragmentState* state,
}
Status LlvmCodeGen::Init(unique_ptr<llvm::Module> module) {
- DCHECK(module != NULL);
+ DCHECK(module != nullptr);
llvm::CodeGenOpt::Level opt_level = llvm::CodeGenOpt::Aggressive;
#ifndef NDEBUG
@@ -469,43 +469,52 @@ Status LlvmCodeGen::Init(unique_ptr<llvm::Module> module) {
builder.setMAttrs(cpu_attrs_);
builder.setErrorStr(&error_string_);
- execution_engine_.reset(builder.create());
- if (execution_engine_ == NULL) {
- module_ = NULL; // module_ was owned by builder.
+ unique_ptr<llvm::ExecutionEngine> execution_engine =
+ unique_ptr<llvm::ExecutionEngine>(builder.create());
+ if (execution_engine == nullptr) {
+ module_ = nullptr; // module_ was owned by builder.
stringstream ss;
ss << "Could not create ExecutionEngine: " << error_string_;
return Status(ss.str());
}
// The module data layout must match the one selected by the execution engine.
- module_->setDataLayout(execution_engine_->getDataLayout());
+ module_->setDataLayout(execution_engine->getDataLayout());
void_type_ = llvm::Type::getVoidTy(context());
ptr_type_ = llvm::PointerType::get(i8_type(), 0);
true_value_ = llvm::ConstantInt::get(context(), llvm::APInt(1, true, true));
false_value_ = llvm::ConstantInt::get(context(), llvm::APInt(1, false, true));
- SetupJITListeners();
+ unique_ptr<CodegenSymbolEmitter> symbol_emitter = SetupSymbolEmitter(
+ execution_engine.get());
+
+ execution_engine_wrapper_ = make_shared<LlvmExecutionEngineWrapper>(
+ std::move(execution_engine), std::move(symbol_emitter));
RETURN_IF_ERROR(LoadIntrinsics());
return Status::OK();
}
-void LlvmCodeGen::SetupJITListeners() {
+unique_ptr<CodegenSymbolEmitter> LlvmCodeGen::SetupSymbolEmitter(
+ llvm::ExecutionEngine* execution_engine) {
bool need_symbol_emitter = !FLAGS_asm_module_dir.empty() || FLAGS_perf_map;
- if (!need_symbol_emitter) return;
- symbol_emitter_.reset(new CodegenSymbolEmitter(id_));
- execution_engine_->RegisterJITEventListener(symbol_emitter_.get());
- symbol_emitter_->set_emit_perf_map(FLAGS_perf_map);
+ if (!need_symbol_emitter) return nullptr;
+ unique_ptr<CodegenSymbolEmitter> symbol_emitter =
+ make_unique<CodegenSymbolEmitter>(id_);
+ execution_engine->RegisterJITEventListener(symbol_emitter.get());
+ symbol_emitter->set_emit_perf_map(FLAGS_perf_map);
if (!FLAGS_asm_module_dir.empty()) {
- symbol_emitter_->set_asm_path(Substitute("$0/$1.asm", FLAGS_asm_module_dir, id_));
+ symbol_emitter->set_asm_path(Substitute("$0/$1.asm", FLAGS_asm_module_dir, id_));
}
+
+ return symbol_emitter;
}
LlvmCodeGen::~LlvmCodeGen() {
- DCHECK(execution_engine_ == nullptr) << "Must Close() before destruction";
+ DCHECK(execution_engine_wrapper_ == nullptr) << "Must Close() before destruction";
}
void LlvmCodeGen::Close() {
@@ -517,10 +526,8 @@ void LlvmCodeGen::Close() {
}
if (mem_tracker_ != nullptr) mem_tracker_->Close();
- // Execution engine executes callback on event listener, so tear down engine first.
- execution_engine_.reset();
- execution_engine_cached_.reset();
- symbol_emitter_.reset();
+ execution_engine_wrapper_.reset();
+ execution_engine_wrapper_cached_.reset();
module_ = nullptr;
}
@@ -933,7 +940,7 @@ Status LlvmCodeGen::LoadFunction(const TFunction& fn, const string& symbol,
#endif
// Associate the dynamically loaded function pointer with the Function* we defined.
// This tells LLVM where the compiled function definition is located in memory.
- execution_engine_->addGlobalMapping(*llvm_fn, fn_ptr);
+ execution_engine()->addGlobalMapping(*llvm_fn, fn_ptr);
// Disable the codegen cache because codegen cache uses the llvm module bitcode as
// the key while the bitcode doesn't contain the global function mapping of the
// execution engine. If the mapping is changed during running, like udf recreation,
@@ -1150,12 +1157,13 @@ bool LlvmCodeGen::LookupCache(CodeGenCacheKey& cache_key) {
CodeGenCache* cache = ExecEnv::GetInstance()->codegen_cache();
DCHECK(cache != nullptr);
Status lookup_status = cache->Lookup(cache_key,
- state_->query_options().codegen_cache_mode, &entry, &execution_engine_cached_);
- bool entry_exist = lookup_status.ok() && !entry.Empty();
+ state_->query_options().codegen_cache_mode, &entry,
+ &execution_engine_wrapper_cached_);
+ bool entry_exists = lookup_status.ok() && !entry.Empty();
LOG(INFO) << DebugCacheEntryString(cache_key, true /*is_lookup*/,
CodeGenCacheModeAnalyzer::is_debug(state_->query_options().codegen_cache_mode),
- entry_exist);
- if (entry_exist) {
+ entry_exists);
+ if (entry_exists) {
// Fallback to normal procedure if function names hashcode is not expected.
// The names hashcode should be the same unless there is a collision on the
// key, we expect this case is very rare.
@@ -1169,9 +1177,11 @@ bool LlvmCodeGen::LookupCache(CodeGenCacheKey& cache_key) {
return false;
}
- // execution_engine_cached_ is used to keep the life of the jitted functions in
- // case the engine is evicted in the global cache.
- DCHECK(execution_engine_cached_ != nullptr);
+ // execution_engine_wrapper_cached_ is used to keep the life of the jitted functions
+ // in case the engine is evicted in the global cache.
+ DCHECK(execution_engine_wrapper_cached_ != nullptr);
+ llvm::ExecutionEngine* cached_execution_engine =
+ execution_engine_wrapper_cached_->execution_engine();
vector<void*> jitted_funcs;
// Get pointers to all codegen'd functions.
for (int i = 0; i < fns_to_jit_compile_.size(); ++i) {
@@ -1183,7 +1193,7 @@ bool LlvmCodeGen::LookupCache(CodeGenCacheKey& cache_key) {
// for key collision cases, we expect all the functions should be in the
// cached execution engine.
void* jitted_function = reinterpret_cast<void*>(
- execution_engine_cached_->getFunctionAddress(function->getName()));
+ cached_execution_engine->getFunctionAddress(function->getName()));
if (jitted_function == nullptr) {
LOG(WARNING) << "Failed to get a jitted function from cache: "
<< function->getName().data()
@@ -1204,8 +1214,8 @@ bool LlvmCodeGen::LookupCache(CodeGenCacheKey& cache_key) {
COUNTER_SET(num_functions_, entry.num_functions);
COUNTER_SET(num_instructions_, entry.num_instructions);
}
- cache->IncHitOrMissCount(/*hit*/ entry_exist);
- return entry_exist;
+ cache->IncHitOrMissCount(/*hit*/ entry_exists);
+ return entry_exists;
}
string LlvmCodeGen::GetAllFunctionNames() {
@@ -1250,7 +1260,7 @@ void LlvmCodeGen::PruneModule() {
}
llvm::ModuleAnalysisManager module_analysis_manager;
- llvm::PassBuilder pass_builder(execution_engine_->getTargetMachine());
+ llvm::PassBuilder pass_builder(execution_engine()->getTargetMachine());
pass_builder.registerModuleAnalyses(module_analysis_manager);
llvm::ModulePassManager module_pass_manager;
@@ -1348,7 +1358,7 @@ Status LlvmCodeGen::FinalizeModule() {
{
SCOPED_TIMER(compile_timer_);
// Finalize module, which compiles all functions.
- execution_engine_->finalizeObject();
+ execution_engine()->finalizeObject();
}
SetFunctionPointers();
@@ -1413,7 +1423,7 @@ Status LlvmCodeGen::OptimizeModule() {
// TODO: we can likely muck with this to get better compile speeds or write
// our own passes. Our subexpression elimination optimization can be rolled into
// a pass.
- llvm::PassBuilder pass_builder(execution_engine_->getTargetMachine());
+ llvm::PassBuilder pass_builder(execution_engine()->getTargetMachine());
pass_builder.registerModuleAnalyses(MAM);
pass_builder.registerCGSCCAnalyses(CGAM);
pass_builder.registerFunctionAnalyses(FAM);
@@ -1458,7 +1468,7 @@ void LlvmCodeGen::SetFunctionPointers() {
for (const std::pair<llvm::Function*, CodegenFnPtrBase*>& fn_pair
: fns_to_jit_compile_) {
llvm::Function* function = fn_pair.first;
- void* jitted_function = execution_engine_->getPointerToFunction(function);
+ void* jitted_function = execution_engine()->getPointerToFunction(function);
DCHECK(jitted_function != nullptr) << "Failed to jit " << function->getName().data();
fn_pair.second->store(jitted_function);
}
@@ -1473,7 +1483,7 @@ void LlvmCodeGen::DestroyModule() {
llvm_intrinsics_.clear();
hash_fns_.clear();
fns_to_jit_compile_.clear();
- execution_engine_->removeModule(module_);
+ execution_engine()->removeModule(module_);
module_ = NULL;
}
diff --git a/be/src/codegen/llvm-codegen.h b/be/src/codegen/llvm-codegen.h
index 9175ddc05..b0ca8d828 100644
--- a/be/src/codegen/llvm-codegen.h
+++ b/be/src/codegen/llvm-codegen.h
@@ -38,6 +38,7 @@
#include <llvm/Support/MemoryBuffer.h>
#include <llvm/Support/raw_ostream.h>
+#include "codegen/llvm-execution-engine-wrapper.h"
#include "exprs/scalar-expr.h"
#include "impala-ir/impala-ir-functions.h"
#include "runtime/types.h"
@@ -323,7 +324,10 @@ class LlvmCodeGen {
llvm::LLVMContext& context() { return *context_.get(); }
/// Returns execution engine interface
- llvm::ExecutionEngine* execution_engine() { return execution_engine_.get(); }
+ llvm::ExecutionEngine* execution_engine() {
+ if (execution_engine_wrapper_ == nullptr) return nullptr;
+ return execution_engine_wrapper_->execution_engine();
+ }
/// Register a expr function with unique id. It can be subsequently retrieved via
/// GetRegisteredExprFn with that id.
@@ -701,9 +705,11 @@ class LlvmCodeGen {
/// generated IR to be inlined into cross-compiled functions' IR and vice versa.
static void SetCPUAttrs(llvm::Function* function);
- // Setup any JIT listeners to process generated machine code object, e.g. to generate
- // perf symbol map or disassembly.
- void SetupJITListeners();
+ /// If a symbol emitter is needed, creates one and registers it as a listener of
+ /// 'execution_engine'. It is used to generate perf symbol map or disassembly.
+ /// If no symbol emitter is needed, returns NULL.
+ std::unique_ptr<CodegenSymbolEmitter> SetupSymbolEmitter(
+ llvm::ExecutionEngine* execution_engine);
/// Load the intrinsics impala needs. This is a one time initialization.
/// Values are stored in 'llvm_intrinsics_'
@@ -898,15 +904,16 @@ class LlvmCodeGen {
/// We can have multiple instances of the LlvmCodeGen object in different threads
std::unique_ptr<llvm::LLVMContext> context_;
- /// Top level codegen object. Contains everything to jit one 'unit' of code.
- /// module_ is set by Init(). module_ is owned by execution_engine_.
+ /// Top level codegen object. Contains everything to jit one 'unit' of code. module_ is
+ /// set by Init(). module_ is owned by the execution engine in
+ /// execution_engine_wrapper_.
llvm::Module* module_;
- /// Execution/Jitting engine.
- std::shared_ptr<llvm::ExecutionEngine> execution_engine_;
+ /// Execution/Jitting engine in a wrapper.
+ std::shared_ptr<LlvmExecutionEngineWrapper> execution_engine_wrapper_;
- /// Cached Execution/Jitting engine.
- std::shared_ptr<llvm::ExecutionEngine> execution_engine_cached_;
+ /// Cached Execution/Jitting engine in a wrapper.
+ std::shared_ptr<LlvmExecutionEngineWrapper> execution_engine_wrapper_cached_;
/// The memory manager used by 'execution_engine_'. Owned by 'execution_engine_'.
ImpalaMCJITMemoryManager* memory_manager_;
@@ -959,11 +966,6 @@ class LlvmCodeGen {
llvm::Constant* true_value_;
llvm::Constant* false_value_;
- /// The symbol emitted associated with 'execution_engine_'. Methods on
- /// 'symbol_emitter_' are called by 'execution_engine_' when code is emitted or freed.
- /// The lifetime of the symbol emitter must be longer than 'execution_engine_'.
- boost::scoped_ptr<CodegenSymbolEmitter> symbol_emitter_;
-
/// Provides an implementation of a LLVM diagnostic handler and maintains the error
/// information from its callbacks.
class DiagnosticHandler {
diff --git a/be/src/codegen/llvm-execution-engine-wrapper.h b/be/src/codegen/llvm-execution-engine-wrapper.h
new file mode 100644
index 000000000..67e11179b
--- /dev/null
+++ b/be/src/codegen/llvm-execution-engine-wrapper.h
@@ -0,0 +1,59 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <memory>
+
+#include "codegen/codegen-symbol-emitter.h"
+
+#include <llvm/ExecutionEngine/ExecutionEngine.h>
+
+namespace impala {
+
+/// Class that encapsulates a non-NULL 'llvm::ExecutionEngine' and other objects the
+/// lifetimes of which are tied to the execution engine but are not owned by it.
+class LlvmExecutionEngineWrapper {
+ public:
+ LlvmExecutionEngineWrapper(std::unique_ptr<llvm::ExecutionEngine> execution_engine,
+ std::unique_ptr<CodegenSymbolEmitter> symbol_emitter)
+ : symbol_emitter_(std::move(symbol_emitter)),
+ execution_engine_(std::move(execution_engine)) {
+ DCHECK(execution_engine_ != nullptr);
+ }
+
+ ~LlvmExecutionEngineWrapper() {
+ execution_engine_.reset();
+ symbol_emitter_.reset();
+ }
+
+ llvm::ExecutionEngine* execution_engine() {
+ return execution_engine_.get();
+ }
+
+ private:
+ /// The symbol emitter associated with 'execution_engine_'. Methods on
+ /// 'symbol_emitter_' are called by 'execution_engine_' when code is emitted or
+ /// freed. The lifetime of the symbol emitter must be longer than
+ /// 'execution_engine_'.
+ std::unique_ptr<CodegenSymbolEmitter> symbol_emitter_;
+
+ /// Execution/Jitting engine.
+ std::unique_ptr<llvm::ExecutionEngine> execution_engine_;
+};
+
+} // namespace impala
diff --git a/tests/custom_cluster/test_codegen_cache.py b/tests/custom_cluster/test_codegen_cache.py
index 611ba5a03..5712dbc27 100644
--- a/tests/custom_cluster/test_codegen_cache.py
+++ b/tests/custom_cluster/test_codegen_cache.py
@@ -24,7 +24,6 @@ from tests.common.skip import SkipIf, SkipIfNotHdfsMinicluster
from tests.common.test_result_verifier import assert_codegen_cache_hit
from tests.util.filesystem_utils import get_fs_path
-
@SkipIf.not_hdfs
@SkipIfNotHdfsMinicluster.scheduling
class TestCodegenCache(CustomClusterTestSuite):
@@ -141,6 +140,54 @@ class TestCodegenCache(CustomClusterTestSuite):
self._test_codegen_cache(vector,
"select sum(identity(bigint_col)) from functional.alltypes", False)
+ @pytest.mark.execute_serially
+ @CustomClusterTestSuite.with_args(cluster_size=1,
+ impalad_args="--codegen_cache_capacity=2.5MB --asm_module_dir=/dev/null")
+ # Regression test for IMPALA-12260.
+ def test_codegen_cache_with_asm_module_dir(self, vector):
+ self._test_codegen_cache_with_symbol_emitter(vector)
+
+ @pytest.mark.execute_serially
+ @CustomClusterTestSuite.with_args(cluster_size=1,
+ impalad_args="--codegen_cache_capacity=2.5MB --perf_map")
+ # Regression test for IMPALA-12260.
+ def test_codegen_cache_with_perf_map(self, vector):
+ self._test_codegen_cache_with_symbol_emitter(vector)
+
+ def _test_codegen_cache_with_symbol_emitter(self, vector):
+ # Regression test for IMPALA-12260. The first query produces two entries in the cache,
+ # and one of them has a 'CodegenSymbolEmitter' as an event listener because of the
+ # '--asm_module_dir' or '--perf_map' flag. The second query inserts new entries in the
+ # cache - the size of the cache is chosen such that both entries from the first query
+ # fit in it but both are evicted during the second query. When an
+ # 'llvm::ExecutionEngine', which is part of the cache entry, is destroyed, it notifies
+ # its listeners, so the listeners should be alive at this time. Prior to IMPALA-12260
+ # the 'CodegenSymbolEmitter' was destroyed at the end of the first query, causing a
+ # crash during the second one.
+ exec_options = copy(vector.get_value('exec_option'))
+ exec_options['exec_single_node_rows_threshold'] = 0
+
+ q1 = """select int_col, tinyint_col from functional_parquet.alltypessmall
+ order by int_col desc limit 20"""
+ q2 = """select bool_col, year, month
+ from functional_parquet.alltypes
+ group by id, bool_col, smallint_col, bigint_col, float_col, double_col,
+ date_string_col, string_col, timestamp_col, year, month
+ order by id, bool_col, smallint_col, bigint_col, float_col, double_col,
+ date_string_col, string_col, timestamp_col, year, month
+ limit 20"""
+
+ self._check_metric_expect_init()
+ self.execute_query_expect_success(self.client, q1, exec_options)
+ assert self.get_metric('impala.codegen-cache.entries-in-use') == 2
+ assert self.get_metric('impala.codegen-cache.entries-evicted') == 0
+ assert self.get_metric('impala.codegen-cache.hits') == 0
+
+ self.execute_query_expect_success(self.client, q2, exec_options)
+ assert self.get_metric('impala.codegen-cache.entries-in-use') == 3
+ assert self.get_metric('impala.codegen-cache.entries-evicted') == 2
+ assert self.get_metric('impala.codegen-cache.hits') == 0
+
def _check_metric_expect_init(self):
# Verifies that the cache metrics are all zero.
assert self.get_metric('impala.codegen-cache.entries-evicted') == 0