[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

Author: Teemperor

clang/include/clang/Basic/DiagnosticOptions.def

@@ -49,6 +49,7 @@ DIAGOPT(Pedantic, 1, 0)         /// -pedantic
 DIAGOPT(PedanticErrors, 1, 0)   /// -pedantic-errors
 DIAGOPT(ShowColumn, 1, 1)       /// Show column number on diagnostics.
 DIAGOPT(ShowLocation, 1, 1)     /// Show source location information.
+DIAGOPT(ShowLevel, 1, 1)        /// Show diagnostic level.
 DIAGOPT(AbsolutePath, 1, 0)     /// Use absolute paths.
 DIAGOPT(ShowCarets, 1, 1)       /// Show carets in diagnostics.
 DIAGOPT(ShowFixits, 1, 1)       /// Show fixit information.

clang/lib/Frontend/TextDiagnostic.cpp

@@ -683,8 +683,9 @@ void TextDiagnostic::emitDiagnosticMessage(
   if (DiagOpts->ShowColors)
     OS.resetColor();
 
-  printDiagnosticLevel(OS, Level, DiagOpts->ShowColors,
-                       DiagOpts->CLFallbackMode);
+  if (DiagOpts->ShowLevel)
+    printDiagnosticLevel(OS, Level, DiagOpts->ShowColors,
+                         DiagOpts->CLFallbackMode);
   printDiagnosticMessage(OS,
                          /*IsSupplemental*/ Level == DiagnosticsEngine::Note,
                          Message, OS.tell() - StartOfLocationInfo,

lldb/packages/Python/lldbsuite/test/commands/expression/diagnostics/Makefile

@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/packages/Python/lldbsuite/test/commands/expression/diagnostics/TestExprDiagnostics.py

@@ -0,0 +1,113 @@
+"""
+Test the diagnostics emitted by our embeded Clang instance that parses expressions.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+
+class ExprDiagnosticsTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+
+        self.main_source = "main.cpp"
+        self.main_source_spec = lldb.SBFileSpec(self.main_source)
+
+    def test_source_and_caret_printing(self):
+        """Test that the source and caret positions LLDB prints are correct"""
+        self.build()
+
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+                                          '// Break here', self.main_source_spec)
+        frame = thread.GetFrameAtIndex(0)
+
+        # Test that source/caret are at the right position.
+        value = frame.EvaluateExpression("unknown_identifier")
+        self.assertFalse(value.GetError().Success())
+        # We should get a nice diagnostic with a caret pointing at the start of
+        # the identifier.
+        self.assertIn("\nunknown_identifier\n^\n", value.GetError().GetCString())
+        self.assertIn("<user expression 0>:1:1", value.GetError().GetCString())
+
+        # Same as above but with the identifier in the middle.
+        value = frame.EvaluateExpression("1 + unknown_identifier  ")
+        self.assertFalse(value.GetError().Success())
+        self.assertIn("\n1 + unknown_identifier", value.GetError().GetCString())
+        self.assertIn("\n    ^\n", value.GetError().GetCString())
+
+        # Multiline expressions.
+        value = frame.EvaluateExpression("int a = 0;\nfoobar +=1;\na")
+        self.assertFalse(value.GetError().Success())
+        # We should still get the right line information and caret position.
+        self.assertIn("\nfoobar +=1;\n^\n", value.GetError().GetCString())
+        # It's the second line of the user expression.
+        self.assertIn("<user expression 2>:2:1", value.GetError().GetCString())
+
+        # Top-level expressions.
+        top_level_opts = lldb.SBExpressionOptions();
+        top_level_opts.SetTopLevel(True)
+
+        value = frame.EvaluateExpression("void foo(unknown_type x) {}", top_level_opts)
+        self.assertFalse(value.GetError().Success())
+        self.assertIn("\nvoid foo(unknown_type x) {}\n         ^\n", value.GetError().GetCString())
+        # Top-level expressions might use a different wrapper code, but the file name should still
+        # be the same.
+        self.assertIn("<user expression 3>:1:10", value.GetError().GetCString())
+
+        # Multiline top-level expressions.
+        value = frame.EvaluateExpression("void x() {}\nvoid foo(unknown_type x) {}", top_level_opts)
+        self.assertFalse(value.GetError().Success())
+        self.assertIn("\nvoid foo(unknown_type x) {}\n         ^\n", value.GetError().GetCString())
+        self.assertIn("<user expression 4>:2:10", value.GetError().GetCString())
+
+        # Test that we render Clang's 'notes' correctly.
+        value = frame.EvaluateExpression("struct SFoo{}; struct SFoo { int x; };", top_level_opts)
+        self.assertFalse(value.GetError().Success())
+        self.assertIn("<user expression 5>:1:8: previous definition is here\nstruct SFoo{}; struct SFoo { int x; };\n       ^\n", value.GetError().GetCString())
+
+        # Declarations from the debug information currently have no debug information. It's not clear what
+        # we should do in this case, but we should at least not print anything that's wrong.
+        # In the future our declarations should have valid source locations.
+        value = frame.EvaluateExpression("struct FooBar { double x };", top_level_opts)
+        self.assertFalse(value.GetError().Success())
+        self.assertEqual("error: <user expression 6>:1:8: redefinition of 'FooBar'\nstruct FooBar { double x };\n       ^\n", value.GetError().GetCString())
+
+        value = frame.EvaluateExpression("foo(1, 2)")
+        self.assertFalse(value.GetError().Success())
+        self.assertEqual("error: <user expression 7>:1:1: no matching function for call to 'foo'\nfoo(1, 2)\n^~~\nnote: candidate function not viable: requires single argument 'x', but 2 arguments were provided\n\n", value.GetError().GetCString())
+
+        # Redefine something that we defined in a user-expression. We should use the previous expression file name
+        # for the original decl.
+        value = frame.EvaluateExpression("struct Redef { double x; };", top_level_opts)
+        value = frame.EvaluateExpression("struct Redef { float y; };", top_level_opts)
+        self.assertFalse(value.GetError().Success())
+        self.assertIn("error: <user expression 9>:1:8: redefinition of 'Redef'\nstruct Redef { float y; };\n       ^\n<user expression 8>:1:8: previous definition is here\nstruct Redef { double x; };\n       ^", value.GetError().GetCString())
+
+    @skipUnlessDarwin
+    def test_source_locations_from_objc_modules(self):
+        self.build()
+
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+                                          '// Break here', self.main_source_spec)
+        frame = thread.GetFrameAtIndex(0)
+
+        # Import foundation so that the Obj-C module is loaded (which contains source locations
+        # that can be used by LLDB).
+        self.runCmd("expr @import Foundation")
+        value = frame.EvaluateExpression("NSLog(1);")
+        self.assertFalse(value.GetError().Success())
+        print(value.GetError().GetCString())
+        # LLDB should print the source line that defines NSLog. To not rely on any
+        # header paths/line numbers or the actual formatting of the Foundation headers, only look
+        # for a few tokens in the output.
+        # File path should come from Foundation framework.
+        self.assertIn("/Foundation.framework/", value.GetError().GetCString())
+        # The NSLog definition source line should be printed. Return value and
+        # the first argument are probably stable enough that this test can check for them.
+        self.assertIn("void NSLog(NSString *format", value.GetError().GetCString())
+

lldb/packages/Python/lldbsuite/test/commands/expression/diagnostics/main.cpp

@@ -0,0 +1,11 @@
+void foo(int x) {}
+
+struct FooBar {
+  int i;
+};
+
+int main() {
+  FooBar f;
+  foo(1);
+  return 0; // Break here
+}

lldb/packages/Python/lldbsuite/test/lang/objc/foundation/TestObjCMethods.py

@@ -191,7 +191,7 @@ def test_data_type_and_expr(self):
             "expression self->non_existent_member",
             COMMAND_FAILED_AS_EXPECTED,
             error=True,
-            startstr="error: 'MyString' does not have a member named 'non_existent_member'")
+            substrs=["error:", "'MyString' does not have a member named 'non_existent_member'"])
 
         # Use expression parser.
         self.runCmd("expression self->str")

lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h

@@ -29,7 +29,7 @@ class ClangDiagnostic : public Diagnostic {
     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) {}
 

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

@@ -137,25 +137,27 @@ class ClangExpressionParser::LLDBPreprocessorCallbacks : public PPCallbacks {
 
 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;
   }
 
   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 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
         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 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
         }
       }
     }
-
-    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)));
   }
 

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h

@@ -53,9 +53,14 @@ class ClangExpressionParser : public ExpressionParser {
   /// @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 @@ class ClangExpressionParser : public ExpressionParser {
   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;
 };
 }