[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added a comment.

Thanks for adding me in the loop. And thanks for looking into the fix, though 
this bit is a bit confusing to me:

> Ensure that we explicitly indicate that we would like the move semantics
> in the assignment. With MSVC 14.35.32215, the assignment would not
> trigger a NVRO and copy constructor would be invoked

From https://llvm.org/doxygen/classllvm_1_1Error.html, `llvm::Error` has both 
copy constructor and copy assignment deleted, so I don't understand how a copy 
constructor could be invoked? I believe I followed 
https://llvm.org/docs/ProgrammersManual.html#recoverable-errors in handling the 
error, so if not having `std::move` there is really an issue, that page should 
also be updated.

But it is probably like @sgraenitz said, it is just an effect of the error not 
being consumed when logging is disabled.

> If logging is enabled, we are moving from the same object twice, no?



> In the if body you can not std::move() from err twice.

The doc comment of the move constructor/assignment does say "original error 
becomes a checked Success value, regardless of its original state" and it is 
exactly what happens in the code, so moving from the same error twice should be 
fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147669

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

[Lldb-commits] [PATCH] D143347: [lldb][DWARF] Infer no_unique_address attribute

[Pavel Kosov via Phabricator via lldb-commits]

kpdev42 updated this revision to Diff 511683.
kpdev42 edited the summary of this revision.
kpdev42 added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143347

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
  lldb/test/API/lang/cpp/no_unique_address/Makefile
  lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py
  lldb/test/API/lang/cpp/no_unique_address/main.cpp

Index: lldb/test/API/lang/cpp/no_unique_address/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/no_unique_address/main.cpp
@@ -0,0 +1,67 @@
+struct C
+{
+ long c,d;
+};
+
+struct Q
+{
+ long h;
+};
+
+struct D
+{
+};
+
+struct B
+{
+  [[no_unique_address]] D x;
+};
+
+struct E
+{
+  [[no_unique_address]] D x;
+};
+
+struct Foo1 : B,E,C
+{
+ long a = 42,b = 52;
+} _f1;
+
+struct Foo2 : B,E
+{
+ long v = 42;
+} _f2;
+
+struct Foo3 : C,B,E
+{
+ long v = 42;
+} _f3;
+
+struct Foo4 : B,C,E,Q
+{
+ long v = 42;
+} _f4;
+
+struct Foo5 : B,C,E
+{
+ [[no_unique_address]] D x1;
+ [[no_unique_address]] D x2;
+ long v1 = 42;
+ [[no_unique_address]] D y1;
+ [[no_unique_address]] D y2;
+ long v2 = 52;
+ [[no_unique_address]] D z1;
+ [[no_unique_address]] D z2;
+} _f5;
+
+struct Foo6 : B,E
+{
+ long v1 = 42;
+ [[no_unique_address]] D y1;
+ [[no_unique_address]] D y2;
+ long v2 = 52;
+} _f6;
+
+int main() {
+  return 0; // Set breakpoint here.
+}
Index: lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/no_unique_address/TestNoUniqueAddress.py
@@ -0,0 +1,28 @@
+"""
+Test that we correctly handle [[no_unique_address]] attribute.
+"""
+
+import lldb
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestInlineNamespace(TestBase):
+
+def test(self):
+self.build()
+
+lldbutil.run_to_source_breakpoint(self,
+"// Set breakpoint here.", lldb.SBFileSpec("main.cpp"))
+
+self.expect_expr("_f1.a", result_type="long", result_value="42")
+self.expect_expr("_f1.b", result_type="long", result_value="52")
+self.expect_expr("_f2.v", result_type="long", result_value="42")
+self.expect_expr("_f3.v", result_type="long", result_value="42")
+self.expect_expr("_f4.v", result_type="long", result_value="42")
+self.expect_expr("_f5.v1", result_type="long", result_value="42")
+self.expect_expr("_f5.v2", result_type="long", result_value="52")
+self.expect_expr("_f6.v1", result_type="long", result_value="42")
+self.expect_expr("_f6.v2", result_type="long", result_value="52")
Index: lldb/test/API/lang/cpp/no_unique_address/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/no_unique_address/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
@@ -223,6 +223,7 @@
 uint64_t bit_size = 0;
 uint64_t bit_offset = 0;
 bool is_bitfield = false;
+clang::FieldDecl *field_decl = nullptr;
 
 FieldInfo() = default;
 
@@ -275,6 +276,10 @@
   const ParsedDWARFTypeAttributes &attrs);
   lldb::TypeSP ParsePointerToMemberType(const DWARFDIE &die,
 const ParsedDWARFTypeAttributes &attrs);
+  void FixupBaseClasses(
+  std::vector> &base_classes,
+  const lldb_private::ClangASTImporter::LayoutInfo &layout_info,
+  long byte_offset);
 
   /// Parses a DW_TAG_inheritance DIE into a base/super class.
   ///
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1366,6 +1366,28 @@
   return nullptr;
 }
 
+void DWARFASTParserClang::FixupBaseClasses(
+std::vector> &base_classes,
+const ClangASTImporter::LayoutInfo &layout_info, long byte_offset) {
+  for (auto it = base_classes.rbegin(); it != base_classes.rend(); ++it) {
+clang::CXXRecordDecl *prev_base_decl =
+(*it)->getType()->getAsCXXRecordDecl();
+// We've already marked this class, exit.
+if (prev_base_decl->isEmpty())
+  break;
+auto it_layout = layout_info.base_offsets.find(prev_base_decl);
+if (it_layout == layout_info.base_offsets.end())
+  continue;
+// We found a normal base 

[Lldb-commits] [PATCH] D143347: [lldb][DWARF] Infer no_unique_address attribute

[Pavel Kosov via Phabricator via lldb-commits]

kpdev42 added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1483
+// base with all fields having [[no_unique_address]] attribute.
+for (auto it = base_classes.rbegin(); it != base_classes.rend(); ++it) {
+  clang::CXXRecordDecl *prev_base_decl =

Michael137 wrote:
> Michael137 wrote:
> > The main problem I still see with this is that if we have something like:
> > ```
> > struct A : C, B {
> > 
> > };
> > ```
> > 
> > then we mark `C`'s fields as empty and leave `B` as is. This still leads to 
> > the same crash later on.
> > 
> > Perhaps we should mark we could check the size of the struct and decide 
> > based on that which one is the "empty" one
> Interestingly there was a discussion on the DWARF mailing list about this 
> some time ago: 
> https://www.mail-archive.com/dwarf-discuss@lists.dwarfstd.org/msg00880.html
> 
> There might be room to changing the emitted DWARF to make it easier to 
> determine the empty structure. I will gauge opinions on this thread later 
> today
Unfortunately we cannot analyze record size, because it is always 1 for empty 
records, whether or not [[no_unique_address]] is used. However we still can 
analyze field offsets, I think. This what an updated patch does and it seems to 
handle more different cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143347

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

[Lldb-commits] [PATCH] D143347: [lldb][DWARF] Infer no_unique_address attribute

[Pavel Kosov via Phabricator via lldb-commits]

kpdev42 added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1483
+// base with all fields having [[no_unique_address]] attribute.
+for (auto it = base_classes.rbegin(); it != base_classes.rend(); ++it) {
+  clang::CXXRecordDecl *prev_base_decl =

kpdev42 wrote:
> Michael137 wrote:
> > Michael137 wrote:
> > > The main problem I still see with this is that if we have something like:
> > > ```
> > > struct A : C, B {
> > > 
> > > };
> > > ```
> > > 
> > > then we mark `C`'s fields as empty and leave `B` as is. This still leads 
> > > to the same crash later on.
> > > 
> > > Perhaps we should mark we could check the size of the struct and decide 
> > > based on that which one is the "empty" one
> > Interestingly there was a discussion on the DWARF mailing list about this 
> > some time ago: 
> > https://www.mail-archive.com/dwarf-discuss@lists.dwarfstd.org/msg00880.html
> > 
> > There might be room to changing the emitted DWARF to make it easier to 
> > determine the empty structure. I will gauge opinions on this thread later 
> > today
> Unfortunately we cannot analyze record size, because it is always 1 for empty 
> records, whether or not [[no_unique_address]] is used. However we still can 
> analyze field offsets, I think. This what an updated patch does and it seems 
> to handle more different cases
Yes, this will help a lot, however many people use older versions of clang 
compiler and also gcc. This fix might be useful for them so far


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143347

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

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

In D147669#4249968 , @sgraenitz wrote:

> First of all, yes the existing code is incorrect. Thanks for looking into 
> this. However, I am afraid this isn't the right solution.



> You probably mean `LLVM_ENABLE_ABI_BREAKING_CHECKS`? For `Error` this enables 
> logic to make sure the error was checked: 
> https://github.com/llvm/llvm-project/blob/release/16.x/llvm/include/llvm/Support/Error.h#L724
>  I think your observation is a side effect of the issue you look at and has 
> nothing to do with compiler optimizations like NRVO. If logging is off, the 
> error isn't consumed.

Yes, I did indeed mean checks and not changes.  I had originally tried with 
only the consumption in the non-logging case, but that still seemed to abort 
similarly.  Adding the `std::move` on the assignment for the conditional was 
also required for some reason.

I do wonder if we should keep the consume to be unconditional or not as 
@alvinhochun points out that `llvm::Error` does guarantee this behaviour.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147669

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

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added a comment.

In D147669#4251441 , @compnerd wrote:

> In D147669#4249968 , @sgraenitz 
> wrote:
>
>> First of all, yes the existing code is incorrect. Thanks for looking into 
>> this. However, I am afraid this isn't the right solution.
>
>
>
>> You probably mean `LLVM_ENABLE_ABI_BREAKING_CHECKS`? For `Error` this 
>> enables logic to make sure the error was checked: 
>> https://github.com/llvm/llvm-project/blob/release/16.x/llvm/include/llvm/Support/Error.h#L724
>>  I think your observation is a side effect of the issue you look at and has 
>> nothing to do with compiler optimizations like NRVO. If logging is off, the 
>> error isn't consumed.
>
> Yes, I did indeed mean checks and not changes.  I had originally tried with 
> only the consumption in the non-logging case, but that still seemed to abort 
> similarly.  Adding the `std::move` on the assignment for the conditional was 
> also required for some reason.

Hmm, now I'm wondering if I had run the wrong binary, because I do remember 
that I somehow ended up running the wrong binary once or twice.  Let's go with 
removing the confusing `std::move` in the `if` because that _shouldn't_ be 
needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147669

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

[Lldb-commits] [PATCH] D147669: PECOFF: consume errors properly

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd updated this revision to Diff 511706.
compnerd retitled this revision from "PECOFF: enforce move semantics and 
consume errors properly" to "PECOFF: consume errors properly".
compnerd edited the summary of this revision.

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

https://reviews.llvm.org/D147669

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp


Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -867,6 +867,7 @@
"ObjectFilePECOFF::AppendFromExportTable - failed to get export 
"
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));
   continue;
 }
 Symbol symbol;
@@ -888,6 +889,7 @@
  "ObjectFilePECOFF::AppendFromExportTable - failed to get "
  "forwarder name of forwarder export '{0}': {1}",
  sym_name, llvm::fmt_consume(std::move(err)));
+llvm::consumeError(std::move(err));
 continue;
   }
   llvm::SmallString<256> new_name = 
{symbol.GetDisplayName().GetStringRef(),
@@ -903,6 +905,7 @@
"ObjectFilePECOFF::AppendFromExportTable - failed to get "
"address of export entry '{0}': {1}",
sym_name, llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));
   continue;
 }
 // Skip the symbol if it doesn't look valid.


Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -867,6 +867,7 @@
"ObjectFilePECOFF::AppendFromExportTable - failed to get export "
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));
   continue;
 }
 Symbol symbol;
@@ -888,6 +889,7 @@
  "ObjectFilePECOFF::AppendFromExportTable - failed to get "
  "forwarder name of forwarder export '{0}': {1}",
  sym_name, llvm::fmt_consume(std::move(err)));
+llvm::consumeError(std::move(err));
 continue;
   }
   llvm::SmallString<256> new_name = {symbol.GetDisplayName().GetStringRef(),
@@ -903,6 +905,7 @@
"ObjectFilePECOFF::AppendFromExportTable - failed to get "
"address of export entry '{0}': {1}",
sym_name, llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));
   continue;
 }
 // Skip the symbol if it doesn't look valid.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147748: [lldb] Implement SymbolFile::ContainsCompileOption

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

