[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

I've been thinking about what you said and I'm having second thoughts on my 
implementation. I'll share more context:

- I want to work in the short term on reverse debugging and reconstruction of 
stack traces, for that i'll need to know the instruction type of each 
instruction in the trace, which will be used as part of some heuristics to 
identify calls and returns between functions.
- A future application that we plan to work on is adding profiling information 
to the instructions
- Right now the intel-pt plugin is storing the decoded instructions in a 
vector, which works for small traces but wont' for gigantic traces. I imagine 
that I'll end up removing that vector and make the TraverseInstruction API 
decode instructions in place keeping one instruction in memory at a time within 
the intel pt plugin for a given traversal. For that I'll need accessors that 
can provide information of the current Instruction. As there could be two or 
more concurrent traversals happening at the same time (I'm trying to be generic 
here), it might make sense to create an abstract class TraceInstruction that 
can be extended by each plug-in and implement its getters.

I'm thinking about something like this

  class TraceInstruction {
virtual lldb::addr_t GetLoadAddress() = 0;
virtual TraceInstructionType() GetInstructionType() = 0;
virtual uint64_t GetTimestamp() = 0;
... anything that can appear in the future 
  };

and have no members, leaving to each plug-in the decision of which of those 
methods to implement and how.

What do you think of this? I think this incorporates your feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103588

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

[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

In D103588#2806611 , @wallace wrote:

>> This approach - of prototyping trace analyses on a vector 
>> representation and then rewriting them later when scaling issues arise - 
>> doesn't sound good to me. Even for something simple, like finding a 
>> backtrace, the window of instructions needed for analysis can easily grow to 
>> a point where the full vector can't be maintained in 
>> memory.
>
> It's worth noticing that the vector is internal to the 
> Intel PT plugin. It's not exposed at the Trace.h interface level, so any 
> consumers can't make any assumptions on how the data is stored. We will 
> improve the intel pt plugin itself as we make progress on this, but we are 
> ensuring that the interfaces are correct for the future.

This doesn't quite alleviate my concern. We'd like for the generic trace 
infrastructure (outside of the PT plugin) to support different tracing 
technologies as well. It's not clear that the TraceInstruction concept can do 
that.

Two questions that have remained unanswered here: (1) how would 
TraceInstruction be used by non-PT plugins? (2) how do we guarantee the generic 
analyses you're planning to write will scale?

>> To clarify: I think that's a perfectly reasonable way to prototype trace 
>> analyses, but I don't think that kind of prototyping should be done at the 
>> SB API level, as there are extra bincompat & portability concerns here.
>
> I know, and I want to stress out that we are not exposing any of this 
> instruction information at the SB API level and probably we won't for a 
> while, i.e. I don't plan on creating a SBTraceInstruction class, as there's 
> too much boilerplate needed to expose an SBInstruction object. For now, we 
> want to have some code that processes instructions and that exists at the 
> Trace.h level, outside of the intel-pt plugin, as we want to make it generic 
> enough to be utilized by different tracing technologies.

lldb's SB interfaces have always been relatively thin wrappers around classes 
in include/lldb (SBTarget -> Target, SBProcess -> Process, etc.). So it's 
really worth spending the extra time to get the generic interfaces right, even 
if they're not SB API yet, since that's ultimately the direction in which we'll 
want to go.

>> It's not at all clear to me that lldb needs to surface a generic 
>> TraceInstruction object, with metadata like m_byte_size, m_inst_type, etc. 
>> stored inline. It seems more flexible to instead vend a cursor (just an 
>> integer) that indicates a position in the trace; a separate set of APIs 
>> could answer queries given a cursor, e.g. 'getInstTypeAtCursor(u64 cursor)'.
>
> There's an advantage. Decoding intel-pt instructions is very slow and we are 
> able to produce correct indices only if we decode the raw trace sequentially, 
> either backwards (for negative indices) or forwards (for positive indices). 
> If we start from the middle, we don't know which instruction we are at. If we 
> allow the user to query properties of arbitrary indices, we might need to 
> decode a big prefix or a big suffix of the trace until we get to that index. 
> It seems safer to me to expose right away the properties of these 
> instructions as soon as they are decoded, so that we don't need any further 
> decoding if the user invokes a query method with a possibly wrong index.

I don't see how the fact that PT decoding is slow necessitates the 
TraceInstruction representation. I don't find that the issues you're ascribing 
to a cursor representation are inevitable, or all that different from the 
issues you'd run into with generating a large number of TraceInstructions for a 
subset of a trace (also not clear how this subset would be specified).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103588

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

[Lldb-commits] [PATCH] D103500: [trace][intel-pt] Create basic SB API

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 350725.
wallace marked 11 inline comments as done.
wallace added a comment.
Herald added a subscriber: mgorny.

address all comments. I appreciate your feedback!

- I ended up deleting all the deprecated methods, as I doubt anyone is using 
them. Intel wrote those methods several years ago and they were used by the old 
intel pt plugin, which was effectively deleted by 
https://reviews.llvm.org/D102866, so I imagine no one using it anymore. Btw, 
that old plugin has been broken for a while.
- I also fixed the USE_SB_API error in the python test. Good catch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103500

Files:
  lldb/bindings/headers.swig
  lldb/bindings/interface/SBProcess.i
  lldb/bindings/interface/SBStructuredData.i
  lldb/bindings/interface/SBTarget.i
  lldb/bindings/interface/SBTrace.i
  lldb/bindings/interface/SBTraceOptions.i
  lldb/bindings/interfaces.swig
  lldb/docs/.htaccess
  lldb/docs/design/overview.rst
  lldb/docs/lldb-gdb-remote.txt
  lldb/include/lldb/API/LLDB.h
  lldb/include/lldb/API/SBDefines.h
  lldb/include/lldb/API/SBProcess.h
  lldb/include/lldb/API/SBStructuredData.h
  lldb/include/lldb/API/SBTarget.h
  lldb/include/lldb/API/SBThread.h
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/API/SBTraceOptions.h
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Utility/TraceGDBRemotePackets.h
  lldb/include/lldb/Utility/TraceOptions.h
  lldb/include/lldb/lldb-forward.h
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/API/CMakeLists.txt
  lldb/source/API/SBProcess.cpp
  lldb/source/API/SBReproducer.cpp
  lldb/source/API/SBStructuredData.cpp
  lldb/source/API/SBTarget.cpp
  lldb/source/API/SBTrace.cpp
  lldb/source/API/SBTraceOptions.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
  lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/Trace.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceLoad.py
  lldb/test/API/commands/trace/TestTraceSchema.py
  lldb/test/API/commands/trace/TestTraceStartStop.py
  
lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
  lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
===
--- lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -12,7 +12,6 @@
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/StructuredData.h"
-#include "lldb/Utility/TraceOptions.h"
 #include "lldb/lldb-enumerations.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Testing/Support/Error.h"
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
@@ -1,53 +1,49 @@
 import lldb
+from intelpt_testcase import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 from lldbsuite.test.decorators import *
 
-ADDRESS_REGEX = '0x[0-9a-fA-F]*'
-
-class TestTraceStartStopMultipleThreads(TestBase):
+class TestTraceStartStopMultipleThreads(TraceIntelPTTestCaseBase):
 
 mydir = TestBase.compute_mydir(__file__)
-NO_DEBUG_INFO_TESTCASE = True
-
-def setUp(self):
-TestBase.setUp(self)
-if 'intel-pt' not in configuration.enabled_plugins:
-self.skipTest("The intel-pt test plugin is not enabled")
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
+@testSBAPIAndCommands
 def testStartMultipleLiveThreads(self):
 self.build()
-target = self.createTestTarget()
+exe = self.getBuildArtifact("a.out")
+
+self.dbg.CreateTarget(exe)
 
 self.expect("b main")
 self.expect("b 6")
 self.expect("b 11")
 
 self.expect("r")
-self.expect("proce trace start")
-
+

[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

> This approach - of prototyping trace analyses on a vector 
> representation and then rewriting them later when scaling issues arise - 
> doesn't sound good to me. Even for something simple, like finding a 
> backtrace, the window of instructions needed for analysis can easily grow to 
> a point where the full vector can't be maintained in memory.

It's worth noticing that the vector is internal to the Intel 
PT plugin. It's not exposed at the Trace.h interface level, so any consumers 
can't make any assumptions on how the data is stored. We will improve the intel 
pt plugin itself as we make progress on this, but we are ensuring that the 
interfaces are correct for the future.

> To clarify: I think that's a perfectly reasonable way to prototype trace 
> analyses, but I don't think that kind of prototyping should be done at the SB 
> API level, as there are extra bincompat & portability concerns here.

I know, and I want to stress out that we are not exposing any of this 
instruction information at the SB API level and probably we won't for a while, 
i.e. I don't plan on creating a SBTraceInstruction class, as there's too much 
boilerplate needed to expose an SBInstruction object. For now, we want to have 
some code that processes instructions and that exists at the Trace.h level, 
outside of the intel-pt plugin, as we want to make it generic enough to be 
utilized by different tracing technologies.

> It's not at all clear to me that lldb needs to surface a generic 
> TraceInstruction object, with metadata like m_byte_size, m_inst_type, etc. 
> stored inline. It seems more flexible to instead vend a cursor (just an 
> integer) that indicates a position in the trace; a separate set of APIs could 
> answer queries given a cursor, e.g. 'getInstTypeAtCursor(u64 cursor)'.

There's an advantage. Decoding intel-pt instructions is very slow and we are 
able to produce correct indices only if we decode the raw trace sequentially, 
either backwards (for negative indices) or forwards (for positive indices). If 
we start from the middle, we don't know which instruction we are at. If we 
allow the user to query properties of arbitrary indices, we might need to 
decode a big prefix or a big suffix of the trace until we get to that index. It 
seems safer to me to expose right away the properties of these instructions as 
soon as they are decoded, so that we don't need any further decoding if the 
user invokes a query method with a possibly wrong index.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103588

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

[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

[Vedant Kumar via Phabricator via lldb-commits]

vsk added a comment.

In D103588#2806512 , @wallace wrote:

> Right now we haven't implemented lazy decoding; we are decoding the entire 
> trace. But that's just because so far we have been working with small traces. 
> As we progress in this work, we'll start working with larger traces and we'll 
> have to do implement the lazy decoding, for which the TraverseInstructions 
> API will come handy.

This approach - of prototyping trace analyses on a vector 
representation and then rewriting them later when scaling issues arise - 
doesn't sound good to me. Even for something simple, like finding a backtrace, 
the window of instructions needed for analysis can easily grow to a point where 
the full vector can't be maintained in memory.

To clarify: I think that's a perfectly reasonable way to prototype trace 
analyses, but I don't think that kind of prototyping should be done at the SB 
API level, as there are extra bincompat & portability concerns here.

>> What alternatives to the vector representation have you 
>> considered? One idea might be to implement your program analyses on top of a 
>> generic interface for navigating forward/backward through a trace and 
>> extracting info about the instruction via a set of API calls; this leaves 
>> the actual in-memory representation of "TraceInstruction" unspecified.
>
> That's exactly what I described as the TraverseInstructions method :) The 
> intel-pt plugin or any other trace plugin could have an internal 
> representation for traces, but we still need a generic TraceInstruction 
> object for consumers that want to be agnostic of the trace technology.

It's not at all clear to me that lldb needs to surface a generic 
TraceInstruction object, with metadata like m_byte_size, m_inst_type, etc. 
stored inline. It seems more flexible to instead vend a cursor (just an 
integer) that indicates a position in the trace; a separate set of APIs could 
answer queries given a cursor, e.g. 'getInstTypeAtCursor(u64 cursor)'.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103588

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

[Lldb-commits] [lldb] ae1a699 - [LLDB][NFC] Remove parameter names from forward declarations from hand written expressions used in heap.py

[Shafik Yaghmour via lldb-commits]

Author: Shafik Yaghmour
Date: 2021-06-08T14:27:02-07:00
New Revision: ae1a699554cfa01d9fb307a964c3a9f71831a62e

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

LOG: [LLDB][NFC] Remove parameter names from forward declarations from hand 
written expressions used in heap.py

heap.py has a lot of large hand written expressions and each name in the
expression will be looked up by clang during expression parsing. For
function parameters this will be in Sema::ActOnParamDeclarator(...) in order to
catch redeclarations of parameters. The names are not needed and we have seen
some rare cases where since we don't have symbols we end up in
SymbolContext::FindBestGlobalDataSymbol(...) which may conflict with other 
global
symbols.

There may be a way to make this lookup smarter to avoid these cases but it is
not clear how well tested this path is and how much work it would be to fix it.
So we will go with this fix while we investigate more.

Ref: rdar://78265641

Added: 


Modified: 
lldb/examples/darwin/heap_find/heap.py

Removed: 




diff  --git a/lldb/examples/darwin/heap_find/heap.py 
b/lldb/examples/darwin/heap_find/heap.py
index 4174d396feb6b..830e851e21056 100644
--- a/lldb/examples/darwin/heap_find/heap.py
+++ b/lldb/examples/darwin/heap_find/heap.py
@@ -36,7 +36,7 @@ def get_iterate_memory_expr(
 typedef natural_t task_t;
 typedef int kern_return_t;
 #define KERN_SUCCESS 0
-typedef void (*range_callback_t)(task_t task, void *baton, unsigned type, 
uintptr_t ptr_addr, uintptr_t ptr_size);
+typedef void (*range_callback_t)(task_t, void *, unsigned, uintptr_t, 
uintptr_t);
 '''
 if options.search_vm_regions:
 expr += '''
@@ -120,8 +120,8 @@ def get_iterate_memory_expr(
 vm_address_t   address;
 vm_size_t  size;
 } vm_range_t;
-typedef kern_return_t (*memory_reader_t)(task_t task, vm_address_t 
remote_address, vm_size_t size, void **local_memory);
-typedef void (*vm_range_recorder_t)(task_t task, void *baton, unsigned type, 
vm_range_t *range, unsigned size);
+typedef kern_return_t (*memory_reader_t)(task_t, vm_address_t, vm_size_t, void 
**);
+typedef void (*vm_range_recorder_t)(task_t, void *, unsigned, vm_range_t *, 
unsigned);
 typedef struct malloc_introspection_t {
 kern_return_t (*enumerator)(task_t task, void *, unsigned type_mask, 
vm_address_t zone_address, memory_reader_t reader, vm_range_recorder_t 
recorder); /* enumerates all the malloc pointers in use */
 } malloc_introspection_t;
@@ -130,7 +130,7 @@ def get_iterate_memory_expr(
 struct malloc_introspection_t  *introspect;
 } malloc_zone_t;
 kern_return_t malloc_get_all_zones(task_t task, memory_reader_t reader, 
vm_address_t **addresses, unsigned *count);
-memory_reader_t task_peek = [](task_t task, vm_address_t remote_address, 
vm_size_t size, void **local_memory) -> kern_return_t {
+memory_reader_t task_peek = [](task_t, vm_address_t remote_address, vm_size_t, 
void **local_memory) -> kern_return_t {
 *local_memory = (void*) remote_address;
 return KERN_SUCCESS;
 };



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

Re: [Lldb-commits] [lldb] e05b03c - [lldb] Set return status to failed when adding a command error

[Jim Ingham via lldb-commits]

Hey, David,

This commit seems to have caused a new failure in TestRegisters.py, e.g.:

http://green.lab.llvm.org/green/blue/organizations/jenkins/lldb-cmake/detail/lldb-cmake/32693/pipeline/

Do you have time to take a look?

Jim




> On Jun 8, 2021, at 1:41 AM, David Spickett via lldb-commits 
>  wrote:
> 
> 
> Author: David Spickett
> Date: 2021-06-08T09:41:07+01:00
> New Revision: e05b03cf4f45ac5ee63c59a3464e7d484884645c
> 
> URL: 
> https://github.com/llvm/llvm-project/commit/e05b03cf4f45ac5ee63c59a3464e7d484884645c
> DIFF: 
> https://github.com/llvm/llvm-project/commit/e05b03cf4f45ac5ee63c59a3464e7d484884645c.diff
> 
> LOG: [lldb] Set return status to failed when adding a command error
> 
> There is a common pattern:
> result.AppendError(...);
> result.SetStatus(eReturnStatusFailed);
> 
> I found that some commands don't actually "fail" but only
> print "error: ..." because the second line got missed.
> 
> This can cause you to miss a failed command when you're
> using the Python interface during testing.
> (and produce some confusing script results)
> 
> I did not find any place where you would want to add
> an error without setting the return status, so just
> set eReturnStatusFailed whenever you add an error to
> a command result.
> 
> This change does not remove any of the now redundant
> SetStatus. This should allow us to see if there are any
> tests that have commands unexpectedly fail with this change.
> (the test suite passes for me but I don't have access to all
> the systems we cover so there could be some corner cases)
> 
> Some tests that failed on x86 and AArch64 have been modified
> to work with the new behaviour.
> 
> Differential Revision: https://reviews.llvm.org/D103701
> 
> Added: 
>lldb/test/Shell/Commands/command-backtrace-parser-1.test
>lldb/test/Shell/Commands/command-backtrace-parser-2.test
> 
> Modified: 
>lldb/source/Interpreter/CommandReturnObject.cpp
>lldb/test/API/commands/register/register/register_command/TestRegisters.py
> 
> Removed: 
>lldb/test/Shell/Commands/command-backtrace.test
> 
> 
> 
> diff  --git a/lldb/source/Interpreter/CommandReturnObject.cpp 
> b/lldb/source/Interpreter/CommandReturnObject.cpp
> index 77d94bd9389c3..980d39bfb1681 100644
> --- a/lldb/source/Interpreter/CommandReturnObject.cpp
> +++ b/lldb/source/Interpreter/CommandReturnObject.cpp
> @@ -46,6 +46,8 @@ CommandReturnObject::CommandReturnObject(bool colors)
>   m_interactive(true) {}
> 
> void CommandReturnObject::AppendErrorWithFormat(const char *format, ...) {
> +  SetStatus(eReturnStatusFailed);
> +
>   if (!format)
> return;
>   va_list args;
> @@ -100,6 +102,7 @@ void CommandReturnObject::AppendWarning(llvm::StringRef 
> in_string) {
> void CommandReturnObject::AppendError(llvm::StringRef in_string) {
>   if (in_string.empty())
> return;
> +  SetStatus(eReturnStatusFailed);
>   error(GetErrorStream()) << in_string.rtrim() << '\n';
> }
> 
> @@ -116,7 +119,6 @@ void CommandReturnObject::SetError(llvm::StringRef 
> error_str) {
> return;
> 
>   AppendError(error_str);
> -  SetStatus(eReturnStatusFailed);
> }
> 
> // Similar to AppendError, but do not prepend 'Status: ' to message, and don't
> @@ -126,6 +128,7 @@ void CommandReturnObject::AppendRawError(llvm::StringRef 
> in_string) {
>   if (in_string.empty())
> return;
>   GetErrorStream() << in_string;
> +  SetStatus(eReturnStatusFailed);
> }
> 
> void CommandReturnObject::SetStatus(ReturnStatus status) { m_status = status; 
> }
> 
> diff  --git 
> a/lldb/test/API/commands/register/register/register_command/TestRegisters.py 
> b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
> index 5ec46c175e621..7acf3a4098756 100644
> --- 
> a/lldb/test/API/commands/register/register/register_command/TestRegisters.py
> +++ 
> b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
> @@ -41,13 +41,18 @@ def test_register_commands(self):
> self.expect("register read -a", MISSING_EXPECTED_REGISTERS,
> substrs=['registers were unavailable'], matching=False)
> 
> +all_registers = self.res.GetOutput()
> +
> if self.getArchitecture() in ['amd64', 'i386', 'x86_64']:
> self.runCmd("register read xmm0")
> -self.runCmd("register read ymm15")  # may be available
> -self.runCmd("register read bnd0")  # may be available
> +if "ymm15 = " in all_registers:
> +  self.runCmd("register read ymm15")  # may be available
> +if "bnd0 = " in all_registers:
> +  self.runCmd("register read bnd0")  # may be available
> elif self.getArchitecture() in ['arm', 'armv7', 'armv7k', 'arm64', 
> 'arm64e', 'arm64_32']:
> self.runCmd("register read s0")
> -self.runCmd("register read q15")  # may be available
> +if "q15 = " in all_registers:
> +   

[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

[walter erquinigo via Phabricator via lldb-commits]

wallace requested review of this revision.
wallace added a comment.

> At Apple, we use lldb to navigate instruction traces that contain billions of 
> instructions. Allocating 16 bytes per instruction simply will not scale for 
> our workflows. We require the in-memory overhead to be approximately 1-2 bits 
> per instruction. I'm not familiar with how large IntelPT traces can get, but 
> presumably (for long enough traces) you will encounter the same scaling 
> problem.

I have thought about that and I'm taking it into account. We implemented a 
TraverseInstructions API in Trace.h that receives a callback (see 
https://lldb.llvm.org/cpp_reference/classlldb__private_1_1Trace.html#a02e346b117c15cef1cb0568c343f7e1c).
 The idea is that the intel pt plugin can decode the instructions on the fly as 
part of the iteration. 
In terms of memory, there are two kinds of trace objects: one is the raw 
undecoded instruction trace that uses 1 bit per instruction in avg. That one is 
decoded in 
https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp#L163
 into TraceInstructions, which effectively use 16 bytes in the duration of 
their lives. Right now we haven't implemented lazy decoding; we are decoding 
the entire trace. But that's just because so far we have been working with 
small traces. As we progress in this work, we'll start working with larger 
traces and we'll have to do implement the lazy decoding, for which the 
TraverseInstructions API will come handy.

> What alternatives to the vector representation have you 
> considered? One idea might be to implement your program analyses on top of a 
> generic interface for navigating forward/backward through a trace and 
> extracting info about the instruction via a set of API calls; this leaves the 
> actual in-memory representation of "TraceInstruction" unspecified.

That's exactly what I described as the TraverseInstructions method :) The 
intel-pt plugin or any other trace plugin could have an internal representation 
for traces, but we still need a generic TraceInstruction object for consumers 
that want to be agnostic of the trace technology.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103588

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

[Lldb-commits] [PATCH] D103349: [lldb] Don't print script output twice in HandleCommand

[Jonas Devlieghere 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 rG1a216fb15a18: [lldb] Dont print script output twice in 
HandleCommand (authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D103349?vs=349651=350709#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103349

Files:
  lldb/include/lldb/Interpreter/CommandReturnObject.h
  lldb/lldb/test/Shell/Breakpoint/breakpoint-command.test
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Interpreter/CommandReturnObject.cpp
  lldb/test/Shell/Breakpoint/breakpoint-command.test
  lldb/test/Shell/ScriptInterpreter/Lua/nested_sessions.test

Index: lldb/test/Shell/ScriptInterpreter/Lua/nested_sessions.test
===
--- lldb/test/Shell/ScriptInterpreter/Lua/nested_sessions.test
+++ lldb/test/Shell/ScriptInterpreter/Lua/nested_sessions.test
@@ -7,6 +7,5 @@
 # CHECK-NEXT: foo foo
 # CHECK-NEXT: foo bar
 # CHECK-NEXT: foo bar
-# CHECK-NEXT: foo bar
 # CHECK: script
 # CHECK-NEXT: bar bar
Index: lldb/test/Shell/Breakpoint/breakpoint-command.test
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/breakpoint-command.test
@@ -0,0 +1,5 @@
+# RUN: %build %p/Inputs/dummy-target.c -o %t.out
+# RUN: %lldb %t.out -o 'b main' -o 'break command add 1 -o "script print(95000 + 126)"' -o 'r'
+
+# CHECK: 95125
+# CHECK-NOT: 95126
Index: lldb/source/Interpreter/CommandReturnObject.cpp
===
--- lldb/source/Interpreter/CommandReturnObject.cpp
+++ lldb/source/Interpreter/CommandReturnObject.cpp
@@ -41,9 +41,7 @@
 }
 
 CommandReturnObject::CommandReturnObject(bool colors)
-: m_out_stream(colors), m_err_stream(colors),
-  m_status(eReturnStatusStarted), m_did_change_process_state(false),
-  m_interactive(true) {}
+: m_out_stream(colors), m_err_stream(colors) {}
 
 void CommandReturnObject::AppendErrorWithFormat(const char *format, ...) {
   SetStatus(eReturnStatusFailed);
@@ -154,6 +152,7 @@
 static_cast(stream_sp.get())->Clear();
   m_status = eReturnStatusStarted;
   m_did_change_process_state = false;
+  m_suppress_immediate_output = false;
   m_interactive = true;
 }
 
@@ -168,3 +167,11 @@
 bool CommandReturnObject::GetInteractive() const { return m_interactive; }
 
 void CommandReturnObject::SetInteractive(bool b) { m_interactive = b; }
+
+bool CommandReturnObject::GetSuppressImmediateOutput() const {
+  return m_suppress_immediate_output;
+}
+
+void CommandReturnObject::SetSuppressImmediateOutput(bool b) {
+  m_suppress_immediate_output = b;
+}
Index: lldb/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/source/Interpreter/CommandInterpreter.cpp
+++ lldb/source/Interpreter/CommandInterpreter.cpp
@@ -2306,6 +2306,7 @@
 
 CommandReturnObject tmp_result(m_debugger.GetUseColor());
 tmp_result.SetInteractive(result.GetInteractive());
+tmp_result.SetSuppressImmediateOutput(true);
 
 // We might call into a regex or alias command, in which case the
 // add_to_history will get lost.  This m_command_source_depth dingus is the
Index: lldb/lldb/test/Shell/Breakpoint/breakpoint-command.test
===
--- /dev/null
+++ lldb/lldb/test/Shell/Breakpoint/breakpoint-command.test
@@ -0,0 +1,5 @@
+# RUN: %build %p/Inputs/dummy-target.c -o %t.out
+# RUN: %lldb %t.out -o 'b main' -o 'break command add 1 -o "script print(95000 + 126)"' -o 'r'
+
+# CHECK: 95125
+# CHECK-NOT: 95126
Index: lldb/include/lldb/Interpreter/CommandReturnObject.h
===
--- lldb/include/lldb/Interpreter/CommandReturnObject.h
+++ lldb/include/lldb/Interpreter/CommandReturnObject.h
@@ -63,20 +63,28 @@
   }
 
   void SetImmediateOutputFile(lldb::FileSP file_sp) {
+if (m_suppress_immediate_output)
+  return;
 lldb::StreamSP stream_sp(new StreamFile(file_sp));
 m_out_stream.SetStreamAtIndex(eImmediateStreamIndex, stream_sp);
   }
 
   void SetImmediateErrorFile(lldb::FileSP file_sp) {
+if (m_suppress_immediate_output)
+  return;
 lldb::StreamSP stream_sp(new StreamFile(file_sp));
 m_err_stream.SetStreamAtIndex(eImmediateStreamIndex, stream_sp);
   }
 
   void SetImmediateOutputStream(const lldb::StreamSP _sp) {
+if (m_suppress_immediate_output)
+  return;
 m_out_stream.SetStreamAtIndex(eImmediateStreamIndex, stream_sp);
   }
 
   void SetImmediateErrorStream(const lldb::StreamSP _sp) {
+if (m_suppress_immediate_output)
+  return;
 m_err_stream.SetStreamAtIndex(eImmediateStreamIndex, stream_sp);
   }
 
@@ -142,16 +150,23 @@
 
   void 

[Lldb-commits] [lldb] 1a216fb - [lldb] Don't print script output twice in HandleCommand

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2021-06-08T13:57:39-07:00
New Revision: 1a216fb15a188f9ac7de6d79267b3cfb2946f792

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

LOG: [lldb] Don't print script output twice in HandleCommand

When executing a script command in HandleCommand(s) we currently print
its output twice
You can see this issue in action when adding a breakpoint command:

(lldb) b main
Breakpoint 1: where = main.out`main + 13 at main.cpp:2:3, address = 
0x00013fad
(lldb) break command add 1 -o "script print(\"Hey!\")"
(lldb) r
Process 76041 launched: '/tmp/main.out' (x86_64)
Hey!
(lldb)  script print("Hey!")
Hey!
Process 76041 stopped

The issue is caused by HandleCommands using a temporary
CommandReturnObject and one of the commands (`script` in this case)
setting an immediate output stream. This causes the result to be printed
twice: once directly to the immediate output stream and once when
printing the result of HandleCommands.

This patch fixes the issue by introducing a new option to suppress
immediate output for temporary CommandReturnObjects.

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

Added: 
lldb/lldb/test/Shell/Breakpoint/breakpoint-command.test
lldb/test/Shell/Breakpoint/breakpoint-command.test

Modified: 
lldb/include/lldb/Interpreter/CommandReturnObject.h
lldb/source/Interpreter/CommandInterpreter.cpp
lldb/source/Interpreter/CommandReturnObject.cpp
lldb/test/Shell/ScriptInterpreter/Lua/nested_sessions.test

Removed: 




diff  --git a/lldb/include/lldb/Interpreter/CommandReturnObject.h 
b/lldb/include/lldb/Interpreter/CommandReturnObject.h
index c638b4bc70b2c..e2b237c6d0f1c 100644
--- a/lldb/include/lldb/Interpreter/CommandReturnObject.h
+++ b/lldb/include/lldb/Interpreter/CommandReturnObject.h
@@ -63,20 +63,28 @@ class CommandReturnObject {
   }
 
   void SetImmediateOutputFile(lldb::FileSP file_sp) {
+if (m_suppress_immediate_output)
+  return;
 lldb::StreamSP stream_sp(new StreamFile(file_sp));
 m_out_stream.SetStreamAtIndex(eImmediateStreamIndex, stream_sp);
   }
 
   void SetImmediateErrorFile(lldb::FileSP file_sp) {
+if (m_suppress_immediate_output)
+  return;
 lldb::StreamSP stream_sp(new StreamFile(file_sp));
 m_err_stream.SetStreamAtIndex(eImmediateStreamIndex, stream_sp);
   }
 
   void SetImmediateOutputStream(const lldb::StreamSP _sp) {
+if (m_suppress_immediate_output)
+  return;
 m_out_stream.SetStreamAtIndex(eImmediateStreamIndex, stream_sp);
   }
 
   void SetImmediateErrorStream(const lldb::StreamSP _sp) {
+if (m_suppress_immediate_output)
+  return;
 m_err_stream.SetStreamAtIndex(eImmediateStreamIndex, stream_sp);
   }
 
@@ -142,16 +150,23 @@ class CommandReturnObject {
 
   void SetInteractive(bool b);
 
+  bool GetSuppressImmediateOutput() const;
+
+  void SetSuppressImmediateOutput(bool b);
+
 private:
   enum { eStreamStringIndex = 0, eImmediateStreamIndex = 1 };
 
   StreamTee m_out_stream;
   StreamTee m_err_stream;
 
-  lldb::ReturnStatus m_status;
-  bool m_did_change_process_state;
-  bool m_interactive; // If true, then the input handle from the debugger will
-  // be hooked up
+  lldb::ReturnStatus m_status = lldb::eReturnStatusStarted;
+
+  bool m_did_change_process_state = false;
+  bool m_suppress_immediate_output = false;
+
+  /// If true, then the input handle from the debugger will be hooked up.
+  bool m_interactive = true;
 };
 
 } // namespace lldb_private

diff  --git a/lldb/lldb/test/Shell/Breakpoint/breakpoint-command.test 
b/lldb/lldb/test/Shell/Breakpoint/breakpoint-command.test
new file mode 100644
index 0..6104713cde5ae
--- /dev/null
+++ b/lldb/lldb/test/Shell/Breakpoint/breakpoint-command.test
@@ -0,0 +1,5 @@
+# RUN: %build %p/Inputs/dummy-target.c -o %t.out
+# RUN: %lldb %t.out -o 'b main' -o 'break command add 1 -o "script print(95000 
+ 126)"' -o 'r'
+
+# CHECK: 95125
+# CHECK-NOT: 95126

diff  --git a/lldb/source/Interpreter/CommandInterpreter.cpp 
b/lldb/source/Interpreter/CommandInterpreter.cpp
index 75c26c42e18d1..85025b92d367f 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -2306,6 +2306,7 @@ void CommandInterpreter::HandleCommands(const StringList 
,
 
 CommandReturnObject tmp_result(m_debugger.GetUseColor());
 tmp_result.SetInteractive(result.GetInteractive());
+tmp_result.SetSuppressImmediateOutput(true);
 
 // We might call into a regex or alias command, in which case the
 // add_to_history will get lost.  This m_command_source_depth dingus is the

diff  --git a/lldb/source/Interpreter/CommandReturnObject.cpp 
b/lldb/source/Interpreter/CommandReturnObject.cpp
index 

[Lldb-commits] [PATCH] D103652: [lldb][NFC] Refactor name to index maps in Symtab

[Alex Langford 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 rG64576a1be887: [lldb][NFC] Refactor name to index maps in 
Symtab (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103652

Files:
  lldb/include/lldb/Symbol/Symtab.h
  lldb/source/Symbol/Symtab.cpp

Index: lldb/source/Symbol/Symtab.cpp
===
--- lldb/source/Symbol/Symtab.cpp
+++ lldb/source/Symbol/Symtab.cpp
@@ -29,8 +29,17 @@
 
 Symtab::Symtab(ObjectFile *objfile)
 : m_objfile(objfile), m_symbols(), m_file_addr_to_index(*this),
-  m_name_to_index(), m_mutex(), m_file_addr_to_index_computed(false),
-  m_name_indexes_computed(false) {}
+  m_name_to_symbol_indices(), m_mutex(),
+  m_file_addr_to_index_computed(false), m_name_indexes_computed(false) {
+  m_name_to_symbol_indices.emplace(std::make_pair(
+  lldb::eFunctionNameTypeNone, UniqueCStringMap()));
+  m_name_to_symbol_indices.emplace(std::make_pair(
+  lldb::eFunctionNameTypeBase, UniqueCStringMap()));
+  m_name_to_symbol_indices.emplace(std::make_pair(
+  lldb::eFunctionNameTypeMethod, UniqueCStringMap()));
+  m_name_to_symbol_indices.emplace(std::make_pair(
+  lldb::eFunctionNameTypeSelector, UniqueCStringMap()));
+}
 
 Symtab::~Symtab() {}
 
@@ -51,7 +60,8 @@
   // Clients should grab the mutex from this symbol table and lock it manually
   // when calling this function to avoid performance issues.
   uint32_t symbol_idx = m_symbols.size();
-  m_name_to_index.Clear();
+  auto _to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
+  name_to_index.Clear();
   m_file_addr_to_index.Clear();
   m_symbols.push_back(symbol);
   m_file_addr_to_index_computed = false;
@@ -65,7 +75,8 @@
 }
 
 void Symtab::SectionFileAddressesChanged() {
-  m_name_to_index.Clear();
+  auto _to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
+  name_to_index.Clear();
   m_file_addr_to_index_computed = false;
 }
 
@@ -252,9 +263,17 @@
   if (!m_name_indexes_computed) {
 m_name_indexes_computed = true;
 LLDB_SCOPED_TIMER();
+
+auto _to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
+auto _to_index =
+GetNameToSymbolIndexMap(lldb::eFunctionNameTypeBase);
+auto _to_index =
+GetNameToSymbolIndexMap(lldb::eFunctionNameTypeMethod);
+auto _to_index =
+GetNameToSymbolIndexMap(lldb::eFunctionNameTypeSelector);
 // Create the name index vector to be able to quickly search by name
 const size_t num_symbols = m_symbols.size();
-m_name_to_index.Reserve(num_symbols);
+name_to_index.Reserve(num_symbols);
 
 // The "const char *" in "class_contexts" and backlog::value_type::second
 // must come from a ConstString::GetCString()
@@ -279,14 +298,14 @@
   // stored in the mangled field.
   Mangled  = symbol->GetMangled();
   if (ConstString name = mangled.GetMangledName()) {
-m_name_to_index.Append(name, value);
+name_to_index.Append(name, value);
 
 if (symbol->ContainsLinkerAnnotations()) {
   // If the symbol has linker annotations, also add the version without
   // the annotations.
   ConstString stripped = ConstString(
   m_objfile->StripLinkerSymbolAnnotations(name.GetStringRef()));
-  m_name_to_index.Append(stripped, value);
+  name_to_index.Append(stripped, value);
 }
 
 const SymbolType type = symbol->GetType();
@@ -299,25 +318,25 @@
   // Symbol name strings that didn't match a Mangled::ManglingScheme, are
   // stored in the demangled field.
   if (ConstString name = mangled.GetDemangledName()) {
-m_name_to_index.Append(name, value);
+name_to_index.Append(name, value);
 
 if (symbol->ContainsLinkerAnnotations()) {
   // If the symbol has linker annotations, also add the version without
   // the annotations.
   name = ConstString(
   m_objfile->StripLinkerSymbolAnnotations(name.GetStringRef()));
-  m_name_to_index.Append(name, value);
+  name_to_index.Append(name, value);
 }
 
 // If the demangled name turns out to be an ObjC name, and is a category
 // name, add the version without categories to the index too.
 ObjCLanguage::MethodName objc_method(name.GetStringRef(), true);
 if (objc_method.IsValid(true)) {
-  m_selector_to_index.Append(objc_method.GetSelector(), value);
+  selector_to_index.Append(objc_method.GetSelector(), value);
 
   if (ConstString objc_method_no_category =
   objc_method.GetFullNameWithoutCategory(true))
-m_name_to_index.Append(objc_method_no_category, value);
+name_to_index.Append(objc_method_no_category, 

[Lldb-commits] [lldb] 64576a1 - [lldb][NFC] Refactor name to index maps in Symtab

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2021-06-08T12:36:54-07:00
New Revision: 64576a1be887b7afcacf0534e6c168805fba5677

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

LOG: [lldb][NFC] Refactor name to index maps in Symtab

The various maps in Symtab lead to some repetative code. This should
improve the situation somewhat.

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

Added: 


Modified: 
lldb/include/lldb/Symbol/Symtab.h
lldb/source/Symbol/Symtab.cpp

Removed: 




diff  --git a/lldb/include/lldb/Symbol/Symtab.h 
b/lldb/include/lldb/Symbol/Symtab.h
index c232925eec75..fbfa3a5e0cec 100644
--- a/lldb/include/lldb/Symbol/Symtab.h
+++ b/lldb/include/lldb/Symbol/Symtab.h
@@ -13,6 +13,7 @@
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Utility/RangeMap.h"
 #include "lldb/lldb-private.h"
+#include 
 #include 
 #include 
 
@@ -173,15 +174,21 @@ class Symtab {
   ObjectFile *m_objfile;
   collection m_symbols;
   FileRangeToIndexMap m_file_addr_to_index;
-  UniqueCStringMap m_name_to_index;
-  UniqueCStringMap m_basename_to_index;
-  UniqueCStringMap m_method_to_index;
-  UniqueCStringMap m_selector_to_index;
+
+  /// Maps function names to symbol indices (grouped by FunctionNameTypes)
+  std::map>
+  m_name_to_symbol_indices;
   mutable std::recursive_mutex
   m_mutex; // Provide thread safety for this symbol table
   bool m_file_addr_to_index_computed : 1, m_name_indexes_computed : 1;
 
 private:
+  UniqueCStringMap &
+  GetNameToSymbolIndexMap(lldb::FunctionNameType type) {
+auto map = m_name_to_symbol_indices.find(type);
+assert(map != m_name_to_symbol_indices.end());
+return map->second;
+  }
   bool CheckSymbolAtIndex(size_t idx, Debug symbol_debug_type,
   Visibility symbol_visibility) const {
 switch (symbol_debug_type) {

diff  --git a/lldb/source/Symbol/Symtab.cpp b/lldb/source/Symbol/Symtab.cpp
index 7f8424334708..0deefa5dd5c0 100644
--- a/lldb/source/Symbol/Symtab.cpp
+++ b/lldb/source/Symbol/Symtab.cpp
@@ -29,8 +29,17 @@ using namespace lldb_private;
 
 Symtab::Symtab(ObjectFile *objfile)
 : m_objfile(objfile), m_symbols(), m_file_addr_to_index(*this),
-  m_name_to_index(), m_mutex(), m_file_addr_to_index_computed(false),
-  m_name_indexes_computed(false) {}
+  m_name_to_symbol_indices(), m_mutex(),
+  m_file_addr_to_index_computed(false), m_name_indexes_computed(false) {
+  m_name_to_symbol_indices.emplace(std::make_pair(
+  lldb::eFunctionNameTypeNone, UniqueCStringMap()));
+  m_name_to_symbol_indices.emplace(std::make_pair(
+  lldb::eFunctionNameTypeBase, UniqueCStringMap()));
+  m_name_to_symbol_indices.emplace(std::make_pair(
+  lldb::eFunctionNameTypeMethod, UniqueCStringMap()));
+  m_name_to_symbol_indices.emplace(std::make_pair(
+  lldb::eFunctionNameTypeSelector, UniqueCStringMap()));
+}
 
 Symtab::~Symtab() {}
 
@@ -51,7 +60,8 @@ uint32_t Symtab::AddSymbol(const Symbol ) {
   // Clients should grab the mutex from this symbol table and lock it manually
   // when calling this function to avoid performance issues.
   uint32_t symbol_idx = m_symbols.size();
-  m_name_to_index.Clear();
+  auto _to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
+  name_to_index.Clear();
   m_file_addr_to_index.Clear();
   m_symbols.push_back(symbol);
   m_file_addr_to_index_computed = false;
@@ -65,7 +75,8 @@ size_t Symtab::GetNumSymbols() const {
 }
 
 void Symtab::SectionFileAddressesChanged() {
-  m_name_to_index.Clear();
+  auto _to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
+  name_to_index.Clear();
   m_file_addr_to_index_computed = false;
 }
 
@@ -252,9 +263,17 @@ void Symtab::InitNameIndexes() {
   if (!m_name_indexes_computed) {
 m_name_indexes_computed = true;
 LLDB_SCOPED_TIMER();
+
+auto _to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
+auto _to_index =
+GetNameToSymbolIndexMap(lldb::eFunctionNameTypeBase);
+auto _to_index =
+GetNameToSymbolIndexMap(lldb::eFunctionNameTypeMethod);
+auto _to_index =
+GetNameToSymbolIndexMap(lldb::eFunctionNameTypeSelector);
 // Create the name index vector to be able to quickly search by name
 const size_t num_symbols = m_symbols.size();
-m_name_to_index.Reserve(num_symbols);
+name_to_index.Reserve(num_symbols);
 
 // The "const char *" in "class_contexts" and backlog::value_type::second
 // must come from a ConstString::GetCString()
@@ -279,14 +298,14 @@ void Symtab::InitNameIndexes() {
   // stored in the mangled field.
   Mangled  = symbol->GetMangled();
   if (ConstString name = mangled.GetMangledName()) {
-m_name_to_index.Append(name, value);
+name_to_index.Append(name, value);
 
 if 

[Lldb-commits] [PATCH] D103652: [lldb][NFC] Refactor name to index maps in Symtab

[Alex Langford via Phabricator via lldb-commits]

bulbazord updated this revision to Diff 350685.
bulbazord added a comment.

Rename function :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103652

Files:
  lldb/include/lldb/Symbol/Symtab.h
  lldb/source/Symbol/Symtab.cpp

Index: lldb/source/Symbol/Symtab.cpp
===
--- lldb/source/Symbol/Symtab.cpp
+++ lldb/source/Symbol/Symtab.cpp
@@ -29,8 +29,17 @@
 
 Symtab::Symtab(ObjectFile *objfile)
 : m_objfile(objfile), m_symbols(), m_file_addr_to_index(*this),
-  m_name_to_index(), m_mutex(), m_file_addr_to_index_computed(false),
-  m_name_indexes_computed(false) {}
+  m_name_to_symbol_indices(), m_mutex(),
+  m_file_addr_to_index_computed(false), m_name_indexes_computed(false) {
+  m_name_to_symbol_indices.emplace(std::make_pair(
+  lldb::eFunctionNameTypeNone, UniqueCStringMap()));
+  m_name_to_symbol_indices.emplace(std::make_pair(
+  lldb::eFunctionNameTypeBase, UniqueCStringMap()));
+  m_name_to_symbol_indices.emplace(std::make_pair(
+  lldb::eFunctionNameTypeMethod, UniqueCStringMap()));
+  m_name_to_symbol_indices.emplace(std::make_pair(
+  lldb::eFunctionNameTypeSelector, UniqueCStringMap()));
+}
 
 Symtab::~Symtab() {}
 
@@ -51,7 +60,8 @@
   // Clients should grab the mutex from this symbol table and lock it manually
   // when calling this function to avoid performance issues.
   uint32_t symbol_idx = m_symbols.size();
-  m_name_to_index.Clear();
+  auto _to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
+  name_to_index.Clear();
   m_file_addr_to_index.Clear();
   m_symbols.push_back(symbol);
   m_file_addr_to_index_computed = false;
@@ -65,7 +75,8 @@
 }
 
 void Symtab::SectionFileAddressesChanged() {
-  m_name_to_index.Clear();
+  auto _to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
+  name_to_index.Clear();
   m_file_addr_to_index_computed = false;
 }
 
@@ -252,9 +263,17 @@
   if (!m_name_indexes_computed) {
 m_name_indexes_computed = true;
 LLDB_SCOPED_TIMER();
+
+auto _to_index = GetNameToSymbolIndexMap(lldb::eFunctionNameTypeNone);
+auto _to_index =
+GetNameToSymbolIndexMap(lldb::eFunctionNameTypeBase);
+auto _to_index =
+GetNameToSymbolIndexMap(lldb::eFunctionNameTypeMethod);
+auto _to_index =
+GetNameToSymbolIndexMap(lldb::eFunctionNameTypeSelector);
 // Create the name index vector to be able to quickly search by name
 const size_t num_symbols = m_symbols.size();
-m_name_to_index.Reserve(num_symbols);
+name_to_index.Reserve(num_symbols);
 
 // The "const char *" in "class_contexts" and backlog::value_type::second
 // must come from a ConstString::GetCString()
@@ -279,14 +298,14 @@
   // stored in the mangled field.
   Mangled  = symbol->GetMangled();
   if (ConstString name = mangled.GetMangledName()) {
-m_name_to_index.Append(name, value);
+name_to_index.Append(name, value);
 
 if (symbol->ContainsLinkerAnnotations()) {
   // If the symbol has linker annotations, also add the version without
   // the annotations.
   ConstString stripped = ConstString(
   m_objfile->StripLinkerSymbolAnnotations(name.GetStringRef()));
-  m_name_to_index.Append(stripped, value);
+  name_to_index.Append(stripped, value);
 }
 
 const SymbolType type = symbol->GetType();
@@ -299,25 +318,25 @@
   // Symbol name strings that didn't match a Mangled::ManglingScheme, are
   // stored in the demangled field.
   if (ConstString name = mangled.GetDemangledName()) {
-m_name_to_index.Append(name, value);
+name_to_index.Append(name, value);
 
 if (symbol->ContainsLinkerAnnotations()) {
   // If the symbol has linker annotations, also add the version without
   // the annotations.
   name = ConstString(
   m_objfile->StripLinkerSymbolAnnotations(name.GetStringRef()));
-  m_name_to_index.Append(name, value);
+  name_to_index.Append(name, value);
 }
 
 // If the demangled name turns out to be an ObjC name, and is a category
 // name, add the version without categories to the index too.
 ObjCLanguage::MethodName objc_method(name.GetStringRef(), true);
 if (objc_method.IsValid(true)) {
-  m_selector_to_index.Append(objc_method.GetSelector(), value);
+  selector_to_index.Append(objc_method.GetSelector(), value);
 
   if (ConstString objc_method_no_category =
   objc_method.GetFullNameWithoutCategory(true))
-m_name_to_index.Append(objc_method_no_category, value);
+name_to_index.Append(objc_method_no_category, value);
 }
   }
 }
@@ -326,14 +345,14 @@
   RegisterBacklogEntry(record.first, record.second, class_contexts);
 }
 
-

[Lldb-commits] [PATCH] D103500: [trace][intel-pt] Create basic SB API

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

Took a look at the SBAPI changes and left few comments but overall, that part 
looked good to me. It would be nice if you could delete the depreciated 
method/class.




Comment at: lldb/bindings/interface/SBTrace.i:32-51
+  /// deprecated
   void StopTrace(SBError ,
  lldb::tid_t thread_id);
 
+  /// deprecated
   void GetTraceConfig(SBTraceOptions ,
   SBError );

JDevlieghere wrote:
> Do you have clients that already rely on these functions? Writing deprecated 
> above them means nothing. When I was doing the reproducers, I included a 
> warning in the header saying that this was under development and the API 
> wasn't final, which allowed me to iterate on it. Can we just remove these 
> methods and to the same here? 
+1



Comment at: lldb/bindings/interface/SBTraceOptions.i:12
 %feature("docstring",
-"Represents the possible options when doing processor tracing.
-
-See :py:class:`SBProcess.StartTrace`."
+"deprecated"
 ) SBTraceOptions;

Same comment as @JDevlieghere here



Comment at: lldb/include/lldb/API/SBTrace.h:100
 
-protected:
-  typedef std::shared_ptr TraceImplSP;
-
-  friend class SBProcess;
+  /// Deprecated
+  /// \{

Same here.



Comment at: lldb/source/API/SBTrace.cpp:92
   LLDB_RECORD_METHOD_NO_ARGS(bool, SBTrace, IsValid);
-  return this->operator bool();
+  return LLDB_RECORD_RESULT(this->operator bool());
 }

No need to use `LLDB_RECORD_RESULT` on primitive types such as `bool`.



Comment at: lldb/source/API/SBTrace.cpp:97
   LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBTrace, operator bool);
+  return LLDB_RECORD_RESULT((bool)m_opaque_sp);
+}

Same here.



Comment at: lldb/source/API/SBTrace.cpp:103
+ size_t offset, lldb::tid_t thread_id) {
+  LLDB_RECORD_DUMMY(size_t, SBTrace, GetTraceData,
+(lldb::SBError &, void *, size_t, size_t, lldb::tid_t),

Either use `LLDB_RECORD_DUMMY` on all the deprecated method or don't use it at 
all :) If you could get rid of the deprecated methods that would be even better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103500

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

[Lldb-commits] [PATCH] D103271: [lldb/Target] Select most relevant frame only in case of signal

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I don't think it's right to do it in GetStackFrameAtIndex.  For instance, 
suppose lldb stops somewhere, and the user does a backtrace, then they select a 
different frame on the stack, and then invoke some other command that happens 
to ask for the frame at index 0.  If that subsequent GetStackFrameAtIndex(0) 
request triggers the recognizer to select a different frame, that would be 
surprising and not desirable.

I think you need to do this somewhere in the StackFrameList, somewhere where we 
start filling in frames (maybe GetFramesUpTo) but you are going to have to be 
careful here to avoid recursion as well.


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

https://reviews.llvm.org/D103271

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

[Lldb-commits] [PATCH] D103271: [lldb/Target] Select most relevant frame only in case of signal

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

"Unwinding" the zero-th frame is really just getting the PC & FP.  Since you 
are very likely to need this information on any given stop, lldb has 
accelerated versions of the gdb-remote stop reply packets that provide this 
information up front so this sort of request is cheap to fulfill.  I know that 
debugserver & lldbserver both provide these accelerated stop-reply packet.  If 
you are using either of these debug stubs, this shouldn't be a performance 
problem.  Are you using a different stub, and if so and you can switch to using 
the lldb-server stub - or fixing your stub to provide the accelerated stop 
reply packets, your lldb sessions will be a lot more efficient...


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

https://reviews.llvm.org/D103271

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

[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

[Vedant Kumar via Phabricator via lldb-commits]

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

I'm quite concerned about the design decision to represent a trace as a vector 
of TraceInstruction. This bakes considerations that appear to be specific to 
Facebook's usage of IntelPT way too deep into lldb's APIs, and isn't workable 
for our use cases.

At Apple, we use lldb to navigate instruction traces that contain billions of 
instructions. Allocating 16 bytes per instruction simply will not scale for our 
workflows. We require the in-memory overhead to be approximately 1-2 bits per 
instruction. I'm not familiar with how large IntelPT traces can get, but 
presumably (for long enough traces) you will encounter the same scaling problem.

What alternatives to the vector representation have you 
considered? One idea might be to implement your program analyses on top of a 
generic interface for navigating forward/backward through a trace and 
extracting info about the instruction via a set of API calls; this leaves the 
actual in-memory representation of "TraceInstruction" unspecified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103588

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

[Lldb-commits] [PATCH] D103500: [trace][intel-pt] Create basic SB API

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/bindings/interface/SBTrace.i:32-51
+  /// deprecated
   void StopTrace(SBError ,
  lldb::tid_t thread_id);
 
+  /// deprecated
   void GetTraceConfig(SBTraceOptions ,
   SBError );

Do you have clients that already rely on these functions? Writing deprecated 
above them means nothing. When I was doing the reproducers, I included a 
warning in the header saying that this was under development and the API wasn't 
final, which allowed me to iterate on it. Can we just remove these methods and 
to the same here? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103500

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

[Lldb-commits] [PATCH] D103500: [trace][intel-pt] Create basic SB API

[Vedant Kumar via Phabricator via lldb-commits]

vsk requested changes to this revision.
vsk added a subscriber: mib.
vsk added a comment.
This revision now requires changes to proceed.

Thanks, excited to see this work progress!




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

See my other note about returning references to shared pointers. This would be 
nice to clean up either before or after landing this patch.



Comment at: lldb/include/lldb/Target/Target.h:1132
+  /// Create a \a Trace object for the current target using the using the
+  /// default supported tracing technology for this process. This
+  ///

Dangling 'This'?



Comment at: lldb/include/lldb/Target/Target.h:1137
+  /// the trace couldn't be created.
+  llvm::Expected CreateTrace();
+

I don't think returning a `TraceSP &` is a good practice. This returns a 
reference to a SP: unless the SP is stored in a long-lived instance (which 
would defeat the purpose of using shared pointers in the first place), the 
returned reference will be dangling.

It might make more sense to either return the shared pointer (lldb::TraceSP) 
directly, or to return a bare pointer/reference.



Comment at: lldb/include/lldb/Target/Trace.h:267
   /// \a llvm::Error otherwise.
-  llvm::Error StopThreads(const std::vector );
+  llvm::Error Stop(const std::vector );
 

Consider using ArrayRef, as this permits the caller to use a 
larger variety of vector-like containers.



Comment at: 
lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py:11
+def wrapper(*args, **kwargs):
+USE_SB_API = True
+func(*args, **kwargs)

I don't think this works. In this wrapper, "USE_SB_API" is a local variable, 
*not* a reference to a TraceIntelPTTestCaseBase instance's "USE_SB_API" field.

To see why, consider the following example -- it prints "False" twice:
```
def make_wrapper(func):
def wrapper(*args):
FLAG = True
func(*args)
FLAG = False
func(*args)
return wrapper

class Klass:
FLAG = False

@make_wrapper
def test(self):
print(self.FLAG)

Klass().test()
```

How did you verify that the USE_SB_API = True case behaves as expected?



Comment at: lldb/source/API/SBTarget.cpp:2458
 
+lldb::SBTrace SBTarget::GetTrace() {
+  LLDB_RECORD_METHOD_NO_ARGS(lldb::SBTrace, SBTarget, GetTrace);

Paging @mib - would you mind taking a closer look at these API/*.cpp changes? 
You've got more experience than I do in this area :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103500

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

[Lldb-commits] [PATCH] D103588: [trace] Create a top-level instruction class

[walter erquinigo via Phabricator via lldb-commits]

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

I tried to use a ConstString instead of a const char *, but that's not trivial 
because
the compilers wants a safe destructor all the way down, and it seems that this 
is non-standard
across compilers, except for C++17 and onwards, which has the std::variant type.

So I'm just adding a comment as suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103588

Files:
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/lldb-enumerations.h
  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/source/Target/Trace.cpp

Index: lldb/source/Target/Trace.cpp
===
--- lldb/source/Target/Trace.cpp
+++ lldb/source/Target/Trace.cpp
@@ -52,6 +52,23 @@
 } // namespace json
 } // namespace llvm
 
+TraceInstruction::TraceInstruction() : m_is_error(true) {
+  m_data.m_error = "invalid instruction";
+}
+
+TraceInstruction::TraceInstruction(lldb::addr_t load_address, uint8_t byte_size,
+   lldb::TraceInstructionType instruction_type)
+: m_is_error(false), m_byte_size(byte_size),
+  m_instruction_type(instruction_type) {
+  m_data.m_load_address = load_address;
+}
+
+TraceInstruction::TraceInstruction(Error error) : m_is_error(true) {
+  // We are assuming that errors will be rare, so using the ConstrString pool
+  // helps us make this class smaller.
+  m_data.m_error = ConstString(toString(std::move(error))).GetCString();
+}
+
 static Error createInvalidPlugInError(StringRef plugin_name) {
   return createStringError(
   std::errc::invalid_argument,
@@ -124,9 +141,9 @@
 struct InstructionSymbolInfo {
   SymbolContext sc;
   Address address;
-  lldb::addr_t load_address;
+  TraceInstruction instruction;
   lldb::DisassemblerSP disassembler;
-  lldb::InstructionSP instruction;
+  lldb::InstructionSP instruction_sp;
   lldb_private::ExecutionContext exe_ctx;
 };
 
@@ -144,7 +161,7 @@
 Trace , Thread , size_t position,
 Trace::TraceDirection direction, SymbolContextItem symbol_scope,
 bool include_disassembler,
-std::function insn)>
+std::function
 callback) {
   InstructionSymbolInfo prev_insn;
 
@@ -199,18 +216,18 @@
 
   trace.TraverseInstructions(
   thread, position, direction,
-  [&](size_t index, Expected load_address) -> bool {
-if (!load_address)
-  return callback(index, load_address.takeError());
-
+  [&](size_t index, const TraceInstruction ) -> bool {
 InstructionSymbolInfo insn;
-insn.load_address = *load_address;
+insn.instruction = instruction;
+if (instruction.IsError())
+  return callback(index, insn);
+
 insn.exe_ctx = exe_ctx;
-insn.address.SetLoadAddress(*load_address, );
+insn.address.SetLoadAddress(instruction.GetLoadAddress(), );
 if (symbol_scope != 0)
   insn.sc = calculate_symbol_context(insn.address);
 if (include_disassembler)
-  std::tie(insn.disassembler, insn.instruction) =
+  std::tie(insn.disassembler, insn.instruction_sp) =
   calculate_disass(insn.address, insn.sc);
 prev_insn = insn;
 return callback(index, insn);
@@ -268,7 +285,7 @@
 static void
 DumpInstructionSymbolContext(Stream ,
  Optional prev_insn,
- InstructionSymbolInfo ) {
+ const InstructionSymbolInfo ) {
   if (prev_insn && IsSameInstructionSymbolContext(*prev_insn, insn))
 return;
 
@@ -288,15 +305,16 @@
   s.Printf("\n");
 }
 
-static void DumpInstructionDisassembly(Stream , InstructionSymbolInfo ) {
-  if (!insn.instruction)
+static void DumpInstructionDisassembly(Stream ,
+   const InstructionSymbolInfo ) {
+  if (insn.instruction.IsError())
 return;
   s.Printf("");
-  insn.instruction->Dump(, /*show_address*/ false, /*show_bytes*/ false,
- /*max_opcode_byte_size*/ 0, _ctx, ,
- /*prev_sym_ctx*/ nullptr,
- /*disassembly_addr_format*/ nullptr,
- /*max_address_text_size*/ 0);
+  insn.instruction_sp->Dump(, /*show_address*/ false, /*show_bytes*/ false,
+/*max_opcode_byte_size*/ 0, _ctx, ,
+/*prev_sym_ctx*/ nullptr,
+/*disassembly_addr_format*/ nullptr,
+/*max_address_text_size*/ 0);
 }
 
 void Trace::DumpTraceInstructions(Thread , Stream , size_t count,
@@ -327,10 +345,10 @@
   TraverseInstructionsWithSymbolInfo(
   

[Lldb-commits] [PATCH] D103500: [trace][intel-pt] Create basic SB API

[walter erquinigo via Phabricator via lldb-commits]

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

improved the documentation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103500

Files:
  lldb/bindings/interface/SBStructuredData.i
  lldb/bindings/interface/SBTarget.i
  lldb/bindings/interface/SBTrace.i
  lldb/bindings/interface/SBTraceOptions.i
  lldb/docs/design/overview.rst
  lldb/docs/lldb-gdb-remote.txt
  lldb/include/lldb/API/SBStructuredData.h
  lldb/include/lldb/API/SBTarget.h
  lldb/include/lldb/API/SBThread.h
  lldb/include/lldb/API/SBTrace.h
  lldb/include/lldb/API/SBTraceOptions.h
  lldb/include/lldb/Target/Process.h
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Utility/TraceGDBRemotePackets.h
  lldb/include/lldb/Utility/TraceOptions.h
  lldb/include/lldb/lldb-forward.h
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
  lldb/source/API/SBProcess.cpp
  lldb/source/API/SBReproducer.cpp
  lldb/source/API/SBStructuredData.cpp
  lldb/source/API/SBTarget.cpp
  lldb/source/API/SBTrace.cpp
  lldb/source/API/SBTraceOptions.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Plugins/Process/Linux/IntelPTManager.cpp
  lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTConstants.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTOptions.td
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/Trace.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceLoad.py
  lldb/test/API/commands/trace/TestTraceSchema.py
  lldb/test/API/commands/trace/TestTraceStartStop.py
  
lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
  lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp

Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
===
--- lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
@@ -12,7 +12,6 @@
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/StructuredData.h"
-#include "lldb/Utility/TraceOptions.h"
 #include "lldb/lldb-enumerations.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Testing/Support/Error.h"
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
@@ -1,53 +1,49 @@
 import lldb
+from intelpt_testcase import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 from lldbsuite.test.decorators import *
 
-ADDRESS_REGEX = '0x[0-9a-fA-F]*'
-
-class TestTraceStartStopMultipleThreads(TestBase):
+class TestTraceStartStopMultipleThreads(TraceIntelPTTestCaseBase):
 
 mydir = TestBase.compute_mydir(__file__)
-NO_DEBUG_INFO_TESTCASE = True
-
-def setUp(self):
-TestBase.setUp(self)
-if 'intel-pt' not in configuration.enabled_plugins:
-self.skipTest("The intel-pt test plugin is not enabled")
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
+@testSBAPIAndCommands
 def testStartMultipleLiveThreads(self):
 self.build()
-target = self.createTestTarget()
+exe = self.getBuildArtifact("a.out")
+
+self.dbg.CreateTarget(exe)
 
 self.expect("b main")
 self.expect("b 6")
 self.expect("b 11")
 
 self.expect("r")
-self.expect("proce trace start")
-
+self.traceStartProcess()
 
-# We'll see here the first thread
 self.expect("continue")
 self.expect("thread trace dump instructions", substrs=['main.cpp:9'])
-
+
 # We'll see here the second thread
 self.expect("continue")
 self.expect("thread trace dump instructions", substrs=['main.cpp:4'])
 
 @skipIf(oslist=no_match(['linux']), archs=no_match(['i386', 'x86_64']))
+@testSBAPIAndCommands
 def testStartMultipleLiveThreadsWithStops(self):
 self.build()
-target = self.createTestTarget()
+exe = self.getBuildArtifact("a.out")
+
+self.dbg.CreateTarget(exe)
 
 self.expect("b main")
 self.expect("b 6")
 self.expect("b 11")
 
 self.expect("r")
-self.expect("process trace start")
-
+

[Lldb-commits] [PATCH] D103500: [trace][intel-pt] Create basic SB API

[walter erquinigo via Phabricator via lldb-commits]

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



Comment at: lldb/include/lldb/Target/Target.h:1137-1141
+  llvm::Expected CreateTrace();
+
+  /// If a \a Trace object is present, this returns it, otherwise a new Trace 
is
+  /// created with \a Trace::CreateTrace.
   llvm::Expected GetTraceOrCreate();

clayborg wrote:
> Do we need both of these?
GetTraceOrCreate is quite handy for the CommandObjects. And CreateTrace seems 
more correct from the SB API side, so I'd like to keep them there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103500

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

[Lldb-commits] [lldb] c5d56fe - NFC: .clang-tidy: Inherit configs from parents to improve maintainability

[David Blaikie via lldb-commits]

Author: David Blaikie
Date: 2021-06-08T08:25:59-07:00
New Revision: c5d56fec502f36a0c994835ca23bc93a6c682d95

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

LOG: NFC: .clang-tidy: Inherit configs from parents to improve maintainability

In the interests of disabling misc-no-recursion across LLVM (this seems
like a stylistic choice that is not consistent with LLVM's
style/development approach) this NFC preliminary change adjusts all the
.clang-tidy files to inherit from their parents as much as possible.

This change specifically preserves all the quirks of the current configs
in order to make it easier to review as NFC.

I validatad the change is NFC as follows:

for X in `cat ../files.txt`;
do
  mkdir -p ../tmp/$(dirname $X)
  touch $(dirname $X)/blaikie.cpp
  clang-tidy -dump-config $(dirname $X)/blaikie.cpp > ../tmp/$(dirname $X)/after
  rm $(dirname $X)/blaikie.cpp
done

(similarly for the "before" state, without this patch applied)

for X in `cat ../files.txt`;
do
  echo $X
  diff \
../tmp/$(dirname $X)/before \
<(cat ../tmp/$(dirname $X)/after \
  | sed -e 
"s/,readability-identifier-naming\(.*\),-readability-identifier-naming/\1/" \
  | sed -e "s/,-llvm-include-order\(.*\),llvm-include-order/\1/" \
  | sed -e "s/,-misc-no-recursion\(.*\),misc-no-recursion/\1/" \
  | sed -e "s/,-clang-diagnostic-\*\(.*\),clang-diagnostic-\*/\1/")
done

(using sed to strip some add/remove pairs to reduce the diff and make it easier 
to read)

The resulting report is:
  .clang-tidy
  clang/.clang-tidy
  2c2
  < Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-readability-identifier-naming,-misc-no-recursion'
  ---
  > Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-misc-no-recursion'
  compiler-rt/.clang-tidy
  2c2
  < Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,clang-diagnostic-*,llvm-*,-llvm-header-guard,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes'
  ---
  > Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-llvm-header-guard'
  flang/.clang-tidy
  2c2
  < Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,llvm-*,-llvm-include-order,misc-*,-misc-no-recursion,-misc-unused-parameters,-misc-non-private-member-variables-in-classes'
  ---
  > Checks:  
'clang-diagnostic-*,clang-analyzer-*,-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-llvm-include-order,-misc-no-recursion'
  flang/include/flang/Lower/.clang-tidy
  flang/include/flang/Optimizer/.clang-tidy
  flang/lib/Lower/.clang-tidy
  flang/lib/Optimizer/.clang-tidy
  lld/.clang-tidy
  lldb/.clang-tidy
  llvm/tools/split-file/.clang-tidy
  mlir/.clang-tidy

The `clang/.clang-tidy` change is a no-op, disabling an option that was never 
enabled.
The compiler-rt and flang changes are no-op reorderings of the same flags.

(side note, the .clang-tidy file in parallel-libs is broken and crashes
clang-tidy because it uses "lowerCase" as the style instead of "lower_case" -
so I'll deal with that separately)

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

Added: 


Modified: 
clang/.clang-tidy
compiler-rt/.clang-tidy
flang/.clang-tidy
flang/include/flang/Lower/.clang-tidy
flang/include/flang/Optimizer/.clang-tidy
flang/lib/Lower/.clang-tidy
flang/lib/Optimizer/.clang-tidy
lld/.clang-tidy
lldb/.clang-tidy
llvm/.clang-tidy
llvm/tools/split-file/.clang-tidy
mlir/.clang-tidy

Removed: 




diff  --git a/clang/.clang-tidy b/clang/.clang-tidy
index f517e9246cf85..ed155786f36ba 100644
--- a/clang/.clang-tidy
+++ b/clang/.clang-tidy
@@ -1,24 +1,5 @@
-Checks: 
'-*,clang-diagnostic-*,llvm-*,misc-*,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-readability-identifier-naming,-misc-no-recursion'
 # Note that the readability-identifier-naming check is disabled, there are too
 # many violations in the codebase and they create too much noise in clang-tidy
 # results.
-# Naming settings are kept for documentation purposes and allowing to run the
-# check if the users would override this file, e.g. via a command-line arg.
-CheckOptions:
-  - key: readability-identifier-naming.ClassCase
-value:   CamelCase
-  - key: readability-identifier-naming.EnumCase
-value:   CamelCase
-  - key: readability-identifier-naming.FunctionCase
-value:   camelBack
-  - key: 

[Lldb-commits] [PATCH] D103271: [lldb/Target] Select most relevant frame only in case of signal

[Levon Ter-Grigoryan via Phabricator via lldb-commits]

PatriosTheGreat updated this revision to Diff 350596.
PatriosTheGreat added a comment.

Thanks for your suggestions.
I tried both of them, however the solution to patch assert recognizer doesn’t 
solve the original performance issue. Since the performance degradation doesn’t 
come from the recognizer. It appears since the LLDB needs to unwind at least 
one stack frame for each thread in order to determine does the LLDB even need 
to run recognizers (see this line: 
https://github.com/llvm/llvm-project/blob/main/lldb/source/Target/Thread.cpp#L587)

The solution to select the most relevant frame at the moment when the user 
actually needs a stack frame seems to fork fine. If I understand correctly it 
can be done at the Thread::GetStackFrameAtIndex method, since to provide stack 
trace view to the user all clients need to call it for all stack frames.

One problem I see in this solution is that recognizers should be aware to not 
call Thread::GetStackFrameAtIndex for the zeroth frame (which they actually got 
as a parameter), since otherwise it will bring to the infinity recursion. See 
changes to AssertFrameRecognizer.cpp

I’ve attached this solution to the current revision for further discussion.


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

https://reviews.llvm.org/D103271

Files:
  lldb/include/lldb/Target/Thread.h
  lldb/source/Target/AssertFrameRecognizer.cpp
  lldb/source/Target/Thread.cpp


Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -578,14 +578,8 @@
   return raw_stop_description;
 }
 
-void Thread::SelectMostRelevantFrame() {
+void Thread::SelectMostRelevantFrame(lldb::StackFrameSP _sp) {
   Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD);
-
-  auto frames_list_sp = GetStackFrameList();
-
-  // Only the top frame should be recognized.
-  auto frame_sp = frames_list_sp->GetFrameAtIndex(0);
-
   auto recognized_frame_sp = frame_sp->GetRecognizedFrame();
 
   if (!recognized_frame_sp) {
@@ -606,8 +600,6 @@
 void Thread::WillStop() {
   ThreadPlan *current_plan = GetCurrentPlan();
 
-  SelectMostRelevantFrame();
-
   // FIXME: I may decide to disallow threads with no plans.  In which
   // case this should go to an assert.
 
@@ -1405,6 +1397,15 @@
   exe_ctx.SetContext(shared_from_this());
 }
 
+lldb::StackFrameSP Thread::GetStackFrameAtIndex(uint32_t idx) {
+  auto frame = GetStackFrameList()->GetFrameAtIndex(idx);
+
+  if (frame && idx == 0)
+SelectMostRelevantFrame(frame);
+
+  return frame;
+}
+
 StackFrameListSP Thread::GetStackFrameList() {
   std::lock_guard guard(m_frame_mutex);
 
Index: lldb/source/Target/AssertFrameRecognizer.cpp
===
--- lldb/source/Target/AssertFrameRecognizer.cpp
+++ lldb/source/Target/AssertFrameRecognizer.cpp
@@ -118,8 +118,9 @@
 
   // Fetch most relevant frame
   for (uint32_t frame_index = 0; frame_index < frames_to_fetch; frame_index++) 
{
-prev_frame_sp = thread_sp->GetStackFrameAtIndex(frame_index);
-
+prev_frame_sp = frame_index == 0
+? frame_sp
+: thread_sp->GetStackFrameAtIndex(frame_index);
 if (!prev_frame_sp) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_UNWIND));
   LLDB_LOG(log, "Abort Recognizer: Hit unwinding bound ({1} frames)!",
Index: lldb/include/lldb/Target/Thread.h
===
--- lldb/include/lldb/Target/Thread.h
+++ lldb/include/lldb/Target/Thread.h
@@ -218,7 +218,7 @@
 
   virtual void RefreshStateAfterStop() = 0;
 
-  void SelectMostRelevantFrame();
+  void SelectMostRelevantFrame(lldb::StackFrameSP _sp);
 
   std::string GetStopDescription();
 
@@ -396,9 +396,7 @@
 return GetStackFrameList()->GetNumFrames();
   }
 
-  virtual lldb::StackFrameSP GetStackFrameAtIndex(uint32_t idx) {
-return GetStackFrameList()->GetFrameAtIndex(idx);
-  }
+  virtual lldb::StackFrameSP GetStackFrameAtIndex(uint32_t idx);
 
   virtual lldb::StackFrameSP
   GetFrameWithConcreteFrameIndex(uint32_t unwind_idx);


Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -578,14 +578,8 @@
   return raw_stop_description;
 }
 
-void Thread::SelectMostRelevantFrame() {
+void Thread::SelectMostRelevantFrame(lldb::StackFrameSP _sp) {
   Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD);
-
-  auto frames_list_sp = GetStackFrameList();
-
-  // Only the top frame should be recognized.
-  auto frame_sp = frames_list_sp->GetFrameAtIndex(0);
-
   auto recognized_frame_sp = frame_sp->GetRecognizedFrame();
 
   if (!recognized_frame_sp) {
@@ -606,8 +600,6 @@
 void Thread::WillStop() {
   ThreadPlan *current_plan = GetCurrentPlan();
 
-  

[Lldb-commits] [PATCH] D103701: [lldb] Set return status to failed when adding a command error

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D103701#2804840 , @DavidSpickett 
wrote:

> I've gone ahead and landed it, will revert on failures.
>
> Got patches to remove the reundant calls locally so I'll put those up for 
> review once this has had time to go through.
> (those changes will be fairly mechanical but it's worth someone scanning them 
> for silly mistakes)

SGTM. My macOS node is suffering from some unrelated failures so I couldn't let 
this run over all tests yet, but if the next GreenDragon iteration is green 
then this should be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103701

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

[Lldb-commits] [PATCH] D103701: [lldb] Set return status to failed when adding a command error

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

I've gone ahead and landed it, will revert on failures.

Got patches to remove the reundant calls locally so I'll put those up for 
review once this has had time to go through.
(those changes will be fairly mechanical but it's worth someone scanning them 
for silly mistakes)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103701

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

[Lldb-commits] [PATCH] D103701: [lldb] Set return status to failed when adding a command error

[David Spickett via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe05b03cf4f45: [lldb] Set return status to failed when adding 
a command error (authored by DavidSpickett).

Changed prior to commit:
  https://reviews.llvm.org/D103701?vs=349888=350529#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103701

Files:
  lldb/source/Interpreter/CommandReturnObject.cpp
  lldb/test/API/commands/register/register/register_command/TestRegisters.py
  lldb/test/Shell/Commands/command-backtrace-parser-1.test
  lldb/test/Shell/Commands/command-backtrace-parser-2.test
  lldb/test/Shell/Commands/command-backtrace.test

Index: lldb/test/Shell/Commands/command-backtrace-parser-2.test
===
--- lldb/test/Shell/Commands/command-backtrace-parser-2.test
+++ lldb/test/Shell/Commands/command-backtrace-parser-2.test
@@ -1,11 +1,5 @@
-# Check basic functionality of command bt.
 # RUN: %lldb -s %s 2>&1 | FileCheck %s
 
-# Make sure this is not rejected by the parser as invalid syntax.
-# Blank characters after the '1' are important, as we're testing the parser.
-bt 1  
-# CHECK: error: invalid target
-
 # Make sure this is not rejected by the parser as invalid syntax.
 # Blank characters after the 'all' are important, as we're testing the parser.
 bt all   
Index: lldb/test/Shell/Commands/command-backtrace-parser-1.test
===
--- /dev/null
+++ lldb/test/Shell/Commands/command-backtrace-parser-1.test
@@ -0,0 +1,6 @@
+# RUN: %lldb -s %s 2>&1 | FileCheck %s
+
+# Make sure this is not rejected by the parser as invalid syntax.
+# Blank characters after the '1' are important, as we're testing the parser.
+bt 1  
+# CHECK: error: invalid target
Index: lldb/test/API/commands/register/register/register_command/TestRegisters.py
===
--- lldb/test/API/commands/register/register/register_command/TestRegisters.py
+++ lldb/test/API/commands/register/register/register_command/TestRegisters.py
@@ -41,13 +41,18 @@
 self.expect("register read -a", MISSING_EXPECTED_REGISTERS,
 substrs=['registers were unavailable'], matching=False)
 
+all_registers = self.res.GetOutput()
+
 if self.getArchitecture() in ['amd64', 'i386', 'x86_64']:
 self.runCmd("register read xmm0")
-self.runCmd("register read ymm15")  # may be available
-self.runCmd("register read bnd0")  # may be available
+if "ymm15 = " in all_registers:
+  self.runCmd("register read ymm15")  # may be available
+if "bnd0 = " in all_registers:
+  self.runCmd("register read bnd0")  # may be available
 elif self.getArchitecture() in ['arm', 'armv7', 'armv7k', 'arm64', 'arm64e', 'arm64_32']:
 self.runCmd("register read s0")
-self.runCmd("register read q15")  # may be available
+if "q15 = " in all_registers:
+  self.runCmd("register read q15")  # may be available
 
 self.expect(
 "register read -s 4",
@@ -413,7 +418,8 @@
 self.write_and_read(currentFrame, "ymm7", new_value)
 self.expect("expr $ymm0", substrs=['vector_type'])
 else:
-self.runCmd("register read ymm0")
+self.expect("register read ymm0", substrs=["Invalid register name 'ymm0'"],
+error=True)
 
 if has_mpx:
 # Test write and read for bnd0.
@@ -428,7 +434,8 @@
 self.write_and_read(currentFrame, "bndstatus", new_value)
 self.expect("expr $bndstatus", substrs = ['vector_type'])
 else:
-self.runCmd("register read bnd0")
+self.expect("register read bnd0", substrs=["Invalid register name 'bnd0'"],
+error=True)
 
 def convenience_registers(self):
 """Test convenience registers."""
@@ -450,7 +457,7 @@
 # Now write rax with a unique bit pattern and test that eax indeed
 # represents the lower half of rax.
 self.runCmd("register write rax 0x1234567887654321")
-self.expect("register read rax 0x1234567887654321",
+self.expect("register read rax",
 substrs=['0x1234567887654321'])
 
 def convenience_registers_with_process_attach(self, test_16bit_regs):
Index: lldb/source/Interpreter/CommandReturnObject.cpp
===
--- lldb/source/Interpreter/CommandReturnObject.cpp
+++ lldb/source/Interpreter/CommandReturnObject.cpp
@@ -46,6 +46,8 @@
   

[Lldb-commits] [lldb] e05b03c - [lldb] Set return status to failed when adding a command error

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2021-06-08T09:41:07+01:00
New Revision: e05b03cf4f45ac5ee63c59a3464e7d484884645c

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

LOG: [lldb] Set return status to failed when adding a command error

There is a common pattern:
result.AppendError(...);
result.SetStatus(eReturnStatusFailed);

I found that some commands don't actually "fail" but only
print "error: ..." because the second line got missed.

This can cause you to miss a failed command when you're
using the Python interface during testing.
(and produce some confusing script results)

I did not find any place where you would want to add
an error without setting the return status, so just
set eReturnStatusFailed whenever you add an error to
a command result.

This change does not remove any of the now redundant
SetStatus. This should allow us to see if there are any
tests that have commands unexpectedly fail with this change.
(the test suite passes for me but I don't have access to all
the systems we cover so there could be some corner cases)

Some tests that failed on x86 and AArch64 have been modified
to work with the new behaviour.

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

Added: 
lldb/test/Shell/Commands/command-backtrace-parser-1.test
lldb/test/Shell/Commands/command-backtrace-parser-2.test

Modified: 
lldb/source/Interpreter/CommandReturnObject.cpp
lldb/test/API/commands/register/register/register_command/TestRegisters.py

Removed: 
lldb/test/Shell/Commands/command-backtrace.test



diff  --git a/lldb/source/Interpreter/CommandReturnObject.cpp 
b/lldb/source/Interpreter/CommandReturnObject.cpp
index 77d94bd9389c3..980d39bfb1681 100644
--- a/lldb/source/Interpreter/CommandReturnObject.cpp
+++ b/lldb/source/Interpreter/CommandReturnObject.cpp
@@ -46,6 +46,8 @@ CommandReturnObject::CommandReturnObject(bool colors)
   m_interactive(true) {}
 
 void CommandReturnObject::AppendErrorWithFormat(const char *format, ...) {
+  SetStatus(eReturnStatusFailed);
+
   if (!format)
 return;
   va_list args;
@@ -100,6 +102,7 @@ void CommandReturnObject::AppendWarning(llvm::StringRef 
in_string) {
 void CommandReturnObject::AppendError(llvm::StringRef in_string) {
   if (in_string.empty())
 return;
+  SetStatus(eReturnStatusFailed);
   error(GetErrorStream()) << in_string.rtrim() << '\n';
 }
 
@@ -116,7 +119,6 @@ void CommandReturnObject::SetError(llvm::StringRef 
error_str) {
 return;
 
   AppendError(error_str);
-  SetStatus(eReturnStatusFailed);
 }
 
 // Similar to AppendError, but do not prepend 'Status: ' to message, and don't
@@ -126,6 +128,7 @@ void CommandReturnObject::AppendRawError(llvm::StringRef 
in_string) {
   if (in_string.empty())
 return;
   GetErrorStream() << in_string;
+  SetStatus(eReturnStatusFailed);
 }
 
 void CommandReturnObject::SetStatus(ReturnStatus status) { m_status = status; }

diff  --git 
a/lldb/test/API/commands/register/register/register_command/TestRegisters.py 
b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
index 5ec46c175e621..7acf3a4098756 100644
--- a/lldb/test/API/commands/register/register/register_command/TestRegisters.py
+++ b/lldb/test/API/commands/register/register/register_command/TestRegisters.py
@@ -41,13 +41,18 @@ def test_register_commands(self):
 self.expect("register read -a", MISSING_EXPECTED_REGISTERS,
 substrs=['registers were unavailable'], matching=False)
 
+all_registers = self.res.GetOutput()
+
 if self.getArchitecture() in ['amd64', 'i386', 'x86_64']:
 self.runCmd("register read xmm0")
-self.runCmd("register read ymm15")  # may be available
-self.runCmd("register read bnd0")  # may be available
+if "ymm15 = " in all_registers:
+  self.runCmd("register read ymm15")  # may be available
+if "bnd0 = " in all_registers:
+  self.runCmd("register read bnd0")  # may be available
 elif self.getArchitecture() in ['arm', 'armv7', 'armv7k', 'arm64', 
'arm64e', 'arm64_32']:
 self.runCmd("register read s0")
-self.runCmd("register read q15")  # may be available
+if "q15 = " in all_registers:
+  self.runCmd("register read q15")  # may be available
 
 self.expect(
 "register read -s 4",
@@ -413,7 +418,8 @@ def fp_register_write(self):
 self.write_and_read(currentFrame, "ymm7", new_value)
 self.expect("expr $ymm0", substrs=['vector_type'])
 else:
-self.runCmd("register read ymm0")
+self.expect("register read ymm0", substrs=["Invalid register 
name 'ymm0'"],
+error=True)