date:20230308

[Lldb-commits] [PATCH] D145462: [LLDB][ObjectFileELF] Support LoongArch64 in ApplyReloctions

2023-03-08 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.




Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2623
+(is_signed &&
+ ((int64_t)value > INT32_MAX && (int64_t)value < INT32_MIN))) {
+  Log *log = GetLog(LLDBLog::Modules);

SixWeining wrote:
> DavidSpickett wrote:
> > Should this be `||` not `&&`?
> Yes I think so. This should be an error in original code but not introduced 
> this time. Do you mind I include the fix in current patch or in a separate 
> one?
In this patch is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145462

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

[Lldb-commits] [PATCH] D145550: [LLDB][ObjectFileELF] Correct the return type of RelocOffset64 and RelocAddend64

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added a comment.

Seems to me that member functions of `ELFRelocation` should use the typedefs 
from `lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h` where there is one. 
`elf_sxword` for example.

If you want to do that in another patch, that's fine. Just in case some test 
case is relying on the uin64_t -> unsigned down cast unintentionally.




Comment at: lldb/test/Shell/ObjectFile/ELF/loongarch64-relocations.yaml:37
 Type:R_LARCH_64
-Addend:  0x5678
+Addend:  0x1122334455667788
 Symbols:

I'm not familiar with how these relocation are processed, would it be better to 
use something with the sign bit set here? Or does it not matter, the value is 
just copied into .debug_info verbatim anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145550

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

[Lldb-commits] [PATCH] D145550: [LLDB][ObjectFileELF] Correct the return type of RelocOffset64 and RelocAddend64

2023-03-08 Thread Lu Weining via Phabricator via lldb-commits

SixWeining added a comment.

In D145550#4177291 , @DavidSpickett 
wrote:

> Seems to me that member functions of `ELFRelocation` should use the typedefs 
> from `lldb/source/Plugins/ObjectFile/ELF/ELFHeader.h` where there is one. 
> `elf_sxword` for example.
>
> If you want to do that in another patch, that's fine. Just in case some test 
> case is relying on the uin64_t -> unsigned down cast unintentionally.

Yes, I agree. Let me do that in another patch. Thanks.




Comment at: lldb/test/Shell/ObjectFile/ELF/loongarch64-relocations.yaml:37
 Type:R_LARCH_64
-Addend:  0x5678
+Addend:  0x1122334455667788
 Symbols:

DavidSpickett wrote:
> I'm not familiar with how these relocation are processed, would it be better 
> to use something with the sign bit set here? Or does it not matter, the value 
> is just copied into .debug_info verbatim anyway.
For this case, it is processed by below code:
```
2596 static void ApplyELF64ABS64Relocation(Symtab *symtab, ELFRelocation &rel,
2597   DataExtractor &debug_data,
2598   Section *rel_section) {
2599   Symbol *symbol = 
symtab->FindSymbolByID(ELFRelocation::RelocSymbol64(rel));
2600   if (symbol) {
2601 addr_t value = symbol->GetAddressRef().GetFileAddress();
2602 DataBufferSP &data_buffer_sp = debug_data.GetSharedDataBuffer(); 
2603 // ObjectFileELF creates a WritableDataBuffer in CreateInstance.
2604 WritableDataBuffer *data_buffer =
2605 llvm::cast(data_buffer_sp.get());
2606 uint64_t *dst = reinterpret_cast(
2607 data_buffer->GetBytes() + rel_section->GetFileOffset() +
2608 ELFRelocation::RelocOffset64(rel));
2609 uint64_t val_offset = value + ELFRelocation::RelocAddend64(rel);
2610 memcpy(dst, &val_offset, sizeof(uint64_t));
2611   }
2612 }
```

Memcpy `S + A` to .debug_info.

I can add another relocation entry with the sign bit set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145550

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

[Lldb-commits] [PATCH] D145550: [LLDB][ObjectFileELF] Correct the return type of RelocOffset64 and RelocAddend64

2023-03-08 Thread Lu Weining via Phabricator via lldb-commits

SixWeining updated this revision to Diff 503264.
SixWeining added a comment.

Address @DavidSpickett's comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145550

Files:
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/test/Shell/ObjectFile/ELF/loongarch64-relocations.yaml


Index: lldb/test/Shell/ObjectFile/ELF/loongarch64-relocations.yaml
===
--- lldb/test/Shell/ObjectFile/ELF/loongarch64-relocations.yaml
+++ lldb/test/Shell/ObjectFile/ELF/loongarch64-relocations.yaml
@@ -6,9 +6,9 @@
 # CHECK:  Name: .debug_info
 # CHECK:  Data:  (
 ## Before relocation:
-##:   
+##:     
 ## After relocation:
-# CHECK-NEXT: : 3412 7856 
+# CHECK-NEXT: : 3412 88776655 44332211 8899AABB CCDDEEFF
 # CHECK-NEXT: )
 
 --- !ELF
@@ -22,7 +22,7 @@
 Type:SHT_PROGBITS
   - Name:.debug_info
 Type:SHT_PROGBITS
-Content: 
+Content: 
   - Name:.rela.debug_info
 Type:SHT_RELA
 Info:.debug_info
@@ -34,7 +34,11 @@
   - Offset:  0x0004
 Symbol:  .debug_str
 Type:R_LARCH_64
-Addend:  0x5678
+Addend:  0x1122334455667788
+  - Offset:  0x000C
+Symbol:  .debug_str
+Type:R_LARCH_64
+Addend:  0xFFEEDDCCBBAA9988
 Symbols:
   - Name:.debug_str
 Type:STT_SECTION
Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -116,11 +116,11 @@
 
   static unsigned RelocOffset32(const ELFRelocation &rel);
 
-  static unsigned RelocOffset64(const ELFRelocation &rel);
+  static elf_addr RelocOffset64(const ELFRelocation &rel);
 
   static unsigned RelocAddend32(const ELFRelocation &rel);
 
-  static unsigned RelocAddend64(const ELFRelocation &rel);
+  static elf_sxword RelocAddend64(const ELFRelocation &rel);
 
   bool IsRela() { return (reloc.is()); }
 
@@ -192,7 +192,7 @@
 return rel.reloc.get()->r_offset;
 }
 
-unsigned ELFRelocation::RelocOffset64(const ELFRelocation &rel) {
+elf_addr ELFRelocation::RelocOffset64(const ELFRelocation &rel) {
   if (rel.reloc.is())
 return rel.reloc.get()->r_offset;
   else
@@ -206,7 +206,7 @@
 return rel.reloc.get()->r_addend;
 }
 
-unsigned ELFRelocation::RelocAddend64(const ELFRelocation &rel) {
+elf_sxword  ELFRelocation::RelocAddend64(const ELFRelocation &rel) {
   if (rel.reloc.is())
 return 0;
   else


Index: lldb/test/Shell/ObjectFile/ELF/loongarch64-relocations.yaml
===
--- lldb/test/Shell/ObjectFile/ELF/loongarch64-relocations.yaml
+++ lldb/test/Shell/ObjectFile/ELF/loongarch64-relocations.yaml
@@ -6,9 +6,9 @@
 # CHECK:  Name: .debug_info
 # CHECK:  Data:  (
 ## Before relocation:
-##:   
+##:     
 ## After relocation:
-# CHECK-NEXT: : 3412 7856 
+# CHECK-NEXT: : 3412 88776655 44332211 8899AABB CCDDEEFF
 # CHECK-NEXT: )
 
 --- !ELF
@@ -22,7 +22,7 @@
 Type:SHT_PROGBITS
   - Name:.debug_info
 Type:SHT_PROGBITS
-Content: 
+Content: 
   - Name:.rela.debug_info
 Type:SHT_RELA
 Info:.debug_info
@@ -34,7 +34,11 @@
   - Offset:  0x0004
 Symbol:  .debug_str
 Type:R_LARCH_64
-Addend:  0x5678
+Addend:  0x1122334455667788
+  - Offset:  0x000C
+Symbol:  .debug_str
+Type:R_LARCH_64
+Addend:  0xFFEEDDCCBBAA9988
 Symbols:
   - Name:.debug_str
 Type:STT_SECTION
Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -116,11 +116,11 @@
 
   static unsigned RelocOffset32(const ELFRelocation &rel);
 
-  static unsigned RelocOffset64(const ELFRelocation &rel);
+  static elf_addr RelocOffset64(const ELFRelocation &rel);
 
   static unsigned RelocAddend32(const ELFRelocation &rel);
 
-  static unsig

[Lldb-commits] [PATCH] D145559: [lldb] Remove unused POSIX ProcessMessage files

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett created this revision.
Herald added a subscriber: emaste.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added projects: LLDB, LLVM.
Herald added subscribers: llvm-commits, lldb-commits.

The last use of these was removed in cd443398566b953642ead7c81528ab5b4e211eb9 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145559

Files:
  lldb/source/Plugins/Process/POSIX/CMakeLists.txt
  lldb/source/Plugins/Process/POSIX/ProcessMessage.cpp
  lldb/source/Plugins/Process/POSIX/ProcessMessage.h
  llvm/utils/gn/secondary/lldb/source/Plugins/Process/POSIX/BUILD.gn

Index: llvm/utils/gn/secondary/lldb/source/Plugins/Process/POSIX/BUILD.gn
===
--- llvm/utils/gn/secondary/lldb/source/Plugins/Process/POSIX/BUILD.gn
+++ llvm/utils/gn/secondary/lldb/source/Plugins/Process/POSIX/BUILD.gn
@@ -12,7 +12,6 @@
   sources = [
 "CrashReason.cpp",
 "NativeProcessELF.cpp",
-"ProcessMessage.cpp",
 "ProcessPOSIXLog.cpp",
   ]
 }
Index: lldb/source/Plugins/Process/POSIX/ProcessMessage.h
===
--- lldb/source/Plugins/Process/POSIX/ProcessMessage.h
+++ /dev/null
@@ -1,168 +0,0 @@
-//===-- ProcessMessage.h *- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#ifndef liblldb_ProcessMessage_H_
-#define liblldb_ProcessMessage_H_
-
-#include "CrashReason.h"
-
-#include 
-#include 
-
-#include "lldb/lldb-defines.h"
-#include "lldb/lldb-types.h"
-
-class ProcessMessage {
-public:
-  /// The type of signal this message can correspond to.
-  enum Kind {
-eInvalidMessage,
-eAttachMessage,
-eExitMessage,
-eLimboMessage,
-eSignalMessage,
-eSignalDeliveredMessage,
-eTraceMessage,
-eBreakpointMessage,
-eWatchpointMessage,
-eCrashMessage,
-eNewThreadMessage,
-eExecMessage
-  };
-
-  ProcessMessage()
-  : m_tid(LLDB_INVALID_PROCESS_ID), m_kind(eInvalidMessage),
-m_crash_reason(CrashReason::eInvalidCrashReason), m_status(0),
-m_addr(0) {}
-
-  Kind GetKind() const { return m_kind; }
-
-  lldb::tid_t GetTID() const { return m_tid; }
-
-  /// Indicates that the process \p pid has successfully attached.
-  static ProcessMessage Attach(lldb::pid_t pid) {
-return ProcessMessage(pid, eAttachMessage);
-  }
-
-  /// Indicates that the thread \p tid is about to exit with status \p status.
-  static ProcessMessage Limbo(lldb::tid_t tid, int status) {
-return ProcessMessage(tid, eLimboMessage, status);
-  }
-
-  /// Indicates that the thread \p tid had the signal \p signum delivered.
-  static ProcessMessage Signal(lldb::tid_t tid, int signum) {
-return ProcessMessage(tid, eSignalMessage, signum);
-  }
-
-  /// Indicates that a signal \p signum generated by the debugging process was
-  /// delivered to the thread \p tid.
-  static ProcessMessage SignalDelivered(lldb::tid_t tid, int signum) {
-return ProcessMessage(tid, eSignalDeliveredMessage, signum);
-  }
-
-  /// Indicates that the thread \p tid encountered a trace point.
-  static ProcessMessage Trace(lldb::tid_t tid) {
-return ProcessMessage(tid, eTraceMessage);
-  }
-
-  /// Indicates that the thread \p tid encountered a break point.
-  static ProcessMessage Break(lldb::tid_t tid) {
-return ProcessMessage(tid, eBreakpointMessage);
-  }
-
-  static ProcessMessage Watch(lldb::tid_t tid, lldb::addr_t wp_addr) {
-return ProcessMessage(tid, eWatchpointMessage, 0, wp_addr);
-  }
-
-  /// Indicates that the thread \p tid crashed.
-  static ProcessMessage Crash(lldb::pid_t pid, CrashReason reason, int signo,
-  lldb::addr_t fault_addr) {
-ProcessMessage message(pid, eCrashMessage, signo, fault_addr);
-message.m_crash_reason = reason;
-return message;
-  }
-
-  /// Indicates that the thread \p child_tid was spawned.
-  static ProcessMessage NewThread(lldb::tid_t parent_tid,
-  lldb::tid_t child_tid) {
-return ProcessMessage(parent_tid, eNewThreadMessage, child_tid);
-  }
-
-  /// Indicates that the thread \p tid is about to exit with status \p status.
-  static ProcessMessage Exit(lldb::tid_t tid, int status) {
-return ProcessMessage(tid, eExitMessage, status);
-  }
-
-  /// Indicates that the thread \p pid has exec'd.
-  static ProcessMessage Exec(lldb::tid_t tid) {
-return ProcessMessage(tid, eExecMessage);
-  }
-
-  int GetExitStatus() const {
-assert(GetKind() == eExitMessage || GetKind() == eLimboMessage);
-return m_status;
-  }
-
-  int GetSignal() const {

[Lldb-commits] [PATCH] D145550: [LLDB][ObjectFileELF] Correct the return type of RelocOffset64 and RelocAddend64

2023-03-08 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145550

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

[Lldb-commits] [PATCH] D145561: [lldb] Remove unused CrashReasonAsString function

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

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

The last user was ProcessMessage, which has itself been removed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145561

Files:
  lldb/source/Plugins/Process/POSIX/CrashReason.cpp
  lldb/source/Plugins/Process/POSIX/CrashReason.h

Index: lldb/source/Plugins/Process/POSIX/CrashReason.h
===
--- lldb/source/Plugins/Process/POSIX/CrashReason.h
+++ lldb/source/Plugins/Process/POSIX/CrashReason.h
@@ -54,8 +54,6 @@
 std::string GetCrashReasonString(CrashReason reason, lldb::addr_t fault_addr);
 std::string GetCrashReasonString(CrashReason reason, const siginfo_t &info);
 
-const char *CrashReasonAsString(CrashReason reason);
-
 CrashReason GetCrashReason(const siginfo_t &info);
 
 #endif // #ifndef liblldb_CrashReason_H_
Index: lldb/source/Plugins/Process/POSIX/CrashReason.cpp
===
--- lldb/source/Plugins/Process/POSIX/CrashReason.cpp
+++ lldb/source/Plugins/Process/POSIX/CrashReason.cpp
@@ -247,97 +247,6 @@
   return str;
 }
 
-const char *CrashReasonAsString(CrashReason reason) {
-  const char *str = nullptr;
-
-  switch (reason) {
-  case CrashReason::eInvalidCrashReason:
-str = "eInvalidCrashReason";
-break;
-
-  // SIGSEGV crash reasons.
-  case CrashReason::eInvalidAddress:
-str = "eInvalidAddress";
-break;
-  case CrashReason::ePrivilegedAddress:
-str = "ePrivilegedAddress";
-break;
-  case CrashReason::eBoundViolation:
-str = "eBoundViolation";
-break;
-  case CrashReason::eAsyncTagCheckFault:
-str = "eAsyncTagCheckFault";
-break;
-  case CrashReason::eSyncTagCheckFault:
-str = "eSyncTagCheckFault";
-break;
-
-  // SIGILL crash reasons.
-  case CrashReason::eIllegalOpcode:
-str = "eIllegalOpcode";
-break;
-  case CrashReason::eIllegalOperand:
-str = "eIllegalOperand";
-break;
-  case CrashReason::eIllegalAddressingMode:
-str = "eIllegalAddressingMode";
-break;
-  case CrashReason::eIllegalTrap:
-str = "eIllegalTrap";
-break;
-  case CrashReason::ePrivilegedOpcode:
-str = "ePrivilegedOpcode";
-break;
-  case CrashReason::ePrivilegedRegister:
-str = "ePrivilegedRegister";
-break;
-  case CrashReason::eCoprocessorError:
-str = "eCoprocessorError";
-break;
-  case CrashReason::eInternalStackError:
-str = "eInternalStackError";
-break;
-
-  // SIGBUS crash reasons:
-  case CrashReason::eIllegalAlignment:
-str = "eIllegalAlignment";
-break;
-  case CrashReason::eIllegalAddress:
-str = "eIllegalAddress";
-break;
-  case CrashReason::eHardwareError:
-str = "eHardwareError";
-break;
-
-  // SIGFPE crash reasons:
-  case CrashReason::eIntegerDivideByZero:
-str = "eIntegerDivideByZero";
-break;
-  case CrashReason::eIntegerOverflow:
-str = "eIntegerOverflow";
-break;
-  case CrashReason::eFloatDivideByZero:
-str = "eFloatDivideByZero";
-break;
-  case CrashReason::eFloatOverflow:
-str = "eFloatOverflow";
-break;
-  case CrashReason::eFloatUnderflow:
-str = "eFloatUnderflow";
-break;
-  case CrashReason::eFloatInexactResult:
-str = "eFloatInexactResult";
-break;
-  case CrashReason::eFloatInvalidOperation:
-str = "eFloatInvalidOperation";
-break;
-  case CrashReason::eFloatSubscriptRange:
-str = "eFloatSubscriptRange";
-break;
-  }
-  return str;
-}
-
 CrashReason GetCrashReason(const siginfo_t &info) {
   switch (info.si_signo) {
   case SIGSEGV:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145377: [LLDB] Show sub type of signals for ELF core files

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

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

Add code functionality to UnixSignals instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145377

Files:
  lldb/include/lldb/Target/StopInfo.h
  lldb/include/lldb/Target/UnixSignals.h
  lldb/source/Plugins/Process/Utility/LinuxSignals.cpp
  lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
  lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp
  lldb/source/Plugins/Process/elf-core/ThreadElfCore.h
  lldb/source/Target/StopInfo.cpp
  lldb/source/Target/UnixSignals.cpp
  
lldb/test/API/linux/aarch64/mte_core_file/TestAArch64LinuxMTEMemoryTagCoreFile.py
  lldb/unittests/Signals/UnixSignalsTest.cpp
  llvm/docs/ReleaseNotes.rst

Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -188,6 +188,9 @@
   omit defaulted template parameters. The full template parameter list can still be
   viewed with `expr --raw-output`/`frame var --raw-output`. (`D141828 `_)
 
+* When debugging a Linux core file containing a segfault related to memory tagging,
+  LLDB now reports the specific type of that fault. For example ``SIGSEGV: sync tag check fault``.
+
 Changes to Sanitizers
 -
 
Index: lldb/unittests/Signals/UnixSignalsTest.cpp
===
--- lldb/unittests/Signals/UnixSignalsTest.cpp
+++ lldb/unittests/Signals/UnixSignalsTest.cpp
@@ -23,6 +23,7 @@
 AddSignal(4, "SIG4", true, false, true, "DESC4");
 AddSignal(8, "SIG8", true, true, true, "DESC8");
 AddSignal(16, "SIG16", true, false, false, "DESC16");
+AddSignalCode(16, 1, "a specific type of SIG16");
   }
 };
 
@@ -93,6 +94,25 @@
   EXPECT_EQ(name, signals.GetSignalAsCString(signo));
 }
 
+TEST(UnixSignalsTest, GetAsCString) {
+  TestSignals signals;
+
+  EXPECT_EQ(nullptr, signals.GetSignalAsCString(100));
+  std::string name = signals.GetSignalAsCString(16);
+  EXPECT_EQ("SIG16", name);
+}
+
+TEST(UnixSignalsTest, GetAsString) {
+  TestSignals signals;
+
+  EXPECT_EQ("", signals.GetSignalAsString(100, std::nullopt));
+  EXPECT_EQ("SIG16", signals.GetSignalAsString(16, std::nullopt));
+  EXPECT_EQ("", signals.GetSignalAsString(100, 100));
+  EXPECT_EQ("SIG16", signals.GetSignalAsString(16, 100));
+  EXPECT_EQ("SIG16: a specific type of SIG16",
+signals.GetSignalAsString(16, 1));
+}
+
 TEST(UnixSignalsTest, VersionChange) {
   TestSignals signals;
 
Index: lldb/test/API/linux/aarch64/mte_core_file/TestAArch64LinuxMTEMemoryTagCoreFile.py
===
--- lldb/test/API/linux/aarch64/mte_core_file/TestAArch64LinuxMTEMemoryTagCoreFile.py
+++ lldb/test/API/linux/aarch64/mte_core_file/TestAArch64LinuxMTEMemoryTagCoreFile.py
@@ -166,3 +166,14 @@
 # the MTE core file which does support it but does not allow writing tags.
 self.expect("memory tag write 0 1",
 substrs=["error: Process does not support memory tagging"], error=True)
+
+@skipIfLLVMTargetMissing("AArch64")
+def test_mte_tag_fault_reason(self):
+""" Test that we correctly report the fault reason. """
+self.runCmd("target create --core core.mte")
+
+# There is no fault address shown here because core files do not include
+# si_addr.
+self.expect("bt", substrs=[
+"* thread #1, name = 'a.out.mte', stop reason = signal SIGSEGV: "
+"sync tag check fault"])
Index: lldb/source/Target/UnixSignals.cpp
===
--- lldb/source/Target/UnixSignals.cpp
+++ lldb/source/Target/UnixSignals.cpp
@@ -122,6 +122,14 @@
   ++m_version;
 }
 
+void UnixSignals::AddSignalCode(int signo, int code, const char *description) {
+  collection::iterator signal = m_signals.find(signo);
+  if (signal == m_signals.end())
+return;
+  signal->second.m_codes.insert(std::pair{code, description});
+  ++m_version;
+}
+
 void UnixSignals::RemoveSignal(int signo) {
   collection::iterator pos = m_signals.find(signo);
   if (pos != m_signals.end())
@@ -137,6 +145,27 @@
 return pos->second.m_name.GetCString();
 }
 
+std::string UnixSignals::GetSignalAsString(int32_t signo,
+   std::optional code) const {
+  std::string str;
+
+  collection::const_iterator pos = m_signals.find(signo);
+  if (pos != m_signals.end()) {
+str = pos->second.m_name.GetCString();
+
+if (code) {
+  std::map::const_iterator cpos =
+  pos->second.m_codes.find(*code);
+  if (cpos != pos->second.m_codes.end()) {
+str += ": ";
+str += cpos->second.GetCString();
+  }
+}
+  }
+
+  return str;
+}
+
 bool UnixSignals::SignalIsValid(int32_t signo) const {

[Lldb-commits] [PATCH] D145377: [LLDB] Show sub type of memory tagging SEGV when reading a core file

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added a comment.

This should build anywhere now.

I looked at `thread siginfo` and I see what you mean, but it'll take me some 
time to confirm exactly how much of siginfo is in the core file. I did get it 
to work using the full type, but I'm pretty sure some of the values were 
invalid.

Now I'm looking at replacing the CrashReason code with UnixSignals. Perhaps it 
can learn to append fault addresses and bounds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145377

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

[Lldb-commits] [lldb] 2857eeb - [lldb][test][NFC] TestDataFormatterCpp.py: Remove redundant FIXME

2023-03-08 Thread Michael Buch via lldb-commits


Author: Michael Buch
Date: 2023-03-08T10:45:20Z
New Revision: 2857eeb5f4f7b147a90d6b223f6aa7f90fd53f1c

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

LOG: [lldb][test][NFC] TestDataFormatterCpp.py: Remove redundant FIXME

This got fixed in D145241

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

Added: 


Modified: 

lldb/test/API/functionalities/data-formatter/data-formatter-cpp/TestDataFormatterCpp.py

Removed: 




diff  --git 
a/lldb/test/API/functionalities/data-formatter/data-formatter-cpp/TestDataFormatterCpp.py
 
b/lldb/test/API/functionalities/data-formatter/data-formatter-cpp/TestDataFormatterCpp.py
index 4c6bcff35f720..e26ef1f61f9f3 100644
--- 
a/lldb/test/API/functionalities/data-formatter/data-formatter-cpp/TestDataFormatterCpp.py
+++ 
b/lldb/test/API/functionalities/data-formatter/data-formatter-cpp/TestDataFormatterCpp.py
@@ -291,7 +291,6 @@ def test_mem_func_ptr_formats(self):
 
 lldbutil.run_to_source_breakpoint(self, "Break in 
has_local_mem_func_pointers", lldb.SBFileSpec("main.cpp"))
 
-# FIXME: don't format pointer to members as bytes, but rather as 
regular pointers
 self.expect(
 "frame variable member_ptr",
 patterns=['member_ptr = 0x[0-9a-z]+'])



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

[Lldb-commits] [PATCH] D145566: [lldb] Add RegisterFlags class

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

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

This models the "flags" node from GDB's target XML:
https://sourceware.org/gdb/onlinedocs/gdb/Target-Description-Format.html

This node is used to describe the fields of registers like cpsr on AArch64.

RegisterFlags is a class that contains a list of register fields.
These fields will be extracted from the XML sent by the remote.

We assume that there is at least one field, that the fields are
sorted in descending order and do not overlap. That will be
enforced by the XML processor (the GDB client code in our case).

The fields may not cover the whole register. To account for this
RegisterFields will add anonymous padding fields so that
sizeof(all fields) == sizeof(register). This will save a lot
of hasssle later.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145566

Files:
  lldb/include/lldb/Target/RegisterFlags.h
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/RegisterFlags.cpp
  lldb/unittests/Target/CMakeLists.txt
  lldb/unittests/Target/RegisterFlagsTest.cpp

Index: lldb/unittests/Target/RegisterFlagsTest.cpp
===
--- /dev/null
+++ lldb/unittests/Target/RegisterFlagsTest.cpp
@@ -0,0 +1,124 @@
+//===-- RegisterFlagsTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Target/RegisterFlags.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+TEST(RegisterFlagsTest, Field) {
+  // We assume that start <= end is always true, so that is not tested here.
+
+  RegisterFlags::Field f1("abc", 0, 0, "unknown");
+  ASSERT_EQ(f1.GetName(), "abc");
+  ASSERT_EQ(f1.GetType(), "unknown");
+  // start == end means a 1 bit field.
+  ASSERT_EQ(f1.GetSizeInBits(), (unsigned)1);
+  ASSERT_EQ(f1.GetMask(), (uint64_t)1);
+  ASSERT_EQ(f1.GetValue(0), (uint64_t)0);
+  ASSERT_EQ(f1.GetValue(3), (uint64_t)1);
+
+  // End is inclusive meaning that start 0 to end 1 includes bit 1
+  // to make a 2 bit field.
+  RegisterFlags::Field f2("", 0, 1, "");
+  ASSERT_EQ(f2.GetSizeInBits(), (unsigned)2);
+  ASSERT_EQ(f2.GetMask(), (uint64_t)3);
+  ASSERT_EQ(f2.GetValue(UINT64_MAX), (uint64_t)3);
+  ASSERT_EQ(f2.GetValue(UINT64_MAX & ~(uint64_t)3), (uint64_t)0);
+
+  // If the field doesn't start at 0 we need to shift up/down
+  // to account for it.
+  RegisterFlags::Field f3("", 2, 5, "");
+  ASSERT_EQ(f3.GetSizeInBits(), (unsigned)4);
+  ASSERT_EQ(f3.GetMask(), (uint64_t)0x3c);
+  ASSERT_EQ(f3.GetValue(UINT64_MAX), (uint64_t)0xf);
+  ASSERT_EQ(f3.GetValue(UINT64_MAX & ~(uint64_t)0x3c), (uint64_t)0);
+
+  // Fields are sorted lowest starting bit first.
+  ASSERT_TRUE(f2 < f3);
+  ASSERT_FALSE(f3 < f1);
+  ASSERT_FALSE(f1 < f2);
+  ASSERT_FALSE(f1 < f1);
+}
+
+static RegisterFlags::Field make_field(unsigned start, unsigned end) {
+  return RegisterFlags::Field("", start, end, "");
+}
+
+TEST(RegisterFlagsTest, FieldOverlaps) {
+  // Single bit fields
+  ASSERT_FALSE(make_field(0, 0).Overlaps(make_field(1, 1)));
+  ASSERT_TRUE(make_field(1, 1).Overlaps(make_field(1, 1)));
+  ASSERT_FALSE(make_field(1, 1).Overlaps(make_field(3, 3)));
+
+  ASSERT_TRUE(make_field(0, 1).Overlaps(make_field(1, 2)));
+  ASSERT_TRUE(make_field(1, 2).Overlaps(make_field(0, 1)));
+  ASSERT_FALSE(make_field(0, 1).Overlaps(make_field(2, 3)));
+  ASSERT_FALSE(make_field(2, 3).Overlaps(make_field(0, 1)));
+
+  ASSERT_FALSE(make_field(1, 5).Overlaps(make_field(10, 20)));
+  ASSERT_FALSE(make_field(15, 30).Overlaps(make_field(7, 12)));
+}
+
+TEST(RegisterFlagsTest, PaddingDistance) {
+  // We assume that this method is always called with a more significant
+  // (start bit is higher) field first and that they do not overlap.
+
+  // [field 1][field 2]
+  ASSERT_EQ(make_field(1, 1).PaddingDistance(make_field(0, 0)), 0ULL);
+  // [field 1][..][field 2]
+  ASSERT_EQ(make_field(2, 2).PaddingDistance(make_field(0, 0)), 1ULL);
+  // [field 1][field 1][field 2]
+  ASSERT_EQ(make_field(1, 2).PaddingDistance(make_field(0, 0)), 0ULL);
+  // [field 1][30 bits free][field 2]
+  ASSERT_EQ(make_field(31, 31).PaddingDistance(make_field(0, 0)), 30ULL);
+}
+
+static void test_padding(const std::vector &fields,
+ const std::vector &expected) {
+  RegisterFlags rf("", 4, fields);
+  EXPECT_THAT(expected, ::testing::ContainerEq(rf.GetFields()));
+}
+
+TEST(RegisterFlagsTest, RegisterFlagsPadding) {
+  // When creating a set of flags we assume that:
+  // * There are >= 1 field

[Lldb-commits] [PATCH] D145569: [lldb][InstrumentationRuntime] Make 'data' struct anonymous in order to avoid collisions with types in the debuggee

2023-03-08 Thread Michael Buch via Phabricator via lldb-commits

Michael137 created this revision.
Michael137 added reviewers: aprantl, jingham.
Herald added a project: All.
Michael137 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The `UBSAN`/`ASAN` plugins would previously call the internal helper
structure injected into the source expression `struct data { ... };`

This occasionally collided with user-defined types of the same (quite
common) name and lead to failures running the injected
`InstrumentationRuntime` expressions or simply crash during
`ASTImport`.

This patch makes these structures anonymous so there's not
chance of clashing with other types in the program. This is
already the approach taken in `UBSan/InstrumentationRuntimeABSan.cpp`.

**Testing**

- API tests still pass


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145569

Files:
  
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
  lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp


Index: lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
===
--- lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
+++ lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
@@ -67,8 +67,11 @@
 size_t __asan_get_alloc_stack(void *addr, void **trace, size_t size, 
int *thread_id);
 size_t __asan_get_free_stack(void *addr, void **trace, size_t size, 
int *thread_id);
 }
+)";
 
-struct data {
+const char *memory_history_asan_command_format =
+R"(
+struct {
 void *alloc_trace[256];
 size_t alloc_count;
 int alloc_tid;
@@ -76,12 +79,7 @@
 void *free_trace[256];
 size_t free_count;
 int free_tid;
-};
-)";
-
-const char *memory_history_asan_command_format =
-R"(
-data t;
+} t;
 
 t.alloc_count = __asan_get_alloc_stack((void *)0x%)" PRIx64
 R"(, t.alloc_trace, 256, &t.alloc_tid);
Index: 
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
===
--- 
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
+++ 
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
@@ -67,19 +67,18 @@
 const char **OutMessage, const char **OutFilename, unsigned *OutLine,
 unsigned *OutCol, char **OutMemoryAddr);
 }
