IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors Currently if llvm encounters an error it calls exit() and this kills the impalad process. This patch adds a diagnostic handler that llvm can use to pass the error up to impala instead of crashing it.
Testing: Added a test which induces an error in llvm and makes sure that its passed up to impala code and handled correctly. Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff Reviewed-on: http://gerrit.cloudera.org:8080/8233 Reviewed-by: Bikramjeet Vig <[email protected]> Tested-by: Impala Public Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/048ce2af Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/048ce2af Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/048ce2af Branch: refs/heads/master Commit: 048ce2af9c5c2a8431f4518fa34e11c0db8e0d8b Parents: 8e6dd8a Author: Bikramjeet Vig <[email protected]> Authored: Fri Oct 6 11:27:28 2017 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Wed Oct 18 23:30:04 2017 +0000 ---------------------------------------------------------------------- be/src/codegen/llvm-codegen-test.cc | 21 +++++++++++++++++++++ be/src/codegen/llvm-codegen.cc | 28 ++++++++++++++++++++++++++++ be/src/codegen/llvm-codegen.h | 24 ++++++++++++++++++++++++ be/src/testutil/gtest-util.h | 4 ++++ be/src/util/filesystem-util.cc | 12 ++++++++++++ be/src/util/filesystem-util.h | 4 ++++ 6 files changed, 93 insertions(+) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/048ce2af/be/src/codegen/llvm-codegen-test.cc ---------------------------------------------------------------------- diff --git a/be/src/codegen/llvm-codegen-test.cc b/be/src/codegen/llvm-codegen-test.cc index f6e1a57..9342633 100644 --- a/be/src/codegen/llvm-codegen-test.cc +++ b/be/src/codegen/llvm-codegen-test.cc @@ -27,6 +27,7 @@ #include "runtime/test-env.h" #include "service/fe-support.h" #include "util/cpu-info.h" +#include "util/filesystem-util.h" #include "util/hash-util.h" #include "util/path-builder.h" #include "util/scope-exit-trigger.h" @@ -95,6 +96,10 @@ class LlvmCodeGenTest : public testing:: Test { } static Status FinalizeModule(LlvmCodeGen* codegen) { return codegen->FinalizeModule(); } + + static Status LinkModule(LlvmCodeGen* codegen, const string& file) { + return codegen->LinkModule(file); + } }; // Simple test to just make and destroy llvmcodegen objects. LLVM @@ -456,6 +461,22 @@ TEST_F(LlvmCodeGenTest, HashTest) { CpuInfo::EnableFeature(CpuInfo::SSE4_2, restore_sse_support); } +// Test that an error propagating through codegen's diagnostic handler is +// captured by impala. An error is induced by asking Llvm to link the same lib twice. +TEST_F(LlvmCodeGenTest, HandleLinkageError) { + string ir_file_path("llvm-ir/test-loop.bc"); + string temp_copy_path("/tmp/test-loop.bc"); + string module_file; + PathBuilder::GetFullPath(ir_file_path, &module_file); + ASSERT_OK(FileSystemUtil::CopyFile(module_file, temp_copy_path)); + scoped_ptr<LlvmCodeGen> codegen; + ASSERT_OK(CreateFromFile(module_file.c_str(), &codegen)); + EXPECT_TRUE(codegen.get() != NULL); + Status status = LinkModule(codegen.get(), temp_copy_path); + EXPECT_STR_CONTAINS(status.GetDetail(), "symbol multiply defined"); + codegen->Close(); +} + } int main(int argc, char **argv) { http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/048ce2af/be/src/codegen/llvm-codegen.cc ---------------------------------------------------------------------- diff --git a/be/src/codegen/llvm-codegen.cc b/be/src/codegen/llvm-codegen.cc index 5503969..31f7d9b 100644 --- a/be/src/codegen/llvm-codegen.cc +++ b/be/src/codegen/llvm-codegen.cc @@ -35,6 +35,8 @@ #include <llvm/ExecutionEngine/MCJIT.h> #include <llvm/IR/Constants.h> #include <llvm/IR/DataLayout.h> +#include <llvm/IR/DiagnosticInfo.h> +#include <llvm/IR/DiagnosticPrinter.h> #include <llvm/IR/Function.h> #include <llvm/IR/GlobalVariable.h> #include <llvm/IR/InstIterator.h> @@ -181,6 +183,7 @@ LlvmCodeGen::LlvmCodeGen(RuntimeState* state, ObjectPool* pool, loaded_functions_(IRFunction::FN_END, NULL) { DCHECK(llvm_initialized_) << "Must call LlvmCodeGen::InitializeLlvm first."; + context_->setDiagnosticHandler(&DiagnosticHandler::DiagnosticHandlerFn, this); load_module_timer_ = ADD_TIMER(profile_, "LoadTime"); prepare_module_timer_ = ADD_TIMER(profile_, "PrepareTime"); module_bitcode_size_ = ADD_COUNTER(profile_, "ModuleBitcodeSize", TUnit::BYTES); @@ -298,9 +301,11 @@ Status LlvmCodeGen::LinkModule(const string& file) { } bool error = Linker::linkModules(*module_, std::move(new_module)); + string diagnostic_err = diagnostic_handler_.GetErrorString(); if (error) { stringstream ss; ss << "Problem linking " << file << " to main module."; + if (!diagnostic_err.empty()) ss << " " << diagnostic_err; return Status(ss.str()); } linked_modules_.insert(file); @@ -1610,6 +1615,29 @@ Constant* LlvmCodeGen::ConstantToGVPtr(Type* type, Constant* ir_constant, ArrayRef<Constant*>({GetIntConstant(TYPE_INT, 0)})); } +void LlvmCodeGen::DiagnosticHandler::DiagnosticHandlerFn(const DiagnosticInfo &info, + void *context){ + if (info.getSeverity() == DiagnosticSeverity::DS_Error) { + LlvmCodeGen* codegen = reinterpret_cast<LlvmCodeGen*>(context); + codegen->diagnostic_handler_.error_str_.clear(); + raw_string_ostream error_msg(codegen->diagnostic_handler_.error_str_); + DiagnosticPrinterRawOStream diagnostic_printer(error_msg); + diagnostic_printer << "LLVM diagnostic error: "; + info.print(diagnostic_printer); + error_msg.flush(); + LOG(INFO) << "Query " << codegen->state_->query_id() << " encountered a " + << codegen->diagnostic_handler_.error_str_; + } +} + +string LlvmCodeGen::DiagnosticHandler::GetErrorString() { + if (!error_str_.empty()) { + string return_msg(move(error_str_)); // Also clears error_str_. + return return_msg; + } + return ""; +} + } namespace boost { http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/048ce2af/be/src/codegen/llvm-codegen.h ---------------------------------------------------------------------- diff --git a/be/src/codegen/llvm-codegen.h b/be/src/codegen/llvm-codegen.h index 8aa9f2b..b069203 100644 --- a/be/src/codegen/llvm-codegen.h +++ b/be/src/codegen/llvm-codegen.h @@ -47,6 +47,7 @@ namespace llvm { class AllocaInst; class BasicBlock; class ConstantFolder; + class DiagnosticInfo; class ExecutionEngine; class Function; class LLVMContext; @@ -777,6 +778,29 @@ class LlvmCodeGen { /// 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 { + public: + /// Returns the last error that was reported via DiagnosticHandlerFn() and then + /// clears it. Returns an empty string otherwise. This should be called after any + /// LLVM API call that can fail but returns error info via this mechanism. + /// TODO: IMPALA-6038: use this to check and handle errors wherever needed. + std::string GetErrorString(); + + /// Handler function that sets the state on an instance of this class which is + /// accessible via the LlvmCodeGen object passed to it using the 'context' + /// input parameter. + static void DiagnosticHandlerFn(const llvm::DiagnosticInfo &info, void *context); + + private: + /// Contains the last error that was reported via DiagnosticHandlerFn(). + /// Is cleared by a call to GetErrorString(). + std::string error_str_; + }; + + DiagnosticHandler diagnostic_handler_; + /// Very rough estimate of memory in bytes that the IR and the intermediate data /// structures used by the optimizer may consume per LLVM IR instruction to be /// optimized (after dead code is removed). The number is chosen to avoid pathological http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/048ce2af/be/src/testutil/gtest-util.h ---------------------------------------------------------------------- diff --git a/be/src/testutil/gtest-util.h b/be/src/testutil/gtest-util.h index c7bfc56..68281b7 100644 --- a/be/src/testutil/gtest-util.h +++ b/be/src/testutil/gtest-util.h @@ -37,6 +37,10 @@ namespace impala { ASSERT_TRUE(status_.ok()) << "Error: " << status_.GetDetail(); \ } while (0) +// Substring matches. +#define EXPECT_STR_CONTAINS(str, substr) \ + EXPECT_PRED_FORMAT2(testing::IsSubstring, substr, str) + #define EXPECT_ERROR(status, err) \ do { \ const Status& status_ = (status); \ http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/048ce2af/be/src/util/filesystem-util.cc ---------------------------------------------------------------------- diff --git a/be/src/util/filesystem-util.cc b/be/src/util/filesystem-util.cc index a0cacdf..95ecc53 100644 --- a/be/src/util/filesystem-util.cc +++ b/be/src/util/filesystem-util.cc @@ -158,4 +158,16 @@ uint64_t FileSystemUtil::MaxNumFileHandles() { return 0ul; } +Status FileSystemUtil::CopyFile(const string& from_path, const string& to_path) { + error_code errcode; + filesystem::copy_file(from_path, to_path, filesystem::copy_option::overwrite_if_exists, + errcode); + if (errcode != errc::success) { + return Status(ErrorMsg(TErrorCode::RUNTIME_ERROR, Substitute( + "Encountered exception while copying file from path $0 to $1: $2", + from_path, to_path, errcode.message()))); + } + return Status::OK(); +} + } // namespace impala http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/048ce2af/be/src/util/filesystem-util.h ---------------------------------------------------------------------- diff --git a/be/src/util/filesystem-util.h b/be/src/util/filesystem-util.h index 1d497c3..9b78a4a 100644 --- a/be/src/util/filesystem-util.h +++ b/be/src/util/filesystem-util.h @@ -52,6 +52,10 @@ class FileSystemUtil { /// Returns the currently allowed maximum of possible file descriptors. In case of an /// error returns 0. static uint64_t MaxNumFileHandles(); + + /// Copy the specified file to the specified 'to_path'. Overwrite the file if it + /// already exists. + static Status CopyFile(const string& from_path, const string& to_path); }; }
