[Lldb-commits] [PATCH] D105180: [lldb][AArch64] Add memory tag writing to lldb-server

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1550
+size_t tags_written = tags_vec.iov_len;
+assert(tags_written && (tags_written <= num_tags));
+

This assertion suggests something went wrong on ptrace side. Should we replace 
it with a simple error return? or you think inferior program should no longer 
be debugable after such an error?



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:3567
+
+  // This is bytes here and is unpacked into target specific tags later
+  // We cannot assume that number of bytes == length here because the server

by this you mean packet?



Comment at: 
lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py:56
 # Run the packet stream
 context = self.expect_gdbremote_sequence()
 self.assertIsNotNone(context)

DavidSpickett wrote:
> omjavaid wrote:
> > This test is still not stable given we wait for stop notification while 
> > process may have exited. This causes test to hang till timeout expiry.
> Sure, but I don't think there's a better way to do it. Short of reading from 
> a process that's already exited. (which seems to sort of work for memory but 
> tags have weird results so I don't think it's meant to work)
> 
> Note that the test file does pause after the print: 
> https://github.com/llvm/llvm-project/blob/main/lldb/test/API/tools/lldb-server/memory-tagging/main.c#L16
> 
> We've got a few scenarios:
> * All goes well, we get the print. The test program is in it's delay and we 
> stop it.
> * The test crashes at some point before sending the stop. The test program 
> waits 60s then exits.
> * The test program crashes before printing. The test waits on the print until 
> it times out.
> * The test picks up the print just as the test program is exiting, or has 
> already exited. The test times out waiting for the stop response.
> 
> It's not free of timing issues but it means that you won't have either test 
> or test program hanging around if either fails.
> 
> What do you think?
The problem I was facing was lldb-server existed after inferior launch. I have 
given my configuration below I might be doing something wrong:
LLDB_TEST_COMPILER=/home/omair/work/tools/gcc-linaro-11.0.0-2021.02-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-gcc
LLDB_TEST_ARCH=aarch64

LLDB_PLATFORM_NAME=remote-linux
LLDB_PLATFORM_URL=connect://192.168.53.253:54321
#-u CXXFLAGS -u CFLAGS --env ARCHIVER=/usr/bin/ar --env 
OBJCOPY=/usr/bin/objcopy \

$LLDB_BUILD_DIR/bin/lldb-dotest \
-A $LLDB_TEST_ARCH \
-C $LLDB_TEST_COMPILER \
--build-dir $LLDB_BUILD_DIR/lldb-test-build.noindex \
--executable $LLDB_BUILD_DIR/bin/lldb \
--dsymutil $LLDB_BUILD_DIR/bin/dsymutil \
--llvm-tools-dir $LLDB_BUILD_DIR/bin \
--lldb-libs-dir $LLDB_BUILD_DIR/lib \
--platform-name=$LLDB_PLATFORM_NAME \
--platform-url=$LLDB_PLATFORM_URL \
-p $1 $2 $3



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105180

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

[Lldb-commits] [PATCH] D105717: [trace] [intel pt] Create a "thread trace dump stats" command

[hanbing wang via Phabricator via lldb-commits]

hanbingwang updated this revision to Diff 359253.
hanbingwang edited the summary of this revision.
hanbingwang added a comment.

1. rename "thread trace dump stats" to "thread trace dump info"
2. Additionally, print out the tracing plugin name.


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

https://reviews.llvm.org/D105717

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/test/API/commands/trace/TestTraceDumpInfo.py
  lldb/test/API/commands/trace/TestTraceLoad.py

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -33,7 +33,9 @@
 # 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"])
-
+self.expect("thread trace dump info", substrs=['''Tracing technology: intel-pt
+thread #1: tid = 3842849
+Raw trace size: 4096 bytes'''])
 
 def testLoadInvalidTraces(self):
 src_dir = self.getSourceDir()
Index: lldb/test/API/commands/trace/TestTraceDumpInfo.py
===
--- /dev/null
+++ lldb/test/API/commands/trace/TestTraceDumpInfo.py
@@ -0,0 +1,40 @@
+import lldb
+from intelpt_testcase import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+
+class TestTraceDumpInfo(TraceIntelPTTestCaseBase):
+mydir = TestBase.compute_mydir(__file__)
+
+def testErrorMessages(self):
+# We first check the output when there are no targets
+self.expect("thread trace dump info",
+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 info",
+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 info",
+substrs=["error: Process is not being traced"],
+error=True)
+
+def testDumpRawTraceSize(self):
+self.expect("trace load -v " +
+os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"),
+substrs=["intel-pt"])
+
+self.expect("thread trace dump info",
+substrs=['''Tracing technology: intel-pt
+thread #1: tid = 3842849
+Raw trace size: 4096 bytes'''])
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
@@ -67,6 +67,10 @@
 
   lldb::TraceCursorUP GetCursor(Thread &thread) override;
 
+  void DumpTraceInfo(Thread &thread, Stream &s, bool verbose, lldb_private::ConstString plugin_name) override;
+
+  llvm::Optional GetRawTraceSize(Thread &thread);
+
   void DoRefreshLiveProcessState(
   llvm::Expected state) override;
 
Index: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
+++ lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
@@ -108,6 +108,24 @@
   return Decode(thread)->GetCursor();
 }
 
+void TraceIntelPT::DumpTraceInfo(Thread &thread, Stream &s, bool verbose, lldb_private::ConstString plugin_name) {
+  Optional raw_size = GetRawTraceSize(thread);
+  s.Printf("Tracing technology: %s\nthread #%u: tid = %" PRIu64, plugin_name.AsCString(), thread.GetIndexID(), thread.GetID());
+  if (!raw_size) {
+s.Printf(", not traced\n");
+return;
+  }
+  s.Printf("\nRaw trace size: %zu bytes\n", *raw_size);
+  return;
+}
+
+Optional TraceIntelPT::GetRawTraceSize(Thread &thread) {
+  if (IsTraced(thread))
+return Decode(thread)->GetRawTraceSize();
+  else
+return None;
+}
+
 Expected TraceIntelPT::GetCPUInfoForLiveProcess() {
   Expected> cpu_info = GetLiveProcessBinaryData("cpuInfo");
   if (!cpu_info)
Index: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
===
--- lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
+++ lldb/so

[Lldb-commits] [PATCH] D105630: [lldb][AArch64] Refactor memory tag range handling

[David Spickett via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd046fb62b7e7: [lldb][AArch64] Refactor memory tag range 
handling (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105630

Files:
  lldb/include/lldb/Target/MemoryTagManager.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Commands/CommandObjectMemoryTag.cpp
  lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp
  lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h
  lldb/source/Target/Process.cpp
  lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp

Index: lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
===
--- lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
+++ lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
@@ -94,6 +94,121 @@
   manager.ExpandToGranule(MemoryTagManagerAArch64MTE::TagRange(18, 4)));
 }
 
+static MemoryRegionInfo MakeRegionInfo(lldb::addr_t base, lldb::addr_t size,
+   bool tagged) {
+  return MemoryRegionInfo(
+  MemoryRegionInfo::RangeType(base, size), MemoryRegionInfo::eYes,
+  MemoryRegionInfo::eYes, MemoryRegionInfo::eYes, MemoryRegionInfo::eYes,
+  ConstString(), MemoryRegionInfo::eNo, 0,
+  /*memory_tagged=*/
+  tagged ? MemoryRegionInfo::eYes : MemoryRegionInfo::eNo);
+}
+
+TEST(MemoryTagManagerAArch64MTETest, MakeTaggedRange) {
+  MemoryTagManagerAArch64MTE manager;
+  MemoryRegionInfos memory_regions;
+
+  // No regions means no tagged regions, error
+  ASSERT_THAT_EXPECTED(
+  manager.MakeTaggedRange(0, 0x10, memory_regions),
+  llvm::FailedWithMessage(
+  "Address range 0x0:0x10 is not in a memory tagged region"));
+
+  // Alignment is done before checking regions.
+  // Here 1 is rounded up to the granule size of 0x10.
+  ASSERT_THAT_EXPECTED(
+  manager.MakeTaggedRange(0, 1, memory_regions),
+  llvm::FailedWithMessage(
+  "Address range 0x0:0x10 is not in a memory tagged region"));
+
+  // Range must not be inverted
+  ASSERT_THAT_EXPECTED(
+  manager.MakeTaggedRange(1, 0, memory_regions),
+  llvm::FailedWithMessage(
+  "End address (0x0) must be greater than the start address (0x1)"));
+
+  // Adding a single region to cover the whole range
+  memory_regions.push_back(MakeRegionInfo(0, 0x1000, true));
+
+  // Range can have different tags for begin and end
+  // (which would make it look inverted if we didn't remove them)
+  // Note that range comes back with an untagged base and alginment
+  // applied.
+  MemoryTagManagerAArch64MTE::TagRange expected_range(0x0, 0x10);
+  llvm::Expected got =
+  manager.MakeTaggedRange(0x0f00, 0x0e01,
+  memory_regions);
+  ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+  ASSERT_EQ(*got, expected_range);
+
+  // Error if the range isn't within any region
+  ASSERT_THAT_EXPECTED(
+  manager.MakeTaggedRange(0x1000, 0x1010, memory_regions),
+  llvm::FailedWithMessage(
+  "Address range 0x1000:0x1010 is not in a memory tagged region"));
+
+  // Error if the first part of a range isn't tagged
+  memory_regions.clear();
+  const char *err_msg =
+  "Address range 0x0:0x1000 is not in a memory tagged region";
+
+  // First because it has no region entry
+  memory_regions.push_back(MakeRegionInfo(0x10, 0x1000, true));
+  ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+   llvm::FailedWithMessage(err_msg));
+
+  // Then because the first region is untagged
+  memory_regions.push_back(MakeRegionInfo(0, 0x10, false));
+  ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+   llvm::FailedWithMessage(err_msg));
+
+  // If we tag that first part it succeeds
+  memory_regions.back().SetMemoryTagged(MemoryRegionInfo::eYes);
+  expected_range = MemoryTagManagerAArch64MTE::TagRange(0x0, 0x1000);
+  got = manager.MakeTaggedRange(0, 0x1000, memory_regions);
+  ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
+  ASSERT_EQ(*got, expected_range);
+
+  // Error if the end of a range is untagged
+  memory_regions.clear();
+
+  // First because it has no region entry
+  memory_regions.push_back(MakeRegionInfo(0, 0xF00, true));
+  ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+   llvm::FailedWithMessage(err_msg));
+
+  // Then because the last region is untagged
+  memory_regions.push_back(MakeRegionInfo(0xF00, 0x100, false));
+  ASSERT_THAT_EXPECTED(manager.MakeTaggedRange(0, 0x1000, memory_regions),
+   llvm::FailedWithMessage(err_msg));
+
+  // If we tag the last part it succeeds
+  memory_regions.back().SetMemoryTag

[Lldb-commits] [lldb] d046fb6 - [lldb][AArch64] Refactor memory tag range handling

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2021-07-16T11:02:06+01:00
New Revision: d046fb62b7e7cd273b104fee0162725003411c00

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

LOG: [lldb][AArch64] Refactor memory tag range handling

Previously GetMemoryTagManager checked many things in one:
* architecture supports memory tagging
* process supports memory tagging
* memory range isn't inverted
* memory range is all tagged

Since writing follow up patches for tag writing (in review
at the moment) it has become clear that this gets unwieldy
once we add the features needed for that.

It also implies that the memory tag manager is tied to the
range you used to request it with but it is not. It's a per
process object.

Instead:
* GetMemoryTagManager just checks architecture and process.
* Then the MemoryTagManager can later be asked to check a
  memory range.

This is better because:
* We don't imply that range and manager are tied together.
* A slightly diferent range calculation for tag writing
  doesn't add more code to Process.
