[Lldb-commits] [PATCH] D65646: [lldb] Print better diagnostics for user expressions and modules

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL372203: [lldb] Print better diagnostics for user expressions 
and modules (authored by teemperor, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65646?vs=220439=220623#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65646/new/

https://reviews.llvm.org/D65646

Files:
  cfe/trunk/include/clang/Basic/DiagnosticOptions.def
  cfe/trunk/lib/Frontend/TextDiagnostic.cpp
  
lldb/trunk/packages/Python/lldbsuite/test/commands/expression/diagnostics/Makefile
  
lldb/trunk/packages/Python/lldbsuite/test/commands/expression/diagnostics/TestExprDiagnostics.py
  
lldb/trunk/packages/Python/lldbsuite/test/commands/expression/diagnostics/main.cpp
  
lldb/trunk/packages/Python/lldbsuite/test/lang/objc/foundation/TestObjCMethods.py
  lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
  lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
  lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
  lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
  lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
  lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h

Index: cfe/trunk/lib/Frontend/TextDiagnostic.cpp
===
--- cfe/trunk/lib/Frontend/TextDiagnostic.cpp
+++ cfe/trunk/lib/Frontend/TextDiagnostic.cpp
@@ -683,8 +683,9 @@
   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,
Index: cfe/trunk/include/clang/Basic/DiagnosticOptions.def
===
--- cfe/trunk/include/clang/Basic/DiagnosticOptions.def
+++ cfe/trunk/include/clang/Basic/DiagnosticOptions.def
@@ -49,6 +49,7 @@
 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.
Index: lldb/trunk/packages/Python/lldbsuite/test/commands/expression/diagnostics/TestExprDiagnostics.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/commands/expression/diagnostics/TestExprDiagnostics.py
+++ lldb/trunk/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(":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())
+

[Lldb-commits] [PATCH] D65646: [lldb] Print better diagnostics for user expressions and modules

[Raphael Isemann via Phabricator via lldb-commits]

teemperor updated this revision to Diff 220439.
teemperor retitled this revision from "[lldb] Print better diagnostics for user 
expressions." to "[lldb] Print better diagnostics for user expressions and 
modules".
teemperor added a comment.
Herald added a subscriber: usaxena95.

- Rebased test.
- Also testing Objective-C modules now (as the ClangModulesDeclVendor provides 
valid SourceLocations, so this patch also improves diagnostics from modules).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65646/new/

https://reviews.llvm.org/D65646

Files:
  clang/include/clang/Basic/DiagnosticOptions.def
  clang/lib/Frontend/TextDiagnostic.cpp
  lldb/packages/Python/lldbsuite/test/commands/expression/diagnostics/Makefile
  
lldb/packages/Python/lldbsuite/test/commands/expression/diagnostics/TestExprDiagnostics.py
  lldb/packages/Python/lldbsuite/test/commands/expression/diagnostics/main.cpp
  lldb/packages/Python/lldbsuite/test/lang/objc/foundation/TestObjCMethods.py
  lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
+++ 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 @@
   /// were not able to calculate this position.
   llvm::Optional m_user_expression_start_pos;
   ResultDelegate m_result_delegate;
+  ClangPersistentVariables *m_clang_state;
+  std::unique_ptr 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.
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ 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"
 
@@ -332,6 +331,7 @@
 if (PersistentExpressionState *persistent_state =
 target->GetPersistentExpressionStateForLanguage(
 lldb::eLanguageTypeC)) {
+  m_clang_state = llvm::cast(persistent_state);
   m_result_delegate.RegisterPersistentState(persistent_state);
 } else {
   diagnostic_manager.PutString(
@@ -396,18 +396,18 @@
 DiagnosticManager _manager, ExecutionContext _ctx,
 std::vector 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 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;
@@ -417,7 +417,7 @@
 // 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)
   

[Lldb-commits] [PATCH] D65646: [lldb] Print better diagnostics for user expressions.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a reviewer: LLDB.
teemperor added a comment.

ping :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65646/new/

https://reviews.llvm.org/D65646



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D65646: [lldb] Print better diagnostics for user expressions.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

LGTM, but you might want to have someone with more functional knowledge give 
the go-ahead.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65646/new/

https://reviews.llvm.org/D65646



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D65646: [lldb] Print better diagnostics for user expressions.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor updated this revision to Diff 213705.
teemperor added a comment.

- Addressed some feedback I forgot.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65646/new/

https://reviews.llvm.org/D65646

Files:
  clang/include/clang/Basic/DiagnosticOptions.def
  clang/lib/Frontend/TextDiagnostic.cpp
  lldb/packages/Python/lldbsuite/test/expression_command/diagnostics/Makefile
  
lldb/packages/Python/lldbsuite/test/expression_command/diagnostics/TestExprDiagnostics.py
  lldb/packages/Python/lldbsuite/test/expression_command/diagnostics/main.cpp
  lldb/packages/Python/lldbsuite/test/lang/objc/foundation/TestObjCMethods.py
  lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
+++ 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"
 
@@ -213,6 +214,10 @@
   /// were not able to calculate this position.
   llvm::Optional m_user_expression_start_pos;
   ResultDelegate m_result_delegate;
+  ClangPersistentVariables *m_clang_state;
+  std::unique_ptr 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.
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -21,7 +21,6 @@
 #include "ClangDiagnostic.h"
 #include "ClangExpressionDeclMap.h"
 #include "ClangExpressionParser.h"
-#include "ClangExpressionSourceCode.h"
 #include "ClangModulesDeclVendor.h"
 #include "ClangPersistentVariables.h"
 
@@ -330,6 +329,7 @@
 if (PersistentExpressionState *persistent_state =
 target->GetPersistentExpressionStateForLanguage(
 lldb::eLanguageTypeC)) {
+  m_clang_state = llvm::cast(persistent_state);
   m_result_delegate.RegisterPersistentState(persistent_state);
 } else {
   diagnostic_manager.PutString(
@@ -394,18 +394,18 @@
 DiagnosticManager _manager, ExecutionContext _ctx,
 std::vector 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 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;
@@ -415,7 +415,7 @@
 // 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;
@@ -570,7 +570,7 @@
   // 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);
 
@@ -583,8 

[Lldb-commits] [PATCH] D65646: [lldb] Print better diagnostics for user expressions.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor marked 4 inline comments as done.
teemperor added a comment.

Fixed some of the unrelated comments in a separate commit (that is already 
upstreamed).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65646/new/

https://reviews.llvm.org/D65646



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D65646: [lldb] Print better diagnostics for user expressions.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor updated this revision to Diff 213703.
teemperor added a comment.

- Addressed feedback.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65646/new/

https://reviews.llvm.org/D65646

Files:
  clang/include/clang/Basic/DiagnosticOptions.def
  clang/lib/Frontend/TextDiagnostic.cpp
  lldb/packages/Python/lldbsuite/test/expression_command/diagnostics/Makefile
  
lldb/packages/Python/lldbsuite/test/expression_command/diagnostics/TestExprDiagnostics.py
  lldb/packages/Python/lldbsuite/test/expression_command/diagnostics/main.cpp
  lldb/packages/Python/lldbsuite/test/lang/objc/foundation/TestObjCMethods.py
  lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
+++ 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"
 
@@ -213,6 +214,10 @@
   /// were not able to calculate this position.
   llvm::Optional m_user_expression_start_pos;
   ResultDelegate m_result_delegate;
+  ClangPersistentVariables *m_clang_state;
+  std::unique_ptr 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.
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -21,7 +21,6 @@
 #include "ClangDiagnostic.h"
 #include "ClangExpressionDeclMap.h"
 #include "ClangExpressionParser.h"
-#include "ClangExpressionSourceCode.h"
 #include "ClangModulesDeclVendor.h"
 #include "ClangPersistentVariables.h"
 
@@ -330,6 +329,7 @@
 if (PersistentExpressionState *persistent_state =
 target->GetPersistentExpressionStateForLanguage(
 lldb::eLanguageTypeC)) {
+  m_clang_state = llvm::cast(persistent_state);
   m_result_delegate.RegisterPersistentState(persistent_state);
 } else {
   diagnostic_manager.PutString(
@@ -394,18 +394,18 @@
 DiagnosticManager _manager, ExecutionContext _ctx,
 std::vector 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 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;
@@ -415,7 +415,7 @@
 // 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;
@@ -570,7 +570,7 @@
   // 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);
 
@@ -583,8 +583,8 @@

[Lldb-commits] [PATCH] D65646: [lldb] Print better diagnostics for user expressions.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp:267
 : ExpressionParser(exe_scope, expr, generate_debug_info), m_compiler(),
-  m_pp_callbacks(nullptr),
+  m_pp_callbacks(nullptr), m_filename(filename),
   m_include_directories(std::move(include_directories)) {

`std::move(filename)`



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h:29
+  static ClangExpressionSourceCode *
+  CreateWrapped(std::string filename, const char *prefix, const char *body) {
+return new ClangExpressionSourceCode(filename, "$__lldb_expr", prefix, 
body,

Can we use StringRefs for prefix and body?



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h:31
+return new ClangExpressionSourceCode(filename, "$__lldb_expr", prefix, 
body,
+ true);
   }

Can you use an enum to name the boolean parameter? 

```
enum Wrapping : bool {
Wrap = true,
NoWrap = false,
  };
```



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h:88
 private:
+  // The counter used by GetNextExprFileName.
+  uint32_t m_next_user_file_id = 0;

`///`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65646/new/

https://reviews.llvm.org/D65646



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D65646: [lldb] Print better diagnostics for user expressions.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor updated this revision to Diff 213587.
teemperor edited the summary of this revision.
teemperor added a comment.

- We now enumerate the fake expression files. E.g. ``.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65646/new/

https://reviews.llvm.org/D65646

Files:
  clang/include/clang/Basic/DiagnosticOptions.def
  clang/lib/Frontend/TextDiagnostic.cpp
  lldb/packages/Python/lldbsuite/test/expression_command/diagnostics/Makefile
  
lldb/packages/Python/lldbsuite/test/expression_command/diagnostics/TestExprDiagnostics.py
  lldb/packages/Python/lldbsuite/test/expression_command/diagnostics/main.cpp
  lldb/packages/Python/lldbsuite/test/lang/objc/foundation/TestObjCMethods.py
  lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
+++ 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"
 
@@ -213,6 +214,10 @@
   /// were not able to calculate this position.
   llvm::Optional m_user_expression_start_pos;
   ResultDelegate m_result_delegate;
+  ClangPersistentVariables *m_clang_state;
+  std::unique_ptr 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.
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -21,7 +21,6 @@
 #include "ClangDiagnostic.h"
 #include "ClangExpressionDeclMap.h"
 #include "ClangExpressionParser.h"
-#include "ClangExpressionSourceCode.h"
 #include "ClangModulesDeclVendor.h"
 #include "ClangPersistentVariables.h"
 
@@ -330,6 +329,7 @@
 if (PersistentExpressionState *persistent_state =
 target->GetPersistentExpressionStateForLanguage(
 lldb::eLanguageTypeC)) {
+  m_clang_state = llvm::cast(persistent_state);
   m_result_delegate.RegisterPersistentState(persistent_state);
 } else {
   diagnostic_manager.PutString(
@@ -394,18 +394,18 @@
 DiagnosticManager _manager, ExecutionContext _ctx,
 std::vector 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 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;
@@ -415,7 +415,7 @@
 // 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;
@@ -570,7 +570,7 @@
   // parser_sp will never be empty.
 
   ClangExpressionParser parser(exe_scope, *this, generate_debug_info,
-   m_include_directories);
+   m_include_directories, m_filename);
 
   

[Lldb-commits] [PATCH] D65646: [lldb] Print better diagnostics for user expressions.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

That would be fine.  In the swift REPL, we increment the line number so that it 
appears that all the expressions are one contiguous source file.  But I think 
for the expression parser treating each expression as a separate file is a 
better fiction.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65646/new/

https://reviews.llvm.org/D65646



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D65646: [lldb] Print better diagnostics for user expressions.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

> Is this orthogonal or do we now have to ways of pretending there is a source 
> file backing up the expression?

Jim already answered this, but this patch only changes the file names of our 
expression and pretends for the sake of diagnostics that we are in a separate 
file for the user code.

> But it seem weird that this will make every expression seem to come from the 
> same file and line.

What about enumerating the expression file names we print? E.g. ``, ``?


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65646/new/

https://reviews.llvm.org/D65646



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D65646: [lldb] Print better diagnostics for user expressions.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D65646#1612436 , @aprantl wrote:

> I have a question based on my half-knowledge about the expression evaluator: 
> I thought that we already wrote out a temporary file for the expression in 
> order to support `expr -g`? Is this orthogonal or do we now have to ways of 
> pretending there is a source file backing up the expression?


The file creation happens a little above the change to pass in a memory file:

  // We also want a real file on disk if we generate full debug info.
  should_create_file |= m_compiler->getCodeGenOpts().getDebugInfo() ==
codegenoptions::FullDebugInfo;

around line 891.  Raphael's change doesn't change whether we emit this source 
file or not.

But it seem weird that this will make every expression seem to come from the 
same file and line.  It would be really nice to be able to set breakpoints in 
compiled expressions, but if all the files think they have the same source file 
& starting line, this will be harder to get working.

Note, however, the creation of the source file & tracing back from the 
debug_line that we insert when we JIT the expression to this source file seems 
broken, however.  If I do:

  (lldb) expr --top-level -- int $foo(int) { (int) printf("I am here.\n"); }
  (lldb) b s -n printf
  Breakpoint 3: where = libsystem_c.dylib`printf, address = 0x7fff616588ec
  (lldb) expr -i 0 -- $foo(10)
  1 location added to breakpoint 2
  error: Execution was interrupted, reason: breakpoint 2.1.
  The process has been left at the point where it was interrupted, use "thread 
return -x" to return to the state before expression evaluation.
  Process 3708 stopped
  * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 2.1
  frame #0: 0x000100104b1b $__lldb_expr1`$foo(int) at Parse:1
  Target 0: (something) stopped.

I should have seen the source code for $foo, but we don't seem to have found 
the file.  So something is broken in this process.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65646/new/

https://reviews.llvm.org/D65646



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D65646: [lldb] Print better diagnostics for user expressions.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

I have a question based on my half-knowledge about the expression evaluator: I 
thought that we already wrote out a temporary file for the expression in order 
to support `expr -g`? Is this orthogonal or do we now have to ways of 
pretending there is a source file backing up the expression?




Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp:73
+static const char *c_start_marker = "#line 1 \"\"\n";
+static const char *c_end_marker = "\n;/*LLDB_BODY_END*/\n";
 

The comment seems asymmetric now.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65646/new/

https://reviews.llvm.org/D65646



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D65646: [lldb] Print better diagnostics for user expressions.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor created this revision.
teemperor added a reviewer: LLDB.
Herald added subscribers: lldb-commits, kadircet, arphaman, aprantl.
Herald added a project: LLDB.

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: :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


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D65646

Files:
  clang/include/clang/Basic/DiagnosticOptions.def
  clang/lib/Frontend/TextDiagnostic.cpp
  lldb/packages/Python/lldbsuite/test/expression_command/diagnostics/Makefile
  
lldb/packages/Python/lldbsuite/test/expression_command/diagnostics/TestExprDiagnostics.py
  lldb/packages/Python/lldbsuite/test/expression_command/diagnostics/main.cpp
  lldb/packages/Python/lldbsuite/test/lang/objc/foundation/TestObjCMethods.py
  lldb/source/Plugins/ExpressionParser/Clang/ClangDiagnostic.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
+++ 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 ""
 #ifndef offsetof
 #define offsetof(t, d) __builtin_offsetof(t, d)
 #endif
@@ -67,8 +69,8 @@
 }
 )";
 
-static const char *c_start_marker = "/*LLDB_BODY_START*/\n";
-static const char *c_end_marker = ";\n/*LLDB_BODY_END*/\n";
+static const char *c_start_marker = "#line 1 \"\"\n";
+static const char *c_end_marker = "\n;/*LLDB_BODY_END*/\n";
 
 namespace {
 
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -137,12 +137,14 @@
 
 class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
 public:
-  ClangDiagnosticManagerAdapter()
-  : m_passthrough(new clang::TextDiagnosticBuffer) {}
-
-  ClangDiagnosticManagerAdapter(
-  const std::shared_ptr )
-  : m_passthrough(passthrough) {}
+  ClangDiagnosticManagerAdapter(DiagnosticOptions ) {
+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,11 @@
 
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
 const clang::Diagnostic ) override {
-if (m_manager) {
-  llvm::SmallVector diag_str;
-  Info.FormatDiagnostic(diag_str);
-  diag_str.push_back('\0');
-  const char *data = diag_str.data();
+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 +173,16 @@
 severity = eDiagnosticSeverityRemark;
 break;
   case DiagnosticsEngine::Level::Note:
-m_manager->AppendMessageToDiagnostic(data);
+