[lldb] Print better diagnostics for user expressions and modules

Summary:
Currently our expression evaluators only prints very basic errors that are not very useful when writing complex expressions.

For example, in the expression below the user made a type error, but it's not clear from the diagnostic what went wrong:
```
(lldb) expr printf("Modulos are:", foobar%mo1, foobar%mo2, foobar%mo3)
error: invalid operands to binary expression ('int' and 'double')
```

This patch enables full Clang diagnostics in our expression evaluator. After this patch the diagnostics for the expression look like this:

```
(lldb) expr printf("Modulos are:", foobar%mo1, foobar%mo2, foobar%mo3)
error: <user expression 1>:1:54: invalid operands to binary expression ('int' and 'float')
printf("Modulos are:", foobar%mo1, foobar%mo2, foobar%mo3)
                                               ~~~~~~^~~~
```

To make this possible, we now emulate a user expression file within our diagnostics. This prevents that the user is exposed to
our internal wrapper code we inject.

Note that the diagnostics that refer to declarations from the debug information (e.g. 'note' diagnostics pointing to a called function)
will not be improved by this as they don't have any source locations associated with them, so caret or line printing isn't possible.
We instead just suppress these diagnostics as we already do with warnings as they would otherwise just be a context message
without any context (and the original diagnostic in the user expression should be enough to explain the issue).

Fixes rdar://24306342

Reviewers: JDevlieghere, aprantl, shafik, #lldb

Reviewed By: JDevlieghere, #lldb

Subscribers: usaxena95, davide, jingham, aprantl, arphaman, kadircet, lldb-commits

Tags: #lldb

Differential Revision: https://reviews.llvm.org/D65646

llvm-svn: 372203

8 files changed, 104 insertions, 68 deletions

diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
index 4db1ba096fa..48cd1c4b99f 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
@@ -29,7 +29,7 @@ public:
     return diag->getKind() == eDiagnosticOriginClang;
   }
 
-  ClangDiagnostic(const char *message, DiagnosticSeverity severity,
+  ClangDiagnostic(llvm::StringRef message, DiagnosticSeverity severity,
                   uint32_t compiler_id)
       : Diagnostic(message, severity, eDiagnosticOriginClang, compiler_id) {}
 
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 9487237f8d1..12b3005c259 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -137,12 +137,14 @@ public:
 
 class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
 public:
-  ClangDiagnosticManagerAdapter()
-      : m_passthrough(new clang::TextDiagnosticBuffer) {}
-
-  ClangDiagnosticManagerAdapter(
-      const std::shared_ptr<clang::TextDiagnosticBuffer> &passthrough)
-      : m_passthrough(passthrough) {}
+  ClangDiagnosticManagerAdapter(DiagnosticOptions &opts) {
+    DiagnosticOptions *m_options = new DiagnosticOptions(opts);
+    m_options->ShowPresumedLoc = true;
+    m_options->ShowLevel = false;
+    m_os.reset(new llvm::raw_string_ostream(m_output));
+    m_passthrough.reset(
+        new clang::TextDiagnosticPrinter(*m_os, m_options, false));
+  }
 
   void ResetManager(DiagnosticManager *manager = nullptr) {
     m_manager = manager;
@@ -150,12 +152,12 @@ public:
 
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const clang::Diagnostic &Info) override {
-    if (m_manager) {
-      llvm::SmallVector<char, 32> diag_str;
-      Info.FormatDiagnostic(diag_str);
-      diag_str.push_back('\0');
-      const char *data = diag_str.data();
+    // Render diagnostic message to m_output.
+    m_output.clear();
+    m_passthrough->HandleDiagnostic(DiagLevel, Info);
+    m_os->flush();
 
+    if (m_manager) {
       lldb_private::DiagnosticSeverity severity;
       bool make_new_diagnostic = true;
 
@@ -172,12 +174,16 @@ public:
         severity = eDiagnosticSeverityRemark;
         break;
       case DiagnosticsEngine::Level::Note:
-        m_manager->AppendMessageToDiagnostic(data);
+        m_manager->AppendMessageToDiagnostic(m_output);
         make_new_diagnostic = false;
       }
       if (make_new_diagnostic) {
+        // ClangDiagnostic messages are expected to have no whitespace/newlines
+        // around them.
+        std::string stripped_output = llvm::StringRef(m_output).trim();
+
         ClangDiagnostic *new_diagnostic =
-            new ClangDiagnostic(data, severity, Info.getID());
+            new ClangDiagnostic(stripped_output, severity, Info.getID());
         m_manager->AddDiagnostic(new_diagnostic);
 
         // Don't store away warning fixits, since the compiler doesn't have
@@ -194,23 +200,17 @@ public:
         }
       }
     }
-
-    m_passthrough->HandleDiagnostic(DiagLevel, Info);
-  }
-
-  void FlushDiagnostics(DiagnosticsEngine &Diags) {
-    m_passthrough->FlushDiagnostics(Diags);
-  }
-
-  DiagnosticConsumer *clone(DiagnosticsEngine &Diags) const {
-    return new ClangDiagnosticManagerAdapter(m_passthrough);
   }
 
