[Lldb-commits] [lldb] 0610a25 - [lldb] Delete copy operations on PluginInterface class

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-10-09T10:37:09+02:00
New Revision: 0610a25a85a0a204c994331e1ab275f86010f9ad

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

LOG: [lldb] Delete copy operations on PluginInterface class

This is a polymorphic class, copying it is a bad idea.

This was not a problem because most classes inheriting from it were
deleting their copy operations themselves. However, this enables us to
delete those explicit deletions, and ensure noone forgets to add them in
the future.

Added: 


Modified: 
lldb/include/lldb/Core/Architecture.h
lldb/include/lldb/Core/PluginInterface.h
lldb/include/lldb/Symbol/SymbolVendor.h
lldb/include/lldb/Target/DynamicLoader.h
lldb/include/lldb/Target/LanguageRuntime.h
lldb/include/lldb/Target/OperatingSystem.h
lldb/include/lldb/Target/Platform.h
lldb/include/lldb/Target/UnwindAssembly.h
lldb/source/Core/DynamicLoader.cpp
lldb/source/Plugins/DynamicLoader/Hexagon-DYLD/DynamicLoaderHexagonDYLD.h
lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h
lldb/source/Plugins/DynamicLoader/Static/DynamicLoaderStatic.cpp
lldb/source/Plugins/DynamicLoader/Static/DynamicLoaderStatic.h
lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.h
lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp
lldb/source/Plugins/Platform/Android/PlatformAndroid.h
lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.h
lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
lldb/source/Plugins/Platform/Linux/PlatformLinux.h
lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.h
lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleBridge.h
lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleTV.h
lldb/source/Plugins/Platform/MacOSX/PlatformRemoteAppleWatch.h
lldb/source/Plugins/Platform/MacOSX/PlatformRemoteiOS.h
lldb/source/Plugins/Platform/NetBSD/PlatformNetBSD.cpp
lldb/source/Plugins/Platform/NetBSD/PlatformNetBSD.h
lldb/source/Plugins/Platform/OpenBSD/PlatformOpenBSD.cpp
lldb/source/Plugins/Platform/OpenBSD/PlatformOpenBSD.h
lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
lldb/source/Plugins/Platform/Windows/PlatformWindows.h
lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
lldb/source/Plugins/Process/mach-core/ProcessMachCore.h
lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.h
lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.h
lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.h
lldb/source/Plugins/SymbolVendor/wasm/SymbolVendorWasm.h
lldb/source/Symbol/SymbolVendor.cpp
lldb/source/Target/LanguageRuntime.cpp
lldb/source/Target/OperatingSystem.cpp
lldb/source/Target/UnwindAssembly.cpp

Removed: 




diff  --git a/lldb/include/lldb/Core/Architecture.h 
b/lldb/include/lldb/Core/Architecture.h
index d8dbbb4f540f..2ea8bd31ebf4 100644
--- a/lldb/include/lldb/Core/Architecture.h
+++ b/lldb/include/lldb/Core/Architecture.h
@@ -15,9 +15,6 @@ namespace lldb_private {
 
 class Architecture : public PluginInterface {
 public:
-  Architecture() = default;
-  ~Architecture() override = default;
-
   /// This is currently intended to handle cases where a
   /// program stops at an instruction that won't get executed and it
   /// allows the stop reason, like "breakpoint hit", to be replaced
@@ -100,10 +97,6 @@ class Architecture : public PluginInterface {
Target &target) const {
 return addr;
   }
-
-private:
-  Architecture(const Architecture &) = delete;
-  void operator=(const Architecture &) = delete;
 };
 
 } // namespace lldb_private

diff  --git a/lldb/include/lldb/Core/PluginInterface.h 
b/lldb/include/lldb/Core/PluginInterface.h
index 17f6dc367155..5bdb2f45b665 100644
--- a/lldb/include/lldb/Core/PluginInterface.h
+++ b/lldb/include/lldb/Core/PluginInterface.h
@@ -15,11 +15,15 @@ namespace lldb_private {
 
 class PluginInterface {
 public:
-  virtual ~PluginInterface() {}
+  PluginInterface() = default;
+  virtual ~PluginInterface() = default;
 
   virtual ConstString GetPluginName() = 0;
 
   virtual uint32_t GetPluginVersion() = 0;
+
+  PluginInterface(const PluginInterface &) = delete;
+  PluginInterface &operator=(const PluginInterface &) = delete;
 };
 
 } // namespace lldb_private

diff  --git a/lldb/include/lldb/Symbol/

[Lldb-commits] [PATCH] D88841: [intel pt] Refactor parsing

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

Looks good.

In D88841#2320410 , @wallace wrote:

> - Cannot delete Trace() {}, as there’s a compilation error. Could be because 
> some constructors have been marked as deleted, so the compiler wants explicit 
> declarations.

Right. I was going to say you can delete those too as they are already deleted 
on the base class. Except, it turns out, PluginInterface did not have them 
deleted. Well.. after 0610a25a85 
  it has. 
:)




Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:19
 
 LLDB_PLUGIN_DEFINE_ADV(TraceIntelPT, TraceIntelPT)
 

I think `LLDB_PLUGIN_DEFINE` should work fine. The `_ADV` version is for 
(mostly legacy) cases where the plugin name does not match the class name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88841

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

[Lldb-commits] [PATCH] D88769: [trace] Scaffold "thread trace dump instructions"

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I was waiting for the dependant patch to take shape. This looks ok to me -- 
just one small design question.




Comment at: lldb/include/lldb/Target/Target.h:1120
+  ///   The trace object. It might be undefined.
+  lldb::TraceSP &GetTrace();
+

