[Lldb-commits] [PATCH] D101563: [lldb] [test] Extend aarch64-gp-read test to cover all registers

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added inline comments.



Comment at: lldb/test/Shell/Register/Inputs/aarch64-gp-read.cpp:57
+"ldr  x0,   [%0, #248]\n\t"
+"stp  x29, x0,  [sp, #-16]\n\t"
+"\n\t"

I'm not quite sure what this bit actually tests, with regards to inspecting 
registers - as it loads a value to a simple register and writes it somewhere 
else. I guess the thing it tests is expressions involving `$sp` though?

I guess that's fine but it'd be nice to have it spelled out a bit clearer about 
the aspect that it actually tests.



Comment at: lldb/test/Shell/Register/Inputs/aarch64-gp-read.cpp:67
+"ldp  x16, x17, [%0, #128]\n\t"
+"ldp  x18, x19, [%0, #144]\n\t"
+"ldp  x20, x21, [%0, #160]\n\t"

You're not allowed to clobber x18 on windows (and on darwin, but it's less 
fatal there).

Building this for windows gives this warning:
```
:1:1: warning: inline asm clobber list contains reserved registers: 
X18
ld1  {v0.2d,  v1.2d,  v2.2d,  v3.2d},  [x8], #64

:1:1: note: Reserved registers on the clobber list may not be 
preserved across the asm statement, and clobbering them may lead to undefined 
behaviour.
ld1  {v0.2d,  v1.2d,  v2.2d,  v3.2d},  [x8], #64

```
And after overwriting x18, things fail when we try to stop for the breakpoint.


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

https://reviews.llvm.org/D101563

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

[Lldb-commits] [lldb] 69a3269 - Support AArch64 PAC elf-core register read

[Muhammad Omair Javaid via lldb-commits]

Author: Muhammad Omair Javaid
Date: 2021-05-03T16:04:47+05:00
New Revision: 69a3269250715e31ec49be94065c1c8787a3d305

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

LOG: Support AArch64 PAC elf-core register read

This adds support for reading AArch64 Pointer Authentication regset
from elf-core file. Also includes a test-case for the same. Furthermore
there is also a slight refactoring of RegisterContextPOSIXCore_arm64
members and constructor. linux-aarch64-pac.core file is generated using
lldb/test/API/functionalities/postmortem/elf-core/main.c with following
clang arguments:
-march=armv8.5-a -mbranch-protection=pac-ret+leaf -nostdlib -static -g

Reviewed By: DavidSpickett

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

Added: 
lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-pac.core

Modified: 
lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py

Removed: 




diff  --git 
a/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp 
b/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
index 06b200e19f1e2..c4e7eda1e5d69 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
+++ b/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
@@ -43,6 +43,10 @@ bool RegisterContextPOSIX_arm64::IsSVE(unsigned reg) const {
   return m_register_info_up->IsSVEReg(reg);
 }
 
+bool RegisterContextPOSIX_arm64::IsPAuth(unsigned reg) const {
+  return m_register_info_up->IsPAuthReg(reg);
+}
+
 RegisterContextPOSIX_arm64::RegisterContextPOSIX_arm64(
 lldb_private::Thread &thread,
 std::unique_ptr register_info)

diff  --git a/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h 
b/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
index a3f07bb2823b1..7c301599d3af6 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
+++ b/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
@@ -54,6 +54,7 @@ class RegisterContextPOSIX_arm64 : public 
lldb_private::RegisterContext {
   size_t GetFPUSize() { return sizeof(RegisterInfoPOSIX_arm64::FPU); }
 
   bool IsSVE(unsigned reg) const;
+  bool IsPAuth(unsigned reg) const;
 
   bool IsSVEZ(unsigned reg) const { return m_register_info_up->IsSVEZReg(reg); 
}
   bool IsSVEP(unsigned reg) const { return m_register_info_up->IsSVEPReg(reg); 
}

diff  --git a/lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h 
b/lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
index df6e44faefbfb..ba873ba4436ba 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
+++ b/lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
@@ -109,6 +109,7 @@ class RegisterInfoPOSIX_arm64
   }
 
   bool IsSVEEnabled() const { return m_opt_regsets.AnySet(eRegsetMaskSVE); }
+  bool IsPAuthEnabled() const { return m_opt_regsets.AnySet(eRegsetMaskPAuth); 
}
 
   bool IsSVEReg(unsigned reg) const;
   bool IsSVEZReg(unsigned reg) const;

diff  --git 
a/lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp 
b/lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
index 9379316d1042d..34fd16da38885 100644
--- a/lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
+++ b/lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
@@ -21,32 +21,43 @@ std::unique_ptr
 RegisterContextCorePOSIX_arm64::Create(Thread &thread, const ArchSpec &arch,
const DataExtractor &gpregset,
llvm::ArrayRef notes) {
-  DataExtractor sveregset =
-  getRegset(notes, arch.GetTriple(), AARCH64_SVE_Desc);
-
   Flags opt_regsets = RegisterInfoPOSIX_arm64::eRegsetMaskDefault;
-  if (sveregset.GetByteSize() > sizeof(sve::user_sve_header))
+
+  DataExtractor sve_data = getRegset(notes, arch.GetTriple(), 
AARCH64_SVE_Desc);
+  if (sve_data.GetByteSize() > sizeof(sve::user_sve_header))
 opt_regsets.Set(RegisterInfoPOSIX_arm64::eRegsetMaskSVE);
+
+  // Pointer Authentication register set data is based on struct
+  // user_pac_mask declared in ptrace.h. See reference implementation
+  // in Linux kernel source at arch/arm64/include/uapi/asm/ptrace.h.
+  DataExtractor pac_data = getRegset(notes, arch.GetTriple(), 
AARCH64_PAC_Desc);
+  if (pac_data.GetByteSize

[Lldb-commits] [PATCH] D99941: [LLDB] Support AArch64 PAC elf-core register read

[Muhammad Omair Javaid via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG69a326925071: Support AArch64 PAC elf-core register read 
(authored by omjavaid).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99941

Files:
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-pac.core

Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -443,6 +443,21 @@
 
 self.expect("register read --all")
 
+@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64_pac_regs(self):
+# Test AArch64/Linux Pointer Authenication register read
+target = self.dbg.CreateTarget(None)
+self.assertTrue(target, VALID_TARGET)
+process = target.LoadCore("linux-aarch64-pac.core")
+
+values = {"data_mask": "0x007f000", "code_mask": "0x007f000"}
+
+for regname, value in values.items():
+self.expect("register read {}".format(regname),
+substrs=["{} = {}".format(regname, value)])
+
+self.expect("register read --all")
+
 @skipIfLLVMTargetMissing("ARM")
 def test_arm_core(self):
 # check 32 bit ARM core file
Index: lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
===
--- lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
+++ lldb/source/Plugins/Process/elf-core/RegisterUtilities.h
@@ -111,6 +111,10 @@
 {llvm::Triple::Linux, llvm::Triple::aarch64, llvm::ELF::NT_ARM_SVE},
 };
 
+constexpr RegsetDesc AARCH64_PAC_Desc[] = {
+{llvm::Triple::Linux, llvm::Triple::aarch64, llvm::ELF::NT_ARM_PAC_MASK},
+};
+
 constexpr RegsetDesc PPC_VMX_Desc[] = {
 {llvm::Triple::FreeBSD, llvm::Triple::UnknownArch, llvm::ELF::NT_PPC_VMX},
 {llvm::Triple::Linux, llvm::Triple::UnknownArch, llvm::ELF::NT_PPC_VMX},
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
===
--- lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
+++ lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
@@ -42,7 +42,6 @@
   lldb_private::Thread &thread,
   std::unique_ptr register_info,
   const lldb_private::DataExtractor &gpregset,
-  const lldb_private::DataExtractor &sveregset,
   llvm::ArrayRef notes);
 
   bool ReadGPR() override;
@@ -54,10 +53,10 @@
   bool WriteFPR() override;
 
 private:
-  lldb::DataBufferSP m_gpr_buffer;
-  lldb_private::DataExtractor m_gpr;
-  lldb_private::DataExtractor m_fpregset;
-  lldb_private::DataExtractor m_sveregset;
+  lldb_private::DataExtractor m_gpr_data;
+  lldb_private::DataExtractor m_fpr_data;
+  lldb_private::DataExtractor m_sve_data;
+  lldb_private::DataExtractor m_pac_data;
 
   SVEState m_sve_state;
   uint16_t m_sve_vector_length = 0;
Index: lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
===
--- lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
+++ lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
@@ -21,32 +21,43 @@
 RegisterContextCorePOSIX_arm64::Create(Thread &thread, const ArchSpec &arch,
const DataExtractor &gpregset,
llvm::ArrayRef notes) {
-  DataExtractor sveregset =
-  getRegset(notes, arch.GetTriple(), AARCH64_SVE_Desc);
-
   Flags opt_regsets = RegisterInfoPOSIX_arm64::eRegsetMaskDefault;
-  if (sveregset.GetByteSize() > sizeof(sve::user_sve_header))
+
+  DataExtractor sve_data = getRegset(notes, arch.GetTriple(), AARCH64_SVE_Desc);
+  if (sve_data.GetByteSize() > sizeof(sve::user_sve_header))
 opt_regsets.Set(RegisterInfoPOSIX_arm64::eRegsetMaskSVE);
+
+  // Pointer Authentication register set data is based on struct
+  // user_pac_mask declared in ptrace.h. See reference implementation
+  // in Linux kernel source at arch/arm64/include/uapi/asm/ptrace.h.
+  DataExtractor pac_data = getRegset(notes, arch.GetTriple(), AARCH64_PAC_Desc);
+  if (pac_data.GetBy

[Lldb-commits] [PATCH] D101563: [lldb] [test] Extend aarch64-gp-read test to cover all registers

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.



Comment at: lldb/test/Shell/Register/Inputs/aarch64-gp-read.cpp:57
+"ldr  x0,   [%0, #248]\n\t"
+"stp  x29, x0,  [sp, #-16]\n\t"
+"\n\t"

mstorsjo wrote:
> I'm not quite sure what this bit actually tests, with regards to inspecting 
> registers - as it loads a value to a simple register and writes it somewhere 
> else. I guess the thing it tests is expressions involving `$sp` though?
> 
> I guess that's fine but it'd be nice to have it spelled out a bit clearer 
> about the aspect that it actually tests.
It backs up `x29` and stores another test value on the stack to test `$sp` 
below.



Comment at: lldb/test/Shell/Register/Inputs/aarch64-gp-read.cpp:67
+"ldp  x16, x17, [%0, #128]\n\t"
+"ldp  x18, x19, [%0, #144]\n\t"
+"ldp  x20, x21, [%0, #160]\n\t"

mstorsjo wrote:
> You're not allowed to clobber x18 on windows (and on darwin, but it's less 
> fatal there).
> 
> Building this for windows gives this warning:
> ```
> :1:1: warning: inline asm clobber list contains reserved 
> registers: X18
> ld1  {v0.2d,  v1.2d,  v2.2d,  v3.2d},  [x8], #64
> 
> :1:1: note: Reserved registers on the clobber list may not be 
> preserved across the asm statement, and clobbering them may lead to undefined 
> behaviour.
> ld1  {v0.2d,  v1.2d,  v2.2d,  v3.2d},  [x8], #64
> 
> ```
> And after overwriting x18, things fail when we try to stop for the breakpoint.
Would backing it up on the stack and restoring after the trap work or is it 
entirely forbidden?


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

https://reviews.llvm.org/D101563

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

[Lldb-commits] [PATCH] D101563: [lldb] [test] Extend aarch64-gp-read test to cover all registers

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added inline comments.



Comment at: lldb/test/Shell/Register/Inputs/aarch64-gp-read.cpp:57
+"ldr  x0,   [%0, #248]\n\t"
+"stp  x29, x0,  [sp, #-16]\n\t"
+"\n\t"

mgorny wrote:
> mstorsjo wrote:
> > I'm not quite sure what this bit actually tests, with regards to inspecting 
> > registers - as it loads a value to a simple register and writes it 
> > somewhere else. I guess the thing it tests is expressions involving `$sp` 
> > though?
> > 
> > I guess that's fine but it'd be nice to have it spelled out a bit clearer 
> > about the aspect that it actually tests.
> It backs up `x29` and stores another test value on the stack to test `$sp` 
> below.
Sure. Just clarify that this corresponds with the read of an expression 
relative to `$sp` in the test script.



Comment at: lldb/test/Shell/Register/Inputs/aarch64-gp-read.cpp:67
+"ldp  x16, x17, [%0, #128]\n\t"
+"ldp  x18, x19, [%0, #144]\n\t"
+"ldp  x20, x21, [%0, #160]\n\t"

mgorny wrote:
> mstorsjo wrote:
> > You're not allowed to clobber x18 on windows (and on darwin, but it's less 
> > fatal there).
> > 
> > Building this for windows gives this warning:
> > ```
> > :1:1: warning: inline asm clobber list contains reserved 
> > registers: X18
> > ld1  {v0.2d,  v1.2d,  v2.2d,  v3.2d},  [x8], #64
> > 
> > :1:1: note: Reserved registers on the clobber list may not be 
> > preserved across the asm statement, and clobbering them may lead to 
> > undefined behaviour.
> > ld1  {v0.2d,  v1.2d,  v2.2d,  v3.2d},  [x8], #64
> > 
> > ```
> > And after overwriting x18, things fail when we try to stop for the 
> > breakpoint.
> Would backing it up on the stack and restoring after the trap work or is it 
> entirely forbidden?
It's pretty much forbidden - the problem here seems to be that having x18 set 
to an incorrect value breaks things when hitting the trap. In a straight-line 
code segment with no external interaction you can have x18 set to something 
else intermittently, but it must be correct whenever you interact with 
something else, and traps seem to be a case where it's read by something, 
somewhere, too.



Comment at: lldb/test/Shell/Register/Inputs/aarch64-gp-read.cpp:79
+// restore x29, discard the other value
+"ldp  x29, xzr, [sp, #-16]\n\t"
 :

This could just as well be a `ldr x29, [sp, #-16]` right?


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

https://reviews.llvm.org/D101563

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

[Lldb-commits] [PATCH] D99944: [LLDB] AArch64 Linux and elf-core PAC stack unwinder support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 342357.
omjavaid added a comment.

This merges separate patch D99947  for Linux 
into this one. Linux specific changes are no longer required as we have moved 
mask calculation into ABISysV_arm64 class.


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

https://reviews.llvm.org/D99944

Files:
  lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
  lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-pac.out
  lldb/test/API/functionalities/unwind/aarch64_unwind_pac/Makefile
  
lldb/test/API/functionalities/unwind/aarch64_unwind_pac/TestAArch64UnwindPAC.py
  lldb/test/API/functionalities/unwind/aarch64_unwind_pac/main.c

Index: lldb/test/API/functionalities/unwind/aarch64_unwind_pac/main.c
===
--- /dev/null
+++ lldb/test/API/functionalities/unwind/aarch64_unwind_pac/main.c
@@ -0,0 +1,24 @@
+// This program makes a multi tier nested function call to test AArch64
+// Pointer Authentication feature.
+
+// To enable PAC return address signing compile with following clang arguments:
+// -march=armv8.5-a -mbranch-protection=pac-ret+leaf
+
+#include 
+
+static void __attribute__((noinline)) func_c(void) {
+  exit(0); // Frame func_c
+}
+
+static void __attribute__((noinline)) func_b(void) {
+  func_c(); // Frame func_b
+}
+
+static void __attribute__((noinline)) func_a(void) {
+  func_b(); // Frame func_a
+}
+
+int main(int argc, char *argv[]) {
+  func_a(); // Frame main
+  return 0;
+}
Index: lldb/test/API/functionalities/unwind/aarch64_unwind_pac/TestAArch64UnwindPAC.py
===
--- /dev/null
+++ lldb/test/API/functionalities/unwind/aarch64_unwind_pac/TestAArch64UnwindPAC.py
@@ -0,0 +1,44 @@
+"""
+Test that we can backtrace correctly when AArch64 PAC is enabled
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AArch64UnwindPAC(TestBase):
+mydir = TestBase.compute_mydir(__file__)
+
+@skipIf(archs=no_match(["aarch64"]))
+@skipIf(oslist=no_match(['linux']))
+def test(self):
+"""Test that we can backtrace correctly when AArch64 PAC is enabled"""
+self.build()
+
+self.line = line_number('main.c', '// Frame func_c')
+
+exe = self.getBuildArtifact("a.out")
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(
+self, "main.c", self.line, num_expected_locations=1)
+self.runCmd("run", RUN_SUCCEEDED)
+self.expect("thread backtrace", STOPPED_DUE_TO_BREAKPOINT,
+substrs=["stop reason = breakpoint 1."])
+
+target = self.dbg.GetSelectedTarget()
+process = target.GetProcess()
+thread = process.GetThreadAtIndex(0)
+
+backtrace = ["func_c", "func_b", "func_a", "main", "__libc_start_main", "_start"]
+self.assertEqual(thread.GetNumFrames(), len(backtrace))
+for frame_idx, frame in enumerate(thread.frames):
+frame = thread.GetFrameAtIndex(frame_idx)
+self.assertTrue(frame)
+self.assertEqual(frame.GetFunctionName(), backtrace[frame_idx])
+			# Check line number for functions in main.c
+if (frame_idx < 4):
+self.assertEqual(frame.GetLineEntry().GetLine(),
+ line_number("main.c", "Frame " + backtrace[frame_idx]))
Index: lldb/test/API/functionalities/unwind/aarch64_unwind_pac/Makefile
===
--- /dev/null
+++ lldb/test/API/functionalities/unwind/aarch64_unwind_pac/Makefile
@@ -0,0 +1,5 @@
+C_SOURCES := main.c
+
+CFLAGS := -g -Os -march=armv8.5-a -mbranch-protection=pac-ret+leaf
+
+include Makefile.rules
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -20,6 +20,7 @@
 mydir = TestBase.compute_mydir(__file__)
 
 _aarch64_pid = 37688
+_aarch64_pac_pid = 387
 _i386_pid = 32306
 _x86_64_pid = 32259
 _s390x_pid = 1045
@@ -257,6 +258,18 @@
 
 self.dbg.DeleteTarget(target)
 
+@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64_pac(self):
+"""Test that lldb can unwind stack for AArch64 elf core file with PAC enabled."""
+
+target = self.dbg.CreateTarget("linux-aarch64-pac.out")
+self.assertTrue(target, VALID_TARGET)
+process = target.LoadCore("linux-aarch64-pac.core")
+
+self.check_all(process, self._aarch64_pac_pid, self._aarch64_regions, "a.out")
+

[Lldb-commits] [PATCH] D101128: [lldb-vscode] only report long running progress events

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 342386.
wallace marked 12 inline comments as done.
wallace added a comment.

Address all comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101128

Files:
  lldb/tools/lldb-vscode/ProgressEvent.cpp
  lldb/tools/lldb-vscode/ProgressEvent.h
  lldb/tools/lldb-vscode/VSCode.h

Index: lldb/tools/lldb-vscode/VSCode.h
===
--- lldb/tools/lldb-vscode/VSCode.h
+++ lldb/tools/lldb-vscode/VSCode.h
@@ -125,10 +125,6 @@
   int64_t GetLineForPC(int64_t sourceReference, lldb::addr_t pc) const;
   ExceptionBreakpoint *GetExceptionBreakpoint(const std::string &filter);
   ExceptionBreakpoint *GetExceptionBreakpoint(const lldb::break_id_t bp_id);
-  // Send the JSON in "json_str" to the "out" stream. Correctly send the
-  // "Content-Length:" field followed by the length, followed by the raw
-  // JSON bytes.
-  void SendJSON(const std::string &json_str);
 
   // Serialize the JSON value into a string and send the JSON packet to
   // the "out" stream.
@@ -208,6 +204,12 @@
   /// The callback to execute when the given request is triggered by the
   /// IDE.
   void RegisterRequestCallback(std::string request, RequestCallback callback);
+
+private:
+  // Send the JSON in "json_str" to the "out" stream. Correctly send the
+  // "Content-Length:" field followed by the length, followed by the raw
+  // JSON bytes.
+  void SendJSON(const std::string &json_str);
 };
 
 extern VSCode g_vsc;
Index: lldb/tools/lldb-vscode/ProgressEvent.h
===
--- lldb/tools/lldb-vscode/ProgressEvent.h
+++ lldb/tools/lldb-vscode/ProgressEvent.h
@@ -6,6 +6,10 @@
 //
 //===--===//
 
+#include 
+#include 
+#include 
+
 #include "VSCodeForward.h"
 
 #include "llvm/Support/JSON.h"
@@ -28,35 +32,108 @@
 
   llvm::json::Value ToJSON() const;
 
-  /// This operator returns \b true if two event messages
-  /// would result in the same event for the IDE, e.g.
-  /// same rounded percentage.
-  bool operator==(const ProgressEvent &other) const;
+  /// \return
+  ///   \b true if two event messages would result in the same event for the
+  ///   IDE, e.g. same rounded percentage.
+  bool EqualsForIDE(const ProgressEvent &other) const;
 
   const char *GetEventName() const;
 
+  ProgressEventType GetEventType() const;
+
   bool IsValid() const;
 
   uint64_t GetID() const;
 
+  void MarkAsReported();
+
+  bool Reported() const;
+
+  std::chrono::duration GetTimestamp() const;
+
 private:
   uint64_t m_progress_id;
-  const char *m_message;
+  std::string m_message;
   ProgressEventType m_event_type;
   llvm::Optional m_percentage;
+  std::chrono::duration m_timestamp;
+  bool m_reported;
+};
+
+using ProgressEventReportCallback = std::function;
+
+/// Class that keeps the start event and its most recent update.
+/// It controls when the event should start being reported to the IDE.
+class ProgressEventManager {
+public:
+  ProgressEventManager(const ProgressEvent &start_event,
+   ProgressEventReportCallback report_callback);
+
+  /// Report the start event and the most recent update if the event has lasted
+  /// for long enough.
+  ///
+  /// \return
+  /// \b false if the event hasn't finished and hasn't reported anything
+  /// yet.
+  bool TryReport();
+
+  /// Receive a new progress event for the start event and try to report it if
+  /// appropriate.
+  void Update(const ProgressEvent &event);
+
+  /// \return
+  /// \b true if a \a progressEnd event has been notified. There's no
+  /// need to try to report manually an event that has finished.
+  bool Finished() const;
+
+  /// \return
+  ///  The ID of the underlying start event.
+  uint64_t GetID() const;
+
+private:
+  ProgressEvent m_start_event;
+  llvm::Optional m_last_update_event;
+  std::chrono::duration m_start_event_timestamp;
+  bool m_finished;
+  ProgressEventReportCallback m_report_callback;
 };
 
+using ProgressEventManagerSP = std::shared_ptr;
+
 /// Class that filters out progress event messages that shouldn't be reported
-/// to the IDE, either because they are invalid or because they are too chatty.
+/// to the IDE, because they are invalid, they carry no new information, or they
+/// don't last long enough.
+///
+/// We need to limit the amount of events that are sent to the IDE, as they slow
+/// the render thread of the UI user, and they end up spamming the DAP
+/// connection, which also takes some processing time out of the IDE.
 class ProgressEventFilterQueue {
 public:
-  ProgressEventFilterQueue(std::function callback);
+  /// \param[in] report_callback
+  /// Function to invoke to report the event to the IDE.
+  ProgressEventFilterQueue(ProgressEventReportCallback report_callback);
+
+  ~ProgressEven

[Lldb-commits] [PATCH] D100740: [trace] Dedup different source lines when dumping instructions + refactor

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 342448.
wallace marked 10 inline comments as done.
wallace added a comment.

Address all comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100740

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Target/Trace.cpp
  lldb/test/API/commands/trace/TestTraceStartStop.py

Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -100,7 +100,6 @@
   a.out`main \+ 11 at main.cpp:4
 \[1\] {ADDRESS_REGEX}movl .*
 \[2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-  a.out`main \+ 28 at main.cpp:4
 \[3\] {ADDRESS_REGEX}cmpl .*
 \[4\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
 
Index: lldb/source/Target/Trace.cpp
===
--- lldb/source/Target/Trace.cpp
+++ lldb/source/Target/Trace.cpp
@@ -101,189 +101,262 @@
   return num == 0 ? 1 : static_cast(log10(num)) + 1;
 }
 
-/// Dump the symbol context of the given instruction address if it's different
-/// from the symbol context of the previous instruction in the trace.
-///
-/// \param[in] prev_sc
-/// The symbol context of the previous instruction in the trace.
-///
-/// \param[in] address
-/// The address whose symbol information will be dumped.
-///
 /// \return
-/// The symbol context of the current address, which might differ from the
-/// previous one.
-static SymbolContext DumpSymbolContext(Stream &s, const SymbolContext &prev_sc,
-   Target &target, const Address &address) {
-  AddressRange range;
-  if (prev_sc.GetAddressRange(eSymbolContextEverything, 0,
-  /*inline_block_range*/ false, range) &&
-  range.ContainsFileAddress(address))
-return prev_sc;
+/// \b true if the provided line entries match line, column and source file.
+/// This function assumes that the line entries are valid.
+static bool FileLineAndColumnMatches(const LineEntry &a, const LineEntry &b) {
+  if (a.line != b.line)
+return false;
+  if (a.column != b.column)
+return false;
+
+  return FileSpec::Compare(a.file, b.file, true) == 0;
+}
+
+// This custom LineEntry validator is neded because some line_entries have
+// 0 as line, which is meaningless. Notice that LineEntry::IsValid only
+// checks that line is not LLDB_INVALID_LINE_NUMBER, i.e. UINT32_MAX.
+static bool IsLineEntryValid(const LineEntry &line_entry) {
+  return line_entry.IsValid() && line_entry.line > 0;
+}
 
+/// Helper structure for \a TraverseInstructionsWithSymbolInfo.
+struct InstructionSymbolInfo {
   SymbolContext sc;
-  address.CalculateSymbolContext(&sc, eSymbolContextEverything);
+  Address address;
+  lldb::addr_t load_address;
+  lldb::DisassemblerSP disassembler;
+  lldb::InstructionSP instruction;
+  lldb_private::ExecutionContext exe_ctx;
+};
 
-  if (!prev_sc.module_sp && !sc.module_sp)
-return sc;
-  if (prev_sc.module_sp == sc.module_sp && !sc.function && !sc.symbol &&
-  !prev_sc.function && !prev_sc.symbol)
+/// InstructionSymbolInfo object with symbol information for the given
+/// instruction, calculated efficiently.
+///
+/// \param[in] symbol_scope
+/// If not \b 0, then the \a InstructionSymbolInfo will have its
+/// SymbolContext calculated up to that level.
+///
+/// \param[in] include_disassembler
+/// If \b true, then the \a InstructionSymbolInfo will have the
+/// \a disassembler and \a instruction objects calculated.
+static void TraverseInstructionsWithSymbolInfo(
+Trace &trace, Thread &thread, size_t position,
+Trace::TraceDirection direction, SymbolContextItem symbol_scope,
+bool include_disassembler,
+std::function insn)>
+callback) {
+  InstructionSymbolInfo prev_insn;
+
+  Target &target = thread.GetProcess()->GetTarget();
+  ExecutionContext exe_ctx;
+  target.CalculateExecutionContext(exe_ctx);
+  const ArchSpec &arch = target.GetArchitecture();
+
+  // Find the symbol context for the given address reusing the previous
+  // instruction's symbol context when possible.
+  auto calculate_symbol_context = [&](const Address &address) {
+AddressRange range;
+if (prev_insn.sc.GetAddressRange(symbol_scope, 0,
+ /*inline_block_range*/ false, range) &&
+range.ContainsFileAddress(address))
+  return prev_insn.sc;
+
+SymbolContext sc;
+address.CalculateSymbolContext(&sc, symbol_scope);
 return sc;
+  };
 
-  s.Printf("  ");
+  // Find the disassembler for the given address reusing the previous
+  // instruction's disassembler when possible.
+  auto calculate_disass = 

[Lldb-commits] [PATCH] D100740: [trace] Dedup different source lines when dumping instructions + refactor

[walter erquinigo via Phabricator via lldb-commits]

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

nit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100740

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Target/Trace.cpp
  lldb/test/API/commands/trace/TestTraceStartStop.py

Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -100,7 +100,6 @@
   a.out`main \+ 11 at main.cpp:4
 \[1\] {ADDRESS_REGEX}movl .*
 \[2\] {ADDRESS_REGEX}jmp  .* ; <\+28> at main.cpp:4
-  a.out`main \+ 28 at main.cpp:4
 \[3\] {ADDRESS_REGEX}cmpl .*
 \[4\] {ADDRESS_REGEX}jle  .* ; <\+20> at main.cpp:5'''])
 
Index: lldb/source/Target/Trace.cpp
===
--- lldb/source/Target/Trace.cpp
+++ lldb/source/Target/Trace.cpp
@@ -13,6 +13,7 @@
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Symbol/Function.h"
+#include "lldb/Target/ExecutionContext.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Thread.h"
@@ -101,189 +102,262 @@
   return num == 0 ? 1 : static_cast(log10(num)) + 1;
 }
 
-/// Dump the symbol context of the given instruction address if it's different
-/// from the symbol context of the previous instruction in the trace.
-///
-/// \param[in] prev_sc
-/// The symbol context of the previous instruction in the trace.
-///
-/// \param[in] address
-/// The address whose symbol information will be dumped.
-///
 /// \return
-/// The symbol context of the current address, which might differ from the
-/// previous one.
-static SymbolContext DumpSymbolContext(Stream &s, const SymbolContext &prev_sc,
-   Target &target, const Address &address) {
-  AddressRange range;
-  if (prev_sc.GetAddressRange(eSymbolContextEverything, 0,
-  /*inline_block_range*/ false, range) &&
-  range.ContainsFileAddress(address))
-return prev_sc;
+/// \b true if the provided line entries match line, column and source file.
+/// This function assumes that the line entries are valid.
+static bool FileLineAndColumnMatches(const LineEntry &a, const LineEntry &b) {
+  if (a.line != b.line)
+return false;
+  if (a.column != b.column)
+return false;
+
+  return FileSpec::Compare(a.file, b.file, true) == 0;
+}
+
+// This custom LineEntry validator is neded because some line_entries have
+// 0 as line, which is meaningless. Notice that LineEntry::IsValid only
+// checks that line is not LLDB_INVALID_LINE_NUMBER, i.e. UINT32_MAX.
+static bool IsLineEntryValid(const LineEntry &line_entry) {
+  return line_entry.IsValid() && line_entry.line > 0;
+}
 
+/// Helper structure for \a TraverseInstructionsWithSymbolInfo.
+struct InstructionSymbolInfo {
   SymbolContext sc;
-  address.CalculateSymbolContext(&sc, eSymbolContextEverything);
+  Address address;
+  lldb::addr_t load_address;
+  lldb::DisassemblerSP disassembler;
+  lldb::InstructionSP instruction;
+  lldb_private::ExecutionContext exe_ctx;
+};
 
-  if (!prev_sc.module_sp && !sc.module_sp)
-return sc;
-  if (prev_sc.module_sp == sc.module_sp && !sc.function && !sc.symbol &&
-  !prev_sc.function && !prev_sc.symbol)
+/// InstructionSymbolInfo object with symbol information for the given
+/// instruction, calculated efficiently.
+///
+/// \param[in] symbol_scope
+/// If not \b 0, then the \a InstructionSymbolInfo will have its
+/// SymbolContext calculated up to that level.
+///
+/// \param[in] include_disassembler
+/// If \b true, then the \a InstructionSymbolInfo will have the
+/// \a disassembler and \a instruction objects calculated.
+static void TraverseInstructionsWithSymbolInfo(
+Trace &trace, Thread &thread, size_t position,
+Trace::TraceDirection direction, SymbolContextItem symbol_scope,
+bool include_disassembler,
+std::function insn)>
+callback) {
+  InstructionSymbolInfo prev_insn;
+
+  Target &target = thread.GetProcess()->GetTarget();
+  ExecutionContext exe_ctx;
+  target.CalculateExecutionContext(exe_ctx);
+  const ArchSpec &arch = target.GetArchitecture();
+
+  // Find the symbol context for the given address reusing the previous
+  // instruction's symbol context when possible.
+  auto calculate_symbol_context = [&](const Address &address) {
+AddressRange range;
+if (prev_insn.sc.GetAddressRange(symbol_scope, 0,
+ /*inline_block_range*/ false, range) &&
+range.ContainsFileAddress(address))
+  return prev_insn.sc;
+
+SymbolContext sc;
+address.CalculateSymbolCon

[Lldb-commits] [PATCH] D101128: [lldb-vscode] only report long running progress events

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:35
+
+  if (m_event_type == progressStart)
+m_message = message;





Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:102
+: m_start_event(start_event),
+  m_start_event_timestamp(
+  std::chrono::system_clock::now().time_since_epoch()),

remove m_start_event_timestamp as it is no longer needed since ProgressEvent 
has a timestamp now.



Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:106
+
+bool ProgressEventManager::TryReport() {
+  // We only report if we have already started reporting or if enough time has

rename to "ReportIfNeeded()"?



Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:109-118
+  if (!m_start_event.Reported() &&
+  std::chrono::system_clock::now().time_since_epoch() -
+  m_start_event.GetTimestamp() <
+  std::chrono::seconds(1))
+return false;
+
+  if (!m_start_event.Reported() && !Finished()) {





Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:116-117
+  if (!m_start_event.Reported() && !Finished()) {
+m_report_callback(m_start_event);
+m_start_event.MarkAsReported();
+  }

We have many places in the code that are reporting progress and then having to 
manually mark the event as reported. We should make a ProgressEvent::Report() 
function that looks something like:
```
void ProgressEvent::Report(ProgressEventReportCallback report_callback) {
  if (!m_reported) {
m_reported = true;
report_callback(*this);
  }
}
```







Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:120-121
+  if (m_last_update_event && !m_last_update_event->Reported()) {
+m_report_callback(*m_last_update_event);
+m_last_update_event->MarkAsReported();
+  }





Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:146
+while (!m_aux_reporter_thread_should_exit) {
+  std::this_thread::sleep_for(std::chrono::seconds(2));
+  ReportWaitingEvents();

We have a 1 second timeout before events should be reported, so we should 
probably sleep a shorter amount of time than 2 seconds as if the events come in 
such that a progress start event is queued right after this loop sleeps for 2 
seconds, it can now be three seconds before an event is reported. How about 250 
ms? That way the most we will wait before reporting a start event is 1.25 
seconds. Also since we expect the thread to exit in ~ProgressEventFilterQueue, 
we don't want to have to wait 2 seconds during shut down. 



Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:157
+
+void ProgressEventFilterQueue::ReportWaitingEvents() {
+  std::lock_guard locker(m_reporter_mutex);

Should this be named "ReportStartEvents()"? Is that the only thing this is 
doing here?



Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:165
+else if (event_manager->TryReport())
+  m_sorted_event_managers
+  .pop(); // we remove it from the queue as it started reporting

If we are using m_sorted_event_managers only for reporting start events, we 
should rename to "m_unreported_start_events"



Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:170
+else
+  break; // If we couldn't report it, then the next event in the queue 
won't
+ // be able as well, as it came later.

Ok so this class is a start event reporter then?



Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:132
+  /// Thread used to invoke \a ReportWaitingEvents periodically.
+  std::thread m_aux_reporter_thread;
+  bool m_aux_reporter_thread_should_exit;

m_thread should suffice here.



Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:133
+  std::thread m_aux_reporter_thread;
+  bool m_aux_reporter_thread_should_exit;
+  /// Mutex that prevents running \a Push and \a ReportWaitingEvents

m_thread_should_exit should suffice.



Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:136
+  /// simultaneously, as both read and modify the same underlying objects.
+  std::mutex m_reporter_mutex;
 };

m_mutex is fine unless we get more than one mutex in this class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101128

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

[Lldb-commits] [PATCH] D100740: [trace] Dedup different source lines when dumping instructions + refactor

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/source/Target/Trace.cpp:163
+ /*inline_block_range*/ false, range) &&
+range.ContainsFileAddress(address))
+  return prev_insn.sc;

AddressRange::ContainsFileAddress() can't be used here after looking at how it 
is currently implemented. It will check if the sections are the same first and 
return the correct answer if they are, otherwise it will just extract a file 
addresses and compare them. 

To do this right, we need to uncomment out the currently unused:

```
bool AddressRange::Contains (const Address &addr) const;
```

And we will need to fix and use it by making sure the modules match. If the 
modules don't match, we don't want to extract two file addresses from two 
different modules and then compare those, as two modules can both have a 0x1000 
file address. 

The current commented out implementation of AddressRange::Contains(...) is 
wrong too. Fixing correctly looks like:
```
bool
AddressRange::Contains(const Address &addr) const
{
  SectionSP rangeSectSP = GetBaseAddress().GetSection();
  SectionSP addrSectSP = addr.GetSection();
  if (rangeSectSP) {
if (!addrSectSP || rangeSectSP->GetModule() != addrSectSP->GetModule())
  return false; // Modules do not match.
  } else if (addrSectSP) {
return false; // Range has no module but "addr" does because addr has a 
section
  }
  // Either the modules match, or both have no module, so it is ok to compare 
  // the file addresses in this case only.
  return ContainsFileAddress(addr);
}
```





Comment at: lldb/source/Target/Trace.cpp:173
+  // instruction's disassembler when possible.
+  auto calculate_disass = [&](const Address &address, const SymbolContext &sc) 
{
+if (prev_insn.disassembler) {

Can or should we get the disassembler in calculate_symbol_context above after 
calling address.CalculateSymbolContext(...)? 



Comment at: lldb/source/Target/Trace.cpp:191
+// We fallback to a single instruction disassembler
+AddressRange range(address, arch.GetMaximumOpcodeByteSize() * 1);
+DisassemblerSP disassembler = Disassembler::DisassembleRange(

Remove the " * 1" here



Comment at: lldb/source/Target/Trace.cpp:203
+  thread, position, direction,
+  [&](size_t index, Expected load_address) -> bool {
+if (!load_address)

Can this function really be called without a valid load address?



Comment at: lldb/source/Target/Trace.cpp:210-211
+insn.exe_ctx = exe_ctx;
+target.GetSectionLoadList().ResolveLoadAddress(*load_address,
+   insn.address);
+if (symbol_scope != 0)

Better to use:
```
bool Address::SetLoadAddress(lldb::addr_t load_addr, Target *target);
```





Comment at: lldb/source/Target/Trace.cpp:240-241
+  // symbol checks
+  if (!insn.sc.symbol && !prev_insn.sc.symbol)
 return true;
+  else if (insn.sc.symbol != prev_insn.sc.symbol)

You can't just return if there is no symbol, you can have no symbol, but still 
have a function from debug info if the symbol was a local symbol since it could 
have been stripped from the executable at build time.

Just remove these two lines and the "else" below



Comment at: lldb/source/Target/Trace.cpp:252-260
+  if (IsLineEntryValid(insn.sc.line_entry) !=
+  IsLineEntryValid(prev_insn.sc.line_entry))
+return false;
+  if (!IsLineEntryValid(insn.sc.line_entry) &&
+  !IsLineEntryValid(prev_insn.sc.line_entry))
+return true;
+  else





Comment at: lldb/source/Target/Trace.cpp:310
   size_t end_position, bool raw) {
-  if (!IsTraced(thread)) {
+  Optional instructions_count = GetInstructionCount(thread);
+  if (!instructions_count) {

Should this return a Expected? In case there is an error we want to 
show? Like maybe the trace buffer was truncated, or missing when it was 
expected? Can we have a process that is traced, but by the time we fetch the 
instructions, the circular buffer was filled with other data and even though 
the thread was traced there is no data?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100740

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

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM




Comment at: lldb/source/Breakpoint/BreakpointResolverFileLine.cpp:266-272
+  s->Printf("file = '%s', line = %u, ",
+m_location_spec.GetFileSpec().GetPath().c_str(),
+m_location_spec.GetLine().getValueOr(0));
+  auto column = m_location_spec.GetColumn();
+  if (column)
+s->Printf("column = %u, ", *column);
+  s->Printf("exact_match = %d", m_location_spec.GetExactMatch());

I was going to suggest calling the location spec's dump method, but I assume we 
don't want to change the format. Would it make sense to have a `GetDescription` 
in the  SourceLocationSpec class or change the dump format to match this 
format? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100965

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

[Lldb-commits] [PATCH] D101655: [debugserver] Include LLDB_VERSION_SUFFIX in debugserver version

[Jason Molenda via Phabricator via lldb-commits]

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

Yep looks good.


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

https://reviews.llvm.org/D101655

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

[Lldb-commits] [lldb] 2d5d720 - [debugserver] Include LLDB_VERSION_SUFFIX in debugserver version

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2021-05-03T15:05:32-07:00
New Revision: 2d5d720df0bba25f1aa2cea973d3d38dc5afac45

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

LOG: [debugserver] Include LLDB_VERSION_SUFFIX in debugserver version

The lack of a dot before the suffix is intentional, as the suffix itself
includes a dot or dash.

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

Added: 


Modified: 
lldb/tools/debugserver/source/debugserver_vers.c.in

Removed: 




diff  --git a/lldb/tools/debugserver/source/debugserver_vers.c.in 
b/lldb/tools/debugserver/source/debugserver_vers.c.in
index 00e34c29b07d..0de78f69fd1c 100644
--- a/lldb/tools/debugserver/source/debugserver_vers.c.in
+++ b/lldb/tools/debugserver/source/debugserver_vers.c.in
@@ -1,2 +1,2 @@
-const unsigned char debugserverVersionString[] __attribute__ ((used)) = 
"@(#)PROGRAM:LLDB  
PROJECT:lldb-@LLDB_VERSION_MAJOR@.@LLDB_VERSION_MINOR@.@LLDB_VERSION_PATCH@" 
"\n";
+const unsigned char debugserverVersionString[] __attribute__ ((used)) = 
"@(#)PROGRAM:LLDB  
PROJECT:lldb-@LLDB_VERSION_MAJOR@.@LLDB_VERSION_MINOR@.@LLDB_VERSION_PATCH@@LLDB_VERSION_SUFFIX@"
 "\n";
 const double debugserverVersionNumber __attribute__ ((used)) = 
(double)@LLDB_VERSION_MAJOR@.@LLDB_VERSION_MINOR@;



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

[Lldb-commits] [PATCH] D101655: [debugserver] Include LLDB_VERSION_SUFFIX in debugserver version

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2d5d720df0bb: [debugserver] Include LLDB_VERSION_SUFFIX in 
debugserver version (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101655

Files:
  lldb/tools/debugserver/source/debugserver_vers.c.in


Index: lldb/tools/debugserver/source/debugserver_vers.c.in
===
--- lldb/tools/debugserver/source/debugserver_vers.c.in
+++ lldb/tools/debugserver/source/debugserver_vers.c.in
@@ -1,2 +1,2 @@
-const unsigned char debugserverVersionString[] __attribute__ ((used)) = 
"@(#)PROGRAM:LLDB  
PROJECT:lldb-@LLDB_VERSION_MAJOR@.@LLDB_VERSION_MINOR@.@LLDB_VERSION_PATCH@" 
"\n";
+const unsigned char debugserverVersionString[] __attribute__ ((used)) = 
"@(#)PROGRAM:LLDB  
PROJECT:lldb-@LLDB_VERSION_MAJOR@.@LLDB_VERSION_MINOR@.@LLDB_VERSION_PATCH@@LLDB_VERSION_SUFFIX@"
 "\n";
 const double debugserverVersionNumber __attribute__ ((used)) = 
(double)@LLDB_VERSION_MAJOR@.@LLDB_VERSION_MINOR@;


Index: lldb/tools/debugserver/source/debugserver_vers.c.in
===
--- lldb/tools/debugserver/source/debugserver_vers.c.in
+++ lldb/tools/debugserver/source/debugserver_vers.c.in
@@ -1,2 +1,2 @@
-const unsigned char debugserverVersionString[] __attribute__ ((used)) = "@(#)PROGRAM:LLDB  PROJECT:lldb-@LLDB_VERSION_MAJOR@.@LLDB_VERSION_MINOR@.@LLDB_VERSION_PATCH@" "\n";
+const unsigned char debugserverVersionString[] __attribute__ ((used)) = "@(#)PROGRAM:LLDB  PROJECT:lldb-@LLDB_VERSION_MAJOR@.@LLDB_VERSION_MINOR@.@LLDB_VERSION_PATCH@@LLDB_VERSION_SUFFIX@" "\n";
 const double debugserverVersionNumber __attribute__ ((used)) = (double)@LLDB_VERSION_MAJOR@.@LLDB_VERSION_MINOR@;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 60ad0fd - Clarify the help for "breakpoint command add" and "watchpoint command add".

[Jim Ingham via lldb-commits]

Author: Jim Ingham
Date: 2021-05-03T17:22:43-07:00
New Revision: 60ad0fd3c8bf18dc1a307b97e9e3eb21dd5e4cfb

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

LOG: Clarify the help for "breakpoint command add" and "watchpoint command add".

These two commands add a list of commands to the breakpoint/watchpoint. The 
current
implementation only supports replacing the current command list.  I started with
that as overwrite seems to be the most common operation.  But using "add" will
allow us to later offer other add-modes: "prepend", "append" and "insert".
That and "overwrite" then make up a useful set of options for this operation.

Added: 


Modified: 
lldb/source/Commands/CommandObjectBreakpointCommand.cpp
lldb/source/Commands/CommandObjectWatchpointCommand.cpp

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectBreakpointCommand.cpp 
b/lldb/source/Commands/CommandObjectBreakpointCommand.cpp
index 3489b5cffb9f3..127cde061120d 100644
--- a/lldb/source/Commands/CommandObjectBreakpointCommand.cpp
+++ b/lldb/source/Commands/CommandObjectBreakpointCommand.cpp
@@ -61,7 +61,9 @@ class CommandObjectBreakpointCommandAdd : public 
CommandObjectParsed,
   CommandObjectBreakpointCommandAdd(CommandInterpreter &interpreter)
   : CommandObjectParsed(interpreter, "add",
 "Add LLDB commands to a breakpoint, to be executed 
"
-"whenever the breakpoint is hit."
+"whenever the breakpoint is hit.  "
+"The commands added to the breakpoint replace any "
+"commands previously added to it."
 "  If no breakpoint is specified, adds the "
 "commands to the last created breakpoint.",
 nullptr),

diff  --git a/lldb/source/Commands/CommandObjectWatchpointCommand.cpp 
b/lldb/source/Commands/CommandObjectWatchpointCommand.cpp
index 3df17a0c17f3d..f22687ff4bd87 100644
--- a/lldb/source/Commands/CommandObjectWatchpointCommand.cpp
+++ b/lldb/source/Commands/CommandObjectWatchpointCommand.cpp
@@ -61,7 +61,9 @@ class CommandObjectWatchpointCommandAdd : public 
CommandObjectParsed,
   CommandObjectWatchpointCommandAdd(CommandInterpreter &interpreter)
   : CommandObjectParsed(interpreter, "add",
 "Add a set of LLDB commands to a watchpoint, to be 
"
-"executed whenever the watchpoint is hit.",
+"executed whenever the watchpoint is hit.  "
+"The commands added to the watchpoint replace any "
+"commands previously added to it.",
 nullptr, eCommandRequiresTarget),
 IOHandlerDelegateMultiline("DONE",
IOHandlerDelegate::Completion::LLDBCommand),



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

[Lldb-commits] [PATCH] D101361: [LLDB] Support AArch64/Linux watchpoint on tagged addresses

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 342608.
omjavaid added a comment.

This fixes review comments.


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

https://reviews.llvm.org/D101361

Files:
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
  lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.cpp
  lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h
  lldb/source/Target/Target.cpp
  lldb/test/API/commands/watchpoints/watch_tagged_addr/Makefile
  lldb/test/API/commands/watchpoints/watch_tagged_addr/TestWatchTaggedAddress.py
  lldb/test/API/commands/watchpoints/watch_tagged_addr/main.c

Index: lldb/test/API/commands/watchpoints/watch_tagged_addr/main.c
===
--- /dev/null
+++ lldb/test/API/commands/watchpoints/watch_tagged_addr/main.c
@@ -0,0 +1,17 @@
+#include 
+
+uint32_t global_var = 0; // Watchpoint variable declaration.
+
+int main(int argc, char **argv) {
+  // Move address of global variable into tagged_ptr after tagging
+  // Simple tagging scheme where 62nd bit of tagged address is set
+  uint32_t *tagged_ptr = (uint32_t *)((uint64_t)&global_var | (1ULL << 62));
+
+  // Increment global_var
+  ++global_var; // Set break point at this line.
+
+  // Increment global_var using tagged_ptr
+  ++*tagged_ptr;
+
+  return 0;
+}
Index: lldb/test/API/commands/watchpoints/watch_tagged_addr/TestWatchTaggedAddress.py
===
--- /dev/null
+++ lldb/test/API/commands/watchpoints/watch_tagged_addr/TestWatchTaggedAddress.py
@@ -0,0 +1,130 @@
+"""
+Test LLDB can set and hit watchpoints on tagged addresses
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestWatchTaggedAddresses(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+def setUp(self):
+# Call super's setUp().
+TestBase.setUp(self)
+
+# Set source filename.
+self.source = 'main.c'
+
+# Invoke the default build rule.
+self.build()
+
+# Get the path of the executable
+exe = self.getBuildArtifact("a.out")
+
+# Create a target by the debugger.
+self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+@skipIf(archs=no_match(["aarch64"]))
+@skipIf(oslist=no_match(['linux']))
+def test_watch_hit_tagged_ptr_access(self):
+"""
+Test that LLDB hits watchpoint installed on an untagged address with
+memory access by a tagged pointer.
+"""
+
+# Add a breakpoint to set a watchpoint when stopped on the breakpoint.
+lldbutil.run_break_set_by_symbol(self, 'main')
+
+# Run the program.
+self.runCmd("run", RUN_SUCCEEDED)
+
+# We should be stopped due to the breakpoint.
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+# Set the watchpoint variable declaration line number.
+self.decl = line_number(self.source,
+'// Watchpoint variable declaration.')
+
+# Now let's set a watchpoint on 'global_var'.
+self.expect(
+"watchpoint set variable global_var",
+WATCHPOINT_CREATED,
+substrs=[
+'Watchpoint created',
+'size = 4',
+'type = w',
+'%s:%d' %
+(self.source,
+ self.decl)])
+
+self.verify_watch_hits()
+
+@skipIf(archs=no_match(["aarch64"]))
+@skipIf(oslist=no_match(['linux']))
+def test_watch_set_on_tagged_ptr(self):
+"""Test that LLDB can install and hit watchpoint on a tagged address"""
+
+# Find the line number to break inside main().
+self.line = line_number(self.source, '// Set break point at this line.')
+
+# Add a breakpoint to set a watchpoint when stopped on the breakpoint.
+lldbutil.run_break_set_by_file_and_line(
+self, None, self.line, num_expected_locations=1)
+
+# Run the program.
+self.runCmd("run", RUN_SUCCEEDED)
+
+# We should be stopped due to the breakpoint.
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+# Now let's set a expression watchpoint on 'tagged_ptr'.
+self.expect(
+"watchpoint set expression -s 4 -- tagged_ptr",
+WATCHPOINT_CREATED,
+substrs=[
+'Watchpoint created',
+'size = 4',
+'type = w'])
+
+self.verify_watch_hits()
+
+def verify_watch_hits(self):
+

[Lldb-commits] [PATCH] D101361: [LLDB] Support AArch64/Linux watchpoint on tagged addresses

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added inline comments.



Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:888
+  // consolidated data address mask after ignoring the top byte.
+  if (!ReadPAuthMask().Fail())
+mask |= m_pac_mask.data_mask;

DavidSpickett wrote:
> I'm guessing this returns a Status, I think that has a .Success().
Fixed in current rev.



Comment at: 
lldb/source/Plugins/Process/Utility/NativeRegisterContextDBReg_arm64.h:77
+return hit_addr;
+  }
 };

DavidSpickett wrote:
> This is going to be used by FreeBSD, have you checked if it enables top byte 
> ignore at all?
In current rev I have moved a default implementation of this function which 
only ignores top byte. Should work for all other platforms.



Comment at: 
lldb/test/API/commands/watchpoints/watch_tagged_addr/TestWatchTaggedAddress.py:34
+def test_watch_hit_tagged_ptr_access(self):
+"""Test LLDB hits watchpoints when tagged pointer is used for memory 
access"""
+

DavidSpickett wrote:
> These docstrings should be more specific. E.g. this test puts a watchpoint on 
> an untagged address, where the one below is tagged.
Fixed in current rev.



Comment at: 
lldb/test/API/commands/watchpoints/watch_tagged_addr/TestWatchTaggedAddress.py:103
+
+self.runCmd("process continue")
+

DavidSpickett wrote:
> You could remove some of the continues by adding a command to the watchpoint:
> ```
> (lldb) watchpoint command add 1 -o continue
> ```
> Then just check the stats at the end after a single continue.
> 
> I thought at first this would be bad because you wouldn't know which hit was 
> missed, but we don't know that with the current setup either.
I would like to avoid this and keep the current version as it makes testcase 
more readable for newbies.


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

https://reviews.llvm.org/D101361

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

[Lldb-commits] [PATCH] D101128: [lldb-vscode] only report long running progress events

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 342650.
wallace marked 12 inline comments as done.
wallace added a comment.

Address all comments

- now the reporting timing conditions are in ProgressEvent, which simplifies 
the other classes
- removed the progressInvalid enum by making sure only valid ProgressEvent 
instances are created


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101128

Files:
  lldb/tools/lldb-vscode/ProgressEvent.cpp
  lldb/tools/lldb-vscode/ProgressEvent.h
  lldb/tools/lldb-vscode/VSCode.cpp
  lldb/tools/lldb-vscode/VSCode.h
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -376,8 +376,7 @@
 const char *message = lldb::SBDebugger::GetProgressFromEvent(
 event, progress_id, completed, total, is_debugger_specific);
 if (message)
-  g_vsc.SendProgressEvent(
-  ProgressEvent(progress_id, message, completed, total));
+  g_vsc.SendProgressEvent(progress_id, message, completed, total);
   }
 }
   }
Index: lldb/tools/lldb-vscode/VSCode.h
===
--- lldb/tools/lldb-vscode/VSCode.h
+++ lldb/tools/lldb-vscode/VSCode.h
@@ -114,7 +114,7 @@
   uint32_t reverse_request_seq;
   std::map request_handlers;
   bool waiting_for_run_in_terminal;
-  ProgressEventFilterQueue progress_event_queue;
+  ProgressEventReporter progress_event_reporter;
   // Keep track of the last stop thread index IDs as threads won't go away
   // unless we send a "thread" event to indicate the thread exited.
   llvm::DenseSet thread_ids;
@@ -125,10 +125,6 @@
   int64_t GetLineForPC(int64_t sourceReference, lldb::addr_t pc) const;
   ExceptionBreakpoint *GetExceptionBreakpoint(const std::string &filter);
   ExceptionBreakpoint *GetExceptionBreakpoint(const lldb::break_id_t bp_id);
-  // Send the JSON in "json_str" to the "out" stream. Correctly send the
-  // "Content-Length:" field followed by the length, followed by the raw
-  // JSON bytes.
-  void SendJSON(const std::string &json_str);
 
   // Serialize the JSON value into a string and send the JSON packet to
   // the "out" stream.
@@ -138,7 +134,8 @@
 
   void SendOutput(OutputType o, const llvm::StringRef output);
 
-  void SendProgressEvent(const ProgressEvent &event);
+  void SendProgressEvent(uint64_t progress_id, const char *message,
+ uint64_t completed, uint64_t total);
 
   void __attribute__((format(printf, 3, 4)))
   SendFormattedOutput(OutputType o, const char *format, ...);
@@ -208,6 +205,12 @@
   /// The callback to execute when the given request is triggered by the
   /// IDE.
   void RegisterRequestCallback(std::string request, RequestCallback callback);
+
+private:
+  // Send the JSON in "json_str" to the "out" stream. Correctly send the
+  // "Content-Length:" field followed by the length, followed by the raw
+  // JSON bytes.
+  void SendJSON(const std::string &json_str);
 };
 
 extern VSCode g_vsc;
Index: lldb/tools/lldb-vscode/VSCode.cpp
===
--- lldb/tools/lldb-vscode/VSCode.cpp
+++ lldb/tools/lldb-vscode/VSCode.cpp
@@ -42,7 +42,7 @@
   focus_tid(LLDB_INVALID_THREAD_ID), sent_terminated_event(false),
   stop_at_entry(false), is_attach(false), reverse_request_seq(0),
   waiting_for_run_in_terminal(false),
-  progress_event_queue(
+  progress_event_reporter(
   [&](const ProgressEvent &event) { SendJSON(event.ToJSON()); }) {
   const char *log_file_path = getenv("LLDBVSCODE_LOG");
 #if defined(_WIN32)
@@ -322,8 +322,9 @@
 //   };
 // }
 
-void VSCode::SendProgressEvent(const ProgressEvent &event) {
-  progress_event_queue.Push(event);
+void VSCode::SendProgressEvent(uint64_t progress_id, const char *message,
+   uint64_t completed, uint64_t total) {
+  progress_event_reporter.Push(progress_id, message, completed, total);
 }
 
 void __attribute__((format(printf, 3, 4)))
Index: lldb/tools/lldb-vscode/ProgressEvent.h
===
--- lldb/tools/lldb-vscode/ProgressEvent.h
+++ lldb/tools/lldb-vscode/ProgressEvent.h
@@ -6,6 +6,10 @@
 //
 //===--===//
 
+#include 
+#include 
+#include 
+
 #include "VSCodeForward.h"
 
 #include "llvm/Support/JSON.h"
@@ -13,50 +17,127 @@
 namespace lldb_vscode {
 
 enum ProgressEventType {
-  progressInvalid,
   progressStart,
   progressUpdate,
   progressEnd
 };
 
+class ProgressEvent;
+using ProgressEventReportCallback = std::function;
+
 class ProgressEvent {
 public:
-  ProgressEvent() {}
+  /// Constructor for an start event
+  ProgressEvent(uint64_t progress_i