+)";
 
-struct data {
+static const char *ub_sanitizer_retrieve_report_data_command = R"(
+struct {
   const char *issue_kind;
   const char *message;
   const char *filename;
   unsigned line;
   unsigned col;
   char *memory_addr;
-};
-)";
+} t;
 
-static const char *ub_sanitizer_retrieve_report_data_command = R"(
-data t;
 __ubsan_get_current_report_data(&t.issue_kind, &t.message, &t.filename, 
&t.line,
 &t.col, &t.memory_addr);
 t;


Index: lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
===
--- lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
+++ lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
@@ -67,8 +67,11 @@
 size_t __asan_get_alloc_stack(void *addr, void **trace, size_t size, int *thread_id);
 size_t __asan_get_free_stack(void *addr, void **trace, size_t size, int *thread_id);
 }
+)";
 
-struct data {
+const char *memory_history_asan_command_format =
+R"(
+struct {
 void *alloc_trace[256];
 size_t alloc_count;
 int alloc_tid;
@@ -76,12 +79,7 @@
 void *free_trace[256];
 size_t free_count;
 int free_tid;
-};
-)";
-
-const char *memory_history_asan_command_format =
-R"(
-data t;
+} t;
 
 t.alloc_count = __asan_get_alloc_stack((void *)0x%)" PRIx64
 R"(, t.alloc_trace, 256, &t.alloc_tid);
Index: lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
===
--- lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
+++ lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
@@ -67,19 +67,18 @@
 const char **OutMessage, const char **OutFilename, unsigned *OutLine,
 unsigned *OutCol, char **OutMemoryAddr);
 }
+)";
 
-struct data {
+static const char *ub_sanitizer_retrieve_report_data_command = R"(
+struct {
   const char *issue_kind;
   const char *message;
   const char *filename;
   unsigned line;
   unsigned col;
   char *memory_addr;
-};
-)";
+} t;
 
-static const char *ub_sanitizer_retrieve_report_data_command = R"(
-data t;
 __ubsan_get_current_report_data(&t.issue_kind, &t.message, &t.filename, &t.line,
 &t.col, &t.memory_addr);
 t;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin

[Lldb-commits] [PATCH] D145571: [LLDB][ObjectFileELF] Correct the return type of Reloc{Offset, Addend}32

2023-03-08 Thread Lu Weining via Phabricator via lldb-commits

SixWeining created this revision.
SixWeining added a reviewer: DavidSpickett.
Herald added a subscriber: emaste.
Herald added a project: All.
SixWeining requested review of this revision.
Herald added subscribers: lldb-commits, MaskRay.
Herald added a project: LLDB.

This is a follow up of D145550 .

I think Reloc{Type,Symbol}{32,64} can keep unchanged as they are not
directly returning a field of the ELFRel[a] struct.

Depends on D145550 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145571

Files:
  lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp


Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -114,11 +114,11 @@
 
   static unsigned RelocSymbol64(const ELFRelocation &rel);
 
-  static unsigned RelocOffset32(const ELFRelocation &rel);
+  static elf_addr RelocOffset32(const ELFRelocation &rel);
 
   static elf_addr RelocOffset64(const ELFRelocation &rel);
 
-  static unsigned RelocAddend32(const ELFRelocation &rel);
+  static elf_sxword RelocAddend32(const ELFRelocation &rel);
 
   static elf_sxword RelocAddend64(const ELFRelocation &rel);
 
@@ -185,7 +185,7 @@
 return ELFRela::RelocSymbol64(*rel.reloc.get());
 }
 
-unsigned ELFRelocation::RelocOffset32(const ELFRelocation &rel) {
+elf_addr ELFRelocation::RelocOffset32(const ELFRelocation &rel) {
   if (rel.reloc.is())
 return rel.reloc.get()->r_offset;
   else
@@ -199,7 +199,7 @@
 return rel.reloc.get()->r_offset;
 }
 
-unsigned ELFRelocation::RelocAddend32(const ELFRelocation &rel) {
+elf_sxword ELFRelocation::RelocAddend32(const ELFRelocation &rel) {
   if (rel.reloc.is())
 return 0;
   else


Index: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -114,11 +114,11 @@
 
   static unsigned RelocSymbol64(const ELFRelocation &rel);
 
-  static unsigned RelocOffset32(const ELFRelocation &rel);
+  static elf_addr RelocOffset32(const ELFRelocation &rel);
 
   static elf_addr RelocOffset64(const ELFRelocation &rel);
 
-  static unsigned RelocAddend32(const ELFRelocation &rel);
+  static elf_sxword RelocAddend32(const ELFRelocation &rel);
 
   static elf_sxword RelocAddend64(const ELFRelocation &rel);
 
@@ -185,7 +185,7 @@
 return ELFRela::RelocSymbol64(*rel.reloc.get());
 }
 
-unsigned ELFRelocation::RelocOffset32(const ELFRelocation &rel) {
+elf_addr ELFRelocation::RelocOffset32(const ELFRelocation &rel) {
   if (rel.reloc.is())
 return rel.reloc.get()->r_offset;
   else
@@ -199,7 +199,7 @@
 return rel.reloc.get()->r_offset;
 }
 
-unsigned ELFRelocation::RelocAddend32(const ELFRelocation &rel) {
+elf_sxword ELFRelocation::RelocAddend32(const ELFRelocation &rel) {
   if (rel.reloc.is())
 return 0;
   else
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145571: [LLDB][ObjectFileELF] Correct the return type of Reloc{Offset, Addend}32

2023-03-08 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.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145571

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

[Lldb-commits] [PATCH] D145574: [lldb] Read register fields from target XML

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

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

This teaches ProcessGDBRemote to look for "flags" nodes
in the target XML that tell you what fields a register has.

https://sourceware.org/gdb/onlinedocs/gdb/Target-Description-Format.html

It will check for various invalid inputs like:

- Flags nodes with 0 fields in them.
- Start or end being > the register size.
- Fields that overlap.
- Required properties not being present (e.g. no name).
- Flag sets being redefined.

If anything untoward is found, we'll just drop the field or the
flag set altogether. Register fields are a "nice to have" so LLDB
shouldn't be crashing because of them, instead just log anything
we throw away. So the user can fix their XML/file a bug with their
vendor.

Once that is done it will sort the fields and pass them to
the RegisterFields class I added previously.

There is no way to see these fields yet, so tests for this code
will come later when the formatting code is added.

The fields are stored in a map of unique pointers on the
ProcessGDBRemote class. It will give out raw pointers on the
assumption that the GDB process lives longer than the users
of those pointers do. Which means RegisterInfo is still a trivial struct
but we are properly destroying the fields when the GDB process ends.

We can't store the fields directly in the map because adding new
items may cause its storage to be reallocated, which would invalidate
pointers we've already given out.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145574

Files:
  lldb/include/lldb/Target/DynamicRegisterInfo.h
  lldb/include/lldb/lldb-private-types.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Target/DynamicRegisterInfo.cpp

Index: lldb/source/Target/DynamicRegisterInfo.cpp
===
--- lldb/source/Target/DynamicRegisterInfo.cpp
+++ lldb/source/Target/DynamicRegisterInfo.cpp
@@ -407,7 +407,7 @@
   {reg.regnum_ehframe, reg.regnum_dwarf, reg.regnum_generic,
reg.regnum_remote, local_regnum},
   // value_regs and invalidate_regs are filled by Finalize()
-  nullptr, nullptr, nullptr
+  nullptr, nullptr, reg.flags_type
 };
 
 m_regs.push_back(reg_info);
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -38,6 +38,7 @@
 #include "GDBRemoteRegisterContext.h"
 
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/StringMap.h"
 
 namespace lldb_private {
 namespace repro {
@@ -466,6 +467,15 @@
   // fork helpers
   void DidForkSwitchSoftwareBreakpoints(bool enable);
   void DidForkSwitchHardwareTraps(bool enable);
+
+  // Lists of register fields generated from the remote's target XML.
+  // Pointers to these RegisterFlags will be set in the register info passed
+  // back to the upper levels of lldb. Doing so is safe because this class will
+  // live at least as long as the debug session. We therefore do not store the
+  // data directly in the map because the map may reallocate it's storage as new
+  // entries are added. Which would invalidate any pointers set in the register
+  // info up to that point.
+  llvm::StringMap> m_registers_flags_types;
 };
 
 } // namespace process_gdb_remote
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -53,6 +53,7 @@
 #include "lldb/Target/ABI.h"
 #include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/MemoryRegionInfo.h"
+#include "lldb/Target/RegisterFlags.h"
 #include "lldb/Target/SystemRuntime.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/TargetList.h"
@@ -84,6 +85,7 @@
 #include "lldb/Utility/StringExtractorGDBRemote.h"
 
 #include "llvm/ADT/ScopeExit.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/FormatAdapters.h"
 #include "llvm/Support/Threading.h"
@@ -4029,15 +4031,214 @@
   RegisterSetMap reg_set_map;
 };
 
-bool ParseRegisters(XMLNode feature_node, GdbServerTargetInfo &target_info,
-std::vector ®isters) {
+static std::vector ParseFlagsFields(XMLNode flags_node,
+  unsigned size) {
+  Log *log(GetLog(GDBRLog::Process));
+  const unsigned max_start_bit = size * 8 - 1;
+
+  // Process the fields of this set of flags.
+  std::vector fields;
+  flags_node.ForEachChildElementWithName("field", [

[Lldb-commits] [PATCH] D145569: [lldb][InstrumentationRuntime] Make 'data' struct anonymous in order to avoid collisions with types in the debuggee

2023-03-08 Thread Michael Buch via Phabricator via lldb-commits

Michael137 added a comment.

Not sure there's a test to be added here.

Of course I could add a dummy data type to the API tests called `struct data 
{};` and confirm that it doesn't crash (I did this manually locally), but it 
doesn't seem like something that would catch regressions in the future. But if 
people want I can add them in


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145569

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

[Lldb-commits] [PATCH] D145569: [lldb][InstrumentationRuntime] Make 'data' struct anonymous in order to avoid collisions with types in the debuggee

2023-03-08 Thread Michael Buch via Phabricator via lldb-commits

Michael137 updated this revision to Diff 503308.
Michael137 added a comment.

- Update commit message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145569

Files:
  
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
  lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp


Index: lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
===
--- lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
+++ lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
@@ -67,8 +67,11 @@
 size_t __asan_get_alloc_stack(void *addr, void **trace, size_t size, 
int *thread_id);
 size_t __asan_get_free_stack(void *addr, void **trace, size_t size, 
int *thread_id);
 }
+)";
 
-struct data {
+const char *memory_history_asan_command_format =
+R"(
+struct {
 void *alloc_trace[256];
 size_t alloc_count;
 int alloc_tid;
@@ -76,12 +79,7 @@
 void *free_trace[256];
 size_t free_count;
 int free_tid;
-};
-)";
-
-const char *memory_history_asan_command_format =
-R"(
-data t;
+} t;
 
 t.alloc_count = __asan_get_alloc_stack((void *)0x%)" PRIx64
 R"(, t.alloc_trace, 256, &t.alloc_tid);
Index: 
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
===
--- 
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
+++ 
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
@@ -67,19 +67,18 @@
 const char **OutMessage, const char **OutFilename, unsigned *OutLine,
 unsigned *OutCol, char **OutMemoryAddr);
 }
+)";
 
-struct data {
+static const char *ub_sanitizer_retrieve_report_data_command = R"(
+struct {
   const char *issue_kind;
   const char *message;
   const char *filename;
   unsigned line;
   unsigned col;
   char *memory_addr;
-};
-)";
+} t;
 
-static const char *ub_sanitizer_retrieve_report_data_command = R"(
-data t;
 __ubsan_get_current_report_data(&t.issue_kind, &t.message, &t.filename, 
&t.line,
 &t.col, &t.memory_addr);
 t;


Index: lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
===
--- lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
+++ lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
@@ -67,8 +67,11 @@
 size_t __asan_get_alloc_stack(void *addr, void **trace, size_t size, int *thread_id);
 size_t __asan_get_free_stack(void *addr, void **trace, size_t size, int *thread_id);
 }
+)";
 
-struct data {
+const char *memory_history_asan_command_format =
+R"(
+struct {
 void *alloc_trace[256];
 size_t alloc_count;
 int alloc_tid;
@@ -76,12 +79,7 @@
 void *free_trace[256];
 size_t free_count;
 int free_tid;
-};
-)";
-
-const char *memory_history_asan_command_format =
-R"(
-data t;
+} t;
 
 t.alloc_count = __asan_get_alloc_stack((void *)0x%)" PRIx64
 R"(, t.alloc_trace, 256, &t.alloc_tid);
Index: lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
===
--- lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
+++ lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
@@ -67,19 +67,18 @@
 const char **OutMessage, const char **OutFilename, unsigned *OutLine,
 unsigned *OutCol, char **OutMemoryAddr);
 }
+)";
 
-struct data {
+static const char *ub_sanitizer_retrieve_report_data_command = R"(
+struct {
   const char *issue_kind;
   const char *message;
   const char *filename;
   unsigned line;
   unsigned col;
   char *memory_addr;
-};
-)";
+} t;
 
-static const char *ub_sanitizer_retrieve_report_data_command = R"(
-data t;
 __ubsan_get_current_report_data(&t.issue_kind, &t.message, &t.filename, &t.line,
 &t.col, &t.memory_addr);
 t;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145580: [lldb] Show register fields using bitfield struct types

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett created this revision.
Herald added subscribers: ChuanqiXu, kristof.beyls.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added projects: LLDB, LLVM.
Herald added subscribers: llvm-commits, lldb-commits.

This change uses the information from target.xml sent by
the GDB stub to produce C types that we can use to print
register fields.

lldb-server *does not* produce this information yet. This will
only work with GDB stubs that do. gdbserver or qemu
are 2 I know of. Testing is added that uses a mocked lldb-server.

  (lldb) register read cpsr
  cpsr = 0x60001000
   (N = 0, Z = 1, C = 1, V = 0, TCO = 0, DIT = 0, UAO = 0,
PAN = 0, SS = 0, IL = 0, SSBS = 1, D = 0, A = 0, I = 0,
F = 0, nRW = 0, EL = 0, SP = 0)

Only "register read" will display fields, and only when
we are not printing a register block.

For example, cpsr is a 32 bit register. Using the target's scratch type
system we construct a type:

  struct __attribute__((__packed__)) cpsr {
uint32_t N : 1;
uint32_t Z : 1;
...
uint32_t EL : 2;
uint32_t SP : 1;
  };

If this register had unallocated bits in it, those would
have been filled in by RegisterFlags as anonymous fields.
A new option "SetChildPrintingDecider" is added so we
can disable printing those.

Important things about this type:

- It is packed so that sizeof(struct cpsr) == sizeof(the real register). (this 
will hold for all flags types we create)
- Each field has the same storage type, which is the same as the type of the 
raw register value. This prevents fields being spilt over into more storage 
units, as is allowed by most ABIs.
- Each bitfield size matches that of its register field.
- The most significant field is first.

The last point is required because the most significant bit (MSB)
being on the left/top of a print out matches what you'd expect to
see in an architecture manual. In addition, having lldb print a
different field order on big/little endian hosts is not acceptable.

As a consequence, if the target is little endian we have to
reverse the order of the fields in the value. The value of each field
remains the same. For example 0b01 doesn't become 0b10, it just shifts
up or down.

This is needed because clang's type system assumes that for a struct
like the one above, the least significant bit (LSB) will be first
for a little endian target. We need the MSB to be first.

Finally, if lldb's host is a different endian to the target we have
to byte swap the host endian value to match the endian of the target's
typesystem.

| Host Endian | Target Endian | Field Order Swap | Byte Order Swap |
| --- | - |  | --- |
| Little  | Little| Yes  | No  |
| Big | Little| Yes  | Yes |
| Little  | Big   | No   | Yes |
| Big | Big   | No   | No  |
|

Testing was done as follows:

- Little -> Little
  - LE AArch64 native debug.
- Big -> Little
  - s390x lldb running under QEMU, connected to LE AArch64 target.
- Little -> Big
  - LE AArch64 lldb connected to QEMU's GDB stub, which is running an s390x 
program.
- Big -> Big
  - s390x lldb running under QEMU, connected to another QEMU's GDB stub, which 
is running an s390x program.

Cross endian setups are very unlikely to be tested by buildbots
but I wanted to be sure this worked on s390x (our only supported BE
target).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145580

Files:
  lldb/include/lldb/Core/DumpRegisterValue.h
  lldb/include/lldb/DataFormatters/DumpValueObjectOptions.h
  lldb/include/lldb/Target/RegisterFlags.h
  lldb/source/Commands/CommandObjectRegister.cpp
  lldb/source/Core/DumpRegisterValue.cpp
  lldb/source/DataFormatters/DumpValueObjectOptions.cpp
  lldb/source/DataFormatters/ValueObjectPrinter.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
  lldb/unittests/Target/RegisterFlagsTest.cpp
  llvm/docs/ReleaseNotes.rst

Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -188,6 +188,12 @@
   omit defaulted template parameters. The full template parameter list can still be
   viewed with `expr --raw-output`/`frame var --raw-output`. (`D141828 `_)
 
+* LLDB can now display register fields if they are described in target XML sent
+  by a debug server such as ``gdbserver`` (``lldb-server`` does not currently produce
+  this information). Fields are only printed when reading named registers, for
+  example ``register read cpsr``. They are not shown when reading a register set,
+  ``register read -s 0``.
+
 Changes to Sanitizers
 -
 
Index: lldb/unittests/Target/RegisterFlagsTest.cpp
=

[Lldb-commits] [PATCH] D145574: [lldb] Read register fields from target XML

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added a comment.

Tests are added in https://reviews.llvm.org/D145580, because they use the 
formatting code added there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145574

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

[Lldb-commits] [lldb] 8fe1144 - [lldb] Remove unused POSIX ProcessMessage files

2023-03-08 Thread David Spickett via lldb-commits


Author: David Spickett
Date: 2023-03-08T13:18:52Z
New Revision: 8fe11442855151a790e1a1ffa92442c5ee5203c1

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

LOG: [lldb] Remove unused POSIX ProcessMessage files

The last use of these was removed in cd443398566b953642ead7c81528ab5b4e211eb9.

Reviewed By: emaste

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

Added: 


Modified: 
lldb/source/Plugins/Process/POSIX/CMakeLists.txt
llvm/utils/gn/secondary/lldb/source/Plugins/Process/POSIX/BUILD.gn

Removed: 
lldb/source/Plugins/Process/POSIX/ProcessMessage.cpp
lldb/source/Plugins/Process/POSIX/ProcessMessage.h



