[Lldb-commits] [PATCH] D124597: Select the correct DWARF location list entry when looking for variable locations

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added reviewers: jingham, JDevlieghere.
jasonmolenda added a project: LLDB.
Herald added subscribers: pengfei, kristof.beyls.
Herald added a project: All.
jasonmolenda requested review of this revision.
Herald added a subscriber: lldb-commits.

When lldb is looking for a location list entry for a variable, it is using the 
pc value -- or return-pc value when up the stack -- to pick the correct DWARF 
location list entry.  Because it is using return-pc, the entry selected may be 
an entirely different basic block, or function.  We should be using the "pc for 
symbolication" -- return-pc decremented in most cases -- to select the correct 
block we should be looking up.  I added RegisterContext::GetPCForSymbolication 
and StackFrame::GetFrameCodeAddressForSymbolication last year in 
https://reviews.llvm.org/D97644 and this is a simple patch with those in place.

The bug that drove this is in the middle of a complex project, but I can elicit 
incorrect behavior with a small C file that calls abort() as its last 
instruction, with a different function immediately after that.  It must be 
built at -O1 with clang (on x86_64, AArch64) so that the debug info tracks 
variables in different registers during the function lifetime, using a DWARF 
location list.

I constructed a test case that does this, but it depends on clang's current 
codegen at -O1 on these two architectures.  If the codegen were to shift 
around, the test could pass without correctly exercising this codepath.  One 
possibility would be to include a test binary, DWARF for it, and corefile; that 
would eliminate the possibility of codegen changing.  But it's a lot of data, 
and I'm not sure how necessary it is.  Given that I get the same codegen on two 
architectures with clang at -O1, maybe this is a small enough exposure to the 
optimizer that we can use it in the testsuite.  Open to suggestions.

Without this patch, this test case will show argv as unavailable when you try 
to print it.  In the actual bug that I was tracking down, the return-pc pointed 
to a different location list entry for the variable so lldb showed the wrong 
value for that variable.  A much more subtle and confusing bug for the user.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124597

Files:
  lldb/source/Expression/DWARFExpression.cpp
  lldb/test/API/functionalities/location-list-lookup/Makefile
  lldb/test/API/functionalities/location-list-lookup/TestLocationListLookup.py
  lldb/test/API/functionalities/location-list-lookup/main.c

