date:20210708

[Lldb-commits] [PATCH] D101361: [LLDB] Support AArch64/Linux watchpoint on tagged addresses

2021-07-08 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added inline comments.



Comment at: lldb/test/API/commands/watchpoints/watch_tagged_addr/main.c:11
+
+  __asm__ __volatile__("pacdza %0" : "=r"(tagged_ptr) : "r"(tagged_ptr));
+

Add comments to these inline asm lines to explain the instructions.


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

https://reviews.llvm.org/D101361

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

[Lldb-commits] [PATCH] D101361: [LLDB] Support AArch64/Linux watchpoint on tagged addresses

2021-07-08 Thread David Spickett via Phabricator via lldb-commits

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

LGTM with the comments added.


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

https://reviews.llvm.org/D101361

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

[Lldb-commits] [PATCH] D105483: [LLDB] Testsuite: Add helper to check for AArch64 target

2021-07-08 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1296
+"""Returns true if the architecture is AArch64."""
+return self.getArchitecture().lower() in ["aarch64"]
+

omjavaid wrote:
> DavidSpickett wrote:
> > This can be:
> > ```
> > return self.getArchitecture().lower() == "aarch64"
> > ```
> > 
> > Unless you're expecting "aarch64_be" or "aarch64_32" as well.
> > ```
> > return "aarch64" in self.getArchitecture().lower()
> > ```
> > 
> > Not sure if lldb just has the single name.
> This was intentional as I wanted to keep this helper checking for platform 
> architecture regardless of ABI or endianess. For an ILP32 inferior or be 
> inferior our Native* are same as normal aarch64.
I still think
```
return self.getArchitecture().lower() == "aarch64"
```
Is the same thing unless you intend to expand the list.

(but I understand what you're doing and either way works just a bit less neat 
IMO)


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

https://reviews.llvm.org/D105483

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

[Lldb-commits] [PATCH] D104914: [lldb] Correct format of qMemTags type field

2021-07-08 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added a comment.

Any other comments?




Comment at: 
lldb/test/API/tools/lldb-server/memory-tagging/TestGdbRemoteMemoryTagging.py:22
 """ Test that qMemTags packets are parsed correctly and/or rejected. 
"""
 
 self.build()

omjavaid wrote:
> Missing isAArch64MTE check here?
Added a comment to explain this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104914

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

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

2021-07-08 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added inline comments.