diff  --git a/lldb/source/Plugins/Process/POSIX/CMakeLists.txt 
b/lldb/source/Plugins/Process/POSIX/CMakeLists.txt
index 2db4e795b96bb..81effaaf35585 100644
--- a/lldb/source/Plugins/Process/POSIX/CMakeLists.txt
+++ b/lldb/source/Plugins/Process/POSIX/CMakeLists.txt
@@ -1,7 +1,6 @@
 add_lldb_library(lldbPluginProcessPOSIX
   CrashReason.cpp
   NativeProcessELF.cpp
-  ProcessMessage.cpp
   ProcessPOSIXLog.cpp
 
   LINK_LIBS

diff  --git a/lldb/source/Plugins/Process/POSIX/ProcessMessage.cpp 
b/lldb/source/Plugins/Process/POSIX/ProcessMessage.cpp
deleted file mode 100644
index 4e8c6f1ba2d2c..0
--- a/lldb/source/Plugins/Process/POSIX/ProcessMessage.cpp
+++ /dev/null
@@ -1,61 +0,0 @@
-//===-- ProcessMessage.cpp 
===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#include "ProcessMessage.h"
-
-using namespace lldb_private;
-
-const char *ProcessMessage::PrintCrashReason() const {
-  return CrashReasonAsString(m_crash_reason);
-}
-
-const char *ProcessMessage::PrintKind(Kind kind) {
-  const char *str = nullptr;
-
-  switch (kind) {
-  case eInvalidMessage:
-str = "eInvalidMessage";
-break;
-  case eAttachMessage:
-str = "eAttachMessage";
-break;
-  case eExitMessage:
-str = "eExitMessage";
-break;
-  case eLimboMessage:
-str = "eLimboMessage";
-break;
-  case eSignalMessage:
-str = "eSignalMessage";
-break;
-  case eSignalDeliveredMessage:
-str = "eSignalDeliveredMessage";
-break;
-  case eTraceMessage:
-str = "eTraceMessage";
-break;
-  case eBreakpointMessage:
-str = "eBreakpointMessage";
-break;
-  case eWatchpointMessage:
-str = "eWatchpointMessage";
-break;
-  case eCrashMessage:
-str = "eCrashMessage";
-break;
-  case eNewThreadMessage:
-str = "eNewThreadMessage";
-break;
-  case eExecMessage:
-str = "eExecMessage";
-break;
-  }
-  return str;
-}
-
-const char *ProcessMessage::PrintKind() const { return PrintKind(m_kind); }

diff  --git a/lldb/source/Plugins/Process/POSIX/ProcessMessage.h 
b/lldb/source/Plugins/Process/POSIX/ProcessMessage.h
deleted file mode 100644
index d9c10caaa95e9..0
--- a/lldb/source/Plugins/Process/POSIX/ProcessMessage.h
+++ /dev/null
@@ -1,168 +0,0 @@
-//===-- ProcessMessage.h *- C++ 
-*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#ifndef liblldb_ProcessMessage_H_
-#define liblldb_ProcessMessage_H_
-
-#include "CrashReason.h"
-
-#include 
-#include 
-
-#include "lldb/lldb-defines.h"
-#include "lldb/lldb-types.h"
-
-class ProcessMessage {
-public:
-  /// The type of signal this message can correspond to.
-  enum Kind {
-eInvalidMessage,
-eAttachMessage,
-eExitMessage,
-eLimboMessage,
-eSignalMessage,
-eSignalDeliveredMessage,
-eTraceMessage,
-eBreakpointMessage,
-eWatchpointMessage,
-eCrashMessage,
-eNewThreadMessage,
-eExecMessage
-  };
-
-  ProcessMessage()
-  : m_tid(LLDB_INVALID_PROCESS_ID), m_kind(eInvalidMessage),
-m_crash_reason(CrashReason::eInvalidCrashReason), m_status(0),
-m_addr(0) {}
-
-  Kind GetKind() const { return m_kind; }
-
-  lldb::tid_t GetTID() const { return m_tid; }
-
-  /// Indicates that the process \p pid has successfully attached.
-  static ProcessMessage Attach(lldb::pid_t pid) {
-return ProcessMessage(pid, eAttachMessage);
-  }
-
-  /// Indicates that the thread \p tid is about to exit with status \p status.
-  static ProcessMessage Limbo(lldb::tid_t tid, int status) {
-return ProcessMessage(tid, eLimboMessage, status);
-  }

[Lldb-commits] [PATCH] D145559: [lldb] Remove unused POSIX ProcessMessage files

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8fe114428551: [lldb] Remove unused POSIX ProcessMessage 
files (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145559

Files:
  lldb/source/Plugins/Process/POSIX/CMakeLists.txt
  lldb/source/Plugins/Process/POSIX/ProcessMessage.cpp
  lldb/source/Plugins/Process/POSIX/ProcessMessage.h
  llvm/utils/gn/secondary/lldb/source/Plugins/Process/POSIX/BUILD.gn

Index: llvm/utils/gn/secondary/lldb/source/Plugins/Process/POSIX/BUILD.gn
===
--- llvm/utils/gn/secondary/lldb/source/Plugins/Process/POSIX/BUILD.gn
+++ llvm/utils/gn/secondary/lldb/source/Plugins/Process/POSIX/BUILD.gn
@@ -12,7 +12,6 @@
   sources = [
 "CrashReason.cpp",
 "NativeProcessELF.cpp",
-"ProcessMessage.cpp",
 "ProcessPOSIXLog.cpp",
   ]
 }
Index: lldb/source/Plugins/Process/POSIX/ProcessMessage.h
===
--- lldb/source/Plugins/Process/POSIX/ProcessMessage.h
+++ /dev/null
@@ -1,168 +0,0 @@
-//===-- ProcessMessage.h *- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#ifndef liblldb_ProcessMessage_H_
-#define liblldb_ProcessMessage_H_
-
-#include "CrashReason.h"
-
-#include 
-#include 
-
-#include "lldb/lldb-defines.h"
-#include "lldb/lldb-types.h"
-
-class ProcessMessage {
-public:
-  /// The type of signal this message can correspond to.
-  enum Kind {
-eInvalidMessage,
-eAttachMessage,
-eExitMessage,
-eLimboMessage,
-eSignalMessage,
-eSignalDeliveredMessage,
-eTraceMessage,
-eBreakpointMessage,
-eWatchpointMessage,
-eCrashMessage,
-eNewThreadMessage,
-eExecMessage
-  };
-
-  ProcessMessage()
-  : m_tid(LLDB_INVALID_PROCESS_ID), m_kind(eInvalidMessage),
-m_crash_reason(CrashReason::eInvalidCrashReason), m_status(0),
-m_addr(0) {}
-
-  Kind GetKind() const { return m_kind; }
-
-  lldb::tid_t GetTID() const { return m_tid; }
-
-  /// Indicates that the process \p pid has successfully attached.
-  static ProcessMessage Attach(lldb::pid_t pid) {
-return ProcessMessage(pid, eAttachMessage);
-  }
-
-  /// Indicates that the thread \p tid is about to exit with status \p status.
-  static ProcessMessage Limbo(lldb::tid_t tid, int status) {
-return ProcessMessage(tid, eLimboMessage, status);
-  }
-
-  /// Indicates that the thread \p tid had the signal \p signum delivered.
-  static ProcessMessage Signal(lldb::tid_t tid, int signum) {
-return ProcessMessage(tid, eSignalMessage, signum);
-  }
-
-  /// Indicates that a signal \p signum generated by the debugging process was
-  /// delivered to the thread \p tid.
-  static ProcessMessage SignalDelivered(lldb::tid_t tid, int signum) {
-return ProcessMessage(tid, eSignalDeliveredMessage, signum);
-  }
-
-  /// Indicates that the thread \p tid encountered a trace point.
-  static ProcessMessage Trace(lldb::tid_t tid) {
-return ProcessMessage(tid, eTraceMessage);
-  }
-
-  /// Indicates that the thread \p tid encountered a break point.
-  static ProcessMessage Break(lldb::tid_t tid) {
-return ProcessMessage(tid, eBreakpointMessage);
-  }
-
-  static ProcessMessage Watch(lldb::tid_t tid, lldb::addr_t wp_addr) {
-return ProcessMessage(tid, eWatchpointMessage, 0, wp_addr);
-  }
-
-  /// Indicates that the thread \p tid crashed.
-  static ProcessMessage Crash(lldb::pid_t pid, CrashReason reason, int signo,
-  lldb::addr_t fault_addr) {
-ProcessMessage message(pid, eCrashMessage, signo, fault_addr);
-message.m_crash_reason = reason;
-return message;
-  }
-
-  /// Indicates that the thread \p child_tid was spawned.
-  static ProcessMessage NewThread(lldb::tid_t parent_tid,
-  lldb::tid_t child_tid) {
-return ProcessMessage(parent_tid, eNewThreadMessage, child_tid);
-  }
-
-  /// Indicates that the thread \p tid is about to exit with status \p status.
-  static ProcessMessage Exit(lldb::tid_t tid, int status) {
-return ProcessMessage(tid, eExitMessage, status);
-  }
-
-  /// Indicates that the thread \p pid has exec'd.
-  static ProcessMessage Exec(lldb::tid_t tid) {
-return ProcessMessage(tid, eExecMessage);
-  }
-
-  int GetExitStatus() const {
-assert(GetKind() == eExitMessage || GetKind() == eLimboMessage);
-return m_status;
-  }
-
-  int GetSignal() const {
-assert(GetKind() == eSignalMessage || GetKind() == eCrashMessage ||
-   GetKind() == eSignalDeliveredMessage);
-return m_s

[Lldb-commits] [PATCH] D145566: [lldb] Add RegisterFlags class

2023-03-08 Thread Thorsten via Phabricator via lldb-commits

tschuett added inline comments.



Comment at: lldb/unittests/Target/RegisterFlagsTest.cpp:125
+}
\ No newline at end of file


Missing line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145566

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

[Lldb-commits] [PATCH] D145566: [lldb] Add RegisterFlags class

2023-03-08 Thread Thorsten via Phabricator via lldb-commits

tschuett added inline comments.



Comment at: lldb/include/lldb/Target/RegisterFlags.h:20
+  public:
+Field(const std::string &name, unsigned start, unsigned end,
+  const std::string &type)

There are a lot of `const std::string& ` . Are you allowed to use 
`llvm::StringRef` or even `std::string_view` in LLDB?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145566

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

[Lldb-commits] [PATCH] D145561: [lldb] Remove unused CrashReasonAsString function

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb7217a8fc94d: [lldb] Remove unused CrashReasonAsString 
function (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145561

Files:
  lldb/source/Plugins/Process/POSIX/CrashReason.cpp
  lldb/source/Plugins/Process/POSIX/CrashReason.h

Index: lldb/source/Plugins/Process/POSIX/CrashReason.h
===
--- lldb/source/Plugins/Process/POSIX/CrashReason.h
+++ lldb/source/Plugins/Process/POSIX/CrashReason.h
@@ -54,8 +54,6 @@
 std::string GetCrashReasonString(CrashReason reason, lldb::addr_t fault_addr);
 std::string GetCrashReasonString(CrashReason reason, const siginfo_t &info);
 
-const char *CrashReasonAsString(CrashReason reason);
-
 CrashReason GetCrashReason(const siginfo_t &info);
 
 #endif // #ifndef liblldb_CrashReason_H_
Index: lldb/source/Plugins/Process/POSIX/CrashReason.cpp
===
--- lldb/source/Plugins/Process/POSIX/CrashReason.cpp
+++ lldb/source/Plugins/Process/POSIX/CrashReason.cpp
@@ -247,97 +247,6 @@
   return str;
 }
 
-const char *CrashReasonAsString(CrashReason reason) {
-  const char *str = nullptr;
-
-  switch (reason) {
-  case CrashReason::eInvalidCrashReason:
-str = "eInvalidCrashReason";
-break;
-
-  // SIGSEGV crash reasons.
-  case CrashReason::eInvalidAddress:
-str = "eInvalidAddress";
-break;
-  case CrashReason::ePrivilegedAddress:
-str = "ePrivilegedAddress";
-break;
-  case CrashReason::eBoundViolation:
-str = "eBoundViolation";
-break;
-  case CrashReason::eAsyncTagCheckFault:
-str = "eAsyncTagCheckFault";
-break;
-  case CrashReason::eSyncTagCheckFault:
-str = "eSyncTagCheckFault";
-break;
-
-  // SIGILL crash reasons.
-  case CrashReason::eIllegalOpcode:
-str = "eIllegalOpcode";
-break;
-  case CrashReason::eIllegalOperand:
-str = "eIllegalOperand";
-break;
-  case CrashReason::eIllegalAddressingMode:
-str = "eIllegalAddressingMode";
-break;
-  case CrashReason::eIllegalTrap:
-str = "eIllegalTrap";
-break;
-  case CrashReason::ePrivilegedOpcode:
-str = "ePrivilegedOpcode";
-break;
-  case CrashReason::ePrivilegedRegister:
-str = "ePrivilegedRegister";
-break;
-  case CrashReason::eCoprocessorError:
-str = "eCoprocessorError";
-break;
-  case CrashReason::eInternalStackError:
-str = "eInternalStackError";
-break;
-
-  // SIGBUS crash reasons:
-  case CrashReason::eIllegalAlignment:
-str = "eIllegalAlignment";
-break;
-  case CrashReason::eIllegalAddress:
-str = "eIllegalAddress";
-break;
-  case CrashReason::eHardwareError:
-str = "eHardwareError";
-break;
-
-  // SIGFPE crash reasons:
-  case CrashReason::eIntegerDivideByZero:
-str = "eIntegerDivideByZero";
-break;
-  case CrashReason::eIntegerOverflow:
-str = "eIntegerOverflow";
-break;
-  case CrashReason::eFloatDivideByZero:
-str = "eFloatDivideByZero";
-break;
-  case CrashReason::eFloatOverflow:
-str = "eFloatOverflow";
-break;
-  case CrashReason::eFloatUnderflow:
-str = "eFloatUnderflow";
-break;
-  case CrashReason::eFloatInexactResult:
-str = "eFloatInexactResult";
-break;
-  case CrashReason::eFloatInvalidOperation:
-str = "eFloatInvalidOperation";
-break;
-  case CrashReason::eFloatSubscriptRange:
-str = "eFloatSubscriptRange";
-break;
-  }
-  return str;
-}
-
 CrashReason GetCrashReason(const siginfo_t &info) {
   switch (info.si_signo) {
   case SIGSEGV:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] b7217a8 - [lldb] Remove unused CrashReasonAsString function

2023-03-08 Thread David Spickett via lldb-commits


Author: David Spickett
Date: 2023-03-08T14:27:59Z
New Revision: b7217a8fc94d33f44ec8a2e695e75f70a6eeea1e

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

LOG: [lldb] Remove unused CrashReasonAsString function

The last user was ProcessMessage, which has itself been removed.

Reviewed By: emaste

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

Added: 


Modified: 
lldb/source/Plugins/Process/POSIX/CrashReason.cpp
lldb/source/Plugins/Process/POSIX/CrashReason.h

Removed: 




diff  --git a/lldb/source/Plugins/Process/POSIX/CrashReason.cpp 
b/lldb/source/Plugins/Process/POSIX/CrashReason.cpp
index c6ede61cfe1ba..30fabc071af6f 100644
--- a/lldb/source/Plugins/Process/POSIX/CrashReason.cpp
+++ b/lldb/source/Plugins/Process/POSIX/CrashReason.cpp
@@ -247,97 +247,6 @@ std::string GetCrashReasonString(CrashReason reason, 
lldb::addr_t fault_addr) {
   return str;
 }
 
-const char *CrashReasonAsString(CrashReason reason) {
-  const char *str = nullptr;
-
-  switch (reason) {
-  case CrashReason::eInvalidCrashReason:
-str = "eInvalidCrashReason";
-break;
-
-  // SIGSEGV crash reasons.
-  case CrashReason::eInvalidAddress:
-str = "eInvalidAddress";
-break;
-  case CrashReason::ePrivilegedAddress:
-str = "ePrivilegedAddress";
-break;
-  case CrashReason::eBoundViolation:
-str = "eBoundViolation";
-break;
-  case CrashReason::eAsyncTagCheckFault:
-str = "eAsyncTagCheckFault";
-break;
-  case CrashReason::eSyncTagCheckFault:
-str = "eSyncTagCheckFault";
-break;
-
-  // SIGILL crash reasons.
-  case CrashReason::eIllegalOpcode:
-str = "eIllegalOpcode";
-break;
-  case CrashReason::eIllegalOperand:
-str = "eIllegalOperand";
-break;
-  case CrashReason::eIllegalAddressingMode:
-str = "eIllegalAddressingMode";
-break;
-  case CrashReason::eIllegalTrap:
-str = "eIllegalTrap";
-break;
-  case CrashReason::ePrivilegedOpcode:
-str = "ePrivilegedOpcode";
-break;
-  case CrashReason::ePrivilegedRegister:
-str = "ePrivilegedRegister";
-break;
-  case CrashReason::eCoprocessorError:
-str = "eCoprocessorError";
-break;
-  case CrashReason::eInternalStackError:
-str = "eInternalStackError";
-break;
-
-  // SIGBUS crash reasons:
-  case CrashReason::eIllegalAlignment:
-str = "eIllegalAlignment";
-break;
-  case CrashReason::eIllegalAddress:
-str = "eIllegalAddress";
-break;
-  case CrashReason::eHardwareError:
-str = "eHardwareError";
-break;
-
-  // SIGFPE crash reasons:
-  case CrashReason::eIntegerDivideByZero:
-str = "eIntegerDivideByZero";
-break;
-  case CrashReason::eIntegerOverflow:
-str = "eIntegerOverflow";
-break;
-  case CrashReason::eFloatDivideByZero:
-str = "eFloatDivideByZero";
-break;
-  case CrashReason::eFloatOverflow:
-str = "eFloatOverflow";
-break;
-  case CrashReason::eFloatUnderflow:
-str = "eFloatUnderflow";
-break;
-  case CrashReason::eFloatInexactResult:
-str = "eFloatInexactResult";
-break;
-  case CrashReason::eFloatInvalidOperation:
-str = "eFloatInvalidOperation";
-break;
-  case CrashReason::eFloatSubscriptRange:
-str = "eFloatSubscriptRange";
-break;
-  }
-  return str;
-}
-
 CrashReason GetCrashReason(const siginfo_t &info) {
   switch (info.si_signo) {
   case SIGSEGV:

diff  --git a/lldb/source/Plugins/Process/POSIX/CrashReason.h 
b/lldb/source/Plugins/Process/POSIX/CrashReason.h
index 24acdc08e9007..466cd14e69842 100644
--- a/lldb/source/Plugins/Process/POSIX/CrashReason.h
+++ b/lldb/source/Plugins/Process/POSIX/CrashReason.h
@@ -54,8 +54,6 @@ enum class CrashReason {
 std::string GetCrashReasonString(CrashReason reason, lldb::addr_t fault_addr);
 std::string GetCrashReasonString(CrashReason reason, const siginfo_t &info);
 
-const char *CrashReasonAsString(CrashReason reason);
-
 CrashReason GetCrashReason(const siginfo_t &info);
 
 #endif // #ifndef liblldb_CrashReason_H_



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

[Lldb-commits] [PATCH] D145566: [lldb] Add RegisterFlags class

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added inline comments.



Comment at: lldb/include/lldb/Target/RegisterFlags.h:20
+  public:
+Field(const std::string &name, unsigned start, unsigned end,
+  const std::string &type)

tschuett wrote:
> There are a lot of `const std::string& ` . Are you allowed to use 
> `llvm::StringRef` or even `std::string_view` in LLDB?
We can use `llvm::StringRef`, but in this case this will be passed a 
std::string so I just defaulted to the most conservative version (this happens 
in a later patch).

I suppose you could move here, or pass by copy and assume the compiler will 
figure it out.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145566

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

[Lldb-commits] [PATCH] D145566: [lldb] Add RegisterFlags class

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added inline comments.



Comment at: lldb/include/lldb/Target/RegisterFlags.h:20
+  public:
+Field(const std::string &name, unsigned start, unsigned end,
+  const std::string &type)

DavidSpickett wrote:
> tschuett wrote:
> > There are a lot of `const std::string& ` . Are you allowed to use 
> > `llvm::StringRef` or even `std::string_view` in LLDB?
> We can use `llvm::StringRef`, but in this case this will be passed a 
> std::string so I just defaulted to the most conservative version (this 
> happens in a later patch).
> 
> I suppose you could move here, or pass by copy and assume the compiler will 
> figure it out.
> 
> 
Actually, it doesn't have to be a std::string the XML parser does give you a 
StringRef.

I will update this and the subsequent patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145566

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

[Lldb-commits] [PATCH] D145566: [lldb] Add RegisterFlags class

2023-03-08 Thread Thorsten via Phabricator via lldb-commits

tschuett added a comment.

My only comment is: views look more modern than const-ref. The efficiency is 
not affected at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145566

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

[Lldb-commits] [PATCH] D145566: [lldb] Add RegisterFlags class

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

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

Use StringRef for parameters. This is what the XML parser gives you
by default in any case.

Internally we want to keep a copy of the string because the XML document
will go away after parsing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145566

Files:
  lldb/include/lldb/Target/RegisterFlags.h
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/RegisterFlags.cpp
  lldb/unittests/Target/CMakeLists.txt
  lldb/unittests/Target/RegisterFlagsTest.cpp

Index: lldb/unittests/Target/RegisterFlagsTest.cpp
===
--- /dev/null
+++ lldb/unittests/Target/RegisterFlagsTest.cpp
@@ -0,0 +1,124 @@
+//===-- RegisterFlagsTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Target/RegisterFlags.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+TEST(RegisterFlagsTest, Field) {
+  // We assume that start <= end is always true, so that is not tested here.
+
+  RegisterFlags::Field f1("abc", 0, 0, "unknown");
+  ASSERT_EQ(f1.GetName(), "abc");
+  ASSERT_EQ(f1.GetType(), "unknown");
+  // start == end means a 1 bit field.
+  ASSERT_EQ(f1.GetSizeInBits(), (unsigned)1);
+  ASSERT_EQ(f1.GetMask(), (uint64_t)1);
+  ASSERT_EQ(f1.GetValue(0), (uint64_t)0);
+  ASSERT_EQ(f1.GetValue(3), (uint64_t)1);
+
+  // End is inclusive meaning that start 0 to end 1 includes bit 1
+  // to make a 2 bit field.
+  RegisterFlags::Field f2("", 0, 1, "");
+  ASSERT_EQ(f2.GetSizeInBits(), (unsigned)2);
+  ASSERT_EQ(f2.GetMask(), (uint64_t)3);
+  ASSERT_EQ(f2.GetValue(UINT64_MAX), (uint64_t)3);
+  ASSERT_EQ(f2.GetValue(UINT64_MAX & ~(uint64_t)3), (uint64_t)0);
+
+  // If the field doesn't start at 0 we need to shift up/down
+  // to account for it.
+  RegisterFlags::Field f3("", 2, 5, "");
+  ASSERT_EQ(f3.GetSizeInBits(), (unsigned)4);
+  ASSERT_EQ(f3.GetMask(), (uint64_t)0x3c);
+  ASSERT_EQ(f3.GetValue(UINT64_MAX), (uint64_t)0xf);
+  ASSERT_EQ(f3.GetValue(UINT64_MAX & ~(uint64_t)0x3c), (uint64_t)0);
+
+  // Fields are sorted lowest starting bit first.
+  ASSERT_TRUE(f2 < f3);
+  ASSERT_FALSE(f3 < f1);
+  ASSERT_FALSE(f1 < f2);
+  ASSERT_FALSE(f1 < f1);
+}
+
+static RegisterFlags::Field make_field(unsigned start, unsigned end) {
+  return RegisterFlags::Field("", start, end, "");
+}
+
+TEST(RegisterFlagsTest, FieldOverlaps) {
+  // Single bit fields
+  ASSERT_FALSE(make_field(0, 0).Overlaps(make_field(1, 1)));
+  ASSERT_TRUE(make_field(1, 1).Overlaps(make_field(1, 1)));
+  ASSERT_FALSE(make_field(1, 1).Overlaps(make_field(3, 3)));
+
+  ASSERT_TRUE(make_field(0, 1).Overlaps(make_field(1, 2)));
+  ASSERT_TRUE(make_field(1, 2).Overlaps(make_field(0, 1)));
+  ASSERT_FALSE(make_field(0, 1).Overlaps(make_field(2, 3)));
+  ASSERT_FALSE(make_field(2, 3).Overlaps(make_field(0, 1)));
+
+  ASSERT_FALSE(make_field(1, 5).Overlaps(make_field(10, 20)));
+  ASSERT_FALSE(make_field(15, 30).Overlaps(make_field(7, 12)));
+}
+
+TEST(RegisterFlagsTest, PaddingDistance) {
+  // We assume that this method is always called with a more significant
+  // (start bit is higher) field first and that they do not overlap.
+
+  // [field 1][field 2]
+  ASSERT_EQ(make_field(1, 1).PaddingDistance(make_field(0, 0)), 0ULL);
+  // [field 1][..][field 2]
+  ASSERT_EQ(make_field(2, 2).PaddingDistance(make_field(0, 0)), 1ULL);
+  // [field 1][field 1][field 2]
+  ASSERT_EQ(make_field(1, 2).PaddingDistance(make_field(0, 0)), 0ULL);
+  // [field 1][30 bits free][field 2]
+  ASSERT_EQ(make_field(31, 31).PaddingDistance(make_field(0, 0)), 30ULL);
+}
+
+static void test_padding(const std::vector &fields,
+ const std::vector &expected) {
+  RegisterFlags rf("", 4, fields);
+  EXPECT_THAT(expected, ::testing::ContainerEq(rf.GetFields()));
+}
+
+TEST(RegisterFlagsTest, RegisterFlagsPadding) {
+  // When creating a set of flags we assume that:
+  // * There are >= 1 fields.
+  // * They are sorted in descending order.
+  // * There may be gaps between each field.
+
+  // Needs no padding
+  auto fields =
+  std::vector{make_field(16, 31), make_field(0, 15)};
+  test_padding(fields, fields);
+
+  // Needs padding in between the fields, single bit.
+  test_padding({make_field(17, 31), make_field(0, 15)},
+   {make_field(17, 31), make_field(16, 16), make_field(0, 15)});
+  // Multiple bits of padding.
+  test_padding({make_field(17, 31), make_field(0, 14)},
+   {make_field(17, 31), make_field(15, 16), make_field(0, 14)});
+
+  // Padding before

[Lldb-commits] [PATCH] D145574: [lldb] Read register fields from target XML

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

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

Use StringRef instead of std::string.

Though an empty StringRef is morally equivalent
to optional, I've stuck with optional to keep the code
looking consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145574

Files:
  lldb/include/lldb/Target/DynamicRegisterInfo.h
  lldb/include/lldb/lldb-private-types.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Target/DynamicRegisterInfo.cpp

Index: lldb/source/Target/DynamicRegisterInfo.cpp
===
--- lldb/source/Target/DynamicRegisterInfo.cpp
+++ lldb/source/Target/DynamicRegisterInfo.cpp
@@ -407,7 +407,7 @@
   {reg.regnum_ehframe, reg.regnum_dwarf, reg.regnum_generic,
reg.regnum_remote, local_regnum},
   // value_regs and invalidate_regs are filled by Finalize()
-  nullptr, nullptr, nullptr
+  nullptr, nullptr, reg.flags_type
 };
 
 m_regs.push_back(reg_info);
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -38,6 +38,7 @@
 #include "GDBRemoteRegisterContext.h"
 
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/StringMap.h"
 
 namespace lldb_private {
 namespace repro {
@@ -466,6 +467,15 @@
   // fork helpers
   void DidForkSwitchSoftwareBreakpoints(bool enable);
   void DidForkSwitchHardwareTraps(bool enable);
+
+  // Lists of register fields generated from the remote's target XML.
+  // Pointers to these RegisterFlags will be set in the register info passed
+  // back to the upper levels of lldb. Doing so is safe because this class will
+  // live at least as long as the debug session. We therefore do not store the
+  // data directly in the map because the map may reallocate it's storage as new
+  // entries are added. Which would invalidate any pointers set in the register
+  // info up to that point.
+  llvm::StringMap> m_registers_flags_types;
 };
 
 } // namespace process_gdb_remote
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -53,6 +53,7 @@
 #include "lldb/Target/ABI.h"
 #include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/MemoryRegionInfo.h"
+#include "lldb/Target/RegisterFlags.h"
 #include "lldb/Target/SystemRuntime.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/TargetList.h"
@@ -84,6 +85,7 @@
 #include "lldb/Utility/StringExtractorGDBRemote.h"
 
 #include "llvm/ADT/ScopeExit.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/FormatAdapters.h"
 #include "llvm/Support/Threading.h"
@@ -4029,15 +4031,214 @@
   RegisterSetMap reg_set_map;
 };
 
-bool ParseRegisters(XMLNode feature_node, GdbServerTargetInfo &target_info,
-std::vector ®isters) {
+static std::vector ParseFlagsFields(XMLNode flags_node,
+  unsigned size) {
+  Log *log(GetLog(GDBRLog::Process));
+  const unsigned max_start_bit = size * 8 - 1;
+
+  // Process the fields of this set of flags.
+  std::vector fields;
+  flags_node.ForEachChildElementWithName("field", [&fields, max_start_bit,
+   &log](const XMLNode
+ &field_node) {
+std::optional name;
+std::optional start;
+std::optional end;
+std::optional type;
+
+field_node.ForEachAttribute([&name, &start, &end, &type, max_start_bit,
+ &log](const llvm::StringRef &attr_name,
+   const llvm::StringRef &attr_value) {
+  // Note that XML in general requires that each of these attributes only
+  // appears once, so we don't have to handle that here.
+  if (attr_name == "name") {
+LLDB_LOGF(log,
+  "ProcessGDBRemote::ParseFlags Found field node name \"%s\"",
+  attr_value.data());
+name = attr_value;
+  } else if (attr_name == "start") {
+unsigned parsed_start = 0;
+if (llvm::to_integer(attr_value, parsed_start)) {
+  if (parsed_start > max_start_bit) {
+LLDB_LOGF(
+log,
+"ProcessGDBRemote::ParseFlags Invalid start %d in field node, "
+"cannot be > %d",
+parsed_start, max_start_bit);
+  } else
+start = parsed_start;
+} else {
+

[Lldb-commits] [PATCH] D145580: [lldb] Show register fields using bitfield struct types

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

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

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145580

Files:
  lldb/include/lldb/Core/DumpRegisterValue.h
  lldb/include/lldb/DataFormatters/DumpValueObjectOptions.h
  lldb/include/lldb/Target/RegisterFlags.h
  lldb/source/Commands/CommandObjectRegister.cpp
  lldb/source/Core/DumpRegisterValue.cpp
  lldb/source/DataFormatters/DumpValueObjectOptions.cpp
  lldb/source/DataFormatters/ValueObjectPrinter.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
  lldb/unittests/Target/RegisterFlagsTest.cpp
  llvm/docs/ReleaseNotes.rst

Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -188,6 +188,12 @@
   omit defaulted template parameters. The full template parameter list can still be
   viewed with `expr --raw-output`/`frame var --raw-output`. (`D141828 `_)
 
+* LLDB can now display register fields if they are described in target XML sent
+  by a debug server such as ``gdbserver`` (``lldb-server`` does not currently produce
+  this information). Fields are only printed when reading named registers, for
+  example ``register read cpsr``. They are not shown when reading a register set,
+  ``register read -s 0``.
+
 Changes to Sanitizers
 -
 
Index: lldb/unittests/Target/RegisterFlagsTest.cpp
===
--- lldb/unittests/Target/RegisterFlagsTest.cpp
+++ lldb/unittests/Target/RegisterFlagsTest.cpp
@@ -121,4 +121,20 @@
{make_field(28, 31), make_field(24, 27), make_field(22, 23),
 make_field(20, 21), make_field(12, 19), make_field(8, 11),
 make_field(0, 7)});
-}
\ No newline at end of file
+}
+
+TEST(RegisterFieldsTest, ReverseFieldOrder) {
+  // Unchanged
+  RegisterFlags rf("", 4, {make_field(0, 31)});
+  ASSERT_EQ(0x12345678ULL, rf.ReverseFieldOrder(0x12345678));
+
+  // Swap the two halves around.
+  RegisterFlags rf2("", 4, {make_field(16, 31), make_field(0, 15)});
+  ASSERT_EQ(0x56781234ULL, rf2.ReverseFieldOrder(0x12345678));
+
+  // Many small fields.
+  RegisterFlags rf3("", 4,
+{make_field(31, 31), make_field(30, 30), make_field(29, 29),
+ make_field(28, 28)});
+  ASSERT_EQ(0x0005ULL, rf3.ReverseFieldOrder(0xA000));
+}
Index: lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
===
--- /dev/null
+++ lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
@@ -0,0 +1,479 @@
+""" Check that register fields found in target XML are properly processed."""
+
+from textwrap import dedent
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test.gdbclientutils import *
+from lldbsuite.test.lldbgdbclient import GDBRemoteTestBase
+
+class MyResponder(MockGDBServerResponder):
+def __init__(self, registers):
+  super().__init__()
+  # This *must* begin with the opening tag, leading whitespace is not allowed.
+  self.target_xml = dedent("""\
+
+  
+aarch64
+
+  {}
+
+""".format(registers))
+
+def qXferRead(self, obj, annex, offset, length):
+if annex == "target.xml":
+return self.target_xml, False
+else:
+return None,
+
+def readRegister(self, regnum):
+return "E01"
+
+def readRegisters(self):
+# Data for all registers requested by the tests below.
+# 0x7 and 0xE are used because their lsb and msb are opposites, which
+# is needed for a byte order test.
+return ''.join([
+  '', # 64 bit x0
+  '', # 32 bit cpsr
+  '', # 64 bit pc
+])
+
+class MyMultiDocResponder(MockGDBServerResponder):
+# docs is a dictionary of filename -> file content.
+def __init__(self, docs):
+  super().__init__()
+  self.docs = docs
+
+def qXferRead(self, obj, annex, offset, length):
+try:
+return self.docs[annex], False
+except KeyError:
+return None,
+
+def readRegister(self, regnum):
+return "E01"
+
+def readRegisters(self):
+return ''.join([
+  '', # 64 bit x0
+  '', # 32 bit cpsr
+  '', # 64 bit pc
+])
+
+class TestAArch64XMLRegisterFlags(GDBRemoteTestBase):
+def setup_register_test(self, registers):
+  target = self.createTarget("basic_eh_frame-aarch64.yaml")
+  self.server.responder = MyResponder(registers)
+
+  if self.TraceOn():
+  self.runCmd("log

[Lldb-commits] [PATCH] D145566: [lldb] Add RegisterFlags class

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett updated this revision to Diff 503378.
DavidSpickett marked an inline comment as done.
DavidSpickett added a comment.

Add newline at end of file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145566

Files:
  lldb/include/lldb/Target/RegisterFlags.h
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/RegisterFlags.cpp
  lldb/unittests/Target/CMakeLists.txt
  lldb/unittests/Target/RegisterFlagsTest.cpp

Index: lldb/unittests/Target/RegisterFlagsTest.cpp
===
--- /dev/null
+++ lldb/unittests/Target/RegisterFlagsTest.cpp
@@ -0,0 +1,125 @@
+//===-- RegisterFlagsTest.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Target/RegisterFlags.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+TEST(RegisterFlagsTest, Field) {
+  // We assume that start <= end is always true, so that is not tested here.
+
+  RegisterFlags::Field f1("abc", 0, 0, "unknown");
+  ASSERT_EQ(f1.GetName(), "abc");
+  ASSERT_EQ(f1.GetType(), "unknown");
+  // start == end means a 1 bit field.
+  ASSERT_EQ(f1.GetSizeInBits(), (unsigned)1);
+  ASSERT_EQ(f1.GetMask(), (uint64_t)1);
+  ASSERT_EQ(f1.GetValue(0), (uint64_t)0);
+  ASSERT_EQ(f1.GetValue(3), (uint64_t)1);
+
+  // End is inclusive meaning that start 0 to end 1 includes bit 1
+  // to make a 2 bit field.
+  RegisterFlags::Field f2("", 0, 1, "");
+  ASSERT_EQ(f2.GetSizeInBits(), (unsigned)2);
+  ASSERT_EQ(f2.GetMask(), (uint64_t)3);
+  ASSERT_EQ(f2.GetValue(UINT64_MAX), (uint64_t)3);
+  ASSERT_EQ(f2.GetValue(UINT64_MAX & ~(uint64_t)3), (uint64_t)0);
+
+  // If the field doesn't start at 0 we need to shift up/down
+  // to account for it.
+  RegisterFlags::Field f3("", 2, 5, "");
+  ASSERT_EQ(f3.GetSizeInBits(), (unsigned)4);
+  ASSERT_EQ(f3.GetMask(), (uint64_t)0x3c);
+  ASSERT_EQ(f3.GetValue(UINT64_MAX), (uint64_t)0xf);
+  ASSERT_EQ(f3.GetValue(UINT64_MAX & ~(uint64_t)0x3c), (uint64_t)0);
+
+  // Fields are sorted lowest starting bit first.
+  ASSERT_TRUE(f2 < f3);
+  ASSERT_FALSE(f3 < f1);
+  ASSERT_FALSE(f1 < f2);
+  ASSERT_FALSE(f1 < f1);
+}
+
+static RegisterFlags::Field make_field(unsigned start, unsigned end) {
+  return RegisterFlags::Field("", start, end, "");
+}
+
+TEST(RegisterFlagsTest, FieldOverlaps) {
+  // Single bit fields
+  ASSERT_FALSE(make_field(0, 0).Overlaps(make_field(1, 1)));
+  ASSERT_TRUE(make_field(1, 1).Overlaps(make_field(1, 1)));
+  ASSERT_FALSE(make_field(1, 1).Overlaps(make_field(3, 3)));
+
+  ASSERT_TRUE(make_field(0, 1).Overlaps(make_field(1, 2)));
+  ASSERT_TRUE(make_field(1, 2).Overlaps(make_field(0, 1)));
+  ASSERT_FALSE(make_field(0, 1).Overlaps(make_field(2, 3)));
+  ASSERT_FALSE(make_field(2, 3).Overlaps(make_field(0, 1)));
+
+  ASSERT_FALSE(make_field(1, 5).Overlaps(make_field(10, 20)));
+  ASSERT_FALSE(make_field(15, 30).Overlaps(make_field(7, 12)));
+}
+
+TEST(RegisterFlagsTest, PaddingDistance) {
+  // We assume that this method is always called with a more significant
+  // (start bit is higher) field first and that they do not overlap.
+
+  // [field 1][field 2]
+  ASSERT_EQ(make_field(1, 1).PaddingDistance(make_field(0, 0)), 0ULL);
+  // [field 1][..][field 2]
+  ASSERT_EQ(make_field(2, 2).PaddingDistance(make_field(0, 0)), 1ULL);
+  // [field 1][field 1][field 2]
+  ASSERT_EQ(make_field(1, 2).PaddingDistance(make_field(0, 0)), 0ULL);
+  // [field 1][30 bits free][field 2]
+  ASSERT_EQ(make_field(31, 31).PaddingDistance(make_field(0, 0)), 30ULL);
+}
+
+static void test_padding(const std::vector &fields,
+ const std::vector &expected) {
+  RegisterFlags rf("", 4, fields);
+  EXPECT_THAT(expected, ::testing::ContainerEq(rf.GetFields()));
+}
+
+TEST(RegisterFlagsTest, RegisterFlagsPadding) {
+  // When creating a set of flags we assume that:
+  // * There are >= 1 fields.
+  // * They are sorted in descending order.
+  // * There may be gaps between each field.
+
+  // Needs no padding
+  auto fields =
+  std::vector{make_field(16, 31), make_field(0, 15)};
+  test_padding(fields, fields);
+
+  // Needs padding in between the fields, single bit.
+  test_padding({make_field(17, 31), make_field(0, 15)},
+   {make_field(17, 31), make_field(16, 16), make_field(0, 15)});
+  // Multiple bits of padding.
+  test_padding({make_field(17, 31), make_field(0, 14)},
+   {make_field(17, 31), make_field(15, 16), make_field(0, 14)});
+
+  // Padding before first field, single bit.
+  test_padding({make_field(0, 30)}, {make_field(31, 31), make_field(0, 30)});
+  // Multipl

[Lldb-commits] [PATCH] D145580: [lldb] Show register fields using bitfield struct types

2023-03-08 Thread David Spickett via Phabricator via lldb-commits

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

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145580

Files:
  lldb/include/lldb/Core/DumpRegisterValue.h
  lldb/include/lldb/DataFormatters/DumpValueObjectOptions.h
  lldb/include/lldb/Target/RegisterFlags.h
  lldb/source/Commands/CommandObjectRegister.cpp
  lldb/source/Core/DumpRegisterValue.cpp
  lldb/source/DataFormatters/DumpValueObjectOptions.cpp
  lldb/source/DataFormatters/ValueObjectPrinter.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
  lldb/unittests/Target/RegisterFlagsTest.cpp
  llvm/docs/ReleaseNotes.rst

Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -188,6 +188,12 @@
   omit defaulted template parameters. The full template parameter list can still be
   viewed with `expr --raw-output`/`frame var --raw-output`. (`D141828 `_)
 
+* LLDB can now display register fields if they are described in target XML sent
+  by a debug server such as ``gdbserver`` (``lldb-server`` does not currently produce
+  this information). Fields are only printed when reading named registers, for
+  example ``register read cpsr``. They are not shown when reading a register set,
+  ``register read -s 0``.
+
 Changes to Sanitizers
 -
 
Index: lldb/unittests/Target/RegisterFlagsTest.cpp
===
--- lldb/unittests/Target/RegisterFlagsTest.cpp
+++ lldb/unittests/Target/RegisterFlagsTest.cpp
@@ -123,3 +123,18 @@
 make_field(0, 7)});
 }
 
+TEST(RegisterFieldsTest, ReverseFieldOrder) {
+  // Unchanged
+  RegisterFlags rf("", 4, {make_field(0, 31)});
+  ASSERT_EQ(0x12345678ULL, rf.ReverseFieldOrder(0x12345678));
+
+  // Swap the two halves around.
+  RegisterFlags rf2("", 4, {make_field(16, 31), make_field(0, 15)});
+  ASSERT_EQ(0x56781234ULL, rf2.ReverseFieldOrder(0x12345678));
+
+  // Many small fields.
+  RegisterFlags rf3("", 4,
+{make_field(31, 31), make_field(30, 30), make_field(29, 29),
+ make_field(28, 28)});
+  ASSERT_EQ(0x0005ULL, rf3.ReverseFieldOrder(0xA000));
+}
Index: lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
===
--- /dev/null
+++ lldb/test/API/functionalities/gdb_remote_client/TestXMLRegisterFlags.py
@@ -0,0 +1,479 @@
+""" Check that register fields found in target XML are properly processed."""
+
+from textwrap import dedent
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test.gdbclientutils import *
+from lldbsuite.test.lldbgdbclient import GDBRemoteTestBase
+
+class MyResponder(MockGDBServerResponder):
+def __init__(self, registers):
+  super().__init__()
+  # This *must* begin with the opening tag, leading whitespace is not allowed.
+  self.target_xml = dedent("""\
+
+  
+aarch64
+
+  {}
+
+""".format(registers))
+
+def qXferRead(self, obj, annex, offset, length):
+if annex == "target.xml":
+return self.target_xml, False
+else:
+return None,
+
+def readRegister(self, regnum):
+return "E01"
+
+def readRegisters(self):
+# Data for all registers requested by the tests below.
+# 0x7 and 0xE are used because their lsb and msb are opposites, which
+# is needed for a byte order test.
+return ''.join([
+  '', # 64 bit x0
+  '', # 32 bit cpsr
+  '', # 64 bit pc
+])
+
+class MyMultiDocResponder(MockGDBServerResponder):
+# docs is a dictionary of filename -> file content.
+def __init__(self, docs):
+  super().__init__()
+  self.docs = docs
+
+def qXferRead(self, obj, annex, offset, length):
+try:
+return self.docs[annex], False
+except KeyError:
+return None,
+
+def readRegister(self, regnum):
+return "E01"
+
+def readRegisters(self):
+return ''.join([
+  '', # 64 bit x0
+  '', # 32 bit cpsr
+  '', # 64 bit pc
+])
+
+class TestAArch64XMLRegisterFlags(GDBRemoteTestBase):
+def setup_register_test(self, registers):
+  target = self.createTarget("basic_eh_frame-aarch64.yaml")
+  self.server.responder = MyResponder(registers)
+
+  if self.TraceOn():
+  self.runCmd("log enable gdb-remote packets process")
+  self.addTearDownHook(
+  lambda: self.runCmd("log disable gdb-remote packets process"))
+
+  process = self.connect(targ

[Lldb-commits] [PATCH] D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON

2023-03-08 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere updated this revision to Diff 503432.
JDevlieghere added a comment.

- Add unit tests for JSON deserialization
- Add unit tests for converting JSONSymbol to Symbol


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

https://reviews.llvm.org/D145180

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/include/lldb/Symbol/Symbol.h
  lldb/source/Core/Debugger.cpp
  lldb/source/Plugins/ObjectFile/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/JSON/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp
  lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.h
  lldb/source/Plugins/SymbolFile/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/JSON/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/JSON/SymbolFileJSON.cpp
  lldb/source/Plugins/SymbolFile/JSON/SymbolFileJSON.h
  lldb/source/Symbol/Symbol.cpp
  lldb/test/API/macosx/symbols/Makefile
  lldb/test/API/macosx/symbols/TestSymbolFileJSON.py
  lldb/test/API/macosx/symbols/main.c
  lldb/test/Shell/Diagnostics/Inputs/TestCopyLogs.in
  lldb/test/Shell/Diagnostics/TestCopyLogs.test
  lldb/unittests/Symbol/CMakeLists.txt
  lldb/unittests/Symbol/JSONSymbolTest.cpp

Index: lldb/unittests/Symbol/JSONSymbolTest.cpp
===
--- /dev/null
+++ lldb/unittests/Symbol/JSONSymbolTest.cpp
@@ -0,0 +1,192 @@
+//===-- JSONSymbolTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/Section.h"
+#include "lldb/Symbol/Symbol.h"
+
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace llvm;
+using namespace lldb_private;
+
+static std::string g_error_no_section_list = "no section list provided";
+static std::string g_error_both_value_and_address =
+"symbol cannot contain both a value and an address";
+static std::string g_error_neither_value_or_address =
+"symbol must contain either a value or an address";
+
+TEST(JSONSymbolTest, DeserializeCodeAddress) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32,
+  "address": 4096
+})";
+
+  Expected json = json::parse(text);
+  ASSERT_TRUE(static_cast(json));
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_TRUE(fromJSON(*json, json_symbol, root));
+
+  SectionSP sect_sp(new Section(
+  /*module_sp=*/ModuleSP(),
+  /*obj_file=*/nullptr,
+  /*sect_id=*/1,
+  /*name=*/ConstString(".text"),
+  /*sect_type=*/eSectionTypeCode,
+  /*file_vm_addr=*/0x1000,
+  /*vm_size=*/0x1000,
+  /*file_offset=*/0,
+  /*file_size=*/0,
+  /*log2align=*/5,
+  /*flags=*/0x10203040));
+  SectionList sect_list;
+  sect_list.AddSection(sect_sp);
+
+  Expected symbol = Symbol::FromJSON(json_symbol, §_list);
+  ASSERT_TRUE(static_cast(symbol));
+  EXPECT_EQ(symbol->GetName(), ConstString("foo"));
+  EXPECT_EQ(symbol->GetFileAddress(), static_cast(0x1000));
+  EXPECT_EQ(symbol->GetType(), eSymbolTypeCode);
+}
+
+TEST(JSONSymbolTest, DeserializeCodeValue) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32,
+  "value": 4096
+})";
+
+  Expected json = json::parse(text);
+  ASSERT_TRUE(static_cast(json));
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_TRUE(fromJSON(*json, json_symbol, root));
+
+  SectionList sect_list;
+
+  Expected symbol = Symbol::FromJSON(json_symbol, §_list);
+  ASSERT_TRUE(static_cast(symbol));
+  EXPECT_EQ(symbol->GetName(), ConstString("foo"));
+  EXPECT_EQ(symbol->GetRawValue(), static_cast(0x1000));
+  EXPECT_EQ(symbol->GetType(), eSymbolTypeCode);
+}
+
+TEST(JSONSymbolTest, JSONInvalidValueAndAddress) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32,
+  "value": 4096,
+  "address": 4096
+})";
+
+  Expected json = json::parse(text);
+  ASSERT_TRUE(static_cast(json));
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_FALSE(fromJSON(*json, json_symbol, root));
+}
+
+TEST(JSONSymbolTest, JSONInvalidNoValueOrAddress) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32
+})";
+
+  Expected json = json::parse(text);
+  ASSERT_TRUE(static_cast(json));
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_FALSE(fromJSON(*json, json_symbol, root));
+}
+
+TEST(JSONSymbolTest, JSONInvalidType) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "bogus",
+  "value": 4096,
+  "size": 32
+})";
+
+  Expected json = json::parse(text);
+  ASSERT_TRUE(static_cast(json));
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_FALSE(fromJSON(*json, json_symbol, root));
+}
+
+TEST(JSONSymbolTest, SymbolInvalidNoSectionList) {
+  JSONSymbol json_symbol;
+  json_symbol.value = 0x1;
+
+  Expe

[Lldb-commits] [PATCH] D145529: [lldb] Change default value of dwim-print-verbosity setting

2023-03-08 Thread Dave Lee via Phabricator via lldb-commits

kastiglione added a comment.

> This doesn't affect any tests?

nope


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145529

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

[Lldb-commits] [PATCH] D145529: [lldb] Change default value of dwim-print-verbosity setting

2023-03-08 Thread Dave Lee via Phabricator via lldb-commits

kastiglione added a comment.

Clarification: `TestDWIMPrint.py` uses and tests with this setting, but it 
explicitly sets the value to "full". The default value doesn't affect tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145529

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

[Lldb-commits] [PATCH] D145529: [lldb] Change default value of dwim-print-verbosity setting

2023-03-08 Thread Dave Lee via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG632c396499eb: [lldb] Change default value of 
dwim-print-verbosity setting (authored by kastiglione).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145529

Files:
  lldb/source/Core/CoreProperties.td


Index: lldb/source/Core/CoreProperties.td
===
--- lldb/source/Core/CoreProperties.td
+++ lldb/source/Core/CoreProperties.td
@@ -193,7 +193,7 @@
 Desc<"When displaying suggestion in a color-enabled terminal, use the ANSI 
terminal code specified in this format immediately after the suggestion.">;
   def DWIMPrintVerbosity: Property<"dwim-print-verbosity", "Enum">,
 Global,
-DefaultEnumValue<"eDWIMPrintVerbosityExpression">,
+DefaultEnumValue<"eDWIMPrintVerbosityNone">,
 EnumValues<"OptionEnumValues(g_dwim_print_verbosities)">,
 Desc<"The verbosity level used by dwim-print.">;
 }