Index: lldb/test/API/functionalities/location-list-lookup/main.c
===
--- /dev/null
+++ lldb/test/API/functionalities/location-list-lookup/main.c
@@ -0,0 +1,26 @@
+#include 
+#include 
+
+// The goal with this test is:
+//  1. Have main() followed by foo()
+//  2. Have the no-return call to abort() in main be the last instruction
+//  3. Have the next instruction be the start of foo()
+//  4. The debug info for argv uses a location list.
+// clang at -O0 on x86_64 or arm64 has debuginfo like
+//  DW_AT_location	(0x0049:
+//  [0x00013f15, 0x00013f25): DW_OP_reg4 RSI
+//  [0x00013f25, 0x00013f5b): DW_OP_reg15 R15)
+
+void foo(int);
+int main (int argc, char **argv)
+{
+char *file = argv[0];
+char f0 = file[0];
+printf ("%c\n", f0);
+foo(f0);
+printf ("%s %d\n", argv[0], argc);
+abort();  /// argv is still be accessible here
+}
+void foo(int in) {
+printf ("%d\n", in);
+}
Index: lldb/test/API/functionalities/location-list-lookup/TestLocationListLookup.py
===
--- /dev/null
+++ lldb/test/API/functionalities/location-list-lookup/TestLocationListLookup.py
@@ -0,0 +1,40 @@
+"""Test that lldb picks the correct DWARF location list entry with a return-pc out of bounds."""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class LocationListLookupTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+
+def test_loclist(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+
+# Create a target by the debugger.
+target = self.dbg.CreateTarget(exe)
+self.assertTrue(target, VALID_TARGET)
+self.dbg.SetAsync(False)
+
+li = lldb.SBLaunchInfo(["a.out"])
+error = lldb.SBError()
+process = target.Launch(li, error)
+self.assertTrue(process.IsValid())
+self.assertTrue(process.is_stopped)
+
+# Find `main` on the stack, then 
+# find `argv` local variable, then
+# check that we can read the c-string in argv[0]
+for f in process.GetSelectedThread().f

[Lldb-commits] [PATCH] D124579: Make partial function name matching match on context boundaries

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Yeah, this was very surprising.

The fact that we're still doing substring matches on the context is suspicious, 
and might cause false positives, but given that we first verify that the 
context is valid, I was not able to come up with counterexamples.




Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:293-309
+  // Incoming path has context but this method does not, no match.
+  if (m_context.empty())
+return false;
+
+  // Make sure the incoming path's context is found in the method context:
+  size_t path_pos = m_context.rfind(context);
+  if (path_pos == llvm::StringRef::npos)

```
StringRef haystack = m_context;
if (!haystack.consume_back(context))
  return false;
if (haystack.empty() || !isalnum(haystack.back()))
  return true;
return false;
```



Comment at: lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp:151-155
+  TestCase test_cases_3[] {
+{"func01", true},
+{"func", false},
+{"bar::func01", false},
+  };

what about these?



Comment at: lldb/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp:157
+
+  for (auto test : test_cases_1) {
+EXPECT_EQ(reference_1.ContainsPath(test.path), test.result);

This saves very little compared to spelling out the individual checks 
(`EXPECT_TRUE/FALSE(reference_1.ContainsPath("whatever"))`), and makes the 
error messages a lot more cryptic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124579

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

[Lldb-commits] [PATCH] D124535: [lldb] Fix crash when launching in terminal

[Pavel Labath via Phabricator via lldb-commits]

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

One could add a unit test for the CFD overload, though, as this appears to be 
the only callsite (or other callsites were equally untested and broken), we 
could also remove it altogether.


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

https://reviews.llvm.org/D124535

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

[Lldb-commits] [lldb] 57f99d0 - [lldb] Reduce duplication in DWARFASTParserClang::CopyUniqueClassMethodTypes

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2022-04-28T10:58:54+02:00
New Revision: 57f99d0dc387aab4e4af2cd92f97598e8a5df41f

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

LOG: [lldb] Reduce duplication in 
DWARFASTParserClang::CopyUniqueClassMethodTypes

Use lambdas to replace identical bits of code.

Added: 


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

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 07842ce01593f..296ec524623ec 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -3499,49 +3499,37 @@ bool DWARFASTParserClang::CopyUniqueClassMethodTypes(
   // in "dst_cu" and "dst_class_die"
   class_type->GetFullCompilerType();
 
-  DWARFDIE src_die;
-  DWARFDIE dst_die;
+  auto gather = [](DWARFDIE die, UniqueCStringMap &map,
+   UniqueCStringMap map_artificial) {
+if (die.Tag() != DW_TAG_subprogram)
+  return;
+// Make sure this is a declaration and not a concrete instance by looking
+// for DW_AT_declaration set to 1. Sometimes concrete function instances 
are
+// placed inside the class definitions and shouldn't be included in the 
list
+// of things are are tracking here.
+if (die.GetAttributeValueAsUnsigned(DW_AT_declaration, 0) != 1)
+  return;
+
+if (const char *name = die.GetMangledName()) {
+  ConstString const_name(name);
+  if (die.GetAttributeValueAsUnsigned(DW_AT_artificial, 0))
+map_artificial.Append(const_name, die);
+  else
+map.Append(const_name, die);
+}
+  };
+
   UniqueCStringMap src_name_to_die;
   UniqueCStringMap dst_name_to_die;
   UniqueCStringMap src_name_to_die_artificial;
   UniqueCStringMap dst_name_to_die_artificial;
-  for (src_die = src_class_die.GetFirstChild(); src_die.IsValid();
+  for (DWARFDIE src_die = src_class_die.GetFirstChild(); src_die.IsValid();
src_die = src_die.GetSibling()) {
-if (src_die.Tag() == DW_TAG_subprogram) {
-  // Make sure this is a declaration and not a concrete instance by looking
-  // for DW_AT_declaration set to 1. Sometimes concrete function instances
-  // are placed inside the class definitions and shouldn't be included in
-  // the list of things are are tracking here.
-  if (src_die.GetAttributeValueAsUnsigned(DW_AT_declaration, 0) == 1) {
-const char *src_name = src_die.GetMangledName();
-if (src_name) {
-  ConstString src_const_name(src_name);
-  if (src_die.GetAttributeValueAsUnsigned(DW_AT_artificial, 0))
-src_name_to_die_artificial.Append(src_const_name, src_die);
-  else
-src_name_to_die.Append(src_const_name, src_die);
-}
-  }
-}
+gather(src_die, src_name_to_die, src_name_to_die_artificial);
   }
-  for (dst_die = dst_class_die.GetFirstChild(); dst_die.IsValid();
+  for (DWARFDIE dst_die = dst_class_die.GetFirstChild(); dst_die.IsValid();
dst_die = dst_die.GetSibling()) {
-if (dst_die.Tag() == DW_TAG_subprogram) {
-  // Make sure this is a declaration and not a concrete instance by looking
-  // for DW_AT_declaration set to 1. Sometimes concrete function instances
-  // are placed inside the class definitions and shouldn't be included in
-  // the list of things are are tracking here.
-  if (dst_die.GetAttributeValueAsUnsigned(DW_AT_declaration, 0) == 1) {
-const char *dst_name = dst_die.GetMangledName();
-if (dst_name) {
-  ConstString dst_const_name(dst_name);
-  if (dst_die.GetAttributeValueAsUnsigned(DW_AT_artificial, 0))
-dst_name_to_die_artificial.Append(dst_const_name, dst_die);
-  else
-dst_name_to_die.Append(dst_const_name, dst_die);
-}
-  }
-}
+gather(dst_die, dst_name_to_die, dst_name_to_die_artificial);
   }
   const uint32_t src_size = src_name_to_die.GetSize();
   const uint32_t dst_size = dst_name_to_die.GetSize();
@@ -3556,8 +3544,8 @@ bool DWARFASTParserClang::CopyUniqueClassMethodTypes(
 
   if (fast_path) {
 for (idx = 0; idx < src_size; ++idx) {
-  src_die = src_name_to_die.GetValueAtIndexUnchecked(idx);
-  dst_die = dst_name_to_die.GetValueAtIndexUnchecked(idx);
+  DWARFDIE src_die = src_name_to_die.GetValueAtIndexUnchecked(idx);
+  DWARFDIE dst_die = dst_name_to_die.GetValueAtIndexUnchecked(idx);
 
   if (src_die.Tag() != dst_die.Tag())
 fast_path = false;
@@ -3575,28 +3563,29 @@ bool DWARFASTParserClang::CopyUniqueClassMethodTypes(
 
   DWARFASTParserClang *src_dwarf_ast_parser =
   static_cast(
-  SymbolFileDWA

[Lldb-commits] [PATCH] D124601: [lldb] Use shutil.which instead of distutils find_executable

[David Spickett via Phabricator via lldb-commits]

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

distutils is deprecated and shutil.which is the suggested
replacement for this function.

https://peps.python.org/pep-0632/#migration-advice
https://docs.python.org/3/library/shutil.html#shutil.which

It was added in Python3.3 but given that we're already using
shutil.which elsewhere I think this is ok/no worse than before.

We do have our own find_executable in lldb/test/Shell/helper/build.py
but I'd rather leave that as is for now. Also we have our own versions
of which() but again, a change for another time.

This work is part of #54337.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124601

Files:
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/test/Shell/lit.cfg.py


Index: lldb/test/Shell/lit.cfg.py
===
--- lldb/test/Shell/lit.cfg.py
+++ lldb/test/Shell/lit.cfg.py
@@ -12,7 +12,6 @@
 from lit.llvm import llvm_config
 from lit.llvm.subst import FindTool
 from lit.llvm.subst import ToolSubst
-from distutils.spawn import find_executable
 
 site.addsitedir(os.path.dirname(__file__))
 from helper import toolchain
@@ -121,7 +120,7 @@
 if config.lldb_enable_lzma:
 config.available_features.add('lzma')
 
-if find_executable('xz') != None:
+if shutil.which('xz') != None:
 config.available_features.add('xz')
 
 if config.lldb_system_debugserver:
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -49,7 +49,6 @@
 import sys
 import time
 import traceback
-import distutils.spawn
 
 # Third-party modules
 import unittest2
@@ -1568,7 +1567,7 @@
 
 # Tries to find clang at the same folder as the lldb
 lldb_dir = os.path.dirname(lldbtest_config.lldbExec)
-path = distutils.spawn.find_executable("clang", lldb_dir)
+path = shutil.which("clang", path=lldb_dir)
 if path is not None:
 return path
 


Index: lldb/test/Shell/lit.cfg.py
===
--- lldb/test/Shell/lit.cfg.py
+++ lldb/test/Shell/lit.cfg.py
@@ -12,7 +12,6 @@
 from lit.llvm import llvm_config
 from lit.llvm.subst import FindTool
 from lit.llvm.subst import ToolSubst
-from distutils.spawn import find_executable
 
 site.addsitedir(os.path.dirname(__file__))
 from helper import toolchain
@@ -121,7 +120,7 @@
 if config.lldb_enable_lzma:
 config.available_features.add('lzma')
 
-if find_executable('xz') != None:
+if shutil.which('xz') != None:
 config.available_features.add('xz')
 
 if config.lldb_system_debugserver:
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -49,7 +49,6 @@
 import sys
 import time
 import traceback
-import distutils.spawn
 
 # Third-party modules
 import unittest2
@@ -1568,7 +1567,7 @@
 
 # Tries to find clang at the same folder as the lldb
 lldb_dir = os.path.dirname(lldbtest_config.lldbExec)
-path = distutils.spawn.find_executable("clang", lldb_dir)
+path = shutil.which("clang", path=lldb_dir)
 if path is not None:
 return path
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124601: [lldb] Use shutil.which instead of distutils find_executable

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: JDevlieghere.
DavidSpickett added a comment.

https://reviews.llvm.org/D124429 unblocked doing this.

The github issue is https://github.com/llvm/llvm-project/issues/54337.

Once I know this is ok with the bots I'll see what I can do about our various 
`which` re-implementations (I'm sure they have quirks that are easy to miss).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124601

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

[Lldb-commits] [PATCH] D124370: [lldb] Fix PR54761

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 425727.
labath added a comment.

Here's the updated version. I'll place my new findings into the patch
description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124370

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/test/API/lang/cpp/incomplete-types/members/Makefile
  
lldb/test/API/lang/cpp/incomplete-types/members/TestCppIncompleteTypeMembers.py
  lldb/test/API/lang/cpp/incomplete-types/members/a.h
  lldb/test/API/lang/cpp/incomplete-types/members/f.cpp
  lldb/test/API/lang/cpp/incomplete-types/members/g.cpp
  lldb/test/API/lang/cpp/incomplete-types/members/main.cpp

Index: lldb/test/API/lang/cpp/incomplete-types/members/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/incomplete-types/members/main.cpp
@@ -0,0 +1,9 @@
+#include "a.h"
+
+A::A() = default;
+void A::anchor() {}
+
+int main() {
+  A().f();
+  A().g();
+}
Index: lldb/test/API/lang/cpp/incomplete-types/members/g.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/incomplete-types/members/g.cpp
@@ -0,0 +1,8 @@
+#include "a.h"
+
+int A::g() {
+  struct Ag {
+int a, b;
+  } ag{47, 42};
+  return ag.a + ag.b; // break here
+}
Index: lldb/test/API/lang/cpp/incomplete-types/members/f.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/incomplete-types/members/f.cpp
@@ -0,0 +1,8 @@
+#include "a.h"
+
+int A::f() {
+  struct Af {
+int x, y;
+  } af{42, 47};
+  return af.x + af.y; // break here
+}
Index: lldb/test/API/lang/cpp/incomplete-types/members/a.h
===
--- /dev/null
+++ lldb/test/API/lang/cpp/incomplete-types/members/a.h
@@ -0,0 +1,14 @@
+#ifndef A_H
+#define A_H
+
+class A {
+public:
+  A();
+  virtual void anchor();
+  int f();
+  int g();
+
+  int member = 47;
+};
+
+#endif
Index: lldb/test/API/lang/cpp/incomplete-types/members/TestCppIncompleteTypeMembers.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/incomplete-types/members/TestCppIncompleteTypeMembers.py
@@ -0,0 +1,32 @@
+"""
+Test situations where we don't have a definition for a type, but we have (some)
+of its member functions.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestCppIncompleteTypeMembers(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def test(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// break here",
+lldb.SBFileSpec("f.cpp"))
+
+# Sanity check that we really have to debug info for this type.
+this = self.expect_var_path("this", type="A *")
+self.assertEquals(this.GetType().GetPointeeType().GetNumberOfFields(),
+0, str(this))
+
+self.expect_var_path("af.x", value='42')
+
+lldbutil.run_break_set_by_source_regexp(self, "// break here",
+extra_options="-f g.cpp")
+self.runCmd("continue")
+
+self.expect_var_path("ag.a", value='47')
Index: lldb/test/API/lang/cpp/incomplete-types/members/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/incomplete-types/members/Makefile
@@ -0,0 +1,10 @@
+CXX_SOURCES := main.cpp f.cpp g.cpp
+
+include Makefile.rules
+
+# Force main.cpp to be built with no debug information
+main.o: CFLAGS = $(CFLAGS_NO_DEBUG)
+
+# And force -flimit-debug-info on the rest.
+f.o: CFLAGS_EXTRAS += $(LIMIT_DEBUG_INFO_FLAGS)
+g.o: CFLAGS_EXTRAS += $(LIMIT_DEBUG_INFO_FLAGS)
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1040,10 +1040,8 @@
 // struct and see if this is actually a C++ method
 Type *class_type = dwarf->ResolveType(decl_ctx_die);
 if (class_type) {
-  bool alternate_defn = false;
   if (class_type->GetID() != decl_ctx_die.GetID() ||
   IsClangModuleFwdDecl(decl_ctx_die)) {
-alternate_defn = true;
 
 // We uniqued the parent class of this function to another
 // class so we now need to associate all dies under
@@ -1112,7 +1110,7 @@
 CompilerType class_opaque_type =
 class_type->GetForwardCompilerType();
 if (TypeSystemClang::IsCXXClassType(class_opaque_type)) {
-  if (class_opaque_type.IsBeingDefined() || alternate_defn) {
+  if (class_opaque_type.IsBeingDefined()) {
 if (!is

[Lldb-commits] [PATCH] D124198: [LLDB][Unwind] Add stack scanning as fallback unwind plan if no symbol file is available.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D124198#3468039 , @clayborg wrote:

> So there is real benefit to doing these checks IMHO as it can help us get 
> back on track in live debug sessions. Core file sessions results will vary 
> depending on it we actually have the original object files or not, many times 
> we do not have this info, in which case we would just enable RA search and we 
> might be able to unwind the PC, but not other registers if we can't actually 
> find the prologue or if we don't have stack frame details from breakpad

I'm not saying we can't do that, but I don't think it has done be done straight 
in the first version. A lot of these checks are really just heuristics and can 
have false positives. For example, if we disqualify return addresses pointing 
to the first instruction of a function, then these  cases won't work:

- noreturn functions (the `call` could be the last instruction in the previous 
function)
- signal handler frames, where the handler's return address points to the first 
instruction of the trampoline (because it didn't really call it, but it still 
knows how to unwind from it)

I think it would be better discuss individual checks independently of the 
overall infrastructure (and ideally with more data).

That said, it would definitely be nice give some indication of how much do we 
trust the backtrace that we've computed. A separate command or a setting kind 
of achieve that, but they're very coarse-grained (you don't know how much trust 
to put into a particular backtrace), and they need the user to know of their 
existence. I am wondering whether attaching some kind of flag to a particular 
frame (similar to how we add `[opt]` for optimized functions) to indicate that 
it may not be entirely correct could be a better option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124198

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

[Lldb-commits] [PATCH] D124604: [lldb] Use shutil.which in Shell tests find_executable

[David Spickett via Phabricator via lldb-commits]

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

In build.py we have our own find_executable that looks
a lot like the distutils one that I switched to shutil.which.

This find_executable isn't quite the same as shutil.which
so I've refactored it to call that in the correct way.

Note that the path passed to shutil.which is in the form that
PATH would be, meaning separators are allowed.

>>> shutil.which("gcc", path="/home/david.spickett:/bin")

'/bin/gcc'

We just need to make sure it doesn't ignore the existing PATH
and normalise the result if it does find the binary.

The .exe extension is automatically added to the binary name
if we are on Windows.

Depends on D124601 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124604

Files:
  lldb/test/Shell/helper/build.py


Index: lldb/test/Shell/helper/build.py
===
--- lldb/test/Shell/helper/build.py
+++ lldb/test/Shell/helper/build.py
@@ -4,6 +4,7 @@
 
 import argparse
 import os
+import shutil
 import signal
 import subprocess
 import sys
@@ -170,16 +171,14 @@
 print('{0} = {1}'.format(e, formatted_value))
 
 def find_executable(binary_name, search_paths):
-if sys.platform == 'win32':
-binary_name = binary_name + '.exe'
-
-search_paths = os.pathsep.join(search_paths)
-paths = search_paths + os.pathsep + os.environ.get('PATH', '')
-for path in paths.split(os.pathsep):
-p = os.path.join(path, binary_name)
-if os.path.exists(p) and not os.path.isdir(p):
-return os.path.normpath(p)
-return None
+# shutil.which will ignore PATH if given a path argument, we want to 
include it.
+search_paths.append(os.environ.get('PATH', ''))
+search_path = os.pathsep.join(search_paths)
+binary_path = shutil.which(binary_name, path=search_path)
+if binary_path is not None:
+# So for example, we get '/bin/gcc' instead of '/usr/../bin/gcc'.
+binary_path = os.path.normpath(binary_path)
+return binary_path
 
 def find_toolchain(compiler, tools_dir):
 if compiler == 'msvc':


Index: lldb/test/Shell/helper/build.py
===
--- lldb/test/Shell/helper/build.py
+++ lldb/test/Shell/helper/build.py
@@ -4,6 +4,7 @@
 
 import argparse
 import os
+import shutil
 import signal
 import subprocess
 import sys
@@ -170,16 +171,14 @@
 print('{0} = {1}'.format(e, formatted_value))
 
 def find_executable(binary_name, search_paths):
-if sys.platform == 'win32':
-binary_name = binary_name + '.exe'
-
-search_paths = os.pathsep.join(search_paths)
-paths = search_paths + os.pathsep + os.environ.get('PATH', '')
-for path in paths.split(os.pathsep):
-p = os.path.join(path, binary_name)
-if os.path.exists(p) and not os.path.isdir(p):
-return os.path.normpath(p)
-return None
+# shutil.which will ignore PATH if given a path argument, we want to include it.
+search_paths.append(os.environ.get('PATH', ''))
+search_path = os.pathsep.join(search_paths)
+binary_path = shutil.which(binary_name, path=search_path)
+if binary_path is not None:
+# So for example, we get '/bin/gcc' instead of '/usr/../bin/gcc'.
+binary_path = os.path.normpath(binary_path)
+return binary_path
 
 def find_toolchain(compiler, tools_dir):
 if compiler == 'msvc':
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 0cbad66 - [lldb/DWARF] Fix a typo in 57f99d0dc3

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2022-04-28T15:11:00+02:00
New Revision: 0cbad6635475952fb432ac2af8b1fce202aaeb6e

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

LOG: [lldb/DWARF] Fix a typo in 57f99d0dc3

The lambda should take a reference argument.

Added: 


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

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 296ec524623e..8662578336bd 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -3500,7 +3500,7 @@ bool DWARFASTParserClang::CopyUniqueClassMethodTypes(
   class_type->GetFullCompilerType();
 
   auto gather = [](DWARFDIE die, UniqueCStringMap &map,
-   UniqueCStringMap map_artificial) {
+   UniqueCStringMap &map_artificial) {
 if (die.Tag() != DW_TAG_subprogram)
   return;
 // Make sure this is a declaration and not a concrete instance by looking



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

[Lldb-commits] [lldb] b809c4c - [lldb] Add FixAnyAddress to ABI plugins

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2022-04-28T14:57:40+01:00
New Revision: b809c4cdb70a100c85524555a14332f22ede1b7a

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

LOG: [lldb] Add FixAnyAddress to ABI plugins

FixAnyAddress is to be used when we don't know or don't care
whether we're fixing a code or data address.

By using FixAnyAddress over the others, you document that no
specific choice was made.

On all existing platforms apart from Arm Thumb, you could use
either FixCodeAddress or FixDataAddress and be fine. Up until
now I've chosen to use FixDataAddress but if I had
chosen to use FixCodeAddress that would have broken Arm Thumb.

Hence FixAnyAddress, to give you the "safest" option when you're
in generic code.

Uses of FixDataAddress in memory region code have been changed
to FixAnyAddress. The functionality is unchanged.

Reviewed By: omjavaid, JDevlieghere

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

Added: 


Modified: 
lldb/include/lldb/Target/ABI.h
lldb/source/Commands/CommandObjectMemory.cpp
lldb/source/Target/Process.cpp

Removed: 




diff  --git a/lldb/include/lldb/Target/ABI.h b/lldb/include/lldb/Target/ABI.h
index 8ac6003554d5e..f0753172d3e71 100644
--- a/lldb/include/lldb/Target/ABI.h
+++ b/lldb/include/lldb/Target/ABI.h
@@ -126,6 +126,20 @@ class ABI : public PluginInterface {
   virtual lldb::addr_t FixDataAddress(lldb::addr_t pc) { return pc; }
   /// @}
 
+  /// Use this method when you do not know, or do not care what kind of address
+  /// you are fixing. On platforms where there would be a 
diff erence between the
+  /// two types, it will pick the safest option.
+  ///
+  /// Its purpose is to signal that no specific choice was made and provide an
+  /// alternative to randomly picking FixCode/FixData address. Which could 
break
+  /// platforms where there is a 
diff erence (only Arm Thumb at this time).
+  virtual lldb::addr_t FixAnyAddress(lldb::addr_t pc) {
+// On Arm Thumb fixing a code address zeroes the bottom bit, so FixData is
+// the safe choice. On any other platform (so far) code and data addresses
+// are fixed in the same way.
+return FixDataAddress(pc);
+  }
+
   llvm::MCRegisterInfo &GetMCRegisterInfo() { return *m_mc_register_info_up; }
 
   virtual void

diff  --git a/lldb/source/Commands/CommandObjectMemory.cpp 
b/lldb/source/Commands/CommandObjectMemory.cpp
index 4e63802befdbd..e94306ce33bfe 100644
--- a/lldb/source/Commands/CommandObjectMemory.cpp
+++ b/lldb/source/Commands/CommandObjectMemory.cpp
@@ -1688,7 +1688,7 @@ class CommandObjectMemoryRegion : public 
CommandObjectParsed {
// address size, etc.), the end of mappable memory will be lower
// than that. So if we find any non-address bit set, we must be
// at the end of the mappable range.
-   (abi && (abi->FixDataAddress(load_addr) != load_addr))) {
+   (abi && (abi->FixAnyAddress(load_addr) != load_addr))) {
   result.AppendErrorWithFormat("'%s' takes one argument:\nUsage: %s\n",
m_cmd_name.c_str(), m_cmd_syntax.c_str());
   return false;

diff  --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 5cbe4b66fceb4..79adb2c9d1550 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -5831,7 +5831,7 @@ Process::AdvanceAddressToNextBranchInstruction(Address 
default_stop_addr,
 Status Process::GetMemoryRegionInfo(lldb::addr_t load_addr,
 MemoryRegionInfo &range_info) {
   if (const lldb::ABISP &abi = GetABI())
-load_addr = abi->FixDataAddress(load_addr);
+load_addr = abi->FixAnyAddress(load_addr);
   return DoGetMemoryRegionInfo(load_addr, range_info);
 }
 
@@ -5866,7 +5866,7 @@ Status 
Process::GetMemoryRegions(lldb_private::MemoryRegionInfos ®ion_list) {
   range_end != LLDB_INVALID_ADDRESS &&
   // If we have non-address bits and some are set then the end
   // is at or beyond the end of mappable memory.
-  !(abi && (abi->FixDataAddress(range_end) != range_end)));
+  !(abi && (abi->FixAnyAddress(range_end) != range_end)));
 
   return error;
 }



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

[Lldb-commits] [PATCH] D124000: [lldb] Add FixAnyAddress to ABI plugins

[David Spickett via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb809c4cdb70a: [lldb] Add FixAnyAddress to ABI plugins 
(authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124000

Files:
  lldb/include/lldb/Target/ABI.h
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/source/Target/Process.cpp


Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -5831,7 +5831,7 @@
 Status Process::GetMemoryRegionInfo(lldb::addr_t load_addr,
 MemoryRegionInfo &range_info) {
   if (const lldb::ABISP &abi = GetABI())
-load_addr = abi->FixDataAddress(load_addr);
+load_addr = abi->FixAnyAddress(load_addr);
   return DoGetMemoryRegionInfo(load_addr, range_info);
 }
 
@@ -5866,7 +5866,7 @@
   range_end != LLDB_INVALID_ADDRESS &&
   // If we have non-address bits and some are set then the end
   // is at or beyond the end of mappable memory.
-  !(abi && (abi->FixDataAddress(range_end) != range_end)));
+  !(abi && (abi->FixAnyAddress(range_end) != range_end)));
 
   return error;
 }
Index: lldb/source/Commands/CommandObjectMemory.cpp
===
--- lldb/source/Commands/CommandObjectMemory.cpp
+++ lldb/source/Commands/CommandObjectMemory.cpp
@@ -1688,7 +1688,7 @@
// address size, etc.), the end of mappable memory will be lower
// than that. So if we find any non-address bit set, we must be
// at the end of the mappable range.
-   (abi && (abi->FixDataAddress(load_addr) != load_addr))) {
+   (abi && (abi->FixAnyAddress(load_addr) != load_addr))) {
   result.AppendErrorWithFormat("'%s' takes one argument:\nUsage: %s\n",
m_cmd_name.c_str(), m_cmd_syntax.c_str());
   return false;
Index: lldb/include/lldb/Target/ABI.h
===
--- lldb/include/lldb/Target/ABI.h
+++ lldb/include/lldb/Target/ABI.h
@@ -126,6 +126,20 @@
   virtual lldb::addr_t FixDataAddress(lldb::addr_t pc) { return pc; }
   /// @}
 
+  /// Use this method when you do not know, or do not care what kind of address
+  /// you are fixing. On platforms where there would be a difference between 
the
+  /// two types, it will pick the safest option.
+  ///
+  /// Its purpose is to signal that no specific choice was made and provide an
+  /// alternative to randomly picking FixCode/FixData address. Which could 
break
+  /// platforms where there is a difference (only Arm Thumb at this time).
+  virtual lldb::addr_t FixAnyAddress(lldb::addr_t pc) {
+// On Arm Thumb fixing a code address zeroes the bottom bit, so FixData is
+// the safe choice. On any other platform (so far) code and data addresses
+// are fixed in the same way.
+return FixDataAddress(pc);
+  }
+
   llvm::MCRegisterInfo &GetMCRegisterInfo() { return *m_mc_register_info_up; }
 
   virtual void


Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -5831,7 +5831,7 @@
 Status Process::GetMemoryRegionInfo(lldb::addr_t load_addr,
 MemoryRegionInfo &range_info) {
   if (const lldb::ABISP &abi = GetABI())
-load_addr = abi->FixDataAddress(load_addr);
+load_addr = abi->FixAnyAddress(load_addr);
   return DoGetMemoryRegionInfo(load_addr, range_info);
 }
 
@@ -5866,7 +5866,7 @@
   range_end != LLDB_INVALID_ADDRESS &&
   // If we have non-address bits and some are set then the end
   // is at or beyond the end of mappable memory.
-  !(abi && (abi->FixDataAddress(range_end) != range_end)));
+  !(abi && (abi->FixAnyAddress(range_end) != range_end)));
 
   return error;
 }
Index: lldb/source/Commands/CommandObjectMemory.cpp
===
--- lldb/source/Commands/CommandObjectMemory.cpp
+++ lldb/source/Commands/CommandObjectMemory.cpp
@@ -1688,7 +1688,7 @@
// address size, etc.), the end of mappable memory will be lower
// than that. So if we find any non-address bit set, we must be
// at the end of the mappable range.
-   (abi && (abi->FixDataAddress(load_addr) != load_addr))) {
+   (abi && (abi->FixAnyAddress(load_addr) != load_addr))) {
   result.AppendErrorWithFormat("'%s' takes one argument:\nUsage: %s\n",
m_cmd_name.c_str(), m_cmd_syntax.c_str());
   return false;
Index: lldb/include/lldb/Target/ABI.h
===
--- lldb/include/lldb/Target/ABI

[Lldb-commits] [PATCH] D123743: [lldb] Show the DBGError if dsymForUUID can't find a dSYM

[Stephen Tozer via Phabricator via lldb-commits]

StephenTozer added inline comments.



Comment at: lldb/source/Symbol/LocateSymbolFileMacOSX.cpp.rej:1
+***
+*** 337,343 

This file was presumably included by accident?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123743

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

[Lldb-commits] [PATCH] D118794: [lldb] Remove non-address bits from read/write addresses in lldb

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 425778.
DavidSpickett added a comment.

Use FixAnyAddress instead of FixDataAddress.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118794

Files:
  lldb/source/Target/Process.cpp
  lldb/source/Target/ProcessTrace.cpp
  lldb/source/Target/Target.cpp
  lldb/test/API/linux/aarch64/non_address_bit_memory_access/Makefile
  
lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py
  lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c

Index: lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c
@@ -0,0 +1,25 @@
+#include 
+#include 
+#include 
+
+int main(int argc, char const *argv[]) {
+  size_t page_size = sysconf(_SC_PAGESIZE);
+  // Note that we allocate memory here because if we used
+  // stack or globals lldb might read it in the course of
+  // running to the breakpoint. Before the test can look
+  // for those reads.
+  char *buf = mmap(0, page_size, PROT_READ | PROT_WRITE,
+   MAP_ANONYMOUS | MAP_SHARED, -1, 0);
+  if (buf == MAP_FAILED)
+return 1;
+
+#define sign_ptr(ptr) __asm__ __volatile__("pacdza %0" : "=r"(ptr) : "r"(ptr))
+
+  // Set top byte to something.
+  char *buf_with_non_address = (char *)((size_t)buf | (size_t)0xff << 56);
+  sign_ptr(buf_with_non_address);
+  // Address is now:
+  // <8 bit top byte tag>
+
+  return 0; // Set break point at this line.
+}
Index: lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py
@@ -0,0 +1,182 @@
+"""
+Test that lldb removes non-address bits in situations where they would cause
+failures if not removed. Like when reading memory. Tests are done at command
+and API level because commands may remove non-address bits for display
+reasons which can make it seem like the operation as a whole works but at the
+API level it won't if we don't remove them there also.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AArch64LinuxNonAddressBitMemoryAccessTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+NO_DEBUG_INFO_TESTCASE = True
+
+def setup_test(self):
+if not self.isAArch64PAuth():
+self.skipTest('Target must support pointer authentication.')
+
+self.build()
+self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(self, "main.c",
+line_number('main.c', '// Set break point at this line.'),
+num_expected_locations=1)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+if self.process().GetState() == lldb.eStateExited:
+self.fail("Test program failed to run.")
+
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+def check_cmd_read_write(self, write_to, read_from, data):
+self.runCmd("memory write {} {}".format(write_to, data))
+self.expect("memory read {}".format(read_from),
+substrs=[data])
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+def test_non_address_bit_memory_read_write_cmds(self):
+self.setup_test()
+
+# Writes should be visible through either pointer
+self.check_cmd_read_write("buf", "buf", "01 02 03 04")
+self.check_cmd_read_write("buf_with_non_address", "buf_with_non_address", "02 03 04 05")
+self.check_cmd_read_write("buf", "buf_with_non_address", "03 04 05 06")
+self.check_cmd_read_write("buf_with_non_address", "buf", "04 05 06 07")
+
+# Printing either should get the same result
+self.expect("expression -f hex -- *(uint32_t*)buf", substrs=["0x07060504"])
+self.expect("expression -f hex -- *(uint32_t*)buf_with_non_address",
+substrs=["0x07060504"])
+
+def get_ptr_values(self):
+frame  = self.process().GetThreadAtIndex(0).GetFrameAtIndex(0)
+buf = frame.FindVariable("buf").GetValueAsUnsigned()
+buf_with_non_address = frame.FindVariable("buf_with_non_address").GetValueAsUnsigned()
+return buf, buf_with_non_address
+
+def check_api_read_write(self, write_to, read_from, data):
+error = lldb.SBError()
+written = self.process().WriteMemory(write_to, data, error)
+self.assertTrue(error.Success())
+self.assertEqual(len(data), written)
+buf_content = 

[Lldb-commits] [PATCH] D122411: [lldb][AArch64] Fix corefile memory reads when there are non-address bits

[David Spickett via Phabricator via lldb-commits]

DavidSpickett updated this revision to Diff 425779.
DavidSpickett added a comment.

Use FixAnyAddress.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122411

Files:
  lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
  lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
  
lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py
  lldb/test/API/linux/aarch64/non_address_bit_memory_access/corefile
  lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c

Index: lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c
===
--- lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c
+++ lldb/test/API/linux/aarch64/non_address_bit_memory_access/main.c
@@ -13,6 +13,13 @@
   if (buf == MAP_FAILED)
 return 1;
 
+  // Some known values to go in the corefile, since we cannot
+  // write to corefile memory.
+  buf[0] = 'L';
+  buf[1] = 'L';
+  buf[2] = 'D';
+  buf[3] = 'B';
+
 #define sign_ptr(ptr) __asm__ __volatile__("pacdza %0" : "=r"(ptr) : "r"(ptr))
 
   // Set top byte to something.
@@ -21,5 +28,11 @@
   // Address is now:
   // <8 bit top byte tag>
 
+  // Uncomment this line to crash and generate a corefile.
+  // Prints so we know what fixed address to look for in testing.
+  // printf("buf: %p\n", buf);
+  // printf("buf_with_non_address: %p\n", buf_with_non_address);
+  // *(char*)0 = 0;
+
   return 0; // Set break point at this line.
 }
Index: lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py
===
--- lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py
+++ lldb/test/API/linux/aarch64/non_address_bit_memory_access/TestAArch64LinuxNonAddressBitMemoryAccess.py
@@ -163,6 +163,10 @@
 # Open log ignoring utf-8 decode errors
 with open(log_file, 'r', errors='ignore') as f:
 read_packet = "send packet: $x{:x}"
+
+# Since we allocated a 4k page that page will be aligned to 4k, which
+# also fits the 512 byte line size of the memory cache. So we can expect
+# to find a packet with its exact address.
 read_buf_packet = read_packet.format(buf)
 read_buf_with_non_address_packet = read_packet.format(buf_with_non_address)
 
@@ -180,3 +184,51 @@
 
 if not found_read_buf:
 self.fail("Did not find any reads of buf.")
+
+@skipIfLLVMTargetMissing("AArch64")
+def test_non_address_bit_memory_corefile(self):
+self.runCmd("target create --core corefile")
+
+self.expect("thread list", substrs=['stopped',
+'stop reason = signal SIGSEGV'])
+
+# No caching (the program/corefile are the cache) and no writing
+# to memory. So just check that tagged/untagged addresses read
+# the same location.
+
+# These are known addresses in the corefile, since we won't have symbols.
+buf = 0xa75a5000
+buf_with_non_address = 0xff0ba75a5000
+
+expected = ["4c 4c 44 42", "LLDB"]
+self.expect("memory read 0x{:x}".format(buf), substrs=expected)
+self.expect("memory read 0x{:x}".format(buf_with_non_address), substrs=expected)
+
+# This takes a more direct route to ReadMemory. As opposed to "memory read"
+# above that might fix the addresses up front for display reasons.
+self.expect("expression (char*)0x{:x}".format(buf), substrs=["LLDB"])
+self.expect("expression (char*)0x{:x}".format(buf_with_non_address), substrs=["LLDB"])
+
+def check_reads(addrs, method, num_bytes, expected):
+error = lldb.SBError()
+for addr in addrs:
+if num_bytes is None:
+got = method(addr, error)
+else:
+got = method(addr, num_bytes, error)
+
+self.assertTrue(error.Success())
+self.assertEqual(expected, got)
+
+addr_buf = lldb.SBAddress()
+addr_buf.SetLoadAddress(buf, self.target())
+addr_buf_with_non_address = lldb.SBAddress()
+addr_buf_with_non_address.SetLoadAddress(buf_with_non_address, self.target())
+check_reads([addr_buf, addr_buf_with_non_address], self.target().ReadMemory,
+4, b'LLDB')
+
+addrs = [buf, buf_with_non_address]
+check_reads(addrs, self.process().ReadMemory, 4, b'LLDB')
+check_reads(addrs, self.process().ReadCStringFromMemory, 5, "LLDB")
+check_reads(addrs, self.process().ReadUnsignedFromMemory, 4, 0x42444c4c)
+check_reads(addrs, self.process().ReadPointerFromMemory, None, 0x42444c4c)
Index: lldb/source/Plugins/Process/elf-core/ProcessElfCo

[Lldb-commits] [lldb] f6b7fd2 - [lldb] Remove patch reject file (.rej)

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-04-28T08:16:26-07:00
New Revision: f6b7fd20a52ef83d0462db190eb40800afda2506

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

LOG: [lldb] Remove patch reject file (.rej)

Added: 


Modified: 


Removed: 
lldb/source/Symbol/LocateSymbolFileMacOSX.cpp.rej



diff  --git a/lldb/source/Symbol/LocateSymbolFileMacOSX.cpp.rej 
b/lldb/source/Symbol/LocateSymbolFileMacOSX.cpp.rej
deleted file mode 100644
index 69a1fe1761453..0
--- a/lldb/source/Symbol/LocateSymbolFileMacOSX.cpp.rej
+++ /dev/null
@@ -1,16 +0,0 @@
-***
-*** 337,343 
-  
-  std::string DBGBuildSourcePath;
-  std::string DBGSourcePath;
-- std::string DBGError;
-  
-  // If DBGVersion 1 or DBGVersion missing, ignore DBGSourcePathRemapping.
-  // If DBGVersion 2, strip last two components of path remappings from
 346,351 
-  
-  std::string DBGBuildSourcePath;
-  std::string DBGSourcePath;
-  
-  // If DBGVersion 1 or DBGVersion missing, ignore DBGSourcePathRemapping.
-  // If DBGVersion 2, strip last two components of path remappings from



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

[Lldb-commits] [PATCH] D124572: Fix the encoding and decoding of UniqueCStringMap objects when saved to cache files.

[jeffrey tan via Phabricator via lldb-commits]

yinghuitan added a comment.

Also, to help diagnostics this kind of issue in future, it may worth to add an 
extra debug mode check in in `UniqueCStringMap` to ensure they are sorted. For 
example, in debug mode, add a `m_isSorted` flag which is set by 
`UniqueCStringMap::Sort()` method. All the binary search methods should assert 
`m_isSorted == true`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124572

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

[Lldb-commits] [PATCH] D124409: Filter non-external static members from SBType::GetFieldAtIndex.

[Sigurður Ásgeirsson via Phabricator via lldb-commits]

siggi-alpheus updated this revision to Diff 425790.
siggi-alpheus added a comment.

Add a test that fails against the ToT lldb.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124409

Files:
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_static-member-anonymous-namespace.s

Index: lldb/test/Shell/SymbolFile/DWARF/x86/debug_static-member-anonymous-namespace.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/debug_static-member-anonymous-namespace.s
@@ -0,0 +1,322 @@
+# RUN: llvm-mc --triple=x86_64-pc-linux --filetype=obj %s -o %t
+# RUN: %lldb -o "target variable constant" -b %t | FileCheck %s
+
+# CHECK: (U) constant = (m = 0)
+
+# This tests that a static member in a class declared in the anonymous namespace
+# does not appear as a field of the class. There is a difference between the
+# debug info generated by gcc and clang, where clang flags the static member
+# with DW_AT_external, but gcc does not.
+#
+# This is the code used to generate the assembly:
+#
+# namespace {
+# struct U {
+#   static const int s = 0;
+#  int m;
+# };
+# }
+#
+# U constant;
+#
+# int main() {
+#   return constant.m;
+# }
+#
+# Compiled with:
+#   gcc -S -g test.cpp
+#
+
+	.file	"test.cpp"
+	.text
+.Ltext0:
+	.local	constant
+	.comm	constant,4,4
+	.globl	main
+	.type	main, @function
+main:
+.LFB0:
+	.file 1 "test.cpp"
+	.loc 1 10 12
+	.cfi_startproc
+	endbr64
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset 6, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register 6
+	.loc 1 11 19
+	movl	constant(%rip), %eax
+	.loc 1 12 1
+	popq	%rbp
+	.cfi_def_cfa 7, 8
+	ret
+	.cfi_endproc
+.LFE0:
+	.size	main, .-main
+.Letext0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.long	0x96
+	.value	0x4
+	.long	.Ldebug_abbrev0
+	.byte	0x8
+	.uleb128 0x1
+	.long	.LASF0
+	.byte	0x4
+	.long	.LASF1
+	.long	.LASF2
+	.quad	.Ltext0
+	.quad	.Letext0-.Ltext0
+	.long	.Ldebug_line0
+	.uleb128 0x2
+	.long	0x51
+	.uleb128 0x3
+	.string	"U"
+	.byte	0x4
+	.byte	0x1
+	.byte	0x2
+	.byte	0x8
+	.uleb128 0x4
+	.string	"s"
+	.byte	0x1
+	.byte	0x3
+	.byte	0x14
+	.long	0x60
+	.byte	0
+	.uleb128 0x5
+	.string	"m"
+	.byte	0x1
+	.byte	0x4
+	.byte	0x6
+	.long	0x59
+	.byte	0
+	.byte	0
+	.byte	0
+	.uleb128 0x6
+	.byte	0x1
+	.byte	0x1
+	.byte	0x1
+	.long	0x2d
+	.uleb128 0x7
+	.byte	0x4
+	.byte	0x5
+	.string	"int"
+	.uleb128 0x8
+	.long	0x59
+	.uleb128 0x9
+	.long	.LASF3
+	.byte	0x1
+	.byte	0x8
+	.byte	0x3
+	.long	0x32
+	.uleb128 0x9
+	.byte	0x3
+	.quad	constant
+	.uleb128 0xa
+	.long	.LASF4
+	.byte	0x1
+	.byte	0xa
+	.byte	0x5
+	.long	0x59
+	.quad	.LFB0
+	.quad	.LFE0-.LFB0
+	.uleb128 0x1
+	.byte	0x9c
+	.byte	0
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.uleb128 0x1
+	.uleb128 0x11
+	.byte	0x1
+	.uleb128 0x25
+	.uleb128 0xe
+	.uleb128 0x13
+	.uleb128 0xb
+	.uleb128 0x3
+	.uleb128 0xe
+	.uleb128 0x1b
+	.uleb128 0xe
+	.uleb128 0x11
+	.uleb128 0x1
+	.uleb128 0x12
+	.uleb128 0x7
+	.uleb128 0x10
+	.uleb128 0x17
+	.byte	0
+	.byte	0
+	.uleb128 0x2
+	.uleb128 0x39
+	.byte	0x1
+	.uleb128 0x1
+	.uleb128 0x13
+	.byte	0
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0x13
+	.byte	0x1
+	.uleb128 0x3
+	.uleb128 0x8
+	.uleb128 0xb
+	.uleb128 0xb
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.byte	0
+	.byte	0
+	.uleb128 0x4
+	.uleb128 0xd
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0x8
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.uleb128 0x3c
+	.uleb128 0x19
+	.uleb128 0x1c
+	.uleb128 0xb
+	.byte	0
+	.byte	0
+	.uleb128 0x5
+	.uleb128 0xd
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0x8
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.uleb128 0x38
+	.uleb128 0xb
+	.byte	0
+	.byte	0
+	.uleb128 0x6
+	.uleb128 0x3a
+	.byte	0
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x18
+	.uleb128 0x13
+	.byte	0
+	.byte	0
+	.uleb128 0x7
+	.uleb128 0x24
+	.byte	0
+	.uleb128 0xb
+	.uleb128 0xb
+	.uleb128 0x3e
+	.uleb128 0xb
+	.uleb128 0x3
+	.uleb128 0x8
+	.byte	0
+	.byte	0
+	.uleb128 0x8
+	.uleb128 0x26
+	.byte	0
+	.uleb128 0x49
+	.uleb128 0x13
+	.byte	0
+	.byte	0
+	.uleb128 0x9
+	.uleb128 0x34
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0xe
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.uleb128 0x2
+	.uleb128 0x18
+	.byte	0
+	.byte	0
+	.uleb128 0xa
+	.uleb128 0x2e
+	.byte	0
+	.uleb128 0x3f
+	.uleb128 0x19
+	.uleb128 0x3
+	.uleb128 0xe
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.uleb128 0x11
+	.uleb128 0x1
+	.uleb128 0x12
+	.uleb128 0x7
+	.uleb128 0x40
+	.uleb128 0x18
+	.uleb128 0x2117
+	.uleb128 0x19
+	.byte	0
+	.byte	0
+	.byte	0
+	.section	.debug_ar

[Lldb-commits] [PATCH] D124409: Filter non-external static members from SBType::GetFieldAtIndex.

[Sigurður Ásgeirsson via Phabricator via lldb-commits]

siggi-alpheus added a comment.

Please take a look. This now passes all tests locally, and adds a test that 
fails against the current ToT lldb.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124409

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

[Lldb-commits] [PATCH] D124409: Filter non-external static members from SBType::GetFieldAtIndex.

[Sigurður Ásgeirsson via Phabricator via lldb-commits]

siggi-alpheus updated this revision to Diff 425792.
siggi-alpheus added a comment.

A full diff from origin/main - I hope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124409

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/test/Shell/SymbolFile/DWARF/x86/debug_static-member-anonymous-namespace.s

Index: lldb/test/Shell/SymbolFile/DWARF/x86/debug_static-member-anonymous-namespace.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/x86/debug_static-member-anonymous-namespace.s
@@ -0,0 +1,322 @@
+# RUN: llvm-mc --triple=x86_64-pc-linux --filetype=obj %s -o %t
+# RUN: %lldb -o "target variable constant" -b %t | FileCheck %s
+
+# CHECK: (U) constant = (m = 0)
+
+# This tests that a static member in a class declared in the anonymous namespace
+# does not appear as a field of the class. There is a difference between the
+# debug info generated by gcc and clang, where clang flags the static member
+# with DW_AT_external, but gcc does not.
+#
+# This is the code used to generate the assembly:
+#
+# namespace {
+# struct U {
+#   static const int s = 0;
+#  int m;
+# };
+# }
+#
+# U constant;
+#
+# int main() {
+#   return constant.m;
+# }
+#
+# Compiled with:
+#   gcc -S -g test.cpp
+#
+
+	.file	"test.cpp"
+	.text
+.Ltext0:
+	.local	constant
+	.comm	constant,4,4
+	.globl	main
+	.type	main, @function
+main:
+.LFB0:
+	.file 1 "test.cpp"
+	.loc 1 10 12
+	.cfi_startproc
+	endbr64
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset 6, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register 6
+	.loc 1 11 19
+	movl	constant(%rip), %eax
+	.loc 1 12 1
+	popq	%rbp
+	.cfi_def_cfa 7, 8
+	ret
+	.cfi_endproc
+.LFE0:
+	.size	main, .-main
+.Letext0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.long	0x96
+	.value	0x4
+	.long	.Ldebug_abbrev0
+	.byte	0x8
+	.uleb128 0x1
+	.long	.LASF0
+	.byte	0x4
+	.long	.LASF1
+	.long	.LASF2
+	.quad	.Ltext0
+	.quad	.Letext0-.Ltext0
+	.long	.Ldebug_line0
+	.uleb128 0x2
+	.long	0x51
+	.uleb128 0x3
+	.string	"U"
+	.byte	0x4
+	.byte	0x1
+	.byte	0x2
+	.byte	0x8
+	.uleb128 0x4
+	.string	"s"
+	.byte	0x1
+	.byte	0x3
+	.byte	0x14
+	.long	0x60
+	.byte	0
+	.uleb128 0x5
+	.string	"m"
+	.byte	0x1
+	.byte	0x4
+	.byte	0x6
+	.long	0x59
+	.byte	0
+	.byte	0
+	.byte	0
+	.uleb128 0x6
+	.byte	0x1
+	.byte	0x1
+	.byte	0x1
+	.long	0x2d
+	.uleb128 0x7
+	.byte	0x4
+	.byte	0x5
+	.string	"int"
+	.uleb128 0x8
+	.long	0x59
+	.uleb128 0x9
+	.long	.LASF3
+	.byte	0x1
+	.byte	0x8
+	.byte	0x3
+	.long	0x32
+	.uleb128 0x9
+	.byte	0x3
+	.quad	constant
+	.uleb128 0xa
+	.long	.LASF4
+	.byte	0x1
+	.byte	0xa
+	.byte	0x5
+	.long	0x59
+	.quad	.LFB0
+	.quad	.LFE0-.LFB0
+	.uleb128 0x1
+	.byte	0x9c
+	.byte	0
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.uleb128 0x1
+	.uleb128 0x11
+	.byte	0x1
+	.uleb128 0x25
+	.uleb128 0xe
+	.uleb128 0x13
+	.uleb128 0xb
+	.uleb128 0x3
+	.uleb128 0xe
+	.uleb128 0x1b
+	.uleb128 0xe
+	.uleb128 0x11
+	.uleb128 0x1
+	.uleb128 0x12
+	.uleb128 0x7
+	.uleb128 0x10
+	.uleb128 0x17
+	.byte	0
+	.byte	0
+	.uleb128 0x2
+	.uleb128 0x39
+	.byte	0x1
+	.uleb128 0x1
+	.uleb128 0x13
+	.byte	0
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0x13
+	.byte	0x1
+	.uleb128 0x3
+	.uleb128 0x8
+	.uleb128 0xb
+	.uleb128 0xb
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.byte	0
+	.byte	0
+	.uleb128 0x4
+	.uleb128 0xd
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0x8
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.uleb128 0x3c
+	.uleb128 0x19
+	.uleb128 0x1c
+	.uleb128 0xb
+	.byte	0
+	.byte	0
+	.uleb128 0x5
+	.uleb128 0xd
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0x8
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.uleb128 0x38
+	.uleb128 0xb
+	.byte	0
+	.byte	0
+	.uleb128 0x6
+	.uleb128 0x3a
+	.byte	0
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x18
+	.uleb128 0x13
+	.byte	0
+	.byte	0
+	.uleb128 0x7
+	.uleb128 0x24
+	.byte	0
+	.uleb128 0xb
+	.uleb128 0xb
+	.uleb128 0x3e
+	.uleb128 0xb
+	.uleb128 0x3
+	.uleb128 0x8
+	.byte	0
+	.byte	0
+	.uleb128 0x8
+	.uleb128 0x26
+	.byte	0
+	.uleb128 0x49
+	.uleb128 0x13
+	.byte	0
+	.byte	0
+	.uleb128 0x9
+	.uleb128 0x34
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0xe
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.uleb128 0x2
+	.uleb128 0x18
+	.byte	0
+	.byte	0
+	.uleb128 0xa
+	.uleb128 0x2e
+	.byte	0
+	.uleb128 0x3f
+	.uleb128 0x19
+	.uleb128 0x3
+	.uleb128 0xe
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x39
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.uleb128 0x11
+	.uleb128 0x1
+	.uleb128 0x12
+	.uleb128 0x7
+	.uleb128 0x40
+	.uleb128 0x18
+	.uleb128 0x2117
+	.uleb1

[Lldb-commits] [PATCH] D124601: [lldb] Use shutil.which instead of distutils find_executable

[Jonas Devlieghere via Phabricator via lldb-commits]

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

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124601

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

[Lldb-commits] [PATCH] D124573: [trace][intelpt] Support system-wide tracing [1] - Add a method for accessing the list of logical core ids

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 added inline comments.



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:22
+// List of data kinds used by jLLDBGetState and jLLDBGetBinaryData.
+struct IntelPTDataKinds {
+  static const char *kProcFsCpuInfo;

why not use an enum here?
having "kinds" in the name already makes it sound like an enum and seeing 
IntelPTDataKinds:: everywhere immediately made me thing it was an enum.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124573

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

[Lldb-commits] [PATCH] D123954: [lldb] Add setting for max depth of value object printing (NFC)

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/include/lldb/Interpreter/CommandInterpreter.h:718
+  /// been told.
+  ChildrenOmissionWarningStatus m_truncation_warning;
+  /// Whether we reached the maximum child nesting depth and whether the user

kastiglione wrote:
> shafik wrote:
> > Why not use in class member initialization? It looks like they have default 
> > values. I am not sure why the rest of the values where not caught the other 
> > day when I ran clang-tidy.
> I was following the convention within the file. What's the ideal change, 
> initialize all in this change? Or initialize the two that I have edited?
I think it's best to leave these as is, and make a follow up change that uses 
member initialization for all of these.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123954

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

[Lldb-commits] [PATCH] D123954: [lldb] Add setting for max depth of value object printing (NFC)

[Dave Lee via Phabricator via lldb-commits]

kastiglione updated this revision to Diff 425826.
kastiglione added a comment.

Fixed: comment formatting; bool logic bug


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123954

Files:
  lldb/include/lldb/DataFormatters/DumpValueObjectOptions.h
  lldb/include/lldb/DataFormatters/ValueObjectPrinter.h
  lldb/include/lldb/Interpreter/CommandInterpreter.h
  lldb/include/lldb/Interpreter/OptionGroupValueObjectDisplay.h
  lldb/include/lldb/Interpreter/OptionValueProperties.h
  lldb/include/lldb/Target/Target.h
  lldb/source/Commands/CommandObjectFrame.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/DataFormatters/DumpValueObjectOptions.cpp
  lldb/source/DataFormatters/ValueObjectPrinter.cpp
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Interpreter/OptionGroupValueObjectDisplay.cpp
  lldb/source/Interpreter/OptionValueProperties.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/TargetProperties.td

Index: lldb/source/Target/TargetProperties.td
===
--- lldb/source/Target/TargetProperties.td
+++ lldb/source/Target/TargetProperties.td
@@ -72,6 +72,9 @@
   def MaxChildrenCount: Property<"max-children-count", "SInt64">,
 DefaultUnsignedValue<256>,
 Desc<"Maximum number of children to expand in any level of depth.">;
+  def MaxChildrenDepth: Property<"max-children-depth", "UInt64">,
+DefaultUnsignedValue<0x>,
+Desc<"Maximum depth to expand children.">;
   def MaxSummaryLength: Property<"max-string-summary-length", "SInt64">,
 DefaultUnsignedValue<1024>,
 Desc<"Maximum number of characters to show when using %s in summary strings.">;
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -4236,6 +4236,15 @@
   nullptr, idx, g_target_properties[idx].default_uint_value);
 }
 
+std::pair
+TargetProperties::GetMaximumDepthOfChildrenToDisplay() const {
+  const uint32_t idx = ePropertyMaxChildrenDepth;
+  auto *option_value =
+  m_collection_sp->GetPropertyAtIndexAsOptionValueUInt64(nullptr, idx);
+  bool is_default = !option_value->OptionWasSet();
+  return {option_value->GetCurrentValue(), is_default};
+}
+
 uint32_t TargetProperties::GetMaximumSizeOfStringSummary() const {
   const uint32_t idx = ePropertyMaxSummaryLength;
   return m_collection_sp->GetPropertyAtIndexAsSInt64(
Index: lldb/source/Interpreter/OptionValueProperties.cpp
===
--- lldb/source/Interpreter/OptionValueProperties.cpp
+++ lldb/source/Interpreter/OptionValueProperties.cpp
@@ -412,6 +412,17 @@
   return nullptr;
 }
 
+OptionValueUInt64 *OptionValueProperties::GetPropertyAtIndexAsOptionValueUInt64(
+const ExecutionContext *exe_ctx, uint32_t idx) const {
+  const Property *property = GetPropertyAtIndex(exe_ctx, false, idx);
+  if (property) {
+OptionValue *value = property->GetValue().get();
+if (value)
+  return value->GetAsUInt64();
+  }
+  return nullptr;
+}
+
 int64_t OptionValueProperties::GetPropertyAtIndexAsSInt64(
 const ExecutionContext *exe_ctx, uint32_t idx, int64_t fail_value) const {
   const Property *property = GetPropertyAtIndex(exe_ctx, false, idx);
Index: lldb/source/Interpreter/OptionGroupValueObjectDisplay.cpp
===
--- lldb/source/Interpreter/OptionGroupValueObjectDisplay.cpp
+++ lldb/source/Interpreter/OptionGroupValueObjectDisplay.cpp
@@ -104,6 +104,8 @@
   max_depth = UINT32_MAX;
   error.SetErrorStringWithFormat("invalid max depth '%s'",
  option_arg.str().c_str());
+} else {
+  max_depth_is_default = false;
 }
 break;
 
@@ -163,6 +165,7 @@
   flat_output = false;
   use_objc = false;
   max_depth = UINT32_MAX;
+  max_depth_is_default = true;
   ptr_depth = 0;
   elem_count = 0;
   use_synth = true;
@@ -172,9 +175,11 @@
 
   TargetSP target_sp =
   execution_context ? execution_context->GetTargetSP() : TargetSP();
-  if (target_sp)
+  if (target_sp) {
 use_dynamic = target_sp->GetPreferDynamicValue();
-  else {
+std::tie(max_depth, max_depth_is_default) =
+target_sp->GetMaximumDepthOfChildrenToDisplay();
+  } else {
 // If we don't have any targets, then dynamic values won't do us much good.
 use_dynamic = lldb::eNoDynamicValues;
   }
@@ -190,7 +195,7 @@
 options.SetShowSummary(false);
   else
 options.SetOmitSummaryDepth(no_summary_depth);
-  options.SetMaximumDepth(max_depth)
+  options.SetMaximumDepth(max_depth, max_depth_is_default)
   .SetShowTypes(show_types)
   .SetShowLocation(show_location)
   .SetUseObjectiveC(use_objc)
Index: lldb/source/Interpreter/CommandInterpreter.cpp
=

[Lldb-commits] [PATCH] D124604: [lldb] Use shutil.which in Shell tests find_executable

[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/D124604/new/

https://reviews.llvm.org/D124604

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

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

(sorry, this slipping through somehow - it's on my radar now)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122974

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

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D122974#3424654 , @JDevlieghere 
wrote:

> The ConstString/StringPool is pretty hot so I'm very excited about making it 
> faster.
>
> I'm slightly concerned about the two hash values (the "full" hash vs the 
> single byte one). That's not something that was introduced in this patch, but 
> passing it around adds an opportunity to get it wrong.
>
> I'm wondering if we could wrap this in a struct and pass that around:
>
>   struct HashedStringRef {
> const llvm::StringRef str;
> const unsigned full_hash;
> const uint8_t hash; 
> HashedStringRef(llvm::StringRef s) : str(s), full_hash(djbHash(str)), 
> hash(hash(str)) {}
>   }
>
> It looks like we always need both except in the StringMap implementation, but 
> if I'm reading the code correctly we'll have constructed it in ConstString 
> already anyway?

I'm sort of with you here - I don't think this API feature needs to affect lldb 
much - the scope of these externally-computed hashes is small, but the 
StringMap API could be more robust/less error prone by having a struct like 
this. Specifically `StringMap::hash` could/should return an immutable struct 
that contains the `StringRef` and the hash (not that that's bulletproof - the 
data beneath the `StringRef` could still be modified externally - but the same 
is true of any hashing data structure - the keys might be shallow or have 
mutable state, etc) - and that immutable object must be passed back to the 
hash-providing insertion functions. (these functions could/should probably 
still then assert that the hash is correct in case of that underlying mutation 
- though someone could argue that's overkill and I'd be open to having that 
discussion).

By immutable I mean that the caller can't go and modify either the `StringRef` 
or hash to make these out of sync. These handles are probably still copy 
assignable. The external code shouldn't be able to create their own (ctor 
private/protected, etc) - the only way to get one is to call `StringMap` to 
produce one.




Comment at: lldb/source/Utility/ConstString.cpp:107-109
 llvm::sys::SmartScopedReader rlock(m_string_pools[h].m_mutex);
-auto it = m_string_pools[h].m_string_map.find(string_ref);
+auto it = m_string_pools[h].m_string_map.find(string_ref, string_hash);
 if (it != m_string_pools[h].m_string_map.end())

total aside, but it might be nice to refactor `m_string_pools[h]` out into a 
named variable here - accessing it 3 times and in a context where it's super 
important that it's the same thing in all 3 places, seems like it'd be easier 
to read with a named variable - might make it clear which things need to happen 
under the lock and which ones don't, etc.

(oh, and the 4th and 5th use a few lines down - and I think that'd be the only 
uses of `h`, so `h` could go away and `hash` could be called inline in the 
array index instead, maybe?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122974

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

[Lldb-commits] [PATCH] D124573: [trace][intelpt] Support system-wide tracing [1] - Add a method for accessing the list of logical core ids

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:22
+// List of data kinds used by jLLDBGetState and jLLDBGetBinaryData.
+struct IntelPTDataKinds {
+  static const char *kProcFsCpuInfo;

jj10306 wrote:
> why not use an enum here?
> having "kinds" in the name already makes it sound like an enum and seeing 
> IntelPTDataKinds:: everywhere immediately made me thing it was an enum.
this is actually an enum, but it's an enum of strings. So we have a few options:

- have a normal int enum and have another function that converts int -> const 
char*. 
- create a fake enum with class static variables like here
- use header inline variables, but they are not available in the C++ version 
used by LLDB.

Overall I found this to be the simplest way to implement it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124573

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

[Lldb-commits] [PATCH] D124492: Update CFA to be in terms of $sp instead of $fp when $fp is overwritten in epilogue on AArch64

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 425901.
jasonmolenda added a comment.

cleanup patch to remove an unintended diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124492

Files:
  
lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
  lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp

Index: lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp
===
--- lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp
+++ lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp
@@ -778,3 +778,65 @@
   EXPECT_EQ(32, row_sp->GetCFAValue().GetOffset());
 }
 
+TEST_F(TestArm64InstEmulation, TestCFAResetToSP) {
+  ArchSpec arch("arm64-apple-ios15");
+  std::unique_ptr engine(
+  static_cast(
+  UnwindAssemblyInstEmulation::CreateInstance(arch)));
+  ASSERT_NE(nullptr, engine);
+
+  UnwindPlan::RowSP row_sp;
+  AddressRange sample_range;
+  UnwindPlan unwind_plan(eRegisterKindLLDB);
+  UnwindPlan::Row::RegisterLocation regloc;
+
+  // The called_from_nodebug() from TestStepNoDebug.py
+  // Most of the previous unit tests have $sp being set as
+  // $fp plus an offset, and the unwinder recognizes that
+  // as a CFA change.  This codegen overwrites $fp and we
+  // need to know that CFA is now in terms of $sp.
+  uint8_t data[] = {
+  // prologue
+  0xff, 0x83, 0x00, 0xd1, //  0: 0xd10083ff sub sp, sp, #0x20
+  0xfd, 0x7b, 0x01, 0xa9, //  4: 0xa9017bfd stp x29, x30, [sp, #0x10]
+  0xfd, 0x43, 0x00, 0x91, //  8: 0x910043fd add x29, sp, #0x10
+
+  // epilogue
+  0xfd, 0x7b, 0x41, 0xa9, // 12: 0xa9417bfd ldp x29, x30, [sp, #0x10]
+  0xff, 0x83, 0x00, 0x91, // 16: 0x910083ff add sp, sp, #0x20
+  0xc0, 0x03, 0x5f, 0xd6, // 20: 0xd65f03c0 ret
+  };
+
+  // UnwindPlan we expect:
+  // row[0]:0: CFA=sp +0 =>
+  // row[1]:4: CFA=sp+32 =>
+  // row[2]:8: CFA=sp+32 => fp=[CFA-16] lr=[CFA-8]
+  // row[3]:   12: CFA=fp+16 => fp=[CFA-16] lr=[CFA-8]
+  // row[4]:   16: CFA=sp+32 => x0=  fp=  lr= 
+  // row[5]:   20: CFA=sp +0 => x0=  fp=  lr= 
+
+  // The specific issue we're testing for is after the
+  // ldp x29, x30, [sp, #0x10]
+  // when $fp and $lr have been restored to the original values,
+  // the CFA is now set in terms of the stack pointer.  If it is
+  // left as being in terms of the frame pointer, $fp now has the
+  // caller function's $fp value and our StackID will be wrong etc.
+
+  sample_range = AddressRange(0x1000, sizeof(data));
+
+  EXPECT_TRUE(engine->GetNonCallSiteUnwindPlanFromAssembly(
+  sample_range, data, sizeof(data), unwind_plan));
+
+  // Confirm CFA before epilogue instructions is in terms of $fp
+  row_sp = unwind_plan.GetRowForFunctionOffset(12);
+  EXPECT_EQ(12ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == gpr_fp_arm64);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+
+  // Confirm that after restoring $fp to caller's value, CFA is now in
+  // terms of $sp
+  row_sp = unwind_plan.GetRowForFunctionOffset(16);
+  EXPECT_EQ(16ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == gpr_sp_arm64);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+}
Index: lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
===
--- lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
+++ lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
@@ -614,6 +614,26 @@
   m_curr_row->SetRegisterLocationToSame(reg_num,
 false /*must_replace*/);
   m_curr_row_modified = true;
+
+  // FP has been restored to its original value, we are back
+  // to using SP to calculate the CFA.
+  if (m_fp_is_cfa) {
+m_fp_is_cfa = false;
+RegisterInfo sp_reg_info;
+lldb::RegisterKind sp_reg_kind = eRegisterKindGeneric;
+uint32_t sp_reg_num = LLDB_REGNUM_GENERIC_SP;
+m_inst_emulator_up->GetRegisterInfo(sp_reg_kind, sp_reg_num,
+sp_reg_info);
+RegisterValue sp_reg_val;
+if (GetRegisterValue(sp_reg_info, sp_reg_val)) {
+  m_cfa_reg_info = sp_reg_info;
+  const uint32_t cfa_reg_num =
+  sp_reg_info.kinds[m_unwind_plan_ptr->GetRegisterKind()];
+  assert(cfa_reg_num != LLDB_INVALID_REGNUM);
+  m_curr_row->GetCFAValue().SetIsRegisterPlusOffset(
+  cfa_reg_num, m_initial_sp - sp_reg_val.GetAsUInt64());
+}
+  }
 }
 break;
   case EmulateInstruction::eInfoTypeISA:
_

[Lldb-commits] [lldb] 9aa6a47 - [lldb] Fix crash when launching in terminal

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-04-28T14:39:28-07:00
New Revision: 9aa6a479738c7bf21128f9c45ea4ffcf82d80cbf

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

LOG: [lldb] Fix crash when launching in terminal

This patch fixes a crash when using process launch -t to launch the
inferior from a TTY. The issue is that on Darwin, Host.mm is calling
ConnectionFileDescriptor::Connect without a socket_id_callback_type. The
overload passes nullptr as the function ref, which gets called
unconditionally as the socket_id_callback.

One potential way to fix this is to change all the lambdas to include a
null check, but instead I went with an empty lambda.

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

Added: 


Modified: 
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
lldb/unittests/Host/ConnectionFileDescriptorTest.cpp

Removed: 




diff  --git a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp 
b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
index c8b743ac689ca..dc5b24979fb54 100644
--- a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -122,7 +122,8 @@ bool ConnectionFileDescriptor::IsConnected() const {
 
 ConnectionStatus ConnectionFileDescriptor::Connect(llvm::StringRef path,
Status *error_ptr) {
-  return Connect(path, nullptr, error_ptr);
+  return Connect(
+  path, [](llvm::StringRef) {}, error_ptr);
 }
 
 ConnectionStatus

diff  --git a/lldb/unittests/Host/ConnectionFileDescriptorTest.cpp 
b/lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
index e622ac0b06052..49ff73e410ab4 100644
--- a/lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
+++ b/lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
@@ -23,13 +23,22 @@ class ConnectionFileDescriptorTest : public testing::Test {
 std::unique_ptr socket_a_up;
 std::unique_ptr socket_b_up;
 CreateTCPConnectedSockets(ip, &socket_a_up, &socket_b_up);
-auto socket = socket_a_up.release();
+auto *socket = socket_a_up.release();
 ConnectionFileDescriptor connection_file_descriptor(socket);
 
 std::string uri(connection_file_descriptor.GetURI());
 EXPECT_EQ((URI{"connect", ip, socket->GetRemotePortNumber(), "/"}),
   URI::Parse(uri).getValue());
   }
+
+  void TestConnect(std::string ip, std::string path) {
+std::unique_ptr socket_a_up;
+std::unique_ptr socket_b_up;
+CreateTCPConnectedSockets(ip, &socket_a_up, &socket_b_up);
+auto *socket = socket_a_up.release();
+ConnectionFileDescriptor connection_file_descriptor(socket);
+connection_file_descriptor.Connect(path, nullptr);
+  }
 };
 
 TEST_F(ConnectionFileDescriptorTest, TCPGetURIv4) {
@@ -43,3 +52,15 @@ TEST_F(ConnectionFileDescriptorTest, TCPGetURIv6) {
 return;
   TestGetURI("::1");
 }
+
+TEST_F(ConnectionFileDescriptorTest, Connectv4) {
+  if (!HostSupportsIPv4())
+return;
+  TestConnect("127.0.0.1", "accept://127.0.0.1");
+}
+
+TEST_F(ConnectionFileDescriptorTest, Connectv6) {
+  if (!HostSupportsIPv6())
+return;
+  TestConnect("::1", "accept://::1");
+}



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

[Lldb-commits] [PATCH] D124535: [lldb] Fix crash when launching in terminal

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9aa6a479738c: [lldb] Fix crash when launching in terminal 
(authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D124535?vs=425542&id=425910#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124535

Files:
  lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
  lldb/unittests/Host/ConnectionFileDescriptorTest.cpp


Index: lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
===
--- lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
+++ lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
@@ -23,13 +23,22 @@
 std::unique_ptr socket_a_up;
 std::unique_ptr socket_b_up;
 CreateTCPConnectedSockets(ip, &socket_a_up, &socket_b_up);
-auto socket = socket_a_up.release();
+auto *socket = socket_a_up.release();
 ConnectionFileDescriptor connection_file_descriptor(socket);
 
 std::string uri(connection_file_descriptor.GetURI());
 EXPECT_EQ((URI{"connect", ip, socket->GetRemotePortNumber(), "/"}),
   URI::Parse(uri).getValue());
   }
+
+  void TestConnect(std::string ip, std::string path) {
+std::unique_ptr socket_a_up;
+std::unique_ptr socket_b_up;
+CreateTCPConnectedSockets(ip, &socket_a_up, &socket_b_up);
+auto *socket = socket_a_up.release();
+ConnectionFileDescriptor connection_file_descriptor(socket);
+connection_file_descriptor.Connect(path, nullptr);
+  }
 };
 
 TEST_F(ConnectionFileDescriptorTest, TCPGetURIv4) {
@@ -43,3 +52,15 @@
 return;
   TestGetURI("::1");
 }
+
+TEST_F(ConnectionFileDescriptorTest, Connectv4) {
+  if (!HostSupportsIPv4())
+return;
+  TestConnect("127.0.0.1", "accept://127.0.0.1");
+}
+
+TEST_F(ConnectionFileDescriptorTest, Connectv6) {
+  if (!HostSupportsIPv6())
+return;
+  TestConnect("::1", "accept://::1");
+}
Index: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
===
--- lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -122,7 +122,8 @@
 
 ConnectionStatus ConnectionFileDescriptor::Connect(llvm::StringRef path,
Status *error_ptr) {
-  return Connect(path, nullptr, error_ptr);
+  return Connect(
+  path, [](llvm::StringRef) {}, error_ptr);
 }
 
 ConnectionStatus


Index: lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
===
--- lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
+++ lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
@@ -23,13 +23,22 @@
 std::unique_ptr socket_a_up;
 std::unique_ptr socket_b_up;
 CreateTCPConnectedSockets(ip, &socket_a_up, &socket_b_up);
-auto socket = socket_a_up.release();
+auto *socket = socket_a_up.release();
 ConnectionFileDescriptor connection_file_descriptor(socket);
 
 std::string uri(connection_file_descriptor.GetURI());
 EXPECT_EQ((URI{"connect", ip, socket->GetRemotePortNumber(), "/"}),
   URI::Parse(uri).getValue());
   }
+
+  void TestConnect(std::string ip, std::string path) {
+std::unique_ptr socket_a_up;
+std::unique_ptr socket_b_up;
+CreateTCPConnectedSockets(ip, &socket_a_up, &socket_b_up);
+auto *socket = socket_a_up.release();
+ConnectionFileDescriptor connection_file_descriptor(socket);
+connection_file_descriptor.Connect(path, nullptr);
+  }
 };
 
 TEST_F(ConnectionFileDescriptorTest, TCPGetURIv4) {
@@ -43,3 +52,15 @@
 return;
   TestGetURI("::1");
 }
+
+TEST_F(ConnectionFileDescriptorTest, Connectv4) {
+  if (!HostSupportsIPv4())
+return;
+  TestConnect("127.0.0.1", "accept://127.0.0.1");
+}
+
+TEST_F(ConnectionFileDescriptorTest, Connectv6) {
+  if (!HostSupportsIPv6())
+return;
+  TestConnect("::1", "accept://::1");
+}
Index: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
===
--- lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -122,7 +122,8 @@
 
 ConnectionStatus ConnectionFileDescriptor::Connect(llvm::StringRef path,
Status *error_ptr) {
-  return Connect(path, nullptr, error_ptr);
+  return Connect(
+  path, [](llvm::StringRef) {}, error_ptr);
 }
 
 ConnectionStatus
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D124640: [trace][intelpt] Support system-wide tracing [2] - Add a dummy --per-core-tracing option

[walter erquinigo via Phabricator via lldb-commits]

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

This updates the documentation of the gdb-remote protocol, as well as the help 
messages, to include the new --per-core-tracing option.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124640

Files:
  lldb/docs/lldb-gdb-remote.txt
  lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp
  lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
  lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
  lldb/test/API/commands/trace/TestTraceStartStop.py
  
lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py

Index: lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
===
--- lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
+++ lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
@@ -152,3 +152,17 @@
 self.expect("c", substrs=['Thread', "can't be traced"])
 
 self.traceStopProcess()
+
+@skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
+@testSBAPIAndCommands
+def testStartPerCoreSession(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.dbg.CreateTarget(exe)
+
+self.expect("b main")
+self.expect("r")
+
+self.traceStartProcess(
+error=True, perCoreTracing=True,
+substrs=["Per-core tracing is not supported"])
Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -35,18 +35,18 @@
 self.expect("r")
 
 self.traceStartThread(
-error=True, threadBufferSize=2000,
+error=True, traceBufferSize=2000,
 substrs=["The trace buffer size must be a power of 2", "It was 2000"])
 
 self.traceStartThread(
-error=True, threadBufferSize=5000,
+error=True, traceBufferSize=5000,
 substrs=["The trace buffer size must be a power of 2", "It was 5000"])
 
 self.traceStartThread(
-error=True, threadBufferSize=0,
+error=True, traceBufferSize=0,
 substrs=["The trace buffer size must be a power of 2", "It was 0"])
 
-self.traceStartThread(threadBufferSize=1048576)
+self.traceStartThread(traceBufferSize=1048576)
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
 def testSBAPIHelp(self):
@@ -55,7 +55,7 @@
 self.expect("r")
 
 help = self.getTraceOrCreate().GetStartConfigurationHelp()
-self.assertIn("threadBufferSize", help)
+self.assertIn("traceBufferSize", help)
 self.assertIn("processBufferSizeLimit", help)
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
Index: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
===
--- lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
+++ lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
@@ -20,29 +20,27 @@
   Path path) {
   ObjectMapper o(value, path);
   if (!o || !fromJSON(value, (TraceStartRequest &)packet, path) ||
-  !o.map("enableTsc", packet.enableTsc) ||
-  !o.map("psbPeriod", packet.psbPeriod) ||
-  !o.map("threadBufferSize", packet.threadBufferSize) ||
-  !o.map("processBufferSizeLimit", packet.processBufferSizeLimit))
-return false;
-  if (packet.tids && packet.processBufferSizeLimit) {
-path.report("processBufferSizeLimit must be provided");
-return false;
-  }
-  if (!packet.tids && !packet.processBufferSizeLimit) {
-path.report("processBufferSizeLimit must not be provided");
+  !o.map("enableTsc", packet.enable_tsc) ||
+  !o.map("psbPeriod", packet.psb_period) ||
+  !o.map("traceBufferSize", packet.trace_buffer_size))
 return false;
+
+  if (packet.IsProcessTracing()) {
+if (!o.map("processBufferSizeLimit", packet.process_buffer_size_limit) ||
+!o.map("perCoreTracing", packet.per_core_tracing))
+  return false;
   }
   return true;
 }
 
 json::Value toJSON(const TraceIntelPTStartRequest &packet) {
   json::Value base = toJSON((const Tra

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

[Luboš Luňák via Phabricator via lldb-commits]

llunak added a comment.

In D122974#3480686 , @dblaikie wrote:

> In D122974#3424654 , @JDevlieghere 
> wrote:
>
>> 

...

>>   struct HashedStringRef {
>> const llvm::StringRef str;
>> const unsigned full_hash;
>> const uint8_t hash; 
>> HashedStringRef(llvm::StringRef s) : str(s), full_hash(djbHash(str)), 
>> hash(hash(str)) {}
>>   }

...

> The external code shouldn't be able to create their own (ctor 
> private/protected, etc) - the only way to get one is to call `StringMap` to 
> produce one.

But what if I don't want StringMap to compute the hash at all? There's still 
that 10-15% of CPU time spent in djbHash() when debug info uses exactly[*] that 
(and LLDB's index cache could be modified to store it). Which means it can be 
useful for StringMap to allow accepting precomputed hash, and then what purpose 
will that HashedStringRef have?

(*) Well, not exactly, the seed is different. Doesn't seem an impossible 
problem to solve though.

> (these functions could/should probably still then assert that the hash is 
> correct in case of that underlying mutation - though someone could argue 
> that's overkill and I'd be open to having that discussion).

I'd prefer to have that discussion. If your argument is perfection, then let's 
do the full monty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122974

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

[Lldb-commits] [lldb] 02c2b47 - [lldb] Remove ConnectionFileDescriptorTest.Connectv(4|6)

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-04-28T15:40:21-07:00
New Revision: 02c2b472b510ff55679844c087b66e7837e13dc2

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

LOG: [lldb] Remove ConnectionFileDescriptorTest.Connectv(4|6)

These tests are timing out on the bots.

Added: 


Modified: 
lldb/unittests/Host/ConnectionFileDescriptorTest.cpp

Removed: 




diff  --git a/lldb/unittests/Host/ConnectionFileDescriptorTest.cpp 
b/lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
index 49ff73e410ab..eb4b824c6c14 100644
--- a/lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
+++ b/lldb/unittests/Host/ConnectionFileDescriptorTest.cpp
@@ -30,15 +30,6 @@ class ConnectionFileDescriptorTest : public testing::Test {
 EXPECT_EQ((URI{"connect", ip, socket->GetRemotePortNumber(), "/"}),
   URI::Parse(uri).getValue());
   }
-
-  void TestConnect(std::string ip, std::string path) {
-std::unique_ptr socket_a_up;
-std::unique_ptr socket_b_up;
-CreateTCPConnectedSockets(ip, &socket_a_up, &socket_b_up);
-auto *socket = socket_a_up.release();
-ConnectionFileDescriptor connection_file_descriptor(socket);
-connection_file_descriptor.Connect(path, nullptr);
-  }
 };
 
 TEST_F(ConnectionFileDescriptorTest, TCPGetURIv4) {
@@ -52,15 +43,3 @@ TEST_F(ConnectionFileDescriptorTest, TCPGetURIv6) {
 return;
   TestGetURI("::1");
 }
-
-TEST_F(ConnectionFileDescriptorTest, Connectv4) {
-  if (!HostSupportsIPv4())
-return;
-  TestConnect("127.0.0.1", "accept://127.0.0.1");
-}
-
-TEST_F(ConnectionFileDescriptorTest, Connectv6) {
-  if (!HostSupportsIPv6())
-return;
-  TestConnect("::1", "accept://::1");
-}



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

[Lldb-commits] [PATCH] D124640: [trace][intelpt] Support system-wide tracing [2] - Add a dummy --per-core-tracing option

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 425917.
wallace added a comment.

nit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124640

Files:
  lldb/docs/lldb-gdb-remote.txt
  lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp
  lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
  lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
  lldb/test/API/commands/trace/TestTraceStartStop.py
  
lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py

Index: lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
===
--- lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
+++ lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
@@ -152,3 +152,17 @@
 self.expect("c", substrs=['Thread', "can't be traced"])
 
 self.traceStopProcess()
+
+@skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
+@testSBAPIAndCommands
+def testStartPerCoreSession(self):
+self.build()
+exe = self.getBuildArtifact("a.out")
+self.dbg.CreateTarget(exe)
+
+self.expect("b main")
+self.expect("r")
+
+self.traceStartProcess(
+error=True, perCoreTracing=True,
+substrs=["Per-core tracing is not supported"])
Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -35,18 +35,18 @@
 self.expect("r")
 
 self.traceStartThread(
-error=True, threadBufferSize=2000,
+error=True, traceBufferSize=2000,
 substrs=["The trace buffer size must be a power of 2", "It was 2000"])
 
 self.traceStartThread(
-error=True, threadBufferSize=5000,
+error=True, traceBufferSize=5000,
 substrs=["The trace buffer size must be a power of 2", "It was 5000"])
 
 self.traceStartThread(
-error=True, threadBufferSize=0,
+error=True, traceBufferSize=0,
 substrs=["The trace buffer size must be a power of 2", "It was 0"])
 
-self.traceStartThread(threadBufferSize=1048576)
+self.traceStartThread(traceBufferSize=1048576)
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
 def testSBAPIHelp(self):
@@ -55,7 +55,7 @@
 self.expect("r")
 
 help = self.getTraceOrCreate().GetStartConfigurationHelp()
-self.assertIn("threadBufferSize", help)
+self.assertIn("traceBufferSize", help)
 self.assertIn("processBufferSizeLimit", help)
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
Index: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
===
--- lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
+++ lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
@@ -20,29 +20,27 @@
   Path path) {
   ObjectMapper o(value, path);
   if (!o || !fromJSON(value, (TraceStartRequest &)packet, path) ||
-  !o.map("enableTsc", packet.enableTsc) ||
-  !o.map("psbPeriod", packet.psbPeriod) ||
-  !o.map("threadBufferSize", packet.threadBufferSize) ||
-  !o.map("processBufferSizeLimit", packet.processBufferSizeLimit))
-return false;
-  if (packet.tids && packet.processBufferSizeLimit) {
-path.report("processBufferSizeLimit must be provided");
-return false;
-  }
-  if (!packet.tids && !packet.processBufferSizeLimit) {
-path.report("processBufferSizeLimit must not be provided");
+  !o.map("enableTsc", packet.enable_tsc) ||
+  !o.map("psbPeriod", packet.psb_period) ||
+  !o.map("traceBufferSize", packet.trace_buffer_size))
 return false;
+
+  if (packet.IsProcessTracing()) {
+if (!o.map("processBufferSizeLimit", packet.process_buffer_size_limit) ||
+!o.map("perCoreTracing", packet.per_core_tracing))
+  return false;
   }
   return true;
 }
 
 json::Value toJSON(const TraceIntelPTStartRequest &packet) {
   json::Value base = toJSON((const TraceStartRequest &)packet);
-  base.getAsObject()->try_emplace("threadBufferSize", packet.threadBufferSize);
-  base.getAsObject()->try_emplace("processBufferSizeLimit",
-  

[Lldb-commits] [PATCH] D124370: [lldb/DWARF] Fix linking direction in CopyUniqueClassMethodTypes

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D124370#3474824 , @labath wrote:

> In D124370#3472394 , @clayborg 
> wrote:
>
>> So I believe the reason we need to be able to add methods to a class is for 
>> templates. Templates in DWARF are really poorly emitted all in the name of 
>> saving space. There is no full definition of template classes that get 
>> emitted, just a class definition with _only_ the methods that the user used 
>> in the current compile unit. DWARF doesn't really support emitting a 
>> templatized definition of a class in terms of a type T, it will always emit 
>> instantiations of the type T directly. So in LLDB we must create a template 
>> class like "std::vector" and say it has no member functions, and then 
>> add each member function as a type specific specialization due to how the 
>> types must be created in the clang::ASTContext.
>>
>> in one DWARF compile unit you end up having say "std::vector::clear()" 
>> and in another you would have "std::vector::erase()". In order to make 
>> the most complete definition of template types we need to find all 
>> "std::vector" definitions and manually add any method that we haven't 
>> already seen, otherwise we end up not being able to call methods that might 
>> exist. Of course calling template functions is already fraught with danger 
>> since most are inlined if they come from the STL, but if the user asks for 
>> the class definition for "std::vector", we should show all that we can. 
>> Can you make sure this patch doesn't hurt that functionality? We must have a 
>> test for it.
>>
>> So we can't just say we will accept just one class definition if we have 
>> template types. Given this information, let me know what you think of your 
>> current patch
>
> I think I'm confused. I know we're doing some funny stuff with class 
> templates, and I'm certain I don't fully understand how it works. However, I 
> am also pretty sure your description is not correct either.  A simple snippet 
> like `std::vector v;` will produce a definition for the `vector` 
> class, and that definition will include the declarations for `erase`, 
> `clear`, and any other non-template member function, even though the code 
> definitely does not call them. And this makes perfect sense to me given how 
> DWARF (and clang) describes only template instantiations -- so the template 
> instantiation `vector` is treated the same way as a class `vector_int` 
> with the same interface.
>
> What you're describing resembles the treatment of template member functions 
> -- those are only emitted in the compile units that they are used. This is 
> very unfortunate for us, though I think it still makes sense from the DWARF 
> perspective. However:
> a) In line with the previous paragraph -- this behavior is the same for 
> template **class** instantiations and regular classes. IOW, the important 
> question is whether the method is a template, not whether its enclosing class 
> is
> b) (precisely for this reason) we currently  completely ignore 
> 
>  member function templates when parsing classes
> Given all of that,  I don't see how templates are relevant for this patch. 
> *However*, please read on.

Yep, that mostly lines up with my understand.

Re: member function templates: These aren't the only things that vary between 
copies of a type in the DWARF. The other things that vary are implicit special 
member functions ({default,move,copy} ctor, dtor, {copy,move} assignment 
operator}) - these will only be present when instantiated - and nested types 
(though that's perhaps less of an issue for LLDB, not sure). I have made an 
argument that functions with "auto" return type should be treated this way 
(omitted unless the definition is instantiated and the return type is 
deduced/resolved) too - but for now they are not (they are always included, 
albeit with the "auto" return type on the declaration, and then the definition 
DIE has the real/deduced return type).

> In D124370#3472555 , @dblaikie 
> wrote:
>
>> I take it no tests fail upon removing this condition? Any hints in version 
>> history about its purpose?
>
> No tests broke with that. My first archaeology expedition did not find 
> anything, but now I've tried it once more, and I found D13224 
> . That diff is adding this condition (to 
> support -flimit-debug-info, no less) and it even comes with a test. That test 
> still exists, but it was broken over time (in several ways). Once I fix it so 
> that it tests what it was supposed to test, I see that my patch breaks it.
>
> I'm now going to try to figure out how does that code actually work...

Should the cleanups you found for the previous test case be included in this 
patch (or a separate pre/post-co

[Lldb-commits] [PATCH] D122988: [BOLT][DWARF] Add version 5 split dwarf support

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: bolt/include/bolt/Core/DebugData.h:375-377
+if (Optional DWOId = Unit.getDWOId())
+  return *DWOId;
+return Unit.getOffset();

ayermolo wrote:
> ayermolo wrote:
> > dblaikie wrote:
> > > That seems like a somewhat problematic API - returning two very different 
> > > kinds of data (the DWO_ID or the unit offset) seems quite easy to misuse 
> > > this?
> > The idea I had behind this APIS is for it to return unique ID representing 
> > the CU. As it applies to .debug_addr. For monolithic case is its offset 
> > with .debug_info for Split Dwarf case is its DWO ID. At least in my head 
> > viewed in that context the return data is the same. It's just something 
> > that uniquely identifies this CU, and logic is encapsulated in it.
> > 
> @dblaikie What do you think?
Could you use the offset consistently? The debug_addr section is always in the 
executable - the offset of the skeleton CU would be unique?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122988

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

[Lldb-commits] [PATCH] D123954: [lldb] Add setting for max depth of value object printing (NFC)

[Adrian Prantl via Phabricator via lldb-commits]

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

Few nitpicks inside otherwise I'm happy.




Comment at: lldb/include/lldb/Interpreter/OptionGroupValueObjectDisplay.h:50
   uint32_t max_depth;
+  bool max_depth_is_default;
   uint32_t ptr_depth;

Should this be with the other bools on line 45?



Comment at: lldb/include/lldb/Target/Target.h:169
 
+  std::pair GetMaximumDepthOfChildrenToDisplay() const;
+

Doxygen comment to explain what the bool returns?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123954

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

[Lldb-commits] [PATCH] D124198: [LLDB][Unwind] Add stack scanning as fallback unwind plan if no symbol file is available.

[Zequan Wu via Phabricator via lldb-commits]

zequanwu updated this revision to Diff 425928.
zequanwu marked 4 inline comments as done.
zequanwu added a comment.

Update: address some inline comments.

For testing, I don't know how to create minidump in yaml format like the one in 
`lldb/test/Shell/SymbolFile/Breakpad/Inputs/unwind-via-stack-win.yaml`. Anyone 
knows how? Or have other ideas for testing?

> I am wondering whether attaching some kind of flag to a particular frame 
> (similar to how we add [opt] for optimized functions) to indicate that it may 
> not be entirely correct could be a better option.

I'm lean towards this idea that adding a flag to the frame unwinded from stack 
scanning, maybe all frames after that one should have the flag.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124198

Files:
  lldb/include/lldb/Symbol/UnwindPlan.h
  lldb/include/lldb/Target/ABI.h
  lldb/include/lldb/Target/RegisterContextUnwind.h
  lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp
  lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.h
  lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
  lldb/source/Symbol/UnwindPlan.cpp
  lldb/source/Target/RegisterContextUnwind.cpp
  lldb/test/Shell/SymbolFile/Breakpad/unwind-via-raSearch.test

Index: lldb/test/Shell/SymbolFile/Breakpad/unwind-via-raSearch.test
===
--- lldb/test/Shell/SymbolFile/Breakpad/unwind-via-raSearch.test
+++ lldb/test/Shell/SymbolFile/Breakpad/unwind-via-raSearch.test
@@ -16,13 +16,13 @@
 # CHECK: This UnwindPlan is valid at all instruction locations: no.
 # CHECK: This UnwindPlan is for a trap handler function: no.
 # CHECK: Address range of this UnwindPlan: [unwind-via-stack-win.exe..module_image + 4112-0x107d)
-# CHECK: row[0]:0: CFA=RaSearch@SP+0 => esp=DW_OP_pick 0x0, DW_OP_consts +4, DW_OP_plus  eip=DW_OP_pick 0x0, DW_OP_deref
+# CHECK: row[0]:0: CFA=RaSearch@SP+0, offset=+0 => esp=DW_OP_pick 0x0, DW_OP_consts +4, DW_OP_plus  eip=DW_OP_pick 0x0, DW_OP_deref
 
 image show-unwind -n nonzero_frame_size
 # CHECK-LABEL: image show-unwind -n nonzero_frame_size
 # CHECK: UNWIND PLANS for unwind-via-stack-win.exe`nonzero_frame_size
 # CHECK: Symbol file UnwindPlan:
-# CHECK: row[0]:0: CFA=RaSearch@SP+12 => esp=DW_OP_pick 0x0, DW_OP_consts +4, DW_OP_plus  eip=DW_OP_pick 0x0, DW_OP_deref
+# CHECK: row[0]:0: CFA=RaSearch@SP+12, offset=+0 => esp=DW_OP_pick 0x0, DW_OP_consts +4, DW_OP_plus  eip=DW_OP_pick 0x0, DW_OP_deref
 
 # Then, some invalid rules.
 image show-unwind -n complex_rasearch
Index: lldb/source/Target/RegisterContextUnwind.cpp
===
--- lldb/source/Target/RegisterContextUnwind.cpp
+++ lldb/source/Target/RegisterContextUnwind.cpp
@@ -285,7 +285,18 @@
 cfa_status = true;
 }
 if (!cfa_status) {
-  UnwindLogMsg("could not read CFA value for first frame.");
+  UnwindLogMsg("could not read CFA value for first frame. Trying stack "
+   "walking unwind plan.");
+  if (abi) {
+UnwindPlanSP stack_walk_unwind_plan =
+std::make_shared(lldb::eRegisterKindGeneric);
+if (abi->CreateStackWalkingUnwindPlan(*stack_walk_unwind_plan)) {
+  m_fallback_unwind_plan_sp = stack_walk_unwind_plan;
+  cfa_status = TryFallbackUnwindPlan();
+}
+  }
+}
+if (!cfa_status) {
   m_frame_type = eNotAValidFrame;
   return;
 }
@@ -384,7 +395,7 @@
   // symbol/function information - just stick in some reasonable defaults and
   // hope we can unwind past this frame.  If we're above a trap handler,
   // we may be at a bogus address because we jumped through a bogus function
-  // pointer and trapped, so don't force the arch default unwind plan in that 
+  // pointer and trapped, so don't force the arch default unwind plan in that
   // case.
   ModuleSP pc_module_sp(m_current_pc.GetModule());
   if ((!m_current_pc.IsValid() || !pc_module_sp) &&
@@ -660,9 +671,20 @@
   }
 
   if (!ReadFrameAddress(row_register_kind, active_row->GetCFAValue(), m_cfa)) {
-UnwindLogMsg("failed to get cfa");
-m_frame_type = eNotAValidFrame;
-return;
+UnwindLogMsg("failed to get cfa. Trying stack walking unwind plan.");
+bool cfa_status = false;
+if (abi) {
+  UnwindPlanSP stack_walk_unwind_plan =
+  std::make_shared(lldb::eRegisterKindGeneric);
+  if (abi->CreateStackWalkingUnwindPlan(*stack_walk_unwind_plan)) {
+m_fallback_unwind_plan_sp = stack_walk_unwind_plan;
+cfa_status = TryFallbackUnwindPlan();
+  }
+}
+if (!cfa_status) {
+  m_frame_type = eNotAValidFrame;
+  return;
+}
   }
 
   ReadFrameAddress(row_register_kind, active_row->GetAFAValue(), m_afa);
@@ -1287,7 +1309,7 @@
 // arch default unwind plan is used as the Fast Unwind Plan, we
 // need to recognize this & switch over to the Full Unwi

[Lldb-commits] [PATCH] D124198: [LLDB][Unwind] Add stack scanning as fallback unwind plan if no symbol file is available.

[Zequan Wu via Phabricator via lldb-commits]

zequanwu added inline comments.



Comment at: lldb/include/lldb/Symbol/UnwindPlan.h:225
+  // Not the first search.
+  m_value.ra_search.search_offset |= 1;
+}

clayborg wrote:
> What is this magic number/bit here? Is it ok to clobber bit zero here?
When the last bit isn't set, it's the first ra search. 

The reason why we want to know this info is because after we got cfa via stack 
scanning in `TryFallbackUnwindPlan` when initializing zeroth/nonzeroth frames 
in `RegisterContextUnwind`, we will use that cfa for unwind next frame. If the 
next frame is valid, we are done. Otherwise, we get the incorrect cfa for the 
parent frame. Then we need to redo the stack scanning to find parent frame's 
cfa (`UnwindLLDB::GetOneMoreFrame`) but should skip the region that we have 
already scanned. So, the search_offset tells how many bytes should we skip. 



Comment at: lldb/include/lldb/Symbol/UnwindPlan.h:266
+  int32_t GetRaSearchOffset() const {
+return m_type == isRaSearch ? m_value.ra_search.search_offset & ~1 : 0;
+  }

clayborg wrote:
> Are we assuming "search_offset" must be aligned to at least a 4 bit boundary 
> so that we can put something in bit zero?
search_offset is usually the multiple of 4 or 8, so we can use the last bit.



Comment at: lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp:734
 
+bool ABIWindows_x86_64::CreateStackWalkingUnwindPlan(UnwindPlan &unwind_plan) {
+  unwind_plan.Clear();

clayborg wrote:
> What code actually does the search for a return address? Is that already 
> available somewhere in the unwind plans? Reading through this plan I see that 
> it sets the CFA to be RA search but I fail to see any searching going on.
This sets the unwind plan row to be ra search. The actual ra search happens on 
`RegisterContextUnwind::ReadFrameAddress` at the case 
`UnwindPlan::Row::FAValue::isRaSearch`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124198

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

[Lldb-commits] [PATCH] D124198: [LLDB][Unwind] Add stack scanning as fallback unwind plan if no symbol file is available.

[Zequan Wu via Phabricator via lldb-commits]

zequanwu updated this revision to Diff 425930.
zequanwu added a comment.

Use extra bool variable for is_first_search, because search_offset might be an 
odd number if parameter byte size in stack is an odd number.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124198

Files:
  lldb/include/lldb/Symbol/UnwindPlan.h
  lldb/include/lldb/Target/ABI.h
  lldb/include/lldb/Target/RegisterContextUnwind.h
  lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp
  lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.h
  lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp
  lldb/source/Symbol/UnwindPlan.cpp
  lldb/source/Target/RegisterContextUnwind.cpp
  lldb/test/Shell/SymbolFile/Breakpad/unwind-via-raSearch.test

Index: lldb/test/Shell/SymbolFile/Breakpad/unwind-via-raSearch.test
===
--- lldb/test/Shell/SymbolFile/Breakpad/unwind-via-raSearch.test
+++ lldb/test/Shell/SymbolFile/Breakpad/unwind-via-raSearch.test
@@ -16,13 +16,13 @@
 # CHECK: This UnwindPlan is valid at all instruction locations: no.
 # CHECK: This UnwindPlan is for a trap handler function: no.
 # CHECK: Address range of this UnwindPlan: [unwind-via-stack-win.exe..module_image + 4112-0x107d)
-# CHECK: row[0]:0: CFA=RaSearch@SP+0 => esp=DW_OP_pick 0x0, DW_OP_consts +4, DW_OP_plus  eip=DW_OP_pick 0x0, DW_OP_deref
+# CHECK: row[0]:0: CFA=RaSearch@SP+0, offset=+0 => esp=DW_OP_pick 0x0, DW_OP_consts +4, DW_OP_plus  eip=DW_OP_pick 0x0, DW_OP_deref
 
 image show-unwind -n nonzero_frame_size
 # CHECK-LABEL: image show-unwind -n nonzero_frame_size
 # CHECK: UNWIND PLANS for unwind-via-stack-win.exe`nonzero_frame_size
 # CHECK: Symbol file UnwindPlan:
-# CHECK: row[0]:0: CFA=RaSearch@SP+12 => esp=DW_OP_pick 0x0, DW_OP_consts +4, DW_OP_plus  eip=DW_OP_pick 0x0, DW_OP_deref
+# CHECK: row[0]:0: CFA=RaSearch@SP+12, offset=+0 => esp=DW_OP_pick 0x0, DW_OP_consts +4, DW_OP_plus  eip=DW_OP_pick 0x0, DW_OP_deref
 
 # Then, some invalid rules.
 image show-unwind -n complex_rasearch
Index: lldb/source/Target/RegisterContextUnwind.cpp
===
--- lldb/source/Target/RegisterContextUnwind.cpp
+++ lldb/source/Target/RegisterContextUnwind.cpp
@@ -285,7 +285,18 @@
 cfa_status = true;
 }
 if (!cfa_status) {
-  UnwindLogMsg("could not read CFA value for first frame.");
+  UnwindLogMsg("could not read CFA value for first frame. Trying stack "
+   "walking unwind plan.");
+  if (abi) {
+UnwindPlanSP stack_walk_unwind_plan =
+std::make_shared(lldb::eRegisterKindGeneric);
+if (abi->CreateStackWalkingUnwindPlan(*stack_walk_unwind_plan)) {
+  m_fallback_unwind_plan_sp = stack_walk_unwind_plan;
+  cfa_status = TryFallbackUnwindPlan();
+}
+  }
+}
+if (!cfa_status) {
   m_frame_type = eNotAValidFrame;
   return;
 }
@@ -384,7 +395,7 @@
   // symbol/function information - just stick in some reasonable defaults and
   // hope we can unwind past this frame.  If we're above a trap handler,
   // we may be at a bogus address because we jumped through a bogus function
-  // pointer and trapped, so don't force the arch default unwind plan in that 
+  // pointer and trapped, so don't force the arch default unwind plan in that
   // case.
   ModuleSP pc_module_sp(m_current_pc.GetModule());
   if ((!m_current_pc.IsValid() || !pc_module_sp) &&
@@ -660,9 +671,20 @@
   }
 
   if (!ReadFrameAddress(row_register_kind, active_row->GetCFAValue(), m_cfa)) {
-UnwindLogMsg("failed to get cfa");
-m_frame_type = eNotAValidFrame;
-return;
+UnwindLogMsg("failed to get cfa. Trying stack walking unwind plan.");
+bool cfa_status = false;
+if (abi) {
+  UnwindPlanSP stack_walk_unwind_plan =
+  std::make_shared(lldb::eRegisterKindGeneric);
+  if (abi->CreateStackWalkingUnwindPlan(*stack_walk_unwind_plan)) {
+m_fallback_unwind_plan_sp = stack_walk_unwind_plan;
+cfa_status = TryFallbackUnwindPlan();
+  }
+}
+if (!cfa_status) {
+  m_frame_type = eNotAValidFrame;
+  return;
+}
   }
 
   ReadFrameAddress(row_register_kind, active_row->GetAFAValue(), m_afa);
@@ -1287,7 +1309,7 @@
 // arch default unwind plan is used as the Fast Unwind Plan, we
 // need to recognize this & switch over to the Full Unwind Plan
 // to see what unwind rule that (more knoweldgeable, probably)
-// UnwindPlan has.  If the full UnwindPlan says the register 
+// UnwindPlan has.  If the full UnwindPlan says the register
 // location is Undefined, then it really is.
 if (active_row->GetRegisterInfo(regnum.GetAsKind(unwindplan_registerkind),
 unwindplan_regloc) &&
@@ -1335,14 +1357,14 @@
   m_full_unwind_plan_sp->GetReturnAddressRegister() !=
   

[Lldb-commits] [PATCH] D124648: [trace][intelpt] Support system-wide tracing [3] - Refactor IntelPTThreadTrace

[walter erquinigo via Phabricator via lldb-commits]

wallace created this revision.
wallace added a reviewer: jj10306.
Herald added a subscriber: mgorny.
Herald added a project: All.
wallace requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

I'm refactoring IntelPTThreadTrace into IntelPTSingleBufferTrace so that it can
both single threads or single cores. In this diff I'm basically renaming the
class, moving it to its own file, and removing all the pieces that are not used
along with some basic cleanup.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124648

Files:
  lldb/docs/lldb-gdb-remote.txt
  lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
  lldb/source/Plugins/Process/Linux/CMakeLists.txt
  lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp
  lldb/source/Plugins/Process/Linux/IntelPTCollector.h
  lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.cpp
  lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.h
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/Perf.cpp
  lldb/source/Plugins/Process/Linux/Perf.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp
  lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp

Index: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
===
--- lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
+++ lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
@@ -14,7 +14,7 @@
 namespace lldb_private {
 
 const char *IntelPTDataKinds::kProcFsCpuInfo = "procFsCpuInfo";
-const char *IntelPTDataKinds::kThreadTraceBuffer = "threadTraceBuffer";
+const char *IntelPTDataKinds::kTraceBuffer = "traceBuffer";
 
 bool fromJSON(const json::Value &value, TraceIntelPTStartRequest &packet,
   Path path) {
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp
@@ -49,7 +49,7 @@
 
   llvm::Expected json_session_description =
   TraceSessionSaver::BuildProcessesSection(
-  *live_process, IntelPTDataKinds::kThreadTraceBuffer, directory);
+  *live_process, IntelPTDataKinds::kTraceBuffer, directory);
 
   if (!json_session_description)
 return json_session_description.takeError();
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
@@ -81,8 +81,7 @@
   for (const ThreadPostMortemTraceSP &thread : traced_threads) {
 m_thread_decoders.emplace(thread->GetID(),
   std::make_unique(thread, *this));
-SetPostMortemThreadDataFile(thread->GetID(),
-IntelPTDataKinds::kThreadTraceBuffer,
+SetPostMortemThreadDataFile(thread->GetID(), IntelPTDataKinds::kTraceBuffer,
 thread->GetTraceFile());
   }
 }
@@ -373,8 +372,7 @@
 
 Error TraceIntelPT::OnThreadBufferRead(lldb::tid_t tid,
OnBinaryDataReadCallback callback) {
-  return OnThreadBinaryDataRead(tid, IntelPTDataKinds::kThreadTraceBuffer,
-callback);
+  return OnThreadBinaryDataRead(tid, IntelPTDataKinds::kTraceBuffer, callback);
 }
 
 TaskTimer &TraceIntelPT::GetTimer() { return m_task_timer; }
Index: lldb/source/Plugins/Process/Linux/Perf.h
===
--- lldb/source/Plugins/Process/Linux/Perf.h
+++ lldb/source/Plugins/Process/Linux/Perf.h
@@ -89,6 +89,24 @@
 /// if errors didn't happen.
 llvm::Expected> GetAvailableLogicalCoreIDs();
 
+/// Read data from a cyclic buffer
+///
+/// \param[in] [out] buf
+/// Destination buffer, the buffer will be truncated to written size.
+///
+/// \param[in] src
+/// Source buffer which must be a cyclic buffer.
+///
+/// \param[in] src_cyc_index
+/// The index pointer (start of the valid data in the cyclic
+/// buffer).
+///
+/// \param[in] offset
+/// The offset to begin reading the data in the cyclic buffer.
+void ReadCyclicBuffer(llvm::MutableArrayRef &dst,
+  llvm::ArrayRef src, size_t src_cyc_index,
+  size_t offset);
+
 /// Thin wrapper of the perf_event_open API.
 ///
 /// Exposes the metadata page and data and aux buffers of a perf event.
Index: lldb/source/Plugins/Process/Linux/Perf.cpp
===
--- lldb/source/Plugins/Process/Linux/Perf.cpp
+++ lldb/source/Plugins/Process/Linux/Perf.cpp
@@ -8,6 +8,7 @@
 
 #include "Perf.h"
 
+#include "Plugins/Process/POSIX/ProcessPOSIXLog.h"
 #include "lldb/Host/linux/Su

[Lldb-commits] [PATCH] D124198: [LLDB][Unwind] Add stack scanning as fallback unwind plan if no symbol file is available.

[Zequan Wu via Phabricator via lldb-commits]

zequanwu added inline comments.



Comment at: lldb/include/lldb/Symbol/UnwindPlan.h:266
+  int32_t GetRaSearchOffset() const {
+return m_type == isRaSearch ? m_value.ra_search.search_offset & ~1 : 0;
+  }

zequanwu wrote:
> clayborg wrote:
> > Are we assuming "search_offset" must be aligned to at least a 4 bit 
> > boundary so that we can put something in bit zero?
> search_offset is usually the multiple of 4 or 8, so we can use the last bit.
Updated: 
Use extra bool variable for is_first_search, because search_offset might be an 
odd number if parameter byte size in stack is an odd number.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124198

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

[Lldb-commits] [PATCH] D124573: [trace][intelpt] Support system-wide tracing [1] - Add a method for accessing the list of logical core ids

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 425936.
wallace added a comment.

I made a mistake and I included the second of two commits. This update contains 
all the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124573

Files:
  lldb/docs/lldb-gdb-remote.txt
  lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h
  lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp
  lldb/source/Plugins/Process/Linux/Perf.cpp
  lldb/source/Plugins/Process/Linux/Perf.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp
  lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
  lldb/unittests/Process/Linux/PerfTests.cpp

Index: lldb/unittests/Process/Linux/PerfTests.cpp
===
--- lldb/unittests/Process/Linux/PerfTests.cpp
+++ lldb/unittests/Process/Linux/PerfTests.cpp
@@ -10,9 +10,13 @@
 
 #include "Perf.h"
 
+#include "lldb/Host/linux/Support.h"
+
 #include "llvm/Support/Error.h"
 
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
+
 #include 
 #include 
 
@@ -46,6 +50,92 @@
   return (rdx << 32) + rax;
 }
 
+TEST(Perf, HardcodedLogicalCoreIDs) {
+  Expected> core_ids =
+  GetAvailableLogicalCoreIDs(R"(processor   : 13
+vendor_id   : GenuineIntel
+cpu family  : 6
+model   : 85
+model name  : Intel(R) Xeon(R) Gold 6138 CPU @ 2.00GHz
+stepping: 4
+microcode   : 0x265
+cpu MHz : 2886.370
+cache size  : 28160 KB
+physical id : 1
+siblings: 40
+core id : 19
+cpu cores   : 20
+apicid  : 103
+initial apicid  : 103
+fpu : yes
+fpu_exception   : yes
+cpuid level : 22
+power management:
+
+processor   : 24
+vendor_id   : GenuineIntel
+cpu family  : 6
+model   : 85
+model name  : Intel(R) Xeon(R) Gold 6138 CPU @ 2.00GHz
+stepping: 4
+microcode   : 0x265
+cpu MHz : 2768.494
+cache size  : 28160 KB
+physical id : 1
+siblings: 40
+core id : 20
+cpu cores   : 20
+apicid  : 105
+power management:
+
+processor   : 35
+vendor_id   : GenuineIntel
+cpu family  : 6
+model   : 85
+model name  : Intel(R) Xeon(R) Gold 6138 CPU @ 2.00GHz
+stepping: 4
+microcode   : 0x265
+cpu MHz : 2884.703
+cache size  : 28160 KB
+physical id : 1
+siblings: 40
+core id : 24
+cpu cores   : 20
+apicid  : 113
+
+processor   : 79
+vendor_id   : GenuineIntel
+cpu family  : 6
+model   : 85
+model name  : Intel(R) Xeon(R) Gold 6138 CPU @ 2.00GHz
+stepping: 4
+microcode   : 0x265
+cpu MHz : 3073.955
+cache size  : 28160 KB
+physical id : 1
+siblings: 40
+core id : 28
+cpu cores   : 20
+apicid  : 121
+power management:
+)");
+
+  ASSERT_TRUE((bool)core_ids);
+  ASSERT_THAT(*core_ids, ::testing::ElementsAre(13, 24, 35, 79));
+}
+
+TEST(Perf, RealLogicalCoreIDs) {
+  // We first check we can read /proc/cpuinfo
+  auto buffer_or_error = errorOrToExpected(getProcFile("cpuinfo"));
+  if (!buffer_or_error)
+GTEST_SKIP() << toString(buffer_or_error.takeError());
+
+  // At this point we shouldn't fail parsing the core ids
+  Expected> core_ids = GetAvailableLogicalCoreIDs();
+  ASSERT_TRUE((bool)core_ids);
+  ASSERT_GT((int)core_ids->size(), 0) << "We must see at least one core";
+}
+
 // Test TSC to walltime conversion based on perf conversion values.
 TEST(Perf, TscConversion) {
   // This test works by first reading the TSC value directly before
Index: lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
===
--- lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
+++ lldb/source/Utility/TraceIntelPTGDBRemotePackets.cpp
@@ -13,6 +13,9 @@
 
 namespace lldb_private {
 
+const char *IntelPTDataKinds::kProcFsCpuInfo = "procFsCpuInfo";
+const char *IntelPTDataKinds::kThreadTraceBuffer = "threadTraceBuffer";
+
 bool fromJSON(const json::Value &value, TraceIntelPTStartRequest &packet,
   Path path) {
   ObjectMapper o(value, path);
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp
@@ -48,8 +48,8 @@
 return json_intel_pt_trace.takeError();
 
   llvm::Expected json_session_description =
-  TraceSessionSaver::BuildProcessesSection(*live_process,
-   "threadTraceBuffer", directory);
+  TraceSessionSaver::BuildProcessesSection(
+  *live_process, IntelPTDataKinds::kThreadTraceBuffer, directory);
 
   if (!json_session_description)
 return json_session_