-  clang::TextDiagnosticBuffer *GetPassthrough() { return m_passthrough.get(); }
+  clang::TextDiagnosticPrinter *GetPassthrough() { return m_passthrough.get(); }
 
 private:
   DiagnosticManager *m_manager = nullptr;
-  std::shared_ptr<clang::TextDiagnosticBuffer> m_passthrough;
+  std::shared_ptr<clang::TextDiagnosticPrinter> m_passthrough;
+  /// Output stream of m_passthrough.
+  std::shared_ptr<llvm::raw_string_ostream> m_os;
+  /// Output string filled by m_os.
+  std::string m_output;
 };
 
 static void SetupModuleHeaderPaths(CompilerInstance *compiler,
@@ -258,12 +258,11 @@ static void SetupModuleHeaderPaths(CompilerInstance *compiler,
 // Implementation of ClangExpressionParser
 //===----------------------------------------------------------------------===//
 
-ClangExpressionParser::ClangExpressionParser(
-    ExecutionContextScope *exe_scope, Expression &expr,
-    bool generate_debug_info, std::vector<std::string> include_directories)
+ClangExpressionParser::ClangExpressionParser(ExecutionContextScope *exe_scope, Expression &expr,
+    bool generate_debug_info, std::vector<std::string> include_directories, std::string filename)
     : ExpressionParser(exe_scope, expr, generate_debug_info), m_compiler(),
       m_pp_callbacks(nullptr),
-      m_include_directories(std::move(include_directories)) {
+      m_include_directories(std::move(include_directories)), m_filename(std::move(filename)) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
   // We can't compile expressions without a target.  So if the exe_scope is
@@ -557,7 +556,9 @@ ClangExpressionParser::ClangExpressionParser(
 
   // 6. Set up the diagnostic buffer for reporting errors
 
-  m_compiler->getDiagnostics().setClient(new ClangDiagnosticManagerAdapter);
+  auto diag_mgr = new ClangDiagnosticManagerAdapter(
+      m_compiler->getDiagnostics().getDiagnosticOptions());
+  m_compiler->getDiagnostics().setClient(diag_mgr);
 
   // 7. Set up the source management objects inside the compiler
   m_compiler->createFileManager();
@@ -869,8 +870,7 @@ ClangExpressionParser::ParseInternal(DiagnosticManager &diagnostic_manager,
   ClangDiagnosticManagerAdapter *adapter =
       static_cast<ClangDiagnosticManagerAdapter *>(
           m_compiler->getDiagnostics().getClient());
-  clang::TextDiagnosticBuffer *diag_buf = adapter->GetPassthrough();
-  diag_buf->FlushDiagnostics(m_compiler->getDiagnostics());
+  auto diag_buf = adapter->GetPassthrough();
 
   adapter->ResetManager(&diagnostic_manager);
 
@@ -921,7 +921,7 @@ ClangExpressionParser::ParseInternal(DiagnosticManager &diagnostic_manager,
 
   if (!created_main_file) {
     std::unique_ptr<MemoryBuffer> memory_buffer =
-        MemoryBuffer::getMemBufferCopy(expr_text, "<lldb-expr>");
+        MemoryBuffer::getMemBufferCopy(expr_text, m_filename);
     source_mgr.setMainFileID(source_mgr.createFileID(std::move(memory_buffer)));
   }
 
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
index 80a92691eaa..79ad5728bf7 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
@@ -53,9 +53,14 @@ public:
   /// @param[in] include_directories
   ///     List of include directories that should be used when parsing the
   ///     expression.
+  ///
+  /// @param[in] filename
+  ///     Name of the source file that should be used when rendering
+  ///     diagnostics (i.e. errors, warnings or notes from Clang).
   ClangExpressionParser(ExecutionContextScope *exe_scope, Expression &expr,
                         bool generate_debug_info,
-                        std::vector<std::string> include_directories = {});
+                        std::vector<std::string> include_directories = {},
+                        std::string filename = "<clang expression>");
 
   /// Destructor
   ~ClangExpressionParser() override;
@@ -178,6 +183,8 @@ private:
   std::unique_ptr<ClangASTContext> m_ast_context;
 
   std::vector<std::string> m_include_directories;
+  /// File name used for the user expression.
+  std::string m_filename;
 };
 }
 
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
index 88cfe5f0819..bf1bd5da39b 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
@@ -29,7 +29,9 @@
 
 using namespace lldb_private;
 
-const char *ClangExpressionSourceCode::g_expression_prefix = R"(
+const char *ClangExpressionSourceCode::g_expression_prefix =
+    R"(
+#line 1 "<lldb wrapper prefix>"
 #ifndef offsetof
 #define offsetof(t, d) __builtin_offsetof(t, d)
 #endif
@@ -67,9 +69,6 @@ extern "C"
 }
 )";
 
-static const char *c_start_marker = "    /*LLDB_BODY_START*/\n    ";
-static const char *c_end_marker = ";\n    /*LLDB_BODY_END*/\n";
-
 namespace {
 
 class AddMacroState {
@@ -169,6 +168,17 @@ static void AddMacros(const DebugMacros *dm, CompileUnit *comp_unit,
   }
 }
 
+lldb_private::ClangExpressionSourceCode::ClangExpressionSourceCode(
+    llvm::StringRef filename, llvm::StringRef name, llvm::StringRef prefix,
+    llvm::StringRef body, Wrapping wrap)
+    : ExpressionSourceCode(name, prefix, body, wrap) {
+  // Use #line markers to pretend that we have a single-line source file
+  // containing only the user expression. This will hide our wrapper code
+  // from the user when we render diagnostics with Clang.
+  m_start_marker = "#line 1 \"" + filename.str() + "\"\n";
+  m_end_marker = "\n;\n#line 1 \"<lldb wrapper suffix>\"\n";
+}
+
 namespace {
 /// Allows checking if a token is contained in a given expression.
 class TokenVerifier {
@@ -401,9 +411,9 @@ bool ClangExpressionSourceCode::GetText(
     case lldb::eLanguageTypeC:
     case lldb::eLanguageTypeC_plus_plus:
     case lldb::eLanguageTypeObjC:
-      tagged_body.append(c_start_marker);
+      tagged_body.append(m_start_marker);
       tagged_body.append(m_body);
-      tagged_body.append(c_end_marker);
+      tagged_body.append(m_end_marker);
       break;
     }
     switch (wrapping_language) {
@@ -477,24 +487,19 @@ bool ClangExpressionSourceCode::GetText(
 bool ClangExpressionSourceCode::GetOriginalBodyBounds(
     std::string transformed_text, lldb::LanguageType wrapping_language,
     size_t &start_loc, size_t &end_loc) {
-  const char *start_marker;
-  const char *end_marker;
-
   switch (wrapping_language) {
   default:
     return false;
   case lldb::eLanguageTypeC:
   case lldb::eLanguageTypeC_plus_plus:
   case lldb::eLanguageTypeObjC:
-    start_marker = c_start_marker;
-    end_marker = c_end_marker;
     break;
   }
 
-  start_loc = transformed_text.find(start_marker);
+  start_loc = transformed_text.find(m_start_marker);
   if (start_loc == std::string::npos)
     return false;
-  start_loc += strlen(start_marker);
-  end_loc = transformed_text.find(end_marker);
+  start_loc += m_start_marker.size();
+  end_loc = transformed_text.find(m_end_marker);
   return end_loc != std::string::npos;
 }
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
index 5b5f398dd93..2f966ff07fb 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
@@ -25,9 +25,11 @@ class ClangExpressionSourceCode : public ExpressionSourceCode {
 public:
   static const char *g_expression_prefix;
 
-  static ClangExpressionSourceCode *CreateWrapped(llvm::StringRef prefix,
+  static ClangExpressionSourceCode *CreateWrapped(llvm::StringRef filename,
+                                                  llvm::StringRef prefix,
                                                   llvm::StringRef body) {
-    return new ClangExpressionSourceCode("$__lldb_expr", prefix, body, Wrap);
+    return new ClangExpressionSourceCode(filename, "$__lldb_expr", prefix, body,
+                                         Wrap);
   }
 
   /// Generates the source code that will evaluate the expression.
@@ -54,14 +56,20 @@ public:
   // Given a string returned by GetText, find the beginning and end of the body
   // passed to CreateWrapped. Return true if the bounds could be found.  This
   // will also work on text with FixItHints applied.
-  static bool GetOriginalBodyBounds(std::string transformed_text,
-                                    lldb::LanguageType wrapping_language,
-                                    size_t &start_loc, size_t &end_loc);
+  bool GetOriginalBodyBounds(std::string transformed_text,
+                             lldb::LanguageType wrapping_language,
+                             size_t &start_loc, size_t &end_loc);
 
 protected:
-  ClangExpressionSourceCode(llvm::StringRef name, llvm::StringRef prefix,
-                            llvm::StringRef body, Wrapping wrap)
-      : ExpressionSourceCode(name, prefix, body, wrap) {}
+  ClangExpressionSourceCode(llvm::StringRef filename, llvm::StringRef name,
+                            llvm::StringRef prefix, llvm::StringRef body,
+                            Wrapping wrap);
+
+private:
+  /// String marking the start of the user expression.
+  std::string m_start_marker;
+  /// String marking the end of the user expression.
+  std::string m_end_marker;
 };
 
 } // namespace lldb_private
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
index a53d3b985ec..95e6c3ac963 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
@@ -50,6 +50,15 @@ public:
     return "$";
   }
 
+  /// Returns the next file name that should be used for user expressions.
+  std::string GetNextExprFileName() {
+    std::string name;
+    name.append("<user expression ");
+    name.append(std::to_string(m_next_user_file_id++));
+    name.append(">");
+    return name;
+  }
+
   llvm::Optional<CompilerType>
   GetCompilerTypeFromPersistentDecl(ConstString type_name) override;
 
@@ -66,6 +75,8 @@ public:
   }
 
 private:
+  /// The counter used by GetNextExprFileName.
+  uint32_t m_next_user_file_id = 0;
   // The counter used by GetNextPersistentVariableName
   uint32_t m_next_persistent_variable_id = 0;
 
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
index 40667c62636..ae95c687981 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -23,7 +23,6 @@
 #include "ClangDiagnostic.h"
 #include "ClangExpressionDeclMap.h"
 #include "ClangExpressionParser.h"
-#include "ClangExpressionSourceCode.h"
 #include "ClangModulesDeclVendor.h"
 #include "ClangPersistentVariables.h"
 
@@ -328,6 +327,7 @@ bool ClangUserExpression::SetupPersistentState(DiagnosticManager &diagnostic_man
     if (PersistentExpressionState *persistent_state =
             target->GetPersistentExpressionStateForLanguage(
                 lldb::eLanguageTypeC)) {
+      m_clang_state = llvm::cast<ClangPersistentVariables>(persistent_state);
       m_result_delegate.RegisterPersistentState(persistent_state);
     } else {
       diagnostic_manager.PutString(
@@ -392,18 +392,18 @@ void ClangUserExpression::CreateSourceCode(
     DiagnosticManager &diagnostic_manager, ExecutionContext &exe_ctx,
     std::vector<std::string> modules_to_import, bool for_completion) {
 
+  m_filename = m_clang_state->GetNextExprFileName();
   std::string prefix = m_expr_prefix;
 
   if (m_options.GetExecutionPolicy() == eExecutionPolicyTopLevel) {
     m_transformed_text = m_expr_text;
   } else {
-    std::unique_ptr<ClangExpressionSourceCode> source_code(
-        ClangExpressionSourceCode::CreateWrapped(prefix.c_str(),
-                                                 m_expr_text.c_str()));
+    m_source_code.reset(ClangExpressionSourceCode::CreateWrapped(
+        m_filename, prefix.c_str(), m_expr_text.c_str()));
 
-    if (!source_code->GetText(m_transformed_text, m_expr_lang,
-                              m_in_static_method, exe_ctx, !m_ctx_obj,
-                              for_completion, modules_to_import)) {
+    if (!m_source_code->GetText(m_transformed_text, m_expr_lang,
+                                m_in_static_method, exe_ctx, !m_ctx_obj,
+                                for_completion, modules_to_import)) {
       diagnostic_manager.PutString(eDiagnosticSeverityError,
                                    "couldn't construct expression body");
       return;
@@ -413,7 +413,7 @@ void ClangUserExpression::CreateSourceCode(
     // transformed code. We need this later for the code completion.
     std::size_t original_start;
     std::size_t original_end;
-    bool found_bounds = source_code->GetOriginalBodyBounds(
+    bool found_bounds = m_source_code->GetOriginalBodyBounds(
         m_transformed_text, m_expr_lang, original_start, original_end);
     if (found_bounds)
       m_user_expression_start_pos = original_start;
@@ -568,7 +568,7 @@ bool ClangUserExpression::Parse(DiagnosticManager &diagnostic_manager,
   // parser_sp will never be empty.
 
   ClangExpressionParser parser(exe_scope, *this, generate_debug_info,
-                               m_include_directories);
+                               m_include_directories, m_filename);
 
   unsigned num_errors = parser.Parse(diagnostic_manager);
 
@@ -581,8 +581,8 @@ bool ClangUserExpression::Parse(DiagnosticManager &diagnostic_manager,
         size_t fixed_end;
         const std::string &fixed_expression =
             diagnostic_manager.GetFixedExpression();
-        if (ClangExpressionSourceCode::GetOriginalBodyBounds(
-                fixed_expression, m_expr_lang, fixed_start, fixed_end))
+        if (m_source_code->GetOriginalBodyBounds(fixed_expression, m_expr_lang,
+                                                 fixed_start, fixed_end))
           m_fixed_text =
               fixed_expression.substr(fixed_start, fixed_end - fixed_start);
       }
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
index 9af5ce6cf24..ac28ac4a372 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
@@ -15,6 +15,7 @@
 #include "ASTStructExtractor.h"
 #include "ClangExpressionDeclMap.h"
 #include "ClangExpressionHelper.h"
+#include "ClangExpressionSourceCode.h"
 #include "ClangExpressionVariable.h"
 #include "IRForTarget.h"
 
@@ -216,6 +217,10 @@ private:
   /// were not able to calculate this position.
   llvm::Optional<size_t> m_user_expression_start_pos;
   ResultDelegate m_result_delegate;
+  ClangPersistentVariables *m_clang_state;
+  std::unique_ptr<ClangExpressionSourceCode> m_source_code;
+  /// File name used for the expression.
+  std::string m_filename;
 
   /// The object (if any) in which context the expression is evaluated.
   /// See the comment to `UserExpression::Evaluate` for details.