impala-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sail...@apache.org
Subject [3/4] incubator-impala git commit: IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Date Thu, 19 Oct 2017 22:29:25 GMT
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 <bikramjeet.vig@cloudera.com>
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 <bikramjeet.vig@cloudera.com>
Authored: Fri Oct 6 11:27:28 2017 -0700
Committer: Impala Public Jenkins <impala-public-jenkins@gerrit.cloudera.org>
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);
 };
 
 }


Mime
View raw message