* Range checking code can now be unit tested.

Reviewed By: omjavaid

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

Added: 


Modified: 
lldb/include/lldb/Target/MemoryTagManager.h
lldb/include/lldb/Target/Process.h
lldb/source/Commands/CommandObjectMemoryTag.cpp
lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp
lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h
lldb/source/Target/Process.cpp
lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp

Removed: 




diff  --git a/lldb/include/lldb/Target/MemoryTagManager.h 
b/lldb/include/lldb/Target/MemoryTagManager.h
index f6383d82efcd7..65abfcd763827 100644
--- a/lldb/include/lldb/Target/MemoryTagManager.h
+++ b/lldb/include/lldb/Target/MemoryTagManager.h
@@ -9,6 +9,7 @@
 #ifndef LLDB_TARGET_MEMORYTAGMANAGER_H
 #define LLDB_TARGET_MEMORYTAGMANAGER_H
 
+#include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Utility/RangeMap.h"
 #include "lldb/lldb-private.h"
 #include "llvm/Support/Error.h"
@@ -57,6 +58,20 @@ class MemoryTagManager {
   // expanded to 2 granules.
   virtual TagRange ExpandToGranule(TagRange range) const = 0;
 
+  // Given a range addr to end_addr, check that:
+  // * end_addr >= addr (when memory tags are removed)
+  // * the granule aligned range is completely covered by tagged memory
+  //   (which may include one or more memory regions)
+  //
+  // If so, return a modified range which will have been expanded
+  // to be granule aligned.
+  //
+  // Tags in the input addresses are ignored and not present
+  // in the returned range.
+  virtual llvm::Expected MakeTaggedRange(
+  lldb::addr_t addr, lldb::addr_t end_addr,
+  const lldb_private::MemoryRegionInfos &memory_regions) const = 0;
+
   // Return the type value to use in GDB protocol qMemTags packets to read
   // allocation tags. This is named "Allocation" specifically because the spec
   // allows for logical tags to be read the same way, though we do not use 
that.

diff  --git a/lldb/include/lldb/Target/Process.h 
b/lldb/include/lldb/Target/Process.h
index b125a38636c8b..2457ac31c19db 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -1709,30 +1709,19 @@ class Process : public 
std::enable_shared_from_this,
   lldb::addr_t CallocateMemory(size_t size, uint32_t permissions,
Status &error);
 
-  /// If the address range given is in a memory tagged range and this
-  /// architecture and process supports memory tagging, return a tag
+  /// If this architecture and process supports memory tagging, return a tag
   /// manager that can be used to maniupulate those memory tags.
-  /// Tags present in the addresses given are ignored.
-  ///
-  /// \param[in] addr
-  /// Start of memory range.
-  ///
-  /// \param[in] end_addr
-  /// End of the memory range. Where end is one beyond the last byte to be
-  /// included.
   ///
   /// \return
   /// Either a valid pointer to a tag manager or an error describing why 
one
   /// could not be provided.
-  llvm::Expected
-  GetMemoryTagManager(lldb::addr_t addr, lldb::addr_t end_addr);
+  llvm::Expected GetMemoryTagManager();
 
-  /// Expands the range addr to addr+len to align with granule boundaries and
-  /// then calls DoReadMemoryTags to do the target specific operations.
-  /// Tags are returned unpacked so can be used without conversion.
+  /// Read memory tags for the range addr to addr+len. It is assumed
+  /// that this range has already been granule aligned.
+  /// (see MemoryTagManager::MakeTaggedRange)
   ///
-  /// \param[in] tag_manager
-  /// The tag manager to get memory tagging information from.
+  /// This calls D

[Lldb-commits] [PATCH] D102428: [StopInfoMachException] Summarize arm64e BLRAx/LDRAx auth failures

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: omjavaid.
DavidSpickett added a comment.

Apologies, I missed the ping. A couple of minor comments otherwise this looks 
good to me.

@omjavaid Should take a look too as he's doing/did the PAC work for Arm Linux. 
I figure this can go in for Mac and be made generic later as needed. (e.g. Arm 
Linux won't have the special brk bits)




Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:259
 
+case llvm::Triple::aarch64: {
+  if (DeterminePtrauthFailure(exe_ctx))

Don't need the {} here.



Comment at: lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp:376
 
+case llvm::Triple::aarch64: {
+  if (DeterminePtrauthFailure(exe_ctx))

Also unneeded {} here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102428

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

[Lldb-commits] [PATCH] D105178: [lldb][AArch64] Annotate synchronous tag faults

[David Spickett via Phabricator via lldb-commits]

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

Account for fault addresses not being granule aligned by asking
for fault_addr to fault_addr+1 which always gives you 1 tag back.
(same trick we do in "memory tag read" if we only have a start address)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105178

Files:
  lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeThreadLinux.h
  lldb/test/API/linux/aarch64/mte_tag_faults/Makefile
  
lldb/test/API/linux/aarch64/mte_tag_faults/TestAArch64LinuxMTEMemoryTagFaults.py
  lldb/test/API/linux/aarch64/mte_tag_faults/main.c

Index: lldb/test/API/linux/aarch64/mte_tag_faults/main.c
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/mte_tag_faults/main.c
@@ -0,0 +1,59 @@
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+// Set bits 59-56 to tag, removing any existing tag
+static char *set_tag(char *ptr, size_t tag) {
+  return (char *)(((size_t)ptr & ~((size_t)0xf << 56)) | (tag << 56));
+}
+
+int main(int argc, char const *argv[]) {
+  // We assume that the test runner has checked we're on an MTE system
+
+  // Only expect to get the fault type
+  if (argc != 2)
+return 1;
+
+  unsigned long prctl_arg2 = 0;
+  if (!strcmp(argv[1], "sync"))
+prctl_arg2 = PR_MTE_TCF_SYNC;
+  else if (!strcmp(argv[1], "async"))
+prctl_arg2 = PR_MTE_TCF_ASYNC;
+  else
+return 1;
+
+  // Set fault type
+  if (prctl(PR_SET_TAGGED_ADDR_CTRL, prctl_arg2, 0, 0, 0))
+return 1;
+
+  // Allocate some memory with tagging enabled that we
+  // can read/write if we use correct tags.
+  char *buf = mmap(0, sysconf(_SC_PAGESIZE), PROT_MTE | PROT_READ | PROT_WRITE,
+   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (buf == MAP_FAILED)
+return 1;
+
+  // Our pointer will have tag 9
+  char *tagged_buf = set_tag(buf, 9);
+  // Set allocation tags for the first 2 granules
+  __arm_mte_set_tag(set_tag(tagged_buf, 9));
+  __arm_mte_set_tag(set_tag(tagged_buf + 16, 10));
+
+  // Confirm that we can write when tags match
+  *tagged_buf = ' ';
+
+  // Breakpoint here
+  // Faults because tag 9 in the ptr != allocation tag of 10.
+  // + 16 puts us in the second granule and +1 makes the fault address
+  // misaligned relative to the granule size. This misalignment must
+  // be accounted for by lldb-server.
+  *(tagged_buf + 16 + 1) = '?';
+
+  return 0;
+}
Index: lldb/test/API/linux/aarch64/mte_tag_faults/TestAArch64LinuxMTEMemoryTagFaults.py
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/mte_tag_faults/TestAArch64LinuxMTEMemoryTagFaults.py
@@ -0,0 +1,62 @@
+"""
+Test reporting of MTE tag access faults.
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AArch64LinuxMTEMemoryTagFaultsTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+NO_DEBUG_INFO_TESTCASE = True
+
+def setup_mte_test(self, fault_type):
+if not self.isAArch64MTE():
+self.skipTest('Target must support MTE.')
+
+self.build()
+self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(self, "main.c",
+line_number('main.c', '// Breakpoint here'),
+num_expected_locations=1)
+
+self.runCmd("run {}".format(fault_type), RUN_SUCCEEDED)
+
+if self.process().GetState() == lldb.eStateExited:
+self.fail("Test program failed to run.")
+
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+@skipUnlessAArch64MTELinuxCompiler
+def test_mte_tag_fault_sync(self):
+self.setup_mte_test("sync")
+# The logical tag should be included in the fault address
+# and we know what the bottom byte should be.
+# It will be 0x10 (to be in the 2nd granule), +1 to be 0x11.
+# Which tests that lldb-server handles fault addresses that
+# are not granule aligned.
+self.expect("continue",
+patterns=[
+"\* thread #1, name = 'a.out', stop reason = signal SIGSEGV: "
+"sync tag check fault \(fault address: 0x9[0-9A-Fa-f]+11\ "
+"logical tag: 0x9 allocation tag: 0xa\)"])
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+@skipUnlessAArch64MTELinuxCompiler
+def test_mte_tag_fault_async(self):
+self.setup_mte_test("async")
+self.expect("continue",
+substrs=[
+"* thr

[Lldb-commits] [PATCH] D105178: [lldb][AArch64] Annotate synchronous tag faults

[David Spickett via Phabricator via lldb-commits]

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

Correct potentitally -> potentially.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105178

Files:
  lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeThreadLinux.h
  lldb/test/API/linux/aarch64/mte_tag_faults/Makefile
  
lldb/test/API/linux/aarch64/mte_tag_faults/TestAArch64LinuxMTEMemoryTagFaults.py
  lldb/test/API/linux/aarch64/mte_tag_faults/main.c

Index: lldb/test/API/linux/aarch64/mte_tag_faults/main.c
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/mte_tag_faults/main.c
@@ -0,0 +1,59 @@
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+// Set bits 59-56 to tag, removing any existing tag
+static char *set_tag(char *ptr, size_t tag) {
+  return (char *)(((size_t)ptr & ~((size_t)0xf << 56)) | (tag << 56));
+}
+
+int main(int argc, char const *argv[]) {
+  // We assume that the test runner has checked we're on an MTE system
+
+  // Only expect to get the fault type
+  if (argc != 2)
+return 1;
+
+  unsigned long prctl_arg2 = 0;
+  if (!strcmp(argv[1], "sync"))
+prctl_arg2 = PR_MTE_TCF_SYNC;
+  else if (!strcmp(argv[1], "async"))
+prctl_arg2 = PR_MTE_TCF_ASYNC;
+  else
+return 1;
+
+  // Set fault type
+  if (prctl(PR_SET_TAGGED_ADDR_CTRL, prctl_arg2, 0, 0, 0))
+return 1;
+
+  // Allocate some memory with tagging enabled that we
+  // can read/write if we use correct tags.
+  char *buf = mmap(0, sysconf(_SC_PAGESIZE), PROT_MTE | PROT_READ | PROT_WRITE,
+   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (buf == MAP_FAILED)
+return 1;
+
+  // Our pointer will have tag 9
+  char *tagged_buf = set_tag(buf, 9);
+  // Set allocation tags for the first 2 granules
+  __arm_mte_set_tag(set_tag(tagged_buf, 9));
+  __arm_mte_set_tag(set_tag(tagged_buf + 16, 10));
+
+  // Confirm that we can write when tags match
+  *tagged_buf = ' ';
+
+  // Breakpoint here
+  // Faults because tag 9 in the ptr != allocation tag of 10.
+  // + 16 puts us in the second granule and +1 makes the fault address
+  // misaligned relative to the granule size. This misalignment must
+  // be accounted for by lldb-server.
+  *(tagged_buf + 16 + 1) = '?';
+
+  return 0;
+}
Index: lldb/test/API/linux/aarch64/mte_tag_faults/TestAArch64LinuxMTEMemoryTagFaults.py
===
--- /dev/null
+++ lldb/test/API/linux/aarch64/mte_tag_faults/TestAArch64LinuxMTEMemoryTagFaults.py
@@ -0,0 +1,62 @@
+"""
+Test reporting of MTE tag access faults.
+"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AArch64LinuxMTEMemoryTagFaultsTestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+NO_DEBUG_INFO_TESTCASE = True
+
+def setup_mte_test(self, fault_type):
+if not self.isAArch64MTE():
+self.skipTest('Target must support MTE.')
+
+self.build()
+self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(self, "main.c",
+line_number('main.c', '// Breakpoint here'),
+num_expected_locations=1)
+
+self.runCmd("run {}".format(fault_type), RUN_SUCCEEDED)
+
+if self.process().GetState() == lldb.eStateExited:
+self.fail("Test program failed to run.")
+
+self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+substrs=['stopped',
+ 'stop reason = breakpoint'])
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+@skipUnlessAArch64MTELinuxCompiler
+def test_mte_tag_fault_sync(self):
+self.setup_mte_test("sync")
+# The logical tag should be included in the fault address
+# and we know what the bottom byte should be.
+# It will be 0x10 (to be in the 2nd granule), +1 to be 0x11.
+# Which tests that lldb-server handles fault addresses that
+# are not granule aligned.
+self.expect("continue",
+patterns=[
+"\* thread #1, name = 'a.out', stop reason = signal SIGSEGV: "
+"sync tag check fault \(fault address: 0x9[0-9A-Fa-f]+11\ "
+"logical tag: 0x9 allocation tag: 0xa\)"])
+
+@skipUnlessArch("aarch64")
+@skipUnlessPlatform(["linux"])
+@skipUnlessAArch64MTELinuxCompiler
+def test_mte_tag_fault_async(self):
+self.setup_mte_test("async")
+self.expect("continue",
+substrs=[
+"* thread #1, name = 'a.out', stop reason = "
+"signal SIGSEGV: async tag check fault"])
Index: lldb/test/API/linux/aarch64/mte_tag_faults/Makefile
===

[Lldb-commits] [lldb] adee89f - [lldb][AArch64] Add tag packing and repetition memory tag manager

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2021-07-16T14:21:38+01:00
New Revision: adee89f8bcd11529bc9e046493b71fbe4b49dcd1

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

LOG: [lldb][AArch64] Add tag packing and repetition memory tag manager

PackTags is used by to compress tags to go in the QMemTags packet
and be passed to ptrace when writing memory tags.

The behaviour of RepeatTagsForRange matches that described for QMemTags
in the GDB documentation:
https://sourceware.org/gdb/current/onlinedocs/gdb/General-Query-Packets.html#General-Query-Packets

In addition, unpacking tags with number of tags 0 now means
do not check that number of tags matches the range.
This will be used by lldb-server to unpack tags before repeating
them to fill the requested range.

Reviewed By: omjavaid

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

Added: 


Modified: 
lldb/include/lldb/Target/MemoryTagManager.h
lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp
lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h
lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp

Removed: 




diff  --git a/lldb/include/lldb/Target/MemoryTagManager.h 
b/lldb/include/lldb/Target/MemoryTagManager.h
index 65abfcd76382..a5e0deba14a9 100644
--- a/lldb/include/lldb/Target/MemoryTagManager.h
+++ b/lldb/include/lldb/Target/MemoryTagManager.h
@@ -87,11 +87,45 @@ class MemoryTagManager {
   virtual size_t GetTagSizeInBytes() const = 0;
 
   // Unpack tags from their stored format (e.g. gdb qMemTags data) into 
seperate
-  // tags. Checks that each tag is within the expected value range and that the
-  // number of tags found matches the number of granules we originally asked
-  // for.
+  // tags.
+  //
+  // Checks that each tag is within the expected value range and if granules is
+  // set to non-zero, that the number of tags found matches the number of
+  // granules we expected to cover.
+  virtual llvm::Expected>
+  UnpackTagsData(const std::vector &tags,
+ size_t granules = 0) const = 0;
+
+  // Pack uncompressed tags into their storage format (e.g. for gdb QMemTags).
+  // Checks that each tag is within the expected value range.
+  // We do not check the number of tags or range they apply to because
+  // it is up to the remote to repeat them as needed.
+  virtual llvm::Expected>
+  PackTags(const std::vector &tags) const = 0;
+
+  // Take a set of tags and repeat them as much as needed to cover the given
+  // range. We assume that this range has been previously expanded/aligned to
+  // granules. (this method is used by lldb-server to implement QMemTags
+  // packet handling)
+  //
+  // If the range is empty, zero tags are returned.
+  // If the range is not empty and...
+  //   * there are no tags, an error is returned.
+  //   * there are fewer tags than granules, the tags are repeated to fill the
+  // range.
+  //   * there are more tags than granules, only the tags required to cover
+  // the range are returned.
+  //
+  // When repeating tags it will not always return a multiple of the original
+  // list. For example if your range is 3 granules and your tags are 1 and 2.
+  // You will get tags 1, 2 and 1 returned. Rather than getting 1, 2, 1, 2,
+  // which would be one too many tags for the range.
+  //
+  // A single tag will just be repeated as you'd expected. Tag 1 over 3 
granules
+  // would return 1, 1, 1.
   virtual llvm::Expected>
-  UnpackTagsData(const std::vector &tags, size_t granules) const = 0;
+  RepeatTagsForRange(const std::vector &tags,
+ TagRange range) const = 0;
 
   virtual ~MemoryTagManager() {}
 };

diff  --git 
a/lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp 
b/lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp
index e44a10da73af..d74b66b58afc 100644
--- a/lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp
+++ b/lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp
@@ -125,14 +125,17 @@ MemoryTagManagerAArch64MTE::MakeTaggedRange(
 
 llvm::Expected>
 MemoryTagManagerAArch64MTE::UnpackTagsData(const std::vector &tags,
-   size_t granules) const {
-  size_t num_tags = tags.size() / GetTagSizeInBytes();
-  if (num_tags != granules) {
-return llvm::createStringError(
-llvm::inconvertibleErrorCode(),
-"Packed tag data size does not match expected number of tags. "
-"Expected %zu tag(s) for %zu granules, got %zu tag(s).",
-granules, granules, num_tags);
+   size_t granules /*=0*/) const {
+  // 0 means don't check the number of tags before unpacking
+  if (granules) {
+size_t num_tags = tags.size() / GetTa

[Lldb-commits] [PATCH] D105179: [lldb][AArch64] Add tag packing and repetition memory tag manager

[David Spickett via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGadee89f8bcd1: [lldb][AArch64] Add tag packing and repetition 
memory tag manager (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105179

Files:
  lldb/include/lldb/Target/MemoryTagManager.h
  lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.cpp
  lldb/source/Plugins/Process/Utility/MemoryTagManagerAArch64MTE.h
  lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp

Index: lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
===
--- lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
+++ lldb/unittests/Process/Utility/MemoryTagManagerAArch64MTETest.cpp
@@ -21,7 +21,7 @@
   manager.UnpackTagsData(input, 2),
   llvm::FailedWithMessage(
   "Packed tag data size does not match expected number of tags. "
-  "Expected 2 tag(s) for 2 granules, got 0 tag(s)."));
+  "Expected 2 tag(s) for 2 granule(s), got 0 tag(s)."));
 
   // This is out of the valid tag range
   input.push_back(0x1f);
@@ -41,6 +41,43 @@
   manager.UnpackTagsData(input, 2);
   ASSERT_THAT_EXPECTED(got, llvm::Succeeded());
   ASSERT_THAT(expected, testing::ContainerEq(*got));
+
+  // Error for too much tag data
+  ASSERT_THAT_EXPECTED(
+  manager.UnpackTagsData(input, 1),
+  llvm::FailedWithMessage(
+  "Packed tag data size does not match expected number of tags. "
+  "Expected 1 tag(s) for 1 granule(s), got 2 tag(s)."));
+
+  // By default, we don't check number of tags
+  llvm::Expected> got_zero =
+  manager.UnpackTagsData(input);
+  ASSERT_THAT_EXPECTED(got_zero, llvm::Succeeded());
+  ASSERT_THAT(expected, testing::ContainerEq(*got));
+
+  // Which is the same as granules=0
+  got_zero = manager.UnpackTagsData(input, 0);
+  ASSERT_THAT_EXPECTED(got_zero, llvm::Succeeded());
+  ASSERT_THAT(expected, testing::ContainerEq(*got));
+}
+
+TEST(MemoryTagManagerAArch64MTETest, PackTags) {
+  MemoryTagManagerAArch64MTE manager;
+
+  // Error for tag out of range
+  llvm::Expected> invalid_tag_err =
+  manager.PackTags({0x10});
+  ASSERT_THAT_EXPECTED(
+  invalid_tag_err,
+  llvm::FailedWithMessage(
+  "Found tag 0x10 which is > max MTE tag value of 0xf."));
+
+  // 0xf here is the max tag value that we can pack
+  std::vector tags{0, 1, 0xf};
+  std::vector expected{0, 1, 0xf};
+  llvm::Expected> packed = manager.PackTags(tags);
+  ASSERT_THAT_EXPECTED(packed, llvm::Succeeded());
+  ASSERT_THAT(expected, testing::ContainerEq(*packed));
 }
 
 TEST(MemoryTagManagerAArch64MTETest, GetLogicalTag) {
@@ -233,3 +270,53 @@
   ASSERT_EQ(-32, manager.AddressDiff(0x55114400, 0x44114420));
   ASSERT_EQ(65, manager.AddressDiff(0x99114441, 0x66114400));
 }
+
+// Helper to check that repeating "tags" over "range" gives you
+// "expected_tags".
+static void
+test_repeating_tags(const std::vector &tags,
+MemoryTagManagerAArch64MTE::TagRange range,
+const std::vector &expected_tags) {
+  MemoryTagManagerAArch64MTE manager;
+  llvm::Expected> tags_or_err =
+  manager.RepeatTagsForRange(tags, range);
+  ASSERT_THAT_EXPECTED(tags_or_err, llvm::Succeeded());
+  ASSERT_THAT(expected_tags, testing::ContainerEq(*tags_or_err));
+}
+
+TEST(MemoryTagManagerAArch64MTETest, RepeatTagsForRange) {
+  MemoryTagManagerAArch64MTE manager;
+
+  // Must have some tags if your range is not empty
+  llvm::Expected> no_tags_err =
+  manager.RepeatTagsForRange({},
+ MemoryTagManagerAArch64MTE::TagRange{0, 16});
+  ASSERT_THAT_EXPECTED(
+  no_tags_err, llvm::FailedWithMessage(
+   "Expected some tags to cover given range, got zero."));
+
+  // If the range is empty, you get no tags back
+  test_repeating_tags({1, 2, 3}, MemoryTagManagerAArch64MTE::TagRange{0, 0},
+  {});
+  // And you don't need tags for an empty range
+  test_repeating_tags({}, MemoryTagManagerAArch64MTE::TagRange{0, 0}, {});
+
+  // A single tag will just be multiplied as many times as needed
+  test_repeating_tags({5}, MemoryTagManagerAArch64MTE::TagRange{0, 16}, {5});
+  test_repeating_tags({6}, MemoryTagManagerAArch64MTE::TagRange{0, 32}, {6, 6});
+
+  // If you've got as many tags as granules, it's a roundtrip
+  test_repeating_tags({7, 8}, MemoryTagManagerAArch64MTE::TagRange{0, 32},
+  {7, 8});
+
+  // If you've got fewer tags than granules, they repeat. Exactly or partially
+  // as needed.
+  test_repeating_tags({7, 8}, MemoryTagManagerAArch64MTE::TagRange{0, 64},
+  {7, 8, 7, 8});
+  test_repeating_tags({7, 8}, MemoryTagManagerAArch64MTE::TagRange{0, 48},
+   

[Lldb-commits] [PATCH] D105717: [trace] [intel pt] Create a "thread trace dump stats" command

[hanbing wang via Phabricator via lldb-commits]

hanbingwang marked 2 inline comments as done.
hanbingwang added inline comments.



Comment at: lldb/include/lldb/Target/Trace.h:155
+  /// If \b false, print compact stats
+  virtual void DumpTraceStats(Thread &thread, Stream &s, bool verbose) = 0;
+

clayborg wrote:
> Are any statistics being dumped here? Maybe DumpTraceSummary(...) or 
> DumpTraceInfo(...) would be better?
@clayborg Sorry I didn't turn on the notification! Just saw your comments 
yesterday.
I agree "DumpTraceInfo()" is a better name.



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:118
+  }
+  s.Printf("\nraw trace size %zu\n", *raw_size);
+  return;

wallace wrote:
> clayborg wrote:
> > wallace wrote:
> > > the presentation of this line could be better. Something like this would 
> > > look nicer
> > > 
> > >   thread 1: tid = 123123
> > > 
> > > - Tracing technology: Intel PT
> > > - Raw trace size: 1231232 bytes 
> > The "Tracing technology: Intel PT" should probably come before any of the 
> > thread infos if it is added:
> > ```
> > Tracing technology: Intel PT
> > thread 1: tid = 111, size = 0x1000
> > thread 2: tid = 222, size = 0x1000
> > ```
> That's a pretty good idea.
> 
> @hanbingwang , you can invoke trace_sp->GetPluginName() for getting the name 
> of the tracing technology being used
Hi @wallace @clayborg,

I'm wondering how to let the string "Tracing technology: Intel PT" printed out 
exactly once, when there are more than one threads?  It looks like that 
HandleOneThread() will be called multiple times, which will then call 
DumpTraceInfo() which does the printout.

It seems that it'll be easy to print the string if we know if a thread is the 
*first* to be handled. However the threads are not indexed though?


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

https://reviews.llvm.org/D105717

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

[Lldb-commits] [PATCH] D106171: [lldb] Avoid moving ThreadPlanSP from plans vector

[Dave Lee via Phabricator via lldb-commits]

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

Change `ThreadPlanStack::PopPlan` and `::DiscardPlan` to not do the following:

1. Move the last plan, leaving a moved `ThreadPlanSP` in the plans vector
2. Operate on the last plan
3. Pop the last plan off the plans vector

This leaves a period of time where the last element in the plans vector has 
been moved. I am not sure what, if any, guarantees there are when doing this, 
but it seems like it would/could leave a null `ThreadPlanSP` in the container. 
There are asserts in place to prevent empty/null `ThreadPlanSP` instances from 
being pushed on to the stack, and so this could break that invariant during 
multithreaded access to the thread plan stack.

An open question is whether this use of `std::move` was the result of a measure 
performance problem.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106171

Files:
  lldb/source/Target/ThreadPlanStack.cpp


Index: lldb/source/Target/ThreadPlanStack.cpp
===
--- lldb/source/Target/ThreadPlanStack.cpp
+++ lldb/source/Target/ThreadPlanStack.cpp
@@ -142,7 +142,7 @@
 lldb::ThreadPlanSP ThreadPlanStack::PopPlan() {
   assert(m_plans.size() > 1 && "Can't pop the base thread plan");
 
-  lldb::ThreadPlanSP plan_sp = std::move(m_plans.back());
+  lldb::ThreadPlanSP plan_sp = m_plans.back();
   m_completed_plans.push_back(plan_sp);
   plan_sp->WillPop();
   m_plans.pop_back();
@@ -152,7 +152,7 @@
 lldb::ThreadPlanSP ThreadPlanStack::DiscardPlan() {
   assert(m_plans.size() > 1 && "Can't discard the base thread plan");
 
-  lldb::ThreadPlanSP plan_sp = std::move(m_plans.back());
+  lldb::ThreadPlanSP plan_sp = m_plans.back();
   m_discarded_plans.push_back(plan_sp);
   plan_sp->WillPop();
   m_plans.pop_back();


Index: lldb/source/Target/ThreadPlanStack.cpp
===
--- lldb/source/Target/ThreadPlanStack.cpp
+++ lldb/source/Target/ThreadPlanStack.cpp
@@ -142,7 +142,7 @@
 lldb::ThreadPlanSP ThreadPlanStack::PopPlan() {
   assert(m_plans.size() > 1 && "Can't pop the base thread plan");
 
-  lldb::ThreadPlanSP plan_sp = std::move(m_plans.back());
+  lldb::ThreadPlanSP plan_sp = m_plans.back();
   m_completed_plans.push_back(plan_sp);
   plan_sp->WillPop();
   m_plans.pop_back();
@@ -152,7 +152,7 @@
 lldb::ThreadPlanSP ThreadPlanStack::DiscardPlan() {
   assert(m_plans.size() > 1 && "Can't discard the base thread plan");
 
-  lldb::ThreadPlanSP plan_sp = std::move(m_plans.back());
+  lldb::ThreadPlanSP plan_sp = m_plans.back();
   m_discarded_plans.push_back(plan_sp);
   plan_sp->WillPop();
   m_plans.pop_back();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106122: Add a mutex to guard access to the ThreadPlanStack class

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

I've also opened D106171  as a potential fix. 
It could be that we merge both diffs, they aren't mutually exclusive.




Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:157
   void Clear() {
-for (auto plan : m_plans_list)
+for (auto &plan : m_plans_list)
   plan.second.ThreadDestroyed(nullptr);





Comment at: lldb/source/Target/ThreadPlanStack.cpp:417
   // then scan for absent TID's:
-  for (auto thread_plans : m_plans_list) {
+  for (auto &thread_plans : m_plans_list) {
 lldb::tid_t cur_tid = thread_plans.first;





Comment at: lldb/source/Target/ThreadPlanStack.cpp:432
bool skip_unreported) {
-  for (auto elem : m_plans_list) {
+  for (auto &elem : m_plans_list) {
 lldb::tid_t tid = elem.first;




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106122

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

[Lldb-commits] [PATCH] D106122: Add a mutex to guard access to the ThreadPlanStack class

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:113
   std::unordered_map m_completed_plan_store;
+  mutable std::recursive_mutex m_stack_mutex;
 };

Does this actually have to be a recursive mutex? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106122

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

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a Create Target form for the LLDB GUI. Additionally, an
Arch Field was introduced to input an arch and the file and directory
fields now have a required property.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106192

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -36,6 +36,7 @@
 
 #include "lldb/Interpreter/CommandCompletions.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
+#include "lldb/Interpreter/OptionGroupPlatform.h"
 
 #if LLDB_ENABLE_CURSES
 #include "lldb/Breakpoint/BreakpointLocation.h"
@@ -56,6 +57,7 @@
 #include "lldb/Utility/State.h"
 #endif
 
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 
 #ifdef _WIN32
@@ -1279,13 +1281,15 @@
 class FileFieldDelegate : public TextFieldDelegate {
 public:
   FileFieldDelegate(const char *label, const char *content,
-bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+bool need_to_exist = true, bool required = true)
+  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist),
+m_required(required) {}
 
-  // Set appropriate error messages if the file doesn't exists or is, in fact, a
-  // directory.
   void FieldDelegateExitCallback() override {
-FileSpec file(GetPath());
+if (m_content.empty() && !m_required)
+  return;
+
+FileSpec file = GetFileSpec();
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("File doesn't exist!");
   return;
@@ -1296,23 +1300,33 @@
 }
   }
 
-  // Returns the path of the file.
   const std::string &GetPath() { return m_content; }
 
+  bool IsSpecified() { return !m_content.empty(); }
+
+  FileSpec GetFileSpec() {
+FileSpec file_spec(m_content);
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;
+  }
+
 protected:
   bool m_need_to_exist;
+  bool m_required;
 };
 
 class DirectoryFieldDelegate : public TextFieldDelegate {
 public:
   DirectoryFieldDelegate(const char *label, const char *content,
- bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+ bool need_to_exist = true, bool required = true)
+  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist),
+m_required(required) {}
 
-  // Set appropriate error messages if the directory doesn't exists or is, in
-  // fact, a file.
   void FieldDelegateExitCallback() override {
-FileSpec file(GetPath());
+if (m_content.empty() && !m_required)
+  return;
+
+FileSpec file = GetFileSpec();
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("Directory doesn't exist!");
   return;
@@ -1323,11 +1337,43 @@
 }
   }
 
-  // Returns the path of the file.
   const std::string &GetPath() { return m_content; }
 
+  bool IsSpecified() { return !m_content.empty(); }
+
+  FileSpec GetFileSpec() {
+FileSpec file_spec(m_content);
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;
+  }
+
 protected:
   bool m_need_to_exist;
+  bool m_required;
+};
+
+class ArchFieldDelegate : public TextFieldDelegate {
+public:
+  ArchFieldDelegate(const char *label, const char *content,
+bool required = false)
+  : TextFieldDelegate(label, content), m_required(required) {}
+
+  void FieldDelegateExitCallback() override {
+if (m_content.empty() && !m_required)
+  return;
+
+if (GetArchSpec().IsValid())
+  return;
+
+SetError("Not a valid arch!");
+  }
+
+  const std::string &GetArchString() { return m_content; }
+
+  ArchSpec GetArchSpec() { return ArchSpec(GetArchString()); }
+
+protected:
+  bool m_required;
 };
 
 class BooleanFieldDelegate : public FieldDelegate {
@@ -1863,18 +1909,28 @@
   }
 
   FileFieldDelegate *AddFileField(const char *label, const char *content,
-  bool need_to_exist = true) {
+  bool need_to_exist = true,
+  bool required = true) {
 FileFieldDelegate *delegate =
-new FileFieldDelegate(label, content, need_to_exist);
+new FileFieldDelegate(label, content, need_to_exist, required);
 m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   DirectoryFieldDelegate *AddDirectoryField(const char *label,
 const char *content,
-bool need_

Re: [Lldb-commits] [PATCH] D106122: Add a mutex to guard access to the ThreadPlanStack class

[Dave via lldb-commits]

I believe it does need to be a reclusive mutex.

On Friday, July 16, 2021, Jonas Devlieghere via Phabricator <
revi...@reviews.llvm.org> wrote:

> JDevlieghere accepted this revision.
> JDevlieghere added inline comments.
> This revision is now accepted and ready to land.
>
>
> 
> Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:113
>std::unordered_map m_completed_plan_store;
> +  mutable std::recursive_mutex m_stack_mutex;
>  };
> 
> Does this actually have to be a recursive mutex?
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D106122/new/
>
> https://reviews.llvm.org/D106122
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.
Herald added a subscriber: JDevlieghere.

This doesn't support remote files yet, I am still having trouble testing those. 
Also, there is also an unrelated clang-format change, not sure if I should 
revert it or keep it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106192

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

[Lldb-commits] [PATCH] D106194: Tests for: D100299: Be lazier about loading .dwo files

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil created this revision.
jankratochvil added reviewers: Eric, dblaikie.
jankratochvil added a project: LLDB.
Herald added subscribers: JDevlieghere, emaste.
jankratochvil requested review of this revision.
Herald added a subscriber: MaskRay.

D100299 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106194

Files:
  lldb/test/Shell/ObjectFile/ELF/split-lazy-load.test
  lldb/test/Shell/ObjectFile/ELF/split-optimized.test


Index: lldb/test/Shell/ObjectFile/ELF/split-optimized.test
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/split-optimized.test
@@ -0,0 +1,18 @@
+# REQUIRES: x86
+# RUN: rm -rf %t
+# RUN: mkdir %t
+# RUN: cd %t
+# RUN: echo 'int main(void) { return 0; }' >appleopt.c
+# RUN: %clang --target=x86_64-pc-linux -gdwarf-5 -glldb -gsplit-dwarf -O3 -o 
appleopt.o appleopt.c
+# RUN: llvm-dwarfdump appleopt.o | FileCheck %s --check-prefix DWARFDUMP_O
+# RUN: llvm-dwarfdump appleopt.dwo | FileCheck %s --check-prefix DWARFDUMP_DWO
+# RUN: %lldb -b -o 'script 
lldb.SBDebugger.Create().CreateTarget("appleopt.o").FindFunctions("main",lldb.eFunctionNameTypeAuto).GetContextAtIndex(0).GetFunction().GetIsOptimized()'
 | FileCheck %s
+
+# DWARFDUMP_O-NOT: DW_AT_APPLE_optimized
+#
+# DWARFDUMP_DWO: DW_TAG_compile_unit
+# DWARFDUMP_DWO-NOT: DW_TAG_
+# DWARFDUMP_DWO: DW_AT_APPLE_optimized (true)
+
+# CHECK: (lldb) script lldb.SBDebugger.Create()
+# CHECK-NEXT: True
Index: lldb/test/Shell/ObjectFile/ELF/split-lazy-load.test
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/split-lazy-load.test
@@ -0,0 +1,12 @@
+# REQUIRES: x86
+# RUN: rm -rf %t
+# RUN: mkdir %t
+# RUN: echo 'int main(void) { return 0; }' >%t/1.c
+# RUN: echo 'int x;' >%t/2.c
+# RUN: %clang -gdwarf-5 -gsplit-dwarf -gpubnames --target=x86_64-pc-linux -o 
%t/main %t/1.c %t/2.c
+# RUN: %lldb -b %t/main -o 'log enable ll''db object' -o 'b main' | FileCheck 
%s
+
+# CHECK: (lldb) b main
+# CHECK-NOT: /2.dwo,
+# CHECK: /1.dwo,
+# CHECK-NOT: /2.dwo,


Index: lldb/test/Shell/ObjectFile/ELF/split-optimized.test
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/split-optimized.test
@@ -0,0 +1,18 @@
+# REQUIRES: x86
+# RUN: rm -rf %t
+# RUN: mkdir %t
+# RUN: cd %t
+# RUN: echo 'int main(void) { return 0; }' >appleopt.c
+# RUN: %clang --target=x86_64-pc-linux -gdwarf-5 -glldb -gsplit-dwarf -O3 -o appleopt.o appleopt.c
+# RUN: llvm-dwarfdump appleopt.o | FileCheck %s --check-prefix DWARFDUMP_O
+# RUN: llvm-dwarfdump appleopt.dwo | FileCheck %s --check-prefix DWARFDUMP_DWO
+# RUN: %lldb -b -o 'script lldb.SBDebugger.Create().CreateTarget("appleopt.o").FindFunctions("main",lldb.eFunctionNameTypeAuto).GetContextAtIndex(0).GetFunction().GetIsOptimized()' | FileCheck %s
+
+# DWARFDUMP_O-NOT: DW_AT_APPLE_optimized
+#
+# DWARFDUMP_DWO: DW_TAG_compile_unit
+# DWARFDUMP_DWO-NOT: DW_TAG_
+# DWARFDUMP_DWO: DW_AT_APPLE_optimized	(true)
+
+# CHECK: (lldb) script lldb.SBDebugger.Create()
+# CHECK-NEXT: True
Index: lldb/test/Shell/ObjectFile/ELF/split-lazy-load.test
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/split-lazy-load.test
@@ -0,0 +1,12 @@
+# REQUIRES: x86
+# RUN: rm -rf %t
+# RUN: mkdir %t
+# RUN: echo 'int main(void) { return 0; }' >%t/1.c
+# RUN: echo 'int x;' >%t/2.c
+# RUN: %clang -gdwarf-5 -gsplit-dwarf -gpubnames --target=x86_64-pc-linux -o %t/main %t/1.c %t/2.c
+# RUN: %lldb -b %t/main -o 'log enable ll''db object' -o 'b main' | FileCheck %s
+
+# CHECK: (lldb) b main
+# CHECK-NOT: /2.dwo,
+# CHECK: /1.dwo,
+# CHECK-NOT: /2.dwo,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106194: Tests for: D100299: Be lazier about loading .dwo files

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil updated this revision to Diff 359458.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106194

Files:
  lldb/test/Shell/ObjectFile/ELF/split-lazy-load.test
  lldb/test/Shell/ObjectFile/ELF/split-optimized.test


Index: lldb/test/Shell/ObjectFile/ELF/split-optimized.test
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/split-optimized.test
@@ -0,0 +1,17 @@
+# RUN: rm -rf %t
+# RUN: mkdir %t
+# RUN: cd %t
+# RUN: echo 'int main(void) { return 0; }' >appleopt.c
+# RUN: %clangxx_host -gdwarf-5 -glldb -gsplit-dwarf -O3 -o appleopt.o 
appleopt.c
+# RUN: llvm-dwarfdump appleopt.o | FileCheck %s --check-prefix DWARFDUMP_O
+# RUN: llvm-dwarfdump appleopt.dwo | FileCheck %s --check-prefix DWARFDUMP_DWO
+# RUN: %lldb -b -o 'script 
lldb.SBDebugger.Create().CreateTarget("appleopt.o").FindFunctions("main",lldb.eFunctionNameTypeAuto).GetContextAtIndex(0).GetFunction().GetIsOptimized()'
 | FileCheck %s
+
+# DWARFDUMP_O-NOT: DW_AT_APPLE_optimized
+#
+# DWARFDUMP_DWO: DW_TAG_compile_unit
+# DWARFDUMP_DWO-NOT: DW_TAG_
+# DWARFDUMP_DWO: DW_AT_APPLE_optimized (true)
+
+# CHECK: (lldb) script lldb.SBDebugger.Create()
+# CHECK-NEXT: True
Index: lldb/test/Shell/ObjectFile/ELF/split-lazy-load.test
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/split-lazy-load.test
@@ -0,0 +1,11 @@
+# RUN: rm -rf %t
+# RUN: mkdir %t
+# RUN: echo 'int main(void) { return 0; }' >%t/1.c
+# RUN: echo 'int x;' >%t/2.c
+# RUN: %clangxx_host -gdwarf-5 -gsplit-dwarf -gpubnames -o %t/main %t/1.c 
%t/2.c
+# RUN: %lldb -b %t/main -o 'log enable ll''db object' -o 'b main' | FileCheck 
%s
+
+# CHECK: (lldb) b main
+# CHECK-NOT: /2.dwo,
+# CHECK: /1.dwo,
+# CHECK-NOT: /2.dwo,


Index: lldb/test/Shell/ObjectFile/ELF/split-optimized.test
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/split-optimized.test
@@ -0,0 +1,17 @@
+# RUN: rm -rf %t
+# RUN: mkdir %t
+# RUN: cd %t
+# RUN: echo 'int main(void) { return 0; }' >appleopt.c
+# RUN: %clangxx_host -gdwarf-5 -glldb -gsplit-dwarf -O3 -o appleopt.o appleopt.c
+# RUN: llvm-dwarfdump appleopt.o | FileCheck %s --check-prefix DWARFDUMP_O
+# RUN: llvm-dwarfdump appleopt.dwo | FileCheck %s --check-prefix DWARFDUMP_DWO
+# RUN: %lldb -b -o 'script lldb.SBDebugger.Create().CreateTarget("appleopt.o").FindFunctions("main",lldb.eFunctionNameTypeAuto).GetContextAtIndex(0).GetFunction().GetIsOptimized()' | FileCheck %s
+
+# DWARFDUMP_O-NOT: DW_AT_APPLE_optimized
+#
+# DWARFDUMP_DWO: DW_TAG_compile_unit
+# DWARFDUMP_DWO-NOT: DW_TAG_
+# DWARFDUMP_DWO: DW_AT_APPLE_optimized	(true)
+
+# CHECK: (lldb) script lldb.SBDebugger.Create()
+# CHECK-NEXT: True
Index: lldb/test/Shell/ObjectFile/ELF/split-lazy-load.test
===
--- /dev/null
+++ lldb/test/Shell/ObjectFile/ELF/split-lazy-load.test
@@ -0,0 +1,11 @@
+# RUN: rm -rf %t
+# RUN: mkdir %t
+# RUN: echo 'int main(void) { return 0; }' >%t/1.c
+# RUN: echo 'int x;' >%t/2.c
+# RUN: %clangxx_host -gdwarf-5 -gsplit-dwarf -gpubnames -o %t/main %t/1.c %t/2.c
+# RUN: %lldb -b %t/main -o 'log enable ll''db object' -o 'b main' | FileCheck %s
+
+# CHECK: (lldb) b main
+# CHECK-NOT: /2.dwo,
+# CHECK: /1.dwo,
+# CHECK-NOT: /2.dwo,
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106122: Add a mutex to guard access to the ThreadPlanStack class

[Dave Lee via Phabricator via lldb-commits]

kastiglione accepted this revision.
kastiglione added inline comments.



Comment at: lldb/include/lldb/Target/ThreadPlanStack.h:113
   std::unordered_map m_completed_plan_store;
+  mutable std::recursive_mutex m_stack_mutex;
 };

JDevlieghere wrote:
> Does this actually have to be a recursive mutex? 
For example `DiscardAllPlans` calls `DiscardPlan` both of which declare a 
`lock_guard`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106122

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

[Lldb-commits] [PATCH] D106122: Add a mutex to guard access to the ThreadPlanStack class

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D106122#2884543 , @kastiglione 
wrote:

> I believe it does need to be a reclusive mutex.

Yes, I could refactor this to have a no-lock version, but I don't think it's 
worth complicating things.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106122

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

[Lldb-commits] [lldb] 6eb576d - Add a mutex to guard access to the ThreadPlanStack class

[Jim Ingham via lldb-commits]

Author: Jim Ingham
Date: 2021-07-16T15:40:58-07:00
New Revision: 6eb576dcff45fa53e8ceb30c7648b903c1413e05

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

LOG: Add a mutex to guard access to the ThreadPlanStack class

We've seen reports of crashes (none we've been able to reproduce
locally) that look like they are caused by concurrent access to a
thread plan stack.  It looks like there are error paths when an
interrupt request to debugserver times out that cause this problem.

The thread plan stack access is never in a hot loop, and there
aren't enough of them for the extra data member to matter, so
there's really no good reason not to protect the access.

Adding the mutex revealed a couple of places where we were
using "auto" in an iteration when we should have been using
"auto &" - we didn't intend to copy the stack - and I fixed
those as well.

Except for preventing crashes this should be NFC.

Differential Revision: https\://reviews.llvm.org/D106122

Added: 


Modified: 
lldb/include/lldb/Target/ThreadPlanStack.h
lldb/source/Target/ThreadPlanStack.cpp

Removed: 




diff  --git a/lldb/include/lldb/Target/ThreadPlanStack.h 
b/lldb/include/lldb/Target/ThreadPlanStack.h
index 9ba58b1dec41f..e0f76f8e1df52 100644
--- a/lldb/include/lldb/Target/ThreadPlanStack.h
+++ b/lldb/include/lldb/Target/ThreadPlanStack.h
@@ -110,6 +110,7 @@ class ThreadPlanStack {
   size_t m_completed_plan_checkpoint = 0; // Monotonically increasing token for
   // completed plan checkpoints.
   std::unordered_map m_completed_plan_store;
+  mutable std::recursive_mutex m_stack_mutex;
 };
 
 class ThreadPlanStackMap {
@@ -153,7 +154,7 @@ class ThreadPlanStackMap {
   }
 
   void Clear() {
-for (auto plan : m_plans_list)
+for (auto &plan : m_plans_list)
   plan.second.ThreadDestroyed(nullptr);
 m_plans_list.clear();
   }

diff  --git a/lldb/source/Target/ThreadPlanStack.cpp 
b/lldb/source/Target/ThreadPlanStack.cpp
index 8ae5c948e27b5..d25602d25b91f 100644
--- a/lldb/source/Target/ThreadPlanStack.cpp
+++ b/lldb/source/Target/ThreadPlanStack.cpp
@@ -39,6 +39,7 @@ ThreadPlanStack::ThreadPlanStack(const Thread &thread, bool 
make_null) {
 void ThreadPlanStack::DumpThreadPlans(Stream &s,
   lldb::DescriptionLevel desc_level,
   bool include_internal) const {
+  std::lock_guard guard(m_stack_mutex);
   s.IndentMore();
   PrintOneStack(s, "Active plan stack", m_plans, desc_level, include_internal);
   PrintOneStack(s, "Completed plan stack", m_completed_plans, desc_level,
@@ -52,6 +53,7 @@ void ThreadPlanStack::PrintOneStack(Stream &s, 
llvm::StringRef stack_name,
 const PlanStack &stack,
 lldb::DescriptionLevel desc_level,
 bool include_internal) const {
+  std::lock_guard guard(m_stack_mutex);
   // If the stack is empty, just exit:
   if (stack.empty())
 return;
@@ -80,6 +82,7 @@ void ThreadPlanStack::PrintOneStack(Stream &s, 
llvm::StringRef stack_name,
 }
 
 size_t ThreadPlanStack::CheckpointCompletedPlans() {
+  std::lock_guard guard(m_stack_mutex);
   m_completed_plan_checkpoint++;
   m_completed_plan_store.insert(
   std::make_pair(m_completed_plan_checkpoint, m_completed_plans));
@@ -87,6 +90,7 @@ size_t ThreadPlanStack::CheckpointCompletedPlans() {
 }
 
 void ThreadPlanStack::RestoreCompletedPlanCheckpoint(size_t checkpoint) {
+  std::lock_guard guard(m_stack_mutex);
   auto result = m_completed_plan_store.find(checkpoint);
   assert(result != m_completed_plan_store.end() &&
  "Asked for a checkpoint that didn't exist");
@@ -95,11 +99,13 @@ void ThreadPlanStack::RestoreCompletedPlanCheckpoint(size_t 
checkpoint) {
 }
 
 void ThreadPlanStack::DiscardCompletedPlanCheckpoint(size_t checkpoint) {
+  std::lock_guard guard(m_stack_mutex);
   m_completed_plan_store.erase(checkpoint);
 }
 
 void ThreadPlanStack::ThreadDestroyed(Thread *thread) {
   // Tell the plan stacks that this thread is going away:
+  std::lock_guard guard(m_stack_mutex);
   for (ThreadPlanSP plan : m_plans)
 plan->ThreadDestroyed();
 
@@ -128,6 +134,7 @@ void ThreadPlanStack::PushPlan(lldb::ThreadPlanSP 
new_plan_sp) {
   // If the thread plan doesn't already have a tracer, give it its parent's
   // tracer:
   // The first plan has to be a base plan:
+  std::lock_guard guard(m_stack_mutex);
   assert((m_plans.size() > 0 || new_plan_sp->IsBasePlan()) &&
  "Zeroth plan must be a base plan");
 
@@ -140,6 +147,7 @@ void ThreadPlanStack::PushPlan(lldb::ThreadPlanSP 
new_plan_sp) {
 }
 
 lldb::ThreadPlanSP ThreadPlanStack::PopPlan() {
+  std::lock_guard guard(m_stack_mutex);
   as

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Great start! Many inlined comments.




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1309
+FileSpec file_spec(m_content);
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;

It should be a property of the FileFieldDelegate if the file is for the current 
system. We might be specifying a file on a remote system, so it doesn't always 
have to exist, nor should it be resolved on the current system. When creating a 
target we would always want to specify this file should exist on the current 
system. 





Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1315
   bool m_need_to_exist;
+  bool m_required;
 };

Should this to into the FieldDelegate so we don't need to put it into both 
FileFieldDelegate and DirectoryFieldDelegate? Then the default 
FieldDelegate::FieldDelegateExitCallback() function could be:

```
virtual void FieldDelegate::FieldDelegateExitCallback() {
  if (!m_required)
return;
  // Check value with another virtual function?
  FieldDelegateValueIsValid();
}
```
This seems like many fields would want to require being set and we don't want 
to re-invent this for each kind of field.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1346
+FileSpec file_spec(m_content);
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;

It should be a property of the DirectoryFieldDelegate if the directory is for 
the current system. We might be specifying a working directory on a remote 
system, so it doesn't always have to exist, nor should it be resolved on the 
current system. So if this is for the current system, then it is ok to resolve, 
else, we need to just use the path as is.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1912
   FileFieldDelegate *AddFileField(const char *label, const char *content,
-  bool need_to_exist = true) {
+  bool need_to_exist = true,
+  bool required = true) {

Should we add a "bool resolve_local_path" so we know if we should resolve the 
path on the local system as mentioned above?



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1922
 const char *content,
-bool need_to_exist = true) {
+bool need_to_exist = true,
+bool required = true) {

Should we add a "bool resolve_local_path" so we know if we should resolve the 
path on the local system as mentioned above?





Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2569
+  TargetCreateFormDelegate(Debugger &debugger) : m_debugger(debugger) {
+m_file_field = AddFileField("File", "");
+m_core_file_field = AddFileField("Core File", "", true, false);





Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2573
+m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
+m_arch_field = AddArchField("Architecture", "");
+std::vector load_depentents_options;

we should add a "Platform" TextField that is populated with all of the platform 
names. The default value should be the currently selected platform. To get a 
list of all platform plug-in names you can do this like you did for the process 
plug-ins:

```
  static const char *GetPlatformPluginNameAtIndex(uint32_t idx);
```

To get the current selected platform:
```
ConstString default_platform_name;
lldb::PlatformSP platform_sp = debugger.GetPlatformList()GetSelectedPlatform()
if (platform_sp)
  default_platform_name = platform_sp->GetName();
```
This will help with remote targets. The main reason we need this is many linux 
or android binaries have very minimal target triples encoded into the ELF 
files, so we often need to specify "remote-android" or "remote-linux" when 
creating targets. The user can of course set the "Architecture" with a fully 
specified triple, but they can also avoid setting the arch and just specify the 
platform.




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2576
+load_depentents_options.push_back(
+std::string("Only if the target is an executable."));
+load_depentents_options.push_back(std::string("Yes."));





Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2609
+
+if (!FileSystem::Instance().Exists(file_spec)) {
+  SetError("File doesn't exist!");

Won't this be taken care of already by the File field? We should construct the 
FileDelegate so that:
- the file must exist
- the file gets resolved on the local system
- the file fiel

[Lldb-commits] [PATCH] D105531: [trace][intel pt] Implement the Intel PT cursor

[walter erquinigo via Phabricator via lldb-commits]

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

- Nwo using a single-step iterator method Next
- Changed the direction to a boolean flag called "forwards"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105531

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceInstructionDumper.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  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/source/Target/TraceCursor.cpp
  lldb/source/Target/TraceInstructionDumper.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceStartStop.py
  
lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py

Index: lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
===
--- lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
+++ lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
@@ -97,8 +97,8 @@
 self.expect("continue")
 self.expect("thread trace dump instructions", substrs=['main.cpp:4'])
 self.expect("thread trace dump instructions 3", substrs=['main.cpp:4'])
-self.expect("thread trace dump instructions 1", error=True, substrs=['not traced'])
-self.expect("thread trace dump instructions 2", error=True, substrs=['not traced'])
+self.expect("thread trace dump instructions 1", substrs=['not traced'])
+self.expect("thread trace dump instructions 2", substrs=['not traced'])
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
 def testStartMultipleLiveThreadsWithThreadStartAll(self):
@@ -128,9 +128,9 @@
 
 # We'll stop at the next breakpoint in thread 3, and nothing should be traced
 self.expect("continue")
-self.expect("thread trace dump instructions 3", error=True, substrs=['not traced'])
-self.expect("thread trace dump instructions 1", error=True, substrs=['not traced'])
-self.expect("thread trace dump instructions 2", error=True, substrs=['not traced'])
+self.expect("thread trace dump instructions 3", substrs=['not traced'])
+self.expect("thread trace dump instructions 1", substrs=['not traced'])
+self.expect("thread trace dump instructions 2", substrs=['not traced'])
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
 @testSBAPIAndCommands
Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -65,11 +65,12 @@
 self.expect("r")
 self.expect("thread trace start")
 self.expect("n")
-self.expect("thread trace dump instructions", substrs=["total instructions"])
+self.expect("thread trace dump instructions", substrs=["""0x00400511movl   $0x0, -0x4(%rbp)
+no more data"""])
 # process stopping should stop the thread
 self.expect("process trace stop")
 self.expect("n")
-self.expect("thread trace dump instructions", error=True, substrs=["not traced"])
+self.expect("thread trace dump instructions", substrs=["not traced"])
 
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -110,22 +111,32 @@
 
 # We can reconstruct the single instruction executed in the first line
 self.expect("n")
-self.expect("thread trace dump instructions",
-patterns=[f'''thread #1: tid = .*, total instructions = 1
+self.expect("thread trace dump instructions -f",
+patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[0\] {ADDRESS_REGEX}movl'''])
+\[ 0\] {ADDRESS_REGEX}movl'''])
 
 # We can reconstruct the instructions up to the second line
 self.expect("n")
-self.expect("thread trace dump instructions",
-patterns=[f'''thread #1: tid = .*, total instructions = 5
+self.expect("thread trace dump instructions -f",
+patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[0\] {ADDRESS_REGEX}mo

[Lldb-commits] [PATCH] D106171: [lldb] Avoid moving ThreadPlanSP from plans vector

[Dave Lee via Phabricator via lldb-commits]

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

Also call pop_back just after retrieving back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106171

Files:
  lldb/source/Target/ThreadPlanStack.cpp


Index: lldb/source/Target/ThreadPlanStack.cpp
===
--- lldb/source/Target/ThreadPlanStack.cpp
+++ lldb/source/Target/ThreadPlanStack.cpp
@@ -142,20 +142,20 @@
 lldb::ThreadPlanSP ThreadPlanStack::PopPlan() {
   assert(m_plans.size() > 1 && "Can't pop the base thread plan");
 
-  lldb::ThreadPlanSP plan_sp = std::move(m_plans.back());
+  lldb::ThreadPlanSP plan_sp = m_plans.back();
+  m_plans.pop_back();
   m_completed_plans.push_back(plan_sp);
   plan_sp->WillPop();
-  m_plans.pop_back();
   return plan_sp;
 }
 
 lldb::ThreadPlanSP ThreadPlanStack::DiscardPlan() {
   assert(m_plans.size() > 1 && "Can't discard the base thread plan");
 
-  lldb::ThreadPlanSP plan_sp = std::move(m_plans.back());
+  lldb::ThreadPlanSP plan_sp = m_plans.back();
+  m_plans.pop_back();
   m_discarded_plans.push_back(plan_sp);
   plan_sp->WillPop();
-  m_plans.pop_back();
   return plan_sp;
 }
 


Index: lldb/source/Target/ThreadPlanStack.cpp
===
--- lldb/source/Target/ThreadPlanStack.cpp
+++ lldb/source/Target/ThreadPlanStack.cpp
@@ -142,20 +142,20 @@
 lldb::ThreadPlanSP ThreadPlanStack::PopPlan() {
   assert(m_plans.size() > 1 && "Can't pop the base thread plan");
 
-  lldb::ThreadPlanSP plan_sp = std::move(m_plans.back());
+  lldb::ThreadPlanSP plan_sp = m_plans.back();
+  m_plans.pop_back();
   m_completed_plans.push_back(plan_sp);
   plan_sp->WillPop();
-  m_plans.pop_back();
   return plan_sp;
 }
 
 lldb::ThreadPlanSP ThreadPlanStack::DiscardPlan() {
   assert(m_plans.size() > 1 && "Can't discard the base thread plan");
 
-  lldb::ThreadPlanSP plan_sp = std::move(m_plans.back());
+  lldb::ThreadPlanSP plan_sp = m_plans.back();
+  m_plans.pop_back();
   m_discarded_plans.push_back(plan_sp);
   plan_sp->WillPop();
-  m_plans.pop_back();
   return plan_sp;
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105531: [trace][intel pt] Implement the Intel PT cursor

[Greg Clayton via Phabricator via lldb-commits]

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

Just switch all "-f" to the long option format as mentioned in inlined comments 
and this is good to go.




Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:38
 
-self.expect("thread trace dump instructions --raw",
-substrs=['''thread #1: tid = 3842849, total instructions = 21
+self.expect("thread trace dump instructions --raw --count 21 -f",
+substrs=['''thread #1: tid = 3842849

use long form of option for clarity



Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:63
+# We check if we can pass count and skip
+self.expect("thread trace dump instructions --count 5 --skip 6 --raw 
-f",
+substrs=['''thread #1: tid = 3842849

use long form of option for clarity



Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:146
 # The output should be work when too many instructions are asked
-self.expect("thread trace dump instructions --count 20 --position 2",
-substrs=['''thread #1: tid = 3842849, total instructions = 21
+self.expect("thread trace dump instructions --count 20 -f",
+substrs=['''thread #1: tid = 3842849

use long form of option for clarity



Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:165
 os.path.join(self.getSourceDir(), "intelpt-trace", 
"trace_bad_image.json"))
-self.expect("thread trace dump instructions",
-substrs=['''thread #1: tid = 3842849, total instructions = 2
-[0] 0x00400511error: no memory mapped at this address
-[1] 0x00400518error: no memory mapped at this address'''])
+self.expect("thread trace dump instructions -f",
+substrs=['''thread #1: tid = 3842849

use long form of option for clarity



Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:173
 os.path.join(self.getSourceDir(), "intelpt-trace", 
"trace_wrong_cpu.json"))
-self.expect("thread trace dump instructions",
-substrs=['''thread #1: tid = 3842849, total instructions = 1
-[0] error: unknown cpu'''])
+self.expect("thread trace dump instructions -f",
+substrs=['''thread #1: tid = 3842849

use long form of option for clarity



Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:195
 # Finally, the instruction disassembly is included in the dump.
-self.expect("thread trace dump instructions --count 50",
-substrs=['''thread #1: tid = 815455, total instructions = 46
+self.expect("thread trace dump instructions --count 50 -f",
+substrs=['''thread #1: tid = 815455

use long form of option for clarity



Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:340
+
+self.expect("thread trace dump instructions --skip 100 -f", 
inHistory=True,
+substrs=['''thread #1: tid = 815455

use long form of option for clarity



Comment at: lldb/test/API/commands/trace/TestTraceStartStop.py:114
 self.expect("n")
-self.expect("thread trace dump instructions",
-patterns=[f'''thread #1: tid = .*, total instructions = 1
+self.expect("thread trace dump instructions -f",
+patterns=[f'''thread #1: tid = .*

use long form of option for clarity



Comment at: lldb/test/API/commands/trace/TestTraceStartStop.py:121
 self.expect("n")
-self.expect("thread trace dump instructions",
-patterns=[f'''thread #1: tid = .*, total instructions = 5
+self.expect("thread trace dump instructions -f",
+patterns=[f'''thread #1: tid = .*

use long form of option for clarity



Comment at: lldb/test/API/commands/trace/TestTraceStartStop.py:152
 self.expect("n")
+self.expect("thread trace dump instructions -f",
+patterns=[f'''thread #1: tid = .*

use long form of option for clarity


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105531

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

[Lldb-commits] [PATCH] D105531: [trace][intel pt] Implement the Intel PT cursor

[walter erquinigo via Phabricator via lldb-commits]

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

fix nits


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105531

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceInstructionDumper.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  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/source/Target/TraceCursor.cpp
  lldb/source/Target/TraceInstructionDumper.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceStartStop.py
  
lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py

Index: lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
===
--- lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
+++ lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
@@ -97,8 +97,8 @@
 self.expect("continue")
 self.expect("thread trace dump instructions", substrs=['main.cpp:4'])
 self.expect("thread trace dump instructions 3", substrs=['main.cpp:4'])
-self.expect("thread trace dump instructions 1", error=True, substrs=['not traced'])
-self.expect("thread trace dump instructions 2", error=True, substrs=['not traced'])
+self.expect("thread trace dump instructions 1", substrs=['not traced'])
+self.expect("thread trace dump instructions 2", substrs=['not traced'])
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
 def testStartMultipleLiveThreadsWithThreadStartAll(self):
@@ -128,9 +128,9 @@
 
 # We'll stop at the next breakpoint in thread 3, and nothing should be traced
 self.expect("continue")
-self.expect("thread trace dump instructions 3", error=True, substrs=['not traced'])
-self.expect("thread trace dump instructions 1", error=True, substrs=['not traced'])
-self.expect("thread trace dump instructions 2", error=True, substrs=['not traced'])
+self.expect("thread trace dump instructions 3", substrs=['not traced'])
+self.expect("thread trace dump instructions 1", substrs=['not traced'])
+self.expect("thread trace dump instructions 2", substrs=['not traced'])
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
 @testSBAPIAndCommands
Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -65,11 +65,12 @@
 self.expect("r")
 self.expect("thread trace start")
 self.expect("n")
-self.expect("thread trace dump instructions", substrs=["total instructions"])
+self.expect("thread trace dump instructions", substrs=["""0x00400511movl   $0x0, -0x4(%rbp)
+no more data"""])
 # process stopping should stop the thread
 self.expect("process trace stop")
 self.expect("n")
-self.expect("thread trace dump instructions", error=True, substrs=["not traced"])
+self.expect("thread trace dump instructions", substrs=["not traced"])
 
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -110,22 +111,32 @@
 
 # We can reconstruct the single instruction executed in the first line
 self.expect("n")
-self.expect("thread trace dump instructions",
-patterns=[f'''thread #1: tid = .*, total instructions = 1
+self.expect("thread trace dump instructions -f",
+patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[0\] {ADDRESS_REGEX}movl'''])
+\[ 0\] {ADDRESS_REGEX}movl'''])
 
 # We can reconstruct the instructions up to the second line
 self.expect("n")
-self.expect("thread trace dump instructions",
-patterns=[f'''thread #1: tid = .*, total instructions = 5
+self.expect("thread trace dump instructions -f",
+patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[0\] {ADDRESS_REGEX}movl .*
+\[ 0\] {ADDRESS_REGEX}movl .*
   a.out`main \+ 11 at main.cpp:4
-\[1\] {ADDRESS

[Lldb-commits] [lldb] b0aa707 - [trace][intel pt] Implement the Intel PT cursor

[Walter Erquinigo via lldb-commits]

Author: Walter Erquinigo
Date: 2021-07-16T16:47:43-07:00
New Revision: b0aa70761b8324d0c9ebc57da58a44c9e266ce0e

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

LOG: [trace][intel pt] Implement the Intel PT cursor

D104422 added the interface for TraceCursor, which is the main way to traverse 
instructions in a trace. This diff implements the corresponding cursor class 
for Intel PT and deletes the now obsolete code.

Besides that, the logic for the "thread trace dump instructions" was adapted to 
use this cursor (pretty much I ended up moving code from Trace.cpp to 
TraceCursor.cpp). The command by default traverses the instructions backwards, 
and if the user passes --forwards, then it's not forwards. More information 
about that is in the Options.td file.

Regarding the Intel PT cursor. All Intel PT cursors for the same thread share 
the same DecodedThread instance. I'm not yet implementing lazy decoding because 
we don't need it. That'll be for later. For the time being, the entire thread 
trace is decoded when the first cursor for that thread is requested.

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

Added: 
lldb/include/lldb/Target/TraceInstructionDumper.h
lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
lldb/source/Target/TraceInstructionDumper.cpp

Modified: 
lldb/include/lldb/Target/Trace.h
lldb/include/lldb/Target/TraceCursor.h
lldb/source/Commands/CommandObjectThread.cpp
lldb/source/Commands/Options.td
lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.h
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/source/Target/TraceCursor.cpp
lldb/test/API/commands/trace/TestTraceDumpInstructions.py
lldb/test/API/commands/trace/TestTraceStartStop.py

lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py

Removed: 




diff  --git a/lldb/include/lldb/Target/Trace.h 
b/lldb/include/lldb/Target/Trace.h
index cc84270b80081..c82d17b029557 100644
--- a/lldb/include/lldb/Target/Trace.h
+++ b/lldb/include/lldb/Target/Trace.h
@@ -45,11 +45,6 @@ namespace lldb_private {
 class Trace : public PluginInterface,
   public std::enable_shared_from_this {
 public:
-  enum class TraceDirection {
-Forwards = 0,
-Backwards,
-  };
-
   /// Dump the trace data that this plug-in has access to.
   ///
   /// This function will dump all of the trace data for all threads in a user
@@ -137,62 +132,6 @@ class Trace : public PluginInterface,
   /// The JSON schema of this Trace plug-in.
   virtual llvm::StringRef GetSchema() = 0;
 
-  /// Dump \a count instructions of the given thread's trace ending at the
-  /// given \a end_position position.
-  ///
-  /// The instructions are printed along with their indices or positions, which
-  /// are increasing chronologically. This means that the \a index 0 represents
-  /// the oldest instruction of the trace chronologically.
-  ///
-  /// \param[in] thread
-  /// The thread whose trace will be dumped.
-  ///
-  /// \param[in] s
-  /// The stream object where the instructions are printed.
-  ///
-  /// \param[in] count
-  /// The number of instructions to print.
-  ///
-  /// \param[in] end_position
-  /// The position of the last instruction to print.
-  ///
-  /// \param[in] raw
-  /// Dump only instruction addresses without disassembly nor symbol
-  /// information.
-  void DumpTraceInstructions(Thread &thread, Stream &s, size_t count,
- size_t end_position, bool raw);
-
-  /// Run the provided callback on the instructions of the trace of the given
-  /// thread.
-  ///
-  /// The instructions will be traversed starting at the given \a position
-  /// sequentially until the callback returns \b false, in which case no more
-  /// instructions are inspected.
-  ///
-  /// The purpose of this method is to allow inspecting traced instructions
-  /// without exposing the internal representation of how they are stored on
-  /// memory.
-  ///
-  /// \param[in] thread
-  /// The thread whose trace will be traversed.
-  ///
-  /// \param[in] position
-  /// The instruction position to start iterating on.
-  ///
-  /// \param[in] direction
-  /// If \b TraceDirection::Forwards, then then instructions will be
-  /// traversed forwards chronologically, i.e. with incrementing ind

[Lldb-commits] [PATCH] D105531: [trace][intel pt] Implement the Intel PT cursor

[Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb0aa70761b83: [trace][intel pt] Implement the Intel PT 
cursor (authored by Walter Erquinigo ).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105531

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Target/TraceCursor.h
  lldb/include/lldb/Target/TraceInstructionDumper.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
  lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
  lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp
  lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.h
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
  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/source/Target/TraceCursor.cpp
  lldb/source/Target/TraceInstructionDumper.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceStartStop.py
  
lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py

Index: lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
===
--- lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
+++ lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
@@ -97,8 +97,8 @@
 self.expect("continue")
 self.expect("thread trace dump instructions", substrs=['main.cpp:4'])
 self.expect("thread trace dump instructions 3", substrs=['main.cpp:4'])
-self.expect("thread trace dump instructions 1", error=True, substrs=['not traced'])
-self.expect("thread trace dump instructions 2", error=True, substrs=['not traced'])
+self.expect("thread trace dump instructions 1", substrs=['not traced'])
+self.expect("thread trace dump instructions 2", substrs=['not traced'])
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
 def testStartMultipleLiveThreadsWithThreadStartAll(self):
@@ -128,9 +128,9 @@
 
 # We'll stop at the next breakpoint in thread 3, and nothing should be traced
 self.expect("continue")
-self.expect("thread trace dump instructions 3", error=True, substrs=['not traced'])
-self.expect("thread trace dump instructions 1", error=True, substrs=['not traced'])
-self.expect("thread trace dump instructions 2", error=True, substrs=['not traced'])
+self.expect("thread trace dump instructions 3", substrs=['not traced'])
+self.expect("thread trace dump instructions 1", substrs=['not traced'])
+self.expect("thread trace dump instructions 2", substrs=['not traced'])
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
 @testSBAPIAndCommands
Index: lldb/test/API/commands/trace/TestTraceStartStop.py
===
--- lldb/test/API/commands/trace/TestTraceStartStop.py
+++ lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -65,11 +65,12 @@
 self.expect("r")
 self.expect("thread trace start")
 self.expect("n")
-self.expect("thread trace dump instructions", substrs=["total instructions"])
+self.expect("thread trace dump instructions", substrs=["""0x00400511movl   $0x0, -0x4(%rbp)
+no more data"""])
 # process stopping should stop the thread
 self.expect("process trace stop")
 self.expect("n")
-self.expect("thread trace dump instructions", error=True, substrs=["not traced"])
+self.expect("thread trace dump instructions", substrs=["not traced"])
 
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
@@ -110,22 +111,32 @@
 
 # We can reconstruct the single instruction executed in the first line
 self.expect("n")
-self.expect("thread trace dump instructions",
-patterns=[f'''thread #1: tid = .*, total instructions = 1
+self.expect("thread trace dump instructions -f",
+patterns=[f'''thread #1: tid = .*
   a.out`main \+ 4 at main.cpp:2
-\[0\] {ADDRESS_REGEX}movl'''])
+\[ 0\] {ADDRESS_REGEX}movl'''])
 
 # We can reconstruct the instructions up to the second line
 self.expect("n")
-self.expect("thread trace dump instructions",
-patterns=[f'''thread #1: tid = .*, total instructions = 5
+self.expect("thread trace dump instructions -f",
+patterns=[f'''thre

[Lldb-commits] [PATCH] D106171: [lldb] Avoid moving ThreadPlanSP from plans vector

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Target/ThreadPlanStack.cpp:145
 
-  lldb::ThreadPlanSP plan_sp = std::move(m_plans.back());
+  lldb::ThreadPlanSP plan_sp = m_plans.back();
+  m_plans.pop_back();

Should we comment that we are intentionally making a copy of the SP to avoid a 
race condition with std::move()?



Comment at: lldb/source/Target/ThreadPlanStack.cpp:146
+  lldb::ThreadPlanSP plan_sp = m_plans.back();
+  m_plans.pop_back();
   m_completed_plans.push_back(plan_sp);

It's counterintuitive that WillPop() is being called *after* the value is 
popped now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106171

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

[Lldb-commits] [PATCH] D105717: [trace] [intel pt] Create a "thread trace dump stats" command

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:118
+  }
+  s.Printf("\nraw trace size %zu\n", *raw_size);
+  return;

hanbingwang wrote:
> wallace wrote:
> > clayborg wrote:
> > > wallace wrote:
> > > > the presentation of this line could be better. Something like this 
> > > > would look nicer
> > > > 
> > > >   thread 1: tid = 123123
> > > > 
> > > > - Tracing technology: Intel PT
> > > > - Raw trace size: 1231232 bytes 
> > > The "Tracing technology: Intel PT" should probably come before any of the 
> > > thread infos if it is added:
> > > ```
> > > Tracing technology: Intel PT
> > > thread 1: tid = 111, size = 0x1000
> > > thread 2: tid = 222, size = 0x1000
> > > ```
> > That's a pretty good idea.
> > 
> > @hanbingwang , you can invoke trace_sp->GetPluginName() for getting the 
> > name of the tracing technology being used
> Hi @wallace @clayborg,
> 
> I'm wondering how to let the string "Tracing technology: Intel PT" printed 
> out exactly once, when there are more than one threads?  It looks like that 
> HandleOneThread() will be called multiple times, which will then call 
> DumpTraceInfo() which does the printout.
> 
> It seems that it'll be easy to print the string if we know if a thread is the 
> *first* to be handled. However the threads are not indexed though?
You need to override CommandObjectIterateOverThreads::DoExecute in your command 
object. Something like this (figure out the correct function names)

  bool CommandObjectTraceDumpInfo::DoExecute(Args &command,
CommandReturnObject &result) {
 Target &target = m_exe_ctx.GetTargetRef();
 result.GetOutput().Printf("trace technology: %s", 
target.GetTrace().GetPluginName().Data());

 return CommandObjectIterateOverThreads::DoExecute(command, result);
  }

that way that piece of code is executed before iterating over the threads


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

https://reviews.llvm.org/D105717

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

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

In D106035#2879939 , @teemperor wrote:

> I actually expected after the RFC that we would remove all the non-wchar 
> code, but this seems also fine. I think this LGTM in general, but I feel a 
> bit nervous about touching stuff that depends so much on OS/environment. What 
> OS/environment did you test this patch on? Would be nice to have this tested 
> on a few setups before landing.

This was my mistake, sorry.  I originally went the route in this patch and ran 
into some errors testing, so I switched to what I detailed in the RFC.  But 
then I found the problem (I was using narrow chars for the GetCharacter 
callback when that actually isn't supported). Overall I think it is best to use 
narrow char and string rather than wide-char and wstring because that's more 
consistent with the rest of LLVM.

Regarding platforms, I tested on OS X Big Sur and Monterey, and Debian Linux 
inside a VM.  Jan was able to build on Fedora.  I'm happy to test on more 
platforms - FreeBSD, NetBSD perhaps?

> Also I'm kinda curious if you found any docs/examples that explain whether 
> mixing the wchar/char functions like we do now is actually supported in 
> libedit? IIUC we call now `el_wset` and `el_set` on the same libedit 
> instance. It feels like the `wchar` support in the FreeBSD port was some kind 
> of parallel implementation to the normal `char` support, so I'm surprised 
> that we can just mix those functions and everything still works fine (at 
> least on my Linux machine this seems to work).

Yeah, the original source converts the parameters and calls el_w* functions 
when the narrow-char functions are called.  This is also true on FreeBSD: 
https://github.com/freebsd/freebsd-src/blob/373ffc62c158e52cde86a5b934ab4a51307f9f2e/contrib/libedit/eln.c#L359


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106035

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

[Lldb-commits] [PATCH] D106035: Remove conditional compilation for WCHAR support in libedit

[Neal via Phabricator via lldb-commits]

nealsid updated this revision to Diff 359533.
nealsid added a comment.

Reran clang-format and removed extra braces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106035

Files:
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/include/lldb/Host/Editline.h
  lldb/source/Host/common/Editline.cpp

Index: lldb/source/Host/common/Editline.cpp
===
--- lldb/source/Host/common/Editline.cpp
+++ lldb/source/Host/common/Editline.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include 
+#include 
 #include 
 
 #include "lldb/Host/Editline.h"
@@ -61,40 +62,12 @@
 #define ANSI_UP_N_ROWS ESCAPE "[%dA"
 #define ANSI_DOWN_N_ROWS ESCAPE "[%dB"
 
-#if LLDB_EDITLINE_USE_WCHAR
-
-#define EditLineConstString(str) L##str
-#define EditLineStringFormatSpec "%ls"
-
-#else
-
 #define EditLineConstString(str) str
 #define EditLineStringFormatSpec "%s"
 
-// use #defines so wide version functions and structs will resolve to old
-// versions for case of libedit not built with wide char support
-#define history_w history
-#define history_winit history_init
-#define history_wend history_end
-#define HistoryW History
-#define HistEventW HistEvent
-#define LineInfoW LineInfo
-
-#define el_wgets el_gets
-#define el_wgetc el_getc
-#define el_wpush el_push
-#define el_wparse el_parse
-#define el_wset el_set
-#define el_wget el_get
-#define el_wline el_line
-#define el_winsertstr el_insertstr
-#define el_wdeletestr el_deletestr
-
-#endif // #if LLDB_EDITLINE_USE_WCHAR
-
-bool IsOnlySpaces(const EditLineStringType &content) {
-  for (wchar_t ch : content) {
-if (ch != EditLineCharType(' '))
+bool IsOnlySpaces(const std::string &content) {
+  for (char ch : content) {
+if (ch != ' ')
   return false;
   }
   return true;
@@ -132,17 +105,16 @@
   llvm_unreachable("Fully covered switch!");
 }
 
-
-EditLineStringType CombineLines(const std::vector &lines) {
-  EditLineStringStreamType combined_stream;
-  for (EditLineStringType line : lines) {
+std::string CombineLines(const std::vector &lines) {
+  std::stringstream combined_stream;
+  for (const std::string &line : lines) {
 combined_stream << line.c_str() << "\n";
   }
   return combined_stream.str();
 }
 
-std::vector SplitLines(const EditLineStringType &input) {
-  std::vector result;
+std::vector SplitLines(const std::string &input) {
+  std::vector result;
   size_t start = 0;
   while (start < input.length()) {
 size_t end = input.find('\n', start);
@@ -161,23 +133,18 @@
   return result;
 }
 
-EditLineStringType FixIndentation(const EditLineStringType &line,
-  int indent_correction) {
+std::string FixIndentation(const std::string &line, int indent_correction) {
   if (indent_correction == 0)
 return line;
   if (indent_correction < 0)
 return line.substr(-indent_correction);
-  return EditLineStringType(indent_correction, EditLineCharType(' ')) + line;
+  return std::string(indent_correction, ' ') + line;
 }
 
-int GetIndentation(const EditLineStringType &line) {
-  int space_count = 0;
-  for (EditLineCharType ch : line) {
-if (ch != EditLineCharType(' '))
-  break;
-++space_count;
-  }
-  return space_count;
+int GetIndentation(const std::string &line) {
+  auto firstNonSpace = std::find_if(line.begin(), line.end(),
+[](const char ch) { return ch != ' '; });
+  return firstNonSpace - line.begin();
 }
 
 bool IsInputPending(FILE *file) {
@@ -206,10 +173,10 @@
   // these objects
   EditlineHistory(const std::string &prefix, uint32_t size, bool unique_entries)
   : m_history(nullptr), m_event(), m_prefix(prefix), m_path() {
-m_history = history_winit();
-history_w(m_history, &m_event, H_SETSIZE, size);
+m_history = history_init();
+history(m_history, &m_event, H_SETSIZE, size);
 if (unique_entries)
-  history_w(m_history, &m_event, H_SETUNIQUE, 1);
+  history(m_history, &m_event, H_SETUNIQUE, 1);
   }
 
   const char *GetHistoryFilePath() {
@@ -222,11 +189,7 @@
   // LLDB stores its history in ~/.lldb/. If for some reason this directory
   // isn't writable or cannot be created, history won't be available.
   if (!llvm::sys::fs::create_directory(lldb_history_file)) {
-#if LLDB_EDITLINE_USE_WCHAR
-std::string filename = m_prefix + "-widehistory";
-#else
 std::string filename = m_prefix + "-history";
-#endif
 llvm::sys::path::append(lldb_history_file, filename);
 m_path = std::string(lldb_history_file.str());
   }
@@ -243,7 +206,7 @@
 Save();
 
 if (m_history) {
-  history_wend(m_history);
+  history_end(m_history);
   m_history = nullptr;
 }
   }
@@ -268,18 +231,18 @@
 
   bool IsValid() const { return m_history != nullptr; }
 
-  HistoryW *GetHistoryPtr() { return m_history; }
+