[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Pavel Labath via lldb-commits]

https://github.com/labath edited https://github.com/llvm/llvm-project/pull/87740
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Pavel Labath via lldb-commits]

@@ -64,6 +64,32 @@ class DWARFAcceleratorTable {
   return std::nullopt;
 }
 
+/// Returns the type signature of the Type Unit associated with this
+/// Accelerator Entry or std::nullopt if the Type Unit offset is not
+/// recorded in this Accelerator Entry.
+virtual std::optional getForeignTUTypeSignature() const {
+  // Default return for accelerator tables that don't support type units.
+  return std::nullopt;
+}
+
+// Returns the the CU offset for a foreign TU.
+//
+// Entries that represent foreign type units can have both a
+// DW_IDX_compile_unit and a DW_IDX_type_unit. In this case the

labath wrote:

Reading about DW_IDX_compile_unit in a supposedly-generic interface feels a bit 
out of place. Do these even need to be defined on the interface? AFAICT, the 
only callers are in DebugNamesDWARFIndex, which already know they are dealing 
with a debug_names table, and the debug_names entry class already has a bunch 
of non-virtual methods (`hasParentInformation` and friends) for 
debug_names-specific functionality.

https://github.com/llvm/llvm-project/pull/87740
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Pavel Labath via lldb-commits]

@@ -1741,45 +1741,61 @@ lldb::ModuleSP 
SymbolFileDWARF::GetExternalModule(ConstString name) {
   return pos->second;
 }
 
-DWARFDIE
-SymbolFileDWARF::GetDIE(const DIERef &die_ref) {
-  // This method can be called without going through the symbol vendor so we
-  // need to lock the module.
-  std::lock_guard guard(GetModuleMutex());
-
-  SymbolFileDWARF *symbol_file = nullptr;
-
+SymbolFileDWARF *SymbolFileDWARF::GetDIERefSymbolFile(const DIERef &die_ref) {
   // Anytime we get a "lldb::user_id_t" from an lldb_private::SymbolFile API we
   // must make sure we use the correct DWARF file when resolving things. On
   // MacOSX, when using SymbolFileDWARFDebugMap, we will use multiple
   // SymbolFileDWARF classes, one for each .o file. We can often end up with
   // references to other DWARF objects and we must be ready to receive a
   // "lldb::user_id_t" that specifies a DIE from another SymbolFileDWARF
   // instance.
+
   std::optional file_index = die_ref.file_index();
+
+  // If the file index matches, then we have the right SymbolFileDWARF already.
+  // This will work for both .dwo file and DWARF in .o files for mac. Also if
+  // both the file indexes are invalid, then we have a match.
+  if (GetFileIndex() == file_index)
+return this;
+
+  // If we are currently in a .dwo file and our file index doesn't match we 
need
+  // to let the base symbol file handle this.
+  SymbolFileDWARFDwo *dwo = llvm::dyn_cast_or_null(this);
+  if (dwo)
+return dwo->GetDIERefSymbolFile(die_ref);

labath wrote:

This isn't necessary now that SymbolFileDWARFDwo overrides this function.

https://github.com/llvm/llvm-project/pull/87740
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Pavel Labath via lldb-commits]

https://github.com/labath commented:

Most of the patch is very clean, but I'm bothered by the 
`getForeignTUSkeletonCUOffset` function, how it opens up with a mostly 
redundant (the callers checks this already) call to 
`getForeignTUTypeSignature`, and then proceeds with a reimplementation of 
`getCUOffset` (sans the type unit check). I think we could design a better set 
of APIs for this, but I'm not sure what those is, because I'm unclear of the 
meaning of various combinations of DW_IDX unit entries.

What the new function (I think) essentially does is "give me the CU associated 
with this entry even if the entry does not describe a DIE in this CU" (because 
`DW_IDX_die_offset` is relative to a type unit)". I think this API would make 
sense, even without needing to talk about type units. However, is it actually 
implementable? For single-CU indexes, that's fine, because we can kind of 
assume all entries are related to that CU. But what about multi-CU indexes? The 
only way to determine the CU there is to have an explicit attribute. Are we 
saying that each entry in a multi-CU index must have a DW_IDX_compile_unit (in 
addition to a DW_IDX_type_unit)?

If the answer is yes, then this function can be implemented, but then I think 
the current implementation of `getCUOffset` (which I interpret as "give me the 
CU offset **IF** this entry is relative to that CU") doesn't make sense -- 
because it treats entries with an explicit `DW_IDX_compile_unit` different from 
an implicit/missing `DW_IDX_compile_unit`. And in this world, we assume that 
`DW_IDX_type_unit` takes precedence over `DW_IDX_compile_unit` -- if both are 
present then `DW_IDX_die_offset` is relative to the former. And I'm not sure if 
we would actually want to take up space by putting both values into the index. 
Looking at existing producers would be interesting, but I'm not sure if there 
are any (lld does not merge type unit indexes right now, possibly for this very 
reason). Maybe one could produce something with LTO?

OTOH, if the answer is no, then the function isn't implementable in the generic 
case. That doesn't mean you can't implement this feature -- which is useful for 
guarding against ODR violations (exacerbated by llvm's nonconforming type 
signature computation algorithm...). However, I think it could be implemented 
in a much simpler way. For example, lldb could just check if it's looking at a 
single-CU index, and get the CU offset that way. No extra llvm APIs would be 
needed.

https://github.com/llvm/llvm-project/pull/87740
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Pavel Labath via lldb-commits]

@@ -1726,44 +1725,59 @@ lldb::ModuleSP 
SymbolFileDWARF::GetExternalModule(ConstString name) {
   return pos->second;
 }
 