Index: lldb/source/Core/CoreProperties.td
===
--- lldb/source/Core/CoreProperties.td
+++ lldb/source/Core/CoreProperties.td
@@ -193,7 +193,7 @@
 Desc<"When displaying suggestion in a color-enabled terminal, use the ANSI terminal code specified in this format immediately after the suggestion.">;
   def DWIMPrintVerbosity: Property<"dwim-print-verbosity", "Enum">,
 Global,
-DefaultEnumValue<"eDWIMPrintVerbosityExpression">,
+DefaultEnumValue<"eDWIMPrintVerbosityNone">,
 EnumValues<"OptionEnumValues(g_dwim_print_verbosities)">,
 Desc<"The verbosity level used by dwim-print.">;
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 632c396 - [lldb] Change default value of dwim-print-verbosity setting

2023-03-08 Thread Dave Lee via lldb-commits


Author: Dave Lee
Date: 2023-03-08T11:10:50-08:00
New Revision: 632c396499eb4f6f7a36ba0246ba57e011357a55

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

LOG: [lldb] Change default value of dwim-print-verbosity setting

Reduce the default value of `dwim-print-verbosity` to `eDWIMPrintVerbosityNone`.

Users who wish to see the rewritten expression can set this setting manually. 
Not unlike
`interpreter.expand-regex-aliases`.

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

Added: 


Modified: 
lldb/source/Core/CoreProperties.td

Removed: 




diff  --git a/lldb/source/Core/CoreProperties.td 
b/lldb/source/Core/CoreProperties.td
index fc664be63f429..62729923f366b 100644
--- a/lldb/source/Core/CoreProperties.td
+++ b/lldb/source/Core/CoreProperties.td
@@ -193,7 +193,7 @@ let Definition = "debugger" in {
 Desc<"When displaying suggestion in a color-enabled terminal, use the ANSI 
terminal code specified in this format immediately after the suggestion.">;
   def DWIMPrintVerbosity: Property<"dwim-print-verbosity", "Enum">,
 Global,
-DefaultEnumValue<"eDWIMPrintVerbosityExpression">,
+DefaultEnumValue<"eDWIMPrintVerbosityNone">,
 EnumValues<"OptionEnumValues(g_dwim_print_verbosities)">,
 Desc<"The verbosity level used by dwim-print.">;
 }



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

[Lldb-commits] [lldb] 6c599b1 - [lldb] Let 'v' command directly access ivars of _any_ self/this

2023-03-08 Thread Dave Lee via lldb-commits


Author: Dave Lee
Date: 2023-03-08T11:19:43-08:00
New Revision: 6c599b1e9b7e1b57952565468aed2de16af21082

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

LOG: [lldb] Let 'v' command directly access ivars of _any_ self/this

The `v` (`frame variable`) command can directly access ivars/fields of `this` 
or `self`.
Such as `v field`, instead of `v this->field`. This change relaxes the criteria 
for
finding `this`/`self` variables.

There are cases where a `this`/`self` variable does exist, but up to now the 
`v` command
has not made use of it. The user would have to explicitly run `v this->field` or
`self->_ivar` to access ivars. This change allows such cases to also work 
(without
explicitly dereferencing `this`/`self`).

A very common example in Objective-C (and Swift) is weakly capturing `self`:

```
__weak Type *weakSelf = self;
void (^block)(void) = ^{
   Type *self = weakSelf; // Re-establish strong reference.
   // `v _ivar` should work just as well as `v self->_ivar`.
};
```

In this case, `self` exists but `v` would not have used it. With this change, 
the fact
that a variable named `self` exists is enough for it to be used.

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

Added: 


Modified: 
lldb/include/lldb/Symbol/CompilerDeclContext.h
lldb/include/lldb/Symbol/SymbolContext.h
lldb/include/lldb/Symbol/TypeSystem.h
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
lldb/source/Symbol/CompilerDeclContext.cpp
lldb/source/Symbol/SymbolContext.cpp
lldb/source/Target/StackFrame.cpp
lldb/test/API/commands/frame/var/direct-ivar/objc/Makefile

lldb/test/API/commands/frame/var/direct-ivar/objc/TestFrameVarDirectIvarObjC.py
lldb/test/API/commands/frame/var/direct-ivar/objc/main.m

Removed: 