`const TraceSP &` ?



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:61
+ const std::vector &targets) {
+  TraceSP trace_instance(new TraceIntelPT(pt_cpu, targets));
+  for (const TargetSP &target_sp : targets)

auto trace_instance = std::make_shared(...)



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:63
+  for (const TargetSP &target_sp : targets)
+target_sp->SetTrace(trace_instance->shared_from_this());
+

`SetTrace(trace_instance)`



Comment at: lldb/source/Target/Thread.cpp:1617-1627
+void Thread::DumpTraceInstructions(Stream &s, size_t count,
+   size_t start_position) const {
+  if (!GetProcess()->GetTarget().GetTrace()) {
+s << "error: no trace in this target\n";
+return;
+  }
+  s.Printf("thread #%u: tid = %" PRIu64 ", total instructions = 1000\n",

Given the intended design of having one process (and thread) class serve 
multiple trace types, can this function do anything useful besides calling into 
the Trace object to perform trace-specific dumping ?

And if it cannot, couldn't we just skip it have have callers call the relevant 
Trace method directly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88769

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

[Lldb-commits] [PATCH] D82863: [LLDB] Add support to resize SVE registers at run-time

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

In D82863#2320342 , @jasonmolenda 
wrote:

> In D82863#2319568 , @labath wrote:
>
>> In D82863#2313676 , @jasonmolenda 
>> wrote:
>>
>>> (about g packets...) they cause so many problems if there is a 
>>> mis-coordination between the remote stub and lldb, I never want to stop the 
>>> remote stub from providing those offsets.
>>
>> I am not sure how to interpret this in the context of SVE registers. The 
>> stub cannot send the offsets of those, as their size/offset might change 
>> depending on their configuration.
>
> Yeah, of course you're right, in the case of a dynamic register context like 
> this, the g/G packets shouldn't be used (we should have 
> GDBRemoteCommunicationClient::AvoidGPackets return true).
>
> The jThreadsInfo may give us the full register context (in base10 because 
> json lol) although debugserver only sends the GPRs in the jThreadsInfo 
> response.  If lldb needs to read the full register context, it would need to 
> read them individually (and write them individually if saving the full 
> register context).  We could add qReadAllRegisters and QWriteAllRegisters 
> which have a series of regnum:base16-regval-target-endian, or JSON versions 
> of the same packets if the perf was important.
>
> The original g/G packets were designed for little embedded systems where the 
> stub had to be very small and dumb, and could just memcpy the payload in the 
> packet into the register context & back out again.  Any sensible design today 
> would, at least, have some form of an array of regnum:regval to eliminate 
> many of the problems.
>
>> Unless of course, we make sure SVE regs come last, but that imposes some 
>> constraints on future registers sets. I suppose we might be able to say that 
>> all variable-length or otherwise-funny registers must come last.
>
> Yeah, Omair's patch currently assumes that the SVE regs come last already 
> when they copy & truncate the registers context to heap.  I fear that we'll 
> get to armv12 and we'll be adding registers after the SVE and wonder why 
> they're being truncated somewhere in lldb. :)
>
> @omjavaid , what do you think about disabling g/G when we're working with SVE 
> registers (GDBRemoteCommunicationClient::AvoidGPackets)?  There are some 
> gdb-remote stubs that can *only* read/write registers with g/G, but I think 
> it's reasonable to say "you must implement p/P for a target with SVE", that's 
> a generic packet shared by both lldb and gdb.  We could add a more-modern g/G 
> replacement packet, but no one would have that implemented, and if they were 
> going to add anything, I'd have them implement p/P unless it's perf problems 
> and they need a read-all-registers / write-all-registers packet that works 
> with SVE.

I did consider and second the idea of pulling out SVE registers from g/G packet 
specially for vector length that is more than 8 x (32 bytes) as it creates a 
huge sized g/G packet containing thousands of bytes. Historically, sending 
multiple p/P for all registers was an overhead due to slow communication 
between embedded or serially connected remote targets and host gdb/GDB. This is 
mostly not the case these days and we can come up with an alternate approach 
here as being discussed above.

However, There is some catch where we need to preserve all register data across 
expressions evaluation invocation and the need for offset correctness will 
still be needed as S,D,V and Z regs share same offset in case of SVE.

Also for futuristic point of view I am baking a patch that adds support for MTE 
and Pointer Authentication registers. As MT, Pauth and SVE are optional 
features, therefore registers for these features may or may not be available 
for a given target and will have to be configured dynamically when inferior is 
attached. So I was assuming similar adjustment in offsets of MTE/Pauth 
registers like the ones I have done for SVE registers. However MTE/Pauth 
registers do not change size dynamically and their offset change will only 
occur when they are present alongside SVE registers in a particular target.

My idea for all this implementation is to configure an initial offset of all 
dynamic register register sets at the time of inferior connection in 
RegisterInfoPOSIX_arm64::ConfigureVectorRegisterInfos where we put SVE register 
currently right after GPRs (GPR is the bare minimum that all Arm64 targets 
support) and any dynamically select-able register sets (like MTE, Pauth etc) 
can come before or after SVE registers.


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

https://reviews.llvm.org/D82863

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

[Lldb-commits] [PATCH] D89121: [lldb/Utility] Introduce UnimplementedError

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added a reviewer: JDevlieghere.
Herald added a subscriber: mgorny.
Herald added a project: LLDB.
labath requested review of this revision.

This is essentially a replacement for the PacketUnimplementedError
previously present in the gdb-remote server code.

The reason I am introducing a generic error is because I wanted the
native process classes to be able to signal that they do not support
some functionality. They could not use PacketUnimplementedError as they
are independent of a specific transport protocol. Putting the error
class in the the native process code was also not ideal because the
gdb-remote code is also used for lldb-server's platform mode, which does
not (should not) know how to debug individual processes.

I'm putting it under Utility, as I think it can be generally useful for
notifying about unsupported/unimplemented functionality (and in
particular, for programatically testing whether something is
unsupported).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89121

Files:
  lldb/include/lldb/Utility/UnimplementedError.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Utility/CMakeLists.txt
  lldb/source/Utility/UnimplementedError.cpp
  lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp

Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp
===
--- lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp
@@ -9,9 +9,9 @@
 #include "gtest/gtest.h"
 
 #include "GDBRemoteTestUtils.h"
-
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
 #include "lldb/Utility/Connection.h"
+#include "lldb/Utility/UnimplementedError.h"
 
 namespace lldb_private {
 namespace process_gdb_remote {
@@ -39,8 +39,7 @@
 TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_UnimplementedError) {
   MockServerWithMockConnection server;
 
-  auto error =
-  llvm::make_error("Test unimplemented error");
+  auto error = llvm::make_error();
   server.SendErrorResponse(std::move(error));
 
   EXPECT_THAT(server.GetPackets(), testing::ElementsAre("$#00"));
@@ -61,8 +60,8 @@
 TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_ErrorList) {
   MockServerWithMockConnection server;
 
-  auto error = llvm::joinErrors(llvm::make_error(),
-llvm::make_error());
+  auto error = llvm::joinErrors(llvm::make_error(),
+llvm::make_error());
 
   server.SendErrorResponse(std::move(error));
   // Make sure only one packet is sent even when there are multiple errors.
Index: lldb/source/Utility/UnimplementedError.cpp
===
--- /dev/null
+++ lldb/source/Utility/UnimplementedError.cpp
@@ -0,0 +1,11 @@
+//===-- UnimplementedError.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "lldb/Utility/UnimplementedError.h"
+
+char lldb_private::UnimplementedError::ID;
Index: lldb/source/Utility/CMakeLists.txt
===
--- lldb/source/Utility/CMakeLists.txt
+++ lldb/source/Utility/CMakeLists.txt
@@ -65,6 +65,7 @@
   StructuredData.cpp
   TildeExpressionResolver.cpp
   Timer.cpp
+  UnimplementedError.cpp
   UUID.cpp
   UriParser.cpp
   UserID.cpp
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -10,13 +10,12 @@
 
 #include "lldb/Host/Config.h"
 
-#include "GDBRemoteCommunicationServerLLGS.h"
-#include "lldb/Utility/GDBRemote.h"
 
 #include 
 #include 
 #include 
 
+#include "GDBRemoteCommunicationServerLLGS.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/Debug.h"
 #include "lldb/Host/File.h"
@@ -32,11 +31,13 @@
 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/Endian.h"
+#include "lldb/Utility/GDBRemote.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/RegisterValue.h"
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/StreamString.h"
+#include "lldb/Utility/UnimplementedError.h"
 #include "lldb/Utility/UriParser.h"
 #include 

[Lldb-commits] [PATCH] D89124: [lldb-server][linux] Add ability to allocate memory

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: clayborg, jankratochvil, mgorny.
Herald added subscribers: pengfei, emaste.
Herald added a reviewer: JDevlieghere.
Herald added a project: LLDB.
labath requested review of this revision.

This patch adds support for the _M and _m gdb-remote packets, which
(de)allocate memory in the inferior. This works by "injecting" a
m(un)map syscall into the inferior. This consists of:

- finding an executable page of memory
- writing the syscall opcode to it
- setting up registers according to the os syscall convention
- single stepping over the syscall

The advantage of this approach over calling the mmap function is that
this works even in case the mmap function is buggy or unavailable. The
disadvantage is it is more platform-dependent, which is why this patch
only works on X86 (_32 and _64) right now. Adding support for other
linux architectures should be easy and consist of defining the
appropriate syscall constants. Adding support for other OSes depends on
the its ability to do a similar trick.

Depends on D89121 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89124

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
  lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.h
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
  lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
  lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/test/API/lang/c/stepping/TestStepAndBreakpoints.py
  lldb/test/API/tools/lldb-server/memory-allocation/Makefile
  
lldb/test/API/tools/lldb-server/memory-allocation/TestGdbRemoteMemoryAllocation.py
  lldb/test/API/tools/lldb-server/memory-allocation/main.c
  lldb/test/Shell/Expr/nodefaultlib.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -41,9 +41,6 @@
   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,
@@ -147,4 +144,4 @@
 };
 } // namespace lldb_private
 
-#endif
\ No newline at end of file
+#endif
Index: lldb/test/Shell/Expr/nodefaultlib.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/nodefaultlib.cpp
@@ -0,0 +1,12 @@
+// Test that we're able to evaluate expressions in inferiors without the
+// standard library (and mmap-like functions in particular).
+
+// RUN: %build %s --nodefaultlib -o %t
+// RUN: %lldb %t -o "b main" -o run -o "p call_me(5, 6)" -o exit \
+// RUN:   | FileCheck %s
+
+// CHECK: (int) $0 = 30
+
+int call_me(int x, long y) { return x * y; }
+
+int main() { return call_me(4, 5); }
Index: lldb/test/API/tools/lldb-server/memory-allocation/main.c
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/memory-allocation/main.c
@@ -0,0 +1 @@
+int main() { return 0; }
Index: lldb/test/API/tools/lldb-server/memory-allocation/TestGdbRemoteMemoryAllocation.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/memory-allocation/TestGdbRemoteMemoryAllocation.py
@@ -0,0 +1,101 @@
+
+import gdbremote_testcase
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+supported_linux_archs = ["x86_64", "i386"]
+supported_oses = ["linux"]
+
+class TestGdbRemoteMemoryAllocation(gdbremote_testcase.GdbRemoteTestCaseBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def allocate(self, size, permissions):
+self.test_sequence.add_log_lines(["read packet: $_M{:x},{}#00".format(size, permissions),
+  {"direction": "send",
+   "regex":
+   r"^\$([0-9a-f]+)#[0-9a-fA-F]{2}$",
+   "capture": {
+

[Lldb-commits] [PATCH] D89124: [lldb-server][linux] Add ability to allocate memory

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1350
 
-Status NativeProcessLinux::AllocateMemory(size_t size, uint32_t permissions,
-  lldb::addr_t &addr) {
-// FIXME implementing this requires the equivalent of
-// InferiorCallPOSIX::InferiorCallMmap, which depends on functional ThreadPlans
-// working with Native*Protocol.
-#if 1
-  return Status("not implemented yet");
-#else
-  addr = LLDB_INVALID_ADDRESS;
-
-  unsigned prot = 0;
-  if (permissions & lldb::ePermissionsReadable)
-prot |= eMmapProtRead;
-  if (permissions & lldb::ePermissionsWritable)
-prot |= eMmapProtWrite;
-  if (permissions & lldb::ePermissionsExecutable)
-prot |= eMmapProtExec;
-
-  // TODO implement this directly in NativeProcessLinux
-  // (and lift to NativeProcessPOSIX if/when that class is refactored out).
-  if (InferiorCallMmap(this, addr, 0, size, prot,
-   eMmapFlagsAnon | eMmapFlagsPrivate, -1, 0)) {
-m_addr_to_mmap_size[addr] = size;
-return Status();
-  } else {
-addr = LLDB_INVALID_ADDRESS;
-return Status("unable to allocate %" PRIu64
-  " bytes of memory with permissions %s",
-  size, GetPermissionsAsCString(permissions));
+llvm::Expected
+NativeProcessLinux::Syscall(llvm::ArrayRef args) {

How much of this would be useful for BSDs? Should some of this be pushed to the 
base classes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89124

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

[Lldb-commits] [PATCH] D82863: [LLDB] Add support to resize SVE registers at run-time

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D82863#2321370 , @omjavaid wrote:

> In D82863#2320342 , @jasonmolenda 
> wrote:
>
>> The original g/G packets were designed for little embedded systems where the 
>> stub had to be very small and dumb, and could just memcpy the payload in the 
>> packet into the register context & back out again.  Any sensible design 
>> today would, at least, have some form of an array of regnum:regval to 
>> eliminate many of the problems.
>>
>>> Unless of course, we make sure SVE regs come last, but that imposes some 
>>> constraints on future registers sets. I suppose we might be able to say 
>>> that all variable-length or otherwise-funny registers must come last.
>>
>> Yeah, Omair's patch currently assumes that the SVE regs come last already 
>> when they copy & truncate the registers context to heap.  I fear that we'll 
>> get to armv12 and we'll be adding registers after the SVE and wonder why 
>> they're being truncated somewhere in lldb. :)

Well.. we could design it such that the SVE registers (and any other 
dynamically-sized regs) always come last. Then they wouldn't impact the offsets 
of static registers.

I don't think we have a requirement that newer register sets must be appended. 
Or at least, we shouldn't have, as now both intel and arm have cpu features 
(and registers) that may be arbitrarily combined in future processors.

>> @omjavaid , what do you think about disabling g/G when we're working with 
>> SVE registers (GDBRemoteCommunicationClient::AvoidGPackets)?  There are some 
>> gdb-remote stubs that can *only* read/write registers with g/G, but I think 
>> it's reasonable to say "you must implement p/P for a target with SVE", 
>> that's a generic packet shared by both lldb and gdb.  We could add a 
>> more-modern g/G replacement packet, but no one would have that implemented, 
>> and if they were going to add anything, I'd have them implement p/P unless 
>> it's perf problems and they need a read-all-registers / write-all-registers 
>> packet that works with SVE.
>
> I did consider and second the idea of pulling out SVE registers from g/G 
> packet specially for vector length that is more than 8 x (32 bytes) as it 
> creates a huge sized g/G packet containing thousands of bytes. Historically, 
> sending multiple p/P for all registers was an overhead due to slow 
> communication between embedded or serially connected remote targets and host 
> gdb/GDB. This is mostly not the case these days

While it's certainly less important than it used to be, there are still people 
who care about these things. Just last year, we had a contribution which 
started using them more aggressively. The contributor has a use case for 
fetching all registers on each stop, which meant that our usual expedition 
rules were not sufficient and p packets were slow.

> and we can come up with an alternate approach here as being discussed above.

That's true, but any custom solution is unlikely to be supported by stubs 
(qemu) which are primarily meant for communicating with gdb. And since we have 
people who want to use those stubs with lldb (and I am supporting some of 
them), if we do something custom, it's possible we may end up supporting both.

That's why I'd like to understand more about how gdb does things. The g packet 
definitely has its issues but so does introducing custom packets, and I think 
we should weigh those carefully.

> However, There is some catch where we need to preserve all register data 
> across expressions evaluation invocation and the need for offset correctness 
> will still be needed as S,D,V and Z regs share same offset in case of SVE.

Is this about QSave/RestoreAllRegisters? If it is, then I don't think it's that 
very relevant here, because these saved registers don't make their way across 
the wire, and the fact that we save the registers in a buffer similar to the g 
packet is purely an implementation detail. We can change that any time we like 
and it does not require the client&server to be in sync. (Of course, if we do 
serialize everything into the g packet, then reusing that format for 
QSaveAllRegisters is handy, so it is still /slightly/ relavant).


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

https://reviews.llvm.org/D82863

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

[Lldb-commits] [PATCH] D89124: [lldb-server][linux] Add ability to allocate memory

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1400
+.ToError())
+return std::move(Err);
+

Is it possible that the page we find is executable but not writeable and is 
this part intended to handle that?
(maybe this is a silly question, the code had to be written into that page in 
the first place I guess)



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1404
+
+  int req = SupportHardwareSingleStepping() ? PTRACE_SINGLESTEP : PTRACE_CONT;
+  if (llvm::Error Err =

Could you add a comment here explaining this? Like "either we single step just 
syscall, or continue then hit a trap instruction after the syscall"
(if I have that right)

The description for PTRACE_SINGLESTEP confused me a bit with "next entry to":
> Restart the stopped tracee as for PTRACE_CONT, but arrange for
> the tracee to be stopped at the next entry to or exit from a
> system call, or after execution of a single instruction,
> respectively.

But I assume what happens here is you step "syscall" and then the whole mmap 
call happens, then you come back. Instead of getting kept before the mmap 
actually happens.





Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1419
+  uint64_t result = reg_ctx.ReadRegisterAsUnsigned(syscall_data.Result, 
-ESRCH);
+  uint64_t errno_threshold =
+  (uint64_t(-1) >> (64 - 8 * m_arch.GetAddressByteSize())) - 0x1000;

What does this calculation do/mean? (0x1000 is 4k which is a page size maybe?)



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1421
+  (uint64_t(-1) >> (64 - 8 * m_arch.GetAddressByteSize())) - 0x1000;
+  if (result > errno_threshold) {
+return llvm::errorCodeToError(

For these ifs, are you putting {} because the return is split over two lines? I 
think it would compile without but is this one of the exceptions to the coding 
standard? (if clang-format isn't doing this for you that is)



Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:1249
+  case llvm::Triple::x86:
+return MmapData{192, 91};
+  case llvm::Triple::x86_64:

What's the source for these numbers?



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2346
+
+  const lldb::addr_t size = packet.GetHexMaxU64(false, LLDB_INVALID_ADDRESS);
+  if (size == LLDB_INVALID_ADDRESS)

(assuming this packet type is supported by GDB already) Does it also use hex 
for the size field? I ask because I came across some file loading packets that 
were hex for lldb, int for gdb.

Obviously we don't have a hard requirement to match but if we're adding 
something new might as well.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2366
+}
+  }
+

Is no permissions here a valid packet? (as long as mmap doesn't mind I guess it 
is)



Comment at: lldb/test/API/lang/c/stepping/TestStepAndBreakpoints.py:23
 @expectedFailureAll(oslist=['freebsd'], bugnumber='llvm.org/pr17932')
-@expectedFailureAll(oslist=["linux"], bugnumber="llvm.org/pr14437")
 @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24777")

This test now passes *because* we have the ability to allocate memory, correct? 
(I thought it was a stray change at first)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89124

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

[Lldb-commits] [PATCH] D89121: [lldb/Utility] Introduce UnimplementedError

[David Spickett via Phabricator via lldb-commits]

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

I was going to need something like this myself shortly. LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89121

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

[Lldb-commits] [PATCH] D89124: [lldb-server][linux] Add ability to allocate memory

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/test/API/lang/c/stepping/TestStepAndBreakpoints.py:23
 @expectedFailureAll(oslist=['freebsd'], bugnumber='llvm.org/pr17932')
-@expectedFailureAll(oslist=["linux"], bugnumber="llvm.org/pr14437")
 @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24777")

DavidSpickett wrote:
> This test now passes *because* we have the ability to allocate memory, 
> correct? (I thought it was a stray change at first)
Actually shouldn't this be an expected failure on non x86 linux?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89124

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

Re: [Lldb-commits] [llvm-dev] [cfe-dev] Upcoming upgrade of LLVM buildbot

[Galina Kistanova via lldb-commits]

Hello Andrzej,

Please do not update your bots yet. I will explicitly ask later.

Feel free to move other reliably green bots of yours to the production
buildbot.

Thanks

Galina

On Fri, Oct 9, 2020 at 3:51 AM Andrzej Warzynski 
wrote:

> I switched one of our workers to the main Buildbot and everything seems
> fine #fingers-crossed. I guess that that was a temporary glitch?
>
> We haven't updated our local Buildbot installations - still on 0.8.5.
> Should we update?
>
> -Andrzej
>
> On 09/10/2020 00:44, Galina Kistanova wrote:
> > Hi Paula,
> >
> > This error is fine. The buildbot has tested the worker version. 0.8.x
> > apparently does not have that method.
> > The error gets handled gracefully on the server side. At least it seems
> > so so far.
> >
> > That should not prevent your bot from connecting.
> >
> > Thanks
> >
> > Galina
> >
> > On Thu, Oct 8, 2020 at 2:11 PM Paula Askar  > > wrote:
> >
> > Hey Andrzej,
> >
> > What are you seeing in your buildbot logs? Is it this error?
> > `twisted.spread.flavors.NoSuchMethod: No such method:
> > remote_getWorkerInfo`
> >
> > If so, you might want to try updating your buildbot worker.
> > I updated llvmlibc's to 2.8.4 and that seemed to solve the connection
> > problem:
> >
> https://github.com/llvm/llvm-project/commit/f60686f35cc89504f3411f49cf16a651a74be6eb
> >
> > Best,
> > Paula Askar
> >
> >
> > On Thu, Oct 8, 2020 at 5:43 AM Andrzej Warzynski via llvm-dev
> > mailto:llvm-...@lists.llvm.org>> wrote:
> >  >
> >  > Our Flang-aarch64 buildbots just won't connect to the main
> Buildbot
> >  > master anymore. I switched them to the staging buildbot master
> > instead
> >  > and it seems fine for now. Is there anything that we can/should
> > tweak at
> >  > our end?
> >  >
> >  > http://lab.llvm.org:8014/#/waterfall?tags=flang
> >  >
> >  > -Andrzej
> >  >
> >  > On 08/10/2020 00:31, Galina Kistanova via cfe-dev wrote:
> >  > > They are online now -
> > http://lab.llvm.org:8011/#/waterfall?tags=sanitizer
> >  > >
> >  > > AnnotatedCommand has severe design conflict with the new
> buildbot.
> >  > > We have changed it to be safe and still do something useful,
> > but it will
> >  > > need more love and care.
> >  > >
> >  > > Please let me know if you have some spare time to work on
> porting
> >  > > AnnotatedCommand.
> >  > >
> >  > > Thanks
> >  > >
> >  > > Galina
> >  > >
> >  > > On Wed, Oct 7, 2020 at 2:57 PM Vitaly Buka
> > mailto:vitalyb...@google.com>
> >  > > >>
> > wrote:
> >  > >
> >  > > It looks like all sanitizer builder are still offline
> >  > > http://lab.llvm.org:8011/#/builders
> >  > >
> >  > > On Tue, 6 Oct 2020 at 00:34, Galina Kistanova via
> cfe-commits
> >  > >  > 
> >  > >> wrote:
> >  > >
> >  > > Hello everyone,
> >  > >
> >  > > The staging buildbot was up and running for 6 days now,
> and
> >  > > looks good.
> >  > >
> >  > > Tomorrow at 12:00 PM PDT we will switch the production
> > buildbot
> >  > > to the new version.
> >  > >
> >  > > If you are a bot owner, you do not need to do anything
> > at this
> >  > > point, unless I'll ask you to help.
> >  > > The buildbot will go down for a short period of time,
> > and then a
> >  > > new version will come up and will accept connections
> > from your bots.
> >  > >
> >  > > Please note that the new version has a bit different URL
> >  > > structure. You will need to update the bookmarks or
> > scripts if
> >  > > you have stored direct URLs to inside the buldbot.
> >  > >
> >  > > We will be watching the production and staging bots and
> > will be
> >  > > improving zorg for the next week or so.
> >  > >
> >  > > I will need your feedback about blame e-mails delivery,
> IRC
> >  > > reporting issues, and anything you could spot wrong
> > with the new
> >  > > bot. I  hope the transition will go smoothly and we
> > will handle
> >  > > issues quickly if any would come up.
> >  > >
> >  > > After production is good and we have about a week of
> > running
> >  > > history, I'll ask the bot owners to upgrade buildbots
> > on their
> >  > > side. Please do not upgrade your buildbots unless I'll
> > ask you
> >  > > to. We are trying to limit a number of moving parts at
> this
> >  > > stage. We will start accepting change to zorg

[Lldb-commits] [PATCH] D89124: [lldb-server][linux] Add ability to allocate memory

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1354
+  auto region_it = llvm::find_if(m_mem_region_cache, [](const auto &pair) {
+return pair.first.GetExecutable() == MemoryRegionInfo::eYes;
+  });

Finding arbitrary address would not be safe if LLDB supports [[ 
https://lldb.llvm.org/status/projects.html#non-stop-debugging | non-stop mode 
if implemented ]]. It also makes the address a bit non-deterministic. IIRC GDB 
is using executable's e_entry (="_start" if it exists). But I do not have any 
strong reason for that.




Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1444
+prot |= PROT_EXEC;
+
+  llvm::Expected Result =

Some sanity check 'permissions' does not have set a bit which 
NativeProcessLinux::AllocateMemory does not understand?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89124

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

[Lldb-commits] [lldb] 5d50109 - [lldb] Update docs with new buildbot URLs

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-10-09T10:57:39-07:00
New Revision: 5d501096ca1fae74f910411cfeb0491d94c635b7

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

LOG: [lldb] Update docs with new buildbot URLs

Buildbot got upgraded and now the (LLDB) builders have different URLs.

Added: 


Modified: 
lldb/docs/resources/bots.rst

Removed: 




diff  --git a/lldb/docs/resources/bots.rst b/lldb/docs/resources/bots.rst
index d9ddcde41abc..926259bd92be 100644
--- a/lldb/docs/resources/bots.rst
+++ b/lldb/docs/resources/bots.rst
@@ -7,11 +7,15 @@ Buildbot
 LLVM Buildbot is the place where volunteers provide build machines. Everyone 
can
 `add a buildbot for LLDB `_.
 
-* `lldb-x64-windows-ninja 
`_
-* `lldb-x86_64-debian `_
-* `lldb-aarch64-ubuntu 
`_
-* `lldb-arm-ubuntu `_
-* `lldb-x86_64-fedora `_
+* `lldb-x64-windows-ninja `_
+* `lldb-x86_64-debian `_
+* `lldb-aarch64-ubuntu `_
+* `lldb-arm-ubuntu `_
+* `lldb-x86_64-fedora `_
+
+An overview of all LLDB builders can be found here:
+
+`http://lab.llvm.org:8011/#/builders?tags=lldb 
`_
 
 GreenDragon
 ---



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

[Lldb-commits] [PATCH] D89155: [lldb] Minidump: check for .text hash match with directory

[Joseph Tremoulet via Phabricator via lldb-commits]

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

When opening a minidump, we might discover that it reports a UUID for a
module that doesn't match the build ID, but rather a hash of the .text
section (according to either of two different hash functions, used by
breakpad and Facebook respectively).  The current logic searches for a
module by filename only to check the hash; this change updates it to
first search by directory+filename.  This is important when the
directory specified in the minidump must be interpreted relative to a
user-provided sysoort, as the leaf directory won't be in the search path
in that case.

Also add a regression test; without this change, module validation fails
because we have just the placeholder module which reports as its path
the platform path in the minidump.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89155

Files:
  lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
  lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py


Index: lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
===
--- lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
+++ lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
@@ -22,11 +22,14 @@
 def verify_module(self, module, verify_path, verify_uuid):
 # Compare the filename and the directory separately. We are avoiding
 # SBFileSpec.fullpath because it causes a slash/backslash confusion
-# on Windows.
+# on Windows.  Similarly, we compare the directories using normcase
+# because they may contain a Linux-style relative path from the
+# minidump appended to a Windows-style root path from the host.
 self.assertEqual(
 os.path.basename(verify_path), module.GetFileSpec().basename)
 self.assertEqual(
-os.path.dirname(verify_path), module.GetFileSpec().dirname or "")
+os.path.normcase(os.path.dirname(verify_path)),
+os.path.normcase(module.GetFileSpec().dirname or ""))
 self.assertEqual(verify_uuid, module.GetUUIDString())
 
 def get_minidump_modules(self, yaml_file):
@@ -201,6 +204,25 @@
 # will check that this matches.
 self.verify_module(modules[0], so_path, "D9C480E8")
 
+def test_breakpad_hash_match_sysroot(self):
+"""
+Check that we can match the breakpad .text section hash when the
+module is located under a user-provided sysroot.
+"""
+sysroot_path = os.path.join(self.getBuildDir(), "mock_sysroot")
+# Create the directory under the sysroot where the minidump reports
+# the module.
+so_dir = os.path.join(sysroot_path, "invalid", "path", "on", 
"current", "system")
+so_path = os.path.join(so_dir, "libbreakpad.so")
+lldbutil.mkdir_p(so_dir)
+self.yaml2obj("libbreakpad.yaml", so_path)
+self.runCmd("platform select remote-linux --sysroot '%s'" % 
sysroot_path)
+modules = 
self.get_minidump_modules("linux-arm-breakpad-uuid-match.yaml")
+self.assertEqual(1, len(modules))
+# LLDB makes up its own UUID as well when there is no build ID so we
+# will check that this matches.
+self.verify_module(modules[0], so_path, "D9C480E8")
+
 def test_breakpad_overflow_hash_match(self):
 """
 This is a similar to test_breakpad_hash_match, but it verifies that
Index: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -523,9 +523,13 @@
   // directories that contain executables that can be searched for matches.
   ModuleSpec basename_module_spec(module_spec);
   basename_module_spec.GetUUID().Clear();
-  basename_module_spec.GetFileSpec().GetDirectory().Clear();
   module_sp = GetTarget().GetOrCreateModule(basename_module_spec,
 true /* notify */, &error);
+  if (!module_sp) {
+basename_module_spec.GetFileSpec().GetDirectory().Clear();
+module_sp = GetTarget().GetOrCreateModule(basename_module_spec,
+  true /* notify */, &error);
+  }
   if (module_sp) {
 // We consider the module to be a match if the minidump UUID is a
 // prefix of the actual UUID, or if either of the UUIDs are empty.


Index: lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
===
--- lldb/test/API/functionalities/postmortem/minidum

[Lldb-commits] [PATCH] D89156: [lldb] GetSharedModule: Collect old modules in SmallVector

[Joseph Tremoulet via Phabricator via lldb-commits]

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

The various GetSharedModule methods have an optional out parameter for
the old module when a file has changed or been replaced, which the
Target uses to keep its module list current/correct.  We've been using
a single ModuleSP to track "the" old module, and this change switches
to using a SmallVector of ModuleSP, which has a couple benefits:

- There are multiple codepaths which may discover an old module, and this 
centralizes the code for how to handle multiples in one place, in the Target 
code.  With the single ModuleSP, each place that may discover an old module is 
responsible for how it handles multiples, and the current code is inconsistent 
(some code paths drop the first old module, others drop the second).
- The API will be more natural for identifying old modules in routines that 
work on sets, like ModuleList::ReplaceEquivalent (which I plan on updating to 
report old module(s) in a subsequent change to fix a bug).

I'm not convinced we can ever actually run into the case that multiple
old modules are found in the same GetOrCreateModule call, but I think
this change makes sense regardless, in light of the above.

When an old module is reported, Target::GetOrCreateModule calls
m_images.ReplaceModule, which doesn't allow multiple "old" modules; the
new code calls ReplaceModule for the first "old" module, and for any
subsequent old modules it logs the event and calls m_images.Remove.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89156

Files:
  lldb/include/lldb/Core/ModuleList.h
  lldb/include/lldb/Target/Platform.h
  lldb/source/Core/ModuleList.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
  lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.h
  lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.h
  lldb/source/Target/Platform.cpp
  lldb/source/Target/Target.cpp

Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -1971,8 +1971,9 @@
 module_sp = m_images.FindFirstModule(module_spec);
 
   if (!module_sp) {
-ModuleSP old_module_sp; // This will get filled in if we have a new version
-// of the library
+llvm::SmallVector
+old_modules; // This will get filled in if we have a new version
+ // of the library
 bool did_create_module = false;
 FileSpecList search_paths = GetExecutableSearchPaths();
 // If there are image search path entries, try to use them first to acquire
@@ -1985,7 +1986,7 @@
 transformed_spec.GetFileSpec().GetFilename() =
 module_spec.GetFileSpec().GetFilename();
 error = ModuleList::GetSharedModule(transformed_spec, module_sp,
-&search_paths, &old_module_sp,
+&search_paths, &old_modules,
 &did_create_module);
   }
 }
@@ -2003,7 +2004,7 @@
 // We have a UUID, it is OK to check the global module list...
 error =
 ModuleList::GetSharedModule(module_spec, module_sp, &search_paths,
-&old_module_sp, &did_create_module);
+&old_modules, &did_create_module);
   }
 
   if (!module_sp) {
@@ -2012,7 +2013,7 @@
 if (m_platform_sp) {
   error = m_platform_sp->GetSharedModule(
   module_spec, m_process_sp.get(), module_sp, &search_paths,
-  &old_module_sp, &did_create_module);
+  &old_modules, &did_create_module);
 } else {
   error.SetErrorString("no platform is currently set");
 }
@@ -2063,18 +2064,18 @@
 // this target. So let's remove the UUID from the module list, and look
 // in the target's module list. Only do this if there is SOMETHING else
 // in the module spec...
-if (!old_module_sp) {
-  if (module_spec.GetUUID().IsValid() &&
-  !module_spec.GetFileSpec().GetFilename().IsEmpty() &&
-  !module_spec.GetFileSpec().GetDirectory().IsEmpty()) {
-ModuleSpec module_spec_copy(module_spec.GetFileSpec());
-module_spec_copy.GetUUID().Clear();
-
-ModuleList found_modules;

[Lldb-commits] [PATCH] D89157: [lldb] Report old modules from ModuleList::ReplaceEquivalent

[Joseph Tremoulet via Phabricator via lldb-commits]

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

This allows the Target to update its module list when loading a shared
module replaces an equivalent one.

A testcase is added which hits this codepath -- without the fix, the
target reports libbreakpad.so twice in its module list.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89157

Files:
  lldb/include/lldb/Core/ModuleList.h
  lldb/source/Core/ModuleList.cpp
  lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py

Index: lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
===
--- lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
+++ lldb/test/API/functionalities/postmortem/minidump-new/TestMiniDumpUUID.py
@@ -29,10 +29,10 @@
 os.path.dirname(verify_path), module.GetFileSpec().dirname or "")
 self.assertEqual(verify_uuid, module.GetUUIDString())
 
-def get_minidump_modules(self, yaml_file):
+def get_minidump_modules(self, yaml_file, exe = None):
 minidump_path = self.getBuildArtifact(os.path.basename(yaml_file) + ".dmp")
 self.yaml2obj(yaml_file, minidump_path)
-self.target = self.dbg.CreateTarget(None)
+self.target = self.dbg.CreateTarget(exe)
 self.process = self.target.LoadCore(minidump_path)
 return self.target.modules
 
@@ -219,6 +219,24 @@
 # will check that this matches.
 self.verify_module(modules[0], so_path, "48EB9FD7")
 
+def test_breakpad_hash_match_exe_outside_sysroot(self):
+"""
+Check that we can match the breakpad .text section hash when the
+module is specified as the exe during launch, and a syroot is
+provided, which does not contain the exe.
+"""
+sysroot_path = os.path.join(self.getBuildDir(), "mock_sysroot")
+lldbutil.mkdir_p(sysroot_path)
+so_dir = os.path.join(self.getBuildDir(), "binary")
+so_path = os.path.join(so_dir, "libbreakpad.so")
+lldbutil.mkdir_p(so_dir)
+self.yaml2obj("libbreakpad.yaml", so_path)
+self.runCmd("platform select remote-linux --sysroot '%s'" % sysroot_path)
+modules = self.get_minidump_modules("linux-arm-breakpad-uuid-match.yaml", so_path)
+self.assertEqual(1, len(modules))
+# LLDB makes up its own UUID as well when there is no build ID so we
+# will check that this matches.
+self.verify_module(modules[0], so_path, "D9C480E8")
 
 def test_facebook_hash_match(self):
 """
Index: lldb/source/Core/ModuleList.cpp
===
--- lldb/source/Core/ModuleList.cpp
+++ lldb/source/Core/ModuleList.cpp
@@ -171,7 +171,9 @@
   AppendImpl(module_sp, notify);
 }
 
-void ModuleList::ReplaceEquivalent(const ModuleSP &module_sp) {
+void ModuleList::ReplaceEquivalent(
+const ModuleSP &module_sp,
+llvm::SmallVectorImpl *old_modules) {
   if (module_sp) {
 std::lock_guard guard(m_modules_mutex);
 
@@ -184,11 +186,14 @@
 
 size_t idx = 0;
 while (idx < m_modules.size()) {
-  ModuleSP module_sp(m_modules[idx]);
-  if (module_sp->MatchesModuleSpec(equivalent_module_spec))
+  ModuleSP test_module_sp(m_modules[idx]);
+  if (test_module_sp->MatchesModuleSpec(equivalent_module_spec)) {
+if (old_modules)
+  old_modules->push_back(test_module_sp);
 RemoveImpl(m_modules.begin() + idx);
-  else
+  } else {
 ++idx;
+  }
 }
 // Now add the new module to the list
 Append(module_sp);
@@ -820,7 +825,7 @@
   *did_create_ptr = true;
 }
 
-shared_module_list.ReplaceEquivalent(module_sp);
+shared_module_list.ReplaceEquivalent(module_sp, old_modules);
 return error;
   }
 }
@@ -857,7 +862,7 @@
 if (did_create_ptr)
   *did_create_ptr = true;
 
-shared_module_list.ReplaceEquivalent(module_sp);
+shared_module_list.ReplaceEquivalent(module_sp, old_modules);
 return Status();
   }
 }
@@ -955,7 +960,7 @@
   if (did_create_ptr)
 *did_create_ptr = true;
 
-  shared_module_list.ReplaceEquivalent(module_sp);
+  shared_module_list.ReplaceEquivalent(module_sp, old_modules);
 }
   } else {
 located_binary_modulespec.GetFileSpec().GetPath(path, sizeof(path));
Index: lldb/include/lldb/Core/ModuleList.h
===
--- lldb/include/lldb/Core/ModuleList.h
+++ lldb/include/lldb/Core/ModuleList.h
@@ -139,7 +139,9 @@
   ///
   /// \param[in] module_sp
   /// A shared pointer to a module to replace in this collection.
-  void ReplaceEquivalent

[Lldb-commits] [PATCH] D89165: [nfc] [lldb] Simplify calling SymbolFileDWARF::GetDWARFCompileUnit

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil created this revision.
jankratochvil added a reviewer: labath.
jankratochvil added a project: LLDB.
Herald added a subscriber: JDevlieghere.
jankratochvil requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.

Only `SymbolFileDWARF::ParseCompileUnit` creates a `CompileUnit` and it uses 
`DWARFCompileUnit` for that.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89165

Files:
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h


Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -346,7 +346,7 @@
 
   lldb::CompUnitSP ParseCompileUnit(DWARFCompileUnit &dwarf_cu);
 
-  virtual DWARFUnit *
+  virtual DWARFCompileUnit *
   GetDWARFCompileUnit(lldb_private::CompileUnit *comp_unit);
 
   DWARFUnit *GetNextUnparsedDWARFCompileUnit(DWARFUnit *prev_cu);
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -632,8 +632,7 @@
   return *m_info;
 }
 
-DWARFUnit *
-SymbolFileDWARF::GetDWARFCompileUnit(lldb_private::CompileUnit *comp_unit) {
+DWARFCompileUnit *SymbolFileDWARF::GetDWARFCompileUnit(CompileUnit *comp_unit) 
{
   if (!comp_unit)
 return nullptr;
 
@@ -641,7 +640,9 @@
   DWARFUnit *dwarf_cu = DebugInfo().GetUnitAtIndex(comp_unit->GetID());
   if (dwarf_cu && dwarf_cu->GetUserData() == nullptr)
 dwarf_cu->SetUserData(comp_unit);
-  return dwarf_cu;
+
+  // It must be DWARFCompileUnit when it created a CompileUnit.
+  return llvm::cast_or_null(dwarf_cu);
 }
 
 DWARFDebugRanges *SymbolFileDWARF::GetDebugRanges() {
@@ -1599,8 +1600,7 @@
 llvm::Optional SymbolFileDWARF::GetDWOId() {
   if (GetNumCompileUnits() == 1) {
 if (auto comp_unit = GetCompileUnitAtIndex(0))
-  if (DWARFCompileUnit *cu = llvm::dyn_cast_or_null(
-  GetDWARFCompileUnit(comp_unit.get(
+  if (DWARFCompileUnit *cu = GetDWARFCompileUnit(comp_unit.get()))
 if (DWARFDebugInfoEntry *cu_die = cu->DIE().GetDIE())
   if (uint64_t dwo_id = ::GetDWOId(*cu, *cu_die))
 return dwo_id;


Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -346,7 +346,7 @@
 
   lldb::CompUnitSP ParseCompileUnit(DWARFCompileUnit &dwarf_cu);
 
-  virtual DWARFUnit *
+  virtual DWARFCompileUnit *
   GetDWARFCompileUnit(lldb_private::CompileUnit *comp_unit);
 
   DWARFUnit *GetNextUnparsedDWARFCompileUnit(DWARFUnit *prev_cu);
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -632,8 +632,7 @@
   return *m_info;
 }
 
-DWARFUnit *
-SymbolFileDWARF::GetDWARFCompileUnit(lldb_private::CompileUnit *comp_unit) {
+DWARFCompileUnit *SymbolFileDWARF::GetDWARFCompileUnit(CompileUnit *comp_unit) {
   if (!comp_unit)
 return nullptr;
 
@@ -641,7 +640,9 @@
   DWARFUnit *dwarf_cu = DebugInfo().GetUnitAtIndex(comp_unit->GetID());
   if (dwarf_cu && dwarf_cu->GetUserData() == nullptr)
 dwarf_cu->SetUserData(comp_unit);
-  return dwarf_cu;
+
+  // It must be DWARFCompileUnit when it created a CompileUnit.
+  return llvm::cast_or_null(dwarf_cu);
 }
 
 DWARFDebugRanges *SymbolFileDWARF::GetDebugRanges() {
@@ -1599,8 +1600,7 @@
 llvm::Optional SymbolFileDWARF::GetDWOId() {
   if (GetNumCompileUnits() == 1) {
 if (auto comp_unit = GetCompileUnitAtIndex(0))
-  if (DWARFCompileUnit *cu = llvm::dyn_cast_or_null(
-  GetDWARFCompileUnit(comp_unit.get(
+  if (DWARFCompileUnit *cu = GetDWARFCompileUnit(comp_unit.get()))
 if (DWARFDebugInfoEntry *cu_die = cu->DIE().GetDIE())
   if (uint64_t dwo_id = ::GetDWOId(*cu, *cu_die))
 return dwo_id;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D89155: [lldb] Minidump: check for .text hash match with directory

[Greg Clayton via Phabricator via lldb-commits]

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



Comment at: lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp:533
+  }
   if (module_sp) {
 // We consider the module to be a match if the minidump UUID is a

This whole "if" statement will need to be tried with both files. We might end 
up finding a file in the sysroot that is a valid file, but does not actually 
match the .text hash. Then the search by basename, which will search using the 
"target.exec-search-paths" or "target.debug-file-search-paths" settings might 
find another file that you might have downloaded, and it will need to go 
through all of the .text checks as well.

So as we now have more than 1 file to check, this "if (module_sp)" and its 
contents should be made into a function. Look at the code above I would create 
a method of ProcessMinidump that does all this:

```
ModuleSP ProcessMinidump::GetOrCreateModule(UUID minidump_uuid, ModuleSpec 
module_spec) {
  ModuleSP module_sp = GetTarget().GetOrCreateModule(module_spec, true /* 
notify */, &error);
  if (!module_sp)
return module_sp;
  // We consider the module to be a match if the minidump UUID is a
  // prefix of the actual UUID, or if either of the UUIDs are empty.
  const auto dmp_bytes = minidump_uuid.GetBytes();
  const auto mod_bytes = module_sp->GetUUID().GetBytes();
  const bool match = dmp_bytes.empty() || mod_bytes.empty() ||
  mod_bytes.take_front(dmp_bytes.size()) == dmp_bytes;
  if (match) {
LLDB_LOG(log, "Partial uuid match for {0}.", name);
return module_sp;
  }

  // Breakpad generates minindump files, and if there is no GNU build
  // ID in the binary, it will calculate a UUID by hashing first 4096
  // bytes of the .text section and using that as the UUID for a module
  // in the minidump. Facebook uses a modified breakpad client that
  // uses a slightly modified this hash to avoid collisions. Check for
  // UUIDs from the minindump that match these cases and accept the
  // module we find if they do match.
  std::vector breakpad_uuid;
  std::vector facebook_uuid;
  HashElfTextSection(module_sp, breakpad_uuid, facebook_uuid);
  if (dmp_bytes == llvm::ArrayRef(breakpad_uuid)) {
LLDB_LOG(log, "Breakpad .text hash match for {0}.", name);
return module_sp;
  }
  if (dmp_bytes == llvm::ArrayRef(facebook_uuid)) {
LLDB_LOG(log, "Facebook .text hash match for {0}.", name);
return module_sp;
  }
  // The UUID wasn't a partial match and didn't match the .text hash
  // so remove the module from the target, we will need to create a
  // placeholder object file.
  GetTarget().GetImages().Remove(module_sp);
  module_sp.reset();
  return module_sp;
}
```

Then call this function once with basename_module_spec with a full path, and if 
no matching module is returned, call the remove the directory and call it again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89155

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

[Lldb-commits] [PATCH] D88841: [intel pt] Refactor parsing

[Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGea1f49741ec4: [intel pt] Refactor parsing (authored by 
Walter Erquinigo ).

Changed prior to commit:
  https://reviews.llvm.org/D88841?vs=297075&id=297368#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88841

Files:
  lldb/include/lldb/Core/PluginManager.h
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Target/TraceSessionFileParser.h
  lldb/include/lldb/Target/TraceSettingsParser.h
  lldb/include/lldb/lldb-forward.h
  lldb/include/lldb/lldb-private-interfaces.h
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Core/PluginManager.cpp
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.h
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceSessionFileParser.cpp
  lldb/source/Target/TraceSettingsParser.cpp
  lldb/test/API/commands/trace/TestTraceLoad.py
  lldb/test/API/commands/trace/TestTraceSchema.py

Index: lldb/test/API/commands/trace/TestTraceSchema.py
===
--- lldb/test/API/commands/trace/TestTraceSchema.py
+++ lldb/test/API/commands/trace/TestTraceSchema.py
@@ -20,3 +20,15 @@
 def testInvalidPluginSchema(self):
 self.expect("trace schema invalid-plugin", error=True,
 substrs=['error: no trace plug-in matches the specified type: "invalid-plugin"'])
+
+def testAllSchemas(self):
+self.expect("trace schema all", substrs=['''{
+  "trace": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor": "intel" | "unknown",
+  "family": integer,
+  "model": integer,
+  "stepping": integer
+}
+  },'''])
Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -40,7 +40,7 @@
 src_dir = self.getSourceDir()
 # We test first an invalid type
 self.expect("trace load -v " + os.path.join(src_dir, "intelpt-trace", "trace_bad.json"), error=True,
-  substrs=['''error: expected object at settings.processes[0]
+  substrs=['''error: expected object at traceSession.processes[0]
 
 Context:
 {
@@ -53,7 +53,7 @@
 
 Schema:
 {
- "trace": {
+  "trace": {
 "type": "intel-pt",
 "pt_cpu": {
   "vendor": "intel" | "unknown",
@@ -63,32 +63,35 @@
 }
   },'''])
 
-# Now we test a missing field in the global settings
+# Now we test a missing field in the global session file
 self.expect("trace load -v " + os.path.join(src_dir, "intelpt-trace", "trace_bad2.json"), error=True,
-substrs=['error: missing value at settings.processes[1].triple', "Context", "Schema"])
+substrs=['error: missing value at traceSession.processes[1].triple', "Context", "Schema"])
 
 # Now we test a missing field in the intel-pt settings
 self.expect("trace load -v " + os.path.join(src_dir, "intelpt-trace", "trace_bad4.json"), error=True,
-substrs=['''error: missing value at settings.trace.pt_cpu.family
+substrs=['''error: missing value at traceSession.trace.pt_cpu.family
 
 Context:
 {
-  "pt_cpu": /* error: missing value */ {
-"model": 79,
-"stepping": 1,
-"vendor": "intel"
-  },
-  "type": "intel-pt"
+  "processes": [],
+  "trace": {
+"pt_cpu": /* error: missing value */ {
+  "model": 79,
+  "stepping": 1,
+  "vendor": "intel"
+},
+"type": "intel-pt"
+  }
 }''', "Schema"])
 
 # Now we test an incorrect load address in the intel-pt settings
 self.expect("trace load -v " + os.path.join(src_dir, "intelpt-trace", "trace_bad5.json"), error=True,
-substrs=['error: expected numeric string at settings.processes[0].modules[0].loadAddress',
+substrs=['error: expected numeric string at traceSession.processes[0].modules[0].loadAddress',
  '"loadAddress": /* error: expected numeric string */ 40,', "Schema"])
 
 # The following wrong schema will have a valid target and an invalid one. In the case of failure,
 # no targets should be created.
 self.assertEqual(self.dbg.GetNumT

[Lldb-commits] [lldb] ea1f497 - [intel pt] Refactor parsing

[Walter Erquinigo via lldb-commits]

Author: Walter Erquinigo
Date: 2020-10-09T17:32:04-07:00
New Revision: ea1f49741ec4c0a37796b88f6bb8d09d7ab1e8c3

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

LOG: [intel pt] Refactor parsing

With the feedback I was getting in different diffs, I realized that splitting 
the parsing logic into two classes was not easy to deal with. I do see value in 
doing that, but I'd rather leave that as a refactor after most of the intel-pt 
logic is in place. Thus, I'm merging the common parser into the intel pt one, 
having thus only one that is fully aware of Intel PT during parsing and object 
creation.

Besides, based on the feedback in https://reviews.llvm.org/D88769, I'm creating 
a ThreadIntelPT class that will be able to orchestrate decoding of its own 
trace and can handle the stop events correctly.

This leaves the TraceIntelPT class as an initialization class that glues 
together different components. Right now it can initialize a trace session from 
a json file, and in the future will be able to initialize a trace session from 
a live process.

Besides, I'm renaming SettingsParser to SessionParser, which I think is a 
better name, as the json object represents a trace session of possibly many 
processes.

With the current set of targets, we have the following

- Trace: main interface for dealing with trace sessions
- TraceIntelPT: plugin Trace for dealing with intel pt sessions
- TraceIntelPTSessionParser: a parser of a json trace session file that can 
create a corresponding TraceIntelPT instance along with Targets, ProcessTraces 
(to be created in https://reviews.llvm.org/D88769), and ThreadIntelPT threads.
- ProcessTrace: (to be created in https://reviews.llvm.org/D88769) can handle 
the correct state of the traces as the user traverses the trace. I don't think 
there'll be a need an intel-pt specific implementation of this class.
- ThreadIntelPT: a thread implementation that can handle the decoding of its 
own trace file, along with keeping track of the current position the user is 
looking at when doing reverse debugging.

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

Added: 
lldb/include/lldb/Target/TraceSessionFileParser.h
lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.cpp
lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.h
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h
lldb/source/Target/TraceSessionFileParser.cpp

Modified: 
lldb/include/lldb/Core/PluginManager.h
lldb/include/lldb/Target/Trace.h
lldb/include/lldb/lldb-forward.h
lldb/include/lldb/lldb-private-interfaces.h
lldb/source/Commands/CommandObjectTrace.cpp
lldb/source/Core/PluginManager.cpp
lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
lldb/source/Target/CMakeLists.txt
lldb/source/Target/Trace.cpp
lldb/test/API/commands/trace/TestTraceLoad.py
lldb/test/API/commands/trace/TestTraceSchema.py

Removed: 
lldb/include/lldb/Target/TraceSettingsParser.h
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.cpp
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.h
lldb/source/Target/TraceSettingsParser.cpp



diff  --git a/lldb/include/lldb/Core/PluginManager.h 
b/lldb/include/lldb/Core/PluginManager.h
index cd962b668163..77ace59a98b8 100644
--- a/lldb/include/lldb/Core/PluginManager.h
+++ b/lldb/include/lldb/Core/PluginManager.h
@@ -332,12 +332,35 @@ class PluginManager {
 
   // Trace
   static bool RegisterPlugin(ConstString name, const char *description,
- TraceCreateInstance create_callback);
+ TraceCreateInstance create_callback,
+ llvm::StringRef schema);
 
   static bool UnregisterPlugin(TraceCreateInstance create_callback);
 
   static TraceCreateInstance GetTraceCreateCallback(ConstString plugin_name);
 
+  /// Get the JSON schema for a trace session file corresponding to the given
+  /// plugin.
+  ///
+  /// \param[in] plugin_name
+  /// The name of the plugin.
+  ///
+  /// \return
+  /// An empty \a StringRef if no plugin was found with that plugin name,
+  /// otherwise the actual schema is returned.
+  static llvm::StringRef GetTraceSchema(ConstString plugin_name);
+
+  /// Get the JSON schema for a trace session file corresponding to the plugin
+  /// given by its index.
+  ///
+  /// \param[in] index
+  /// The index of the plugin to get the schema of.
+  ///
+  /// \return
+  /// An empty \a StringRef if the index is greater than or equal to the
+  /// number plugins, otherwise the actual sche

[Lldb-commits] [PATCH] D88769: [trace] Scaffold "thread trace dump instructions"

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/source/Target/Thread.cpp:1617-1627
+void Thread::DumpTraceInstructions(Stream &s, size_t count,
+   size_t start_position) const {
+  if (!GetProcess()->GetTarget().GetTrace()) {
+s << "error: no trace in this target\n";
+return;
+  }
+  s.Printf("thread #%u: tid = %" PRIu64 ", total instructions = 1000\n",

labath wrote:
> Given the intended design of having one process (and thread) class serve 
> multiple trace types, can this function do anything useful besides calling 
> into the Trace object to perform trace-specific dumping ?
> 
> And if it cannot, couldn't we just skip it have have callers call the 
> relevant Trace method directly?
That's a very good point! There's really no need to have this method here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88769

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

[Lldb-commits] [PATCH] D88769: [trace] Scaffold "thread trace dump instructions"

[walter erquinigo via Phabricator via lldb-commits]

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



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:61
+ const std::vector &targets) {
+  TraceSP trace_instance(new TraceIntelPT(pt_cpu, targets));
+  for (const TargetSP &target_sp : targets)

labath wrote:
> auto trace_instance = std::make_shared(...)
I can't do this because the constructor is private (specifically to 
std::shared_ptr).
I want to have this method as the main way to create instances because I need 
to use shared_ptr of this instance to set up everything correctly, and I can't 
do it from the constructor itself.
I've checked ways to make std::make_shared(...), but it looks 
like too much code that outweighs the benefit of the fancy allocator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88769

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

[Lldb-commits] [PATCH] D88769: [trace] Scaffold "thread trace dump instructions"

[walter erquinigo via Phabricator via lldb-commits]

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

- Moved the thread dumping logic to Trace.h.
- Did the small fixes that were requested.
- Left a comment regarding the shared_ptr instantiation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88769

Files:
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Target/Thread.h
  lldb/include/lldb/Target/Trace.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Process/CMakeLists.txt
  lldb/source/Plugins/Process/Trace/CMakeLists.txt
  lldb/source/Plugins/Process/Trace/ProcessTrace.cpp
  lldb/source/Plugins/Process/Trace/ProcessTrace.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/Trace.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceLoad.py
  lldb/test/API/commands/trace/intelpt-trace/trace_2threads.json

Index: lldb/test/API/commands/trace/intelpt-trace/trace_2threads.json
===
--- /dev/null
+++ lldb/test/API/commands/trace/intelpt-trace/trace_2threads.json
@@ -0,0 +1,35 @@
+{
+  "trace": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor": "intel",
+  "family": 6,
+  "model": 79,
+  "stepping": 1
+}
+  },
+  "processes": [
+{
+  "pid": 1234,
+  "triple": "x86_64-*-linux",
+  "threads": [
+{
+  "tid": 3842849,
+  "traceFile": "3842849.trace"
+},
+{
+  "tid": 3842850,
+  "traceFile": "3842849.trace"
+}
+  ],
+  "modules": [
+{
+  "file": "a.out",
+  "systemPath": "a.out",
+  "loadAddress": "0x0040",
+  "uuid": "6AA9A4E2-6F28-2F33-377D-59FECE874C71-5B41261A"
+}
+  ]
+}
+  ]
+}
Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -35,6 +35,10 @@
 
 self.assertEqual("6AA9A4E2-6F28-2F33-377D-59FECE874C71-5B41261A", module.GetUUIDString())
 
+# check that the Process and Thread objects were created correctly
+self.expect("thread info", substrs=["tid = 3842849"])
+self.expect("thread list", substrs=["Process 1234 stopped", "tid = 3842849"])
+
 
 def testLoadInvalidTraces(self):
 src_dir = self.getSourceDir()
Index: lldb/test/API/commands/trace/TestTraceDumpInstructions.py
===
--- /dev/null
+++ lldb/test/API/commands/trace/TestTraceDumpInstructions.py
@@ -0,0 +1,92 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+
+class TestTraceDumpInstructions(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+def setUp(self):
+TestBase.setUp(self)
+if 'intel-pt' not in configuration.enabled_plugins:
+self.skipTest("The intel-pt test plugin is not enabled")
+
+def testErrorMessages(self):
+# We first check the output when there are no targets
+self.expect("thread trace dump instructions",
+substrs=["error: invalid target, create a target using the 'target create' command"],
+error=True)
+
+# We now check the output when there's a non-running target
+self.expect("target create " + os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
+
+self.expect("thread trace dump instructions",
+substrs=["error: invalid process"],
+error=True)
+
+# Now we check the output when there's a running target without a trace
+self.expect("b main")
+self.expect("run")
+
+self.expect("thread trace dump instructions",
+substrs=["error: this thread is not being traced"],
+error=True)
+
+def testDumpInstructions(self):
+self.expect("trace load -v " + os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"),
+substrs=["intel-pt"])
+
+self.expect("thread trace dump instructions",
+substrs=['thread #1: tid = 3842849, total instructions = 1000',
+ 'would print 20 instructions from position 0'])
+
+# We check if we can pass count and offset
+self.expect("thread trace dump instructions --count 5 --start-position 10",
+substrs=['thread #1: tid = 3842849, total instructions = 1000',
+ 'would print 5 instructions from position 10'])
+
+# We check if we can access the thr