[Lldb-commits] [PATCH] D150772: Add code snippet line numbers to TestExprDiagnostics output

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

I think, given the support for multi-line expressions, updating the test case 
is IMHO the right call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150772

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

[Lldb-commits] [PATCH] D139740: [lldb] Disable macro redefinition warnings in expression wrapper

[Raphael Isemann via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9f3a3e1f3f97: [lldb] Disable macro redefinition warnings in 
expression wrapper (authored by teemperor).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139740

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/test/API/commands/expression/macros/TestMacros.py


Index: lldb/test/API/commands/expression/macros/TestMacros.py
===
--- lldb/test/API/commands/expression/macros/TestMacros.py
+++ lldb/test/API/commands/expression/macros/TestMacros.py
@@ -129,3 +129,9 @@
 result = frame.EvaluateExpression("MACRO_2")
 self.assertTrue(result.GetError().Fail(),
 "Printing MACRO_2 fails in the header file")
+
+# Check that the macro definitions do not trigger bogus Clang
+# diagnostics about macro redefinitions.
+result = frame.EvaluateExpression("does_not_parse")
+self.assertNotIn("macro redefined", str(result.GetError()))
+self.assertNotIn("redefining builtin macro", str(result.GetError()))
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
@@ -14,6 +14,7 @@
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h"
@@ -141,6 +142,17 @@
   if (dm == nullptr)
 return;
 
+  // The macros directives below can potentially redefine builtin macros of the
+  // Clang instance which parses the user expression. The Clang diagnostics
+  // caused by this are not useful for the user as the source code here is
+  // generated by LLDB.
+  stream << "#pragma clang diagnostic push\n";
+  stream << "#pragma clang diagnostic ignored \"-Wmacro-redefined\"\n";
+  stream << "#pragma clang diagnostic ignored \"-Wbuiltin-macro-redefined\"\n";
+  auto pop_warning = llvm::make_scope_exit([](){
+stream << "#pragma clang diagnostic pop\n";
+  });
+
   for (size_t i = 0; i < dm->GetNumMacroEntries(); i++) {
 const DebugMacroEntry  = dm->GetMacroEntryAtIndex(i);
 uint32_t line;


Index: lldb/test/API/commands/expression/macros/TestMacros.py
===
--- lldb/test/API/commands/expression/macros/TestMacros.py
+++ lldb/test/API/commands/expression/macros/TestMacros.py
@@ -129,3 +129,9 @@
 result = frame.EvaluateExpression("MACRO_2")
 self.assertTrue(result.GetError().Fail(),
 "Printing MACRO_2 fails in the header file")
+
+# Check that the macro definitions do not trigger bogus Clang
+# diagnostics about macro redefinitions.
+result = frame.EvaluateExpression("does_not_parse")
+self.assertNotIn("macro redefined", str(result.GetError()))
+self.assertNotIn("redefining builtin macro", str(result.GetError()))
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
@@ -14,6 +14,7 @@
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h"
@@ -141,6 +142,17 @@
   if (dm == nullptr)
 return;
 
+  // The macros directives below can potentially redefine builtin macros of the
+  // Clang instance which parses the user expression. The Clang diagnostics
+  // caused by this are not useful for the user as the source code here is
+  // generated by LLDB.
+  stream << "#pragma clang diagnostic push\n";
+  stream << "#pragma clang diagnostic ignored \"-Wmacro-redefined\"\n";
+  stream << "#pragma clang diagnostic ignored \"-Wbuiltin-macro-redefined\"\n";
+  auto pop_warning = llvm::make_scope_exit([](){
+stream << "#pragma clang diagnostic pop\n";
+  });
+
   for (size_t i = 0; i < dm->GetNumMacroEntries(); i++) {
 const DebugMacroEntry  = dm->GetMacroEntryAtIndex(i);
 uint32_t line;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D139740: [lldb] Disable macro redefinition warnings in expression wrapper

[Raphael Isemann via Phabricator via lldb-commits]

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

- Address builtin redefining (Thanks Michael!)


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

https://reviews.llvm.org/D139740

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/test/API/commands/expression/macros/TestMacros.py


Index: lldb/test/API/commands/expression/macros/TestMacros.py
===
--- lldb/test/API/commands/expression/macros/TestMacros.py
+++ lldb/test/API/commands/expression/macros/TestMacros.py
@@ -129,3 +129,9 @@
 result = frame.EvaluateExpression("MACRO_2")
 self.assertTrue(result.GetError().Fail(),
 "Printing MACRO_2 fails in the header file")
+
+# Check that the macro definitions do not trigger bogus Clang
+# diagnostics about macro redefinitions.
+result = frame.EvaluateExpression("does_not_parse")
+self.assertNotIn("macro redefined", str(result.GetError()))
+self.assertNotIn("redefining builtin macro", str(result.GetError()))
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
@@ -14,6 +14,7 @@
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h"
@@ -141,6 +142,17 @@
   if (dm == nullptr)
 return;
 
+  // The macros directives below can potentially redefine builtin macros of the
+  // Clang instance which parses the user expression. The Clang diagnostics
+  // caused by this are not useful for the user as the source code here is
+  // generated by LLDB.
+  stream << "#pragma clang diagnostic push\n";
+  stream << "#pragma clang diagnostic ignored \"-Wmacro-redefined\"\n";
+  stream << "#pragma clang diagnostic ignored \"-Wbuiltin-macro-redefined\"\n";
+  auto pop_warning = llvm::make_scope_exit([](){
+stream << "#pragma clang diagnostic pop\n";
+  });
+
   for (size_t i = 0; i < dm->GetNumMacroEntries(); i++) {
 const DebugMacroEntry  = dm->GetMacroEntryAtIndex(i);
 uint32_t line;


Index: lldb/test/API/commands/expression/macros/TestMacros.py
===
--- lldb/test/API/commands/expression/macros/TestMacros.py
+++ lldb/test/API/commands/expression/macros/TestMacros.py
@@ -129,3 +129,9 @@
 result = frame.EvaluateExpression("MACRO_2")
 self.assertTrue(result.GetError().Fail(),
 "Printing MACRO_2 fails in the header file")
+
+# Check that the macro definitions do not trigger bogus Clang
+# diagnostics about macro redefinitions.
+result = frame.EvaluateExpression("does_not_parse")
+self.assertNotIn("macro redefined", str(result.GetError()))
+self.assertNotIn("redefining builtin macro", str(result.GetError()))
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
@@ -14,6 +14,7 @@
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h"
@@ -141,6 +142,17 @@
   if (dm == nullptr)
 return;
 
+  // The macros directives below can potentially redefine builtin macros of the
+  // Clang instance which parses the user expression. The Clang diagnostics
+  // caused by this are not useful for the user as the source code here is
+  // generated by LLDB.
+  stream << "#pragma clang diagnostic push\n";
+  stream << "#pragma clang diagnostic ignored \"-Wmacro-redefined\"\n";
+  stream << "#pragma clang diagnostic ignored \"-Wbuiltin-macro-redefined\"\n";
+  auto pop_warning = llvm::make_scope_exit([](){
+stream << "#pragma clang diagnostic pop\n";
+  });
+
   for (size_t i = 0; i < dm->GetNumMacroEntries(); i++) {
 const DebugMacroEntry  = dm->GetMacroEntryAtIndex(i);
 uint32_t line;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D139740: [lldb] Disable macro redefinition warnings in expression wrapper

[Raphael Isemann via Phabricator via lldb-commits]

teemperor created this revision.
teemperor added a reviewer: Michael137.
teemperor added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
teemperor requested review of this revision.
Herald added a subscriber: lldb-commits.

GCC emits macro definitions into debug info when compiling with `-g3`.
LLDB is translating this information into `#define` directives which are 
injected into the
source code of user expressions. While this mechanism itself works fine, it can 
lead to
spurious "... macro redefined" warnings when the defined macro is also a 
builtin Clang macro:

  warning: :46:9: '__VERSION__' macro redefined
  #define __VERSION__ "12.1.0"
  ^
  :19:9: previous definition is here
  #define __VERSION__ "Clang 14.0.6"
  [repeated about a 100 more times for every builtin macro]

This patch just disables the diagnostic when parsing LLDB's generated list of
macros definitions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139740

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
  lldb/test/API/commands/expression/macros/TestMacros.py


Index: lldb/test/API/commands/expression/macros/TestMacros.py
===
--- lldb/test/API/commands/expression/macros/TestMacros.py
+++ lldb/test/API/commands/expression/macros/TestMacros.py
@@ -129,3 +129,8 @@
 result = frame.EvaluateExpression("MACRO_2")
 self.assertTrue(result.GetError().Fail(),
 "Printing MACRO_2 fails in the header file")
+
+# Check that the macro definitions do not trigger bogus Clang
+# diagnostics about macro redefinitions.
+result = frame.EvaluateExpression("does_not_parse")
+self.assertNotIn("macro redefined", str(result.GetError()))
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
@@ -14,6 +14,7 @@
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h"
@@ -141,6 +142,16 @@
   if (dm == nullptr)
 return;
 
+  // The macros directives below can potentially redefine builtin macros of the
+  // Clang instance which parses the user expression. The Clang diagnostics
+  // caused by this are not useful for the user as the source code here is
+  // generated by LLDB.
+  stream << "#pragma clang diagnostic push\n";
+  stream << "#pragma clang diagnostic ignored \"-Wmacro-redefined\"\n";
+  auto pop_warning = llvm::make_scope_exit([](){
+stream << "#pragma clang diagnostic pop\n";
+  });
+
   for (size_t i = 0; i < dm->GetNumMacroEntries(); i++) {
 const DebugMacroEntry  = dm->GetMacroEntryAtIndex(i);
 uint32_t line;


Index: lldb/test/API/commands/expression/macros/TestMacros.py
===
--- lldb/test/API/commands/expression/macros/TestMacros.py
+++ lldb/test/API/commands/expression/macros/TestMacros.py
@@ -129,3 +129,8 @@
 result = frame.EvaluateExpression("MACRO_2")
 self.assertTrue(result.GetError().Fail(),
 "Printing MACRO_2 fails in the header file")
+
+# Check that the macro definitions do not trigger bogus Clang
+# diagnostics about macro redefinitions.
+result = frame.EvaluateExpression("does_not_parse")
+self.assertNotIn("macro redefined", str(result.GetError()))
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp
@@ -14,6 +14,7 @@
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h"
@@ -141,6 +142,16 @@
   if (dm == nullptr)
 return;
 
+  // The macros directives below can potentially redefine builtin macros of the
+  // Clang instance which parses the user expression. The Clang diagnostics
+  // caused by this are not useful for the user as the source code here is
+  // generated by LLDB.
+  stream << "#pragma clang diagnostic push\n";
+  stream << "#pragma clang diagnostic ignored \"-Wmacro-redefined\"\n";
+  auto pop_warning = llvm::make_scope_exit([](){
+stream << "#pragma clang diagnostic pop\n";
+  });
+
   for (size_t i = 0; i < dm->GetNumMacroEntries(); i++) {
 const DebugMacroEntry  = 

[Lldb-commits] [PATCH] D126057: [lldb] Fix that empty target.run-args are not actually used when launching process

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa8bec6117998: [lldb] Fix that empty target.run-args are not 
actually used when launching… (authored by teemperor).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126057

Files:
  lldb/source/Interpreter/OptionValueProperties.cpp
  lldb/test/API/python_api/target/TestTargetAPI.py


Index: lldb/test/API/python_api/target/TestTargetAPI.py
===
--- lldb/test/API/python_api/target/TestTargetAPI.py
+++ lldb/test/API/python_api/target/TestTargetAPI.py
@@ -191,6 +191,15 @@
 self.assertIn('arg: foo', output)
 self.assertIn('env: bar=baz', output)
 
+# Clear all the run args set above.
+self.runCmd("setting clear target.run-args")
+process = target.LaunchSimple(None, None,
+  self.get_process_working_directory())
+process.Continue()
+self.assertEqual(process.GetState(), lldb.eStateExited)
+output = process.GetSTDOUT()
+self.assertNotIn('arg: foo', output)
+
 self.runCmd("settings set target.disable-stdio true")
 process = target.LaunchSimple(
 None, None, self.get_process_working_directory())
Index: lldb/source/Interpreter/OptionValueProperties.cpp
===
--- lldb/source/Interpreter/OptionValueProperties.cpp
+++ lldb/source/Interpreter/OptionValueProperties.cpp
@@ -248,16 +248,22 @@
 return false;
 
   const OptionValueArgs *arguments = value->GetAsArgs();
-  if (arguments)
-return arguments->GetArgs(args);
+  if (arguments) {
+arguments->GetArgs(args);
+return true;
+  }
 
   const OptionValueArray *array = value->GetAsArray();
-  if (array)
-return array->GetArgs(args);
+  if (array) {
+array->GetArgs(args);
+return true;
+  }
 
   const OptionValueDictionary *dict = value->GetAsDictionary();
-  if (dict)
-return dict->GetArgs(args);
+  if (dict) {
+dict->GetArgs(args);
+return true;
+  }
 
   return false;
 }


Index: lldb/test/API/python_api/target/TestTargetAPI.py
===
--- lldb/test/API/python_api/target/TestTargetAPI.py
+++ lldb/test/API/python_api/target/TestTargetAPI.py
@@ -191,6 +191,15 @@
 self.assertIn('arg: foo', output)
 self.assertIn('env: bar=baz', output)
 
+# Clear all the run args set above.
+self.runCmd("setting clear target.run-args")
+process = target.LaunchSimple(None, None,
+  self.get_process_working_directory())
+process.Continue()
+self.assertEqual(process.GetState(), lldb.eStateExited)
+output = process.GetSTDOUT()
+self.assertNotIn('arg: foo', output)
+
 self.runCmd("settings set target.disable-stdio true")
 process = target.LaunchSimple(
 None, None, self.get_process_working_directory())
Index: lldb/source/Interpreter/OptionValueProperties.cpp
===
--- lldb/source/Interpreter/OptionValueProperties.cpp
+++ lldb/source/Interpreter/OptionValueProperties.cpp
@@ -248,16 +248,22 @@
 return false;
 
   const OptionValueArgs *arguments = value->GetAsArgs();
-  if (arguments)
-return arguments->GetArgs(args);
+  if (arguments) {
+arguments->GetArgs(args);
+return true;
+  }
 
   const OptionValueArray *array = value->GetAsArray();
-  if (array)
-return array->GetArgs(args);
+  if (array) {
+array->GetArgs(args);
+return true;
+  }
 
   const OptionValueDictionary *dict = value->GetAsDictionary();
-  if (dict)
-return dict->GetArgs(args);
+  if (dict) {
+dict->GetArgs(args);
+return true;
+  }
 
   return false;
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131605: [lldb][tests] Test queue-specific breakpoints

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.



Comment at: lldb/test/API/macosx/queues/TestQueues.py:131
+ "The breakpoint was set for queue %s, but the 
breakpoint's queue name is %s" % (queue_breakpoint.GetQueueName(), 
queue1.GetName()))
+self.assertTrue(queue_breakpoint.GetHitCount() == 1,
+"The breakpoint for queue %s has not been hit" % 
(queue_breakpoint.GetQueueName()))

missed assertEqual here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131605

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

[Lldb-commits] [PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

import-std-module test changes look good to me, thanks for fixing that up.

And yes, ideally the tests should never use any libc++ internal names (and LLDB 
should never print them for those tests). So I think not having those in the 
here is a step in the right direction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

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

[Lldb-commits] [PATCH] D126668: LLDB: Fix resolving nested template parameters

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

> I don't really have the full context here, but I am wondering if we shouldn't 
> somehow take the DW_AT_declaration attribute into account here. It seems like 
> that should give a more definitive answer as to whether we can expect to see 
> a full set of template parameters or not.

I think that is actually a good approach. I think the current patch rejects 
forward declarations (which is fine as we don't have the required information 
in DWARF) and empty parameter packs (which is the edge case the original patch 
fixed).

Example: this is currently rejected even though we have all the required 
information:

  template
  struct X {};
  
  int main() {
X x;
  }

->

  0x0059:   DW_TAG_structure_type
  DW_AT_calling_convention(DW_CC_pass_by_value)
  DW_AT_name  ("X")
  DW_AT_byte_size (0x01)
  DW_AT_decl_file 
("/home/teemperor/test/someoneshouldreallypaymeforthis.cpp")
  DW_AT_decl_line (2)
  
  0x0062: DW_TAG_GNU_template_parameter_pack
DW_AT_name("T")
  
  0x0067:   DW_TAG_template_type_parameter
  DW_AT_type  (0x0052 "int")
  
  0x006c:   NULL
  
  0x006d: NULL
  
  0x006e:   NULL

I think having a fake class with a weird identifiers that contains template 
arguments is better than the missing arguments, so this looks in general good 
to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126668

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

[Lldb-commits] [PATCH] D126057: [lldb] Fix that empty target.run-args are not actually used when launching process

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Don't have access to a macOS machine that can run the tests, so maybe someone 
should give this a spin before landing :)




Comment at: lldb/source/Interpreter/OptionValueProperties.cpp:257
   const OptionValueArray *array = value->GetAsArray();
-  if (array)
-return array->GetArgs(args);
+  if (array) {
+array->GetArgs(args);

The fix for the test is the change a few lines above. This change and the one 
below seem to have the same bug. Not sure if any properties are using this code 
at the moment, so technically this is an untested chance. I can drop if people 
care.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126057

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

[Lldb-commits] [PATCH] D126057: [lldb] Fix that empty target.run-args are not actually used when launching process

[Raphael Isemann via Phabricator via lldb-commits]

teemperor created this revision.
teemperor added reviewers: JDevlieghere, kastiglione.
teemperor added a project: LLDB.
Herald added a project: All.
teemperor requested review of this revision.
Herald added a subscriber: lldb-commits.

`GetPropertyAtIndexAsArgs` returns true on success and false on failure. Right
now it returns the converted `size_t` returned from `GetArgs` which describes
the number of arguments in the argument list. So for empty argument lists
(`(size_t)0` -> `(bool)false`) this function always fails.

The only observable effect of this seems to be that empty arguments are never
propagated to the internal LaunchInfo for a process. This causes that once any
argument has been added to `target.run-args`, clearing `target.run-args` doesn't
have any effect.

Fixes issue #55568


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126057

Files:
  lldb/source/Interpreter/OptionValueProperties.cpp
  lldb/test/API/python_api/target/TestTargetAPI.py


Index: lldb/test/API/python_api/target/TestTargetAPI.py
===
--- lldb/test/API/python_api/target/TestTargetAPI.py
+++ lldb/test/API/python_api/target/TestTargetAPI.py
@@ -196,6 +196,15 @@
 self.assertIn('arg: foo', output)
 self.assertIn('env: bar=baz', output)
 
+# Clear all the run args set above.
+self.runCmd("setting clear target.run-args")
+process = target.LaunchSimple(None, None,
+  self.get_process_working_directory())
+process.Continue()
+self.assertEqual(process.GetState(), lldb.eStateExited)
+output = process.GetSTDOUT()
+self.assertNotIn('arg: foo', output)
+
 self.runCmd("settings set target.disable-stdio true")
 process = target.LaunchSimple(
 None, None, self.get_process_working_directory())
Index: lldb/source/Interpreter/OptionValueProperties.cpp
===
--- lldb/source/Interpreter/OptionValueProperties.cpp
+++ lldb/source/Interpreter/OptionValueProperties.cpp
@@ -248,16 +248,22 @@
 return false;
 
   const OptionValueArgs *arguments = value->GetAsArgs();
-  if (arguments)
-return arguments->GetArgs(args);
+  if (arguments) {
+arguments->GetArgs(args);
+return true;
+  }
 
   const OptionValueArray *array = value->GetAsArray();
-  if (array)
-return array->GetArgs(args);
+  if (array) {
+array->GetArgs(args);
+return true;
+  }
 
   const OptionValueDictionary *dict = value->GetAsDictionary();
-  if (dict)
-return dict->GetArgs(args);
+  if (dict) {
+dict->GetArgs(args);
+return true;
+  }
 
   return false;
 }


Index: lldb/test/API/python_api/target/TestTargetAPI.py
===
--- lldb/test/API/python_api/target/TestTargetAPI.py
+++ lldb/test/API/python_api/target/TestTargetAPI.py
@@ -196,6 +196,15 @@
 self.assertIn('arg: foo', output)
 self.assertIn('env: bar=baz', output)
 
+# Clear all the run args set above.
+self.runCmd("setting clear target.run-args")
+process = target.LaunchSimple(None, None,
+  self.get_process_working_directory())
+process.Continue()
+self.assertEqual(process.GetState(), lldb.eStateExited)
+output = process.GetSTDOUT()
+self.assertNotIn('arg: foo', output)
+
 self.runCmd("settings set target.disable-stdio true")
 process = target.LaunchSimple(
 None, None, self.get_process_working_directory())
Index: lldb/source/Interpreter/OptionValueProperties.cpp
===
--- lldb/source/Interpreter/OptionValueProperties.cpp
+++ lldb/source/Interpreter/OptionValueProperties.cpp
@@ -248,16 +248,22 @@
 return false;
 
   const OptionValueArgs *arguments = value->GetAsArgs();
-  if (arguments)
-return arguments->GetArgs(args);
+  if (arguments) {
+arguments->GetArgs(args);
+return true;
+  }
 
   const OptionValueArray *array = value->GetAsArray();
-  if (array)
-return array->GetArgs(args);
+  if (array) {
+array->GetArgs(args);
+return true;
+  }
 
   const OptionValueDictionary *dict = value->GetAsDictionary();
-  if (dict)
-return dict->GetArgs(args);
+  if (dict) {
+dict->GetArgs(args);
+return true;
+  }
 
   return false;
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D91835: [lldb] Add Python bindings to print stack traces on crashes.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.

This is beautiful


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

https://reviews.llvm.org/D91835

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

[Lldb-commits] [PATCH] D121064: [lldb] Add a setting to change the autosuggestion ANSI escape codes

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM, thanks! FWIW, if you have `fish` installed you might wanna check what 
they are doing on your terminal for its autosuggestions (and we might wanna 
steal their default, even though I thought this was 'faint').


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

https://reviews.llvm.org/D121064

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

[Lldb-commits] [PATCH] D85719: Initialize static const fields in the AST for expression evaluation

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Btw, that patch that I referenced hasn't landed yet (it just lacks the tests 
and probably rebasing), but I'm OOO for an unknown duration so feel free to 
land it yourself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85719

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

[Lldb-commits] [PATCH] D110827: [LLDB] Provide target specific directories to libclang

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM, thanks for fixing this!




Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp:86
 
-  // Check for /usr/include. On Linux this might be /usr/include/bits, so
-  // we should remove that '/bits' suffix to get the actual include directory.
-  if (posix_dir.endswith("/usr/include/bits"))
-posix_dir.consume_back("/bits");
-  if (posix_dir.endswith("/usr/include"))
-return m_c_inc.TrySet(posix_dir);
+  llvm::Optional inc_path;
+  // Target specific paths contains /usr/include, so we check them first

This can be moved to the assignments below (and then the double parenthesis can 
disappear too).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110827

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

[Lldb-commits] [PATCH] D113498: [lldb] Constant-resolve operands to `getelementptr`

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D113498#3131336 , @werat wrote:

> In D113498#3124525 , @teemperor 
> wrote:
>
>> I really like the solution, but I think by fixing the `CanInterpret` you 
>> also made the test case no longer reach the actual changed interpreting 
>> logic?
>>
>> So, `CanInterpret` says "we can't interpret this" (which is correct), but 
>> then the interpreter won't run it and your change to `ResolveConstantValue` 
>> isn't actually tested. There is no other test that touches that logic from 
>> what I can see. You could try throwing in some other expression at this that 
>> tests that new code? Maybe some kind of pointer arithmetic on a variable 
>> defined in your expression itself (so it can be constant evaluated).
>
> I think you're right, the interpreter now doesn't get to evaluating the 
> operands of `GetElementPtr`. However, I've failed to construct an expression 
> that would have a constant expression with `getelementptr` instruction. So 
> far I've only been able to reproduce it with the example from the test case 
> (which is being rejected during `CanInterpret`). I've tried expressions like 
> `const int x = 1; (int*)100 + (long long)()` and similar (also with `x` 
> being a global variable), but they're are being compiled in a way that 
> `getelementptr` is not a constant expression anymore.

Not sure how flexible your fuzzer setup is regarding downstream patches, but 
did you try putting some kind of `assert("coverage" && false);` in that new 
code and try fuzzing until you reach it?

> In D113498#3124525 , @teemperor 
> wrote:
>
>> We could also split out the interpreting change and then this is good to go.
>
> I think the `Interpret` and `CanInterpret` should really be in-sync with each 
> other, otherwise more bugs can follow. Given that we can't get an expression 
> for the logic I've implemented, I can make the check more strict -- just 
> verify that all indexes are `ConstantInt`. What do you think?

Sure, that would also work for me.

FWIW, I'm OOO for an undefined amount of time so I am not sure when I can take 
a look at this again. Feel free to ping in case you don't find another reviewer.




Comment at: lldb/source/Expression/IRInterpreter.cpp:490
+ constant_expr->op_end());
+for (Value *op : operands) {
+  Constant *constant_op = dyn_cast(op);

werat wrote:
> teemperor wrote:
> > `for (Value *op : constant_expr->ops())` ?
> `ConstantExpr` doesn't have `ops()` accessor, only `op_begin/op_end`. Am I 
> missing something?
My bad, the function name was `operands()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113498

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

[Lldb-commits] [PATCH] D104886: [lldb] Fix that the embedded Python REPL crashes if it receives SIGINT

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a subscriber: labath.
teemperor added a comment.

@labath raised some concerns in IRC about the setup code being run in a 
potential multithreaded env (which makes sense). Feel free to revert (not sure 
when/if I'll get around to address the issues)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104886

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

[Lldb-commits] [PATCH] D104886: [lldb] Fix that the embedded Python REPL crashes if it receives SIGINT

[Raphael Isemann via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcef1e07cc6d0: [lldb] Fix that the embedded Python REPL 
crashes if it receives SIGINT (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104886

Files:
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/test/API/iohandler/sigint/TestIOHandlerPythonREPLSigint.py

Index: lldb/test/API/iohandler/sigint/TestIOHandlerPythonREPLSigint.py
===
--- /dev/null
+++ lldb/test/API/iohandler/sigint/TestIOHandlerPythonREPLSigint.py
@@ -0,0 +1,73 @@
+"""
+Test sending SIGINT to the embedded Python REPL.
+"""
+
+import os
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.lldbpexpect import PExpectTest
+
+class TestCase(PExpectTest):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def start_python_repl(self):
+""" Starts up the embedded Python REPL."""
+self.launch()
+# Start the embedded Python REPL via the 'script' command.
+self.child.send("script -l python --\n")
+# Wait for the Python REPL prompt.
+self.child.expect(">>>")
+
+# PExpect uses many timeouts internally and doesn't play well
+# under ASAN on a loaded machine..
+@skipIfAsan
+def test_while_evaluating_code(self):
+""" Tests SIGINT handling while Python code is being evaluated."""
+self.start_python_repl()
+
+# Start a long-running command that we try to abort with SIGINT.
+# Note that we dont actually wait 1s in this code as pexpect or
+# lit will kill the test way before that.
+self.child.send("import time; print('running' + 'now'); time.sleep(1);\n")
+
+# Make sure the command is actually being evaluated at the moment by
+# looking at the string that the command is printing.
+# Don't check for a needle that also occurs in the program itself to
+# prevent that echoing will make this check pass unintentionally.
+self.child.expect("runningnow")
+
+# Send SIGINT to the LLDB process.
+self.child.sendintr()
+
+# This should get transformed to a KeyboardInterrupt which is the same
+# behaviour as the standalone Python REPL. It should also interrupt
+# the evaluation of our sleep statement.
+self.child.expect("KeyboardInterrupt")
+# Send EOF to quit the Python REPL.
+self.child.sendeof()
+
+self.quit()
+
+# PExpect uses many timeouts internally and doesn't play well
+# under ASAN on a loaded machine..
+@skipIfAsan
+# FIXME: On Linux the Python code that reads from stdin seems to block until
+# it has finished reading a line before handling any queued signals.
+@skipIfLinux
+def test_while_waiting_on_input(self):
+""" Tests SIGINT handling while the REPL is waiting on input from
+stdin."""
+self.start_python_repl()
+
+# Send SIGINT to the LLDB process.
+self.child.sendintr()
+# This should get transformed to a KeyboardInterrupt which is the same
+# behaviour as the standalone Python REPL.
+self.child.expect("KeyboardInterrupt")
+# Send EOF to quit the Python REPL.
+self.child.sendeof()
+
+self.quit()
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -1067,6 +1067,23 @@
 }
 
 bool ScriptInterpreterPythonImpl::Interrupt() {
+  // PyErr_SetInterrupt was introduced in 3.2.
+#if (PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION >= 2) || (PY_MAJOR_VERSION > 3)
+  // If the interpreter isn't evaluating any Python at the moment then return
+  // false to signal that this function didn't handle the interrupt and the
+  // next component should try handling it.
+  if (!IsExecutingPython())
+return false;
+
+  // Tell Python that it should pretend to have received a SIGINT.
+  PyErr_SetInterrupt();
+  // PyErr_SetInterrupt has no way to return an error so we can only pretend the
+  // signal got successfully handled and return true.
+  // Python 3.10 introduces PyErr_SetInterruptEx that could return an error, but
+  // the error handling is limited to checking the arguments which would be
+  // just our (hardcoded) input signal code SIGINT, so that's not useful at all.
+  return true;
+#else
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_SCRIPT));
 
   if (IsExecutingPython()) {
@@ -1088,6 +1105,7 @@
 

[Lldb-commits] [PATCH] D113687: [lldb][NFC] Inclusive language: replace master/slave names for ptys

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113687

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

[Lldb-commits] [PATCH] D113673: [lldb] Unwrap the type when dereferencing the value

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

Thanks a lot for fixing this, could you point to D103532 
 as the cause for the regression in the 
commit message?

This LGTM in general, but I think there could still be some obscure cases where 
this could fail.

So the switch statement this code is in (and which detects whether its a 
reference type) is defined as:

  clang::QualType parent_qual_type(
  RemoveWrappingTypes(GetCanonicalQualType(type)));
  switch (parent_qual_type->getTypeClass()) {

`GetCanonicalQualType` should strip all the outer and inner type sugar there 
is. And `RemoveWrappingTypes` removes *most* of the outer type sugar (which 
isn't necessary anymore as it should be all gone), but also makes types 
non-atomic (for simplicity reasons).

Meanwhile in our reference-case code we try to remove the outer type sugar (and 
atomic) via `RemoveWrappingTypes`. And if this doesn't end up in a 
ReferenceType the cast fails.

So IIUC there is still a possibility that we fail to cast here in case 
`GetCanonicalQualType` removes more outer sugar than `RemoveWrappingTypes`, 
which I think can only happen for type sugar classes not implemented by 
`RemoveWrappingTypes` (e.g., a quick grep tells me AttributedType and some 
template stuff is sugar that we don't handle).

So I would propose we do the following:

1. This patch can land now as-is to fix the regression.
2. It would be nice if we could expand the test with a few more variations of 
sugar. E.g., typedef to a reference of a typedef type and all that stuff.
3. I think we can simplify `RemoveWrappingTypes` to remove all type sugar (not 
just our small list we maintain there). That should eliminate the obscure cases 
and we no longer need to maintain a list of type sugar there.




Comment at: lldb/test/API/lang/cpp/dereferencing_references/main.cpp:10
+  // typedef of a reference
+  td_int_ref td = i;
+

nit: Could you give this a more unique name such as `td_to_ref_type`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113673

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

[Lldb-commits] [PATCH] D113498: [lldb] Constant-resolve operands to `getelementptr`

[Raphael Isemann via Phabricator via lldb-commits]

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

I really like the solution, but I think by fixing the `CanInterpret` you also 
made the test case no longer reach the actual changed interpreting logic?

So, `CanInterpret` says "we can't interpret this" (which is correct), but then 
the interpreter won't run it and your change to `ResolveConstantValue` isn't 
actually tested. There is no other test that touches that logic from what I can 
see. You could try throwing in some other expression at this that tests that 
new code? Maybe some kind of pointer arithmetic on a variable defined in your 
expression itself (so it can be constant evaluated). We could also split out 
the interpreting change and then this is good to go.

Also I think a second set of eyes on this would be nice as I rarely touch the 
IRInterpreter, but not sure who the best person for that is. I'll add the LLDB 
group and let's see if anyone has a second opinion on this, but in general this 
LGTM module the test situation.




Comment at: lldb/source/Expression/IRInterpreter.cpp:286
   SmallVector indices(op_cursor, op_end);
+  SmallVector const_indices;
+

Maybe `resolved_indices`? `const_` always sounds a bit like it's meaning 
'const' qualified version of indices or so.



Comment at: lldb/source/Expression/IRInterpreter.cpp:288
+
+  for (Value *idx : indices) {
+Constant *constant_idx = dyn_cast(idx);

I think this deserves a comment that `getIndexedOffsetInType` can only handle 
ConstantInt indices (and that's why we're resolving them here).



Comment at: lldb/source/Expression/IRInterpreter.cpp:490
+ constant_expr->op_end());
+for (Value *op : operands) {
+  Constant *constant_op = dyn_cast(op);

`for (Value *op : constant_expr->ops())` ?



Comment at: lldb/source/Expression/IRInterpreter.cpp:494
+return false;
+  if (!CanResolveConstant(constant_op)) {
+return false;

nit no `{}` for single line ifs



Comment at: lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py:71
+value = self.target().EvaluateExpression(expr)
+self.assertTrue(value.GetError().Success())

`self.assertSuccess` (that will print the error on failure to the test log)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113498

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

[Lldb-commits] [PATCH] D113605: [lldb] Fix documentation for EncodingDataType

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!




Comment at: lldb/include/lldb/Symbol/Type.h:69
   enum EncodingDataType {
+/// Invalid encoding
 eEncodingInvalid,

nit: LLVM comments end in a period.


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

https://reviews.llvm.org/D113605

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

[Lldb-commits] [PATCH] D113634: [lldb] Add support for DW_TAG_immutable_type

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D113634#3124401 , @ljmf00 wrote:

> In D113634#3124042 , @teemperor 
> wrote:
>
>> Are the DWARFASTParserClang changes meant as a step towards making it parse 
>> D?
>
> Yes, not only D but any language that currently falls here 
> (https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp#L106-L112).
>  AFAIK, rust have immutable variables too. Since I'm working on a 
> DWARFFASTParser for D this won't affect it in the future, but for languages 
> like Rust that uses the Python API and rely on Clang DWARFParser, this could 
> be beneficial.

I know the change is well intended, but the Rust support is 100% untested and 
completely broken (it can't deal with pretty much any non-trivial program from 
my recollections). So I would split out the Clang changes (which probably 
require some longer discussion) and just keep the Type/DWARFDIe changes (which 
look good to me).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113634

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

[Lldb-commits] [PATCH] D111409: proposed support for Java interface to Scripting Bridge

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D111409#3124194 , @labath wrote:

> In D111409#3124075 , @teemperor 
> wrote:
>
>>> Are you asking for dedicated physical resources for running nightly builds?
>>
>> I don't think any of the current bots have a Java installation so I think 
>> it's either that or we get someone with a bot to setup the required Java 
>> installation.
>
> I don't have a problem with installing the necessary packages on the bot I 
> manage, but I cannot subscribe to tracking down any failures (or flaky 
> tests!) for this configuration (and, in my experience, any new feature like 
> this is going to have flaky tests). Flaky tests (probably just one) are the 
> reason that lua integration is not enabled on this bot.

Sure, I think it should anyway not be up to any bot owner to track down flaky 
tests. And that nested_sessions lua test is anyway randomly failing everywhere 
from what I know.

>> FWIW, if no one wants to host a bot for this then I won't mind testing this 
>> in own CI , but I am not using buildbot so we'll 
>> have to see if that is acceptable for the community (I could also migrate it 
>> to buildbot, but the buildbot interface is just painful to use and I would 
>> prefer not to).
>
> I would rather not proliferate test infrastructures.
>
> I'm not sure which pain points are you referring to, but setting up a 
> buildbot instance is a lot simpler these days than it used to be (in 
> particular, you don't need to track down and install any outdated packages).

It's not the setup, it's just that the lab.llvm.org interface is far less 
usable than Jenkins for tracking down regressions in tests (and Jenkins is 
already not great, so that says something). But that's just my personal 
preference and I agree that lab.llvm.org should be the central place for 
infrastructure.

Anyway, if @labath can run them on his fancy build bot then I would prefer that 
over having it on my bot (because I pay out of my own pocket and CPU cycles are 
$$$).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D111409

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

[Lldb-commits] [PATCH] D111409: proposed support for Java interface to Scripting Bridge

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

> Are you asking for dedicated physical resources for running nightly builds?

I don't think any of the current bots have a Java installation so I think it's 
either that or we get someone with a bot to setup the required Java 
installation.

FWIW, if no one wants to host a bot for this then I won't mind testing this in 
own CI , but I am not using buildbot so we'll have to 
see if that is acceptable for the community (I could also migrate it to 
buildbot, but the buildbot interface is just painful to use and I would prefer 
not to).

> Do you really have a separate build machines for the Python and Lua scripting 
> environments?

Python is more or less mandatory so all LLDB build bots have that. Lua is 
tested here: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/

On a more general note: I haven't followed this thread very closely, but I am 
surprised that this is now adding a Java REPL to LLDB. I think the original 
"Just add Java bindings" seemed like a reasonable patch but I am not so sure 
about this. Could we split out the changes for just adding Java bindings to 
SWIG (which anyway seems like a standalone patch) and then we can talk about 
the whole Java scripting stuff in a separate review. I don't expect the first 
one to be controversial so this should also speed things up a bit.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D111409

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

[Lldb-commits] [PATCH] D113634: [lldb] Add support for DW_TAG_immutable_type

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Are the DWARFASTParserClang changes meant as a step towards making it parse D?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113634

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

[Lldb-commits] [PATCH] D110578: [lldb] Add support for D programming language

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

I changed the commit name from "Add support for D programming language" to 
"[lldb] Add support for demangling D symbols" when landing (which is more 
accurate).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110578

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

[Lldb-commits] [PATCH] D110578: [lldb] Add support for D programming language

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG96a735990839: [lldb] Add support for demangling D symbols 
(authored by ljmf00, committed by teemperor).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110578

Files:
  lldb/include/lldb/Core/Mangled.h
  lldb/source/Core/Mangled.cpp
  lldb/source/Symbol/Symtab.cpp
  lldb/unittests/Core/MangledTest.cpp

Index: lldb/unittests/Core/MangledTest.cpp
===
--- lldb/unittests/Core/MangledTest.cpp
+++ lldb/unittests/Core/MangledTest.cpp
@@ -72,6 +72,24 @@
   EXPECT_STREQ("", the_demangled.GetCString());
 }
 
+TEST(MangledTest, ResultForValidDLangName) {
+  ConstString mangled_name("_Dmain");
+  Mangled the_mangled(mangled_name);
+  ConstString the_demangled = the_mangled.GetDemangledName();
+
+  ConstString expected_result("D main");
+  EXPECT_STREQ(expected_result.GetCString(), the_demangled.GetCString());
+}
+
+TEST(MangledTest, EmptyForInvalidDLangName) {
+  ConstString mangled_name("_DDD");
+  Mangled the_mangled(mangled_name);
+  ConstString the_demangled = the_mangled.GetDemangledName();
+
+  EXPECT_STREQ("", the_demangled.GetCString());
+}
+
+
 TEST(MangledTest, NameIndexes_FindFunctionSymbols) {
   SubsystemRAII
   subsystems;
Index: lldb/source/Symbol/Symtab.cpp
===
--- lldb/source/Symbol/Symtab.cpp
+++ lldb/source/Symbol/Symtab.cpp
@@ -248,10 +248,8 @@
 
   // No filters for this scheme yet. Include all names in indexing.
   case Mangled::eManglingSchemeMSVC:
-return false;
-
-  // No filters for this scheme yet. Include all names in indexing.
   case Mangled::eManglingSchemeRustV0:
+  case Mangled::eManglingSchemeD:
 return false;
 
   // Don't try and demangle things we can't categorize.
Index: lldb/source/Core/Mangled.cpp
===
--- lldb/source/Core/Mangled.cpp
+++ lldb/source/Core/Mangled.cpp
@@ -45,6 +45,9 @@
   if (name.startswith("_R"))
 return Mangled::eManglingSchemeRustV0;
 
+  if (name.startswith("_D"))
+return Mangled::eManglingSchemeD;
+
   if (name.startswith("_Z"))
 return Mangled::eManglingSchemeItanium;
 
@@ -185,6 +188,19 @@
   return demangled_cstr;
 }
 
+static char *GetDLangDemangledStr(const char *M) {
+  char *demangled_cstr = llvm::dlangDemangle(M);
+
+  if (Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_DEMANGLE)) {
+if (demangled_cstr && demangled_cstr[0])
+  LLDB_LOG(log, "demangled dlang: {0} -> \"{1}\"", M, demangled_cstr);
+else
+  LLDB_LOG(log, "demangled dlang: {0} -> error: failed to demangle", M);
+  }
+
+  return demangled_cstr;
+}
+
 // Explicit demangling for scheduled requests during batch processing. This
 // makes use of ItaniumPartialDemangler's rich demangle info
 bool Mangled::DemangleWithRichManglingInfo(
@@ -244,7 +260,8 @@
   }
 
   case eManglingSchemeRustV0:
-// Rich demangling scheme is not supported for Rust
+  case eManglingSchemeD:
+// Rich demangling scheme is not supported
 return false;
   }
   llvm_unreachable("Fully covered switch above!");
@@ -278,6 +295,9 @@
   case eManglingSchemeRustV0:
 demangled_name = GetRustV0DemangledStr(mangled_name);
 break;
+  case eManglingSchemeD:
+demangled_name = GetDLangDemangledStr(mangled_name);
+break;
   case eManglingSchemeNone:
 llvm_unreachable("eManglingSchemeNone was handled already");
   }
Index: lldb/include/lldb/Core/Mangled.h
===
--- lldb/include/lldb/Core/Mangled.h
+++ lldb/include/lldb/Core/Mangled.h
@@ -44,7 +44,8 @@
 eManglingSchemeNone = 0,
 eManglingSchemeMSVC,
 eManglingSchemeItanium,
-eManglingSchemeRustV0
+eManglingSchemeRustV0,
+eManglingSchemeD
   };
 
   /// Default constructor.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D113604: [lldb] Format lldb/include/lldb/Symbol/Type.h

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.



Comment at: lldb/include/lldb/Symbol/Type.h:86
 eEncodingIsAtomicUID,  ///< This type is the type whose UID is
/// m_encoding_uid as an atomic type.
 eEncodingIsSyntheticUID

teemperor wrote:
> FWIW, we usually update the comments to something less >squished< such as:
> ```
> ///< This type is the type whose UID is m_encoding_uid
> eEcodingIsUID.
> /// This type is the type whose UID is m_encoding_uid with the const 
> qualifier added
> eEncodingIsConstUID,
> ```
> 
> I think this patch is good as-is, but there would be bonus points for fixing 
> up the comment style :)
I meant
```
lang=c++
/// This type is the type whose UID is m_encoding_uid
eEcodingIsUID.
/// This type is the type whose UID is m_encoding_uid with the const qualifier 
added
eEncodingIsConstUID,
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113604

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

[Lldb-commits] [PATCH] D113604: [lldb] Format lldb/include/lldb/Symbol/Type.h

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

I guess that's just clang-format ran over the file? If yes, then LGTM




Comment at: lldb/include/lldb/Symbol/Type.h:86
 eEncodingIsAtomicUID,  ///< This type is the type whose UID is
/// m_encoding_uid as an atomic type.
 eEncodingIsSyntheticUID

FWIW, we usually update the comments to something less >squished< such as:
```
///< This type is the type whose UID is m_encoding_uid
eEcodingIsUID.
/// This type is the type whose UID is m_encoding_uid with the const qualifier 
added
eEncodingIsConstUID,
```

I think this patch is good as-is, but there would be bonus points for fixing up 
the comment style :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113604

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

[Lldb-commits] [PATCH] D113449: Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls"

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

I actually didn't see that the patch deleted the TestCppReferenceToOuterClass 
test. Could you just add a `@skipIf # Crashes` or so above its `def test...` 
method? The test itself is still valid user code that we shouldn't crash on.

I left some nits and the test source probably needs to be made a bit more 
expressive in terms of what's its trying to test, but this can all be done 
later. Let's just land this to get the regression fixed.




Comment at: lldb/test/API/commands/expression/pr52257/TestExprCrash.py:17
+self.build()
+self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.expect("expr b", substrs=["tag_set_ = nullptr"])

nit: `self.createTestTarget()` (which generates useful error messages on 
failure)



Comment at: lldb/test/API/commands/expression/pr52257/TestExprCrash.py:18
+self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.expect("expr b", substrs=["tag_set_ = nullptr"])

nit: `self.expect_expr("b", result_type="B", 
result_children=[ValueCheck(name="tag_set_")])`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113449

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

[Lldb-commits] [PATCH] D113533: [LLDB] Remove access check of decl in TypeSystemClang.cpp

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

I'm pretty sure you're trying to solve the same problem as here: D85993 


In short: You're calling `CreateFunctionDeclaration` to create a function in a 
record, but you actually want to call `AddMethodToCXXRecordType` which allows 
passing and setting an access specifier (which is what the assert here checks). 
You can fix this in the PDB plugin by looking at the DeclContext and if 
`decl_ctx->isRecord()` -> `AddMethodToCXXRecordType` and otherwise 
`CreateFunctionDeclaration`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113533

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

[Lldb-commits] [PATCH] D113449: Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls"

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.



Comment at: lldb/test/Shell/SymbolFile/DWARF/PR52257.cpp:21
+};
+B b;

FWIW, I think probably should be an API test (for a bunch of reasons from not 
relying on formatting output to remote device testing), but given this is just 
a revert I won't insist on that. Thanks again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113449

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

[Lldb-commits] [PATCH] D113449: Revert "[lldb] Disable minimal import mode for RecordDecls that back FieldDecls"

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM, thanks for this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113449

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

[Lldb-commits] [PATCH] D113175: [lldb][NFC] Remove a bunch of unnecessary nullptr checks

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7323d07483f2: [lldb][NFC] Remove a bunch of unnecessary 
nullptr checks (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113175

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp


Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -1327,19 +1327,16 @@
   decl->setAnonymousStructOrUnion(true);
   }
 
-  if (decl) {
-if (metadata)
-  SetMetadata(decl, *metadata);
+  if (metadata)
+SetMetadata(decl, *metadata);
 
-if (access_type != eAccessNone)
-  decl->setAccess(ConvertAccessTypeToAccessSpecifier(access_type));
+  if (access_type != eAccessNone)
+decl->setAccess(ConvertAccessTypeToAccessSpecifier(access_type));
 
-if (decl_ctx)
-  decl_ctx->addDecl(decl);
+  if (decl_ctx)
+decl_ctx->addDecl(decl);
 
-return GetType(ast.getTagDeclType(decl));
-  }
-  return CompilerType();
+  return GetType(ast.getTagDeclType(decl));
 }
 
 namespace {
@@ -1605,15 +1602,13 @@
   template_cxx_decl->setDescribedClassTemplate(class_template_decl);
   SetOwningModule(class_template_decl, owning_module);
 
-  if (class_template_decl) {
-if (access_type != eAccessNone)
-  class_template_decl->setAccess(
-  ConvertAccessTypeToAccessSpecifier(access_type));
+  if (access_type != eAccessNone)
+class_template_decl->setAccess(
+ConvertAccessTypeToAccessSpecifier(access_type));
 
-decl_ctx->addDecl(class_template_decl);
+  decl_ctx->addDecl(class_template_decl);
 
-VerifyDecl(class_template_decl);
-  }
+  VerifyDecl(class_template_decl);
 
   return class_template_decl;
 }
@@ -1803,7 +1798,7 @@
   decl->setImplicit(isInternal);
   SetOwningModule(decl, owning_module);
 
-  if (decl && metadata)
+  if (metadata)
 SetMetadata(decl, *metadata);
 
   return GetType(ast.getObjCInterfaceType(decl));
@@ -2141,8 +2136,7 @@
   ? ConstexprSpecKind::Constexpr
   : ConstexprSpecKind::Unspecified);
   SetOwningModule(func_decl, owning_module);
