[Lldb-commits] [PATCH] D52403: [LLDB] - Support the single file split DWARF.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I believe you should be able to use `lldb-test` to write this kind of a test. I 
am thinking of a sequence like:

  yaml2obj %p/Inputs/test.yaml > %t/test.yaml
  yaml2obj %p/Inputs/test.o.yaml > %t/test.o.yaml
  lldb-test breakpoint %t/test.yaml %s | FileCheck %s
  b main
  CHECK-LABEL: b main
  CHECK: Address: {{.*}}main + 13 at test.cpp:2:3
  
  b foo
  CHECK-LABEL: b foo
  CHECK: Address: {{.*}}foo() + 4 at test.cpp:6:1

You can look at existing tests in lit/Breakpoints for details.


https://reviews.llvm.org/D52403



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

[Lldb-commits] [PATCH] D52152: Add NativeProcessProtocol unit tests

[Pavel Labath via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB342875: Add NativeProcessProtocol unit tests (authored by 
labath, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52152?vs=165688&id=166674#toc

Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52152

Files:
  unittests/Host/CMakeLists.txt
  unittests/Host/NativeProcessProtocolTest.cpp

Index: unittests/Host/CMakeLists.txt
===
--- unittests/Host/CMakeLists.txt
+++ unittests/Host/CMakeLists.txt
@@ -3,6 +3,7 @@
   HostInfoTest.cpp
   HostTest.cpp
   MainLoopTest.cpp
+  NativeProcessProtocolTest.cpp
   SocketAddressTest.cpp
   SocketTest.cpp
   SymbolsTest.cpp
@@ -22,4 +23,5 @@
 lldbCore
 lldbHost
 lldbUtilityHelpers
+LLVMTestingSupport
   )
Index: unittests/Host/NativeProcessProtocolTest.cpp
===
--- unittests/Host/NativeProcessProtocolTest.cpp
+++ unittests/Host/NativeProcessProtocolTest.cpp
@@ -0,0 +1,154 @@
+//===-- NativeProcessProtocolTest.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Host/common/NativeProcessProtocol.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+
+using namespace lldb_private;
+using namespace lldb;
+using namespace testing;
+
+namespace {
+class MockDelegate : public NativeProcessProtocol::NativeDelegate {
+public:
+  MOCK_METHOD1(InitializeDelegate, void(NativeProcessProtocol *Process));
+  MOCK_METHOD2(ProcessStateChanged,
+   void(NativeProcessProtocol *Process, StateType State));
+  MOCK_METHOD1(DidExec, void(NativeProcessProtocol *Process));
+};
+
+class MockProcess : public NativeProcessProtocol {
+public:
+  MockProcess(NativeDelegate &Delegate, const ArchSpec &Arch,
+  lldb::pid_t Pid = 1)
+  : NativeProcessProtocol(Pid, -1, Delegate), Arch(Arch) {}
+
+  MOCK_METHOD1(Resume, Status(const ResumeActionList &ResumeActions));
+  MOCK_METHOD0(Halt, Status());
+  MOCK_METHOD0(Detach, Status());
+  MOCK_METHOD1(Signal, Status(int Signo));
+  MOCK_METHOD0(Kill, Status());
+  MOCK_METHOD3(AllocateMemory,
+   Status(size_t Size, uint32_t Permissions, addr_t &Addr));
+  MOCK_METHOD1(DeallocateMemory, Status(addr_t Addr));
+  MOCK_METHOD0(GetSharedLibraryInfoAddress, addr_t());
+  MOCK_METHOD0(UpdateThreads, size_t());
+  MOCK_CONST_METHOD0(GetAuxvData,
+ llvm::ErrorOr>());
+  MOCK_METHOD2(GetLoadedModuleFileSpec,
+   Status(const char *ModulePath, FileSpec &Spec));
+  MOCK_METHOD2(GetFileLoadAddress,
+   Status(const llvm::StringRef &FileName, addr_t &Addr));
+
+  const ArchSpec &GetArchitecture() const override { return Arch; }
+  Status SetBreakpoint(lldb::addr_t Addr, uint32_t Size,
+   bool Hardware) override {
+if (Hardware)
+  return SetHardwareBreakpoint(Addr, Size);
+else
+  return SetSoftwareBreakpoint(Addr, Size);
+  }
+
+  // Redirect base class Read/Write Memory methods to functions whose signatures
+  // are more mock-friendly.
+  Status ReadMemory(addr_t Addr, void *Buf, size_t Size,
+size_t &BytesRead) override;
+  Status WriteMemory(addr_t Addr, const void *Buf, size_t Size,
+ size_t &BytesWritten) override;
+
+  MOCK_METHOD2(ReadMemory,
+   llvm::Expected>(addr_t Addr, size_t Size));
+  MOCK_METHOD2(WriteMemory,
+   llvm::Expected(addr_t Addr,
+  llvm::ArrayRef Data));
+
+  using NativeProcessProtocol::GetSoftwareBreakpointTrapOpcode;
+
+private:
+  ArchSpec Arch;
+};
+} // namespace
+
+Status MockProcess::ReadMemory(addr_t Addr, void *Buf, size_t Size,
+   size_t &BytesRead) {
+  auto ExpectedMemory = ReadMemory(Addr, Size);
+  if (!ExpectedMemory) {
+BytesRead = 0;
+return Status(ExpectedMemory.takeError());
+  }
+  BytesRead = ExpectedMemory->size();
+  assert(BytesRead <= Size);
+  std::memcpy(Buf, ExpectedMemory->data(), BytesRead);
+  return Status();
+}
+
+Status MockProcess::WriteMemory(addr_t Addr, const void *Buf, size_t Size,
+size_t &BytesWritten) {
+  auto ExpectedBytes = WriteMemory(
+  Addr, llvm::makeArrayRef(static_cast(Buf), Size));
+  if (!ExpectedBytes) {
+BytesWritten = 0;
+return Status(ExpectedBytes.takeError());
+  }
+  BytesWritten = *ExpectedBytes;
+  return Status();
+}
+
+TEST(NativeProcessProtocolTest, SetBreakpoint) {
+  NiceMock DummyDelegate;
+  MockProcess Process(DummyDelegate, ArchSpec("x86_64-pc

[Lldb-commits] [PATCH] D48393: Make DWARFParsing more thread-safe

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D48393#1243195, @JDevlieghere wrote:

> In https://reviews.llvm.org/D48393#1139327, @labath wrote:
>
> > The only sane algorithm I can come up right now is to make the list of 
> > parsed dies local to each thread/parsing entity (e.g. via a "visited" 
> > list), and only update the global map once the parsing has completed 
> > (successfully or not). This can potentially duplicate some effort where one 
> > thread parses a type only to find out that it has already been parsed, but 
> > hopefully that is not going to be the common case. The alternative is some 
> > complicated resource cycle detection scheme.
>
>
> I gave this a shot in https://reviews.llvm.org/D52406 but I'm afraid it's too 
> simple to be correct. Pavel, could you give it a look and let me know whether 
> that was what you had in mind?


Yeah, I don't think this will make things fully correct. This avoids the 
problem when two threads are misinterpreting the DIE_IS_BEING_PARSED markers 
left by the other thread. However, it is still not correct when two threads are 
parsing the same DIE concurrently. Imagine the following sequence of events:

- thread A starts parsing DIE 1, checks that it still hasn't been parsed, 
inserts the DIE_IS_BEING_PARSED into the local map.
- thread B does the same
- thread A finishes parsing DIE 1. constructs a clang AST (C1) for it sets it 
as the map value
- thread B does the same overwrites the value with C2
- thread A carries on using C1
- thread B carries on using C2

This sounds like something that is not supposed to happen, though I don't 
really know what the consequences of that are. Also I am not entirely convinced 
that the mere construction of C1 and C2 does not touch some global structures 
(which would not be protected by a lock).

I agree with Greg that it would be best to restrict things such that there is 
only one instance of parsing going on at any given time for a single module. I 
think this was pretty much the state we reached when this thread fizzled out 
the last time (there are some extra emails that are not showing in phabricator 
history, the interesting ones start around here: 
http://lists.llvm.org/pipermail/lldb-commits/Week-of-Mon-20180618/041937.html). 
I think the main part that needed to be resolved is whether we need to go 
anything special when resolving debug info references *between* modules (e.g. 
to prevent A/B deadlocks).


https://reviews.llvm.org/D48393



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

[Lldb-commits] [PATCH] D52139: [lldb-mi] Fix hanging of target-select-so-path.test

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D52139#1243170, @teemperor wrote:

> Anyway, the actual issue is related Python 3: Arch Linux (and probably
>  some other distributions that "already" upgraded to Python 3 as
>  default) will launch Python 3 when the test script calls `python ...`.
>  And for some reason in Python 3.7, we will not inherit our FD from our
>  pipe to the subprocess, which causes this strange behavior when write
>  to the unrelated FD number in ConnectToRemote. When I explicitly call
>  Python 2.7 from the test, everything runs as expected.
>
> The python docs don't see to mention this change (and it seems like a
>  bug to me), so I'm open for ideas how to fix this properly. In any
>  case this problem doesn't block this review.


It's more like python 2 had a bug where it always inherited the file 
descriptor, and then python3 fixed that and introduced a special popen argument 
to control the inheriting behavior.

Back when we were designing this test, I demonstrated the necessary 
incantations for this to work on both python2 and 3 
https://reviews.llvm.org/D49739?id=157310#inline-438290. It seems that did not 
make it into the final version..


Repository:
  rL LLVM

https://reviews.llvm.org/D52139



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

[Lldb-commits] [PATCH] D52461: [PDB] Introduce `PDBNameParser`

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think you should look at CPlusPlusLanguage::MethodName. It already contains a 
parser (in fact, two of them) of c++ names, and I think it should be easy to 
extend it to do what you want.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52461



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

[Lldb-commits] [PATCH] D52498: [lldb-mi] Fix bugs in target-select-so-path.test

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

LGTM. (BTW, python 3.1's EOL was 6 years ago)


https://reviews.llvm.org/D52498



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

[Lldb-commits] [PATCH] D52516: [lldbinline] Set directory attribute on test-specific classes

[Pavel Labath via Phabricator via lldb-commits]

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

Ooh, nice catch. This must have been fun to debug.


https://reviews.llvm.org/D52516



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

[Lldb-commits] [PATCH] D52532: Pull GetSoftwareBreakpointPCOffset into base class

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: krytarowski, zturner.

This function encodes the knowledge of whether the PC points to the
breakpoint instruction of the one following it after the breakpoint is
"hit". This behavior mainly(*) depends on the architecture and not on the
OS, so it makes sense for it to be implemented in the base class, where
it can be shared between different implementations (Linux and NetBSD
atm).

(*) It is possible for an OS to expose a different API, perhaps by doing
some fixups in the kernel. In this case, the implementation can override
this function to implement custom behavior.


https://reviews.llvm.org/D52532

Files:
  include/lldb/Host/common/NativeProcessProtocol.h
  source/Host/common/NativeProcessProtocol.cpp
  source/Plugins/Process/Linux/NativeProcessLinux.cpp
  source/Plugins/Process/Linux/NativeProcessLinux.h
  source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp

Index: source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
===
--- source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
+++ source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
@@ -322,35 +322,16 @@
   return error;
 }
 
-Status NativeProcessNetBSD::GetSoftwareBreakpointPCOffset(
-uint32_t &actual_opcode_size) {
-  // FIXME put this behind a breakpoint protocol class that can be
-  // set per architecture.  Need ARM, MIPS support here.
-  static const uint8_t g_i386_opcode[] = {0xCC};
-  switch (m_arch.GetMachine()) {
-  case llvm::Triple::x86_64:
-actual_opcode_size = static_cast(sizeof(g_i386_opcode));
-return Status();
-  default:
-assert(false && "CPU type not supported!");
-return Status("CPU type not supported");
-  }
-}
-
 Status
 NativeProcessNetBSD::FixupBreakpointPCAsNeeded(NativeThreadNetBSD &thread) {
   Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_BREAKPOINTS));
   Status error;
   // Find out the size of a breakpoint (might depend on where we are in the
   // code).
   NativeRegisterContext& context = thread.GetRegisterContext();
-  uint32_t breakpoint_size = 0;
-  error = GetSoftwareBreakpointPCOffset(breakpoint_size);
-  if (error.Fail()) {
-LLDB_LOG(log, "GetBreakpointSize() failed: {0}", error);
-return error;
-  } else
-LLDB_LOG(log, "breakpoint size: {0}", breakpoint_size);
+  uint32_t breakpoint_size = GetSoftwareBreakpointPCOffset();
+  LLDB_LOG(log, "breakpoint size: {0}", breakpoint_size);
+
   // First try probing for a breakpoint at a software breakpoint location: PC -
   // breakpoint size.
   const lldb::addr_t initial_pc_addr =
Index: source/Plugins/Process/Linux/NativeProcessLinux.h
===
--- source/Plugins/Process/Linux/NativeProcessLinux.h
+++ source/Plugins/Process/Linux/NativeProcessLinux.h
@@ -182,8 +182,6 @@
 
   NativeThreadLinux &AddThread(lldb::tid_t thread_id);
 
-  Status GetSoftwareBreakpointPCOffset(uint32_t &actual_opcode_size);
-
   Status FixupBreakpointPCAsNeeded(NativeThreadLinux &thread);
 
   /// Writes a siginfo_t structure corresponding to the given thread ID to the
Index: source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -1502,40 +1502,6 @@
   return m_threads.size();
 }
 
-Status NativeProcessLinux::GetSoftwareBreakpointPCOffset(
-uint32_t &actual_opcode_size) {
-  // FIXME put this behind a breakpoint protocol class that can be
-  // set per architecture.  Need ARM, MIPS support here.
-  static const uint8_t g_i386_opcode[] = {0xCC};
-  static const uint8_t g_s390x_opcode[] = {0x00, 0x01};
-
-  switch (m_arch.GetMachine()) {
-  case llvm::Triple::x86:
-  case llvm::Triple::x86_64:
-actual_opcode_size = static_cast(sizeof(g_i386_opcode));
-return Status();
-
-  case llvm::Triple::systemz:
-actual_opcode_size = static_cast(sizeof(g_s390x_opcode));
-return Status();
-
-  case llvm::Triple::arm:
-  case llvm::Triple::aarch64:
-  case llvm::Triple::mips64:
-  case llvm::Triple::mips64el:
-  case llvm::Triple::mips:
-  case llvm::Triple::mipsel:
-  case llvm::Triple::ppc64le:
-// On these architectures the PC don't get updated for breakpoint hits
-actual_opcode_size = 0;
-return Status();
-
-  default:
-assert(false && "CPU type not supported!");
-return Status("CPU type not supported");
-  }
-}
-
 Status NativeProcessLinux::SetBreakpoint(lldb::addr_t addr, uint32_t size,
  bool hardware) {
   if (hardware)
@@ -1763,13 +1729,8 @@
   // code).
   NativeRegisterContext &context = thread.GetRegisterContext();
 
-  uint32_t breakpoint_size = 0;
-  error = GetSoftwareBreakpointPCOffset(breakpoint_size);
-  if (error.Fail()) {
-LLDB_LOG(log, "GetBreakpointSize() failed: {0}", error);
-return error;
-  } else

[Lldb-commits] [PATCH] D52532: Pull GetSoftwareBreakpointPCOffset into base class

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D52532#1246173, @krytarowski wrote:

> I was wondering whether we want to normalize this inside the kernel and 
> always advance the Program Counter.. but it's easier to manage it in userland.


I am generally in favour of keeping the kernel simple, particularly when ptrace 
is concerned. However, there is one difference in behaviour that annoys me. 
Right now, if an application itself inserts a trap into it's source code, on 
intel it will be easy to resume it from debugger simply by continuing. OTOH, on 
arm, the user will have to manually update the PC first. So I am not sure which 
is better here...


https://reviews.llvm.org/D52532



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

[Lldb-commits] [PATCH] D52532: Pull GetSoftwareBreakpointPCOffset into base class

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL343409: Pull GetSoftwareBreakpointPCOffset into base class 
(authored by labath, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D52532

Files:
  lldb/trunk/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/trunk/source/Host/common/NativeProcessProtocol.cpp
  lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp

Index: lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
===
--- lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
+++ lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
@@ -322,35 +322,16 @@
   return error;
 }
 
-Status NativeProcessNetBSD::GetSoftwareBreakpointPCOffset(
-uint32_t &actual_opcode_size) {
-  // FIXME put this behind a breakpoint protocol class that can be
-  // set per architecture.  Need ARM, MIPS support here.
-  static const uint8_t g_i386_opcode[] = {0xCC};
-  switch (m_arch.GetMachine()) {
-  case llvm::Triple::x86_64:
-actual_opcode_size = static_cast(sizeof(g_i386_opcode));
-return Status();
-  default:
-assert(false && "CPU type not supported!");
-return Status("CPU type not supported");
-  }
-}
-
 Status
 NativeProcessNetBSD::FixupBreakpointPCAsNeeded(NativeThreadNetBSD &thread) {
   Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_BREAKPOINTS));
   Status error;
   // Find out the size of a breakpoint (might depend on where we are in the
   // code).
   NativeRegisterContext& context = thread.GetRegisterContext();
-  uint32_t breakpoint_size = 0;
-  error = GetSoftwareBreakpointPCOffset(breakpoint_size);
-  if (error.Fail()) {
-LLDB_LOG(log, "GetBreakpointSize() failed: {0}", error);
-return error;
-  } else
-LLDB_LOG(log, "breakpoint size: {0}", breakpoint_size);
+  uint32_t breakpoint_size = GetSoftwareBreakpointPCOffset();
+  LLDB_LOG(log, "breakpoint size: {0}", breakpoint_size);
+
   // First try probing for a breakpoint at a software breakpoint location: PC -
   // breakpoint size.
   const lldb::addr_t initial_pc_addr =
Index: lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -1502,40 +1502,6 @@
   return m_threads.size();
 }
 
-Status NativeProcessLinux::GetSoftwareBreakpointPCOffset(
-uint32_t &actual_opcode_size) {
-  // FIXME put this behind a breakpoint protocol class that can be
-  // set per architecture.  Need ARM, MIPS support here.
-  static const uint8_t g_i386_opcode[] = {0xCC};
-  static const uint8_t g_s390x_opcode[] = {0x00, 0x01};
-
-  switch (m_arch.GetMachine()) {
-  case llvm::Triple::x86:
-  case llvm::Triple::x86_64:
-actual_opcode_size = static_cast(sizeof(g_i386_opcode));
-return Status();
-
-  case llvm::Triple::systemz:
-actual_opcode_size = static_cast(sizeof(g_s390x_opcode));
-return Status();
-
-  case llvm::Triple::arm:
-  case llvm::Triple::aarch64:
-  case llvm::Triple::mips64:
-  case llvm::Triple::mips64el:
-  case llvm::Triple::mips:
-  case llvm::Triple::mipsel:
-  case llvm::Triple::ppc64le:
-// On these architectures the PC don't get updated for breakpoint hits
-actual_opcode_size = 0;
-return Status();
-
-  default:
-assert(false && "CPU type not supported!");
-return Status("CPU type not supported");
-  }
-}
-
 Status NativeProcessLinux::SetBreakpoint(lldb::addr_t addr, uint32_t size,
  bool hardware) {
   if (hardware)
@@ -1763,13 +1729,8 @@
   // code).
   NativeRegisterContext &context = thread.GetRegisterContext();
 
-  uint32_t breakpoint_size = 0;
-  error = GetSoftwareBreakpointPCOffset(breakpoint_size);
-  if (error.Fail()) {
-LLDB_LOG(log, "GetBreakpointSize() failed: {0}", error);
-return error;
-  } else
-LLDB_LOG(log, "breakpoint size: {0}", breakpoint_size);
+  uint32_t breakpoint_size = GetSoftwareBreakpointPCOffset();
+  LLDB_LOG(log, "breakpoint size: {0}", breakpoint_size);
 
   // First try probing for a breakpoint at a software breakpoint location: PC -
   // breakpoint size.
Index: lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.h
===
--- lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.h
+++ lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.h
@@ -182,8 +182,6 @@
 
   NativeThreadLinux &AddThread(lldb::tid_t thread_id);
 
-  Status GetSoftwareBreakpointPCOffset(uint32_t &actual_opcode_size);
-
   Status FixupBreakpointPCAsNeeded(NativeThreadLinux &thread);
 
   /// Write

[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.
Herald added a subscriber: arphaman.



Comment at: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:179
+  if (!m_die_array.empty()) {
+lldbassert(!m_first_die || m_first_die == m_die_array.front());
+m_first_die = m_die_array.front();

xbolva00 wrote:
> xbolva00 wrote:
> > @jankratochvil is this correct assert? Our downstream lldb based on lldb 7 
> > with our custom targets hits this assert.  If we remove it, we see no 
> > obvious breakages.
> cc @labath 
I agree with Jan. The two dies should be the same. The fact that they aren't 
probably means there is a bug somewhere. It would be good to know how the two 
dies differ and what is the input dwarf they are generated from.


Repository:
  rL LLVM

https://reviews.llvm.org/D46810



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

[Lldb-commits] [PATCH] D52461: [PDB] Introduce `PDBNameParser`

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D52461#1249259, @aleksandr.urakov wrote:

> It seems that `CPlusPlusLanguage::MethodName` is backed by LLDB 
> `CPlusPlusNameParser`, which can't parse demangled names...


What makes you say that? If you look at the MethodName unit tests 
(`unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp`), you will see that 
they all operate on demangled names -- there isn't a mangled name in the whole 
file.

Using RichManglingContext would work too, though I'm not sure if that will buy 
you anything, as that is just a wrapper around this parser.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52461



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

[Lldb-commits] [PATCH] D52719: Pull FixupBreakpointPCAsNeeded into base class

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added a reviewer: krytarowski.

This function existed (with identical code) in both NativeProcessLinux
and NativeProcessNetBSD, and it is likely that it would be useful to any
future implementation of NativeProcessProtocol.

Therefore I move it to the base class.


https://reviews.llvm.org/D52719

Files:
  include/lldb/Host/common/NativeProcessProtocol.h
  source/Host/common/NativeProcessProtocol.cpp
  source/Plugins/Process/Linux/NativeProcessLinux.cpp
  source/Plugins/Process/Linux/NativeProcessLinux.h
  source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
  source/Plugins/Process/NetBSD/NativeProcessNetBSD.h

Index: source/Plugins/Process/NetBSD/NativeProcessNetBSD.h
===
--- source/Plugins/Process/NetBSD/NativeProcessNetBSD.h
+++ source/Plugins/Process/NetBSD/NativeProcessNetBSD.h
@@ -112,7 +112,6 @@
   void MonitorSIGTRAP(lldb::pid_t pid);
   void MonitorSignal(lldb::pid_t pid, int signal);
 
-  Status FixupBreakpointPCAsNeeded(NativeThreadNetBSD &thread);
   Status PopulateMemoryRegionCache();
   void SigchldHandler();
 
Index: source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
===
--- source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
+++ source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
@@ -322,81 +322,6 @@
   return error;
 }
 
-Status
-NativeProcessNetBSD::FixupBreakpointPCAsNeeded(NativeThreadNetBSD &thread) {
-  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_BREAKPOINTS));
-  Status error;
-  // Find out the size of a breakpoint (might depend on where we are in the
-  // code).
-  NativeRegisterContext& context = thread.GetRegisterContext();
-  uint32_t breakpoint_size = GetSoftwareBreakpointPCOffset();
-  LLDB_LOG(log, "breakpoint size: {0}", breakpoint_size);
-
-  // First try probing for a breakpoint at a software breakpoint location: PC -
-  // breakpoint size.
-  const lldb::addr_t initial_pc_addr =
-  context.GetPCfromBreakpointLocation();
-  lldb::addr_t breakpoint_addr = initial_pc_addr;
-  if (breakpoint_size > 0) {
-// Do not allow breakpoint probe to wrap around.
-if (breakpoint_addr >= breakpoint_size)
-  breakpoint_addr -= breakpoint_size;
-  }
-  // Check if we stopped because of a breakpoint.
-  NativeBreakpointSP breakpoint_sp;
-  error = m_breakpoint_list.GetBreakpoint(breakpoint_addr, breakpoint_sp);
-  if (!error.Success() || !breakpoint_sp) {
-// We didn't find one at a software probe location.  Nothing to do.
-LLDB_LOG(log,
- "pid {0} no lldb breakpoint found at current pc with "
- "adjustment: {1}",
- GetID(), breakpoint_addr);
-return Status();
-  }
-  // If the breakpoint is not a software breakpoint, nothing to do.
-  if (!breakpoint_sp->IsSoftwareBreakpoint()) {
-LLDB_LOG(
-log,
-"pid {0} breakpoint found at {1:x}, not software, nothing to adjust",
-GetID(), breakpoint_addr);
-return Status();
-  }
-  //
-  // We have a software breakpoint and need to adjust the PC.
-  //
-  // Sanity check.
-  if (breakpoint_size == 0) {
-// Nothing to do!  How did we get here?
-LLDB_LOG(log,
- "pid {0} breakpoint found at {1:x}, it is software, but the "
- "size is zero, nothing to do (unexpected)",
- GetID(), breakpoint_addr);
-return Status();
-  }
-  //
-  // We have a software breakpoint and need to adjust the PC.
-  //
-  // Sanity check.
-  if (breakpoint_size == 0) {
-// Nothing to do!  How did we get here?
-LLDB_LOG(log,
- "pid {0} breakpoint found at {1:x}, it is software, but the "
- "size is zero, nothing to do (unexpected)",
- GetID(), breakpoint_addr);
-return Status();
-  }
-  // Change the program counter.
-  LLDB_LOG(log, "pid {0} tid {1}: changing PC from {2:x} to {3:x}", GetID(),
-   thread.GetID(), initial_pc_addr, breakpoint_addr);
-  error = context.SetPC(breakpoint_addr);
-  if (error.Fail()) {
-LLDB_LOG(log, "pid {0} tid {1}: failed to set PC: {2}", GetID(),
- thread.GetID(), error);
-return error;
-  }
-  return error;
-}
-
 Status NativeProcessNetBSD::Resume(const ResumeActionList &resume_actions) {
   Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PROCESS));
   LLDB_LOG(log, "pid {0}", GetID());