At a higher level I wonder if this is really the best interface. If you ever 
need all the compile options, you probably want something like `Args 
SymbolFile::GetCompileOptions()`. Wouldn't that be a more generic way to do the 
same thing here? Or do we expect that for `DW_AT_APPLE_flags` the only possible 
use case is to check whether a particular flag is set?




Comment at: lldb/include/lldb/Symbol/SymbolFile.h:440
+  /// the symbol file.
+  virtual bool ContainsCompileOption(const char *option) {
+return false;

Please use `StringRef`s instead of raw char pointers.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:4271
+Args compiler_args(flags);
+for (auto &arg : compiler_args.GetArgumentArrayRef())
+  if (strcmp(arg, option) == 0)

It's not obvious from the function name what `GetArgumentArrayRef` is going to 
return. Looking at the header, it's apparently a `llvm::ArrayRef`, so `auto&` is a reference to a pointer?



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:4282-4285
+DWARFUnit *dwarf_cu = debug_info.GetUnitAtIndex(cu_idx);
+if (dwarf_cu) {
+  if (check_in_dwarf_cu(dwarf_cu))
+return true;




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147748

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

[Lldb-commits] [PATCH] D147748: [lldb] Implement SymbolFile::ContainsCompileOption

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

(adding myself as a reviewer so this shows up in my review queue)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147748

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

[Lldb-commits] [PATCH] D147748: [lldb] Implement SymbolFile::ContainsCompileOption

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 added a comment.

In D147748#4251543 , @JDevlieghere 
wrote:

> At a higher level I wonder if this is really the best interface. If you ever 
> need all the compile options, you probably want something like `Args 
> SymbolFile::GetCompileOptions()`. Wouldn't that be a more generic way to do 
> the same thing here? Or do we expect that for `DW_AT_APPLE_flags` the only 
> possible use case is to check whether a particular flag is set?

I agree. I implemented it in such a a way to mirror the `GetCompileOption` 
function (which apparently doesn't exist upstream anymore). I'll update it to 
return the `Args` instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147748

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

[Lldb-commits] [PATCH] D147753: Move "SelectMostRelevantFrame" from Thread::WillStop

[Med Ismail Bennani via Phabricator via lldb-commits]

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

This makes sense to me. LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147753

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

[Lldb-commits] [PATCH] D147801: [lldb] Add unittests for a few FileSpec methods

[Alex Langford via Phabricator via lldb-commits]

bulbazord created this revision.
bulbazord added reviewers: JDevlieghere, jingham, mib, clayborg.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This adds tests for:

- FileSpec::TestFileNameExtensions
- FileSpec::TestFileNameStrippingExtension
- FileSpec::IsSourceImplementationFile

This additionally updates incorrect documentation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147801

Files:
  lldb/include/lldb/Utility/FileSpec.h
  lldb/unittests/Utility/FileSpecTest.cpp


Index: lldb/unittests/Utility/FileSpecTest.cpp
===
--- lldb/unittests/Utility/FileSpecTest.cpp
+++ lldb/unittests/Utility/FileSpecTest.cpp
@@ -448,3 +448,59 @@
   file.PrependPathComponent("/tmp");
   EXPECT_TRUE(file.IsAbsolute());
 }
+
+TEST(FileSpecTest, TestFileNameExtensions) {
+  FileSpec dylib = PosixSpec("/tmp/foo.dylib");
+  FileSpec exe = PosixSpec("/tmp/foo");
+  FileSpec dSYM = PosixSpec("/tmp/foo.dSYM/");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(dylib.GetFileNameExtension() == ".dylib");
+  EXPECT_TRUE(exe.GetFileNameExtension() == ConstString(nullptr));
+  EXPECT_TRUE(dSYM.GetFileNameExtension() == ".dSYM");
+  EXPECT_TRUE(just_dot.GetFileNameExtension() == ".");
+
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+
+  EXPECT_TRUE(dll.GetFileNameExtension() == ".dll");
+  EXPECT_TRUE(win_noext.GetFileNameExtension() == ConstString(nullptr));
+}
+
+TEST(FileSpecTest, TestFileNameStrippingExtension) {
+  FileSpec dylib = PosixSpec("/tmp/foo.dylib");
+  FileSpec exe = PosixSpec("/tmp/foo");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(dylib.GetFileNameStrippingExtension() == "foo");
+  EXPECT_TRUE(exe.GetFileNameStrippingExtension() == "foo");
+  EXPECT_TRUE(just_dot.GetFileNameStrippingExtension() == "bar");
+
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+
+  EXPECT_TRUE(dll.GetFileNameStrippingExtension() == "foo");
+  EXPECT_TRUE(win_noext.GetFileNameStrippingExtension() == "foo");
+}
+
+TEST(FileSpecTest, TestIsSourceImplementationFile) {
+  FileSpec c_src = PosixSpec("/tmp/foo.c");
+  FileSpec txt_file = PosixSpec("/tmp/foo.txt");
+  FileSpec executable = PosixSpec("/tmp/foo");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(c_src.IsSourceImplementationFile());
+  EXPECT_FALSE(txt_file.IsSourceImplementationFile());
+  EXPECT_FALSE(executable.IsSourceImplementationFile());
+  EXPECT_FALSE(just_dot.IsSourceImplementationFile());
+
+  FileSpec cpp_src = WindowsSpec("C:\\tmp\\foo.cpp");
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+  FileSpec exe = WindowsSpec("C:\\tmp\\foo.exe");
+
+  EXPECT_TRUE(cpp_src.IsSourceImplementationFile());
+  EXPECT_FALSE(dll.IsSourceImplementationFile());
+  EXPECT_FALSE(win_noext.IsSourceImplementationFile());
+  EXPECT_FALSE(exe.IsSourceImplementationFile());
+}
Index: lldb/include/lldb/Utility/FileSpec.h
===
--- lldb/include/lldb/Utility/FileSpec.h
+++ lldb/include/lldb/Utility/FileSpec.h
@@ -253,7 +253,7 @@
   /// (files with a ".c", ".cpp", ".m", ".mm" (many more) extension).
   ///
   /// \return
-  /// \b true if the filespec represents an implementation source
+  /// \b true if the FileSpec represents an implementation source
   /// file, \b false otherwise.
   bool IsSourceImplementationFile() const;
 
@@ -327,7 +327,7 @@
   /// Returns a ConstString that represents the extension of the filename for
   /// this FileSpec object. If this object does not represent a file, or the
   /// filename has no extension, ConstString(nullptr) is returned. The dot
-  /// ('.') character is not returned as part of the extension
+  /// ('.') character is the first character in the returned string.
   ///
   /// \return Returns the extension of the file as a ConstString object.
   ConstString GetFileNameExtension() const;


Index: lldb/unittests/Utility/FileSpecTest.cpp
===
--- lldb/unittests/Utility/FileSpecTest.cpp
+++ lldb/unittests/Utility/FileSpecTest.cpp
@@ -448,3 +448,59 @@
   file.PrependPathComponent("/tmp");
   EXPECT_TRUE(file.IsAbsolute());
 }
+
+TEST(FileSpecTest, TestFileNameExtensions) {
+  FileSpec dylib = PosixSpec("/tmp/foo.dylib");
+  FileSpec exe = PosixSpec("/tmp/foo");
+  FileSpec dSYM = PosixSpec("/tmp/foo.dSYM/");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(dylib.GetFileNameExtension() == ".dylib");
+  EXPECT_TRUE(exe.GetFileNameExtension() == ConstString(nullptr));
+  EXPECT_TRUE(dSYM.GetFileNameExtension() == ".dSYM");
+  EXPECT_TRUE(just_dot.GetFileNameExtension() == ".");
+
+  FileSpec dll = WindowsSpec("C:\\t

[Lldb-commits] [PATCH] D147801: [lldb] Add unittests for a few FileSpec methods

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/include/lldb/Utility/FileSpec.h:330
   /// filename has no extension, ConstString(nullptr) is returned. The dot
-  /// ('.') character is not returned as part of the extension
+  /// ('.') character is the first character in the returned string.
   ///

This sounds like a pretty intrusive change ... was the comment wrong or did 
change the implementation on another commit ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147801

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

[Lldb-commits] [lldb] 730c8e1 - [lldb] Move "SelectMostRelevantFrame" from Thread::WillStop

[Jonas Devlieghere via lldb-commits]

Author: Jim Ingham
Date: 2023-04-07T12:21:00-07:00
New Revision: 730c8e160c9dead94ba534d5ad7cc8e47ce892db

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

LOG: [lldb] Move "SelectMostRelevantFrame" from Thread::WillStop

SelectMostRelevantFrame triggers the StackFrameRecognizer construction,
which can run arbitrary Python code, call expressions etc. WillStop gets
called on every private stop while the recognizers are a user-facing
feature, so first off doing this work on every stop is inefficient. But
more importantly, you can get in to situations where the recognizer
causes an expression to get run, then when we fetch the stop event at
the end of the expression evaluation, we call WillStop again on the
expression handling thread, which will do the same StackFrameRecognizer
work again. If anyone is locking along that path, you will end up with a
deadlock between the two threads.

The example that brought this to my attention was the
objc_exception_throw recognizer which can cause the objc runtime
introspection functions to get run, and those take a lock in
AppleObjCRuntimeV2::DynamicClassInfoExtractor::UpdateISAToDescriptorMap
along this path, so the second thread servicing the expression deadlocks
against the first thread waiting for the expression to complete.

It makes more sense to have the frame recognizers run on demand, either
when someone asks for the variables for the frame, or when someone does
GetSelectedFrame. The former already worked that way, the only reason
this was being done in WillStop was because the StackFrameRecognizers
can change the SelectedFrame, so you needed to run them before the
anyone requested the SelectedFrame.

This patch moves SelectMostRelevantFrame to StackFrameList, and runs it
when GetSelectedFrame is called for the first time on a given stop. If
you call SetSelectedFrame before GetSelectedFrame, then you should NOT
run the recognizer & change the frame out from under you. This patch
also makes that work. There were already tests for this behavior, and
for the feature that caused the hang, but the hang is racy, and it
doesn't trigger all the time, so I don't have a way to test that
explicitly.

One more detail: it's actually pretty easy to end up calling
GetSelectedFrame, for instance if you ask for the best ExecutionContext
from an ExecutionContextRef it will fill the StackFrame with the result
of GetSelectedFrame and that would still have the same problems if this
happens on the Private State Thread. So this patch also short-circuits
SelectMostRelevantFrame if run on the that thread. I can't think of any
reason the computations that go on on the Private State Thread would
actually want the SelectedFrame - that's a user-facing concept, so
avoiding that complication is the best way to go.

rdar://107643231

Differential revision: https://reviews.llvm.org/D147753

Added: 


Modified: 
lldb/include/lldb/Target/StackFrameList.h
lldb/include/lldb/Target/Thread.h
lldb/source/Target/StackFrameList.cpp
lldb/source/Target/Thread.cpp

Removed: 




diff  --git a/lldb/include/lldb/Target/StackFrameList.h 
b/lldb/include/lldb/Target/StackFrameList.h
index e05a398e3c0bc..ae998534d1f2c 100644
--- a/lldb/include/lldb/Target/StackFrameList.h
+++ b/lldb/include/lldb/Target/StackFrameList.h
@@ -46,7 +46,7 @@ class StackFrameList {
   uint32_t SetSelectedFrame(lldb_private::StackFrame *frame);
 
   /// Get the currently selected frame index.
-  uint32_t GetSelectedFrameIndex() const;
+  uint32_t GetSelectedFrameIndex();
 
   /// Mark a stack frame as the currently selected frame using the frame index
   /// \p idx. Like \ref GetFrameAtIndex, invisible frames cannot be selected.
@@ -110,6 +110,8 @@ class StackFrameList {
 
   void SetCurrentInlinedDepth(uint32_t new_depth);
 
+  void SelectMostRelevantFrame();
+
   typedef std::vector collection;
   typedef collection::iterator iterator;
   typedef collection::const_iterator const_iterator;
@@ -134,8 +136,10 @@ class StackFrameList {
   /// changes.
   collection m_frames;
 
-  /// The currently selected frame.
-  uint32_t m_selected_frame_idx;
+  /// The currently selected frame. An optional is used to record whether 
anyone
+  /// has set the selected frame on this stack yet. We only let recognizers
+  /// change the frame if this is the first time GetSelectedFrame is called.
+  std::optional m_selected_frame_idx;
 
   /// The number of concrete frames fetched while filling the frame list. This
   /// is only used when synthetic frames are enabled.

diff  --git a/lldb/include/lldb/Target/Thread.h 
b/lldb/include/lldb/Target/Thread.h
index efe82721a04e3..882f001e3af13 100644
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -217,8 +217,6 @@ class Thread : public 

[Lldb-commits] [PATCH] D147753: Move "SelectMostRelevantFrame" from Thread::WillStop

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG730c8e160c9d: [lldb] Move 
"SelectMostRelevantFrame" from Thread::WillStop (authored by jingham, 
committed by JDevlieghere).

Changed prior to commit:
  https://reviews.llvm.org/D147753?vs=511579&id=511768#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147753

Files:
  lldb/include/lldb/Target/StackFrameList.h
  lldb/include/lldb/Target/Thread.h
  lldb/source/Target/StackFrameList.cpp
  lldb/source/Target/Thread.cpp

Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -588,36 +588,9 @@
   return raw_stop_description;
 }
 
-void Thread::SelectMostRelevantFrame() {
-  Log *log = GetLog(LLDBLog::Thread);
-
-  auto frames_list_sp = GetStackFrameList();
-
-  // Only the top frame should be recognized.
-  auto frame_sp = frames_list_sp->GetFrameAtIndex(0);
-
-  auto recognized_frame_sp = frame_sp->GetRecognizedFrame();
-
-  if (!recognized_frame_sp) {
-LLDB_LOG(log, "Frame #0 not recognized");
-return;
-  }
-
-  if (StackFrameSP most_relevant_frame_sp =
-  recognized_frame_sp->GetMostRelevantFrame()) {
-LLDB_LOG(log, "Found most relevant frame at index {0}",
- most_relevant_frame_sp->GetFrameIndex());
-SetSelectedFrame(most_relevant_frame_sp.get());
-  } else {
-LLDB_LOG(log, "No relevant frame!");
-  }
-}
-
 void Thread::WillStop() {
   ThreadPlan *current_plan = GetCurrentPlan();
 
-  SelectMostRelevantFrame();
-
   // FIXME: I may decide to disallow threads with no plans.  In which
   // case this should go to an assert.
 
Index: lldb/source/Target/StackFrameList.cpp
===
--- lldb/source/Target/StackFrameList.cpp
+++ lldb/source/Target/StackFrameList.cpp
@@ -18,6 +18,7 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Target/RegisterContext.h"
 #include "lldb/Target/StackFrame.h"
+#include "lldb/Target/StackFrameRecognizer.h"
 #include "lldb/Target/StopInfo.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
@@ -38,7 +39,7 @@
const lldb::StackFrameListSP &prev_frames_sp,
bool show_inline_frames)
 : m_thread(thread), m_prev_frames_sp(prev_frames_sp), m_mutex(), m_frames(),
-  m_selected_frame_idx(0), m_concrete_frames_fetched(0),
+  m_selected_frame_idx(), m_concrete_frames_fetched(0),
   m_current_inlined_depth(UINT32_MAX),
   m_current_inlined_pc(LLDB_INVALID_ADDRESS),
   m_show_inlined_frames(show_inline_frames) {
@@ -252,7 +253,7 @@
 using CallSequence = std::vector;
 
 /// Find the unique path through the call graph from \p begin (with return PC
-/// \p return_pc) to \p end. On success this path is stored into \p path, and 
+/// \p return_pc) to \p end. On success this path is stored into \p path, and
 /// on failure \p path is unchanged.
 static void FindInterveningFrames(Function &begin, Function &end,
   ExecutionContext &exe_ctx, Target &target,
@@ -781,9 +782,46 @@
   return false; // resize failed, out of memory?
 }
 
-uint32_t StackFrameList::GetSelectedFrameIndex() const {
+void StackFrameList::SelectMostRelevantFrame() {
+  // Don't call into the frame recognizers on the private state thread as
+  // they can cause code to run in the target, and that can cause deadlocks
+  // when fetching stop events for the expression.
+  if (m_thread.GetProcess()->CurrentThreadIsPrivateStateThread())
+return;
+
+  Log *log = GetLog(LLDBLog::Thread);
+
+  // Only the top frame should be recognized.
+  StackFrameSP frame_sp = GetFrameAtIndex(0);
+  if (!frame_sp) {
+LLDB_LOG(log, "Failed to construct Frame #0");
+return;
+  }
+
+  RecognizedStackFrameSP recognized_frame_sp = frame_sp->GetRecognizedFrame();
+
+  if (!recognized_frame_sp) {
+LLDB_LOG(log, "Frame #0 not recognized");
+return;
+  }
+
+  if (StackFrameSP most_relevant_frame_sp =
+  recognized_frame_sp->GetMostRelevantFrame()) {
+LLDB_LOG(log, "Found most relevant frame at index {0}",
+ most_relevant_frame_sp->GetFrameIndex());
+SetSelectedFrame(most_relevant_frame_sp.get());
+  } else {
+LLDB_LOG(log, "No relevant frame!");
+  }
+}
+
+uint32_t StackFrameList::GetSelectedFrameIndex() {
   std::lock_guard guard(m_mutex);
-  return m_selected_frame_idx;
+  if (!m_selected_frame_idx)
+SelectMostRelevantFrame();
+  if (!m_selected_frame_idx)
+m_selected_frame_idx = 0;
+  return *m_selected_frame_idx;
 }
 
 uint32_t StackFrameList::SetSelectedFrame(lldb_private::StackFrame *frame) {
@@ -792,17 +830,19 @@
   const_iterator begin = m_frames.begin();
   const_iterator end = m_frames.end();
   m_selected_frame_idx = 0;
+
   for (pos = begin; pos != end; ++pos) 

[Lldb-commits] [PATCH] D147753: Move "SelectMostRelevantFrame" from Thread::WillStop

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Jim asked me to land this on his behalf as he's OOO. I've addressed my own 
comments :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147753

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

[Lldb-commits] [PATCH] D147801: [lldb] Add unittests for a few FileSpec methods

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/include/lldb/Utility/FileSpec.h:330
   /// filename has no extension, ConstString(nullptr) is returned. The dot
-  /// ('.') character is not returned as part of the extension
+  /// ('.') character is the first character in the returned string.
   ///

mib wrote:
> This sounds like a pretty intrusive change ... was the comment wrong or did 
> change the implementation on another commit ?
I changed the behavior in ad8d48f9036f9 but forgot to update the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147801

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

[Lldb-commits] [PATCH] D147801: [lldb] Add unittests for a few FileSpec methods

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere 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/D147801/new/

https://reviews.llvm.org/D147801

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

[Lldb-commits] [PATCH] D147801: [lldb] Add unittests for a few FileSpec methods

[Med Ismail Bennani via Phabricator via lldb-commits]

mib accepted this revision.
mib added inline comments.



Comment at: lldb/include/lldb/Utility/FileSpec.h:330
   /// filename has no extension, ConstString(nullptr) is returned. The dot
-  /// ('.') character is not returned as part of the extension
+  /// ('.') character is the first character in the returned string.
   ///

JDevlieghere wrote:
> mib wrote:
> > This sounds like a pretty intrusive change ... was the comment wrong or did 
> > change the implementation on another commit ?
> I changed the behavior in ad8d48f9036f9 but forgot to update the comment.
LGTM then :) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147801

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

[Lldb-commits] [PATCH] D147805: [lldb-vscode] Fix two issues with runInTerminal test.

[Jorge Gorbe Moya via Phabricator via lldb-commits]

jgorbe created this revision.
jgorbe added reviewers: clayborg, wallace.
jgorbe added a project: LLDB.
Herald added subscribers: JDevlieghere, krytarowski.
Herald added a project: All.
jgorbe requested review of this revision.

With ptrace_scope = 1 the kernel only allows tracing descendants of a process.
When using runInTerminal, the target process is not launched by the debugger,
so we need to modify `LaunchRunInTerminal` to explicitly allow tracing.
This should fix a problem reported in https://reviews.llvm.org/D84974#3903716

Also, remove a special case from the `launch` method in the lldbvscode_testcase
test harness. The existing test was using stopOnEntry, so the first stop didn't
happen at the expected breakpoint unless the harness did configurationDone
first.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147805

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
  lldb/test/API/tools/lldb-vscode/runInTerminal/TestVSCode_runInTerminal.py
  lldb/tools/lldb-vscode/lldb-vscode.cpp


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -32,6 +32,10 @@
 #include 
 #endif
 
+#if defined(__linux__)
+#include 
+#endif
+
 #include 
 #include 
 #include 
@@ -3146,6 +3150,14 @@
   llvm::errs() << "runInTerminal is only supported on POSIX systems\n";
   exit(EXIT_FAILURE);
 #else
+
+  // On Linux with the Yama security module enabled, a process can only attach
+  // to its descendants by default. In the runInTerminal case the target
+  // process is launched by the client so we need to allow tracing explicitly.
+#if defined(__linux__)
+  (void)prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, 0, 0, 0);
+#endif
+
   RunInTerminalLauncherCommChannel comm_channel(comm_file);
   if (llvm::Error err = comm_channel.NotifyPid()) {
 llvm::errs() << llvm::toString(std::move(err)) << "\n";
Index: lldb/test/API/tools/lldb-vscode/runInTerminal/TestVSCode_runInTerminal.py
===
--- lldb/test/API/tools/lldb-vscode/runInTerminal/TestVSCode_runInTerminal.py
+++ lldb/test/API/tools/lldb-vscode/runInTerminal/TestVSCode_runInTerminal.py
@@ -57,8 +57,7 @@
 program = self.getBuildArtifact("a.out")
 source = 'main.c'
 self.build_and_launch(
-program, stopOnEntry=True, runInTerminal=True, args=["foobar"],
-env=["FOO=bar"])
+program, runInTerminal=True, args=["foobar"], env=["FOO=bar"])
 
 breakpoint_line = line_number(source, '// breakpoint')
 
@@ -88,7 +87,7 @@
 return
 self.build_and_create_debug_adaptor()
 response = self.launch(
-"INVALIDPROGRAM", stopOnEntry=True, runInTerminal=True, 
args=["foobar"], env=["FOO=bar"], expectFailure=True)
+"INVALIDPROGRAM", runInTerminal=True, args=["foobar"], 
env=["FOO=bar"], expectFailure=True)
 self.assertFalse(response['success'])
 self.assertIn("Could not create a target for a program 
'INVALIDPROGRAM': unable to find executable",
 response['message'])
Index: 
lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
@@ -331,10 +331,6 @@
 if not (response and response['success']):
 self.assertTrue(response['success'],
 'launch failed (%s)' % (response['message']))
-# We need to trigger a request_configurationDone after we've 
successfully
-# attached a runInTerminal process to finish initialization.
-if runInTerminal:
-self.vscode.request_configurationDone()
 return response
 
 


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -32,6 +32,10 @@
 #include 
 #endif
 
+#if defined(__linux__)
+#include 
+#endif
+
 #include 
 #include 
 #include 
@@ -3146,6 +3150,14 @@
   llvm::errs() << "runInTerminal is only supported on POSIX systems\n";
   exit(EXIT_FAILURE);
 #else
+
+  // On Linux with the Yama security module enabled, a process can only attach
+  // to its descendants by default. In the runInTerminal case the target
+  // process is launched by the client so we need to allow tracing explicitly.
+#if defined(__linux__)
+  (void)prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, 0, 0, 0);
+#endif
+
   RunInTerminalLauncherCommChannel comm_channel(comm_file);
   if (llvm::Error err = comm_channel.NotifyPid()) {
 llvm::errs() << llvm::toString(std::move(err)) << "\n";
Index: lldb/test/API/tools/lldb-vscode/r

[Lldb-commits] [PATCH] D147805: [lldb-vscode] Fix two issues with runInTerminal test.

[Jordan Rupprecht via Phabricator via lldb-commits]

rupprecht added inline comments.



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3158
+#if defined(__linux__)
+  (void)prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, 0, 0, 0);
+#endif

https://manpages.debian.org/bullseye/manpages-dev/prctl.2.en.html#PR_SET_PTRACER

It would be nice if we could avoid `PR_SET_PTRACER_ANY` and set the pid 
directly. (Maybe you will need an additional flag, along with --comm-file and 
--launch-target?). Using `PR_SET_PTRACER_ANY` effectively circumvents the 
system's ptrace policy, IIUC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147805

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

[Lldb-commits] [lldb] de5f96e - [lldb] Add unittests for a few FileSpec methods

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2023-04-07T13:50:27-07:00
New Revision: de5f96e99aedd641cc0bbb9ae4a156db4ae3c4c4

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

LOG: [lldb] Add unittests for a few FileSpec methods

This adds tests for:
- FileSpec::TestFileNameExtensions
- FileSpec::TestFileNameStrippingExtension
- FileSpec::IsSourceImplementationFile

This additionally updates incorrect documentation.

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

Added: 


Modified: 
lldb/include/lldb/Utility/FileSpec.h
lldb/unittests/Utility/FileSpecTest.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/FileSpec.h 
b/lldb/include/lldb/Utility/FileSpec.h
index e4d3d12979c2..08fd1017eccd 100644
--- a/lldb/include/lldb/Utility/FileSpec.h
+++ b/lldb/include/lldb/Utility/FileSpec.h
@@ -253,7 +253,7 @@ class FileSpec {
   /// (files with a ".c", ".cpp", ".m", ".mm" (many more) extension).
   ///
   /// \return
-  /// \b true if the filespec represents an implementation source
+  /// \b true if the FileSpec represents an implementation source
   /// file, \b false otherwise.
   bool IsSourceImplementationFile() const;
 
@@ -327,7 +327,7 @@ class FileSpec {
   /// Returns a ConstString that represents the extension of the filename for
   /// this FileSpec object. If this object does not represent a file, or the
   /// filename has no extension, ConstString(nullptr) is returned. The dot
-  /// ('.') character is not returned as part of the extension
+  /// ('.') character is the first character in the returned string.
   ///
   /// \return Returns the extension of the file as a ConstString object.
   ConstString GetFileNameExtension() const;

diff  --git a/lldb/unittests/Utility/FileSpecTest.cpp 
b/lldb/unittests/Utility/FileSpecTest.cpp
index 76781246322c..96f19953c3b7 100644
--- a/lldb/unittests/Utility/FileSpecTest.cpp
+++ b/lldb/unittests/Utility/FileSpecTest.cpp
@@ -448,3 +448,59 @@ TEST(FileSpecTest, TestAbsoluteCaching) {
   file.PrependPathComponent("/tmp");
   EXPECT_TRUE(file.IsAbsolute());
 }
+
+TEST(FileSpecTest, TestFileNameExtensions) {
+  FileSpec dylib = PosixSpec("/tmp/foo.dylib");
+  FileSpec exe = PosixSpec("/tmp/foo");
+  FileSpec dSYM = PosixSpec("/tmp/foo.dSYM/");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(dylib.GetFileNameExtension() == ".dylib");
+  EXPECT_TRUE(exe.GetFileNameExtension() == ConstString(nullptr));
+  EXPECT_TRUE(dSYM.GetFileNameExtension() == ".dSYM");
+  EXPECT_TRUE(just_dot.GetFileNameExtension() == ".");
+
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+
+  EXPECT_TRUE(dll.GetFileNameExtension() == ".dll");
+  EXPECT_TRUE(win_noext.GetFileNameExtension() == ConstString(nullptr));
+}
+
+TEST(FileSpecTest, TestFileNameStrippingExtension) {
+  FileSpec dylib = PosixSpec("/tmp/foo.dylib");
+  FileSpec exe = PosixSpec("/tmp/foo");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(dylib.GetFileNameStrippingExtension() == "foo");
+  EXPECT_TRUE(exe.GetFileNameStrippingExtension() == "foo");
+  EXPECT_TRUE(just_dot.GetFileNameStrippingExtension() == "bar");
+
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+
+  EXPECT_TRUE(dll.GetFileNameStrippingExtension() == "foo");
+  EXPECT_TRUE(win_noext.GetFileNameStrippingExtension() == "foo");
+}
+
+TEST(FileSpecTest, TestIsSourceImplementationFile) {
+  FileSpec c_src = PosixSpec("/tmp/foo.c");
+  FileSpec txt_file = PosixSpec("/tmp/foo.txt");
+  FileSpec executable = PosixSpec("/tmp/foo");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(c_src.IsSourceImplementationFile());
+  EXPECT_FALSE(txt_file.IsSourceImplementationFile());
+  EXPECT_FALSE(executable.IsSourceImplementationFile());
+  EXPECT_FALSE(just_dot.IsSourceImplementationFile());
+
+  FileSpec cpp_src = WindowsSpec("C:\\tmp\\foo.cpp");
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+  FileSpec exe = WindowsSpec("C:\\tmp\\foo.exe");
+
+  EXPECT_TRUE(cpp_src.IsSourceImplementationFile());
+  EXPECT_FALSE(dll.IsSourceImplementationFile());
+  EXPECT_FALSE(win_noext.IsSourceImplementationFile());
+  EXPECT_FALSE(exe.IsSourceImplementationFile());
+}



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

[Lldb-commits] [PATCH] D147801: [lldb] Add unittests for a few FileSpec methods

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGde5f96e99aed: [lldb] Add unittests for a few FileSpec 
methods (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147801

Files:
  lldb/include/lldb/Utility/FileSpec.h
  lldb/unittests/Utility/FileSpecTest.cpp


Index: lldb/unittests/Utility/FileSpecTest.cpp
===
--- lldb/unittests/Utility/FileSpecTest.cpp
+++ lldb/unittests/Utility/FileSpecTest.cpp
@@ -448,3 +448,59 @@
   file.PrependPathComponent("/tmp");
   EXPECT_TRUE(file.IsAbsolute());
 }
+
+TEST(FileSpecTest, TestFileNameExtensions) {
+  FileSpec dylib = PosixSpec("/tmp/foo.dylib");
+  FileSpec exe = PosixSpec("/tmp/foo");
+  FileSpec dSYM = PosixSpec("/tmp/foo.dSYM/");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(dylib.GetFileNameExtension() == ".dylib");
+  EXPECT_TRUE(exe.GetFileNameExtension() == ConstString(nullptr));
+  EXPECT_TRUE(dSYM.GetFileNameExtension() == ".dSYM");
+  EXPECT_TRUE(just_dot.GetFileNameExtension() == ".");
+
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+
+  EXPECT_TRUE(dll.GetFileNameExtension() == ".dll");
+  EXPECT_TRUE(win_noext.GetFileNameExtension() == ConstString(nullptr));
+}
+
+TEST(FileSpecTest, TestFileNameStrippingExtension) {
+  FileSpec dylib = PosixSpec("/tmp/foo.dylib");
+  FileSpec exe = PosixSpec("/tmp/foo");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(dylib.GetFileNameStrippingExtension() == "foo");
+  EXPECT_TRUE(exe.GetFileNameStrippingExtension() == "foo");
+  EXPECT_TRUE(just_dot.GetFileNameStrippingExtension() == "bar");
+
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+
+  EXPECT_TRUE(dll.GetFileNameStrippingExtension() == "foo");
+  EXPECT_TRUE(win_noext.GetFileNameStrippingExtension() == "foo");
+}
+
+TEST(FileSpecTest, TestIsSourceImplementationFile) {
+  FileSpec c_src = PosixSpec("/tmp/foo.c");
+  FileSpec txt_file = PosixSpec("/tmp/foo.txt");
+  FileSpec executable = PosixSpec("/tmp/foo");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(c_src.IsSourceImplementationFile());
+  EXPECT_FALSE(txt_file.IsSourceImplementationFile());
+  EXPECT_FALSE(executable.IsSourceImplementationFile());
+  EXPECT_FALSE(just_dot.IsSourceImplementationFile());
+
+  FileSpec cpp_src = WindowsSpec("C:\\tmp\\foo.cpp");
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+  FileSpec exe = WindowsSpec("C:\\tmp\\foo.exe");
+
+  EXPECT_TRUE(cpp_src.IsSourceImplementationFile());
+  EXPECT_FALSE(dll.IsSourceImplementationFile());
+  EXPECT_FALSE(win_noext.IsSourceImplementationFile());
+  EXPECT_FALSE(exe.IsSourceImplementationFile());
+}
Index: lldb/include/lldb/Utility/FileSpec.h
===
--- lldb/include/lldb/Utility/FileSpec.h
+++ lldb/include/lldb/Utility/FileSpec.h
@@ -253,7 +253,7 @@
   /// (files with a ".c", ".cpp", ".m", ".mm" (many more) extension).
   ///
   /// \return
-  /// \b true if the filespec represents an implementation source
+  /// \b true if the FileSpec represents an implementation source
   /// file, \b false otherwise.
   bool IsSourceImplementationFile() const;
 
@@ -327,7 +327,7 @@
   /// Returns a ConstString that represents the extension of the filename for
   /// this FileSpec object. If this object does not represent a file, or the
   /// filename has no extension, ConstString(nullptr) is returned. The dot
-  /// ('.') character is not returned as part of the extension
+  /// ('.') character is the first character in the returned string.
   ///
   /// \return Returns the extension of the file as a ConstString object.
   ConstString GetFileNameExtension() const;


Index: lldb/unittests/Utility/FileSpecTest.cpp
===
--- lldb/unittests/Utility/FileSpecTest.cpp
+++ lldb/unittests/Utility/FileSpecTest.cpp
@@ -448,3 +448,59 @@
   file.PrependPathComponent("/tmp");
   EXPECT_TRUE(file.IsAbsolute());
 }
+
+TEST(FileSpecTest, TestFileNameExtensions) {
+  FileSpec dylib = PosixSpec("/tmp/foo.dylib");
+  FileSpec exe = PosixSpec("/tmp/foo");
+  FileSpec dSYM = PosixSpec("/tmp/foo.dSYM/");
+  FileSpec just_dot = PosixSpec("/tmp/bar.");
+
+  EXPECT_TRUE(dylib.GetFileNameExtension() == ".dylib");
+  EXPECT_TRUE(exe.GetFileNameExtension() == ConstString(nullptr));
+  EXPECT_TRUE(dSYM.GetFileNameExtension() == ".dSYM");
+  EXPECT_TRUE(just_dot.GetFileNameExtension() == ".");
+
+  FileSpec dll = WindowsSpec("C:\\tmp\\foo.dll");
+  FileSpec win_noext = WindowsSpec("C:\\tmp\\foo");
+
+  EXPECT_TRUE(dll.GetFileNameExtension() == ".dll");
+  EXPECT_TRUE(win_noext.GetFileNameExtension() == ConstString(n

[Lldb-commits] [PATCH] D147805: [lldb-vscode] Fix two issues with runInTerminal test.

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

wow, nice improvement. I don't have anything else to add besides what 
@rupprecht said


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147805

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

[Lldb-commits] [PATCH] D146765: [lldb/crashlog] Load inlined symbol into interactive crashlog

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 511805.
mib marked 7 inline comments as done and an inline comment as not done.
mib added a comment.

Address some of the comments.


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

https://reviews.llvm.org/D146765

Files:
  lldb/examples/python/crashlog.py
  lldb/include/lldb/API/SBModuleSpec.h
  lldb/include/lldb/API/SBTarget.h
  lldb/source/API/SBModuleSpec.cpp
  lldb/source/API/SBTarget.cpp

Index: lldb/source/API/SBTarget.cpp
===
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -1555,6 +1555,19 @@
   return sb_module;
 }
 
+SBModule SBTarget::FindModule(const SBModuleSpec &sb_module_spec) {
+  LLDB_INSTRUMENT_VA(this, sb_module_spec);
+
+  SBModule sb_module;
+  TargetSP target_sp(GetSP());
+  if (target_sp && sb_module_spec.IsValid()) {
+// The module list is thread safe, no need to lock
+sb_module.SetSP(
+target_sp->GetImages().FindFirstModule(*sb_module_spec.m_opaque_up));
+  }
+  return sb_module;
+}
+
 SBSymbolContextList SBTarget::FindCompileUnits(const SBFileSpec &sb_file_spec) {
   LLDB_INSTRUMENT_VA(this, sb_file_spec);
 
Index: lldb/source/API/SBModuleSpec.cpp
===
--- lldb/source/API/SBModuleSpec.cpp
+++ lldb/source/API/SBModuleSpec.cpp
@@ -132,6 +132,13 @@
   return m_opaque_up->GetUUID().GetBytes().size();
 }
 
+bool SBModuleSpec::SetUUIDFromString(const char *uuid) {
+  LLDB_INSTRUMENT_VA(this, uuid)
+
+  m_opaque_up->GetUUID().SetFromStringRef(llvm::StringRef(uuid));
+  return m_opaque_up->GetUUID().IsValid();
+}
+
 bool SBModuleSpec::SetUUIDBytes(const uint8_t *uuid, size_t uuid_len) {
   LLDB_INSTRUMENT_VA(this, uuid, uuid_len)
   m_opaque_up->GetUUID() = UUID(uuid, uuid_len);
Index: lldb/include/lldb/API/SBTarget.h
===
--- lldb/include/lldb/API/SBTarget.h
+++ lldb/include/lldb/API/SBTarget.h
@@ -305,6 +305,8 @@
 
   lldb::SBModule FindModule(const lldb::SBFileSpec &file_spec);
 
+  lldb::SBModule FindModule(const lldb::SBModuleSpec &module_spec);
+
   /// Find compile units related to *this target and passed source
   /// file.
   ///
Index: lldb/include/lldb/API/SBModuleSpec.h
===
--- lldb/include/lldb/API/SBModuleSpec.h
+++ lldb/include/lldb/API/SBModuleSpec.h
@@ -75,6 +75,8 @@
 
   size_t GetUUIDLength();
 
+  bool SetUUIDFromString(const char *uuid);
+
   bool SetUUIDBytes(const uint8_t *uuid, size_t uuid_len);
 
   bool GetDescription(lldb::SBStream &description);
Index: lldb/examples/python/crashlog.py
===
--- lldb/examples/python/crashlog.py
+++ lldb/examples/python/crashlog.py
@@ -40,6 +40,7 @@
 import string
 import subprocess
 import sys
+import tempfile
 import threading
 import time
 import uuid
@@ -431,6 +432,7 @@
 self.path = os.path.expanduser(path)
 self.verbose = verbose
 self.crashlog = CrashLog(debugger, self.path, self.verbose)
+self.symbol_data = {}
 
 @abc.abstractmethod
 def parse(self):
@@ -536,8 +538,22 @@
 self.crashlog.idents.append(ident)
 
 frame_offset = int(json_frame['imageOffset'])
-image_addr = self.get_used_image(image_id)['base']
-pc = image_addr + frame_offset
+pc = json_image['base'] + frame_offset
+
+if 'symbol' in json_frame and pc != 0:
+image_uuid = json_image['uuid']
+if not image_uuid in self.symbol_data:
+self.symbol_data[image_uuid] = {
+"symbols" : list(),
+"uuid": image_uuid,
+"triple": None
+}
+self.symbol_data[image_uuid]["symbols"].append({
+"name": json_frame['symbol'],
+"type": "code",
+"size": 0,
+"address": pc - frame_offset,
+})
 thread.frames.append(self.crashlog.Frame(idx, pc, frame_offset))
 
 # on arm64 systems, if it jump through a null function pointer,
@@ -586,8 +602,12 @@
 print("error: can't parse application specific backtrace.")
 return False
 
-(frame_id, frame_img_name, frame_addr,
-frame_ofs) = frame_match.groups()
+if len(frame_match.groups()) == 4:
+(frame_id, frame_img_name, frame_addr,
+frame_ofs) = frame_match.groups()
+else:
+(frame_id, frame_img_name, frame_addr,
+frame_ofs, frame_symbol, frame_offset) = frame_match.groups()
 
 thread.add_ident(frame_img_name)
 if frame_img_name not in self.crashlog.idents:
@@ -641,11 +661,11 @@
 thread_re

[Lldb-commits] [PATCH] D147816: Clarify how watchpoint description in stop packets work, fix AArch64 unintended behavior

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added reviewers: labath, omjavaid, DavidSpickett.
jasonmolenda added a project: LLDB.
Herald added subscribers: JDevlieghere, atanasyan, kristof.beyls, arichardson, 
sdardis.
Herald added a project: All.
jasonmolenda requested review of this revision.
Herald added a subscriber: lldb-commits.

Orig posted in https://reviews.llvm.org/D147674 as part of a debugserver patch, 
creating two phabs to track those separate patches so it's easier to review.

This patch fixes a problem with watchpoints on AArch targets using lldb-server 
(and soon debugserver), where a large write that begins before a watched 
region, and extends into the watched region, is reported as a trap address 
before the watched memory range.  I can show this on an AArch64 Ubuntu install:

  {
 uint8_t buf[8];
 uint64_t *u64_p = (uint64_t *) buf;
 *u64_p = 5; // break here
 (*u64_p)++;

At the `break here` line, if and do `watch set variable buf[2]` and `c`, the 
packets from lldb server look like

  <  21> send packet: $Z2,f36a,1#75
  <   6> read packet: $OK#9a
  <   5> send packet: $c#63
  
  <838> read packet: $T05thread:493;name:a.out;  [...] 
;reason:watchpoint;description:323831343734393736373037343334203320323831343734393736373037343332;#db

The `description` is `281474976707434 3 281474976707432` aka 
`0xf36a 3 0xf368`.  The first address is an address 
contained within the hit watchpoint.  The second number is the hardware 
register index.  The third number is the actual address that was accessed,

  (lldb)  v &buf u64_p
  (uint8_t (*)[8]) &buf = 0xf368
  (uint64_t *) u64_p = 0xf368
  (lldb) 

Right now, lldb has a rule that "for MIPS an ARM, if the third number is NOT 
contained within any watchpoint region, silently continue past this 
watchpoint".  From the user's perspective, lldb misses the watchpoint 
modification.  We see this a lot on macOS with bzero()/memset() when you're 
watching an element of an object and it is overwritten.

For MIPS this behavior is correct because any access to an 8B granule triggers 
a watchpoint in that granule.  If I watch 4 bytes at 0x1004, and someone 
accesses 1 byte at 0x1002, a watchpoint will trigger.  Jaydeep Patil in 2015 ( 
https://reviews.llvm.org/D11672 ) added this third argument and the "silently 
step over it and don't tell the user" behavior; that patch decodes the 
instruction that caused the trap to determine the actual address that was 
accessed.

However, the intended use/meaning of this third field was very confusing (to 
me), and in 2016 Omair  ( https://reviews.llvm.org/D21516 ) enabled the same 
behavior on ARM targets and reported the actual accessed address.  I'm not 
exactly sure what issue motivated this, it may have simply been a 
misunderstanding of how StopInfo decodes and uses these three addresses.  If 
nothing is done on ARM targets and an access outside a watched region, lldb 
stops execution and doesn't know how to disable the correct watchpoint & 
advance execution, my guess is that he was addressing that issue.

As the reason:watchpoint's `description` value has grown from "wp addr + hw reg 
index" to "wp addr + hw reg index + actual trap address", with implicit 
behavior on MIPS that an actual trap address silently continues if it's outside 
the range of a watchpoint, this is not the best. We should stop using 
`description` and add 2-4 keys in the stop info packet, e.g. `wp-address:`, 
`wp-reg-index:`, `wp-hit-address:`, `silently-continue:` that can be provided 
as needed.  We can remove the "on MIPS do this" behavior from generic lldb -- 
the stub is probably in the best position to know if this is a false watchpoint 
tigger that should be ignored, and it can tell lldb.  Looking at the latest ARM 
ARM A-Profile docs, this seems like a situation that can exist in some modes as 
well, we may need to flag this from AArch64 debugserver/lldb-server in the 
future.

We don't currently have MIPS supported in lldb - it was removed c. 2021 in 
anticipation of someone starting to actively maintain it again.  But this 
"silently continue" behavior may be needed for AArch64, and it may be needed 
again for MIPS if that target is revived.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147816

Files:
  lldb/docs/lldb-gdb-remote.txt
  lldb/include/lldb/Target/StopInfo.h
  lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Target/StopInfo.cpp

Index: lldb/source/Target/StopInfo.cpp
===
--- lldb/source/Target/StopInfo.cpp
+++ lldb/source/Target/StopInfo.cpp
@@ -666,9 +666,8 @@
 WatchpointSP watchpoint_sp;
   };
 
-  StopInfoWatchpoint(Thread &thread, break_id_t watch_id,
- lldb::addr_t watch_hit_addr)
-  : StopInfo(thread, watch_id), m_watch_hit_addr(w

[Lldb-commits] [PATCH] D146765: [lldb/crashlog] Load inlined symbol into interactive crashlog

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/examples/python/crashlog.py:669
+ r'(0x[0-9a-fA-F]{4,}) +'   # addr (4 
chars or more)
+ r'((.*)(?:(?: +\+ +)([0-9]+))|[^\s]+)' # symbol + 
offset
 )

kastiglione wrote:
> mib wrote:
> > kastiglione wrote:
> > > mib wrote:
> > > > @kastiglione may be you have a better idea how to handle `symbol + 
> > > > offset` where ` + offset` might be optional.
> > > symbol is always present?
> > Technically, in the past the `offs` (symbol + offset) was optional:
> > ```
> > r'(?: +(.*))?'
> > ```
> > So I guess `symbol` could be missing.
> Breaking it down, there is:
> 
> 1. a symbol (maybe)
> 2. the literal text " + " followed by a number (also maybe)
> 
> I'll start start with 2:
> 
> ```
>  \+ (\d+)
> ```
> 
> For the symbol, `.*`, it should instead have at least a length of 1. I'd use 
> `+` instead of `*`. And, it shouldn't be _any_ character. At the very least 
> it should be non-space, which is `\S`.
> 
> To combine these at a high level it looks like:
> 
> ```
> (?:()(?:)?)?
> ```
> 
> Filling in these two with symbol='\S+' and offset=' \+ (\d+)', it becomes:
> 
> ```
> (?:(\S+)(?: \+ (\d+))?)?
> ```
> 
> Here's some python real session that minimally exercises this code:
> 
> ```
> % python
> >>> import re
> >>> pat = re.compile(r"(?:(\S+)(?: \+ (\d+))?)?")
> >>> pat.match("func + 123").groups()
> ('func', '123')
> >>> pat.match("func").groups()
> ('func', None)
> >>> pat.match("").groups()
> (None, None)
> >>> 
> ```
```
(?:(?: +\+ +)([0-9]+))
```

Non-capturing groups are only needed when you use a quantifier, like `?`, `*`, 
or `+`. These regex has two non-capturing groups that don't use a quantifier, 
which means you can remove the groups:

```
 +\+ +([0-9]+)
```


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

https://reviews.llvm.org/D146765

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

[Lldb-commits] [PATCH] D147820: debugserver: move AArch64 watchpoint traps within a watchpointed region, parse ESR flags and send them to lldb

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added a reviewer: JDevlieghere.
jasonmolenda added a project: LLDB.
Herald added subscribers: omjavaid, atanasyan, kristof.beyls, arichardson, 
sdardis.
Herald added a project: All.
jasonmolenda requested review of this revision.
Herald added a subscriber: lldb-commits.

Currently on Darwin arm64 systems using debugserver, if you watch a memory 
region and a large write starts before the watched region, and extends in to 
the watched region, we will have a watchpoint exception but the address 
reported may be the start of the access -- before the watched memory range.  In 
this case, lldb will not recognize which watchpoint to disable to step past the 
watchpoint and execution will stop, leaving the user to disable/stepi/re-enable 
manually.

This patch takes the trap address (FAR register) and finds the nearest 
watchpoint if it is not contained in any watched region.  It also parses the 
ESR register flags and if the processor reported the watchpoint index number 
instead of an address in the FAR register, handle that.  Send (1) an address 
within the watched mem range, (2) the watchpoint hardware index, and (3) the 
actual trap address which may exist outside a watched mem range to lldb in a 
`description` string in the stop packet.

Add a test case that has a uint8_t[8] array, watches a one-byte element in that 
array, and then does a 64-bit write to the entire array - so our FAR address 
may be the start of the uint8_t[8] array, and confirm that lldb correctly 
associates this with the watchpoint we set.

This patch depends on https://reviews.llvm.org/D147816 ("Clarify how watchpoint 
description in stop packets work, fix AArch64 unintended behavior") being 
present.  Without that patch, when we have a trap address outside the range of 
all watched memory ranges, lldb will silently continue past this watchpoint.  
Correct behavior on MIPS targets, but not on AArch64.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147820

Files:
  lldb/test/API/commands/watchpoints/unaligned-watchpoint/Makefile
  
lldb/test/API/commands/watchpoints/unaligned-watchpoint/TestUnalignedWatchpoint.py
  lldb/test/API/commands/watchpoints/unaligned-watchpoint/main.c
  lldb/tools/debugserver/source/DNBBreakpoint.cpp
  lldb/tools/debugserver/source/DNBBreakpoint.h
  lldb/tools/debugserver/source/DNBDefs.h
  lldb/tools/debugserver/source/MacOSX/MachException.cpp
  lldb/tools/debugserver/source/MacOSX/MachProcess.h
  lldb/tools/debugserver/source/MacOSX/MachProcess.mm
  lldb/tools/debugserver/source/RNBRemote.cpp

Index: lldb/tools/debugserver/source/RNBRemote.cpp
===
--- lldb/tools/debugserver/source/RNBRemote.cpp
+++ lldb/tools/debugserver/source/RNBRemote.cpp
@@ -2841,11 +2841,21 @@
   InitializeRegisters();
 
 if (g_reg_entries != NULL) {
+  auto interesting_regset = [](int regset) -> bool {
+#if defined(__arm64__) || defined(__aarch64__)
+// GPRs and exception registers, helpful for debugging
+// from packet logs.
+return regset == 1 || regset == 3;
+#else
+return regset == 1;
+#endif
+  };
+
   DNBRegisterValue reg_value;
   for (uint32_t reg = 0; reg < g_num_reg_entries; reg++) {
 // Expedite all registers in the first register set that aren't
 // contained in other registers
-if (g_reg_entries[reg].nub_info.set == 1 &&
+if (interesting_regset(g_reg_entries[reg].nub_info.set) &&
 g_reg_entries[reg].nub_info.value_regs == NULL) {
   if (!DNBThreadGetRegisterValueByID(
   pid, tid, g_reg_entries[reg].nub_info.set,
@@ -2860,6 +2870,50 @@
 
 if (did_exec) {
   ostrm << "reason:exec;";
+} else if (tid_stop_info.reason == eStopTypeWatchpoint) {
+  ostrm << "reason:watchpoint;";
+  ostrm << "description:";
+  std::ostringstream wp_desc;
+  wp_desc << tid_stop_info.details.watchpoint.addr << " ";
+  wp_desc << tid_stop_info.details.watchpoint.hw_idx << " ";
+  wp_desc << tid_stop_info.details.watchpoint.mach_exception_addr;
+  append_hexified_string(ostrm, wp_desc.str());
+  ostrm << ";";
+
+  // Temporarily, print all of the fields we've parsed out of the ESR
+  // on a watchpoint exception.  Normally this is something we would
+  // log for LOG_WATCHPOINTS only, but this was implemented from the
+  // ARM ARM spec and hasn't been exercised on real hardware that can
+  // set most of these fields yet.  It may need to be debugged in the
+  // future, so include all of these purely for debugging by human reasons.
+  ostrm << "watch_addr:" << std::hex
+<< tid_stop_info.details.watchpoint.addr << ";";
+  ostrm << "me_watch_addr:" << std::hex
+<< tid_stop_info.details.watchpoint.mach_exception_addr << ";";
+  ostrm << "wp_hw_idx:" << std::hex
+<< tid_stop_info.details.

[Lldb-commits] [PATCH] D147674: Interpret ESR/FAR bits directly on watchpoint exceptions in debugserver, clarify how watchpoint descriptions in stop packets work

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda abandoned this revision.
jasonmolenda added a comment.

Intermixing generic/linux AArch64 lldb watchpoint StopInfo changes in with a 
change to debugserver made this a difficult patch to ask anyone to review in 
total.  I've separated it into one phab for the lldb watchpoint StopInfo 
handling https://reviews.llvm.org/D147816 and a separate phab for the 
debugserver changes to find the nearest watchpoint when we have a trap address 
outside any watched memory region and pass the ESR fields up to lldb 
https://reviews.llvm.org/D147820 .  Closing this phab.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147674

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

[Lldb-commits] [PATCH] D147748: [lldb] Implement SymbolFile::ContainsCompileOption

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 updated this revision to Diff 511815.
augusto2112 marked an inline comment as done.
augusto2112 added a comment.
Herald added a reviewer: jdoerfert.
Herald added subscribers: jplehr, sstefan1.

Changed implementation to GetCompileOptions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147748

Files:
  lldb/include/lldb/Symbol/SymbolFile.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
@@ -153,6 +153,9 @@
   // Statistics overrides.
   lldb_private::ModuleList GetDebugInfoModules() override;
 
+  void GetCompileOptions(
+  std::unordered_map &args) override;
+
 protected:
   enum { kHaveInitializedOSOs = (1 << 0), kNumFlags };
 
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
@@ -1549,3 +1549,12 @@
   }
   return Status();
 }
+
+void SymbolFileDWARFDebugMap::GetCompileOptions(
+std::unordered_map &args) {
+
+  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
+oso_dwarf->GetCompileOptions(args);
+return false;
+  });
+}
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -523,6 +523,9 @@
 
   void InitializeFirstCodeAddress();
 
+  void GetCompileOptions(
+  std::unordered_map &args) override;
+
   lldb::ModuleWP m_debug_map_module_wp;
   SymbolFileDWARFDebugMap *m_debug_map_symfile;
 
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -4255,3 +4255,30 @@
   return Status("no variable information is available in debug info for this "
 "compile unit");
 }
+
+void SymbolFileDWARF::GetCompileOptions(
+std::unordered_map &args) {
+
+  const uint32_t num_compile_units = GetNumCompileUnits();
+
+  for (uint32_t cu_idx = 0; cu_idx < num_compile_units; ++cu_idx) {
+lldb::CompUnitSP comp_unit = GetCompileUnitAtIndex(cu_idx);
+if (!comp_unit)
+  continue;
+
+DWARFUnit *dwarf_cu = GetDWARFCompileUnit(comp_unit.get());
+if (!dwarf_cu)
+  continue;
+
+const DWARFBaseDIE die = dwarf_cu->GetUnitDIEOnly();
+if (!die)
+  continue;
+
+const char *flags = die.GetAttributeValueAsString(DW_AT_APPLE_flags, NULL);
+
+if (!flags)
+  continue;
+args.insert({comp_unit, Args(flags)});
+  }
+}
+
Index: lldb/include/lldb/Symbol/SymbolFile.h
===
--- lldb/include/lldb/Symbol/SymbolFile.h
+++ lldb/include/lldb/Symbol/SymbolFile.h
@@ -30,6 +30,7 @@
 
 #include 
 #include 
+#include 
 
 #if defined(LLDB_CONFIGURATION_DEBUG)
 #define ASSERT_MODULE_LOCK(expr) (expr->AssertModuleLock())
@@ -435,9 +436,20 @@
 
   virtual lldb::TypeSP CopyType(const lldb::TypeSP &other_type) = 0;
 
+  /// Returns a map of compilation unit to the compile option arguments
+  /// associated with that compilation unit.
+  std::unordered_map GetCompileOptions() {
+std::unordered_map args;
+GetCompileOptions(args);
+return args;
+  }
+
 protected:
   void AssertModuleLock();
 
+  virtual void GetCompileOptions(
+  std::unordered_map &args) {}
+
 private:
   SymbolFile(const SymbolFile &) = delete;
   const SymbolFile &operator=(const SymbolFile &) = delete;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147805: [lldb-vscode] Fix two issues with runInTerminal test.

[Jorge Gorbe Moya via Phabricator via lldb-commits]

jgorbe updated this revision to Diff 511818.
jgorbe added a comment.

Added a `--debugger-pid` flag that needs to be passed with `--launch-target` so
that we can restrict `PR_SET_PTRACER` to the main lldb-vscode instance instead 
of
using `PR_SET_PTRACER_ANY`.


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

https://reviews.llvm.org/D147805

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
  lldb/test/API/tools/lldb-vscode/runInTerminal/TestVSCode_runInTerminal.py
  lldb/tools/lldb-vscode/JSONUtils.cpp
  lldb/tools/lldb-vscode/JSONUtils.h
  lldb/tools/lldb-vscode/Options.td
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -32,6 +32,10 @@
 #include 
 #endif
 
+#if defined(__linux__)
+#include 
+#endif
+
 #include 
 #include 
 #include 
@@ -1562,8 +1566,12 @@
 
   RunInTerminalDebugAdapterCommChannel comm_channel(comm_file.m_path);
 
+  unsigned long debugger_pid = 0;
+#if !defined(_WIN32)
+  debugger_pid = getpid();
+#endif
   llvm::json::Object reverse_request = CreateRunInTerminalReverseRequest(
-  launch_request, g_vsc.debug_adaptor_path, comm_file.m_path);
+  launch_request, g_vsc.debug_adaptor_path, comm_file.m_path, debugger_pid);
   llvm::json::Object reverse_response;
   lldb_vscode::PacketStatus status =
   g_vsc.SendReverseRequest(reverse_request, reverse_response);
@@ -3141,11 +3149,20 @@
 // In case of errors launching the target, a suitable error message will be
 // emitted to the debug adaptor.
 void LaunchRunInTerminalTarget(llvm::opt::Arg &target_arg,
-   llvm::StringRef comm_file, char *argv[]) {
+   llvm::StringRef comm_file,
+   unsigned long debugger_pid, char *argv[]) {
 #if defined(_WIN32)
   llvm::errs() << "runInTerminal is only supported on POSIX systems\n";
   exit(EXIT_FAILURE);
 #else
+
+  // On Linux with the Yama security module enabled, a process can only attach
+  // to its descendants by default. In the runInTerminal case the target
+  // process is launched by the client so we need to allow tracing explicitly.
+#if defined(__linux__)
+  (void)prctl(PR_SET_PTRACER, debugger_pid, 0, 0, 0);
+#endif
+
   RunInTerminalLauncherCommChannel comm_channel(comm_file);
   if (llvm::Error err = comm_channel.NotifyPid()) {
 llvm::errs() << llvm::toString(std::move(err)) << "\n";
@@ -3237,18 +3254,27 @@
   }
 
   if (llvm::opt::Arg *target_arg = input_args.getLastArg(OPT_launch_target)) {
-if (llvm::opt::Arg *comm_file = input_args.getLastArg(OPT_comm_file)) {
+llvm::opt::Arg *debugger_pid = input_args.getLastArg(OPT_debugger_pid);
+llvm::opt::Arg *comm_file = input_args.getLastArg(OPT_comm_file);
+if (comm_file && debugger_pid) {
+  char *remainder;
+  unsigned long pid = strtoul(debugger_pid->getValue(), &remainder, 0);
+  if (remainder == debugger_pid->getValue() || *remainder != '\0') {
+llvm::errs() << "'" << debugger_pid->getValue() << "' is not a valid "
+"PID\n";
+return EXIT_FAILURE;
+  }
   int target_args_pos = argc;
   for (int i = 0; i < argc; i++)
 if (strcmp(argv[i], "--launch-target") == 0) {
   target_args_pos = i + 1;
   break;
 }
-  LaunchRunInTerminalTarget(*target_arg, comm_file->getValue(),
+  LaunchRunInTerminalTarget(*target_arg, comm_file->getValue(), pid,
 argv + target_args_pos);
 } else {
-  llvm::errs() << "\"--launch-target\" requires \"--comm-file\" to be "
-  "specified\n";
+  llvm::errs() << "\"--launch-target\" requires \"--comm-file\" and "
+  "\"--debugger-pid\" to be specified\n";
   return EXIT_FAILURE;
 }
   }
Index: lldb/tools/lldb-vscode/Options.td
===
--- lldb/tools/lldb-vscode/Options.td
+++ lldb/tools/lldb-vscode/Options.td
@@ -28,9 +28,14 @@
   MetaVarName<"">,
   HelpText<"Launch a target for the launchInTerminal request. Any argument "
 "provided after this one will be passed to the target. The parameter "
-"--comm-files-prefix must also be specified.">;
+"--comm-file must also be specified.">;
 
 def comm_file: Separate<["--", "-"], "comm-file">,
   MetaVarName<"">,
-  HelpText<"The fifo file used to communicate the with the debug adaptor"
+  HelpText<"The fifo file used to communicate the with the debug adaptor "
 "when using --launch-target.">;
+
+def debugger_pid: Separate<["--", "-"], "debugger-pid">,
+  MetaVarName<"">,
+  HelpText<"The PID of the lldb-vscode instance that sent the launchInTerminal "
+"request when using --launch-target.">;
Index: lldb/tools/lldb-vscode/JSONUtils.h
==

[Lldb-commits] [PATCH] D147748: [lldb] Implement SymbolFile::GetCompileOptions

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM with or without the DenseMap, whichever makes the most sense.




Comment at: lldb/include/lldb/Symbol/SymbolFile.h:441
+  /// associated with that compilation unit.
+  std::unordered_map GetCompileOptions() {
+std::unordered_map args;

Any reason you picked `unordered_map`? Not that I expected this code to be 
particularly hot, but I would've gone for an `llvm::DenseMap` which should 
offer much better performance characteristics. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147748

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

[Lldb-commits] [PATCH] D147748: [lldb] Implement SymbolFile::GetCompileOptions

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 added inline comments.



Comment at: lldb/include/lldb/Symbol/SymbolFile.h:441
+  /// associated with that compilation unit.
+  std::unordered_map GetCompileOptions() {
+std::unordered_map args;

JDevlieghere wrote:
> Any reason you picked `unordered_map`? Not that I expected this code to be 
> particularly hot, but I would've gone for an `llvm::DenseMap` which should 
> offer much better performance characteristics. 
Mainly because there's no implementation of shared pointer as keys for dense 
maps (`DenseMapInfo` where the `T` is a `shared_ptr`) at the moment, and I 
don't expect this to be super hot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147748

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

[Lldb-commits] [PATCH] D147674: Interpret ESR/FAR bits directly on watchpoint exceptions in debugserver, clarify how watchpoint descriptions in stop packets work

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

I agree that silent continue was wrong and should be fixed. I tried for  to 
remember what I was trying to do when I wrote that patch ... still dont 
remember much but digged out some information that may be useful for this patch 
review.

We had a bunch of funny behaving hardware mostly Nexus phones with different 
types of watchpoint behavior being implement by every vendor.

From our local record i found that stp issue was never fixed. Some vendor 
machines reported correct hit_address while some didnt. In LLVM we do have bug 
report for another of these issues in one of the cases where STP instruction 
can trigger multiple watchpoints located side by side. 
https://bugs.llvm.org/show_bug.cgi?id=30758

On Linux ptrace is responsible for reporting a watchpoint hit address and also 
responsible for setting/unsetting watchpoints. In case of Arm64 ptrace while 
reporting watchpoints performs some heuristic based calculations to exactly 
cater for the case you have mentioned where access reports a address out of 
range. See watchpoint_handler code 
here:https://elixir.bootlin.com/linux/latest/source/arch/arm64/kernel/hw_breakpoint.c#L754

And this comment copied from same file :
/*

- Arm64 hardware does not always report a watchpoint hit address that matches
- one of the watchpoints set. It can also report an address "near" the
- watchpoint if a single instruction access both watched and unwatched
- addresses. There is no straight-forward way, short of disassembling the
- offending instruction, to map that address back to the watchpoint. This
- function computes the distance of the memory access from the watchpoint as a
- heuristic for the likelihood that a given access triggered the watchpoint. *
- See Section D2 .10.5 "Determining the memory 
location that caused a Watchpoint
- exception" of ARMv8 Architecture Reference Manual for details. *
- The function returns the distance of the address from the bytes watched by
- the watchpoint. In case of an exact match, it returns 0. */


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147674

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

[Lldb-commits] [PATCH] D147816: Clarify how watchpoint description in stop packets work, fix AArch64 unintended behavior

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

I agree that silent continue was wrong and should be fixed. I tried for to 
remember what I was trying to do when I wrote that patch ... still dont 
remember much but digged out some information that may be useful for this patch 
review.

We had a bunch of funny behaving hardware mostly Nexus phones with different 
types of watchpoint behavior being implement by every vendor.

From our local record i found that stp issue was never fixed. Some vendor 
machines reported correct hit_address while some didnt. In LLVM we do have bug 
report for another of these issues in one of the cases where STP instruction 
can trigger multiple watchpoints located side by side. 
https://bugs.llvm.org/show_bug.cgi?id=30758

On Linux ptrace is responsible for reporting a watchpoint hit address and also 
responsible for setting/unsetting watchpoints. In case of Arm64 ptrace while 
reporting watchpoints performs some heuristic based calculations to exactly 
cater for the case you have mentioned where access reports a address out of 
range. See watchpoint_handler code 
here:https://elixir.bootlin.com/linux/latest/source/arch/arm64/kernel/hw_breakpoint.c#L754

And this comment copied from same file :
/*

Arm64 hardware does not always report a watchpoint hit address that matches
one of the watchpoints set. It can also report an address "near" the
watchpoint if a single instruction access both watched and unwatched
addresses. There is no straight-forward way, short of disassembling the
offending instruction, to map that address back to the watchpoint. This
function computes the distance of the memory access from the watchpoint as a
heuristic for the likelihood that a given access triggered the watchpoint. *
See Section D2 .10.5 "Determining the memory 
location that caused a Watchpoint
exception" of ARMv8 Architecture Reference Manual for details. *
The function returns the distance of the address from the bytes watched by
the watchpoint. In case of an exact match, it returns 0. */


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147816

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

[Lldb-commits] [PATCH] D147816: Clarify how watchpoint description in stop packets work, fix AArch64 unintended behavior

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Thanks Omair!  Yeah we have the same problem on Darwin - AArch64 doesn't 
require that the address we get on a watchpoint trap in the FAR register point 
within the address range we are watching.  I never fixed this on Darwin so I 
know the failure mode - lldb says "there was a watchpoint hit, but I couldn't 
possibly figure out which watchpoint it is, I will stop execution now" and the 
user has to figure out that they need to manually disable the watchpoint, 
instruction step, and re-insert the watchpoint.  I'm sure you were looking at 
the same issue and fixed it by using the MIPS silently-skip codepath in 2016 by 
accident.  I misunderstood what those three fields in the `description` were 
doing for a few days before I finally figured it out myself, I'm not at all 
surprised other people misunderstood it too.

It looks like some future AArch64 cpus may give us a watchpoint number in the 
ESR, instead of an address that was accessed, that will be a nice improvement 
over having heuristics to figure out which watchpoint was likely tripped.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147816

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

[Lldb-commits] [PATCH] D147831: [lldb-vscode] Implement RestartRequest

[Jorge Gorbe Moya via Phabricator via lldb-commits]

jgorbe created this revision.
jgorbe added reviewers: clayborg, wallace, labath, rupprecht.
jgorbe added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
jgorbe requested review of this revision.

This is an optional request, but supporting it makes the experience better when
re-launching a big binary that takes significant time to parse: instead of
tearing down and re-create the whole session we just need to kill the current
process and launch a new one.

Some non-obvious comments that might help review this change:

- After killing the process, we get an "exited" event for it. Because the 
process no longer exists some interesting things can occur that manifest as 
flaky failures if not dealt with:
  - `EventIsProcessEvent` relies on `SBEvent::GetBroadcasterClass`, which can 
crash if the broadcaster is no longer there: the event only holds a weak_ptr to 
its broadcaster, and `GetBroadcasterClass` uses it without checking.

Other `EventIs*` functions look at the flavor of the EventData, so I have 
modified EventIsProcessEvent to do that.
  - We keep the PID of the old process so we can detect its "exited" event and 
not terminate the whole session. But sometimes the SBProcess we get from the 
event won't have a PID, for some reason.

- I have factored out the code to launch a process out to a new LaunchProcess 
function, so it can be used from both `request_launch` and `request_restart`.

- The restart_runInTerminal test has the same problem with debug builds as the 
original runInTerminal test: when attaching to the launcher instance of 
lldb-vscode it takes a long time to parse its debug info. I have used the same 
workaround to disable that particular test for debug builds.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147831

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
  lldb/source/API/SBProcess.cpp
  lldb/test/API/tools/lldb-vscode/restart/Makefile
  lldb/test/API/tools/lldb-vscode/restart/TestVSCode_restart.py
  lldb/test/API/tools/lldb-vscode/restart/TestVSCode_restart_runInTerminal.py
  lldb/test/API/tools/lldb-vscode/restart/main.c
  lldb/tools/lldb-vscode/VSCode.cpp
  lldb/tools/lldb-vscode/VSCode.h
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -458,7 +458,8 @@
 // manually send a stopped event in request_configurationDone(...)
 // so don't send any before then.
 if (g_vsc.configuration_done_sent) {
-  // Only report a stopped event if the process was not restarted.
+  // Only report a stopped event if the process was not
+  // automatically restarted.
   if (!lldb::SBProcess::GetRestartedFromEvent(event)) {
 SendStdOutStdErr(process);
 SendThreadStoppedEvent();
@@ -468,14 +469,22 @@
   case lldb::eStateRunning:
 g_vsc.WillContinue();
 break;
-  case lldb::eStateExited: {
-// Run any exit LLDB commands the user specified in the
-// launch.json
-g_vsc.RunExitCommands();
-SendProcessExitedEvent(process);
-SendTerminatedEvent();
-done = true;
-  } break;
+  case lldb::eStateExited:
+// When restarting, we can get an "exited" event for the process we
+// just killed with the old PID, or even with no PID. In that case
+// we don't have to terminate the session.
+if (process.GetProcessID() == LLDB_INVALID_PROCESS_ID ||
+process.GetProcessID() == g_vsc.restarting_process_id) {
+  g_vsc.restarting_process_id = LLDB_INVALID_PROCESS_ID;
+} else {
+  // Run any exit LLDB commands the user specified in the
+  // launch.json
+  g_vsc.RunExitCommands();
+  SendProcessExitedEvent(process);
+  SendTerminatedEvent();
+  done = true;
+}
+break;
   }
 } else if ((event_mask & lldb::SBProcess::eBroadcastBitSTDOUT) ||
(event_mask & lldb::SBProcess::eBroadcastBitSTDERR)) {
@@ -1527,7 +1536,7 @@
   // The debug adapter supports the RestartRequest. In this case a client
   // should not implement 'restart' by terminating and relaunching the adapter
   // but by calling the RestartRequest.
-  body.try_emplace("supportsRestartRequest", false);
+  body.try_emplace("supportsRestartRequest", true);
   // The debug adapter supports 'exceptionOptions' on the
   // setExceptionBreakpoints request.
   body.try_emplace("supportsExceptionOptions", true);
@@ -1622,6 +1631,71 @@
  error.GetCString());
 }
 
+// Takes a LaunchRequest object and launches the pr

[Lldb-commits] [PATCH] D147805: [lldb-vscode] Fix two issues with runInTerminal test.

[Jordan Rupprecht via Phabricator via lldb-commits]

rupprecht added a reviewer: rupprecht.
rupprecht added a comment.

The `pid` plumbing looks fine for the happy case, but I think we could be more 
lenient if (for whatever reason) the pid flag isn't being set on non-Linux 
systems that won't actually be using it. Even on a Linux system, this is only 
needed if ptrace_scope != 0, so if the `pid` isn't being provided there, we 
could just skip the `prctl` call and hope the user's system has a value of 0 
for that.

(Also, 0 is a weird value, we should probably just reject that)




Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:1120
   debug_adaptor_path.str(), "--comm-file", comm_file.str(),
+  "--debugger-pid", std::to_string(debugger_pid),
   "--launch-target", GetString(launch_request_arguments, "program").str()};

Only pass this if != 0

(or get fancy and use a std::optional or whatever)



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1569
 
+  unsigned long debugger_pid = 0;
+#if !defined(_WIN32)

unsigned long -> lldb::pid_t



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3163
+#if defined(__linux__)
+  (void)prctl(PR_SET_PTRACER, debugger_pid, 0, 0, 0);
+#endif

Only invoke this if debugger_pid != 0



Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3261
+  char *remainder;
+  unsigned long pid = strtoul(debugger_pid->getValue(), &remainder, 0);
+  if (remainder == debugger_pid->getValue() || *remainder != '\0') {

`StringRef` is usually used for parsing strings as numbers, something like:

```
unsigned long pid = 0;
  llvm::StringRef debugger_pid_value = debugger_pid->getValue())
  if (debugger_pid_value.getAsInteger(10, pid)) {
llvm::errs() << ...
  }
}
```


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

https://reviews.llvm.org/D147805

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

[Lldb-commits] [PATCH] D147833: [lldb] Change return type of EventData::GetFlavor

[Alex Langford via Phabricator via lldb-commits]

bulbazord created this revision.
bulbazord added reviewers: JDevlieghere, labath, jingham, mib, jasonmolenda.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

There's no reason these strings need to be in the ConstString
StringPool, they're already string literals with static lifetime.

I plan on addressing other similar functions in follow up commits.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147833

Files:
  lldb/include/lldb/Breakpoint/Breakpoint.h
  lldb/include/lldb/Breakpoint/Watchpoint.h
  lldb/include/lldb/Core/DebuggerEvents.h
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Target/Thread.h
  lldb/include/lldb/Utility/Event.h
  lldb/source/API/SBEvent.cpp
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Breakpoint/Watchpoint.cpp
  lldb/source/Core/DebuggerEvents.cpp
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/Thread.cpp
  lldb/source/Utility/Event.cpp

Index: lldb/source/Utility/Event.cpp
===
--- lldb/source/Utility/Event.cpp
+++ lldb/source/Utility/Event.cpp
@@ -114,12 +114,9 @@
 
 EventDataBytes::~EventDataBytes() = default;
 
-ConstString EventDataBytes::GetFlavorString() {
-  static ConstString g_flavor("EventDataBytes");
-  return g_flavor;
-}
+const char *EventDataBytes::GetFlavorString() { return "EventDataBytes"; }
 
-ConstString EventDataBytes::GetFlavor() const {
+const char *EventDataBytes::GetFlavor() const {
   return EventDataBytes::GetFlavorString();
 }
 
@@ -182,6 +179,10 @@
   m_bytes.swap(new_bytes);
 }
 
+const char *EventDataReceipt::GetFlavorString() {
+  return "Process::ProcessEventData";
+}
+
 #pragma mark -
 #pragma mark EventStructuredData
 
@@ -200,7 +201,7 @@
 
 // EventDataStructuredData member functions
 
-ConstString EventDataStructuredData::GetFlavor() const {
+const char *EventDataStructuredData::GetFlavor() const {
   return EventDataStructuredData::GetFlavorString();
 }
 
@@ -280,7 +281,6 @@
 return StructuredDataPluginSP();
 }
 
-ConstString EventDataStructuredData::GetFlavorString() {
-  static ConstString s_flavor("EventDataStructuredData");
-  return s_flavor;
+const char *EventDataStructuredData::GetFlavorString() {
+  return "EventDataStructuredData";
 }
Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -150,9 +150,8 @@
 
 // Thread Event Data
 
-ConstString Thread::ThreadEventData::GetFlavorString() {
-  static ConstString g_flavor("Thread::ThreadEventData");
-  return g_flavor;
+const char *Thread::ThreadEventData::GetFlavorString() {
+  return "Thread::ThreadEventData";
 }
 
 Thread::ThreadEventData::ThreadEventData(const lldb::ThreadSP thread_sp)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -4786,9 +4786,8 @@
 
 Target::TargetEventData::~TargetEventData() = default;
 
-ConstString Target::TargetEventData::GetFlavorString() {
-  static ConstString g_flavor("Target::TargetEventData");
-  return g_flavor;
+const char *Target::TargetEventData::GetFlavorString() {
+  return "Target::TargetEventData";
 }
 
 void Target::TargetEventData::Dump(Stream *s) const {
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -3929,12 +3929,11 @@
 
 Process::ProcessEventData::~ProcessEventData() = default;
 
-ConstString Process::ProcessEventData::GetFlavorString() {
-  static ConstString g_flavor("Process::ProcessEventData");
-  return g_flavor;
+const char *Process::ProcessEventData::GetFlavorString() {
+  return "Process::ProcessEventData";
 }
 
-ConstString Process::ProcessEventData::GetFlavor() const {
+const char *Process::ProcessEventData::GetFlavor() const {
   return ProcessEventData::GetFlavorString();
 }
 
Index: lldb/source/Core/DebuggerEvents.cpp
===
--- lldb/source/Core/DebuggerEvents.cpp
+++ lldb/source/Core/DebuggerEvents.cpp
@@ -23,12 +23,9 @@
   return nullptr;
 }
 
-ConstString ProgressEventData::GetFlavorString() {
-  static ConstString g_flavor("ProgressEventData");
-  return g_flavor;
-}
+const char *ProgressEventData::GetFlavorString() { return "ProgressEventData"; }
 
-ConstString ProgressEventData::GetFlavor() const {
+const char *ProgressEventData::GetFlavor() const {
   return ProgressEventData::GetFlavorString();
 }
 
@@ -94,12 +91,11 @@
   s->Flush();
 }
 
-ConstString DiagnosticEventData::GetFlavorString() {
-  static ConstString g_flavor("DiagnosticEventData");
-  return g_flavor;
+const char *DiagnosticEventData::GetFlavorStrin

[Lldb-commits] [PATCH] D147833: [lldb] Change return type of EventData::GetFlavor

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Am I missing something, how does this work when we have uses like 
`event_data->GetFlavor() == TargetEventData::GetFlavorString()` - is this 
changing from a one-time construction of a ConstString to a construction of a 
ConstString every time it's called by implicit construction?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147833

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

[Lldb-commits] [PATCH] D147833: [lldb] Change return type of EventData::GetFlavor

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D147833#4252651 , @jasonmolenda 
wrote:

> Am I missing something, how does this work when we have uses like 
> `event_data->GetFlavor() == TargetEventData::GetFlavorString()` - is this 
> changing from a one-time construction of a ConstString to a construction of a 
> ConstString every time it's called by implicit construction?

`event_data->GetFlavor() == TargetEventData::GetFlavorString()` would be just a 
straight pointer comparison, no ConstString in the picture. These strings don't 
need to be in the ConstString pool -- they only exist in one place each and are 
guaranteed to be there for the lifetime of a running lldb process. There are 
other places in lldb where I'd like to do this, but I figured I'd do it in a 
few smaller patches rather than one giant patch.




Comment at: lldb/source/Utility/Event.cpp:183
+const char *EventDataReceipt::GetFlavorString() {
+  return "Process::ProcessEventData";
+}

Note: this is probably the wrong flavor string (i.e. a bug) but I'm just 
preserving existing behavior here, for better or worse.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147833

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

[Lldb-commits] [PATCH] D147833: [lldb] Change return type of EventData::GetFlavor

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

In D147833#4252681 , @bulbazord wrote:

> In D147833#4252651 , @jasonmolenda 
> wrote:
>
>> Am I missing something, how does this work when we have uses like 
>> `event_data->GetFlavor() == TargetEventData::GetFlavorString()` - is this 
>> changing from a one-time construction of a ConstString to a construction of 
>> a ConstString every time it's called by implicit construction?
>
> `event_data->GetFlavor() == TargetEventData::GetFlavorString()` would be just 
> a straight pointer comparison, no ConstString in the picture. These strings 
> don't need to be in the ConstString pool -- they only exist in one place each 
> and are guaranteed to be there for the lifetime of a running lldb process. 
> There are other places in lldb where I'd like to do this, but I figured I'd 
> do it in a few smaller patches rather than one giant patch.

OK, I see.  I saw the `==` and thought something must be constructed to an 
object for that operator to work, I didn't even think of address comparing 
constant data c-strings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147833

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

[Lldb-commits] [PATCH] D147841: [lldb][NFC] Update syntax description for language cplusplus demangle

[Alex Langford via Phabricator via lldb-commits]

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

Also added some tests because this is completely untested.

rdar://107780577


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147841

Files:
  
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
  lldb/test/Shell/Commands/command-language-cplusplus-demangle.test


Index: lldb/test/Shell/Commands/command-language-cplusplus-demangle.test
===
--- /dev/null
+++ lldb/test/Shell/Commands/command-language-cplusplus-demangle.test
@@ -0,0 +1,22 @@
+# RUN: %lldb -b -o "language cplusplus demangle __ZN3Foo7DoThingEv" \
+# RUN:   | FileCheck --check-prefix=DOUBLE-UNDERSCORE %s
+# RUN: %lldb -b -o "language cplusplus demangle _ZN3Foo7DoThingEv" \
+# RUN:   | FileCheck --check-prefix=SINGLE-UNDERSCORE %s
+# RUN: not %lldb -b -o "language cplusplus demangle foo" 2>&1 \
+# RUN:   | FileCheck --check-prefix=NOT-MANGLED %s
+# RUN: not %lldb -b -o "language cplusplus demangle _ZN3Foo7DoThingEv foo" 
2>&1 \
+# RUN:   | FileCheck --check-prefix=MULTI-ARG %s
+# RUN: %lldb -b -o "help language cplusplus demangle" \
+# RUN:   | FileCheck --check-prefix=HELP-MESSAGE %s
+
+# DOUBLE-UNDERSCORE: __ZN3Foo7DoThingEv ---> Foo::DoThing()
+
+# SINGLE-UNDERSCORE: _ZN3Foo7DoThingEv ---> Foo::DoThing()
+
+# NOT-MANGLED: error: foo is not a valid C++ mangled name
+
+# MULTI-ARG: _ZN3Foo7DoThingEv ---> Foo::DoThing()
+# MULTI-ARG: error: foo is not a valid C++ mangled name
+
+# HELP-MESSAGE: Demangle a C++ mangled name.
+# HELP-MESSAGE: Syntax: language cplusplus demangle [ ...] 
Index: 
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
===
--- 
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
+++ 
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
@@ -318,9 +318,9 @@
 class CommandObjectMultiwordItaniumABI_Demangle : public CommandObjectParsed {
 public:
   CommandObjectMultiwordItaniumABI_Demangle(CommandInterpreter &interpreter)
-  : CommandObjectParsed(interpreter, "demangle",
-"Demangle a C++ mangled name.",
-"language cplusplus demangle") {
+  : CommandObjectParsed(
+interpreter, "demangle", "Demangle a C++ mangled name.",
+"language cplusplus demangle [ ...]") {
 CommandArgumentEntry arg;
 CommandArgumentData index_arg;
 


Index: lldb/test/Shell/Commands/command-language-cplusplus-demangle.test
===
--- /dev/null
+++ lldb/test/Shell/Commands/command-language-cplusplus-demangle.test
@@ -0,0 +1,22 @@
+# RUN: %lldb -b -o "language cplusplus demangle __ZN3Foo7DoThingEv" \
+# RUN:   | FileCheck --check-prefix=DOUBLE-UNDERSCORE %s
+# RUN: %lldb -b -o "language cplusplus demangle _ZN3Foo7DoThingEv" \
+# RUN:   | FileCheck --check-prefix=SINGLE-UNDERSCORE %s
+# RUN: not %lldb -b -o "language cplusplus demangle foo" 2>&1 \
+# RUN:   | FileCheck --check-prefix=NOT-MANGLED %s
+# RUN: not %lldb -b -o "language cplusplus demangle _ZN3Foo7DoThingEv foo" 2>&1 \
+# RUN:   | FileCheck --check-prefix=MULTI-ARG %s
+# RUN: %lldb -b -o "help language cplusplus demangle" \
+# RUN:   | FileCheck --check-prefix=HELP-MESSAGE %s
+
+# DOUBLE-UNDERSCORE: __ZN3Foo7DoThingEv ---> Foo::DoThing()
+
+# SINGLE-UNDERSCORE: _ZN3Foo7DoThingEv ---> Foo::DoThing()
+
+# NOT-MANGLED: error: foo is not a valid C++ mangled name
+
+# MULTI-ARG: _ZN3Foo7DoThingEv ---> Foo::DoThing()
+# MULTI-ARG: error: foo is not a valid C++ mangled name
+
+# HELP-MESSAGE: Demangle a C++ mangled name.
+# HELP-MESSAGE: Syntax: language cplusplus demangle [ ...] 
Index: lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
===
--- lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
+++ lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
@@ -318,9 +318,9 @@
 class CommandObjectMultiwordItaniumABI_Demangle : public CommandObjectParsed {
 public:
   CommandObjectMultiwordItaniumABI_Demangle(CommandInterpreter &interpreter)
-  : CommandObjectParsed(interpreter, "demangle",
-"Demangle a C++ mangled name.",
-"language cplusplus demangle") {
+  : CommandObjectParsed(
+interpreter, "demangle", "Demangle a C++ mangled name.",
+"language cplusplus demangle [ ...]") {
 CommandArgumentEntry arg;
 CommandArgumentData index_arg;
 
___