-  if (func_decl)
-decl_ctx->addDecl(func_decl);
+  decl_ctx->addDecl(func_decl);
 
   VerifyDecl(func_decl);
 
@@ -2305,18 +2299,15 @@
   enum_decl->setScopedUsingClassTag(is_scoped);
   enum_decl->setFixed(false);
   SetOwningModule(enum_decl, owning_module);
-  if (enum_decl) {
-if (decl_ctx)
-  decl_ctx->addDecl(enum_decl);
+  if (decl_ctx)
+decl_ctx->addDecl(enum_decl);
 
-// TODO: check if we should be setting the promotion type too?
-enum_decl->setIntegerType(ClangUtil::GetQualType(integer_clang_type));
+  // TODO: check if we should be setting the promotion type too?
+  enum_decl->setIntegerType(ClangUtil::GetQualType(integer_clang_type));
 
-enum_decl->setAccess(AS_public); // TODO respect what's in the debug info
+  enum_decl->setAccess(AS_public); // TODO respect what's in the debug info
 
-return GetType(ast.getTagDeclType(enum_decl));
-  }
-  return CompilerType();
+  return GetType(ast.getTagDeclType(enum_decl));
 }
 
 CompilerType TypeSystemClang::GetIntTypeFromBitSize(size_t bit_size,


Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -1327,19 +1327,16 @@
   decl->setAnonymousStructOrUnion(true);
   }
 