diff  --git a/lldb/include/lldb/Symbol/CompilerDeclContext.h 
b/lldb/include/lldb/Symbol/CompilerDeclContext.h
index ca404a6641d5e..63e5f7b680e63 100644
--- a/lldb/include/lldb/Symbol/CompilerDeclContext.h
+++ b/lldb/include/lldb/Symbol/CompilerDeclContext.h
@@ -61,15 +61,21 @@ class CompilerDeclContext {
 
   /// Checks if this decl context represents a method of a class.
   ///
-  /// \param[out] language_object_name_ptr
-  /// If non NULL and \b true is returned from this function,
-  /// this will indicate if implicit object name for the language
-  /// like "this" for C++, and "self" for Objective C.
-  ///
   /// \return
   /// Returns true if this is a decl context that represents a method
   /// in a struct, union or class.
-  bool IsClassMethod(ConstString *language_object_name_ptr = nullptr);
+  bool IsClassMethod();
+
+  /// Determines the original language of the decl context.
+  lldb::LanguageType GetLanguage();
+
+  /// Determines the name of the instance variable for the this decl context.
+  ///
+  /// For C++ the name is "this", for Objective-C the name is "self".
+  ///
+  /// \return
+  /// Returns a string for the name of the instance variable.
+  ConstString GetInstanceVariableName(lldb::LanguageType language);
 
   /// Check if the given other decl context is contained in the lookup
   /// of this decl context (for example because the other context is a nested

diff  --git a/lldb/include/lldb/Symbol/SymbolContext.h 
b/lldb/include/lldb/Symbol/SymbolContext.h
index bb9e031d4..73fa25514aff3 100644
--- a/lldb/include/lldb/Symbol/SymbolContext.h
+++ b/lldb/include/lldb/Symbol/SymbolContext.h
@@ -245,17 +245,13 @@ class SymbolContext {
   /// represented by this symbol context object, nullptr otherwise.
   Block *GetFunctionBlock();
 
-  /// If this symbol context represents a function that is a method, return
-  /// true and provide information about the method.
+  /// Determines the name of the instance variable for the this decl context.
   ///
-  /// \param[out] language_object_name
-  /// If \b true is returned, the name of the artificial variable
-  /// for the language ("this" for C++, "self" for ObjC).
+  /// For C++ the name is "this", for Objective-C the name is "self".
   ///
   /// \return
-  /// \b True if this symbol context represents a function that
-  /// is a method of a class, \b false otherwise.
-  bool GetFunctionMethodInfo(ConstString &language_object_name);
+  /// Returns a string for the name of the instance variable.
+  ConstString GetInstanceVariableName();
 
   /// Sorts the types in TypeMap according to SymbolContext to TypeList
   ///

diff  --git a/lldb/include/lldb/Symbol/TypeSystem.h 
b/lldb/include/lldb/Symbol/TypeSystem.h
index 7681a700766a2..0777d4d5ad6f3 100644
--- a/lldb/include/lldb/Symbol/TypeSystem.h
+++ b/lldb/include/lldb/Symbol/TypeSystem.h
@@ -127,13 +127,13 @@ class Typ

[Lldb-commits] [PATCH] D145276: [lldb] Let 'v' command directly access ivars of _any_ self/this

2023-03-08 Thread Dave Lee via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6c599b1e9b7e: [lldb] Let 'v' command directly 
access ivars of _any_ self/this (authored by kastiglione).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145276

Files:
  lldb/include/lldb/Symbol/CompilerDeclContext.h
  lldb/include/lldb/Symbol/SymbolContext.h
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
  lldb/source/Symbol/CompilerDeclContext.cpp
  lldb/source/Symbol/SymbolContext.cpp
  lldb/source/Target/StackFrame.cpp
  lldb/test/API/commands/frame/var/direct-ivar/objc/Makefile
  
lldb/test/API/commands/frame/var/direct-ivar/objc/TestFrameVarDirectIvarObjC.py
  lldb/test/API/commands/frame/var/direct-ivar/objc/main.m

Index: lldb/test/API/commands/frame/var/direct-ivar/objc/main.m
===
--- lldb/test/API/commands/frame/var/direct-ivar/objc/main.m
+++ lldb/test/API/commands/frame/var/direct-ivar/objc/main.m
@@ -7,13 +7,25 @@
 @end
 
 @implementation Classic
-- (int)fun {
+- (void)fun {
   // check self
 }
+
+- (void)run {
+  __weak Classic *weakSelf = self;
+  ^{
+Classic *self = weakSelf;
+// check idiomatic self
+
+// Use `self` to extend its lifetime (for lldb to inspect the variable).
+[self copy];
+  }();
+}
 @end
 
 int main() {
   Classic *c = [Classic new];
   c->_ivar = 30;
   [c fun];
+  [c run];
 }
Index: lldb/test/API/commands/frame/var/direct-ivar/objc/TestFrameVarDirectIvarObjC.py
===
--- lldb/test/API/commands/frame/var/direct-ivar/objc/TestFrameVarDirectIvarObjC.py
+++ lldb/test/API/commands/frame/var/direct-ivar/objc/TestFrameVarDirectIvarObjC.py
@@ -10,3 +10,11 @@
 self.build()
 lldbutil.run_to_source_breakpoint(self, "// check self", lldb.SBFileSpec("main.m"))
 self.expect("frame variable _ivar", startstr="(int) _ivar = 30")
+
+@skipUnlessDarwin
+def test_objc_self_capture_idiom(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// check idiomatic self", lldb.SBFileSpec("main.m"))
+self.expect("frame variable weakSelf", startstr="(Classic *) weakSelf = 0x")
+self.expect("frame variable self", startstr="(Classic *) self = 0x")
+self.expect("frame variable _ivar", startstr="(int) _ivar = 30")
Index: lldb/test/API/commands/frame/var/direct-ivar/objc/Makefile
===
--- lldb/test/API/commands/frame/var/direct-ivar/objc/Makefile
+++ lldb/test/API/commands/frame/var/direct-ivar/objc/Makefile
@@ -1,2 +1,4 @@
 OBJC_SOURCES := main.m
+CFLAGS_EXTRAS := -fblocks -fobjc-arc
+LD_EXTRAS := -lobjc
 include Makefile.rules
Index: lldb/source/Target/StackFrame.cpp
===
--- lldb/source/Target/StackFrame.cpp
+++ lldb/source/Target/StackFrame.cpp
@@ -567,23 +567,20 @@
 // Check for direct ivars access which helps us with implicit access to
 // ivars using "this" or "self".
 GetSymbolContext(eSymbolContextFunction | eSymbolContextBlock);
-ConstString method_object_name;
-if (m_sc.GetFunctionMethodInfo(method_object_name)) {
-  if (method_object_name) {
-var_sp = variable_list->FindVariable(method_object_name);
-if (var_sp) {
-  separator_idx = 0;
-  if (Type *var_type = var_sp->GetType())
-if (auto compiler_type = var_type->GetForwardCompilerType())
-  if (!compiler_type.IsPointerType())
-var_expr_storage = ".";
-
-  if (var_expr_storage.empty())
-var_expr_storage = "->";
-  var_expr_storage += var_expr;
-  var_expr = var_expr_storage;
-  synthetically_added_instance_object = true;
-}
+if (auto instance_var_name = m_sc.GetInstanceVariableName()) {
+  var_sp = variable_list->FindVariable(instance_var_name);
+  if (var_sp) {
+separator_idx = 0;
+if (Type *var_type = var_sp->GetType())
+  if (auto compiler_type = var_type->GetForwardCompilerType())
+if (!compiler_type.IsPointerType())
+  var_expr_storage = ".";
+
+if (var_expr_storage.empty())
+  var_expr_storage = "->";
+var_expr_storage += var_expr;
+var_expr = var_expr_storage;
+synthetically_added_instance_object = true;
   }
 }
   }
Index: lldb/source/Symbol/SymbolContext.cpp
===
--- lldb/source/Symbol/SymbolContext.cpp
+++ lldb/source/Symbol/SymbolContext.cpp
@@ -539,14 +539,16 @@
   return nullptr;
 }
 
-bool SymbolContext::GetFunctionMethodInfo(ConstString &language_object_name) {
-  Block *function_block = Get

[Lldb-commits] [PATCH] D143695: [lldb] Make repeat commands work for regex commands

2023-03-08 Thread Dave Lee via Phabricator via lldb-commits

kastiglione added a comment.

> But `allow_repeat_command` is a misleading name for that. It makes is sound 
> like if I don't pass that flag I won't get a repeat command.  
> `force_repeat_command` would be a better name

I agree. I did have some loose thoughts matching this, when writing it, yet 
somehow ended up with `allow_`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143695

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

[Lldb-commits] [PATCH] D143695: [lldb] Make repeat commands work for regex commands

2023-03-08 Thread Dave Lee via Phabricator via lldb-commits

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

Rename variable to force_repeat_command and add comment block


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143695

Files:
  lldb/include/lldb/Interpreter/CommandInterpreter.h
  lldb/source/Commands/CommandObjectRegexCommand.cpp
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/test/API/functionalities/alias/TestBtAliasRepeat.py


Index: lldb/test/API/functionalities/alias/TestBtAliasRepeat.py
===
--- /dev/null
+++ lldb/test/API/functionalities/alias/TestBtAliasRepeat.py
@@ -0,0 +1,19 @@
+import lldb
+from lldbsuite.test.lldbtest import TestBase
+from lldbsuite.test import lldbutil
+
+
+class TestCase(TestBase):
+def test(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "return", 
lldb.SBFileSpec("main.c"))
+
+# Expect "frame #0" but not "frame #1".
+self.expect("bt 1", inHistory=True, patterns=["frame #0", "^(?!.*frame 
#1)"])
+
+# Run an empty command to run the repeat command for `bt`.
+# The repeat command for `bt N` lists the subsequent N frames.
+#
+# In this case, after printing the frame 0 with `bt 1`, the repeat
+# command will print "frame #1" (and won't print "frame #0").
+self.expect("", patterns=["^(?!.*frame #0)", "frame #1"])
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -1874,8 +1874,8 @@
 
 bool CommandInterpreter::HandleCommand(const char *command_line,
LazyBool lazy_add_to_history,
-   CommandReturnObject &result) {
-
+   CommandReturnObject &result,
+   bool force_repeat_command) {
   std::string command_string(command_line);
   std::string original_command_string(command_line);
 
@@ -2004,17 +2004,26 @@
   // arguments.
 
   if (cmd_obj != nullptr) {
+bool generate_repeat_command = add_to_history;
 // If we got here when empty_command was true, then this command is a
 // stored "repeat command" which we should give a chance to produce it's
 // repeat command, even though we don't add repeat commands to the history.
-if (add_to_history || empty_command) {
+generate_repeat_command |= empty_command;
+// For `command regex`, the regex command (ex `bt`) is added to history, 
but
+// the resolved command (ex `thread backtrace`) is _not_ added to history.
+// However, the resolved command must be given the opportunity to provide a
+// repeat command. `force_repeat_command` supports this case.
+generate_repeat_command |= force_repeat_command;
+if (generate_repeat_command) {
   Args command_args(command_string);
   std::optional repeat_command =
   cmd_obj->GetRepeatCommand(command_args, 0);
-  if (repeat_command)
+  if (repeat_command) {
+LLDB_LOGF(log, "Repeat command: %s", repeat_command->data());
 m_repeat_command.assign(*repeat_command);
-  else
+  } else {
 m_repeat_command.assign(original_command_string);
+  }
 }
 
 if (add_to_history)
Index: lldb/source/Commands/CommandObjectRegexCommand.cpp
===
--- lldb/source/Commands/CommandObjectRegexCommand.cpp
+++ lldb/source/Commands/CommandObjectRegexCommand.cpp
@@ -72,8 +72,9 @@
 result.GetOutputStream().Printf("%s\n", new_command->c_str());
   // We don't have to pass an override_context here, as the command that 
   // called us should have set up the context appropriately.
-  return m_interpreter.HandleCommand(new_command->c_str(),
- eLazyBoolNo, result);
+  bool force_repeat_command = true;
+  return m_interpreter.HandleCommand(new_command->c_str(), eLazyBoolNo,
+ result, force_repeat_command);
 }
   }
   result.SetStatus(eReturnStatusFailed);
Index: lldb/include/lldb/Interpreter/CommandInterpreter.h
===
--- lldb/include/lldb/Interpreter/CommandInterpreter.h
+++ lldb/include/lldb/Interpreter/CommandInterpreter.h
@@ -352,7 +352,8 @@
  CommandReturnObject &result);
 
   bool HandleCommand(const char *command_line, LazyBool add_to_history,
- CommandReturnObject &result);
+ CommandReturnObject &result,
+ bool force_repeat_command = false);
 
   bool WasInterrupted() const;
 


Index: lldb/test/API/functionalities/alias/TestBtAliasRepeat.py
==

[Lldb-commits] [PATCH] D145609: [lldb] Change dwim-print to default to disabled persistent results

2023-03-08 Thread Dave Lee via Phabricator via lldb-commits

kastiglione created this revision.
kastiglione added reviewers: aprantl, jingham.
Herald added a project: All.
kastiglione requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Change `dwim-print` to now disable persistent results by default, unless 
requested by
the user with the `--persistent-result` flag.

Ex:

  (lldb) dwim-print 1 + 1
  (int) 2
  (lldb) dwim-print --persistent-result on -- 1 + 1
  (int) $0 = 2

Users who wish to enable persistent results can make and use an alias that 
includes
`--persistent-result on`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145609

Files:
  lldb/source/Commands/CommandObjectDWIMPrint.cpp
  lldb/source/Commands/CommandObjectExpression.cpp
  lldb/source/Commands/CommandObjectExpression.h
  lldb/test/API/commands/dwim-print/TestDWIMPrint.py

Index: lldb/test/API/commands/dwim-print/TestDWIMPrint.py
===
--- lldb/test/API/commands/dwim-print/TestDWIMPrint.py
+++ lldb/test/API/commands/dwim-print/TestDWIMPrint.py
@@ -16,8 +16,7 @@
 self.ci.HandleCommand(cmd, result)
 return result.GetOutput().rstrip()
 
-VAR_IDENT_RAW = r"(?:\$\d+|\w+) = "
-VAR_IDENT = re.compile(VAR_IDENT_RAW)
+VAR_IDENT = re.compile(r"(?:\$\d+|\w+) = ")
 
 def _mask_persistent_var(self, string: str) -> str:
 """
@@ -26,8 +25,7 @@
 be matched against other `expression` results.
 """
 before, after = self.VAR_IDENT.split(string, maxsplit=1)
-# Support either a frame variable (\w+) or a persistent result (\$\d+).
-return re.escape(before) + self.VAR_IDENT_RAW + re.escape(after)
+return before + after
 
 def _expect_cmd(
 self,
@@ -51,14 +49,13 @@
 # Verify dwim-print chose the expected command.
 self.runCmd("settings set dwim-print-verbosity full")
 substrs = [f"note: ran `{resolved_cmd}`"]
-patterns = []
 
 if self.VAR_IDENT.search(expected_output):
-patterns.append(self._mask_persistent_var(expected_output))
+substrs.append(self._mask_persistent_var(expected_output))
 else:
 substrs.append(expected_output)
 
-self.expect(dwim_cmd, substrs=substrs, patterns=patterns)
+self.expect(dwim_cmd, substrs=substrs)
 
 def test_variables(self):
 """Test dwim-print with variables."""
Index: lldb/source/Commands/CommandObjectExpression.h
===
--- lldb/source/Commands/CommandObjectExpression.h
+++ lldb/source/Commands/CommandObjectExpression.h
@@ -53,7 +53,7 @@
 lldb::LanguageType language;
 LanguageRuntimeDescriptionDisplayVerbosity m_verbosity;
 LazyBool auto_apply_fixits;
-bool suppress_persistent_result;
+LazyBool suppress_persistent_result;
   };
 
   CommandObjectExpression(CommandInterpreter &interpreter);
Index: lldb/source/Commands/CommandObjectExpression.cpp
===
--- lldb/source/Commands/CommandObjectExpression.cpp
+++ lldb/source/Commands/CommandObjectExpression.cpp
@@ -151,7 +151,7 @@
 bool persist_result =
 OptionArgParser::ToBoolean(option_arg, true, &success);
 if (success)
-  suppress_persistent_result = !persist_result;
+  suppress_persistent_result = !persist_result ? eLazyBoolYes : eLazyBoolNo;
 else
   error.SetErrorStringWithFormat(
   "could not convert \"%s\" to a boolean value.",
@@ -187,7 +187,7 @@
   auto_apply_fixits = eLazyBoolCalculate;
   top_level = false;
   allow_jit = true;
-  suppress_persistent_result = false;
+  suppress_persistent_result = eLazyBoolCalculate;
 }
 
 llvm::ArrayRef
@@ -202,8 +202,9 @@
   options.SetCoerceToId(display_opts.use_objc);
   // Explicitly disabling persistent results takes precedence over the
   // m_verbosity/use_objc logic.
-  if (suppress_persistent_result)
-options.SetSuppressPersistentResult(true);
+  if (suppress_persistent_result != eLazyBoolCalculate)
+options.SetSuppressPersistentResult(suppress_persistent_result ==
+eLazyBoolYes);
   else if (m_verbosity == eLanguageRuntimeDescriptionDisplayVerbosityCompact)
 options.SetSuppressPersistentResult(display_opts.use_objc);
   options.SetUnwindOnError(unwind_on_error);
Index: lldb/source/Commands/CommandObjectDWIMPrint.cpp
===
--- lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -71,6 +71,10 @@
 return false;
   }
 
+  // If the user has not specified, default to disabling persistent results.
+  if (m_expr_options.suppress_persistent_result == eLazyBoolCalculate)
+m_expr_options.suppress_persistent_result = eLazyBoolYes;
+
   auto verbosity = GetDebugger().GetDWIMPrintVerbosity();
 
   Target *t

[Lldb-commits] [PATCH] D145612: [lldb] Only replace valobj with persisted one if not null in DWIMPrint

2023-03-08 Thread Augusto Noronha via Phabricator via lldb-commits

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145612

Files:
  lldb/source/Commands/CommandObjectDWIMPrint.cpp


Index: lldb/source/Commands/CommandObjectDWIMPrint.cpp
===
--- lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -88,8 +88,11 @@
   if (StackFrame *frame = m_exe_ctx.GetFramePtr()) {
 auto valobj_sp = frame->FindVariable(ConstString(expr));
 if (valobj_sp && valobj_sp->GetError().Success()) {
-  if (!eval_options.GetSuppressPersistentResult())
-valobj_sp = valobj_sp->Persist();
+  if (!eval_options.GetSuppressPersistentResult()) {
+auto persisted_valobj = valobj_sp->Persist();
+if (persisted_valobj)
+  valobj_sp = persisted_valobj;
+  }
 
   if (verbosity == eDWIMPrintVerbosityFull) {
 StringRef flags;


Index: lldb/source/Commands/CommandObjectDWIMPrint.cpp
===
--- lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -88,8 +88,11 @@
   if (StackFrame *frame = m_exe_ctx.GetFramePtr()) {
 auto valobj_sp = frame->FindVariable(ConstString(expr));
 if (valobj_sp && valobj_sp->GetError().Success()) {
-  if (!eval_options.GetSuppressPersistentResult())
-valobj_sp = valobj_sp->Persist();
+  if (!eval_options.GetSuppressPersistentResult()) {
+auto persisted_valobj = valobj_sp->Persist();
+if (persisted_valobj)
+  valobj_sp = persisted_valobj;
+  }
 
   if (verbosity == eDWIMPrintVerbosityFull) {
 StringRef flags;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145547: When setting load addresses on darwin kernel kexts, handle case where in-memory load commands are not updated

2023-03-08 Thread Jason Molenda via Phabricator via lldb-commits

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

Show an alternate approach, where we check the file types of everything in the 
kernel+kext list, and if something is not a kernel/kext, don't try to load it 
at all.  v. the first change in 
`DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule` where I 
call `!IsKernel() && !obj_macho->IsKext()` to detect something in the list 
which isn't processed the standard way that kexts & the kernel are.  Either one 
would work in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145547

Files:
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/CMakeLists.txt
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h

Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
@@ -90,6 +90,8 @@
 
   bool IsSharedCacheBinary() const;
 
+  bool IsKext() const;
+
   uint32_t GetAddressByteSize() const override;
 
   lldb_private::AddressClass GetAddressClass(lldb::addr_t file_addr) override;
@@ -151,6 +153,8 @@
 
   bool AllowAssemblyEmulationUnwindPlans() override;
 
+  lldb_private::Section *GetMachHeaderSection();
+
   // PluginInterface protocol
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
 
@@ -193,8 +197,6 @@
   /// should not be used.
   void GetLLDBSharedCacheUUID(lldb::addr_t &base_addir, lldb_private::UUID &uuid);
 
-  lldb_private::Section *GetMachHeaderSection();
-
   lldb::addr_t CalculateSectionLoadAddressForMemoryImage(
   lldb::addr_t mach_header_load_address,
   const lldb_private::Section *mach_header_section,
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -1141,6 +1141,9 @@
   return m_header.flags & MH_DYLIB_IN_CACHE;
 }
 
+bool ObjectFileMachO::IsKext() const {
+  return m_header.filetype == MH_KEXT_BUNDLE;
+}
 uint32_t ObjectFileMachO::GetAddressByteSize() const {
   return m_data.GetAddressByteSize();
 }
Index: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===
--- lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 #include "Plugins/Platform/MacOSX/PlatformDarwinKernel.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Debugger.h"
@@ -713,6 +714,7 @@
 
 bool DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
 Process *process) {
+  Log *log = GetLog(LLDBLog::DynamicLoader);
   if (IsLoaded())
 return true;
 
@@ -810,6 +812,25 @@
   }
 }
 
+ObjectFile *ondisk_object_file = m_module_sp->GetObjectFile();
+ObjectFileMachO *ondisk_objfile_macho = nullptr;
+if (ondisk_object_file)
+  ondisk_objfile_macho =
+  llvm::dyn_cast(ondisk_object_file);
+
+if (m_module_sp && m_uuid.IsValid() && m_module_sp->GetUUID() == m_uuid) {
+  if (ondisk_objfile_macho) {
+// If we have something in the kext list which is not a kext
+// and not the kernel, don't try to load it.
+if (!IsKernel() && !obj_macho->IsKext()) {
+  LLDB_LOGF(log,
+"'%s' with UUID %s is not a kext or kernel, not loading.",
+m_name.c_str(), m_uuid.GetAsString().c_str());
+  m_module_sp.reset();
+}
+  }
+}
+
 // If we managed to find a module, append it to the target's list of
 // images. If we also have a memory module, require that they have matching
 // UUIDs
@@ -837,6 +858,35 @@
 // it.
 const bool ignore_linkedit = !IsKernel();
 
+// Normally a kext will have its segment load commands
+// (LC_SEGMENT vmaddrs) corrected in memory to have their
+// actual segment addresses.
+// Userland proceses have their libraries updated the same way
+// by dyld.  The Mach-O load commands in memory are the canonical
+// addresses.
+//
+// If the kernel gives us a binary where the in-memory segment
+// vmaddr is incorrect, then this binary was put in memory without
+// updating its Mach-O load commands.  We should assume a static
+// slide

[Lldb-commits] [PATCH] D145609: [lldb] Change dwim-print to default to disabled persistent results

2023-03-08 Thread Dave Lee via Phabricator via lldb-commits

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

Cleanup TestDWIMPrint


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145609

Files:
  lldb/source/Commands/CommandObjectDWIMPrint.cpp
  lldb/source/Commands/CommandObjectExpression.cpp
  lldb/source/Commands/CommandObjectExpression.h
  lldb/test/API/commands/dwim-print/TestDWIMPrint.py

Index: lldb/test/API/commands/dwim-print/TestDWIMPrint.py
===
--- lldb/test/API/commands/dwim-print/TestDWIMPrint.py
+++ lldb/test/API/commands/dwim-print/TestDWIMPrint.py
@@ -16,18 +16,16 @@
 self.ci.HandleCommand(cmd, result)
 return result.GetOutput().rstrip()
 
-VAR_IDENT_RAW = r"(?:\$\d+|\w+) = "
-VAR_IDENT = re.compile(VAR_IDENT_RAW)
+VAR_IDENT = re.compile(r"(?:\$\d+|\w+) = ")
 
-def _mask_persistent_var(self, string: str) -> str:
+def _strip_result_var(self, string: str) -> str:
 """
-Replace persistent result variables (ex '$0', '$1', etc) with a regex
-that matches any persistent result (r'\$\d+'). The returned string can
-be matched against other `expression` results.
+Strip (persistent) result variables (ex '$0 = ', or 'someVar = ', etc).
+
+This allows for using the output of `expression`/`frame variable`, to
+compare it to `dwim-print` output, which disables result variables.
 """
-before, after = self.VAR_IDENT.split(string, maxsplit=1)
-# Support either a frame variable (\w+) or a persistent result (\$\d+).
-return re.escape(before) + self.VAR_IDENT_RAW + re.escape(after)
+return self.VAR_IDENT.subn("", string, 1)[0]
 
 def _expect_cmd(
 self,
@@ -46,19 +44,16 @@
 if actual_cmd == "frame variable":
 resolved_cmd = resolved_cmd.replace(" -- ", " ", 1)
 
-expected_output = self._run_cmd(resolved_cmd)
+resolved_cmd_output = self._run_cmd(resolved_cmd)
+dwim_cmd_output = self._strip_result_var(resolved_cmd_output)
 
 # Verify dwim-print chose the expected command.
 self.runCmd("settings set dwim-print-verbosity full")
-substrs = [f"note: ran `{resolved_cmd}`"]
-patterns = []
 
-if self.VAR_IDENT.search(expected_output):
-patterns.append(self._mask_persistent_var(expected_output))
-else:
-substrs.append(expected_output)
-
-self.expect(dwim_cmd, substrs=substrs, patterns=patterns)
+self.expect(dwim_cmd, substrs=[
+f"note: ran `{resolved_cmd}`",
+dwim_cmd_output,
+])
 
 def test_variables(self):
 """Test dwim-print with variables."""
Index: lldb/source/Commands/CommandObjectExpression.h
===
--- lldb/source/Commands/CommandObjectExpression.h
+++ lldb/source/Commands/CommandObjectExpression.h
@@ -53,7 +53,7 @@
 lldb::LanguageType language;
 LanguageRuntimeDescriptionDisplayVerbosity m_verbosity;
 LazyBool auto_apply_fixits;
-bool suppress_persistent_result;
+LazyBool suppress_persistent_result;
   };
 
   CommandObjectExpression(CommandInterpreter &interpreter);
Index: lldb/source/Commands/CommandObjectExpression.cpp
===
--- lldb/source/Commands/CommandObjectExpression.cpp
+++ lldb/source/Commands/CommandObjectExpression.cpp
@@ -151,7 +151,7 @@
 bool persist_result =
 OptionArgParser::ToBoolean(option_arg, true, &success);
 if (success)
-  suppress_persistent_result = !persist_result;
+  suppress_persistent_result = !persist_result ? eLazyBoolYes : eLazyBoolNo;
 else
   error.SetErrorStringWithFormat(
   "could not convert \"%s\" to a boolean value.",
@@ -187,7 +187,7 @@
   auto_apply_fixits = eLazyBoolCalculate;
   top_level = false;
   allow_jit = true;
-  suppress_persistent_result = false;
+  suppress_persistent_result = eLazyBoolCalculate;
 }
 
 llvm::ArrayRef
@@ -202,8 +202,9 @@
   options.SetCoerceToId(display_opts.use_objc);
   // Explicitly disabling persistent results takes precedence over the
   // m_verbosity/use_objc logic.
-  if (suppress_persistent_result)
-options.SetSuppressPersistentResult(true);
+  if (suppress_persistent_result != eLazyBoolCalculate)
+options.SetSuppressPersistentResult(suppress_persistent_result ==
+eLazyBoolYes);
   else if (m_verbosity == eLanguageRuntimeDescriptionDisplayVerbosityCompact)
 options.SetSuppressPersistentResult(display_opts.use_objc);
   options.SetUnwindOnError(unwind_on_error);
Index: lldb/source/Commands/CommandObjectDWIMPrint.cpp
===
--- lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ lldb/source/Commands/CommandObje

[Lldb-commits] [PATCH] D145612: [lldb] Only replace valobj with persisted one if not null in DWIMPrint

2023-03-08 Thread Dave Lee via Phabricator via lldb-commits

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

thank you




Comment at: lldb/source/Commands/CommandObjectDWIMPrint.cpp:92-94
+auto persisted_valobj = valobj_sp->Persist();
+if (persisted_valobj)
+  valobj_sp = persisted_valobj;




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145612

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

[Lldb-commits] [lldb] 581ac50 - [lldb] Only replace valobj with persisted one if not null in DWIMPrint

2023-03-08 Thread Augusto Noronha via lldb-commits


Author: Augusto Noronha
Date: 2023-03-08T14:18:40-08:00
New Revision: 581ac50d58b99a37244e9d4e0d8d12c9c810f472

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

LOG: [lldb] Only replace valobj with persisted one if not null in DWIMPrint

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

Added: 


Modified: 
lldb/source/Commands/CommandObjectDWIMPrint.cpp

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectDWIMPrint.cpp 
b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
index e2e6f6706dee0..d8bc7a1e89696 100644
--- a/lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ b/lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -88,8 +88,10 @@ bool CommandObjectDWIMPrint::DoExecute(StringRef command,
   if (StackFrame *frame = m_exe_ctx.GetFramePtr()) {
 auto valobj_sp = frame->FindVariable(ConstString(expr));
 if (valobj_sp && valobj_sp->GetError().Success()) {
-  if (!eval_options.GetSuppressPersistentResult())
-valobj_sp = valobj_sp->Persist();
+  if (!eval_options.GetSuppressPersistentResult()) {
+if (auto persisted_valobj = valobj_sp->Persist())
+  valobj_sp = persisted_valobj;
+  }
 
   if (verbosity == eDWIMPrintVerbosityFull) {
 StringRef flags;



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

[Lldb-commits] [PATCH] D145612: [lldb] Only replace valobj with persisted one if not null in DWIMPrint

2023-03-08 Thread Augusto Noronha via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG581ac50d58b9: [lldb] Only replace valobj with persisted one 
if not null in DWIMPrint (authored by augusto2112).

Changed prior to commit:
  https://reviews.llvm.org/D145612?vs=503503&id=503518#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145612

Files:
  lldb/source/Commands/CommandObjectDWIMPrint.cpp


Index: lldb/source/Commands/CommandObjectDWIMPrint.cpp
===
--- lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -88,8 +88,10 @@
   if (StackFrame *frame = m_exe_ctx.GetFramePtr()) {
 auto valobj_sp = frame->FindVariable(ConstString(expr));
 if (valobj_sp && valobj_sp->GetError().Success()) {
-  if (!eval_options.GetSuppressPersistentResult())
-valobj_sp = valobj_sp->Persist();
+  if (!eval_options.GetSuppressPersistentResult()) {
+if (auto persisted_valobj = valobj_sp->Persist())
+  valobj_sp = persisted_valobj;
+  }
 
   if (verbosity == eDWIMPrintVerbosityFull) {
 StringRef flags;


Index: lldb/source/Commands/CommandObjectDWIMPrint.cpp
===
--- lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -88,8 +88,10 @@
   if (StackFrame *frame = m_exe_ctx.GetFramePtr()) {
 auto valobj_sp = frame->FindVariable(ConstString(expr));
 if (valobj_sp && valobj_sp->GetError().Success()) {
-  if (!eval_options.GetSuppressPersistentResult())
-valobj_sp = valobj_sp->Persist();
+  if (!eval_options.GetSuppressPersistentResult()) {
+if (auto persisted_valobj = valobj_sp->Persist())
+  valobj_sp = persisted_valobj;
+  }
 
   if (verbosity == eDWIMPrintVerbosityFull) {
 StringRef flags;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON

2023-03-08 Thread Greg Clayton via Phabricator via lldb-commits

clayborg added inline comments.



Comment at: lldb/include/lldb/Core/Debugger.h:594
   lldb::TargetSP m_dummy_target_sp;
+  Diagnostics::CallbackID m_diagnostics_callback_id;
 

I am guessing these changes are in Debugger.h and Debugger.cpp are not related 
to this diff?



Comment at: lldb/source/Core/Debugger.cpp:830-845
+  if (Diagnostics::Enabled()) {
+m_diagnostics_callback_id = Diagnostics::Instance().AddCallback(
+[this](const FileSpec &dir) -> llvm::Error {
+  for (auto &entry : m_stream_handlers) {
+llvm::StringRef log_path = entry.first();
+llvm::StringRef file_name = llvm::sys::path::filename(log_path);
+FileSpec destination = dir.CopyByAppendingPathComponent(file_name);

I am guessing these changes are in Debugger.h and Debugger.cpp are not related 
to this diff?



Comment at: lldb/source/Symbol/Symbol.cpp:780-781
+
+bool fromJSON(const llvm::json::Value &value, lldb_private::JSONSymbol &symbol,
+  llvm::json::Path path) {
+  llvm::json::ObjectMapper o(value, path);

Should this return an llvm::Expected instead of a 
bool? Or is this fromJSON pattern used everywhere? Then we wouldn't need to 
fill in "path" and could return an error?



Comment at: lldb/source/Symbol/Symbol.cpp:804-805
+
+bool fromJSON(const llvm::json::Value &value, lldb::SymbolType &type,
+  llvm::json::Path path) {
+  if (auto str = value.getAsString()) {

Should this return an llvm::Expected instead of a bool? Or is this 
fromJSON pattern used everywhere? Then we wouldn't need to fill in "path" and 
could return an error?



Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:34
+  Expected json = json::parse(text);
+  ASSERT_TRUE(static_cast(json));
+

Change over to using EXPECT_THAT_EXPECTED. Repeat for all cases below.



Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:56
+  Expected symbol = Symbol::FromJSON(json_symbol, §_list);
+  ASSERT_TRUE(static_cast(symbol));
+  EXPECT_EQ(symbol->GetName(), ConstString("foo"));

Use EXPECT_THAT_EXPECTED for clarity



Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:81
+  Expected symbol = Symbol::FromJSON(json_symbol, §_list);
+  ASSERT_TRUE(static_cast(symbol));
+  EXPECT_EQ(symbol->GetName(), ConstString("foo"));





Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:143-144
+  Expected symbol = Symbol::FromJSON(json_symbol, nullptr);
+  ASSERT_FALSE(static_cast(symbol));
+  EXPECT_EQ(toString(symbol.takeError()), g_error_no_section_list);
+}

Use EXPECT_THAT_EXPECTED for all "Expected" error cases.



Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:155-156
+  Expected symbol = Symbol::FromJSON(json_symbol, §_list);
+  ASSERT_FALSE(static_cast(symbol));
+  EXPECT_EQ(toString(symbol.takeError()), g_error_both_value_and_address);
+}

Use EXPECT_THAT_EXPECTED for all "Expected" error cases.



Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:165-166
+  Expected symbol = Symbol::FromJSON(json_symbol, §_list);
+  ASSERT_FALSE(static_cast(symbol));
+  EXPECT_EQ(toString(symbol.takeError()), g_error_neither_value_or_address);
+}

Use EXPECT_THAT_EXPECTED for all "Expected" error cases.



Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:189-191
+  ASSERT_FALSE(static_cast(symbol));
+  EXPECT_EQ(toString(symbol.takeError()),
+"no section found for address: 0xfff");

Use EXPECT_THAT_EXPECTED for all "Expected" error cases


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

https://reviews.llvm.org/D145180

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

[Lldb-commits] [PATCH] D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON

2023-03-08 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere updated this revision to Diff 503543.
JDevlieghere marked 21 inline comments as done.
JDevlieghere added a comment.

- Remove 9d311dd6a71b 
 from patch
- Use `EXPECT_THAT_EXPECTED`


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

https://reviews.llvm.org/D145180

Files:
  lldb/include/lldb/Symbol/Symbol.h
  lldb/source/Core/Debugger.cpp
  lldb/source/Plugins/ObjectFile/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/JSON/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp
  lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.h
  lldb/source/Plugins/SymbolFile/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/JSON/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/JSON/SymbolFileJSON.cpp
  lldb/source/Plugins/SymbolFile/JSON/SymbolFileJSON.h
  lldb/source/Symbol/Symbol.cpp
  lldb/test/API/macosx/symbols/Makefile
  lldb/test/API/macosx/symbols/TestSymbolFileJSON.py
  lldb/test/API/macosx/symbols/main.c
  lldb/unittests/Symbol/CMakeLists.txt
  lldb/unittests/Symbol/JSONSymbolTest.cpp

Index: lldb/unittests/Symbol/JSONSymbolTest.cpp
===
--- /dev/null
+++ lldb/unittests/Symbol/JSONSymbolTest.cpp
@@ -0,0 +1,192 @@
+//===-- JSONSymbolTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/Section.h"
+#include "lldb/Symbol/Symbol.h"
+#include "llvm/Testing/Support/Error.h"
+
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace llvm;
+using namespace lldb_private;
+
+static std::string g_error_no_section_list = "no section list provided";
+static std::string g_error_both_value_and_address =
+"symbol cannot contain both a value and an address";
+static std::string g_error_neither_value_or_address =
+"symbol must contain either a value or an address";
+
+TEST(JSONSymbolTest, DeserializeCodeAddress) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32,
+  "address": 4096
+})";
+
+  Expected json = json::parse(text);
+  ASSERT_TRUE(static_cast(json));
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_TRUE(fromJSON(*json, json_symbol, root));
+
+  SectionSP sect_sp(new Section(
+  /*module_sp=*/ModuleSP(),
+  /*obj_file=*/nullptr,
+  /*sect_id=*/1,
+  /*name=*/ConstString(".text"),
+  /*sect_type=*/eSectionTypeCode,
+  /*file_vm_addr=*/0x1000,
+  /*vm_size=*/0x1000,
+  /*file_offset=*/0,
+  /*file_size=*/0,
+  /*log2align=*/5,
+  /*flags=*/0x10203040));
+  SectionList sect_list;
+  sect_list.AddSection(sect_sp);
+
+  Expected symbol = Symbol::FromJSON(json_symbol, §_list);
+  EXPECT_THAT_EXPECTED(symbol, llvm::Succeeded());
+  EXPECT_EQ(symbol->GetName(), ConstString("foo"));
+  EXPECT_EQ(symbol->GetFileAddress(), static_cast(0x1000));
+  EXPECT_EQ(symbol->GetType(), eSymbolTypeCode);
+}
+
+TEST(JSONSymbolTest, DeserializeCodeValue) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32,
+  "value": 4096
+})";
+
+  Expected json = json::parse(text);
+  EXPECT_THAT_EXPECTED(json, llvm::Succeeded());
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_TRUE(fromJSON(*json, json_symbol, root));
+
+  SectionList sect_list;
+
+  Expected symbol = Symbol::FromJSON(json_symbol, §_list);
+  EXPECT_THAT_EXPECTED(symbol, llvm::Succeeded());
+  EXPECT_EQ(symbol->GetName(), ConstString("foo"));
+  EXPECT_EQ(symbol->GetRawValue(), static_cast(0x1000));
+  EXPECT_EQ(symbol->GetType(), eSymbolTypeCode);
+}
+
+TEST(JSONSymbolTest, JSONInvalidValueAndAddress) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32,
+  "value": 4096,
+  "address": 4096
+})";
+
+  Expected json = json::parse(text);
+  EXPECT_THAT_EXPECTED(json, llvm::Succeeded());
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_FALSE(fromJSON(*json, json_symbol, root));
+}
+
+TEST(JSONSymbolTest, JSONInvalidNoValueOrAddress) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32
+})";
+
+  Expected json = json::parse(text);
+  EXPECT_THAT_EXPECTED(json, llvm::Succeeded());
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_FALSE(fromJSON(*json, json_symbol, root));
+}
+
+TEST(JSONSymbolTest, JSONInvalidType) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "bogus",
+  "value": 4096,
+  "size": 32
+})";
+
+  Expected json = json::parse(text);
+  EXPECT_THAT_EXPECTED(json, llvm::Succeeded());
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_FALSE(fromJSON(*json, json_symbol, root));
+}
+
+TEST(JSONSymbolTest, SymbolInvalidN

[Lldb-commits] [PATCH] D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON

2023-03-08 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere added inline comments.



Comment at: lldb/include/lldb/Core/Debugger.h:594
   lldb::TargetSP m_dummy_target_sp;
+  Diagnostics::CallbackID m_diagnostics_callback_id;
 

clayborg wrote:
> I am guessing these changes are in Debugger.h and Debugger.cpp are not 
> related to this diff?
Yup. This got unintentionally mixed with D135631. 



Comment at: lldb/source/Symbol/Symbol.cpp:780-781
+
+bool fromJSON(const llvm::json::Value &value, lldb_private::JSONSymbol &symbol,
+  llvm::json::Path path) {
+  llvm::json::ObjectMapper o(value, path);

clayborg wrote:
> Should this return an llvm::Expected instead of a 
> bool? Or is this fromJSON pattern used everywhere? Then we wouldn't need to 
> fill in "path" and could return an error?
No, these are specializations/overloads for the JSON library in LLVM. Same 
below. 


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

https://reviews.llvm.org/D145180

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

[Lldb-commits] [PATCH] D145181: Revert "Remove the LINK_COMPONENTS entry from lldb-instr CMakery"

2023-03-08 Thread Heejin Ahn via Phabricator via lldb-commits

aheejin added a comment.

Thanks for the info. I asked questions in 
https://github.com/llvm/llvm-project/issues/60314 but they were not answered 
either. What should I do with this CL? Can you at least let me know why this 
wouldn't work given that there are many files like this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145181

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

[Lldb-commits] [PATCH] D143695: [lldb] Make repeat commands work for regex commands

2023-03-08 Thread Jim Ingham via Phabricator via lldb-commits

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143695

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

[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

2023-03-08 Thread Alex Langford via Phabricator via lldb-commits

bulbazord created this revision.
bulbazord added reviewers: JDevlieghere, mib, clayborg, jasonmolenda, jingham, 
labath.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

MemoryCache::Read is not resilient to partial reads when reading memory
chunks less than or equal in size to L2 cache lines. There have been
attempts in the past to fix this but nothing really solved the root of
the issue.

I first created a test exercising MemoryCache's implementation and
documenting how I believe MemoryCache::Read should behave. I then
rewrote the implementation of MemoryCache::Read as needed to make sure
that the different scenarios behaved correctly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145624

Files:
  lldb/source/Target/Memory.cpp
  lldb/unittests/Target/CMakeLists.txt
  lldb/unittests/Target/MemoryTest.cpp

Index: lldb/unittests/Target/MemoryTest.cpp
===
--- /dev/null
+++ lldb/unittests/Target/MemoryTest.cpp
@@ -0,0 +1,228 @@
+//===-- MemoryTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Target/Memory.h"
+#include "Plugins/Platform/MacOSX/PlatformMacOSX.h"
+#include "Plugins/Platform/MacOSX/PlatformRemoteMacOSX.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Target.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/DataBufferHeap.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb_private::repro;
+using namespace lldb;
+
+namespace {
+class MemoryTest : public ::testing::Test {
+public:
+  void SetUp() override {
+FileSystem::Initialize();
+HostInfo::Initialize();
+PlatformMacOSX::Initialize();
+  }
+  void TearDown() override {
+PlatformMacOSX::Terminate();
+HostInfo::Terminate();
+FileSystem::Terminate();
+  }
+};
+
+class DummyProcess : public Process {
+public:
+  DummyProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+  : Process(target_sp, listener_sp), m_bytes_left(0) {}
+
+  // Required overrides
+  bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) override {
+return true;
+  }
+  Status DoDestroy() override { return {}; }
+  void RefreshStateAfterStop() override {}
+  size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
+  Status &error) override {
+if (m_bytes_left == 0)
+  return 0;
+
+size_t num_bytes_to_write = size;
+if (m_bytes_left < size) {
+  num_bytes_to_write = m_bytes_left;
+  m_bytes_left = 0;
+} else {
+  m_bytes_left -= size;
+}
+
+memset(buf, 'B', num_bytes_to_write);
+return num_bytes_to_write;
+  }
+  bool DoUpdateThreadList(ThreadList &old_thread_list,
+  ThreadList &new_thread_list) override {
+return false;
+  }
+  llvm::StringRef GetPluginName() override { return "Dummy"; }
+
+  // Test-specific additions
+  size_t m_bytes_left;
+  MemoryCache &GetMemoryCache() { return m_memory_cache; }
+  void SetMaxReadSize(size_t size) { m_bytes_left = size; }
+};
+} // namespace
+
+TargetSP CreateTarget(DebuggerSP &debugger_sp, ArchSpec &arch) {
+  PlatformSP platform_sp;
+  TargetSP target_sp;
+  debugger_sp->GetTargetList().CreateTarget(
+  *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
+  return target_sp;
+}
+
+TEST_F(MemoryTest, TesetMemoryCacheRead) {
+  ArchSpec arch("x86_64-apple-macosx-");
+
+  Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch));
+
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+
+  TargetSP target_sp = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP process_sp = std::make_shared(target_sp, listener_sp);
+  ASSERT_TRUE(process_sp);
+
+  DummyProcess *process = static_cast(process_sp.get());
+  MemoryCache &mem_cache = process->GetMemoryCache();
+  const uint64_t l2_cache_size = process->GetMemoryCacheLineSize();
+  Status error;
+  auto data_sp = std::make_shared(l2_cache_size * 2, '\0');
+  size_t bytes_read = 0;
+
+  // Cache empty, memory read fails, size > l2 cache size
+  process->SetMaxReadSize(0);
+  bytes_read = mem_cache.Read(0x1000, data_sp->GetBytes(),
+  data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == 0);
+
+  // Cache empty, memory read fails, size <= l2 cache size
+  data_sp->SetByteSize(l2_cache_size);
+  bytes_re

[Lldb-commits] [PATCH] D145569: [lldb][InstrumentationRuntime] Make 'data' struct anonymous in order to avoid collisions with types in the debuggee

2023-03-08 Thread Jim Ingham via Phabricator via lldb-commits

jingham added a comment.

The TSAN instrumenter has the same `struct data` definition.  I actually got a 
report of this crashing in TSAN yesterday.  You should do that one as well.  
Otherwise, LGTM and I don't think you need to add a test with "struct data" in 
it on the off chance that someone reverts this back to exactly `struct data`...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145569

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

[Lldb-commits] [PATCH] D145547: When setting load addresses on darwin kernel kexts, handle case where in-memory load commands are not updated

2023-03-08 Thread Jason Molenda via Phabricator via lldb-commits

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

Add an additional bit to the "don't load non-kexts, non-kernels" addition.  I 
refined this by checking to see if the binary is already registered in the 
Target, and already has a load address set. If the binary is, then we don't 
re-register it.

If we encounter a program that is a non-kext, non-kernel binary and isn't yet 
loaded in the Target, we will load it.  So the code that handles in-memory load 
commands with incorrect vmaddrs is still needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145547

Files:
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/CMakeLists.txt
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h

Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
@@ -90,6 +90,8 @@
 
   bool IsSharedCacheBinary() const;
 
+  bool IsKext() const;
+
   uint32_t GetAddressByteSize() const override;
 
   lldb_private::AddressClass GetAddressClass(lldb::addr_t file_addr) override;
@@ -151,6 +153,8 @@
 
   bool AllowAssemblyEmulationUnwindPlans() override;
 
+  lldb_private::Section *GetMachHeaderSection();
+
   // PluginInterface protocol
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
 
@@ -193,8 +197,6 @@
   /// should not be used.
   void GetLLDBSharedCacheUUID(lldb::addr_t &base_addir, lldb_private::UUID &uuid);
 
-  lldb_private::Section *GetMachHeaderSection();
-
   lldb::addr_t CalculateSectionLoadAddressForMemoryImage(
   lldb::addr_t mach_header_load_address,
   const lldb_private::Section *mach_header_section,
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -1141,6 +1141,9 @@
   return m_header.flags & MH_DYLIB_IN_CACHE;
 }
 
+bool ObjectFileMachO::IsKext() const {
+  return m_header.filetype == MH_KEXT_BUNDLE;
+}
 uint32_t ObjectFileMachO::GetAddressByteSize() const {
   return m_data.GetAddressByteSize();
 }
Index: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===
--- lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 #include "Plugins/Platform/MacOSX/PlatformDarwinKernel.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Debugger.h"
@@ -713,6 +714,7 @@
 
 bool DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
 Process *process) {
+  Log *log = GetLog(LLDBLog::DynamicLoader);
   if (IsLoaded())
 return true;
 
@@ -810,6 +812,32 @@
   }
 }
 
+ObjectFile *ondisk_object_file = nullptr;
+ObjectFileMachO *ondisk_objfile_macho = nullptr;
+if (m_module_sp)
+  ondisk_object_file = m_module_sp->GetObjectFile();
+if (ondisk_object_file)
+  ondisk_objfile_macho =
+  llvm::dyn_cast(ondisk_object_file);
+
+if (m_module_sp && m_uuid.IsValid() && m_module_sp->GetUUID() == m_uuid) {
+  if (ondisk_objfile_macho) {
+if (!IsKernel() && !ondisk_objfile_macho->IsKext()) {
+  // We have a non-kext, non-kernel binary.  If we already have this
+  // loaded in the Target with load addresses, don't re-load it again.
+  ModuleSP existing_module_sp = target.GetImages().FindModule(m_uuid);
+  if (existing_module_sp &&
+  existing_module_sp->IsLoadedInTarget(&target)) {
+LLDB_LOGF(log,
+  "'%s' with UUID %s is not a kext or kernel, and is "
+  "already registered in target, not loading.",
+  m_name.c_str(), m_uuid.GetAsString().c_str());
+m_module_sp.reset();
+  }
+}
+  }
+}
+
 // If we managed to find a module, append it to the target's list of
 // images. If we also have a memory module, require that they have matching
 // UUIDs
@@ -837,6 +865,35 @@
 // it.
 const bool ignore_linkedit = !IsKernel();
 
+// Normally a kext will have its segment load commands
+// (LC_SEGMENT vmaddrs) corrected in memory to have their
+// actual segment addresses.
+// Userland proceses have their librarie

[Lldb-commits] [PATCH] D145294: [lldb/API] Introduce SBProcess::ForceScriptedState method

2023-03-08 Thread Jim Ingham via Phabricator via lldb-commits

jingham added a comment.

The code seems fine, but I don't think "private state" is a concept that people 
who aren't intimately familiar with lldb are likely to understand.  I think you 
could just leave out private and just say it changes the process state to the 
new state.  That's all people using it really need to know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145294

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

[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

2023-03-08 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere added inline comments.



Comment at: lldb/source/Target/Memory.cpp:131
+
   size_t bytes_left = dst_len;
 

This isn't used until line 180. I'd move it down, closer to where it is being 
used.



Comment at: lldb/source/Target/Memory.cpp:133-141
   // Check the L1 cache for a range that contain the entire memory read. If we
-  // find a range in the L1 cache that does, we use it. Else we fall back to
-  // reading memory in m_L2_cache_line_byte_size byte sized chunks. The L1
-  // cache contains chunks of memory that are not required to be
+  // find a range in the L1 cache that does, we use it. If not:
+  //  - dst_len > L2 cache line size: Attempt to read the entire memory chunk
+  //and insert whatever we get back into the L1 cache.
+  //  - dst_len <= L2 cache line size: Attempt to read it from the L2 cache, 
and
+  //if its not there, attempt to populate the cache.
+  // The L1 cache contains chunks of memory that are not required to be

Instead of describing the algorithm here, would it make sense to break this up 
and put it above the relevant code below? It seems like it matches pretty well 
with the code structure. Looking at the signature of `FindEntryThatContains` 
and the fact that it doesn't take the size, I assume it's because we only check 
the start address? 



Comment at: lldb/source/Target/Memory.cpp:145-147
+  // FIXME: We should do a more thorough check to make sure that we're not
+  // overlapping with any invalid ranges (e.g. Read 0x100 - 0x200 but there's 
an
+  // invalid range 0x180 - 0x280).

What would a more thorough check look like? Or phrased differently: what is the 
current check missing?



Comment at: lldb/source/Target/Memory.cpp:149
+  if (m_invalid_ranges.FindEntryThatContains(addr)) {
+error.SetErrorStringWithFormat("memory read failed for 0x%" PRIx64, addr);
+return 0;

Should the error mention that if failed due to the address overlapping with an 
invalid range? Is an invalid range something that is meaningful to the user? 



Comment at: lldb/source/Target/Memory.cpp:196-198
+  // If we didn't fill out the cache line completely and the offset is
+  // bigger than what we have available, we can't do anything further
+  // here.

Why can't we read from the process? Same question below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145624

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

[Lldb-commits] [PATCH] D143695: [lldb] Make repeat commands work for regex commands

2023-03-08 Thread Dave Lee via Phabricator via lldb-commits

kastiglione added a comment.

thanks for the feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143695

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

[Lldb-commits] [lldb] 5e0ee1b - [lldb] Make repeat commands work for regex commands

2023-03-08 Thread Dave Lee via lldb-commits


Author: Dave Lee
Date: 2023-03-08T16:21:42-08:00
New Revision: 5e0ee1b395fa6fca9f23f278885c5949277de666

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

LOG: [lldb] Make repeat commands work for regex commands

Fix logic for repeat commands, so that regex commands (specificially `bt`) are
given the opportunity to provide a repeat command.

rdar://104562616

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

Added: 
lldb/test/API/functionalities/alias/TestBtAliasRepeat.py

Modified: 
lldb/include/lldb/Interpreter/CommandInterpreter.h
lldb/source/Commands/CommandObjectRegexCommand.cpp
lldb/source/Interpreter/CommandInterpreter.cpp

Removed: 




diff  --git a/lldb/include/lldb/Interpreter/CommandInterpreter.h 
b/lldb/include/lldb/Interpreter/CommandInterpreter.h
index 38ff061612b11..ad6427330dfd4 100644
--- a/lldb/include/lldb/Interpreter/CommandInterpreter.h
+++ b/lldb/include/lldb/Interpreter/CommandInterpreter.h
@@ -352,7 +352,8 @@ class CommandInterpreter : public Broadcaster,
  CommandReturnObject &result);
 
   bool HandleCommand(const char *command_line, LazyBool add_to_history,
- CommandReturnObject &result);
+ CommandReturnObject &result,
+ bool force_repeat_command = false);
 
   bool WasInterrupted() const;
 

diff  --git a/lldb/source/Commands/CommandObjectRegexCommand.cpp 
b/lldb/source/Commands/CommandObjectRegexCommand.cpp
index 2d44ada0acc93..90b32b193e329 100644
--- a/lldb/source/Commands/CommandObjectRegexCommand.cpp
+++ b/lldb/source/Commands/CommandObjectRegexCommand.cpp
@@ -72,8 +72,9 @@ bool CommandObjectRegexCommand::DoExecute(llvm::StringRef 
command,
 result.GetOutputStream().Printf("%s\n", new_command->c_str());
   // We don't have to pass an override_context here, as the command that 
   // called us should have set up the context appropriately.
-  return m_interpreter.HandleCommand(new_command->c_str(),
- eLazyBoolNo, result);
+  bool force_repeat_command = true;
+  return m_interpreter.HandleCommand(new_command->c_str(), eLazyBoolNo,
+ result, force_repeat_command);
 }
   }
   result.SetStatus(eReturnStatusFailed);

diff  --git a/lldb/source/Interpreter/CommandInterpreter.cpp 
b/lldb/source/Interpreter/CommandInterpreter.cpp
index c03806d94210c..f42dd6cb40794 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -1874,8 +1874,8 @@ bool CommandInterpreter::HandleCommand(const char 
*command_line,
 
 bool CommandInterpreter::HandleCommand(const char *command_line,
LazyBool lazy_add_to_history,
-   CommandReturnObject &result) {
-
+   CommandReturnObject &result,
+   bool force_repeat_command) {
   std::string command_string(command_line);
   std::string original_command_string(command_line);
 
@@ -2004,17 +2004,26 @@ bool CommandInterpreter::HandleCommand(const char 
*command_line,
   // arguments.
 
   if (cmd_obj != nullptr) {
+bool generate_repeat_command = add_to_history;
 // If we got here when empty_command was true, then this command is a
 // stored "repeat command" which we should give a chance to produce it's
 // repeat command, even though we don't add repeat commands to the history.
-if (add_to_history || empty_command) {
+generate_repeat_command |= empty_command;
+// For `command regex`, the regex command (ex `bt`) is added to history, 
but
+// the resolved command (ex `thread backtrace`) is _not_ added to history.
+// However, the resolved command must be given the opportunity to provide a
+// repeat command. `force_repeat_command` supports this case.
+generate_repeat_command |= force_repeat_command;
+if (generate_repeat_command) {
   Args command_args(command_string);
   std::optional repeat_command =
   cmd_obj->GetRepeatCommand(command_args, 0);
-  if (repeat_command)
+  if (repeat_command) {
+LLDB_LOGF(log, "Repeat command: %s", repeat_command->data());
 m_repeat_command.assign(*repeat_command);
-  else
+  } else {
 m_repeat_command.assign(original_command_string);
+  }
 }
 
 if (add_to_history)

diff  --git a/lldb/test/API/functionalities/alias/TestBtAliasRepeat.py 
b/lldb/test/API/functionalities/alias/TestBtAliasRepeat.py
new file mode 100644
index 0..42b5accddc0ba
--- /dev/null
+++ b/lldb/test/API/functionalities/alias/TestBtAliasRepeat.py
@@ -0,0 +1,19 @@
+import lldb
+from lldbsuite.test.lldbtest

[Lldb-commits] [PATCH] D143695: [lldb] Make repeat commands work for regex commands

2023-03-08 Thread Dave Lee via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5e0ee1b395fa: [lldb] Make repeat commands work for regex 
commands (authored by kastiglione).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143695

Files:
  lldb/include/lldb/Interpreter/CommandInterpreter.h
  lldb/source/Commands/CommandObjectRegexCommand.cpp
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/test/API/functionalities/alias/TestBtAliasRepeat.py


Index: lldb/test/API/functionalities/alias/TestBtAliasRepeat.py
===
--- /dev/null
+++ lldb/test/API/functionalities/alias/TestBtAliasRepeat.py
@@ -0,0 +1,19 @@
+import lldb
+from lldbsuite.test.lldbtest import TestBase
+from lldbsuite.test import lldbutil
+
+
+class TestCase(TestBase):
+def test(self):
+self.build()
+lldbutil.run_to_source_breakpoint(self, "return", 
lldb.SBFileSpec("main.c"))
+
+# Expect "frame #0" but not "frame #1".
+self.expect("bt 1", inHistory=True, patterns=["frame #0", "^(?!.*frame 
#1)"])
+
+# Run an empty command to run the repeat command for `bt`.
+# The repeat command for `bt N` lists the subsequent N frames.
+#
+# In this case, after printing the frame 0 with `bt 1`, the repeat
+# command will print "frame #1" (and won't print "frame #0").
+self.expect("", patterns=["^(?!.*frame #0)", "frame #1"])
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -1874,8 +1874,8 @@
 
 bool CommandInterpreter::HandleCommand(const char *command_line,
LazyBool lazy_add_to_history,
-   CommandReturnObject &result) {
-
+   CommandReturnObject &result,
+   bool force_repeat_command) {
   std::string command_string(command_line);
   std::string original_command_string(command_line);
 
@@ -2004,17 +2004,26 @@
   // arguments.
 
   if (cmd_obj != nullptr) {
+bool generate_repeat_command = add_to_history;
 // If we got here when empty_command was true, then this command is a
 // stored "repeat command" which we should give a chance to produce it's
 // repeat command, even though we don't add repeat commands to the history.
-if (add_to_history || empty_command) {
+generate_repeat_command |= empty_command;
+// For `command regex`, the regex command (ex `bt`) is added to history, 
but
+// the resolved command (ex `thread backtrace`) is _not_ added to history.
+// However, the resolved command must be given the opportunity to provide a
+// repeat command. `force_repeat_command` supports this case.
+generate_repeat_command |= force_repeat_command;
+if (generate_repeat_command) {
   Args command_args(command_string);
   std::optional repeat_command =
   cmd_obj->GetRepeatCommand(command_args, 0);
-  if (repeat_command)
+  if (repeat_command) {
+LLDB_LOGF(log, "Repeat command: %s", repeat_command->data());
 m_repeat_command.assign(*repeat_command);
-  else
+  } else {
 m_repeat_command.assign(original_command_string);
+  }
 }
 
 if (add_to_history)
Index: lldb/source/Commands/CommandObjectRegexCommand.cpp
===
--- lldb/source/Commands/CommandObjectRegexCommand.cpp
+++ lldb/source/Commands/CommandObjectRegexCommand.cpp
@@ -72,8 +72,9 @@
 result.GetOutputStream().Printf("%s\n", new_command->c_str());
   // We don't have to pass an override_context here, as the command that 
   // called us should have set up the context appropriately.
-  return m_interpreter.HandleCommand(new_command->c_str(),
- eLazyBoolNo, result);
+  bool force_repeat_command = true;
+  return m_interpreter.HandleCommand(new_command->c_str(), eLazyBoolNo,
+ result, force_repeat_command);
 }
   }
   result.SetStatus(eReturnStatusFailed);
Index: lldb/include/lldb/Interpreter/CommandInterpreter.h
===
--- lldb/include/lldb/Interpreter/CommandInterpreter.h
+++ lldb/include/lldb/Interpreter/CommandInterpreter.h
@@ -352,7 +352,8 @@
  CommandReturnObject &result);
 
   bool HandleCommand(const char *command_line, LazyBool add_to_history,
- CommandReturnObject &result);
+ CommandReturnObject &result,
+ bool force_repeat_command = false);
 
   bool WasInterrupted() const;
 


Index: lldb/test/API/functionalities/alias/Tes

[Lldb-commits] [PATCH] D145547: When setting load addresses on darwin kernel kexts, handle case where in-memory load commands are not updated

2023-03-08 Thread Jonas Devlieghere via Phabricator via lldb-commits

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

Some code style nits but the change itself LGTM.




Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:819-821
+if (ondisk_object_file)
+  ondisk_objfile_macho =
+  llvm::dyn_cast(ondisk_object_file);

You can use `dyn_cast_or_null` to eliminate the null check:
 ```ObjectFileMachO *ondisk_objfile_macho = 
llvm::dyn_cast_or_null(ondisk_object_file);```

I hate temporaries that are used only once below, so I'd probably write:

```
ObjectFileMachO *ondisk_objfile_macho = 
llvm::dyn_cast_or_null(m_module_sp ? 
m_module_sp->GetObjectFile() : nullptr);
```
but I'm probably the only one that likes that better.



Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:835
+  m_name.c_str(), m_uuid.GetAsString().c_str());
+m_module_sp.reset();
+  }

What's the purpose of reseting the shared pointer? It'll just go out of scope 
right after. 



Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:881-883
+ObjectFileMachO *memory_objfile_macho =
+llvm::dyn_cast(memory_object_file);
+if (memory_objfile_macho) {

Since you already did it for `Section` below...
```
if(ObjectFileMachO *memory_objfile_macho =
llvm::dyn_cast(memory_object_file)) {
```



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1146
+  return m_header.filetype == MH_KEXT_BUNDLE;
+}
 uint32_t ObjectFileMachO::GetAddressByteSize() const {

Newline below


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145547

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

[Lldb-commits] [PATCH] D145629: When a ValueObjectDynamicValue fails to update, return a not valid ValueObject so the static one is used instead

2023-03-08 Thread Jim Ingham via Phabricator via lldb-commits

jingham created this revision.
jingham added reviewers: JDevlieghere, kastiglione, mib, delcypher.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

When we can't find a valid type for a dynamic value (for instance we just have 
a void * and can't compute anything for it) we used to try to hand out a 
dynamic value that "behaves like the static value".  That mostly works, but 
getting something to behave like ValueObjectConstResult without being it didn't 
work - for instance AddressOf fails for this DynamicValue.

The comment before this section of code mentions that we might should return an 
invalid dynamic value instead, so that the static one would get used.  I 
switched over to that, and I just needed to fix up a few cases where we were 
handing out a dynamic value w/o making sure it was valid first.  That ends up 
being much simpler than trying to get this ValueObjectDynamicValue to be "just 
like" ValueObjectConstResult.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145629

Files:
  lldb/source/Core/ValueObject.cpp
  lldb/source/Core/ValueObjectConstResult.cpp
  lldb/source/Core/ValueObjectDynamicValue.cpp
  
lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
  lldb/test/API/python_api/value/addr_of_void_star/Makefile
  
lldb/test/API/python_api/value/addr_of_void_star/TestValueAPIAddressOfVoidStar.py
  lldb/test/API/python_api/value/addr_of_void_star/main.c

Index: lldb/test/API/python_api/value/addr_of_void_star/main.c
===
--- /dev/null
+++ lldb/test/API/python_api/value/addr_of_void_star/main.c
@@ -0,0 +1,6 @@
+int main (int argc, char const *argv[]) {
+  char *char_ptr = "Some pointer here";
+  void *void_ptr = &char_ptr;
+  
+  return 0; // Break at this line
+}
Index: lldb/test/API/python_api/value/addr_of_void_star/TestValueAPIAddressOfVoidStar.py
===
--- /dev/null
+++ lldb/test/API/python_api/value/addr_of_void_star/TestValueAPIAddressOfVoidStar.py
@@ -0,0 +1,38 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class ValueAPIVoidStarTestCase(TestBase):
+
+def test(self):
+self.build()
+
+target, process, thread, _ = lldbutil.run_to_source_breakpoint(self,
+   "Break at this line",
+   lldb.SBFileSpec("main.c"))
+frame = thread.GetFrameAtIndex(0)
+
+# Verify that the expression result for a void * behaves the same way as the
+# variable value.
+
+var_val = frame.FindVariable("void_ptr")
+self.assertSuccess(var_val.GetError(), "Var version made correctly")
+
+expr_val = frame.EvaluateExpression("void_ptr")
+self.assertSuccess(expr_val.GetError(), "Expr version succeeds")
+
+# The pointer values should be equal:
+self.assertEqual(var_val.unsigned, expr_val.unsigned, "Values are equal")
+
+# Both versions should have valid AddressOf, and they should be the same.
+
+val_addr_of = var_val.AddressOf()
+self.assertNotEqual(val_addr_of, lldb.LLDB_INVALID_ADDRESS, "Var addr of right")
+
+expr_addr_of = expr_val.AddressOf()
+self.assertNotEqual(expr_addr_of, lldb.LLDB_INVALID_ADDRESS, "Expr addr of right")
+
+# The AddressOf values should also be equal.
+self.assertEqual(expr_addr_of.unsigned, val_addr_of.unsigned, "Addr of equal")
+
Index: lldb/test/API/python_api/value/addr_of_void_star/Makefile
===
--- /dev/null
+++ lldb/test/API/python_api/value/addr_of_void_star/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
Index: lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
===
--- lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
+++ lldb/source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp
@@ -583,7 +583,11 @@
   ValueObjectSP exception = ValueObject::CreateValueObjectFromData(
   "exception", exception_isw.GetAsData(m_process->GetByteOrder()), exe_ctx,
   voidstar);
-  exception = exception->GetDynamicValue(eDynamicDontRunTarget);
+  ValueObjectSP dyn_exception 
+  = exception->GetDynamicValue(eDynamicDontRunTarget);
+  // If we succeed in making a dynamic value, return that:
+  if (dyn_exception) 
+ return dyn_exception;
 
   return exception;
 }
Index: lldb/source/Core/ValueObjectDynamicValue.cpp
===
--- lldb/source/Cor

[Lldb-commits] [PATCH] D145547: When setting load addresses on darwin kernel kexts, handle case where in-memory load commands are not updated

2023-03-08 Thread Jason Molenda via Phabricator via lldb-commits

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



Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:835
+  m_name.c_str(), m_uuid.GetAsString().c_str());
+m_module_sp.reset();
+  }

JDevlieghere wrote:
> What's the purpose of reseting the shared pointer? It'll just go out of scope 
> right after. 
No, this method is trying a few different ways to find the on-disk binary for 
the kext, and it puts that Module in m_module_sp once it finds it.  (there's a 
bunch of blocks that start like `if (!m_module_sp) { ... try another way of 
finding the Module ...}` and the goal when we have a non-kext/non-kernel image 
that is already in the Target is to avoid re-loading it into the Target.  So by 
clearing it, we're behaving as if it couldn't be found.  

altho, tbh, looking this method I could just `return` here and it would 
accomplish the same.  And be clearer.



Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:881-883
+ObjectFileMachO *memory_objfile_macho =
+llvm::dyn_cast(memory_object_file);
+if (memory_objfile_macho) {

JDevlieghere wrote:
> Since you already did it for `Section` below...
> ```
> if(ObjectFileMachO *memory_objfile_macho =
> llvm::dyn_cast(memory_object_file)) {
> ```
yeah, I thought about that but it made for a really long line that would be a 
multi-line expression in the `if` and i wasn't thrilled about it.  but i don't 
care that much.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145547

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

[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

2023-03-08 Thread Med Ismail Bennani via Phabricator via lldb-commits

mib added inline comments.



Comment at: lldb/source/Target/Memory.cpp:175
 
-  if (dst && bytes_left > 0) {
-const uint32_t cache_line_byte_size = m_L2_cache_line_byte_size;
-uint8_t *dst_buf = (uint8_t *)dst;
-addr_t curr_addr = addr - (addr % cache_line_byte_size);
-addr_t cache_offset = addr - curr_addr;
-
-while (bytes_left > 0) {
-  if (m_invalid_ranges.FindEntryThatContains(curr_addr)) {
-error.SetErrorStringWithFormat("memory read failed for 0x%" PRIx64,
-   curr_addr);
-return dst_len - bytes_left;
-  }
+  const uint32_t cache_line_byte_size = m_L2_cache_line_byte_size;
+  uint8_t *dst_buf = (uint8_t *)dst;

nit: Is this necessary ?



Comment at: lldb/source/Target/Memory.cpp:177-178
+  uint8_t *dst_buf = (uint8_t *)dst;
+  addr_t cache_offset = addr % cache_line_byte_size;
+  addr_t curr_cache_line_base_addr = addr - cache_offset;
+

Could use a comment explaining what we're doing here.



Comment at: lldb/source/Target/Memory.cpp:200
+  if (cache_offset >= cached_line_size)
+return dst_len - bytes_left;
 

Shouldn't this be the size of the cached line ?



Comment at: lldb/source/Target/Memory.cpp:210
+  bytes_left -= curr_read_size;
+  curr_cache_line_base_addr += cache_line_byte_size;
+  cache_offset = 0;

IIUC, now that the read succeeded, you're shifting the current cache line base 
address to point to the next cache line base address, so you can continue 
reading if necessary. I had troubles understanding the point of this line 
before reading the rest of the code so either this should move closer to where 
it's used or at least it should have a comment explaining what it's doing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145624

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

[Lldb-commits] [PATCH] D145547: When setting load addresses on darwin kernel kexts, handle case where in-memory load commands are not updated

2023-03-08 Thread Jason Molenda via Phabricator via lldb-commits

jasonmolenda updated this revision to Diff 503569.
jasonmolenda marked an inline comment as done.
jasonmolenda added a comment.

Update for Jonas' suggestions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145547

Files:
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/CMakeLists.txt
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h

Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
@@ -90,6 +90,8 @@
 
   bool IsSharedCacheBinary() const;
 
+  bool IsKext() const;
+
   uint32_t GetAddressByteSize() const override;
 
   lldb_private::AddressClass GetAddressClass(lldb::addr_t file_addr) override;
@@ -151,6 +153,8 @@
 
   bool AllowAssemblyEmulationUnwindPlans() override;
 
+  lldb_private::Section *GetMachHeaderSection();
+
   // PluginInterface protocol
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
 
@@ -193,8 +197,6 @@
   /// should not be used.
   void GetLLDBSharedCacheUUID(lldb::addr_t &base_addir, lldb_private::UUID &uuid);
 
-  lldb_private::Section *GetMachHeaderSection();
-
   lldb::addr_t CalculateSectionLoadAddressForMemoryImage(
   lldb::addr_t mach_header_load_address,
   const lldb_private::Section *mach_header_section,
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -1141,6 +1141,10 @@
   return m_header.flags & MH_DYLIB_IN_CACHE;
 }
 
+bool ObjectFileMachO::IsKext() const {
+  return m_header.filetype == MH_KEXT_BUNDLE;
+}
+
 uint32_t ObjectFileMachO::GetAddressByteSize() const {
   return m_data.GetAddressByteSize();
 }
Index: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===
--- lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 #include "Plugins/Platform/MacOSX/PlatformDarwinKernel.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Debugger.h"
@@ -713,6 +714,7 @@
 
 bool DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
 Process *process) {
+  Log *log = GetLog(LLDBLog::DynamicLoader);
   if (IsLoaded())
 return true;
 
@@ -810,6 +812,28 @@
   }
 }
 
+if (m_module_sp && m_uuid.IsValid() && m_module_sp->GetUUID() == m_uuid) {
+  ObjectFileMachO *ondisk_objfile_macho =
+  llvm::dyn_cast_or_null(
+  m_module_sp ? m_module_sp->GetObjectFile() : nullptr);
+  if (ondisk_objfile_macho) {
+if (!IsKernel() && !ondisk_objfile_macho->IsKext()) {
+  // We have a non-kext, non-kernel binary.  If we already have this
+  // loaded in the Target with load addresses, don't re-load it again.
+  ModuleSP existing_module_sp = target.GetImages().FindModule(m_uuid);
+  if (existing_module_sp &&
+  existing_module_sp->IsLoadedInTarget(&target)) {
+LLDB_LOGF(log,
+  "'%s' with UUID %s is not a kext or kernel, and is "
+  "already registered in target, not loading.",
+  m_name.c_str(), m_uuid.GetAsString().c_str());
+// It's already loaded, return true.
+return true;
+  }
+}
+  }
+}
+
 // If we managed to find a module, append it to the target's list of
 // images. If we also have a memory module, require that they have matching
 // UUIDs
@@ -837,6 +861,34 @@
 // it.
 const bool ignore_linkedit = !IsKernel();
 
+// Normally a kext will have its segment load commands
+// (LC_SEGMENT vmaddrs) corrected in memory to have their
+// actual segment addresses.
+// Userland proceses have their libraries updated the same way
+// by dyld.  The Mach-O load commands in memory are the canonical
+// addresses.
+//
+// If the kernel gives us a binary where the in-memory segment
+// vmaddr is incorrect, then this binary was put in memory without
+// updating its Mach-O load commands.  We should assume a static
+// slide value will be applied to every segment; we don't have the
+// correct addresses for each in

[Lldb-commits] [lldb] 794d208 - Don't load non-kexts in darwin kernel debug; handle unslid segs

2023-03-08 Thread Jason Molenda via lldb-commits


Author: Jason Molenda
Date: 2023-03-08T16:51:27-08:00
New Revision: 794d2089cd741135b60e94ac2495174a03b38143

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

LOG: Don't load non-kexts in darwin kernel debug; handle unslid segs

We have some non-kexts in the binary list in the Darwin kernel
in some situations.  The binary has likely already been loaded;
check if it has been, and don't re-load it.  Also, if we do need
to load it at this point, if in-memory segment vmaddrs have not
been updated to the actual load addresses, calculate a fixed slide
for the in-memory image and apply that slide to the ondisk binary.

Differential Revision: https://reviews.llvm.org/D145547
rdar://106343477

Added: 


Modified: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/CMakeLists.txt

lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h

Removed: 




diff  --git a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/CMakeLists.txt 
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/CMakeLists.txt
index fb838f42489db..a22a83363ffea 100644
--- a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/CMakeLists.txt
+++ b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/CMakeLists.txt
@@ -17,6 +17,7 @@ add_lldb_library(lldbPluginDynamicLoaderDarwinKernel PLUGIN
 lldbSymbol
 lldbTarget
 lldbUtility
+lldbPluginObjectFileMachO
   )
 
 add_dependencies(lldbPluginDynamicLoaderDarwinKernel

diff  --git 
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp 
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index fcbaf7e1ed7d2..b3b199e1bf71d 100644
--- 
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ 
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -6,6 +6,7 @@
 //
 
//===--===//
 
+#include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 #include "Plugins/Platform/MacOSX/PlatformDarwinKernel.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Debugger.h"
@@ -713,6 +714,7 @@ void 
DynamicLoaderDarwinKernel::KextImageInfo::SetIsKernel(bool is_kernel) {
 
 bool DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
 Process *process) {
+  Log *log = GetLog(LLDBLog::DynamicLoader);
   if (IsLoaded())
 return true;
 
@@ -810,6 +812,28 @@ bool 
DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
   }
 }
 
+if (m_module_sp && m_uuid.IsValid() && m_module_sp->GetUUID() == m_uuid) {
+  ObjectFileMachO *ondisk_objfile_macho =
+  llvm::dyn_cast_or_null(
+  m_module_sp ? m_module_sp->GetObjectFile() : nullptr);
+  if (ondisk_objfile_macho) {
+if (!IsKernel() && !ondisk_objfile_macho->IsKext()) {
+  // We have a non-kext, non-kernel binary.  If we already have this
+  // loaded in the Target with load addresses, don't re-load it again.
+  ModuleSP existing_module_sp = target.GetImages().FindModule(m_uuid);
+  if (existing_module_sp &&
+  existing_module_sp->IsLoadedInTarget(&target)) {
+LLDB_LOGF(log,
+  "'%s' with UUID %s is not a kext or kernel, and is "
+  "already registered in target, not loading.",
+  m_name.c_str(), m_uuid.GetAsString().c_str());
+// It's already loaded, return true.
+return true;
+  }
+}
+  }
+}
+
 // If we managed to find a module, append it to the target's list of
 // images. If we also have a memory module, require that they have matching
 // UUIDs
@@ -837,6 +861,34 @@ bool 
DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
 // it.
 const bool ignore_linkedit = !IsKernel();
 
+// Normally a kext will have its segment load commands
+// (LC_SEGMENT vmaddrs) corrected in memory to have their
+// actual segment addresses.
+// Userland proceses have their libraries updated the same way
+// by dyld.  The Mach-O load commands in memory are the canonical
+// addresses.
+//
+// If the kernel gives us a binary where the in-memory segment
+// vmaddr is incorrect, then this binary was put in memory without
+// updating its Mach-O load commands.  We should assume a static
+// slide value will be applied to every segment; we don't have the
+// correct addresses for each individual segment.
+addr_t fixed_slide = LLDB_INVALID_ADDRESS;
+

[Lldb-commits] [PATCH] D145547: When setting load addresses on darwin kernel kexts, handle case where in-memory load commands are not updated

2023-03-08 Thread Jason Molenda via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG794d2089cd74: Don't load non-kexts in darwin kernel 
debug; handle unslid segs (authored by jasonmolenda).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145547

Files:
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/CMakeLists.txt
  lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h

Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
@@ -90,6 +90,8 @@
 
   bool IsSharedCacheBinary() const;
 
+  bool IsKext() const;
+
   uint32_t GetAddressByteSize() const override;
 
   lldb_private::AddressClass GetAddressClass(lldb::addr_t file_addr) override;
@@ -151,6 +153,8 @@
 
   bool AllowAssemblyEmulationUnwindPlans() override;
 
+  lldb_private::Section *GetMachHeaderSection();
+
   // PluginInterface protocol
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
 
@@ -193,8 +197,6 @@
   /// should not be used.
   void GetLLDBSharedCacheUUID(lldb::addr_t &base_addir, lldb_private::UUID &uuid);
 
-  lldb_private::Section *GetMachHeaderSection();
-
   lldb::addr_t CalculateSectionLoadAddressForMemoryImage(
   lldb::addr_t mach_header_load_address,
   const lldb_private::Section *mach_header_section,
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -1141,6 +1141,10 @@
   return m_header.flags & MH_DYLIB_IN_CACHE;
 }
 
+bool ObjectFileMachO::IsKext() const {
+  return m_header.filetype == MH_KEXT_BUNDLE;
+}
+
 uint32_t ObjectFileMachO::GetAddressByteSize() const {
   return m_data.GetAddressByteSize();
 }
Index: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===
--- lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 #include "Plugins/Platform/MacOSX/PlatformDarwinKernel.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Debugger.h"
@@ -713,6 +714,7 @@
 
 bool DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
 Process *process) {
+  Log *log = GetLog(LLDBLog::DynamicLoader);
   if (IsLoaded())
 return true;
 
@@ -810,6 +812,28 @@
   }
 }
 
+if (m_module_sp && m_uuid.IsValid() && m_module_sp->GetUUID() == m_uuid) {
+  ObjectFileMachO *ondisk_objfile_macho =
+  llvm::dyn_cast_or_null(
+  m_module_sp ? m_module_sp->GetObjectFile() : nullptr);
+  if (ondisk_objfile_macho) {
+if (!IsKernel() && !ondisk_objfile_macho->IsKext()) {
+  // We have a non-kext, non-kernel binary.  If we already have this
+  // loaded in the Target with load addresses, don't re-load it again.
+  ModuleSP existing_module_sp = target.GetImages().FindModule(m_uuid);
+  if (existing_module_sp &&
+  existing_module_sp->IsLoadedInTarget(&target)) {
+LLDB_LOGF(log,
+  "'%s' with UUID %s is not a kext or kernel, and is "
+  "already registered in target, not loading.",
+  m_name.c_str(), m_uuid.GetAsString().c_str());
+// It's already loaded, return true.
+return true;
+  }
+}
+  }
+}
+
 // If we managed to find a module, append it to the target's list of
 // images. If we also have a memory module, require that they have matching
 // UUIDs
@@ -837,6 +861,34 @@
 // it.
 const bool ignore_linkedit = !IsKernel();
 
+// Normally a kext will have its segment load commands
+// (LC_SEGMENT vmaddrs) corrected in memory to have their
+// actual segment addresses.
+// Userland proceses have their libraries updated the same way
+// by dyld.  The Mach-O load commands in memory are the canonical
+// addresses.
+//
+// If the kernel gives us a binary where the in-memory segment
+// vmaddr is incorrect, then this binary was put in memory without
+// updating its Mach-O load commands.  We should assume a static
+// slide value will be applied to every segment; we don't have the
+

[Lldb-commits] [PATCH] D145569: [lldb][InstrumentationRuntime] Make 'data' struct anonymous in order to avoid collisions with types in the debuggee

2023-03-08 Thread Michael Buch via Phabricator via lldb-commits

Michael137 updated this revision to Diff 503572.
Michael137 edited the summary of this revision.
Michael137 added a comment.

- Apply fix to TSAN structure as well


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145569

Files:
  lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
  
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
  lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp


Index: lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
===
--- lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
+++ lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
@@ -67,8 +67,11 @@
 size_t __asan_get_alloc_stack(void *addr, void **trace, size_t size, 
int *thread_id);
 size_t __asan_get_free_stack(void *addr, void **trace, size_t size, 
int *thread_id);
 }
+)";
 
-struct data {
+const char *memory_history_asan_command_format =
+R"(
+struct {
 void *alloc_trace[256];
 size_t alloc_count;
 int alloc_tid;
@@ -76,12 +79,7 @@
 void *free_trace[256];
 size_t free_count;
 int free_tid;
-};
-)";
-
-const char *memory_history_asan_command_format =
-R"(
-data t;
+} t;
 
 t.alloc_count = __asan_get_alloc_stack((void *)0x%)" PRIx64
 R"(, t.alloc_trace, 256, &t.alloc_tid);
Index: 
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
===
--- 
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
+++ 
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
@@ -67,19 +67,18 @@
 const char **OutMessage, const char **OutFilename, unsigned *OutLine,
 unsigned *OutCol, char **OutMemoryAddr);
 }
+)";
 
-struct data {
+static const char *ub_sanitizer_retrieve_report_data_command = R"(
+struct {
   const char *issue_kind;
   const char *message;
   const char *filename;
   unsigned line;
   unsigned col;
   char *memory_addr;
-};
-)";
+} t;
 
-static const char *ub_sanitizer_retrieve_report_data_command = R"(
-data t;
 __ubsan_get_current_report_data(&t.issue_kind, &t.message, &t.filename, 
&t.line,
 &t.col, &t.memory_addr);
 t;
Index: 
lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
===
--- 
lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
+++ 
lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
@@ -87,6 +87,9 @@
 void *dlsym(void* handle, const char* symbol);
 int (*ptr__tsan_get_report_loc_object_type)(void *report, unsigned long 
idx, const char **object_type);
 }
+)";
+
+const char *thread_sanitizer_retrieve_report_data_command = R"(
 
 const int REPORT_TRACE_SIZE = 128;
 const int REPORT_ARRAY_SIZE = 4;
