[Lldb-commits] [lldb] 14b5abb - [lldb][AArch64][Linux] Add field information for the fpsr register (#71651)

[via lldb-commits]

Author: David Spickett
Date: 2023-11-09T08:18:44Z
New Revision: 14b5abbfe2071d7ec5eded67f45988e2e484874e

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

LOG: [lldb][AArch64][Linux] Add field information for the fpsr register (#71651)

This one is easy because none of the fields depend on extensions. Only
thing to note is that I've ignored some AArch32 only fields.

```
(lldb) register read fpsr
fpsr = 0x
 = (QC = 0, IDC = 0, IXC = 0, UFC = 0, OFC = 0, DZC = 0, IOC = 0)
```

Added: 


Modified: 
lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
lldb/test/API/commands/register/register/register_command/TestRegisters.py
lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py

Removed: 




diff  --git a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp 
b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
index 043789bd6d21e47..f2f8b3b3211663d 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
+++ b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
@@ -20,6 +20,26 @@
 
 using namespace lldb_private;
 
+LinuxArm64RegisterFlags::Fields
+LinuxArm64RegisterFlags::DetectFPSRFields(uint64_t hwcap, uint64_t hwcap2) {
+  // fpsr's contents are constant.
+  (void)hwcap;
+  (void)hwcap2;
+
+  return {
+  // Bits 31-28 are N/Z/C/V, only used by AArch32.
+  {"QC", 27},
+  // Bits 26-8 reserved.
+  {"IDC", 7},
+  // Bits 6-5 reserved.
+  {"IXC", 4},
+  {"UFC", 3},
+  {"OFC", 2},
+  {"DZC", 1},
+  {"IOC", 0},
+  };
+}
+
 LinuxArm64RegisterFlags::Fields
 LinuxArm64RegisterFlags::DetectCPSRFields(uint64_t hwcap, uint64_t hwcap2) {
   // The fields here are a combination of the Arm manual's SPSR_EL1,

diff  --git a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h 
b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
index 6c7a3b61a1425ee..1058ac2c70bdc5d 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
+++ b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
@@ -56,6 +56,7 @@ class LinuxArm64RegisterFlags {
   using DetectorFn = std::function;
 
   static Fields DetectCPSRFields(uint64_t hwcap, uint64_t hwcap2);
+  static Fields DetectFPSRFields(uint64_t hwcap, uint64_t hwcap2);
 
   struct RegisterEntry {
 RegisterEntry(llvm::StringRef name, unsigned size, DetectorFn detector)
@@ -65,8 +66,9 @@ class LinuxArm64RegisterFlags {
 llvm::StringRef m_name;
 RegisterFlags m_flags;
 DetectorFn m_detector;
-  } m_registers[1] = {
+  } m_registers[2] = {
   RegisterEntry("cpsr", 4, DetectCPSRFields),
+  RegisterEntry("fpsr", 4, DetectFPSRFields),
   };
 
   // Becomes true once field detection has been run for all registers.

diff  --git 
a/lldb/test/API/commands/register/register/register_command/TestRegisters.py 
b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
index f29b2ab5d8f259d..2bd1d6f08f08478 100644
--- a/lldb/test/API/commands/register/register/register_command/TestRegisters.py
+++ b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
@@ -622,13 +622,14 @@ def test_info_register(self):
 @skipUnlessPlatform(["linux"])
 @skipIf(archs=no_match(["aarch64"]))
 def test_register_read_fields(self):
-"""Test that when debugging a live process, we see the fields of the
-CPSR register."""
+"""Test that when debugging a live process, we see the fields of 
certain
+registers."""
 self.build()
 self.common_setup()
 
 # N/Z/C/V bits will always be present, so check only for those.
 self.expect("register read cpsr", substrs=["= (N = 0, Z = 1, C = 1, V 
= 0"])
+self.expect("register read fpsr", substrs=["= (QC = 0, IDC = 0, IXC = 
0"])
 
 @skipUnlessPlatform(["linux"])
 @skipIf(archs=no_match(["x86_64"]))

diff  --git 
a/lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py 
b/lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
index a083fab18eabcbc..1a913c2c0843cbc 100644
--- a/lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ b/lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -577,6 +577,7 @@ def test_aarch64_sve_regs_full(self):
 # Register field information should work with core files as it does a 
live process.
 # The N/Z/C/V bits are always present so just check for those.
 self.expect("register read cpsr", substrs=["= (N = 0, Z = 0, C = 0, V 
= 0"])
+self.expect("register read fpsr", substrs=["= (QC = 0, IDC = 0, IXC = 
0"])
 
 @skipIfLLVMTarge

[Lldb-commits] [lldb] [lldb][AArch64][Linux] Add field information for the fpsr register (PR #71651)

[David Spickett via lldb-commits]

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

[Lldb-commits] [lldb] [lldb] Fix assert in ScriptedProcess destructor (PR #71744)

[David Spickett via lldb-commits]

DavidSpickett wrote:

This did the job, https://lab.llvm.org/buildbot/#/builders/219/builds/6810, 
thanks.

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

[Lldb-commits] [lldb] [lldb][AArch64][Linux] Add fields for FPCR register (PR #71694)

[David Spickett via lldb-commits]

https://github.com/DavidSpickett updated 
https://github.com/llvm/llvm-project/pull/71694

>From 634ee0ebebdbb5bd3e4d3a7f167a592a0da41a33 Mon Sep 17 00:00:00 2001
From: David Spickett 
Date: Wed, 8 Nov 2023 10:50:07 +
Subject: [PATCH] [lldb][AArch64][Linux] Add fields for FPCR register

Follows the format laid out in the Arm manual, AArch32 only fields are ignored.

```
(lldb) register read fpcr
fpcr = 0x
 = (AHP = 0, DN = 0, FZ = 0, RMMode = 0, FZ16 = 0, IDE = 0, IXE = 0, 
UFE = 0, OFE = 0, DZE = 0, IOE = 0)
```

Tests use the first 4 fields that we know are always present.

Converted all the HCWAP defines to `UL` because I'm bound to
forget one if I don't do it now.
---
 .../Utility/RegisterFlagsLinux_arm64.cpp  | 48 +--
 .../Utility/RegisterFlagsLinux_arm64.h|  4 +-
 .../register_command/TestRegisters.py |  4 ++
 .../postmortem/elf-core/TestLinuxCore.py  |  4 ++
 4 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp 
b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
index f2f8b3b3211663d..ba1a0d5599e7703 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
+++ b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
@@ -12,14 +12,54 @@
 // This file is built on all systems because it is used by native processes and
 // core files, so we manually define the needed HWCAP values here.
 
-#define HWCAP_DIT (1 << 24)
-#define HWCAP_SSBS (1 << 28)
+#define HWCAP_FPHP (1UL << 9)
+#define HWCAP_ASIMDHP (1UL << 10)
+#define HWCAP_DIT (1UL << 24)
+#define HWCAP_SSBS (1UL << 28)
 
-#define HWCAP2_BTI (1 << 17)
-#define HWCAP2_MTE (1 << 18)
+#define HWCAP2_BTI (1UL << 17)
+#define HWCAP2_MTE (1UL << 18)
+#define HWCAP2_AFP (1UL << 20)
+#define HWCAP2_EBF16 (1UL << 32)
 
 using namespace lldb_private;
 
+LinuxArm64RegisterFlags::Fields
+LinuxArm64RegisterFlags::DetectFPCRFields(uint64_t hwcap, uint64_t hwcap2) {
+  std::vector fpcr_fields{
+  {"AHP", 26}, {"DN", 25}, {"FZ", 24}, {"RMMode", 22, 23},
+  // Bits 21-20 are "Stride" which is unused in AArch64 state.
+  };
+
+  // FEAT_FP16 is indicated by the presence of FPHP (floating point half
+  // precision) and ASIMDHP (Advanced SIMD half precision) features.
+  if ((hwcap & HWCAP_FPHP) && (hwcap & HWCAP_ASIMDHP))
+fpcr_fields.push_back({"FZ16", 19});
+
+  // Bits 18-16 are "Len" which is unused in AArch64 state.
+
+  fpcr_fields.push_back({"IDE", 15});
+
+  // Bit 14 is unused.
+  if (hwcap2 & HWCAP2_EBF16)
+fpcr_fields.push_back({"EBF", 13});
+
+  fpcr_fields.push_back({"IXE", 12});
+  fpcr_fields.push_back({"UFE", 11});
+  fpcr_fields.push_back({"OFE", 10});
+  fpcr_fields.push_back({"DZE", 9});
+  fpcr_fields.push_back({"IOE", 8});
+  // Bits 7-3 reserved.
+
+  if (hwcap2 & HWCAP2_AFP) {
+fpcr_fields.push_back({"NEP", 2});
+fpcr_fields.push_back({"AH", 1});
+fpcr_fields.push_back({"FIZ", 0});
+  }
+
+  return fpcr_fields;
+}
+
 LinuxArm64RegisterFlags::Fields
 LinuxArm64RegisterFlags::DetectFPSRFields(uint64_t hwcap, uint64_t hwcap2) {
   // fpsr's contents are constant.
diff --git a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h 
b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
index 1058ac2c70bdc5d..651a8c86f7c86a9 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
+++ b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
@@ -57,6 +57,7 @@ class LinuxArm64RegisterFlags {
 
   static Fields DetectCPSRFields(uint64_t hwcap, uint64_t hwcap2);
   static Fields DetectFPSRFields(uint64_t hwcap, uint64_t hwcap2);
+  static Fields DetectFPCRFields(uint64_t hwcap, uint64_t hwcap2);
 
   struct RegisterEntry {
 RegisterEntry(llvm::StringRef name, unsigned size, DetectorFn detector)
@@ -66,9 +67,10 @@ class LinuxArm64RegisterFlags {
 llvm::StringRef m_name;
 RegisterFlags m_flags;
 DetectorFn m_detector;
-  } m_registers[2] = {
+  } m_registers[3] = {
   RegisterEntry("cpsr", 4, DetectCPSRFields),
   RegisterEntry("fpsr", 4, DetectFPSRFields),
+  RegisterEntry("fpcr", 4, DetectFPCRFields),
   };
 
   // Becomes true once field detection has been run for all registers.
diff --git 
a/lldb/test/API/commands/register/register/register_command/TestRegisters.py 
b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
index 2bd1d6f08f08478..386991c18db7c8c 100644
--- a/lldb/test/API/commands/register/register/register_command/TestRegisters.py
+++ b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
@@ -630,6 +630,10 @@ def test_register_read_fields(self):
 # N/Z/C/V bits will always be present, so check only for those.
 self.expect("register read cpsr", substrs=["= (N = 0, Z = 1, C = 1, V 
= 0"])
 self.expect("register read fpsr", substrs=["= (QC = 0, IDC = 0, IXC = 
0"])
+# AHP/DN/FZ/RMMode

[Lldb-commits] [lldb] e3d750c - [lldb][AArch64][Linux] Add fields for FPCR register (#71694)

[via lldb-commits]

Author: David Spickett
Date: 2023-11-09T09:32:24Z
New Revision: e3d750cc40e4cea281924142859dd4b9a6465f99

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

LOG: [lldb][AArch64][Linux] Add fields for FPCR register (#71694)

Follows the format laid out in the Arm manual, AArch32 only fields are
ignored.

```
(lldb) register read fpcr
fpcr = 0x
 = (AHP = 0, DN = 0, FZ = 0, RMMode = 0, FZ16 = 0, IDE = 0, IXE = 0, 
UFE = 0, OFE = 0, DZE = 0, IOE = 0)
```

Tests use the first 4 fields that we know are always present.

Converted all the HCWAP defines to `UL` because I'm bound to
forget one if I don't do it now.

Added: 


Modified: 
lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
lldb/test/API/commands/register/register/register_command/TestRegisters.py
lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py

Removed: 




diff  --git a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp 
b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
index f2f8b3b3211663d..ba1a0d5599e7703 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
+++ b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
@@ -12,14 +12,54 @@
 // This file is built on all systems because it is used by native processes and
 // core files, so we manually define the needed HWCAP values here.
 
-#define HWCAP_DIT (1 << 24)
-#define HWCAP_SSBS (1 << 28)
+#define HWCAP_FPHP (1UL << 9)
+#define HWCAP_ASIMDHP (1UL << 10)
+#define HWCAP_DIT (1UL << 24)
+#define HWCAP_SSBS (1UL << 28)
 
-#define HWCAP2_BTI (1 << 17)
-#define HWCAP2_MTE (1 << 18)
+#define HWCAP2_BTI (1UL << 17)
+#define HWCAP2_MTE (1UL << 18)
+#define HWCAP2_AFP (1UL << 20)
+#define HWCAP2_EBF16 (1UL << 32)
 
 using namespace lldb_private;
 
+LinuxArm64RegisterFlags::Fields
+LinuxArm64RegisterFlags::DetectFPCRFields(uint64_t hwcap, uint64_t hwcap2) {
+  std::vector fpcr_fields{
+  {"AHP", 26}, {"DN", 25}, {"FZ", 24}, {"RMMode", 22, 23},
+  // Bits 21-20 are "Stride" which is unused in AArch64 state.
+  };
+
+  // FEAT_FP16 is indicated by the presence of FPHP (floating point half
+  // precision) and ASIMDHP (Advanced SIMD half precision) features.
+  if ((hwcap & HWCAP_FPHP) && (hwcap & HWCAP_ASIMDHP))
+fpcr_fields.push_back({"FZ16", 19});
+
+  // Bits 18-16 are "Len" which is unused in AArch64 state.
+
+  fpcr_fields.push_back({"IDE", 15});
+
+  // Bit 14 is unused.
+  if (hwcap2 & HWCAP2_EBF16)
+fpcr_fields.push_back({"EBF", 13});
+
+  fpcr_fields.push_back({"IXE", 12});
+  fpcr_fields.push_back({"UFE", 11});
+  fpcr_fields.push_back({"OFE", 10});
+  fpcr_fields.push_back({"DZE", 9});
+  fpcr_fields.push_back({"IOE", 8});
+  // Bits 7-3 reserved.
+
+  if (hwcap2 & HWCAP2_AFP) {
+fpcr_fields.push_back({"NEP", 2});
+fpcr_fields.push_back({"AH", 1});
+fpcr_fields.push_back({"FIZ", 0});
+  }
+
+  return fpcr_fields;
+}
+
 LinuxArm64RegisterFlags::Fields
 LinuxArm64RegisterFlags::DetectFPSRFields(uint64_t hwcap, uint64_t hwcap2) {
   // fpsr's contents are constant.

diff  --git a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h 
b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
index 1058ac2c70bdc5d..651a8c86f7c86a9 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
+++ b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
@@ -57,6 +57,7 @@ class LinuxArm64RegisterFlags {
 
   static Fields DetectCPSRFields(uint64_t hwcap, uint64_t hwcap2);
   static Fields DetectFPSRFields(uint64_t hwcap, uint64_t hwcap2);
+  static Fields DetectFPCRFields(uint64_t hwcap, uint64_t hwcap2);
 
   struct RegisterEntry {
 RegisterEntry(llvm::StringRef name, unsigned size, DetectorFn detector)
@@ -66,9 +67,10 @@ class LinuxArm64RegisterFlags {
 llvm::StringRef m_name;
 RegisterFlags m_flags;
 DetectorFn m_detector;
-  } m_registers[2] = {
+  } m_registers[3] = {
   RegisterEntry("cpsr", 4, DetectCPSRFields),
   RegisterEntry("fpsr", 4, DetectFPSRFields),
+  RegisterEntry("fpcr", 4, DetectFPCRFields),
   };
 
   // Becomes true once field detection has been run for all registers.

diff  --git 
a/lldb/test/API/commands/register/register/register_command/TestRegisters.py 
b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
index 2bd1d6f08f08478..386991c18db7c8c 100644
--- a/lldb/test/API/commands/register/register/register_command/TestRegisters.py
+++ b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
@@ -630,6 +630,10 @@ def test_register_read_fields(self):
 # N/Z/C/V bits will always be present, so check only for those.
  

[Lldb-commits] [lldb] [lldb][AArch64][Linux] Add fields for FPCR register (PR #71694)

[David Spickett via lldb-commits]

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

[Lldb-commits] [lldb] c51a896 - [lldb][AArch64][Linux] Fix HWCAP constant sizes when built on 32 bit

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2023-11-09T10:55:43Z
New Revision: c51a8968a410fb7cfc45acc9f86273ff1cb11736

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

LOG: [lldb][AArch64][Linux] Fix HWCAP constant sizes when built on 32 bit

One of them is << 32 which means it must be a 64 bit value.

Fixes e3d750cc40e4cea281924142859dd4b9a6465f99.

Added: 


Modified: 
lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp 
b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
index ba1a0d5599e7703..87f2464a9eb4868 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
+++ b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
@@ -12,15 +12,15 @@
 // This file is built on all systems because it is used by native processes and
 // core files, so we manually define the needed HWCAP values here.
 
-#define HWCAP_FPHP (1UL << 9)
-#define HWCAP_ASIMDHP (1UL << 10)
-#define HWCAP_DIT (1UL << 24)
-#define HWCAP_SSBS (1UL << 28)
-
-#define HWCAP2_BTI (1UL << 17)
-#define HWCAP2_MTE (1UL << 18)
-#define HWCAP2_AFP (1UL << 20)
-#define HWCAP2_EBF16 (1UL << 32)
+#define HWCAP_FPHP (1ULL << 9)
+#define HWCAP_ASIMDHP (1ULL << 10)
+#define HWCAP_DIT (1ULL << 24)
+#define HWCAP_SSBS (1ULL << 28)
+
+#define HWCAP2_BTI (1ULL << 17)
+#define HWCAP2_MTE (1ULL << 18)
+#define HWCAP2_AFP (1ULL << 20)
+#define HWCAP2_EBF16 (1ULL << 32)
 
 using namespace lldb_private;
 



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

[Lldb-commits] [lldb] [lldb] Replace some register handling asserts with lldbassert (PR #71175)

[David Spickett via lldb-commits]

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

[Lldb-commits] [lldb] [lldb] Replace some register handling asserts with lldbassert (PR #71175)

[David Spickett via lldb-commits]

DavidSpickett wrote:

Discussion continues on the RFC, I will come back to these asserts with smaller 
changes tailored to each use.

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

[Lldb-commits] [lldb] [lldb/Interpreter] Make Scripted*Interface base class abstract (PR #71465)

[Leandro Lupori via lldb-commits]

luporl wrote:

Thanks for the fix.

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

[Lldb-commits] [lldb] Reland "[lldb][DWARFASTParserClang] Fetch constant value from variable defintion if available" (PR #71800)

[Michael Buch via lldb-commits]

https://github.com/Michael137 created 
https://github.com/llvm/llvm-project/pull/71800

This patch relands https://github.com/llvm/llvm-project/pull/71004 which was 
reverted because the clang change it depends on was reverted.

In addition to the original patch, this PR includes a change to 
`SymbolFileDWARF::ParseVariableDIE` to support CU-level variable DIEs that 
don't have locations. Previously, when debug-maps were available, we would 
assume that a variable with "static lifetime" (which in this case means "has a 
linkage name") has a valid address, which isn't the case for non-locationed 
constants. We could omit this additional change if we stopped attaching linkage 
names to global non-locationed constants.

Original commit message:
"""
https://github.com/llvm/llvm-project/pull/71780 proposes moving the 
`DW_AT_const_value` on inline static members from the declaration DIE to the 
definition DIE. This patch makes sure the LLDB's expression evaluator can 
continue to support static initialisers even if the declaration doesn't have a 
`DW_AT_const_value` anymore.

Previously the expression evaluator would find the constant for a VarDecl from 
its declaration `DW_TAG_member` DIE. In cases where the initialiser was 
specified out-of-class, LLDB could find it during symbol resolution.

However, neither of those will work for constants, since we don't have a 
constant attribute on the declaration anymore and we don't have constants in 
the symbol table.
"""

Depends on:
* https://github.com/llvm/llvm-project/pull/71780

>From d3e0391685605491383114f82d906c811f899b5d Mon Sep 17 00:00:00 2001
From: Michael Buch 
Date: Mon, 6 Nov 2023 11:34:09 +
Subject: [PATCH] Reland "[lldb][DWARFASTParserClang] Fetch constant value from
 variable defintion if available"

---
 .../SymbolFile/DWARF/DWARFASTParserClang.cpp  | 63 ++-
 .../SymbolFile/DWARF/DWARFASTParserClang.h| 11 
 .../SymbolFile/DWARF/SymbolFileDWARF.cpp  |  7 +--
 .../SymbolFile/DWARF/SymbolFileDWARF.h| 10 +--
 .../SymbolFile/DWARF/SymbolFileDWARFDwo.cpp   |  7 +++
 .../SymbolFile/DWARF/SymbolFileDWARFDwo.h |  5 ++
 .../TestConstStaticIntegralMember.py  | 53 
 .../cpp/const_static_integral_member/main.cpp | 20 ++
 8 files changed, 166 insertions(+), 10 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 3174c18c97d888c..4e41858674467a3 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -31,6 +31,7 @@
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/TypeList.h"
 #include "lldb/Symbol/TypeMap.h"
+#include "lldb/Symbol/VariableList.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
@@ -133,6 +134,54 @@ static lldb::ModuleSP GetContainingClangModule(const 
DWARFDIE &die) {
   return lldb::ModuleSP();
 }
 
+std::optional
+DWARFASTParserClang::FindConstantOnVariableDefinition(DWARFDIE die) {
+  assert(die.Tag() == llvm::dwarf::DW_TAG_member);
+
+  auto *dwarf = die.GetDWARF();
+  if (!dwarf)
+return {};
+
+  ConstString name{die.GetName()};
+  if (!name)
+return {};
+
+  auto *CU = die.GetCU();
+  if (!CU)
+return {};
+
+  DWARFASTParser *dwarf_ast = dwarf->GetDWARFParser(*CU);
+  auto parent_decl_ctx = dwarf_ast->GetDeclContextContainingUIDFromDWARF(die);
+
+  // Make sure we populate the GetDieToVariable cache.
+  VariableList variables;
+  dwarf->FindGlobalVariables(name, parent_decl_ctx, UINT_MAX, variables);
+
+  // The cache contains the variable definition whose DW_AT_specification
+  // points to our declaration DIE. Look up that definition using our
+  // declaration.
+  auto const &die_to_var = dwarf->GetDIEToVariable();
+  auto it = die_to_var.find(die.GetDIE());
+  if (it == die_to_var.end())
+return {};
+
+  auto var_sp = it->getSecond();
+  assert(var_sp != nullptr);
+
+  if (!var_sp->GetLocationIsConstantValueData())
+return {};
+
+  auto def = dwarf->GetDIE(var_sp->GetID());
+  auto def_attrs = def.GetAttributes();
+  DWARFFormValue form_value;
+  if (!def_attrs.ExtractFormValueAtIndex(
+  def_attrs.FindAttributeIndex(llvm::dwarf::DW_AT_const_value),
+  form_value))
+return {};
+
+  return form_value;
+}
+
 TypeSP DWARFASTParserClang::ParseTypeFromClangModule(const SymbolContext &sc,
  const DWARFDIE &die,
  Log *log) {
@@ -2906,9 +2955,21 @@ void DWARFASTParserClang::ParseSingleMember(
 
   bool unused;
   // TODO: Support float/double static members as well.
-  if (!attrs.const_value_form || !ct.IsIntegerOrEnumerationType(unused))
+  if (!ct.IsIntegerOrEnumerationType(unused))
 return;
 
+  // Newer versions of Clang don't emit the DW_AT_con

[Lldb-commits] [lldb] Reland "[lldb][DWARFASTParserClang] Fetch constant value from variable defintion if available" (PR #71800)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)


Changes

This patch relands https://github.com/llvm/llvm-project/pull/71004 which was 
reverted because the clang change it depends on was reverted.

In addition to the original patch, this PR includes a change to 
`SymbolFileDWARF::ParseVariableDIE` to support CU-level variable DIEs that 
don't have locations. Previously, when debug-maps were available, we would 
assume that a variable with "static lifetime" (which in this case means "has a 
linkage name") has a valid address, which isn't the case for non-locationed 
constants. We could omit this additional change if we stopped attaching linkage 
names to global non-locationed constants.

Original commit message:
"""
https://github.com/llvm/llvm-project/pull/71780 proposes moving the 
`DW_AT_const_value` on inline static members from the declaration DIE to the 
definition DIE. This patch makes sure the LLDB's expression evaluator can 
continue to support static initialisers even if the declaration doesn't have a 
`DW_AT_const_value` anymore.

Previously the expression evaluator would find the constant for a VarDecl from 
its declaration `DW_TAG_member` DIE. In cases where the initialiser was 
specified out-of-class, LLDB could find it during symbol resolution.

However, neither of those will work for constants, since we don't have a 
constant attribute on the declaration anymore and we don't have constants in 
the symbol table.
"""

Depends on:
* https://github.com/llvm/llvm-project/pull/71780

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


8 Files Affected:

- (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
(+62-1) 
- (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h (+11) 
- (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+3-4) 
- (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h (+5-5) 
- (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp (+7) 
- (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h (+5) 
- (modified) 
lldb/test/API/lang/cpp/const_static_integral_member/TestConstStaticIntegralMember.py
 (+53) 
- (modified) lldb/test/API/lang/cpp/const_static_integral_member/main.cpp (+20) 


``diff
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 3174c18c97d888c..4e41858674467a3 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -31,6 +31,7 @@
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/TypeList.h"
 #include "lldb/Symbol/TypeMap.h"
+#include "lldb/Symbol/VariableList.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
@@ -133,6 +134,54 @@ static lldb::ModuleSP GetContainingClangModule(const 
DWARFDIE &die) {
   return lldb::ModuleSP();
 }
 
+std::optional
+DWARFASTParserClang::FindConstantOnVariableDefinition(DWARFDIE die) {
+  assert(die.Tag() == llvm::dwarf::DW_TAG_member);
+
+  auto *dwarf = die.GetDWARF();
+  if (!dwarf)
+return {};
+
+  ConstString name{die.GetName()};
+  if (!name)
+return {};
+
+  auto *CU = die.GetCU();
+  if (!CU)
+return {};
+
+  DWARFASTParser *dwarf_ast = dwarf->GetDWARFParser(*CU);
+  auto parent_decl_ctx = dwarf_ast->GetDeclContextContainingUIDFromDWARF(die);
+
+  // Make sure we populate the GetDieToVariable cache.
+  VariableList variables;
+  dwarf->FindGlobalVariables(name, parent_decl_ctx, UINT_MAX, variables);
+
+  // The cache contains the variable definition whose DW_AT_specification
+  // points to our declaration DIE. Look up that definition using our
+  // declaration.
+  auto const &die_to_var = dwarf->GetDIEToVariable();
+  auto it = die_to_var.find(die.GetDIE());
+  if (it == die_to_var.end())
+return {};
+
+  auto var_sp = it->getSecond();
+  assert(var_sp != nullptr);
+
+  if (!var_sp->GetLocationIsConstantValueData())
+return {};
+
+  auto def = dwarf->GetDIE(var_sp->GetID());
+  auto def_attrs = def.GetAttributes();
+  DWARFFormValue form_value;
+  if (!def_attrs.ExtractFormValueAtIndex(
+  def_attrs.FindAttributeIndex(llvm::dwarf::DW_AT_const_value),
+  form_value))
+return {};
+
+  return form_value;
+}
+
 TypeSP DWARFASTParserClang::ParseTypeFromClangModule(const SymbolContext &sc,
  const DWARFDIE &die,
  Log *log) {
@@ -2906,9 +2955,21 @@ void DWARFASTParserClang::ParseSingleMember(
 
   bool unused;
   // TODO: Support float/double static members as well.
-  if (!attrs.const_value_form || !ct.IsIntegerOrEnumerationType(unused))
+  if (!ct.IsIntegerOrEnumerationType(unused))
 return;
 
+  // Newer versions of Clang don't emit the DW_AT_const_value
+ 

[Lldb-commits] [lldb] Reland "[lldb][DWARFASTParserClang] Fetch constant value from variable defintion if available" (PR #71800)

[Michael Buch via lldb-commits]

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

[Lldb-commits] [lldb] 220abb0 - [lldb][Utility] Fix GCC suggest braces warning for checksum value

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2023-11-09T12:38:01Z
New Revision: 220abb02cb4684d2856ca1a463a531215ffeafd2

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

LOG: [lldb][Utility] Fix GCC suggest braces warning for checksum value

[4314/5686] Building CXX object 
tools/lldb/sou...lity/CMakeFiles/lldbUtility.dir/Checksum.cpp.o
<...>/Checksum.cpp:43:46: warning: suggest braces around initialization of 
subobject [-Wmissing-braces]
llvm::MD5::MD5Result Checksum::g_sentinel = {0, 0, 0, 0, 0, 0, 0, 0,
 ^~~
 {

Added: 


Modified: 
lldb/source/Utility/Checksum.cpp

Removed: 




diff  --git a/lldb/source/Utility/Checksum.cpp 
b/lldb/source/Utility/Checksum.cpp
index a7ad4759f1be809..8943b4e1285208e 100644
--- a/lldb/source/Utility/Checksum.cpp
+++ b/lldb/source/Utility/Checksum.cpp
@@ -40,5 +40,5 @@ std::string Checksum::digest() const {
   return std::string(m_checksum.digest());
 }
 
-llvm::MD5::MD5Result Checksum::g_sentinel = {0, 0, 0, 0, 0, 0, 0, 0,
- 0, 0, 0, 0, 0, 0, 0, 0};
+llvm::MD5::MD5Result Checksum::g_sentinel = {
+{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}};



___
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 ignore artificial variables and members for coroutines (PR #70779)

[Haojian Wu via lldb-commits]

https://github.com/hokein updated 
https://github.com/llvm/llvm-project/pull/70779

>From 2ff56f181659a0079c66ce646d50780844ffb080 Mon Sep 17 00:00:00 2001
From: Haojian Wu 
Date: Tue, 31 Oct 2023 11:15:45 +0100
Subject: [PATCH 1/3] [LLDB] Don't ignore artificial variables and members for
 coroutines

* always populate all fields for the generated coroutine frame type;
* make the artificial variables `__promise`, `__coro_frame` visible, so
  that they are present in the `frame var` command;
---
 .../Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp | 5 -
 lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp | 3 ++-
 lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp | 4 
 lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h   | 3 +++
 4 files changed, 13 insertions(+), 2 deletions(-)

diff --git 
a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp 
b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
index c2488eaa9f5b50d..c7d9eb37f9b5199 100644
--- a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
+++ b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
@@ -41,7 +41,10 @@ CPPLanguageRuntime::CPPLanguageRuntime(Process *process)
 : LanguageRuntime(process) {}
 
 bool CPPLanguageRuntime::IsAllowedRuntimeValue(ConstString name) {
-  return name == g_this;
+  // FIXME: use a list when the list grows more.
+  return name == g_this ||
+ name == ConstString("__promise") ||
+ name == ConstString("__coro_frame");
 }
 
 bool CPPLanguageRuntime::GetObjectDescription(Stream &str,
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 3174c18c97d888c..48e85cac7b4256f 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -3061,7 +3061,8 @@ void DWARFASTParserClang::ParseSingleMember(
   // artificial member with (unnamed bitfield) padding.
   // FIXME: This check should verify that this is indeed an artificial member
   // we are supposed to ignore.
-  if (attrs.is_artificial) {
+  if (attrs.is_artificial &&
+  !TypeSystemClang::IsCoroutineFrameType(class_clang_type)) {
 last_field_info.SetIsArtificial(true);
 return;
   }
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp 
b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index 6f65587c4acedd1..590ba1f6a986ea5 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -771,6 +771,10 @@ TypeSystemClang 
*TypeSystemClang::GetASTContext(clang::ASTContext *ast) {
   return clang_ast;
 }
 
+bool TypeSystemClang::IsCoroutineFrameType(const CompilerType &Type) {
+  return Type.GetTypeName().GetStringRef().ends_with(".coro_frame_ty");
+}
+
 clang::MangleContext *TypeSystemClang::getMangleContext() {
   if (m_mangle_ctx_up == nullptr)
 m_mangle_ctx_up.reset(getASTContext().createMangleContext());
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h 
b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
index 0ec2d026e996105..6168a065eb522e9 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -154,6 +154,9 @@ class TypeSystemClang : public TypeSystem {
 
   static TypeSystemClang *GetASTContext(clang::ASTContext *ast_ctx);
 
+  // Returns true if the given type is a coroutine frame debug type.
+  static bool IsCoroutineFrameType(const CompilerType &Type);
+
   /// Returns the display name of this TypeSystemClang that indicates what
   /// purpose it serves in LLDB. Used for example in logs.
   llvm::StringRef getDisplayName() const { return m_display_name; }

>From ac813b4acd5dbe79fb6d3559e00a454169985067 Mon Sep 17 00:00:00 2001
From: Haojian Wu 
Date: Fri, 3 Nov 2023 12:07:23 +0100
Subject: [PATCH 2/3] Add and use `ShouldIgnoreArtificialField` API to ignore
 the indeed artificial members we should ignore.

---
 lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp | 4 +---
 lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp | 4 ++--
 lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h   | 5 +++--
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 48e85cac7b4256f..63260f28a7da85d 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -3059,10 +3059,8 @@ void DWARFASTParserClang::ParseSingleMember(
   // This needs to be done after updating FieldInfo which keeps track of where
   // field start/end so we don't later try to fill the space of this
   // artificial member with (unnamed bitfield) padding.

[Lldb-commits] [lldb] [LLDB] Don't ignore artificial variables and members for coroutines (PR #70779)

[Haojian Wu via lldb-commits]

https://github.com/hokein edited https://github.com/llvm/llvm-project/pull/70779
___
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 ignore artificial variables and members for coroutines (PR #70779)

[Haojian Wu via lldb-commits]

@@ -3059,9 +3059,8 @@ void DWARFASTParserClang::ParseSingleMember(
   // This needs to be done after updating FieldInfo which keeps track of where
   // field start/end so we don't later try to fill the space of this
   // artificial member with (unnamed bitfield) padding.
-  // FIXME: This check should verify that this is indeed an artificial member
-  // we are supposed to ignore.
-  if (attrs.is_artificial) {
+  if (attrs.is_artificial &&
+  TypeSystemClang::ShouldIgnoreArtificialField(attrs.name)) {

hokein wrote:

This function is only used to parse `DW_TAG_MEMBER`. I took a look on all 
related usages of `createMemberType` with `llvm::DINode::FlagArtificial` in the 
llvm source repo:
- vptr pointer: 
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGDebugInfo.cpp#L2426
- coroutines:
   - 
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Coroutines/CoroFrame.cpp#L1058
  
   - 
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Coroutines/CoroFrame.cpp#L1234

so two cases are virtual pointer, and coroutines frames. And to my best 
knowledge, not ignoring coroutine artificial fields should be fine, the 
coroutine frame is just a regular plain struct . 

https://github.com/llvm/llvm-project/pull/70779
___
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 ignore artificial variables and members for coroutines (PR #70779)

[Haojian Wu via lldb-commits]

@@ -41,7 +41,10 @@ CPPLanguageRuntime::CPPLanguageRuntime(Process *process)
 : LanguageRuntime(process) {}
 
 bool CPPLanguageRuntime::IsAllowedRuntimeValue(ConstString name) {
-  return name == g_this;
+  // FIXME: use a list when the list grows more.
+  return name == g_this ||
+ name == ConstString("__promise") ||
+ name == ConstString("__coro_frame");

hokein wrote:

I removed this change from this pull request now (will sent as a followup). 
Let's focus on addressing the ignore-artificial-fields issue in 
`DWARFASTParserClang.cpp`.

https://github.com/llvm/llvm-project/pull/70779
___
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 ignore artificial variables and members for coroutines (PR #70779)

[Haojian Wu via lldb-commits]

@@ -771,6 +771,10 @@ TypeSystemClang 
*TypeSystemClang::GetASTContext(clang::ASTContext *ast) {
   return clang_ast;
 }
 
+bool TypeSystemClang::ShouldIgnoreArtificialField(llvm::StringRef Name) {
+  return Name.starts_with("_vptr$");

hokein wrote:

Thanks, good catch. Handled the gdb case and added a test case (I thought the 
Clang name in the filename and path name indicates that these part of code only 
cares about clang-generated code, but it looks like we use Clang type system to 
parse the DWARF debug info).

Do we care about the msvc-built program as well? I think? probably not, as we 
use a different debug info format `PDB` on Windows. And this class is only used 
in parsing DWARF.

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

[Lldb-commits] [lldb] [LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember (PR #70779)

[Haojian Wu via lldb-commits]

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

[Lldb-commits] [lldb] [LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember (PR #70779)

[Haojian Wu via lldb-commits]

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

[Lldb-commits] [lldb] [LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember (PR #70779)

[Haojian Wu via lldb-commits]

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

[Lldb-commits] [lldb] [LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember (PR #70779)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Haojian Wu (hokein)


Changes

Part of the fixes #69309.

Address the FIXME, this will allow the lldb print all fields of the generated 
coroutine frame structure.

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


4 Files Affected:

- (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
(+2-3) 
- (modified) lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp (+6) 
- (modified) lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h (+4) 
- (added) lldb/test/Shell/SymbolFile/DWARF/ignored_artificial_fields.test (+17) 


``diff
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 3174c18c97d888c..63260f28a7da85d 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -3059,9 +3059,8 @@ void DWARFASTParserClang::ParseSingleMember(
   // This needs to be done after updating FieldInfo which keeps track of where
   // field start/end so we don't later try to fill the space of this
   // artificial member with (unnamed bitfield) padding.
-  // FIXME: This check should verify that this is indeed an artificial member
-  // we are supposed to ignore.
-  if (attrs.is_artificial) {
+  if (attrs.is_artificial &&
+  TypeSystemClang::ShouldIgnoreArtificialField(attrs.name)) {
 last_field_info.SetIsArtificial(true);
 return;
   }
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp 
b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index 6f65587c4acedd1..e81fdc7bdfd8e40 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -771,6 +771,12 @@ TypeSystemClang 
*TypeSystemClang::GetASTContext(clang::ASTContext *ast) {
   return clang_ast;
 }
 
+bool TypeSystemClang::ShouldIgnoreArtificialField(llvm::StringRef Name) {
+  return Name.starts_with("_vptr$")
+ // gdb emit vtable pointer as "_vptr.classname"
+ || Name.starts_with("_vptr.");
+}
+
 clang::MangleContext *TypeSystemClang::getMangleContext() {
   if (m_mangle_ctx_up == nullptr)
 m_mangle_ctx_up.reset(getASTContext().createMangleContext());
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h 
b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
index 0ec2d026e996105..477b655bb7c86ea 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -154,6 +154,10 @@ class TypeSystemClang : public TypeSystem {
 
   static TypeSystemClang *GetASTContext(clang::ASTContext *ast_ctx);
 
+  // Returns true if the given artificial field name should be ignored when
+  // parsing the DWARF.
+  static bool ShouldIgnoreArtificialField(llvm::StringRef FieldName);
+
   /// Returns the display name of this TypeSystemClang that indicates what
   /// purpose it serves in LLDB. Used for example in logs.
   llvm::StringRef getDisplayName() const { return m_display_name; }
diff --git a/lldb/test/Shell/SymbolFile/DWARF/ignored_artificial_fields.test 
b/lldb/test/Shell/SymbolFile/DWARF/ignored_artificial_fields.test
new file mode 100644
index 000..e7d3bc4b796224a
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/ignored_artificial_fields.test
@@ -0,0 +1,17 @@
+# UNSUPPORTED: system-darwin, system-windows
+
+# Make sure the artifical field `vptr.ClassName` from gcc debug info is 
ignored.
+# RUN: %build --compiler=gcc %S/Inputs/debug-types-expressions.cpp -o %t
+# RUN: %lldb %t -s %s -o exit | FileCheck %s
+
+breakpoint set -n foo
+process launch
+
+# CHECK: Process {{.*}} stopped
+
+frame variable *a
+# CHECK-LABEL: frame variable *a
+# CHECK:  (B) *a = {
+# CHECK-NEXT:   A = (i = 47)
+# CHECK-NEXT:   j = 42
+# CHECK-NEXT: }

``




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

[Lldb-commits] [lldb] [LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember (PR #70779)

[Haojian Wu via lldb-commits]

hokein wrote:

Thanks for the comment, I updated the patch, this is ready for another round of 
review.

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

[Lldb-commits] [lldb] [LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember (PR #70779)

[Michael Buch via lldb-commits]

@@ -771,6 +771,10 @@ TypeSystemClang 
*TypeSystemClang::GetASTContext(clang::ASTContext *ast) {
   return clang_ast;
 }
 
+bool TypeSystemClang::ShouldIgnoreArtificialField(llvm::StringRef Name) {
+  return Name.starts_with("_vptr$");

Michael137 wrote:

`TypeSystemClang` should be agnostic to the debug-info format. The various 
debug-info->AST parsers (e.g., 
`DWARFASTParserClang`/`PDBASTParser`/`npdb::PdbAstBuilder`, the latter two 
being the MSVC debug-info parsers) use `TypeSystemClang` after they've parsed 
debug-info to construct clang AST nodes. But I'm not sure how the PDB parsers 
handle the vptr case and artificial variables. From a brief glance it doesn't 
look like it cares about artificial member variables at all.

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

[Lldb-commits] [lldb] [LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember (PR #70779)

[Michael Buch via lldb-commits]

@@ -771,6 +771,10 @@ TypeSystemClang 
*TypeSystemClang::GetASTContext(clang::ASTContext *ast) {
   return clang_ast;
 }
 
+bool TypeSystemClang::ShouldIgnoreArtificialField(llvm::StringRef Name) {
+  return Name.starts_with("_vptr$");

Michael137 wrote:

Are you still planning on using this API from the `CPPLanguageRuntime`? If not, 
you could just put it inside of `DWARFASTParserClang.cpp`.

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

[Lldb-commits] [lldb] [lldb][AArch64][Linux] Add field information for the mte_ctrl register (PR #71808)

[David Spickett via lldb-commits]

https://github.com/DavidSpickett created 
https://github.com/llvm/llvm-project/pull/71808

This is a Linux pseudo register provided by the NT_ARM_TAGGED_ADDR_CTRL 
register set. It reflects the value passed to prctl PR_SET_TAGGED_ADDR_CTRL.

https://docs.kernel.org/arch/arm64/memory-tagging-extension.html

The fields are made from the #defines the kernel provides for setting the 
value. Its contents are constant so no runtime detection is needed (once we've 
decided we have this register in the first place).

The permitted generated tags is technically a bitfield but at this time we 
don't have a way to mark a field as preferring hex formatting.

```
(lldb) register read mte_ctrl
mte_ctrl = 0x0007fffb
 = (TAGS = 65535, TCF_ASYNC = 0, TCF_SYNC = 1, TAGGED_ADDR_ENABLE = 1)
```

(4 bit tags mean 16 possible tags, 16 bit bitfield)

Testing has been added to TestMTECtrlRegister.py, which needed a more granular 
way to check for XML support, so I've added hasXMLSupport that can be used 
within a test case instead of skipping whole tests if XML isn't supported.

Same for the core file tests.

>From 92e5fc0bf7f5a72a1d89f505715db6a7756b3d52 Mon Sep 17 00:00:00 2001
From: David Spickett 
Date: Wed, 11 Oct 2023 16:17:33 +0100
Subject: [PATCH] [lldb][AArch64][Linux] Add field information for the mte_ctrl
 register

This is a Linux pseudo register provided by the NT_ARM_TAGGED_ADDR_CTRL
register set. It reflects the value passed to prctl PR_SET_TAGGED_ADDR_CTRL.

https://docs.kernel.org/arch/arm64/memory-tagging-extension.html

The fields are made from the #defines the kernel provides for setting
the value. Its contents are constant so no runtime detection is needed
(once we've decided we have this register in the first place).

The permitted generated tags is technically a bitfield but
at this time we don't have a way to mark a field as preferring hex
formatting.

```
(lldb) register read mte_ctrl
mte_ctrl = 0x0007fffb
 = (TAGS = 65535, TCF_ASYNC = 0, TCF_SYNC = 1, TAGGED_ADDR_ENABLE = 1)
```

(4 bit tags mean 16 possible tags, 16 bit bitfield)

Testing has been added to TestMTECtrlRegister.py, which needed a more
granular way to check for XML support, so I've added hasXMLSupport
that can be used within a test case instead of skipping whole tests
if XML isn't supported.

Same for the core file tests.
---
 .../Python/lldbsuite/test/lldbtest.py | 11 
 .../Utility/RegisterFlagsLinux_arm64.cpp  | 13 +
 .../Utility/RegisterFlagsLinux_arm64.h|  4 ++-
 .../TestMTECtrlRegister.py| 27 +--
 .../TestAArch64LinuxMTEMemoryTagCoreFile.py   |  9 ++-
 5 files changed, 54 insertions(+), 10 deletions(-)

diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py 
b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index 15e8ba21266c896..19ba0e8c133edcf 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1224,6 +1224,17 @@ def dumpSessionInfo(self):
 # (enables reading of the current test configuration)
 # 
 
+def hasXMLSupport(self):
+"""Returns True if lldb was built with XML support. Use this check to
+enable parts of tests, if you want to skip a whole test use 
skipIfXmlSupportMissing
+instead."""
+return (
+lldb.SBDebugger.GetBuildConfiguration()
+.GetValueForKey("xml")
+.GetValueForKey("value")
+.GetBooleanValue(False)
+)
+
 def isMIPS(self):
 """Returns true if the architecture is MIPS."""
 arch = self.getArchitecture()
diff --git a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp 
b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
index 87f2464a9eb4868..ca247b628abe78f 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
+++ b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
@@ -24,6 +24,19 @@
 
 using namespace lldb_private;
 
+LinuxArm64RegisterFlags::Fields
+LinuxArm64RegisterFlags::DetectMTECtrlFields(uint64_t hwcap, uint64_t hwcap2) {
+  (void)hwcap;
+  (void)hwcap2;
+  // Represents the contents of NT_ARM_TAGGED_ADDR_CTRL and the value passed
+  // to prctl(PR_TAGGED_ADDR_CTRL...). Fields are derived from the defines
+  // used to build the value.
+  return {{"TAGS", 3, 18}, // 16 bit bitfield shifted up by PR_MTE_TAG_SHIFT.
+  {"TCF_ASYNC", 2},
+  {"TCF_SYNC", 1},
+  {"TAGGED_ADDR_ENABLE", 0}};
+}
+
 LinuxArm64RegisterFlags::Fields
 LinuxArm64RegisterFlags::DetectFPCRFields(uint64_t hwcap, uint64_t hwcap2) {
   std::vector fpcr_fields{
diff --git a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h 
b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
index 651a8c86f7c86a9..deff977a03d0a12 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64

[Lldb-commits] [lldb] [lldb][AArch64][Linux] Add field information for the mte_ctrl register (PR #71808)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: David Spickett (DavidSpickett)


Changes

This is a Linux pseudo register provided by the NT_ARM_TAGGED_ADDR_CTRL 
register set. It reflects the value passed to prctl PR_SET_TAGGED_ADDR_CTRL.

https://docs.kernel.org/arch/arm64/memory-tagging-extension.html

The fields are made from the #defines the kernel provides for setting the 
value. Its contents are constant so no runtime detection is needed (once we've 
decided we have this register in the first place).

The permitted generated tags is technically a bitfield but at this time we 
don't have a way to mark a field as preferring hex formatting.

```
(lldb) register read mte_ctrl
mte_ctrl = 0x0007fffb
 = (TAGS = 65535, TCF_ASYNC = 0, TCF_SYNC = 1, TAGGED_ADDR_ENABLE = 1)
```

(4 bit tags mean 16 possible tags, 16 bit bitfield)

Testing has been added to TestMTECtrlRegister.py, which needed a more granular 
way to check for XML support, so I've added hasXMLSupport that can be used 
within a test case instead of skipping whole tests if XML isn't supported.

Same for the core file tests.

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


5 Files Affected:

- (modified) lldb/packages/Python/lldbsuite/test/lldbtest.py (+11) 
- (modified) lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp 
(+13) 
- (modified) lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h 
(+3-1) 
- (modified) 
lldb/test/API/commands/register/register/aarch64_mte_ctrl_register/TestMTECtrlRegister.py
 (+19-8) 
- (modified) 
lldb/test/API/linux/aarch64/mte_core_file/TestAArch64LinuxMTEMemoryTagCoreFile.py
 (+8-1) 


``diff
diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py 
b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index 15e8ba21266c896..19ba0e8c133edcf 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1224,6 +1224,17 @@ def dumpSessionInfo(self):
 # (enables reading of the current test configuration)
 # 
 
+def hasXMLSupport(self):
+"""Returns True if lldb was built with XML support. Use this check to
+enable parts of tests, if you want to skip a whole test use 
skipIfXmlSupportMissing
+instead."""
+return (
+lldb.SBDebugger.GetBuildConfiguration()
+.GetValueForKey("xml")
+.GetValueForKey("value")
+.GetBooleanValue(False)
+)
+
 def isMIPS(self):
 """Returns true if the architecture is MIPS."""
 arch = self.getArchitecture()
diff --git a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp 
b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
index 87f2464a9eb4868..ca247b628abe78f 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
+++ b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
@@ -24,6 +24,19 @@
 
 using namespace lldb_private;
 
+LinuxArm64RegisterFlags::Fields
+LinuxArm64RegisterFlags::DetectMTECtrlFields(uint64_t hwcap, uint64_t hwcap2) {
+  (void)hwcap;
+  (void)hwcap2;
+  // Represents the contents of NT_ARM_TAGGED_ADDR_CTRL and the value passed
+  // to prctl(PR_TAGGED_ADDR_CTRL...). Fields are derived from the defines
+  // used to build the value.
+  return {{"TAGS", 3, 18}, // 16 bit bitfield shifted up by PR_MTE_TAG_SHIFT.
+  {"TCF_ASYNC", 2},
+  {"TCF_SYNC", 1},
+  {"TAGGED_ADDR_ENABLE", 0}};
+}
+
 LinuxArm64RegisterFlags::Fields
 LinuxArm64RegisterFlags::DetectFPCRFields(uint64_t hwcap, uint64_t hwcap2) {
   std::vector fpcr_fields{
diff --git a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h 
b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
index 651a8c86f7c86a9..deff977a03d0a12 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
+++ b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
@@ -58,6 +58,7 @@ class LinuxArm64RegisterFlags {
   static Fields DetectCPSRFields(uint64_t hwcap, uint64_t hwcap2);
   static Fields DetectFPSRFields(uint64_t hwcap, uint64_t hwcap2);
   static Fields DetectFPCRFields(uint64_t hwcap, uint64_t hwcap2);
+  static Fields DetectMTECtrlFields(uint64_t hwcap, uint64_t hwcap2);
 
   struct RegisterEntry {
 RegisterEntry(llvm::StringRef name, unsigned size, DetectorFn detector)
@@ -67,10 +68,11 @@ class LinuxArm64RegisterFlags {
 llvm::StringRef m_name;
 RegisterFlags m_flags;
 DetectorFn m_detector;
-  } m_registers[3] = {
+  } m_registers[4] = {
   RegisterEntry("cpsr", 4, DetectCPSRFields),
   RegisterEntry("fpsr", 4, DetectFPSRFields),
   RegisterEntry("fpcr", 4, DetectFPCRFields),
+  RegisterEntry("mte_ctrl", 8, DetectMTECtrlFields),
   };
 
   // Becomes true once field detection has been run for all registers.
diff --git 
a/lldb/test/API/commands/register/register/aarch64_m

[Lldb-commits] [lldb] [lldb][AArch64][Linux] Add register field information for SME's SVCR register (PR #71809)

[David Spickett via lldb-commits]

https://github.com/DavidSpickett created 
https://github.com/llvm/llvm-project/pull/71809

This register is a pseudo register but mirrors the architectural
register's contents. See:
https://developer.arm.com/documentation/ddi0616/latest/

For the full details. Example output:
```
(lldb) register read svcr
svcr = 0x0002
 = (ZA = 1, SM = 0)
```

>From 92e5fc0bf7f5a72a1d89f505715db6a7756b3d52 Mon Sep 17 00:00:00 2001
From: David Spickett 
Date: Wed, 11 Oct 2023 16:17:33 +0100
Subject: [PATCH 1/2] [lldb][AArch64][Linux] Add field information for the
 mte_ctrl register

This is a Linux pseudo register provided by the NT_ARM_TAGGED_ADDR_CTRL
register set. It reflects the value passed to prctl PR_SET_TAGGED_ADDR_CTRL.

https://docs.kernel.org/arch/arm64/memory-tagging-extension.html

The fields are made from the #defines the kernel provides for setting
the value. Its contents are constant so no runtime detection is needed
(once we've decided we have this register in the first place).

The permitted generated tags is technically a bitfield but
at this time we don't have a way to mark a field as preferring hex
formatting.

```
(lldb) register read mte_ctrl
mte_ctrl = 0x0007fffb
 = (TAGS = 65535, TCF_ASYNC = 0, TCF_SYNC = 1, TAGGED_ADDR_ENABLE = 1)
```

(4 bit tags mean 16 possible tags, 16 bit bitfield)

Testing has been added to TestMTECtrlRegister.py, which needed a more
granular way to check for XML support, so I've added hasXMLSupport
that can be used within a test case instead of skipping whole tests
if XML isn't supported.

Same for the core file tests.
---
 .../Python/lldbsuite/test/lldbtest.py | 11 
 .../Utility/RegisterFlagsLinux_arm64.cpp  | 13 +
 .../Utility/RegisterFlagsLinux_arm64.h|  4 ++-
 .../TestMTECtrlRegister.py| 27 +--
 .../TestAArch64LinuxMTEMemoryTagCoreFile.py   |  9 ++-
 5 files changed, 54 insertions(+), 10 deletions(-)

diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py 
b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index 15e8ba21266c896..19ba0e8c133edcf 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1224,6 +1224,17 @@ def dumpSessionInfo(self):
 # (enables reading of the current test configuration)
 # 
 
+def hasXMLSupport(self):
+"""Returns True if lldb was built with XML support. Use this check to
+enable parts of tests, if you want to skip a whole test use 
skipIfXmlSupportMissing
+instead."""
+return (
+lldb.SBDebugger.GetBuildConfiguration()
+.GetValueForKey("xml")
+.GetValueForKey("value")
+.GetBooleanValue(False)
+)
+
 def isMIPS(self):
 """Returns true if the architecture is MIPS."""
 arch = self.getArchitecture()
diff --git a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp 
b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
index 87f2464a9eb4868..ca247b628abe78f 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
+++ b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
@@ -24,6 +24,19 @@
 
 using namespace lldb_private;
 
+LinuxArm64RegisterFlags::Fields
+LinuxArm64RegisterFlags::DetectMTECtrlFields(uint64_t hwcap, uint64_t hwcap2) {
+  (void)hwcap;
+  (void)hwcap2;
+  // Represents the contents of NT_ARM_TAGGED_ADDR_CTRL and the value passed
+  // to prctl(PR_TAGGED_ADDR_CTRL...). Fields are derived from the defines
+  // used to build the value.
+  return {{"TAGS", 3, 18}, // 16 bit bitfield shifted up by PR_MTE_TAG_SHIFT.
+  {"TCF_ASYNC", 2},
+  {"TCF_SYNC", 1},
+  {"TAGGED_ADDR_ENABLE", 0}};
+}
+
 LinuxArm64RegisterFlags::Fields
 LinuxArm64RegisterFlags::DetectFPCRFields(uint64_t hwcap, uint64_t hwcap2) {
   std::vector fpcr_fields{
diff --git a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h 
b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
index 651a8c86f7c86a9..deff977a03d0a12 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
+++ b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
@@ -58,6 +58,7 @@ class LinuxArm64RegisterFlags {
   static Fields DetectCPSRFields(uint64_t hwcap, uint64_t hwcap2);
   static Fields DetectFPSRFields(uint64_t hwcap, uint64_t hwcap2);
   static Fields DetectFPCRFields(uint64_t hwcap, uint64_t hwcap2);
+  static Fields DetectMTECtrlFields(uint64_t hwcap, uint64_t hwcap2);
 
   struct RegisterEntry {
 RegisterEntry(llvm::StringRef name, unsigned size, DetectorFn detector)
@@ -67,10 +68,11 @@ class LinuxArm64RegisterFlags {
 llvm::StringRef m_name;
 RegisterFlags m_flags;
 DetectorFn m_detector;
-  } m_registers[3] = {
+  } m_registers[4] = {
   RegisterEntry("cpsr", 4, DetectCPSRFields),
   

[Lldb-commits] [lldb] [lldb][AArch64][Linux] Add register field information for SME's SVCR register (PR #71809)

[David Spickett via lldb-commits]

DavidSpickett wrote:

First commit is https://github.com/llvm/llvm-project/pull/71808, review the 
second.

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

[Lldb-commits] [lldb] [lldb][AArch64][Linux] Add register field information for SME's SVCR register (PR #71809)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: David Spickett (DavidSpickett)


Changes

This register is a pseudo register but mirrors the architectural
register's contents. See:
https://developer.arm.com/documentation/ddi0616/latest/

For the full details. Example output:
```
(lldb) register read svcr
svcr = 0x0002
 = (ZA = 1, SM = 0)
```

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


7 Files Affected:

- (modified) lldb/packages/Python/lldbsuite/test/lldbtest.py (+11) 
- (modified) lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp 
(+26) 
- (modified) lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h 
(+5-1) 
- (modified) 
lldb/test/API/commands/register/register/aarch64_mte_ctrl_register/TestMTECtrlRegister.py
 (+19-8) 
- (modified) 
lldb/test/API/commands/register/register/aarch64_sme_z_registers/save_restore/TestSMEZRegistersSaveRestore.py
 (+8-4) 
- (modified) 
lldb/test/API/linux/aarch64/mte_core_file/TestAArch64LinuxMTEMemoryTagCoreFile.py
 (+8-1) 
- (modified) 
lldb/test/API/linux/aarch64/sme_core_file/TestAArch64LinuxSMECoreFile.py (+9-4) 


``diff
diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py 
b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index 15e8ba21266c896..19ba0e8c133edcf 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1224,6 +1224,17 @@ def dumpSessionInfo(self):
 # (enables reading of the current test configuration)
 # 
 
+def hasXMLSupport(self):
+"""Returns True if lldb was built with XML support. Use this check to
+enable parts of tests, if you want to skip a whole test use 
skipIfXmlSupportMissing
+instead."""
+return (
+lldb.SBDebugger.GetBuildConfiguration()
+.GetValueForKey("xml")
+.GetValueForKey("value")
+.GetBooleanValue(False)
+)
+
 def isMIPS(self):
 """Returns true if the architecture is MIPS."""
 arch = self.getArchitecture()
diff --git a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp 
b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
index 87f2464a9eb4868..77c7116d3c624ae 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
+++ b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.cpp
@@ -24,6 +24,32 @@
 
 using namespace lldb_private;
 
+LinuxArm64RegisterFlags::Fields
+LinuxArm64RegisterFlags::DetectSVCRFields(uint64_t hwcap, uint64_t hwcap2) {
+  (void)hwcap;
+  (void)hwcap2;
+  // Represents the pseudo register that lldb-server builds, which itself
+  // matches the architectural register SCVR. The fields match SVCR in the Arm
+  // manual.
+  return {
+  {"ZA", 1},
+  {"SM", 0},
+  };
+}
+
+LinuxArm64RegisterFlags::Fields
+LinuxArm64RegisterFlags::DetectMTECtrlFields(uint64_t hwcap, uint64_t hwcap2) {
+  (void)hwcap;
+  (void)hwcap2;
+  // Represents the contents of NT_ARM_TAGGED_ADDR_CTRL and the value passed
+  // to prctl(PR_TAGGED_ADDR_CTRL...). Fields are derived from the defines
+  // used to build the value.
+  return {{"TAGS", 3, 18}, // 16 bit bitfield shifted up by PR_MTE_TAG_SHIFT.
+  {"TCF_ASYNC", 2},
+  {"TCF_SYNC", 1},
+  {"TAGGED_ADDR_ENABLE", 0}};
+}
+
 LinuxArm64RegisterFlags::Fields
 LinuxArm64RegisterFlags::DetectFPCRFields(uint64_t hwcap, uint64_t hwcap2) {
   std::vector fpcr_fields{
diff --git a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h 
b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
index 651a8c86f7c86a9..660bef08700f4c8 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
+++ b/lldb/source/Plugins/Process/Utility/RegisterFlagsLinux_arm64.h
@@ -58,6 +58,8 @@ class LinuxArm64RegisterFlags {
   static Fields DetectCPSRFields(uint64_t hwcap, uint64_t hwcap2);
   static Fields DetectFPSRFields(uint64_t hwcap, uint64_t hwcap2);
   static Fields DetectFPCRFields(uint64_t hwcap, uint64_t hwcap2);
+  static Fields DetectMTECtrlFields(uint64_t hwcap, uint64_t hwcap2);
+  static Fields DetectSVCRFields(uint64_t hwcap, uint64_t hwcap2);
 
   struct RegisterEntry {
 RegisterEntry(llvm::StringRef name, unsigned size, DetectorFn detector)
@@ -67,10 +69,12 @@ class LinuxArm64RegisterFlags {
 llvm::StringRef m_name;
 RegisterFlags m_flags;
 DetectorFn m_detector;
-  } m_registers[3] = {
+  } m_registers[5] = {
   RegisterEntry("cpsr", 4, DetectCPSRFields),
   RegisterEntry("fpsr", 4, DetectFPSRFields),
   RegisterEntry("fpcr", 4, DetectFPCRFields),
+  RegisterEntry("mte_ctrl", 8, DetectMTECtrlFields),
+  RegisterEntry("svcr", 8, DetectSVCRFields),
   };
 
   // Becomes true once field detection has been run for all registers.
diff --git 
a/lldb/test/API/commands/register/register/aarch64_mte_ctrl_register/TestMTECtrlRe

[Lldb-commits] [lldb] [LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember (PR #70779)

[Haojian Wu via lldb-commits]

@@ -771,6 +771,10 @@ TypeSystemClang 
*TypeSystemClang::GetASTContext(clang::ASTContext *ast) {
   return clang_ast;
 }
 
+bool TypeSystemClang::ShouldIgnoreArtificialField(llvm::StringRef Name) {
+  return Name.starts_with("_vptr$");

hokein wrote:

> TypeSystemClang should be agnostic to the debug-info format. The various 
> debug-info->AST parsers (e.g., 
> DWARFASTParserClang/PDBASTParser/npdb::PdbAstBuilder, the latter two being 
> the MSVC debug-info parsers) use TypeSystemClang after they've parsed 
> debug-info to construct clang AST nodes.

Thanks for the clarification. 

That makes sense. I don't have any plan to use this API. It is better to move 
DWARFASTParserClang.cpp (better layering).

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

[Lldb-commits] [lldb] [LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember (PR #70779)

[Haojian Wu via lldb-commits]

https://github.com/hokein updated 
https://github.com/llvm/llvm-project/pull/70779

>From 2ff56f181659a0079c66ce646d50780844ffb080 Mon Sep 17 00:00:00 2001
From: Haojian Wu 
Date: Tue, 31 Oct 2023 11:15:45 +0100
Subject: [PATCH 1/4] [LLDB] Don't ignore artificial variables and members for
 coroutines

* always populate all fields for the generated coroutine frame type;
* make the artificial variables `__promise`, `__coro_frame` visible, so
  that they are present in the `frame var` command;
---
 .../Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp | 5 -
 lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp | 3 ++-
 lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp | 4 
 lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h   | 3 +++
 4 files changed, 13 insertions(+), 2 deletions(-)

diff --git 
a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp 
b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
index c2488eaa9f5b50d..c7d9eb37f9b5199 100644
--- a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
+++ b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
@@ -41,7 +41,10 @@ CPPLanguageRuntime::CPPLanguageRuntime(Process *process)
 : LanguageRuntime(process) {}
 
 bool CPPLanguageRuntime::IsAllowedRuntimeValue(ConstString name) {
-  return name == g_this;
+  // FIXME: use a list when the list grows more.
+  return name == g_this ||
+ name == ConstString("__promise") ||
+ name == ConstString("__coro_frame");
 }
 
 bool CPPLanguageRuntime::GetObjectDescription(Stream &str,
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 3174c18c97d888c..48e85cac7b4256f 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -3061,7 +3061,8 @@ void DWARFASTParserClang::ParseSingleMember(
   // artificial member with (unnamed bitfield) padding.
   // FIXME: This check should verify that this is indeed an artificial member
   // we are supposed to ignore.
-  if (attrs.is_artificial) {
+  if (attrs.is_artificial &&
+  !TypeSystemClang::IsCoroutineFrameType(class_clang_type)) {
 last_field_info.SetIsArtificial(true);
 return;
   }
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp 
b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index 6f65587c4acedd1..590ba1f6a986ea5 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -771,6 +771,10 @@ TypeSystemClang 
*TypeSystemClang::GetASTContext(clang::ASTContext *ast) {
   return clang_ast;
 }
 
+bool TypeSystemClang::IsCoroutineFrameType(const CompilerType &Type) {
+  return Type.GetTypeName().GetStringRef().ends_with(".coro_frame_ty");
+}
+
 clang::MangleContext *TypeSystemClang::getMangleContext() {
   if (m_mangle_ctx_up == nullptr)
 m_mangle_ctx_up.reset(getASTContext().createMangleContext());
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h 
b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
index 0ec2d026e996105..6168a065eb522e9 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -154,6 +154,9 @@ class TypeSystemClang : public TypeSystem {
 
   static TypeSystemClang *GetASTContext(clang::ASTContext *ast_ctx);
 
+  // Returns true if the given type is a coroutine frame debug type.
+  static bool IsCoroutineFrameType(const CompilerType &Type);
+
   /// Returns the display name of this TypeSystemClang that indicates what
   /// purpose it serves in LLDB. Used for example in logs.
   llvm::StringRef getDisplayName() const { return m_display_name; }

>From ac813b4acd5dbe79fb6d3559e00a454169985067 Mon Sep 17 00:00:00 2001
From: Haojian Wu 
Date: Fri, 3 Nov 2023 12:07:23 +0100
Subject: [PATCH 2/4] Add and use `ShouldIgnoreArtificialField` API to ignore
 the indeed artificial members we should ignore.

---
 lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp | 4 +---
 lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp | 4 ++--
 lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h   | 5 +++--
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 48e85cac7b4256f..63260f28a7da85d 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -3059,10 +3059,8 @@ void DWARFASTParserClang::ParseSingleMember(
   // This needs to be done after updating FieldInfo which keeps track of where
   // field start/end so we don't later try to fill the space of this
   // artificial member with (unnamed bitfield) padding.

[Lldb-commits] [PATCH] D132734: [lldb] Fix member access in GetExpressionPath

[Andy Yankovsky via Phabricator via lldb-commits]

werat added a comment.

In D132734#3887018 , @JDevlieghere 
wrote:

> Thanks Will, let me apply this locally and see if that addresses the issue 
> for which I reverted the original patch.

Hy Jonas, sorry for bumping an old thread, but did you manage to check if the 
latest revision fixes the issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132734

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

[Lldb-commits] [lldb] [LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember (PR #70779)

[Michael Buch via lldb-commits]

@@ -0,0 +1,17 @@
+# UNSUPPORTED: system-darwin, system-windows

Michael137 wrote:

Why unsupported on Darwin? Pretty sure it should just work

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

[Lldb-commits] [lldb] [LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember (PR #70779)

[Michael Buch via lldb-commits]

@@ -0,0 +1,17 @@
+# UNSUPPORTED: system-darwin, system-windows

Michael137 wrote:

I think we can just remove these and if the test does end up failing we'll just 
add the requirements back after the fact.

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

[Lldb-commits] [lldb] [LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember (PR #70779)

[Michael Buch via lldb-commits]

@@ -0,0 +1,17 @@
+# UNSUPPORTED: system-darwin, system-windows

Michael137 wrote:

Oh you're using gcc for this. Nvm

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

[Lldb-commits] [lldb] [LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember (PR #70779)

[Michael Buch via lldb-commits]

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

LGTM

Are you still going to make the change to `CommandObjectFrame` such that `frame 
var` doesn't hide the artificial fields?

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

[Lldb-commits] [lldb] [LLDB] Ensure the data of apple accelerator tables is kept around (PR #71828)

[Walter Erquinigo via lldb-commits]

https://github.com/walter-erquinigo created 
https://github.com/llvm/llvm-project/pull/71828

lldb's AppleDWARFIndex is created with a few `llvm::AppleAcceleratorTable` 
variables, but they only hold pointers to the underlying data, which doesn't 
prevent the data owner (lldb's DataBufferHeap) from being disposed.

Because of this, an asan build of lldb was showing an error in which a jitted 
accelerator table was being fred before read, which made my lldb fall in an 
infinite loop trying to parse corrupted accel table data. This issue only 
happens when I'm using a jitted execution and not when debugging a precompiled 
binary, probably because in the jit case the underlying data has to necessarily 
be copied via gdb-remote instead of mmaping a debug info file.

The correct fix seems to also store the underlying storage along with the 
accelerator tables in AppleDWARFIndex.


>From ce56e57c27f8386d7be65c0f2c549b38dddfaa28 Mon Sep 17 00:00:00 2001
From: walter erquinigo 
Date: Thu, 9 Nov 2023 11:40:23 -0500
Subject: [PATCH] [LLDB] Ensure the data of apple accelerator tables is kept
 around

lldb's AppleDWARFIndex is created with a few `llvm::AppleAcceleratorTable` 
variables, but they only hold pointers to the underlying data, which doesn't 
prevent the data owner (lldb's DataBufferHeap) from being disposed.

Because of this, an asan build of lldb was showing an error in which a jitted 
accelerator table was being fred before read, which made my lldb fall in an 
infinite loop trying to parse corrupted accel table data. This issue only 
happens when I'm using a jitted execution and not when debugging a precompiled 
binary, probably because in the jit case the underlying data has to necessarily 
be copied via gdb-remote instead of mmaping a debug info file.

The correct fix seems to also store the underlying storage along with the 
accelerator tables in AppleDWARFIndex.
---
 .../SymbolFile/DWARF/AppleDWARFIndex.cpp  |  4 +++-
 .../SymbolFile/DWARF/AppleDWARFIndex.h| 20 +--
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
index 325517ca1d2499b..e25b965e0f5ac8f 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
@@ -57,7 +57,9 @@ std::unique_ptr AppleDWARFIndex::Create(
 return std::make_unique(
 module, std::move(apple_names_table_up),
 std::move(apple_namespaces_table_up), std::move(apple_types_table_up),
-std::move(apple_objc_table_up));
+std::move(apple_objc_table_up), apple_names.GetSharedDataBuffer(),
+apple_namespaces.GetSharedDataBuffer(),
+apple_types.GetSharedDataBuffer(), apple_objc.GetSharedDataBuffer());
 
   return nullptr;
 }
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.h 
b/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.h
index a1fb99700d10a21..73de75b583bd419 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.h
@@ -25,11 +25,19 @@ class AppleDWARFIndex : public DWARFIndex {
   std::unique_ptr apple_names,
   std::unique_ptr 
apple_namespaces,
   std::unique_ptr apple_types,
-  std::unique_ptr apple_objc)
+  std::unique_ptr apple_objc,
+  lldb::DataBufferSP apple_names_storage,
+  lldb::DataBufferSP apple_namespaces_storage,
+  lldb::DataBufferSP apple_types_storage,
+  lldb::DataBufferSP apple_objc_storage)
   : DWARFIndex(module), m_apple_names_up(std::move(apple_names)),
 m_apple_namespaces_up(std::move(apple_namespaces)),
 m_apple_types_up(std::move(apple_types)),
-m_apple_objc_up(std::move(apple_objc)) {}
+m_apple_objc_up(std::move(apple_objc)),
+m_apple_names_storage(apple_names_storage),
+m_apple_namespaces_storage(apple_namespaces_storage),
+m_apple_types_storage(apple_types_storage),
+m_apple_objc_storage(apple_objc_storage) {}
 
   void Preload() override {}
 
@@ -67,6 +75,14 @@ class AppleDWARFIndex : public DWARFIndex {
   std::unique_ptr m_apple_namespaces_up;
   std::unique_ptr m_apple_types_up;
   std::unique_ptr m_apple_objc_up;
+  /// The following storage variables hold the data that the apple accelerator
+  /// tables tables above point to.
+  /// {
+  lldb::DataBufferSP m_apple_names_storage;
+  lldb::DataBufferSP m_apple_namespaces_storage;
+  lldb::DataBufferSP m_apple_types_storage;
+  lldb::DataBufferSP m_apple_objc_storage;
+  /// }
 
   /// Search for entries whose name is `name` in `table`, calling `callback` 
for
   /// each match. If `search_for_tag` is provided, ignore entries whose tag is

___
lldb-commits mailing list
lldb

[Lldb-commits] [lldb] [LLDB] Ensure the data of apple accelerator tables is kept around (PR #71828)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Walter Erquinigo (walter-erquinigo)


Changes

lldb's AppleDWARFIndex is created with a few `llvm::AppleAcceleratorTable` 
variables, but they only hold pointers to the underlying data, which doesn't 
prevent the data owner (lldb's DataBufferHeap) from being disposed.

Because of this, an asan build of lldb was showing an error in which a jitted 
accelerator table was being fred before read, which made my lldb fall in an 
infinite loop trying to parse corrupted accel table data. This issue only 
happens when I'm using a jitted execution and not when debugging a precompiled 
binary, probably because in the jit case the underlying data has to necessarily 
be copied via gdb-remote instead of mmaping a debug info file.

The correct fix seems to also store the underlying storage along with the 
accelerator tables in AppleDWARFIndex.


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


2 Files Affected:

- (modified) lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp (+3-1) 
- (modified) lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.h (+18-2) 


``diff
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
index 325517ca1d2499b..e25b965e0f5ac8f 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
@@ -57,7 +57,9 @@ std::unique_ptr AppleDWARFIndex::Create(
 return std::make_unique(
 module, std::move(apple_names_table_up),
 std::move(apple_namespaces_table_up), std::move(apple_types_table_up),
-std::move(apple_objc_table_up));
+std::move(apple_objc_table_up), apple_names.GetSharedDataBuffer(),
+apple_namespaces.GetSharedDataBuffer(),
+apple_types.GetSharedDataBuffer(), apple_objc.GetSharedDataBuffer());
 
   return nullptr;
 }
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.h 
b/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.h
index a1fb99700d10a21..73de75b583bd419 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.h
@@ -25,11 +25,19 @@ class AppleDWARFIndex : public DWARFIndex {
   std::unique_ptr apple_names,
   std::unique_ptr 
apple_namespaces,
   std::unique_ptr apple_types,
-  std::unique_ptr apple_objc)
+  std::unique_ptr apple_objc,
+  lldb::DataBufferSP apple_names_storage,
+  lldb::DataBufferSP apple_namespaces_storage,
+  lldb::DataBufferSP apple_types_storage,
+  lldb::DataBufferSP apple_objc_storage)
   : DWARFIndex(module), m_apple_names_up(std::move(apple_names)),
 m_apple_namespaces_up(std::move(apple_namespaces)),
 m_apple_types_up(std::move(apple_types)),
-m_apple_objc_up(std::move(apple_objc)) {}
+m_apple_objc_up(std::move(apple_objc)),
+m_apple_names_storage(apple_names_storage),
+m_apple_namespaces_storage(apple_namespaces_storage),
+m_apple_types_storage(apple_types_storage),
+m_apple_objc_storage(apple_objc_storage) {}
 
   void Preload() override {}
 
@@ -67,6 +75,14 @@ class AppleDWARFIndex : public DWARFIndex {
   std::unique_ptr m_apple_namespaces_up;
   std::unique_ptr m_apple_types_up;
   std::unique_ptr m_apple_objc_up;
+  /// The following storage variables hold the data that the apple accelerator
+  /// tables tables above point to.
+  /// {
+  lldb::DataBufferSP m_apple_names_storage;
+  lldb::DataBufferSP m_apple_namespaces_storage;
+  lldb::DataBufferSP m_apple_types_storage;
+  lldb::DataBufferSP m_apple_objc_storage;
+  /// }
 
   /// Search for entries whose name is `name` in `table`, calling `callback` 
for
   /// each match. If `search_for_tag` is provided, ignore entries whose tag is

``




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

[Lldb-commits] [lldb] [lldb] Read Checksum from DWARF line tables (PR #71458)

[Jonas Devlieghere via lldb-commits]

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

[Lldb-commits] [lldb] 5da98de - [lldb] Read Checksum from DWARF line tables (#71458)

[via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-11-09T08:59:03-08:00
New Revision: 5da98dec7ab8c3adc3f70147ea666428431adf34

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

LOG: [lldb] Read Checksum from DWARF line tables (#71458)

Read the MD5 checksum from DWARF line tables and store it in the
corresponding support files.

Added: 


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

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index aabd83a194932ff..79d44bce3d663b6 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -229,8 +229,15 @@ ParseSupportFilesFromPrologue(const lldb::ModuleSP &module,
 remapped_file = std::move(*file_path);
 }
 
+Checksum checksum;
+if (prologue.ContentTypes.HasMD5) {
+  const llvm::DWARFDebugLine::FileNameEntry &file_name_entry =
+  prologue.getFileNameEntry(idx);
+  checksum = file_name_entry.Checksum;
+}
+
 // Unconditionally add an entry, so the indices match up.
-support_files.EmplaceBack(remapped_file, style);
+support_files.EmplaceBack(remapped_file, style, checksum);
   }
 
   return support_files;



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

[Lldb-commits] [lldb] Add new API in SBTarget for loading core from SBFile (PR #71769)

[Alex Langford via lldb-commits]

@@ -184,6 +184,8 @@ class LLDB_API SBTarget {
 
   SBProcess LoadCore(const char *core_file);
   SBProcess LoadCore(const char *core_file, lldb::SBError &error);
+  SBProcess LoadCore(SBFile &file);

bulbazord wrote:

+1, please don't add a new API that gives no feedback about what may have gone 
wrong.

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

[Lldb-commits] [lldb] Add new API in SBTarget for loading core from SBFile (PR #71769)

[Alex Langford via lldb-commits]

@@ -244,9 +245,45 @@ SBProcess SBTarget::LoadCore(const char *core_file, 
lldb::SBError &error) {
   TargetSP target_sp(GetSP());
   if (target_sp) {
 FileSpec filespec(core_file);
-FileSystem::Instance().Resolve(filespec);
+auto file = FileSystem::Instance().Open(
+filespec, lldb_private::File::eOpenOptionReadOnly);
+if (!file) {
+  error.SetErrorStringWithFormat("Failed to open the core file: %s",
+ llvm::toString(file.takeError()).c_str());
+  return sb_process;
+}
+ProcessSP process_sp(
+target_sp->CreateProcess(target_sp->GetDebugger().GetListener(), "",
+ std::move(file.get()), false));
+if (process_sp) {
+  error.SetError(process_sp->LoadCore());
+  if (error.Success())
+sb_process.SetSP(process_sp);
+} else {
+  error.SetErrorString("Failed to create the process");
+}
+  } else {
+error.SetErrorString("SBTarget is invalid");
+  }

bulbazord wrote:

If you flip the condition that checks target_sp for validity, you can save a 
level of indentation for readability (if that matters to you).

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

[Lldb-commits] [lldb] Add new API in SBTarget for loading core from SBFile (PR #71769)

[Alex Langford via lldb-commits]

@@ -390,12 +389,12 @@ class Process : public 
std::enable_shared_from_this,
   ConstString &GetBroadcasterClass() const override {
 return GetStaticBroadcasterClass();
   }
-  
-/// A notification structure that can be used by clients to listen
-/// for changes in a process's lifetime.
-///
-/// \see RegisterNotificationCallbacks (const Notifications&) @see
-/// UnregisterNotificationCallbacks (const Notifications&)
+
+  /// A notification structure that can be used by clients to listen
+  /// for changes in a process's lifetime.
+  ///
+  /// \see RegisterNotificationCallbacks (const Notifications&) @see
+  /// UnregisterNotificationCallbacks (const Notifications&)

bulbazord wrote:

Unrelated formatting change

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

[Lldb-commits] [lldb] Add new API in SBTarget for loading core from SBFile (PR #71769)

[Alex Langford via lldb-commits]

@@ -61,13 +62,15 @@ void ProcessMachCore::Terminate() {
 
 lldb::ProcessSP ProcessMachCore::CreateInstance(lldb::TargetSP target_sp,
 ListenerSP listener_sp,
-const FileSpec *crash_file,
+lldb::FileSP crash_file_sp,
 bool can_connect) {
   lldb::ProcessSP process_sp;
-  if (crash_file && !can_connect) {
+  if (crash_file_sp && !can_connect) {
 const size_t header_size = sizeof(llvm::MachO::mach_header);
-auto data_sp = FileSystem::Instance().CreateDataBuffer(
-crash_file->GetPath(), header_size, 0);
+const FileSpec file_spec;
+crash_file_sp->GetFileSpec(const_cast(file_spec));

bulbazord wrote:

Also here

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

[Lldb-commits] [lldb] Add new API in SBTarget for loading core from SBFile (PR #71769)

[Alex Langford via lldb-commits]

@@ -102,10 +102,10 @@ void ProcessKDP::Terminate() {
 
 lldb::ProcessSP ProcessKDP::CreateInstance(TargetSP target_sp,
ListenerSP listener_sp,
-   const FileSpec *crash_file_path,
+   FileSP crash_file_sp,
bool can_connect) {
   lldb::ProcessSP process_sp;
-  if (crash_file_path == NULL)
+  if (crash_file_sp == NULL)

bulbazord wrote:

Since you're already here, you can drop the `== NULL` and make it a little 
nicer.

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

[Lldb-commits] [lldb] Add new API in SBTarget for loading core from SBFile (PR #71769)

[Alex Langford via lldb-commits]

@@ -49,17 +50,18 @@ void ProcessElfCore::Terminate() {
 
 lldb::ProcessSP ProcessElfCore::CreateInstance(lldb::TargetSP target_sp,
lldb::ListenerSP listener_sp,
-   const FileSpec *crash_file,
+   lldb::FileSP crash_file_sp,
bool can_connect) {
   lldb::ProcessSP process_sp;
-  if (crash_file && !can_connect) {
+  if (crash_file_sp && !can_connect) {
 // Read enough data for an ELF32 header or ELF64 header Note: Here we care
 // about e_type field only, so it is safe to ignore possible presence of
 // the header extension.
 const size_t header_size = sizeof(llvm::ELF::Elf64_Ehdr);
-
-auto data_sp = FileSystem::Instance().CreateDataBuffer(
-crash_file->GetPath(), header_size, 0);
+const FileSpec file_spec;
+crash_file_sp->GetFileSpec(const_cast(file_spec));

bulbazord wrote:

Here too

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

[Lldb-commits] [lldb] Add new API in SBTarget for loading core from SBFile (PR #71769)

[Alex Langford via lldb-commits]

@@ -67,30 +67,33 @@ class ProcessFreeBSDKernelKVM : public ProcessFreeBSDKernel 
{
 } // namespace
 
 ProcessFreeBSDKernel::ProcessFreeBSDKernel(lldb::TargetSP target_sp,
-   ListenerSP listener_sp)
-: PostMortemProcess(target_sp, listener_sp) {}
+   ListenerSP listener_sp,
+   lldb::FileSP crash_file_sp)
+: PostMortemProcess(target_sp, listener_sp, crash_file_sp) {}
 
 lldb::ProcessSP ProcessFreeBSDKernel::CreateInstance(lldb::TargetSP target_sp,
  ListenerSP listener_sp,
- const FileSpec 
*crash_file,
+ lldb::FileSP 
crash_file_sp,
  bool can_connect) {
   ModuleSP executable = target_sp->GetExecutableModule();
   if (crash_file && !can_connect && executable) {
+const FileSpec file_spec;
+crash_file_sp->GetFileSpec(const_cast(file_spec));

bulbazord wrote:

Same here as before, why are you making a const file spec and then const 
casting to fill it in?

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

[Lldb-commits] [lldb] Add new API in SBTarget for loading core from SBFile (PR #71769)

[Alex Langford via lldb-commits]

@@ -196,9 +196,10 @@ void ProcessGDBRemote::Terminate() {
 
 lldb::ProcessSP ProcessGDBRemote::CreateInstance(
 lldb::TargetSP target_sp, ListenerSP listener_sp,
-const FileSpec *crash_file_path, bool can_connect) {
+ lldb::FileSP crash_file_sp,
+ bool can_connect) {
   lldb::ProcessSP process_sp;
-  if (crash_file_path == nullptr)
+  if (crash_file_sp == nullptr)

bulbazord wrote:

You can also drop the `== nullptr` here too

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

[Lldb-commits] [lldb] Add new API in SBTarget for loading core from SBFile (PR #71769)

[Alex Langford via lldb-commits]

@@ -24,7 +26,20 @@ class PostMortemProcess : public Process {
   using Process::Process;
 
 public:
+  PostMortemProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
+lldb::FileSP file_sp)
+  : Process(target_sp, listener_sp), m_core_file(file_sp) {}
+
   bool IsLiveDebugSession() const override { return false; }
+
+  FileSpec GetCoreFile() const override {
+const FileSpec file_spec;
+m_core_file->GetFileSpec(const_cast(file_spec));
+return file_spec;

bulbazord wrote:

+1

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

[Lldb-commits] [lldb] Add new API in SBTarget for loading core from SBFile (PR #71769)

[Alex Langford via lldb-commits]

@@ -1469,6 +1468,14 @@ class Process : public 
std::enable_shared_from_this,
 
   virtual bool IsLiveDebugSession() const { return true; };
 
+
+  /// Provide a way to retrieve the core dump file that is loaded for 
debugging.
+  /// Only available if IsLiveDebugSession() returns true.

bulbazord wrote:

Shouldn't it only be available if `IsLiveDebugSession` is false? You're 
debugging a core dump which isn't really a "live debug session" since you're 
examining the corpse of a process, no?

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

[Lldb-commits] [lldb] [LLDB] Ignore actual-needed artificial members in DWARFASTParserClang::ParseSingleMember (PR #70779)

[Haojian Wu via lldb-commits]

hokein wrote:

Thank you very much for the review!

> Are you still going to make the change to CommandObjectFrame such that frame 
> var doesn't hide the artificial fields?

Yes, that's my next step.

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Jason Molenda via lldb-commits]

@@ -704,7 +705,37 @@ class Process : public 
std::enable_shared_from_this,
   /// is not supported by the plugin, error otherwise.
   virtual llvm::Expected SaveCore(llvm::StringRef outfile);
 
+  struct CoreFileMemoryRange {
+llvm::AddressRange range; /// The address range to save into the core file.
+uint32_t lldb_permissions; /// A bit set of lldb::Permissions bits.
+
+bool operator==(const CoreFileMemoryRange &rhs) const {
+  return range == rhs.range && lldb_permissions == rhs.lldb_permissions;
+}
+
+bool operator!=(const CoreFileMemoryRange &rhs) const {
+  return !(*this == rhs);
+}
+
+bool operator<(const CoreFileMemoryRange &rhs) const {
+  if (range < rhs.range)
+return true;
+  if (range == rhs.range)
+return lldb_permissions < rhs.lldb_permissions;
+  return false;
+}
+  };
+
+  using CoreFileMemoryRanges = std::vector;
+
+  /// Helper function for Process::SaveCore(...) that calculates the address
+  /// ranges that should be save. This allows all core file plug-ins to save

jasonmolenda wrote:

"should be saved"

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Jason Molenda via lldb-commits]

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

A couple of small nits but looks good to me.  I do have a little concern about 
pulling in `llvm/ADT/AddressRanges.h` in Process.h to get llvm::AddressRange 
when we have an lldb_private::AddressRange, which is used in a number of places 
in lldb.  I'm surprised you only needed to add `lldb_private::` namespace 
qualifications in that TraceDumper after adding the llvm header in Process.h.  
I don't have a good suggestion here but it seems like the kind of thing that's 
going to annoy someone once a year or so when they stumble on the ambiguity. :) 

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Jason Molenda via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,
+  Process::CoreFileMemoryRanges &ranges) {
+  const auto &dirty_page_list = region.GetDirtyPageList();
+  if (!dirty_page_list)
+return false;
+  const uint32_t lldb_permissions = region.GetLLDBPermissions();
+  const addr_t page_size = region.GetPageSize();
+  if (page_size == 0)
+return false;
+  llvm::AddressRange range(0, 0);
+  for (addr_t page_addr : *dirty_page_list) {
+if (range.empty()) {
+  // No range yet, initialize the range with the current dirty page.
+  range = llvm::AddressRange(page_addr, page_addr + page_size);
+} else {
+  if (range.end() == page_addr) {
+// Combine consective ranges.
+range = llvm::AddressRange(range.start(), page_addr + page_size);
+  } else {
+// Add previous contiguous range and init the new range with the
+// current dirty page.
+ranges.push_back({range, lldb_permissions});
+range = llvm::AddressRange(page_addr, page_addr + page_size);
+  }
+}
+  }
+  // The last range
+  if (!range.empty())
+ranges.push_back({range, lldb_permissions});
+  return true;
+}
+
+// Given a region, add the region to \a ranges.
+//
+// Only add the region if it isn't empty and if it has some permissions.
+// If \a try_dirty_pages is true, then try to add only the dirty pages for a
+// given region. If the region has dirty page information, only dirty pages
+// will be added to \a ranges, else the entire range will be added to \a
+// ranges.
+static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,
+  Process::CoreFileMemoryRanges &ranges) {
+  // Don't add empty ranges or ranges with no permissions.
+  if (region.GetRange().GetByteSize() == 0 || region.GetLLDBPermissions() == 0)
+return;
+  if (try_dirty_pages && AddDirtyPages(region, ranges))
+return;
+  ranges.push_back(CreateCoreFileMemoryRange(region));
+}
+
+// Save all memory regions that are not empty or have at least some permissions
+// for a full core file style.
+static Status
+GetCoreFileSaveRangesFull(Process &process,
+  const MemoryRegionInfos ®ions,
+  Process::CoreFileMemoryRanges &ranges) {
+
+  // Don't add only dirty pages, add full regions.
+  const bool try_dirty_pages = false;
+  for (const auto ®ion: regions)
+AddRegion(region, try_dirty_pages, ranges);
+  return Status();
+}
+
+// Save only the dirty pages to the core file. Make sure the process has at
+// least some dirty pages, as some OS versions don't support reporting what
+// pages are dirty within an memory region. If no memory regions have dirty
+// page information, return an error.
+static Status
+GetCoreFileSaveRangesDirtyOnly(Process &process,
+   const MemoryRegionInfos ®ions,
+   Process::CoreFileMemoryRanges &ranges) {
+  // Iterate over the regions and find all dirty pages.
+  bool have_dirty_page_info = false;
+  for (const auto ®ion: regions) {
+if (AddDirtyPages(region, ranges))
+  have_dirty_page_info = true;
+  }
+
+  if (!have_dirty_page_info)
+return Status("no process memory regions have dirty page information");
+
+  return Status();
+}
+
+// Save all thread stacks to the core file. Some OS versions support reporting
+// when a memory region is stack related. We check on this information, but we
+// also use the stack pointers of each thread and add those in case the OS
+// doesn't support reporting stack memory. This function does unique the stack
+// regions and won't add the same range twice. This function also attempts to
+// only emit dirty pages from the stack if the memory regions support reporting
+// dirty regions. If the process doesn't support dirty regions, then it will
+// fall back to adding the full stack region.
+static Status
+GetCoreFileSaveRangesStackOnly(Process &process,
+   const MemoryRegionInfos ®ions,
+   Process::CoreFileMemoryRanges &ranges) {
+  // Some platforms support annotating the region information that tell us that
+  // it comes from a thread stack. So look for those regions first.
+
+  // Keep t

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Jason Molenda via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,
+  Process::CoreFileMemoryRanges &ranges) {
+  const auto &dirty_page_list = region.GetDirtyPageList();
+  if (!dirty_page_list)
+return false;
+  const uint32_t lldb_permissions = region.GetLLDBPermissions();
+  const addr_t page_size = region.GetPageSize();
+  if (page_size == 0)
+return false;
+  llvm::AddressRange range(0, 0);
+  for (addr_t page_addr : *dirty_page_list) {
+if (range.empty()) {
+  // No range yet, initialize the range with the current dirty page.
+  range = llvm::AddressRange(page_addr, page_addr + page_size);
+} else {
+  if (range.end() == page_addr) {
+// Combine consective ranges.
+range = llvm::AddressRange(range.start(), page_addr + page_size);
+  } else {
+// Add previous contiguous range and init the new range with the
+// current dirty page.
+ranges.push_back({range, lldb_permissions});
+range = llvm::AddressRange(page_addr, page_addr + page_size);
+  }
+}
+  }
+  // The last range
+  if (!range.empty())
+ranges.push_back({range, lldb_permissions});
+  return true;
+}
+
+// Given a region, add the region to \a ranges.
+//
+// Only add the region if it isn't empty and if it has some permissions.
+// If \a try_dirty_pages is true, then try to add only the dirty pages for a
+// given region. If the region has dirty page information, only dirty pages
+// will be added to \a ranges, else the entire range will be added to \a
+// ranges.
+static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,
+  Process::CoreFileMemoryRanges &ranges) {
+  // Don't add empty ranges or ranges with no permissions.
+  if (region.GetRange().GetByteSize() == 0 || region.GetLLDBPermissions() == 0)
+return;
+  if (try_dirty_pages && AddDirtyPages(region, ranges))
+return;
+  ranges.push_back(CreateCoreFileMemoryRange(region));
+}
+
+// Save all memory regions that are not empty or have at least some permissions
+// for a full core file style.
+static Status
+GetCoreFileSaveRangesFull(Process &process,
+  const MemoryRegionInfos ®ions,
+  Process::CoreFileMemoryRanges &ranges) {
+
+  // Don't add only dirty pages, add full regions.
+  const bool try_dirty_pages = false;
+  for (const auto ®ion: regions)
+AddRegion(region, try_dirty_pages, ranges);
+  return Status();
+}
+
+// Save only the dirty pages to the core file. Make sure the process has at
+// least some dirty pages, as some OS versions don't support reporting what
+// pages are dirty within an memory region. If no memory regions have dirty
+// page information, return an error.
+static Status
+GetCoreFileSaveRangesDirtyOnly(Process &process,
+   const MemoryRegionInfos ®ions,
+   Process::CoreFileMemoryRanges &ranges) {
+  // Iterate over the regions and find all dirty pages.
+  bool have_dirty_page_info = false;
+  for (const auto ®ion: regions) {
+if (AddDirtyPages(region, ranges))
+  have_dirty_page_info = true;
+  }
+
+  if (!have_dirty_page_info)
+return Status("no process memory regions have dirty page information");
+
+  return Status();
+}
+
+// Save all thread stacks to the core file. Some OS versions support reporting
+// when a memory region is stack related. We check on this information, but we
+// also use the stack pointers of each thread and add those in case the OS
+// doesn't support reporting stack memory. This function does unique the stack
+// regions and won't add the same range twice. This function also attempts to
+// only emit dirty pages from the stack if the memory regions support reporting
+// dirty regions. If the process doesn't support dirty regions, then it will
+// fall back to adding the full stack region.
+static Status
+GetCoreFileSaveRangesStackOnly(Process &process,
+   const MemoryRegionInfos ®ions,
+   Process::CoreFileMemoryRanges &ranges) {
+  // Some platforms support annotating the region information that tell us that
+  // it comes from a thread stack. So look for those regions first.
+
+  // Keep t

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Jason Molenda via lldb-commits]

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

[Lldb-commits] [lldb] [lldb-dap] Add an option to show function args in stack frames (PR #71843)

[Walter Erquinigo via lldb-commits]

https://github.com/walter-erquinigo created 
https://github.com/llvm/llvm-project/pull/71843

When this option is enabled, display names of stack frames are generated using 
the `${function.name-with-args}` formatter instead of simply calling 
`SBFrame::GetDisplayFunctionName`. This makes lldb-dap show an output similar 
to the one in the CLI.

This option is disabled by default because of its performance cost. It's a good 
option for non-gigantic programs.


>From 233aade4eb058ad2eba04334b0bbd96f5307ac48 Mon Sep 17 00:00:00 2001
From: walter erquinigo 
Date: Thu, 9 Nov 2023 13:15:55 -0500
Subject: [PATCH] [lldb-dap] Add an option to show function args in stack
 frames

When this option is enabled, display names of stack frames are generated using 
the `${function.name-with-args}` formatter instead of simply calling 
`SBFrame::GetDisplayFunctionName`. This makes lldb-dap show an output similar 
to the one in the CLI.

This option is disabled by default because of its performance cost. It's a good 
option for non-gigantic programs.
---
 lldb/include/lldb/API/SBFrame.h   |  9 -
 lldb/include/lldb/Target/StackFrame.h | 18 -
 .../test/tools/lldb-dap/dap_server.py |  2 +
 .../test/tools/lldb-dap/lldbdap_testcase.py   |  4 ++
 lldb/source/API/SBFrame.cpp   | 38 ---
 lldb/source/Target/StackFrame.cpp | 30 +--
 .../lldb-dap/stackTrace/TestDAP_stackTrace.py | 23 +--
 lldb/tools/lldb-dap/DAP.h |  1 +
 lldb/tools/lldb-dap/JSONUtils.cpp | 17 ++---
 lldb/tools/lldb-dap/lldb-dap.cpp  |  4 ++
 lldb/tools/lldb-dap/package.json  | 10 +
 11 files changed, 130 insertions(+), 26 deletions(-)

diff --git a/lldb/include/lldb/API/SBFrame.h b/lldb/include/lldb/API/SBFrame.h
index 7c4477f9125d1cd..75e04d794baf848 100644
--- a/lldb/include/lldb/API/SBFrame.h
+++ b/lldb/include/lldb/API/SBFrame.h
@@ -87,8 +87,15 @@ class LLDB_API SBFrame {
   // display to a user
   const char *GetDisplayFunctionName();
 
+  /// Similar to \a GetDisplayFunctionName() but with function arguments and
+  /// their values inserted into the function display name whenever possible.
+  ///
+  /// \param[in] output
+  ///   The stream where the display name is written to.
+  void GetDisplayFunctionNameWithArgs(SBStream &output);
+
   const char *GetFunctionName() const;
-  
+
   // Return the frame function's language.  If there isn't a function, then
   // guess the language type from the mangled name.
   lldb::LanguageType GuessLanguage() const;
diff --git a/lldb/include/lldb/Target/StackFrame.h 
b/lldb/include/lldb/Target/StackFrame.h
index 6824d916030a024..7b8510dfb4b40c6 100644
--- a/lldb/include/lldb/Target/StackFrame.h
+++ b/lldb/include/lldb/Target/StackFrame.h
@@ -14,6 +14,7 @@
 
 #include "lldb/Utility/Flags.h"
 
+#include "lldb/Core/FormatEntity.h"
 #include "lldb/Core/ValueObjectList.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Target/ExecutionContextScope.h"
@@ -324,8 +325,23 @@ class StackFrame : public ExecutionContextScope,
   ///C string with the assembly instructions for this function.
   const char *Disassemble();
 
+  /// Print a description of this frame using the provided frame format.
+  /// If the format is invalid, then the default formatter will be used (see \a
+  /// StackFrame::Dump()), in which case \b false is returned. Otherwise, \b
+  /// true is returned.
+  ///
+  /// \param[in] strm
+  ///   The Stream to print the description to.
+  ///
+  /// \param[in] frame_marker
+  ///   Optional string that will be prepended to the frame output description.
+  bool DumpUsingFormat(Stream &strm,
+   const lldb_private::FormatEntity::Entry *format,
+   llvm::StringRef frame_marker = {});
+
   /// Print a description for this frame using the frame-format formatter
-  /// settings.
+  /// settings. If the current frame-format settings are invalid, then the
+  /// default formatter will be used (see \a StackFrame::Dump()).
   ///
   /// \param [in] strm
   ///   The Stream to print the description to.
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py 
b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
index d1fb478bc8bb9ee..0c305fdd1ad9bbe 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -732,6 +732,7 @@ def request_launch(
 enableAutoVariableSummaries=False,
 enableSyntheticChildDebugging=False,
 commandEscapePrefix="`",
+showFramesWithFunctionArgs=False,
 ):
 args_dict = {"program": program}
 if args:
@@ -773,6 +774,7 @@ def request_launch(
 args_dict["runInTerminal"] = runInTerminal
 if postRunCommands:
 args_dict["postRunCommands"] = postRunCommands
+args_dict["showFramesWithF

[Lldb-commits] [lldb] [lldb-dap] Add an option to show function args in stack frames (PR #71843)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Walter Erquinigo (walter-erquinigo)


Changes

When this option is enabled, display names of stack frames are generated using 
the `${function.name-with-args}` formatter instead of simply calling 
`SBFrame::GetDisplayFunctionName`. This makes lldb-dap show an output similar 
to the one in the CLI.

This option is disabled by default because of its performance cost. It's a good 
option for non-gigantic programs.


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


11 Files Affected:

- (modified) lldb/include/lldb/API/SBFrame.h (+8-1) 
- (modified) lldb/include/lldb/Target/StackFrame.h (+17-1) 
- (modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py 
(+2) 
- (modified) 
lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py (+4) 
- (modified) lldb/source/API/SBFrame.cpp (+33-5) 
- (modified) lldb/source/Target/StackFrame.cpp (+19-11) 
- (modified) lldb/test/API/tools/lldb-dap/stackTrace/TestDAP_stackTrace.py 
(+20-3) 
- (modified) lldb/tools/lldb-dap/DAP.h (+1) 
- (modified) lldb/tools/lldb-dap/JSONUtils.cpp (+12-5) 
- (modified) lldb/tools/lldb-dap/lldb-dap.cpp (+4) 
- (modified) lldb/tools/lldb-dap/package.json (+10) 


``diff
diff --git a/lldb/include/lldb/API/SBFrame.h b/lldb/include/lldb/API/SBFrame.h
index 7c4477f9125d1cd..75e04d794baf848 100644
--- a/lldb/include/lldb/API/SBFrame.h
+++ b/lldb/include/lldb/API/SBFrame.h
@@ -87,8 +87,15 @@ class LLDB_API SBFrame {
   // display to a user
   const char *GetDisplayFunctionName();
 
+  /// Similar to \a GetDisplayFunctionName() but with function arguments and
+  /// their values inserted into the function display name whenever possible.
+  ///
+  /// \param[in] output
+  ///   The stream where the display name is written to.
+  void GetDisplayFunctionNameWithArgs(SBStream &output);
+
   const char *GetFunctionName() const;
-  
+
   // Return the frame function's language.  If there isn't a function, then
   // guess the language type from the mangled name.
   lldb::LanguageType GuessLanguage() const;
diff --git a/lldb/include/lldb/Target/StackFrame.h 
b/lldb/include/lldb/Target/StackFrame.h
index 6824d916030a024..7b8510dfb4b40c6 100644
--- a/lldb/include/lldb/Target/StackFrame.h
+++ b/lldb/include/lldb/Target/StackFrame.h
@@ -14,6 +14,7 @@
 
 #include "lldb/Utility/Flags.h"
 
+#include "lldb/Core/FormatEntity.h"
 #include "lldb/Core/ValueObjectList.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Target/ExecutionContextScope.h"
@@ -324,8 +325,23 @@ class StackFrame : public ExecutionContextScope,
   ///C string with the assembly instructions for this function.
   const char *Disassemble();
 
+  /// Print a description of this frame using the provided frame format.
+  /// If the format is invalid, then the default formatter will be used (see \a
+  /// StackFrame::Dump()), in which case \b false is returned. Otherwise, \b
+  /// true is returned.
+  ///
+  /// \param[in] strm
+  ///   The Stream to print the description to.
+  ///
+  /// \param[in] frame_marker
+  ///   Optional string that will be prepended to the frame output description.
+  bool DumpUsingFormat(Stream &strm,
+   const lldb_private::FormatEntity::Entry *format,
+   llvm::StringRef frame_marker = {});
+
   /// Print a description for this frame using the frame-format formatter
-  /// settings.
+  /// settings. If the current frame-format settings are invalid, then the
+  /// default formatter will be used (see \a StackFrame::Dump()).
   ///
   /// \param [in] strm
   ///   The Stream to print the description to.
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py 
b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
index d1fb478bc8bb9ee..0c305fdd1ad9bbe 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -732,6 +732,7 @@ def request_launch(
 enableAutoVariableSummaries=False,
 enableSyntheticChildDebugging=False,
 commandEscapePrefix="`",
+showFramesWithFunctionArgs=False,
 ):
 args_dict = {"program": program}
 if args:
@@ -773,6 +774,7 @@ def request_launch(
 args_dict["runInTerminal"] = runInTerminal
 if postRunCommands:
 args_dict["postRunCommands"] = postRunCommands
+args_dict["showFramesWithFunctionArgs"] = showFramesWithFunctionArgs
 args_dict["enableAutoVariableSummaries"] = enableAutoVariableSummaries
 args_dict["enableSyntheticChildDebugging"] = 
enableSyntheticChildDebugging
 args_dict["commandEscapePrefix"] = commandEscapePrefix
diff --git 
a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py 
b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
index aa89ffe24c3e026..0775922500ea4a8 100644
--- a/lldb/packages/Python

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Alex Langford via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,

bulbazord wrote:

It seems like there are multiple reasons why we may return `false` but `false` 
itself doesn't really accurately capture what went wrong. Maybe we can return 
an `llvm::Error` here?

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Alex Langford via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,
+  Process::CoreFileMemoryRanges &ranges) {
+  const auto &dirty_page_list = region.GetDirtyPageList();
+  if (!dirty_page_list)
+return false;
+  const uint32_t lldb_permissions = region.GetLLDBPermissions();
+  const addr_t page_size = region.GetPageSize();
+  if (page_size == 0)
+return false;
+  llvm::AddressRange range(0, 0);
+  for (addr_t page_addr : *dirty_page_list) {
+if (range.empty()) {
+  // No range yet, initialize the range with the current dirty page.
+  range = llvm::AddressRange(page_addr, page_addr + page_size);
+} else {
+  if (range.end() == page_addr) {
+// Combine consective ranges.
+range = llvm::AddressRange(range.start(), page_addr + page_size);
+  } else {
+// Add previous contiguous range and init the new range with the
+// current dirty page.
+ranges.push_back({range, lldb_permissions});
+range = llvm::AddressRange(page_addr, page_addr + page_size);
+  }
+}
+  }
+  // The last range
+  if (!range.empty())
+ranges.push_back({range, lldb_permissions});
+  return true;
+}
+
+// Given a region, add the region to \a ranges.
+//
+// Only add the region if it isn't empty and if it has some permissions.
+// If \a try_dirty_pages is true, then try to add only the dirty pages for a
+// given region. If the region has dirty page information, only dirty pages
+// will be added to \a ranges, else the entire range will be added to \a
+// ranges.
+static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,
+  Process::CoreFileMemoryRanges &ranges) {
+  // Don't add empty ranges or ranges with no permissions.
+  if (region.GetRange().GetByteSize() == 0 || region.GetLLDBPermissions() == 0)
+return;
+  if (try_dirty_pages && AddDirtyPages(region, ranges))
+return;
+  ranges.push_back(CreateCoreFileMemoryRange(region));
+}
+
+// Save all memory regions that are not empty or have at least some permissions
+// for a full core file style.
+static Status
+GetCoreFileSaveRangesFull(Process &process,
+  const MemoryRegionInfos ®ions,
+  Process::CoreFileMemoryRanges &ranges) {
+
+  // Don't add only dirty pages, add full regions.
+  const bool try_dirty_pages = false;
+  for (const auto ®ion: regions)
+AddRegion(region, try_dirty_pages, ranges);
+  return Status();

bulbazord wrote:

Why does this function return a Status? Doesn't seem like it actually does much 
here?

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Alex Langford via lldb-commits]

@@ -704,7 +705,37 @@ class Process : public 
std::enable_shared_from_this,
   /// is not supported by the plugin, error otherwise.
   virtual llvm::Expected SaveCore(llvm::StringRef outfile);
 
+  struct CoreFileMemoryRange {
+llvm::AddressRange range; /// The address range to save into the core file.
+uint32_t lldb_permissions; /// A bit set of lldb::Permissions bits.
+
+bool operator==(const CoreFileMemoryRange &rhs) const {
+  return range == rhs.range && lldb_permissions == rhs.lldb_permissions;
+}
+
+bool operator!=(const CoreFileMemoryRange &rhs) const {
+  return !(*this == rhs);
+}
+
+bool operator<(const CoreFileMemoryRange &rhs) const {
+  if (range < rhs.range)
+return true;
+  if (range == rhs.range)
+return lldb_permissions < rhs.lldb_permissions;
+  return false;
+}
+  };
+
+  using CoreFileMemoryRanges = std::vector;
+
+  /// Helper function for Process::SaveCore(...) that calculates the address
+  /// ranges that should be save. This allows all core file plug-ins to save
+  /// consistent memory ranges given a \a core_style.
+  Status CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
+ CoreFileMemoryRanges &ranges);
+

bulbazord wrote:

What do you think of something like `llvm::Expected` 
instead? Status is kind of easy to ignore in terms of error handling and you 
won't need the out parameter.

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Alex Langford via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());

bulbazord wrote:

Do you think it'd be useful to be able to create an AddressRange from a 
MemoryRegionInfo object?

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Alex Langford via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,
+  Process::CoreFileMemoryRanges &ranges) {
+  const auto &dirty_page_list = region.GetDirtyPageList();
+  if (!dirty_page_list)
+return false;
+  const uint32_t lldb_permissions = region.GetLLDBPermissions();
+  const addr_t page_size = region.GetPageSize();
+  if (page_size == 0)
+return false;
+  llvm::AddressRange range(0, 0);
+  for (addr_t page_addr : *dirty_page_list) {
+if (range.empty()) {
+  // No range yet, initialize the range with the current dirty page.
+  range = llvm::AddressRange(page_addr, page_addr + page_size);
+} else {
+  if (range.end() == page_addr) {
+// Combine consective ranges.
+range = llvm::AddressRange(range.start(), page_addr + page_size);
+  } else {
+// Add previous contiguous range and init the new range with the
+// current dirty page.
+ranges.push_back({range, lldb_permissions});
+range = llvm::AddressRange(page_addr, page_addr + page_size);
+  }
+}
+  }
+  // The last range
+  if (!range.empty())
+ranges.push_back({range, lldb_permissions});
+  return true;
+}
+
+// Given a region, add the region to \a ranges.
+//
+// Only add the region if it isn't empty and if it has some permissions.
+// If \a try_dirty_pages is true, then try to add only the dirty pages for a
+// given region. If the region has dirty page information, only dirty pages
+// will be added to \a ranges, else the entire range will be added to \a
+// ranges.
+static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,
+  Process::CoreFileMemoryRanges &ranges) {

bulbazord wrote:

Seems like you'd want this class to return something? There seems to be 
multiple failure conditions here

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Alex Langford via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,
+  Process::CoreFileMemoryRanges &ranges) {
+  const auto &dirty_page_list = region.GetDirtyPageList();
+  if (!dirty_page_list)
+return false;
+  const uint32_t lldb_permissions = region.GetLLDBPermissions();
+  const addr_t page_size = region.GetPageSize();
+  if (page_size == 0)
+return false;
+  llvm::AddressRange range(0, 0);
+  for (addr_t page_addr : *dirty_page_list) {
+if (range.empty()) {
+  // No range yet, initialize the range with the current dirty page.
+  range = llvm::AddressRange(page_addr, page_addr + page_size);
+} else {
+  if (range.end() == page_addr) {
+// Combine consective ranges.

bulbazord wrote:

`consective` -> `consecutive`

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Alex Langford via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,
+  Process::CoreFileMemoryRanges &ranges) {
+  const auto &dirty_page_list = region.GetDirtyPageList();
+  if (!dirty_page_list)
+return false;
+  const uint32_t lldb_permissions = region.GetLLDBPermissions();
+  const addr_t page_size = region.GetPageSize();
+  if (page_size == 0)
+return false;
+  llvm::AddressRange range(0, 0);
+  for (addr_t page_addr : *dirty_page_list) {
+if (range.empty()) {
+  // No range yet, initialize the range with the current dirty page.
+  range = llvm::AddressRange(page_addr, page_addr + page_size);
+} else {
+  if (range.end() == page_addr) {
+// Combine consective ranges.
+range = llvm::AddressRange(range.start(), page_addr + page_size);
+  } else {
+// Add previous contiguous range and init the new range with the
+// current dirty page.
+ranges.push_back({range, lldb_permissions});
+range = llvm::AddressRange(page_addr, page_addr + page_size);
+  }
+}
+  }
+  // The last range
+  if (!range.empty())
+ranges.push_back({range, lldb_permissions});
+  return true;
+}
+
+// Given a region, add the region to \a ranges.
+//
+// Only add the region if it isn't empty and if it has some permissions.
+// If \a try_dirty_pages is true, then try to add only the dirty pages for a
+// given region. If the region has dirty page information, only dirty pages
+// will be added to \a ranges, else the entire range will be added to \a
+// ranges.
+static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,

bulbazord wrote:

I think `try_dirty_pages` is kind of a confusing name. The intent of the 
parameter is to only add dirty pages if `AddDirtyPages` succeeds right? What do 
you think about `dirty_pages_only`? Why might you want to try only dirty pages 
and do the regular thing only if it fails?

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Alex Langford via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,
+  Process::CoreFileMemoryRanges &ranges) {
+  const auto &dirty_page_list = region.GetDirtyPageList();
+  if (!dirty_page_list)
+return false;
+  const uint32_t lldb_permissions = region.GetLLDBPermissions();
+  const addr_t page_size = region.GetPageSize();
+  if (page_size == 0)
+return false;

bulbazord wrote:

Why might the page size be 0? If the MemoryRegionInfo is "invalid"?

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Alex Langford via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,
+  Process::CoreFileMemoryRanges &ranges) {
+  const auto &dirty_page_list = region.GetDirtyPageList();
+  if (!dirty_page_list)
+return false;
+  const uint32_t lldb_permissions = region.GetLLDBPermissions();
+  const addr_t page_size = region.GetPageSize();
+  if (page_size == 0)
+return false;
+  llvm::AddressRange range(0, 0);
+  for (addr_t page_addr : *dirty_page_list) {
+if (range.empty()) {
+  // No range yet, initialize the range with the current dirty page.
+  range = llvm::AddressRange(page_addr, page_addr + page_size);
+} else {
+  if (range.end() == page_addr) {
+// Combine consective ranges.
+range = llvm::AddressRange(range.start(), page_addr + page_size);
+  } else {
+// Add previous contiguous range and init the new range with the
+// current dirty page.
+ranges.push_back({range, lldb_permissions});
+range = llvm::AddressRange(page_addr, page_addr + page_size);
+  }
+}
+  }
+  // The last range
+  if (!range.empty())
+ranges.push_back({range, lldb_permissions});
+  return true;
+}
+
+// Given a region, add the region to \a ranges.
+//
+// Only add the region if it isn't empty and if it has some permissions.
+// If \a try_dirty_pages is true, then try to add only the dirty pages for a
+// given region. If the region has dirty page information, only dirty pages
+// will be added to \a ranges, else the entire range will be added to \a
+// ranges.
+static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,
+  Process::CoreFileMemoryRanges &ranges) {
+  // Don't add empty ranges or ranges with no permissions.
+  if (region.GetRange().GetByteSize() == 0 || region.GetLLDBPermissions() == 0)
+return;
+  if (try_dirty_pages && AddDirtyPages(region, ranges))
+return;
+  ranges.push_back(CreateCoreFileMemoryRange(region));
+}
+
+// Save all memory regions that are not empty or have at least some permissions
+// for a full core file style.
+static Status
+GetCoreFileSaveRangesFull(Process &process,
+  const MemoryRegionInfos ®ions,
+  Process::CoreFileMemoryRanges &ranges) {
+
+  // Don't add only dirty pages, add full regions.
+  const bool try_dirty_pages = false;
+  for (const auto ®ion: regions)
+AddRegion(region, try_dirty_pages, ranges);
+  return Status();
+}
+
+// Save only the dirty pages to the core file. Make sure the process has at
+// least some dirty pages, as some OS versions don't support reporting what
+// pages are dirty within an memory region. If no memory regions have dirty
+// page information, return an error.
+static Status
+GetCoreFileSaveRangesDirtyOnly(Process &process,
+   const MemoryRegionInfos ®ions,
+   Process::CoreFileMemoryRanges &ranges) {
+  // Iterate over the regions and find all dirty pages.
+  bool have_dirty_page_info = false;
+  for (const auto ®ion: regions) {
+if (AddDirtyPages(region, ranges))
+  have_dirty_page_info = true;
+  }
+
+  if (!have_dirty_page_info)
+return Status("no process memory regions have dirty page information");
+
+  return Status();
+}
+
+// Save all thread stacks to the core file. Some OS versions support reporting
+// when a memory region is stack related. We check on this information, but we
+// also use the stack pointers of each thread and add those in case the OS
+// doesn't support reporting stack memory. This function does unique the stack
+// regions and won't add the same range twice. This function also attempts to
+// only emit dirty pages from the stack if the memory regions support reporting
+// dirty regions. If the process doesn't support dirty regions, then it will
+// fall back to adding the full stack region.
+static Status
+GetCoreFileSaveRangesStackOnly(Process &process,
+   const MemoryRegionInfos ®ions,
+   Process::CoreFileMemoryRanges &ranges) {
+  // Some platforms support annotating the region information that tell us that
+  // it comes from a thread stack. So look for those regions first.
+
+  // Keep t

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Alex Langford via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,
+  Process::CoreFileMemoryRanges &ranges) {
+  const auto &dirty_page_list = region.GetDirtyPageList();
+  if (!dirty_page_list)
+return false;
+  const uint32_t lldb_permissions = region.GetLLDBPermissions();
+  const addr_t page_size = region.GetPageSize();
+  if (page_size == 0)
+return false;
+  llvm::AddressRange range(0, 0);
+  for (addr_t page_addr : *dirty_page_list) {
+if (range.empty()) {
+  // No range yet, initialize the range with the current dirty page.
+  range = llvm::AddressRange(page_addr, page_addr + page_size);
+} else {
+  if (range.end() == page_addr) {
+// Combine consective ranges.
+range = llvm::AddressRange(range.start(), page_addr + page_size);
+  } else {
+// Add previous contiguous range and init the new range with the
+// current dirty page.
+ranges.push_back({range, lldb_permissions});
+range = llvm::AddressRange(page_addr, page_addr + page_size);
+  }
+}
+  }
+  // The last range
+  if (!range.empty())
+ranges.push_back({range, lldb_permissions});
+  return true;
+}
+
+// Given a region, add the region to \a ranges.
+//
+// Only add the region if it isn't empty and if it has some permissions.
+// If \a try_dirty_pages is true, then try to add only the dirty pages for a
+// given region. If the region has dirty page information, only dirty pages
+// will be added to \a ranges, else the entire range will be added to \a
+// ranges.
+static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,
+  Process::CoreFileMemoryRanges &ranges) {
+  // Don't add empty ranges or ranges with no permissions.
+  if (region.GetRange().GetByteSize() == 0 || region.GetLLDBPermissions() == 0)
+return;
+  if (try_dirty_pages && AddDirtyPages(region, ranges))
+return;
+  ranges.push_back(CreateCoreFileMemoryRange(region));
+}
+
+// Save all memory regions that are not empty or have at least some permissions
+// for a full core file style.
+static Status
+GetCoreFileSaveRangesFull(Process &process,
+  const MemoryRegionInfos ®ions,
+  Process::CoreFileMemoryRanges &ranges) {
+
+  // Don't add only dirty pages, add full regions.
+  const bool try_dirty_pages = false;
+  for (const auto ®ion: regions)
+AddRegion(region, try_dirty_pages, ranges);
+  return Status();
+}
+
+// Save only the dirty pages to the core file. Make sure the process has at
+// least some dirty pages, as some OS versions don't support reporting what
+// pages are dirty within an memory region. If no memory regions have dirty
+// page information, return an error.
+static Status
+GetCoreFileSaveRangesDirtyOnly(Process &process,
+   const MemoryRegionInfos ®ions,
+   Process::CoreFileMemoryRanges &ranges) {
+  // Iterate over the regions and find all dirty pages.
+  bool have_dirty_page_info = false;
+  for (const auto ®ion: regions) {
+if (AddDirtyPages(region, ranges))
+  have_dirty_page_info = true;
+  }
+
+  if (!have_dirty_page_info)
+return Status("no process memory regions have dirty page information");
+
+  return Status();
+}
+
+// Save all thread stacks to the core file. Some OS versions support reporting
+// when a memory region is stack related. We check on this information, but we
+// also use the stack pointers of each thread and add those in case the OS
+// doesn't support reporting stack memory. This function does unique the stack
+// regions and won't add the same range twice. This function also attempts to

bulbazord wrote:

I think you can drop the part about uniquing stack regions and just say that it 
won't add the same range twice.

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Greg Clayton via lldb-commits]

@@ -704,7 +705,37 @@ class Process : public 
std::enable_shared_from_this,
   /// is not supported by the plugin, error otherwise.
   virtual llvm::Expected SaveCore(llvm::StringRef outfile);
 
+  struct CoreFileMemoryRange {
+llvm::AddressRange range; /// The address range to save into the core file.
+uint32_t lldb_permissions; /// A bit set of lldb::Permissions bits.
+
+bool operator==(const CoreFileMemoryRange &rhs) const {
+  return range == rhs.range && lldb_permissions == rhs.lldb_permissions;
+}
+
+bool operator!=(const CoreFileMemoryRange &rhs) const {
+  return !(*this == rhs);
+}
+
+bool operator<(const CoreFileMemoryRange &rhs) const {
+  if (range < rhs.range)
+return true;
+  if (range == rhs.range)
+return lldb_permissions < rhs.lldb_permissions;
+  return false;
+}
+  };
+
+  using CoreFileMemoryRanges = std::vector;
+
+  /// Helper function for Process::SaveCore(...) that calculates the address
+  /// ranges that should be save. This allows all core file plug-ins to save
+  /// consistent memory ranges given a \a core_style.
+  Status CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
+ CoreFileMemoryRanges &ranges);
+

clayborg wrote:

I did that initially but then it was always just converted to Status in the end 
so it seemed like using Expected when we didn't need to. I am happy to change 
to Expected<> if you really think it should be that way

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Greg Clayton via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,

clayborg wrote:

We aren't going to propagate the error, we just need to know if it succeeded or 
not, so I didn't have it return an error. Otherwise every single range from a 
linux lldb-server will return an error "dirty pages not supported". For Darwin 
we added special extra features to track dirty pages, but no one else has 
these. Again, I can add an error here, but I will just end up consuming the 
error because lack of dirty page information isn't going to stop a core file 
from being created. Also many regions on mac will not have any dirty pages if 
they don't have write permissions, so most regions would end up returning an 
error, which again, we won't do anything with, we will just ignore.

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Greg Clayton via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());

clayborg wrote:

This is an llvm::AddressRange here. We also have lldb_private::AddressRange 
which contains a lldb_private::Address (section offset address) + byte size. 
The llvm::AddressRange just has a start address and end address. I could add 
accessors to MemoryRegionInfo for these, but as Jason already eluded to we have 
llvm::AddressRange and lldb_private::AddressRange and if we include both header 
files in MemoryRegionInfo.h, we might end up with more qualifications needed on 
bare `AddressRange` types in source files.

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Greg Clayton via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,
+  Process::CoreFileMemoryRanges &ranges) {
+  const auto &dirty_page_list = region.GetDirtyPageList();
+  if (!dirty_page_list)
+return false;
+  const uint32_t lldb_permissions = region.GetLLDBPermissions();
+  const addr_t page_size = region.GetPageSize();
+  if (page_size == 0)
+return false;
+  llvm::AddressRange range(0, 0);
+  for (addr_t page_addr : *dirty_page_list) {
+if (range.empty()) {
+  // No range yet, initialize the range with the current dirty page.
+  range = llvm::AddressRange(page_addr, page_addr + page_size);
+} else {
+  if (range.end() == page_addr) {
+// Combine consective ranges.
+range = llvm::AddressRange(range.start(), page_addr + page_size);
+  } else {
+// Add previous contiguous range and init the new range with the
+// current dirty page.
+ranges.push_back({range, lldb_permissions});
+range = llvm::AddressRange(page_addr, page_addr + page_size);
+  }
+}
+  }
+  // The last range
+  if (!range.empty())
+ranges.push_back({range, lldb_permissions});
+  return true;
+}
+
+// Given a region, add the region to \a ranges.
+//
+// Only add the region if it isn't empty and if it has some permissions.
+// If \a try_dirty_pages is true, then try to add only the dirty pages for a
+// given region. If the region has dirty page information, only dirty pages
+// will be added to \a ranges, else the entire range will be added to \a
+// ranges.
+static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,

clayborg wrote:

`try_dirty_pages` is saying "if you can add only the dirty pages from the 
current region because the process plugin supports it, then please add those, 
if not please add the full region. If you note in the code below we only try to 
add dirty pages if we are asked to when `try_dirty_pages == true`

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Greg Clayton via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,
+  Process::CoreFileMemoryRanges &ranges) {
+  const auto &dirty_page_list = region.GetDirtyPageList();
+  if (!dirty_page_list)
+return false;
+  const uint32_t lldb_permissions = region.GetLLDBPermissions();
+  const addr_t page_size = region.GetPageSize();
+  if (page_size == 0)
+return false;
+  llvm::AddressRange range(0, 0);
+  for (addr_t page_addr : *dirty_page_list) {
+if (range.empty()) {
+  // No range yet, initialize the range with the current dirty page.
+  range = llvm::AddressRange(page_addr, page_addr + page_size);
+} else {
+  if (range.end() == page_addr) {
+// Combine consective ranges.
+range = llvm::AddressRange(range.start(), page_addr + page_size);
+  } else {
+// Add previous contiguous range and init the new range with the
+// current dirty page.
+ranges.push_back({range, lldb_permissions});
+range = llvm::AddressRange(page_addr, page_addr + page_size);
+  }
+}
+  }
+  // The last range
+  if (!range.empty())
+ranges.push_back({range, lldb_permissions});
+  return true;
+}
+
+// Given a region, add the region to \a ranges.
+//
+// Only add the region if it isn't empty and if it has some permissions.
+// If \a try_dirty_pages is true, then try to add only the dirty pages for a
+// given region. If the region has dirty page information, only dirty pages
+// will be added to \a ranges, else the entire range will be added to \a
+// ranges.
+static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,
+  Process::CoreFileMemoryRanges &ranges) {

clayborg wrote:

If we have empty ranges or a memory region without permissions, we don't need 
to add it to the core file, and this function will determine if it needs to add 
the region to the list of regions to save to the core file. The user won't care 
or need to know about any of the error conditions.

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Greg Clayton via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,
+  Process::CoreFileMemoryRanges &ranges) {
+  const auto &dirty_page_list = region.GetDirtyPageList();
+  if (!dirty_page_list)
+return false;
+  const uint32_t lldb_permissions = region.GetLLDBPermissions();
+  const addr_t page_size = region.GetPageSize();
+  if (page_size == 0)
+return false;
+  llvm::AddressRange range(0, 0);
+  for (addr_t page_addr : *dirty_page_list) {
+if (range.empty()) {
+  // No range yet, initialize the range with the current dirty page.
+  range = llvm::AddressRange(page_addr, page_addr + page_size);
+} else {
+  if (range.end() == page_addr) {
+// Combine consective ranges.
+range = llvm::AddressRange(range.start(), page_addr + page_size);
+  } else {
+// Add previous contiguous range and init the new range with the
+// current dirty page.
+ranges.push_back({range, lldb_permissions});
+range = llvm::AddressRange(page_addr, page_addr + page_size);
+  }
+}
+  }
+  // The last range
+  if (!range.empty())
+ranges.push_back({range, lldb_permissions});
+  return true;
+}
+
+// Given a region, add the region to \a ranges.
+//
+// Only add the region if it isn't empty and if it has some permissions.
+// If \a try_dirty_pages is true, then try to add only the dirty pages for a
+// given region. If the region has dirty page information, only dirty pages
+// will be added to \a ranges, else the entire range will be added to \a
+// ranges.
+static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,
+  Process::CoreFileMemoryRanges &ranges) {
+  // Don't add empty ranges or ranges with no permissions.
+  if (region.GetRange().GetByteSize() == 0 || region.GetLLDBPermissions() == 0)
+return;
+  if (try_dirty_pages && AddDirtyPages(region, ranges))
+return;
+  ranges.push_back(CreateCoreFileMemoryRange(region));
+}
+
+// Save all memory regions that are not empty or have at least some permissions
+// for a full core file style.
+static Status
+GetCoreFileSaveRangesFull(Process &process,
+  const MemoryRegionInfos ®ions,
+  Process::CoreFileMemoryRanges &ranges) {
+
+  // Don't add only dirty pages, add full regions.
+  const bool try_dirty_pages = false;
+  for (const auto ®ion: regions)
+AddRegion(region, try_dirty_pages, ranges);
+  return Status();

clayborg wrote:

Each of the `GetCoreFileSaveRanges*` funtions all have the same return value to 
keep it consistent. When we ask explicitly for dirty pages only with the 
`eSaveCoreDirtyOnly` core style, that function returns an error if dirty pages 
are not supported by any regions. Not much can go wrong in this function. I can 
remove the Status return value if needed.

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Greg Clayton via lldb-commits]

@@ -6252,3 +6243,188 @@ Status Process::WriteMemoryTags(lldb::addr_t addr, 
size_t len,
   return DoWriteMemoryTags(addr, len, tag_manager->GetAllocationTagType(),
*packed_tags);
 }
+
+// Create a CoreFileMemoryRange from a MemoryRegionInfo
+static Process::CoreFileMemoryRange
+CreateCoreFileMemoryRange(const MemoryRegionInfo ®ion) {
+  const addr_t addr = region.GetRange().GetRangeBase();
+  llvm::AddressRange range(addr, addr + region.GetRange().GetByteSize());
+  return {range, region.GetLLDBPermissions()};
+}
+
+// Add dirty pages to the core file ranges and return true if dirty pages
+// were added. Return false if the dirty page information is not valid or in
+// the region.
+static bool AddDirtyPages(const MemoryRegionInfo ®ion,
+  Process::CoreFileMemoryRanges &ranges) {
+  const auto &dirty_page_list = region.GetDirtyPageList();
+  if (!dirty_page_list)
+return false;
+  const uint32_t lldb_permissions = region.GetLLDBPermissions();
+  const addr_t page_size = region.GetPageSize();
+  if (page_size == 0)
+return false;
+  llvm::AddressRange range(0, 0);
+  for (addr_t page_addr : *dirty_page_list) {
+if (range.empty()) {
+  // No range yet, initialize the range with the current dirty page.
+  range = llvm::AddressRange(page_addr, page_addr + page_size);
+} else {
+  if (range.end() == page_addr) {
+// Combine consective ranges.
+range = llvm::AddressRange(range.start(), page_addr + page_size);
+  } else {
+// Add previous contiguous range and init the new range with the
+// current dirty page.
+ranges.push_back({range, lldb_permissions});
+range = llvm::AddressRange(page_addr, page_addr + page_size);
+  }
+}
+  }
+  // The last range
+  if (!range.empty())
+ranges.push_back({range, lldb_permissions});
+  return true;
+}
+
+// Given a region, add the region to \a ranges.
+//
+// Only add the region if it isn't empty and if it has some permissions.
+// If \a try_dirty_pages is true, then try to add only the dirty pages for a
+// given region. If the region has dirty page information, only dirty pages
+// will be added to \a ranges, else the entire range will be added to \a
+// ranges.
+static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,
+  Process::CoreFileMemoryRanges &ranges) {
+  // Don't add empty ranges or ranges with no permissions.
+  if (region.GetRange().GetByteSize() == 0 || region.GetLLDBPermissions() == 0)
+return;
+  if (try_dirty_pages && AddDirtyPages(region, ranges))
+return;
+  ranges.push_back(CreateCoreFileMemoryRange(region));
+}
+
+// Save all memory regions that are not empty or have at least some permissions
+// for a full core file style.
+static Status
+GetCoreFileSaveRangesFull(Process &process,
+  const MemoryRegionInfos ®ions,
+  Process::CoreFileMemoryRanges &ranges) {
+
+  // Don't add only dirty pages, add full regions.
+  const bool try_dirty_pages = false;
+  for (const auto ®ion: regions)
+AddRegion(region, try_dirty_pages, ranges);
+  return Status();
+}
+
+// Save only the dirty pages to the core file. Make sure the process has at
+// least some dirty pages, as some OS versions don't support reporting what
+// pages are dirty within an memory region. If no memory regions have dirty
+// page information, return an error.
+static Status
+GetCoreFileSaveRangesDirtyOnly(Process &process,
+   const MemoryRegionInfos ®ions,
+   Process::CoreFileMemoryRanges &ranges) {
+  // Iterate over the regions and find all dirty pages.
+  bool have_dirty_page_info = false;
+  for (const auto ®ion: regions) {
+if (AddDirtyPages(region, ranges))
+  have_dirty_page_info = true;
+  }
+
+  if (!have_dirty_page_info)
+return Status("no process memory regions have dirty page information");
+
+  return Status();
+}
+
+// Save all thread stacks to the core file. Some OS versions support reporting
+// when a memory region is stack related. We check on this information, but we
+// also use the stack pointers of each thread and add those in case the OS
+// doesn't support reporting stack memory. This function does unique the stack
+// regions and won't add the same range twice. This function also attempts to

clayborg wrote:

sounds good


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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Greg Clayton via lldb-commits]

https://github.com/clayborg updated 
https://github.com/llvm/llvm-project/pull/71772

>From feea395f4ef165dfc057dfdc0649c6948895eeb3 Mon Sep 17 00:00:00 2001
From: Greg Clayton 
Date: Wed, 8 Nov 2023 21:14:49 -0800
Subject: [PATCH] Centralize the code that figures out which memory ranges to
 save into core files.

Prior to this patch, each core file plugin (ObjectFileMachO.cpp and 
ObjectFileMinindump.cpp) would calculate the address ranges to save in 
different ways. This patch adds a new function to Process.h/.cpp:

```
Status Process::CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style, 
CoreFileMemoryRanges &ranges);
```

The patch updates the ObjectFileMachO::SaveCore(...) and 
ObjectFileMinindump::SaveCore(...) to use same code. This will allow core files 
to be consistent with the lldb::SaveCoreStyle across different core file 
creators and will allow us to add new core file saving features that do more 
complex things in future patches.
---
 lldb/include/lldb/Target/MemoryRegionInfo.h   |   8 +-
 lldb/include/lldb/Target/Process.h|  46 -
 .../ObjectFile/Mach-O/ObjectFileMachO.cpp | 137 +++--
 .../Minidump/MinidumpFileBuilder.cpp  |  37 +---
 .../ObjectFile/Minidump/MinidumpFileBuilder.h |   3 +-
 .../Minidump/ObjectFileMinidump.cpp   |   9 +-
 lldb/source/Target/Process.cpp| 189 +-
 lldb/source/Target/TraceDumper.cpp|   7 +-
 8 files changed, 267 insertions(+), 169 deletions(-)

diff --git a/lldb/include/lldb/Target/MemoryRegionInfo.h 
b/lldb/include/lldb/Target/MemoryRegionInfo.h
index 47d4c9d6398728c..66a4b3ed1b42d5f 100644
--- a/lldb/include/lldb/Target/MemoryRegionInfo.h
+++ b/lldb/include/lldb/Target/MemoryRegionInfo.h
@@ -81,11 +81,11 @@ class MemoryRegionInfo {
   // lldb::Permissions
   uint32_t GetLLDBPermissions() const {
 uint32_t permissions = 0;
-if (m_read)
+if (m_read == eYes)
   permissions |= lldb::ePermissionsReadable;
-if (m_write)
+if (m_write == eYes)
   permissions |= lldb::ePermissionsWritable;
-if (m_execute)
+if (m_execute == eYes)
   permissions |= lldb::ePermissionsExecutable;
 return permissions;
   }
@@ -151,7 +151,7 @@ class MemoryRegionInfo {
   int m_pagesize = 0;
   std::optional> m_dirty_pages;
 };
-  
+
 inline bool operator<(const MemoryRegionInfo &lhs,
   const MemoryRegionInfo &rhs) {
   return lhs.GetRange() < rhs.GetRange();
diff --git a/lldb/include/lldb/Target/Process.h 
b/lldb/include/lldb/Target/Process.h
index a6d3e6c2d16926e..08e3c60f7c324e6 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -54,6 +54,7 @@
 #include "lldb/Utility/UserIDResolver.h"
 #include "lldb/lldb-private.h"
 
+#include "llvm/ADT/AddressRanges.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/VersionTuple.h"
@@ -354,12 +355,10 @@ class Process : public 
std::enable_shared_from_this,
   };
   // This is all the event bits the public process broadcaster broadcasts.
   // The process shadow listener signs up for all these bits...
-  static constexpr int g_all_event_bits = eBroadcastBitStateChanged 
-| eBroadcastBitInterrupt
-| eBroadcastBitSTDOUT 
-| eBroadcastBitSTDERR
-| eBroadcastBitProfileData 
-| eBroadcastBitStructuredData;
+  static constexpr int g_all_event_bits =
+  eBroadcastBitStateChanged | eBroadcastBitInterrupt | eBroadcastBitSTDOUT 
|
+  eBroadcastBitSTDERR | eBroadcastBitProfileData |
+  eBroadcastBitStructuredData;
 
   enum {
 eBroadcastInternalStateControlStop = (1 << 0),
@@ -390,7 +389,7 @@ class Process : public 
std::enable_shared_from_this,
   ConstString &GetBroadcasterClass() const override {
 return GetStaticBroadcasterClass();
   }
-  
+
 /// A notification structure that can be used by clients to listen
 /// for changes in a process's lifetime.
 ///
@@ -579,7 +578,7 @@ class Process : public 
std::enable_shared_from_this,
   /// of CommandObject like CommandObjectRaw, CommandObjectParsed,
   /// or CommandObjectMultiword.
   virtual CommandObject *GetPluginCommandObject() { return nullptr; }
-  
+
   /// The underlying plugin might store the low-level communication history for
   /// this session.  Dump it into the provided stream.
   virtual void DumpPluginHistory(Stream &s) { return; }
@@ -614,7 +613,7 @@ class Process : public 
std::enable_shared_from_this,
 return error;
   }
 
-  /// The "ShadowListener" for a process is just an ordinary Listener that 
+  /// The "ShadowListener" for a process is just an ordinary Listener that
   /// listens for all the Process event bits.  It's convenient because you can
   /// specify it in the LaunchInfo or AttachInfo, so it will get events from
 

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Greg Clayton via lldb-commits]

clayborg wrote:

I addressed most of the feedback. Alex let me know if you still really want 
llvm::Error and llvm::Expected to be used as I can add that if you think it is 
required. I also ran clang format.

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Greg Clayton via lldb-commits]

clayborg wrote:

@jasonmolenda I was wondering if we should modify 
`GetCoreFileSaveRangesDirtyOnly(...)` to try and add all dirty pages and see if 
any regions have the dirty page info, but if no memory region infos have the 
dirty pages information, then fall back to adding all memory regions with 
`write` access. What do you think? This would allow systems to not support 
dirty pages, but get a more minimal core file saved out. The other option is to 
add a new `lldb::SaveCoreStyle` enum like 
`lldb::SaveCoreStyle::eSaveCoreWriteOnly` to save only sections that have write 
permissions.

Right now we have `eSaveCoreFull` which saves all memory regions that have any 
valid permissions, `eSaveCoreDirtyOnly` which will currently only save dirty 
pages if it is supported by the process plugin (ProcessGDBRemove for most 
people, where `debugserver` supports the page stuff, but `lldb-server` 
doesn't), and `eSaveCoreStackOnly` which only emits the stacks (where it will 
use the dirty page info if available and fall back to saving the entire stack 
range if dirty page support is not available).

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Greg Clayton via lldb-commits]

clayborg wrote:

@bulbazord let me know if you require any changes after reading my inline 
comments.

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

[Lldb-commits] [lldb] [lldb-dap] Add an option to show function args in stack frames (PR #71843)

[Greg Clayton via lldb-commits]

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

[Lldb-commits] [lldb] [lldb-dap] Add an option to show function args in stack frames (PR #71843)

[Greg Clayton via lldb-commits]

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

So this hardcodes the frame display to only be the function name, or function 
name with args. Do we want to allow users to specify a frame format in the 
lldb-vscode settings? We could have two settings: one to enable the feature 
with something like `showFramesWithCustomFormat` and on that contains the 
format string like `customFrameFormat` which would default to 
`"${function.name-with-args}"`. Then users have complete control over how and 
what gets displayed. Some people might want to show the pc value and then the 
function name like:
```
  "customFrameFormat": "${frame.pc} ${function.name-with-args}"
```
If we want this then we will want to modify the SBFrame changes to be something 
like:
```
 SBError SBFrame::ApplyFormat(SBStream &output, const char *format_string);
```
We might even consider makding a `lldb::SBFormat` class that allows us to 
compile a format string into an object and then use it. If we do this then the 
API above will be:
```
SBError SBFrame::ApplyFormat(SBStream &output, lldb::SBFormat format);
```

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

[Lldb-commits] [lldb] [lldb-dap] Add an option to show function args in stack frames (PR #71843)

[Greg Clayton via lldb-commits]

@@ -1232,6 +1234,32 @@ const char *SBFrame::GetFunctionName() const {
   return name;
 }
 
+void SBFrame::GetDisplayFunctionNameWithArgs(SBStream &output) {
+  Stream &strm = output.ref();
+
+  std::unique_lock lock;
+  ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
+
+  StackFrame *frame = nullptr;
+  Target *target = exe_ctx.GetTargetPtr();
+  Process *process = exe_ctx.GetProcessPtr();
+
+  if (target && process) {
+Process::StopLocker stop_locker;
+if (stop_locker.TryLock(&process->GetRunLock())) {
+  frame = exe_ctx.GetFramePtr();
+  if (frame) {
+FormatEntity::Entry format;

clayborg wrote:

Make this static and add a llvm::once call around the intiailization so we 
don't need to parse the format each time we call this function. If we make this 
static, rename to `g_format` to indicate it is a global.

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

[Lldb-commits] [lldb] Colorize output when searching for symbols in lldb (PR #69422)

[via lldb-commits]

=?utf-8?q?José?= L. Junior 
Message-ID:
In-Reply-To: 


taalhaataahir0102 wrote:

Hi David!
`ninja check-lldb` is failing for us. We've re-compiled the latest llvm project 
(without adding our implementation) but that one is also giving us the same 
error:
![image](https://github.com/llvm/llvm-project/assets/77788288/7a53677f-2438-4bb6-b4fa-10d6680de054)

```
Testing Time: 88.62s

Total Discovered Tests: 2888
  Skipped  :1 (0.03%)
  Unsupported  :  491 (17.00%)
  Passed   : 1674 (57.96%)
  Expectedly Failed:   19 (0.66%)
  Unresolved   :  656 (22.71%)
  Failed   :   47 (1.63%)

2 warning(s) in tests
FAILED: tools/lldb/test/CMakeFiles/check-lldb 
/home/talha/llvm-latest/llvm-project-main/build/tools/lldb/test/CMakeFiles/check-lldb
 
cd /home/talha/llvm-latest/llvm-project-main/build/tools/lldb/test && 
/usr/bin/python3.10 
/home/talha/llvm-latest/llvm-project-main/build/./bin/llvm-lit -sv 
/home/talha/llvm-latest/llvm-project-main/build/tools/lldb/test
ninja: build stopped: subcommand failed.
```

We've used the following command for building lldb:
`cmake -G Ninja -DLLVM_ENABLE_PROJECTS="lldb;clang" -DLLVM_USE_LINKER=lld 
-DLLVM_BUILD_TESTS=true -DCMAKE_BUILD_TYPE=Release 
/home/talha/llvm-latest/llvm-project-main/llvm`

Can you please confirm if there's something wrong in our build. Also I've the 
complete log of ninja `check-lldb` but it's pretty long. Do you need it to 
figure out the issue?

Plus there's one more thing we are not sure of as we are new to github. There 
have been some updates in a file we've changed in our draft 
(`source/Commands/CommandObjectTarget.cpp`). So do we need a fresh build and 
add our implementation there before the final review? As the draft also says 
that

> This branch has conflicts that must be resolved

> Conflicting files
> lldb/source/Commands/CommandObjectTarget.cpp

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

[Lldb-commits] [lldb] [lldb][AArch64][Linux] Add field information for the mte_ctrl register (PR #71808)

[Alex Langford via lldb-commits]

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

LGTM

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

[Lldb-commits] [lldb] [lldb] Read Checksum from DWARF line tables (PR #71458)

[via lldb-commits]

dyung wrote:

Hi @JDevlieghere, this change seems to be causing 59 test failures in the 
cross-project-test suite. Can you take a look?

https://lab.llvm.org/buildbot/#/builders/217/builds/30979
https://lab.llvm.org/buildbot/#/builders/243/builds/15142

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

[Lldb-commits] [lldb] [LLDB] Ensure the data of apple accelerator tables is kept around (PR #71828)

[Alex Langford via lldb-commits]

https://github.com/bulbazord commented:

The idea of this makes sense to me, but I think you should let @felipepiovezan 
take a look first.

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

[Lldb-commits] [lldb] [lldb] Read Checksum from DWARF line tables (PR #71458)

[Jonas Devlieghere via lldb-commits]

JDevlieghere wrote:

> Hi @JDevlieghere, this change seems to be causing 59 test failures in the 
> cross-project-test suite. Can you take a look?
> 
> https://lab.llvm.org/buildbot/#/builders/217/builds/30979 
> https://lab.llvm.org/buildbot/#/builders/243/builds/15142

I'll revert the change, but I doubt this is the cause as this patch should just 
be storing a member. However there's nothing in the logs to exonerate me so 
let's see if the revert changes anything. 

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

[Lldb-commits] [lldb] Revert "[lldb] Read Checksum from DWARF line tables" (PR #71864)

[Jonas Devlieghere via lldb-commits]

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

Reverts llvm/llvm-project#71458 as it might have caused cross-project-test 
failures. 

>From 5b740aafd3631fbe4bbe56cbe9e206da926a3c06 Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere 
Date: Thu, 9 Nov 2023 12:42:28 -0800
Subject: [PATCH] Revert "[lldb] Read Checksum from DWARF line tables (#71458)"

This reverts commit 5da98dec7ab8c3adc3f70147ea666428431adf34.
---
 lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | 9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 79d44bce3d663b6..aabd83a194932ff 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -229,15 +229,8 @@ ParseSupportFilesFromPrologue(const lldb::ModuleSP &module,
 remapped_file = std::move(*file_path);
 }
 
-Checksum checksum;
-if (prologue.ContentTypes.HasMD5) {
-  const llvm::DWARFDebugLine::FileNameEntry &file_name_entry =
-  prologue.getFileNameEntry(idx);
-  checksum = file_name_entry.Checksum;
-}
-
 // Unconditionally add an entry, so the indices match up.
-support_files.EmplaceBack(remapped_file, style, checksum);
+support_files.EmplaceBack(remapped_file, style);
   }
 
   return support_files;

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

[Lldb-commits] [lldb] 73519ba - Revert "[lldb] Read Checksum from DWARF line tables" (#71864)

[via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-11-09T12:43:53-08:00
New Revision: 73519ba27a29c7e95ec93ed8ce44ff7874851599

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

LOG: Revert "[lldb] Read Checksum from DWARF line tables" (#71864)

Reverts llvm/llvm-project#71458 as it might have caused
cross-project-test failures.

Added: 


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

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 79d44bce3d663b6..aabd83a194932ff 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -229,15 +229,8 @@ ParseSupportFilesFromPrologue(const lldb::ModuleSP &module,
 remapped_file = std::move(*file_path);
 }
 
-Checksum checksum;
-if (prologue.ContentTypes.HasMD5) {
-  const llvm::DWARFDebugLine::FileNameEntry &file_name_entry =
-  prologue.getFileNameEntry(idx);
-  checksum = file_name_entry.Checksum;
-}
-
 // Unconditionally add an entry, so the indices match up.
-support_files.EmplaceBack(remapped_file, style, checksum);
+support_files.EmplaceBack(remapped_file, style);
   }
 
   return support_files;



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

[Lldb-commits] [lldb] Revert "[lldb] Read Checksum from DWARF line tables" (PR #71864)

[Jonas Devlieghere via lldb-commits]

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

[Lldb-commits] [lldb] [lldb] Read Checksum from DWARF line tables (PR #71458)

[Jonas Devlieghere via lldb-commits]

JDevlieghere wrote:

Reverted in #71864.

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

[Lldb-commits] [lldb] [lldb] Read Checksum from DWARF line tables (PR #71458)

[via lldb-commits]

dyung wrote:

> > Hi @JDevlieghere, this change seems to be causing 59 test failures in the 
> > cross-project-test suite. Can you take a look?
> > https://lab.llvm.org/buildbot/#/builders/217/builds/30979 
> > https://lab.llvm.org/buildbot/#/builders/243/builds/15142
> 
> I'll revert the change, but I doubt this is the cause as this patch should 
> just be storing a member. However there's nothing in the logs to exonerate me 
> so let's see if the revert changes anything.

I did try building the commit immediately before yours, and then yours and that 
is how I determined it to be caused by your commit.

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

[Lldb-commits] [lldb] Revert "[lldb] Read Checksum from DWARF line tables" (PR #71864)

[via lldb-commits]

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)


Changes

Reverts llvm/llvm-project#71458 as it might have caused 
cross-project-test failures. 

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


1 Files Affected:

- (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+1-8) 


``diff
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 79d44bce3d663b6..aabd83a194932ff 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -229,15 +229,8 @@ ParseSupportFilesFromPrologue(const lldb::ModuleSP &module,
 remapped_file = std::move(*file_path);
 }
 
-Checksum checksum;
-if (prologue.ContentTypes.HasMD5) {
-  const llvm::DWARFDebugLine::FileNameEntry &file_name_entry =
-  prologue.getFileNameEntry(idx);
-  checksum = file_name_entry.Checksum;
-}
-
 // Unconditionally add an entry, so the indices match up.
-support_files.EmplaceBack(remapped_file, style, checksum);
+support_files.EmplaceBack(remapped_file, style);
   }
 
   return support_files;

``




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

[Lldb-commits] [lldb] [lldb] Read Checksum from DWARF line tables (PR #71458)

[via lldb-commits]

dyung wrote:

> Reverted in #71864.

Thanks, they seem to be passing now:
https://lab.llvm.org/buildbot/#/builders/243/builds/15146
https://lab.llvm.org/buildbot/#/builders/217/builds/30989

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

[Lldb-commits] [lldb] [lldb] Read Checksum from DWARF line tables (PR #71458)

[Jonas Devlieghere via lldb-commits]

JDevlieghere wrote:

It was also passing before with the change, so the issue isn't fully 
deterministic: https://lab.llvm.org/buildbot/#/builders/217/builds/30986 

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Alex Langford via lldb-commits]

bulbazord wrote:

> I addressed most of the feedback. Alex let me know if you still really want 
> llvm::Error and llvm::Expected to be used as I can add that if you think it 
> is required. I also ran clang format.

I think your answers make sense to me. I don't think you need to add them here 
since it all eventually turns into Status objects anyway... I generally prefer 
it over Status though. Thanks for checking. 👍 

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

[Lldb-commits] [lldb] [lldb] Read Checksum from DWARF line tables (PR #71458)

[via lldb-commits]

dyung wrote:

> It was also passing before with the change, so the issue isn't fully 
> deterministic: https://lab.llvm.org/buildbot/#/builders/217/builds/30986

That was me forcing the build of the commit immediately preceding yours to 
verify it was passing:

`reason
A build was forced by '': Build a particular revision `

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

[Lldb-commits] [lldb] Centralize the code that figures out which memory ranges to save into core files (PR #71772)

[Jason Molenda via lldb-commits]

jasonmolenda wrote:

> @jasonmolenda I was wondering if we should modify 
> `GetCoreFileSaveRangesDirtyOnly(...)` to try and add all dirty pages and see 
> if any regions have the dirty page info, but if no memory region infos have 
> the dirty pages information, then fall back to adding all memory regions with 
> `write` access. What do you think? 

Another good idea.  Do we get page permissions like that on Linux?  I'm curious 
what system it would work on.

I tried a full memory coredump on darwin and there ARE some regions that are 
marked `r` or `rx` and yet have a dirty page.  Some of them are tagged as 
malloc-metadata so maybe the page protections were flipped to avoid buggy 
overwriting by the process or something, I didn't look too closely into them, 
it was only a dozen pages of memory total.  So at least on darwin, if 
debugserver didn't provide the dirty-pages list for each memory region, it'd 
probably be indistinguishable by developers who are mostly wanting to get 
stack+heap with these things.

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