-  if (decl) {
-if (metadata)
-  SetMetadata(decl, *metadata);
+  if (metadata)
+SetMetadata(decl, *metadata);
 
-if (access_type != eAccessNone)
-  decl->setAccess(ConvertAccessTypeToAccessSpecifier(access_type));
+  if (access_type != eAccessNone)
+decl->setAccess(ConvertAccessTypeToAccessSpecifier(access_type));
 
-if (decl_ctx)
-  decl_ctx->addDecl(decl);
+  if (decl_ctx)
+decl_ctx->addDecl(decl);
 
-return GetType(ast.getTagDeclType(decl));
-  }
-  return CompilerType();
+  return GetType(ast.getTagDeclType(decl));
 }
 
 namespace {
@@ -1605,15 +1602,13 @@
   template_cxx_decl->setDescribedClassTemplate(class_template_decl);
   SetOwningModule(class_template_decl, owning_module);
 
-  if (class_template_decl) {
-if (access_type != eAccessNone)
-  class_template_decl->setAccess(
-  ConvertAccessTypeToAccessSpecifier(access_type));
+  if (access_type != eAccessNone)
+class_template_decl->setAccess(
+ConvertAccessTypeToAccessSpecifier(access_type));
 
-decl_ctx->addDecl(class_template_decl);
+  decl_ctx->addDecl(class_template_decl);
 
-VerifyDecl(class_template_decl);
-  }
+  

[Lldb-commits] [PATCH] D113176: [lldb][NFC] StringRef-ify the name parameter in CreateEnumerationType

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb738a69ab8e3: [lldb][NFC] StringRef-ify the name parameter 
in CreateEnumerationType (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113176

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
  lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h


Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -417,7 +417,7 @@
size_t element_count, bool is_vector);
 
   // Enumeration Types
-  CompilerType CreateEnumerationType(const char *name,
+  CompilerType CreateEnumerationType(llvm::StringRef name,
  clang::DeclContext *decl_ctx,
  OptionalClangModuleID owning_module,
  const Declaration ,
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -2288,7 +2288,7 @@
 #pragma mark Enumeration Types
 
 CompilerType TypeSystemClang::CreateEnumerationType(
-const char *name, clang::DeclContext *decl_ctx,
+llvm::StringRef name, clang::DeclContext *decl_ctx,
 OptionalClangModuleID owning_module, const Declaration ,
 const CompilerType _clang_type, bool is_scoped) {
   // TODO: Do something intelligent with the Declaration object passed in
@@ -2299,7 +2299,7 @@
   //const bool IsFixed = false;
   EnumDecl *enum_decl = EnumDecl::CreateDeserialized(ast, 0);
   enum_decl->setDeclContext(decl_ctx);
-  if (name && name[0])
+  if (!name.empty())
 enum_decl->setDeclName((name));
   enum_decl->setScoped(is_scoped);
   enum_decl->setScopedUsingClassTag(is_scoped);
Index: lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
@@ -497,7 +497,7 @@
   // Class). Set it false for now.
   bool isScoped = false;
 
-  ast_enum = m_ast.CreateEnumerationType(name.c_str(), decl_context,
+  ast_enum = m_ast.CreateEnumerationType(name, decl_context,
  OptionalClangModuleID(), decl,
  builtin_type, isScoped);
 
Index: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
@@ -1105,7 +1105,7 @@
 
   Declaration declaration;
   CompilerType enum_ct = m_clang.CreateEnumerationType(
-  uname.c_str(), decl_context, OptionalClangModuleID(), declaration,
+  uname, decl_context, OptionalClangModuleID(), declaration,
   ToCompilerType(underlying_type), er.isScoped());
 
   TypeSystemClang::StartTagDeclarationDefinition(enum_ct);
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -843,7 +843,8 @@
 }
 
 clang_type = m_ast.CreateEnumerationType(
-attrs.name.GetCString(), GetClangDeclContextContainingDIE(die, 
nullptr),
+attrs.name.GetStringRef(),
+GetClangDeclContextContainingDIE(die, nullptr),
 GetOwningClangModule(die), attrs.decl, enumerator_clang_type,
 attrs.is_scoped_enum);
   } else {


Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -417,7 +417,7 @@
size_t element_count, bool is_vector);
 
   // Enumeration Types
-  CompilerType CreateEnumerationType(const char *name,
+  CompilerType CreateEnumerationType(llvm::StringRef name,
  clang::DeclContext *decl_ctx,
  OptionalClangModuleID owning_module,
  const Declaration ,
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

[Lldb-commits] [PATCH] D104413: Fixed use of -o and -k in LLDB under Windows when statically compiled with vcruntime.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!




Comment at: lldb/test/API/python_api/file_handle/TestFileHandle.py:920
+self.assertTrue(re.search(r'Show a list of all debugger commands', 
output))
+self.assertTrue(re.search(r'llvm revision', output))

Can you drop this line? Downstream tends to patch the version output with their 
own branding. Maybe `help command alias` and then check for `Define a custom 
command in terms of an existing command`.

Also I think you can use `self.assertIn('Show a list of all debugger commands', 
output)` here.


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

https://reviews.llvm.org/D104413

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

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D112439#3102752 , @xujuntwt95329 
wrote:

> Thanks a lot for your comments, they are very useful and I learned a lot 
> about C++ by talking with you.
>
> I'll address these comments and submit a patch and request your review.
>
> Much appreciated！

You're very welcome. Also these comments are all just small nits and not 
urgent, so feel free to address them whenever you have time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

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

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Fixed in 58dd658583eec9af24ca1262e1bce9f884d65487 


(Also left some nits in the test because I anyway looked over the code.)




Comment at: lldb/unittests/Target/FindFileTest.cpp:27
+  FileSpec original;
+  llvm::StringRef remapped;
+  Matches(const char *o, const char *r) : original(o), remapped(r) {}

I think this can just be a `std::string`. Right now it's relying on the caller 
to preserve the C-string storage, but that really isn't obvious to the whoever 
might update the test (and then has to debug a use-after-free).



Comment at: lldb/unittests/Target/FindFileTest.cpp:28
+  llvm::StringRef remapped;
+  Matches(const char *o, const char *r) : original(o), remapped(r) {}
+  Matches(const char *o, llvm::sys::path::Style style, const char *r)

No one is calling this IIUC?



Comment at: lldb/unittests/Target/FindFileTest.cpp:29
+  Matches(const char *o, const char *r) : original(o), remapped(r) {}
+  Matches(const char *o, llvm::sys::path::Style style, const char *r)
+  : original(o, style), remapped(r) {}

I think both of those parameters can be `StringRefs` (FileSpec anyway converts 
it back to a StringRef).



Comment at: lldb/unittests/Target/FindFileTest.cpp:36
+  void SetUp() override {
+FileSystem::Initialize();
+HostInfo::Initialize();

You can simplify this code by giving this test struct a member 
`SubsystemRAII subsystems;`. It will automatically call 
these functions for you in the right order on test setUp/tearDown. It also 
automatically adds error handling for init errors to your test. So this whole 
class can all be:

```
lang=c++
struct FindFileTest : public testing::Test {
  SubsystemRAII subsystems;
};
```



Comment at: lldb/unittests/Target/FindFileTest.cpp:58
+
+EXPECT_TRUE(bool(remapped = map.FindFile(match.original)));
+EXPECT_TRUE(FileSpec(remapped.getValue()).GetPath() ==

Couldn't `remapped` just be initialized to the FindFile value? Then you don't 
have to do the whole dance with converting to bool and avoiding the compiler 
warnings for assignments that look like comparisons.



Comment at: lldb/unittests/Target/FindFileTest.cpp:59
+EXPECT_TRUE(bool(remapped = map.FindFile(match.original)));
+EXPECT_TRUE(FileSpec(remapped.getValue()).GetPath() ==
+ConstString(match.remapped).GetStringRef());

`EXPECT_EQ` (StringRef is supported in the gtest macros)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

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

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D112439#3102559 , @xujuntwt95329 
wrote:

> In D112439#3102548 , @teemperor 
> wrote:
>
>> In D112439#3102533 , 
>> @xujuntwt95329 wrote:
>>
>>> In D112439#3102506 , @teemperor 
>>> wrote:
>>>
 In D112439#3098307 , 
 @xujuntwt95329 wrote:

> Seems that patch can't build by CI because it is based on this patch. In 
> my understanding we need to merge this patch firstly and rebase that NFC 
> patch to let CI work, right?

 You can set parent/child revisions in Phabricator that should handle this 
 situation (not sure if the premerge-checks are respecting that, but that's 
 how Phab usually manages unmerged dependencies).

 But to be clear, the premerge checks on Phabricator are *not* building or 
 testing LLDB at all. See the CI log:

   INFOprojects: {'lldb'}
   INFOall excluded projects(*) {'check-cxxabi', 'flang', 'libunwind', 
 'compiler-rt', 'cross-project-tests', 'libcxx', 'libcxxabi', 'libc', 
 'openmp', 'lldb'}
   INFOeffective projects list set()
>>>
>>> Thanks a lot for the detailed information!
>>>
>>> BTW, I see there are some test failure reported by CI, but I can't 
>>> reproduce them in my local environment (X86_64 Ubuntu and Windows), is 
>>> there any docker images to simulate the CI environment?
>>
>> Can you try a Release + Assert build? I'm looking at the failures at the 
>> moment too and it seems to only fail in non-Debug builds.
>
> Sure, I'll try the `release + assert` build. Thanks again!

Actually the solution seems pretty straightforward. `ArrayRef fails{ 
...` is not actually extending the lifetime or copying the initializer list 
you're passing. You have to use a std::vector or something similar to actually 
store the FileSpecs (right now they are being destroyed before the 
`TestFileFindings` call).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

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

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D112439#3102533 , @xujuntwt95329 
wrote:

> In D112439#3102506 , @teemperor 
> wrote:
>
>> In D112439#3098307 , 
>> @xujuntwt95329 wrote:
>>
>>> Seems that patch can't build by CI because it is based on this patch. In my 
>>> understanding we need to merge this patch firstly and rebase that NFC patch 
>>> to let CI work, right?
>>
>> You can set parent/child revisions in Phabricator that should handle this 
>> situation (not sure if the premerge-checks are respecting that, but that's 
>> how Phab usually manages unmerged dependencies).
>>
>> But to be clear, the premerge checks on Phabricator are *not* building or 
>> testing LLDB at all. See the CI log:
>>
>>   INFOprojects: {'lldb'}
>>   INFOall excluded projects(*) {'check-cxxabi', 'flang', 'libunwind', 
>> 'compiler-rt', 'cross-project-tests', 'libcxx', 'libcxxabi', 'libc', 
>> 'openmp', 'lldb'}
>>   INFOeffective projects list set()
>
> Thanks a lot for the detailed information!
>
> BTW, I see there are some test failure reported by CI, but I can't reproduce 
> them in my local environment (X86_64 Ubuntu and Windows), is there any docker 
> images to simulate the CI environment?

Can you try a Release + Assert build? I'm looking at the failures at the moment 
too and it seems to only fail in non-Debug builds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

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

[Lldb-commits] [PATCH] D112439: normalize file path when searching the source map

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D112439#3098307 , @xujuntwt95329 
wrote:

> Seems that patch can't build by CI because it is based on this patch. In my 
> understanding we need to merge this patch firstly and rebase that NFC patch 
> to let CI work, right?

You can set parent/child revisions in Phabricator that should handle this 
situation (not sure if the premerge-checks are respecting that, but that's how 
Phab usually manages unmerged dependencies).

But to be clear, the premerge checks on Phabricator are *not* building or 
testing LLDB at all. See the CI log:

  INFOprojects: {'lldb'}
  INFOall excluded projects(*) {'check-cxxabi', 'flang', 'libunwind', 
'compiler-rt', 'cross-project-tests', 'libcxx', 'libcxxabi', 'libc', 'openmp', 
'lldb'}
  INFOeffective projects list set()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112439

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

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

I think this was just fixed by 48677f58b06cfb8715902173c5bc3d1764d7c8c6 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

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

[Lldb-commits] [PATCH] D112165: Cleanup a few more PR36048 skips

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a subscriber: labath.
teemperor added a comment.

In D112165#3100929 , @dblaikie wrote:

> In D112165#3100608 , @teemperor 
> wrote:
>
>> Small note: gmodules test are never run on Linux, so you actually have to 
>> run them on macOS (or I think FreeBSD) to know whether the tests work.
>
> Yeah, I'll admit I didn't test this, but seemed consistent with the other 
> changes/cleanup - did this cause any breakage you know of?
>
> Could gmodules be tested on Linux? (I had originally really hoped/tried to 
> encourage gmodules to be implemented in a way that'd be compatible with Split 
> DWARF but it never quite got there, unfortunately... would've made more 
> overlap in functionality/testability/portability, I think)
>
> (I should setup a build environment on my Macbook Pro, but haven't got around 
> to it as yet)

There was just one test on macOS with data-formatter-cpp having defining 
something that is also in a system header (already fixed that). Also I think 
watching Green Dragon is enough. There would be a macOS CI if anyone actually 
cared about Green Dragon being always green.

The tests could in theory be run on Linux just fine, the problem is they just 
won't test anything. For better or worse (actually, just for the worse), the 
gmodules testing pretty much relies on re-running all normal API tests on 
`-fmodules -fcxx-modules -gmodules -fimplicit-module-maps` compiled test 
sources. But only about 5 of the 1000 API tests even define a module. So the 
remaining 995 tests just rerun their test as-is unless they include by accident 
a system header that has a modulemap on the system. No Linux distribution I 
know comes with modulemaps for its libc so no gmodules functionality will be 
exercised on the other 995 tests. So gmodules was just disabled as it just made 
the test suite longer without any benefit. Also I think there were some 
problems with enabling -fmodules on setups where we had a libc++ module but no 
modularized libc on the system. I think @labath knows the rationale best.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112165

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

[Lldb-commits] [PATCH] D112165: Cleanup a few more PR36048 skips

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Small note: gmodules test are never run on Linux, so you actually have to run 
them on macOS (or I think FreeBSD) to know whether the tests work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112165

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

[Lldb-commits] [PATCH] D112856: [lldb] Only specify LLVM_ENABLE_RUNTIMES in the libcxx error message.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM


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

https://reviews.llvm.org/D112856

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

[Lldb-commits] [PATCH] D112379: [lldb][NFC] Modernize for-loops in ModuleList

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4cf9d1e4492f: [lldb][NFC] Modernize for-loops in ModuleList 
(authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112379

Files:
  lldb/include/lldb/Core/ModuleList.h
  lldb/source/Core/ModuleList.cpp

Index: lldb/source/Core/ModuleList.cpp
===
--- lldb/source/Core/ModuleList.cpp
+++ lldb/source/Core/ModuleList.cpp
@@ -200,16 +200,15 @@
   }
 }
 
-bool ModuleList::AppendIfNeeded(const ModuleSP _sp, bool notify) {
-  if (module_sp) {
+bool ModuleList::AppendIfNeeded(const ModuleSP _module, bool notify) {
+  if (new_module) {
 std::lock_guard guard(m_modules_mutex);
-collection::iterator pos, end = m_modules.end();
-for (pos = m_modules.begin(); pos != end; ++pos) {
-  if (pos->get() == module_sp.get())
+for (const ModuleSP _sp : m_modules) {
+  if (module_sp.get() == new_module.get())
 return false; // Already in the list
 }
 // Only push module_sp on the list if it wasn't already in there.
-Append(module_sp, notify);
+Append(new_module, notify);
 return true;
   }
   return false;
@@ -372,10 +371,10 @@
 Module::LookupInfo lookup_info(name, name_type_mask, eLanguageTypeUnknown);
 
 std::lock_guard guard(m_modules_mutex);
-collection::const_iterator pos, end = m_modules.end();
-for (pos = m_modules.begin(); pos != end; ++pos) {
-  (*pos)->FindFunctions(lookup_info.GetLookupName(), CompilerDeclContext(),
-lookup_info.GetNameTypeMask(), options, sc_list);
+for (const ModuleSP _sp : m_modules) {
+  module_sp->FindFunctions(lookup_info.GetLookupName(),
+   CompilerDeclContext(),
+   lookup_info.GetNameTypeMask(), options, sc_list);
 }
 
 const size_t new_size = sc_list.GetSize();
@@ -384,10 +383,9 @@
   lookup_info.Prune(sc_list, old_size);
   } else {
 std::lock_guard guard(m_modules_mutex);
-collection::const_iterator pos, end = m_modules.end();
-for (pos = m_modules.begin(); pos != end; ++pos) {
-  (*pos)->FindFunctions(name, CompilerDeclContext(), name_type_mask,
-options, sc_list);
+for (const ModuleSP _sp : m_modules) {
+  module_sp->FindFunctions(name, CompilerDeclContext(), name_type_mask,
+   options, sc_list);
 }
   }
 }
@@ -401,10 +399,9 @@
 Module::LookupInfo lookup_info(name, name_type_mask, eLanguageTypeUnknown);
 
 std::lock_guard guard(m_modules_mutex);
-collection::const_iterator pos, end = m_modules.end();
-for (pos = m_modules.begin(); pos != end; ++pos) {
-  (*pos)->FindFunctionSymbols(lookup_info.GetLookupName(),
-  lookup_info.GetNameTypeMask(), sc_list);
+for (const ModuleSP _sp : m_modules) {
+  module_sp->FindFunctionSymbols(lookup_info.GetLookupName(),
+ lookup_info.GetNameTypeMask(), sc_list);
 }
 
 const size_t new_size = sc_list.GetSize();
@@ -413,9 +410,8 @@
   lookup_info.Prune(sc_list, old_size);
   } else {
 std::lock_guard guard(m_modules_mutex);
-collection::const_iterator pos, end = m_modules.end();
-for (pos = m_modules.begin(); pos != end; ++pos) {
-  (*pos)->FindFunctionSymbols(name, name_type_mask, sc_list);
+for (const ModuleSP _sp : m_modules) {
+  module_sp->FindFunctionSymbols(name, name_type_mask, sc_list);
 }
   }
 }
@@ -424,28 +420,23 @@
const ModuleFunctionSearchOptions ,
SymbolContextList _list) {
   std::lock_guard guard(m_modules_mutex);
-  collection::const_iterator pos, end = m_modules.end();
-  for (pos = m_modules.begin(); pos != end; ++pos) {
-(*pos)->FindFunctions(name, options, sc_list);
-  }
+  for (const ModuleSP _sp : m_modules)
+module_sp->FindFunctions(name, options, sc_list);
 }
 
 void ModuleList::FindCompileUnits(const FileSpec ,
   SymbolContextList _list) const {
   std::lock_guard guard(m_modules_mutex);
-  collection::const_iterator pos, end = m_modules.end();
-  for (pos = m_modules.begin(); pos != end; ++pos) {
-(*pos)->FindCompileUnits(path, sc_list);
-  }
+  for (const ModuleSP _sp : m_modules)
+module_sp->FindCompileUnits(path, sc_list);
 }
 
 void ModuleList::FindGlobalVariables(ConstString name, size_t max_matches,
  VariableList _list) const {
   std::lock_guard guard(m_modules_mutex);
-  collection::const_iterator pos, end = m_modules.end();
-  for (pos = m_modules.begin(); pos != end; ++pos) {
-(*pos)->FindGlobalVariables(name, CompilerDeclContext(), max_matches,
-

[Lldb-commits] [PATCH] D112615: [lldb] Make SBType::IsTypeComplete more consistent by forcing the loading of definitions

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG85bcc1eb2f56: [lldb] Make SBType::IsTypeComplete more 
consistent by forcing the loading of… (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112615

Files:
  lldb/bindings/interface/SBType.i
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/test/API/functionalities/type_completion/Makefile
  lldb/test/API/functionalities/type_completion/TestTypeCompletion.py
  lldb/test/API/functionalities/type_completion/main.cpp
  lldb/test/API/lang/cpp/complete-type-check/Makefile
  lldb/test/API/lang/cpp/complete-type-check/TestCppIsTypeComplete.py
  lldb/test/API/lang/cpp/complete-type-check/main.cpp
  lldb/test/API/lang/objc/complete-type-check/Makefile
  lldb/test/API/lang/objc/complete-type-check/TestObjCIsTypeComplete.py
  lldb/test/API/lang/objc/complete-type-check/main.m

Index: lldb/test/API/lang/objc/complete-type-check/main.m
===
--- /dev/null
+++ lldb/test/API/lang/objc/complete-type-check/main.m
@@ -0,0 +1,19 @@
+#include 
+
+@class IncompleteClass;
+
+@interface CompleteClass : NSObject
+@end
+
+@interface CompleteClassWithImpl : NSObject
+@end
+@implementation CompleteClassWithImpl
+@end
+
+IncompleteClass *incomplete = 0;
+CompleteClass *complete = 0;
+CompleteClassWithImpl *complete_impl = 0;
+
+int main() {
+  return 0; // break here
+}
Index: lldb/test/API/lang/objc/complete-type-check/TestObjCIsTypeComplete.py
===
--- /dev/null
+++ lldb/test/API/lang/objc/complete-type-check/TestObjCIsTypeComplete.py
@@ -0,0 +1,39 @@
+""" Tests SBType.IsTypeComplete on Objective-C types. """
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipUnlessDarwin
+@no_debug_info_test
+def test(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.m"))
+
+# A class that is only forward declared is not complete.
+incomplete = self.expect_expr("incomplete", result_type="IncompleteClass *")
+self.assertTrue(incomplete.IsValid())
+incomplete_class = incomplete.GetType().GetPointeeType()
+self.assertTrue(incomplete_class.IsValid())
+self.assertFalse(incomplete_class.IsTypeComplete())
+
+# A class that has its interface fully declared is complete.
+complete = self.expect_expr("complete", result_type="CompleteClass *")
+self.assertTrue(complete.IsValid())
+complete_class = complete.GetType().GetPointeeType()
+self.assertTrue(complete_class.IsValid())
+self.assertTrue(complete_class.IsTypeComplete())
+
+# A class that has its interface fully declared and an implementation
+# is also complete.
+complete_with_impl = self.expect_expr("complete_impl",
+result_type="CompleteClassWithImpl *")
+self.assertTrue(complete_with_impl.IsValid())
+complete_class_with_impl = complete_with_impl.GetType().GetPointeeType()
+self.assertTrue(complete_class_with_impl.IsValid())
+self.assertTrue(complete_class_with_impl.IsTypeComplete())
Index: lldb/test/API/lang/objc/complete-type-check/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/objc/complete-type-check/Makefile
@@ -0,0 +1,5 @@
+OBJC_SOURCES := main.m
+LD_EXTRAS := -framework Foundation
+CFLAGS_EXTRAS := -fobjc-arc
+
+include Makefile.rules
Index: lldb/test/API/lang/cpp/complete-type-check/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/complete-type-check/main.cpp
@@ -0,0 +1,36 @@
+struct EmptyClass {};
+struct DefinedClass {
+  int i;
+};
+typedef DefinedClass DefinedClassTypedef;
+
+struct FwdClass;
+typedef FwdClass FwdClassTypedef;
+
+template  struct DefinedTemplateClass {};
+template <> struct DefinedTemplateClass {};
+
+template  struct FwdTemplateClass;
+template <> struct FwdTemplateClass;
+
+enum class EnumClassFwd;
+
+enum DefinedEnum { Case1 };
+enum DefinedEnumClass { Case2 };
+
+EmptyClass empty_class;
+DefinedClass defined_class;
+DefinedClassTypedef defined_class_typedef;
+
+FwdClass *fwd_class;
+FwdClassTypedef *fwd_class_typedef;
+
+DefinedTemplateClass defined_template_class;
+FwdTemplateClass *fwd_template_class;
+
+EnumClassFwd *fwd_enum_class = nullptr;
+
+DefinedEnum defined_enum = Case1;
+DefinedEnumClass defined_enum_class = DefinedEnumClass::Case2;
+
+int main() {}
Index: lldb/test/API/lang/cpp/complete-type-check/TestCppIsTypeComplete.py

[Lldb-commits] [PATCH] D112697: [lldb] Update field offset/sizes when encountering artificial members such as vtable pointers

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe2ede1715d41: [lldb] Update field offset/sizes when 
encountering artificial members such as… (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112697

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
  lldb/test/API/lang/cpp/bitfields/main.cpp


Index: lldb/test/API/lang/cpp/bitfields/main.cpp
===
--- lldb/test/API/lang/cpp/bitfields/main.cpp
+++ lldb/test/API/lang/cpp/bitfields/main.cpp
@@ -97,7 +97,7 @@
 
 struct WithVTableAndUnnamed {
   virtual ~WithVTableAndUnnamed() {}
-  unsigned a : 4;
+  unsigned : 4;
   unsigned b : 4;
   unsigned c : 4;
 };
@@ -146,7 +146,6 @@
   with_vtable.b = 0;
   with_vtable.c = 5;
 
-  with_vtable_and_unnamed.a = 5;
   with_vtable_and_unnamed.b = 0;
   with_vtable_and_unnamed.c = 5;
 
Index: lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
===
--- lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
+++ lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
@@ -145,8 +145,6 @@
 self.expect_expr("base_with_vtable", 
result_children=base_with_vtable_children)
 self.expect_var_path("base_with_vtable", 
children=base_with_vtable_children)
 
-# FIXME: These all crash due the vtable ptr.
-@skipIf
 @no_debug_info_test
 def test_bitfield_behind_vtable_ptr(self):
 self.build()
@@ -164,7 +162,7 @@
 
 # Test a class with a vtable ptr and unnamed bitfield directly after.
 with_vtable_and_unnamed_children = [
-ValueCheck(name="", type="unsigned int:4", value="0"),
+ValueCheck(name="", type="int:4", value="0"),
 ValueCheck(name="b", type="unsigned int:4", value="0"),
 ValueCheck(name="c", type="unsigned int:4", value="5")
 ]
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2675,11 +2675,6 @@
 
   // FIXME: Remove the workarounds below and make this const.
   MemberAttributes attrs(die, parent_die, module_sp);
-  // Skip artificial members such as vtable pointers.
-  // FIXME: This check should verify that this is indeed an artificial member
-  // we are supposed to ignore.
-  if (attrs.is_artificial)
-return;
 
   const bool class_is_objc_object_or_interface =
   TypeSystemClang::IsObjCObjectOrInterfaceType(class_clang_type);
@@ -2845,6 +2840,17 @@
 last_field_info.SetIsBitfield(false);
   }
 
+  // Don't turn artificial members such as vtable pointers into real FieldDecls
+  // in our AST. Clang will re-create those articial members and they would
+  // otherwise just overlap in the layout with the FieldDecls we add here.
+  // This needs to be done after updating FieldInfo which keeps track of where
+  // field start/end so we don't later try to fill the the space of this
+  // artificial member with (unnamed bitfield) padding.
+  // FIXME: This check should verify that this is indeed an artificial member
+  // we are supposed to ignore.
+  if (attrs.is_artificial)
+return;
+
   if (!member_clang_type.IsCompleteType())
 member_clang_type.GetCompleteType();
 


Index: lldb/test/API/lang/cpp/bitfields/main.cpp
===
--- lldb/test/API/lang/cpp/bitfields/main.cpp
+++ lldb/test/API/lang/cpp/bitfields/main.cpp
@@ -97,7 +97,7 @@
 
 struct WithVTableAndUnnamed {
   virtual ~WithVTableAndUnnamed() {}
-  unsigned a : 4;
+  unsigned : 4;
   unsigned b : 4;
   unsigned c : 4;
 };
@@ -146,7 +146,6 @@
   with_vtable.b = 0;
   with_vtable.c = 5;
 
-  with_vtable_and_unnamed.a = 5;
   with_vtable_and_unnamed.b = 0;
   with_vtable_and_unnamed.c = 5;
 
Index: lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
===
--- lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
+++ lldb/test/API/lang/cpp/bitfields/TestCppBitfields.py
@@ -145,8 +145,6 @@
 self.expect_expr("base_with_vtable", result_children=base_with_vtable_children)
 self.expect_var_path("base_with_vtable", children=base_with_vtable_children)
 
-# FIXME: These all crash due the vtable ptr.
-@skipIf
 @no_debug_info_test
 def test_bitfield_behind_vtable_ptr(self):
 self.build()
@@ -164,7 +162,7 @@
 
 # Test a class with a vtable ptr and unnamed bitfield directly after.
 with_vtable_and_unnamed_children = [
-ValueCheck(name="", type="unsigned int:4", value="0"),
+ 

[Lldb-commits] [PATCH] D112802: [lldb/test] Replace shlex.join with shlex.quote

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

Thanks!




Comment at: lldb/packages/Python/lldbsuite/support/seven.py:54
+
+def join_for_shell(split_command):
+return " ".join([shlex.quote(part) for part in split_command])

```
TODO: Replace this with `shlex.join` when minimum Python version is >= 3.8
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112802

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

[Lldb-commits] [PATCH] D112586: [lldb] Remove forgotten FIXME on CPlusPlus formatters

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGac73f567cffb: [lldb] Remove forgotten FIXME on CPlusPlus 
formatters (authored by ljmf00, committed by teemperor).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112586

Files:
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp


Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -1014,8 +1014,6 @@
   .SetShowMembersOneLiner(false)
   .SetHideItemNames(false);
 
-  // FIXME because of a bug in the FormattersContainer we need to add a summary
-  // for both X* and const X* ()
   AddCXXSummary(
   cpp_category_sp, lldb_private::formatters::Char8StringSummaryProvider,
   "char8_t * summary provider", ConstString("char8_t *"), string_flags);


Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -1014,8 +1014,6 @@
   .SetShowMembersOneLiner(false)
   .SetHideItemNames(false);
 
-  // FIXME because of a bug in the FormattersContainer we need to add a summary
-  // for both X* and const X* ()
   AddCXXSummary(
   cpp_category_sp, lldb_private::formatters::Char8StringSummaryProvider,
   "char8_t * summary provider", ConstString("char8_t *"), string_flags);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112586: [lldb] Remove forgotten FIXME on CPlusPlus formatters

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D112586#3094519 , @ljmf00 wrote:

> In D112586#3093490 , @teemperor 
> wrote:
>
>> LGTM, thanks.
>
> Note: I can't land it

I can land it for you tomorrow (unless someone beats me to it :) )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112586

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

[Lldb-commits] [PATCH] D110827: [LLDB] Provide target specific directories to libclang

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

Apologies for the delay, this slipped out of my review queue by accident.

The patch looks in general pretty much ready, I just have a few nits here and 
there.

(Also just to clarify what this code does. It scans the support files of some 
(LLDB) modules and then returns a list of include paths that are needed to 
compile the correct libc++/libc of the target. I fixed up the class docs a bit 
to make this clearer).




Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp:34
 
-bool CppModuleConfiguration::analyzeFile(const FileSpec ) {
+static void getTargetIncludePaths(const llvm::Triple ,
+  std::vector ) {

Could this just return a `llvm::SmallVector` ? Then you can 
just use it directly in the for-range below without any variable.

Some docs would be nice:
```
lang=c++
/// Returns the target-specific default include paths for libc.
```



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp:46
+
+static bool guessIncludePath(llvm::StringRef pathToFile,
+ llvm::StringRef pattern, llvm::StringRef ) 
{

Could this just return `llvm::Optional`? Also the parameter should 
technically be `path_to_file` (this file uses LLDB's `variable_naming_style` 
instead of the `usualNamingStyle`, even though the existing code already has 
some code style issues where it deviated towards LLVM's style but oh well)



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp:46
+
+static bool guessIncludePath(llvm::StringRef pathToFile,
+ llvm::StringRef pattern, llvm::StringRef ) 
{

teemperor wrote:
> Could this just return `llvm::Optional`? Also the parameter 
> should technically be `path_to_file` (this file uses LLDB's 
> `variable_naming_style` instead of the `usualNamingStyle`, even though the 
> existing code already has some code style issues where it deviated towards 
> LLVM's style but oh well)
```
lang=c++
/// Returns the include path matching the given pattern for the given file path 
(or None if the path doesn't match the pattern).
```



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp:78
+
+posix_dir.consume_back("c++/v1");
+llvm::SmallVector tmp;

```
lang=c++
// Check if this is a target-specific libc++ include directory.
```



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp:81
+return m_std_target_inc.TrySet(
+(posix_dir + triple.str() + "/c++/v1").toStringRef(tmp));
   }

`.str()` instead of `.toStringRef(tmp)` should also work.



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h:46
+  /// If valid, the per-target include path used for the std module.
+  SetOncePath m_std_target_inc;
   /// If valid, the include path to the C library (e.g. /usr/include).

Please add a comment that this is optional unlike the other members:
```/// This is an optional path only required on some systems.```



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h:51
+  /// (e.g. /usr/include/x86_64-linux-gnu).
+  SetOncePath m_c_target_inc;
   /// The Clang resource include path for this configuration.

Same as above.



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h:65
   /// Creates a configuration by analyzing the given list of used source files.
-  explicit CppModuleConfiguration(const FileSpecList _files);
+  explicit CppModuleConfiguration(const FileSpecList _files,
+  const llvm::Triple );

```
lang=c++
/// The triple (if valid) is used to search for target-specific include paths.
```



Comment at: lldb/unittests/Expression/CppModuleConfigurationTest.cpp:80
+   libcpp_target));
 }
 

Could you please duplicate this test and make the target-specific directory its 
own new test? E.g. `TEST_F(CppModuleConfigurationTest, 
LinuxTargetSpecificInclude)`. Just to make sure we also still support the old 
behaviour.



Comment at: lldb/unittests/Expression/CppModuleConfigurationTest.cpp:132
   EXPECT_THAT(config.GetIncludeDirs(),
-  testing::ElementsAre(libcpp, ResourceInc(), usr));
+  testing::ElementsAre(libcpp, ResourceInc(), usr, libcpp_target));
 }

Same as above: both the new and old behaviour should work so just copy this 
into a new test.


Repository:
  rG LLVM Github Monorepo

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

[Lldb-commits] [PATCH] D112708: [lldb] Make TypeSystemClang::GetFullyUnqualifiedType work for constant arrays

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

In D112708#3093318 , @labath wrote:

> In D112708#3093252 , @teemperor 
> wrote:
>
>> LGTM, could you also extend a non-unit test to test the change within the 
>> whole FormatManager/etc. setup? I think `TestDataFormatterAdv.py` already 
>> has a very similar section about ignoring cv on types.
>
> I've added a simple check to that test. I didn't want to go overboard, as I 
> already have an made an exhaustive test in the follow-up patch.

Fair, LGTM then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112708

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

[Lldb-commits] [PATCH] D112586: [lldb] Remove forgotten FIXME on CPlusPlus formatters

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added subscribers: labath, teemperor.
teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112586

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

[Lldb-commits] [PATCH] D112708: [lldb] Make TypeSystemClang::GetFullyUnqualifiedType work for constant arrays

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

LGTM, could you also extend a non-unit test to test the change within the whole 
FormatManager/etc. setup? I think `TestDataFormatterAdv.py` already has a very 
similar section about ignoring cv on types.




Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:4243
+   ast->getAsConstantArrayType(qual_type)) {
+arr->getElementType();
+qual_type = ast->getConstantArrayType(

?



Comment at: lldb/unittests/Symbol/TestTypeSystemClang.cpp:919
+  EXPECT_EQ(bool_, cv_bool.GetFullyUnqualifiedType());
+  EXPECT_EQ(bool_.GetArrayType(47),
+cv_bool.GetArrayType(47).GetFullyUnqualifiedType());

My head has a hard time parsing what those asserts do or test. Maybe add some 
summary above or something similar?
```
lang=c++
// unqualified(const volatile bool[47]) -> bool[47]
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112708

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

[Lldb-commits] [PATCH] D112706: [lldb/test] Allow indentation in inline tests

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM, thanks.

On a more general note: I wish we would just kill off inline tests instead of 
trying to make them usable. But I think that's a discussion for the mailing 
lists...




Comment at: lldb/test/API/test_utils/TestInlineTest.py:10
+
+def test(self):
+filename = self.getBuildArtifact("test_file.cpp")

nit: maybe give this a more unique name + a doc string.



Comment at: lldb/test/API/test_utils/TestInlineTest.py:10
+
+def test(self):
+filename = self.getBuildArtifact("test_file.cpp")

teemperor wrote:
> nit: maybe give this a more unique name + a doc string.
Also I think this should be a NO_DEBUG_INFO_TESTCASE ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112706

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

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Given that `shlex.join` is only used for making the diagnostics copy-pastable 
into the terminal, we could probably get away by just making it use the normal 
string conversion of list or something like `" ".join(...)`. I don't think we 
have anyone that really debugs tests in Python2 builds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112212

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

[Lldb-commits] [PATCH] D112379: [lldb][NFC] Modernize for-loops in ModuleList

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Straightforward but looking for a second set of eyes




Comment at: lldb/source/Core/ModuleList.cpp:204
+bool ModuleList::AppendIfNeeded(const ModuleSP _module, bool notify) {
+  if (new_module) {
 std::lock_guard guard(m_modules_mutex);

I'll make this early-exit as a follow-up (along with some other functions that 
could do the same).


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

https://reviews.llvm.org/D112379

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

[Lldb-commits] [PATCH] D112379: [lldb][NFC] Modernize for-loops in ModuleList

[Raphael Isemann via Phabricator via lldb-commits]

teemperor created this revision.
teemperor added a reviewer: LLDB.
teemperor added a project: LLDB.
Herald added a subscriber: JDevlieghere.
teemperor requested review of this revision.

https://reviews.llvm.org/D112379

Files:
  lldb/include/lldb/Core/ModuleList.h
  lldb/source/Core/ModuleList.cpp

Index: lldb/source/Core/ModuleList.cpp
===
--- lldb/source/Core/ModuleList.cpp
+++ lldb/source/Core/ModuleList.cpp
@@ -200,16 +200,15 @@
   }
 }
 
-bool ModuleList::AppendIfNeeded(const ModuleSP _sp, bool notify) {
-  if (module_sp) {
+bool ModuleList::AppendIfNeeded(const ModuleSP _module, bool notify) {
+  if (new_module) {
 std::lock_guard guard(m_modules_mutex);
-collection::iterator pos, end = m_modules.end();
-for (pos = m_modules.begin(); pos != end; ++pos) {
-  if (pos->get() == module_sp.get())
+for (const ModuleSP _sp : m_modules) {
+  if (module_sp.get() == new_module.get())
 return false; // Already in the list
 }
 // Only push module_sp on the list if it wasn't already in there.
-Append(module_sp, notify);
+Append(new_module, notify);
 return true;
   }
   return false;
@@ -372,10 +371,10 @@
 Module::LookupInfo lookup_info(name, name_type_mask, eLanguageTypeUnknown);
 
 std::lock_guard guard(m_modules_mutex);
-collection::const_iterator pos, end = m_modules.end();
-for (pos = m_modules.begin(); pos != end; ++pos) {
-  (*pos)->FindFunctions(lookup_info.GetLookupName(), CompilerDeclContext(),
-lookup_info.GetNameTypeMask(), options, sc_list);
+for (const ModuleSP _sp : m_modules) {
+  module_sp->FindFunctions(lookup_info.GetLookupName(),
+   CompilerDeclContext(),
+   lookup_info.GetNameTypeMask(), options, sc_list);
 }
 
 const size_t new_size = sc_list.GetSize();
@@ -384,10 +383,9 @@
   lookup_info.Prune(sc_list, old_size);
   } else {
 std::lock_guard guard(m_modules_mutex);
-collection::const_iterator pos, end = m_modules.end();
-for (pos = m_modules.begin(); pos != end; ++pos) {
-  (*pos)->FindFunctions(name, CompilerDeclContext(), name_type_mask,
-options, sc_list);
+for (const ModuleSP _sp : m_modules) {
+  module_sp->FindFunctions(name, CompilerDeclContext(), name_type_mask,
+   options, sc_list);
 }
   }
 }
@@ -401,10 +399,9 @@
 Module::LookupInfo lookup_info(name, name_type_mask, eLanguageTypeUnknown);
 
 std::lock_guard guard(m_modules_mutex);
-collection::const_iterator pos, end = m_modules.end();
-for (pos = m_modules.begin(); pos != end; ++pos) {
-  (*pos)->FindFunctionSymbols(lookup_info.GetLookupName(),
-  lookup_info.GetNameTypeMask(), sc_list);
+for (const ModuleSP _sp : m_modules) {
+  module_sp->FindFunctionSymbols(lookup_info.GetLookupName(),
+ lookup_info.GetNameTypeMask(), sc_list);
 }
 
 const size_t new_size = sc_list.GetSize();
@@ -413,9 +410,8 @@
   lookup_info.Prune(sc_list, old_size);
   } else {
 std::lock_guard guard(m_modules_mutex);
-collection::const_iterator pos, end = m_modules.end();
-for (pos = m_modules.begin(); pos != end; ++pos) {
-  (*pos)->FindFunctionSymbols(name, name_type_mask, sc_list);
+for (const ModuleSP _sp : m_modules) {
+  module_sp->FindFunctionSymbols(name, name_type_mask, sc_list);
 }
   }
 }
@@ -424,28 +420,23 @@
const ModuleFunctionSearchOptions ,
SymbolContextList _list) {
   std::lock_guard guard(m_modules_mutex);
-  collection::const_iterator pos, end = m_modules.end();
-  for (pos = m_modules.begin(); pos != end; ++pos) {
-(*pos)->FindFunctions(name, options, sc_list);
-  }
+  for (const ModuleSP _sp : m_modules)
+module_sp->FindFunctions(name, options, sc_list);
 }
 
 void ModuleList::FindCompileUnits(const FileSpec ,
   SymbolContextList _list) const {
   std::lock_guard guard(m_modules_mutex);
-  collection::const_iterator pos, end = m_modules.end();
-  for (pos = m_modules.begin(); pos != end; ++pos) {
-(*pos)->FindCompileUnits(path, sc_list);
-  }
+  for (const ModuleSP _sp : m_modules)
+module_sp->FindCompileUnits(path, sc_list);
 }
 
 void ModuleList::FindGlobalVariables(ConstString name, size_t max_matches,
  VariableList _list) const {
   std::lock_guard guard(m_modules_mutex);
-  collection::const_iterator pos, end = m_modules.end();
-  for (pos = m_modules.begin(); pos != end; ++pos) {
-(*pos)->FindGlobalVariables(name, CompilerDeclContext(), max_matches,
-variable_list);
+  for (const ModuleSP _sp : m_modules) {
+module_sp->FindGlobalVariables(name, CompilerDeclContext(), max_matches,
+  

[Lldb-commits] [PATCH] D112180: Libcpp bitset syntethic children and tests

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

Thanks! This is looking pretty good, I just have some final minor comments 
about some code that I think we can also drop/split-out.




Comment at: lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp:20
 
-class BitsetFrontEnd : public SyntheticChildrenFrontEnd {
+// This class can be used for handling bitsets from both libcxx and libstdcpp.
+class GenericBitsetFrontEnd : public SyntheticChildrenFrontEnd {

nit: `///` to make this into a proper doc string.



Comment at: lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp:23
 public:
-  BitsetFrontEnd(ValueObject );
+  enum class StdLib {
+LibCxx,

I like it!



Comment at: lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp:90
+  // template argument.
+  ValueObjectSP dereferenced_type = m_backend.GetSP();
+  if (m_backend.GetCompilerType().IsPointerOrReferenceType()) {

If you look in `CPlusPlusLanguage.cpp` there is a thing called 
`stl_deref_flags` and you can just use the same two lines that define that 
variable in the Libstdc++ init function. The 
`stl_deref_flags.SetFrontEndWantsDereference();` will set a flag that does this 
whole deref-stuff here automatically (that's why this code wasn't necessary for 
the libc++ provider).



Comment at: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/bitset/TestDataFormatterGenericBitset.py:66
+self.check("ptr", 13)
+self.expect("p ref",
+substrs=['[0] = false',

is there a specific reason for these added `self.expect("p ...", substrs=[])` checks? I think testing the formatter with the expression evaluator 
is good, but there are far less verbose ways to test this (to be specific, 
`self.primes` could be a list of ValueCheck objects and you can pass that same 
ValueObject list as the `result_children`/`children` parameters to 
`self.expect_expr` or `self.expect_var_path`. This could be done in `check` so 
it would also cover all the checks in this test.).

So I would suggest we drop the added checks here and instead leave this up to a 
separate commit that refactors the test (you can do that if you want, but I 
don't mind doing it myself). This commit is in the current state just 
repurposing the existing libcxx formatter (with all the potentially existing 
bugs), so I think this is good enough for landing without expanding the test. 
Otherwise I think these checks could be implemented in a better way with the 
ValueCheck approach (see above).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112180

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

[Lldb-commits] [PATCH] D112340: [lldb/Formatters] Remove space from vector type string summaries (NFCI)

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D112340#3081532 , @dblaikie wrote:

> Sorry I missed this - are these tested anywhere/should I have been able to 
> discover if these needed to be changed before I made the change?

TestCompactVectors tests this but its unfortunately marked as Darwin-only (as 
it includes the `Accelerate` framework). Providing a platform-neutral test that 
just typedef's the same things as Accelerate (in addition to the test including 
the real framework) seems like a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112340

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

[Lldb-commits] [PATCH] D112340: [lldb/Formatters] Remove space from vector type string summaries (NFCI)

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

Please also remove the FIXME, LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112340

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

[Lldb-commits] [PATCH] D112325: [lldb] Pass the target triple to the compiler when determining the DWARF version

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

Some nits but otherwise LGTM




Comment at: lldb/packages/Python/lldbsuite/test/builders/builder.py:26
+"""Returns the triple for the given architecture or an empty string."""
+return ""
+

Maybe return `None`? Then the calling code crashes if it doesn't handle the 
value instead of silently picking the next arg as some bogus triple value.



Comment at: lldb/packages/Python/lldbsuite/test/builders/darwin.py:60
+vendor, os, version, env = get_triple()
+if arch is None or vendor is None or os is None or version is None or 
env is None:
+return ""


```
lang=python
values = [arch, vendor, os, version, env]
if None in values:
return ""
return '-'.join(values)
```


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

https://reviews.llvm.org/D112325

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

[Lldb-commits] [PATCH] D112147: [lldb] Fix lookup for global constants in namespaces

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added reviewers: clayborg, jankratochvil.
teemperor added a comment.

LGTM, but I have never touched this function so +Greg and Jan.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112147

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

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

This LGTM, but `shlex.join` is actually Py3 exclusive and I don't think there 
is a good Py2 replacement. I think we're just in time for the Py2->3 migration 
according to the timeline Jonas posted last year 
, so let's 
use this patch to actually do that? Then we can also get rid of all the `six` 
stuff etc.

Let's see if Jonas has any objections against dropping Py2 with this, otherwise 
this is good to go.




Comment at: lldb/test/API/test_utils/TestBaseTest.py:1
+"""
+Test TestBase test functions.

Could we move this file into `test_utils/build` or some other subdir? Then I 
can also make the few other 'test'-tests their own subdir of `test_utils` 
(which seems like a good place for all of this).



Comment at: lldb/test/API/test_utils/TestBaseTest.py:18
+
+def trace(self, *args, **kwargs):
+io = six.StringIO()

I think a comment that this overrides the normal test `trace` method would be 
nice (I wish Python had some builtin thing for indicating overrides...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112212

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

[Lldb-commits] [PATCH] D111686: Modify "statistics dump" to dump JSON.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D111686#3079248 , @clayborg wrote:

> So there is a buildbot failure due to an expression which succeeds, but 
> because the "expression" command options don't apply cleanly to the 
> expression result the "expression" command fails. Question: do we want 
> "expressionEvaluation" to truly track expression evaluation failures, or just 
> track the "expression" command success and fails? I updated this patch to 
> track expression failures only by modifying the Target::EvaluateExpression(), 
> so if this succeeds and "expr" command fails, we consider the expression a 
> success right now.

I have to look into this but could we skip that test in the meantime?

There is also another failure that looks like it's caused by some accidental 
change (see inline)




Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:761
 
+def getShlibBuildArtifact(self, DYLIB_NAME):
+"""Return absolute path to a shared library artifact given the library

clayborg wrote:
> wallace wrote:
> > is this actually used somewhere?
> Not yet, I will remove it
I think you also removed some existing stuff here:

```
PASS: LLDB (C:\buildbot\lldb-x64-windows-ninja\build\bin\clang.exe-x86_64) :: 
test_zero_uuid_modules (TestMiniDumpUUID.MiniDumpUUIDTestCase)
==
ERROR: test_relative_module_name (TestMiniDumpUUID.MiniDumpUUIDTestCase)
--
Traceback (most recent call last):
  File 
"C:\buildbot\lldb-x64-windows-ninja\llvm-project\lldb\test\API\functionalities\postmortem\minidump-new\TestMiniDumpUUID.py",
 line 317, in test_relative_module_name
self.getSourcePath("relative_module_name.yaml"))
AttributeError: 'MiniDumpUUIDTestCase' object has no attribute 'getSourcePath'
Config=x86_64-C:\buildbot\lldb-x64-windows-ninja\build\bin\clang.exe
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111686

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

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D112212#3078681 , @dblaikie wrote:

> Does this sort of thing itself get tested? (like this one had a test: 
> https://reviews.llvm.org/D111978 but not sure how much that 
> generalizes/whether there are different parts of the infrastructure are more 
> or less testable)

I think historically we haven't really tested the test infrastructure, but 
we're not historians (badum-ts) so I would vote in favour of adding tests for 
this (and the other testing infra).

(I'll also gladly write some tests for the already existing testing utils 
unless someone objects).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112212

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

[Lldb-commits] [PATCH] D112222: [LLDB] libcxx summary formatters for std::string_view

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D11#3077988 , @puremourning 
wrote:

> I'm happy to revert/split out the change for empty strings, as this is 
> perhaps contentious (and not exactly minimal for this patch).

I would actually prefer if we could split this into a separate change :) 
(Sorry!)

> Also, I'm only implementing libcxx (for now). We can follow up with libstdc++ 
> in another patch if this is accepted.

Sounds good to me.

> This is my first lldb patch, so apologies if I did something out of ordinary.

Nope, I think this is actually looking pretty good. I just have a few nits that 
I'll try get around today/tomorrow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D11

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

[Lldb-commits] [PATCH] D112180: Libcpp bitset syntethic children and tests

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

Thanks for working on this!

Some notes/nits:

- nit: The patch seems to have a bunch of unrelated clang-format changes for 
CPlusPlusLanguage.cpp in it. You can prevent that by using git clang-format on 
your local git commit before putting the diff on Phabricator (git tells 
clang-format which lines were modified by you and clang-format will only format 
those).
- LibStdcppBitset.cpp seems to be a 99% identical copy of LibCxxBitset.cpp. I 
think those two classes can just be one class with some variable for the 
different field name in each implementation.
- Same for the test. I think we can merge that into something like 
`.../data-formatter-stl/generic/bitset` and then just have two `test_...` 
methods where one is decorated for libc++ and one for libstdc++. You can change 
the Makefile values for each test with the `dictionary` argument of 
`self.build` (something like `self.build(dictionary={"USE_LIBSTDCPP":"1"})` 
should work I believe).




Comment at: lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt:9
   LibCxxBitset.cpp
+  LibStdcppBitset.cpp
   LibCxxInitializerList.cpp

I think the idea is to keep this sorted alphabetically (even though this 
position probably make more sense, but oh well...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112180

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

[Lldb-commits] [PATCH] D112163: Enable libc++ in the build for libcxx initializerlist pretty printers

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

There is one on Green Dragon with a few Clang versions 
 and I there 
is my Linux one with a all GCC/Clang versions 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112163

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

[Lldb-commits] [PATCH] D112163: Enable libc++ in the build for libcxx initializerlist pretty printers

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!

Tests with libc++ as a category are not run on Windows or when GCC is the test 
compiler, so those decorators are redundant. Removing the Clang XFAIL also 
seems fine, I would just see if any matrix bot complains and then re-apply it 
with a proper minimum required version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112163

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

[Lldb-commits] [PATCH] D112061: [lldb] Remove ConstString from GetPluginNameStatic of some plugins

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112061

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

[Lldb-commits] [PATCH] D111936: [lldb] Allow dumping the state of all scratch TypeSystems

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9a57d1e52680: [lldb] Allow dumping the state of all scratch 
TypeSystems (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111936

Files:
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/test/API/commands/target/dump/Makefile
  lldb/test/API/commands/target/dump/TestTargetDumpTypeSystem.py
  lldb/test/API/commands/target/dump/main.cpp

Index: lldb/test/API/commands/target/dump/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/target/dump/main.cpp
@@ -0,0 +1,7 @@
+struct DummyStruct {
+  int i;
+};
+
+DummyStruct s;
+
+int main() { return s.i; }
Index: lldb/test/API/commands/target/dump/TestTargetDumpTypeSystem.py
===
--- /dev/null
+++ lldb/test/API/commands/target/dump/TestTargetDumpTypeSystem.py
@@ -0,0 +1,33 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+import lldbsuite.test.lldbutil as lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@no_debug_info_test
+def test_dumping(self):
+""" Tests dumping an empty and non-empty scratch AST. """
+self.build()
+self.createTestTarget()
+
+# Make sure DummyStruct is not in the scratch AST by default.
+self.expect("target dump typesystem", matching=False, substrs=["struct DummyStruct"])
+
+# Move DummyStruct into the scratch AST via the expression evaluator.
+# FIXME: Once there is an SB API for using variable paths on globals
+# then this should be done this way.
+self.expect_expr("s", result_type="DummyStruct")
+
+# Dump the scratch AST and make sure DummyStruct is in there.
+self.expect("target dump typesystem", substrs=["struct DummyStruct"])
+
+@no_debug_info_test
+def test_invalid_arg(self):
+""" Test an invalid invocation on 'target dump typesystem'. """
+self.build()
+self.createTestTarget()
+self.expect("target dump typesystem arg", error=True,
+substrs=["error: target dump typesystem doesn't take arguments."])
Index: lldb/test/API/commands/target/dump/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/target/dump/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -938,7 +938,8 @@
   LLVM_DUMP_METHOD void dump(lldb::opaque_compiler_type_t type) const override;
 #endif
 
-  void Dump(Stream );
+  /// \see lldb_private::TypeSystem::Dump
+  void Dump(llvm::raw_ostream ) override;
 
   /// Dump clang AST types from the symbol file.
   ///
@@ -1162,6 +1163,9 @@
 return GetForTarget(target, InferIsolatedASTKindFromLangOpts(lang_opts));
   }
 
+  /// \see lldb_private::TypeSystem::Dump
+  void Dump(llvm::raw_ostream ) override;
+
   UserExpression *
   GetUserExpression(llvm::StringRef expr, llvm::StringRef prefix,
 lldb::LanguageType language,
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -8358,9 +8358,8 @@
 }
 #endif
 
-void TypeSystemClang::Dump(Stream ) {
-  Decl *tu = Decl::castFromDeclContext(GetTranslationUnitDecl());
-  tu->dump(s.AsRawOstream());
+void TypeSystemClang::Dump(llvm::raw_ostream ) {
+  GetTranslationUnitDecl()->dump(output);
 }
 
 void TypeSystemClang::DumpFromSymbolFile(Stream ,
@@ -9746,6 +9745,41 @@
   return _ast.GetIsolatedAST(*ast_kind);
 }
 
+/// Returns a human-readable name that uniquely identifiers the sub-AST kind.
+static llvm::StringRef
+GetNameForIsolatedASTKind(ScratchTypeSystemClang::IsolatedASTKind kind) {
+  switch (kind) {
+  case ScratchTypeSystemClang::IsolatedASTKind::CppModules:
+return "C++ modules";
+  }
+  llvm_unreachable("Unimplemented IsolatedASTKind?");
+}
+
+void ScratchTypeSystemClang::Dump(llvm::raw_ostream ) {
+  // First dump the main scratch AST.
+  output << "State of scratch Clang type 

[Lldb-commits] [PATCH] D111931: [lldb] Filter duplicates in Target::GetScratchTypeSystems

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcfaa5c344d5b: [lldb] Filter duplicates in 
Target::GetScratchTypeSystems (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111931

Files:
  lldb/source/Target/Target.cpp
  lldb/test/API/lang/c/builtin-types/TestCBuiltinTypes.py


Index: lldb/test/API/lang/c/builtin-types/TestCBuiltinTypes.py
===
--- /dev/null
+++ lldb/test/API/lang/c/builtin-types/TestCBuiltinTypes.py
@@ -0,0 +1,20 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@no_debug_info_test
+def test_FindTypes_on_scratch_AST(self):
+"""
+Tests FindTypes invoked with only LLDB's scratch AST present.
+"""
+target = self.dbg.GetDummyTarget()
+# There should be only one instance of 'unsigned long' in our single
+# scratch AST. Note: FindTypes/SBType hahave no filter by language, so
+# pick something that is unlikely to also be found in the scratch
+# TypeSystem of other language plugins.
+self.assertEqual(len(target.FindTypes("unsigned long")), 1)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -60,6 +60,7 @@
 #include "lldb/Utility/Timer.h"
 
 #include "llvm/ADT/ScopeExit.h"
+#include "llvm/ADT/SetVector.h"
 
 #include 
 #include 
@@ -2231,7 +2232,10 @@
   if (!m_valid)
 return {};
 
-  std::vector scratch_type_systems;
+  // Some TypeSystem instances are associated with several LanguageTypes so
+  // they will show up several times in the loop below. The SetVector filters
+  // out all duplicates as they serve no use for the caller.
+  llvm::SetVector scratch_type_systems;
 
   LanguageSet languages_for_expressions =
   Language::GetLanguagesSupportingTypeSystemsForExpressions();
@@ -2247,10 +2251,10 @@
  "system available",
  Language::GetNameForLanguageType(language));
 else
-  scratch_type_systems.emplace_back(_system_or_err.get());
+  scratch_type_systems.insert(_system_or_err.get());
   }
 
-  return scratch_type_systems;
+  return scratch_type_systems.takeVector();
 }
 
 PersistentExpressionState *


Index: lldb/test/API/lang/c/builtin-types/TestCBuiltinTypes.py
===
--- /dev/null
+++ lldb/test/API/lang/c/builtin-types/TestCBuiltinTypes.py
@@ -0,0 +1,20 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@no_debug_info_test
+def test_FindTypes_on_scratch_AST(self):
+"""
+Tests FindTypes invoked with only LLDB's scratch AST present.
+"""
+target = self.dbg.GetDummyTarget()
+# There should be only one instance of 'unsigned long' in our single
+# scratch AST. Note: FindTypes/SBType hahave no filter by language, so
+# pick something that is unlikely to also be found in the scratch
+# TypeSystem of other language plugins.
+self.assertEqual(len(target.FindTypes("unsigned long")), 1)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -60,6 +60,7 @@
 #include "lldb/Utility/Timer.h"
 
 #include "llvm/ADT/ScopeExit.h"
+#include "llvm/ADT/SetVector.h"
 
 #include 
 #include 
@@ -2231,7 +2232,10 @@
   if (!m_valid)
 return {};
 
-  std::vector scratch_type_systems;
+  // Some TypeSystem instances are associated with several LanguageTypes so
+  // they will show up several times in the loop below. The SetVector filters
+  // out all duplicates as they serve no use for the caller.
+  llvm::SetVector scratch_type_systems;
 
   LanguageSet languages_for_expressions =
   Language::GetLanguagesSupportingTypeSystemsForExpressions();
@@ -2247,10 +2251,10 @@
  "system available",
  Language::GetNameForLanguageType(language));
 else
-  scratch_type_systems.emplace_back(_system_or_err.get());
+  scratch_type_systems.insert(_system_or_err.get());
   }
 
-  return scratch_type_systems;
+  return scratch_type_systems.takeVector();
 }
 
 PersistentExpressionState *
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D111989: [lldb] Reduce code duplication around inferior building

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111989

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

[Lldb-commits] [PATCH] D111981: [lldb] Fix missing dependency on libc++ from LLDB test suite on non-Darwin platforms

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D111981#3071349 , @dblaikie wrote:

> So this'll add the right test dependency, if the libcxx project is enabled in 
> the build? (& if the build hasn't enabled libcxx, the tests will run, but 
> against the system compiler - and if that system compiler happens to default 
> to libc++ ( 
> https://github.com/llvm/llvm-project/blob/e9e4fc0fd3e0780207c731a1f2b8f6aacd24e8f8/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/list/TestDataFormatterLibcxxList.py#L49
>  )) that should remove the "different results if you run `ninja check-lldb` 
> before or after `ninja check-libcxx`" issue? Great, thanks!
>
> Hmm, it doesn't appear to be working for me, running this (in a clean build 
> directly, synced to 74c4d44d47b282769f6584153e9b433e8e5fa671 
>  which 
> includes 366fb539485a9753e4a8167fe5140bf4fb00a098 
>  (& 
> validated by inspecting `lldb/test/CMakeLists.txt`) still shows the tests as 
> unsupported:
>
>   CC=clang CXX=clang++ cmake -G Ninja -DCMAKE_BUILD_TYPE=Release 
> -DLLVM_ENABLE_WERROR=true 
> -DLLVM_ENABLE_PROJECTS='llvm;clang;clang-tools-extra;lld;lldb;cross-project-tests'
>  -DLLVM_ENABLE_RUNTIMES='compiler-rt;libcxx;libcxxabi;libunwind' 
> -DCMAKE_INSTALL_PREFIX=$HOME/install ~/dev/llvm/src/llvm
>   ninja 
> check-lldb-api-functionalities-data-formatter-data-formatter-stl-libcxx
>
> And then running `ninja cxx` and the same check again:
>
>   
>   FAIL: lldb-api :: 
> functionalities/data-formatter/data-formatter-stl/libcxx/set/TestDataFormatterLibcxxSet.py
>  (23 of 23)
>    TEST 'lldb-api :: 
> functionalities/data-formatter/data-formatter-stl/libcxx/set/TestDataFormatterLibcxxSet.py'
>  FAILED 
>   Script:
>   --
>   /usr/bin/python3.9 
> /usr/local/google/home/blaikie/dev/llvm/src/lldb/test/API/dotest.py -u 
> CXXFLAGS -u CFLAGS --env ARCHIVER=/usr/bin/ar --env OBJCOPY=/usr/bin/objcopy 
> --env 
> LLVM_LIBS_DIR=/usr/local/google/home/blaikie/dev/llvm/build/release/./lib 
> --arch x86_64 --build-dir 
> /usr/local/google/home/blaikie/dev/llvm/build/release/lldb-test-build.noindex 
> --lldb-module-cache-dir 
> /usr/local/google/home/blaikie/dev/llvm/build/release/lldb-test-build.noindex/module-cache-lldb/lldb-api
>  --clang-module-cache-dir 
> /usr/local/google/home/blaikie/dev/llvm/build/release/lldb-test-build.noindex/module-cache-clang/lldb-api
>  --executable 
> /usr/local/google/home/blaikie/dev/llvm/build/release/./bin/lldb --compiler 
> /usr/local/google/home/blaikie/dev/llvm/build/release/./bin/clang --dsymutil 
> /usr/local/google/home/blaikie/dev/llvm/build/release/./bin/dsymutil 
> --llvm-tools-dir /usr/local/google/home/blaikie/dev/llvm/build/release/./bin 
> --lldb-libs-dir /usr/local/google/home/blaikie/dev/llvm/build/release/./lib 
> /usr/local/google/home/blaikie/dev/llvm/src/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/set
>  -p TestDataFormatterLibcxxSet.py
>   --
>   Exit Code: 1
>   
>   Command Output (stdout):
>   --
>   lldb version 14.0.0 (g...@github.com:llvm/llvm-project.git revision 
> 74c4d44d47b282769f6584153e9b433e8e5fa671)
> clang revision 74c4d44d47b282769f6584153e9b433e8e5fa671
> llvm revision 74c4d44d47b282769f6584153e9b433e8e5fa671
>   Skipping the following test categories: ['dsym', 'gmodules', 'debugserver', 
> 'objc']
>   
>   --
>   Command Output (stderr):
>   --
>   UNSUPPORTED: LLDB 
> (/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
> test_ref_and_ptr_dsym 
> (TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase) (test case does 
> not fall in any category of interest for this run) 
>   FAIL: LLDB 
> (/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
> test_ref_and_ptr_dwarf 
> (TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase)
>   FAIL: LLDB 
> (/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
> test_ref_and_ptr_dwo 
> (TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase)
>   UNSUPPORTED: LLDB 
> (/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
> test_ref_and_ptr_gmodules 
> (TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase) (test case does 
> not fall in any category of interest for this run) 
>   UNSUPPORTED: LLDB 
> (/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
> test_with_run_command_dsym 
> (TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase) (test case does 
> not fall in any category of interest for this run) 
>   FAIL: LLDB 
> (/usr/local/google/home/blaikie/dev/llvm/build/release/bin/clang-x86_64) :: 
> test_with_run_command_dwarf 
> (TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase)
>   FAIL: LLDB 
> 

[Lldb-commits] [PATCH] D111981: [lldb] Fix missing dependency on libc++ from LLDB test suite on non-Darwin platforms

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG366fb539485a: [lldb] Fix missing dependency on libc++ from 
LLDB test suite on non-Darwin… (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111981

Files:
  lldb/test/CMakeLists.txt


Index: lldb/test/CMakeLists.txt
===
--- lldb/test/CMakeLists.txt
+++ lldb/test/CMakeLists.txt
@@ -135,9 +135,16 @@
 "`LLDB_INCLUDE_TESTS=OFF`.")
 endif()
   endif()
-  add_lldb_test_dependency(cxx)
 endif()
   endif()
+
+  # Add libc++ dependency for libc++-specific tests. This is an optional
+  # dependency as it's also possible to run the libc++ tests against the libc++
+  # installed on the system.
+  if (TARGET cxx)
+add_lldb_test_dependency(cxx)
+  endif()
+
 endif()
 
 if (LLDB_BUILT_STANDALONE)


Index: lldb/test/CMakeLists.txt
===
--- lldb/test/CMakeLists.txt
+++ lldb/test/CMakeLists.txt
@@ -135,9 +135,16 @@
 "`LLDB_INCLUDE_TESTS=OFF`.")
 endif()
   endif()
-  add_lldb_test_dependency(cxx)
 endif()
   endif()
+
+  # Add libc++ dependency for libc++-specific tests. This is an optional
+  # dependency as it's also possible to run the libc++ tests against the libc++
+  # installed on the system.
+  if (TARGET cxx)
+add_lldb_test_dependency(cxx)
+  endif()
+
 endif()
 
 if (LLDB_BUILT_STANDALONE)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D111715: [WIP] [lldb] change name demangling to be consistent between windows and linx

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D111715#3068412 , @thakis wrote:

> I don't have an opinion on this change and I don't mind the demangler change, 
> but isn't the type information helpful? The mangled itanium name doesn't 
> include type information which is why it's not printed, but the mangled ms 
> name does include it.
>
> But as I said, I don't have an opinion either way.

I think the motivation for this patch is just that LLDB uses the demangler to 
determine variable names and with the current MS demangler, the variable names 
returned by LLDB are for example `int (*array2d)[10]` on Windows instead of 
`array2d` (on platforms that use the itanium demangler). And that's clearly not 
what any user expects when they ask for the name of a variable behind a symbol. 
Note that type information in LLDB is a separate part of the information, so 
having the type information in the name gives the user no additional 
information.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111715

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

[Lldb-commits] [PATCH] D111632: [lldb] Split ParseSingleMember into Obj-C property and normal member/ivar parsing code.

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG60b96aa65e59: [lldb] Split ParseSingleMember into Obj-C 
property and normal member/ivar… (authored by teemperor).
Herald added a subscriber: lldb-commits.

Changed prior to commit:
  https://reviews.llvm.org/D111632?vs=378925=380175#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111632

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -187,11 +187,26 @@
 }
   };
 
+  /// Parses a DW_TAG_APPLE_property DIE and appends the parsed data to the
+  /// list of delayed Objective-C properties.
+  ///
+  /// Note: The delayed property needs to be finalized to actually create the
+  /// property declarations in the module AST.
+  ///
+  /// \param die The DW_TAG_APPLE_property DIE that will be parsed.
+  /// \param parent_die The parent DIE.
+  /// \param class_clang_type The Objective-C class that will contain the
+  /// created property.
+  /// \param delayed_properties The list of delayed properties that the result
+  /// will be appended to.
+  void ParseObjCProperty(const DWARFDIE , const DWARFDIE _die,
+ const lldb_private::CompilerType _clang_type,
+ DelayedPropertyList _properties);
+
   void
   ParseSingleMember(const DWARFDIE , const DWARFDIE _die,
 const lldb_private::CompilerType _clang_type,
 lldb::AccessType default_accessibility,
-DelayedPropertyList _properties,
 lldb_private::ClangASTImporter::LayoutInfo _info,
 FieldInfo _field_info);
 
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1915,11 +1915,10 @@
   const CompilerType _opaque_type, // The property type is only
 // required if you don't have an
 // ivar decl
-  clang::ObjCIvarDecl *ivar_decl, const char *property_setter_name,
-  const char *property_getter_name, uint32_t property_attributes,
-  const ClangASTMetadata *metadata)
+  const char *property_setter_name, const char *property_getter_name,
+  uint32_t property_attributes, const ClangASTMetadata *metadata)
   : m_class_opaque_type(class_opaque_type), m_property_name(property_name),
-m_property_opaque_type(property_opaque_type), m_ivar_decl(ivar_decl),
+m_property_opaque_type(property_opaque_type),
 m_property_setter_name(property_setter_name),
 m_property_getter_name(property_getter_name),
 m_property_attributes(property_attributes) {
@@ -1938,7 +1937,6 @@
 m_class_opaque_type = rhs.m_class_opaque_type;
 m_property_name = rhs.m_property_name;
 m_property_opaque_type = rhs.m_property_opaque_type;
-m_ivar_decl = rhs.m_ivar_decl;
 m_property_setter_name = rhs.m_property_setter_name;
 m_property_getter_name = rhs.m_property_getter_name;
 m_property_attributes = rhs.m_property_attributes;
@@ -1953,7 +1951,7 @@
   bool Finalize() {
 return TypeSystemClang::AddObjCClassProperty(
 m_class_opaque_type, m_property_name, m_property_opaque_type,
-m_ivar_decl, m_property_setter_name, m_property_getter_name,
+/*ivar_decl=*/nullptr, m_property_setter_name, m_property_getter_name,
 m_property_attributes, m_metadata_up.get());
   }
 
@@ -1961,7 +1959,6 @@
   CompilerType m_class_opaque_type;
   const char *m_property_name;
   CompilerType m_property_opaque_type;
-  clang::ObjCIvarDecl *m_ivar_decl;
   const char *m_property_setter_name;
   const char *m_property_getter_name;
   uint32_t m_property_attributes;
@@ -2639,13 +2636,51 @@
   }
 }
 
+void DWARFASTParserClang::ParseObjCProperty(
+const DWARFDIE , const DWARFDIE _die,
+const lldb_private::CompilerType _clang_type,
+DelayedPropertyList _properties) {
+  // This function can only parse DW_TAG_APPLE_property.
+  assert(die.Tag() == DW_TAG_APPLE_property);
+
+  ModuleSP module_sp = parent_die.GetDWARF()->GetObjectFile()->GetModule();
+
+  const MemberAttributes attrs(die, parent_die, module_sp);
+  const PropertyAttributes propAttrs(die);
+
+  if (!propAttrs.prop_name) {
+module_sp->ReportError(
+"0x%8.8" PRIx64 ": DW_TAG_APPLE_property has no name.", die.GetID());
+return;
+  }
+
+  Type *member_type = 

[Lldb-commits] [PATCH] D111877: [lldb] Return StringRef from PluginInterface::GetPluginName

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

I'm wondering if some of those should be `std::string` instead, but this is in 
any case a step in the right direction so LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111877

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

[Lldb-commits] [PATCH] D111816: [lldb] Remove logging from Platform::~Platform

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Fixed, thanks!

In D111816#3064923 , @thakis wrote:

> http://45.33.8.238/mac/37050/step_4.txt
>
>   In file included from 
> ../../lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:9:
>   In file included from 
> ../../lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.h:12:
>   In file included from 
> ../../../chrome/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/map:495:
>   In file included from 
> ../../../chrome/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/__node_handle:63:
>   In file included from 
> ../../../chrome/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/memory:682:
>   In file included from 
> ../../../chrome/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/__memory/shared_ptr.h:25:
>   
> ../../../chrome/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/__memory/unique_ptr.h:50:19:
>  error: invalid application of 'sizeof' to an incomplete type 
> 'lldb_private::ModuleCache'
>   static_assert(sizeof(_Tp) > 0,
> ^~~
>   
> ../../../chrome/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/__memory/unique_ptr.h:315:7:
>  note: in instantiation of member function 
> 'std::default_delete::operator()' requested here
> __ptr_.second()(__tmp);
> ^
>   
> ../../../chrome/src/third_party/llvm-build/Release+Asserts/bin/../include/c++/v1/__memory/unique_ptr.h:269:19:
>  note: in instantiation of member function 
> 'std::unique_ptr::reset' requested here
> ~unique_ptr() { reset(); }
> ^
>   ../../lldb/include/lldb/Target/Platform.h:78:3: note: in instantiation of 
> member function 'std::unique_ptr::~unique_ptr' 
> requested here
> ~Platform() override = default;
> ^
>   ../../lldb/include/lldb/Target/Platform.h:39:7: note: forward declaration 
> of 'lldb_private::ModuleCache'
>   class ModuleCache;
> ^




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111816

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

[Lldb-commits] [PATCH] D111816: [lldb] Remove logging from Platform::~Platform

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe632e900ac10: [lldb] Remove logging from Platform::~Platform 
(authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111816

Files:
  lldb/include/lldb/Target/Platform.h
  lldb/source/Target/Platform.cpp


Index: lldb/source/Target/Platform.cpp
===
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -395,15 +395,6 @@
   LLDB_LOGF(log, "%p Platform::Platform()", static_cast(this));
 }
 
-/// Destructor.
-///
-/// The destructor is virtual since this class is designed to be
-/// inherited from by the plug-in instance.
-Platform::~Platform() {
-  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT));
-  LLDB_LOGF(log, "%p Platform::~Platform()", static_cast(this));
-}
-
 void Platform::GetStatus(Stream ) {
   std::string s;
   strm.Printf("  Platform: %s\n", GetPluginName().GetCString());
Index: lldb/include/lldb/Target/Platform.h
===
--- lldb/include/lldb/Target/Platform.h
+++ lldb/include/lldb/Target/Platform.h
@@ -73,11 +73,9 @@
   /// Default Constructor
   Platform(bool is_host_platform);
 
-  /// Destructor.
-  ///
   /// The destructor is virtual since this class is designed to be inherited
   /// from by the plug-in instance.
-  ~Platform() override;
+  ~Platform() override = default;
 
   static void Initialize();
 


Index: lldb/source/Target/Platform.cpp
===
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -395,15 +395,6 @@
   LLDB_LOGF(log, "%p Platform::Platform()", static_cast(this));
 }
 
-/// Destructor.
-///
-/// The destructor is virtual since this class is designed to be
-/// inherited from by the plug-in instance.
-Platform::~Platform() {
-  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT));
-  LLDB_LOGF(log, "%p Platform::~Platform()", static_cast(this));
-}
-
 void Platform::GetStatus(Stream ) {
   std::string s;
   strm.Printf("  Platform: %s\n", GetPluginName().GetCString());
Index: lldb/include/lldb/Target/Platform.h
===
--- lldb/include/lldb/Target/Platform.h
+++ lldb/include/lldb/Target/Platform.h
@@ -73,11 +73,9 @@
   /// Default Constructor
   Platform(bool is_host_platform);
 
-  /// Destructor.
-  ///
   /// The destructor is virtual since this class is designed to be inherited
   /// from by the plug-in instance.
-  ~Platform() override;
+  ~Platform() override = default;
 
   static void Initialize();
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D111715: [WIP] [lldb] change name demangling to be consistent between windows and linx

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added reviewers: mstorsjo, rnk.
teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: JDevlieghere.

This looks in general good to me. You probably want to add a test for this 
(`llvm/test/Demangle/ms-options.test` seems like the right place). I added some 
folks to review the Demangler changes.

The LLDB itself LGTM, but it seems like this patch is also clang-formatting 
some lines that are unrelated to this patch, so please just clang-format those 
files as a preparatory NFC commit (If you don't have commit access then @werat 
or me can also just do that quickly for you). FWIW, there is a git clang-format 
script that limits clang-format changes to just the lines you touched.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111715

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

[Lldb-commits] [PATCH] D111634: [lldb] Print embedded nuls in char arrays (PR44649)

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: 
lldb/test/API/functionalities/data-formatter/stringprinter/main.cpp:40
+//% self.expect_var_path("c.data", summary=r'"F\0O"')
 //% self.runCmd("setting set escape-non-printables false")
 //% self.expect_var_path('stdstring', summary='"Hello\t\tWorld\nI am 
here\t\tto say hello\n"')

I think the whole 'empty lines break inline tests' thing has been fixed at some 
point, so could you drop a new line between this and the printable thing when 
you land?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111634

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

[Lldb-commits] [PATCH] D111494: [lldb][NFCI] Refactor out attribute parsing from DWARFASTParserClang::ParseSingleMember

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf110999bf6b5: [lldb][NFCI] Refactor out attribute parsing 
from DWARFASTParserClang… (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111494

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -2344,56 +2344,52 @@
   return nullptr;
 }
 
-void DWARFASTParserClang::ParseSingleMember(
-const DWARFDIE , const DWARFDIE _die,
-const lldb_private::CompilerType _clang_type,
-lldb::AccessType default_accessibility,
-DelayedPropertyList _properties,
-lldb_private::ClangASTImporter::LayoutInfo _info,
-FieldInfo _field_info) {
-  ModuleSP module_sp = parent_die.GetDWARF()->GetObjectFile()->GetModule();
-  const dw_tag_t tag = die.Tag();
-  // Get the parent byte size so we can verify any members will fit
-  const uint64_t parent_byte_size =
-  parent_die.GetAttributeValueAsUnsigned(DW_AT_byte_size, UINT64_MAX);
-  const uint64_t parent_bit_size =
-  parent_byte_size == UINT64_MAX ? UINT64_MAX : parent_byte_size * 8;
-
-  DWARFAttributes attributes;
-  const size_t num_attributes = die.GetAttributes(attributes);
-  if (num_attributes == 0)
-return;
-
+namespace {
+/// Parsed form of all attributes that are relevant for parsing type members.
+struct MemberAttributes {
+  explicit MemberAttributes(const DWARFDIE , const DWARFDIE _die,
+ModuleSP module_sp);
   const char *name = nullptr;
+  /// Indicates how many bits into the word (according to the host endianness)
+  /// the low-order bit of the field starts. Can be negative.
+  int64_t bit_offset = 0;
+  /// Indicates the size of the field in bits.
+  size_t bit_size = 0;
+  uint64_t data_bit_offset = UINT64_MAX;
+  AccessType accessibility = eAccessNone;
+  llvm::Optional byte_size;
+  DWARFFormValue encoding_form;
+  /// Indicates the byte offset of the word from the base address of the
+  /// structure.
+  uint32_t member_byte_offset;
+  bool is_artificial = false;
+  /// On DW_TAG_members, this means the member is static.
+  bool is_external = false;
+};
+
+/// Parsed form of all attributes that are relevant for parsing Objective-C
+/// properties.
+struct PropertyAttributes {
+  explicit PropertyAttributes(const DWARFDIE );
   const char *prop_name = nullptr;
   const char *prop_getter_name = nullptr;
   const char *prop_setter_name = nullptr;
+  /// \see clang::ObjCPropertyAttribute
   uint32_t prop_attributes = 0;
+};
+} // namespace
 
-  bool is_artificial = false;
-  DWARFFormValue encoding_form;
-  AccessType accessibility = eAccessNone;
-  uint32_t member_byte_offset =
-  (parent_die.Tag() == DW_TAG_union_type) ? 0 : UINT32_MAX;
-  llvm::Optional byte_size;
-  int64_t bit_offset = 0;
-  uint64_t data_bit_offset = UINT64_MAX;
-  size_t bit_size = 0;
-  bool is_external =
-  false; // On DW_TAG_members, this means the member is static
-  uint32_t i;
-  for (i = 0; i < num_attributes && !is_artificial; ++i) {
+MemberAttributes::MemberAttributes(const DWARFDIE ,
+   const DWARFDIE _die,
+   ModuleSP module_sp) {
+  member_byte_offset = (parent_die.Tag() == DW_TAG_union_type) ? 0 : UINT32_MAX;
+
+  DWARFAttributes attributes;
+  const size_t num_attributes = die.GetAttributes(attributes);
+  for (std::size_t i = 0; i < num_attributes; ++i) {
 const dw_attr_t attr = attributes.AttributeAtIndex(i);
 DWARFFormValue form_value;
 if (attributes.ExtractFormValueAtIndex(i, form_value)) {
-  // DW_AT_data_member_location indicates the byte offset of the
-  // word from the base address of the structure.
-  //
-  // DW_AT_bit_offset indicates how many bits into the word
-  // (according to the host endianness) the low-order bit of the
-  // field starts.  AT_bit_offset can be negative.
-  //
-  // DW_AT_bit_size indicates the size of the field in bits.
   switch (attr) {
   case DW_AT_name:
 name = form_value.AsCString();
@@ -2444,6 +2440,42 @@
   case DW_AT_artificial:
 is_artificial = form_value.Boolean();
 break;
+  case DW_AT_external:
+is_external = form_value.Boolean();
+break;
+  default:
+break;
+  }
+}
+  }
+
+  // Clang has a DWARF generation bug where sometimes it represents
+  // fields that are references with bad byte size and bit size/offset
+  // information such as:
+  //
+  //  DW_AT_byte_size( 0x00 )
+  //  DW_AT_bit_size( 0x40 )
+  //  DW_AT_bit_offset( 0xffc0 )
+  

[Lldb-commits] [PATCH] D108629: [lldb] Add support for DW_AT_calling_convention to the DWARF parser

[Raphael Isemann via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3256aa8fe6fd: [lldb] Add support for 
DW_AT_calling_convention to the DWARF parser (authored by teemperor).
Herald added a subscriber: lldb-commits.

Changed prior to commit:
  https://reviews.llvm.org/D108629?vs=368332=378623#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108629

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/test/API/lang/c/calling-conventions/Makefile
  lldb/test/API/lang/c/calling-conventions/TestCCallingConventions.py
  lldb/test/API/lang/c/calling-conventions/fastcall.c
  lldb/test/API/lang/c/calling-conventions/ms_abi.c
  lldb/test/API/lang/c/calling-conventions/pascal.c
  lldb/test/API/lang/c/calling-conventions/regcall.c
  lldb/test/API/lang/c/calling-conventions/stdcall.c
  lldb/test/API/lang/c/calling-conventions/sysv_abi.c
  lldb/test/API/lang/c/calling-conventions/vectorcall.c
  lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp

Index: lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
===
--- lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
+++ lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp
@@ -116,3 +116,159 @@
   testing::UnorderedElementsAre(decl_ctxs[0], decl_ctxs[3]));
 }
 
+TEST_F(DWARFASTParserClangTests, TestCallingConventionParsing) {
+  // Tests parsing DW_AT_calling_convention values.
+
+  // The DWARF below just declares a list of function types with
+  // DW_AT_calling_convention on them.
+  const char *yamldata = R"(
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:ELFDATA2LSB
+  Type:ET_EXEC
+  Machine: EM_386
+DWARF:
+  debug_str:
+- func1
+- func2
+- func3
+- func4
+- func5
+- func6
+- func7
+- func8
+- func9
+  debug_abbrev:
+- ID:  0
+  Table:
+- Code:0x1
+  Tag: DW_TAG_compile_unit
+  Children:DW_CHILDREN_yes
+  Attributes:
+- Attribute:   DW_AT_language
+  Form:DW_FORM_data2
+- Code:0x2
+  Tag: DW_TAG_subprogram
+  Children:DW_CHILDREN_no
+  Attributes:
+- Attribute:   DW_AT_low_pc
+  Form:DW_FORM_addr
+- Attribute:   DW_AT_high_pc
+  Form:DW_FORM_data4
+- Attribute:   DW_AT_name
+  Form:DW_FORM_strp
+- Attribute:   DW_AT_calling_convention
+  Form:DW_FORM_data1
+- Attribute:   DW_AT_external
+  Form:DW_FORM_flag_present
+  debug_info:
+- Version: 4
+  AddrSize:4
+  Entries:
+- AbbrCode:0x1
+  Values:
+- Value:   0xC
+- AbbrCode:0x2
+  Values:
+- Value:   0x0
+- Value:   0x5
+- Value:   0x00
+- Value:   0xCB
+- Value:   0x1
+- AbbrCode:0x2
+  Values:
+- Value:   0x10
+- Value:   0x5
+- Value:   0x06
+- Value:   0xB3
+- Value:   0x1
+- AbbrCode:0x2
+  Values:
+- Value:   0x20
+- Value:   0x5
+- Value:   0x0C
+- Value:   0xB1
+- Value:   0x1
+- AbbrCode:0x2
+  Values:
+- Value:   0x30
+- Value:   0x5
+- Value:   0x12
+- Value:   0xC0
+- Value:   0x1
+- AbbrCode:0x2
+  Values:
+- Value:   0x40
+- Value:   0x5
+- Value:   0x18
+- Value:   0xB2
+- Value:   0x1
+- AbbrCode:0x2
+  Values:
+- Value:   0x50
+- Value:   0x5
+- Value:   0x1E
+- Value:   0xC1
+- Value:   0x1
+- AbbrCode:0x2
+  Values:
+- Value:   0x60
+- Value:   0x5
+- Value:   0x24
+- Value:   0xC2
+- Value:   0x1
+- AbbrCode:0x2
+  Values:
+- Value:   0x70
+- Value:   0x5
+- Value:   0x2a
+- Value:   0xEE
+- Value:   0x1
+- 

[Lldb-commits] [PATCH] D111149: [lldb] Don't print to stderr in TypeSystemClang::GetBuiltinTypeForDWARFEncodingAndBitSize

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG592e89cc4e9a: [lldb] Dont print to stderr in 
TypeSystemClang… (authored by teemperor).
Herald added a subscriber: lldb-commits.

Changed prior to commit:
  https://reviews.llvm.org/D49?vs=377456=378622#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D49

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp


Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -1155,20 +1155,12 @@
 }
 break;
   }
-  // This assert should fire for anything that we don't catch above so we know
-  // to fix any issues we run into.
-  if (!type_name.empty()) {
-std::string type_name_str = type_name.str();
-Host::SystemLog(Host::eSystemLogError,
-"error: need to add support for DW_TAG_base_type '%s' "
-"encoded with DW_ATE = 0x%x, bit_size = %u\n",
-type_name_str.c_str(), dw_ate, bit_size);
-  } else {
-Host::SystemLog(Host::eSystemLogError, "error: need to add support for "
-   "DW_TAG_base_type encoded with "
-   "DW_ATE = 0x%x, bit_size = %u\n",
-dw_ate, bit_size);
-  }
+
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_TYPES);
+  LLDB_LOG(log,
+   "error: need to add support for DW_TAG_base_type '{0}' "
+   "encoded with DW_ATE = {1:x}, bit_size = {2}",
+   type_name, dw_ate, bit_size);
   return CompilerType();
 }
 


Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -1155,20 +1155,12 @@
 }
 break;
   }
-  // This assert should fire for anything that we don't catch above so we know
-  // to fix any issues we run into.
-  if (!type_name.empty()) {
-std::string type_name_str = type_name.str();
-Host::SystemLog(Host::eSystemLogError,
-"error: need to add support for DW_TAG_base_type '%s' "
-"encoded with DW_ATE = 0x%x, bit_size = %u\n",
-type_name_str.c_str(), dw_ate, bit_size);
-  } else {
-Host::SystemLog(Host::eSystemLogError, "error: need to add support for "
-   "DW_TAG_base_type encoded with "
-   "DW_ATE = 0x%x, bit_size = %u\n",
-dw_ate, bit_size);
-  }
+
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_TYPES);
+  LLDB_LOG(log,
+   "error: need to add support for DW_TAG_base_type '{0}' "
+   "encoded with DW_ATE = {1:x}, bit_size = {2}",
+   type_name, dw_ate, bit_size);
   return CompilerType();
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D111399: [lldb] Make char[N] formatters respect the end of the array (PR44649)

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM, but maybe give the test comment a FIXME.




Comment at: 
lldb/test/API/functionalities/data-formatter/stringprinter/main.cpp:39
+ //% self.expect_var_path('constcharstar', summary='"Hello\t\tWorld\nI am 
here\t\tto say hello\n"')
 //% 
self.assertTrue(self.frame().FindVariable('longstring').GetSummary().endswith('"...'))
 //% 
self.assertTrue(self.frame().FindVariable('longconstcharstar').GetSummary().endswith('"...'))

If you align the start of these comments when merging this, then I'll address 
you from now on as "Pavel the Great".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111399

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

[Lldb-commits] [PATCH] D111409: proposed support for Java interface to Scripting Bridge

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

No problem, first time using Phabricator is always a bit confusing. You can 
just do a `git diff -U99 > ~/java-patch.diff`, click the "Update Diff" 
button on the top right of this website and then select *just* this diff file 
that contains your changes. Phabricator will render the diff properly for you 
(-> it will hide all the diff context by default). There is need to attach a 
separate diff file or anything else (users can just download the diff you 
uploaded).

Regarding the tests: We would essentially just need some basic test that 
exercises the new API a bit so that we know this works. The test code itself 
will be straightforward, but we would need a nice way to (automatically) find 
the system JRE and then set it up to be able to run the test code.

In D111409#3051140 , @d-millar wrote:

> Am obviously brand new to your process and a bit of an old dog when it comes 
> to learning new tricks.  Would you prefer I make a new submission with the 
> -U99 diff?   Also, am more than willing to help with the Java tests if 
> that would be useful.
>
> 
>
> From: Raphael Isemann via Phabricator 
> Sent: Friday, October 8, 2021 10:46:50 AM
> To: David Millar; anoro...@apple.com; fallk...@yahoo.com; kkle...@redhat.com; 
> medismail.benn...@gmail.com; jo...@devlieghere.com; tedw...@quicinc.com; 
> jmole...@apple.com; syaghm...@apple.com; jing...@apple.com; v...@apple.com; 
> boris.ulasev...@gmail.com; lldb-commits@lists.llvm.org; h.imai@nitech.jp; 
> bruce.mitche...@gmail.com; david.spick...@linaro.org; 
> quic_soura...@quicinc.com; djordje.todoro...@syrmia.com; 
> serhiy.re...@gmail.com; liburd1...@outlook.com
> Cc: mgo...@gentoo.org
> Subject: [PATCH] D111409 : proposed support 
> for Java interface to Scripting Bridge
>
> teemperor added a comment.
>
> In D111409#3051110  
> https://reviews.llvm.org/D111409#3051110, @d-millar wrote:
>
>> Apologies for the inclusion of that last file "patch" - that is the "git 
>> diff -U999" result, should that be useful.
>
> You can just upload that diff file and Phabricator will display it properly. 
> There is no need to include the raw diff as part of the patch itself (it just 
> makes this diff 100 times larger than it needs to be) :)
>
> Anyway, I think this seems like a reasonable thing to have. We have to figure 
> out though how we can properly set up some Java tests for this and it would 
> be nice if we also find a bot that could actually run the tests for us.
>
>
>
>
> Comment at: lldb/bindings/java/CMakeLists.txt:3
> + * IP: Apache License 2.0 with LLVM Exceptions
> + */
>
> +add_custom_command(
> 
>
> I don't think CMake accepts this as a comment and I think we anyway don't put 
> license headers in CMake scripts.
>
>
>
>
> Comment at: lldb/source/API/CMakeLists.txt:84
>
>   SBTrace.cpp
>
> +  SBTraceOptions.cpp
>
>   SBType.cpp
>
> 
>
> I think this is some conflict with one of the SBTrace patches.
>
> Repository:
>
>   rLLDB LLDB
>
> CHANGES SINCE LAST ACTION
>
>   https://reviews.llvm.org/D111409/new/
>
> https://reviews.llvm.org/D111409




Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D111409

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

[Lldb-commits] [PATCH] D111409: proposed support for Java interface to Scripting Bridge

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D111409#3051110 , @d-millar wrote:

> Apologies for the inclusion of that last file "patch" - that is the "git diff 
> -U999" result, should that be useful.

You can just upload that diff file and Phabricator will display it properly. 
There is no need to include the raw diff as part of the patch itself (it just 
makes this diff 100 times larger than it needs to be) :)

Anyway, I think this seems like a reasonable thing to have. We have to figure 
out though how we can properly set up some Java tests for this and it would be 
nice if we also find a bot that could actually run the tests for us.




Comment at: lldb/bindings/java/CMakeLists.txt:3
+ * IP: Apache License 2.0 with LLVM Exceptions
+ */
+add_custom_command(

I don't think CMake accepts this as a comment and I think we anyway don't put 
license headers in CMake scripts.



Comment at: lldb/source/API/CMakeLists.txt:84
   SBTrace.cpp
+  SBTraceOptions.cpp
   SBType.cpp

I think this is some conflict with one of the SBTrace patches.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D111409

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

[Lldb-commits] [PATCH] D111402: [lldb] [test] Use secondary pty end for testing Terminal

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

If Green Dragon was still with us this would make it happy (and green), so LGTM.


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

https://reviews.llvm.org/D111402

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

[Lldb-commits] [PATCH] D111387: [NFC] [Clang] Remove pre-computed complex float types

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: JDevlieghere.

Would be nice to have the motivation for this in the commit message (IIUC this 
is based on the suggestion from D109948  ). 
Patch itself LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111387

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

[Lldb-commits] [PATCH] D110696: [lldb][import-std-module] Prefer the non-module diagnostics when in fallback mode

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6fcb857746c1: [lldb][import-std-module] Prefer the 
non-module diagnostics when in fallback… (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110696

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
  
lldb/test/API/commands/expression/import-std-module/module-build-errors/Makefile
  
lldb/test/API/commands/expression/import-std-module/module-build-errors/TestStdModuleBuildErrors.py
  
lldb/test/API/commands/expression/import-std-module/module-build-errors/main.cpp
  
lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/algorithm
  
lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/module.modulemap
  
lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/vector
  
lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/stdio.h
  
lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py

Index: lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
===
--- lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
+++ lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
@@ -48,16 +48,6 @@
 self.expect("expr --top-level -- int i = std::max(1, 2);", error=True,
 substrs=["no member named 'max' in namespace 'std'"])
 
-# Check that diagnostics from the first parse attempt don't show up
-# in the C++ module parse attempt. In the expression below, we first
-# fail to parse 'std::max'. Then we retry with a loaded C++ module
-# and succeed to parse the 'std::max' part. However, the
-# trailing 'unknown_identifier' will fail to parse even with the
-# loaded module. The 'std::max' diagnostic from the first attempt
-# however should not be shown to the user.
-self.expect("expr std::max(1, 2); unknown_identifier", error=True,
-matching=False,
-substrs=["no member named 'max'"])
 # The proper diagnostic however should be shown on the retry.
 self.expect("expr std::max(1, 2); unknown_identifier", error=True,
 substrs=["use of undeclared identifier 'unknown_identifier'"])
Index: lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/stdio.h
===
--- /dev/null
+++ lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/stdio.h
@@ -0,0 +1 @@
+struct libc_struct {};
Index: lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/module.modulemap
===
--- /dev/null
+++ lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/module.modulemap
@@ -0,0 +1,3 @@
+module std {
+  module "algorithm" { header "algorithm" export * }
+}
Index: lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/algorithm
===
--- /dev/null
+++ lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/algorithm
@@ -0,0 +1,18 @@
+// This is only defined when building, but LLDB is missing this flag when loading the standard
+// library module so the actual contents of the module are missing.
+#ifdef BUILDING_OUTSIDE_LLDB
+
+#include "stdio.h"
+
+namespace std {
+  inline namespace __1 {
+// Pretend to be a std::vector template we need to instantiate
+// in LLDB.
+template
+struct vector { T i; int size() { return 2; } };
+  }
+}
+#else
+// Break the build when parsing from within LLDB.
+random_token_to_fail_the_build
+#endif // BUILDING_OUTSIDE_LLDB
Index: lldb/test/API/commands/expression/import-std-module/module-build-errors/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/import-std-module/module-build-errors/main.cpp
@@ -0,0 +1,8 @@
+#include 
+
+int main(int argc, char **argv) {
+  // Makes sure we have the mock libc headers in the debug information.
+  libc_struct s;
+  std::vector v;
+  return 0; // Set break point at this line.
+}
Index: lldb/test/API/commands/expression/import-std-module/module-build-errors/TestStdModuleBuildErrors.py
===
--- /dev/null

[Lldb-commits] [PATCH] D111052: [lldb] [gdb-remote] Correct st_dev values in vFile:fstat packet

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

LGTM. Could you maybe add a `FIXME:` at the end of that comment to point out 
that the whole 'console' thing isn't implemented/supported.

Probably wait and see if Pavel has any objections against this, but I think 
this is more correct than the old implementation so I think this can land.


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

https://reviews.llvm.org/D111052

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

[Lldb-commits] [PATCH] D108228: Fix error handling in the libcxx std::string formatter

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

In D108228#2961132 , @jingham wrote:

> Raphael's analysis of what the test needs is right.  We always check pointers 
> for validity before we do operations on them, so we wouldn't have tried to 
> get the summary, and the expression evaluation will just crash if we do * of 
> a null ptr, so there wouldn't be anything to format.
>
> The reason I passed the string reference to a function and check it there is 
> because from many years of experience, I don't trust compilers even at -O0 
> not to elide references to silly unused variables.  But at -O0 a function 
> argument is never going away.
>
> But if folks think this is over paranoid on my part, I can simplify the test.

I don't have a strong objection against the extra step and so on, so LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108228

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

[Lldb-commits] [PATCH] D110827: [LLDB] Provide target specific directories to libclang

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

Thanks for the patch! Out of curiosity: What distribution is using 
target-specific include paths?

I can take a look at this tomorrow, but just from scrolling over I think the 
general direction of this patch seems fine, so I think I only have a few nits 
here and there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110827

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

[Lldb-commits] [PATCH] D110804: Add a new command "target metrics".

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.



Comment at: lldb/include/lldb/Core/Module.h:940
+  /// ElapsedTime
+  double () { return m_symtab_parse_time; }
+  double () { return m_symtab_names_time; }

labath wrote:
> teemperor wrote:
> > Could the `double` and `uint64_t` here/below be a typedef to some unit? 
> > `typedef double Seconds`/`typedef uint64_t ByteCount` or something along 
> > those lines. Would make the unit we're expecting more obvious and I can 
> > easily find all the related code.
> *cough* *cough* std::chrono *cough* *cough*
Yeah I didn't want to bring std::chrono in just yet but that would be the next 
logical step. FWIW, `ElapsedTime::Duration` already seems to be pretty much 
what we would want here, so we could just use that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110804

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

[Lldb-commits] [PATCH] D110804: Add a new command "target metrics".

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added subscribers: vsk, teemperor.
teemperor added a comment.

+Vedant who had/has some plans with the statistics command at some point IIRC.

FWIW, I wanted to throw away the current statistics implementation for quite a 
while but I didn't have anything to replace it with. If we can replace it with 
this then that would be nice.




Comment at: lldb/include/lldb/Core/Module.h:940
+  /// ElapsedTime
+  double () { return m_symtab_parse_time; }
+  double () { return m_symtab_names_time; }

Could the `double` and `uint64_t` here/below be a typedef to some unit? 
`typedef double Seconds`/`typedef uint64_t ByteCount` or something along those 
lines. Would make the unit we're expecting more obvious and I can easily find 
all the related code.



Comment at: lldb/test/API/commands/target/metrics/TestTargetMetrics.py:89
+self.verify_key_presence(metrics, '"metrics"', keys_exist, 
keys_missing)
+self.assertTrue(metrics['targetCreateTime'] >  0.0)
+self.assertTrue(metrics['totalBreakpointResolveTime'] == 0.0)

We stopped using `assertTrue` unless there is really no better `assert*` 
alternative as it doesn't produce any error diagnostics on failure. There's a 
good overview of the replacement methods 
[here](https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertAlmostEqual)
 which all print nice errors.



Comment at: lldb/test/API/commands/target/metrics/TestTargetMetrics.py:199
+"""
+exe = self.getBuildArtifact("a.out")
+self.runCmd("target create  " + exe, CURRENT_EXECUTABLE_SET)

This and the following line can be just `target = self.createTestTarget()` 
(here and in the rest of the test).



Comment at: lldb/test/API/commands/target/metrics/main.cpp:2
+extern int a_function();
+extern int b_function();
+

Unused function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110804

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

[Lldb-commits] [PATCH] D110693: [lldb] [Host] Remove TerminalStateSwitcher

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.

LGTM


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

https://reviews.llvm.org/D110693

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

[Lldb-commits] [PATCH] D110578: [lldb] Add support for D programming language

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D110578#3027707 , @ljmf00 wrote:

> In D110578#3027692 , @teemperor 
> wrote:
>
>> Do you have commit access or should someone land this for you?
>
> I don't have commit access, although this is a stacked revision, so there are 
> some dependent patches.

Alright, feel free to ping then if this is ready to land (or maybe ping the 
person for the dependent patch to land this too). Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110578

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