@@ -154,11 +157,7 @@
 int idx;
 int tid;
 } unique_tids[REPORT_ARRAY_SIZE];
-};
-)";
-
-const char *thread_sanitizer_retrieve_report_data_command = R"(
-data t = {0};
+} t = {0};
 
 ptr__tsan_get_report_loc_object_type = 
(typeof(ptr__tsan_get_report_loc_object_type))(void *)dlsym((void*)-2 
/*RTLD_DEFAULT*/, "__tsan_get_report_loc_object_type");
 


Index: lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
===
--- lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
+++ lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
@@ -67,8 +67,11 @@
 size_t __asan_get_alloc_stack(void *addr, void **trace, size_t size, int *thread_id);
 size_t __asan_get_free_stack(void *addr, void **trace, size_t size, int *thread_id);
 }
+)";
 
-struct data {
+const char *memory_history_asan_command_format =
+R"(
+struct {
 void *alloc_trace[256];
 size_t alloc_count;
 int alloc_tid;
@@ -76,12 +79,7 @@
 void *free_trace[256];
 size_t free_count;
 int free_tid;
-};
-)";
-
-const char *memory_history_asan_command_format =
-R"(
-data t;
+} t;
 
 t.alloc_count = __asan_get_alloc_stack((void *)0x%)" PRIx64
 R"(, t.alloc_trace, 256, &t.alloc_tid);
Index: lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
===
--- lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
+++ lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
@@ -67,19 +67,18 @@
 const char **OutMessage, const char **OutFilename, unsigned *OutLine,
 unsigned *OutCol, char **OutMemoryAddr);
 }
+)";
 
-struct data {
+static const char *ub_sanitizer_retriev

[Lldb-commits] [PATCH] D145569: [lldb][InstrumentationRuntime] Make 'data' struct anonymous in order to avoid collisions with types in the debuggee

2023-03-08 Thread Michael Buch via Phabricator via lldb-commits

Michael137 added a comment.

In D145569#4179648 , @jingham wrote:

> The TSAN instrumenter has the same `struct data` definition.  I actually got 
> a report of this crashing in TSAN yesterday.  You should do that one as well. 
>  Otherwise, LGTM and I don't think you need to add a test with "struct data" 
> in it on the off chance that someone reverts this back to exactly `struct 
> data`...

Ah good point, done


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145569

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

[Lldb-commits] [PATCH] D145629: When a ValueObjectDynamicValue fails to update, return a not valid ValueObject so the static one is used instead

2023-03-08 Thread Med Ismail Bennani via Phabricator via lldb-commits

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

LGTM with a comment.




Comment at: lldb/source/Core/ValueObject.cpp:1862
   }