Index: source/Plugins/Process/Linux/NativeProcessLinux.h
===
--- source/Plugins/Process/Linux/NativeProcessLinux.h
+++ source/Plugins/Process/Linux/NativeProcessLinux.h
@@ -182,8 +182,6 @@
 
   NativeThreadLinux &AddThread(lldb::tid_t thread_id);
 
-  Status FixupBreakpointPCAsNeeded(NativeThreadLinux &thread);
-
   /// Writes a siginfo_t structure corresponding to the given thread ID to the
   /// memory region pointed to by @p siginfo.
   Status GetSignalInfo(lldb::tid_t tid, void *sigin

[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lit/Expr/TestIRMemoryMapWindows.test:1-12
+# REQUIRES: windows
+
+# RUN: clang-cl /Zi %p/Inputs/call-function.cpp -o %t
+
+# RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-basic
+# RUN: lldb-test ir-memory-map -host-only %t %S/Inputs/ir-memory-map-basic
+

The only difference in this test is the command line used to compile the 
inferior right? That sounds like something that we will run into for a lot of 
lit test, so I think it's important to work something our right away. Making a 
windows-flavoured copy of each test is not tractable.

Is there a reason you have to use clang-cl here? I was under the impression 
that clang.exe worked fine on windows too (and used a gcc-compatible command 
line)...


https://reviews.llvm.org/D52618



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

[Lldb-commits] [PATCH] D52461: [PDB] Introduce `PDBNameParser`

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D52461#1250555, @aleksandr.urakov wrote:

> I've tried to parse with it a name like
>
>   N0::`unnamed namespase'::Name
>
>
> and it can't parse it correctly. May be it just can't parse MSVC demangled 
> names?


I expect the backqoutes are confusing it. If you try something without 
anonymous namespaces, it should work fine, and adding support for them 
shouldn't be too hard (though we may run also into problems with function 
pointers or other funky names, if they don't demangle the same way as with 
itanium).

Regardless of how this is exactly implemented, I think it's important to make 
the CPlusPlusLanguage::MethodName class understand these MSVC names, as this 
class is already used in a bunch of places. So, if it chokes on MSVC names, 
you're bound to run into more problems down the line.

> Unfortunately, I can't look at the tests right now, I have a vacation. I'll 
> look at these a week later, ok?

That's fine. There's no hurry here..


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52461



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

[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D52618#1250909, @zturner wrote:

> One idea would be to define some lit substitutions like %debuginfo. It’s
>  true you can produce a gcc style command line that will be equivalent to a
>  clang-cl invocation but it won’t be easy. eg you’ll needing to pass
>  -fms-compatibility as well as various -I for includes.
>
> It may be easier to have substitutions instead


Using substitutions SGTM.

I am not sure if this is a good idea, but it had occured to me that we could 
put `-fms-compatibility` and friends into a substitution of it's own, which 
would be computed by lit (through some equivalent of `clang++ -###` ?). That 
way, the tests could still use g++ syntax, only the command lines would contain 
an extra `%cflags` argument. This has the advantage of extra flexibility over a 
predefined set of compile commands (%compile_with_debug, 
%compile_without_debug, ...), and it might be sufficient to make 
cross-compiling work, if we ever need it.


https://reviews.llvm.org/D52618



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

[Lldb-commits] [PATCH] D52719: Pull FixupBreakpointPCAsNeeded into base class

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL343683: Pull FixupBreakpointPCAsNeeded into base class 
(authored by labath, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D52719

Files:
  lldb/trunk/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/trunk/source/Host/common/NativeProcessProtocol.cpp
  lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
  lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h

Index: lldb/trunk/include/lldb/Host/common/NativeProcessProtocol.h
===
--- lldb/trunk/include/lldb/Host/common/NativeProcessProtocol.h
+++ lldb/trunk/include/lldb/Host/common/NativeProcessProtocol.h
@@ -457,6 +457,11 @@
   /// PC, this offset will be the size of the breakpoint opcode.
   virtual size_t GetSoftwareBreakpointPCOffset();
 
+  // Adjust the thread's PC after hitting a software breakpoint. On
+  // architectures where the PC points after the breakpoint instruction, this
+  // resets it to point to the breakpoint itself.
+  void FixupBreakpointPCAsNeeded(NativeThreadProtocol &thread);
+
   // ---
   /// Notify the delegate that an exec occurred.
   ///
Index: lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h
===
--- lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h
+++ lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h
@@ -112,7 +112,6 @@
   void MonitorSIGTRAP(lldb::pid_t pid);
   void MonitorSignal(lldb::pid_t pid, int signal);
 
-  Status FixupBreakpointPCAsNeeded(NativeThreadNetBSD &thread);
   Status PopulateMemoryRegionCache();
   void SigchldHandler();
 
Index: lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
===
--- lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
+++ lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
@@ -322,81 +322,6 @@
   return error;
 }
 
-Status
-NativeProcessNetBSD::FixupBreakpointPCAsNeeded(NativeThreadNetBSD &thread) {
-  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_BREAKPOINTS));
-  Status error;
-  // Find out the size of a breakpoint (might depend on where we are in the
-  // code).
-  NativeRegisterContext& context = thread.GetRegisterContext();
-  uint32_t breakpoint_size = GetSoftwareBreakpointPCOffset();
-  LLDB_LOG(log, "breakpoint size: {0}", breakpoint_size);
-
-  // First try probing for a breakpoint at a software breakpoint location: PC -
-  // breakpoint size.
-  const lldb::addr_t initial_pc_addr =
-  context.GetPCfromBreakpointLocation();
-  lldb::addr_t breakpoint_addr = initial_pc_addr;
-  if (breakpoint_size > 0) {
-// Do not allow breakpoint probe to wrap around.
-if (breakpoint_addr >= breakpoint_size)
-  breakpoint_addr -= breakpoint_size;
-  }
-  // Check if we stopped because of a breakpoint.
-  NativeBreakpointSP breakpoint_sp;
-  error = m_breakpoint_list.GetBreakpoint(breakpoint_addr, breakpoint_sp);
-  if (!error.Success() || !breakpoint_sp) {
-// We didn't find one at a software probe location.  Nothing to do.
-LLDB_LOG(log,
- "pid {0} no lldb breakpoint found at current pc with "
- "adjustment: {1}",
- GetID(), breakpoint_addr);
-return Status();
-  }
-  // If the breakpoint is not a software breakpoint, nothing to do.
-  if (!breakpoint_sp->IsSoftwareBreakpoint()) {
-LLDB_LOG(
-log,
-"pid {0} breakpoint found at {1:x}, not software, nothing to adjust",
-GetID(), breakpoint_addr);
-return Status();
-  }
-  //
-  // We have a software breakpoint and need to adjust the PC.
-  //
-  // Sanity check.
-  if (breakpoint_size == 0) {
-// Nothing to do!  How did we get here?
-LLDB_LOG(log,
- "pid {0} breakpoint found at {1:x}, it is software, but the "
- "size is zero, nothing to do (unexpected)",
- GetID(), breakpoint_addr);
-return Status();
-  }
-  //
-  // We have a software breakpoint and need to adjust the PC.
-  //
-  // Sanity check.
-  if (breakpoint_size == 0) {
-// Nothing to do!  How did we get here?
-LLDB_LOG(log,
- "pid {0} breakpoint found at {1:x}, it is software, but the "
- "size is zero, nothing to do (unexpected)",
- GetID(), breakpoint_addr);
-return Status();
-  }
-  // Change the program counter.
-  LLDB_LOG(log, "pid {0} tid {1}: changing PC from {2:x} to {3:x}", GetID(),
-   thread.GetID(), initial_pc_addr, breakpoint_addr);
-  error = context.SetPC(breakpoint_addr);
-  if (error.Fail()) {
-LLDB_LOG(log, "pid {0} tid

[Lldb-commits] [PATCH] D52941: NativeProcessProtocol: Simplify breakpoint setting code

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: krytarowski, zturner, clayborg.
Herald added a subscriber: mgorny.

A fairly simple operation as setting a breakpoint (writing a breakpoint
opcode) at a given address was going through three classes:
NativeProcessProtocol which called NativeBreakpointList, which then
called SoftwareBrekpoint, only to end up again in NativeProcessProtocol
to do the actual writing itself. This is unnecessarily complex and can
be simplified by moving all of the logic into NativeProcessProtocol
class itself, removing a lot of boilerplate.

One of the reeasons for this complexity was that (it seems)
NativeBreakpointList class was meant to hold both software and hardware
breakpoints. However, that never materialized, and hardware breakpoints
are stored in a separate map holding only hardware breakpoints.
Essentially, this patch makes software breakpoints follow that approach
by replacing the heavy SoftwareBraekpoint with a light struct of the
same name, which holds only the data necessary to describe one
breakpoint. The rest of the logic is in the main class. As, at the
lldb-server level, handling software and hardware breakpoints is very
different, this seems like a reasonable state of things.


https://reviews.llvm.org/D52941

Files:
  include/lldb/Host/common/NativeBreakpoint.h
  include/lldb/Host/common/NativeBreakpointList.h
  include/lldb/Host/common/NativeProcessProtocol.h
  include/lldb/Host/common/SoftwareBreakpoint.h
  include/lldb/lldb-private-forward.h
  lldb.xcodeproj/project.pbxproj
  source/Host/CMakeLists.txt
  source/Host/common/NativeBreakpoint.cpp
  source/Host/common/NativeBreakpointList.cpp
  source/Host/common/NativeProcessProtocol.cpp
  source/Host/common/NativeThreadProtocol.cpp
  source/Host/common/SoftwareBreakpoint.cpp
  source/Plugins/Process/Linux/NativeProcessLinux.cpp
  source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp

Index: source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
===
--- source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
+++ source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
@@ -16,7 +16,6 @@
 // Other libraries and framework includes
 #include "Plugins/Process/POSIX/ProcessPOSIXLog.h"
 #include "lldb/Host/HostProcess.h"
-#include "lldb/Host/common/NativeBreakpoint.h"
 #include "lldb/Host/common/NativeRegisterContext.h"
 #include "lldb/Host/posix/ProcessLauncherPosixFork.h"
 #include "lldb/Target/Process.h"
Index: source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -29,7 +29,6 @@
 #include "lldb/Host/HostProcess.h"
 #include "lldb/Host/PseudoTerminal.h"
 #include "lldb/Host/ThreadLauncher.h"
-#include "lldb/Host/common/NativeBreakpoint.h"
 #include "lldb/Host/common/NativeRegisterContext.h"
 #include "lldb/Host/linux/Ptrace.h"
 #include "lldb/Host/linux/Uio.h"
Index: source/Host/common/SoftwareBreakpoint.cpp
===
--- source/Host/common/SoftwareBreakpoint.cpp
+++ /dev/null
@@ -1,313 +0,0 @@
-//===-- SoftwareBreakpoint.cpp --*- C++ -*-===//
-//
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===--===//
-
-#include "lldb/Host/common/SoftwareBreakpoint.h"
-
-#include "lldb/Host/Debug.h"
-#include "lldb/Utility/Log.h"
-#include "lldb/Utility/Status.h"
-
-#include "lldb/Host/common/NativeProcessProtocol.h"
-
-using namespace lldb_private;
-
-// --- static
-// members ---
-
-Status SoftwareBreakpoint::CreateSoftwareBreakpoint(
-NativeProcessProtocol &process, lldb::addr_t addr, size_t size_hint,
-NativeBreakpointSP &breakpoint_sp) {
-  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
-  if (log)
-log->Printf("SoftwareBreakpoint::%s addr = 0x%" PRIx64, __FUNCTION__, addr);
-
-  // Validate the address.
-  if (addr == LLDB_INVALID_ADDRESS)
-return Status("SoftwareBreakpoint::%s invalid load address specified.",
-  __FUNCTION__);
-
-  // Ask the NativeProcessProtocol subclass to fill in the correct software
-  // breakpoint trap for the breakpoint site.
-  auto expected_opcode = process.GetSoftwareBreakpointTrapOpcode(size_hint);
-  if (!expected_opcode)
-return Status(expected_opcode.takeError());
-
-  assert(expected_opcode->size() > 0);
-  assert(expected_opcode->size() <= MAX_TRAP_OPCODE_SIZE);
-
-  // Enable the breakpoint.
-  uint8_t saved_opcode_bytes[MAX_TRAP_OPCODE_SIZE];
-  Status error =
-  EnableSoftwareB

[Lldb-commits] [PATCH] D52404: Prevent double import of _lldb module

[Pavel Labath via Phabricator via lldb-commits]

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

Sorry for the delay. I *think* this should be fine, but with these things its 
hard to tell whether it won't break someone's build/release setup. However, it 
does solve a real problem (I've hit this issue myself a couple of times on 
windows), so I think we should give it a try.

It might be good to note that this will only ever make a difference if one is 
using swig>=3.0.11.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52404



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

[Lldb-commits] [PATCH] D52404: Prevent double import of _lldb module

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D52404#1257347, @vadimcn wrote:

> Thanks,
>  What changed in SWIG 3.0.11?


That's the version which added support for the `moduleimport` attribute. Before 
that, the modules were imported using a fixed algorithm (I am not sure whether 
that algorithm was compatible or not with what you're doing here.)


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52404



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

[Lldb-commits] [PATCH] D53090: [Windows] Fix a bug that causes lldb to freeze

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Do we have a test for this? It sounds like it shouldn't be hard to replicate 
the dependent library load failure in a test...


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53090



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

[Lldb-commits] [PATCH] D53086: [PDB] Fix flaky `variables-locations.test` after PR38857

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D53086#1261704, @aleksandr.urakov wrote:

> As for aligned stack cross-platform problems, I mean also problems with stack 
> unwinding. They seem to appear on non-Windows too. It's because 
> `x86AssemblyInspectionEngine` doesn't support stack alignment now. I've made 
> some changes locally to fix it, but they are still raw to publish. The main 
> idea is to save one more frame address (along with CFA) for every row of an 
> unwind plan (I've called this AFA - aligned frame address), and add an 
> analysis for `and esp, ...` etc. to `x86AssemblyInspectionEngine`. What do 
> you think about a such approach?


I am not sure I fully understand the discussion here (I got lost in the 
windows-specific jargon), but are we talking about the situation where a 
function re-aligns it's stack pointer on entry via some sequence like:

  mov %esp, %ebp
  and %-8, %esp
  ...
  mov %ebp, %esp
  ret

?

If so, then I don't see why the instruction emulator should have a problem with 
this sequence, because after `mov %esp, %ebp` it will conclude that the frame 
of this function is ebp-based, and use that for unwinding (I know there were 
some issues here in the past, but I hope I have fixed those already). Or are 
you saying that your compiler manages to align the stack without producing a 
frame pointer? I think that would be very tricky, as the function itself needs 
to restore the original %esp value somehow so it can return properly. Can you 
show me the disassembly of the function in question?


https://reviews.llvm.org/D53086



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

[Lldb-commits] [PATCH] D52461: [PDB] Introduce `PDBNameParser`

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D52461#1266302, @aleksandr.urakov wrote:

>   `operator<'::`2'::B::operator>
>


The reason we had to use clang lexer for parsing itanium names is because 
parsing itanium demangled names is tricky precisely for cases like these. If 
the MSVC demangler makes these cases trivial by enclosing them in quotes, maybe 
a separate (simpler) parser is not such a bad idea.

However, I still think this should be done within the scope of 
`CPlusPlusLanguage::MethodName` otherwise, you'll have to special case MSVC for 
all existing uses of this class.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52461



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

[Lldb-commits] [PATCH] D53255: Fix: Assertion failed: (!m_first_die || m_first_die == m_die_array.front()), function ExtractDIEsRWLocked

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It looks like it should be easy to write a test for this by hand-crafting  a 
minimal .s file and feeding it to lldb-test.


Repository:
  rL LLVM

https://reviews.llvm.org/D53255



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

[Lldb-commits] [PATCH] D53086: [PDB] Fix flaky `variables-locations.test` after PR38857

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

That's an interesting code snippet. Thanks for sharing that.

The thing that's not clear to me is: are you specifically interested in 
unwinding from these kinds of functions **without debug info**? Because it 
sounds to me like the info Zachary provided is enough to unwind from these 
functions, assuming debug info is present. And in this case there shouldn't be 
any need for instruction emulation.


https://reviews.llvm.org/D53086



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

[Lldb-commits] [PATCH] D53086: [PDB] Fix flaky `variables-locations.test` after PR38857

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Yes it sounds like the situation is very much similar here as it is in  dwarf 
land -- the compilers there also don't tend to emit unwind info for prologues 
and epilogues.

The thing to realize about the instruction emulation is that it is always going 
to be a best effort enterprise. We can try to pattern match the sequences 
generated by the compilers, but it's always going to be playing catch up, and 
prone to false positives. That's why it's important to use the debug info 
provided by the compiler, as it is in a much better position to generate the 
unwind info.

Even the emulator itself has two modes of operation: in the first one it 
synthesizes the unwind info out-of-the-blue. In the second one it takes the 
existing unwind info and "augments" it with additional rows. Obviously, the 
second one is much more easier to implement and reliable.

The conversion of the FPO programs into DWARF does not sound like a 
particularly fun enterprise, though I'm not sure if that's the only way to do 
this kind of thing. In any case, it sounds like the process should be easily 
unit-testable. As for the saved registers, won't the compiler's unwind info 
contain that information? I would expect that is necessary for exception 
unwinding..

tl;dr; I am not against improving the instruction emulator. I just think it's 
not the most important priority right now. Getting the compiler-generated info 
working puts the emulator out of the critical path, and leaves it just 
responsible for the less frequent cases when someone is stopped in the middle 
of the prologue, or needs to access a variable that resides in a spilled 
register. But if you want to work on the emulator first, then go ahead..


https://reviews.llvm.org/D53086



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

[Lldb-commits] [PATCH] D53532: [FileSpec] Add VFS support

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Back when the FileSystem class was introduced, the idea was that *it* would 
eventually become the gateway to the real filesystem, and FileSpec would just 
deal with abstract path manipulation.

I still think that is a good idea, particularly in light of this patch. So I 
would propose to integrate this VFS functionality into that class, and then 
remove any operations that touch real files from the file spec class.

(Also, given that now about 50% of filesystem operations go through FileSpec 
and the other half through FileSystem (or other methods), any attempts to use 
FileSpec to control how filenames are interpreted will not be complete).


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53532



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

[Lldb-commits] [PATCH] D53532: [FileSpec] Add VFS support to FileSpec convenience methods.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I also think it we should try to keep FileSpec in the Utility module. We should 
be able to do that by deleting the "utility" functions in the FileSpec class, 
and updating everything to go through the FileSystem class.

After the VFS introduction, something like `file_spec.Exists()` will not even 
be well-defined anyway, as one will have to specify which file system are we 
checking the existence in. So, it would be more correct to say 
`file_spec.Exists(my_vfs)`, at which point it's not too much of a stretch to 
change this into `my_vfs.Exists(file_spec)`.

(This is the same argument I was giving earlier, since even before VFS we kinda 
were dealing with multiple file systems (host, remote device). In fact, after 
this it might be a nice cleanup to change the Platform class from vending a 
bunch of FS-like methods to (FileExists, Unlink, CreateSymlink, ...), to have a 
single `getRemoteFS` method returning a VFS which does the right thing "under 
the hood").

tl;dr: I'd propose to (in two or more patches):

- replace FileSpec "utility" methods with equivalent FileSystem functionality
- do the necessary FileSystem changes to support VFS




Comment at: source/Host/common/FileSystem.cpp:24-29
+  static FileSystem *g_fs;
+  static llvm::once_flag g_once_flag;
+  llvm::call_once(g_once_flag, []() {
+if (g_fs == nullptr)
+  g_fs = new FileSystem();
+  });

replace with `static FileSystem *g_fs = new FileSystem();`



Comment at: source/Host/common/FileSystem.cpp:33-35
+void FileSystem::ChangeFileSystem(IntrusiveRefCntPtr fs) {
+  m_fs = fs;
+}

I'm not sure what's your use case here, but would it make more sense instead of 
modifying the singleton object to have more that one FileSystem instance 
floating around? It could be local to each `Debugger` (?) object and everyone 
would fetch it from there when needed.


https://reviews.llvm.org/D53532



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

[Lldb-commits] [PATCH] D53532: [FileSystem] Extend file system and have it use the VFS.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Now that I know your use case (I forgot that you're working on the replay now, 
and the mention of VFS threw me off in a completely different direction), I 
agree that having a single global filesystem is fine. As for the increased 
verbosity, I wonder if we should make the fs accessor a free function instead 
of a member (so that you write something like `hostFS().Symlink(..)` instead of 
`FileSystem::instance().Symlink(...)`.

(Theoretically, we might even make the VFS instance a global variable, and then 
we could keep the current pattern of accessing stuff via static methods, but I 
like the fact that this VFS thingy could then be reused for working with files 
on remote devices. Also, you may need a working FS instance in the guts of the 
replay code, to e.g., read the replay data.)




Comment at: include/lldb/Host/FileSystem.h:35
+
+  void ChangeFileSystem(llvm::IntrusiveRefCntPtr fs);
+

I am wondering if we could make this work without exposing the ability to 
change the FS implementation mid-flight to everyone. How are you planning to 
initialize the record/replay? Would it be possible to pass in the VFS instance 
via some kind of an `Initialize` function (and have it be immutable from that 
point on)?



Comment at: include/lldb/Utility/FileSpec.h:423
   /// @return
-  /// Returns a std::string with the directory and filename
+  /// Retuthe rns a std::string with the directory and filename
   /// concatenated.

accidental change?


https://reviews.llvm.org/D53532



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

[Lldb-commits] [PATCH] D53511: [NativePDB] Support type lookup by name

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

> For the purposes of testing I decided to bypass lldb-test and go with a 
> scripted lldbinit file. I'm sticking with this approach wherever possible to 
> prove that this is "true" cross-platform debugger functionality. However, 
> there are definite limitations. For example, the output format of type lookup 
> has some limitations. It doesn't print the layout of the type in any kind of 
> detail (e.g. field offsets), it doesn't support lookup by regex, it doesn't 
> print the underlying type of an enumeration, it doesn't support limiting the 
> scope of the search to specific kinds of types, so there is definitely room 
> for lldb-test to come into the picture here to expand the coverage since we 
> have full control over the output format.

When implementing the debug_names accelerator tables, I also needed to test the 
lookup funtionality. That's how lldb-test symbols 
-find=(function|namespace|...) came to be.  However, there I needed to just 
test that a type is found and not any of it's details, so I just used whatever 
dumping method we had implemented already. It would definitely be possible to 
change this dumping format to print more detailed (and FileCheckable) 
information.

BTW, it might be interesting to take some of the tests in 
`lit/SymbolFile/DWARF/find-***` and add a `RUN: clang-cl ...` line to them, to 
verify that the same types/namespaces/variables are found regardless of whether 
we use DWARF or PDB (right now they test that .debug_names and manual dwarf 
indexes return the same results).


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53511



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

[Lldb-commits] [PATCH] D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules()

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It sounds like at least part of this could be tested by adding the ability to 
dump dependent modules to `lldb-test object-file` (which I think would fit very 
nicely within the current information printed by that command). I seem to 
recall seeing code like that in some patch but it seems to be missing here now.


https://reviews.llvm.org/D53094



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

[Lldb-commits] [PATCH] D53677: Fix a bug PlatformDarwin::SDKSupportsModule

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

lgtm. Sorry about the trouble.


https://reviews.llvm.org/D53677



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

[Lldb-commits] [PATCH] D53785: [FileSystem] Move EnumerateDirectory from FileSpec to FileSystem.

[Pavel Labath via Phabricator via lldb-commits]

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

lgtm


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53785



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

[Lldb-commits] [PATCH] D53532: [FileSystem] Extend file system and have it use the VFS.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D53532#1279022, @JDevlieghere wrote:

> Address Pavel's feedback:
>
> - Change to `Initialize` method which can only be called once.


I suppose this is slightly better, but what I meant was to have a `static` 
(like all other functions with the same name) `Initialize` function, which has 
to be called before the first call to `Instance()`. During normal startup, this 
could be called from `SystemInitializer`, but I am not sure how would that work 
for record/replay (that's why I was asking how you planned to initialize 
those). It would require to setup the record/replay machinery very early on, 
which may be tricky. But on the other hand, you probably want to initialize 
these as early as possible anyway.

Do you think something like that would be possible?


https://reviews.llvm.org/D53532



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

[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Ok, if that's how you guys feel, then I won't stand in your way. I am fine with 
this patch then.


https://reviews.llvm.org/D52618



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

[Lldb-commits] [PATCH] D53532: [FileSystem] Extend file system and have it use the VFS.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D53532#1280734, @JDevlieghere wrote:

> In https://reviews.llvm.org/D53532#1280018, @labath wrote:
>
> > In https://reviews.llvm.org/D53532#1279022, @JDevlieghere wrote:
> >
> > > Address Pavel's feedback:
> > >
> > > - Change to `Initialize` method which can only be called once.
> >
> >
> > I suppose this is slightly better, but what I meant was to have a `static` 
> > (like all other functions with the same name) `Initialize` function, which 
> > has to be called before the first call to `Instance()`. During normal 
> > startup, this could be called from `SystemInitializer`, but I am not sure 
> > how would that work for record/replay (that's why I was asking how you 
> > planned to initialize those). It would require to setup the record/replay 
> > machinery very early on, which may be tricky. But on the other hand, you 
> > probably want to initialize these as early as possible anyway.
> >
> > Do you think something like that would be possible?
>
>
> Alright, I see what you mean. You know more about the architecture, but it 
> looks like we initialize the system before even reading the command line 
> arguments in the driver. I have to read the reproducer before I can set the 
> appropriate file system, so I'm not sure if this is possible.


Yes, right now it certainly seems to be the case that we parse cmdline late in 
the game. However, it's not clear to me whether that has to be the case.

I can't say I have thought this through to the end, but it seems to me that 
setting up the repro engine should be one of the first (if not THE first) SB 
calls from the driver. Right now the parsing happens too late (e.g. we already 
have an SBDebugger instance created at that point). I am not sure you could 
safely initialize record/replay at that point, even if you got FileSystem 
switching working.

Architecturally, the cleanest solution to me seems to be doing the command line 
parsing (even if it's just the repro-related args) before 
SBDebugger::Initialize, and then have special versions of the `Initialize` 
function like `Initialize(args_for_record)` and `Initialize(args_for_replay)`. 
The interesting repro-related stuff would then happen in these functions, which 
can then call the `Initialize` functions of relevant components (like 
`FileSystem`) and pass appropriate arguments.

However, I can't say how would that fit into your intended design or any code 
you already have implemented.


https://reviews.llvm.org/D53532



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

[Lldb-commits] [PATCH] D53834: [FileSystem] Remove ResolveExecutableLocation() from FileSpec

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Why did you change the function name? I think it would be nice to keep 
"executable" or something to that effect in the name (e.g. the llvm function 
has "program"), given that the underlying function checks for the executable 
bit, and so it cannot be used for general searches.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53834



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

[Lldb-commits] [PATCH] D53532: [FileSystem] Extend file system and have it use the VFS.

[Pavel Labath via Phabricator via lldb-commits]

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

Thanks for bearing with me. I am very happy with how this turned out in the 
end. I have more suggestion on how to clean up the initialization, which you 
can incorporate if you like it, but I don't want to hold up the patch over that.




Comment at: include/lldb/Host/FileSystem.h:21
 
+#include 
 #include 

Not needed anymore?



Comment at: include/lldb/Utility/FileSpec.h:423
   /// @return
-  /// Returns a std::string with the directory and filename
+  /// Retuthe rns a std::string with the directory and filename
   /// concatenated.

labath wrote:
> accidental change?
It seems this is still here... :(



Comment at: source/Host/common/FileSystem.cpp:32
+  FileSystem &instance = Instance();
+  instance.m_initialized = true;
+}

Instead of manual `m_initialized` management, what would you say to this:
- have a private `InstanceImpl` (or whatever) function, which returns an 
`Optional &`
- `Initialize` does `InstanceImpl().emplace(VFS);` (it can check that the value 
is `None` beforehand if you want, but I am not worried about people 
accidentally calling `Initialize` twice).
- `Terminate` does `InstanceImpl().reset();`
- `Instance` does `return *InstanceImpl();`

I think this would be a bit cleaner as it would allow is to specify the VFS 
directly during construction. We would avoid having to construct a `FileSystem` 
object with a dummy VFS, only to replace it with another one later. And also 
you wouldn't need to subclass FileSystem in the unit tests just to change the 
implementation.



Comment at: unittests/Host/FileSystemTest.cpp:302-310
+  EXPECT_EQ(visited.size(), (size_t)4);
+  EXPECT_TRUE(std::find(visited.begin(), visited.end(), "/foo") !=
+  visited.end());
+  EXPECT_TRUE(std::find(visited.begin(), visited.end(), "/bar") !=
+  visited.end());
+  EXPECT_TRUE(std::find(visited.begin(), visited.end(), "/baz") !=
+  visited.end());

You can write this more concisely (and with better error messages) as 
`EXPECT_THAT(visited, testing::UnorderedElementsAre("/foo", "/bar", "/baz", 
"/qux"));` (you need to include `gmock.h` for that to work).


https://reviews.llvm.org/D53532



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

[Lldb-commits] [PATCH] D52461: [PDB] Introduce `MSVCUndecoratedNameParser`

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It's not fully clear to me from the previous comments if you are proceeding 
with this or not, but in case you are, I have made comments inline. I see that 
you've added some lit tests, but I also think you it would be good add some 
unit tests for the name parser functionality per-se (similar to the existing 
name parsing tests), as it is much easier to see what is going on from those.

Right now, I don't think this solution can be specific to the "old" PDB plugin, 
as this functionality is used from other places as well (RichManglingContext 
being the most important one). Maybe once we start using the fancy MSVC 
demangler there, we can revisit that. (But given that the declared intent of 
that class is to chop up demangled names, I think it would make sense to keep 
this there even then. Unless it turns out we can delete the whole class at that 
point.)




Comment at: 
source/Plugins/Language/CPlusPlus/MSVCUndecoratedNameParser.cpp:62-64
+  m_specifiers.emplace_back(llvm::StringRef(name.data(), i - 1),
+llvm::StringRef(name.data() + last_base_start,
+i - last_base_start - 1));

Rewrite this (and all other instances of StringRef -> char * -> StringRef 
roundtripping) in terms of StringRef functions.
Maybe something like: `emplace_back(name.take_front(i-1), 
name.slice(last_base_start, i-1));` ?



Comment at: source/Plugins/Language/CPlusPlus/MSVCUndecoratedNameParser.h:35-39
+  std::size_t GetSpecifiersCount() const { return m_specifiers.size(); }
+
+  MSVCUndecoratedNameSpecifier GetSpecifierAtIndex(std::size_t index) const {
+return m_specifiers[index];
+  }

Could we replace these by something like 
`ArrayRef GetSpecifiers()`


https://reviews.llvm.org/D52461



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

[Lldb-commits] [PATCH] D54020: [FileSystem] Open File instances through the FileSystem.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I am not sure what do you mean by "translating paths" in the VFS. If you mean 
something like "return a `FILE *` for `/whatever/my_reproducer/vfs/bin/ls` when 
I ask for `/bin/ls`", then I think that's a good idea, as it's most likely to 
work with all of our existing code (e.g. mmapping). Although, in this case, i 
am not sure how much benefit will the llvm VFS bring to the game, as most of 
the operations could then be implemented by plainly prepending some  prefix to 
a given path.

Getting rid of `FILE *` is not going to be easy, as it is used for some of our 
external dependencies (libedit). It's possible to create a fake FILE* on posix 
systems (`fopencookie, fmemopen`), but I don't think it's possible to do 
something like that on windows (which is why I stopped when I was looking at 
this some time ago).

Also, be aware that there are some places where we open `raw_ostream`s directly 
(`Debugger::EnableLog` comes to mind). I guess those will need to go through 
the `FileSystem` too...




Comment at: source/Host/common/FileSystem.cpp:253
+static int GetOpenFlags(uint32_t options) {
+  const bool read = options & File::OpenOptions::eOpenOptionRead;
+  const bool write = options & File::OpenOptions::eOpenOptionWrite;

`File::eOpenOptionRead` should be sufficient, no?



Comment at: source/Host/posix/FileSystem.cpp:74-79
+FILE *FileSystem::Fopen(const char *path, const char *mode) const {
   return ::fopen(path, mode);
 }
 
-int FileSystem::Open(const char *path, int flags, int mode) {
+int FileSystem::Open(const char *path, int flags, int mode) const {
   return ::open(path, flags, mode);

Why are these two `const`? It seems that at least with `eOpenOptionCanCreate`, 
one can create a new file, which is definitely not a "const" operation.



Comment at: source/Utility/FileSpec.cpp:321-322
 
+bool FileSpec::Empty() const { return m_directory && m_filename; }
+
 //--

We already have `operator bool` for this.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54020



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

[Lldb-commits] [PATCH] D54020: [FileSystem] Open File instances through the FileSystem.

[Pavel Labath via Phabricator via lldb-commits]

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

In https://reviews.llvm.org/D54020#1285539, @JDevlieghere wrote:

> In https://reviews.llvm.org/D54020#1285201, @labath wrote:
>
> > I am not sure what do you mean by "translating paths" in the VFS. If you 
> > mean something like "return a `FILE *` for 
> > `/whatever/my_reproducer/vfs/bin/ls` when I ask for `/bin/ls`", then I 
> > think that's a good idea, as it's most likely to work with all of our 
> > existing code (e.g. mmapping). Although, in this case, i am not sure how 
> > much benefit will the llvm VFS bring to the game, as most of the operations 
> > could then be implemented by plainly prepending some  prefix to a given 
> > path.
>
>
> Let me try to explain this better. This is mostly a question about what kind 
> of API the VFS (which lives in LLVM) provides to deal with files in lldb 
> (i.e. through `FILE*` and file descriptors).
>
> The first possibility is providing a method in the VFS that takes a "virtual" 
> path and returns the "underlying" path. Something like  `Optional 
> vfs::getUnderlyingPath(path)`. This doesn't just have to be a prefix thing 
> but you are right that it's mostly going to be just that. The problem is that 
> this method wouldn't work for virtual file systems that don't have files on 
> disk. Hence the lack of generality.


Ok, I think we're on the same page here. What I was wondering is that given 
this lack of generality (this operation would only be supported on 
`DiskBackedFilesystem`, and not all other kinds of file systems), whether it is 
not better to just do a custom solution instead of going through the VFS layer 
(thereby removing one layer, since now we have `lldb_private::FileSystem` 
sitting on top of `llvm::VirtualFileSystem`, sitting on top of the real thing). 
E.g., if we know we can restrict ourselves to the case where the on disk layout 
matches the real system, then all filesystem operations can be implemented very 
trivially via something like:

  auto virtual_op(const Twine &Path, ...) { return real_op(Prefix+Path, ...); }

I am not sure whether this is a good idea (there's definitely a case to be made 
for reusing existing infrastructure), but it is something to think about.

>> Also, be aware that there are some places where we open `raw_ostream`s 
>> directly (`Debugger::EnableLog` comes to mind). I guess those will need to 
>> go through the `FileSystem` too...
> 
> Yup, we need to audit all file access. This particular example actually goes 
> through file so it should be covered by this change.

You're right, I should have looked at this more closely. It should be fine 
(assuming we can have real fds for the virtual files).




Comment at: source/Host/common/FileSystem.cpp:253
+static int GetOpenFlags(uint32_t options) {
+  const bool read = options & File::OpenOptions::eOpenOptionRead;
+  const bool write = options & File::OpenOptions::eOpenOptionWrite;

JDevlieghere wrote:
> labath wrote:
> > `File::eOpenOptionRead` should be sufficient, no?
> Sorry, I don't know what you mean?
I meant to remove the superfluous `OpenOptions` qualification, As this is a 
non-class enum, it is not necessary, and it doesn't help clarity, since the 
enum value already contains the type name. None of the existing code uses this 
kind of qualification.


https://reviews.llvm.org/D54020



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

[Lldb-commits] [PATCH] D53989: Fix formatting of wchar, char16, and char32

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

lldb-server is used on architectures that don't have python (readily) 
available, and it uses the same codebase as the rest of lldb. (Granted, it only 
needs a small subset of that codebase, and this subset doesn't/shouldn't care 
about python, but we aren't able to split out that part cleanly (yet)). So I 
don't think requiring python is a good idea.

What we could do is make it a build-time error if we failed to detect&configure 
python *AND* the user hasn't explicitly disabled it. This would catch this 
problem early and avoid other surprises later on (e.g. the entire dotest test 
suite only works with python enabled. without it, you'd likely get some weird 
error).


https://reviews.llvm.org/D53989



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

[Lldb-commits] [PATCH] D54053: [NativePDB] Add the ability to create clang record decls from mangled names.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

A very nice use of the structured demangler.


https://reviews.llvm.org/D54053



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

[Lldb-commits] [PATCH] D52461: [PDB] Introduce `MSVCUndecoratedNameParser`

[Pavel Labath via Phabricator via lldb-commits]

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

Thanks for your patience. This looks good to me now.


https://reviews.llvm.org/D52461



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

[Lldb-commits] [PATCH] D52941: NativeProcessProtocol: Simplify breakpoint setting code

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Kamil, I think you're the best qualified person to look at the lldb-server 
changes right now, so I'd like to hear your opinion on the other changes too. 
Otherwise, I'll just commit this some time next week.


https://reviews.llvm.org/D52941



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

[Lldb-commits] [PATCH] D54074: CPlusPlusLanguage: Use new demangler API to implement type substitution

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: sgraenitz, erik.pilkington, JDevlieghere.

Now that llvm demangler supports more generic customization, we can
implement type substitution directly on top of this API. This will allow
us to remove the specialized hooks which were added to the demangler to
support this use case.


https://reviews.llvm.org/D54074

Files:
  source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

Index: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
===
--- source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -21,7 +21,7 @@
 
 // Other libraries and framework includes
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Demangle/Demangle.h"
+#include "llvm/Demangle/ItaniumDemangle.h"
 
 // Project includes
 #include "lldb/Core/PluginManager.h"
@@ -274,72 +274,67 @@
   return false;
 }
 
-/// Given a mangled function `mangled`, replace all the primitive function type
-/// arguments of `search` with type `replace`.
-static ConstString SubsPrimitiveParmItanium(llvm::StringRef mangled,
-llvm::StringRef search,
-llvm::StringRef replace) {
-  class PrimitiveParmSubs {
-llvm::StringRef mangled;
-llvm::StringRef search;
-llvm::StringRef replace;
-ptrdiff_t read_pos;
-std::string output;
-std::back_insert_iterator writer;
+namespace {
+class NodeAllocator {
+  llvm::BumpPtrAllocator Alloc;
 
-  public:
-PrimitiveParmSubs(llvm::StringRef m, llvm::StringRef s, llvm::StringRef r)
-: mangled(m), search(s), replace(r), read_pos(0),
-  writer(std::back_inserter(output)) {}
-
-void Substitute(llvm::StringRef tail) {
-  assert(tail.data() >= mangled.data() &&
- tail.data() < mangled.data() + mangled.size() &&
- "tail must point into range of mangled");
-
-  if (tail.startswith(search)) {
-auto reader = mangled.begin() + read_pos;
-ptrdiff_t read_len = tail.data() - (mangled.data() + read_pos);
-
-// First write the unmatched part of the original. Then write the
-// replacement string. Finally skip the search string in the original.
-writer = std::copy(reader, reader + read_len, writer);
-writer = std::copy(replace.begin(), replace.end(), writer);
-read_pos += read_len + search.size();
-  }
-}
+public:
+  void reset() { Alloc.Reset(); }
 
-ConstString Finalize() {
-  // If we did a substitution, write the remaining part of the original.
-  if (read_pos > 0) {
-writer = std::copy(mangled.begin() + read_pos, mangled.end(), writer);
-read_pos = mangled.size();
-  }
+  template  T *makeNode(Args &&... args) {
+return new (Alloc.Allocate(sizeof(T), alignof(T)))
+T(std::forward(args)...);
+  }
 
-  return ConstString(output);
+  void *allocateNodeArray(size_t sz) {
+return Alloc.Allocate(sizeof(llvm::itanium_demangle::Node *) * sz,
+  alignof(llvm::itanium_demangle::Node *));
+  }
+};
+
+/// Given a mangled function `Mangled`, replace all the primitive function type
+/// arguments of `Search` with type `Replace`.
+class TypeSubstitutor
+: public llvm::itanium_demangle::AbstractManglingParser {
+  const char *Written;
+  llvm::StringRef Search;
+  llvm::StringRef Replace;
+  llvm::SmallString<128> Result;
+  llvm::raw_svector_ostream OS{Result};
+  bool Substituted = false;
+
+  TypeSubstitutor(llvm::StringRef Mangled, llvm::StringRef Search,
+  llvm::StringRef Replace)
+  : AbstractManglingParser(Mangled.begin(), Mangled.end()),
+Written(Mangled.begin()), Search(Search), Replace(Replace) {}
+
+public:
+  static ConstString substitute(llvm::StringRef Mangled, llvm::StringRef From,
+llvm::StringRef To) {
+Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_LANGUAGE);
+
+TypeSubstitutor TS(Mangled, From, To);
+if (TS.parse() == nullptr) {
+  LLDB_LOG(log, "Failed to substitute mangling in {0}", Mangled);
+  return ConstString();
 }
+if (!TS.Substituted)
+  return ConstString();
+TS.OS << llvm::StringRef(TS.Written, TS.First - TS.Written);
+LLDB_LOG(log, "Substituted mangling {0} -> {1}", Mangled, TS.OS.str());
+return ConstString(TS.OS.str());
+  }
 
-static void Callback(void *context, const char *match) {
-  ((PrimitiveParmSubs *)context)->Substitute(llvm::StringRef(match));
+  llvm::itanium_demangle::Node *parseType() {
+if (llvm::StringRef(First, Last - First).startswith(Search)) {
+  OS << llvm::StringRef(Written, First - Written) << Replace;
+  Written = First + Search.size();
+  Substituted = true;
 }
-  };
-
-  // The demangler will call back for each instance of a primitive type,
-  // allowing us to perform substitution
-  Pr

[Lldb-commits] [PATCH] D54072: Add a command to dump a module's clang ast.

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:2275
+SymbolFile *sf = m->GetSymbolVendor()->GetSymbolFile();
+sf->DumpClangAST();
+  }

The dump function should take the output stream from the `result` object, so 
the output ends up where it's expected to go. (`clang::DeclBase::dump` has an 
overload which takes a `raw_ostream`, and lldb streams have a `raw_ostream` 
accessor now).


https://reviews.llvm.org/D54072



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

[Lldb-commits] [PATCH] D52941: NativeProcessProtocol: Simplify breakpoint setting code

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

thanks.


https://reviews.llvm.org/D52941



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

[Lldb-commits] [PATCH] D52941: NativeProcessProtocol: Simplify breakpoint setting code

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346093: NativeProcessProtocol: Simplify breakpoint setting 
code (authored by labath, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D52941

Files:
  lldb/trunk/include/lldb/Host/common/NativeBreakpoint.h
  lldb/trunk/include/lldb/Host/common/NativeBreakpointList.h
  lldb/trunk/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/trunk/include/lldb/Host/common/SoftwareBreakpoint.h
  lldb/trunk/include/lldb/lldb-private-forward.h
  lldb/trunk/lldb.xcodeproj/project.pbxproj
  lldb/trunk/source/Host/CMakeLists.txt
  lldb/trunk/source/Host/common/NativeBreakpoint.cpp
  lldb/trunk/source/Host/common/NativeBreakpointList.cpp
  lldb/trunk/source/Host/common/NativeProcessProtocol.cpp
  lldb/trunk/source/Host/common/NativeThreadProtocol.cpp
  lldb/trunk/source/Host/common/SoftwareBreakpoint.cpp
  lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/trunk/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp

Index: lldb/trunk/include/lldb/Host/common/NativeProcessProtocol.h
===
--- lldb/trunk/include/lldb/Host/common/NativeProcessProtocol.h
+++ lldb/trunk/include/lldb/Host/common/NativeProcessProtocol.h
@@ -25,6 +25,8 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include 
+#include 
 #include 
 
 namespace lldb_private {
@@ -35,8 +37,6 @@
 // NativeProcessProtocol
 //--
 class NativeProcessProtocol {
-  friend class SoftwareBreakpoint;
-
 public:
   virtual ~NativeProcessProtocol() {}
 
@@ -111,10 +111,6 @@
 
   virtual Status RemoveBreakpoint(lldb::addr_t addr, bool hardware = false);
 
-  virtual Status EnableBreakpoint(lldb::addr_t addr);
-
-  virtual Status DisableBreakpoint(lldb::addr_t addr);
-
   //--
   // Hardware Breakpoint functions
   //--
@@ -402,6 +398,13 @@
   }
 
 protected:
+  struct SoftwareBreakpoint {
+uint32_t ref_count;
+llvm::SmallVector saved_opcodes;
+llvm::ArrayRef breakpoint_opcodes;
+  };
+
+  std::unordered_map m_software_breakpoints;
   lldb::pid_t m_pid;
 
   std::vector> m_threads;
@@ -415,7 +418,6 @@
 
   std::recursive_mutex m_delegates_mutex;
   std::vector m_delegates;
-  NativeBreakpointList m_breakpoint_list;
   NativeWatchpointList m_watchpoint_list;
   HardwareBreakpointMap m_hw_breakpoints_map;
   int m_terminal_fd;
@@ -446,7 +448,9 @@
   // --- Internal
   // interface for software breakpoints
   // ---
+
   Status SetSoftwareBreakpoint(lldb::addr_t addr, uint32_t size_hint);
+  Status RemoveSoftwareBreakpoint(lldb::addr_t addr);
 
   virtual llvm::Expected>
   GetSoftwareBreakpointTrapOpcode(size_t size_hint);
@@ -474,6 +478,8 @@
 
 private:
   void SynchronouslyNotifyProcessStateChanged(lldb::StateType state);
+  llvm::Expected
+  EnableSoftwareBreakpoint(lldb::addr_t addr, uint32_t size_hint);
 };
 } // namespace lldb_private
 
Index: lldb/trunk/include/lldb/Host/common/NativeBreakpointList.h
===
--- lldb/trunk/include/lldb/Host/common/NativeBreakpointList.h
+++ lldb/trunk/include/lldb/Host/common/NativeBreakpointList.h
@@ -10,13 +10,9 @@
 #ifndef liblldb_NativeBreakpointList_h_
 #define liblldb_NativeBreakpointList_h_
 
-#include "lldb/Utility/Status.h"
 #include "lldb/lldb-private-forward.h"
-// #include "lldb/Host/NativeBreakpoint.h"
-
-#include 
+#include "lldb/lldb-types.h"
 #include 
-#include 
 
 namespace lldb_private {
 
@@ -26,35 +22,6 @@
 };
 
 using HardwareBreakpointMap = std::map;
-
-class NativeBreakpointList {
-public:
-  typedef std::function
-  CreateBreakpointFunc;
-
-  NativeBreakpointList();
-
-  Status AddRef(lldb::addr_t addr, size_t size_hint, bool hardware,
-CreateBreakpointFunc create_func);
-
-  Status DecRef(lldb::addr_t addr);
-
-  Status EnableBreakpoint(lldb::addr_t addr);
-
-  Status DisableBreakpoint(lldb::addr_t addr);
-
-  Status GetBreakpoint(lldb::addr_t addr, NativeBreakpointSP &breakpoint_sp);
-
-  Status RemoveTrapsFromBuffer(lldb::addr_t addr, void *buf, size_t size) const;
-
-private:
-  typedef std::map BreakpointMap;
-
-  std::recursive_mutex m_mutex;
-  BreakpointMap m_breakpoints;
-};
 }
 
 #endif // ifndef liblldb_NativeBreakpointList_h_
Index: lldb/trunk/include/lldb/lldb-private-forward.h
===
--- lldb/trunk/include/lldb/lldb-private-forward.h
+++ lldb/trunk/include/lldb/lldb-private-forward.h
@@ -10,26 +10,15 @@
 #ifndef LLDB_lldb_private_forward_h_
 #define LLDB_l

[Lldb-commits] [PATCH] D54072: Add a command to dump a module's clang ast.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D54072#1286748, @zturner wrote:

> Unfortunately then color output is impossible. Where else would the output
>  be expected to go?


If you execute the command over the SB API 
(SBCommandInterpreter::HandleCommand) then it will go into the provided result 
object. Also, some clients (typically IDEs) use SetInput/Output/ErrorFileHandle 
to set what is considered to be the default input/output streams for a given 
debugger instance (typically, to redirect these to some console window).

I don't think this directly precludes colored output, although it may require a 
bit more plumbing to pass the information whether the final consumer is willing 
to accept color escape codes. (We can already get that via 
`StreamFile->GetFile().GetIsTerminalWithColors()`, so you would just need to 
somehow pass this information to the proxy raw_ostream you give to the clang 
dump function.)


https://reviews.llvm.org/D54072



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

[Lldb-commits] [PATCH] D54072: Add a command to dump a module's clang ast.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

If it comes down to choosing between colored output going to stderr and plain 
output going where it should, i'd go with the second option.

The way I'd implement this to support both things is approximately this:

- add color (`has_colors`, `changeColor` and friends) support to `Stream` 
class, same as `llvm::raw_ostream`. Apart from solving this problem, this will 
make the two classes more similar, making eventual llvm transition smoother.
- the `raw_ostream` shim in the `Stream` class can just forward the relevant 
operations
- make the colored-ness of StringStream configurable at construction time. When 
constructing the StringStream holding the command output, we would look at 
whether the final output supports colors (Raphael, would that be 
`Debugger.getUseColor()?`) or not, and initialize it appropriately.
- as an alternative to previous step, it might be good to check how can look a

It may be more work than you're ready to put into this right now, but it would 
be very cool nonetheless, as it would unlock colored output for all other 
commands (and I can think of a couple who could use that).

The downside here is that this would require duplicating the ansi escape 
sequence code in raw_fd_ostream. However, I believe we already have a code for 
handling those, so that can hopefully be reused.


https://reviews.llvm.org/D54072



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

[Lldb-commits] [PATCH] D54074: CPlusPlusLanguage: Use new demangler API to implement type substitution

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 172583.
labath added a comment.

Thanks for the review.

I think that creating the new BumpPtrAllocator every call doesn't matter much 
here, this function
get's called only on a few symbols for each evaluated expression. However, it 
also wasn't too
hard to reuse it (at least at the level of FindAlternateFunctionManglings), so 
I just did that.

I also added a simple failure test for this function.


https://reviews.llvm.org/D54074

Files:
  source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp

Index: unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
===
--- unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
+++ unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
@@ -184,4 +184,5 @@
   EXPECT_THAT(FindAlternate("_ZN1A1fEx"), Contains("_ZN1A1fEl"));
   EXPECT_THAT(FindAlternate("_ZN1A1fEy"), Contains("_ZN1A1fEm"));
   EXPECT_THAT(FindAlternate("_ZN1A1fEai"), Contains("_ZN1A1fEci"));
+  EXPECT_THAT(FindAlternate("_bogus"), IsEmpty());
 }
Index: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
===
--- source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -21,7 +21,7 @@
 
 // Other libraries and framework includes
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Demangle/Demangle.h"
+#include "llvm/Demangle/ItaniumDemangle.h"
 
 // Project includes
 #include "lldb/Core/PluginManager.h"
@@ -274,72 +274,74 @@
   return false;
 }
 
-/// Given a mangled function `mangled`, replace all the primitive function type
-/// arguments of `search` with type `replace`.
-static ConstString SubsPrimitiveParmItanium(llvm::StringRef mangled,
-llvm::StringRef search,
-llvm::StringRef replace) {
-  class PrimitiveParmSubs {
-llvm::StringRef mangled;
-llvm::StringRef search;
-llvm::StringRef replace;
-ptrdiff_t read_pos;
-std::string output;
-std::back_insert_iterator writer;
+namespace {
+class NodeAllocator {
+  llvm::BumpPtrAllocator Alloc;
 
-  public:
-PrimitiveParmSubs(llvm::StringRef m, llvm::StringRef s, llvm::StringRef r)
-: mangled(m), search(s), replace(r), read_pos(0),
-  writer(std::back_inserter(output)) {}
-
-void Substitute(llvm::StringRef tail) {
-  assert(tail.data() >= mangled.data() &&
- tail.data() < mangled.data() + mangled.size() &&
- "tail must point into range of mangled");
-
-  if (tail.startswith(search)) {
-auto reader = mangled.begin() + read_pos;
-ptrdiff_t read_len = tail.data() - (mangled.data() + read_pos);
-
-// First write the unmatched part of the original. Then write the
-// replacement string. Finally skip the search string in the original.
-writer = std::copy(reader, reader + read_len, writer);
-writer = std::copy(replace.begin(), replace.end(), writer);
-read_pos += read_len + search.size();
-  }
-}
+public:
+  void reset() { Alloc.Reset(); }
 
-ConstString Finalize() {
-  // If we did a substitution, write the remaining part of the original.
-  if (read_pos > 0) {
-writer = std::copy(mangled.begin() + read_pos, mangled.end(), writer);
-read_pos = mangled.size();
-  }
+  template  T *makeNode(Args &&... args) {
+return new (Alloc.Allocate(sizeof(T), alignof(T)))
+T(std::forward(args)...);
+  }
 
-  return ConstString(output);
-}
+  void *allocateNodeArray(size_t sz) {
+return Alloc.Allocate(sizeof(llvm::itanium_demangle::Node *) * sz,
+  alignof(llvm::itanium_demangle::Node *));
+  }
+};
+
+/// Given a mangled function `Mangled`, replace all the primitive function type
+/// arguments of `Search` with type `Replace`.
+class TypeSubstitutor
+: public llvm::itanium_demangle::AbstractManglingParser {
+  const char *Written;
+  llvm::StringRef Search;
+  llvm::StringRef Replace;
+  llvm::SmallString<128> Result;
+  bool Substituted;
+
+  void reset(llvm::StringRef Mangled, llvm::StringRef Search,
+  llvm::StringRef Replace) {
+AbstractManglingParser::reset(Mangled.begin(), Mangled.end());
+Written = Mangled.begin();
+this->Search = Search;
+this->Replace = Replace;
+Result.clear();
+Substituted = false;
+  }
 
-static void Callback(void *context, const char *match) {
-  ((PrimitiveParmSubs *)context)->Substitute(llvm::StringRef(match));
-}
-  };
+public:
+  TypeSubstitutor() : AbstractManglingParser(nullptr, nullptr) {}
+
+  ConstString substitute(llvm::StringRef Mangled, llvm::StringRef From,
+llvm::StringRef To) {
+Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_LANGUAGE);
 
-  // The demangler

[Lldb-commits] [PATCH] D54074: CPlusPlusLanguage: Use new demangler API to implement type substitution

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 172764.
labath added a comment.

Thanks for the review. I have added some comments, and moved appending of the 
unmodified portion of the name into a separate function to aid readability.


https://reviews.llvm.org/D54074

Files:
  source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp

Index: unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
===
--- unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
+++ unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
@@ -184,4 +184,5 @@
   EXPECT_THAT(FindAlternate("_ZN1A1fEx"), Contains("_ZN1A1fEl"));
   EXPECT_THAT(FindAlternate("_ZN1A1fEy"), Contains("_ZN1A1fEm"));
   EXPECT_THAT(FindAlternate("_ZN1A1fEai"), Contains("_ZN1A1fEci"));
+  EXPECT_THAT(FindAlternate("_bogus"), IsEmpty());
 }
Index: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
===
--- source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -21,7 +21,7 @@
 
 // Other libraries and framework includes
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Demangle/Demangle.h"
+#include "llvm/Demangle/ItaniumDemangle.h"
 
 // Project includes
 #include "lldb/Core/PluginManager.h"
@@ -274,72 +274,89 @@
   return false;
 }
 
-/// Given a mangled function `mangled`, replace all the primitive function type
-/// arguments of `search` with type `replace`.
-static ConstString SubsPrimitiveParmItanium(llvm::StringRef mangled,
-llvm::StringRef search,
-llvm::StringRef replace) {
-  class PrimitiveParmSubs {
-llvm::StringRef mangled;
-llvm::StringRef search;
-llvm::StringRef replace;
-ptrdiff_t read_pos;
-std::string output;
-std::back_insert_iterator writer;
+namespace {
+class NodeAllocator {
+  llvm::BumpPtrAllocator Alloc;
 
-  public:
-PrimitiveParmSubs(llvm::StringRef m, llvm::StringRef s, llvm::StringRef r)
-: mangled(m), search(s), replace(r), read_pos(0),
-  writer(std::back_inserter(output)) {}
-
-void Substitute(llvm::StringRef tail) {
-  assert(tail.data() >= mangled.data() &&
- tail.data() < mangled.data() + mangled.size() &&
- "tail must point into range of mangled");
-
-  if (tail.startswith(search)) {
-auto reader = mangled.begin() + read_pos;
-ptrdiff_t read_len = tail.data() - (mangled.data() + read_pos);
-
-// First write the unmatched part of the original. Then write the
-// replacement string. Finally skip the search string in the original.
-writer = std::copy(reader, reader + read_len, writer);
-writer = std::copy(replace.begin(), replace.end(), writer);
-read_pos += read_len + search.size();
-  }
-}
+public:
+  void reset() { Alloc.Reset(); }
 
-ConstString Finalize() {
-  // If we did a substitution, write the remaining part of the original.
-  if (read_pos > 0) {
-writer = std::copy(mangled.begin() + read_pos, mangled.end(), writer);
-read_pos = mangled.size();
-  }
+  template  T *makeNode(Args &&... args) {
+return new (Alloc.Allocate(sizeof(T), alignof(T)))
+T(std::forward(args)...);
+  }
 
-  return ConstString(output);
-}
+  void *allocateNodeArray(size_t sz) {
+return Alloc.Allocate(sizeof(llvm::itanium_demangle::Node *) * sz,
+  alignof(llvm::itanium_demangle::Node *));
+  }
+};
+
+/// Given a mangled function `Mangled`, replace all the primitive function type
+/// arguments of `Search` with type `Replace`.
+class TypeSubstitutor
+: public llvm::itanium_demangle::AbstractManglingParser {
+  /// Input character until which we have constructed the respective output
+  /// already
+  const char *Written;
+
+  llvm::StringRef Search;
+  llvm::StringRef Replace;
+  llvm::SmallString<128> Result;
+
+  /// Whether we have performed any substitutions.
+  bool Substituted;
+
+  void reset(llvm::StringRef Mangled, llvm::StringRef Search,
+ llvm::StringRef Replace) {
+AbstractManglingParser::reset(Mangled.begin(), Mangled.end());
+Written = Mangled.begin();
+this->Search = Search;
+this->Replace = Replace;
+Result.clear();
+Substituted = false;
+  }
 
-static void Callback(void *context, const char *match) {
-  ((PrimitiveParmSubs *)context)->Substitute(llvm::StringRef(match));
+  void appendUnchangedInput() {
+Result += llvm::StringRef(Written, First - Written);
+Written = First;
+  }
+
+public:
+  TypeSubstitutor() : AbstractManglingParser(nullptr, nullptr) {}
+
+  ConstString substitute(llvm::StringRef Mangled, llvm::StringRef From,
+ llvm::StringRef To) {
+Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_LANG

[Lldb-commits] [PATCH] D54074: CPlusPlusLanguage: Use new demangler API to implement type substitution

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346233: CPlusPlusLanguage: Use new demangler API to 
implement type substitution (authored by labath, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D54074

Files:
  lldb/trunk/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/trunk/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp

Index: lldb/trunk/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
===
--- lldb/trunk/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
+++ lldb/trunk/unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp
@@ -191,4 +191,5 @@
   EXPECT_THAT(FindAlternate("_ZN1A1fEx"), Contains("_ZN1A1fEl"));
   EXPECT_THAT(FindAlternate("_ZN1A1fEy"), Contains("_ZN1A1fEm"));
   EXPECT_THAT(FindAlternate("_ZN1A1fEai"), Contains("_ZN1A1fEci"));
+  EXPECT_THAT(FindAlternate("_bogus"), IsEmpty());
 }
Index: lldb/trunk/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
===
--- lldb/trunk/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ lldb/trunk/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -21,7 +21,7 @@
 
 // Other libraries and framework includes
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Demangle/Demangle.h"
+#include "llvm/Demangle/ItaniumDemangle.h"
 
 // Project includes
 #include "lldb/Core/PluginManager.h"
@@ -279,72 +279,89 @@
   return false;
 }
 
-/// Given a mangled function `mangled`, replace all the primitive function type
-/// arguments of `search` with type `replace`.
-static ConstString SubsPrimitiveParmItanium(llvm::StringRef mangled,
-llvm::StringRef search,
-llvm::StringRef replace) {
-  class PrimitiveParmSubs {
-llvm::StringRef mangled;
-llvm::StringRef search;
-llvm::StringRef replace;
-ptrdiff_t read_pos;
-std::string output;
-std::back_insert_iterator writer;
+namespace {
+class NodeAllocator {
+  llvm::BumpPtrAllocator Alloc;
+
+public:
+  void reset() { Alloc.Reset(); }
+
+  template  T *makeNode(Args &&... args) {
+return new (Alloc.Allocate(sizeof(T), alignof(T)))
+T(std::forward(args)...);
+  }
 
-  public:
-PrimitiveParmSubs(llvm::StringRef m, llvm::StringRef s, llvm::StringRef r)
-: mangled(m), search(s), replace(r), read_pos(0),
-  writer(std::back_inserter(output)) {}
-
-void Substitute(llvm::StringRef tail) {
-  assert(tail.data() >= mangled.data() &&
- tail.data() < mangled.data() + mangled.size() &&
- "tail must point into range of mangled");
-
-  if (tail.startswith(search)) {
-auto reader = mangled.begin() + read_pos;
-ptrdiff_t read_len = tail.data() - (mangled.data() + read_pos);
-
-// First write the unmatched part of the original. Then write the
-// replacement string. Finally skip the search string in the original.
-writer = std::copy(reader, reader + read_len, writer);
-writer = std::copy(replace.begin(), replace.end(), writer);
-read_pos += read_len + search.size();
-  }
-}
+  void *allocateNodeArray(size_t sz) {
+return Alloc.Allocate(sizeof(llvm::itanium_demangle::Node *) * sz,
+  alignof(llvm::itanium_demangle::Node *));
+  }
+};
 
-ConstString Finalize() {
-  // If we did a substitution, write the remaining part of the original.
-  if (read_pos > 0) {
-writer = std::copy(mangled.begin() + read_pos, mangled.end(), writer);
-read_pos = mangled.size();
-  }
+/// Given a mangled function `Mangled`, replace all the primitive function type
+/// arguments of `Search` with type `Replace`.
+class TypeSubstitutor
+: public llvm::itanium_demangle::AbstractManglingParser {
+  /// Input character until which we have constructed the respective output
+  /// already
+  const char *Written;
+
+  llvm::StringRef Search;
+  llvm::StringRef Replace;
+  llvm::SmallString<128> Result;
+
+  /// Whether we have performed any substitutions.
+  bool Substituted;
+
+  void reset(llvm::StringRef Mangled, llvm::StringRef Search,
+ llvm::StringRef Replace) {
+AbstractManglingParser::reset(Mangled.begin(), Mangled.end());
+Written = Mangled.begin();
+this->Search = Search;
+this->Replace = Replace;
+Result.clear();
+Substituted = false;
+  }
 
-  return ConstString(output);
-}
+  void appendUnchangedInput() {
+Result += llvm::StringRef(Written, First - Written);
+Written = First;
+  }
+
+public:
+  TypeSubstitutor() : AbstractManglingParser(nullptr, nullptr) {}
 
-static void Callback(void *context, const char *match) {
-  ((PrimitiveParmSubs *)context)->Substitute(llvm::StringRef(match));
+  ConstString substitute(llvm::StringRef

[Lldb-commits] [PATCH] D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules()

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Could we reverse the dependencies between the two? I.e., first add the 
necessary functionality to lldb-test and then write the test using that? Or 
just merge that into a single patch?

Some of the information tested here is already available there (list of 
sections) other can be added easily (list of symbols).


https://reviews.llvm.org/D53094



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

[Lldb-commits] [PATCH] D54221: Add setting to require hardware breakpoints.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I recall something about linux on arm having a magic unmodifiable (even by 
ptrace) page of memory, so this could be useful there too. However, it's not 
clear to me how a user is going to figure out that he needs to enable this 
setting. Would it make sense to automatically try setting a hardware breakpoint 
if software breakpoint fails?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54221



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

[Lldb-commits] [PATCH] D54221: Add setting to require hardware breakpoints.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D54221#1290638, @JDevlieghere wrote:

> In https://reviews.llvm.org/D54221#1290572, @labath wrote:
>
> > I recall something about linux on arm having a magic unmodifiable (even by 
> > ptrace) page of memory, so this could be useful there too. However, it's 
> > not clear to me how a user is going to figure out that he needs to enable 
> > this setting. Would it make sense to automatically try setting a hardware 
> > breakpoint if software breakpoint fails?
>
>
> My main concern would be that hardware breakpoints are a limited resource and 
> not something we want to make transparent to the user, because it's only a 
> matter of time before it fails.


That is true, but on the other hand, you would only use hw breakpoints on those 
pieces of memory where you really need to instead of everywhere, which means 
(at least for the use case I have in mind) it be used very rarely. Of course, 
we would have to be careful do differentiate between reasons why setting a sw 
breakpoint failed (is it because the memory is RO, or some other reason like 
there is no memory at that address).

However, with this approach, it's still not clear to me how will the user know 
that he has to enable this setting? Will he get some sort of an error pointing 
here when the sw breakpoint fails? Or will you just enable this setting by 
default for targets where you know this is an issue?


https://reviews.llvm.org/D54221



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

[Lldb-commits] [PATCH] D54272: Extract construction of DataBufferLLVM into FileSystem

[Pavel Labath via Phabricator via lldb-commits]

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

Looks fine to me, I'd just ask you to consider shortening the method names a 
bit.




Comment at: include/lldb/Host/FileSystem.h:119-122
+  CreateDataBufferFromPath(const llvm::Twine &path, uint64_t size = 0,
+   uint64_t offset = 0);
+  std::shared_ptr
+  CreateDataBufferFromPath(const FileSpec &file_spec, uint64_t size = 0,

Can we streamline these names? I don't think the `FromPath` part brings 
anything here when the method is on the filesystem class, as it's completely 
natural and expected for those methods to take paths. Maybe just 
`CreateDataBuffer` or `ToDataBuffer`


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54272



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

[Lldb-commits] [PATCH] D54417: Fix a crash when parsing incorrect DWARF

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: clayborg, jankratochvil, JDevlieghere.

While parsing a childless compile unit DIE we could crash if the DIE was
followed by any extra data (such as a superfluous end-of-children
marker). This happened because the break-on-depth=0 check was performed
only when parsing the null DIE, which was not correct because with a
childless root DIE, we could reach the end of the unit without ever
encountering the null DIE.

If the compile unit contribution ended directly after the CU DIE,
everything would be fine as we would terminate parsing due to reaching
EOF. However, if the contribution contained extra data (perhaps a
superfluous end-of-children marker), we would crash because we would
treat that data as the begging of another compile unit.

This fixes the crash by moving the depth=0 check to a more generic
place, and also adds a regression test.


https://reviews.llvm.org/D54417

Files:
  lit/SymbolFile/DWARF/childless-compile-unit.s
  source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp


Index: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -244,9 +244,6 @@
 
   if (depth > 0)
 --depth;
-  if (depth == 0)
-break; // We are done with this compile unit!
-
   prev_die_had_children = false;
 } else {
   die_index_stack.back() = m_die_array.size() - 1;
@@ -258,6 +255,9 @@
   }
   prev_die_had_children = die_has_children;
 }
+
+if (depth == 0)
+  break; // We are done with this compile unit!
   }
 
   if (!m_die_array.empty()) {
Index: lit/SymbolFile/DWARF/childless-compile-unit.s
===
--- /dev/null
+++ lit/SymbolFile/DWARF/childless-compile-unit.s
@@ -0,0 +1,45 @@
+# Test that we don't crash when parsing slightly invalid DWARF.  The compile
+# unit in this file sets DW_CHILDREN_no, but it still includes an
+# end-of-children marker in its contribution.
+
+# RUN: llvm-mc -triple x86_64-pc-linux %s -filetype=obj > %t.o
+# RUN: lldb-test symbols %t.o
+
+   .section.debug_str,"MS",@progbits,1
+.Linfo_string0:
+   .asciz  "Hand-written DWARF"
+.Linfo_string1:
+   .asciz  "-"
+.Linfo_string2:
+   .asciz  "/tmp"
+
+   .section.debug_abbrev,"",@progbits
+   .byte   1   # Abbreviation Code
+   .byte   17  # DW_TAG_compile_unit
+   .byte   0   # DW_CHILDREN_no
+   .byte   37  # DW_AT_producer
+   .byte   14  # DW_FORM_strp
+   .byte   19  # DW_AT_language
+   .byte   5   # DW_FORM_data2
+   .byte   3   # DW_AT_name
+   .byte   14  # DW_FORM_strp
+   .byte   27  # DW_AT_comp_dir
+   .byte   14  # DW_FORM_strp
+   .byte   0   # EOM(1)
+   .byte   0   # EOM(2)
+   .byte   0   # EOM(3)
+
+   .section.debug_info,"",@progbits
+.Lcu_begin0:
+   .long   .Lcu_length_end-.Lcu_length_start # Length of Unit
+.Lcu_length_start:
+   .short  4   # DWARF version number
+   .long   .debug_abbrev   # Offset Into Abbrev. Section
+   .byte   8   # Address Size (in bytes)
+   .byte   1   # Abbrev [1] 0xb:0x30 
DW_TAG_compile_unit
+   .long   .Linfo_string0  # DW_AT_producer
+   .short  12  # DW_AT_language
+   .long   .Linfo_string1  # DW_AT_name
+   .long   .Linfo_string2  # DW_AT_comp_dir
+   .byte   0   # Bogus End Of Children Mark
+.Lcu_length_end:


Index: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -244,9 +244,6 @@
 
   if (depth > 0)
 --depth;
-  if (depth == 0)
-break; // We are done with this compile unit!
-
   prev_die_had_children = false;
 } else {
   die_index_stack.back() = m_die_array.size() - 1;
@@ -258,6 +255,9 @@
   }
   prev_die_had_children = die_has_children;
 }
+
+if (depth == 0)
+  break; // We are done with this compile unit!
   }
 
   if (!m_die_array.empty()) {
Index: lit/SymbolFile/DWARF/childless-compile-unit.s
===
--- /dev/null
+++ lit/SymbolFile/DWARF/childless-compile-unit.s
@@ -0,0 +1,45 @@
+# Test that we don't crash when parsing slightly invalid DWARF.  The compile
+# unit in this file sets DW_CHILDREN_no, but it still includes an
+# end-of-children 

[Lldb-commits] [PATCH] D54417: Fix a crash when parsing incorrect DWARF

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB346849: Fix a crash when parsing incorrect DWARF 
(authored by labath, committed by ).
Herald added a subscriber: abidh.

Changed prior to commit:
  https://reviews.llvm.org/D54417?vs=173634&id=174006#toc

Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54417

Files:
  lit/SymbolFile/DWARF/childless-compile-unit.s
  source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp


Index: lit/SymbolFile/DWARF/childless-compile-unit.s
===
--- lit/SymbolFile/DWARF/childless-compile-unit.s
+++ lit/SymbolFile/DWARF/childless-compile-unit.s
@@ -0,0 +1,45 @@
+# Test that we don't crash when parsing slightly invalid DWARF.  The compile
+# unit in this file sets DW_CHILDREN_no, but it still includes an
+# end-of-children marker in its contribution.
+
+# RUN: llvm-mc -triple x86_64-pc-linux %s -filetype=obj > %t.o
+# RUN: lldb-test symbols %t.o
+
+   .section.debug_str,"MS",@progbits,1
+.Linfo_string0:
+   .asciz  "Hand-written DWARF"
+.Linfo_string1:
+   .asciz  "-"
+.Linfo_string2:
+   .asciz  "/tmp"
+
+   .section.debug_abbrev,"",@progbits
+   .byte   1   # Abbreviation Code
+   .byte   17  # DW_TAG_compile_unit
+   .byte   0   # DW_CHILDREN_no
+   .byte   37  # DW_AT_producer
+   .byte   14  # DW_FORM_strp
+   .byte   19  # DW_AT_language
+   .byte   5   # DW_FORM_data2
+   .byte   3   # DW_AT_name
+   .byte   14  # DW_FORM_strp
+   .byte   27  # DW_AT_comp_dir
+   .byte   14  # DW_FORM_strp
+   .byte   0   # EOM(1)
+   .byte   0   # EOM(2)
+   .byte   0   # EOM(3)
+
+   .section.debug_info,"",@progbits
+.Lcu_begin0:
+   .long   .Lcu_length_end-.Lcu_length_start # Length of Unit
+.Lcu_length_start:
+   .short  4   # DWARF version number
+   .long   .debug_abbrev   # Offset Into Abbrev. Section
+   .byte   8   # Address Size (in bytes)
+   .byte   1   # Abbrev [1] 0xb:0x30 
DW_TAG_compile_unit
+   .long   .Linfo_string0  # DW_AT_producer
+   .short  12  # DW_AT_language
+   .long   .Linfo_string1  # DW_AT_name
+   .long   .Linfo_string2  # DW_AT_comp_dir
+   .byte   0   # Bogus End Of Children Mark
+.Lcu_length_end:
Index: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -244,9 +244,6 @@
 
   if (depth > 0)
 --depth;
-  if (depth == 0)
-break; // We are done with this compile unit!
-
   prev_die_had_children = false;
 } else {
   die_index_stack.back() = m_die_array.size() - 1;
@@ -258,6 +255,9 @@
   }
   prev_die_had_children = die_has_children;
 }
+
+if (depth == 0)
+  break; // We are done with this compile unit!
   }
 
   if (!m_die_array.empty()) {


Index: lit/SymbolFile/DWARF/childless-compile-unit.s
===
--- lit/SymbolFile/DWARF/childless-compile-unit.s
+++ lit/SymbolFile/DWARF/childless-compile-unit.s
@@ -0,0 +1,45 @@
+# Test that we don't crash when parsing slightly invalid DWARF.  The compile
+# unit in this file sets DW_CHILDREN_no, but it still includes an
+# end-of-children marker in its contribution.
+
+# RUN: llvm-mc -triple x86_64-pc-linux %s -filetype=obj > %t.o
+# RUN: lldb-test symbols %t.o
+
+	.section	.debug_str,"MS",@progbits,1
+.Linfo_string0:
+	.asciz	"Hand-written DWARF"
+.Linfo_string1:
+	.asciz	"-"
+.Linfo_string2:
+	.asciz	"/tmp"
+
+	.section	.debug_abbrev,"",@progbits
+	.byte	1   # Abbreviation Code
+	.byte	17  # DW_TAG_compile_unit
+	.byte	0   # DW_CHILDREN_no
+	.byte	37  # DW_AT_producer
+	.byte	14  # DW_FORM_strp
+	.byte	19  # DW_AT_language
+	.byte	5   # DW_FORM_data2
+	.byte	3   # DW_AT_name
+	.byte	14  # DW_FORM_strp
+	.byte	27  # DW_AT_comp_dir
+	.byte	14  # DW_FORM_strp
+	.byte	0   # EOM(1)
+	.byte	0   # EOM(2)
+	.byte	0   # EOM(3)
+
+	.section	.debug_info,"",@progbits
+.Lcu_begin0:
+	.long	.Lcu_length_end-.Lcu_length_start # Length of Unit
+.Lcu_length_start:
+	.short	4   # DWARF version number
+	.long	.debug_a

[Lldb-commits] [PATCH] D54476: [CMake] Streamline code signing for debugserver

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: tools/debugserver/source/CMakeLists.txt:101
+option(LLDB_NO_DEBUGSERVER "Delete debugserver after building it, and don't 
try to codesign it" OFF)
+option(LLDB_USE_SYSTEM_DEBUGSERVER "Neither build nor codesign debugserver. 
Use the system's debugserver instead (Darwin only)." OFF)
+

sgraenitz wrote:
> JDevlieghere wrote:
> > Should we emit and error if these variables are in an inconsistent state, 
> > e.g. when both are set? 
> Yes, could do that.
Alternatively, we could make this a single tri-state variable. 
`LLDB_DEBUGSERVER_MODE=OFF|SYSTEM|TREE` or something ?


https://reviews.llvm.org/D54476



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

[Lldb-commits] [PATCH] D54617: [wip][Reproducers] Add file provider

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I am confused by differing scopes of various objects that are interacting here. 
It seems you are implementing capture/replay as something that can be flicked 
on/off at any point during a debug session (`Debugger` lifetime). However, you 
are modifying global state (the filesystem singleton, at least), which is 
shared by all debug sessions. If multiple debug sessions try to enable 
capture/replay (or even access the filesystem concurrently, as none of this is 
synchronized in any way), things will break horribly. This seems like a bad 
starting point in implementing a feature, as I don't see any way to fix 
incrementally in the future.

Maybe you meant that by `Initialization of the FS needs to happen in the 
driver.`? In any case, for me the initialization part is the interesting/hard 
part of this problem. The details of how to collect the files are trivial 
compared to that and possibly depend on the result of the decisions made there.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54617



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

[Lldb-commits] [PATCH] D54682: [Driver] Extract option parsing and option processing.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

If by //"it would cause errors to no longer appear in the same order as 
specified by the user, but in an arbitrary order specified by the driver 
implementation"//, you mean that now for a command line like:

  $ new/lldb -S /tmp/sadg --file /tmp/qwr

I get

  error: file specified in --file (-f) option doesn't exist: '/tmp/qwr'

instead of

  error: file specified in --source (-s) option doesn't exist: '/tmp/sadg'

then that's something I really don't care about.

I have to agree that the new --help output is less readable than the old one, 
but that's a price I would be willing to pay for getting rid of getopt. And 
this can be easily improved in the future, either by rolling our own --help 
output (as we do now anyway), or by improving the central libOption help 
printer (thereby improving `clang --help` output as well, which I also find 
lacking in readability).

So yes, I'm in favour of the libOption approach, or at least cleaning this up 
by "sacrificing" the error message order.

(I also have to point out that for libOption to work, someone will have to dive 
into the xcode project to get the tablegen rules in there.)


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54682



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

[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: include/lldb/Utility/Reproducer.h:59-64
+  void InitializeFileInfo(llvm::StringRef name,
+  llvm::ArrayRef files) {
+m_info.name = name;
+for (auto file : files)
+  m_info.files.push_back(file);
+  }

Could this be replaced by additional arguments to the constructor (avoiding 
two-stage initialization)?



Comment at: include/lldb/Utility/Reproducer.h:99
+  template  T *Get() {
+auto it = m_providers.find(T::NAME);
+if (it == m_providers.end())

Is the value of the `NAME` used anywhere? If not, you could just have each 
class define a `char` variable (like `llvm::ErrorInfo` subclasses do) and then 
use its address as a key, making everything faster.

(Even if name is useful (for printing to the user or whatever), it might be 
worth to switch to using the char-address for lookup and then just have 
`getName` method for other things.)



Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:304
+ProcessGDBRemoteProvider *provider =
+g->GetOrCreate();
 // Set the history stream to the stream owned by the provider.

could `GetOrCreate` keep returning a reference (to avoid spurious `guaranteed 
to be not null` comments)?



Comment at: source/Utility/Reproducer.cpp:131-133
+  assert(m_providers.empty() &&
+ "Changing the root directory after providers have "
+ "been registered would invalidate the index.");

Given these limitations, would it be possible to set the root once in the 
constructor and keep it immutable since then?



Comment at: unittests/Utility/ReproducerTest.cpp:44-46
+auto error = reproducer.SetReplay(FileSpec("/bogus/path"));
+EXPECT_TRUE(static_cast(error));
+llvm::consumeError(std::move(error));

Shorter and with better error messages:
`EXPECT_THAT_ERROR(reproducer.SetReplay(FileSpec("/bogus/path")), 
llvm::Succeeded());`



Comment at: unittests/Utility/ReproducerTest.cpp:79-81
+auto error = reproducer.SetCapture(FileSpec("/bogus/path"));
+EXPECT_TRUE(static_cast(error));
+llvm::consumeError(std::move(error));

`EXPECT_THAT_ERROR(..., llvm::Failed());`


https://reviews.llvm.org/D54616



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

[Lldb-commits] [PATCH] D54617: [wip][Reproducers] Add file provider

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D54617#1302376, @JDevlieghere wrote:

> In https://reviews.llvm.org/D54617#1302366, @labath wrote:
>
> > I am confused by differing scopes of various objects that are interacting 
> > here. It seems you are implementing capture/replay as something that can be 
> > flicked on/off at any point during a debug session (`Debugger` lifetime). 
> > However, you are modifying global state (the filesystem singleton, at 
> > least), which is shared by all debug sessions. If multiple debug sessions 
> > try to enable capture/replay (or even access the filesystem concurrently, 
> > as none of this is synchronized in any way), things will break horribly. 
> > This seems like a bad starting point in implementing a feature, as I don't 
> > see any way to fix incrementally in the future.
>
>
> Maybe I should've added a comment (I didn't want to litter the code with 
> design goals) but the goal of the commands is not really to enable/disable 
> the reproducer feature. I'm currently abusing the commands to do that, but 
> the long term goal is that you can enable/disable part of the reproducer. For 
> example, you could say only capture GDB packets or capture only files. It 
> makes testing a lot easier if you can isolate a single source of information. 
> Since there's currently only GDB packets I didn't split it up (yet).
>
> Also I piggy-backed off this to enable/disable the feature as a whole because 
> I needed to go through the debugger. See my reply below on why and how 
> that'll be fixed by the next patch :-)


Ok, that makes sense, thanks for the explanation. I guess that means that 
filesystem capturing (due to it being global) will then be one of the things 
that cannot be enabled/disabled at runtime ?




Comment at: source/Host/common/FileSystem.cpp:71-74
+  llvm::ErrorOr> buffer =
+  m_fs->getBufferForFile(mapping);
+  if (!buffer)
+return;

Do you want to propagate this error up the stack?



Comment at: source/Utility/FileCollector.cpp:31-32
+  // default.
+  for (auto &C : path)
+upper_dest.push_back(toUpper(C));
+  if (sys::fs::real_path(upper_dest, real_dest) && path.equals(real_dest))

`upper_dest = path.upper();` ?



Comment at: source/Utility/FileCollector.cpp:82-87
+  // Remove redundant leading "./" pieces and consecutive separators.
+  absolute_src = sys::path::remove_leading_dotslash(absolute_src);
+
+  // Canonicalize the source path by removing "..", "." components.
+  SmallString<256> virtual_path = absolute_src;
+  sys::path::remove_dots(virtual_path, /*remove_dot_dot=*/true);

Is there anything which `remove_leading_dotslash` does, which wouldn't be done 
by the latter `remove_dots` call?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54617



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

[Lldb-commits] [PATCH] D54731: [lit] Enable the use of custom user-defined lit commands

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think that something like this would go a long way towards solving the 
problems with lit tests we're having in lldb.

However, the part that is not clear to me is whether it is actually necessary 
to modify lit (shtest) to achieve this. It seems to me an equivalent effect to 
the command from the motivating example could be achieved via something like

  RUN: %compile --source=%p/Inputs/foo.cpp --mode=debug --opt=none --link=no 
--output=%t.o --clean=yes

where `%compile` expands to a python script living somewhere in the lldb 
repository. This script could do the same thing that the implementation of 
`COMPILE: ` would do, except it would be done in a separate process.

The only downside of that I see is the extra process will incur some overhead, 
slowing down the testing, but I am not sure if we care about that (or if it 
would even be measurable). OTOH, the benefits are:

- decreased complexity of lit
- decreased level of surprise of developers seeing new lit commands
- easier reproducibility of tests when debugging (just copy paste the 
`%compile` run-line to rebuild the executable)


https://reviews.llvm.org/D54731



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

[Lldb-commits] [PATCH] D54731: [lit] Enable the use of custom user-defined lit commands

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'd go with the "conservative" approach first. The idea of having lldb loaded 
inside a lit process does not excite me. One of the problems we have with 
dotest is that when lldb crashes during the test, it takes a part of the test 
driver with it which causes some tests to be skipped and the complicates the 
reporting of the result of the crashed test. It's not as bad right now, as 
there is still the main process left to report some kind of an error (back in 
the days when tests were run sequentially in a single process, the entire test 
suite would just stop), but I still think it would be nice to avoid these 
issues in the new framework.


https://reviews.llvm.org/D54731



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

[Lldb-commits] [PATCH] D54843: [Expr] Check the language before ignoring Objective C keywords

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: 
source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp:401-404
-// FIXME: the following language option is a temporary workaround,
-// to "ask for C++, get ObjC++".  Apple hopes to remove this requirement on
-// non-Apple platforms, but for now it is needed.
-m_compiler->getLangOpts().ObjC = true;

aleksandr.urakov wrote:
> Is it still necessary? We can do here something like `#ifdef __APPLE__`, but 
> then the test will fail on Apple platforms. Can we somehow specify a platform 
> requirement in the test?
Instead of `ifdef` it would be better to do a 
`target->GetArchitecture().getTriple().getVendor() == llvm::Triple::Apple` and 
then `@skipIfDarwin` in the test. But it would be certainly better to remove 
this altogether :)


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D54843



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

[Lldb-commits] [PATCH] D54863: [ASTImporter] Set MustBuildLookupTable on PrimaryContext

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a reviewer: clayborg.
labath added a comment.

The change looks pretty safe to me. Adding Greg in case he has any concerns.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D54863



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

[Lldb-commits] [PATCH] D28305: [Host] Handle short reads and writes, take 3

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This is a stale patch which I am abandoning. I am just cleaning up my queue.


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

https://reviews.llvm.org/D28305



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

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

> There’s actually been a slow push away from cl::opt. It’s less flexible
>  and doesn’t support some things that the TableGen approach does.
>  Recently there’s been a few efforts to port existing tools onto TableGen
>  options from cl::opt.
> 
> I don’t think cl::opt is going away anytime soon so if it works I don’t
>  have a strong opinion, but it’s kinda nice to standardize on “the one
>  true method” if that’s the direction things are heading anyway

Another reason for using libOption is that it is also usable as a parser for 
the lldb command line, whereas cl::opt is definitely not (it uses global 
variables). And there's value in consistency between the lldb driver and the 
built-in command line.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D54692



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

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D54692#1308207 , @zturner wrote:

> In D54692#1308190 , @labath wrote:
>
> > Another reason for using libOption is that it is also usable as a parser 
> > for the lldb command line, whereas cl::opt is definitely not (it uses 
> > global variables). And there's value in consistency between the lldb driver 
> > and the built-in command line.
>
>
> This is true too.  Although I believe libOption doesn't support subcommands, 
> which would be required in order to use it for the interactive lldb command 
> line, but again, there would be value in adding that to libOption outside of 
> llvm (cl::opt supports it, so it's required in order to port some remaining 
> llvm tools to libOption)


Adding subcommands is one way. Another would be to simply keep the existing 
subcommand-parsing code (which we  already have, as getopt doesn't support that 
either), and just replace the getopt part with libOption.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D54692



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

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: tools/driver/Driver.cpp:876-888
+  usage << indent << tool_name
+<< " -a  -f  [-c ] [-s ] [-o "
+   "] [-S ] [-O ] [-k ] [-K ] "
+   "[-Q] [-b] [-e] [-x] [-X] [-l ] [-d] [-z "
+   "] [[--]  [ ...]]\n";
+  usage << indent << tool_name
+<< " -n  -w [-s ] [-o ] [-S "

I am not entirely thrilled by the hard-coding of the option combinations here. 
It sounds like the kind of thing that will invariably get out of sync (I think 
it would be better to just not have it). Instead of trying to exhaustively list 
all possible option combinations (which I generally find too long to make sense 
of), and still getting it wrong (Why is there a `[[--]  
[ ...]]` after `-v` in the second option form?), maybe it would 
be better to just output a short prose here explaining the general principle. 
Maybe something like "LLDB can be started in several modes. Passing an 
executable as a positional arguments prepares lldb to debug the given 
executable. Using one of the attach options causes lldb to immediately attach 
to the given process. Command options can be combined with either mode and 
cause lldb to run the specified commands before starting the interactive shell. 
Using --repl starts lldb in REPL mode. etc."

However, if everyone is happy with the current approach then I won't stand in 
the way..


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

https://reviews.llvm.org/D54692



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

[Lldb-commits] [PATCH] D54914: Add a generic build script for building test inferiors

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I didn't look at the code in detail, as most of it deals with windows stuff, 
and I don't know much about those anyway. However, the interesting question for 
me would be how to make this useful for cross-compiling. Right now that sort of 
works for NativePDB tests because --compiler=clang-cl implies windows, but that 
won't help if I want to compile say a linux arm64 binary. I think that instead 
--arch, we should have a `--triple` argument, which specifies the exact target 
you want to build for. That can default to "host", and we can have special 
pseudo-triples like "host32" and "host64", if we want to be able to say "I want 
to build for a 32-bit flavour of the host arch". That could also make gcc 
detection easier, since you could just search for `$triple-gcc`.

Another route to take might be to say that this script only supports building 
for the host, and tests that want to target a specific architecture can just 
invoke the appropriate compiler directly -- since they know the target arch, 
they also know the correct compiler option syntax. Plus, these kinds of tests 
are the ones that are most likely to need fancy compiler command line switches 
that might be hard to represent generically. In this world, the NativePDB tests 
would continue to invoke %clang-cl (which would point to the build dir without 
any fancy logic), and build.py would be used by scripts that just want to build 
an executable for the host, and don't care much about the details of how this 
is accomplished (the stop-hook tests are a good example of this).

(Among the cosmetic things, I'd suggest to make `--source` a positional 
argument and enable spelling of `--output` as `-o`, just to make things 
shorter.)


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

https://reviews.llvm.org/D54914



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

[Lldb-commits] [PATCH] D53759: [PDB] Support PDB-backed expressions evaluation

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: source/Core/RichManglingContext.cpp:134-135
 get(m_cxx_method_parser)->GetBasename();
+if (!m_buffer.data())
+  m_buffer = llvm::StringRef("", 0);
 return;

Why is this necessary? It looks like somebody is misusing the returned 
StringRef by assuming that it always points to at least a single valid byte 
(which is definitely not the case in general, even for StringRefs with a 
non-null `data()`).

It would be better to fix the caller instead.


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

https://reviews.llvm.org/D53759



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

[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I haven't been following the reproducer work in detail, but this seems 
reasonable to me. Thanks for incorporating my drive-by suggestions.




Comment at: include/lldb/Utility/Reproducer.h:104
+  template  T *Create() {
+std::unique_ptr provider(new T(m_root));
+return static_cast(Register(std::move(provider)));

You should still be able to use make_unique here.



Comment at: include/lldb/Utility/Reproducer.h:134
 
-  std::vector> m_providers;
+  /// List of providers indexed by their name for easy access.
+  llvm::DenseMap> m_providers;

Out of date comment.


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

https://reviews.llvm.org/D54616



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

[Lldb-commits] [PATCH] D54914: Add a generic build script for building test inferiors

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D54914#1310337 , @zturner wrote:

> In D54914#1309700 , @labath wrote:
>
> > I didn't look at the code in detail, as most of it deals with windows 
> > stuff, and I don't know much about those anyway. However, the interesting 
> > question for me would be how to make this useful for cross-compiling. Right 
> > now that sort of works for NativePDB tests because --compiler=clang-cl 
> > implies windows, but that won't help if I want to compile say a linux arm64 
> > binary. I think that instead --arch, we should have a `--triple` argument, 
> > which specifies the exact target you want to build for. That can default to 
> > "host", and we can have special pseudo-triples like "host32" and "host64", 
> > if we want to be able to say "I want to build for a 32-bit flavour of the 
> > host arch". That could also make gcc detection easier, since you could just 
> > search for `$triple-gcc`.
>
>
> A triple is one way.  I don't know much about gcc, does it support the same 
> triple format as clang?


Gcc is different that clang in that the target triple is baked into the binary. 
For example, on my machine, I have a binary called `x86_64-pc-linux-gnu-gcc` 
(`gcc` is just a symlink to that), which always produces binaries for x86_64 
linux. If I had a arm64 cross-gcc installed, it would be called something like 
`aarch64-linux-gnu-gcc` and so on. So the actual mechanism to search for the 
compiler and invoke it will be different, but a triple is still a good starting 
point.

> Also, are there any existing use cases for specifying a triple in the lit 
> test suite?

You can take a look at the tests in `lit/SymbolFile/DWARF`. Their purpose is 
very similar to the NativePDB tests -- check the operation of the dwarf parser, 
regardless of the host platform. Since mac uses a somewhat different flavour of 
dwarf than  other platforms, the sources are usually compiled twice, targetting 
linux and mac.

>   I do agree we will need the ability to support triple specification, but it 
> seems better to do this as a followup.  I don't think it should be too hard 
> as long as we can make some assumptions such as "specification of --triple 
> limits the possible supported compilers", etc.

I don't think there needs to be much code initially, but I think it would be 
good to decide on the direction up front. For example, if we decide to go the 
triple way then we could avoid the churn in the tests by making all of them use 
`--triple=x86_64-pc-windows` instead of the current `--arch=64`, even if the 
actual implementation simply aborts in the more complicated cases.


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

https://reviews.llvm.org/D54914



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

[Lldb-commits] [PATCH] D54914: Add a generic build script for building test inferiors

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D54914#1309901 , @aprantl wrote:

> I would like to ask a general question that I (indirectly) also asked in 
> D54731 : Why do we want to implement support 
> for building inferiors in LIT-based tests? IMHO, if we need to support for 
> dealing with specific compilers, we should implement that once in 
> `Makefile.rules` (which is in a declarative domain-specific-language for 
> describing build logic) and write a `dotest.py`-style test instead. I'm 
> assuming here that we need the support in `Makefile.rules` anyway to support 
> the bulk of the tests. Having this support in two places increases the 
> maintenance effort and cognitive load.


While I agree that having two ways to do something is bad, I have to say that 
the current Makefile.rules system is far from perfect. First of, it is geared 
towards creating a fully working executables (the kind that dotest tests 
expect), and if you want to do something more exotic, things get a lot harder. 
Also writing more complex logic in makefiles is hard, particularly when you 
need to be portable across various shells and make implementations.  In fact, 
the biggest feature of make, automatic dependency tracking, is unused now that 
we build tests out of tree and clean by nuking the build dir.


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

https://reviews.llvm.org/D54914



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

[Lldb-commits] [PATCH] D55038: [Reproducers] Change how reproducers are initialized.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I have a lot of comments, but I think they're mostly cosmetic. I think the 
general idea here is good.




Comment at: include/lldb/API/SBInitializerOptions.h:15
+#include "lldb/API/SBFileSpec.h"
+#include "lldb/Initialization/SystemInitializer.h"
+

You cannot include non-API headers from SB headers (cpp files are fine). 
`SystemInitializer::Options` needs to forward-declared. (I guess that means 
moving it out of the SystemInitializer class).



Comment at: include/lldb/API/SBInitializerOptions.h:17
+
+#include 
+

not needed?



Comment at: include/lldb/API/SBInitializerOptions.h:32
+
+  mutable std::unique_ptr 
m_opaque_up;
+};

I think this will make the copy-constructor of this class deleted, which will 
not play well with python. I think you'll need to declare your own copy 
operations here (something like SBExpressionOptions, I guess).

Also, why is this `mutable`? SBExpressionOptions seems to have that too, but 
it's not clear to me why would that be needed?



Comment at: include/lldb/Initialization/SystemInitializer.h:28
+
+  virtual void Initialize(Options options) = 0;
   virtual void Terminate() = 0;

`const Options &` ? (it contains a string so it's not trivial to copy).



Comment at: lit/Reproducer/TestGDBRemoteRepro.test:7
+
+# RUN: %clang %S/Inputs/simple.c -g -o %t.out --target=x86_64-apple-macosx
+# RUN: %lldb -x -b -s %S/Inputs/GDBRemoteCapture.in --capture %T/reproducer -- 
%t.out | FileCheck %s --check-prefix CHECK --check-prefix REPLAY

With  `--target` this test will not run on other platforms. This sounds like it 
should work on platforms using gdb-remote (i.e., mac & linux). Maybe remove the 
--target and set an appropriate REQUIRES directive.



Comment at: lit/Reproducer/TestGDBRemoteRepro.test:8
+# RUN: %clang %S/Inputs/simple.c -g -o %t.out --target=x86_64-apple-macosx
+# RUN: %lldb -x -b -s %S/Inputs/GDBRemoteCapture.in --capture %T/reproducer -- 
%t.out | FileCheck %s --check-prefix CHECK --check-prefix REPLAY
+# RUN: %lldb -x -b -s %S/Inputs/GDBRemoteReplay.in --replay %T/reproducer -- 
%t.out | FileCheck %s --check-prefix CHECK --check-prefix REPLAY

`s/REPLAY/CAPTURE` ?



Comment at: scripts/interface/SBInitializerOptions.i:15
+public:
+SBInitializerOptions ();
+

Weird formatting.



Comment at: source/Initialization/SystemInitializerCommon.cpp:74
+
+  Reproducer::Initialize(mode, FileSpec(options.reproducer_path.c_str()));
   FileSystem::Initialize();

remove `.c_str()`



Comment at: source/Utility/Reproducer.cpp:41-42
+Reproducer::Reproducer(ReproducerMode mode, llvm::Optional root) {
+  // It's unfortunate that we have to do so much I/O here that can fail. The
+  // best we can do is not initialize the reproducer.
+  switch (mode) {

It should be possible to bubble this all the way up to the 
`SBDebugger::Initialize` call. Is there a reason to not do that?



Comment at: tools/driver/CMakeLists.txt:21
 liblldb
+lldbUtility
 ${host_lib}

This is wrong. The driver should only  use the public API exposed via liblldb. 
Why did you need to do that?



Comment at: unittests/Utility/ReproducerTest.cpp:39-43
+  llvm::Error SetCapture(bool b) { return Reproducer::SetCapture(b); }
+
+  llvm::Error SetReplay(llvm::Optional root) {
+return Reproducer::SetReplay(root);
+  }

You should be able to change the visibility of these via `using` directives. 
(`using Reproducer::SetCapture`).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55038



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

[Lldb-commits] [PATCH] D54914: Add a generic build script for building test inferiors

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D54914#1311492 , @zturner wrote:

> I think it would be good to use the way dotest works as a starting point.  
> You can specify --arch, and then you can run the test on multiple arches this 
> way by running dotest several times in succession, each with different arch 
> flags.
>
> I think a --triple option could be useful in limited scenarios, but I think 
> that most of the use cases will not need it.  Most tests will probably want 
> to specify nothing at all and let the lit configuration pass the correct 
> information through.  Actually, I think this is the same with the --arch flag 
> though.  Because as soon as you specify something, then it limits the ability 
> of the test to run over and over with different parameters.


I am not sure about their relative ratio, but yes, I agree that we have (or 
want to have) two kinds of tests. One would specify the target and the other 
would not.

> Perhaps we could support *both* a --triple and a --arch flag.  Maybe we can 
> make the script error if they're both specified.  This would give each test 
> the capability to be written in the simplest way possible.

Yes, I suppose that would work


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

https://reviews.llvm.org/D54914



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

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I've recently started looking at adding a new symbol file format (breakpad 
symbols). While researching the best way to achieve that, I started comparing 
the operation of PDB and DWARF symbol files. I noticed a very important 
difference there, and I think that is the cause of our problems here. In the 
DWARF implementation, a symbol file is an overlay on top of an object file - it 
takes the data contained by the object file and presents it in a more 
structured way.

However, that is not the case with PDB (both implementations). These take the 
debug information from a completely different file, which is not backed by an 
ObjectFile instance, and then present that. Since the SymbolFile interface 
requires them to be backed by an object file, they both pretend they are backed 
by the original EXE file, but in reality the data comes from elsewhere.

If we had an ObjectFilePDB (which not also not ideal, though in a way it is a 
better fit to the current lldb organization), then this could expose the PDB 
symtab via the existing ObjectFile interface and we could reuse the existing 
mechanism for merging symtabs from two object files.

I am asking this because now I am facing a choice in how to implement breakpad 
symbols. I could go the PDB way, and read the symbols without an intervening 
object file, or I could create an ObjectFileBreakpad and then (possibly) a 
SymbolFileBreakpad sitting on top of that.

The drawbacks of the PDB approach I see are:

- I lose the ability to do matching of the (real) object file via symbol 
vendors. The PDB symbol files now basically implement their own little symbol 
vendors inside them, which is mostly fine if you just need to find the PDB next 
to the exe file. However, things could get a bit messy if you wanted to 
implement some more complex searching on multiple paths, or downloading them 
from the internet.
- I'll hit issues when attempting to unwind (which is the real meat of the 
breakpad symbols), because unwind info is currently provided via the ObjectFile 
interface (ObjectFile::GetUnwindTable).

The drawbacks of the ObjectFile approach are:

- more code  - it needs a new ObjectFile and a new SymbolFile class (possibly 
also a SymbolVendor)
- it will probably look a bit weird because Breakpad files (and PDBs) aren't 
really object files

I'd like to hear your thoughts on this, if you have any.


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

https://reviews.llvm.org/D53368



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

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D53368#1313145 , @zturner wrote:

> In D53368#1313124 , @labath wrote:
>
> > I've recently started looking at adding a new symbol file format (breakpad 
> > symbols). While researching the best way to achieve that, I started 
> > comparing the operation of PDB and DWARF symbol files. I noticed a very 
> > important difference there, and I think that is the cause of our problems 
> > here. In the DWARF implementation, a symbol file is an overlay on top of an 
> > object file - it takes the data contained by the object file and presents 
> > it in a more structured way.
> >
> > However, that is not the case with PDB (both implementations). These take 
> > the debug information from a completely different file, which is not backed 
> > by an ObjectFile instance, and then present that. Since the SymbolFile 
> > interface requires them to be backed by an object file, they both pretend 
> > they are backed by the original EXE file, but in reality the data comes 
> > from elsewhere.
>
>
> Don't DWARF DWP files work this way as well?  How is support for this 
> implemented in LLDB?


There are some similarities, but DWP is a bit different. The main difference is 
that the DWP file is still an ELF (or whatever) file, so we still have a 
ObjectFile sitting below the symbol file. The other difference is that in case 
of DWP we still have a significant chunk of debug information present in the 
main executable (mainly various offsets that need to be applied to the unlinked 
debug info in the dwo/dwp files), so you can still very well say that the 
symbol file is reading information from the main executable. What DWARF does in 
this case is it creates a main SymbolFileDWARF for reading data from the main 
object file, and then a bunch of inner SymbolFileDWARFDwo/Dwp instances which 
read data from the other files. There are plenty of things to not like here as 
well, but at least this maintains the property that each symbol file sits on 
top of the object file from which it reads the data from. (and symtab doesn't 
go into the dwp file, so there are no issues with that).

>> I am asking this because now I am facing a choice in how to implement 
>> breakpad symbols. I could go the PDB way, and read the symbols without an 
>> intervening object file, or I could create an ObjectFileBreakpad and then 
>> (possibly) a SymbolFileBreakpad sitting on top of that.
> 
> What if `SymbolFile` interface provided a new method such as `GetSymtab()` 
> while `ObjectFile` provides a method called `HasExternalSymtab()`.  When you 
> call `ObjectFilePECOFF::GetSymtab()`, it could first check if 
> `HasExternalSymtab()` is true, and if so it could call the SymbolFile plugin 
> and return that

I don't think this would be good because there's no way for the PECOFF file to 
know if we will have a PDB file on top of it. If we don't find the pdb file, 
then the best we can do is use the list of external symbols as the symtab for 
the PECOFF file. I think a better way would ask the SymbolFile for the symtab. 
Then the symbol file can either return it's own symtab, or just forward the 
symtab from the object file (we already have a SymbolFileSymtab for cases when 
we have no debug info). That is more-or-less what this patch is doing, except 
that here the SymbolFile is inserting it's own symbols into the symtab created 
by the object file.


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

https://reviews.llvm.org/D53368



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

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

> Great observations Pavel! I think it's really important to have 
>  orthogonal/composable abstractions here: the symbols should be decoupled 
>  from the container format IMO.
> 
> You know more about the ObjectFile than me so I can't say if 
>  ObjectFileBreakpad is the best interface, but here are my initial 
>  observations (in a somewhat random order):

Even though it doesn't sound like that, ironically, Breakpad might be now 
better of as an ObjectFile rather than a SymbolFile. The main three pieces of 
information contained in breakpad files are:

- list of symbols
- unwind information
- line tables

Of these, the first two are presently vended by object files, and only the line 
table is done by symbol files. The auxiliary pieces of information in the 
breakpad files (architecture, OS, UUID), are also a property of object files in 
lldb.

> 1. We need clear and separate abstractions for a container (ELF, PE file, 
> Breakpad symbols) vs. the content (debug Information).

I agree, unfortunately I think we're quite far from that now. This is 
complicated by the fact that different "symbol file" formats have different 
notion of what "symbols" are. E.g. it's obvious that Symtab ended up being 
vended by the object files because that's how elf+macho/dwarf do things, but 
that's not the case for pecoff/pdb.

> 2. We need to be able to consume symbols when the corresponding module binary 
> is not available. This is common for postmortem debugging (ex. having a 
> minidump + PDBs, but not all the .DLLs or EXE files).

This is also going to be a bit tricky, though slightly orthogonal requirement. 
Right now things generally assume that a Module has an ObjectFile to read the 
data from, even though ProcessMinidump tries to work around that with special 
kinds of Modules. That might be enough to make addresses resolve somewhat 
reasonably, but I'm not sure what will happen once we start providing symbols 
for those kinds of modules.

> 
> 
> 3. I'm not a fan of the PDB model, where the symbols are searched in both the 
> symtabs then in the symbol files. I'd rather like to see the symtab an 
> interface for symbols regardless of where they come from. (Zach expressed 
> some efficiency concerns if we'd need to build a symtab from a PDB for 
> example as opposed to accessing the PDB symfile directly, although I think we 
> can design to address this - ie. multiple concrete symtab implementations, 
> some of which are *internally* aware of the source container, but w/o leaking 
> this through the interface)

I am afraid I am a bit lost here, as I don't know much about PDBs. I'll have to 
study this in more detail.

> 
> 
> 4. The symbol vendor observation is very important. Right now LLDB has basic 
> support for looking up DWARF symbols and none for PDBs. (for example, IMO 
> LLDB could greatly benefit from something like Microsoft's symsrv - I'm 
> actually planning to look into it soon) (Whatever we do, this should be one 
> of the key requirements IMO)

agreed.


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

https://reviews.llvm.org/D53368



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

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

On 29/11/2018 21:29, Leonard Mosescu wrote:

> Hi Aleksandr, yes, no objections to this patch.
> 
> I was responding to Pavel's comments, which I also assume are 
>  forward-looking as well, not strictly related to this patch.

Agreed, and I apologise for hijacking your review (I seem to be getting in the 
habit of that). I initially thought that having a ObjectFilePDB might mean that 
this patch would not be needed, but now I am slowly growing to like it. I think 
it makes sense for a symbol file to be able to provide additional symbols 
regardless of whether this could be done differently too.


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

https://reviews.llvm.org/D53368



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

[Lldb-commits] [PATCH] D55038: [Reproducers] Change how reproducers are initialized.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D55038#1314026 , @JDevlieghere 
wrote:

> Test didn't run. Is there a way to REQUIRE either darwin or linux?


I think the canonical way to do that would be to define a new feature in lit, 
which gets set when the target supports remote debugging and then use that 
feature in the REQUIRES directive.




Comment at: source/Utility/Reproducer.cpp:41-42
+Reproducer::Reproducer(ReproducerMode mode, llvm::Optional root) {
+  // It's unfortunate that we have to do so much I/O here that can fail. The
+  // best we can do is not initialize the reproducer.
+  switch (mode) {

JDevlieghere wrote:
> labath wrote:
> > It should be possible to bubble this all the way up to the 
> > `SBDebugger::Initialize` call. Is there a reason to not do that?
> Do you mean having the private Initialize function return an error (and an 
> SBError for the SB variant)?
Yes, that's what I meant. Up until now, our initialization functions were 
mostly just hooking up various pointers, which are things that can't 
(shouldn't) fail, but if now a simple typo in the reproducer path can cause the 
initialization to fail, then I think it would be good to report that.


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

https://reviews.llvm.org/D55038



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

[Lldb-commits] [PATCH] D55122: [PDB] Fix location retrieval for function local variables and arguments that are stored relative to VFRAME

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I like how you've separated out the conversion function doing the actual 
conversion. That should make it easy to write unit tests for it (including 
tests for malformed input). Can you add something like that? I am particularly 
interested in what will the merging code do when it encounters reference loops.

However, I find the usage of shared_ptr in the parsing code overkill. Surely we 
can come up with something better when the lifetime of all of the objects is 
local to the translation function. I think the most llvm-y solution would be to 
use a BumpPtrAllocator to create all nodes, and then just dispose of that when 
the function returns. (this assumes the nodes are trivially destructible, which 
it looks like they will be after they stop storing shared_ptrs).




Comment at: source/Plugins/SymbolFile/PDB/PDBFPOProgramToDWARFExpression.cpp:203
+private:
+  const std::map &m_dependent_programs;
+};

use `llvm::DenseMap` ?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55122



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

[Lldb-commits] [PATCH] D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules()

[Pavel Labath via Phabricator via lldb-commits]

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

Thanks for adding the test. looks good to me.




Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h:289-291
+  mutable std::unique_ptr m_filespec_ap;
+  typedef llvm::object::OwningBinary OWNBINType;
+  mutable std::unique_ptr m_owningbin_up;

Consider using `Optional` instead of `unique_ptr` to reduce pointer chasing.

Also, do these have to be `mutable`? I was expecting that you needed to 
lazy-initialize them from some `const` member, but I couldn't find any evidence 
of that...


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

https://reviews.llvm.org/D53094



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

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I don't see any tests :(.

Also, the three bullet points in the description sound like rather independent 
pieces of functionality. Would it be possible to split them up into separate 
patches? That would make things easier to review, particularly for those who 
don't look at this code very often :).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55142



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

[Lldb-commits] [PATCH] D55038: [Reproducers] Change how reproducers are initialized.

[Pavel Labath via Phabricator via lldb-commits]

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

In D55038#1314968 , @JDevlieghere 
wrote:

> In D55038#1314336 , @labath wrote:
>
> > I think the canonical way to do that would be to define a new feature in 
> > lit, which gets set when the target supports remote debugging and then use 
> > that feature in the REQUIRES directive.
>
>
> I've changed the check to non-windows (as it wouldn't compile there anyway). 
> Is there anything not covered by this that would warrant this new feature?


Well.. FreeBSD also uses a local debugging plugin, though I don't believe 
anyone runs tests there on a regular basis. I suppose that could be handled by 
adding `system-freebsd` into the `UNSUPPORTED` clause...

Looks good to me, modulo one comment.




Comment at: source/Initialization/SystemLifetimeManager.cpp:27
 
-void SystemLifetimeManager::Initialize(
+llvm::Error SystemLifetimeManager::Initialize(
 std::unique_ptr initializer,

The calls to this function in lldb-server and lldb-test need to be updated to 
handle the newly returned Error object.


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

https://reviews.llvm.org/D55038



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

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: clayborg, zturner, lemo, amccarth.
Herald added subscribers: fedor.sergeev, mgorny.

This patch adds the scaffolding necessary for lldb to recognise symbol
files generated by breakpad. These (textual) files contain just enough
information to be able to produce a backtrace from a crash
dump. This information includes:

- UUID, architecture and name of the module
- line tables
- list of symbols
- unwind information

A minimal breakpad file could look like this:
MODULE Linux x86_64 24B5D199F0F766FF5DC30 a.out
INFO CODE_ID B52499D1F0F766FF5DC3
FILE 0 /tmp/a.c
FUNC 1010 10 0 _start
1010 4 4 0
1014 5 5 0
1019 5 6 0
101e 2 7 0
PUBLIC 1010 0 _start
STACK CFI INIT 1010 10 .cfa: $rsp 8 + .ra: .cfa -8 + ^
STACK CFI 1011 $rbp: .cfa -16 + ^ .cfa: $rsp 16 +
STACK CFI 1014 .cfa: $rbp 16 +

Even though this data would normally be considered "symbol" information,
in the current lldb infrastructure it is assumed every SymbolFile object
is backed by an ObjectFile instance. So, in order to better interoperate
with the rest of the code (particularly symbol vendors).

In this patch I just parse the breakpad header, which is enough to
populate the UUID and architecture fields of the ObjectFile interface.
The rough plan for followup patches is to expose the individual parts of
the breakpad file as ObjectFile "sections", which can then be used by
other parts of the codebase (SymbolFileBreakpad ?) to vend the necessary
information.


https://reviews.llvm.org/D55214

Files:
  include/lldb/Symbol/ObjectFile.h
  lit/Modules/Breakpad/Inputs/identification-linux.syms
  lit/Modules/Breakpad/Inputs/identification-macosx.syms
  lit/Modules/Breakpad/Inputs/identification-windows.syms
  lit/Modules/Breakpad/breakpad-identification.test
  lit/Modules/Breakpad/lit.local.cfg
  source/Plugins/ObjectFile/Breakpad/CMakeLists.txt
  source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp
  source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h
  source/Plugins/ObjectFile/CMakeLists.txt
  source/Symbol/ObjectFile.cpp
  tools/lldb-test/SystemInitializerTest.cpp
  tools/lldb-test/lldb-test.cpp

Index: tools/lldb-test/lldb-test.cpp
===
--- tools/lldb-test/lldb-test.cpp
+++ tools/lldb-test/lldb-test.cpp
@@ -734,9 +734,16 @@
   continue;
 }
 
+auto *ObjectPtr = ModulePtr->GetObjectFile();
+
+Printer.formatLine("Plugin name: {0}", ObjectPtr->GetPluginName());
 Printer.formatLine("Architecture: {0}",
ModulePtr->GetArchitecture().GetTriple().getTriple());
 Printer.formatLine("UUID: {0}", ModulePtr->GetUUID().GetAsString());
+Printer.formatLine("Executable: {0}", ObjectPtr->IsExecutable());
+Printer.formatLine("Stripped: {0}", ObjectPtr->IsStripped());
+Printer.formatLine("Type: {0}", ObjectPtr->GetType());
+Printer.formatLine("Strata: {0}", ObjectPtr->GetStrata());
 
 size_t Count = Sections->GetNumSections(0);
 Printer.formatLine("Showing {0} sections", Count);
Index: tools/lldb-test/SystemInitializerTest.cpp
===
--- tools/lldb-test/SystemInitializerTest.cpp
+++ tools/lldb-test/SystemInitializerTest.cpp
@@ -52,6 +52,7 @@
 #include "Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h"
 #include "Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.h"
 #include "Plugins/MemoryHistory/asan/MemoryHistoryASan.h"
+#include "Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h"
 #include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
 #include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 #include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
@@ -114,6 +115,7 @@
 void SystemInitializerTest::Initialize() {
   SystemInitializerCommon::Initialize();
 
+  breakpad::ObjectFileBreakpad::Initialize();
   ObjectFileELF::Initialize();
   ObjectFileMachO::Initialize();
   ObjectFilePECOFF::Initialize();
@@ -327,6 +329,7 @@
   PlatformDarwinKernel::Terminate();
 #endif
 
+  breakpad::ObjectFileBreakpad::Terminate();
   ObjectFileELF::Terminate();
   ObjectFileMachO::Terminate();
   ObjectFilePECOFF::Terminate();
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -688,3 +688,63 @@
  uint64_t Offset) {
   return FileSystem::Instance().CreateDataBuffer(file.GetPath(), Size, Offset);
 }
+
+void llvm::format_provider::format(
+const ObjectFile::Type &type, raw_ostream &OS, StringRef Style) {
+  switch (type) {
+  case ObjectFile::eTypeInvalid:
+OS << "invalid";
+break;
+  case ObjectFile::eTypeCoreFile:
+OS << "core file";
+break;
+  case ObjectFile::eTypeExecutable:
+OS << "executable";
+break;
+  case ObjectFile::eTypeDebugInfo:
+OS << "debug info";
+break;
+  case ObjectFile::eTyp

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:66-84
+  if (os == llvm::Triple::Win32) {
+// In binary form, the module id should have 20 bytes: 16 bytes for UUID,
+// and 4 bytes for the "age". However, in the textual format, the 4 bytes 
of
+// age are printed via %x, which can lead to shorter strings. So, we pad 
the
+// string with zeroes after the 16 bytes, to obtain a string of appropriate
+// size.
+if (token.size() < 33 || token.size() > 40)

@lemo: Does this part make sense? It seems that on linux the breakpad files 
have the `INFO CODE_ID` section, which contains the UUID without the funny 
trailing zero. So I could try fetching the UUID from there instead, but only on 
linux, as that section is not present mac (and on windows it contains something 
completely different). Right now I compute the UUID on linux by chopping off 
the trailing zero (as I have to do that anyway for mac), but I could do 
something different is there's any advantage to that.


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

https://reviews.llvm.org/D55214



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

[Lldb-commits] [PATCH] D55240: [FileSystem] Migrate CommandCompletions

[Pavel Labath via Phabricator via lldb-commits]

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

Looks good, modulo the filesystem copy comment.




Comment at: source/Commands/CommandCompletions.cpp:180
 
+  FileSystem fs = FileSystem::Instance();
   std::error_code EC;

This should surely be a reference, no? Making a copy of a filesystem sounds 
weird (and should probably be forbidden by deleting the copy constructor).


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

https://reviews.llvm.org/D55240



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

[Lldb-commits] [PATCH] D55122: [PDB] Fix location retrieval for function local variables and arguments that are stored relative to VFRAME

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thank you. I like the tests a lot. I think I'll steal the implementation of 
this when I get around to parsing breakpad unwind instructions. ;)

This looks fine to me, but one of the windows folks should take a look at it 
too.




Comment at: 
source/Plugins/SymbolFile/PDB/PDBFPOProgramToDWARFExpression.cpp:336-337
+break;
+  default:
+llvm_unreachable("Invalid binary operation");
+  }

These default labels go against llvm style guide 
.



Comment at: 
source/Plugins/SymbolFile/PDB/PDBFPOProgramToDWARFExpression.cpp:483-487
+if (lvalue_name == register_name) {
+  // found target assignment program - no need to parse further
+  result = rvalue_ast;
+  break;
+}

Is this true? Is it not possible for a program to depend on a value of a 
register which will be defined later?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55122



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

[Lldb-commits] [PATCH] D55122: [PDB] Fix location retrieval for function local variables and arguments that are stored relative to VFRAME

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D55122#1318295 , @leonid.mashinskiy 
wrote:

> > Is this true? Is it not possible for a program to depend on a value of a 
> > register which will be defined later?
>
> I am not totally sure about this, but all valid fpo programs that I've seen 
> in existing pdb support this invariant.
>  So I think that we can assume that every assignment depends only to 
> precedent statements.


Ok, that's fine with me, I just wanted to check that was intentional. It might 
be worth mentioning that in a comment somewhere.


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

https://reviews.llvm.org/D55122



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

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 176615.
labath added a comment.

- implement the module_id/code_id logic suggested by Mark Mentovai
- fix module_id endianness handling to make sure the UUID matches the one we 
get from the minidump files


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

https://reviews.llvm.org/D55214

Files:
  include/lldb/Symbol/ObjectFile.h
  lit/Modules/Breakpad/Inputs/identification-linux.syms
  lit/Modules/Breakpad/Inputs/identification-macosx.syms
  lit/Modules/Breakpad/Inputs/identification-windows.syms
  lit/Modules/Breakpad/breakpad-identification.test
  lit/Modules/Breakpad/lit.local.cfg
  source/Plugins/ObjectFile/Breakpad/CMakeLists.txt
  source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp
  source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h
  source/Plugins/ObjectFile/CMakeLists.txt
  source/Symbol/ObjectFile.cpp
  tools/lldb-test/SystemInitializerTest.cpp
  tools/lldb-test/lldb-test.cpp

Index: tools/lldb-test/lldb-test.cpp
===
--- tools/lldb-test/lldb-test.cpp
+++ tools/lldb-test/lldb-test.cpp
@@ -734,9 +734,16 @@
   continue;
 }
 
+auto *ObjectPtr = ModulePtr->GetObjectFile();
+
+Printer.formatLine("Plugin name: {0}", ObjectPtr->GetPluginName());
 Printer.formatLine("Architecture: {0}",
ModulePtr->GetArchitecture().GetTriple().getTriple());
 Printer.formatLine("UUID: {0}", ModulePtr->GetUUID().GetAsString());
+Printer.formatLine("Executable: {0}", ObjectPtr->IsExecutable());
+Printer.formatLine("Stripped: {0}", ObjectPtr->IsStripped());
+Printer.formatLine("Type: {0}", ObjectPtr->GetType());
+Printer.formatLine("Strata: {0}", ObjectPtr->GetStrata());
 
 size_t Count = Sections->GetNumSections(0);
 Printer.formatLine("Showing {0} sections", Count);
Index: tools/lldb-test/SystemInitializerTest.cpp
===
--- tools/lldb-test/SystemInitializerTest.cpp
+++ tools/lldb-test/SystemInitializerTest.cpp
@@ -52,6 +52,7 @@
 #include "Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h"
 #include "Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.h"
 #include "Plugins/MemoryHistory/asan/MemoryHistoryASan.h"
+#include "Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h"
 #include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
 #include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 #include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
@@ -116,6 +117,7 @@
   if (auto e = SystemInitializerCommon::Initialize(options))
 return e;
 
+  breakpad::ObjectFileBreakpad::Initialize();
   ObjectFileELF::Initialize();
   ObjectFileMachO::Initialize();
   ObjectFilePECOFF::Initialize();
@@ -331,6 +333,7 @@
   PlatformDarwinKernel::Terminate();
 #endif
 
+  breakpad::ObjectFileBreakpad::Terminate();
   ObjectFileELF::Terminate();
   ObjectFileMachO::Terminate();
   ObjectFilePECOFF::Terminate();
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -688,3 +688,63 @@
  uint64_t Offset) {
   return FileSystem::Instance().CreateDataBuffer(file.GetPath(), Size, Offset);
 }
+
+void llvm::format_provider::format(
+const ObjectFile::Type &type, raw_ostream &OS, StringRef Style) {
+  switch (type) {
+  case ObjectFile::eTypeInvalid:
+OS << "invalid";
+break;
+  case ObjectFile::eTypeCoreFile:
+OS << "core file";
+break;
+  case ObjectFile::eTypeExecutable:
+OS << "executable";
+break;
+  case ObjectFile::eTypeDebugInfo:
+OS << "debug info";
+break;
+  case ObjectFile::eTypeDynamicLinker:
+OS << "dynamic linker";
+break;
+  case ObjectFile::eTypeObjectFile:
+OS << "object file";
+break;
+  case ObjectFile::eTypeSharedLibrary:
+OS << "shared library";
+break;
+  case ObjectFile::eTypeStubLibrary:
+OS << "stub library";
+break;
+  case ObjectFile::eTypeJIT:
+OS << "jit";
+break;
+  case ObjectFile::eTypeUnknown:
+OS << "unknown";
+break;
+  }
+}
+
+void llvm::format_provider::format(
+const ObjectFile::Strata &strata, raw_ostream &OS, StringRef Style) {
+  switch (strata) {
+  case ObjectFile::eStrataInvalid:
+OS << "invalid";
+break;
+  case ObjectFile::eStrataUnknown:
+OS << "unknown";
+break;
+  case ObjectFile::eStrataUser:
+OS << "user";
+break;
+  case ObjectFile::eStrataKernel:
+OS << "kernel";
+break;
+  case ObjectFile::eStrataRawImage:
+OS << "raw image";
+break;
+  case ObjectFile::eStrataJIT:
+OS << "jit";
+break;
+  }
+}
Index: source/Plugins/ObjectFile/CMakeLists.txt
===
--- source/Plugins/ObjectFile/CMakeLists.txt
+++ source/Plugins/ObjectFile/CMakeLists.txt
@@ -1,4 +1,5 @@
+add_subdi

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: markmentovai.
labath marked 3 inline comments as done.
labath added inline comments.



Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:66-84
+  if (os == llvm::Triple::Win32) {
+// In binary form, the module id should have 20 bytes: 16 bytes for UUID,
+// and 4 bytes for the "age". However, in the textual format, the 4 bytes 
of
+// age are printed via %x, which can lead to shorter strings. So, we pad 
the
+// string with zeroes after the 16 bytes, to obtain a string of appropriate
+// size.
+if (token.size() < 33 || token.size() > 40)

markmentovai wrote:
> labath wrote:
> > @lemo: Does this part make sense? It seems that on linux the breakpad files 
> > have the `INFO CODE_ID` section, which contains the UUID without the funny 
> > trailing zero. So I could try fetching the UUID from there instead, but 
> > only on linux, as that section is not present mac (and on windows it 
> > contains something completely different). Right now I compute the UUID on 
> > linux by chopping off the trailing zero (as I have to do that anyway for 
> > mac), but I could do something different is there's any advantage to that.
> INFO CODE_ID, if present, is a better thing to use than what you find in 
> MODULE, except on Windows, where it’s absolutely the wrong thing to use but 
> MODULE is fine.
> 
> So, suggested logic:
> 
>   if has_code_id and not is_win:
> id = code_id
>   else:
> id = module_id
> 
> Aside from special-casing Windows against using INFO CODE_ID, I don’t think 
> you should hard-code any OS checks here. There’s no reason Mac dump_syms 
> couldn’t emit INFO CODE_ID, even though it doesn’t currently.
> 
> (In fact, you don’t even need to special-case for Windows. You could just 
> detect the presence of a filename token after the ID in INFO CODE_ID. As your 
> test data shows, Windows dump_syms always puts the module filename here, as 
> in “INFO CODE_ID 5C01672A4000 a.exe”, but other dump_syms will only have the 
> uncorrupted debug ID.
Thanks. I've implemented the logic you suggested and fixed byte-swapping issues 
when parsing the module id.

Note I still have to special-case windows to strip the "age" field from the 
module_id in order for our UUID to match the ones we normally get on mac. (We 
do the same thing when opening minidump files: 
).
 


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

https://reviews.llvm.org/D55214



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

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 176781.
labath marked 17 inline comments as done.
labath added a comment.

Updated according to review comments.

Also added a couple of tests for invalid inputs.


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

https://reviews.llvm.org/D55214

Files:
  include/lldb/Symbol/ObjectFile.h
  lit/Modules/Breakpad/Inputs/bad-module-id-1.syms
  lit/Modules/Breakpad/Inputs/bad-module-id-2.syms
  lit/Modules/Breakpad/Inputs/bad-module-id-3.syms
  lit/Modules/Breakpad/Inputs/identification-linux.syms
  lit/Modules/Breakpad/Inputs/identification-macosx.syms
  lit/Modules/Breakpad/Inputs/identification-windows.syms
  lit/Modules/Breakpad/breakpad-identification.test
  lit/Modules/Breakpad/lit.local.cfg
  source/API/SystemInitializerFull.cpp
  source/Plugins/ObjectFile/Breakpad/CMakeLists.txt
  source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp
  source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h
  source/Plugins/ObjectFile/CMakeLists.txt
  source/Symbol/ObjectFile.cpp
  tools/lldb-test/SystemInitializerTest.cpp
  tools/lldb-test/lldb-test.cpp

Index: tools/lldb-test/lldb-test.cpp
===
--- tools/lldb-test/lldb-test.cpp
+++ tools/lldb-test/lldb-test.cpp
@@ -724,6 +724,14 @@
 ModuleSpec Spec{FileSpec(File)};
 
 auto ModulePtr = std::make_shared(Spec);
+
+ObjectFile *ObjectPtr = ModulePtr->GetObjectFile();
+if (!ObjectPtr) {
+  WithColor::error() << File << " not recognised as an object file\n";
+  HadErrors = 1;
+  continue;
+}
+
 // Fetch symbol vendor before we get the section list to give the symbol
 // vendor a chance to populate it.
 ModulePtr->GetSymbolVendor();
@@ -734,9 +742,14 @@
   continue;
 }
 
+Printer.formatLine("Plugin name: {0}", ObjectPtr->GetPluginName());
 Printer.formatLine("Architecture: {0}",
ModulePtr->GetArchitecture().GetTriple().getTriple());
 Printer.formatLine("UUID: {0}", ModulePtr->GetUUID().GetAsString());
+Printer.formatLine("Executable: {0}", ObjectPtr->IsExecutable());
+Printer.formatLine("Stripped: {0}", ObjectPtr->IsStripped());
+Printer.formatLine("Type: {0}", ObjectPtr->GetType());
+Printer.formatLine("Strata: {0}", ObjectPtr->GetStrata());
 
 size_t Count = Sections->GetNumSections(0);
 Printer.formatLine("Showing {0} sections", Count);
Index: tools/lldb-test/SystemInitializerTest.cpp
===
--- tools/lldb-test/SystemInitializerTest.cpp
+++ tools/lldb-test/SystemInitializerTest.cpp
@@ -52,6 +52,7 @@
 #include "Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h"
 #include "Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.h"
 #include "Plugins/MemoryHistory/asan/MemoryHistoryASan.h"
+#include "Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h"
 #include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
 #include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
 #include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
@@ -116,6 +117,7 @@
   if (auto e = SystemInitializerCommon::Initialize(options))
 return e;
 
+  breakpad::ObjectFileBreakpad::Initialize();
   ObjectFileELF::Initialize();
   ObjectFileMachO::Initialize();
   ObjectFilePECOFF::Initialize();
@@ -331,6 +333,7 @@
   PlatformDarwinKernel::Terminate();
 #endif
 
+  breakpad::ObjectFileBreakpad::Terminate();
   ObjectFileELF::Terminate();
   ObjectFileMachO::Terminate();
   ObjectFilePECOFF::Terminate();
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -688,3 +688,63 @@
  uint64_t Offset) {
   return FileSystem::Instance().CreateDataBuffer(file.GetPath(), Size, Offset);
 }
+
+void llvm::format_provider::format(
+const ObjectFile::Type &type, raw_ostream &OS, StringRef Style) {
+  switch (type) {
+  case ObjectFile::eTypeInvalid:
+OS << "invalid";
+break;
+  case ObjectFile::eTypeCoreFile:
+OS << "core file";
+break;
+  case ObjectFile::eTypeExecutable:
+OS << "executable";
+break;
+  case ObjectFile::eTypeDebugInfo:
+OS << "debug info";
+break;
+  case ObjectFile::eTypeDynamicLinker:
+OS << "dynamic linker";
+break;
+  case ObjectFile::eTypeObjectFile:
+OS << "object file";
+break;
+  case ObjectFile::eTypeSharedLibrary:
+OS << "shared library";
+break;
+  case ObjectFile::eTypeStubLibrary:
+OS << "stub library";
+break;
+  case ObjectFile::eTypeJIT:
+OS << "jit";
+break;
+  case ObjectFile::eTypeUnknown:
+OS << "unknown";
+break;
+  }
+}
+
+void llvm::format_provider::format(
+const ObjectFile::Strata &strata, raw_ostream &OS, StringRef Style) {
+  switch (strata) {
+  case ObjectFile::eStrataInvalid:
+OS << "invalid";
+break;
+  case ObjectF

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lit/Modules/Breakpad/lit.local.cfg:1
+config.suffixes = ['.test']

zturner wrote:
> This shouldn't be necessary, the top-level `lit.cfg.py` already recognizes 
> `.test` extension.  You only need a lit.local.cfg if you're changing 
> something.
Yes, but then `lit/Modules/lit.local.cfg` overrides it by specifying it's own 
list of suffixes. I could fix that by adding `.test.` to that file, or by 
making that file use `+=`, but it's not clear to me whether that is better than 
just being explicit here.

If you have any preference, let me know.



Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:20-43
+static llvm::Triple::OSType toOS(llvm::StringRef str) {
+  using llvm::Triple;
+  return llvm::StringSwitch(str)
+  .Case("Linux", Triple::Linux)
+  .Case("mac", Triple::MacOSX)
+  .Case("windows", Triple::Win32)
+  .Default(Triple::UnknownOS);

zturner wrote:
> LLVM already has these functions in `Triple.cpp`, but they are hidden as 
> private implementations in the CPP file.  Perhaps we should expose them from 
> headers in Triple.h.
I've already checked out the available functions in `llvm::Triple`, and 
unfortunately each of them uses a slightly different form for some of the 
values. For example, `getArchTypeNameForLLVMName` uses `x86-64` instead of 
`x86_64`, `parseArch` uses `i386` instead of `x86`, `parseOS` uses `linux` 
instead of `Linux`, and so on...

Since this particular encoding is specific to the breakpad format, it made 
sense to me to have the parsing functions live here (as opposed to adding new 
cases to the `Triple` functions for instance), and leave everything else 
working with the "canonical" forms.



Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:53
+  static_assert(sizeof(data) == 20, "");
+  if (str.size() < 33 || str.size() > 40)
+return UUID();

lemo wrote:
> these magic integer literals make it hard to follow the intent - what's 
> special about 33, 40, 8, 16, ... ? (symbolic constants might help)
I've rewritten this to gradually chop bytes off from the start of the string, 
instead of always indexing into the original one. That should reduce the number 
of magic numbers (and hopefully reduce confusion).



Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:56-59
+  if (to_integer(str.substr(0, 8), t, 16))
+data.uuid1 = t;
+  else
+return UUID();

zturner wrote:
> Consider using `StringRef::consumeInteger()` here.
I don't think consumeInteger can help, as these "fields" are not delimited here 
in any way, so that function will happily try to parse the whole string. If you 
had a specific patter in mind let me know (but hopefully the new implementation 
won't be so bad either).



Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:160-161
+  }
+  llvm::StringRef text(reinterpret_cast(data_sp->GetBytes()),
+   data_sp->GetByteSize());
+

zturner wrote:
> We have `GetData()` which returns an `ArrayRef`, and another function 
> `toStringRef` which converts an `ArrayRef` to a `StringRef`.  So this might 
> be cleaner to write as `auto text = llvm::toStringRef(data_sp->GetData());`
Cool, I didn't know about that. Thanks.



Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h:74
+
+  bool IsStripped() override { return false; }
+

zturner wrote:
> Is this always true for breakpad files?
Well.. the whole point of these files is to provide symbol information, so it 
would be weird if they were stripped. The breakpad `dump_syms` allows you to 
omit generating unwind information, but I don't think that's enough to call 
this "stripped". It is certainly possible to create a file by hand which 
contains just a `MODULE` directive and nothing else, but I would say that is a 
(non-stripped) file which describes an empty module, and not a stripped file.

In reality, this doesn't really matter, as this function is called from just 
one place 
, 
and I don't think that will be relevant for breakpad files.



Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h:98
+
+private:
+  struct Header {

zturner wrote:
> lemo wrote:
> > Nit: I personally prefer not to mix data, type and function members in the 
> > same "access" section - is there an LLVM/LLDB guideline which requires 
> > everything in the same place?
> > 
> > If not, can you please add a private section for the destructor, followed 
> > by a section for the private data members?
> Given that we don't actually store an instance of the header anywhere, we 
> just use it as a constructor parameter, perhaps we could go one step further 
> and move

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

[Pavel Labath via Phabricator via lldb-commits]

labath marked 3 inline comments as done.
labath added inline comments.



Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h:74
+
+  bool IsStripped() override { return false; }
+

markmentovai wrote:
> labath wrote:
> > zturner wrote:
> > > Is this always true for breakpad files?
> > Well.. the whole point of these files is to provide symbol information, so 
> > it would be weird if they were stripped. The breakpad `dump_syms` allows 
> > you to omit generating unwind information, but I don't think that's enough 
> > to call this "stripped". It is certainly possible to create a file by hand 
> > which contains just a `MODULE` directive and nothing else, but I would say 
> > that is a (non-stripped) file which describes an empty module, and not a 
> > stripped file.
> > 
> > In reality, this doesn't really matter, as this function is called from 
> > just one place 
> > ,
> >  and I don't think that will be relevant for breakpad files.
> Correct, "stripped" isn't really useful for Breakpad dump_syms output. What 
> does LLDB do with the result of IsStripped()?
> 
> Stripped dump_syms output would be what you get from running dump_syms on a 
> stripped module. I can't imagine why anyone would do this intentionally, but 
> you'd also be hard-pressed to tell that's what had happened given only the 
> dumped symbol file.
Not much. The only relevant use is linked to above. I don't fully understand 
that code, but my rough idea is the following: we create a "synthetic" symbol 
in the main object file when we know some symbol must be at the given address, 
but we don't know it's name. Then when we are looking up an address and it 
resolves to this synthetic symbol (and the object file is marked as stripped), 
we go to the symbol file (if we have one) to see if it can provide us with a 
name for it.

So this isn't even relevant for breakpad files, as they will never be the 
"main" object file, but I had to put something here, and "false" seems the best 
option.


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

https://reviews.llvm.org/D55214



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

[Lldb-commits] [PATCH] D55318: [Expressions] Add support of expressions evaluation in some object's context

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Sounds like an interesting feature to me.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55318



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