[Lldb-commits] [PATCH] D138724: [lldb][Target] Flush the scratch TypeSystem when process gets deleted

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.



Comment at: lldb/source/Core/ModuleList.cpp:1080
+  bool ret = true;
+  ForEach([&](const ModuleSP &module_sp) {
+ret &= callback(module_sp);

aprantl wrote:
> kastiglione wrote:
> > I wonder why ForEach doesn't deal out a `Module &`? I would think a 
> > ModuleList should not allow for null Module pointers.
> I think this is historic. +1 for taking a Module & (unless we for some reason 
> need a shared_ptr in the lambda).
I was wondering that myself. I took a stab at changing it to pass a reference 
but turns out there's several places where we end up storing the shared_ptr. If 
we changed this to a reference we'd have to call `shared_from_this` for those 
cases. Which is a bit iffy considering calling it on references that are not 
shared_ptr owned would throw (which can't happen currently but seems similarly 
awkward to the current API).

A compromise could be to document that these pointers are guaranteed to be 
non-null and add an assert into `ForEach`. AFAICT all call-sites assume the 
pointer is non-null anyway


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138724

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

[Lldb-commits] [PATCH] D138724: [lldb][Target] Flush the scratch TypeSystem when process gets deleted

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.



Comment at: lldb/source/Target/Target.cpp:1686
+const bool should_flush_type_systems =
+module_list.AllOf([](const lldb::ModuleSP &module_sp) {
+  if (!module_sp)

kastiglione wrote:
> How come this is `AllOf` and not a `AnyOf`?
Here I'm checking whether all unloaded modules are regular executables or 
object files. Otherwise we wouldn't want to clear the TypeSystems. E.g., for 
JITted modules this would not be desirable. I'll add a comment about this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138724

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

[Lldb-commits] [PATCH] D138724: [lldb][Target] Flush the scratch TypeSystem when process gets deleted

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.



Comment at: lldb/source/Target/Target.cpp:1686
+const bool should_flush_type_systems =
+module_list.AllOf([](const lldb::ModuleSP &module_sp) {
+  if (!module_sp)

Michael137 wrote:
> kastiglione wrote:
> > How come this is `AllOf` and not a `AnyOf`?
> Here I'm checking whether all unloaded modules are regular executables or 
> object files. Otherwise we wouldn't want to clear the TypeSystems. E.g., for 
> JITted modules this would not be desirable. I'll add a comment about this
Though perhaps `AnyOf` would be the safer thing to do. Not sure when we'd have 
a mixture of object-files in here. But I suppose if we unloaded any that 
could've been a source AST then we're in unsafe territory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138724

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

[Lldb-commits] [PATCH] D138724: [lldb][Target] Flush the scratch TypeSystem when process gets deleted

[Michael Buch via Phabricator via lldb-commits]

Michael137 updated this revision to Diff 479190.
Michael137 added a comment.

- Reflow comment
- Remove redundant null-check


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138724

Files:
  lldb/include/lldb/Core/ModuleList.h
  lldb/source/Core/ModuleList.cpp
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/rerun_and_expr/Makefile
  lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py
  lldb/test/API/functionalities/rerun_and_expr/main.cpp
  lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp

Index: lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp
@@ -0,0 +1,12 @@
+struct Base {
+  int m_base_val = 42;
+};
+
+struct Foo : public Base {
+  int m_derived_val = 137;
+};
+
+int main() {
+  Foo foo;
+  return 0;
+}
Index: lldb/test/API/functionalities/rerun_and_expr/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/rerun_and_expr/main.cpp
@@ -0,0 +1,8 @@
+struct Foo {
+  int m_val = 42;
+};
+
+int main() {
+  Foo foo;
+  return 0;
+}
Index: lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py
===
--- /dev/null
+++ lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py
@@ -0,0 +1,71 @@
+"""
+Test that re-running a process from within the same target
+after rebuilding the executable flushes the scratch TypeSystems
+tied to that process.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestRerun(TestBase):
+def test(self):
+"""
+Tests whether re-launching a process without destroying
+the owning target keeps invalid ASTContexts in the
+scratch AST's importer.
+
+We test this by:
+1. Evaluating an expression to import 'struct Foo' into
+   the scratch AST
+2. Change the definition of 'struct Foo' and rebuild the executable
+3. Re-launch the process
+4. Evaluate the same expression in (1). We expect to have only
+   the latest definition of 'struct Foo' in the scratch AST.
+"""
+self.build(dictionary={'CXX_SOURCES':'main.cpp', 'EXE':'a.out'})
+(target, _, _, bkpt) = \
+lldbutil.run_to_source_breakpoint(self, 'return', lldb.SBFileSpec('main.cpp'))
+
+target.BreakpointCreateBySourceRegex('return', lldb.SBFileSpec('rebuild.cpp', False))
+
+self.expect_expr('foo', result_type='Foo', result_children=[
+ValueCheck(name='m_val', value='42')
+])
+
+self.build(dictionary={'CXX_SOURCES':'rebuild.cpp', 'EXE':'a.out'})
+
+self.runCmd('process launch')
+
+self.expect_expr('foo', result_type='Foo', result_children=[
+ValueCheck(name='Base', children=[
+ValueCheck(name='m_base_val', value='42')
+]),
+ValueCheck(name='m_derived_val', value='137')
+])
+
+self.filecheck("target module dump ast", __file__)
+
+# The new definition 'struct Foo' is in the scratch AST
+# CHECK:  |-CXXRecordDecl {{.*}} struct Foo definition
+# CHECK-NEXT: | |-DefinitionData pass_in_registers standard_layout trivially_copyable trivial literal
+# CHECK-NEXT: | | |-DefaultConstructor exists trivial needs_implicit
+# CHECK-NEXT: | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
+# CHECK-NEXT: | | |-MoveConstructor exists simple trivial needs_implicit
+# CHECK-NEXT: | | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
+# CHECK-NEXT: | | |-MoveAssignment exists simple trivial needs_implicit
+# CHECK-NEXT: | | `-Destructor simple irrelevant trivial needs_implicit
+# CHECK-NEXT: | |-public 'Base'
+# CHECK-NEXT: | `-FieldDecl {{.*}} m_derived_val 'int'
+# CHECK-NEXT: `-CXXRecordDecl {{.*}} struct Base definition
+# CHECK-NEXT:   |-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
+# CHECK-NEXT:   | |-DefaultConstructor exists trivial needs_implicit
+# CHECK-NEXT:   | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
+# CHECK-NEXT:   | |-MoveConstructor exists simple trivial needs_implicit
+# CHECK-NEXT:   | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
+# CHECK-NEXT:   | |-MoveAssignment exists simple trivial needs_implicit
+# CHECK-NEXT:   | `-Destructor simple irrelevant trivial needs_implicit
+
+# ...but the original definition of 'struct Foo' is not in 

[Lldb-commits] [PATCH] D139082: [lldb][Module] Document ModuleList::ForEach and assert nullness

[Michael Buch via Phabricator via lldb-commits]

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

Currently all callsites already assume the pointer is non-null.
This patch just asserts this assumption.

This is practically enforced by `ModuleList::Append`
which won't add `nullptr`s to `m_modules`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139082

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
@@ -1067,9 +1067,10 @@
 void ModuleList::ForEach(
 std::function const &callback) const {
   std::lock_guard guard(m_modules_mutex);
-  for (const auto &module : m_modules) {
+  for (const auto &module_sp : m_modules) {
+assert(module_sp != nullptr);
 // If the callback returns false, then stop iterating and break out
-if (!callback(module))
+if (!callback(module_sp))
   break;
   }
 }
Index: lldb/include/lldb/Core/ModuleList.h
===
--- lldb/include/lldb/Core/ModuleList.h
+++ lldb/include/lldb/Core/ModuleList.h
@@ -464,6 +464,12 @@
 
   static bool RemoveSharedModuleIfOrphaned(const Module *module_ptr);
 
+  /// Applies 'callback' to each module in this ModuleList.
+  /// If 'callback' returns false, iteration terminates.
+  /// The 'module_sp' passed to 'callback' is guaranteed to
+  /// be non-null.
+  ///
+  /// This function is thread-safe.
   void ForEach(std::function const
&callback) const;
 


Index: lldb/source/Core/ModuleList.cpp
===
--- lldb/source/Core/ModuleList.cpp
+++ lldb/source/Core/ModuleList.cpp
@@ -1067,9 +1067,10 @@
 void ModuleList::ForEach(
 std::function const &callback) const {
   std::lock_guard guard(m_modules_mutex);
-  for (const auto &module : m_modules) {
+  for (const auto &module_sp : m_modules) {
+assert(module_sp != nullptr);
 // If the callback returns false, then stop iterating and break out
-if (!callback(module))
+if (!callback(module_sp))
   break;
   }
 }
Index: lldb/include/lldb/Core/ModuleList.h
===
--- lldb/include/lldb/Core/ModuleList.h
+++ lldb/include/lldb/Core/ModuleList.h
@@ -464,6 +464,12 @@
 
   static bool RemoveSharedModuleIfOrphaned(const Module *module_ptr);
 
+  /// Applies 'callback' to each module in this ModuleList.
+  /// If 'callback' returns false, iteration terminates.
+  /// The 'module_sp' passed to 'callback' is guaranteed to
+  /// be non-null.
+  ///
+  /// This function is thread-safe.
   void ForEach(std::function const
&callback) const;
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D139083: [lldb][Module][NFC] Add ModuleList::AnyOf

[Michael Buch via Phabricator via lldb-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139083

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
@@ -1074,3 +1074,15 @@
   break;
   }
 }
+
+bool ModuleList::AnyOf(
+std::function const &callback) const {
+  std::lock_guard guard(m_modules_mutex);
+  for (const auto &module_sp : m_modules) {
+assert(module_sp != nullptr);
+if (callback(module_sp))
+  return true;
+  }
+
+  return false;
+}
Index: lldb/include/lldb/Core/ModuleList.h
===
--- lldb/include/lldb/Core/ModuleList.h
+++ lldb/include/lldb/Core/ModuleList.h
@@ -473,6 +473,13 @@
   void ForEach(std::function const
&callback) const;
 
+  /// Returns true if 'callback' returns true for one of the modules
+  /// in this ModuleList.
+  ///
+  /// This function is thread-safe.
+  bool AnyOf(std::function const
+ &callback) const;
+
 protected:
   // Class typedefs.
   typedef std::vector


Index: lldb/source/Core/ModuleList.cpp
===
--- lldb/source/Core/ModuleList.cpp
+++ lldb/source/Core/ModuleList.cpp
@@ -1074,3 +1074,15 @@
   break;
   }
 }
+
+bool ModuleList::AnyOf(
+std::function const &callback) const {
+  std::lock_guard guard(m_modules_mutex);
+  for (const auto &module_sp : m_modules) {
+assert(module_sp != nullptr);
+if (callback(module_sp))
+  return true;
+  }
+
+  return false;
+}
Index: lldb/include/lldb/Core/ModuleList.h
===
--- lldb/include/lldb/Core/ModuleList.h
+++ lldb/include/lldb/Core/ModuleList.h
@@ -473,6 +473,13 @@
   void ForEach(std::function const
&callback) const;
 
+  /// Returns true if 'callback' returns true for one of the modules
+  /// in this ModuleList.
+  ///
+  /// This function is thread-safe.
+  bool AnyOf(std::function const
+ &callback) const;
+
 protected:
   // Class typedefs.
   typedef std::vector
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138724: [lldb][Target] Flush the scratch TypeSystem when process gets deleted

[Michael Buch via Phabricator via lldb-commits]

Michael137 updated this revision to Diff 479196.
Michael137 added a comment.

- Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138724

Files:
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/rerun_and_expr/Makefile
  lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py
  lldb/test/API/functionalities/rerun_and_expr/main.cpp
  lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp

Index: lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp
@@ -0,0 +1,12 @@
+struct Base {
+  int m_base_val = 42;
+};
+
+struct Foo : public Base {
+  int m_derived_val = 137;
+};
+
+int main() {
+  Foo foo;
+  return 0;
+}
Index: lldb/test/API/functionalities/rerun_and_expr/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/rerun_and_expr/main.cpp
@@ -0,0 +1,8 @@
+struct Foo {
+  int m_val = 42;
+};
+
+int main() {
+  Foo foo;
+  return 0;
+}
Index: lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py
===
--- /dev/null
+++ lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py
@@ -0,0 +1,71 @@
+"""
+Test that re-running a process from within the same target
+after rebuilding the executable flushes the scratch TypeSystems
+tied to that process.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestRerun(TestBase):
+def test(self):
+"""
+Tests whether re-launching a process without destroying
+the owning target keeps invalid ASTContexts in the
+scratch AST's importer.
+
+We test this by:
+1. Evaluating an expression to import 'struct Foo' into
+   the scratch AST
+2. Change the definition of 'struct Foo' and rebuild the executable
+3. Re-launch the process
+4. Evaluate the same expression in (1). We expect to have only
+   the latest definition of 'struct Foo' in the scratch AST.
+"""
+self.build(dictionary={'CXX_SOURCES':'main.cpp', 'EXE':'a.out'})
+(target, _, _, bkpt) = \
+lldbutil.run_to_source_breakpoint(self, 'return', lldb.SBFileSpec('main.cpp'))
+
+target.BreakpointCreateBySourceRegex('return', lldb.SBFileSpec('rebuild.cpp', False))
+
+self.expect_expr('foo', result_type='Foo', result_children=[
+ValueCheck(name='m_val', value='42')
+])
+
+self.build(dictionary={'CXX_SOURCES':'rebuild.cpp', 'EXE':'a.out'})
+
+self.runCmd('process launch')
+
+self.expect_expr('foo', result_type='Foo', result_children=[
+ValueCheck(name='Base', children=[
+ValueCheck(name='m_base_val', value='42')
+]),
+ValueCheck(name='m_derived_val', value='137')
+])
+
+self.filecheck("target module dump ast", __file__)
+
+# The new definition 'struct Foo' is in the scratch AST
+# CHECK:  |-CXXRecordDecl {{.*}} struct Foo definition
+# CHECK-NEXT: | |-DefinitionData pass_in_registers standard_layout trivially_copyable trivial literal
+# CHECK-NEXT: | | |-DefaultConstructor exists trivial needs_implicit
+# CHECK-NEXT: | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
+# CHECK-NEXT: | | |-MoveConstructor exists simple trivial needs_implicit
+# CHECK-NEXT: | | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
+# CHECK-NEXT: | | |-MoveAssignment exists simple trivial needs_implicit
+# CHECK-NEXT: | | `-Destructor simple irrelevant trivial needs_implicit
+# CHECK-NEXT: | |-public 'Base'
+# CHECK-NEXT: | `-FieldDecl {{.*}} m_derived_val 'int'
+# CHECK-NEXT: `-CXXRecordDecl {{.*}} struct Base definition
+# CHECK-NEXT:   |-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
+# CHECK-NEXT:   | |-DefaultConstructor exists trivial needs_implicit
+# CHECK-NEXT:   | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
+# CHECK-NEXT:   | |-MoveConstructor exists simple trivial needs_implicit
+# CHECK-NEXT:   | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
+# CHECK-NEXT:   | |-MoveAssignment exists simple trivial needs_implicit
+# CHECK-NEXT:   | `-Destructor simple irrelevant trivial needs_implicit
+
+# ...but the original definition of 'struct Foo' is not in the scratch AST anymore
+# CHECK-NOT: FieldDecl {{.*}} m_val 'int'
+
Index: lldb/test/API/functionalit

[Lldb-commits] [PATCH] D139083: [lldb][Module][NFC] Add ModuleList::AnyOf

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

I suppose I could just make ForEach return a boolean instead of breaking. Could 
then re-use that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139083

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

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

In D138939#3958185 , @cjdb wrote:

>> The clang-side interface to this seems a touch clunky, and I fear it'll make 
>> continuing its use/generalizing its use less likely.
>
> Is this w.r.t. the `FormatDiagnostic` being split up, or is it something 
> else? If it's the former: I'll be changing that to `FormatLegacyDiagnostic`, 
> which //almost// gets us back to where we started.

Urgh, I was a bit afraid you'd ask that :D  It is more of a feeling I guess, 
which is perhaps biased by this patch being particularly in the 
diagnostics-handling code.  However, I suspect that over time, you're going to 
want to start switching all these uses of `FormatLegacyReason` over to 
`FormatSarifReason`, and I would want the 'easy way' to be the 'right' way in 
either case.  Having it be a 2-liner, or over-specifying what is otherwise the 
'default' behavior is a bit disconcerting.

In D138939#3958231 , @cjdb wrote:

>> though I find myself wondering if the "FormatDiagnostic" call should stay 
>> the same, and choose the legacy-reason only when a sarif reason doesn't 
>> exist? Or for some level of command line control?
>
> Hmm... isn't this the point of the diagnostic consumers?

I don't have a great feeling of that?  I haven't done much thinking into the 
diagnostics architecture over the years.  That said, the use of when we'd 
choose one vs the other isn't completely clear to me yet.




Comment at: clang/lib/Basic/Diagnostic.cpp:791
 
 /// FormatDiagnostic - Format this diagnostic into a string, substituting the
 /// formal arguments into the %0 slots.  The result is appended onto the Str

Comment no longer matches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939

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

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[Thorsten via Phabricator via lldb-commits]

tschuett added a comment.

Maybe `void FormatDiagnostic(SmallVectorImpl &OutStr, DiagnosticMode 
mode)`instead of `void FormatDiagnostic(SmallVectorImpl &OutStr)`?
To make the transition easer and future proof.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939

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

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

In D138939#3963473 , @tschuett wrote:

> Maybe `void FormatDiagnostic(SmallVectorImpl &OutStr, DiagnosticMode 
> mode)`instead of `void FormatDiagnostic(SmallVectorImpl &OutStr)`?
> To make the transition easer and future proof.

I like this idea.  Perhaps with DiagnosticMode being a 3-way enum:

  enum struct DiagnosticMode {
Legacy,
Sarif,  
Default = Legacy
  }

I like the idea in particular, since it makes a command line flag to modify 
"Default" to be whichever the user prefers pretty trivial.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939

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

[Lldb-commits] [PATCH] D138558: [lldb][DataFormatter] Add std::ranges::ref_view formatter

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

You may want to check that this kind of automatic dereferencing does not send 
lldb into a tailspin if the printed data structure is recursive. I know we had 
problems like that with smart pointer pretty printers.

I'd try some code like:

  #include 
  #include 
  
  struct A {
std::ranges::ref_view> a;
  };
  
  int main() {
  std::vector v;
  v.push_back(A{v});
  v[0].a = v;
  // print v ?
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138558

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

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The explanation makes sense, and I *think* the patch is ok, but it's hard to 
review it with all the noise. I still believe the DIERef change would be better 
off as a separate patch, so that the change is not obscured by the (hopefully 
mechanical) aspects of increasing the size of the offset field.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138618

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

[Lldb-commits] [PATCH] D137900: Make only one function that needs to be implemented when searching for types.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/include/lldb/Core/Module.h:435
+  ///   match: "b::a", "c::b::a", "d::b::a", "e::f::b::a".
+  lldb::TypeSP FindFirstType(ConstString type_name, bool exact_match);
 

clayborg wrote:
> aprantl wrote:
> > Why is this not taking a TypeQuery that wraps a name?
> This function doesn't need to exist as long as the "TypeSP 
> TypeQuery::FindFirstType(Module)" function is ok to have around.
It would be good to have some consistency in the interface:
- Either TypeQuery provides Find* methods that take a Module 
- or Module provides Find* methods that take a TypeQuery.

Why does this interface exist instead of having a "FindFirstOnly" flag in 
TypeQuery? Is it because of the overhead of having to dig the type out of a 
TypeMap?

Otherwise we could just have one method

Module::FindTypes(const TypeQuery &query, TypeResults &results);

that can be called like this
```
module_sp->FindTypes(TypeQuery::createFindFirstByName("name", e_exact_match), 
result);
TypeSP type = result.GetTypeAtIndex(0); /// better have a special method for 
this too
```



Comment at: lldb/include/lldb/Symbol/Type.h:296
+
+class TypeResults {
+public:

`/// This is the result of a \ref TypeQuery.`



Comment at: lldb/include/lldb/Symbol/Type.h:299
+  /// Construct a type results object
+  TypeResults(bool find_one) : m_find_one(find_one) {}
+

document what the bool flag does?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137900

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

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

[Alexander Yermolovich via Phabricator via lldb-commits]

ayermolo added a comment.

In D138618#3963767 , @labath wrote:

> The explanation makes sense, and I *think* the patch is ok, but it's hard to 
> review it with all the noise. I still believe the DIERef change would be 
> better off as a separate patch, so that the change is not obscured by the 
> (hopefully mechanical) aspects of increasing the size of the offset field.

I don't think that would be mechanical, because of implicit and explicit 
assumptions of bit layout, and trying to keep the size of DIERef from doubling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138618

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

[Lldb-commits] [lldb] e2a10d8 - [lldb] Remove timer from Module::GetNumCompileUnits

[Dave Lee via lldb-commits]

Author: Dave Lee
Date: 2022-12-01T09:41:51-08:00
New Revision: e2a10d8ca34a3554d8d19d2bbdd3133970e4d09b

URL: 
https://github.com/llvm/llvm-project/commit/e2a10d8ca34a3554d8d19d2bbdd3133970e4d09b
DIFF: 
https://github.com/llvm/llvm-project/commit/e2a10d8ca34a3554d8d19d2bbdd3133970e4d09b.diff

LOG: [lldb] Remove timer from Module::GetNumCompileUnits

`GetNumCompileUnits` has fast execution, and is high firing. Fast and frequent 
functions are not good candidates for timers. In a recent profile, 
`GetNumCompileUnits` was called >>10k times with an average duration of 1 
microsecond.

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

Added: 


Modified: 
lldb/source/Core/Module.cpp

Removed: 




diff  --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index b1cc0dd58211d..dd2569eb81f8e 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -419,8 +419,6 @@ void Module::DumpSymbolContext(Stream *s) {
 
 size_t Module::GetNumCompileUnits() {
   std::lock_guard guard(m_mutex);
-  LLDB_SCOPED_TIMERF("Module::GetNumCompileUnits (module = %p)",
- static_cast(this));
   if (SymbolFile *symbols = GetSymbolFile())
 return symbols->GetNumCompileUnits();
   return 0;



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

[Lldb-commits] [PATCH] D138878: [lldb] Remove timer from Module::GetNumCompileUnits

[Dave Lee via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe2a10d8ca34a: [lldb] Remove timer from 
Module::GetNumCompileUnits (authored by kastiglione).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138878

Files:
  lldb/source/Core/Module.cpp


Index: lldb/source/Core/Module.cpp
===
--- lldb/source/Core/Module.cpp
+++ lldb/source/Core/Module.cpp
@@ -419,8 +419,6 @@
 
 size_t Module::GetNumCompileUnits() {
   std::lock_guard guard(m_mutex);
-  LLDB_SCOPED_TIMERF("Module::GetNumCompileUnits (module = %p)",
- static_cast(this));
   if (SymbolFile *symbols = GetSymbolFile())
 return symbols->GetNumCompileUnits();
   return 0;


Index: lldb/source/Core/Module.cpp
===
--- lldb/source/Core/Module.cpp
+++ lldb/source/Core/Module.cpp
@@ -419,8 +419,6 @@
 
 size_t Module::GetNumCompileUnits() {
   std::lock_guard guard(m_mutex);
-  LLDB_SCOPED_TIMERF("Module::GetNumCompileUnits (module = %p)",
- static_cast(this));
   if (SymbolFile *symbols = GetSymbolFile())
 return symbols->GetNumCompileUnits();
   return 0;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D139066: [lldb] Make sure the value of `eSymbolContextVariable` is not conflicting with `RESOLVED_FRAME_CODE_ADDR`

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I'm not as sure about this.  I think the idea was that eSymbolContextEverything 
would have a value like 0b and so we know only 4 bits (in my example here) 
are used for the eSymbolContext* enums.  StackFrame's m_flags wants to use the 
bits above that range for RESOLVED_FRAME_CODE_ADDR, 
RESOLVED_FRAME_ID_SYMBOL_SCOPE, GOT_FRAME_BASE, RESOLVED_VARIABLES, 
RESOLVED_GLOBAL_VARIABLES flags.  So it's setting RESOLVED_FRAME_CODE_ADDR to 
(in my example) 0b1, leaving those 4 low bits free to indicate the 
eSymbolContext values.  That is, we would have

0b start of eSymbolContext range
0b end of eSymbolContext range
0b0001 RESOLVED_FRAME_CODE_ADDR
0b0010 RESOLVED_FRAME_ID_SYMBOL_SCOPE
0b0011 GOT_FRAME_BASE
0b0100 RESOLVED_VARIABLES
0b0101 RESOLVED_GLOBAL_VARIABLES

The idea is to have two unrelated set of flags in this single m_flags which I 
am suspicious of how necessary that is, tbh.  In a typical big multi threaded 
GUI app you might have 20-30 threads with 40-50 stack frames, that's like 1500 
StackFrames.  I think we keep the stack frame list from the previous step when 
stepping, so twice that.  Having an extra uint32_t for 3k StackFrame objects 
does not concern me.

I agree that having eSymbolContextVariable be outside the range of 
eSymbolContextEverything breaks this scheme, but I don't think this is the 
correct fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139066

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

[Lldb-commits] [PATCH] D139066: [lldb] Make sure the value of `eSymbolContextVariable` is not conflicting with `RESOLVED_FRAME_CODE_ADDR`

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

a simple fix would be to stick the 5 extra flags the top of the 32-bit m_flags 
and hope that eSymbolContext* doesn't go that high.  I wonder if there's some 
compile time error that could check that eSymbolContextEverything is less than 
a certain value or something like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139066

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

[Lldb-commits] [PATCH] D139066: [lldb] Make sure the value of `eSymbolContextVariable` is not conflicting with `RESOLVED_FRAME_CODE_ADDR`

[Adrian Prantl via Phabricator via lldb-commits]

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

I'll defer to @jasonmolenda


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139066

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

[Lldb-commits] [PATCH] D139066: [lldb] Make sure the value of `eSymbolContextVariable` is not conflicting with `RESOLVED_FRAME_CODE_ADDR`

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

FWIW I think the only change needed to the original patch is to keep using 
`#define RESOLVED_FRAME_CODE_ADDR (uint32_t(eSymbolContextEverything + 1))` but 
switch to the new eSymbolContextLastItem that is defined.  Possibly the comment 
about what this code is doing could be made a little clearer, because it is 
definitely not. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139066

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

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[Christopher Di Bella via Phabricator via lldb-commits]

cjdb marked 3 inline comments as done.
cjdb added a comment.

In D138939#3963496 , @erichkeane 
wrote:

> In D138939#3963473 , @tschuett 
> wrote:
>
>> Maybe `void FormatDiagnostic(SmallVectorImpl &OutStr, DiagnosticMode 
>> mode)`instead of `void FormatDiagnostic(SmallVectorImpl &OutStr)`?
>> To make the transition easer and future proof.
>
> I like this idea.  Perhaps with DiagnosticMode being a 3-way enum:
>
>   enum struct DiagnosticMode {
> Legacy,
> Sarif,  
> Default = Legacy
>   }
>
> I like the idea in particular, since it makes a command line flag to modify 
> "Default" to be whichever the user prefers pretty trivial.

There's already a flag for this: `-fdiagnostics-format=sarif`. Why do we need a 
second diagnostic mode flag?




Comment at: clang/tools/clang-format/ClangFormat.cpp:397
+  Info.FormatSummary(vec);
+  Info.FormatLegacyReason(vec);
 errs() << "clang-format error:" << vec << "\n";

rymiel wrote:
> I don't think this indent change was intended?
I find it ironic that clang-format misformatted ClangFormat.cpp.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939

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

[Lldb-commits] [PATCH] D139066: [lldb] Make sure the value of `eSymbolContextVariable` is not conflicting with `RESOLVED_FRAME_CODE_ADDR`

[Argyrios Kyrtzidis via Phabricator via lldb-commits]

akyrtzi added a comment.

In D139066#3964353 , @jasonmolenda 
wrote:

> FWIW I think the only change needed to the original patch is to keep using 
> `#define RESOLVED_FRAME_CODE_ADDR (uint32_t(eSymbolContextEverything + 1))` 
> but switch to the new eSymbolContextLastItem that is defined.  Possibly the 
> comment about what this code is doing could be made a little clearer, because 
> it is definitely not. :)

Thank you for the feedback, but it's unclear to me what you mean, are you 
saying leave the `#define RESOLVED_FRAME_CODE_ADDR` line unmodified? If yes, 
what does "switch to the new eSymbolContextLastItem" mean?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139066

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

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

In D138939#3964404 , @cjdb wrote:

> In D138939#3963496 , @erichkeane 
> wrote:
>
>> In D138939#3963473 , @tschuett 
>> wrote:
>>
>>> Maybe `void FormatDiagnostic(SmallVectorImpl &OutStr, DiagnosticMode 
>>> mode)`instead of `void FormatDiagnostic(SmallVectorImpl &OutStr)`?
>>> To make the transition easer and future proof.
>>
>> I like this idea.  Perhaps with DiagnosticMode being a 3-way enum:
>>
>>   enum struct DiagnosticMode {
>> Legacy,
>> Sarif,  
>> Default = Legacy
>>   }
>>
>> I like the idea in particular, since it makes a command line flag to modify 
>> "Default" to be whichever the user prefers pretty trivial.
>
> There's already a flag for this: `-fdiagnostics-format=sarif`. Why do we need 
> a second diagnostic mode flag?

Ah, oh... is the Sarif formatting being done with a new formatter?  That seems 
unfortunate, since folks using the other formatters won't be able to use the 
user friendly formats.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939

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

[Lldb-commits] [PATCH] D138724: [lldb][Target] Flush the scratch TypeSystem when process gets deleted

[Adrian Prantl via Phabricator via lldb-commits]

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

LGTM with a slightly more relaxed test




Comment at: lldb/source/Target/Target.cpp:1707
+
+if (should_flush_type_systems) {
+  m_scratch_type_system_map.Clear();

nit: no {} in LLVM style



Comment at: lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py:47
+
+self.filecheck("target module dump ast", __file__)
+

This is really great — I didn't know we already had this!



Comment at: lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py:51
+# CHECK:  |-CXXRecordDecl {{.*}} struct Foo definition
+# CHECK-NEXT: | |-DefinitionData pass_in_registers standard_layout 
trivially_copyable trivial literal
+# CHECK-NEXT: | | |-DefaultConstructor exists trivial needs_implicit

This may be checking too much detail. I don't know how frequently the Clang AST 
changes, but if there is any information we don't really need I would try to 
omit it so we don't have to update this test every couple of months.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138724

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

[Lldb-commits] [PATCH] D139083: [lldb][Module][NFC] Add ModuleList::AnyOf

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

This is definitely useful — I just have one question: Isn't ForEach a special 
case of AnyOf? Could we implement on in terms of the other?




Comment at: lldb/include/lldb/Core/ModuleList.h:480
+  /// This function is thread-safe.
+  bool AnyOf(std::function const
+ &callback) const;

Why not `std::function` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139083

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

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[Erich Keane via Phabricator via lldb-commits]

erichkeane added a comment.

In D138939#3964439 , @erichkeane 
wrote:

> In D138939#3964404 , @cjdb wrote:
>
>> In D138939#3963496 , @erichkeane 
>> wrote:
>>
>>> In D138939#3963473 , @tschuett 
>>> wrote:
>>>
 Maybe `void FormatDiagnostic(SmallVectorImpl &OutStr, DiagnosticMode 
 mode)`instead of `void FormatDiagnostic(SmallVectorImpl &OutStr)`?
 To make the transition easer and future proof.
>>>
>>> I like this idea.  Perhaps with DiagnosticMode being a 3-way enum:
>>>
>>>   enum struct DiagnosticMode {
>>> Legacy,
>>> Sarif,  
>>> Default = Legacy
>>>   }
>>>
>>> I like the idea in particular, since it makes a command line flag to modify 
>>> "Default" to be whichever the user prefers pretty trivial.
>>
>> There's already a flag for this: `-fdiagnostics-format=sarif`. Why do we 
>> need a second diagnostic mode flag?
>
> Ah, oh... is the Sarif formatting being done with a new formatter?  That 
> seems unfortunate, since folks using the other formatters won't be able to 
> use the user friendly formats.

I've been alerted offline that I am misunderstanding the Sarif proposal, and 
where this is going.  I'll note that I wasn't present/invited at the calls 
where all of this was discussed, so I am admittedly not completely up to date.  
The above concern shouldn't stop others from reviewing this, particularly if 
you better understand the intent here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939

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

[Lldb-commits] [PATCH] D139083: [lldb][Module][NFC] Add ModuleList::AnyOf

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.



Comment at: lldb/include/lldb/Core/ModuleList.h:480
+  /// This function is thread-safe.
+  bool AnyOf(std::function const
+ &callback) const;

aprantl wrote:
> Why not `std::function` ?
Unfortunately a lot of APIs of `Module` are non-const since they rely on 
updating internal state :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139083

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

[Lldb-commits] [PATCH] D139083: [lldb][Module][NFC] Add ModuleList::AnyOf

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

In D139083#3964612 , @aprantl wrote:

> This is definitely useful — I just have one question: Isn't ForEach a special 
> case of AnyOf? Could we implement on in terms of the other?

Yup it is. But it looked a bit too clever and just iterating over `m_modules` 
seemed cleaner.

  bool ret = false;
  ForEach([&](auto const& module) {
  if (callback(module)) {
  ret = true;
  return false;
  }
  
  return true;
  }
  
  return ret;

It looked a bit awkward




Comment at: lldb/include/lldb/Core/ModuleList.h:480
+  /// This function is thread-safe.
+  bool AnyOf(std::function const
+ &callback) const;

Michael137 wrote:
> aprantl wrote:
> > Why not `std::function` ?
> Unfortunately a lot of APIs of `Module` are non-const since they rely on 
> updating internal state :(
Re. why shared_ptr, that's because I just waned to align it with `ForEach`. I 
suppose if we're starting fresh we could just pass the reference. It would have 
to be mutable though


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139083

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

[Lldb-commits] [lldb] cebb87e - [lldb] Enable use of dummy target from dwim-print

[Dave Lee via lldb-commits]

Author: Dave Lee
Date: 2022-12-01T13:21:24-08:00
New Revision: cebb87e7dce891b9a70ccb28e7da8c1fc71f2439

URL: 
https://github.com/llvm/llvm-project/commit/cebb87e7dce891b9a70ccb28e7da8c1fc71f2439
DIFF: 
https://github.com/llvm/llvm-project/commit/cebb87e7dce891b9a70ccb28e7da8c1fc71f2439.diff

LOG: [lldb] Enable use of dummy target from dwim-print

Allow `dwim-print` to evaluate expressions using the dummy target if no real
target exists.

This adds some parity to `expression`. With this, both of the following work:

```
lldb -o 'expr 1+2'
lldb -o 'dwim-print 1+2'
```

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

Added: 


Modified: 
lldb/source/Commands/CommandObjectDWIMPrint.cpp
lldb/test/API/commands/dwim-print/TestDWIMPrint.py

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectDWIMPrint.cpp 
b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
index 232e877f53d76..e15e723de5880 100644
--- a/lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -22,12 +22,11 @@ using namespace lldb;
 using namespace lldb_private;
 
 CommandObjectDWIMPrint::CommandObjectDWIMPrint(CommandInterpreter &interpreter)
-: CommandObjectRaw(
-  interpreter, "dwim-print", "Print a variable or expression.",
-  "dwim-print [ | ]",
-  eCommandProcessMustBePaused | eCommandTryTargetAPILock |
-  eCommandRequiresFrame | eCommandProcessMustBeLaunched |
-  eCommandRequiresProcess) {}
+: CommandObjectRaw(interpreter, "dwim-print",
+   "Print a variable or expression.",
+   "dwim-print [ | ]",
+   eCommandProcessMustBePaused | eCommandTryTargetAPILock) 
{
+}
 
 bool CommandObjectDWIMPrint::DoExecute(StringRef expr,
CommandReturnObject &result) {
@@ -40,14 +39,10 @@ bool CommandObjectDWIMPrint::DoExecute(StringRef expr,
 return false;
   }
 
-  // eCommandRequiresFrame guarantees a frame.
-  StackFrame *frame = m_exe_ctx.GetFramePtr();
-  assert(frame);
-
   auto verbosity = GetDebugger().GetDWIMPrintVerbosity();
 
-  // First, try `expr` as the name of a variable.
-  {
+  // First, try `expr` as the name of a frame variable.
+  if (StackFrame *frame = m_exe_ctx.GetFramePtr()) {
 auto valobj_sp = frame->FindVariable(ConstString(expr));
 if (valobj_sp && valobj_sp->GetError().Success()) {
   if (verbosity == eDWIMPrintVerbosityFull)
@@ -60,12 +55,13 @@ bool CommandObjectDWIMPrint::DoExecute(StringRef expr,
 
   // Second, also lastly, try `expr` as a source expression to evaluate.
   {
-// eCommandRequiresProcess guarantees a target.
-Target *target = m_exe_ctx.GetTargetPtr();
-assert(target);
+Target *target_ptr = m_exe_ctx.GetTargetPtr();
+// Fallback to the dummy target, which can allow for expression evaluation.
+Target &target = target_ptr ? *target_ptr : GetDummyTarget();
 
+auto *exe_scope = m_exe_ctx.GetBestExecutionContextScope();
 ValueObjectSP valobj_sp;
-if (target->EvaluateExpression(expr, frame, valobj_sp) ==
+if (target.EvaluateExpression(expr, exe_scope, valobj_sp) ==
 eExpressionCompleted) {
   if (verbosity != eDWIMPrintVerbosityNone)
 result.AppendMessageWithFormatv("note: ran `expression -- {0}`", expr);

diff  --git a/lldb/test/API/commands/dwim-print/TestDWIMPrint.py 
b/lldb/test/API/commands/dwim-print/TestDWIMPrint.py
index 7bc1448441cc9..6b3266deaad86 100644
--- a/lldb/test/API/commands/dwim-print/TestDWIMPrint.py
+++ b/lldb/test/API/commands/dwim-print/TestDWIMPrint.py
@@ -10,11 +10,6 @@
 
 
 class TestCase(TestBase):
-def setUp(self):
-TestBase.setUp(self)
-self.build()
-lldbutil.run_to_name_breakpoint(self, "main")
-
 def _run_cmd(self, cmd: str) -> str:
 """Run the given lldb command and return its output."""
 result = lldb.SBCommandReturnObject()
@@ -51,18 +46,28 @@ def _expect_cmd(self, expr: str, base_cmd: str) -> None:
 
 def test_variables(self):
 """Test dwim-print with variables."""
+self.build()
+lldbutil.run_to_name_breakpoint(self, "main")
 vars = ("argc", "argv")
 for var in vars:
 self._expect_cmd(var, "frame variable")
 
 def test_variable_paths(self):
 """Test dwim-print with variable path expressions."""
+self.build()
+lldbutil.run_to_name_breakpoint(self, "main")
 exprs = ("&argc", "*argv", "argv[0]")
 for expr in exprs:
 self._expect_cmd(expr, "expression --")
 
 def test_expressions(self):
 """Test dwim-print with expressions."""
+self.build()
+lldbutil.run_to_name_breakpoint(self, "main")
 exprs = ("argc + 1", "(void)argc", "(int)abs(argc)")
 for expr in exprs:
 self._expect_cmd(expr, "expre

[Lldb-commits] [PATCH] D138960: [lldb] Enable use of dummy target from dwim-print

[Dave Lee via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcebb87e7dce8: [lldb] Enable use of dummy target from 
dwim-print (authored by kastiglione).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138960

Files:
  lldb/source/Commands/CommandObjectDWIMPrint.cpp
  lldb/test/API/commands/dwim-print/TestDWIMPrint.py

Index: lldb/test/API/commands/dwim-print/TestDWIMPrint.py
===
--- lldb/test/API/commands/dwim-print/TestDWIMPrint.py
+++ lldb/test/API/commands/dwim-print/TestDWIMPrint.py
@@ -10,11 +10,6 @@
 
 
 class TestCase(TestBase):
-def setUp(self):
-TestBase.setUp(self)
-self.build()
-lldbutil.run_to_name_breakpoint(self, "main")
-
 def _run_cmd(self, cmd: str) -> str:
 """Run the given lldb command and return its output."""
 result = lldb.SBCommandReturnObject()
@@ -51,18 +46,28 @@
 
 def test_variables(self):
 """Test dwim-print with variables."""
+self.build()
+lldbutil.run_to_name_breakpoint(self, "main")
 vars = ("argc", "argv")
 for var in vars:
 self._expect_cmd(var, "frame variable")
 
 def test_variable_paths(self):
 """Test dwim-print with variable path expressions."""
+self.build()
+lldbutil.run_to_name_breakpoint(self, "main")
 exprs = ("&argc", "*argv", "argv[0]")
 for expr in exprs:
 self._expect_cmd(expr, "expression --")
 
 def test_expressions(self):
 """Test dwim-print with expressions."""
+self.build()
+lldbutil.run_to_name_breakpoint(self, "main")
 exprs = ("argc + 1", "(void)argc", "(int)abs(argc)")
 for expr in exprs:
 self._expect_cmd(expr, "expression --")
+
+def test_dummy_target_expressions(self):
+"""Test dwim-print's ability to evaluate expressions without a target."""
+self._expect_cmd("1 + 2", "expression --")
Index: lldb/source/Commands/CommandObjectDWIMPrint.cpp
===
--- lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -22,12 +22,11 @@
 using namespace lldb_private;
 
 CommandObjectDWIMPrint::CommandObjectDWIMPrint(CommandInterpreter &interpreter)
-: CommandObjectRaw(
-  interpreter, "dwim-print", "Print a variable or expression.",
-  "dwim-print [ | ]",
-  eCommandProcessMustBePaused | eCommandTryTargetAPILock |
-  eCommandRequiresFrame | eCommandProcessMustBeLaunched |
-  eCommandRequiresProcess) {}
+: CommandObjectRaw(interpreter, "dwim-print",
+   "Print a variable or expression.",
+   "dwim-print [ | ]",
+   eCommandProcessMustBePaused | eCommandTryTargetAPILock) {
+}
 
 bool CommandObjectDWIMPrint::DoExecute(StringRef expr,
CommandReturnObject &result) {
@@ -40,14 +39,10 @@
 return false;
   }
 
-  // eCommandRequiresFrame guarantees a frame.
-  StackFrame *frame = m_exe_ctx.GetFramePtr();
-  assert(frame);
-
   auto verbosity = GetDebugger().GetDWIMPrintVerbosity();
 
-  // First, try `expr` as the name of a variable.
-  {
+  // First, try `expr` as the name of a frame variable.
+  if (StackFrame *frame = m_exe_ctx.GetFramePtr()) {
 auto valobj_sp = frame->FindVariable(ConstString(expr));
 if (valobj_sp && valobj_sp->GetError().Success()) {
   if (verbosity == eDWIMPrintVerbosityFull)
@@ -60,12 +55,13 @@
 
   // Second, also lastly, try `expr` as a source expression to evaluate.
   {
-// eCommandRequiresProcess guarantees a target.
-Target *target = m_exe_ctx.GetTargetPtr();
-assert(target);
+Target *target_ptr = m_exe_ctx.GetTargetPtr();
+// Fallback to the dummy target, which can allow for expression evaluation.
+Target &target = target_ptr ? *target_ptr : GetDummyTarget();
 
+auto *exe_scope = m_exe_ctx.GetBestExecutionContextScope();
 ValueObjectSP valobj_sp;
-if (target->EvaluateExpression(expr, frame, valobj_sp) ==
+if (target.EvaluateExpression(expr, exe_scope, valobj_sp) ==
 eExpressionCompleted) {
   if (verbosity != eDWIMPrintVerbosityNone)
 result.AppendMessageWithFormatv("note: ran `expression -- {0}`", expr);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D138618#3963767 , @labath wrote:

> The explanation makes sense, and I *think* the patch is ok, but it's hard to 
> review it with all the noise. I still believe the DIERef change would be 
> better off as a separate patch, so that the change is not obscured by the 
> (hopefully mechanical) aspects of increasing the size of the offset field.

I am saying we can't make the DIERef change separately because it breaks the 
buildbots. Without modifying the OSO stuff, we can't make this patch work. If 
we need to change the max DIE offsets size, we need all these fixes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138618

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

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

We have modified the DIERef class over the years and made it work for both 
DWARF in .o files (OSO) for mac and then for fission (.dwo). The two made 
different changes that never affected either one but with these changes they 
need to coexist and actually work with DIERef in the same way with no 
assumptions on the DIE offset being in the lower 32 bits of a user_id_t.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138618

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

[Lldb-commits] [PATCH] D139083: [lldb][Module][NFC] Add ModuleList::AnyOf

[Michael Buch via Phabricator via lldb-commits]

Michael137 updated this revision to Diff 479444.
Michael137 added a comment.

- Reference insstead of `shared_ptr`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139083

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
@@ -1074,3 +1074,16 @@
   break;
   }
 }
+
+bool ModuleList::AnyOf(
+std::function const &callback)
+const {
+  std::lock_guard guard(m_modules_mutex);
+  for (const auto &module_sp : m_modules) {
+assert(module_sp != nullptr);
+if (callback(*module_sp))
+  return true;
+  }
+
+  return false;
+}
Index: lldb/include/lldb/Core/ModuleList.h
===
--- lldb/include/lldb/Core/ModuleList.h
+++ lldb/include/lldb/Core/ModuleList.h
@@ -473,6 +473,13 @@
   void ForEach(std::function const
&callback) const;
 
+  /// Returns true if 'callback' returns true for one of the modules
+  /// in this ModuleList.
+  ///
+  /// This function is thread-safe.
+  bool AnyOf(
+  std::function const &callback) const;
+
 protected:
   // Class typedefs.
   typedef std::vector


Index: lldb/source/Core/ModuleList.cpp
===
--- lldb/source/Core/ModuleList.cpp
+++ lldb/source/Core/ModuleList.cpp
@@ -1074,3 +1074,16 @@
   break;
   }
 }
+
+bool ModuleList::AnyOf(
+std::function const &callback)
+const {
+  std::lock_guard guard(m_modules_mutex);
+  for (const auto &module_sp : m_modules) {
+assert(module_sp != nullptr);
+if (callback(*module_sp))
+  return true;
+  }
+
+  return false;
+}
Index: lldb/include/lldb/Core/ModuleList.h
===
--- lldb/include/lldb/Core/ModuleList.h
+++ lldb/include/lldb/Core/ModuleList.h
@@ -473,6 +473,13 @@
   void ForEach(std::function const
&callback) const;
 
+  /// Returns true if 'callback' returns true for one of the modules
+  /// in this ModuleList.
+  ///
+  /// This function is thread-safe.
+  bool AnyOf(
+  std::function const &callback) const;
+
 protected:
   // Class typedefs.
   typedef std::vector
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D138724: [lldb][Target] Flush the scratch TypeSystem when process gets deleted

[Michael Buch via Phabricator via lldb-commits]

Michael137 updated this revision to Diff 479446.
Michael137 added a comment.

- Add dylib tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138724

Files:
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/rerun_and_expr/Makefile
  lldb/test/API/functionalities/rerun_and_expr/TestRerunAndExpr.py
  lldb/test/API/functionalities/rerun_and_expr/main.cpp
  lldb/test/API/functionalities/rerun_and_expr/rebuild.cpp
  lldb/test/API/functionalities/rerun_and_expr_dylib/Makefile
  lldb/test/API/functionalities/rerun_and_expr_dylib/TestRerunAndExprDylib.py
  lldb/test/API/functionalities/rerun_and_expr_dylib/lib.cpp
  lldb/test/API/functionalities/rerun_and_expr_dylib/main.cpp
  lldb/test/API/functionalities/rerun_and_expr_dylib/rebuild.cpp

Index: lldb/test/API/functionalities/rerun_and_expr_dylib/rebuild.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/rerun_and_expr_dylib/rebuild.cpp
@@ -0,0 +1,7 @@
+struct Base {
+  int m_base_val = 42;
+};
+
+LLDB_DYLIB_EXPORT struct Foo : public Base {
+  int m_derived_val = 137;
+} global_foo;
Index: lldb/test/API/functionalities/rerun_and_expr_dylib/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/rerun_and_expr_dylib/main.cpp
@@ -0,0 +1,12 @@
+#include 
+#include 
+
+extern struct Foo imported;
+
+int main() {
+  void *handle = dlopen("libfoo.dylib", RTLD_NOW);
+  struct Foo *foo = (struct Foo *)dlsym(handle, "global_foo");
+  assert(foo != nullptr);
+
+  return 0;
+}
Index: lldb/test/API/functionalities/rerun_and_expr_dylib/lib.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/rerun_and_expr_dylib/lib.cpp
@@ -0,0 +1 @@
+LLDB_DYLIB_EXPORT struct Foo { int m_val = 42; } global_foo;
Index: lldb/test/API/functionalities/rerun_and_expr_dylib/TestRerunAndExprDylib.py
===
--- /dev/null
+++ lldb/test/API/functionalities/rerun_and_expr_dylib/TestRerunAndExprDylib.py
@@ -0,0 +1,72 @@
+"""
+Test that re-running a process from within the same target
+after rebuilding the a dynamic library flushes the scratch
+TypeSystems tied to that process.
+"""
+
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestRerun(TestBase):
+def test(self):
+"""
+Tests whether re-launching a process without destroying
+the owning target keeps invalid ASTContexts in the
+scratch AST's importer.
+
+We test this by:
+1. Evaluating an expression to import 'struct Foo' into
+   the scratch AST
+2. Change the definition of 'struct Foo' and rebuild the dylib
+3. Re-launch the process
+4. Evaluate the same expression in (1). We expect to have only
+   the latest definition of 'struct Foo' in the scratch AST.
+"""
+
+# Build a.out
+self.build(dictionary={'EXE':'a.out',
+   'CXX_SOURCES':'main.cpp'})
+
+# Build libfoo.dylib
+self.build(dictionary={'DYLIB_CXX_SOURCES':'lib.cpp',
+   'DYLIB_ONLY':'YES',
+   'DYLIB_NAME':'foo',
+   'USE_LIBDL':'1',
+   'LD_EXTRAS':'-L.'})
+
+(target, _, _, bkpt) = \
+lldbutil.run_to_source_breakpoint(self, 'return', lldb.SBFileSpec('main.cpp'))
+
+self.expect_expr('*foo', result_type='Foo', result_children=[
+ValueCheck(name='m_val', value='42')
+])
+
+# Re-build libfoo.dylib
+self.build(dictionary={'DYLIB_CXX_SOURCES':'rebuild.cpp',
+   'DYLIB_ONLY':'YES',
+   'DYLIB_NAME':'foo',
+   'USE_LIBDL':'1',
+   'LD_EXTRAS':'-L.'})
+
+self.runCmd('process launch')
+(target, _, _, bkpt) = \
+lldbutil.run_to_source_breakpoint(self, 'return', lldb.SBFileSpec('main.cpp'))
+
+self.expect_expr('*foo', result_type='Foo', result_children=[
+ValueCheck(name='Base', children=[
+ValueCheck(name='m_base_val', value='42')
+]),
+ValueCheck(name='m_derived_val', value='137')
+])
+
+self.filecheck("target module dump ast", __file__)
+
+# The new definition 'struct Foo' is in the scratch AST
+# CHECK:  |-CXXRecordDecl {{.*}} struct Foo definition
+# CHECK:  | |-public 'Base'
+# CHECK-NEXT: | `-FieldDecl {{.*}} m_derived_val 'int'
+# CHECK-NEXT: `-CXXRecordDecl {{.*}} struct Base definition
+
+# ...but the original definition of 'struct Foo' is not in the scratch AST anymore

[Lldb-commits] [PATCH] D138834: [lldb] Fix simple template names interaction with debug info declarations

[David Blaikie via Phabricator via lldb-commits]

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

I think this is good - welcome to wait for a second opinion if you prefer, or 
folks can provide feedback post-commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138834

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

[Lldb-commits] [PATCH] D139083: [lldb][Module][NFC] Add ModuleList::AnyOf

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Core/ModuleList.cpp:1079
+bool ModuleList::AnyOf(
+std::function const &callback)
+const {

module


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139083

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

[Lldb-commits] [PATCH] D138060: Improve error logging when xcrun fails to execute successfully

[Adrian Prantl via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG33cbda4cacb8: Improve error logging when xcrun fails to 
execute successfully (authored by aprantl).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138060

Files:
  lldb/include/lldb/Host/HostInfoBase.h
  lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
  lldb/source/Core/Module.cpp
  lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
  lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
  lldb/unittests/Host/CMakeLists.txt
  lldb/unittests/Host/HostInfoTest.cpp

Index: lldb/unittests/Host/HostInfoTest.cpp
===
--- lldb/unittests/Host/HostInfoTest.cpp
+++ lldb/unittests/Host/HostInfoTest.cpp
@@ -56,11 +56,21 @@
 
 #if defined(__APPLE__)
 TEST_F(HostInfoTest, GetXcodeSDK) {
-  EXPECT_FALSE(HostInfo::GetXcodeSDKPath(XcodeSDK("MacOSX.sdk")).empty());
+  auto get_sdk = [](std::string sdk, bool error = false) -> llvm::StringRef {
+auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK(std::move(sdk)));
+if (!error) {
+  EXPECT_TRUE((bool)sdk_path_or_err);
+  return *sdk_path_or_err;
+}
+EXPECT_FALSE((bool)sdk_path_or_err);
+llvm::consumeError(sdk_path_or_err.takeError());
+return {};
+  };
+  EXPECT_FALSE(get_sdk("MacOSX.sdk").empty());
   // These are expected to fall back to an available version.
-  EXPECT_FALSE(HostInfo::GetXcodeSDKPath(XcodeSDK("MacOSX.sdk")).empty());
+  EXPECT_FALSE(get_sdk("MacOSX.sdk").empty());
   // This is expected to fail.
-  EXPECT_TRUE(HostInfo::GetXcodeSDKPath(XcodeSDK("CeciNestPasUnOS.sdk")).empty());
+  EXPECT_TRUE(get_sdk("CeciNestPasUnOS.sdk", true).empty());
 }
 #endif
 
Index: lldb/unittests/Host/CMakeLists.txt
===
--- lldb/unittests/Host/CMakeLists.txt
+++ lldb/unittests/Host/CMakeLists.txt
@@ -32,6 +32,7 @@
   ${FILES}
   LINK_LIBS
 lldbHost
+lldbCore
 lldbUtilityHelpers
 lldbHostHelpers
 LLVMTestingSupport
Index: lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
@@ -17,6 +17,7 @@
 #include "PlatformRemoteAppleWatch.h"
 #endif
 #include "lldb/Breakpoint/BreakpointLocation.h"
+#include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
@@ -123,8 +124,14 @@
   }
 
   // Use the default SDK as a fallback.
-  FileSpec fspec(
-  HostInfo::GetXcodeSDKPath(lldb_private::XcodeSDK::GetAnyMacOS()));
+  auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK::GetAnyMacOS());
+  if (!sdk_path_or_err) {
+Debugger::ReportError("Error while searching for Xcode SDK: " +
+  toString(sdk_path_or_err.takeError()));
+return {};
+  }
+
+  FileSpec fspec(*sdk_path_or_err);
   if (fspec) {
 if (FileSystem::Instance().Exists(fspec))
   return ConstString(fspec.GetPath());
Index: lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
===
--- lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
@@ -12,6 +12,7 @@
 #include 
 #endif
 
+#include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Host/HostInfo.h"
@@ -278,9 +279,19 @@
 static llvm::StringRef GetXcodeSDKDir(std::string preferred,
   std::string secondary) {
   llvm::StringRef sdk;
-  sdk = HostInfo::GetXcodeSDKPath(XcodeSDK(std::move(preferred)));
+  auto get_sdk = [&](std::string sdk) -> llvm::StringRef {
+auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK(std::move(sdk)));
+if (!sdk_path_or_err) {
+  Debugger::ReportError("Error while searching for Xcode SDK: " +
+toString(sdk_path_or_err.takeError()));
+  return {};
+}
+return *sdk_path_or_err;
+  };
+
+  sdk = get_sdk(preferred);
   if (sdk.empty())
-sdk = HostInfo::GetXcodeSDKPath(XcodeSDK(std::move(secondary)));
+sdk = get_sdk(secondary);
   return sdk;
 }
 
Index: lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
===
--- lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
+++ lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
@@ -6,15 +6,15 @@
 //
 //===--===//
 
+#include "lldb/Host/macosx/HostInfoMacOSX.h"
+#include "Utility/UuidCompatibility.h"
 #inclu

[Lldb-commits] [lldb] 33cbda4 - Improve error logging when xcrun fails to execute successfully

[Adrian Prantl via lldb-commits]

Author: Adrian Prantl
Date: 2022-12-01T16:22:14-08:00
New Revision: 33cbda4cacb87103bbb5f4f9f2368bbb442132f4

URL: 
https://github.com/llvm/llvm-project/commit/33cbda4cacb87103bbb5f4f9f2368bbb442132f4
DIFF: 
https://github.com/llvm/llvm-project/commit/33cbda4cacb87103bbb5f4f9f2368bbb442132f4.diff

LOG: Improve error logging when xcrun fails to execute successfully

Because Host::RunShellCommand runs commands through $SHELL there is an
opportunity for this to fail spectacularly on systems that use custom
shells with odd behaviors. This patch makes these situations easier to
debug by at least logging the result of the failed xcrun invocation.

It also doesn't run xcrun through a shell any more.

rdar://102389438

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

Added: 


Modified: 
lldb/include/lldb/Host/HostInfoBase.h
lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
lldb/source/Core/Module.cpp
lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
lldb/unittests/Host/CMakeLists.txt
lldb/unittests/Host/HostInfoTest.cpp

Removed: 




diff  --git a/lldb/include/lldb/Host/HostInfoBase.h 
b/lldb/include/lldb/Host/HostInfoBase.h
index eeed881101d03..38c488cc73b83 100644
--- a/lldb/include/lldb/Host/HostInfoBase.h
+++ b/lldb/include/lldb/Host/HostInfoBase.h
@@ -108,7 +108,9 @@ class HostInfoBase {
   static FileSpec GetXcodeDeveloperDirectory() { return {}; }
   
   /// Return the directory containing a specific Xcode SDK.
-  static llvm::StringRef GetXcodeSDKPath(XcodeSDK sdk) { return {}; }
+  static llvm::Expected GetXcodeSDKPath(XcodeSDK sdk) {
+return "";
+  }
 
   /// Return information about module \p image_name if it is loaded in
   /// the current process's address space.

diff  --git a/lldb/include/lldb/Host/macosx/HostInfoMacOSX.h 
b/lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
index de7ecc1e6d803..913af1ffe70ba 100644
--- a/lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
+++ b/lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
@@ -30,7 +30,7 @@ class HostInfoMacOSX : public HostInfoPosix {
   static FileSpec GetXcodeDeveloperDirectory();
 
   /// Query xcrun to find an Xcode SDK directory.
-  static llvm::StringRef GetXcodeSDKPath(XcodeSDK sdk);
+  static llvm::Expected GetXcodeSDKPath(XcodeSDK sdk);
 
   /// Shared cache utilities
   static SharedCacheImageInfo

diff  --git a/lldb/source/Core/Module.cpp b/lldb/source/Core/Module.cpp
index dd2569eb81f8e..63b79b3b71d88 100644
--- a/lldb/source/Core/Module.cpp
+++ b/lldb/source/Core/Module.cpp
@@ -1645,7 +1645,15 @@ Module::RemapSourceFile(llvm::StringRef path) const {
 void Module::RegisterXcodeSDK(llvm::StringRef sdk_name,
   llvm::StringRef sysroot) {
   XcodeSDK sdk(sdk_name.str());
-  llvm::StringRef sdk_path(HostInfo::GetXcodeSDKPath(sdk));
+  auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(sdk);
+
+  if (!sdk_path_or_err) {
+Debugger::ReportError("Error while searching for Xcode SDK: " +
+  toString(sdk_path_or_err.takeError()));
+return;
+  }
+
+  auto sdk_path = *sdk_path_or_err;
   if (sdk_path.empty())
 return;
   // If the SDK changed for a previously registered source path, update it.

diff  --git a/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm 
b/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
index d0d964b0903be..ddc66c8f5b50b 100644
--- a/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
+++ b/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
@@ -6,15 +6,15 @@
 //
 
//===--===//
 
+#include "lldb/Host/macosx/HostInfoMacOSX.h"
+#include "Utility/UuidCompatibility.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
-#include "lldb/Host/macosx/HostInfoMacOSX.h"
 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Timer.h"
-#include "Utility/UuidCompatibility.h"
 
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
@@ -337,7 +337,14 @@ static void ParseOSVersion(llvm::VersionTuple &version, 
NSString *Key) {
   }
 }
 
-FileSpec fspec(HostInfo::GetXcodeSDKPath(XcodeSDK::GetAnyMacOS()));
+auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK::GetAnyMacOS());
+if (!sdk_path_or_err) {
+  Log *log = GetLog(LLDBLog::Host);
+  LLDB_LOGF(log, "Error while searching for Xcode SDK: %s",
+toString(sdk_path_or_err.takeError()).c_str());
+  return;
+}
+FileSpec fspec(*sdk_path_or_err);
 if (fspec) {
   if (FileSystem::Instance().Exists(fspec)) {
 std::string xcode_contents_dir =
@@ -365,12 +372,18 @@ static void ParseOSVersion(llvm::VersionTuple &version, 
NSString *Key) {
   retur

[Lldb-commits] [PATCH] D139082: [lldb][Module] Document ModuleList::ForEach and assert nullness

[Adrian Prantl via Phabricator via lldb-commits]

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

If we assert non-null, why still pass a shared pointer? Could we have the 
lambda take a `Module &`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139082

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

[Lldb-commits] [PATCH] D139158: [LLDB][LoongArch] Make software single stepping work

[Hui Li via Phabricator via lldb-commits]

lh03061238 created this revision.
lh03061238 added reviewers: SixWeining, wangleiat, xen0n, xry111, MaskRay, 
DavidSpickett.
Herald added subscribers: StephenFan, s.egerton, simoncook.
Herald added a project: All.
lh03061238 requested review of this revision.
Herald added subscribers: lldb-commits, pcwang-thead.
Herald added a project: LLDB.

Hardware single stepping is not currently supported by the linux kernel. 
In order to support single step debugging, add EmulateInstructionLoongArch 
to implement the software Single Stepping. This patch only support the 
simplest single step execution of non-jump instructions.

Here is a simple test:

  loongson@linux:~$ cat test.c
  #include
  
  int main()
  {
int i = 1;
  
i = i + 2;
  
return 0;
  }
  loongson@linux:~$ clang -g test.c  -o test

Without this patch:

  loongson@linux:~$ llvm-project/llvm/build/bin/lldb test
  (lldb) target create "test"
  Current executable set to '/home/loongson/test' (loongarch64).
  (lldb) b main
  Breakpoint 1: where = test`main + 24 at test.c:5:6, address = 
0x00012628
  (lldb) r
  Process 43090 launched: '/home/loongson/test' (loongarch64)
  Process 43090 exited with status = -1 (0x) lost connection
  (lldb)

With this patch:

  loongson@linux:~$ llvm-project/llvm/build/bin/lldb test
  (lldb) target create "test"
  Current executable set to '/home/loongson/test' (loongarch64).
  (lldb) b main
  Breakpoint 1: where = test`main + 24 at test.c:5:6, address = 
0x00012628
  (lldb) r
  Process 42467 launched: '/home/loongson/test' (loongarch64)
  Process 42467 stopped
  * thread #1, name = 'test', stop reason = breakpoint 1.1
  frame #0: 0x00012628 test`main at test.c:5:6
 2  
 3  int main()
 4  {
  -> 5  int i = 1;
 6  
 7  i = i + 2;
 8  
  (lldb) n
  Process 42467 stopped
  * thread #1, name = 'test', stop reason = step over
  frame #0: 0x0001262c test`main at test.c:7:6
 4  {
 5  int i = 1;
 6  
  -> 7  i = i + 2;
 8  
 9  return 0;
 10 }
  (lldb) var i
  (int) i = 1
  (lldb) n
  Process 42467 stopped
  * thread #1, name = 'test', stop reason = step over
  frame #0: 0x0001263c test`main at test.c:9:2
 6  
 7  i = i + 2;
 8  
  -> 9  return 0;
 10 }
  (lldb) var i
  (int) i = 3
  (lldb) n
  Process 42467 exited with status = 0 (0x) 
  (lldb) q
  loongson@linux:~$


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139158

Files:
  lldb/source/Plugins/Instruction/CMakeLists.txt
  lldb/source/Plugins/Instruction/LoongArch/CMakeLists.txt
  lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.cpp
  lldb/source/Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
  lldb/tools/lldb-server/CMakeLists.txt
  lldb/tools/lldb-server/SystemInitializerLLGS.cpp

Index: lldb/tools/lldb-server/SystemInitializerLLGS.cpp
===
--- lldb/tools/lldb-server/SystemInitializerLLGS.cpp
+++ lldb/tools/lldb-server/SystemInitializerLLGS.cpp
@@ -46,6 +46,11 @@
 #include "Plugins/Instruction/RISCV/EmulateInstructionRISCV.h"
 #endif
 
+#if defined(__loongarch) || defined(__loongarch64)
+#define LLDB_TARGET_LoongArch
+#include "Plugins/Instruction/LoongArch/EmulateInstructionLoongArch.h"
+#endif
+
 using namespace lldb_private;
 
 llvm::Error SystemInitializerLLGS::Initialize() {
@@ -66,6 +71,9 @@
 #if defined(LLDB_TARGET_RISCV)
   EmulateInstructionRISCV::Initialize();
 #endif
+#if defined(LLDB_TARGET_LoongArch)
+  EmulateInstructionLoongArch::Initialize();
+#endif
 
   return llvm::Error::success();
 }
@@ -85,6 +93,9 @@
 #if defined(LLDB_TARGET_RISCV)
   EmulateInstructionRISCV::Terminate();
 #endif
+#if defined(LLDB_TARGET_LoongArch)
+  EmulateInstructionLoongArch::Terminate();
+#endif
 
   SystemInitializerCommon::Terminate();
 }
Index: lldb/tools/lldb-server/CMakeLists.txt
===
--- lldb/tools/lldb-server/CMakeLists.txt
+++ lldb/tools/lldb-server/CMakeLists.txt
@@ -54,6 +54,7 @@
   lldbPluginInstructionMIPS
   lldbPluginInstructionMIPS64
   lldbPluginInstructionRISCV
+  lldbPluginInstructionLoongArch
   ${LLDB_SYSTEM_LIBS}
 
 LINK_COMPONENTS
Index: lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
===
--- lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
+++ lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
@@ -168,7 +168,7 @@
   size_hint = 4;
 }
 

[Lldb-commits] [PATCH] D139082: [lldb][Module] Document ModuleList::ForEach and assert nullness

[Michael Buch via Phabricator via lldb-commits]

Michael137 added a comment.

In D139082#3965316 , @aprantl wrote:

> If we assert non-null, why still pass a shared pointer? Could we have the 
> lambda take a `Module &`?

I took a stab at changing it to pass a reference but turns out there's several 
places where we end up storing the shared_ptr. If we changed this to a 
reference we'd have to call shared_from_this for those cases. Which is a bit 
iffy considering calling it on references that are not shared_ptr owned would 
throw (which can't happen currently but seems similarly awkward to the current 
API).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139082

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

[Lldb-commits] [PATCH] D139158: [LLDB][LoongArch] Make software single stepping work

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Note that the two most common uses of single step in lldb are 1) stepping over 
the instruction under a breakpoint (which may be a branch) and 2) stepping to 
the next instruction from a branch instruction.  So stepping won't work 
correctly till you get single step past a branch instruction working.  But you 
probably knew that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139158

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

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[Christopher Di Bella via Phabricator via lldb-commits]

cjdb updated this revision to Diff 479492.
cjdb marked 4 inline comments as done.
cjdb added a comment.

responds to feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clangd/Compiler.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang/include/clang/Basic/Diagnostic.h
  clang/include/clang/Basic/Diagnostic.td
  clang/include/clang/Basic/DiagnosticAST.h
  clang/include/clang/Basic/DiagnosticAnalysis.h
  clang/include/clang/Basic/DiagnosticComment.h
  clang/include/clang/Basic/DiagnosticCrossTU.h
  clang/include/clang/Basic/DiagnosticDriver.h
  clang/include/clang/Basic/DiagnosticFrontend.h
  clang/include/clang/Basic/DiagnosticIDs.h
  clang/include/clang/Basic/DiagnosticLex.h
  clang/include/clang/Basic/DiagnosticParse.h
  clang/include/clang/Basic/DiagnosticRefactoring.h
  clang/include/clang/Basic/DiagnosticSema.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/DiagnosticSerialization.h
  clang/include/clang/Basic/PartialDiagnostic.h
  clang/include/clang/Frontend/ASTUnit.h
  clang/lib/Basic/Diagnostic.cpp
  clang/lib/Basic/DiagnosticIDs.cpp
  clang/lib/Format/TokenAnalyzer.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/LogDiagnosticPrinter.cpp
  clang/lib/Frontend/SARIFDiagnosticPrinter.cpp
  clang/lib/Frontend/SerializedDiagnosticPrinter.cpp
  clang/lib/Frontend/TextDiagnosticBuffer.cpp
  clang/lib/Frontend/TextDiagnosticPrinter.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/Frontend/sarif-reason.cpp
  clang/test/TableGen/DiagnosticBase.inc
  clang/test/TableGen/deferred-diag.td
  clang/tools/clang-format/ClangFormat.cpp
  clang/tools/clang-import-test/clang-import-test.cpp
  clang/tools/diagtool/DiagnosticNames.cpp
  clang/unittests/Driver/SimpleDiagnosticConsumer.h
  clang/unittests/Driver/ToolChainTest.cpp
  clang/unittests/Frontend/FrontendActionTest.cpp
  clang/unittests/Tooling/RewriterTestContext.h
  clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
  flang/lib/Frontend/TextDiagnosticBuffer.cpp
  flang/lib/Frontend/TextDiagnosticPrinter.cpp
  libcxxabi/test/test_demangle.pass.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  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
@@ -697,7 +697,7 @@
 const clang::Diagnostic &info) override {
 if (m_log) {
   llvm::SmallVector diag_str(10);
-  info.FormatDiagnostic(diag_str);
+  info.FormatLegacyDiagnostic(diag_str);
   diag_str.push_back('\0');
   LLDB_LOGF(m_log, "Compiler diagnostic: %s\n", diag_str.data());
 }
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -192,7 +192,7 @@
   Log *log = GetLog(LLDBLog::Expressions);
   if (log) {
 llvm::SmallVector diag_str;
-Info.FormatDiagnostic(diag_str);
+Info.FormatLegacyDiagnostic(diag_str);
 diag_str.push_back('\0');
 const char *plain_diag = diag_str.data();
 LLDB_LOG(log, "Received diagnostic outside parsing: {0}", plain_diag);
Index: flang/lib/Frontend/TextDiagnosticPrinter.cpp
===
--- flang/lib/Frontend/TextDiagnosticPrinter.cpp
+++ flang/lib/Frontend/TextDiagnosticPrinter.cpp
@@ -37,7 +37,8 @@
   // Render the diagnostic message into a temporary buffer eagerly. We'll use
   // this later as we print out the diagnostic to the terminal.
   llvm::SmallString<100> outStr;
-  info.FormatDiagnostic(outStr);
+  info.FormatSummary(outStr);
+  info.FormatLegacyDiagnostic(outStr);
 
   llvm::raw_svector_ostream diagMessageStream(outStr);
 
Index: flang/lib/Frontend/TextDiagnosticBuffer.cpp
===
--- flang/lib/Frontend/TextDiagnosticBuffer.cpp
+++ flang/lib/Frontend/TextDiagnosticBuffer.cpp
@@ -29,7 +29,8 @@
   DiagnosticConsumer::HandleDiagnostic(level, info);
 
   llvm::SmallString<100> buf;
-  info.FormatDiagnostic(buf);
+  info.FormatSummary(buf);
+  info.FormatLegacyDiagnostic(buf);
   switch (level) {
   default:
 llvm_unreachable("Diagnostic not handled during diagnostic buffering!");
Index: clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
===
--- clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
+++ clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
@@ -60

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

[Christopher Di Bella via Phabricator via lldb-commits]

cjdb added a comment.

In D138939#3964677 , @erichkeane 
wrote:

> In D138939#3964439 , @erichkeane 
> wrote:
>
>> In D138939#3964404 , @cjdb wrote:
>>
>>> In D138939#3963496 , @erichkeane 
>>> wrote:
>>>
 In D138939#3963473 , @tschuett 
 wrote:

> Maybe `void FormatDiagnostic(SmallVectorImpl &OutStr, 
> DiagnosticMode mode)`instead of `void 
> FormatDiagnostic(SmallVectorImpl &OutStr)`?
> To make the transition easer and future proof.

 I like this idea.  Perhaps with DiagnosticMode being a 3-way enum:

   enum struct DiagnosticMode {
 Legacy,
 Sarif,  
 Default = Legacy
   }

 I like the idea in particular, since it makes a command line flag to 
 modify "Default" to be whichever the user prefers pretty trivial.
>>>
>>> There's already a flag for this: `-fdiagnostics-format=sarif`. Why do we 
>>> need a second diagnostic mode flag?
>>
>> Ah, oh... is the Sarif formatting being done with a new formatter?  That 
>> seems unfortunate, since folks using the other formatters won't be able to 
>> use the user friendly formats.
>
> I've been alerted offline that I am misunderstanding the Sarif proposal, and 
> where this is going.  I'll note that I wasn't present/invited at the calls 
> where all of this was discussed, so I am admittedly not completely up to 
> date.  The above concern shouldn't stop others from reviewing this, 
> particularly if you better understand the intent here.

I don't necessarily think you're the one misunderstanding here. We're 
definitely talking past one another, but I think it's equally likely that 
you're asking for something reasonable, and I'm mixing it up with something 
else.




Comment at: clang/include/clang/Frontend/DiagnosticRenderer.h:130
   /// \param Message The diagnostic message to emit.
+  /// \param Reason Supplementary information for the message.
   /// \param Ranges The underlined ranges for this code snippet.

rymiel wrote:
> Which parameter is this doxygen comment referring to?
Thanks, that was from an old iteration of the CL.



Comment at: clang/lib/Basic/Diagnostic.cpp:791
 
 /// FormatDiagnostic - Format this diagnostic into a string, substituting the
 /// formal arguments into the %0 slots.  The result is appended onto the Str

erichkeane wrote:
> Comment no longer matches.
I've deleted the comment because it's already in the header, and risks going 
stale over and over.



Comment at: clang/test/Frontend/sarif-reason.cpp:15
+void g() {
+  f1<0>(); // expected-error{{no matching function for call to 'f1'}}
+  f1(); // expected-error{{no matching function for call to 'f1'}}

erichkeane wrote:
> This is definitely a case where I'd love the diagnostics formatted/arranged 
> differently here.  If you can use the #BOOKMARK style to make sure errors and 
> notes are together, it would better illustrate what you're trying to do here.
This is maybe done? I'm not sure if this is the #BOOKMARK style you're 
referring to, but it should capture the same intent. Lemme know if you had 
something else in mind and I'll happily change it 🙂 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138939

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

[Lldb-commits] [PATCH] D137900: Make only one function that needs to be implemented when searching for types.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/include/lldb/Core/Module.h:435
+  ///   match: "b::a", "c::b::a", "d::b::a", "e::f::b::a".
+  lldb::TypeSP FindFirstType(ConstString type_name, bool exact_match);
 

aprantl wrote:
> clayborg wrote:
> > aprantl wrote:
> > > Why is this not taking a TypeQuery that wraps a name?
> > This function doesn't need to exist as long as the "TypeSP 
> > TypeQuery::FindFirstType(Module)" function is ok to have around.
> It would be good to have some consistency in the interface:
> - Either TypeQuery provides Find* methods that take a Module 
> - or Module provides Find* methods that take a TypeQuery.
> 
> Why does this interface exist instead of having a "FindFirstOnly" flag in 
> TypeQuery? Is it because of the overhead of having to dig the type out of a 
> TypeMap?
> 
> Otherwise we could just have one method
> 
> Module::FindTypes(const TypeQuery &query, TypeResults &results);
> 
> that can be called like this
> ```
> module_sp->FindTypes(TypeQuery::createFindFirstByName("name", e_exact_match), 
> result);
> TypeSP type = result.GetTypeAtIndex(0); /// better have a special method for 
> this too
> ```
See what you think of the current code and let me know any changes you would 
like to see.



Comment at: lldb/include/lldb/Symbol/CompilerDecl.h:93
+  void GetCompilerContext(
+  llvm::SmallVectorImpl &context) const;
+

clayborg wrote:
> aprantl wrote:
> > aprantl wrote:
> > > Why can't this be a return value? The context objects are tiny.
> > ping?
> Will change!
I looked around LLVM code and there are many accessors that use this technique. 
Otherwise we need to enforce an exact SmallVector type as the return code and 
we can't use "llvm::SmallVectorImpl". I can 
change this to return a "llvm::SmallVector" if needed? But 
that locks us into a specific small vector with a specific size of preached 
items.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137900

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

[Lldb-commits] [PATCH] D137900: Make only one function that needs to be implemented when searching for types.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/include/lldb/Symbol/CompilerDecl.h:93
+  void GetCompilerContext(
+  llvm::SmallVectorImpl &context) const;
+

clayborg wrote:
> clayborg wrote:
> > aprantl wrote:
> > > aprantl wrote:
> > > > Why can't this be a return value? The context objects are tiny.
> > > ping?
> > Will change!
> I looked around LLVM code and there are many accessors that use this 
> technique. Otherwise we need to enforce an exact SmallVector type as the 
> return code and we can't use 
> "llvm::SmallVectorImpl". I can change this to 
> return a "llvm::SmallVector" if needed? But that locks us 
> into a specific small vector with a specific size of preached items.
change "preached" to "preset" above...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137900

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