Comment at: lldb/source/Target/Process.cpp:6108
+range_callback) {
   Architecture *arch = GetTarget().GetArchitecturePlugin();
   const MemoryTagManager *tag_manager =

DavidSpickett wrote:
> omjavaid wrote:
> > The point I was trying to establish above is that GetMemoryTagManagerImpl 
> > function here may remain as it was i-e GetMemoryTagManager. It could only 
> > run once on Process object creation. We know our process architecture so we 
> > can host a pointer to relevent tag manager in Process class. All the tag 
> > ranges corresponding to the current process address space should be able to 
> > get a pointer to MemoryTagManager as was being done previously. 
> I see what you mean. I'm going to try this on top of main as a new change, 
> then I'll refactor this based on that if it works out.
> It could only run once on Process object creation.

Turns out that the arch object, where the plugin is stored, changes 2/3 times 
between process creation and loading the program file. So we could store the 
pointer but we'd have to invalidate it on all these events. Ultimately we'd 
spend the time saved doing invalidation.

Which is probably why things like Target::GetCallableLoadAddress just get a new 
pointer each time.

You gave me some good ideas about simplifying this whole process though so I'm 
still going to work on that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105181

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

[Lldb-commits] [PATCH] D105470: [lldb] Clear children of ValueObject on value update

2021-07-08 Thread Andy Yankovsky via Phabricator via lldb-commits

werat added a reviewer: jingham.
werat added a comment.

Jim, can you take a look, please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105470

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

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

2021-07-08 Thread David Spickett via Phabricator via lldb-commits

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

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.


Repository:
  rG LLVM Github Monorepo

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, 0x

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

2021-07-08 Thread David Spickett via Phabricator via lldb-commits

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

This is a response to feedback on https://reviews.llvm.org/D105181 but would be 
intended to come before any of the changes in that series if it looks good.

I'm yet to refactor the tag writing changes on to this, wanted to get some 
feedback first. Roughly tag writing would instead of adding 
`GetMemoryTagManagerForGranules` to process, it would add 
`MakeTaggedRangeForGranules` to the tag manager. Then the `memory tag write` 
command calls that instead of relying on `GetMemoryTagManager` to implement 
that logic.

Then Process simply has `GetMemoryTagManager`, `ReadMemoryTags` and 
`WriteMemoryTags`. Instead of growing all these variants of 
`GetMemoryTagManager`.

Side note: I actually found a bug when writing the unit tests, so I think 
that's a big plus to this approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105630

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

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

2021-07-08 Thread David Spickett via Phabricator via lldb-commits

DavidSpickett added a comment.

Similarly, I have a patch locally that has a "best effort" read where it 
returns a sparse set of results (for annotating memory reads). For that there 
would be a `MakeTaggedRange<...>` that returns a list of ranges that you can 
then read in a loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105630

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

[Lldb-commits] [PATCH] D105470: [lldb] Clear children of ValueObject on value update

2021-07-08 Thread Jim Ingham via Phabricator via lldb-commits

jingham added a comment.

This change is wrong for ValueObjectConstResult's.  The original point of 
ValueObjectConstResult's was to store the results of expressions so that, even 
if the process is not at the original stop point, you could still check the 
value.  It should have the value it had at the time the expression was 
evaluated.  Updating the value is exactly what you don't want to do.  So I 
think you need to make sure that we can't change their value after they are 
created.

Note, ExpressionResult variables are different from 
ExpressionPersistentVariables.  The former should not be updated, and after the 
process moves on any children that weren't gathered become "unknown".  But 
ExpressionPersistentVariables should be able to be assigned to, and when the 
reference target object, it should update them live.  I think this is a useful 
distinction, but it's somewhat messily expressed in the current state of this 
code.  That should get cleaned up IMO but in the meantime we shouldn't make it 
muddier...

The other thing to check about this patch is whether it defeats detecting 
changed values in child elements.  If I make a ValueObject that tracks a value 
in the target - i.e. ValueObjectVariable - and fetch some of its children, then 
step, I should be able to call ValueObject::GetValueDidChange on the child 
element and get a correct result.  It seems to me that if you throw away the 
children at the beginning of the update I don't see how you would have anything 
to compare to.  There are a few tests for GetValueDidChange 
(python_api/value_var_update/TestValueVarUpdate.py).  It would be good to first 
understand whether they just didn't test thoroughly, or if this IS still 
working, how.  It wouldn't be good if this continued working by some accident 
that was going to break later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105470

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

[Lldb-commits] [PATCH] D105389: [lldb] Add AllocateMemory/DeallocateMemory to the SBProcess API

2021-07-08 Thread Greg Clayton via Phabricator via lldb-commits

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

LGTM. Jim, chime in soon if you have any other objections!


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

https://reviews.llvm.org/D105389

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

[Lldb-commits] [PATCH] D105389: [lldb] Add AllocateMemory/DeallocateMemory to the SBProcess API

2021-07-08 Thread Jim Ingham via Phabricator via lldb-commits

jingham accepted this revision.
jingham added a comment.

LGTM


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

https://reviews.llvm.org/D105389

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

[Lldb-commits] [PATCH] D105389: [lldb] Add AllocateMemory/DeallocateMemory to the SBProcess API

2021-07-08 Thread Peter S. Housel via Phabricator via lldb-commits

housel added a comment.

In D105389#2864941 , @clayborg wrote:

> LGTM. Jim, chime in soon if you have any other objections!

I don't have commit access, could some one take care of committing this for me? 
Thanks!


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

https://reviews.llvm.org/D105389

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

[Lldb-commits] [PATCH] D105649: [LLDB] Move Trace-specific classes into separate library

2021-07-08 Thread Alex Langford via Phabricator via lldb-commits

bulbazord created this revision.
bulbazord added reviewers: wallace, clayborg.
Herald added a subscriber: mgorny.
bulbazord requested review of this revision.
Herald added a project: LLDB.

These two classes, TraceSessionFileParser and ThreadPostMortemTrace,
seem to be useful primarily for tracing. Currently it looks like
intel-pt is the sole user of these, but that other tracing plugins could
be written in the future that take advantage of these. Unfortunately
with them in Target, there is a dependency on PluginProcessUtility. I'd
like to sever that dependency, so I moved them into a `TraceCommon`
plugin.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105649

Files:
  lldb/include/lldb/Target/ThreadPostMortemTrace.h
  lldb/include/lldb/Target/TraceSessionFileParser.h
  lldb/include/lldb/lldb-forward.h
  lldb/source/Plugins/Trace/CMakeLists.txt
  lldb/source/Plugins/Trace/common/CMakeLists.txt
  lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.cpp
  lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.h
  lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
  lldb/source/Plugins/Trace/common/TraceSessionFileParser.h
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/ThreadPostMortemTrace.cpp
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceSessionFileParser.cpp

Index: lldb/source/Target/Trace.cpp
===
--- lldb/source/Target/Trace.cpp
+++ lldb/source/Target/Trace.cpp
@@ -17,7 +17,6 @@
 #include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Thread.h"
-#include "lldb/Target/ThreadPostMortemTrace.h"
 #include "lldb/Utility/Stream.h"
 
 using namespace lldb;
Index: lldb/source/Target/CMakeLists.txt
===
--- lldb/source/Target/CMakeLists.txt
+++ lldb/source/Target/CMakeLists.txt
@@ -66,10 +66,8 @@
   ThreadPlanTracer.cpp
   ThreadPlanStack.cpp
   ThreadSpec.cpp
-  ThreadPostMortemTrace.cpp
   Trace.cpp
   TraceCursor.cpp
-  TraceSessionFileParser.cpp
   UnixSignals.cpp
   UnwindAssembly.cpp
   UnwindLLDB.cpp
Index: lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
===
--- lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
+++ lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
@@ -24,6 +24,7 @@
 lldbCore
 lldbSymbol
 lldbTarget
+lldbPluginTraceCommon
 ${LIBIPT_LIBRARY}
   LINK_COMPONENTS
 Support
Index: lldb/source/Plugins/Trace/common/TraceSessionFileParser.h
===
--- lldb/source/Plugins/Trace/common/TraceSessionFileParser.h
+++ lldb/source/Plugins/Trace/common/TraceSessionFileParser.h
@@ -11,7 +11,7 @@
 
 #include "llvm/Support/JSON.h"
 
-#include "lldb/Target/ThreadPostMortemTrace.h"
+#include "ThreadPostMortemTrace.h"
 
 namespace lldb_private {
 
Index: lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
===
--- lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
+++ lldb/source/Plugins/Trace/common/TraceSessionFileParser.cpp
@@ -6,7 +6,8 @@
 //
 //===--===/
 
-#include "lldb/Target/TraceSessionFileParser.h"
+#include "TraceSessionFileParser.h"
+#include "ThreadPostMortemTrace.h"
 
 #include 
 
@@ -14,7 +15,6 @@
 #include "lldb/Core/Module.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
-#include "lldb/Target/ThreadPostMortemTrace.h"
 
 using namespace lldb;
 using namespace lldb_private;
Index: lldb/include/lldb/Target/ThreadPostMortemTrace.h
===
--- /dev/null
+++ lldb/include/lldb/Target/ThreadPostMortemTrace.h
@@ -1,60 +0,0 @@
-//===-- ThreadPostMortemTrace.h -*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-
-#ifndef LLDB_TARGET_THREADPOSTMORTEMTRACE_H
-#define LLDB_TARGET_THREADPOSTMORTEMTRACE_H
-
-#include "lldb/Target/Thread.h"
-
-namespace lldb_private {
-
-/// \class ThreadPostMortemTrace ThreadPostMortemTrace.h
-///
-/// Thread implementation used for representing threads gotten from trace
-/// session files, which are similar to threads from core files.
-///
-/// See \a TraceSessionFileParser for more information regarding trace session
-/// files.
-class ThreadPostMortemTrace : public Thread {
-public:
-  /// \param[in] process
-  /// The process who owns this thread.
-  ///
-  /// \param[in] tid
-  /// The tid of this thread.
-  ///
-  /// \param[in] trace_f

[Lldb-commits] [PATCH] D105649: [LLDB] Move Trace-specific classes into separate library

2021-07-08 Thread walter erquinigo via Phabricator via lldb-commits

wallace accepted this revision.
wallace added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: JDevlieghere.

this looks good to me. Let's wait for Greg's opinion as well


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105649

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

[Lldb-commits] [PATCH] D105215: [lldb] Remove CPlusPlusLanguage from Mangled

2021-07-08 Thread Alex Langford via Phabricator via lldb-commits

bulbazord added a comment.

In D105215#2850988 , @jingham wrote:

> In D105215#2850821 , @bulbazord 
> wrote:
>
>> I kind of feel that `Language::GetDemangledFunctionNameWithoutArguments` may 
>> be a bit too specific for a generalized language plugins. I think it may be 
>> worth it to make `Mangled` an interface that language plugins can implement 
>> (e.g. `CPlusPlusMangledName`) but I haven't totally thought out what the 
>> ramifications of that would be yet.
>
> The name is unfortunate, but the notion that function types have an 
> identifier, that is then decorated by arguments and maybe return types, seems 
> pretty common.  So in this particular case, maybe we just need a better name? 
>  GetBaseName isn't right since this function also returns any namespace 
> information.  Maybe GetFullyQualifiedBaseName?

This may be my ignorance of other languages speaking, but 
`GetFullyQualifiedBaseName` sounds a little more specific to C++. I'm not sure 
if other languages use this terminology, so I tried to keep it as generic as 
possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105215

___
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

2021-07-08 Thread walter erquinigo via Phabricator via lldb-commits

wallace added inline comments.



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:121
 /// stopped at. See \a Trace::GetCursorPosition for more information.
-class DecodedThread {
+class DecodedThread : public std::enable_shared_from_this {
 public:

clayborg wrote:
> You only need to inherit from "std::enable_shared_from_this" if you ever need 
> to take a "DecodedThread *" and get ahold of the original 
> std::shared_ptr. Is that the case here?
i'm using it for calling shared_from_this inside DecodedThread.cpp, which is 
very handy


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

2021-07-08 Thread walter erquinigo via Phabricator via lldb-commits

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

Addressed all issues

- Created a TraceInstructionDumper class that keeps the index to print, which 
leaves the TraceCursor unaffected. It also keeps some other useful state.


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 =

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

2021-07-08 Thread 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 form window to attach a process, either by PID or by
name. This patch also adds support for dynamic field visibility such
that the form delegate can hide or show certain fields based on some
conditions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105655

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -608,9 +608,19 @@
   m_delete = del;
 }
   }
-  //
+
   // Get the rectangle in our parent window
   Rect GetBounds() const { return Rect(GetParentOrigin(), GetSize()); }
+
+  Rect GetCenteredRect(int width, int height) {
+Size size = GetSize();
+width = std::min(size.width, width);
+height = std::min(size.height, height);
+int x = (size.width - width) / 2;
+int y = (size.height - height) / 2;
+return Rect(Point(x, y), Size(width, height));
+  }
+
   int GetChar() { return ::wgetch(m_window); }
   Point GetParentOrigin() const { return Point(GetParentX(), GetParentY()); }
   int GetParentX() const { return getparx(m_window); }
@@ -1050,6 +1060,15 @@
 
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
+
+  bool FieldDelegateIsVisible() { return m_is_visible; }
+
+  void FieldDelegateHide() { m_is_visible = false; }
+
+  void FieldDelegateShow() { m_is_visible = true; }
+
+protected:
+  bool m_is_visible = true;
 };
 
 typedef std::shared_ptr FieldDelegateSP;
@@ -1812,6 +1831,10 @@
 
   virtual ~FormDelegate() = default;
 
+  virtual std::string GetName() = 0;
+
+  virtual void UpdateFieldsVisibility() { return; }
+
   FieldDelegateSP &GetField(int field_index) { return m_fields[field_index]; }
 
   FormAction &GetAction(int action_index) { return m_actions[action_index]; }
@@ -1948,6 +1971,8 @@
 int height = 0;
 height += GetErrorHeight();
 for (int i = 0; i < m_delegate_sp->GetNumberOfFields(); i++) {
+  if (!m_delegate_sp->GetField(i)->FieldDelegateIsVisible())
+continue;
   height += m_delegate_sp->GetField(i)->FieldDelegateGetHeight();
 }
 height += GetActionsHeight();
@@ -1963,6 +1988,8 @@
 
 int offset = GetErrorHeight();
 for (int i = 0; i < m_selection_index; i++) {
+  if (!m_delegate_sp->GetField(i)->FieldDelegateIsVisible())
+continue;
   offset += m_delegate_sp->GetField(i)->FieldDelegateGetHeight();
 }
 context.Offset(offset);
@@ -2018,6 +2045,8 @@
 int width = surface.GetWidth();
 bool a_field_is_selected = m_selection_type == SelectionType::Field;
 for (int i = 0; i < m_delegate_sp->GetNumberOfFields(); i++) {
+  if (!m_delegate_sp->GetField(i)->FieldDelegateIsVisible())
+continue;
   bool is_field_selected = a_field_is_selected && m_selection_index == i;
   FieldDelegateSP &field = m_delegate_sp->GetField(i);
   int height = field->FieldDelegateGetHeight();
@@ -2080,10 +2109,12 @@
   }
 
   bool WindowDelegateDraw(Window &window, bool force) override {
+m_delegate_sp->UpdateFieldsVisibility();
 
 window.Erase();
 
-window.DrawTitleBox(window.GetName(), "Press Esc to cancel");
+window.DrawTitleBox(m_delegate_sp->GetName().c_str(),
+"Press Esc to cancel");
 
 Rect content_bounds = window.GetFrame();
 content_bounds.Inset(2, 2);
@@ -2093,6 +2124,21 @@
 return true;
   }
 
+  void SkipNextHiddenFields() {
+while (true) {
+  if (m_delegate_sp->GetField(m_selection_index)->FieldDelegateIsVisible())
+return;
+
+  if (m_selection_index == m_delegate_sp->GetNumberOfFields() - 1) {
+m_selection_type = SelectionType::Action;
+m_selection_index = 0;
+return;
+  }
+
+  m_selection_index++;
+}
+  }
+
   HandleCharResult SelecteNext(int key) {
 if (m_selection_type == SelectionType::Action) {
   if (m_selection_index < m_delegate_sp->GetNumberOfActions() - 1) {
@@ -2102,8 +2148,12 @@
 
   m_selection_index = 0;
   m_selection_type = SelectionType::Field;
-  FieldDelegateSP &next_field = m_delegate_sp->GetField(m_selection_index);
-  next_field->FieldDelegateSelectFirstElement();
+  SkipNextHiddenFields();
+  if (m_selection_type == SelectionType::Field) {
+FieldDelegateSP &next_field =
+m_delegate_sp->GetField(m_selection_index);
+next_field->FieldDelegateSelectFirstElement();
+  }
   return eKeyHandled;
 }
 
@@ -2121,13 +2171,31 @@
 }
 
 m_selection_index++;
+SkipNextHiddenFields();
 
-FieldDelegateSP &next_field = m_delegate_sp-

[Lldb-commits] [lldb] 1def257 - PR51018: Remove explicit conversions from SmallString to StringRef to future-proof against C++23

2021-07-08 Thread David Blaikie via lldb-commits


Author: David Blaikie
Date: 2021-07-08T13:37:57-07:00
New Revision: 1def2579e10dd84405465f403e8c31acebff0c97

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

LOG: PR51018: Remove explicit conversions from SmallString to StringRef to 
future-proof against C++23

C++23 will make these conversions ambiguous - so fix them to make the
codebase forward-compatible with C++23 (& a follow-up change I've made
will make this ambiguous/invalid even in Children.emplace_back(genReference(Index, InfoPath));
 else
-  SpanBody->Children.emplace_back(genReference(
-  Index, InfoPath, StringRef{Index.JumpToSection.getValue()}));
+  SpanBody->Children.emplace_back(
+  genReference(Index, InfoPath, Index.JumpToSection.getValue().str()));
   }
   if (Index.Children.empty())
 return Out;

diff  --git a/clang-tools-extra/clangd/JSONTransport.cpp 
b/clang-tools-extra/clangd/JSONTransport.cpp
index 3e8caceda21c..49cd4e0903e3 100644
--- a/clang-tools-extra/clangd/JSONTransport.cpp
+++ b/clang-tools-extra/clangd/JSONTransport.cpp
@@ -230,7 +230,7 @@ bool JSONTransport::readStandardMessage(std::string &JSON) {
   return false;
 InMirror << Line;
 
-llvm::StringRef LineRef(Line);
+llvm::StringRef LineRef = Line;
 
 // We allow comments in headers. Technically this isn't part
 
@@ -298,7 +298,7 @@ bool JSONTransport::readDelimitedMessage(std::string &JSON) 
{
   llvm::SmallString<128> Line;
   while (readLine(In, Line)) {
 InMirror << Line;
-auto LineRef = llvm::StringRef(Line).trim();
+auto LineRef = Line.str().trim();
 if (LineRef.startswith("#")) // comment
   continue;
 

diff  --git a/clang-tools-extra/clangd/QueryDriverDatabase.cpp 
b/clang-tools-extra/clangd/QueryDriverDatabase.cpp
index 9704cb8e480f..5e51837b4820 100644
--- a/clang-tools-extra/clangd/QueryDriverDatabase.cpp
+++ b/clang-tools-extra/clangd/QueryDriverDatabase.cpp
@@ -175,8 +175,7 @@ extractSystemIncludesAndTarget(llvm::SmallString<128> 
Driver,
   auto CleanUp = llvm::make_scope_exit(
   [&StdErrPath]() { llvm::sys::fs::remove(StdErrPath); });
 
-  llvm::Optional Redirects[] = {
-  {""}, {""}, llvm::StringRef(StdErrPath)};
+  llvm::Optional Redirects[] = {{""}, {""}, StdErrPath.str()};
 
   llvm::SmallVector Args = {Driver, "-E", "-x",
  Lang,   "-",  "-v"};

diff  --git a/clang-tools-extra/clangd/Selection.cpp 
b/clang-tools-extra/clangd/Selection.cpp
index ad41dec9f20f..b4f767fde095 100644
--- a/clang-tools-extra/clangd/Selection.cpp
+++ b/clang-tools-extra/clangd/Selection.cpp
@@ -771,8 +771,7 @@ llvm::SmallString<256> abbreviatedString(DynTypedNode N,
   }
   auto Pos = Result.find('\n');
   if (Pos != llvm::StringRef::npos) {
-bool MoreText =
-!llvm::all_of(llvm::StringRef(Result).drop_front(Pos), llvm::isSpace);
+bool MoreText = !llvm::all_of(Result.str().drop_front(Pos), llvm::isSpace);
 Result.resize(Pos);
 if (MoreText)
   Result.append(" …");

diff  --git 
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h 
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index 7d0881343478..82dc0b8fdb57 100644
--- 
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -65,7 +65,7 @@ class CachedFileSystemEntry {
   return MaybeStat.getError();
 assert(!MaybeStat->isDirectory() && "not a file");
 assert(isValid() && "not initialized");
-return StringRef(Contents);
+return Contents.str();
   }
 
   /// \returns The error or the status of the entry.

diff  --git a/clang/lib/AST/MicrosoftMangle.cpp 
b/clang/lib/AST/MicrosoftMangle.cpp
index d3ce9aaced7c..3176077804fc 100644
--- a/clang/lib/AST/MicrosoftMangle.cpp
+++ b/clang/lib/AST/MicrosoftMangle.cpp
@@ -3676,7 +3676,7 @@ void 
MicrosoftMangleContextImpl::mangleCXXRTTICompleteObjectLocator(
   assert(VFTableMangling.startswith("??_7") ||
  VFTableMangling.startswith("??_S"));
 
-  Out << "??_R4" << StringRef(VFTableMangling).drop_front(4);
+  Out << "??_R4" << VFTableMangling.str().drop_front(4);
 }
 
 void MicrosoftMangleContextImpl::mangleSEHFilterExpression(

diff  --git a/clang/lib/Analysis/MacroExpansionContext.cpp 
b/clang/lib/Analysis/MacroExpansionContext.cpp
index f261ba8d5389..290510691891 100644
--- a/clang/lib/Analysis/MacroExpansionContext.cpp
+++ b/clang/lib/Analysis/MacroExpansionContext.cpp
@@ -111,7 +111,7 @@ MacroExpansionContext::getExpandedText(SourceLocation 
MacroExpansionLoc) const {
 return StringRef{""};
 
   // Otherwise we have the actual token sequence as string.
-  return StringRef{It->getSecond()};
+  return It->getSecond().str();
 }
 
 Optional

diff  --git a/clang/lib/Basic/FileManager.cpp b/clang/li

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

2021-07-08 Thread Omar Emara via Phabricator via lldb-commits

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

I initially created two forms for attach by name and attach by PID, because the 
options were divided between them. Today I tried to reimplement that such that 
it is a single form with a choices field at the top that determines if it is by 
name or PID. The fields are then shown or hidden based on that enum. 
Additionally, other fields can control the visibility of other fields, for 
instance, the "Include existing processes" field. I think it works fine and the 
UX looks good. Here is an example, what do you think?

F17834972: 20210708-223744.mp4 <https://reviews.llvm.org/F17834972>


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

___
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

2021-07-08 Thread Greg Clayton via Phabricator via lldb-commits

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



Comment at: lldb/source/Commands/CommandObjectThread.cpp:2111
   bool m_create_repeat_command_just_invoked;
-  size_t m_consecutive_repetitions = 0;
+  std::map> m_dumpers;
 };

"Thread *" isn't a stable thing to use, I would suggest using the "tid" of the 
thread. Any reason you are making a map of thread to unique pointers? Can't 
this just be:
```
std::map m_dumpers;
```



Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:41
+lldb::addr_t IntelPTInstruction::GetLoadAddress() const {
+  return IsError() ? LLDB_INVALID_ADDRESS : m_pt_insn.ip;
 }

Shouldn't the m_pt_insn.ip be set to LLDB_INVALID_ADDRESS already?



Comment at: lldb/source/Target/TraceInstructionDumper.cpp:1
+//===-- TraceCursor.cpp 
---===//
+//

Filename should be TraceInstructionDumper.cpp



Comment at: lldb/source/Target/TraceInstructionDumper.cpp:22
+TraceInstructionDumper::TraceInstructionDumper(lldb::TraceCursorUP &&cursor_up,
+   bool forwards, size_t skip,
+   bool raw)

Seems like the "forwards" and "skip" parameter shouldn't be in this class at 
all? Just setup the cursor before giving it to this class?



Comment at: lldb/source/Target/TraceInstructionDumper.cpp:25-30
+  if (forwards)
+m_cursor_up->SeekToBegin();
+  else
+m_cursor_up->SeekToEnd();
+
+  Skip(skip);

Remove and have user set this up prior to creating one of these classes?



Comment at: lldb/source/Target/TraceInstructionDumper.cpp:35
+  if (m_forwards) {
+if (!m_cursor_up->Next()) {
+  m_no_more_data = true;

Should we modify the TraceCursor API for Next(...) to be able to take a count? 
Seems like that might be much more efficient than recursively calling this 
function?



Comment at: lldb/source/Target/TraceInstructionDumper.cpp:41
+  } else {
+if (!m_cursor_up->Prev()) {
+  m_no_more_data = true;

Ditto, should Prev(...) be able to take a count as well?



Comment at: lldb/source/Target/TraceInstructionDumper.cpp:156-159
+/*show_fullpath*/ false,
+/*show_module*/ true, /*show_inlined_frames*/ 
false,
+/*show_function_arguments*/ true,
+/*show_function_name*/ true);

to match llvm coding guidelines, add '=' after variable name



Comment at: lldb/source/Target/TraceInstructionDumper.cpp:167-171
+  insn.instruction->Dump(&s, /*show_address*/ false, /*show_bytes*/ false,
+ /*max_opcode_byte_size*/ 0, &insn.exe_ctx, &insn.sc,
+ /*prev_sym_ctx*/ nullptr,
+ /*disassembly_addr_format*/ nullptr,
+ /*max_address_text_size*/ 0);

Ditto about adding '=', same as above comment.



Comment at: lldb/source/Target/TraceInstructionDumper.cpp:291
+s.Printf("\n");
+TryMoveOneInstruction();
+  }

You should be watching the return value of this right? What if this returns 
false?


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

Re: [Lldb-commits] [PATCH] D105215: [lldb] Remove CPlusPlusLanguage from Mangled

2021-07-08 Thread Jim Ingham via lldb-commits

> On Jul 8, 2021, at 12:25 PM, Alex Langford via Phabricator 
>  wrote:
> 
> bulbazord added a comment.
> 
> In D105215#2850988 , @jingham wrote:
> 
>> In D105215#2850821 , @bulbazord 
>> wrote:
>> 
>>> I kind of feel that `Language::GetDemangledFunctionNameWithoutArguments` 
>>> may be a bit too specific for a generalized language plugins. I think it 
>>> may be worth it to make `Mangled` an interface that language plugins can 
>>> implement (e.g. `CPlusPlusMangledName`) but I haven't totally thought out 
>>> what the ramifications of that would be yet.
>> 
>> The name is unfortunate, but the notion that function types have an 
>> identifier, that is then decorated by arguments and maybe return types, 
>> seems pretty common.  So in this particular case, maybe we just need a 
>> better name?  GetBaseName isn't right since this function also returns any 
>> namespace information.  Maybe GetFullyQualifiedBaseName?
> 
> This may be my ignorance of other languages speaking, but 
> `GetFullyQualifiedBaseName` sounds a little more specific to C++. I'm not 
> sure if other languages use this terminology, so I tried to keep it as 
> generic as possible.
> 

Many other languages (except for C and Fortran) have some notion of namespaces 
& classes which qualify a bare function identifier.  And most have contexts in 
which you can use just the identifier and others where you can or have to 
include the namespace/class qualifiers.  "FullyQualifiedBaseName" was my best 
shot at describing this difference.  Mangling/Demangling seemed to me to have a 
more C++ specific flavor, and plus GetDemangledFunctionNameWithoutArguments 
only obliquely tells you you will be getting the full path to the name, which 
you infer because the C++ demangled name includes this info.

But I was mostly arguing that the concept, which I understood to be "give me 
the full path to this identifier", is a general one, regardless of the name.

Jim

> 
> Repository:
>  rG LLVM Github Monorepo
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D105215/new/
> 
> https://reviews.llvm.org/D105215
> 

___
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

2021-07-08 Thread walter erquinigo via Phabricator via lldb-commits

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



Comment at: lldb/source/Commands/CommandObjectThread.cpp:2111
   bool m_create_repeat_command_just_invoked;
-  size_t m_consecutive_repetitions = 0;
+  std::map> m_dumpers;
 };

clayborg wrote:
> "Thread *" isn't a stable thing to use, I would suggest using the "tid" of 
> the thread. Any reason you are making a map of thread to unique pointers? 
> Can't this just be:
> ```
> std::map m_dumpers;
> ```
I would suggest using the "tid" of the thread -> makes sense

The cursor is a Unique pointer, which renders the TraceInstructionDumper not 
copyable.



Comment at: lldb/source/Target/TraceInstructionDumper.cpp:22
+TraceInstructionDumper::TraceInstructionDumper(lldb::TraceCursorUP &&cursor_up,
+   bool forwards, size_t skip,
+   bool raw)

clayborg wrote:
> Seems like the "forwards" and "skip" parameter shouldn't be in this class at 
> all? Just setup the cursor before giving it to this class?
the direction is useful to knowing whether to call Next or Prev in the 
iterations

the skip is also useful to have inside, because that way it's easier to know if 
all the data has been consumed by the skip. That's stored in the m_no_more_data 
flag. When a trace has been fully consumed, it's still pointing to the last 
instruction, so another variable is needed to prevent more iterations



Comment at: lldb/source/Target/TraceInstructionDumper.cpp:35
+  if (m_forwards) {
+if (!m_cursor_up->Next()) {
+  m_no_more_data = true;

clayborg wrote:
> Should we modify the TraceCursor API for Next(...) to be able to take a 
> count? Seems like that might be much more efficient than recursively calling 
> this function?
that's a pretty good idea


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

2021-07-08 Thread walter erquinigo via Phabricator via lldb-commits

wallace added inline comments.



Comment at: lldb/source/Target/TraceInstructionDumper.cpp:291
+s.Printf("\n");
+TryMoveOneInstruction();
+  }

clayborg wrote:
> You should be watching the return value of this right? What if this returns 
> false?
that's taken care of by 
   if (m_no_more_data) {
  s.Printf("no more data\n");
  break;
}
in the beginning of the iteration


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

2021-07-08 Thread walter erquinigo via Phabricator via lldb-commits

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

Address comments.

- I ended up creating new method Next(int count) and Prev(int count) in the 
cursor for the skip operation. It might be useful for other things. I didn't 
add the count parameter to the existing Prev and Next methods because it makes 
it ambiguous because of the
- I got skip out of the constructor of the Dumper and now the dumper doesn't 
reset the initial position of the cursor. The cursor is set up outside.
- Other improvements here and there


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

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

2021-07-08 Thread Greg Clayton via Phabricator via lldb-commits

clayborg added a comment.

Check my current comments before I proceed to look at the rest. Now that we are 
using the API for something, it is showing some issues with the usability of 
the API in TraceCursor. Let me know what you think




Comment at: lldb/include/lldb/Target/TraceCursor.h:85-105
   virtual bool Next(lldb::TraceInstructionControlFlowType granularity =
 lldb::eTraceInstructionControlFlowTypeInstruction,
 bool ignore_errors = false) = 0;
 
   /// Similar to \a TraceCursor::Next(), but moves backwards chronologically.
   virtual bool Prev(lldb::TraceInstructionControlFlowType granularity =
 lldb::eTraceInstructionControlFlowTypeInstruction,

Since we haven't made the trace cursor API public yet, I wonder if this would 
be a better interface:

```
virtual void SetGranularity(lldb::TraceInstructionControlFlowType granularity);
virtual void SetIgnoreErrors(bool ignore_errors);
virtual void SetDirection(bool forward);
virtual size_t Move(size_t count);
```

Then we don't need Next(...) and Prev(...) now since the direction is set via 
an accessor. The Move(...) function would advance "count" times and it would 
obey the the granularity that is currently active. It also simplifies all of 
the arguments that used to be needed to be passed to the Next(...) and 
Prev(...) functions and gets rid of the default parameters.





Comment at: lldb/include/lldb/Target/TraceCursor.h:97
+  /// \param[in] count
+  /// How many positions to move.
+  ///

clarify that this would move "count" times the granularity right?



Comment at: lldb/include/lldb/Target/TraceCursor.h:107-113
   /// Force the cursor to point to the end of the trace, i.e. the most recent
   /// item.
   virtual void SeekToEnd() = 0;
 
   /// Force the cursor to point to the beginning of the trace, i.e. the oldest
   /// item.
   virtual void SeekToBegin() = 0;

Wondering if we want to replace these with more of a file like seek with a 
offset and whence?:
```
enum class SeekType { Set, Current, End };
bool Seek(int64_t offset, SeekType whence);
```
Then SeekToEnd() would become:
```
Seek(0, SeekType::End);
```
And SeekToBegin():
```
Seek(0, SeekType::Set);
```
And to skip some instructions:
```
Seek(10, SeekType::Current); // Advance in the direction by 10 granularity units
Seek(-10, SeekType::Current); // Reverse in the direction by 10 granularity 
units
```


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] D105630: [lldb][AArch64] Refactor memory tag range handling

2021-07-08 Thread Muhammad Omair Javaid via Phabricator via lldb-commits

omjavaid added a comment.

I think this approach is a lot better given that the process of tagged ranged 
calculation deserved a separation and may be more explanation too. I guess 
somewhere in the comments of same function you can explain interaction between 
memory regions and tagged ranges. It will become a bit more complicated for a 
reader who have no knowledge about memory tag range and disjoint region.




Comment at: lldb/include/lldb/Target/MemoryTagManager.h:66
+  // If so, return a modified range. This will be expanded to be
+  // granule aligned and have an untagged base address.
+  virtual llvm::Expected MakeTaggedRange(

I couldnt understand this point in comment  "have an untagged base address" 



Comment at: lldb/source/Commands/CommandObjectMemoryTag.cpp:70
 Process *process = m_exe_ctx.GetProcessPtr();
 llvm::Expected tag_manager_or_err =
+process->GetMemoryTagManager();

If we also pull out the SupportsMemoryTagging out of GetMemoryTagManager we can 
directly return a tag manager pointer here.

Also SupportsMemoryTagging may also run once for the life time of a process? We 
can store this information is a flag or something.





Comment at: lldb/source/Commands/CommandObjectMemoryTag.cpp:71
 llvm::Expected tag_manager_or_err =
-process->GetMemoryTagManager(start_addr, end_addr);
+process->GetMemoryTagManager();
 

should we process pointer validity check before this call?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105630

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

[Lldb-commits] [PATCH] D101361: [LLDB] Support AArch64/Linux watchpoint on tagged addresses

[Lldb-commits] [PATCH] D101361: [LLDB] Support AArch64/Linux watchpoint on tagged addresses

[Lldb-commits] [PATCH] D105483: [LLDB] Testsuite: Add helper to check for AArch64 target

[Lldb-commits] [PATCH] D104914: [lldb] Correct format of qMemTags type field

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

[Lldb-commits] [PATCH] D105470: [lldb] Clear children of ValueObject on value update

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

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

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

[Lldb-commits] [PATCH] D105470: [lldb] Clear children of ValueObject on value update

[Lldb-commits] [PATCH] D105389: [lldb] Add AllocateMemory/DeallocateMemory to the SBProcess API

[Lldb-commits] [PATCH] D105389: [lldb] Add AllocateMemory/DeallocateMemory to the SBProcess API

[Lldb-commits] [PATCH] D105389: [lldb] Add AllocateMemory/DeallocateMemory to the SBProcess API

[Lldb-commits] [PATCH] D105649: [LLDB] Move Trace-specific classes into separate library

[Lldb-commits] [PATCH] D105649: [LLDB] Move Trace-specific classes into separate library

[Lldb-commits] [PATCH] D105215: [lldb] Remove CPlusPlusLanguage from Mangled

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

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

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Lldb-commits] [lldb] 1def257 - PR51018: Remove explicit conversions from SmallString to StringRef to future-proof against C++23

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

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

Re: [Lldb-commits] [PATCH] D105215: [lldb] Remove CPlusPlusLanguage from Mangled

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

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

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

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

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

28 matches

Site Navigation

Mail list logo

Footer information