-  if (m_dynamic_value)
+  if (m_dynamic_value && m_dynamic_value->GetError().Success())
 return m_dynamic_value->GetSP();

It would be nice if `ValueObjectDynamicValue` had a `IsValid` method that 
returned the result of its Status attribute. Without reading the commit 
message, `GetError` doesn't tell us much about what we're trying to achieve 
here.



Comment at: lldb/source/Core/ValueObjectConstResult.cpp:290
 }
-if (m_dynamic_value)
+if (m_dynamic_value && m_dynamic_value->GetError().Success())
   return m_dynamic_value->GetSP();

ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145629

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

[Lldb-commits] [PATCH] D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON

2023-03-08 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere updated this revision to Diff 503575.

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

https://reviews.llvm.org/D145180

Files:
  lldb/include/lldb/Symbol/Symbol.h
  lldb/source/Plugins/ObjectFile/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/JSON/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp
  lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.h
  lldb/source/Plugins/SymbolFile/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/JSON/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/JSON/SymbolFileJSON.cpp
  lldb/source/Plugins/SymbolFile/JSON/SymbolFileJSON.h
  lldb/source/Symbol/Symbol.cpp
  lldb/test/API/macosx/symbols/Makefile
  lldb/test/API/macosx/symbols/TestSymbolFileJSON.py
  lldb/test/API/macosx/symbols/main.c
  lldb/unittests/Symbol/CMakeLists.txt
  lldb/unittests/Symbol/JSONSymbolTest.cpp

Index: lldb/unittests/Symbol/JSONSymbolTest.cpp
===
--- /dev/null
+++ lldb/unittests/Symbol/JSONSymbolTest.cpp
@@ -0,0 +1,192 @@
+//===-- JSONSymbolTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/Section.h"
+#include "lldb/Symbol/Symbol.h"
+#include "llvm/Testing/Support/Error.h"
+
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace llvm;
+using namespace lldb_private;
+
+static std::string g_error_no_section_list = "no section list provided";
+static std::string g_error_both_value_and_address =
+"symbol cannot contain both a value and an address";
+static std::string g_error_neither_value_or_address =
+"symbol must contain either a value or an address";
+
+TEST(JSONSymbolTest, DeserializeCodeAddress) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32,
+  "address": 4096
+})";
+
+  Expected json = json::parse(text);
+  ASSERT_TRUE(static_cast(json));
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_TRUE(fromJSON(*json, json_symbol, root));
+
+  SectionSP sect_sp(new Section(
+  /*module_sp=*/ModuleSP(),
+  /*obj_file=*/nullptr,
+  /*sect_id=*/1,
+  /*name=*/ConstString(".text"),
+  /*sect_type=*/eSectionTypeCode,
+  /*file_vm_addr=*/0x1000,
+  /*vm_size=*/0x1000,
+  /*file_offset=*/0,
+  /*file_size=*/0,
+  /*log2align=*/5,
+  /*flags=*/0x10203040));
+  SectionList sect_list;
+  sect_list.AddSection(sect_sp);
+
+  Expected symbol = Symbol::FromJSON(json_symbol, §_list);
+  EXPECT_THAT_EXPECTED(symbol, llvm::Succeeded());
+  EXPECT_EQ(symbol->GetName(), ConstString("foo"));
+  EXPECT_EQ(symbol->GetFileAddress(), static_cast(0x1000));
+  EXPECT_EQ(symbol->GetType(), eSymbolTypeCode);
+}
+
+TEST(JSONSymbolTest, DeserializeCodeValue) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32,
+  "value": 4096
+})";
+
+  Expected json = json::parse(text);
+  EXPECT_THAT_EXPECTED(json, llvm::Succeeded());
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_TRUE(fromJSON(*json, json_symbol, root));
+
+  SectionList sect_list;
+
+  Expected symbol = Symbol::FromJSON(json_symbol, §_list);
+  EXPECT_THAT_EXPECTED(symbol, llvm::Succeeded());
+  EXPECT_EQ(symbol->GetName(), ConstString("foo"));
+  EXPECT_EQ(symbol->GetRawValue(), static_cast(0x1000));
+  EXPECT_EQ(symbol->GetType(), eSymbolTypeCode);
+}
+
+TEST(JSONSymbolTest, JSONInvalidValueAndAddress) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32,
+  "value": 4096,
+  "address": 4096
+})";
+
+  Expected json = json::parse(text);
+  EXPECT_THAT_EXPECTED(json, llvm::Succeeded());
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_FALSE(fromJSON(*json, json_symbol, root));
+}
+
+TEST(JSONSymbolTest, JSONInvalidNoValueOrAddress) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32
+})";
+
+  Expected json = json::parse(text);
+  EXPECT_THAT_EXPECTED(json, llvm::Succeeded());
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_FALSE(fromJSON(*json, json_symbol, root));
+}
+
+TEST(JSONSymbolTest, JSONInvalidType) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "bogus",
+  "value": 4096,
+  "size": 32
+})";
+
+  Expected json = json::parse(text);
+  EXPECT_THAT_EXPECTED(json, llvm::Succeeded());
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_FALSE(fromJSON(*json, json_symbol, root));
+}
+
+TEST(JSONSymbolTest, SymbolInvalidNoSectionList) {
+  JSONSymbol json_symbol;
+  json_symbol.value = 0x1;
+
+  Expected symbol = Symbol::FromJSON(json_symbol, nullptr);
+  ASSERT_FALSE(static_cast(symbol));
+  EXPECT_EQ(toString(symbol.takeError()), g_error_no_section_list);
+}
+

[Lldb-commits] [PATCH] D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON

2023-03-08 Thread Greg Clayton via Phabricator via lldb-commits

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

Thanks for the changes! LGTM. Just one missed EXPECT_THAT_EXPECTED, but 
accepted.




Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:144-145
+  Expected symbol = Symbol::FromJSON(json_symbol, nullptr);
+  ASSERT_FALSE(static_cast(symbol));
+  EXPECT_EQ(toString(symbol.takeError()), g_error_no_section_list);
+}

Missed one EXPECT_THAT_EXPECTED. Feel free to fix and submit without approval.


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

https://reviews.llvm.org/D145180

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

[Lldb-commits] [PATCH] D145547: When setting load addresses on darwin kernel kexts, handle case where in-memory load commands are not updated

2023-03-08 Thread Alex Langford via Phabricator via lldb-commits

bulbazord added inline comments.



Comment at: 
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:816-819
+  ObjectFileMachO *ondisk_objfile_macho =
+  llvm::dyn_cast_or_null(
+  m_module_sp ? m_module_sp->GetObjectFile() : nullptr);
+  if (ondisk_objfile_macho) {

I have a suggestion that may make this easier to read. You already know that 
m_module_sp is valid because you have that as a condition to even enter this 
block. So you can do something like
```
ObjectFileMachO *ondisk_objfile_macho = m_module_sp->GetObjectFile();
```

But right below you only act on this if the object is valid, so you could stick 
it in the if condition itself:
```
if (auto *ondisk_objfile_macho = m_module_sp->GetObjectFile()) {
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145547

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

[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

2023-03-08 Thread Greg Clayton via Phabricator via lldb-commits

clayborg added a comment.

There was a fix that was never submitted for Google Stadia for the memory cache 
here:

https://github.com/googlestadia/vsi-lldb/tree/master/patches/llvm-project

Might be worth checking what they did to ensure we have all of the same 
abilities.




Comment at: lldb/source/Target/Memory.cpp:181-183
+// FIXME: We should be able to wrap this into the check near the beginning
+// of this function but we don't check for overlapping ranges so we will
+// additionally check each cache line we read.

Not on the FIXME: We can't really check this near the beginning, because this 
happens for each cache line we as we advance the "curr_cache_line_base_addr" 
right? 

One thing to note about this code is that we might need to read at most 2 cache 
lines for any requests that make it to this code since we check above for "if 
(dst_len > m_L2_cache_line_byte_size)..." and use the L1 cache if that is true. 
So we know that we will read at most 2 cache lines depending on the offset. 
Might be nice to read the 2 cache lines in one memory access below if possible, 
and then make two cache entries with the result, but it will be either one 
cache line read, or two



Comment at: lldb/source/Target/Memory.cpp:196-198
+  // If we didn't fill out the cache line completely and the offset is
+  // bigger than what we have available, we can't do anything further
+  // here.

JDevlieghere wrote:
> Why can't we read from the process? Same question below.
Because if we did a memory request before from a valid 
"curr_cache_line_base_addr", and we got back fewer bytes that requested, then 
the bytes won't be available later right?



Comment at: lldb/source/Target/Memory.cpp:245
+  assert((curr_cache_line_base_addr % cache_line_byte_size) == 0);
+  std::unique_ptr data_buffer_heap_up(
+  new DataBufferHeap(cache_line_byte_size, 0));

Should we just create a DataBufferSP right away here instead of creating a 
unique pointer and releasing it later?



Comment at: lldb/source/Target/Memory.cpp:255-256
 
-  // We need to read from the process
+  if (process_bytes_read < cache_line_byte_size)
+data_buffer_heap_up->SetByteSize(process_bytes_read);
 

If we don't read an entire cache line, should we populate this into the L1 
cache instead? It might make the logic for accessing data in the L2 cache a bit 
simpler?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145624

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

[Lldb-commits] [lldb] 8db8a4e - Clean up conditional, don't set load binaries twice

2023-03-08 Thread Jason Molenda via lldb-commits


Author: Jason Molenda
Date: 2023-03-08T18:02:20-08:00
New Revision: 8db8a4e8eddf91bfaee62f83ff10a51e7125d4e4

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

LOG: Clean up conditional, don't set load binaries twice

Follow Alex Langford's feedback to my patch from
https://reviews.llvm.org/D145547 , and fix a
side issue I noticed while testing this, where
binaries loaded via LC_NOTE metadata were loaded
in the Target twice unnecessarily.

Added: 


Modified: 

lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp 
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index b3b199e1bf71..ea2cb46af009 100644
--- 
a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ 
b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -812,11 +812,10 @@ bool 
DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
   }
 }
 
-if (m_module_sp && m_uuid.IsValid() && m_module_sp->GetUUID() == m_uuid) {
-  ObjectFileMachO *ondisk_objfile_macho =
-  llvm::dyn_cast_or_null(
-  m_module_sp ? m_module_sp->GetObjectFile() : nullptr);
-  if (ondisk_objfile_macho) {
+if (m_module_sp && m_uuid.IsValid() && m_module_sp->GetUUID() == m_uuid &&
+m_module_sp->GetObjectFile()) {
+  if (ObjectFileMachO *ondisk_objfile_macho =
+  llvm::dyn_cast(m_module_sp->GetObjectFile())) {
 if (!IsKernel() && !ondisk_objfile_macho->IsKext()) {
   // We have a non-kext, non-kernel binary.  If we already have this
   // loaded in the Target with load addresses, don't re-load it again.

diff  --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp 
b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index d41279ce864c..c5f04557ae61 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -7032,6 +7032,8 @@ bool 
ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &process) {
   &process, image.filename, image.uuid, image.load_address,
   false /* value_is_offset */, image.currently_executing,
   false /* notify */);
+  if (module_sp)
+continue;
 }
 
 // If we have a slide, we need to find the original binary
@@ -7042,6 +7044,8 @@ bool 
ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &process) {
   &process, image.filename, image.uuid, image.slide,
   true /* value_is_offset */, image.currently_executing,
   false /* notify */);
+  if (module_sp)
+continue;
 }
 
 // Try to find the binary by UUID or filename on the local



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

[Lldb-commits] [lldb] cf3524a - [lldb] Introduce new SymbolFileJSON and ObjectFileJSON

2023-03-08 Thread Jonas Devlieghere via lldb-commits


Author: Jonas Devlieghere
Date: 2023-03-08T20:56:11-08:00
New Revision: cf3524a5746f9498280b3a9180b75575c0065d1a

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

LOG: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON

Introduce a new object and symbol file format with the goal of mapping
addresses to symbol names. I'd like to think of is as an extremely
simple textual symtab. The file format consists of a triple, a UUID and
a list of symbols. JSON is used for the encoding, but that's mostly an
implementation detail. The goal of the format was to be simple and human
readable.

The new file format is motivated by two use cases:

 - Stripped binaries: when a binary is stripped, you lose the ability to
   do thing like setting symbolic breakpoints. You can keep the
   unstripped binary around, but if all you need is the stripped
   symbols then that's a lot of overhead. Instead, we could save the
   stripped symbols to a file and load them in the debugger when
   needed. I want to extend llvm-strip to have a mode where it emits
   this new file format.

 - Interactive crashlogs: with interactive crashlogs, if we don't have
   the binary or the dSYM for a particular module, we currently show an
   unnamed symbol for those frames. This is a regression compared to the
   textual format, that has these frames pre-symbolicated. Given that
   this information is available in the JSON crashlog, we need a way to
   tell LLDB about it. With the new symbol file format, we can easily
   synthesize a symbol file for each of those modules and load them to
   symbolicate those frames.

Here's an example of the file format:

 {
 "triple": "arm64-apple-macosx13.0.0",
 "uuid": "36D0CCE7-8ED2-3CA3-96B0-48C1764DA908",
 "symbols": [
 {
 "name": "main",
 "type": "code",
 "size": 32,
 "address": 4294983568
 },
 {
 "name": "foo",
 "type": "code",
 "size": 8,
 "address": 4294983560
 }
 ]
 }

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

Added: 
lldb/source/Plugins/ObjectFile/JSON/CMakeLists.txt
lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp
lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.h
lldb/source/Plugins/SymbolFile/JSON/CMakeLists.txt
lldb/source/Plugins/SymbolFile/JSON/SymbolFileJSON.cpp
lldb/source/Plugins/SymbolFile/JSON/SymbolFileJSON.h
lldb/test/API/macosx/symbols/Makefile
lldb/test/API/macosx/symbols/TestSymbolFileJSON.py
lldb/test/API/macosx/symbols/main.c
lldb/unittests/Symbol/JSONSymbolTest.cpp

Modified: 
lldb/include/lldb/Symbol/Symbol.h
lldb/source/Plugins/ObjectFile/CMakeLists.txt
lldb/source/Plugins/SymbolFile/CMakeLists.txt
lldb/source/Symbol/Symbol.cpp
lldb/unittests/Symbol/CMakeLists.txt

Removed: 




diff  --git a/lldb/include/lldb/Symbol/Symbol.h 
b/lldb/include/lldb/Symbol/Symbol.h
index 164fafd437dcb..44a2d560010fe 100644
--- a/lldb/include/lldb/Symbol/Symbol.h
+++ b/lldb/include/lldb/Symbol/Symbol.h
@@ -11,12 +11,23 @@
 
 #include "lldb/Core/AddressRange.h"
 #include "lldb/Core/Mangled.h"
+#include "lldb/Core/Section.h"
 #include "lldb/Symbol/SymbolContextScope.h"
 #include "lldb/Utility/UserID.h"
 #include "lldb/lldb-private.h"
+#include "llvm/Support/JSON.h"
 
 namespace lldb_private {
 
+struct JSONSymbol {
+  std::optional address;
+  std::optional value;
+  std::optional size;
+  std::optional id;
+  std::optional type;
+  std::string name;
+};
+
 class Symbol : public SymbolContextScope {
 public:
   // ObjectFile readers can classify their symbol table entries and searches
@@ -39,6 +50,9 @@ class Symbol : public SymbolContextScope {
 
   const Symbol &operator=(const Symbol &rhs);
 
+  static llvm::Expected FromJSON(const JSONSymbol &symbol,
+ SectionList *section_list);
+
   void Clear();
 
   bool Compare(ConstString name, lldb::SymbolType type) const;
@@ -187,7 +201,7 @@ class Symbol : public SymbolContextScope {
 
   bool IsWeak() const { return m_is_weak; }
 
-  void SetIsWeak (bool b) { m_is_weak = b; }
+  void SetIsWeak(bool b) { m_is_weak = b; }
 
   bool GetByteSizeIsValid() const { return m_size_is_valid; }
 
@@ -332,4 +346,16 @@ class Symbol : public SymbolContextScope {
 
 } // namespace lldb_private
 
+namespace llvm {
+namespace json {
+
+bool fromJSON(const llvm::json::Value &value, lldb_private::JSONSymbol &symbol,
+  llvm::json::Path path);
+
+bool fromJSON(const llvm::json::Value &value, lldb::SymbolType &type,
+  llvm::json::Path path);
+
+} // namespace json
+} // namespace llvm
+
 #endif // LLDB_SYMBOL_SYMBOL_H

diff  --git a/lldb/source/Plugins/ObjectFile/CMakeLists.

[Lldb-commits] [PATCH] D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON

2023-03-08 Thread Jonas Devlieghere via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcf3524a5746f: [lldb] Introduce new SymbolFileJSON and 
ObjectFileJSON (authored by JDevlieghere).

Changed prior to commit:
  https://reviews.llvm.org/D145180?vs=503575&id=503611#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145180

Files:
  lldb/include/lldb/Symbol/Symbol.h
  lldb/source/Plugins/ObjectFile/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/JSON/CMakeLists.txt
  lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.cpp
  lldb/source/Plugins/ObjectFile/JSON/ObjectFileJSON.h
  lldb/source/Plugins/SymbolFile/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/JSON/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/JSON/SymbolFileJSON.cpp
  lldb/source/Plugins/SymbolFile/JSON/SymbolFileJSON.h
  lldb/source/Symbol/Symbol.cpp
  lldb/test/API/macosx/symbols/Makefile
  lldb/test/API/macosx/symbols/TestSymbolFileJSON.py
  lldb/test/API/macosx/symbols/main.c
  lldb/unittests/Symbol/CMakeLists.txt
  lldb/unittests/Symbol/JSONSymbolTest.cpp

Index: lldb/unittests/Symbol/JSONSymbolTest.cpp
===
--- /dev/null
+++ lldb/unittests/Symbol/JSONSymbolTest.cpp
@@ -0,0 +1,192 @@
+//===-- JSONSymbolTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Core/Section.h"
+#include "lldb/Symbol/Symbol.h"
+#include "llvm/Testing/Support/Error.h"
+
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace llvm;
+using namespace lldb_private;
+
+static std::string g_error_no_section_list = "no section list provided";
+static std::string g_error_both_value_and_address =
+"symbol cannot contain both a value and an address";
+static std::string g_error_neither_value_or_address =
+"symbol must contain either a value or an address";
+
+TEST(JSONSymbolTest, DeserializeCodeAddress) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32,
+  "address": 4096
+})";
+
+  Expected json = json::parse(text);
+  ASSERT_TRUE(static_cast(json));
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_TRUE(fromJSON(*json, json_symbol, root));
+
+  SectionSP sect_sp(new Section(
+  /*module_sp=*/ModuleSP(),
+  /*obj_file=*/nullptr,
+  /*sect_id=*/1,
+  /*name=*/ConstString(".text"),
+  /*sect_type=*/eSectionTypeCode,
+  /*file_vm_addr=*/0x1000,
+  /*vm_size=*/0x1000,
+  /*file_offset=*/0,
+  /*file_size=*/0,
+  /*log2align=*/5,
+  /*flags=*/0x10203040));
+  SectionList sect_list;
+  sect_list.AddSection(sect_sp);
+
+  Expected symbol = Symbol::FromJSON(json_symbol, §_list);
+  EXPECT_THAT_EXPECTED(symbol, llvm::Succeeded());
+  EXPECT_EQ(symbol->GetName(), ConstString("foo"));
+  EXPECT_EQ(symbol->GetFileAddress(), static_cast(0x1000));
+  EXPECT_EQ(symbol->GetType(), eSymbolTypeCode);
+}
+
+TEST(JSONSymbolTest, DeserializeCodeValue) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32,
+  "value": 4096
+})";
+
+  Expected json = json::parse(text);
+  EXPECT_THAT_EXPECTED(json, llvm::Succeeded());
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_TRUE(fromJSON(*json, json_symbol, root));
+
+  SectionList sect_list;
+
+  Expected symbol = Symbol::FromJSON(json_symbol, §_list);
+  EXPECT_THAT_EXPECTED(symbol, llvm::Succeeded());
+  EXPECT_EQ(symbol->GetName(), ConstString("foo"));
+  EXPECT_EQ(symbol->GetRawValue(), static_cast(0x1000));
+  EXPECT_EQ(symbol->GetType(), eSymbolTypeCode);
+}
+
+TEST(JSONSymbolTest, JSONInvalidValueAndAddress) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32,
+  "value": 4096,
+  "address": 4096
+})";
+
+  Expected json = json::parse(text);
+  EXPECT_THAT_EXPECTED(json, llvm::Succeeded());
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_FALSE(fromJSON(*json, json_symbol, root));
+}
+
+TEST(JSONSymbolTest, JSONInvalidNoValueOrAddress) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "code",
+  "size": 32
+})";
+
+  Expected json = json::parse(text);
+  EXPECT_THAT_EXPECTED(json, llvm::Succeeded());
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_FALSE(fromJSON(*json, json_symbol, root));
+}
+
+TEST(JSONSymbolTest, JSONInvalidType) {
+  std::string text = R"(
+{
+  "name": "foo",
+  "type": "bogus",
+  "value": 4096,
+  "size": 32
+})";
+
+  Expected json = json::parse(text);
+  EXPECT_THAT_EXPECTED(json, llvm::Succeeded());
+
+  json::Path::Root root;
+  JSONSymbol json_symbol;
+  ASSERT_FALSE(fromJSON(*json, json_symbol, root));
+}
+
+TEST(JSONSymbolT

[Lldb-commits] [lldb] 15653dc - [lldb] Temporarily disable TestSymbolFileJSON on non Darwin platforms

2023-03-08 Thread Jonas Devlieghere via lldb-commits


Author: Jonas Devlieghere
Date: 2023-03-08T21:32:45-08:00
New Revision: 15653dcb62dff58ec5f653d0a2884b1299f971d6

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

LOG: [lldb] Temporarily disable TestSymbolFileJSON on non Darwin platforms

The new test is triggering a crash in LLDB on the Windows and Linux
bots. Temporarily disable the test while I investigate.

Added: 


Modified: 
lldb/test/API/macosx/symbols/TestSymbolFileJSON.py

Removed: 




diff  --git a/lldb/test/API/macosx/symbols/TestSymbolFileJSON.py 
b/lldb/test/API/macosx/symbols/TestSymbolFileJSON.py
index 31814ba5c85e2..2e6d05ac6306e 100644
--- a/lldb/test/API/macosx/symbols/TestSymbolFileJSON.py
+++ b/lldb/test/API/macosx/symbols/TestSymbolFileJSON.py
@@ -12,6 +12,7 @@ def setUp(self):
 self.source = 'main.c'
 
 @no_debug_info_test
+@skipUnlessDarwin
 def test_symbol_file_json_address(self):
 """Test that 'target symbols add' can load the symbols from a JSON 
file using file addresses."""
 



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

[Lldb-commits] [lldb] 12f709d - lldb] Re-enable TestSymbolFileJSON on non Darwin platforms

2023-03-08 Thread Jonas Devlieghere via lldb-commits


Author: Jonas Devlieghere
Date: 2023-03-08T22:12:34-08:00
New Revision: 12f709db0d0e7e8145175e12965c4e3db592143a

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

LOG: lldb] Re-enable TestSymbolFileJSON on non Darwin platforms

Fix the crash in SymbolVendorELF and re-enable the test.

Added: 


Modified: 
lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
lldb/test/API/macosx/symbols/TestSymbolFileJSON.py

Removed: 




diff  --git a/lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp 
b/lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
index 61e06cdfa02df..3595966b42e46 100644
--- a/lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
+++ b/lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
@@ -111,6 +111,9 @@ SymbolVendorELF::CreateInstance(const lldb::ModuleSP 
&module_sp,
   SectionList *module_section_list = module_sp->GetSectionList();
   SectionList *objfile_section_list = dsym_objfile_sp->GetSectionList();
 
+  if (!module_section_list || !!objfile_section_list)
+return nullptr;
+
   static const SectionType g_sections[] = {
   eSectionTypeDWARFDebugAbbrev, eSectionTypeDWARFDebugAddr,
   eSectionTypeDWARFDebugAranges,eSectionTypeDWARFDebugCuIndex,

diff  --git a/lldb/test/API/macosx/symbols/TestSymbolFileJSON.py 
b/lldb/test/API/macosx/symbols/TestSymbolFileJSON.py
index 2e6d05ac6306e..31814ba5c85e2 100644
--- a/lldb/test/API/macosx/symbols/TestSymbolFileJSON.py
+++ b/lldb/test/API/macosx/symbols/TestSymbolFileJSON.py
@@ -12,7 +12,6 @@ def setUp(self):
 self.source = 'main.c'
 
 @no_debug_info_test
-@skipUnlessDarwin
 def test_symbol_file_json_address(self):
 """Test that 'target symbols add' can load the symbols from a JSON 
file using file addresses."""
 



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

[Lldb-commits] [lldb] 73058e3 - [lldb] Fix typo in SymbolVendorELF

2023-03-08 Thread Jonas Devlieghere via lldb-commits


Author: Jonas Devlieghere
Date: 2023-03-08T22:15:36-08:00
New Revision: 73058e330134d275070d49dba80e5fda50ec015b

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

LOG: [lldb] Fix typo in SymbolVendorELF

Added: 


Modified: 
lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp

Removed: 




diff  --git a/lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp 
b/lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
index 3595966b42e4..55a663bb1b96 100644
--- a/lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
+++ b/lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
@@ -111,7 +111,7 @@ SymbolVendorELF::CreateInstance(const lldb::ModuleSP 
&module_sp,
   SectionList *module_section_list = module_sp->GetSectionList();
   SectionList *objfile_section_list = dsym_objfile_sp->GetSectionList();
 
-  if (!module_section_list || !!objfile_section_list)
+  if (!module_section_list || !objfile_section_list)
 return nullptr;
 
   static const SectionType g_sections[] = {



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

[Lldb-commits] [lldb] f009241 - [lldb] Fix -Wdangling-else warning in CommunicationTest

2023-03-08 Thread Jonas Devlieghere via lldb-commits


Author: Jonas Devlieghere
Date: 2023-03-08T22:21:00-08:00
New Revision: f009241ae595ae6db5a859e498f72fd7e5dc6ce8

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

LOG: [lldb] Fix -Wdangling-else warning in CommunicationTest

Fixed warning: suggest explicit braces to avoid ambiguous ‘else’
[-Wdangling-else]

Added: 


Modified: 
lldb/unittests/Core/CommunicationTest.cpp

Removed: 




diff  --git a/lldb/unittests/Core/CommunicationTest.cpp 
b/lldb/unittests/Core/CommunicationTest.cpp
index 19ca4cb3d2493..df9ff089a0d73 100644
--- a/lldb/unittests/Core/CommunicationTest.cpp
+++ b/lldb/unittests/Core/CommunicationTest.cpp
@@ -86,8 +86,9 @@ static void CommunicationReadTest(bool use_read_thread) {
 
   // Test using Communication that is disconnected.
   ASSERT_EQ(comm.Disconnect(), lldb::eConnectionStatusSuccess);
-  if (use_read_thread)
+  if (use_read_thread) {
 ASSERT_TRUE(comm.StartReadThread());
+  }
   error.Clear();
   EXPECT_EQ(
   comm.Read(buf, sizeof(buf), std::chrono::seconds(5), status, &error), 
0U);
@@ -97,8 +98,9 @@ static void CommunicationReadTest(bool use_read_thread) {
 
   // Test using Communication without a connection.
   comm.SetConnection(nullptr);
-  if (use_read_thread)
+  if (use_read_thread) {
 ASSERT_TRUE(comm.StartReadThread());
+  }
   error.Clear();
   EXPECT_EQ(
   comm.Read(buf, sizeof(buf), std::chrono::seconds(5), status, &error), 
0U);



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

83 matches

Mail list logo