-DWARFDIE
-SymbolFileDWARF::GetDIE(const DIERef &die_ref) {
-  // This method can be called without going through the symbol vendor so we
-  // need to lock the module.
-  std::lock_guard guard(GetModuleMutex());
-
-  SymbolFileDWARF *symbol_file = nullptr;
-
+SymbolFileDWARF *SymbolFileDWARF::GetDIERefSymbolFile(const DIERef &die_ref) {
   // Anytime we get a "lldb::user_id_t" from an lldb_private::SymbolFile API we
   // must make sure we use the correct DWARF file when resolving things. On
   // MacOSX, when using SymbolFileDWARFDebugMap, we will use multiple
   // SymbolFileDWARF classes, one for each .o file. We can often end up with
   // references to other DWARF objects and we must be ready to receive a
   // "lldb::user_id_t" that specifies a DIE from another SymbolFileDWARF
   // instance.
+
   std::optional file_index = die_ref.file_index();
+
+  // If the file index matches, then we have the right SymbolFileDWARF already.
+  // This will work for both .dwo file and DWARF in .o files for mac. Also if
+  // both the file indexes are invalid, then we have a match.
+  if (GetFileIndex() == file_index)
+return this;
+
+  // If we are currently in a .dwo file and our file index doesn't match we 
need
+  // to let the base symbol file handle this.
+  SymbolFileDWARFDwo *dwo = llvm::dyn_cast_or_null(this);
+  if (dwo)
+return dwo->GetBaseSymbolFile().GetDIERefSymbolFile(die_ref);
+
   if (file_index) {
-if (SymbolFileDWARFDebugMap *debug_map = GetDebugMapSymfile()) {
-  symbol_file = debug_map->GetSymbolFileByOSOIndex(*file_index); // OSO 
case
-  if (symbol_file)
-return symbol_file->DebugInfo().GetDIE(die_ref);
-  return DWARFDIE();
-}
+SymbolFileDWARFDebugMap *debug_map = GetDebugMapSymfile();
+if (debug_map) {

labath wrote:

+1, and also remove the else after return.

https://github.com/llvm/llvm-project/pull/87740
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Pavel Labath via lldb-commits]

@@ -0,0 +1,127 @@
+// REQUIRES: lld
+
+// This test will make a type that will be compiled differently into two
+// different .dwo files in a type unit with the same type hash, but with
+// differing contents. Clang's type unit signature is based only on the mangled
+// name of the type, regardless of the contents of the type, so that will help
+// us test foreign type units in the .debug_names section of the main
+// executable. When a DWP file is made, only one type unit will be kept and the
+// type unit that is kept has the .dwo file name that it came from. When LLDB
+// loads the foreign type units, it needs to verify that any entries from
+// foreign type units come from the right .dwo file. We test this since the
+// contents of type units are not always the same even though they have the 
same
+// type hash. We don't want invalid accelerator table entries to come from one
+// .dwo file and be used on a type unit from another since this could cause
+// invalid lookups to happen. LLDB knows how to track down which .dwo file a
+// type unit comes from by looking at the DW_AT_dwo_name attribute in the
+// DW_TAG_type_unit.
+
+// Now test with DWARF5
+// RUN: %clang -target x86_64-pc-linux -gdwarf-5 -gsplit-dwarf \
+// RUN:   -fdebug-types-section -gpubnames -c %s -o %t.main.o
+// RUN: %clang -target x86_64-pc-linux -gdwarf-5 -gsplit-dwarf -DVARIANT \
+// RUN:   -fdebug-types-section -gpubnames -c %s -o %t.foo.o
+// RUN: ld.lld %t.main.o %t.foo.o -o %t
+
+// Check when have no .dwp file that we can find the types in both .dwo files.
+// RUN: rm -f %t.dwp
+// RUN: %lldb \
+// RUN:   -o "type lookup IntegerType" \
+// RUN:   -o "type lookup FloatType" \
+// RUN:   -o "type lookup CustomType" \
+// RUN:   -b %t | FileCheck %s --check-prefix=NODWP
+// NODWP: (lldb) type lookup IntegerType
+// NODWP-NEXT: int
+// NODWP-NEXT: unsigned int
+// NODWP: (lldb) type lookup FloatType
+// NODWP-NEXT: double
+// NODWP-NEXT: float
+// NODWP: (lldb) type lookup CustomType
+// NODWP-NEXT: struct CustomType {
+// NODWP-NEXT: typedef int IntegerType;
+// NODWP-NEXT: typedef double FloatType;
+// NODWP-NEXT: CustomType::IntegerType x;
+// NODWP-NEXT: CustomType::FloatType y;
+// NODWP-NEXT: }
+// NODWP-NEXT: struct CustomType {
+// NODWP-NEXT: typedef unsigned int IntegerType;
+// NODWP-NEXT: typedef float FloatType;
+// NODWP-NEXT: CustomType::IntegerType x;
+// NODWP-NEXT: CustomType::FloatType y;
+// NODWP-NEXT: }
+
+// Check when we make the .dwp file with %t.main.dwo first so it will
+// pick the type unit from %t.main.dwo. Verify we find only the types from
+// %t.main.dwo's type unit.
+// RUN: llvm-dwp %t.main.dwo %t.foo.dwo -o %t.dwp
+// RUN: %lldb \
+// RUN:   -o "type lookup IntegerType" \
+// RUN:   -o "type lookup FloatType" \
+// RUN:   -o "type lookup CustomType" \
+// RUN:   -b %t | FileCheck %s --check-prefix=DWPMAIN
+// DWPMAIN: (lldb) type lookup IntegerType
+// DWPMAIN-NEXT: int
+// DWPMAIN: (lldb) type lookup FloatType
+// DWPMAIN-NEXT: double
+// DWPMAIN: (lldb) type lookup CustomType
+// DWPMAIN-NEXT: struct CustomType {
+// DWPMAIN-NEXT: typedef int IntegerType;
+// DWPMAIN-NEXT: typedef double FloatType;
+// DWPMAIN-NEXT: CustomType::IntegerType x;
+// DWPMAIN-NEXT: CustomType::FloatType y;
+// DWPMAIN-NEXT: }
+
+// Next we check when we make the .dwp file with %t.foo.dwo first so it will
+// pick the type unit from %t.main.dwo. Verify we find only the types from
+// %t.main.dwo's type unit.
+// RUN: llvm-dwp %t.foo.dwo %t.main.dwo -o %t.dwp
+// RUN: %lldb \
+// RUN:   -o "type lookup IntegerType" \
+// RUN:   -o "type lookup FloatType" \
+// RUN:   -o "type lookup CustomType" \
+// RUN:   -b %t | FileCheck %s --check-prefix=DWPFOO
+
+// DWPFOO: (lldb) type lookup IntegerType
+// DWPFOO-NEXT: unsigned int
+// DWPFOO: (lldb) type lookup FloatType
+// DWPFOO-NEXT: float
+// DWPFOO: (lldb) type lookup CustomType
+// DWPFOO-NEXT: struct CustomType {
+// DWPFOO-NEXT: typedef unsigned int IntegerType;
+// DWPFOO-NEXT: typedef float FloatType;
+// DWPFOO-NEXT: CustomType::IntegerType x;
+// DWPFOO-NEXT: CustomType::FloatType y;
+// DWPFOO-NEXT: }
+
+// We need to do this so we end with a type unit in each .dwo file and that has
+// the same signature but different contents. When we make the .dwp file, then
+// one of the type units will end up in the .dwp file and we will have
+// .debug_names accelerator tables for both type units and we need to ignore
+// the type units .debug_names entries that don't match the .dwo file whose
+// copy of the type unit ends up in the final .dwp file. To do this, LLDB will
+// look at the type unit and take the DWO name attribute and make sure it
+// matches, and if it doesn't, it will ignore the accelerator table entry.

labath wrote:

This feels very repetitive. Can you merge this comment with the one at the top. 
(Optionally, you can also move the source code closer to the

[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Pavel Labath via lldb-commits]

@@ -0,0 +1,127 @@
+// REQUIRES: lld
+
+// This test will make a type that will be compiled differently into two
+// different .dwo files in a type unit with the same type hash, but with
+// differing contents. Clang's type unit signature is based only on the mangled
+// name of the type, regardless of the contents of the type, so that will help
+// us test foreign type units in the .debug_names section of the main
+// executable. When a DWP file is made, only one type unit will be kept and the
+// type unit that is kept has the .dwo file name that it came from. When LLDB
+// loads the foreign type units, it needs to verify that any entries from
+// foreign type units come from the right .dwo file. We test this since the
+// contents of type units are not always the same even though they have the 
same
+// type hash. We don't want invalid accelerator table entries to come from one
+// .dwo file and be used on a type unit from another since this could cause
+// invalid lookups to happen. LLDB knows how to track down which .dwo file a
+// type unit comes from by looking at the DW_AT_dwo_name attribute in the
+// DW_TAG_type_unit.
+
+// Now test with DWARF5

labath wrote:

This line doesn't make sense here. I guess it was copied from a test with 
multiple flavours.

https://github.com/llvm/llvm-project/pull/87740
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][test] Add test for completing ObjCObjectType (PR #95405)

[Michael Buch via lldb-commits]

https://github.com/Michael137 closed 
https://github.com/llvm/llvm-project/pull/95405
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] b650764 - [lldb][test] Add test for completing ObjCObjectType (#95405)

[via lldb-commits]

Author: Michael Buch
Date: 2024-06-14T11:49:12+01:00
New Revision: b650764b16b5c8790325775ac5f87f0b1c0beca7

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

LOG: [lldb][test] Add test for completing ObjCObjectType (#95405)

This is a minimal reproducer for a crash where we would try to call
`DumpTypeDescription` on an incomplete type. This crash surfaced as part
of an NFC refactor of some of the logic in `GetCompleteQualType`:
```
(lldb) expr -l objc -- *(id)0x1234
Stack dump:
0.  Program arguments: ./bin/lldb a.out -o "b main" -o run -o "expr -l objc 
-- *(id)0x1234"
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH 
or set the environment var LLVM_SYMBOLIZER_PATH to point to it):
0  lldb 0x000102ec768c 
llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 56
1  lldb 0x000102ec6010 llvm::sys::RunSignalHandlers() + 
112
2  lldb 0x000102ec7fa8 SignalHandler(int) + 292
3  libsystem_platform.dylib 0x00018c7a8c44 _sigtramp + 56
4  LLDB 0x000116b2030c 
lldb_private::TypeSystemClang::DumpTypeDescription(void*, 
lldb_private::Stream&, lldb::DescriptionLevel, 
lldb_private::ExecutionContextScope*) + 588
5  LLDB 0x0001166b5124 
lldb_private::CompilerType::DumpTypeDescription(lldb_private::Stream*, 
lldb::DescriptionLevel, lldb_private::ExecutionContextScope*) const + 228
6  LLDB 0x000116d4f08c 
IRForTarget::CreateResultVariable(llvm::Function&) + 2076
```

rdar://129633122

Added: 
lldb/test/Shell/Expr/Inputs/objc-cast.cpp
lldb/test/Shell/Expr/TestObjCIDCast.test

Modified: 


Removed: 




diff  --git a/lldb/test/Shell/Expr/Inputs/objc-cast.cpp 
b/lldb/test/Shell/Expr/Inputs/objc-cast.cpp
new file mode 100644
index 0..76e8197013aab
--- /dev/null
+++ b/lldb/test/Shell/Expr/Inputs/objc-cast.cpp
@@ -0,0 +1 @@
+int main() { return 0; }

diff  --git a/lldb/test/Shell/Expr/TestObjCIDCast.test 
b/lldb/test/Shell/Expr/TestObjCIDCast.test
new file mode 100644
index 0..0611171da09e2
--- /dev/null
+++ b/lldb/test/Shell/Expr/TestObjCIDCast.test
@@ -0,0 +1,9 @@
+// UNSUPPORTED: system-linux, system-windows
+//
+// RUN: %clangxx_host %p/Inputs/objc-cast.cpp -g -o %t
+// RUN: %lldb %t \
+// RUN:   -o "b main" -o run -o "expression --language objc -- *(id)0x1" \
+// RUN:   2>&1 | FileCheck %s
+
+// CHECK: (lldb) expression --language objc -- *(id)0x1
+// CHECK: error: Couldn't apply expression side effects : Couldn't 
dematerialize a result variable: couldn't read its memory



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

[Lldb-commits] [lldb] [WIP][lldb] Use forward decls with redeclared definitions instead of minimal import for records (PR #95100)

[Michael Buch via lldb-commits]

Michael137 wrote:

> A possibly silly question, but my understanding was that this effort will 
> better align us with the interfaces that are used within clang (for compiling 
> modules I think). Is that correct?

Regarding Clang interfaces, one of the main benefits of this patch is that we 
can simplify LLDB's interaction with the ASTImporter and also align us with the 
more tested/robust way of completing types in Clang. E.g., calls to 
`CompleteType` such as the following might no longer be necessary (at least for 
LLDB, not sure if other external sources started relying on this): 
https://github.com/llvm/llvm-project/blob/ad702e057cf7fc1ffdc0f78f563b416170ea7d57/clang/lib/Sema/SemaType.cpp#L9078-L9081

It is true that the `ASTReader` external source implements the completion 
mechanism via `CompleteRedeclChain`/`ExternalASTSource::IncrementGeneration`, 
and this patch aligns us with that mechanism. But I'm not familiar enough with 
the modules infrastructure to confidently say whether there'll be ways to 
re-use some of it in LLDB (I suspect not, but definitely worth exploring).

>  And if so, does it mean that clang does this explosive import whenever it 
> imports a type from a (precompiled) module?

Good question, I haven't dug into the modules import mechanism yet, but if this 
is true, then that could give us an opportunity to introduce a "lazy method 
loading" mechanism on the ExternalASTSource that would fit both LLDB and 
modules? Will try to confirm this (unless someone else knows the answer to this 
already, e.g., @dwblaikie).

https://github.com/llvm/llvm-project/pull/95100
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][riscv] Fix setting breakpoint for undecoded instruction (PR #90075)

[via lldb-commits]

https://github.com/ita-sc updated 
https://github.com/llvm/llvm-project/pull/90075

>From a6e96025f399b27395778656aa720ea97d3d1e92 Mon Sep 17 00:00:00 2001
From: Ivan Tetyushkin 
Date: Fri, 7 Jun 2024 11:23:24 +0300
Subject: [PATCH] [lldb][riscv] Fix setting breakpoint for undecoded
 instruction

Copy gdb behaviour:
For RISCV we can set breakpoint even for unknown instruction, as
RISCV encoding have information about size of instruction.
---
 lldb/include/lldb/Core/EmulateInstruction.h   |  2 +
 .../RISCV/EmulateInstructionRISCV.cpp | 23 +-
 .../RISCV/EmulateInstructionRISCV.h   |  3 +
 .../NativeProcessSoftwareSingleStep.cpp   | 77 ---
 lldb/test/API/riscv/break-undecoded/Makefile  |  3 +
 .../break-undecoded/TestBreakpointIllegal.py  | 44 +++
 .../API/riscv/break-undecoded/compressed.c|  7 ++
 lldb/test/API/riscv/break-undecoded/main.c|  7 ++
 8 files changed, 137 insertions(+), 29 deletions(-)
 create mode 100644 lldb/test/API/riscv/break-undecoded/Makefile
 create mode 100644 lldb/test/API/riscv/break-undecoded/TestBreakpointIllegal.py
 create mode 100644 lldb/test/API/riscv/break-undecoded/compressed.c
 create mode 100644 lldb/test/API/riscv/break-undecoded/main.c

diff --git a/lldb/include/lldb/Core/EmulateInstruction.h 
b/lldb/include/lldb/Core/EmulateInstruction.h
index 93c16537adba1..b459476883fc5 100644
--- a/lldb/include/lldb/Core/EmulateInstruction.h
+++ b/lldb/include/lldb/Core/EmulateInstruction.h
@@ -369,6 +369,8 @@ class EmulateInstruction : public PluginInterface {
 
   virtual bool ReadInstruction() = 0;
 
+  virtual std::optional GetLastInstrSize() { return std::nullopt; }
+
   virtual bool EvaluateInstruction(uint32_t evaluate_options) = 0;
 
   virtual InstructionCondition GetInstructionCondition() {
diff --git a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp 
b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
index 6c46618b337c2..3f61e011d620a 100644
--- a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
+++ b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.cpp
@@ -624,9 +624,26 @@ std::optional 
EmulateInstructionRISCV::Decode(uint32_t inst) {
   uint16_t try_rvc = uint16_t(inst & 0x);
   // check whether the compressed encode could be valid
   uint16_t mask = try_rvc & 0b11;
-  bool is_rvc = try_rvc != 0 && mask != 3;
   uint8_t inst_type = RV64;
 
+  // Try to get size of RISCV instruction.
+  // 1.2 Instruction Length Encoding
+  bool is_16b = (inst & 0b11) != 0b11;
+  bool is_32b = (inst & 0x1f) != 0x1f;
+  bool is_48b = (inst & 0x3f) != 0x1f;
+  bool is_64b = (inst & 0x7f) != 0x3f;
+  if (is_16b)
+m_last_size = 2;
+  else if (is_32b)
+m_last_size = 4;
+  else if (is_48b)
+m_last_size = 6;
+  else if (is_64b)
+m_last_size = 8;
+  else
+// Not Valid
+m_last_size = std::nullopt;
+
   // if we have ArchSpec::eCore_riscv128 in the future,
   // we also need to check it here
   if (m_arch.GetCore() == ArchSpec::eCore_riscv32)
@@ -638,8 +655,8 @@ std::optional 
EmulateInstructionRISCV::Decode(uint32_t inst) {
   LLDB_LOGF(
   log, "EmulateInstructionRISCV::%s: inst(%x at %" PRIx64 ") was 
decoded to %s",
   __FUNCTION__, inst, m_addr, pat.name);
-  auto decoded = is_rvc ? pat.decode(try_rvc) : pat.decode(inst);
-  return DecodeResult{decoded, inst, is_rvc, pat};
+  auto decoded = is_16b ? pat.decode(try_rvc) : pat.decode(inst);
+  return DecodeResult{decoded, inst, is_16b, pat};
 }
   }
   LLDB_LOGF(log, "EmulateInstructionRISCV::%s: inst(0x%x) was unsupported",
diff --git a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h 
b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
index 8bca73a7f589d..53ac11c2e1102 100644
--- a/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
+++ b/lldb/source/Plugins/Instruction/RISCV/EmulateInstructionRISCV.h
@@ -60,6 +60,7 @@ class EmulateInstructionRISCV : public EmulateInstruction {
 
   bool SetTargetTriple(const ArchSpec &arch) override;
   bool ReadInstruction() override;
+  std::optional GetLastInstrSize() override { return m_last_size; }
   bool EvaluateInstruction(uint32_t options) override;
   bool TestEmulation(Stream &out_stream, ArchSpec &arch,
  OptionValueDictionary *test_data) override;
@@ -99,6 +100,8 @@ class EmulateInstructionRISCV : public EmulateInstruction {
 private:
   /// Last decoded instruction from m_opcode
   DecodeResult m_decoded;
+  /// Last decoded instruction size estimate.
+  std::optional m_last_size;
 };
 
 } // namespace lldb_private
diff --git 
a/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp 
b/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
index 6bf8a0dc28b22..ef71a964eaf20 100644
--- a/lldb/source/Plugins/Process/Utility/NativeProcessSoftwareSingleStep.cpp
+++ b/lldb/source/Plugins/Process/Utility/NativeProcess

[Lldb-commits] [lldb] [lldb][riscv] Fix setting breakpoint for undecoded instruction (PR #90075)

[via lldb-commits]

@@ -94,6 +94,39 @@ static lldb::addr_t ReadFlags(NativeRegisterContext 
®siter_context) {
  LLDB_INVALID_ADDRESS);
 }
 
+static int GetSoftwareBreakpointSize(const ArchSpec &arch,
+ lldb::addr_t next_flags) {
+  if (arch.GetMachine() == llvm::Triple::arm) {
+if (next_flags & 0x20)
+  // Thumb mode
+  return 2;
+else

ita-sc wrote:

Fixed

https://github.com/llvm/llvm-project/pull/90075
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][riscv] Fix setting breakpoint for undecoded instruction (PR #90075)

[via lldb-commits]

@@ -94,6 +94,39 @@ static lldb::addr_t ReadFlags(NativeRegisterContext 
®siter_context) {
  LLDB_INVALID_ADDRESS);
 }
 
+static int GetSoftwareBreakpointSize(const ArchSpec &arch,
+ lldb::addr_t next_flags) {
+  if (arch.GetMachine() == llvm::Triple::arm) {
+if (next_flags & 0x20)
+  // Thumb mode
+  return 2;
+else
+  // Arm mode
+  return 4;
+  }
+  if (arch.IsMIPS() || arch.GetTriple().isPPC64() ||
+  arch.GetTriple().isRISCV() || arch.GetTriple().isLoongArch())
+return 4;
+  return 0;
+}
+
+static Status SetSoftwareBreakpointOnPC(const ArchSpec &arch, lldb::addr_t pc,

ita-sc wrote:

I would like to keep `Status` in this MR, as from my point of view, this MR 
will do more that it should -- it will add a new functionality and change the 
way we work with errors here.

Moreover, changing this will result in unnecessary casts from `Status` to 
`Error` (`process.SetBreakpoint`) and from `Error` to 
`Status`(`SetupSoftwareSingleStepping`).

Note: I've checked that we have `Status::ToError` and `Status(llvm::Error 
error)` ctor, but I'm not sure these functions will suffice (AFAIU `m_code` 
will be saved only if `m_type == ErrorType::eErrorTypePOSIX`).

https://github.com/llvm/llvm-project/pull/90075
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Alexander Yermolovich via lldb-commits]

ayermolo wrote:

> Most of the patch is very clean, but I'm bothered by the 
> `getForeignTUSkeletonCUOffset` function, how it opens up with a mostly 
> redundant (the callers checks this already) call to 
> `getForeignTUTypeSignature`, and then proceeds with a reimplementation of 
> `getCUOffset` (sans the type unit check). I think we could design a better 
> set of APIs for this, but I'm not sure what those is, because I'm unclear of 
> the meaning of various combinations of DW_IDX unit entries.
> 
> What the new function (I think) essentially does is "give me the CU 
> associated with this entry even if the entry does not describe a DIE in this 
> CU" (because `DW_IDX_die_offset` is relative to a type unit)". I think this 
> API would make sense, even without needing to talk about type units. However, 
> is it actually implementable? For single-CU indexes, that's fine, because we 
> can kind of assume all entries are related to that CU. But what about 
> multi-CU indexes? The only way to determine the CU there is to have an 
> explicit attribute. Are we saying that each entry in a multi-CU index must 
> have a DW_IDX_compile_unit (in addition to a DW_IDX_type_unit)?
> 
> If the answer is yes, then this function can be implemented, but then I think 
> the current implementation of `getCUOffset` (which I interpret as "give me 
> the CU offset **IF** this entry is relative to that CU") doesn't make sense 
> -- because it treats entries with an explicit `DW_IDX_compile_unit` different 
> from an implicit/missing `DW_IDX_compile_unit`. And in this world, we assume 
> that `DW_IDX_type_unit` takes precedence over `DW_IDX_compile_unit` -- if 
> both are present then `DW_IDX_die_offset` is relative to the former. And I'm 
> not sure if we would actually want to take up space by putting both values 
> into the index. Looking at existing producers would be interesting, but I'm 
> not sure if there are any (lld does not merge type unit indexes right now, 
> possibly for this very reason). Maybe one could produce something with LTO?
> 
> OTOH, if the answer is no, then the function isn't implementable in the 
> generic case. That doesn't mean you can't implement this feature -- which is 
> useful for guarding against ODR violations (exacerbated by llvm's 
> nonconforming type signature computation algorithm...). However, I think it 
> could be implemented in a much simpler way. For example, lldb could just 
> check if it's looking at a single-CU index, and get the CU offset that way. 
> No extra llvm APIs would be needed.

BOLT does as I mentioned in:
https://github.com/llvm/llvm-project/pull/87740#issuecomment-2060023287


https://github.com/llvm/llvm-project/pull/87740
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][API] Add Find(Ranges)InMemory() to Process SB API (PR #95007)

[Alex Langford via lldb-commits]

https://github.com/bulbazord edited 
https://github.com/llvm/llvm-project/pull/95007
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][API] Add Find(Ranges)InMemory() to Process SB API (PR #95007)

[Alex Langford via lldb-commits]

@@ -209,6 +209,14 @@ class LLDB_API SBProcess {
 
   lldb::addr_t ReadPointerFromMemory(addr_t addr, lldb::SBError &error);
 
+  lldb::SBAddressRangeList
+  FindRangesInMemory(const void *buf, uint64_t size, SBAddressRangeList 
&ranges,
+ uint32_t alignment, uint32_t max_matches, SBError &error);
+
+  lldb::addr_t FindInMemory(const void *buf, uint64_t size,
+SBAddressRange &range, uint32_t alignment,

bulbazord wrote:

Could you make `range` const?

https://github.com/llvm/llvm-project/pull/95007
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][API] Add Find(Ranges)InMemory() to Process SB API (PR #95007)

[Alex Langford via lldb-commits]

https://github.com/bulbazord commented:

Sorry for coming in a bit late here, I know this has been in review for a while.

I didn't look at this in great detail, but I did look at the changes you're 
making to the SBAPI. Can you change the reference parameters that are 
input-only to be marked `const`? Downstream we maintain some clang-based 
tooling that generates code from the SBAPI headers and having input-only 
reference parameters marked as `const` makes a difference for us.

https://github.com/llvm/llvm-project/pull/95007
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][API] Add Find(Ranges)InMemory() to Process SB API (PR #95007)

[Alex Langford via lldb-commits]

@@ -209,6 +209,14 @@ class LLDB_API SBProcess {
 
   lldb::addr_t ReadPointerFromMemory(addr_t addr, lldb::SBError &error);
 
+  lldb::SBAddressRangeList
+  FindRangesInMemory(const void *buf, uint64_t size, SBAddressRangeList 
&ranges,

bulbazord wrote:

Could you make `ranges` const?

https://github.com/llvm/llvm-project/pull/95007
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Reapply PR/87550 (again) (PR #95571)

[Vy Nguyen via lldb-commits]

https://github.com/oontvoo created 
https://github.com/llvm/llvm-project/pull/95571

New fixes:
-  properly init the `std::optional` to an empty vector as opposed 
to `{}` (which was effectively `std::nullopt`).


>From 018c7a6052add708e0b0d09b911a904b52199da5 Mon Sep 17 00:00:00 2001
From: Vy Nguyen 
Date: Tue, 11 Jun 2024 14:15:43 -0400
Subject: [PATCH 1/3] Reapply "Reapply PR/87550 (#94625)"

This reverts commit adcf33f8fbcc0f068bd4b8254994b16dda525009.
---
 lldb/include/lldb/API/SBDebugger.h|  2 +
 lldb/include/lldb/Symbol/TypeSystem.h |  1 +
 lldb/source/API/SBDebugger.cpp|  4 ++
 lldb/source/Symbol/TypeSystem.cpp | 11 +
 lldb/tools/lldb-dap/DAP.cpp   | 61 ++-
 lldb/tools/lldb-dap/DAP.h |  5 ++-
 lldb/tools/lldb-dap/lldb-dap.cpp  |  6 ++-
 7 files changed, 77 insertions(+), 13 deletions(-)

diff --git a/lldb/include/lldb/API/SBDebugger.h 
b/lldb/include/lldb/API/SBDebugger.h
index af19b1faf3bf5..84ea9c0f772e1 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -57,6 +57,8 @@ class LLDB_API SBDebugger {
 
   static const char *GetBroadcasterClass();
 
+  static bool SupportsLanguage(lldb::LanguageType language);
+
   lldb::SBBroadcaster GetBroadcaster();
 
   /// Get progress data from a SBEvent whose type is eBroadcastBitProgress.
diff --git a/lldb/include/lldb/Symbol/TypeSystem.h 
b/lldb/include/lldb/Symbol/TypeSystem.h
index b4025c173a186..7d48f9b316138 100644
--- a/lldb/include/lldb/Symbol/TypeSystem.h
+++ b/lldb/include/lldb/Symbol/TypeSystem.h
@@ -209,6 +209,7 @@ class TypeSystem : public PluginInterface,
   // TypeSystems can support more than one language
   virtual bool SupportsLanguage(lldb::LanguageType language) = 0;
 
+  static bool SupportsLanguageStatic(lldb::LanguageType language);
   // Type Completion
 
   virtual bool GetCompleteType(lldb::opaque_compiler_type_t type) = 0;
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 7ef0d6efd4aaa..29da7d33dd80b 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1742,3 +1742,7 @@ bool SBDebugger::InterruptRequested()   {
 return m_opaque_sp->InterruptRequested();
   return false;
 }
+
+bool SBDebugger::SupportsLanguage(lldb::LanguageType language) {
+  return TypeSystem::SupportsLanguageStatic(language);
+}
diff --git a/lldb/source/Symbol/TypeSystem.cpp 
b/lldb/source/Symbol/TypeSystem.cpp
index 4956f10a0b0a7..5d56d9b1829da 100644
--- a/lldb/source/Symbol/TypeSystem.cpp
+++ b/lldb/source/Symbol/TypeSystem.cpp
@@ -335,3 +335,14 @@ TypeSystemMap::GetTypeSystemForLanguage(lldb::LanguageType 
language,
   }
   return GetTypeSystemForLanguage(language);
 }
+
+bool TypeSystem::SupportsLanguageStatic(lldb::LanguageType language) {
+  if (language == eLanguageTypeUnknown)
+return false;
+
+  LanguageSet languages =
+  PluginManager::GetAllTypeSystemSupportedLanguagesForTypes();
+  if (languages.Empty())
+return false;
+  return languages[language];
+}
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index d419f821999e6..263e841b7254d 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -32,14 +32,7 @@ namespace lldb_dap {
 DAP g_dap;
 
 DAP::DAP()
-: broadcaster("lldb-dap"),
-  exception_breakpoints(
-  {{"cpp_catch", "C++ Catch", lldb::eLanguageTypeC_plus_plus},
-   {"cpp_throw", "C++ Throw", lldb::eLanguageTypeC_plus_plus},
-   {"objc_catch", "Objective-C Catch", lldb::eLanguageTypeObjC},
-   {"objc_throw", "Objective-C Throw", lldb::eLanguageTypeObjC},
-   {"swift_catch", "Swift Catch", lldb::eLanguageTypeSwift},
-   {"swift_throw", "Swift Throw", lldb::eLanguageTypeSwift}}),
+: broadcaster("lldb-dap"), exception_breakpoints(),
   focus_tid(LLDB_INVALID_THREAD_ID), stop_at_entry(false), 
is_attach(false),
   enable_auto_variable_summaries(false),
   enable_synthetic_child_debugging(false),
@@ -65,8 +58,51 @@ DAP::DAP()
 
 DAP::~DAP() = default;
 
+void DAP::PopulateExceptionBreakpoints() {
+  llvm::call_once(initExceptionBreakpoints, [this]() {
+exception_breakpoints = {};
+if (lldb::SBDebugger::SupportsLanguage(lldb::eLanguageTypeC_plus_plus)) {
+  exception_breakpoints->emplace_back("cpp_catch", "C++ Catch",
+  lldb::eLanguageTypeC_plus_plus);
+  exception_breakpoints->emplace_back("cpp_throw", "C++ Throw",
+  lldb::eLanguageTypeC_plus_plus);
+}
+if (lldb::SBDebugger::SupportsLanguage(lldb::eLanguageTypeObjC)) {
+  exception_breakpoints->emplace_back("objc_catch", "Objective-C Catch",
+  lldb::eLanguageTypeObjC);
+  exception_breakpoints->emplace_back("objc_throw", "Objective-C Throw",
+  lldb::eLanguageTypeObjC);
+}
+if (lldb::SBDebugger::Support

[Lldb-commits] [lldb] Reapply PR/87550 (again) (PR #95571)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Vy Nguyen (oontvoo)


Changes

New fixes:
-  properly init the `std::optional` to an empty vector as 
opposed to `{}` (which was effectively `std::nullopt`).


---
Full diff: https://github.com/llvm/llvm-project/pull/95571.diff


7 Files Affected:

- (modified) lldb/include/lldb/API/SBDebugger.h (+2) 
- (modified) lldb/include/lldb/Symbol/TypeSystem.h (+1) 
- (modified) lldb/source/API/SBDebugger.cpp (+4) 
- (modified) lldb/source/Symbol/TypeSystem.cpp (+11) 
- (modified) lldb/tools/lldb-dap/DAP.cpp (+54-10) 
- (modified) lldb/tools/lldb-dap/DAP.h (+4-1) 
- (modified) lldb/tools/lldb-dap/lldb-dap.cpp (+4-2) 


``diff
diff --git a/lldb/include/lldb/API/SBDebugger.h 
b/lldb/include/lldb/API/SBDebugger.h
index af19b1faf3bf5..84ea9c0f772e1 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -57,6 +57,8 @@ class LLDB_API SBDebugger {
 
   static const char *GetBroadcasterClass();
 
+  static bool SupportsLanguage(lldb::LanguageType language);
+
   lldb::SBBroadcaster GetBroadcaster();
 
   /// Get progress data from a SBEvent whose type is eBroadcastBitProgress.
diff --git a/lldb/include/lldb/Symbol/TypeSystem.h 
b/lldb/include/lldb/Symbol/TypeSystem.h
index b4025c173a186..7d48f9b316138 100644
--- a/lldb/include/lldb/Symbol/TypeSystem.h
+++ b/lldb/include/lldb/Symbol/TypeSystem.h
@@ -209,6 +209,7 @@ class TypeSystem : public PluginInterface,
   // TypeSystems can support more than one language
   virtual bool SupportsLanguage(lldb::LanguageType language) = 0;
 
+  static bool SupportsLanguageStatic(lldb::LanguageType language);
   // Type Completion
 
   virtual bool GetCompleteType(lldb::opaque_compiler_type_t type) = 0;
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 7ef0d6efd4aaa..29da7d33dd80b 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -1742,3 +1742,7 @@ bool SBDebugger::InterruptRequested()   {
 return m_opaque_sp->InterruptRequested();
   return false;
 }
+
+bool SBDebugger::SupportsLanguage(lldb::LanguageType language) {
+  return TypeSystem::SupportsLanguageStatic(language);
+}
diff --git a/lldb/source/Symbol/TypeSystem.cpp 
b/lldb/source/Symbol/TypeSystem.cpp
index 4956f10a0b0a7..931ce1b0203a9 100644
--- a/lldb/source/Symbol/TypeSystem.cpp
+++ b/lldb/source/Symbol/TypeSystem.cpp
@@ -335,3 +335,14 @@ TypeSystemMap::GetTypeSystemForLanguage(lldb::LanguageType 
language,
   }
   return GetTypeSystemForLanguage(language);
 }
+
+bool TypeSystem::SupportsLanguageStatic(lldb::LanguageType language) {
+  if (language == eLanguageTypeUnknown || language >= eNumLanguageTypes)
+return false;
+
+  LanguageSet languages =
+  PluginManager::GetAllTypeSystemSupportedLanguagesForTypes();
+  if (languages.Empty())
+return false;
+  return languages[language];
+}
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index d419f821999e6..e4fb3181a2d99 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -32,14 +32,7 @@ namespace lldb_dap {
 DAP g_dap;
 
 DAP::DAP()
-: broadcaster("lldb-dap"),
-  exception_breakpoints(
-  {{"cpp_catch", "C++ Catch", lldb::eLanguageTypeC_plus_plus},
-   {"cpp_throw", "C++ Throw", lldb::eLanguageTypeC_plus_plus},
-   {"objc_catch", "Objective-C Catch", lldb::eLanguageTypeObjC},
-   {"objc_throw", "Objective-C Throw", lldb::eLanguageTypeObjC},
-   {"swift_catch", "Swift Catch", lldb::eLanguageTypeSwift},
-   {"swift_throw", "Swift Throw", lldb::eLanguageTypeSwift}}),
+: broadcaster("lldb-dap"), exception_breakpoints(),
   focus_tid(LLDB_INVALID_THREAD_ID), stop_at_entry(false), 
is_attach(false),
   enable_auto_variable_summaries(false),
   enable_synthetic_child_debugging(false),
@@ -65,8 +58,54 @@ DAP::DAP()
 
 DAP::~DAP() = default;
 
+void DAP::PopulateExceptionBreakpoints() {
+  llvm::call_once(initExceptionBreakpoints, [this]() {
+exception_breakpoints = std::vector {};
+
+if (lldb::SBDebugger::SupportsLanguage(lldb::eLanguageTypeC_plus_plus)) {
+  exception_breakpoints->emplace_back("cpp_catch", "C++ Catch",
+  lldb::eLanguageTypeC_plus_plus);
+  exception_breakpoints->emplace_back("cpp_throw", "C++ Throw",
+  lldb::eLanguageTypeC_plus_plus);
+}
+if (lldb::SBDebugger::SupportsLanguage(lldb::eLanguageTypeObjC)) {
+  exception_breakpoints->emplace_back("objc_catch", "Objective-C Catch",
+  lldb::eLanguageTypeObjC);
+  exception_breakpoints->emplace_back("objc_throw", "Objective-C Throw",
+  lldb::eLanguageTypeObjC);
+}
+if (lldb::SBDebugger::SupportsLanguage(lldb::eLanguageTypeSwift)) {
+  exception_breakpoints->emplace_back("swift_catch", "Swift Catch",
+   

[Lldb-commits] [lldb] Reapply PR/87550 (again) (PR #95571)

[via lldb-commits]

github-actions[bot] wrote:

⚠️ We detected that you are using a GitHub private e-mail address to contribute 
to the repo. Please turn off [Keep my email addresses 
private](https://github.com/settings/emails) setting in your account. See 
[LLVM 
Discourse](https://discourse.llvm.org/t/hidden-emails-on-github-should-we-do-something-about-it)
 for more information.

https://github.com/llvm/llvm-project/pull/95571
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Reapply PR/87550 (again) (PR #95571)

[via lldb-commits]

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff bbe9119d9cb37662faafe7fe273e792b1b70145e 
ac59dccc8f0c9d75ceca2cc4019535426301211a -- lldb/include/lldb/API/SBDebugger.h 
lldb/include/lldb/Symbol/TypeSystem.h lldb/source/API/SBDebugger.cpp 
lldb/source/Symbol/TypeSystem.cpp lldb/tools/lldb-dap/DAP.cpp 
lldb/tools/lldb-dap/DAP.h lldb/tools/lldb-dap/lldb-dap.cpp
``





View the diff from clang-format here.


``diff
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index e4fb3181a2..b3718650d7 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -60,8 +60,8 @@ DAP::~DAP() = default;
 
 void DAP::PopulateExceptionBreakpoints() {
   llvm::call_once(initExceptionBreakpoints, [this]() {
-exception_breakpoints = std::vector {};
-
+exception_breakpoints = std::vector{};
+
 if (lldb::SBDebugger::SupportsLanguage(lldb::eLanguageTypeC_plus_plus)) {
   exception_breakpoints->emplace_back("cpp_catch", "C++ Catch",
   lldb::eLanguageTypeC_plus_plus);

``




https://github.com/llvm/llvm-project/pull/95571
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Tweak Python interpreter workaround on macOS (PR #95582)

[Jonas Devlieghere via lldb-commits]

https://github.com/JDevlieghere created 
https://github.com/llvm/llvm-project/pull/95582

Avoid copying the Python interpreter when running in a virtual environment as 
it will already have its own copy of the Python interpreter. Also leave a 
breadcrumb that we're running with a different Python interpreter.

>From 76e0dd2cef42105f5112a19a7f5c822939e5fdf1 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere 
Date: Fri, 14 Jun 2024 11:41:54 -0700
Subject: [PATCH] [lldb] Tweak Python interpreter workaround on macOS

Avoid copying the Python interpreter when running in a virtual
environment as it will already have its own copy of the Python
interpreter. Also leave a breadcrumb that we're running with a different
Python interpreter.
---
 lldb/test/API/lit.cfg.py | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/lldb/test/API/lit.cfg.py b/lldb/test/API/lit.cfg.py
index d934349fe3ca3..1e99c8cb95d16 100644
--- a/lldb/test/API/lit.cfg.py
+++ b/lldb/test/API/lit.cfg.py
@@ -58,6 +58,15 @@ def find_shlibpath_var():
 # enabled, we can't inject libraries into system binaries at all, so we need a
 # copy of the "real" python to work with.
 def find_python_interpreter():
+# This is only necessary when using DYLD_INSERT_LIBRARIES.
+if "DYLD_INSERT_LIBRARIES" not in config.environment:
+return None
+
+# If we're running in a virtual environment, we already have a copy of the
+# Python executable.
+if "VIRTUAL_ENV" in config.environment:
+return None
+
 # Avoid doing any work if we already copied the binary.
 copied_python = os.path.join(config.lldb_build_directory, "copied-python")
 if os.path.isfile(copied_python):
@@ -84,7 +93,7 @@ def find_python_interpreter():
 # RPATH and cannot be copied.
 try:
 # We don't care about the output, just make sure it runs.
-subprocess.check_output([copied_python, "-V"], 
stderr=subprocess.STDOUT)
+subprocess.check_call([copied_python, "-V"])
 except subprocess.CalledProcessError:
 # The copied Python didn't work. Assume we're dealing with the Python
 # interpreter in Xcode. Given that this is not a system binary SIP
@@ -130,8 +139,13 @@ def delete_module_cache(path):
 "libclang_rt.tsan_osx_dynamic.dylib"
 )
 
-if "DYLD_INSERT_LIBRARIES" in config.environment and platform.system() == 
"Darwin":
-config.python_executable = find_python_interpreter()
+if platform.system() == "Darwin":
+python_executable = find_python_interpreter()
+if python_executable:
+lit_config.note(
+"Using {} instead of {}".format(python_executable, 
config.python_executable)
+)
+config.python_executable = python_executable
 
 # Shared library build of LLVM may require LD_LIBRARY_PATH or equivalent.
 if is_configured("shared_libs"):

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

[Lldb-commits] [lldb] [lldb] Tweak Python interpreter workaround on macOS (PR #95582)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)


Changes

Avoid copying the Python interpreter when running in a virtual environment as 
it will already have its own copy of the Python interpreter. Also leave a 
breadcrumb that we're running with a different Python interpreter.

---
Full diff: https://github.com/llvm/llvm-project/pull/95582.diff


1 Files Affected:

- (modified) lldb/test/API/lit.cfg.py (+17-3) 


``diff
diff --git a/lldb/test/API/lit.cfg.py b/lldb/test/API/lit.cfg.py
index d934349fe3ca3..1e99c8cb95d16 100644
--- a/lldb/test/API/lit.cfg.py
+++ b/lldb/test/API/lit.cfg.py
@@ -58,6 +58,15 @@ def find_shlibpath_var():
 # enabled, we can't inject libraries into system binaries at all, so we need a
 # copy of the "real" python to work with.
 def find_python_interpreter():
+# This is only necessary when using DYLD_INSERT_LIBRARIES.
+if "DYLD_INSERT_LIBRARIES" not in config.environment:
+return None
+
+# If we're running in a virtual environment, we already have a copy of the
+# Python executable.
+if "VIRTUAL_ENV" in config.environment:
+return None
+
 # Avoid doing any work if we already copied the binary.
 copied_python = os.path.join(config.lldb_build_directory, "copied-python")
 if os.path.isfile(copied_python):
@@ -84,7 +93,7 @@ def find_python_interpreter():
 # RPATH and cannot be copied.
 try:
 # We don't care about the output, just make sure it runs.
-subprocess.check_output([copied_python, "-V"], 
stderr=subprocess.STDOUT)
+subprocess.check_call([copied_python, "-V"])
 except subprocess.CalledProcessError:
 # The copied Python didn't work. Assume we're dealing with the Python
 # interpreter in Xcode. Given that this is not a system binary SIP
@@ -130,8 +139,13 @@ def delete_module_cache(path):
 "libclang_rt.tsan_osx_dynamic.dylib"
 )
 
-if "DYLD_INSERT_LIBRARIES" in config.environment and platform.system() == 
"Darwin":
-config.python_executable = find_python_interpreter()
+if platform.system() == "Darwin":
+python_executable = find_python_interpreter()
+if python_executable:
+lit_config.note(
+"Using {} instead of {}".format(python_executable, 
config.python_executable)
+)
+config.python_executable = python_executable
 
 # Shared library build of LLVM may require LD_LIBRARY_PATH or equivalent.
 if is_configured("shared_libs"):

``




https://github.com/llvm/llvm-project/pull/95582
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Tweak Python interpreter workaround on macOS (PR #95582)

[Adrian Prantl via lldb-commits]

https://github.com/adrian-prantl approved this pull request.


https://github.com/llvm/llvm-project/pull/95582
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Tweak Python interpreter workaround on macOS (PR #95582)

[Felipe de Azevedo Piovezan via lldb-commits]

https://github.com/felipepiovezan approved this pull request.


https://github.com/llvm/llvm-project/pull/95582
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 4368726 - [lldb] Tweak Python interpreter workaround on macOS (#95582)

[via lldb-commits]

Author: Jonas Devlieghere
Date: 2024-06-14T11:52:27-07:00
New Revision: 436872693a8a57487bf4510437183878d1e35cfb

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

LOG: [lldb] Tweak Python interpreter workaround on macOS (#95582)

Avoid copying the Python interpreter when running in a virtual
environment as it will already have its own copy of the Python
interpreter. Also leave a breadcrumb that we're running with a different
Python interpreter.

Added: 


Modified: 
lldb/test/API/lit.cfg.py

Removed: 




diff  --git a/lldb/test/API/lit.cfg.py b/lldb/test/API/lit.cfg.py
index d934349fe3ca3..1e99c8cb95d16 100644
--- a/lldb/test/API/lit.cfg.py
+++ b/lldb/test/API/lit.cfg.py
@@ -58,6 +58,15 @@ def find_shlibpath_var():
 # enabled, we can't inject libraries into system binaries at all, so we need a
 # copy of the "real" python to work with.
 def find_python_interpreter():
+# This is only necessary when using DYLD_INSERT_LIBRARIES.
+if "DYLD_INSERT_LIBRARIES" not in config.environment:
+return None
+
+# If we're running in a virtual environment, we already have a copy of the
+# Python executable.
+if "VIRTUAL_ENV" in config.environment:
+return None
+
 # Avoid doing any work if we already copied the binary.
 copied_python = os.path.join(config.lldb_build_directory, "copied-python")
 if os.path.isfile(copied_python):
@@ -84,7 +93,7 @@ def find_python_interpreter():
 # RPATH and cannot be copied.
 try:
 # We don't care about the output, just make sure it runs.
-subprocess.check_output([copied_python, "-V"], 
stderr=subprocess.STDOUT)
+subprocess.check_call([copied_python, "-V"])
 except subprocess.CalledProcessError:
 # The copied Python didn't work. Assume we're dealing with the Python
 # interpreter in Xcode. Given that this is not a system binary SIP
@@ -130,8 +139,13 @@ def delete_module_cache(path):
 "libclang_rt.tsan_osx_dynamic.dylib"
 )
 
-if "DYLD_INSERT_LIBRARIES" in config.environment and platform.system() == 
"Darwin":
-config.python_executable = find_python_interpreter()
+if platform.system() == "Darwin":
+python_executable = find_python_interpreter()
+if python_executable:
+lit_config.note(
+"Using {} instead of {}".format(python_executable, 
config.python_executable)
+)
+config.python_executable = python_executable
 
 # Shared library build of LLVM may require LD_LIBRARY_PATH or equivalent.
 if is_configured("shared_libs"):



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

[Lldb-commits] [lldb] [lldb] Tweak Python interpreter workaround on macOS (PR #95582)

[Jonas Devlieghere via lldb-commits]

https://github.com/JDevlieghere closed 
https://github.com/llvm/llvm-project/pull/95582
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [LLDB] Don't cache module sp when Activate() fails. (PR #95586)

[via lldb-commits]

https://github.com/thetruestblue created 
https://github.com/llvm/llvm-project/pull/95586

Currently, the instrumentation runtime is caching a library the first time it 
sees it in the module list. However, in some rare cases on Darwin, the cached 
pre-run unloaded modules are different from the runtime module that is loaded 
at runtime. This patch removes the cached module if the plugin fails to 
activate, ensuring that on subsequent calls we don't try to activate using the 
unloaded cached module.

There are a few related bugs to fix in a follow up: CheckIfRuntimeValid should 
have a stronger check to ensure the module is loaded and can be activated. 
Further investigation in UpdateSpecialBinariesFromNewImageInfos calling 
ModulesDidLoad when the module list may have unloaded modules.

I have not included a test for the following reasons:
1. This is an incredibly rare occurance and is only observed in a specific 
circumstance on Darwin. It is tied to behavior in the DynamicLoader thai is not 
commonly encountered.

2. It is difficult to reproduce -- this bug requires precise conditions on 
darwin and it is unclear how we'd reproduce that in a controlled testing 
environment.

rdar://128971453


>From db161e5c87eecbd5607f78e7afd00af183aa415c Mon Sep 17 00:00:00 2001
From: Blue Gaston 
Date: Fri, 14 Jun 2024 12:53:27 -0400
Subject: [PATCH] [LLDB] Don't cache module sp when Activate() fails.

Currently, the instrumentation runtime is caching a library the first time it 
sees it in the module list. However, in some rare cases on Darwin, the cached 
pre-run unloaded modules are different from the runtime module that is loaded 
at runtime. This patch removes the cached module if the plugin fails to 
activate, ensuring that on subsequent calls we don't try to activate using the 
unloaded cached module.

There are a few related bugs to fix in a follow up: CheckIfRuntimeValid should 
have a stronger check to ensure the module is loaded and can be activated. 
Further investigation in UpdateSpecialBinariesFromNewImageInfos calling 
ModulesDidLoad when the module list may have unloaded modules.

I have not included a test for the following reasons:
1. This is an incredibly rare occurance and is only observed in a specific 
circumstance on Darwin. It is tied to behavior in the DynamicLoader thai is not 
commonly encountered.

2. It is difficult to reproduce -- this bug requires precise conditions on 
darwin and it is unclear how we'd reproduce that in a controlled testing 
environment.

rdar://128971453
---
 lldb/source/Target/InstrumentationRuntime.cpp | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lldb/source/Target/InstrumentationRuntime.cpp 
b/lldb/source/Target/InstrumentationRuntime.cpp
index 9f22a1be20ccb..94f4ca353d7ef 100644
--- a/lldb/source/Target/InstrumentationRuntime.cpp
+++ b/lldb/source/Target/InstrumentationRuntime.cpp
@@ -60,6 +60,8 @@ void InstrumentationRuntime::ModulesDidLoad(
   if (CheckIfRuntimeIsValid(module_sp)) {
 SetRuntimeModuleSP(module_sp);
 Activate();
+if (IsActive())
+  SetRuntimeModuleSP({}); // Don't cache module if activation failed.
 return false; // Stop iterating, we're done.
   }
 }

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

[Lldb-commits] [lldb] [LLDB] Don't cache module sp when Activate() fails. (PR #95586)

[via lldb-commits]

https://github.com/thetruestblue updated 
https://github.com/llvm/llvm-project/pull/95586

>From db161e5c87eecbd5607f78e7afd00af183aa415c Mon Sep 17 00:00:00 2001
From: Blue Gaston 
Date: Fri, 14 Jun 2024 12:53:27 -0400
Subject: [PATCH] [LLDB] Don't cache module sp when Activate() fails.

Currently, the instrumentation runtime is caching a library the first time it 
sees it in the module list. However, in some rare cases on Darwin, the cached 
pre-run unloaded modules are different from the runtime module that is loaded 
at runtime. This patch removes the cached module if the plugin fails to 
activate, ensuring that on subsequent calls we don't try to activate using the 
unloaded cached module.

There are a few related bugs to fix in a follow up: CheckIfRuntimeValid should 
have a stronger check to ensure the module is loaded and can be activated. 
Further investigation in UpdateSpecialBinariesFromNewImageInfos calling 
ModulesDidLoad when the module list may have unloaded modules.

I have not included a test for the following reasons:
1. This is an incredibly rare occurance and is only observed in a specific 
circumstance on Darwin. It is tied to behavior in the DynamicLoader thai is not 
commonly encountered.

2. It is difficult to reproduce -- this bug requires precise conditions on 
darwin and it is unclear how we'd reproduce that in a controlled testing 
environment.

rdar://128971453
---
 lldb/source/Target/InstrumentationRuntime.cpp | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lldb/source/Target/InstrumentationRuntime.cpp 
b/lldb/source/Target/InstrumentationRuntime.cpp
index 9f22a1be20ccb..94f4ca353d7ef 100644
--- a/lldb/source/Target/InstrumentationRuntime.cpp
+++ b/lldb/source/Target/InstrumentationRuntime.cpp
@@ -60,6 +60,8 @@ void InstrumentationRuntime::ModulesDidLoad(
   if (CheckIfRuntimeIsValid(module_sp)) {
 SetRuntimeModuleSP(module_sp);
 Activate();
+if (IsActive())
+  SetRuntimeModuleSP({}); // Don't cache module if activation failed.
 return false; // Stop iterating, we're done.
   }
 }

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

[Lldb-commits] [lldb] [LLDB] Don't cache module sp when Activate() fails. (PR #95586)

[via lldb-commits]

https://github.com/thetruestblue closed 
https://github.com/llvm/llvm-project/pull/95586
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Add options to "statistics dump" to control what sections are dumped (PR #95075)

[Greg Clayton via lldb-commits]

https://github.com/clayborg edited 
https://github.com/llvm/llvm-project/pull/95075
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Add options to "statistics dump" to control what sections are dumped (PR #95075)

[Greg Clayton via lldb-commits]

@@ -44,6 +44,30 @@ void SBStatisticsOptions::SetSummaryOnly(bool b) {
 
 bool SBStatisticsOptions::GetSummaryOnly() { return m_opaque_up->summary_only; 
}
 
+void SBStatisticsOptions::SetIncludeTargets(bool b) {
+  m_opaque_up->include_targets = b;
+}
+
+bool SBStatisticsOptions::GetIncludeTargets() const {
+  return m_opaque_up->include_targets;
+}
+
+void SBStatisticsOptions::SetIncludeModules(bool b) {
+  m_opaque_up->include_modules = b;
+}
+
+bool SBStatisticsOptions::GetIncludeModules() const {
+  return m_opaque_up->include_modules;
+}
+
+void SBStatisticsOptions::SetIncludeTranscript(bool b) {
+  m_opaque_up->include_transcript = b;
+}
+
+bool SBStatisticsOptions::GetIncludeTranscript() const {
+  return m_opaque_up->include_transcript;
+}
+

clayborg wrote:

If we switch the internal `StatisticsOptions` over to use optionals + accessors 
as suggested above, this will accurately track when and if any settings are set 
here via the API, and do the right thing, just like the command line command.

https://github.com/llvm/llvm-project/pull/95075
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Add options to "statistics dump" to control what sections are dumped (PR #95075)

[Greg Clayton via lldb-commits]

@@ -133,7 +133,9 @@ struct ConstStringStats {
 struct StatisticsOptions {
   bool summary_only = false;
   bool load_all_debug_info = false;
-  bool include_transcript = false;
+  bool include_targets = true;
+  bool include_modules = true;

clayborg wrote:

Do we want to make all of the StatisticsOptions bool values 
`std::optional` values? Then we will know if the user has set them and we 
can do the reasoning. Accessors can be added to this `StatisticsOptions` class 
to do the right thing. Then instead of people directly accessing each 
`std::optional` we would have accessors. Something like:
```
class StatisticsOptions {
private: // Stop direct access to the member variables
  std::optional m_summary_only;
  std::optional m_load_all_debug_info;
  std::optional m_include_transcript;
  std::optional m_include_targets;
  std::optional m_include_modules;
public:
  // Summary only defaults to false.
  bool GetSummaryOnly() const { return m_summary_only.value_or(false); }
  void SetSummaryOnly(bool value) { m_summary_only = value; }
  // Now we can reason about what values to return depending on m_summary_only:
  bool GetIncludeTargets() const {
if (m_include_targets.has_value())
  return m_include_targets.value();
// m_include_targets has no value set, so return a value base on 
m_summary_only
return !GetSummaryOnly();
  }
```
Then this allows you to always do the right thing depending on what options 
people select.

https://github.com/llvm/llvm-project/pull/95075
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Add options to "statistics dump" to control what sections are dumped (PR #95075)

[Greg Clayton via lldb-commits]

@@ -133,7 +133,9 @@ struct ConstStringStats {
 struct StatisticsOptions {
   bool summary_only = false;
   bool load_all_debug_info = false;
-  bool include_transcript = false;

clayborg wrote:

I would move this line that was removed back to where it was on line 136 (it is 
now 138). The diff will be smaller

https://github.com/llvm/llvm-project/pull/95075
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Add options to "statistics dump" to control what sections are dumped (PR #95075)

[Greg Clayton via lldb-commits]

@@ -77,12 +78,33 @@ class CommandObjectStatsDump : public CommandObjectParsed {
 break;
   case 's':
 m_stats_options.summary_only = true;
+// In the "summary" mode, the following sections should be omitted by
+// default unless their corresponding options are explicitly given.
+// If such options were processed before 's', `m_seen_options` should
+// contain them, and we will skip setting them to `false` here. If such
+// options are not yet processed, we will set them to `false` here, and
+// they will be overwritten when the options are processed.
+if (m_seen_options.find((int)'r') == m_seen_options.end())
+  m_stats_options.include_targets = false;
+if (m_seen_options.find((int)'m') == m_seen_options.end())
+  m_stats_options.include_modules = false;
+if (m_seen_options.find((int)'t') == m_seen_options.end())
+  m_stats_options.include_transcript = false;
 break;

clayborg wrote:

We should put these smarts into the `StatisticsOptions` class so that we don't 
have to do this logic here _and_ this means that when people use the 
`SBStatisticsOptions` class via the API, we will do the right thing when using 
the API and it will match the behavior here.

https://github.com/llvm/llvm-project/pull/95075
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Add options to "statistics dump" to control what sections are dumped (PR #95075)

[Greg Clayton via lldb-commits]

https://github.com/clayborg requested changes to this pull request.


https://github.com/llvm/llvm-project/pull/95075
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [LLDB] Don't cache module sp when Activate() fails. (PR #95586)

[via lldb-commits]

https://github.com/thetruestblue reopened 
https://github.com/llvm/llvm-project/pull/95586
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [LLDB] Don't cache module sp when Activate() fails. (PR #95586)

[via lldb-commits]

https://github.com/thetruestblue ready_for_review 
https://github.com/llvm/llvm-project/pull/95586
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [LLDB] Don't cache module sp when Activate() fails. (PR #95586)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: None (thetruestblue)


Changes

Currently, the instrumentation runtime is caching a library the first time it 
sees it in the module list. However, in some rare cases on Darwin, the cached 
pre-run unloaded modules are different from the runtime module that is loaded 
at runtime. This patch removes the cached module if the plugin fails to 
activate, ensuring that on subsequent calls we don't try to activate using the 
unloaded cached module.

There are a few related bugs to fix in a follow up: CheckIfRuntimeValid should 
have a stronger check to ensure the module is loaded and can be activated. 
Further investigation in UpdateSpecialBinariesFromNewImageInfos calling 
ModulesDidLoad when the module list may have unloaded modules.

I have not included a test for the following reasons:
1. This is an incredibly rare occurance and is only observed in a specific 
circumstance on Darwin. It is tied to behavior in the DynamicLoader thai is not 
commonly encountered.

2. It is difficult to reproduce -- this bug requires precise conditions on 
darwin and it is unclear how we'd reproduce that in a controlled testing 
environment.

rdar://128971453


---
Full diff: https://github.com/llvm/llvm-project/pull/95586.diff


1 Files Affected:

- (modified) lldb/source/Target/InstrumentationRuntime.cpp (+2) 


``diff
diff --git a/lldb/source/Target/InstrumentationRuntime.cpp 
b/lldb/source/Target/InstrumentationRuntime.cpp
index 9f22a1be20ccb..94f4ca353d7ef 100644
--- a/lldb/source/Target/InstrumentationRuntime.cpp
+++ b/lldb/source/Target/InstrumentationRuntime.cpp
@@ -60,6 +60,8 @@ void InstrumentationRuntime::ModulesDidLoad(
   if (CheckIfRuntimeIsValid(module_sp)) {
 SetRuntimeModuleSP(module_sp);
 Activate();
+if (IsActive())
+  SetRuntimeModuleSP({}); // Don't cache module if activation failed.
 return false; // Stop iterating, we're done.
   }
 }

``




https://github.com/llvm/llvm-project/pull/95586
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Pavel Labath via lldb-commits]

labath wrote:

> BOLT does as I mentioned in: [#87740 
> (comment)](https://github.com/llvm/llvm-project/pull/87740#issuecomment-2060023287)

I'm sorry, I missed that part. I'm bit of late to the party here.

> 
> It will put all the CUs/Tus into one one module, thus for foreign TUs both 
> DW_IDX_compile_unit and DW_IDX_type_unit is needed.

Right, the CU is necessary to find the DWO file containing the type unit. I 
forgot about that. That resolves _some_ of my questions. However, I still find 
the current implementation of `getCUOffset` strange, namely that:
- if the entry contains a `DW_IDX_compile_unit` (perhaps because we have a 
multi-cu index), it returns that CU, regardless of anything else
- if it does not contain the attribute (and we have a single-CU index), it 
returns that CU *only* if the index also does not contain a DW_IDX_type_unit 
(even though the standard says "In a per-CU index, index entries without this 
attribute implicitly refer to the single CU")

This doesn't seem particularly useful. I think it should either always return 
CU offset (regardless of whether it's implicit or explicit) or *only* return it 
if the DW_IDX_type_unit is not present. And of these two, I think the first one 
is better, because the caller can always explicitly check for the presence of 
the type attribute, and it avoids the need to introduce another function (which 
is needed because we now have a caller who wants the CU offset even though the 
entry refers to a type unit).

>From what I can see, that type unit check was added relatively recently 
>(#72952), and it was to support this piece of code in lldb:
```
+  // Look for a DWARF unit offset (CU offset or local TU offset) as they are
+  // both offsets into the .debug_info section.
+  std::optional unit_offset = entry.getCUOffset();
+  if (!unit_offset) {
+unit_offset = entry.getLocalTUOffset();
+if (!unit_offset)
+  return std::nullopt;
+  }
```
If we just reversed the order of those checks in lldb, then it wouldn't matter 
that this function return a CU offset even if the index entry does not refer to 
a DIE in that CU.

Or am I (still) missing something?

https://github.com/llvm/llvm-project/pull/87740
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 445fc51 - [lldb][test] Force dwarf4 usage in test requiring it (#95449)

[via lldb-commits]

Author: Felipe de Azevedo Piovezan
Date: 2024-06-14T12:40:30-07:00
New Revision: 445fc51800d391d0c912d8c6c918b016e0604319

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

LOG: [lldb][test] Force dwarf4 usage in test requiring it (#95449)

This test is explicitly checking for dwarf 4 behavior on Apple
platforms, so we should explicitly use the dwarf4 flag.

Related to https://github.com/llvm/llvm-project/pull/95164

Added: 


Modified: 
lldb/test/Shell/SymbolFile/DWARF/x86/apple-index-is-used.cpp

Removed: 




diff  --git a/lldb/test/Shell/SymbolFile/DWARF/x86/apple-index-is-used.cpp 
b/lldb/test/Shell/SymbolFile/DWARF/x86/apple-index-is-used.cpp
index 00440531e99f7..5bcb2cbcbbe29 100644
--- a/lldb/test/Shell/SymbolFile/DWARF/x86/apple-index-is-used.cpp
+++ b/lldb/test/Shell/SymbolFile/DWARF/x86/apple-index-is-used.cpp
@@ -1,5 +1,5 @@
 // Test that we use the apple indexes.
-// RUN: %clang %s -g -c -o %t --target=x86_64-apple-macosx
+// RUN: %clang %s -g -c -o %t --target=x86_64-apple-macosx -gdwarf-4
 // RUN: lldb-test symbols %t | FileCheck %s
 
 // CHECK: .apple_names index present



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

[Lldb-commits] [lldb] [lldb][test] Force dwarf4 usage in test requiring it (PR #95449)

[Felipe de Azevedo Piovezan via lldb-commits]

https://github.com/felipepiovezan closed 
https://github.com/llvm/llvm-project/pull/95449
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Add options to "statistics dump" to control what sections are dumped (PR #95075)

[Nikita Popov via lldb-commits]

https://github.com/nikic edited https://github.com/llvm/llvm-project/pull/95075
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [WIP][lldb] Use forward decls with redeclared definitions instead of minimal import for records (PR #95100)

[Adrian Prantl via lldb-commits]

https://github.com/adrian-prantl commented:

Very excited to see this happening!

https://github.com/llvm/llvm-project/pull/95100
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [WIP][lldb] Use forward decls with redeclared definitions instead of minimal import for records (PR #95100)

[Adrian Prantl via lldb-commits]

https://github.com/adrian-prantl edited 
https://github.com/llvm/llvm-project/pull/95100
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [WIP][lldb] Use forward decls with redeclared definitions instead of minimal import for records (PR #95100)

[Adrian Prantl via lldb-commits]

@@ -228,6 +246,9 @@ class DWARFASTParserClang : public 
lldb_private::plugin::dwarf::DWARFASTParser {
   void LinkDeclToDIE(clang::Decl *decl,
  const lldb_private::plugin::dwarf::DWARFDIE &die);
 
+  void RegisterDIE(lldb_private::plugin::dwarf::DWARFDebugInfoEntry *die,

adrian-prantl wrote:

Maybe add a doxygen comment for this (or an override keyword)?

https://github.com/llvm/llvm-project/pull/95100
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [WIP][lldb] Use forward decls with redeclared definitions instead of minimal import for records (PR #95100)

[Adrian Prantl via lldb-commits]

@@ -279,9 +281,16 @@ class SemaSourceWithPriorities : public 
clang::ExternalSemaSource {
 return nullptr;
   }
 
+  /// Call ExternalASTSource::CompleteRedeclChain(D)
+  /// on each AST source. Returns as soon as we got
+  /// a definition for D.
   void CompleteRedeclChain(const clang::Decl *D) override {
-for (size_t i = 0; i < Sources.size(); ++i)
+for (size_t i = 0; i < Sources.size(); ++i) {

adrian-prantl wrote:

while we're here: `for (auto source : Sources)`?

https://github.com/llvm/llvm-project/pull/95100
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Fix flaky TestDAP_console test. (PR #94494)

[Adrian Prantl via lldb-commits]

@@ -200,7 +200,10 @@ def test_commands(self):
 # Get output from the console. This should contain both the
 # "exitCommands" that were run after the second breakpoint was hit
 # and the "terminateCommands" due to the debugging session ending
-output = self.collect_console(duration=1.0)
+output = self.collect_console(
+timeout_secs=1.0,

adrian-prantl wrote:

These timeouts look like they're way too small for CI (should be more in the 
10s of seconds range and under ASAN they should be multipled by a factor 10).

https://github.com/llvm/llvm-project/pull/94494
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix the semantics of SupportFile equivalence (PR #95606)

[Jonas Devlieghere via lldb-commits]

https://github.com/JDevlieghere created 
https://github.com/llvm/llvm-project/pull/95606

Currently, two SupportFiles with the same FileSpec are considered different if 
one of them has a Checksum and the other doesn't. However, this is overly 
strict. It's totally valid to mix LineTables that do and do not contain 
Checksums. This patch makes it so that the Checksum is only compared if both 
SupportFiles have a valid Checksum.

>From 1f04fd4a21ffd003183139dc8318deffa57f7f22 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere 
Date: Fri, 14 Jun 2024 13:38:56 -0700
Subject: [PATCH] [lldb] Fix the semantics of SupportFile equivalence

Currently, two SupportFiles with the same FileSpec are considered
different if one of them has a Checksum and the other doesn't. However,
this is overly strict. It's totally valid to mix LineTables that do and
do not contain Checksums. This patch makes it so that the Checksum is
only compared if both SupportFiles have a valid Checksum.
---
 lldb/include/lldb/Utility/SupportFile.h | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lldb/include/lldb/Utility/SupportFile.h 
b/lldb/include/lldb/Utility/SupportFile.h
index 7505d7f345c5d..d65156cea768f 100644
--- a/lldb/include/lldb/Utility/SupportFile.h
+++ b/lldb/include/lldb/Utility/SupportFile.h
@@ -30,8 +30,12 @@ class SupportFile {
 
   virtual ~SupportFile() = default;
 
+  /// Return true if both SupportFiles have the same FileSpec and, if both have
+  /// a valid Checksum, the Checksum is the same.
   bool operator==(const SupportFile &other) const {
-return m_file_spec == other.m_file_spec && m_checksum == other.m_checksum;
+if (m_checksum && other.m_checksum)
+  return m_file_spec == other.m_file_spec && m_checksum == 
other.m_checksum;
+return m_file_spec == other.m_file_spec;
   }
 
   bool operator!=(const SupportFile &other) const { return !(*this == other); }

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

[Lldb-commits] [lldb] [lldb] Fix the semantics of SupportFile equivalence (PR #95606)

[Jonas Devlieghere via lldb-commits]

JDevlieghere wrote:

Credit to @jasonmolenda for uncovering this bug. 

https://github.com/llvm/llvm-project/pull/95606
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix the semantics of SupportFile equivalence (PR #95606)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)


Changes

Currently, two SupportFiles with the same FileSpec are considered different if 
one of them has a Checksum and the other doesn't. However, this is overly 
strict. It's totally valid to mix LineTables that do and do not contain 
Checksums. This patch makes it so that the Checksum is only compared if both 
SupportFiles have a valid Checksum.

---
Full diff: https://github.com/llvm/llvm-project/pull/95606.diff


1 Files Affected:

- (modified) lldb/include/lldb/Utility/SupportFile.h (+5-1) 


``diff
diff --git a/lldb/include/lldb/Utility/SupportFile.h 
b/lldb/include/lldb/Utility/SupportFile.h
index 7505d7f345c5d..d65156cea768f 100644
--- a/lldb/include/lldb/Utility/SupportFile.h
+++ b/lldb/include/lldb/Utility/SupportFile.h
@@ -30,8 +30,12 @@ class SupportFile {
 
   virtual ~SupportFile() = default;
 
+  /// Return true if both SupportFiles have the same FileSpec and, if both have
+  /// a valid Checksum, the Checksum is the same.
   bool operator==(const SupportFile &other) const {
-return m_file_spec == other.m_file_spec && m_checksum == other.m_checksum;
+if (m_checksum && other.m_checksum)
+  return m_file_spec == other.m_file_spec && m_checksum == 
other.m_checksum;
+return m_file_spec == other.m_file_spec;
   }
 
   bool operator!=(const SupportFile &other) const { return !(*this == other); }

``




https://github.com/llvm/llvm-project/pull/95606
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix the semantics of SupportFile equivalence (PR #95606)

[via lldb-commits]

https://github.com/jimingham approved this pull request.

The previous check does seem overly restrictive.

LGTM

https://github.com/llvm/llvm-project/pull/95606
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix the semantics of SupportFile equivalence (PR #95606)

[Alex Langford via lldb-commits]

https://github.com/bulbazord approved this pull request.


https://github.com/llvm/llvm-project/pull/95606
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix the semantics of SupportFile equivalence (PR #95606)

[Jason Molenda via lldb-commits]

https://github.com/jasonmolenda approved this pull request.

Thanks for coming up with the right fix, this looks good to me.

https://github.com/llvm/llvm-project/pull/95606
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Fix the semantics of SupportFile equivalence (PR #95606)

[Jonas Devlieghere via lldb-commits]

JDevlieghere wrote:

Thanks for the overwhelming support :-) 

https://github.com/llvm/llvm-project/pull/95606
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 19ad7a0 - [lldb] Fix the semantics of SupportFile equivalence (#95606)

[via lldb-commits]

Author: Jonas Devlieghere
Date: 2024-06-14T14:25:03-07:00
New Revision: 19ad7a046b5cf435ba95c100170fc0e36231d620

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

LOG: [lldb] Fix the semantics of SupportFile equivalence (#95606)

Currently, two SupportFiles with the same FileSpec are considered
different if one of them has a Checksum and the other doesn't. However,
this is overly strict. It's totally valid to mix LineTables that do and
do not contain Checksums. This patch makes it so that the Checksum is
only compared if both SupportFiles have a valid Checksum.

Added: 


Modified: 
lldb/include/lldb/Utility/SupportFile.h

Removed: 




diff  --git a/lldb/include/lldb/Utility/SupportFile.h 
b/lldb/include/lldb/Utility/SupportFile.h
index 7505d7f345c5d..d65156cea768f 100644
--- a/lldb/include/lldb/Utility/SupportFile.h
+++ b/lldb/include/lldb/Utility/SupportFile.h
@@ -30,8 +30,12 @@ class SupportFile {
 
   virtual ~SupportFile() = default;
 
+  /// Return true if both SupportFiles have the same FileSpec and, if both have
+  /// a valid Checksum, the Checksum is the same.
   bool operator==(const SupportFile &other) const {
-return m_file_spec == other.m_file_spec && m_checksum == other.m_checksum;
+if (m_checksum && other.m_checksum)
+  return m_file_spec == other.m_file_spec && m_checksum == 
other.m_checksum;
+return m_file_spec == other.m_file_spec;
   }
 
   bool operator!=(const SupportFile &other) const { return !(*this == other); }



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

[Lldb-commits] [lldb] [lldb] Fix the semantics of SupportFile equivalence (PR #95606)

[Jonas Devlieghere via lldb-commits]

https://github.com/JDevlieghere closed 
https://github.com/llvm/llvm-project/pull/95606
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [LLDB] Don't cache module sp when Activate() fails. (PR #95586)

[via lldb-commits]

https://github.com/jimingham approved this pull request.

LGTM
I also looked at this a bit, and couldn't come up with a reliable way to write 
a test.

https://github.com/llvm/llvm-project/pull/95586
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [LLDB] Don't cache module sp when Activate() fails. (PR #95586)

[Jonas Devlieghere via lldb-commits]

@@ -60,6 +60,8 @@ void InstrumentationRuntime::ModulesDidLoad(
   if (CheckIfRuntimeIsValid(module_sp)) {
 SetRuntimeModuleSP(module_sp);
 Activate();
+if (IsActive())
+  SetRuntimeModuleSP({}); // Don't cache module if activation failed.

JDevlieghere wrote:

Should this be `if (!IsActive())`? 

https://github.com/llvm/llvm-project/pull/95586
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [LLDB] Don't cache module sp when Activate() fails. (PR #95586)

[via lldb-commits]

@@ -60,6 +60,8 @@ void InstrumentationRuntime::ModulesDidLoad(
   if (CheckIfRuntimeIsValid(module_sp)) {
 SetRuntimeModuleSP(module_sp);
 Activate();
+if (IsActive())
+  SetRuntimeModuleSP({}); // Don't cache module if activation failed.

thetruestblue wrote:

Thanks Jonas. Yes, this should have been amended. 

https://github.com/llvm/llvm-project/pull/95586
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [LLDB] Don't cache module sp when Activate() fails. (PR #95586)

[via lldb-commits]

https://github.com/thetruestblue updated 
https://github.com/llvm/llvm-project/pull/95586

>From db161e5c87eecbd5607f78e7afd00af183aa415c Mon Sep 17 00:00:00 2001
From: Blue Gaston 
Date: Fri, 14 Jun 2024 12:53:27 -0400
Subject: [PATCH 1/2] [LLDB] Don't cache module sp when Activate() fails.

Currently, the instrumentation runtime is caching a library the first time it 
sees it in the module list. However, in some rare cases on Darwin, the cached 
pre-run unloaded modules are different from the runtime module that is loaded 
at runtime. This patch removes the cached module if the plugin fails to 
activate, ensuring that on subsequent calls we don't try to activate using the 
unloaded cached module.

There are a few related bugs to fix in a follow up: CheckIfRuntimeValid should 
have a stronger check to ensure the module is loaded and can be activated. 
Further investigation in UpdateSpecialBinariesFromNewImageInfos calling 
ModulesDidLoad when the module list may have unloaded modules.

I have not included a test for the following reasons:
1. This is an incredibly rare occurance and is only observed in a specific 
circumstance on Darwin. It is tied to behavior in the DynamicLoader thai is not 
commonly encountered.

2. It is difficult to reproduce -- this bug requires precise conditions on 
darwin and it is unclear how we'd reproduce that in a controlled testing 
environment.

rdar://128971453
---
 lldb/source/Target/InstrumentationRuntime.cpp | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lldb/source/Target/InstrumentationRuntime.cpp 
b/lldb/source/Target/InstrumentationRuntime.cpp
index 9f22a1be20ccb..94f4ca353d7ef 100644
--- a/lldb/source/Target/InstrumentationRuntime.cpp
+++ b/lldb/source/Target/InstrumentationRuntime.cpp
@@ -60,6 +60,8 @@ void InstrumentationRuntime::ModulesDidLoad(
   if (CheckIfRuntimeIsValid(module_sp)) {
 SetRuntimeModuleSP(module_sp);
 Activate();
+if (IsActive())
+  SetRuntimeModuleSP({}); // Don't cache module if activation failed.
 return false; // Stop iterating, we're done.
   }
 }

>From f3731ad28824f38ef556665b9ec49d7f610f58b6 Mon Sep 17 00:00:00 2001
From: thetruestblue <92476612+thetruestb...@users.noreply.github.com>
Date: Fri, 14 Jun 2024 19:22:43 -0400
Subject: [PATCH 2/2] Update InstrumentationRuntime.cpp

---
 lldb/source/Target/InstrumentationRuntime.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lldb/source/Target/InstrumentationRuntime.cpp 
b/lldb/source/Target/InstrumentationRuntime.cpp
index 94f4ca353d7ef..9da06e8e155af 100644
--- a/lldb/source/Target/InstrumentationRuntime.cpp
+++ b/lldb/source/Target/InstrumentationRuntime.cpp
@@ -60,7 +60,7 @@ void InstrumentationRuntime::ModulesDidLoad(
   if (CheckIfRuntimeIsValid(module_sp)) {
 SetRuntimeModuleSP(module_sp);
 Activate();
-if (IsActive())
+if (!IsActive())
   SetRuntimeModuleSP({}); // Don't cache module if activation failed.
 return false; // Stop iterating, we're done.
   }

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

[Lldb-commits] [lldb] [LLDB] Don't cache module sp when Activate() fails. (PR #95586)

[via lldb-commits]

https://github.com/thetruestblue edited 
https://github.com/llvm/llvm-project/pull/95586
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Remove SupportFile::Update and use the original Checksum (PR #95623)

[Jonas Devlieghere via lldb-commits]

https://github.com/JDevlieghere created 
https://github.com/llvm/llvm-project/pull/95623

When Jason was looking into the issue caused by #95606 he suggested using the 
Checksum from the original file in LineEntry. I like the idea because it makes 
sense semantically, but also allows us to get rid of the Update method and 
ensures we make a new copy, in case someone else is holding onto the old 
SupportFile.

>From 47d220b4ba90d8cb0339b88c2f03f3f76ac86ba0 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere 
Date: Fri, 14 Jun 2024 16:39:38 -0700
Subject: [PATCH] [lldb] Remove SupportFile::Update and use the original
 Checksum

When Jason was looking into the issue caused by #95606 he suggested
using the Checksum from the original file in LineEntry. I like the idea
because it makes sense semantically, but also allows us to get rid of
the Update method and ensures we make a new copy, in case someone else
is holding onto the old SupportFile.
---
 lldb/include/lldb/Utility/SupportFile.h | 3 ---
 lldb/source/Symbol/LineEntry.cpp| 6 --
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/lldb/include/lldb/Utility/SupportFile.h 
b/lldb/include/lldb/Utility/SupportFile.h
index d65156cea768f..21b986dcaba28 100644
--- a/lldb/include/lldb/Utility/SupportFile.h
+++ b/lldb/include/lldb/Utility/SupportFile.h
@@ -49,9 +49,6 @@ class SupportFile {
   /// Materialize the file to disk and return the path to that temporary file.
   virtual const FileSpec &Materialize() { return m_file_spec; }
 
-  /// Change the file name.
-  void Update(const FileSpec &file_spec) { m_file_spec = file_spec; }
-
 protected:
   FileSpec m_file_spec;
   Checksum m_checksum;
diff --git a/lldb/source/Symbol/LineEntry.cpp b/lldb/source/Symbol/LineEntry.cpp
index 461399e0326e9..19e9bb561375b 100644
--- a/lldb/source/Symbol/LineEntry.cpp
+++ b/lldb/source/Symbol/LineEntry.cpp
@@ -244,7 +244,9 @@ void LineEntry::ApplyFileMappings(lldb::TargetSP target_sp) 
{
   if (target_sp) {
 // Apply any file remappings to our file.
 if (auto new_file_spec = target_sp->GetSourcePathMap().FindFile(
-original_file_sp->GetSpecOnly()))
-  file_sp->Update(*new_file_spec);
+original_file_sp->GetSpecOnly())) {
+  file_sp = std::make_shared(*new_file_spec,
+  original_file_sp->GetChecksum());
+}
   }
 }

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

[Lldb-commits] [lldb] [lldb] Remove SupportFile::Update and use the original Checksum (PR #95623)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)


Changes

When Jason was looking into the issue caused by #95606 he suggested 
using the Checksum from the original file in LineEntry. I like the idea because 
it makes sense semantically, but also allows us to get rid of the Update method 
and ensures we make a new copy, in case someone else is holding onto the old 
SupportFile.

---
Full diff: https://github.com/llvm/llvm-project/pull/95623.diff


2 Files Affected:

- (modified) lldb/include/lldb/Utility/SupportFile.h (-3) 
- (modified) lldb/source/Symbol/LineEntry.cpp (+4-2) 


``diff
diff --git a/lldb/include/lldb/Utility/SupportFile.h 
b/lldb/include/lldb/Utility/SupportFile.h
index d65156cea768f..21b986dcaba28 100644
--- a/lldb/include/lldb/Utility/SupportFile.h
+++ b/lldb/include/lldb/Utility/SupportFile.h
@@ -49,9 +49,6 @@ class SupportFile {
   /// Materialize the file to disk and return the path to that temporary file.
   virtual const FileSpec &Materialize() { return m_file_spec; }
 
-  /// Change the file name.
-  void Update(const FileSpec &file_spec) { m_file_spec = file_spec; }
-
 protected:
   FileSpec m_file_spec;
   Checksum m_checksum;
diff --git a/lldb/source/Symbol/LineEntry.cpp b/lldb/source/Symbol/LineEntry.cpp
index 461399e0326e9..19e9bb561375b 100644
--- a/lldb/source/Symbol/LineEntry.cpp
+++ b/lldb/source/Symbol/LineEntry.cpp
@@ -244,7 +244,9 @@ void LineEntry::ApplyFileMappings(lldb::TargetSP target_sp) 
{
   if (target_sp) {
 // Apply any file remappings to our file.
 if (auto new_file_spec = target_sp->GetSourcePathMap().FindFile(
-original_file_sp->GetSpecOnly()))
-  file_sp->Update(*new_file_spec);
+original_file_sp->GetSpecOnly())) {
+  file_sp = std::make_shared(*new_file_spec,
+  original_file_sp->GetChecksum());
+}
   }
 }

``




https://github.com/llvm/llvm-project/pull/95623
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Remove SupportFile::Update and use the original Checksum (PR #95623)

[Jason Molenda via lldb-commits]

https://github.com/jasonmolenda approved this pull request.

LGTM.

https://github.com/llvm/llvm-project/pull/95623
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Add support for using foreign type units in .debug_names. (PR #87740)

[Greg Clayton via lldb-commits]

clayborg wrote:

> Most of the patch is very clean, but I'm bothered by the 
> `getForeignTUSkeletonCUOffset` function, how it opens up with a mostly 
> redundant (the callers checks this already) call to 
> `getForeignTUTypeSignature`, and then proceeds with a reimplementation of 
> `getCUOffset` (sans the type unit check). I think we could design a better 
> set of APIs for this, but I'm not sure what those is, because I'm unclear of 
> the meaning of various combinations of DW_IDX unit entries.

If we have an entry with a `DW_IDX_type_unit`, it can be a normal type unit, or 
a foreign type unit, that depends on the indexes value. If it is less than the 
number of local type units, then it is a local type unit and there should be no 
`DW_IDX_comp_unit` in the entry, else it is a foreign type unit from a .dwo or 
.dwp file. If we have a .dwp file, we need the `DW_IDX_comp_unit` to be able to 
tell if the foreign type unit made it into the .dwp file, or if this is an 
entry that represents the a different type unit and we should ignore it.  If we 
have no .dwp file, then all entries for foreign type units are valid and we 
don't need to check the compile unit it came from. 

So if you just ask an entry for its `DW_IDX_comp_unit`, you will get an answer 
for an entry from a normal compile unit, or for a foreign type unit with a 
compile unit that will be used to discriminate for the .dwp file case. 

So this function is named `getForeignTUSkeletonCUOffset()` to help say "this 
might have  compile unit entry, but we only want it if this is a foreign type 
unit. I took me a while to understand the spec when it came to foreign type 
units...

> What the new function (I think) essentially does is "give me the CU 
> associated with this entry even if the entry does not describe a DIE in this 
> CU" (because `DW_IDX_die_offset` is relative to a type unit)". I think this 
> API would make sense, even without needing to talk about type units. However, 
> is it actually implementable? For single-CU indexes, that's fine, because we 
> can kind of assume all entries are related to that CU. But what about 
> multi-CU indexes? The only way to determine the CU there is to have an 
> explicit attribute. Are we saying that each entry in a multi-CU index must 
> have a DW_IDX_compile_unit (in addition to a DW_IDX_type_unit)?

Yes, but only if the `DW_IDX_type_unit` represents a foreign type unit, and we 
will only need the `DW_IDX_comp_unit` if we have a .dwp file. But since we 
don't know if the user will use a .dwp file, we always need to put the data for 
the originating compile uint in as a `DW_IDX_comp_unit`.

I would rather have had a `DW_IDX_skeleton_unit` to differentiate between a 
`DW_IDX_comp_unit` and requiring the user to know that they can't just find the 
compile unit, but they must find the non-skeleton compile unit,  before adding 
the `DW_IDX_die_offset`. But we don't have that in the spec...

> 
> If the answer is yes, then this function can be implemented, but then I think 
> the current implementation of `getCUOffset` (which I interpret as "give me 
> the CU offset **IF** this entry is relative to that CU") doesn't make sense 
> -- because it treats entries with an explicit `DW_IDX_compile_unit` different 
> from an implicit/missing `DW_IDX_compile_unit`. 

It doesn't treat them differently. if you ask an entry for its 
`DW_IDX_compile_unit` and it doesn't have one, we can fall back to grabbing a 
CU from the CU table, but only if the CU table has only 1 entry in it.

> And in this world, we assume that `DW_IDX_type_unit` takes precedence over 
> `DW_IDX_compile_unit` -- if both are present then `DW_IDX_die_offset` is 
> relative to the former. And I'm not sure if we would actually want to take up 
> space by putting both values into the index. Looking at existing producers 
> would be interesting, but I'm not sure if there are any (lld does not merge 
> type unit indexes right now, possibly for this very reason). Maybe one could 
> produce something with LTO?

LTO and bolt and soon lld will produce these single tables with multiple 
compile units. 

And we MUST have the `DW_IDX_comp_unit` for foreign type units because type 
units can generate differing content and one copy of the type unit will make it 
into the .dwp and we need to know from which .dwo file it came and only use the 
entries from the matching .dwo file because the offsets can be meaningless for 
type units with the same signature but that came from different .dwo files.

> 
> OTOH, if the answer is no, then the function isn't implementable in the 
> generic case. That doesn't mean you can't implement this feature -- which is 
> useful for guarding against ODR violations (exacerbated by llvm's 
> nonconforming type signature computation algorithm...). However, I think it 
> could be implemented in a much simpler way. For example, lldb could just 
> check if it's looking at a single-CU index, and get the CU offset that way. 
> No extra 

[Lldb-commits] [lldb] [llvm] [LLDB][Minidump] Add 64b support to LLDB's minidump file builder. (PR #95312)

[Greg Clayton via lldb-commits]

@@ -858,10 +953,247 @@ Status MinidumpFileBuilder::Dump(lldb::FileUP 
&core_file) const {
   return error;
 }
 
-size_t MinidumpFileBuilder::GetDirectoriesNum() const {
-  return m_directories.size();
+static size_t GetLargestRange(const Process::CoreFileMemoryRanges &ranges) {
+  size_t max_size = 0;
+  for (const auto &core_range : ranges) {
+max_size = std::max(max_size, core_range.range.size());
+  }

clayborg wrote:

remove `{}` from single line for statement per llvm coding guidelines

https://github.com/llvm/llvm-project/pull/95312
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] [LLDB][Minidump] Add 64b support to LLDB's minidump file builder. (PR #95312)

[Greg Clayton via lldb-commits]

@@ -791,26 +812,101 @@ void MinidumpFileBuilder::AddLinuxFileStreams(
   size_t size = memory_buffer->getBufferSize();
   if (size == 0)
 continue;
-  AddDirectory(stream, size);
+  error = AddDirectory(stream, size);
+  if (error.Fail())
+return error;
   m_data.AppendData(memory_buffer->getBufferStart(), size);
 }
   }
+
+  return error;
 }
 
-Status MinidumpFileBuilder::Dump(lldb::FileUP &core_file) const {
-  constexpr size_t header_size = sizeof(llvm::minidump::Header);
-  constexpr size_t directory_size = sizeof(llvm::minidump::Directory);
+Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
+  Status error;
+
+  Process::CoreFileMemoryRanges ranges_32;
+  Process::CoreFileMemoryRanges ranges_64;
+  error = m_process_sp->CalculateCoreFileSaveRanges(
+  SaveCoreStyle::eSaveCoreStackOnly, ranges_32);
+  if (error.Fail())
+return error;
+
+  std::set stack_start_addresses;
+  for (const auto &core_range : ranges_32)
+stack_start_addresses.insert(core_range.range.start());
+
+  uint64_t total_size =
+  ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor);
+  for (const auto &core_range : ranges_32)
+total_size += core_range.range.size();
+
+  if (total_size >= UINT32_MAX) {
+error.SetErrorStringWithFormat("Unable to write minidump. Stack memory "
+   "exceeds 32b limit. (Num Stacks %zu)",
+   ranges_32.size());
+return error;
+  }
+
+  Process::CoreFileMemoryRanges all_core_memory_ranges;
+  if (core_style != SaveCoreStyle::eSaveCoreStackOnly) {
+error = m_process_sp->CalculateCoreFileSaveRanges(core_style,
+  all_core_memory_ranges);
+if (error.Fail())
+  return error;
+  }
+
+
+  // We need to calculate the MemoryDescriptor size in the worst case
+  // Where all memory descriptors are 64b. We also leave some additional 
padding
+  // So that we convert over to 64b with space to spare. This does not waste
+  // space in the dump But instead converts some memory from being in the
+  // memorylist_32 to the memorylist_64.
+  total_size += 256 + (all_core_memory_ranges.size() - 
stack_start_addresses.size()) *
+  sizeof(llvm::minidump::MemoryDescriptor_64);
 
+  for (const auto &core_range : all_core_memory_ranges) {
+addr_t size_to_add =
+core_range.range.size() + sizeof(llvm::minidump::MemoryDescriptor);
+if (stack_start_addresses.count(core_range.range.start()) > 0) 
+  // Don't double save stacks.
+  continue;
+
+if (total_size + size_to_add < UINT32_MAX) {
+  ranges_32.push_back(core_range);
+  total_size += core_range.range.size();
+  total_size += sizeof(llvm::minidump::MemoryDescriptor);
+} else {
+  ranges_64.push_back(core_range);
+}
+  }
+
+  error = AddMemoryList_32(ranges_32);
+  if (error.Fail())
+return error;
+
+  // Add the remaining memory as a 64b range.
+  if (ranges_64.size() > 0) {

clayborg wrote:

```
if (!ranges_64.empty()) {
```

https://github.com/llvm/llvm-project/pull/95312
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] [LLDB][Minidump] Add 64b support to LLDB's minidump file builder. (PR #95312)

[Greg Clayton via lldb-commits]

@@ -858,10 +953,247 @@ Status MinidumpFileBuilder::Dump(lldb::FileUP 
&core_file) const {
   return error;
 }
 
-size_t MinidumpFileBuilder::GetDirectoriesNum() const {
-  return m_directories.size();
+static size_t GetLargestRange(const Process::CoreFileMemoryRanges &ranges) {
+  size_t max_size = 0;
+  for (const auto &core_range : ranges) {
+max_size = std::max(max_size, core_range.range.size());
+  }
+  return max_size;
 }
 
-size_t MinidumpFileBuilder::GetCurrentDataEndOffset() const {
-  return sizeof(llvm::minidump::Header) + m_data.GetByteSize();
+Status MinidumpFileBuilder::AddMemoryList_32(
+  Process::CoreFileMemoryRanges &ranges) {
+  std::vector descriptors;
+  Status error;
+  if (ranges.size() == 0)
+return error;
+
+  Log *log = GetLog(LLDBLog::Object);
+  size_t region_index = 0;
+  auto data_up = std::make_unique(GetLargestRange(ranges), 0);
+  for (const auto &core_range : ranges) {
+// Take the offset before we write.
+const size_t offset_for_data = GetCurrentDataEndOffset();
+const addr_t addr = core_range.range.start();
+const addr_t size = core_range.range.size();
+const addr_t end = core_range.range.end();
+
+LLDB_LOGF(log,
+  "AddMemoryList %zu/%zu reading memory for region "
+  "(%zu bytes) [%zx, %zx)",
+  region_index, ranges.size(), size, addr, addr + size);
+++region_index;
+
+const size_t bytes_read =
+m_process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
+if (error.Fail() || bytes_read == 0) {
+  LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: 
%s",
+bytes_read, error.AsCString());
+  // Just skip sections with errors or zero bytes in 32b mode
+  continue;
+} else if (bytes_read != size) {
+  LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr,
+size);
+}
+
+MemoryDescriptor descriptor;
+descriptor.StartOfMemoryRange =
+static_cast(addr);
+descriptor.Memory.DataSize =
+static_cast(bytes_read);
+descriptor.Memory.RVA =
+static_cast(offset_for_data);
+descriptors.push_back(descriptor);
+if (m_thread_by_range_end.count(end) > 0)
+  m_thread_by_range_end[end].Stack = descriptor;
+
+// Add the data to the buffer, flush as needed.
+error = AddData(data_up->GetBytes(), bytes_read);
+if (error.Fail())
+  return error;
+  }
+
+  // Add a directory that references this list
+  // With a size of the number of ranges as a 32 bit num
+  // And then the size of all the ranges
+  error = AddDirectory(StreamType::MemoryList,
+   sizeof(llvm::support::ulittle32_t) +
+   descriptors.size() *
+   sizeof(llvm::minidump::MemoryDescriptor));

clayborg wrote:

indent ok here? Run clang-format on this

https://github.com/llvm/llvm-project/pull/95312
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] [LLDB][Minidump] Add 64b support to LLDB's minidump file builder. (PR #95312)

[Greg Clayton via lldb-commits]

@@ -81,38 +82,42 @@ Status 
MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
   // Now offset the file by the directores so we can write them in later.
   offset_t directory_offset = m_expected_directories * directory_size;
   m_saved_data_size += directory_offset;
-  // Replace this when we make a better way to do this.
   Status error;
-  Header empty_header;
-  size_t bytes_written;
-  bytes_written = header_size;
-  error = m_core_file->Write(&empty_header, bytes_written);
-  if (error.Fail() || bytes_written != header_size) {
-if (bytes_written != header_size)
+  size_t zeroes = 0; // 8 0's
+  size_t remaining_bytes = m_saved_data_size;
+  while (remaining_bytes > 0) {
+// Keep filling in zero's until we preallocate enough space for the header
+// and directory sections.
+size_t bytes_written = std::min(remaining_bytes, sizeof(size_t));
+error = m_core_file->Write(&zeroes, bytes_written);
+if (error.Fail()) {
   error.SetErrorStringWithFormat(
-  "unable to write the header (written %zd/%zd)", bytes_written,
-  header_size);
-return error;
-  }
-
-  for (uint i = 0; i < m_expected_directories; i++) {
-size_t bytes_written;
-bytes_written = directory_size;
-Directory empty_directory;
-error = m_core_file->Write(&empty_directory, bytes_written);
-if (error.Fail() || bytes_written != directory_size) {
-  if (bytes_written != directory_size)
-error.SetErrorStringWithFormat(
-"unable to write the directory (written %zd/%zd)", bytes_written,
-directory_size);
-  return error;
+"Unable to write header and directory padding (written %zd/%zd)",
+remaining_bytes - m_saved_data_size, m_saved_data_size);
+  break;
 }
+
+remaining_bytes -= bytes_written;
   }

clayborg wrote:

Remove this while look and complex functionality and just call:
```
m_core_file->SeekFromStart(m_saved_data_size);
```
No need to write zeroes.

https://github.com/llvm/llvm-project/pull/95312
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] [LLDB][Minidump] Add 64b support to LLDB's minidump file builder. (PR #95312)

[Greg Clayton via lldb-commits]

@@ -797,20 +794,89 @@ void MinidumpFileBuilder::AddLinuxFileStreams(
   }
 }
 
-Status MinidumpFileBuilder::Dump(lldb::FileUP &core_file) const {
-  constexpr size_t header_size = sizeof(llvm::minidump::Header);
-  constexpr size_t directory_size = sizeof(llvm::minidump::Directory);
+Status MinidumpFileBuilder::AddMemory(SaveCoreStyle core_style) {
+  Status error;
+
+  Process::CoreFileMemoryRanges ranges_32;
+  Process::CoreFileMemoryRanges ranges_64;
+  error = m_process_sp->CalculateCoreFileSaveRanges(
+  SaveCoreStyle::eSaveCoreStackOnly, ranges_32);
+  if (error.Fail())
+return error;
 
+  std::set stack_start_addresses;
+  for (const auto &core_range : ranges_32)
+stack_start_addresses.insert(core_range.range.start());
+
+  uint64_t total_size =
+  ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor);
+  for (const auto &core_range : ranges_32)
+total_size += core_range.range.size();
+
+  if (total_size >= UINT32_MAX) {
+error.SetErrorStringWithFormat("Unable to write minidump. Stack memory "
+   "exceeds 32b limit. (Num Stacks %zu)",
+   ranges_32.size());
+return error;
+  }
+
+  Process::CoreFileMemoryRanges all_core_memory_ranges;
+  if (core_style != SaveCoreStyle::eSaveCoreStackOnly) {
+error = m_process_sp->CalculateCoreFileSaveRanges(core_style,
+  all_core_memory_ranges);
+if (error.Fail())
+  return error;
+  }
+
+  // We need to calculate the MemoryDescriptor size in the worst case
+  // Where all memory descriptors are 64b. We also leave some additional 
padding
+  // So that we convert over to 64b with space to spare. This does not waste
+  // space in the dump But instead converts some memory from being in the
+  // memorylist_32 to the memorylist_64.
+  total_size += 256 + (ranges_64.size() - stack_start_addresses.size()) *
+  sizeof(llvm::minidump::MemoryDescriptor_64);
+
+  for (const auto &core_range : all_core_memory_ranges) {
+addr_t size_to_add =
+core_range.range.size() + sizeof(llvm::minidump::MemoryDescriptor);
+if (stack_start_addresses.count(core_range.range.start()) > 0) {
+  // Don't double save stacks.
+  continue;
+} else if (total_size + size_to_add < UINT32_MAX) {
+  ranges_32.push_back(core_range);
+  total_size += core_range.range.size();
+  total_size += sizeof(llvm::minidump::MemoryDescriptor);

clayborg wrote:

This can still be just:
```
total_size += size_to_add;
```

https://github.com/llvm/llvm-project/pull/95312
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] [LLDB][Minidump] Add 64b support to LLDB's minidump file builder. (PR #95312)

[Greg Clayton via lldb-commits]

@@ -858,10 +953,247 @@ Status MinidumpFileBuilder::Dump(lldb::FileUP 
&core_file) const {
   return error;
 }
 
-size_t MinidumpFileBuilder::GetDirectoriesNum() const {
-  return m_directories.size();
+static size_t GetLargestRange(const Process::CoreFileMemoryRanges &ranges) {
+  size_t max_size = 0;
+  for (const auto &core_range : ranges) {
+max_size = std::max(max_size, core_range.range.size());
+  }
+  return max_size;
 }
 
-size_t MinidumpFileBuilder::GetCurrentDataEndOffset() const {
-  return sizeof(llvm::minidump::Header) + m_data.GetByteSize();
+Status MinidumpFileBuilder::AddMemoryList_32(
+  Process::CoreFileMemoryRanges &ranges) {
+  std::vector descriptors;
+  Status error;
+  if (ranges.size() == 0)
+return error;
+
+  Log *log = GetLog(LLDBLog::Object);
+  size_t region_index = 0;
+  auto data_up = std::make_unique(GetLargestRange(ranges), 0);
+  for (const auto &core_range : ranges) {
+// Take the offset before we write.
+const size_t offset_for_data = GetCurrentDataEndOffset();
+const addr_t addr = core_range.range.start();
+const addr_t size = core_range.range.size();
+const addr_t end = core_range.range.end();
+
+LLDB_LOGF(log,
+  "AddMemoryList %zu/%zu reading memory for region "
+  "(%zu bytes) [%zx, %zx)",
+  region_index, ranges.size(), size, addr, addr + size);
+++region_index;
+
+const size_t bytes_read =
+m_process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
+if (error.Fail() || bytes_read == 0) {
+  LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: 
%s",
+bytes_read, error.AsCString());
+  // Just skip sections with errors or zero bytes in 32b mode
+  continue;
+} else if (bytes_read != size) {
+  LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr,
+size);
+}
+
+MemoryDescriptor descriptor;
+descriptor.StartOfMemoryRange =
+static_cast(addr);
+descriptor.Memory.DataSize =
+static_cast(bytes_read);
+descriptor.Memory.RVA =
+static_cast(offset_for_data);
+descriptors.push_back(descriptor);
+if (m_thread_by_range_end.count(end) > 0)
+  m_thread_by_range_end[end].Stack = descriptor;
+
+// Add the data to the buffer, flush as needed.
+error = AddData(data_up->GetBytes(), bytes_read);
+if (error.Fail())
+  return error;
+  }
+
+  // Add a directory that references this list
+  // With a size of the number of ranges as a 32 bit num
+  // And then the size of all the ranges
+  error = AddDirectory(StreamType::MemoryList,
+   sizeof(llvm::support::ulittle32_t) +
+   descriptors.size() *
+   sizeof(llvm::minidump::MemoryDescriptor));
+  if (error.Fail())
+return error;
+
+  llvm::support::ulittle32_t memory_ranges_num =
+  static_cast(descriptors.size());

clayborg wrote:

```
llvm::support::ulittle32_t memory_ranges_num(descriptors.size());
```
No need for static_cast right. Lots of places already had these incorrect extra 
static_cast calls..

https://github.com/llvm/llvm-project/pull/95312
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] [LLDB][Minidump] Add 64b support to LLDB's minidump file builder. (PR #95312)

[Greg Clayton via lldb-commits]

@@ -28,17 +29,90 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/BinaryFormat/Minidump.h"
 #include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/Endian.h"
 #include "llvm/Support/Error.h"
+#include "llvm/TargetParser/Triple.h"
 
 #include "Plugins/Process/minidump/MinidumpTypes.h"
+#include "lldb/lldb-enumerations.h"
+#include "lldb/lldb-forward.h"
+#include "lldb/lldb-types.h"
 
+#include 
 #include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace llvm::minidump;
 
-void MinidumpFileBuilder::AddDirectory(StreamType type, size_t stream_size) {
+Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
+  // First set the offset on the file, and on the bytes saved
+  m_saved_data_size += header_size;

clayborg wrote:

Is there a better name for `m_saved_data_size`? 

https://github.com/llvm/llvm-project/pull/95312
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] [LLDB][Minidump] Add 64b support to LLDB's minidump file builder. (PR #95312)

[Greg Clayton via lldb-commits]

@@ -20,25 +20,104 @@
 #include "lldb/Target/RegisterContext.h"
 #include "lldb/Target/StopInfo.h"
 #include "lldb/Target/ThreadList.h"
+#include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
+#include "lldb/Utility/RangeMap.h"
 #include "lldb/Utility/RegisterValue.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/BinaryFormat/Minidump.h"
 #include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/Endian.h"
 #include "llvm/Support/Error.h"
+#include "llvm/TargetParser/Triple.h"
 
 #include "Plugins/Process/minidump/MinidumpTypes.h"
+#include "lldb/lldb-enumerations.h"
+#include "lldb/lldb-forward.h"
+#include "lldb/lldb-types.h"
 
+#include 
 #include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace llvm::minidump;
 
-void MinidumpFileBuilder::AddDirectory(StreamType type, size_t stream_size) {
+Status MinidumpFileBuilder::AddHeaderAndCalculateDirectories() {
+  // First set the offset on the file, and on the bytes saved
+  m_saved_data_size += header_size;
+  // We know we will have at least Misc, SystemInfo, Modules, and ThreadList
+  // (corresponding memory list for stacks) And an additional memory list for
+  // non-stacks.
+  lldb_private::Target &target = m_process_sp->GetTarget();
+  m_expected_directories = 6;
+  // Check if OS is linux and reserve directory space for all linux specific 
breakpad extension directories.
+  if (target.GetArchitecture().GetTriple().getOS() ==
+  llvm::Triple::OSType::Linux)
+m_expected_directories += 9;
+
+  // Go through all of the threads and check for exceptions.
+  lldb_private::ThreadList thread_list = m_process_sp->GetThreadList();
+  const uint32_t num_threads = thread_list.GetSize();
+  for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
+ThreadSP thread_sp(thread_list.GetThreadAtIndex(thread_idx));
+StopInfoSP stop_info_sp = thread_sp->GetStopInfo();
+if (stop_info_sp &&
+stop_info_sp->GetStopReason() == StopReason::eStopReasonException) {
+  m_expected_directories++;
+}
+  }
+
+  // Now offset the file by the directores so we can write them in later.
+  offset_t directory_offset = m_expected_directories * directory_size;
+  m_saved_data_size += directory_offset;
+  Status error;
+  size_t zeroes = 0; // 8 0's
+  size_t remaining_bytes = m_saved_data_size;
+  while (remaining_bytes > 0) {
+// Keep filling in zero's until we preallocate enough space for the header
+// and directory sections.
+size_t bytes_written = std::min(remaining_bytes, sizeof(size_t));
+error = m_core_file->Write(&zeroes, bytes_written);
+if (error.Fail()) {
+  error.SetErrorStringWithFormat(
+"Unable to write header and directory padding (written %zd/%zd)",
+remaining_bytes - m_saved_data_size, m_saved_data_size);
+  break;
+}
+
+remaining_bytes -= bytes_written;
+  }

clayborg wrote:

Just set the file position in the `m_core_file`:
```
m_core_file->SeekFromStart(m_saved_data_size);
```
And remove this entire while loop.

https://github.com/llvm/llvm-project/pull/95312
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] [LLDB][Minidump] Add 64b support to LLDB's minidump file builder. (PR #95312)

[Greg Clayton via lldb-commits]

@@ -858,10 +953,247 @@ Status MinidumpFileBuilder::Dump(lldb::FileUP 
&core_file) const {
   return error;
 }
 
-size_t MinidumpFileBuilder::GetDirectoriesNum() const {
-  return m_directories.size();
+static size_t GetLargestRange(const Process::CoreFileMemoryRanges &ranges) {
+  size_t max_size = 0;
+  for (const auto &core_range : ranges) {
+max_size = std::max(max_size, core_range.range.size());
+  }
+  return max_size;
 }
 
-size_t MinidumpFileBuilder::GetCurrentDataEndOffset() const {
-  return sizeof(llvm::minidump::Header) + m_data.GetByteSize();
+Status MinidumpFileBuilder::AddMemoryList_32(
+  Process::CoreFileMemoryRanges &ranges) {
+  std::vector descriptors;
+  Status error;
+  if (ranges.size() == 0)
+return error;
+
+  Log *log = GetLog(LLDBLog::Object);
+  size_t region_index = 0;
+  auto data_up = std::make_unique(GetLargestRange(ranges), 0);
+  for (const auto &core_range : ranges) {
+// Take the offset before we write.
+const size_t offset_for_data = GetCurrentDataEndOffset();
+const addr_t addr = core_range.range.start();
+const addr_t size = core_range.range.size();
+const addr_t end = core_range.range.end();
+
+LLDB_LOGF(log,
+  "AddMemoryList %zu/%zu reading memory for region "
+  "(%zu bytes) [%zx, %zx)",
+  region_index, ranges.size(), size, addr, addr + size);
+++region_index;
+
+const size_t bytes_read =
+m_process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
+if (error.Fail() || bytes_read == 0) {
+  LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: 
%s",
+bytes_read, error.AsCString());
+  // Just skip sections with errors or zero bytes in 32b mode
+  continue;
+} else if (bytes_read != size) {
+  LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr,
+size);
+}
+
+MemoryDescriptor descriptor;
+descriptor.StartOfMemoryRange =
+static_cast(addr);
+descriptor.Memory.DataSize =
+static_cast(bytes_read);
+descriptor.Memory.RVA =
+static_cast(offset_for_data);
+descriptors.push_back(descriptor);
+if (m_thread_by_range_end.count(end) > 0)
+  m_thread_by_range_end[end].Stack = descriptor;
+
+// Add the data to the buffer, flush as needed.
+error = AddData(data_up->GetBytes(), bytes_read);
+if (error.Fail())
+  return error;
+  }
+
+  // Add a directory that references this list
+  // With a size of the number of ranges as a 32 bit num
+  // And then the size of all the ranges
+  error = AddDirectory(StreamType::MemoryList,
+   sizeof(llvm::support::ulittle32_t) +
+   descriptors.size() *
+   sizeof(llvm::minidump::MemoryDescriptor));
+  if (error.Fail())
+return error;
+
+  llvm::support::ulittle32_t memory_ranges_num =
+  static_cast(descriptors.size());
+  m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle32_t));
+  // For 32b we can get away with writing off the descriptors after the data.
+  // This means no cleanup loop needed.
+  m_data.AppendData(descriptors.data(),
+descriptors.size() * sizeof(MemoryDescriptor));
+
+  return error;
+}
+
+Status MinidumpFileBuilder::AddMemoryList_64(
+Process::CoreFileMemoryRanges &ranges) {
+  Status error;
+  if (ranges.empty()) 
+return error;
+
+  error = AddDirectory(StreamType::Memory64List,
+   (sizeof(llvm::support::ulittle64_t) * 2) +
+   ranges.size() * 
sizeof(llvm::minidump::MemoryDescriptor_64));
+  if (error.Fail())
+return error;
+
+  llvm::support::ulittle64_t memory_ranges_num =
+  static_cast(ranges.size());
+  m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle64_t));
+  // Capture the starting offset for all the descriptors so we can clean them 
up
+  // if needed.
+  offset_t starting_offset = GetCurrentDataEndOffset() + 
sizeof(llvm::support::ulittle64_t);
+  // The base_rva needs to start after the directories, which is right after
+  // this 8 byte variable.
+  offset_t base_rva = starting_offset + (ranges.size() * 
sizeof(llvm::minidump::MemoryDescriptor_64));
+  llvm::support::ulittle64_t memory_ranges_base_rva =
+  static_cast(base_rva);
+  m_data.AppendData(&memory_ranges_base_rva,
+sizeof(llvm::support::ulittle64_t));
+
+  bool cleanup_required = false;
+  std::vector descriptors;
+  // Enumerate the ranges and create the memory descriptors so we can append
+  // them first
+  for (const auto core_range : ranges) {
+// Add the space required to store the memory descriptor
+MemoryDescriptor_64 memory_desc;
+memory_desc.StartOfMemoryRange =
+static_cast(core_range.range.start());
+memory_desc.DataSize =
+static_cast(core_range.range.size());

clayborg wrote:

remove static cast

https://github.com/llvm/llvm-project/pull/95312

[Lldb-commits] [lldb] [llvm] [LLDB][Minidump] Add 64b support to LLDB's minidump file builder. (PR #95312)

[Greg Clayton via lldb-commits]

@@ -858,10 +953,247 @@ Status MinidumpFileBuilder::Dump(lldb::FileUP 
&core_file) const {
   return error;
 }
 
-size_t MinidumpFileBuilder::GetDirectoriesNum() const {
-  return m_directories.size();
+static size_t GetLargestRange(const Process::CoreFileMemoryRanges &ranges) {
+  size_t max_size = 0;
+  for (const auto &core_range : ranges) {
+max_size = std::max(max_size, core_range.range.size());
+  }
+  return max_size;
 }
 
-size_t MinidumpFileBuilder::GetCurrentDataEndOffset() const {
-  return sizeof(llvm::minidump::Header) + m_data.GetByteSize();
+Status MinidumpFileBuilder::AddMemoryList_32(
+  Process::CoreFileMemoryRanges &ranges) {
+  std::vector descriptors;
+  Status error;
+  if (ranges.size() == 0)
+return error;
+
+  Log *log = GetLog(LLDBLog::Object);
+  size_t region_index = 0;
+  auto data_up = std::make_unique(GetLargestRange(ranges), 0);
+  for (const auto &core_range : ranges) {
+// Take the offset before we write.
+const size_t offset_for_data = GetCurrentDataEndOffset();
+const addr_t addr = core_range.range.start();
+const addr_t size = core_range.range.size();
+const addr_t end = core_range.range.end();
+
+LLDB_LOGF(log,
+  "AddMemoryList %zu/%zu reading memory for region "
+  "(%zu bytes) [%zx, %zx)",
+  region_index, ranges.size(), size, addr, addr + size);
+++region_index;
+
+const size_t bytes_read =
+m_process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
+if (error.Fail() || bytes_read == 0) {
+  LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: 
%s",
+bytes_read, error.AsCString());
+  // Just skip sections with errors or zero bytes in 32b mode
+  continue;
+} else if (bytes_read != size) {
+  LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr,
+size);
+}
+
+MemoryDescriptor descriptor;
+descriptor.StartOfMemoryRange =
+static_cast(addr);
+descriptor.Memory.DataSize =
+static_cast(bytes_read);
+descriptor.Memory.RVA =
+static_cast(offset_for_data);
+descriptors.push_back(descriptor);
+if (m_thread_by_range_end.count(end) > 0)
+  m_thread_by_range_end[end].Stack = descriptor;
+
+// Add the data to the buffer, flush as needed.
+error = AddData(data_up->GetBytes(), bytes_read);
+if (error.Fail())
+  return error;
+  }
+
+  // Add a directory that references this list
+  // With a size of the number of ranges as a 32 bit num
+  // And then the size of all the ranges
+  error = AddDirectory(StreamType::MemoryList,
+   sizeof(llvm::support::ulittle32_t) +
+   descriptors.size() *
+   sizeof(llvm::minidump::MemoryDescriptor));
+  if (error.Fail())
+return error;
+
+  llvm::support::ulittle32_t memory_ranges_num =
+  static_cast(descriptors.size());
+  m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle32_t));
+  // For 32b we can get away with writing off the descriptors after the data.
+  // This means no cleanup loop needed.
+  m_data.AppendData(descriptors.data(),
+descriptors.size() * sizeof(MemoryDescriptor));
+
+  return error;
+}
+
+Status MinidumpFileBuilder::AddMemoryList_64(
+Process::CoreFileMemoryRanges &ranges) {
+  Status error;
+  if (ranges.empty()) 
+return error;
+
+  error = AddDirectory(StreamType::Memory64List,
+   (sizeof(llvm::support::ulittle64_t) * 2) +
+   ranges.size() * 
sizeof(llvm::minidump::MemoryDescriptor_64));
+  if (error.Fail())
+return error;
+
+  llvm::support::ulittle64_t memory_ranges_num =
+  static_cast(ranges.size());
+  m_data.AppendData(&memory_ranges_num, sizeof(llvm::support::ulittle64_t));
+  // Capture the starting offset for all the descriptors so we can clean them 
up
+  // if needed.
+  offset_t starting_offset = GetCurrentDataEndOffset() + 
sizeof(llvm::support::ulittle64_t);
+  // The base_rva needs to start after the directories, which is right after
+  // this 8 byte variable.
+  offset_t base_rva = starting_offset + (ranges.size() * 
sizeof(llvm::minidump::MemoryDescriptor_64));
+  llvm::support::ulittle64_t memory_ranges_base_rva =
+  static_cast(base_rva);

clayborg wrote:

no static cast

https://github.com/llvm/llvm-project/pull/95312
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] [LLDB][Minidump] Add 64b support to LLDB's minidump file builder. (PR #95312)

[Greg Clayton via lldb-commits]

@@ -791,26 +812,101 @@ void MinidumpFileBuilder::AddLinuxFileStreams(
   size_t size = memory_buffer->getBufferSize();
   if (size == 0)
 continue;
-  AddDirectory(stream, size);
+  error = AddDirectory(stream, size);
+  if (error.Fail())
+return error;
   m_data.AppendData(memory_buffer->getBufferStart(), size);
 }
   }
+
+  return error;
 }
 
-Status MinidumpFileBuilder::Dump(lldb::FileUP &core_file) const {
-  constexpr size_t header_size = sizeof(llvm::minidump::Header);
-  constexpr size_t directory_size = sizeof(llvm::minidump::Directory);
+Status MinidumpFileBuilder::AddMemoryList(SaveCoreStyle core_style) {
+  Status error;
+
+  Process::CoreFileMemoryRanges ranges_32;
+  Process::CoreFileMemoryRanges ranges_64;
+  error = m_process_sp->CalculateCoreFileSaveRanges(
+  SaveCoreStyle::eSaveCoreStackOnly, ranges_32);
+  if (error.Fail())
+return error;
+
+  std::set stack_start_addresses;
+  for (const auto &core_range : ranges_32)
+stack_start_addresses.insert(core_range.range.start());
+
+  uint64_t total_size =
+  ranges_32.size() * sizeof(llvm::minidump::MemoryDescriptor);
+  for (const auto &core_range : ranges_32)
+total_size += core_range.range.size();

clayborg wrote:

Merge this `total_size` calculation stuff into the loop above on line 836. No 
need to iterate over this twice.

https://github.com/llvm/llvm-project/pull/95312
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] [LLDB][Minidump] Add 64b support to LLDB's minidump file builder. (PR #95312)

[Greg Clayton via lldb-commits]

@@ -858,10 +953,247 @@ Status MinidumpFileBuilder::Dump(lldb::FileUP 
&core_file) const {
   return error;
 }
 
-size_t MinidumpFileBuilder::GetDirectoriesNum() const {
-  return m_directories.size();
+static size_t GetLargestRange(const Process::CoreFileMemoryRanges &ranges) {
+  size_t max_size = 0;
+  for (const auto &core_range : ranges) {
+max_size = std::max(max_size, core_range.range.size());
+  }
+  return max_size;
 }
 
-size_t MinidumpFileBuilder::GetCurrentDataEndOffset() const {
-  return sizeof(llvm::minidump::Header) + m_data.GetByteSize();
+Status MinidumpFileBuilder::AddMemoryList_32(
+  Process::CoreFileMemoryRanges &ranges) {
+  std::vector descriptors;
+  Status error;
+  if (ranges.size() == 0)
+return error;
+
+  Log *log = GetLog(LLDBLog::Object);
+  size_t region_index = 0;
+  auto data_up = std::make_unique(GetLargestRange(ranges), 0);
+  for (const auto &core_range : ranges) {
+// Take the offset before we write.
+const size_t offset_for_data = GetCurrentDataEndOffset();
+const addr_t addr = core_range.range.start();
+const addr_t size = core_range.range.size();
+const addr_t end = core_range.range.end();
+
+LLDB_LOGF(log,
+  "AddMemoryList %zu/%zu reading memory for region "
+  "(%zu bytes) [%zx, %zx)",
+  region_index, ranges.size(), size, addr, addr + size);
+++region_index;
+
+const size_t bytes_read =
+m_process_sp->ReadMemory(addr, data_up->GetBytes(), size, error);
+if (error.Fail() || bytes_read == 0) {
+  LLDB_LOGF(log, "Failed to read memory region. Bytes read: %zu, error: 
%s",
+bytes_read, error.AsCString());
+  // Just skip sections with errors or zero bytes in 32b mode
+  continue;
+} else if (bytes_read != size) {
+  LLDB_LOGF(log, "Memory region at: %zu failed to read %zu bytes", addr,
+size);
+}
+
+MemoryDescriptor descriptor;
+descriptor.StartOfMemoryRange =
+static_cast(addr);
+descriptor.Memory.DataSize =
+static_cast(bytes_read);
+descriptor.Memory.RVA =
+static_cast(offset_for_data);

clayborg wrote:

You can just assign these values with out using the `static_cast` (and yes 
there are many places in this code doing these static_cast calls unecessarily:
```
 descriptor.StartOfMemoryRange = addr;
 descriptor.Memory.DataSize = bytes_read;
 descriptor.Memory.RVA = offset_for_data;
```

https://github.com/llvm/llvm-project/pull/95312
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb] Adds additional fields to ProcessInfo (PR #91544)

[Fred Grim via lldb-commits]

https://github.com/feg208 closed https://github.com/llvm/llvm-project/pull/91544
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 8f74725 - [lldb] Adds additional fields to ProcessInfo (#91544)

[via lldb-commits]

Author: Fred Grim
Date: 2024-06-14T18:30:23-07:00
New Revision: 8f74725731bf431fb97929e1dd962e9a0db20865

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

LOG: [lldb] Adds additional fields to ProcessInfo (#91544)

To implement SaveCore for elf binaries we need to populate some
additional fields in the prpsinfo struct. Those fields are the nice
value of the process whose core is to be taken as well as a boolean flag
indicating whether or not that process is a zombie. This commit adds
those as well as tests to ensure that the values are consistent with
expectations

Added: 


Modified: 
lldb/include/lldb/Utility/ProcessInfo.h
lldb/source/Host/linux/Host.cpp
lldb/source/Utility/ProcessInfo.cpp
lldb/unittests/Host/linux/HostTest.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/ProcessInfo.h 
b/lldb/include/lldb/Utility/ProcessInfo.h
index 54ac000dc7fc2..78ade4bbb1ee6 100644
--- a/lldb/include/lldb/Utility/ProcessInfo.h
+++ b/lldb/include/lldb/Utility/ProcessInfo.h
@@ -15,6 +15,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/NameMatches.h"
 #include "lldb/Utility/StructuredData.h"
+#include 
 #include 
 
 namespace lldb_private {
@@ -147,8 +148,7 @@ class ProcessInstanceInfo : public ProcessInfo {
   ProcessInstanceInfo() = default;
 
   ProcessInstanceInfo(const char *name, const ArchSpec &arch, lldb::pid_t pid)
-  : ProcessInfo(name, arch, pid), m_euid(UINT32_MAX), m_egid(UINT32_MAX),
-m_parent_pid(LLDB_INVALID_PROCESS_ID) {}
+  : ProcessInfo(name, arch, pid) {}
 
   void Clear() {
 ProcessInfo::Clear();
@@ -237,6 +237,16 @@ class ProcessInstanceInfo : public ProcessInfo {
m_cumulative_system_time.tv_usec > 0;
   }
 
+  std::optional GetPriorityValue() const { return m_priority_value; }
+
+  void SetPriorityValue(int8_t priority_value) {
+m_priority_value = priority_value;
+  }
+
+  void SetIsZombie(bool is_zombie) { m_zombie = is_zombie; }
+
+  std::optional IsZombie() const { return m_zombie; }
+
   void Dump(Stream &s, UserIDResolver &resolver) const;
 
   static void DumpTableHeader(Stream &s, bool show_args, bool verbose);
@@ -250,10 +260,12 @@ class ProcessInstanceInfo : public ProcessInfo {
   lldb::pid_t m_parent_pid = LLDB_INVALID_PROCESS_ID;
   lldb::pid_t m_process_group_id = LLDB_INVALID_PROCESS_ID;
   lldb::pid_t m_process_session_id = LLDB_INVALID_PROCESS_ID;
-  struct timespec m_user_time {};
-  struct timespec m_system_time {};
-  struct timespec m_cumulative_user_time {};
-  struct timespec m_cumulative_system_time {};
+  struct timespec m_user_time;
+  struct timespec m_system_time;
+  struct timespec m_cumulative_user_time;
+  struct timespec m_cumulative_system_time;
+  std::optional m_priority_value = std::nullopt;
+  std::optional m_zombie = std::nullopt;
 };
 
 typedef std::vector ProcessInstanceInfoList;

diff  --git a/lldb/source/Host/linux/Host.cpp b/lldb/source/Host/linux/Host.cpp
index c6490f2fc9e2f..5545f9ef4d70e 100644
--- a/lldb/source/Host/linux/Host.cpp
+++ b/lldb/source/Host/linux/Host.cpp
@@ -37,6 +37,7 @@ using namespace lldb;
 using namespace lldb_private;
 
 namespace {
+
 enum class ProcessState {
   Unknown,
   Dead,
@@ -70,6 +71,12 @@ struct StatFields {
   long unsigned stime;
   long cutime;
   long cstime;
+  // In proc_pid_stat(5) this field is specified as priority
+  // but documented as realtime priority. To keep with the adopted
+  // nomenclature in ProcessInstanceInfo, we adopt the documented
+  // naming here.
+  long realtime_priority;
+  long priority;
   //  other things. We don't need them below
 };
 }
@@ -91,14 +98,16 @@ static bool GetStatusInfo(::pid_t Pid, ProcessInstanceInfo 
&ProcessInfo,
   if (Rest.empty())
 return false;
   StatFields stat_fields;
-  if (sscanf(Rest.data(),
- "%d %s %c %d %d %d %d %d %u %lu %lu %lu %lu %lu %lu %ld %ld",
- &stat_fields.pid, stat_fields.comm, &stat_fields.state,
- &stat_fields.ppid, &stat_fields.pgrp, &stat_fields.session,
- &stat_fields.tty_nr, &stat_fields.tpgid, &stat_fields.flags,
- &stat_fields.minflt, &stat_fields.cminflt, &stat_fields.majflt,
- &stat_fields.cmajflt, &stat_fields.utime, &stat_fields.stime,
- &stat_fields.cutime, &stat_fields.cstime) < 0) {
+  if (sscanf(
+  Rest.data(),
+  "%d %s %c %d %d %d %d %d %u %lu %lu %lu %lu %lu %lu %ld %ld %ld %ld",
+  &stat_fields.pid, stat_fields.comm, &stat_fields.state,
+  &stat_fields.ppid, &stat_fields.pgrp, &stat_fields.session,
+  &stat_fields.tty_nr, &stat_fields.tpgid, &stat_fields.flags,
+  &stat_fields.minflt, &stat_fields.cminflt, &stat_fields.majflt,
+  &stat_fields.cmajflt, &stat_fields.utime, &stat_fie

[Lldb-commits] [lldb] [lldb] Adds additional fields to ProcessInfo (PR #91544)

[Fred Grim via lldb-commits]

feg208 wrote:

@JDevlieghere and @clayborg sorry for the line noise. It's merged

https://github.com/llvm/llvm-project/pull/91544
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits