date:20210909

[Lldb-commits] [PATCH] D108739: [LLDB] AArch64 SVE restore SVE registers after expression

2021-09-09 Thread David Spickett via Phabricator via lldb-commits

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

LGTM with the clang-format warning fixed.


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

https://reviews.llvm.org/D108739

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

[Lldb-commits] [lldb] d1280f6 - [lldb] [test] Add tests for coredumps with multiple threads

2021-09-09 Thread Michał Górny via lldb-commits


Author: Michał Górny
Date: 2021-09-09T09:59:52+02:00
New Revision: d1280f6967db1ca8fa4e0c39414003e717b40feb

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

LOG: [lldb] [test] Add tests for coredumps with multiple threads

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

Added: 
lldb/test/Shell/Register/Core/Inputs/aarch64-freebsd-multithread.core
lldb/test/Shell/Register/Core/Inputs/multithread.cpp
lldb/test/Shell/Register/Core/Inputs/x86-32-freebsd-multithread.core
lldb/test/Shell/Register/Core/Inputs/x86-32-linux-multithread.core
lldb/test/Shell/Register/Core/Inputs/x86-32-netbsd-multithread.core
lldb/test/Shell/Register/Core/Inputs/x86-64-freebsd-multithread.core
lldb/test/Shell/Register/Core/Inputs/x86-64-linux-multithread.core
lldb/test/Shell/Register/Core/Inputs/x86-64-netbsd-multithread.core
lldb/test/Shell/Register/Core/aarch64-freebsd-multithread.test
lldb/test/Shell/Register/Core/x86-32-freebsd-multithread.test
lldb/test/Shell/Register/Core/x86-32-linux-multithread.test
lldb/test/Shell/Register/Core/x86-32-netbsd-multithread.test
lldb/test/Shell/Register/Core/x86-64-freebsd-multithread.test
lldb/test/Shell/Register/Core/x86-64-linux-multithread.test
lldb/test/Shell/Register/Core/x86-64-netbsd-multithread.test

Modified: 


Removed: 




diff  --git 
a/lldb/test/Shell/Register/Core/Inputs/aarch64-freebsd-multithread.core 
b/lldb/test/Shell/Register/Core/Inputs/aarch64-freebsd-multithread.core
new file mode 100644
index 0..e3ba2d7519af8
Binary files /dev/null and 
b/lldb/test/Shell/Register/Core/Inputs/aarch64-freebsd-multithread.core 
diff er

diff  --git a/lldb/test/Shell/Register/Core/Inputs/multithread.cpp 
b/lldb/test/Shell/Register/Core/Inputs/multithread.cpp
new file mode 100644
index 0..19c6682b32cf8
--- /dev/null
+++ b/lldb/test/Shell/Register/Core/Inputs/multithread.cpp
@@ -0,0 +1,77 @@
+// This program is used to generate a core dump for testing multithreading
+// support.
+
+#include 
+#include 
+#include 
+#include 
+
+std::atomic_int start_counter{3};
+
+void pseudobarrier_wait() {
+  start_counter--;
+  while (start_counter > 0);
+}
+
+void fcommon(uint32_t a, uint32_t b, uint32_t c, uint32_t d, double fa, double 
fb, double fc, double fd, bool segv) {
+  if (segv) {
+int *foo = nullptr;
+*foo = 0;
+  }
+  while (1);
+}
+
+void f1() {
+  volatile uint32_t a = 0x01010101;
+  volatile uint32_t b = 0x02020202;
+  volatile uint32_t c = 0x03030303;
+  volatile uint32_t d = 0x04040404;
+  volatile double fa = 2.0;
+  volatile double fb = 4.0;
+  volatile double fc = 8.0;
+  volatile double fd = 16.0;
+
+  pseudobarrier_wait();
+  std::this_thread::sleep_for(std::chrono::milliseconds(200));
+  fcommon(a, b, c, d, fa, fb, fc, fd, true);
+}
+
+void f2() {
+  volatile uint32_t a = 0x;
+  volatile uint32_t b = 0x12121212;
+  volatile uint32_t c = 0x13131313;
+  volatile uint32_t d = 0x14141414;
+  volatile double fa = 3.0;
+  volatile double fb = 6.0;
+  volatile double fc = 9.0;
+  volatile double fd = 12.0;
+
+  pseudobarrier_wait();
+  fcommon(a, b, c, d, fa, fb, fc, fd, false);
+}
+
+void f3() {
+  volatile uint32_t a = 0x21212121;
+  volatile uint32_t b = 0x;
+  volatile uint32_t c = 0x23232323;
+  volatile uint32_t d = 0x24242424;
+  volatile double fa = 5.0;
+  volatile double fb = 10.0;
+  volatile double fc = 15.0;
+  volatile double fd = 20.0;
+
+  pseudobarrier_wait();
+  fcommon(a, b, c, d, fa, fb, fc, fd, false);
+}
+
+int main() {
+  std::thread t1{f1};
+  std::thread t2{f2};
+  std::thread t3{f3};
+
+  t3.join();
+  t2.join();
+  t1.join();
+
+  return 0;
+}

diff  --git 
a/lldb/test/Shell/Register/Core/Inputs/x86-32-freebsd-multithread.core 
b/lldb/test/Shell/Register/Core/Inputs/x86-32-freebsd-multithread.core
new file mode 100644
index 0..342d5fb40f2b2
Binary files /dev/null and 
b/lldb/test/Shell/Register/Core/Inputs/x86-32-freebsd-multithread.core 
diff er

diff  --git 
a/lldb/test/Shell/Register/Core/Inputs/x86-32-linux-multithread.core 
b/lldb/test/Shell/Register/Core/Inputs/x86-32-linux-multithread.core
new file mode 100644
index 0..376a13217977d
Binary files /dev/null and 
b/lldb/test/Shell/Register/Core/Inputs/x86-32-linux-multithread.core 
diff er

diff  --git 
a/lldb/test/Shell/Register/Core/Inputs/x86-32-netbsd-multithread.core 
b/lldb/test/Shell/Register/Core/Inputs/x86-32-netbsd-multithread.core
new file mode 100644
index 0..e21821b5e1132
Binary files /dev/null and 
b/lldb/test/Shell/Register/Core/Inputs/x86-32-netbsd-multithread.core 
diff er

diff  --git 
a/lldb/test/Shell/Register/Core/Inputs/x86-64-freebsd-multithread.core 
b/lldb/test/Shell/Register/Core/Inputs/x86-64-freebsd-multithr

[Lldb-commits] [PATCH] D101157: [lldb] [test] Add tests for coredumps with multiple threads

2021-09-09 Thread Michał Górny 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 rGd1280f6967db: [lldb] [test] Add tests for coredumps with 
multiple threads (authored by mgorny).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101157

Files:
  lldb/test/Shell/Register/Core/Inputs/aarch64-freebsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/multithread.cpp
  lldb/test/Shell/Register/Core/Inputs/x86-32-freebsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-32-linux-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-32-netbsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-64-freebsd-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-64-linux-multithread.core
  lldb/test/Shell/Register/Core/Inputs/x86-64-netbsd-multithread.core
  lldb/test/Shell/Register/Core/aarch64-freebsd-multithread.test
  lldb/test/Shell/Register/Core/x86-32-freebsd-multithread.test
  lldb/test/Shell/Register/Core/x86-32-linux-multithread.test
  lldb/test/Shell/Register/Core/x86-32-netbsd-multithread.test
  lldb/test/Shell/Register/Core/x86-64-freebsd-multithread.test
  lldb/test/Shell/Register/Core/x86-64-linux-multithread.test
  lldb/test/Shell/Register/Core/x86-64-netbsd-multithread.test

Index: lldb/test/Shell/Register/Core/x86-64-netbsd-multithread.test
===
--- /dev/null
+++ lldb/test/Shell/Register/Core/x86-64-netbsd-multithread.test
@@ -0,0 +1,41 @@
+# RUN: %lldb -b -s %s -c %p/Inputs/x86-64-netbsd-multithread.core | FileCheck %s
+
+thread list
+# CHECK: * thread #1: tid = 2, 0x00400f82, stop reason = signal SIGSEGV 
+# CHECK-NEXT:   thread #2: tid = 4, 0x00400f88, stop reason = signal 0 
+# CHECK-NEXT:   thread #3: tid = 3, 0x00400f88, stop reason = signal 0 
+# CHECK-NEXT:   thread #4: tid = 1, 0x791280ca1faa, stop reason = signal 0 
+
+register read --all
+# CHECK-DAG: ecx = 0x04040404
+# CHECK-DAG: edx = 0x03030303
+# CHECK-DAG: edi = 0x01010101
+# CHECK-DAG: esi = 0x02020202
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x10 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x20 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x30 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+
+thread select 3
+# CHECK: (lldb) thread select 3
+register read --all
+# CHECK-DAG: ecx = 0x14141414
+# CHECK-DAG: edx = 0x13131313
+# CHECK-DAG: edi = 0x
+# CHECK-DAG: esi = 0x12121212
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x08 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x18 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x22 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x28 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+
+thread select 2
+# CHECK: (lldb) thread select 2
+register read --all
+# CHECK-DAG: ecx = 0x24242424
+# CHECK-DAG: edx = 0x23232323
+# CHECK-DAG: edi = 0x21212121
+# CHECK-DAG: esi = 0x
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x14 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x24 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x2e 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm3 = {0x00 0x00 0x00 0x00 0x00 0x00 0x34 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
Index: lldb/test/Shell/Register/Core/x86-64-linux-multithread.test
===
--- /dev/null
+++ lldb/test/Shell/Register/Core/x86-64-linux-multithread.test
@@ -0,0 +1,41 @@
+# RUN: %lldb -b -s %s -c %p/Inputs/x86-64-linux-multithread.core | FileCheck %s
+
+thread list
+# CHECK: * thread #1: tid = 329384, 0x00401262, name = 'a.out', stop reason = signal SIGSEGV
+# CHECK-NEXT:   thread #2: tid = 329385, 0x0040126d, stop reason = signal 0
+# CHECK-NEXT:   thread #3: tid = 329386, 0x0040126d, stop reason = signal 0
+# CHECK-NEXT:   thread #4: tid = 329383, 0x7fcf5582f762, stop reason = signal 0
+
+register read --all
+# CHECK-DAG: ecx = 0x04040404
+# CHECK-DAG: edx = 0x03030303
+# CHECK-DAG: edi = 0x01010101
+# CHECK-DAG: esi = 0x02020202
+# CHECK-DAG: xmm0 = {0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm1 = {0x00 0x00 0x00 0x00 0x00 0x00 0x10 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG: xmm2 = {0x00 0x00 0x00 0x00 0x00 0x00 0x20 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}
+# CHECK-DAG:

[Lldb-commits] [PATCH] D109495: [lldb] [test] Add synchronization fix Subprocess test flakiness

2021-09-09 Thread Michał Górny via Phabricator via lldb-commits

mgorny created this revision.
mgorny added reviewers: labath, emaste, krytarowski.
mgorny requested review of this revision.

Add synchronization routines to ensure that Subprocess tests output
in a predictable order, and all test strings are output before the tests
terminate.


https://reviews.llvm.org/D109495

Files:
  lldb/test/Shell/Subprocess/Inputs/fork.cpp
  lldb/test/Shell/Subprocess/clone-follow-child-softbp.test
  lldb/test/Shell/Subprocess/clone-follow-child-wp.test
  lldb/test/Shell/Subprocess/clone-follow-child.test
  lldb/test/Shell/Subprocess/clone-follow-parent-softbp.test
  lldb/test/Shell/Subprocess/clone-follow-parent-wp.test
  lldb/test/Shell/Subprocess/clone-follow-parent.test
  lldb/test/Shell/Subprocess/fork-follow-child-softbp.test
  lldb/test/Shell/Subprocess/fork-follow-child-wp.test
  lldb/test/Shell/Subprocess/fork-follow-child.test
  lldb/test/Shell/Subprocess/fork-follow-parent-softbp.test
  lldb/test/Shell/Subprocess/fork-follow-parent-wp.test
  lldb/test/Shell/Subprocess/fork-follow-parent.test
  lldb/test/Shell/Subprocess/vfork-follow-child-softbp.test
  lldb/test/Shell/Subprocess/vfork-follow-child-wp.test
  lldb/test/Shell/Subprocess/vfork-follow-child.test
  lldb/test/Shell/Subprocess/vfork-follow-parent-softbp.test
  lldb/test/Shell/Subprocess/vfork-follow-parent-wp.test
  lldb/test/Shell/Subprocess/vfork-follow-parent.test

Index: lldb/test/Shell/Subprocess/vfork-follow-parent.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-parent.test
+++ lldb/test/Shell/Subprocess/vfork-follow-parent.test
@@ -8,4 +8,5 @@
 # CHECK: stop reason = breakpoint
 continue
 # CHECK: function run in parent
-# CHECK: child exited: 0
+# CHECK: function run in exec'd child
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/test/Shell/Subprocess/vfork-follow-parent-wp.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-parent-wp.test
+++ lldb/test/Shell/Subprocess/vfork-follow-parent-wp.test
@@ -11,4 +11,5 @@
 # CHECK: stop reason = watchpoint
 continue
 # CHECK: function run in parent
-# CHECK: child exited: 0
+# CHECK: function run in exec'd child
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/test/Shell/Subprocess/vfork-follow-parent-softbp.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-parent-softbp.test
+++ lldb/test/Shell/Subprocess/vfork-follow-parent-softbp.test
@@ -10,4 +10,5 @@
 # CHECK: stop reason = breakpoint
 continue
 # CHECK: function run in parent
-# CHECK: child exited: 0
+# CHECK: function run in exec'd child
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/test/Shell/Subprocess/vfork-follow-child.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-child.test
+++ lldb/test/Shell/Subprocess/vfork-follow-child.test
@@ -1,12 +1,12 @@
 # REQUIRES: native
 # UNSUPPORTED: system-darwin
 # UNSUPPORTED: system-windows
-# This test is very flaky on aarch64.
-# UNSUPPORTED: system-linux && target-aarch64
 # RUN: %clangxx_host %p/Inputs/fork.cpp -DTEST_FORK=vfork -o %t
 # RUN: %lldb -b -s %s %t | FileCheck %s
 settings set target.process.follow-fork-mode child
+settings set target.process.stop-on-exec false
 b parent_func
 process launch
 # CHECK: function run in parent
-# CHECK: child exited: 0
+# CHECK: function run in exec'd child
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/test/Shell/Subprocess/vfork-follow-child-wp.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-child-wp.test
+++ lldb/test/Shell/Subprocess/vfork-follow-child-wp.test
@@ -1,13 +1,14 @@
 # REQUIRES: native && dbregs-set
 # UNSUPPORTED: system-darwin
 # UNSUPPORTED: system-windows
-# This test is very flaky on aarch64.
-# UNSUPPORTED: system-linux && target-aarch64
 # RUN: %clangxx_host -g %p/Inputs/fork.cpp -DTEST_FORK=vfork -o %t
 # RUN: %lldb -b -s %s %t | FileCheck %s
 settings set target.process.follow-fork-mode child
+settings set target.process.stop-on-exec false
 process launch -s
 watchpoint set variable -w write g_val
 # CHECK: Watchpoint created:
 continue
-# CHECK: child exited: 0
+# CHECK: function run in parent
+# CHECK: function run in exec'd child
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/test/Shell/Subprocess/vfork-follow-child-softbp.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-child-softbp.test
+++ lldb/test/Shell/Subprocess/vfork-follow-child-softbp.test
@@ -1,13 +1,13 @@
 # REQUIRES: native
 # UNSUPPORTED: system-darwin
 # UNSUPPORTED: system-windows
-# This test is very flaky on aarch64.
-# UNSUPPORTED: system-linux && target-aarch64
 # RUN: %clangxx_host %p/Inputs/fork.cpp -DTEST_FORK=vfork -o %t
 # RUN: %lldb

[Lldb-commits] [lldb] 8901f8b - AArch64 SVE restore SVE registers after expression

2021-09-09 Thread Muhammad Omair Javaid via lldb-commits


Author: Muhammad Omair Javaid
Date: 2021-09-09T16:06:48+05:00
New Revision: 8901f8beea3a70f92be8c0b80313260502f03727

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

LOG: AArch64 SVE restore SVE registers after expression

This patch fixes register save/restore on expression call to also include SVE 
registers.

This will fix expression calls like:

re re p1



p 

re re p1



In above example register P1 should remain the same before and after the 
expression evaluation.

Reviewed By: DavidSpickett

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

Added: 


Modified: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h

lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/TestSVERegisters.py

lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/main.c

Removed: 




diff  --git 
a/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp 
b/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
index ebde0a499acf7..f28bddcb9a99b 100644
--- a/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
+++ b/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
@@ -46,8 +46,6 @@
 #define HWCAP_PACA (1 << 30)
 #define HWCAP2_MTE (1 << 18)
 
-#define REG_CONTEXT_SIZE (GetGPRSize() + GetFPRSize())
-
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::process_linux;
@@ -452,30 +450,73 @@ Status NativeRegisterContextLinux_arm64::WriteRegister(
 
 Status NativeRegisterContextLinux_arm64::ReadAllRegisterValues(
 lldb::DataBufferSP &data_sp) {
-  Status error;
+  // AArch64 register data must contain GPRs, either FPR or SVE registers
+  // and optional MTE register. Pointer Authentication (PAC) registers are
+  // read-only and will be skiped.
 
-  data_sp.reset(new DataBufferHeap(REG_CONTEXT_SIZE, 0));
+  // In order to create register data checkpoint we first read all register
+  // values if not done already and calculate total size of register set data.
+  // We store all register values in data_sp by copying full PTrace data that
+  // corresponds to register sets enabled by current register context.
 
+  Status error;
+  uint32_t reg_data_byte_size = GetGPRBufferSize();
   error = ReadGPR();
   if (error.Fail())
 return error;
 
-  error = ReadFPR();
+  // If SVE is enabled we need not copy FPR separately.
+  if (GetRegisterInfo().IsSVEEnabled()) {
+reg_data_byte_size += GetSVEBufferSize();
+error = ReadAllSVE();
+  } else {
+reg_data_byte_size += GetFPRSize();
+error = ReadFPR();
+  }
   if (error.Fail())
 return error;
 
+  if (GetRegisterInfo().IsMTEEnabled()) {
+reg_data_byte_size += GetMTEControlSize();
+error = ReadMTEControl();
+if (error.Fail())
+  return error;
+  }
+
+  data_sp.reset(new DataBufferHeap(reg_data_byte_size, 0));
   uint8_t *dst = data_sp->GetBytes();
-  ::memcpy(dst, GetGPRBuffer(), GetGPRSize());
-  dst += GetGPRSize();
-  ::memcpy(dst, GetFPRBuffer(), GetFPRSize());
+
+  ::memcpy(dst, GetGPRBuffer(), GetGPRBufferSize());
+  dst += GetGPRBufferSize();
+
+  if (GetRegisterInfo().IsSVEEnabled()) {
+::memcpy(dst, GetSVEBuffer(), GetSVEBufferSize());
+dst += GetSVEBufferSize();
+  } else {
+::memcpy(dst, GetFPRBuffer(), GetFPRSize());
+dst += GetFPRSize();
+  }
+
+  if (GetRegisterInfo().IsMTEEnabled())
+::memcpy(dst, GetMTEControl(), GetMTEControlSize());
 
   return error;
 }
 
 Status NativeRegisterContextLinux_arm64::WriteAllRegisterValues(
 const lldb::DataBufferSP &data_sp) {
-  Status error;
+  // AArch64 register data must contain GPRs, either FPR or SVE registers
+  // and optional MTE register. Pointer Authentication (PAC) registers are
+  // read-only and will be skiped.
+
+  // We store all register values in data_sp by copying full PTrace data that
+  // corresponds to register sets enabled by current register context. In order
+  // to restore from register data checkpoint we will first restore GPRs, based
+  // on size of remaining register data either SVE or FPRs should be restored
+  // next. SVE is not enabled if we have register data size less than or equal
+  // to size of GPR + FPR + MTE.
 
+  Status error;
   if (!data_sp) {
 error.SetErrorStringWithFormat(
 "NativeRegisterContextLinux_arm64::%s invalid data_sp provided",
@@ -483,14 +524,6 @@ Status 
NativeRegisterContextLinux_arm64::WriteAllRegisterValues(
 return error;
   }
 
-  if (data_sp->GetByteSize() != REG_CONTEXT_SIZE) {
-error.SetErrorStringWithFormat(
-"N

[Lldb-commits] [PATCH] D108739: [LLDB] AArch64 SVE restore SVE registers after expression

2021-09-09 Thread 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 rG8901f8beea3a: AArch64 SVE restore SVE registers after 
expression (authored by omjavaid).

Changed prior to commit:
  https://reviews.llvm.org/D108739?vs=371380&id=371548#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108739

Files:
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.h
  lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.h
  
lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/TestSVERegisters.py
  
lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/main.c

Index: lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/main.c
===
--- lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/main.c
+++ lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/main.c
@@ -1,4 +1,6 @@
-int main() {
+#include 
+
+void write_sve_regs() {
   asm volatile("setffr\n\t");
   asm volatile("ptrue p0.b\n\t");
   asm volatile("ptrue p1.h\n\t");
@@ -49,5 +51,20 @@
   asm volatile("cpy  z29.b, p5/z, #30\n\t");
   asm volatile("cpy  z30.b, p10/z, #31\n\t");
   asm volatile("cpy  z31.b, p15/z, #32\n\t");
+}
+
+// This function will be called using jitted expression call. We change vector
+// length and write SVE registers. Our program context should restore to
+// orignal vector length and register values after expression evaluation.
+int expr_eval_func() {
+  prctl(PR_SVE_SET_VL, 8 * 2);
+  write_sve_regs();
+  prctl(PR_SVE_SET_VL, 8 * 4);
+  write_sve_regs();
+  return 1;
+}
+
+int main() {
+  write_sve_regs();
   return 0; // Set a break point here.
 }
Index: lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/TestSVERegisters.py
===
--- lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/TestSVERegisters.py
+++ lldb/test/API/commands/register/register/aarch64_sve_registers/rw_access_static_config/TestSVERegisters.py
@@ -17,6 +17,51 @@
 self.assertEqual(reg_value.GetByteSize(), expected,
  'Verify "%s" == %i' % (name, expected))
 
+def check_sve_regs_read(self, z_reg_size):
+p_reg_size = int(z_reg_size / 8)
+
+for i in range(32):
+z_regs_value = '{' + \
+' '.join('0x{:02x}'.format(i + 1)
+ for _ in range(z_reg_size)) + '}'
+self.expect("register read " + 'z%i' %
+(i), substrs=[z_regs_value])
+
+p_value_bytes = ['0xff', '0x55', '0x11', '0x01', '0x00']
+for i in range(16):
+p_regs_value = '{' + \
+' '.join(p_value_bytes[i % 5] for _ in range(p_reg_size)) + '}'
+self.expect("register read " + 'p%i' % (i), substrs=[p_regs_value])
+
+self.expect("register read ffr", substrs=[p_regs_value])
+
+def check_sve_regs_read_after_write(self, z_reg_size):
+p_reg_size = int(z_reg_size / 8)
+
+z_regs_value = '{' + \
+' '.join(('0x9d' for _ in range(z_reg_size))) + '}'
+
+p_regs_value = '{' + \
+' '.join(('0xee' for _ in range(p_reg_size))) + '}'
+
+for i in range(32):
+self.runCmd('register write ' + 'z%i' %
+(i) + " '" + z_regs_value + "'")
+
+for i in range(32):
+self.expect("register read " + 'z%i' % (i), substrs=[z_regs_value])
+
+for i in range(16):
+self.runCmd('register write ' + 'p%i' %
+(i) + " '" + p_regs_value + "'")
+
+for i in range(16):
+self.expect("register read " + 'p%i' % (i), substrs=[p_regs_value])
+
+self.runCmd('register write ' + 'ffr ' + "'" + p_regs_value + "'")
+
+self.expect("register read " + 'ffr', substrs=[p_regs_value])
+
 mydir = TestBase.compute_mydir(__file__)
 
 @no_debug_info_test
@@ -117,43 +162,17 @@
 
 z_reg_size = vg_reg_value * 8
 
-p_reg_size = int(z_reg_size / 8)
-
-for i in range(32):
-z_regs_value = '{' + \
-' '.join('0x{:02x}'.format(i + 1)
- for _ in range(z_reg_size)) + '}'
-self.expect("register read " + 'z%i' %
-(i), substrs=[z_regs_value])
+self.check_sve_regs_read(z_reg_size)
 
-p_value_bytes = ['0xff', '0x55', '0x11', '0x01', '0x00']
-for i in range(16):
-p_regs_value = '{' + \
-' '.join(p_value_bytes[i % 5] for _ in range(p

[Lldb-commits] [PATCH] D109495: [lldb] [test] Add synchronization fix Subprocess test flakiness

2021-09-09 Thread Michał Górny via Phabricator via lldb-commits

mgorny updated this revision to Diff 371554.
mgorny added a comment.

Remove unnecessary `child_done` pipe, as pointed out by @labath.


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

https://reviews.llvm.org/D109495

Files:
  lldb/test/Shell/Subprocess/Inputs/fork.cpp
  lldb/test/Shell/Subprocess/clone-follow-child-softbp.test
  lldb/test/Shell/Subprocess/clone-follow-child-wp.test
  lldb/test/Shell/Subprocess/clone-follow-child.test
  lldb/test/Shell/Subprocess/clone-follow-parent-softbp.test
  lldb/test/Shell/Subprocess/clone-follow-parent-wp.test
  lldb/test/Shell/Subprocess/clone-follow-parent.test
  lldb/test/Shell/Subprocess/fork-follow-child-softbp.test
  lldb/test/Shell/Subprocess/fork-follow-child-wp.test
  lldb/test/Shell/Subprocess/fork-follow-child.test
  lldb/test/Shell/Subprocess/fork-follow-parent-softbp.test
  lldb/test/Shell/Subprocess/fork-follow-parent-wp.test
  lldb/test/Shell/Subprocess/fork-follow-parent.test
  lldb/test/Shell/Subprocess/vfork-follow-child-softbp.test
  lldb/test/Shell/Subprocess/vfork-follow-child-wp.test
  lldb/test/Shell/Subprocess/vfork-follow-child.test
  lldb/test/Shell/Subprocess/vfork-follow-parent-softbp.test
  lldb/test/Shell/Subprocess/vfork-follow-parent-wp.test
  lldb/test/Shell/Subprocess/vfork-follow-parent.test

Index: lldb/test/Shell/Subprocess/vfork-follow-parent.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-parent.test
+++ lldb/test/Shell/Subprocess/vfork-follow-parent.test
@@ -8,4 +8,5 @@
 # CHECK: stop reason = breakpoint
 continue
 # CHECK: function run in parent
-# CHECK: child exited: 0
+# CHECK: function run in exec'd child
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/test/Shell/Subprocess/vfork-follow-parent-wp.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-parent-wp.test
+++ lldb/test/Shell/Subprocess/vfork-follow-parent-wp.test
@@ -11,4 +11,5 @@
 # CHECK: stop reason = watchpoint
 continue
 # CHECK: function run in parent
-# CHECK: child exited: 0
+# CHECK: function run in exec'd child
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/test/Shell/Subprocess/vfork-follow-parent-softbp.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-parent-softbp.test
+++ lldb/test/Shell/Subprocess/vfork-follow-parent-softbp.test
@@ -10,4 +10,5 @@
 # CHECK: stop reason = breakpoint
 continue
 # CHECK: function run in parent
-# CHECK: child exited: 0
+# CHECK: function run in exec'd child
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/test/Shell/Subprocess/vfork-follow-child.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-child.test
+++ lldb/test/Shell/Subprocess/vfork-follow-child.test
@@ -1,12 +1,12 @@
 # REQUIRES: native
 # UNSUPPORTED: system-darwin
 # UNSUPPORTED: system-windows
-# This test is very flaky on aarch64.
-# UNSUPPORTED: system-linux && target-aarch64
 # RUN: %clangxx_host %p/Inputs/fork.cpp -DTEST_FORK=vfork -o %t
 # RUN: %lldb -b -s %s %t | FileCheck %s
 settings set target.process.follow-fork-mode child
+settings set target.process.stop-on-exec false
 b parent_func
 process launch
 # CHECK: function run in parent
-# CHECK: child exited: 0
+# CHECK: function run in exec'd child
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/test/Shell/Subprocess/vfork-follow-child-wp.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-child-wp.test
+++ lldb/test/Shell/Subprocess/vfork-follow-child-wp.test
@@ -1,13 +1,14 @@
 # REQUIRES: native && dbregs-set
 # UNSUPPORTED: system-darwin
 # UNSUPPORTED: system-windows
-# This test is very flaky on aarch64.
-# UNSUPPORTED: system-linux && target-aarch64
 # RUN: %clangxx_host -g %p/Inputs/fork.cpp -DTEST_FORK=vfork -o %t
 # RUN: %lldb -b -s %s %t | FileCheck %s
 settings set target.process.follow-fork-mode child
+settings set target.process.stop-on-exec false
 process launch -s
 watchpoint set variable -w write g_val
 # CHECK: Watchpoint created:
 continue
-# CHECK: child exited: 0
+# CHECK: function run in parent
+# CHECK: function run in exec'd child
+# CHECK: Process {{[0-9]+}} exited with status = 0
Index: lldb/test/Shell/Subprocess/vfork-follow-child-softbp.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-child-softbp.test
+++ lldb/test/Shell/Subprocess/vfork-follow-child-softbp.test
@@ -1,13 +1,13 @@
 # REQUIRES: native
 # UNSUPPORTED: system-darwin
 # UNSUPPORTED: system-windows
-# This test is very flaky on aarch64.
-# UNSUPPORTED: system-linux && target-aarch64
 # RUN: %clangxx_host %p/Inputs/fork.cpp -DTEST_FORK=vfork -o %t
 # RUN: %lldb -b -s %s %t | FileCheck %s
 settings set target.process.follow-fork-mode chi

[Lldb-commits] [lldb] cda1450 - [lldb][NFC] Add some tests for function-local classes and document some bugs

2021-09-09 Thread Raphael Isemann via lldb-commits


Author: Raphael Isemann
Date: 2021-09-09T14:12:02+02:00
New Revision: cda1450f1c771712718ef3585315f0ecbb708d8d

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

LOG: [lldb][NFC] Add some tests for function-local classes and document some 
bugs

This feature doesn't seem to have any dedicated test. Instead some random tests
(e.g. the bitfield tests) are declaring function-local classes for some reason.
This adds a dedicated test so we can clean up those other tests.

Also add FIXME's for some basic stuff that doesn't work. The first FIXME is a
good beginner bug which just requires prepending the function name (in case we
decide to fix it instead of documenting this behaviour). The second FIXME is
caused by LLDB searching for definitions by name (which also seems to miss the
function name so there is a conflict with the outer type).

Some more things that should be tested (and might not work):
* Local classes with member functions with local classes.
* Classes in different functions with same name.
* Classes with the same name in different TUs with internal linkage functions of
  the same name.
* Empty classes are parsed by the DWARF parser in a fast path, so that requires
  dedicated tests.
* Repeat some of the tested logic for C.

Added: 
lldb/test/API/lang/cpp/function-local-class/Makefile
lldb/test/API/lang/cpp/function-local-class/TestCppFunctionLocalClass.py
lldb/test/API/lang/cpp/function-local-class/main.cpp

Modified: 


Removed: 




diff  --git a/lldb/test/API/lang/cpp/function-local-class/Makefile 
b/lldb/test/API/lang/cpp/function-local-class/Makefile
new file mode 100644
index 0..8b20bcb05
--- /dev/null
+++ b/lldb/test/API/lang/cpp/function-local-class/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

diff  --git 
a/lldb/test/API/lang/cpp/function-local-class/TestCppFunctionLocalClass.py 
b/lldb/test/API/lang/cpp/function-local-class/TestCppFunctionLocalClass.py
new file mode 100644
index 0..dfbbfb8ad04c5
--- /dev/null
+++ b/lldb/test/API/lang/cpp/function-local-class/TestCppFunctionLocalClass.py
@@ -0,0 +1,58 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@no_debug_info_test
+def test(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// break here", 
lldb.SBFileSpec("main.cpp"))
+
+m_val = self.expect_expr("m", result_type="WithMember", 
result_children=[
+ValueCheck(name="i", value="1")
+])
+# FIXME: The non-display name doesn't include the function, so users
+# can't actually match specific classes by their name. Either document
+# or fix this.
+self.assertEqual(m_val.GetType().GetName(), "WithMember")
+# Try accessing the type in the expression evaluator.
+self.expect_expr("m.i", result_type="int", result_value="1")
+
+self.expect_expr("typedef_unnamed", result_type="TypedefUnnamed", 
result_children=[
+ValueCheck(name="a", value="2")
+])
+self.expect_expr("typedef_unnamed2", result_type="TypedefUnnamed2", 
result_children=[
+ValueCheck(name="b", value="3")
+])
+self.expect_expr("unnamed", result_type="(unnamed struct)", 
result_children=[
+ValueCheck(name="i", value="4")
+])
+self.expect_expr("unnamed2", result_type="(unnamed struct)", 
result_children=[
+ValueCheck(name="j", value="5")
+])
+
+# Try a class that is only forward declared.
+self.expect_expr("fwd", result_type="Forward *")
+self.expect("expression -- fwd->i", error=True, substrs=[
+"member access into incomplete type 'Forward'"
+])
+self.expect("expression -- *fwd", error=True, substrs=[
+"incomplete type 'Forward' where a complete type is required"
+])
+
+# Try a class that has a name that matches a class in the global scope.
+self.expect_expr("fwd_conflict", result_type="ForwardConflict *")
+# FIXME: This pulls in the unrelated type with the same name from the
+# global scope.
+self.expect("expression -- fwd_conflict->i", error=True, substrs=[
+# This should point out that ForwardConflict is incomplete.
+"no member named 'i' in 'ForwardConflict'"
+])
+self.expect("expression -- *fwd_conflict", error=True, substrs=[
+# This should fail to parse instead.
+"couldn't read its memory"
+])

diff  --git a/lldb/test/API/lang/cpp/function-local-class/main.cpp 
b/lldb/test/API/la

[Lldb-commits] [PATCH] D109495: [lldb] [test] Add synchronization fix Subprocess test flakiness

2021-09-09 Thread Michał Górny via Phabricator via lldb-commits

mgorny updated this revision to Diff 371571.
mgorny added a comment.

Force sync earlier for `fork()` and `clone()` (since we don't have to wait for 
exec there). Avoid depending on ordering between child output and LLDB exit.

Stress-testing it now.


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

https://reviews.llvm.org/D109495

Files:
  lldb/test/Shell/Subprocess/Inputs/fork.cpp
  lldb/test/Shell/Subprocess/clone-follow-child-softbp.test
  lldb/test/Shell/Subprocess/clone-follow-child-wp.test
  lldb/test/Shell/Subprocess/clone-follow-child.test
  lldb/test/Shell/Subprocess/clone-follow-parent-softbp.test
  lldb/test/Shell/Subprocess/clone-follow-parent-wp.test
  lldb/test/Shell/Subprocess/clone-follow-parent.test
  lldb/test/Shell/Subprocess/fork-follow-child-softbp.test
  lldb/test/Shell/Subprocess/fork-follow-child-wp.test
  lldb/test/Shell/Subprocess/fork-follow-child.test
  lldb/test/Shell/Subprocess/fork-follow-parent-softbp.test
  lldb/test/Shell/Subprocess/fork-follow-parent-wp.test
  lldb/test/Shell/Subprocess/fork-follow-parent.test
  lldb/test/Shell/Subprocess/vfork-follow-child-softbp.test
  lldb/test/Shell/Subprocess/vfork-follow-child-wp.test
  lldb/test/Shell/Subprocess/vfork-follow-child.test
  lldb/test/Shell/Subprocess/vfork-follow-parent-softbp.test
  lldb/test/Shell/Subprocess/vfork-follow-parent-wp.test
  lldb/test/Shell/Subprocess/vfork-follow-parent.test

Index: lldb/test/Shell/Subprocess/vfork-follow-parent.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-parent.test
+++ lldb/test/Shell/Subprocess/vfork-follow-parent.test
@@ -8,4 +8,4 @@
 # CHECK: stop reason = breakpoint
 continue
 # CHECK: function run in parent
-# CHECK: child exited: 0
+# CHECK: function run in exec'd child
Index: lldb/test/Shell/Subprocess/vfork-follow-parent-wp.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-parent-wp.test
+++ lldb/test/Shell/Subprocess/vfork-follow-parent-wp.test
@@ -11,4 +11,4 @@
 # CHECK: stop reason = watchpoint
 continue
 # CHECK: function run in parent
-# CHECK: child exited: 0
+# CHECK: function run in exec'd child
Index: lldb/test/Shell/Subprocess/vfork-follow-parent-softbp.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-parent-softbp.test
+++ lldb/test/Shell/Subprocess/vfork-follow-parent-softbp.test
@@ -10,4 +10,4 @@
 # CHECK: stop reason = breakpoint
 continue
 # CHECK: function run in parent
-# CHECK: child exited: 0
+# CHECK: function run in exec'd child
Index: lldb/test/Shell/Subprocess/vfork-follow-child.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-child.test
+++ lldb/test/Shell/Subprocess/vfork-follow-child.test
@@ -1,12 +1,11 @@
 # REQUIRES: native
 # UNSUPPORTED: system-darwin
 # UNSUPPORTED: system-windows
-# This test is very flaky on aarch64.
-# UNSUPPORTED: system-linux && target-aarch64
 # RUN: %clangxx_host %p/Inputs/fork.cpp -DTEST_FORK=vfork -o %t
 # RUN: %lldb -b -s %s %t | FileCheck %s
 settings set target.process.follow-fork-mode child
+settings set target.process.stop-on-exec false
 b parent_func
 process launch
 # CHECK: function run in parent
-# CHECK: child exited: 0
+# CHECK: function run in exec'd child
Index: lldb/test/Shell/Subprocess/vfork-follow-child-wp.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-child-wp.test
+++ lldb/test/Shell/Subprocess/vfork-follow-child-wp.test
@@ -1,13 +1,13 @@
 # REQUIRES: native && dbregs-set
 # UNSUPPORTED: system-darwin
 # UNSUPPORTED: system-windows
-# This test is very flaky on aarch64.
-# UNSUPPORTED: system-linux && target-aarch64
 # RUN: %clangxx_host -g %p/Inputs/fork.cpp -DTEST_FORK=vfork -o %t
 # RUN: %lldb -b -s %s %t | FileCheck %s
 settings set target.process.follow-fork-mode child
+settings set target.process.stop-on-exec false
 process launch -s
 watchpoint set variable -w write g_val
 # CHECK: Watchpoint created:
 continue
-# CHECK: child exited: 0
+# CHECK: function run in parent
+# CHECK: function run in exec'd child
Index: lldb/test/Shell/Subprocess/vfork-follow-child-softbp.test
===
--- lldb/test/Shell/Subprocess/vfork-follow-child-softbp.test
+++ lldb/test/Shell/Subprocess/vfork-follow-child-softbp.test
@@ -1,13 +1,12 @@
 # REQUIRES: native
 # UNSUPPORTED: system-darwin
 # UNSUPPORTED: system-windows
-# This test is very flaky on aarch64.
-# UNSUPPORTED: system-linux && target-aarch64
 # RUN: %clangxx_host %p/Inputs/fork.cpp -DTEST_FORK=vfork -o %t
 # RUN: %lldb -b -s %s %t | FileCheck %s
 settings set target.process.follow-fork-mode child
+settings set target.process.stop-on-exec false
 b child_func
 b parent_func
 process launch
 # CHECK: function run in parent
-# CHECK: child

[Lldb-commits] [PATCH] D108061: [lldb] Add support for shared library load when executable called through ld.

2021-09-09 Thread Pavel Labath via Phabricator via lldb-commits

labath added inline comments.



Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:153
+cursor = info_addr =
+ResolveRendezvousAddress(m_process, m_executable_interpreter);
   else

At this point, you can just make `ResolveRendezvousAddress` a (private) member 
of the DYLDRendezvous class.



Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:318
+if (!SOEntryIsMainExecutable(entry)) {
+  if (entry.file_spec.GetFilename().IsEmpty())
+UpdateFileSpecIfNecessary(entry);

Since the function already has `IfNecessary` in the name, you don't need to 
guard the call once more.



Comment at: 
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:453-455
+// When executable is run with argv[0] as ld.so, ld.so corresponds to
+// m_exe_file_spec. In this case, the returned link map
+// has empty entry for executable, which is not the main executable.

How about this:
```
// If were debugging ld.so, then all SOEntries should be treated as libraries, 
including the "main" one (denoted by an empty string). 
if (m_executable_interpreter)
  return false;
return !entry.file_spec;



Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp:552
+  // When argv[0] is ld.so, we need to be update executable path.
+  if (entry.file_spec.GetFilename().IsEmpty()) {
+MemoryRegionInfo region;

I don't think this should make a difference, but `if(!entry.file_spec)` is 
shorter and consistent with the logic in `SOEntryIsMainExecutable`.



Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h:186
 
+  /// It is set to true when executable is run with argv[0] as ld.so.
+  bool m_executable_interpreter;

I get what you're trying to say, but I'm having issues with these comments, as 
they're wrong on multiple levels. For one, argv[0] essentially has no 
relationship to the binary being executed (ld.so even has an argument to supply 
an arbitrary string as argv[0] to the executable). But more importantly, when 
the executable is "run" in this way (as an argument to ld.so), as far as the 
operating system (and lldb) is concerned ld.so _is_ the main executable. You 
can see this in the /proc/$PID/exe symlink, or in all the 
target->GetExecutableModule calls in this patch. What this patch actually does 
is to _disable_ the logic of treating the "main executable" (the one with an 
empty name in the rendezvous structure) as special, so that it can be treated 
as one of the libraries.

With that in mind, I think it'd be better to say that this flag indicates 
whether the main program is the dynamic linker/loader/program interpreter. 



Comment at: 
lldb/test/API/functionalities/dyld-launch-linux/TestDyldLaunchLinux.py:18-24
+candidates = [
+"/lib64/ld-linux-x86-64.so.2",
+"/usr/lib/ld-linux-x86-64.so.2"
+]
+exe = next((c for c in candidates if os.path.exists(c)), None)
+if not os.path.exists(exe):
+return

It should be possible to retrieve the actual interpreter path by inspecting the 
compiled executable. Something like 
`SBModule(exe)->FindSection(".interp")->GetSectionData()` ought to do it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108061

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

[Lldb-commits] [PATCH] D96766: [lldb] [Process/FreeBSD] Introduce mips64 FPU reg support

2021-09-09 Thread Michał Górny via Phabricator via lldb-commits

mgorny updated this revision to Diff 371579.
mgorny marked 3 inline comments as done.
mgorny added a comment.

Rebased after Linux/MIPS removal.


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

https://reviews.llvm.org/D96766

Files:
  lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_mips64.cpp
  lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_mips64.h
  lldb/source/Plugins/Process/Utility/RegisterContextFreeBSD_mips64.cpp
  lldb/source/Plugins/Process/Utility/RegisterInfos_mips64.h
  lldb/source/Plugins/Process/Utility/lldb-mips-freebsd-register-enums.h
  lldb/unittests/Process/Utility/RegisterContextFreeBSDTest.cpp

Index: lldb/unittests/Process/Utility/RegisterContextFreeBSDTest.cpp
===
--- lldb/unittests/Process/Utility/RegisterContextFreeBSDTest.cpp
+++ lldb/unittests/Process/Utility/RegisterContextFreeBSDTest.cpp
@@ -409,6 +409,11 @@
   EXPECT_THAT(GetRegParams(reg_ctx, gpr_##lldb_reg##_mips64),  \
   ::testing::Pair(offsetof(reg, r_regs[fbsd_regno]),   \
   sizeof(reg::r_regs[fbsd_regno])))
+#define EXPECT_FPU_MIPS64(lldb_reg, fbsd_regno)\
+  EXPECT_THAT( \
+  GetRegParams(reg_ctx, fpr_##lldb_reg##_mips64),  \
+  ::testing::Pair(offsetof(fpreg, r_regs[fbsd_regno]) + base_offset,   \
+  sizeof(fpreg::r_regs[fbsd_regno])))
 
 TEST(RegisterContextFreeBSDTest, mips64) {
   ArchSpec arch{"mips64-unknown-freebsd"};
@@ -457,6 +462,10 @@
   EXPECT_GPR_MIPS64(pc, 37);
   EXPECT_GPR_MIPS64(ic, 38);
   EXPECT_GPR_MIPS64(dummy, 39);
+
+  size_t base_offset = reg_ctx.GetRegisterInfo()[fpr_f0_mips64].byte_offset;
+
+  EXPECT_FPU_MIPS64(f0, 0);
 }
 
 #endif // defined(__mips64__)
Index: lldb/source/Plugins/Process/Utility/lldb-mips-freebsd-register-enums.h
===
--- lldb/source/Plugins/Process/Utility/lldb-mips-freebsd-register-enums.h
+++ lldb/source/Plugins/Process/Utility/lldb-mips-freebsd-register-enums.h
@@ -57,9 +57,47 @@
   gpr_dummy_mips64,
   k_last_gpr_mips64 = gpr_dummy_mips64,
 
+  k_first_fpr_mips64,
+  fpr_f0_mips64 = k_first_fpr_mips64,
+  fpr_f1_mips64,
+  fpr_f2_mips64,
+  fpr_f3_mips64,
+  fpr_f4_mips64,
+  fpr_f5_mips64,
+  fpr_f6_mips64,
+  fpr_f7_mips64,
+  fpr_f8_mips64,
+  fpr_f9_mips64,
+  fpr_f10_mips64,
+  fpr_f11_mips64,
+  fpr_f12_mips64,
+  fpr_f13_mips64,
+  fpr_f14_mips64,
+  fpr_f15_mips64,
+  fpr_f16_mips64,
+  fpr_f17_mips64,
+  fpr_f18_mips64,
+  fpr_f19_mips64,
+  fpr_f20_mips64,
+  fpr_f21_mips64,
+  fpr_f22_mips64,
+  fpr_f23_mips64,
+  fpr_f24_mips64,
+  fpr_f25_mips64,
+  fpr_f26_mips64,
+  fpr_f27_mips64,
+  fpr_f28_mips64,
+  fpr_f29_mips64,
+  fpr_f30_mips64,
+  fpr_f31_mips64,
+  fpr_fcsr_mips64,
+  fpr_fir_mips64,
+  k_last_fpr_mips64 = fpr_fir_mips64,
+
   k_num_registers_mips64,
 
-  k_num_gpr_registers_mips64 = k_last_gpr_mips64 - k_first_gpr_mips64 + 1
+  k_num_gpr_registers_mips64 = k_last_gpr_mips64 - k_first_gpr_mips64 + 1,
+  k_num_fpr_registers_mips64 = k_last_fpr_mips64 - k_first_fpr_mips64 + 1,
 };
-}
+} // namespace lldb_private
 #endif // LLDB_SOURCE_PLUGINS_PROCESS_UTILITY_LLDB_MIPS_FREEBSD_REGISTER_ENUMS_H
Index: lldb/source/Plugins/Process/Utility/RegisterInfos_mips64.h
===
--- lldb/source/Plugins/Process/Utility/RegisterInfos_mips64.h
+++ lldb/source/Plugins/Process/Utility/RegisterInfos_mips64.h
@@ -11,12 +11,16 @@
 #include "lldb/Core/dwarf.h"
 #include "llvm/Support/Compiler.h"
 
-
 #ifdef DECLARE_REGISTER_INFOS_MIPS64_STRUCT
 
 // Computes the offset of the given GPR in the user data area.
 #define GPR_OFFSET(regname) (LLVM_EXTENSION offsetof(GPR_freebsd_mips, regname))
 
+// Computes the offset of the given FPR in the extended data area.
+#define FPR_OFFSET(regname)\
+  (sizeof(GPR_freebsd_mips) +  \
+   LLVM_EXTENSION offsetof(FPR_freebsd_mips, regname))
+
 // RegisterKind: EHFrame, DWARF, Generic, Process Plugin, LLDB
 
 // Note that the size and offset will be updated by platform-specific classes.
@@ -29,6 +33,31 @@
   NULL, NULL, NULL, 0  \
   }
 
+const uint8_t dwarf_opcode_mips64[] = {
+llvm::dwarf::DW_OP_regx,  dwarf_sr_mips64,llvm::dwarf::DW_OP_lit1,
+llvm::dwarf::DW_OP_lit26, llvm::dwarf::DW_OP_shl, llvm::dwarf::DW_OP_and,
+llvm::dwarf::DW_OP_lit26, llvm::dwarf::DW_OP_shr};
+
+#define DEFINE_FPR(reg, alt, kind1, kind2, kind3)  \
+  {\
+#reg, alt, sizeof(((FPR_freebsd_mips *) 0)->reg),

[Lldb-commits] [PATCH] D109495: [lldb] [test] Add synchronization fix Subprocess test flakiness

2021-09-09 Thread Michał Górny via Phabricator via lldb-commits

mgorny added a comment.

With this version, I haven't been able to reproduce a test failure in 350 
iterations, so I guess it's good now.


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

https://reviews.llvm.org/D109495

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

[Lldb-commits] [PATCH] D96766: [lldb] [Process/FreeBSD] Introduce mips64 FPU reg support

2021-09-09 Thread Kamil Rytarowski via Phabricator via lldb-commits

krytarowski accepted this revision.
krytarowski added a comment.

Looks still fine.


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

https://reviews.llvm.org/D96766

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

[Lldb-commits] [PATCH] D109508: [lldb] Fix format string in Communication::Write

2021-09-09 Thread Ryan Mansfield via Phabricator via lldb-commits

rmansfield created this revision.
rmansfield added a reviewer: JDevlieghere.
rmansfield added a project: LLDB.
rmansfield requested review of this revision.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109508

Files:
  lldb/source/Core/Communication.cpp


Index: lldb/source/Core/Communication.cpp
===
--- lldb/source/Core/Communication.cpp
+++ lldb/source/Core/Communication.cpp
@@ -176,8 +176,8 @@
 
   std::lock_guard guard(m_write_mutex);
   LLDB_LOG(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_COMMUNICATION),
-   "{0} Communication::Write (src = {1}, src_len = %" PRIu64
-   ") connection = {2}",
+   "{0} Communication::Write (src = {1}, src_len = {2}"
+   ") connection = {3}",
this, src, (uint64_t)src_len, connection_sp.get());
 
   if (connection_sp)


Index: lldb/source/Core/Communication.cpp
===
--- lldb/source/Core/Communication.cpp
+++ lldb/source/Core/Communication.cpp
@@ -176,8 +176,8 @@
 
   std::lock_guard guard(m_write_mutex);
   LLDB_LOG(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_COMMUNICATION),
-   "{0} Communication::Write (src = {1}, src_len = %" PRIu64
-   ") connection = {2}",
+   "{0} Communication::Write (src = {1}, src_len = {2}"
+   ") connection = {3}",
this, src, (uint64_t)src_len, connection_sp.get());
 
   if (connection_sp)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D109508: [lldb] Fix format string in Communication::Write

2021-09-09 Thread Raphael Isemann via Phabricator via lldb-commits

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

Thanks, LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109508

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

[Lldb-commits] [PATCH] D109508: [lldb] Fix format string in Communication::Write

2021-09-09 Thread Ryan Mansfield via Phabricator via lldb-commits

rmansfield added a comment.

Thanks. Could someone please apply on my behalf?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109508

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

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

2021-09-09 Thread Duncan P. N. Exon Smith via Phabricator via lldb-commits

dexonsmith added a comment.

This seems like the right direction to me! Especially like the 
look-through-the-ErrorInfo change for `FileError` -- I hit this before and 
found it annoying.

IMO, it'd be valuable to split out the consequential functional changes:

- Improvements/changes you made to FileError could be committed ahead of time
- Other improvements you discussed to avoid regressions in (e.g.) 
llvm-symbolizer (seems potentially important?)

I agree the other changes are mostly mechanical and don't all need careful 
review-by-subcomponents.

That said, it looks very painful for downstream clients of the LLVM C++ API 
since it's structured as an all-or-nothing change. Especially for managing 
cherry-picks to long-lived stable branches. It's painful because clients will 
have code like this:

  if (auto MaybeFile = MemoryBuffer::getFileOrSTDIN()) {
// Do something with MaybeFile
  }
  // Else path doesn't care about the specific error code.

that will suddenly start crashing at runtime. I even wonder if there like that 
introduced in-tree by your current all-in-one patch, since I think our 
filesystem-error paths are often missing test coverage. (It'd be difficult to 
do a thorough audit.)

I thought through a potential staging that could mitigate the pain:

1. Add `using MemoryBufferCompat = MemoryBuffer` and search/replace all static 
`MemoryBuffer::` API calls over to `MemoryBufferCompat::`. No direct impact on 
downstreams.
2. Change `MemoryBuffer` to use `Error` and `Expected`, leaving behind 
`std::error_code` and `ErrorOr` wrappers in a no-longer-just-a-typedef 
`MemoryBufferCompat`. Easy for downstreams to temporarily revert, and 
cherry-pick once they've finished adapting in the example of (1).
3. Update all code to use the new APIs. Could be done all at once since it's 
mostly mechanical, as you said. Also an option to split up by component (e.g., 
maybe the llvm-symbolizer regression is okay, but it could be nice to get that 
reviewed separately / in isolation).
4. Delete `MemoryBufferCompat`. Downstreams can temporarily revert while they 
follow the example of on (3).

(Given that (1) and (2) are easy to write, you already have `tree` state for 
(4), and (3) is easy to create from (4), then I //think// you could construct 
the above commit sequence without having to redo all the work... then if you 
wanted to split (3) up from there it'd be easy enough.)

(2) seems like the commit mostly likely to cause functional regressions, and 
it'd be isolated and easy to revert/reapply (downstream and/or upstream) 
without much churn.

WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109345

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

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

2021-09-09 Thread David Blaikie via Phabricator via lldb-commits

dblaikie added a comment.

In D109345#2986297 , @thopre wrote:

> Is there no way to split this patch further? It's going to be hard finding 
> someone who can review something so big. If there's no way to split it in 
> incremental changes, you could perhaps split per subsystem only for review 
> and refer to this diff for CI as well as when landing.

The long migration path would be to do this one function at. time (I did a 
whole cluster of functions in MemoryBuffer for consistency - this does reduce 
total code changes somewhat, since some of those APIs are used in similar 
contexts (eg: branches of a conditional operator - so having them differ means 
more revisions to those call sites)) and probably introducing separate names 
for the Expected versions of the functions, migrating call sites incrementally, 
then doing a mechanical rename at the end of all that.

I don't think it's probably worth that level of granularity - it's a fairly 
mechanical patch as it is.

Mostly I sent this out as an FYI and to get feedback on the general direction - 
whether folks thought it was worth doing at all, and if they feel strongly it 
should be done another way (like the incremental ones above) - but I don't 
think it needs a /huge/ amount of scrutiny, review by separate code owners, 
etc. I'd generally be comfortable committing this as other cross-project 
cleanup/refactoring like function renaming, etc.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109345

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

[Lldb-commits] [PATCH] D109185: [gn build] Add build files for LLDB

2021-09-09 Thread Nico Weber via Phabricator via lldb-commits

thakis added a comment.

In D109185#2988837 , @teemperor wrote:

> Thanks! Out of curiosity, is there a public GN bot that is testing this?

The GN bots at http://45.33.8.238/ do the compile step. They don't yet run lldb 
tests. Maybe I'll add that, but maybe I'll wait until more of the test suite is 
brought up.

>> LLDB has many dependency cycles, something GN doesn't allow. For that 
>> reason, I've omitted some dependency edges. Hopefully we can clean up the 
>> cycles one day.
>
> No idea about GN, but I would assume this would break the build in some form? 
> Or does this just mean ninja doesn't know the dependencies and the build 
> process has a race condition?

The CMake build works around this with the `set_target_properties(lldbCore 
PROPERTIES LINK_INTERFACE_MULTIPLICITY 5)` hack in 
lldb/source/Core/CMakeLists.txt.

In the GN build, it means you can't really build the libraries independently. 
But the binaries build fine (…at least with lld and ld64. It's possible BFD ld 
needs a --start-group / --end-group around libraries? Haven't tried that yet.)

>> LLDB has a public/private header distinction, but mostly ignores it.
>> Many libraries include private headers from other modules.
>
> Is this referring to some `private-*.h` `public-*.h` files or what are those 
> public/private headers that are incorrectly included?

LLDB has lldb/include/lldb/Foo which (like in the other LLVM projects) are 
supposed to define a module's public interface from what I understand, while 
headers in lldb/source/Foo are Foo's internal headers. At least that's how it 
works in clang and llvm. In lldb, many modules include another module's private 
headers (in that sense that they're in lldb/source/Foo instead of in 
lldb/include/lldb/Foo), while that's much less common in llvm and clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109185

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

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

2021-09-09 Thread Fangrui Song via Phabricator via lldb-commits

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

`ninja clang` on Windows works. `check-llvm-tools` has a few `REQUIRES: 
system-windows` tests and I am running them. Test invocation is bit slow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109345

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

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

2021-09-09 Thread David Blaikie via Phabricator via lldb-commits

dblaikie added a comment.

In D109345#2987565 , @dexonsmith 
wrote:

> This seems like the right direction to me! Especially like the 
> look-through-the-ErrorInfo change for `FileError` -- I hit this before and 
> found it annoying.

Thanks for taking a look!

> IMO, it'd be valuable to split out the consequential functional changes:
>
> - Improvements/changes you made to FileError could be committed ahead of time

Sure sure, can be committed and unit tested separately for sure.

> - Other improvements you discussed to avoid regressions in (e.g.) 
> llvm-symbolizer (seems potentially important?)

I didn't think the yaml symbolizer output was that important - but turned out 
not to be super hard to fix, so I've done that. (were there other regressions I 
mentioned/should think about?)

> I agree the other changes are mostly mechanical and don't all need careful 
> review-by-subcomponents.
>
> That said, it looks very painful for downstream clients of the LLVM C++ API 
> since it's structured as an all-or-nothing change.

Yeah, certainly crossed my mind.

> Especially for managing cherry-picks to long-lived stable branches. It's 
> painful because clients will have code like this:
>
>   if (auto MaybeFile = MemoryBuffer::getFileOrSTDIN()) {
> // Do something with MaybeFile
>   }
>   // Else path doesn't care about the specific error code.
>
> that will suddenly start crashing at runtime. I even wonder if there like 
> that introduced in-tree by your current all-in-one patch, since I think our 
> filesystem-error paths are often missing test coverage. (It'd be difficult to 
> do a thorough audit.)

Yeah, that came up in a handful of cases that used 'auto' (without using auto 
it's a compile failure).

> I thought through a potential staging that could mitigate the pain:
>
> 1. Add `using MemoryBufferCompat = MemoryBuffer` and search/replace all 
> static `MemoryBuffer::` API calls over to `MemoryBufferCompat::`. No direct 
> impact on downstreams.

Yeah, that's some of the extra churn I was thinking of/hoping to avoid - but 
yeah, it's probably worthwhile.

What's the benefit of having the extra step where everything's renamed twice? 
(if it's going to be a monolithic commit - as mentioned in (3)) Compared to 
doing the mass change while keeping the (1 & 2) pieces for backwards 
compatibility if needed?

> 2. Change `MemoryBuffer` to use `Error` and `Expected`, leaving behind 
> `std::error_code` and `ErrorOr` wrappers in a no-longer-just-a-typedef 
> `MemoryBufferCompat`. Easy for downstreams to temporarily revert, and 
> cherry-pick once they've finished adapting in the example of (1).
> 3. Update all code to use the new APIs. Could be done all at once since it's 
> mostly mechanical, as you said. Also an option to split up by component 
> (e.g., maybe the llvm-symbolizer regression is okay, but it could be nice to 
> get that reviewed separately / in isolation).
> 4. Delete `MemoryBufferCompat`. Downstreams can temporarily revert while they 
> follow the example of on (3).
>
> (Given that (1) and (2) are easy to write, you already have `tree` state for 
> (4), and (3) is easy to create from (4), then I //think// you could construct 
> the above commit sequence without having to redo all the work... then if you 
> wanted to split (3) up from there it'd be easy enough.)
>
> (2) seems like the commit mostly likely to cause functional regressions, and 
> it'd be isolated and easy to revert/reapply (downstream and/or upstream) 
> without much churn.

(3) would be more likely to cause regression? Presumably (2) is really 
uninteresting because it adds a new API (re-adding MemoryBuffer, but unused 
since everything's using MemoryBufferCompat) without any usage, yeah?

Plausible, though a fair bit more churn - I'd probably be up for it, though.

- Dave


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109345

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

[Lldb-commits] [PATCH] D109185: [gn build] Add build files for LLDB

2021-09-09 Thread Raphael Isemann via Phabricator via lldb-commits

teemperor added a comment.

Thanks! Out of curiosity, is there a public GN bot that is testing this?

> LLDB has many dependency cycles, something GN doesn't allow. For

that reason, I've omitted some dependency edges. Hopefully we can
clean up the cycles one day.

No idea about GN, but I would assume this would break the build in some form? 
Or does this just mean ninja doesn't know the dependencies and the build 
process has a race condition?

> LLDB has a public/private header distinction, but mostly ignores it.
> Many libraries include private headers from other modules.

Is this referring to some `private-*.h` `public-*.h` files or what are those 
public/private headers that are incorrectly included?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109185

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

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

2021-09-09 Thread Duncan P. N. Exon Smith via Phabricator via lldb-commits

dexonsmith added a comment.

In D109345#2990527 , @dblaikie wrote:

> (were there other regressions I mentioned/should think about?)

I don't have specific concerns; I was just reading between the lines of your 
description...

>> 1. Add `using MemoryBufferCompat = MemoryBuffer` and search/replace all 
>> static `MemoryBuffer::` API calls over to `MemoryBufferCompat::`. No direct 
>> impact on downstreams.
>
> Yeah, that's some of the extra churn I was thinking of/hoping to avoid - but 
> yeah, it's probably worthwhile.
>
> What's the benefit of having the extra step where everything's renamed twice? 
> (if it's going to be a monolithic commit - as mentioned in (3)) Compared to 
> doing the mass change while keeping the (1 & 2) pieces for backwards 
> compatibility if needed?

Benefits I was seeing of splitting (1-3) up are:

- makes (2) easy for downstreams to integrate separately from (1) and (3) (see 
below for why (2) is interesting)
- prevents any reverts of (3) on main from resulting in churn in downstream 
efforts to migrate in response to (1-2)
- enables (3) to NOT be monolithic -- still debatable how useful it is, but if 
split up then individual pieces can run through buildbots separately (and be 
reverted separately)

>> 2. Change `MemoryBuffer` to use `Error` and `Expected`, leaving behind 
>> `std::error_code` and `ErrorOr` wrappers in a no-longer-just-a-typedef 
>> `MemoryBufferCompat`. Easy for downstreams to temporarily revert, and 
>> cherry-pick once they've finished adapting in the example of (1).
>> 3. Update all code to use the new APIs. Could be done all at once since it's 
>> mostly mechanical, as you said. Also an option to split up by component 
>> (e.g., maybe the llvm-symbolizer regression is okay, but it could be nice to 
>> get that reviewed separately / in isolation).
>> 4. Delete `MemoryBufferCompat`. Downstreams can temporarily revert while 
>> they follow the example of on (3).
>>
>> (Given that (1) and (2) are easy to write, you already have `tree` state for 
>> (4), and (3) is easy to create from (4), then I //think// you could 
>> construct the above commit sequence without having to redo all the work... 
>> then if you wanted to split (3) up from there it'd be easy enough.)
>>
>> (2) seems like the commit mostly likely to cause functional regressions, and 
>> it'd be isolated and easy to revert/reapply (downstream and/or upstream) 
>> without much churn.
>
> (3) would be more likely to cause regression? Presumably (2) is really 
> uninteresting because it adds a new API (re-adding MemoryBuffer, but unused 
> since everything's using MemoryBufferCompat) without any usage, yeah?

(2) changes all downstream clients of MemoryBuffer APIs from `std::error_code` 
to `Error`

- Mostly will cause build failures
- Also runtime regressions, due to `auto`, etc.

The fix is to do a search/replace of `MemoryBuffer::` to `MemoryBufferCompat::` 
on only the downstream code.

- Splitting from (1) means you can sequence this change between (1) and (2) — 
code always builds.
- Splitting from (3) means you can do a simple search/replace. If (2) is packed 
up with (3) it seems a lot more awkward, since you have to avoid accidentally 
undoing (3) during the search/replace. Then if somehow (3) gets reverted you 
need to start over when it relands.

> Plausible, though a fair bit more churn - I'd probably be up for it, though.
>
> - Dave




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109345

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

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

2021-09-09 Thread David Blaikie via Phabricator via lldb-commits

dblaikie added a comment.

In D109345#2990577 , @dexonsmith 
wrote:

> In D109345#2990527 , @dblaikie 
> wrote:
>
>> (were there other regressions I mentioned/should think about?)
>
> I don't have specific concerns; I was just reading between the lines of your 
> description...

Fair - probably my own hedging there.

>>> 1. Add `using MemoryBufferCompat = MemoryBuffer` and search/replace all 
>>> static `MemoryBuffer::` API calls over to `MemoryBufferCompat::`. No direct 
>>> impact on downstreams.
>>
>> Yeah, that's some of the extra churn I was thinking of/hoping to avoid - but 
>> yeah, it's probably worthwhile.
>>
>> What's the benefit of having the extra step where everything's renamed 
>> twice? (if it's going to be a monolithic commit - as mentioned in (3)) 
>> Compared to doing the mass change while keeping the (1 & 2) pieces for 
>> backwards compatibility if needed?
>
> Benefits I was seeing of splitting (1-3) up are:
>
> - makes (2) easy for downstreams to integrate separately from (1) and (3) 
> (see below for why (2) is interesting)
> - prevents any reverts of (3) on main from resulting in churn in downstream 
> efforts to migrate in response to (1-2)
> - enables (3) to NOT be monolithic -- still debatable how useful it is, but 
> if split up then individual pieces can run through buildbots separately (and 
> be reverted separately)
>
>>> 2. Change `MemoryBuffer` to use `Error` and `Expected`, leaving behind 
>>> `std::error_code` and `ErrorOr` wrappers in a no-longer-just-a-typedef 
>>> `MemoryBufferCompat`. Easy for downstreams to temporarily revert, and 
>>> cherry-pick once they've finished adapting in the example of (1).
>>> 3. Update all code to use the new APIs. Could be done all at once since 
>>> it's mostly mechanical, as you said. Also an option to split up by 
>>> component (e.g., maybe the llvm-symbolizer regression is okay, but it could 
>>> be nice to get that reviewed separately / in isolation).
>>> 4. Delete `MemoryBufferCompat`. Downstreams can temporarily revert while 
>>> they follow the example of on (3).
>>>
>>> (Given that (1) and (2) are easy to write, you already have `tree` state 
>>> for (4), and (3) is easy to create from (4), then I //think// you could 
>>> construct the above commit sequence without having to redo all the work... 
>>> then if you wanted to split (3) up from there it'd be easy enough.)
>>>
>>> (2) seems like the commit mostly likely to cause functional regressions, 
>>> and it'd be isolated and easy to revert/reapply (downstream and/or 
>>> upstream) without much churn.
>>
>> (3) would be more likely to cause regression? Presumably (2) is really 
>> uninteresting because it adds a new API (re-adding MemoryBuffer, but unused 
>> since everything's using MemoryBufferCompat) without any usage, yeah?
>
> (2) changes all downstream clients of MemoryBuffer APIs from 
> `std::error_code` to `Error`
>
> - Mostly will cause build failures
> - Also runtime regressions, due to `auto`, etc.

Oooh, right. Good point - thanks for walking me through it!

> The fix is to do a search/replace of `MemoryBuffer::` to 
> `MemoryBufferCompat::` on only the downstream code.
>
> - Splitting from (1) means you can sequence this change between (1) and (2) — 
> code always builds.
> - Splitting from (3) means you can do a simple search/replace. If (2) is 
> packed up with (3) it seems a lot more awkward, since you have to avoid 
> accidentally undoing (3) during the search/replace. Then if somehow (3) gets 
> reverted you need to start over when it relands.

Yeah, think I'm with you.

Given the amount of churn this means, though - reckon it's worth it? Reckon it 
needs more llvm-dev thread/buy-in/etc? Any other bike-shedding for 
MemoryBufferCompat? (MemoryBufferDeprecated? but I don't really mind)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109345

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

[Lldb-commits] [PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

2021-09-09 Thread Chris Lattner via Phabricator via lldb-commits

lattner added a comment.

This patch has a lot of noise, the important part is APInt.h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

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

[Lldb-commits] [PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

2021-09-09 Thread Craig Topper via Phabricator via lldb-commits

craig.topper added inline comments.



Comment at: llvm/include/llvm/ADT/APInt.h:384
   /// value for the APInt's bit width.
   bool isMaxValue() const { return isAllOnesValue(); }
 

isAllOnes()?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

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

[Lldb-commits] [PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

2021-09-09 Thread Chris Lattner via Phabricator via lldb-commits

lattner marked an inline comment as done.
lattner added inline comments.



Comment at: llvm/include/llvm/ADT/APInt.h:384
   /// value for the APInt's bit width.
   bool isMaxValue() const { return isAllOnesValue(); }
 

craig.topper wrote:
> isAllOnes()?
Yep, good catch, fixed thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

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

[Lldb-commits] [PATCH] D109483: [APInt] Normalize naming on keep constructors / predicate methods.

2021-09-09 Thread Craig Topper via Phabricator via lldb-commits

craig.topper added a comment.

I think I read this patch too closely. I'll leave it up to you how much of this 
you want to do.




Comment at: llvm/include/llvm/IR/Constants.h:206
   /// Determine if the value is all ones.
   bool isMinusOne() const { return Val.isAllOnesValue(); }
 

isAllOnes()



Comment at: llvm/include/llvm/Transforms/InstCombine/InstCombiner.h:171
   TrueIfSigned = true;
   return RHS.isAllOnesValue();
 case ICmpInst::ICMP_SGT: // True if LHS s> -1

isAllOnes since you're already in the area



Comment at: llvm/include/llvm/Transforms/InstCombine/InstCombiner.h:174
   TrueIfSigned = false;
   return RHS.isAllOnesValue();
 case ICmpInst::ICMP_SGE: // True if LHS s>= 0

isAllOnes



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3243
"Don't know how to expand this subtraction!");
-Tmp1 = DAG.getNode(ISD::XOR, dl, VT, Node->getOperand(1),
-   DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), dl,
-   VT));
+Tmp1 = DAG.getNode(
+ISD::XOR, dl, VT, Node->getOperand(1),

This could use DAG.getNOT if you're willing to make that change.



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:965
+  DAG.getConstant(APInt::getAllOnes(BitTy.getSizeInBits()), DL, MaskTy);
   SDValue NotMask = DAG.getNode(ISD::XOR, DL, MaskTy, Mask, AllOnes);
 

I think this could also be DAG.getNOT but I'm less sure.



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:1212
+  DAG.getConstant(APInt::getAllOnes(VT.getScalarSizeInBits()), DL, VT);
   SDValue NotMask = DAG.getNode(ISD::XOR, DL, VT, Mask, AllOnes);
 

This could be DAG.getNOT



Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:4021
   // (X & (C l>>/<< Y)) ==/!= 0  -->  ((X <> Y) & C) ==/!= 0
   if (C1.isNullValue())
 if (SDValue CC = optimizeSetCCByHoistingAndByConstFromLogicalShift(

isZero()



Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:4030
   // all bits set:   (X | (Y<<32)) == -1 --> (X & Y) == -1
   bool CmpZero = N1C->getAPIntValue().isNullValue();
+  bool CmpNegOne = N1C->getAPIntValue().isAllOnes();

isNullValue() -> isZero(). I was going to say you could use N1C->isZero() but I 
think it would have to be N1C->isNullValue() because that is the ConstantSDNode 
interface.



Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:12185
 else
-  OtherOp = DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), dl,
-VT);
+  OtherOp = DAG.getConstant(APInt::getAllOnes(VT.getSizeInBits()), dl, VT);
 return true;

I think we have DAG.getAllOnesConstant if you want to use it



Comment at: llvm/lib/Target/Lanai/LanaiISelLowering.cpp:1403
 else
-  OtherOp =
-  DAG.getConstant(APInt::getAllOnesValue(VT.getSizeInBits()), dl, VT);
+  OtherOp = DAG.getConstant(APInt::getAllOnes(VT.getSizeInBits()), dl, VT);
 return true;

I think we have DAG.getAllOnesConstant if you want to use it



Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1983
   Carry = DAG.getNode(M68kISD::ADD, DL, DAG.getVTList(CarryVT, MVT::i32), 
Carry,
   DAG.getConstant(NegOne, DL, CarryVT));
 

This is also getAllOnesConstant



Comment at: llvm/unittests/ADT/APIntTest.cpp:29
 TEST(APIntTest, ShiftLeftByZero) {
-  APInt One = APInt::getNullValue(65) + 1;
+  APInt One = APInt::getZero(65) + 1;
   APInt Shl = One.shl(0);

That's a strange way to write APInt(64, 1) unless there was some specific bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109483

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

[Lldb-commits] [lldb] 4f1c90a - [lldb] Fix format string in Communication::Write

2021-09-09 Thread Raphael Isemann via lldb-commits


Author: Ryan Mansfield
Date: 2021-09-09T17:55:38+02:00
New Revision: 4f1c90a6d4dde9e3c6d3c7d76bb187c1c19960d5

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

LOG: [lldb] Fix format string in Communication::Write

Reviewed By: teemperor

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

Added: 


Modified: 
lldb/source/Core/Communication.cpp

Removed: 




diff  --git a/lldb/source/Core/Communication.cpp 
b/lldb/source/Core/Communication.cpp
index 5640e0510cf1c..fd20b58d18b48 100644
--- a/lldb/source/Core/Communication.cpp
+++ b/lldb/source/Core/Communication.cpp
@@ -176,8 +176,8 @@ size_t Communication::Write(const void *src, size_t src_len,
 
   std::lock_guard guard(m_write_mutex);
   LLDB_LOG(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_COMMUNICATION),
-   "{0} Communication::Write (src = {1}, src_len = %" PRIu64
-   ") connection = {2}",
+   "{0} Communication::Write (src = {1}, src_len = {2}"
+   ") connection = {3}",
this, src, (uint64_t)src_len, connection_sp.get());
 
   if (connection_sp)



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

[Lldb-commits] [PATCH] D109508: [lldb] Fix format string in Communication::Write

2021-09-09 Thread Raphael Isemann 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 rG4f1c90a6d4dd: [lldb] Fix format string in 
Communication::Write (authored by rmansfield, committed by teemperor).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109508

Files:
  lldb/source/Core/Communication.cpp


Index: lldb/source/Core/Communication.cpp
===
--- lldb/source/Core/Communication.cpp
+++ lldb/source/Core/Communication.cpp
@@ -176,8 +176,8 @@
 
   std::lock_guard guard(m_write_mutex);
   LLDB_LOG(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_COMMUNICATION),
-   "{0} Communication::Write (src = {1}, src_len = %" PRIu64
-   ") connection = {2}",
+   "{0} Communication::Write (src = {1}, src_len = {2}"
+   ") connection = {3}",
this, src, (uint64_t)src_len, connection_sp.get());
 
   if (connection_sp)


Index: lldb/source/Core/Communication.cpp
===
--- lldb/source/Core/Communication.cpp
+++ lldb/source/Core/Communication.cpp
@@ -176,8 +176,8 @@
 
   std::lock_guard guard(m_write_mutex);
   LLDB_LOG(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_COMMUNICATION),
-   "{0} Communication::Write (src = {1}, src_len = %" PRIu64
-   ") connection = {2}",
+   "{0} Communication::Write (src = {1}, src_len = {2}"
+   ") connection = {3}",
this, src, (uint64_t)src_len, connection_sp.get());
 
   if (connection_sp)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107780: [lldb] [gdb-remote] Implement fallback to vFile:stat for GetFileSize()

2021-09-09 Thread Michał Górny via Phabricator via lldb-commits

mgorny updated this revision to Diff 371618.
mgorny marked 2 inline comments as done.
mgorny added a comment.

Use LLVM's `ubig*_t` types. Thanks for the suggestion, @labath!

Also add the comment that `Stat()` is only a wrapper over open+fstat.


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

https://reviews.llvm.org/D107780

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py

Index: lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py
@@ -77,3 +77,58 @@
 ])
 finally:
 self.dbg.GetSelectedPlatform().DisconnectRemote()
+
+def test_file_size(self):
+"""Test 'platform get-size'"""
+
+class Responder(MockGDBServerResponder):
+def vFile(self, packet):
+return "F1000"
+
+self.server.responder = Responder()
+
+try:
+self.runCmd("platform select remote-gdb-server")
+self.runCmd("platform connect connect://" +
+self.server.get_connect_address())
+self.assertTrue(self.dbg.GetSelectedPlatform().IsConnected())
+
+self.match("platform get-size /some/file.txt",
+   [r"File size of /some/file\.txt \(remote\): 4096"])
+self.assertPacketLogContains([
+"vFile:size:2f736f6d652f66696c652e747874",
+])
+finally:
+self.dbg.GetSelectedPlatform().DisconnectRemote()
+
+def test_file_size_fallback(self):
+"""Test 'platform get-size fallback to vFile:fstat'"""
+
+class Responder(MockGDBServerResponder):
+def vFile(self, packet):
+if packet.startswith("vFile:open:"):
+return "F5"
+elif packet.startswith("vFile:fstat:"):
+return "F40;" + 28 * "\0" + "\0\0\0\0\0\1\2\3" + 28 * "\0"
+if packet.startswith("vFile:close:"):
+return "F0"
+return ""
+
+self.server.responder = Responder()
+
+try:
+self.runCmd("platform select remote-gdb-server")
+self.runCmd("platform connect connect://" +
+self.server.get_connect_address())
+self.assertTrue(self.dbg.GetSelectedPlatform().IsConnected())
+
+self.match("platform get-size /some/file.txt",
+   [r"File size of /some/file\.txt \(remote\): 66051"])
+self.assertPacketLogContains([
+"vFile:size:2f736f6d652f66696c652e747874",
+"vFile:open:2f736f6d652f66696c652e747874,,",
+"vFile:fstat:5",
+"vFile:close:5",
+])
+finally:
+self.dbg.GetSelectedPlatform().DisconnectRemote()
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -376,6 +376,12 @@
 
   bool CloseFile(lldb::user_id_t fd, Status &error);
 
+  llvm::Optional FStat(lldb::user_id_t fd);
+
+  // NB: this is just a convenience wrapper over open() + fstat().  It does not
+  // work if the file cannot be opened.
+  llvm::Optional Stat(const FileSpec &file_spec);
+
   lldb::user_id_t GetFileSize(const FileSpec &file_spec);
 
   void AutoCompleteDiskFileOrDirectory(CompletionRequest &request,
@@ -581,7 +587,7 @@
   m_supports_QEnvironment : 1, m_supports_QEnvironmentHexEncoded : 1,
   m_supports_qSymbol : 1, m_qSymbol_requests_done : 1,
   m_supports_qModuleInfo : 1, m_supports_jThreadsInfo : 1,
-  m_supports_jModulesInfo : 1;
+  m_supports_jModulesInfo : 1, m_supports_vFileSize : 1;
 
   /// Current gdb remote protocol process identifier for all other operations
   lldb::pid_t m_curr_pid = LLDB_INVALID_PROCESS_ID;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -65,6 +65,7 @@
   m_supports_QEnvironmentHexEncoded(true), m_supports_qSymbol(true),
   m_qSymbol_requests_done(false), m_supports_qModuleInfo(true),
   m_supports_jThreadsInfo(true), m_supports_jModulesInfo(true),
+  m_supports

[Lldb-commits] [PATCH] D108953: [lldb/Plugins] Add memory region support in ScriptedProcess

2021-09-09 Thread Med Ismail Bennani via Phabricator via lldb-commits

mib added a comment.

Following @JDevlieghere question in https://reviews.llvm.org/D107585#2979988, 
here are some explanations:

In order to read memory from the ScriptedProcess, the process plugin calls the 
python `read_memory_at_address` method passing the address at which we should 
start reading (and the number of bytes to be read).

However, the memory buffer, that provided by the python_script, - usually - 
starts at offset 0. This is why having memory regions in the python script 
allows up to:

1. Check that the address that should be read is in one of the our memory 
regions.
2. Shift the address by the region start address so the memory reads are done 
at the right location.

Using a SB class for Scripted Processes could be risky, since it can introduce 
a cyclic dependency: If, for instance, we use a facility that is itself backed 
by an execution context object (process, thread, frame) for one of the Scripted 
execution context objects, that will cause the C++ object to use a Python 
object which will again use a C++ object, and so on.

Luckily, this is not an issue for `lldb.SBMemoryRegionInfo`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108953

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

[Lldb-commits] [PATCH] D108953: [lldb/Plugins] Add memory region support in ScriptedProcess

2021-09-09 Thread Med Ismail Bennani via Phabricator via lldb-commits

mib added a comment.

ping ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108953

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

[Lldb-commits] [PATCH] D107585: [lldb/Plugins] Add support for ScriptedThread in ScriptedProcess

2021-09-09 Thread Med Ismail Bennani via Phabricator via lldb-commits

mib added a comment.

ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107585

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

[Lldb-commits] [PATCH] D107585: [lldb/Plugins] Add support for ScriptedThread in ScriptedProcess

2021-09-09 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere added inline comments.



Comment at: lldb/examples/python/scripted_process/scripted_process.py:242
+"""
+pass
+

Should `eStateStopped` be the default?



Comment at: lldb/examples/python/scripted_process/scripted_process.py:244
+
+# @abstractmethod
+def get_queue(self):

Why's this commented out?



Comment at: lldb/include/lldb/Interpreter/ScriptedProcessInterface.h:77
+  StructuredData::GenericSP
+  CreatePluginObject(const llvm::StringRef class_name,
+ ExecutionContext &exe_ctx,





Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:105
+const char *ScriptedThread::GetName() {
+  if (m_name.empty()) {
+CheckInterpreterAndScriptObject();

We're caching the name and the queue, is this an optimization (to avoid calling 
into Python over and over) or are there assumptions that will break if these 
are not stable? If so, should that be something we call out on the Python side 
or alternatively, should we allow this to change and not cache it? 



Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:141-144
+  uint32_t concrete_frame_idx = 0;
+
+  if (frame)
+concrete_frame_idx = frame->GetConcreteFrameIndex();





Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:152-156
+  auto error_with_message = [](llvm::StringRef message) {
+LLDB_LOGF(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS),
+  "ScriptedProcess::%s ERROR = %s", __FUNCTION__, message.data());
+return false;
+  };

Seems like you have this lambda in a lot of places, I wonder if it's worth the 
trade-off of making this a static function that takes `__FUNCTION__` as an 
extra argument. 



Comment at: lldb/source/Plugins/Process/scripted/ScriptedThread.cpp:158-160
+  if (m_cached_stop_info_sp) {
+SetStopInfo(m_cached_stop_info_sp);
+  } else {

You call `SetStopInfo(m_cached_stop_info_sp);` on line 211, so we could drop 
the case for `m_cached_stop_info_sp` being true? 



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedThreadPythonInterface.cpp:128-133
+  if (!obj || !obj->IsValid() || error.Fail()) {
+return error_with_message(llvm::Twine("Null or invalid object (" +
+  llvm::Twine(error.AsCString()) +
+  llvm::Twine(")."))
+  .str());
+  }

This code is duplicated over and over for objects and dicts. Seems like a 
templated function could help here. Then you wouldn't have to worry about being 
more verbose and differentiating the different errors (null vs invalid vs 
error). Also, is there always an error when `!obj` or `!obj->IsValid()` or are 
there cases where this might print something like "Null or invalid object ()"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107585

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

[Lldb-commits] [PATCH] D108061: [lldb] Add support for shared library load when executable called through ld.

2021-09-09 Thread Rumeet Dhindsa via Phabricator via lldb-commits

rdhindsa updated this revision to Diff 371644.
rdhindsa marked 6 inline comments as done.
rdhindsa added a comment.

Thanks Pavel for investigating and figuring out that the upstream failing test 
had libxml support disabled. I was able to reproduce the failure locally by 
adding -DLLDB_ENABLE_LIBXML2=False to cmake. 
Updated UpdateFileSpecIfNecessary function to only update filename if the 
region.GetName().AsCString()  is not empty, which checks for both null and 
empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108061

Files:
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
  lldb/test/API/functionalities/dyld-launch-linux/Makefile
  lldb/test/API/functionalities/dyld-launch-linux/TestDyldLaunchLinux.py
  lldb/test/API/functionalities/dyld-launch-linux/main.cpp
  lldb/test/API/functionalities/dyld-launch-linux/signal_file.cpp
  lldb/test/API/functionalities/dyld-launch-linux/signal_file.h

Index: lldb/test/API/functionalities/dyld-launch-linux/signal_file.h
===
--- /dev/null
+++ lldb/test/API/functionalities/dyld-launch-linux/signal_file.h
@@ -0,0 +1 @@
+int get_signal_crash();
Index: lldb/test/API/functionalities/dyld-launch-linux/signal_file.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/dyld-launch-linux/signal_file.cpp
@@ -0,0 +1,7 @@
+#include "signal_file.h"
+#include 
+
+int get_signal_crash(void) {
+  raise(SIGSEGV);
+  return 0;
+}
Index: lldb/test/API/functionalities/dyld-launch-linux/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/dyld-launch-linux/main.cpp
@@ -0,0 +1,6 @@
+#include "signal_file.h"
+
+int main() {
+  // Break here
+  return get_signal_crash();
+}
Index: lldb/test/API/functionalities/dyld-launch-linux/TestDyldLaunchLinux.py
===
--- /dev/null
+++ lldb/test/API/functionalities/dyld-launch-linux/TestDyldLaunchLinux.py
@@ -0,0 +1,58 @@
+"""
+Test that LLDB can launch a linux executable through the dynamic loader and still hit a breakpoint.
+"""
+
+import lldb
+import os
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+class TestLinux64LaunchingViaDynamicLoader(TestBase):
+mydir = TestBase.compute_mydir(__file__)
+
+@skipIf(oslist=no_match(['linux']))
+@no_debug_info_test
+def test(self):
+self.build()
+
+# Extracts path of the interpreter.
+spec = lldb.SBModuleSpec()
+spec.SetFileSpec(lldb.SBFileSpec(self.getBuildArtifact("a.out")))
+interp_section = lldb.SBModule(spec).FindSection(".interp")
+if not interp_section:
+  return
+section_data = interp_section.GetSectionData()
+error = lldb.SBError()
+exe = section_data.GetString(error,0)
+if error.Fail():
+  return
+
+target = self.dbg.CreateTarget(exe)
+self.assertTrue(target, VALID_TARGET)
+
+# Set breakpoints both on shared library function as well as on
+# main. Both of them will be pending breakpoints.
+breakpoint_main = target.BreakpointCreateBySourceRegex("// Break here", lldb.SBFileSpec("main.cpp"))
+breakpoint_shared_library = target.BreakpointCreateBySourceRegex("get_signal_crash", lldb.SBFileSpec("signal_file.cpp"))
+launch_info = lldb.SBLaunchInfo([ "--library-path", self.get_process_working_directory(), self.getBuildArtifact("a.out")])
+launch_info.SetWorkingDirectory(self.get_process_working_directory())
+error = lldb.SBError()
+process = target.Launch(launch_info, error)
+self.assertTrue(error.Success())
+
+# Stopped on main here.
+self.assertEqual(process.GetState(), lldb.eStateStopped)
+thread = process.GetSelectedThread()
+self.assertIn("main", thread.GetFrameAtIndex(0).GetDisplayFunctionName())
+process.Continue()
+
+# Stopped on get_signal_crash function here.
+self.assertEqual(process.GetState(), lldb.eStateStopped)
+self.assertIn("get_signal_crash", thread.GetFrameAtIndex(0).GetDisplayFunctionName())
+process.Continue()
+
+# Stopped because of generated signal.
+self.assertEqual(process.GetState(), lldb.eStateStopped)
+self.assertIn("raise", thread.GetFrameAtIndex(0).GetDisplayFunctionName())
+self.assertIn("get_signal_crash", thread.GetFrameAtIndex(1).GetDisplayFunctionName())
Index: lldb/test/API/functionalities/dyld-launch-linux/Makefile
===
--- /dev/null
+++ lldb/test/API/functionalities/dyld-laun

[Lldb-commits] [PATCH] D107811: [lldb] [gdb-remote] Add fallbacks for vFile:mode and vFile:exists

2021-09-09 Thread Michał Górny via Phabricator via lldb-commits

mgorny updated this revision to Diff 371653.
mgorny added a comment.

Undo extra comments. Fix wrong length of test GDB packet.


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

https://reviews.llvm.org/D107811

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py

Index: lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py
===
--- lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py
+++ lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py
@@ -156,6 +156,38 @@
 finally:
 self.dbg.GetSelectedPlatform().DisconnectRemote()
 
+def test_file_permissions_fallback(self):
+"""Test 'platform get-permissions' fallback to fstat"""
+
+class Responder(MockGDBServerResponder):
+def vFile(self, packet):
+if packet.startswith("vFile:open:"):
+return "F5"
+elif packet.startswith("vFile:fstat:"):
+return "F40;" + 8 * "\0" + "\0\0\1\xA4" + 52 * "\0"
+if packet.startswith("vFile:close:"):
+return "F0"
+return ""
+
+self.server.responder = Responder()
+
+try:
+self.runCmd("platform select remote-gdb-server")
+self.runCmd("platform connect connect://" +
+self.server.get_connect_address())
+self.assertTrue(self.dbg.GetSelectedPlatform().IsConnected())
+
+self.match("platform get-permissions /some/file.txt",
+   [r"File permissions of /some/file\.txt \(remote\): 0o0644"])
+self.assertPacketLogContains([
+"vFile:mode:2f736f6d652f66696c652e747874",
+"vFile:open:2f736f6d652f66696c652e747874,,",
+"vFile:fstat:5",
+"vFile:close:5",
+])
+finally:
+self.dbg.GetSelectedPlatform().DisconnectRemote()
+
 def test_file_exists(self):
 """Test 'platform file-exists'"""
 
@@ -201,3 +233,58 @@
 ])
 finally:
 self.dbg.GetSelectedPlatform().DisconnectRemote()
+
+def test_file_exists_fallback(self):
+"""Test 'platform file-exists' fallback to open"""
+
+class Responder(MockGDBServerResponder):
+def vFile(self, packet):
+if packet.startswith("vFile:open:"):
+return "F5"
+if packet.startswith("vFile:close:"):
+return "F0"
+return ""
+
+self.server.responder = Responder()
+
+try:
+self.runCmd("platform select remote-gdb-server")
+self.runCmd("platform connect connect://" +
+self.server.get_connect_address())
+self.assertTrue(self.dbg.GetSelectedPlatform().IsConnected())
+
+self.match("platform file-exists /some/file.txt",
+   [r"File /some/file\.txt \(remote\) exists"])
+self.assertPacketLogContains([
+"vFile:exists:2f736f6d652f66696c652e747874",
+"vFile:open:2f736f6d652f66696c652e747874,,",
+"vFile:close:5",
+])
+finally:
+self.dbg.GetSelectedPlatform().DisconnectRemote()
+
+def test_file_exists_not_fallback(self):
+"""Test 'platform file-exists' fallback to open with non-existing file"""
+
+class Responder(MockGDBServerResponder):
+def vFile(self, packet):
+if packet.startswith("vFile:open:"):
+return "F-1,2"
+return ""
+
+self.server.responder = Responder()
+
+try:
+self.runCmd("platform select remote-gdb-server")
+self.runCmd("platform connect connect://" +
+self.server.get_connect_address())
+self.assertTrue(self.dbg.GetSelectedPlatform().IsConnected())
+
+self.match("platform file-exists /some/file.txt",
+   [r"File /some/file\.txt \(remote\) does not exist"])
+self.assertPacketLogContains([
+"vFile:exists:2f736f6d652f66696c652e747874",
+"vFile:open:2f736f6d652f66696c652e747874,,",
+])
+finally:
+self.dbg.GetSelectedPlatform().DisconnectRemote()
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
@@ -587,7 +587,8 @

[Lldb-commits] [PATCH] D107840: [lldb] [gdb-server] Implement the vFile:fstat packet

2021-09-09 Thread Michał Górny via Phabricator via lldb-commits

mgorny added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp:776
+
+  std::array data;
+  DataEncoder encoder{data.data(), data.size(), lldb::eByteOrderBig,

labath wrote:
> consider:
> ```
> struct GdbStat {
>   llvm::support::ubig32_t st_dev;
>   llvm::support::ubig32_t st_ino;
>   ...
> };
> 
> ...
> 
> translate(gdb_stats.st_dev, file_stats.st_dev, 0); // I'm not sure that this 
> clamping is really necessary.
> ...
> ```
> 
> Seems like it could be nicer, particularly as the vFile_Stat function will 
> need to do the same thing...
What's this `translate()` thing? I don't see anything matching in LLVM or LLDB. 
Or are you saying I should define a helper function?

As for clamping, I think it's better if we send 0 (a "clearly invalid value") 
than e.g. truncated `st_dev` that would be simply wrong.


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

https://reviews.llvm.org/D107840

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

[Lldb-commits] [PATCH] D108090: [lldb/lua] Supplement Lua bindings for lldb module

2021-09-09 Thread Siger Young via Phabricator via lldb-commits

siger-young added inline comments.



Comment at: lldb/bindings/lua/lua-typemaps.swig:219-221
+%typecheck(SWIG_TYPECHECK_STRING_ARRAY) char ** {
+   $1 = (lua_istable(L, $input) || lua_isnil(L, $input));
+}

tammela wrote:
> This is not being generated by SWIG for some reason.
> 
> I think it's more readable to just have an else clause that raises an error 
> on line 212.
I tried commenting and uncommenting these lines then diff, found that the 
typecheck is actually generated on those overloaded functions. 
`SBTarget::Launch` no longer works after commenting in my cases:
```
lua5.3: test.lua:27: Wrong arguments for overloaded function 'SBTarget_Launch'
  Possible C/C++ prototypes are:
lldb::SBTarget::Launch(lldb::SBListener &,char const **,char const **,char 
const *,char const *,char const *,char const *,uint32_t,bool,lldb::SBError &)
lldb::SBTarget::Launch(lldb::SBLaunchInfo &,lldb::SBError &)
```

It seems that SWIG uses "SWIG_isptrtype" to decide arg matches "char**", so 
this typecheck is necessary.

For those functions with just one definition (i.e. not overloaded), this 
typecheck does nothing on them, so it will be `typemap(in)`'s responsibility to 
check arg types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108090

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

[Lldb-commits] [PATCH] D107840: [lldb] [gdb-server] Implement the vFile:fstat packet

2021-09-09 Thread Michał Górny via Phabricator via lldb-commits

mgorny updated this revision to Diff 371674.
mgorny marked an inline comment as done.
mgorny added a comment.

Use fancy endian-magic types ;-).


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

https://reviews.llvm.org/D107840

Files:
  lldb/include/lldb/Utility/StringExtractorGDBRemote.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py

Index: lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py
===
--- lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py
+++ lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py
@@ -9,7 +9,38 @@
 from gdbremote_testcase import GdbRemoteTestCaseBase
 
 import binascii
+import os
 import stat
+import struct
+import typing
+
+
+class GDBStat(typing.NamedTuple):
+st_dev: int
+st_ino: int
+st_mode: int
+st_nlink: int
+st_uid: int
+st_gid: int
+st_rdev: int
+st_size: int
+st_blksize: int
+st_blocks: int
+st_atime: int
+st_mtime: int
+st_ctime: int
+
+
+def uint32_or_zero(x):
+return x if x < 2**32 else 0
+
+
+def uint32_or_max(x):
+return x if x < 2**32 else 2**32 - 1
+
+
+def uint32_trunc(x):
+return x & (2**32 - 1)
 
 
 class TestGdbRemotePlatformFile(GdbRemoteTestCaseBase):
@@ -178,6 +209,70 @@
 True)
 self.expect_gdbremote_sequence()
 
+@skipIfWindows
+@add_test_categories(["llgs"])
+def test_platform_file_fstat(self):
+server = self.connect_to_debug_monitor()
+self.assertIsNotNone(server)
+
+with tempfile.NamedTemporaryFile() as temp_file:
+temp_file.write(b"some test data for stat")
+temp_file.flush()
+
+self.do_handshake()
+self.test_sequence.add_log_lines(
+["read packet: $vFile:open:%s,0,0#00" % (
+binascii.b2a_hex(temp_file.name.encode()).decode(),),
+ {"direction": "send",
+ "regex": r"^\$F([0-9a-fA-F]+)#[0-9a-fA-F]{2}$",
+ "capture": {1: "fd"}}],
+True)
+
+context = self.expect_gdbremote_sequence()
+self.assertIsNotNone(context)
+fd = int(context["fd"], 16)
+
+self.reset_test_sequence()
+self.test_sequence.add_log_lines(
+["read packet: $vFile:fstat:%x#00" % (fd,),
+ {"direction": "send",
+ "regex": r"^\$F([0-9a-fA-F]+);(.*)#[0-9a-fA-F]{2}$",
+ "capture": {1: "size", 2: "data"}}],
+True)
+context = self.expect_gdbremote_sequence()
+self.assertEqual(int(context["size"], 16), 64)
+# NB: we're using .encode() as a hack because the test suite
+# is wrongly using (unicode) str instead of bytes
+gdb_stat = GDBStat(
+*struct.unpack(">IIIQQQIII",
+   self.decode_gdbremote_binary(context["data"])
+.encode("iso-8859-1")))
+sys_stat = os.fstat(temp_file.fileno())
+
+self.assertEqual(gdb_stat.st_dev, uint32_or_zero(sys_stat.st_dev))
+self.assertEqual(gdb_stat.st_ino, uint32_or_zero(sys_stat.st_ino))
+self.assertEqual(gdb_stat.st_mode, uint32_trunc(sys_stat.st_mode))
+self.assertEqual(gdb_stat.st_nlink, uint32_or_max(sys_stat.st_nlink))
+self.assertEqual(gdb_stat.st_uid, uint32_or_zero(sys_stat.st_uid))
+self.assertEqual(gdb_stat.st_gid, uint32_or_zero(sys_stat.st_gid))
+self.assertEqual(gdb_stat.st_rdev, uint32_or_zero(sys_stat.st_rdev))
+self.assertEqual(gdb_stat.st_size, sys_stat.st_size)
+self.assertEqual(gdb_stat.st_blksize, sys_stat.st_blksize)
+self.assertEqual(gdb_stat.st_blocks, sys_stat.st_blocks)
+self.assertEqual(gdb_stat.st_atime,
+ uint32_or_zero(int(sys_stat.st_atime)))
+self.assertEqual(gdb_stat.st_mtime,
+ uint32_or_zero(int(sys_stat.st_mtime)))
+self.assertEqual(gdb_stat.st_ctime,
+ uint32_or_zero(int(sys_stat.st_ctime)))
+
+self.reset_test_sequence()
+self.test_sequence.add_log_lines(
+["read packet: $vFile:close:%x#00" % (fd,),
+ "send packet: $F0#00"],
+True)
+self.expect_gdbremote_sequence()
+
 def expect_error(self):
 self.test_sequence.add_log_lines(
 [{"direction": "send",
Index: lldb/source/Utility/StringExtractorGDBRemote.cpp
===
--- lldb/source/Utility/StringExtractorGDBRemo

[Lldb-commits] [lldb] 766afbc - Don't re-define constants that are now in compact_unwind_encoding.h.

2021-09-09 Thread Jason Molenda via lldb-commits


Author: Jason Molenda
Date: 2021-09-09T17:01:43-07:00
New Revision: 766afbc8042bc45283d52c763d11327a34eb04a6

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

LOG: Don't re-define constants that are now in compact_unwind_encoding.h.

Added: 


Modified: 
lldb/tools/compact-unwind/compact-unwind-dumper.c

Removed: 




diff  --git a/lldb/tools/compact-unwind/compact-unwind-dumper.c 
b/lldb/tools/compact-unwind/compact-unwind-dumper.c
index d4706eaf5386..1551ed92597d 100644
--- a/lldb/tools/compact-unwind/compact-unwind-dumper.c
+++ b/lldb/tools/compact-unwind/compact-unwind-dumper.c
@@ -14,49 +14,6 @@
 #include 
 #include 
 
-enum {
-  UNWIND_ARM64_MODE_MASK = 0x0F00,
-  UNWIND_ARM64_MODE_FRAMELESS = 0x0200,
-  UNWIND_ARM64_MODE_DWARF = 0x0300,
-  UNWIND_ARM64_MODE_FRAME = 0x0400,
-
-  UNWIND_ARM64_FRAME_X19_X20_PAIR = 0x0001,
-  UNWIND_ARM64_FRAME_X21_X22_PAIR = 0x0002,
-  UNWIND_ARM64_FRAME_X23_X24_PAIR = 0x0004,
-  UNWIND_ARM64_FRAME_X25_X26_PAIR = 0x0008,
-  UNWIND_ARM64_FRAME_X27_X28_PAIR = 0x0010,
-  UNWIND_ARM64_FRAME_D8_D9_PAIR = 0x0100,
-  UNWIND_ARM64_FRAME_D10_D11_PAIR = 0x0200,
-  UNWIND_ARM64_FRAME_D12_D13_PAIR = 0x0400,
-  UNWIND_ARM64_FRAME_D14_D15_PAIR = 0x0800,
-
-  UNWIND_ARM64_FRAMELESS_STACK_SIZE_MASK = 0x00FFF000,
-  UNWIND_ARM64_DWARF_SECTION_OFFSET = 0x00FF,
-};
-
-enum {
-  UNWIND_ARM_MODE_MASK = 0x0F00,
-  UNWIND_ARM_MODE_FRAME = 0x0100,
-  UNWIND_ARM_MODE_FRAME_D = 0x0200,
-  UNWIND_ARM_MODE_DWARF = 0x0400,
-
-  UNWIND_ARM_FRAME_STACK_ADJUST_MASK = 0x00C0,
-
-  UNWIND_ARM_FRAME_FIRST_PUSH_R4 = 0x0001,
-  UNWIND_ARM_FRAME_FIRST_PUSH_R5 = 0x0002,
-  UNWIND_ARM_FRAME_FIRST_PUSH_R6 = 0x0004,
-
-  UNWIND_ARM_FRAME_SECOND_PUSH_R8 = 0x0008,
-  UNWIND_ARM_FRAME_SECOND_PUSH_R9 = 0x0010,
-  UNWIND_ARM_FRAME_SECOND_PUSH_R10 = 0x0020,
-  UNWIND_ARM_FRAME_SECOND_PUSH_R11 = 0x0040,
-  UNWIND_ARM_FRAME_SECOND_PUSH_R12 = 0x0080,
-
-  UNWIND_ARM_FRAME_D_REG_COUNT_MASK = 0x0700,
-
-  UNWIND_ARM_DWARF_SECTION_OFFSET = 0x00FF,
-};
-
 #define EXTRACT_BITS(value, mask)  
\
   ((value >> __builtin_ctz(mask)) & (((1 << __builtin_popcount(mask))) - 1))
 



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

[Lldb-commits] [lldb] f3472ad - Add specific error messages around gdb RSP handshake failures

2021-09-09 Thread Jason Molenda via lldb-commits


Author: Jason Molenda
Date: 2021-09-09T17:02:42-07:00
New Revision: f3472ad5c5f88c3425fc54f40d3d5280258d8be5

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

LOG: Add specific error messages around gdb RSP handshake failures

Report timeout exceeded and connection lost error messages
when sending the initial handshake packet in a gdb remote
serial protocol connection, an especially fragile time.

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

Added: 


Modified: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp 
b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 6dcc73c3e43d..16a27af700c6 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -82,6 +82,8 @@ bool GDBRemoteCommunicationClient::HandshakeWithServer(Status 
*error_ptr) {
 
   // Start the read thread after we send the handshake ack since if we fail to
   // send the handshake ack, there is no reason to continue...
+  std::chrono::steady_clock::time_point start_of_handshake =
+  std::chrono::steady_clock::now();
   if (SendAck()) {
 // Wait for any responses that might have been queued up in the remote
 // GDB server and flush them all
@@ -97,8 +99,24 @@ bool 
GDBRemoteCommunicationClient::HandshakeWithServer(Status *error_ptr) {
 if (QueryNoAckModeSupported()) {
   return true;
 } else {
-  if (error_ptr)
-error_ptr->SetErrorString("failed to get reply to handshake packet");
+  std::chrono::steady_clock::time_point end_of_handshake =
+  std::chrono::steady_clock::now();
+  auto handshake_timeout =
+  std::chrono::duration(end_of_handshake - start_of_handshake)
+  .count();
+  if (error_ptr) {
+if (packet_result == PacketResult::ErrorDisconnected)
+  error_ptr->SetErrorString("Connection shut down by remote side "
+"while waiting for reply to initial "
+"handshake packet");
+else if (packet_result == PacketResult::ErrorReplyTimeout)
+  error_ptr->SetErrorStringWithFormat(
+  "failed to get reply to handshake packet within timeout of "
+  "%.1f seconds",
+  handshake_timeout);
+else
+  error_ptr->SetErrorString("failed to get reply to handshake packet");
+  }
 }
   } else {
 if (error_ptr)



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

[Lldb-commits] [PATCH] D108888: Improve error messaging on process connect errors

2021-09-09 Thread Jason Molenda via Phabricator via lldb-commits

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf3472ad5c5f8: Add specific error messages around gdb RSP 
handshake failures (authored by jasonmolenda).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D10

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp


Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -82,6 +82,8 @@
 
   // Start the read thread after we send the handshake ack since if we fail to
   // send the handshake ack, there is no reason to continue...
+  std::chrono::steady_clock::time_point start_of_handshake =
+  std::chrono::steady_clock::now();
   if (SendAck()) {
 // Wait for any responses that might have been queued up in the remote
 // GDB server and flush them all
@@ -97,8 +99,24 @@
 if (QueryNoAckModeSupported()) {
   return true;
 } else {
-  if (error_ptr)
-error_ptr->SetErrorString("failed to get reply to handshake packet");
+  std::chrono::steady_clock::time_point end_of_handshake =
+  std::chrono::steady_clock::now();
+  auto handshake_timeout =
+  std::chrono::duration(end_of_handshake - start_of_handshake)
+  .count();
+  if (error_ptr) {
+if (packet_result == PacketResult::ErrorDisconnected)
+  error_ptr->SetErrorString("Connection shut down by remote side "
+"while waiting for reply to initial "
+"handshake packet");
+else if (packet_result == PacketResult::ErrorReplyTimeout)
+  error_ptr->SetErrorStringWithFormat(
+  "failed to get reply to handshake packet within timeout of "
+  "%.1f seconds",
+  handshake_timeout);
+else
+  error_ptr->SetErrorString("failed to get reply to handshake packet");
+  }
 }
   } else {
 if (error_ptr)


Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -82,6 +82,8 @@
 
   // Start the read thread after we send the handshake ack since if we fail to
   // send the handshake ack, there is no reason to continue...
+  std::chrono::steady_clock::time_point start_of_handshake =
+  std::chrono::steady_clock::now();
   if (SendAck()) {
 // Wait for any responses that might have been queued up in the remote
 // GDB server and flush them all
@@ -97,8 +99,24 @@
 if (QueryNoAckModeSupported()) {
   return true;
 } else {
-  if (error_ptr)
-error_ptr->SetErrorString("failed to get reply to handshake packet");
+  std::chrono::steady_clock::time_point end_of_handshake =
+  std::chrono::steady_clock::now();
+  auto handshake_timeout =
+  std::chrono::duration(end_of_handshake - start_of_handshake)
+  .count();
+  if (error_ptr) {
+if (packet_result == PacketResult::ErrorDisconnected)
+  error_ptr->SetErrorString("Connection shut down by remote side "
+"while waiting for reply to initial "
+"handshake packet");
+else if (packet_result == PacketResult::ErrorReplyTimeout)
+  error_ptr->SetErrorStringWithFormat(
+  "failed to get reply to handshake packet within timeout of "
+  "%.1f seconds",
+  handshake_timeout);
+else
+  error_ptr->SetErrorString("failed to get reply to handshake packet");
+  }
 }
   } else {
 if (error_ptr)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D109495: [lldb] [test] Add synchronization fix Subprocess test flakiness

2021-09-09 Thread Pavel Labath via Phabricator via lldb-commits

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

cool, thanks


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

https://reviews.llvm.org/D109495

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

44 matches

Mail list logo