[Lldb-commits] [PATCH] D133002: [LLDB] Make API tests to run using MSYS tools

[Muhammad Omair Javaid via Phabricator via lldb-commits]

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

MSYS 'uname' on windows returns "MSYS_NT*" instead of windows32 and also
MSYS 'pwd' returns non-windows path string.
This patch fixes Makefile.rules to make adjustments required to run LLDB
API tests using MSYS tools.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133002

Files:
  lldb/packages/Python/lldbsuite/test/make/Makefile.rules


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -49,8 +49,9 @@
 #--
 # If OS is not defined, use 'uname -s' to determine the OS name.
 #
-# uname on Windows gives "windows32" or "server version windows32", but most
-# environments standardize on "Windows_NT", so we'll make it consistent here.
+# GNUWin32 uname gives "windows32" or "server version windows32" while
+# some versions of MSYS uname return "MSYS_NT*", but most environments
+# standardize on "Windows_NT", so we'll make it consistent here. 
 # When running tests from Visual Studio, the environment variable isn't
 # inherited all the way down to the process spawned for make.
 #--
@@ -58,6 +59,11 @@
 ifneq (,$(findstring windows32,$(HOST_OS)))
HOST_OS := Windows_NT
 endif
+
+ifneq (,$(findstring MSYS_NT,$(HOST_OS)))
+   HOST_OS := Windows_NT
+endif
+
 ifeq "$(OS)" ""
OS := $(HOST_OS)
 endif
@@ -68,9 +74,12 @@
 # Some versions of make on Windows will search for other shells such as
 # C:\cygwin\bin\sh.exe. This shell fails for numerous different reasons
 # so default to using cmd.exe.
+# Also reset BUILDDIR value because "pwd" returns cygwin or msys path
+# which needs to be converted to windows path.
 #--
 ifeq "$(OS)" "Windows_NT"
SHELL = $(WINDIR)\system32\cmd.exe
+   BUILDDIR := $(shell echo %cd%)
 endif
 
 #--


Index: lldb/packages/Python/lldbsuite/test/make/Makefile.rules
===
--- lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -49,8 +49,9 @@
 #--
 # If OS is not defined, use 'uname -s' to determine the OS name.
 #
-# uname on Windows gives "windows32" or "server version windows32", but most
-# environments standardize on "Windows_NT", so we'll make it consistent here.
+# GNUWin32 uname gives "windows32" or "server version windows32" while
+# some versions of MSYS uname return "MSYS_NT*", but most environments
+# standardize on "Windows_NT", so we'll make it consistent here. 
 # When running tests from Visual Studio, the environment variable isn't
 # inherited all the way down to the process spawned for make.
 #--
@@ -58,6 +59,11 @@
 ifneq (,$(findstring windows32,$(HOST_OS)))
 	HOST_OS := Windows_NT
 endif
+
+ifneq (,$(findstring MSYS_NT,$(HOST_OS)))
+	HOST_OS := Windows_NT
+endif
+
 ifeq "$(OS)" ""
 	OS := $(HOST_OS)
 endif
@@ -68,9 +74,12 @@
 # Some versions of make on Windows will search for other shells such as
 # C:\cygwin\bin\sh.exe. This shell fails for numerous different reasons
 # so default to using cmd.exe.
+# Also reset BUILDDIR value because "pwd" returns cygwin or msys path
+# which needs to be converted to windows path.
 #--
 ifeq "$(OS)" "Windows_NT"
 	SHELL = $(WINDIR)\system32\cmd.exe
+	BUILDDIR := $(shell echo %cd%)
 endif
 
 #--
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133002: [LLDB] Make API tests to run using MSYS tools

[Martin Storsjö via Phabricator via lldb-commits]

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

LGTM, this looks reasonable to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133002

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

[Lldb-commits] [PATCH] D133011: [LLDB] Make build.py use uname to set platform

[Muhammad Omair Javaid via Phabricator via lldb-commits]

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

This patch makes build helper script build.py to use platform.uname for
machine/architecture detection. Visual studio environment when set using
various batch files like vcvars*.bat set PLATFORM environment variable
however VsDevCmd.bat does not set PLATFORM variable.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133011

Files:
  lldb/test/Shell/helper/build.py


Index: lldb/test/Shell/helper/build.py
===
--- lldb/test/Shell/helper/build.py
+++ lldb/test/Shell/helper/build.py
@@ -2,6 +2,7 @@
 
 import argparse
 import os
+import platform
 import shutil
 import signal
 import subprocess
@@ -274,7 +275,7 @@
 def __init__(self, toolchain_type, args):
 Builder.__init__(self, toolchain_type, args, '.obj')
 
-if os.getenv('PLATFORM') == 'arm64':
+if platform.uname().machine.lower() == 'arm64':
 self.msvc_arch_str = 'arm' if self.arch == '32' else 'arm64'
 else:
 self.msvc_arch_str = 'x86' if self.arch == '32' else 'x64'


Index: lldb/test/Shell/helper/build.py
===
--- lldb/test/Shell/helper/build.py
+++ lldb/test/Shell/helper/build.py
@@ -2,6 +2,7 @@
 
 import argparse
 import os
+import platform
 import shutil
 import signal
 import subprocess
@@ -274,7 +275,7 @@
 def __init__(self, toolchain_type, args):
 Builder.__init__(self, toolchain_type, args, '.obj')
 
-if os.getenv('PLATFORM') == 'arm64':
+if platform.uname().machine.lower() == 'arm64':
 self.msvc_arch_str = 'arm' if self.arch == '32' else 'arm64'
 else:
 self.msvc_arch_str = 'x86' if self.arch == '32' else 'x64'
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133011: [LLDB] Make build.py use uname to set platform

[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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133011

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

[Lldb-commits] [PATCH] D133002: [LLDB] Make API tests to run using MSYS tools

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

Seems reasonable. BTW, these days, I don't think anybody is invoking `make` 
directly from the command line, so it would be fine (maybe even desirable) to 
move some of this logic into python code -- particularly if it makes windows 
compatibility easier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133002

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

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp:246-248
+  DataExtractor data(&promise_addr, sizeof(promise_addr),
+ process_sp->GetByteOrder(),
+ process_sp->GetAddressByteSize());

Have you checked there won't be a use-after-free problem here, given that this 
data extractor will refer to the stack object?

To create persistent data, you need to use the DataBufferSP constructor, but 
I'm wondering if we couldn't fix this by creating the (non-pointer) object 
using the `CreateValueObjectFromAddress` function, as above, but then actually 
use valobj->AddressOf as the synthetic child.

I am also somewhat surprised that we need to use the GetAddressOf trick here, 
as this seems to indicate that the coroutine contains (in the proper C 
"subobject" kind of way) the promise object. That's not necessarily wrong, but 
it makes me think we may be "breaking the cycle" at the wrong place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132815

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

[Lldb-commits] [PATCH] D132940: [lldb] Use just-built libcxx for tests when available

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

> Cross-platform (such as API/macosx/macCatalyst and API/tools/lldb-server).

I find this statement fairly surprising. One should be able to run ~all tests 
in a "cross-platform" manner (i.e. where the test architecture is different 
than lldb (host) arch). What makes these two tests special? Couldn't we detect 
that we're in the cross-platform scenario (in python or cmake) and 
automatically switch to the system c++ library for all tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132940

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

[Lldb-commits] [PATCH] D132954: lldb: Add support for R_386_32 relocations to ObjectFileELF

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D132954#3758856 , @dmlary wrote:

> I'm looking for any suggestions of how to test this.  I can create a simple 
> object file with the needed relocation types with yaml2obj, but I don't see 
> an easy way to get lldb to apply those relocations (in .`.text` for example).

IIRC, lldb explicitly skips any relocations on non-debug sections. Could you 
check if the relocations are being reflected in the output of `lldb-test 
object-file --contents %t`? If so, then you could use that command to print the 
contents of a dummy debug info section (it doesn't even have to be valid DWARF).




Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2650
+ reloc_type(rel));
 assert(false && "unexpected relocation type");
   }

Maybe just remove this assertion now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132954

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

[Lldb-commits] [PATCH] D132283: [lldb] [Core] Reimplement Communication::ReadThread using MainLoop

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Core/Communication.cpp:427
   // Notify the read thread.
-  m_connection_sp->InterruptRead();
 

mgorny wrote:
> labath wrote:
> > Have you considered putting this code (some version of it) inside 
> > `InterruptRead`? Basically replacing the `select` call inside 
> > BytesAvailable with something MainLoop-based ?
> To be honest, I've been considering removing `InterruptRead()` entirely, now 
> that the read loop is triggered by available input rather than 
> read-with-timeout. However, it's still used by editline support.
> 
> That said, I'm not sure what's your idea, given that `Connection` does not 
> have awareness of `Communication` that's using it. I suppose I could pass the 
> `MainLoop` instance as a parameter but we'd still have to maintain a separate 
> version for editline support, and we only have a single caller here.
> To be honest, I've been considering removing `InterruptRead()` entirely, now 
> that the read loop is triggered by available input rather than 
> read-with-timeout. However, it's still used by editline support.
If we could do that, then it would be even better. And it looks like it should 
be possible to use a MainLoop instance inside the Editline class, instead of 
the built-in interruption support.

> That said, I'm not sure what's your idea, given that `Connection` does not 
> have awareness of `Communication` that's using it. I suppose I could pass the 
> `MainLoop` instance as a parameter but we'd still have to maintain a separate 
> version for editline support, and we only have a single caller here.

I don't claim to have thought this out completely, but I was imagining that the 
MainLoop instance would be internal to the Connection class. The external 
interface of the Connection class would remain unchanged (so the Communication 
class would not need to change), and the InterruptRead function would be 
implemented using the MainLoop functionality.


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

https://reviews.llvm.org/D132283

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

[Lldb-commits] [PATCH] D132577: [lldb] [Core] Pass error/status from Communication::ReadThread()

[Pavel Labath via Phabricator via lldb-commits]

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



Comment at: lldb/unittests/Core/CommunicationTest.cpp:19-20
+#include 
+#include 
+#include 
 #include 

I guess these two are not used anymore.


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

https://reviews.llvm.org/D132577

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

[Lldb-commits] [PATCH] D132940: [lldb] Use just-built libcxx for tests when available

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

In D132940#3761038 , @labath wrote:

>> Cross-platform (such as API/macosx/macCatalyst and API/tools/lldb-server).
>
> What makes these two tests special?

These two tests are _always_ building binaries for Mac Catalyst 
(API/macosx/macCatalyst), and watchOS/iOS Simulator 
(API/tools/lldb-server/TestAppleSimulatorOSType.py). Most builds of libcxx 
won't have support for those, so we end up with link errors like:

> building for watchOS Simulator, but linking in dylib built for macOS, file 
> '[...] build/./lib/libc++.dylib' for architecture arm64


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132940

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

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp:246-248
+  DataExtractor data(&promise_addr, sizeof(promise_addr),
+ process_sp->GetByteOrder(),
+ process_sp->GetAddressByteSize());

labath wrote:
> Have you checked there won't be a use-after-free problem here, given that 
> this data extractor will refer to the stack object?
> 
> To create persistent data, you need to use the DataBufferSP constructor, but 
> I'm wondering if we couldn't fix this by creating the (non-pointer) object 
> using the `CreateValueObjectFromAddress` function, as above, but then 
> actually use valobj->AddressOf as the synthetic child.
> 
> I am also somewhat surprised that we need to use the GetAddressOf trick here, 
> as this seems to indicate that the coroutine contains (in the proper C 
> "subobject" kind of way) the promise object. That's not necessarily wrong, 
> but it makes me think we may be "breaking the cycle" at the wrong place.
Thanks for taking a look!

> To create persistent data, you need to use the DataBufferSP constructor

good point, I will keep this in mind as a fallback in case we don't decide to 
follow any of the other directions you hinted at.

> wondering if we couldn't fix this by creating the (non-pointer) object using 
> the CreateValueObjectFromAddress function, as above, but then actually use 
> valobj->AddressOf as the synthetic child

Good idea! I will give it a try


> [...] as this seems to indicate that the coroutine contains (in the proper C 
> "subobject" kind of way) the promise object. That's not necessarily wrong, 
> but it makes me think we may be "breaking the cycle" at the wrong place.

The physical layout of this is:
```
// in the standard library
template
struct exception_handle {
__coro_frame* __hdl; // <--- this is the pointer we read with 
`GetCoroFramePtrFromHandle`
};

// compiler-generated coroutine frame. Generated ad-hoc per coroutine
struct __coro_frame {
 // The ABI guaranteees that hose two pointers are always the first two 
pointers in the struct.
 void (*resume)(void*); // function pointer for type erasure
 void (*destroy)(void*); // function pointer for type erasure
 // Next comes our promise type. This is under the control of the program 
author
 promise_type promise;
 // Next comes any compiler-generated, internal state which gets persisted 
across suspension points. 
 // The functions pointed to by `resume`/`destroy` know how to interpret 
this part of the coroutine frame.
 int __suspension_point_id;
 double __some_internal_state;
 std::string __some_other_internal_state;
 
};
```

The programmer (i.e., most likely the user of this pretty-printer), wrote only 
the "promise" explicitly in his code. Everything else is compiler-generated. As 
such, the lldb-user will usually look for the "promise" first, and I would like 
to make it easy to find it, by exposing it as a top-level children of the 
`exception_handle` instead of hiding it inside a sub-child.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132815

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

[Lldb-commits] [PATCH] D132578: [lldb] [Core] Use thread for Communication::Write() as well

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D132578#3751968 , @mgorny wrote:

> Well, back when working on the async thread, you've indicated that it's a bad 
> idea to read from one thread while writing from another, so I've basically 
> focused on getting to the point when all reads and writes are guaranteed to 
> happen from a single thread.

Ok, I think I know what you mean. That comment was made in the context of the 
GDB communication class, whose functionality is more complex (sending a packet 
involves both reading and writing) than the plain `Communication`.  Sending 
packets from multiple threads is more complex that a hypothetical simple setup 
where you have a single thread just doing reading, and another thread just for 
writing...

In D132578#3751969 , @mgorny wrote:

> (with the final goal of using `StartReadThread()` inside gdb-remote comms to 
> take care of reading the data for concurrent process plugin instances)

... but we don't have such a case, so doing this might be fine. I sort of 
understand the direction you're going with this, but I can't really visualize 
the final state. Could you give a short summary of the desired final 
architecture.

FWIW, my idea was to use the gdb-remote "async" thread for this centralized 
sort of reading(sending)/writing(receiving). The async thread serves a similar 
purpose to the communication read thread, but is completely unrelated to it. I 
thought that would be the easiest to implement, since we already kind of have a 
synchronization mechanism (the "continue" lock) between these thread and other 
threads. However, I don't have a complete picture of how would this work (I'd 
guess we have a single "async" thread for all of the processes, and it would 
somehow synchronize/arbitrate between them), so I am open to other ideas as 
well.


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

https://reviews.llvm.org/D132578

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

[Lldb-commits] [PATCH] D132940: [lldb] Use just-built libcxx for tests when available

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D132940#3761082 , @fdeazeve wrote:

> In D132940#3761038 , @labath wrote:
>
>>> Cross-platform (such as API/macosx/macCatalyst and API/tools/lldb-server).
>>
>> What makes these two tests special?
>
> These two tests are _always_ building binaries for Mac Catalyst 
> (API/macosx/macCatalyst), and watchOS/iOS Simulator 
> (API/tools/lldb-server/TestAppleSimulatorOSType.py).

Ok, that makes more sense. I was confused because the Makefile in 
`lldb/test/API/tools/lldb-server` is used by (a lot) more tests than just 
TestAppleSimulatorOSType. In that case, I would say that this argument should 
be passed directly from python, as a part of the `self.build` command (the test 
already passes a bunch of arguments there anyway).




Comment at: lldb/packages/Python/lldbsuite/test/make/Makefile.rules:381-386
+ifneq ($(and $(USE_LIBSTDCPP), $(USE_LIBCPP)),)
+   $(error Libcxx and Libstdc++ cannot be used together)
+endif
+
+ifeq (1, $(USE_SYSTEM_STDLIB))
+   ifneq ($(or $(USE_LIBSTDCPP), $(USE_LIBCPP)),)

Instead of three distinct variables, it might be nicer to just have one 
argument (STDLIB_KIND ?) which can take three different values...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132940

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

[Lldb-commits] [PATCH] D131160: [WIP][lldb] Add "event" capability to the MainLoop class

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D131160#3751959 , @mgorny wrote:

> In that case, is there something more I should do about this patch or are you 
> going to take over from here?

It wasn't clear to me whether you intend to finish this patch (and I was 
waiting on your response to this ). 
That said, if you don't have any strong opinions there, then I can try to 
finish this.


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

https://reviews.llvm.org/D131160

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

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp:246-248
+  DataExtractor data(&promise_addr, sizeof(promise_addr),
+ process_sp->GetByteOrder(),
+ process_sp->GetAddressByteSize());

avogelsgesang wrote:
> labath wrote:
> > Have you checked there won't be a use-after-free problem here, given that 
> > this data extractor will refer to the stack object?
> > 
> > To create persistent data, you need to use the DataBufferSP constructor, 
> > but I'm wondering if we couldn't fix this by creating the (non-pointer) 
> > object using the `CreateValueObjectFromAddress` function, as above, but 
> > then actually use valobj->AddressOf as the synthetic child.
> > 
> > I am also somewhat surprised that we need to use the GetAddressOf trick 
> > here, as this seems to indicate that the coroutine contains (in the proper 
> > C "subobject" kind of way) the promise object. That's not necessarily 
> > wrong, but it makes me think we may be "breaking the cycle" at the wrong 
> > place.
> Thanks for taking a look!
> 
> > To create persistent data, you need to use the DataBufferSP constructor
> 
> good point, I will keep this in mind as a fallback in case we don't decide to 
> follow any of the other directions you hinted at.
> 
> > wondering if we couldn't fix this by creating the (non-pointer) object 
> > using the CreateValueObjectFromAddress function, as above, but then 
> > actually use valobj->AddressOf as the synthetic child
> 
> Good idea! I will give it a try
> 
> 
> > [...] as this seems to indicate that the coroutine contains (in the proper 
> > C "subobject" kind of way) the promise object. That's not necessarily 
> > wrong, but it makes me think we may be "breaking the cycle" at the wrong 
> > place.
> 
> The physical layout of this is:
> ```
> // in the standard library
> template
> struct exception_handle {
> __coro_frame* __hdl; // <--- this is the pointer we read 
> with `GetCoroFramePtrFromHandle`
> };
> 
> // compiler-generated coroutine frame. Generated ad-hoc per coroutine
> struct __coro_frame {
>  // The ABI guaranteees that hose two pointers are always the first two 
> pointers in the struct.
>  void (*resume)(void*); // function pointer for type erasure
>  void (*destroy)(void*); // function pointer for type erasure
>  // Next comes our promise type. This is under the control of the program 
> author
>  promise_type promise;
>  // Next comes any compiler-generated, internal state which gets 
> persisted across suspension points. 
>  // The functions pointed to by `resume`/`destroy` know how to interpret 
> this part of the coroutine frame.
>  int __suspension_point_id;
>  double __some_internal_state;
>  std::string __some_other_internal_state;
>  
> };
> ```
> 
> The programmer (i.e., most likely the user of this pretty-printer), wrote 
> only the "promise" explicitly in his code. Everything else is 
> compiler-generated. As such, the lldb-user will usually look for the 
> "promise" first, and I would like to make it easy to find it, by exposing it 
> as a top-level children of the `exception_handle` instead of hiding it inside 
> a sub-child.
> As such, the lldb-user will usually look for the "promise" first, and I would 
> like to make it easy to find it, by exposing it as a top-level children of 
> the `exception_handle` instead of hiding it inside a sub-child.

That makes sense. And I think that's a good argument for automatically 
"dereferencing" that object (i.e., status quo). That said, it's not fully clear 
to me how do we end up looping here. I take it the promise object contains a 
(compiler-generated ?) pointer to another __coro_frame object? What would 
happen if we turned *that* into a pointer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132815

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

[Lldb-commits] [PATCH] D133024: [LLDB] Simplify cmake for instruction emulation unit tests

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added subscribers: luke957, s.egerton, simoncook, kristof.beyls, mgorny.
Herald added a project: All.
DavidSpickett requested review of this revision.
Herald added subscribers: lldb-commits, pcwang-thead.
Herald added a project: LLDB.

I got suspicious because of checking "ARM" for an "ARM64" plugin.
As far as I can tell these never needed an llvm target to function.

Looking at the corresponding cmake for the libraries under test they
don't reference target libraries either.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133024

Files:
  lldb/unittests/Instruction/CMakeLists.txt


Index: lldb/unittests/Instruction/CMakeLists.txt
===
--- lldb/unittests/Instruction/CMakeLists.txt
+++ lldb/unittests/Instruction/CMakeLists.txt
@@ -1,29 +1,14 @@
-set(FILES "")
-set(DEPS "")
-
-if ("ARM" IN_LIST LLVM_TARGETS_TO_BUILD)
-  list(APPEND FILES ARM64/TestAArch64Emulator.cpp)
-  list(APPEND DEPS lldbPluginInstructionARM64)
-endif ()
-
-if ("RISCV" IN_LIST LLVM_TARGETS_TO_BUILD)
-  list(APPEND FILES RISCV/TestRISCVEmulator.cpp)
-  list(APPEND DEPS lldbPluginInstructionRISCV)
-endif ()
-
-list(LENGTH FILES LISTLEN)
-
-if (LISTLEN GREATER 0)
-  add_lldb_unittest(EmulatorTests
-${FILES}
-
-LINK_LIBS
-  lldbCore
-  lldbSymbol
-  lldbTarget
-  ${DEPS}
-LINK_COMPONENTS
-  Support
-  ${LLVM_TARGETS_TO_BUILD}
-)
-endif ()
+add_lldb_unittest(EmulatorTests
+  ARM64/TestAArch64Emulator.cpp
+  RISCV/TestRISCVEmulator.cpp
+
+  LINK_LIBS
+lldbCore
+lldbSymbol
+lldbTarget
+lldbPluginInstructionARM64
+lldbPluginInstructionRISCV
+
+  LINK_COMPONENTS
+Support
+  )
\ No newline at end of file


Index: lldb/unittests/Instruction/CMakeLists.txt
===
--- lldb/unittests/Instruction/CMakeLists.txt
+++ lldb/unittests/Instruction/CMakeLists.txt
@@ -1,29 +1,14 @@
-set(FILES "")
-set(DEPS "")
-
-if ("ARM" IN_LIST LLVM_TARGETS_TO_BUILD)
-  list(APPEND FILES ARM64/TestAArch64Emulator.cpp)
-  list(APPEND DEPS lldbPluginInstructionARM64)
-endif ()
-
-if ("RISCV" IN_LIST LLVM_TARGETS_TO_BUILD)
-  list(APPEND FILES RISCV/TestRISCVEmulator.cpp)
-  list(APPEND DEPS lldbPluginInstructionRISCV)
-endif ()
-
-list(LENGTH FILES LISTLEN)
-
-if (LISTLEN GREATER 0)
-  add_lldb_unittest(EmulatorTests
-${FILES}
-
-LINK_LIBS
-  lldbCore
-  lldbSymbol
-  lldbTarget
-  ${DEPS}
-LINK_COMPONENTS
-  Support
-  ${LLVM_TARGETS_TO_BUILD}
-)
-endif ()
+add_lldb_unittest(EmulatorTests
+  ARM64/TestAArch64Emulator.cpp
+  RISCV/TestRISCVEmulator.cpp
+
+  LINK_LIBS
+lldbCore
+lldbSymbol
+lldbTarget
+lldbPluginInstructionARM64
+lldbPluginInstructionRISCV
+
+  LINK_COMPONENTS
+Support
+  )
\ No newline at end of file
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D132815: [LLDB] Do not dereference promise pointer in `coroutine_handle` pretty printer

[Adrian Vogelsgesang via Phabricator via lldb-commits]

avogelsgesang added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp:246-248
+  DataExtractor data(&promise_addr, sizeof(promise_addr),
+ process_sp->GetByteOrder(),
+ process_sp->GetAddressByteSize());

labath wrote:
> avogelsgesang wrote:
> > labath wrote:
> > > Have you checked there won't be a use-after-free problem here, given that 
> > > this data extractor will refer to the stack object?
> > > 
> > > To create persistent data, you need to use the DataBufferSP constructor, 
> > > but I'm wondering if we couldn't fix this by creating the (non-pointer) 
> > > object using the `CreateValueObjectFromAddress` function, as above, but 
> > > then actually use valobj->AddressOf as the synthetic child.
> > > 
> > > I am also somewhat surprised that we need to use the GetAddressOf trick 
> > > here, as this seems to indicate that the coroutine contains (in the 
> > > proper C "subobject" kind of way) the promise object. That's not 
> > > necessarily wrong, but it makes me think we may be "breaking the cycle" 
> > > at the wrong place.
> > Thanks for taking a look!
> > 
> > > To create persistent data, you need to use the DataBufferSP constructor
> > 
> > good point, I will keep this in mind as a fallback in case we don't decide 
> > to follow any of the other directions you hinted at.
> > 
> > > wondering if we couldn't fix this by creating the (non-pointer) object 
> > > using the CreateValueObjectFromAddress function, as above, but then 
> > > actually use valobj->AddressOf as the synthetic child
> > 
> > Good idea! I will give it a try
> > 
> > 
> > > [...] as this seems to indicate that the coroutine contains (in the 
> > > proper C "subobject" kind of way) the promise object. That's not 
> > > necessarily wrong, but it makes me think we may be "breaking the cycle" 
> > > at the wrong place.
> > 
> > The physical layout of this is:
> > ```
> > // in the standard library
> > template
> > struct exception_handle {
> > __coro_frame* __hdl; // <--- this is the pointer we read 
> > with `GetCoroFramePtrFromHandle`
> > };
> > 
> > // compiler-generated coroutine frame. Generated ad-hoc per coroutine
> > struct __coro_frame {
> >  // The ABI guaranteees that hose two pointers are always the first two 
> > pointers in the struct.
> >  void (*resume)(void*); // function pointer for type erasure
> >  void (*destroy)(void*); // function pointer for type erasure
> >  // Next comes our promise type. This is under the control of the 
> > program author
> >  promise_type promise;
> >  // Next comes any compiler-generated, internal state which gets 
> > persisted across suspension points. 
> >  // The functions pointed to by `resume`/`destroy` know how to 
> > interpret this part of the coroutine frame.
> >  int __suspension_point_id;
> >  double __some_internal_state;
> >  std::string __some_other_internal_state;
> >  
> > };
> > ```
> > 
> > The programmer (i.e., most likely the user of this pretty-printer), wrote 
> > only the "promise" explicitly in his code. Everything else is 
> > compiler-generated. As such, the lldb-user will usually look for the 
> > "promise" first, and I would like to make it easy to find it, by exposing 
> > it as a top-level children of the `exception_handle` instead of hiding it 
> > inside a sub-child.
> > As such, the lldb-user will usually look for the "promise" first, and I 
> > would like to make it easy to find it, by exposing it as a top-level 
> > children of the `exception_handle` instead of hiding it inside a sub-child.
> 
> That makes sense. And I think that's a good argument for automatically 
> "dereferencing" that object (i.e., status quo). That said, it's not fully 
> clear to me how do we end up looping here. I take it the promise object 
> contains a (compiler-generated ?) pointer to another __coro_frame object? 
> What would happen if we turned *that* into a pointer?
> I take it the promise object contains a [...] pointer to another __coro_frame 
> object?

yes

> (compiler-generated ?)

no

We end up looping if the user explicitly puts an `coroutine_handle` inside 
`promise_type`. You can find an example of this in 
https://clang.llvm.org/docs/DebuggingCoroutines.html#get-the-asynchronous-stack,
 in particular

```
struct promise_type {
// [...] shortened for readability

std::coroutine_handle<> continuation = std::noop_coroutine();
int result = 0;
};
```

In asynchronous programming, it is common to "chain continuations" by putting a 
`coroutine_handle` into the `promise_type`. This coroutine_handle inside the 
promise_type gives my asynchronous coroutine the answer to the question "where 
should I continue next, after finishing the asychronous operation modelled by 
my own coroutine?".

In normal situations (i.e. in the absence of bugs inside the debugged program), 
those coroutine_handle's should not

[Lldb-commits] [PATCH] D133024: [LLDB] Simplify cmake for instruction emulation unit tests

[Pavel Labath via Phabricator via lldb-commits]

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

If the tests work without the corresponding llvm targets, then yea, ship it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133024

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

[Lldb-commits] [PATCH] D133038: Add GetSourceMap public API

[jeffrey tan via Phabricator via lldb-commits]

yinghuitan created this revision.
yinghuitan added reviewers: clayborg, labath, aadsm, kusmour.
Herald added a project: All.
yinghuitan requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a new SBTarget::GetSourceMap() API which returns a JSON struct
for client to access source mapping entries.

This can be used later by auto deduce source map from source line breakpoint 
feature for testing purpose.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133038

Files:
  lldb/bindings/interface/SBTarget.i
  lldb/include/lldb/API/SBTarget.h
  lldb/include/lldb/Target/PathMappingList.h
  lldb/source/API/SBTarget.cpp
  lldb/source/Target/PathMappingList.cpp
  lldb/test/API/functionalities/source-map/TestTargetSourceMap.py

Index: lldb/test/API/functionalities/source-map/TestTargetSourceMap.py
===
--- lldb/test/API/functionalities/source-map/TestTargetSourceMap.py
+++ lldb/test/API/functionalities/source-map/TestTargetSourceMap.py
@@ -1,11 +1,49 @@
 import lldb
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test.decorators import *
+import json
 import os
 
 
 class TestTargetSourceMap(TestBase):
 
+@no_debug_info_test
+def test_source_map_target_api(self):
+"""
+Test that ensures SBTarget::GetSourceMap() API can correctly fetch
+source mapping entries.
+"""
+# Set the target soure map to map "./" to the current test directory
+src_dir = self.getSourceDir()
+src_path = os.path.join(src_dir, "main.c")
+yaml_path = os.path.join(src_dir, "a.yaml")
+yaml_base, ext = os.path.splitext(yaml_path)
+obj_path = self.getBuildArtifact("main.o")
+self.yaml2obj(yaml_path, obj_path)
+
+# Create a target with the object file we just created from YAML
+target = self.dbg.CreateTarget(obj_path)
+
+initial_source_map = target.GetSourceMap()
+self.assertEquals(initial_source_map.GetSize(), 0,
+"Initial source map should be empty")
+
+src_dir = self.getSourceDir()
+self.runCmd('settings append target.source-map . "%s"' % src_dir)
+
+source_map = target.GetSourceMap()
+self.assertEquals(source_map.GetSize(), 1,
+"source map should be have one appended entry")
+
+stream = lldb.SBStream()
+source_map.GetAsJSON(stream)
+serialized_source_map = json.loads(stream.GetData())
+self.assertEquals(serialized_source_map[0]["first"], ".",
+"source map entry 'first' does not match")
+self.assertEquals(serialized_source_map[0]["second"], src_dir,
+"source map entry 'second' does not match")
+
+
 @no_debug_info_test
 def test_source_map(self):
 """Test target.source-map' functionality."""
Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -131,6 +131,17 @@
   }
 }
 
+llvm::json::Value PathMappingList::ToJSON() {
+  llvm::json::Array entries;
+  for (const auto &pair : m_pairs) {
+llvm::json::Object entry;
+entry.try_emplace("first", pair.first.GetStringRef().str());
+entry.try_emplace("second", pair.second.GetStringRef().str());
+entries.emplace_back(std::move(entry));
+  }
+  return std::move(entries);
+}
+
 void PathMappingList::Clear(bool notify) {
   if (!m_pairs.empty())
 ++m_mod_id;
Index: lldb/source/API/SBTarget.cpp
===
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -211,6 +211,21 @@
   return data;
 }
 
+SBStructuredData SBTarget::GetSourceMap() {
+  LLDB_INSTRUMENT_VA(this);
+
+  SBStructuredData data;
+  TargetSP target_sp(GetSP());
+  if (!target_sp)
+return data;
+
+  std::string json_str =
+  llvm::formatv("{0:2}",
+  target_sp->GetSourcePathMap().ToJSON()).str();
+  data.m_impl_up->SetObjectSP(StructuredData::ParseJSON(json_str));
+  return data;
+}
+
 void SBTarget::SetCollectingStats(bool v) {
   LLDB_INSTRUMENT_VA(this, v);
 
@@ -1590,7 +1605,7 @@
 
 const char *SBTarget::GetABIName() {
   LLDB_INSTRUMENT_VA(this);
-  
+
   TargetSP target_sp(GetSP());
   if (target_sp) {
 std::string abi_name(target_sp->GetABIName().str());
Index: lldb/include/lldb/Target/PathMappingList.h
===
--- lldb/include/lldb/Target/PathMappingList.h
+++ lldb/include/lldb/Target/PathMappingList.h
@@ -13,6 +13,7 @@
 #include 
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Status.h"
+#include "llvm/Support/JSON.h"
 
 namespace lldb_private {
 
@@ -41,6 +42,8 @@
   // By default, dump all pairs.
   void Dump(Stream *s, int pair_index = -1);
 
+  llvm::json::Value ToJSON();
+
   bool IsEmpt

[Lldb-commits] [PATCH] D133038: Add GetSourceMap public API

[jeffrey tan via Phabricator via lldb-commits]

yinghuitan updated this revision to Diff 457046.
yinghuitan added a comment.
Herald added a subscriber: JDevlieghere.

Re-diff with clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133038

Files:
  lldb/bindings/interface/SBTarget.i
  lldb/include/lldb/API/SBTarget.h
  lldb/include/lldb/Target/PathMappingList.h
  lldb/source/API/SBTarget.cpp
  lldb/source/Target/PathMappingList.cpp
  lldb/test/API/functionalities/source-map/TestTargetSourceMap.py

Index: lldb/test/API/functionalities/source-map/TestTargetSourceMap.py
===
--- lldb/test/API/functionalities/source-map/TestTargetSourceMap.py
+++ lldb/test/API/functionalities/source-map/TestTargetSourceMap.py
@@ -1,11 +1,49 @@
 import lldb
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test.decorators import *
+import json
 import os
 
 
 class TestTargetSourceMap(TestBase):
 
+@no_debug_info_test
+def test_source_map_target_api(self):
+"""
+Test that ensures SBTarget::GetSourceMap() API can correctly fetch
+source mapping entries.
+"""
+# Set the target soure map to map "./" to the current test directory
+src_dir = self.getSourceDir()
+src_path = os.path.join(src_dir, "main.c")
+yaml_path = os.path.join(src_dir, "a.yaml")
+yaml_base, ext = os.path.splitext(yaml_path)
+obj_path = self.getBuildArtifact("main.o")
+self.yaml2obj(yaml_path, obj_path)
+
+# Create a target with the object file we just created from YAML
+target = self.dbg.CreateTarget(obj_path)
+
+initial_source_map = target.GetSourceMap()
+self.assertEquals(initial_source_map.GetSize(), 0,
+"Initial source map should be empty")
+
+src_dir = self.getSourceDir()
+self.runCmd('settings append target.source-map . "%s"' % src_dir)
+
+source_map = target.GetSourceMap()
+self.assertEquals(source_map.GetSize(), 1,
+"source map should be have one appended entry")
+
+stream = lldb.SBStream()
+source_map.GetAsJSON(stream)
+serialized_source_map = json.loads(stream.GetData())
+self.assertEquals(serialized_source_map[0]["first"], ".",
+"source map entry 'first' does not match")
+self.assertEquals(serialized_source_map[0]["second"], src_dir,
+"source map entry 'second' does not match")
+
+
 @no_debug_info_test
 def test_source_map(self):
 """Test target.source-map' functionality."""
Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -131,6 +131,17 @@
   }
 }
 
+llvm::json::Value PathMappingList::ToJSON() {
+  llvm::json::Array entries;
+  for (const auto &pair : m_pairs) {
+llvm::json::Object entry;
+entry.try_emplace("first", pair.first.GetStringRef().str());
+entry.try_emplace("second", pair.second.GetStringRef().str());
+entries.emplace_back(std::move(entry));
+  }
+  return std::move(entries);
+}
+
 void PathMappingList::Clear(bool notify) {
   if (!m_pairs.empty())
 ++m_mod_id;
Index: lldb/source/API/SBTarget.cpp
===
--- lldb/source/API/SBTarget.cpp
+++ lldb/source/API/SBTarget.cpp
@@ -211,6 +211,21 @@
   return data;
 }
 
+SBStructuredData SBTarget::GetSourceMap() {
+  LLDB_INSTRUMENT_VA(this);
+
+  SBStructuredData data;
+  TargetSP target_sp(GetSP());
+  if (!target_sp)
+return data;
+
+  std::string json_str =
+  llvm::formatv("{0:2}",
+  target_sp->GetSourcePathMap().ToJSON()).str();
+  data.m_impl_up->SetObjectSP(StructuredData::ParseJSON(json_str));
+  return data;
+}
+
 void SBTarget::SetCollectingStats(bool v) {
   LLDB_INSTRUMENT_VA(this, v);
 
@@ -1590,7 +1605,7 @@
 
 const char *SBTarget::GetABIName() {
   LLDB_INSTRUMENT_VA(this);
-  
+
   TargetSP target_sp(GetSP());
   if (target_sp) {
 std::string abi_name(target_sp->GetABIName().str());
Index: lldb/include/lldb/Target/PathMappingList.h
===
--- lldb/include/lldb/Target/PathMappingList.h
+++ lldb/include/lldb/Target/PathMappingList.h
@@ -9,10 +9,11 @@
 #ifndef LLDB_TARGET_PATHMAPPINGLIST_H
 #define LLDB_TARGET_PATHMAPPINGLIST_H
 
-#include 
-#include 
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Status.h"
+#include "llvm/Support/JSON.h"
+#include 
+#include 
 
 namespace lldb_private {
 
@@ -41,6 +42,8 @@
   // By default, dump all pairs.
   void Dump(Stream *s, int pair_index = -1);
 
+  llvm::json::Value ToJSON();
+
   bool IsEmpty() const { return m_pairs.empty(); }
 
   size_t GetSize() const { return m_pairs.size(); }
Index: lldb/include/lldb/API/SBTarget.h
=

[Lldb-commits] [PATCH] D133042: Add auto deduce source map setting

[jeffrey tan via Phabricator via lldb-commits]

yinghuitan created this revision.
yinghuitan added reviewers: clayborg, labath, jingham, jdoerfert, JDevlieghere, 
aadsm, kusmour.
Herald added a project: All.
yinghuitan requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a new "target.auto-deduce-source-map" setting.

If enabled, this setting may auto deduce a source map entry based on requested
breakpoint path and the original path stored in debug info for resolved 
breakpoint.

As an example, if debug info contains "./a/b/c/main.cpp", user sets a source
breakpoint at "/root/repo/x/y/z/a/b/c/main.cpp". The breakpoint will resolve
correctly now with Greg's patch https://reviews.llvm.org/D130401. However, the
resolved breakpoint will use "./a/b/c/main.cpp" to locate source file during
stop event which would fail most of the time.

With the new "target.auto-deduce-source-map" setting enabled, a auto deduced 
source map entry "." => "/root/repo/x/y/z" will be added. This new mapping will
help lldb to map resolved breakpoint path "./a/b/c/main.cpp" back to 
"/root/repo/x/y/z/a/b/c/main.cpp" and locate it on disk.

If an existing source map entry is used the patch also concatenates the auto
deduced entry with any stripped reverse mapping prefix (see example below).

As a second example, debug info contains "./a/b/c/main.cpp" and user sets
breakpoint at "/root/repo/x/y/z/a/b/c/main.cpp". Let's say there is an existing
source map entry "." => "/root/repo"; this mapping would strip the prefix out 
of 
"/root/repo/x/y/z/a/b/c/main.cpp" and use "x/y/z/a/b/c/main.cpp" to resolve
breakpoint. "target.auto-deduce-source-map" setting would auto deduce a new 
potential mapping of "." => "x/y/z", then it detects that there is a stripped
prefix from reverse mapping and concatenates it as the new mapping:
 "." => "/root/repo/x/y/z" which would correct map "./a/b/c/main.cpp" path to
new path in disk.

This patches depends on https://reviews.llvm.org/D130401 to use new added
SBTarget::GetSourceMap() API for testing.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133042

Files:
  lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h
  lldb/include/lldb/Target/PathMappingList.h
  lldb/include/lldb/Target/Target.h
  lldb/source/Breakpoint/BreakpointResolverFileLine.cpp
  lldb/source/Target/PathMappingList.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/TargetProperties.td
  
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
  lldb/unittests/Target/PathMappingListTest.cpp

Index: lldb/unittests/Target/PathMappingListTest.cpp
===
--- lldb/unittests/Target/PathMappingListTest.cpp
+++ lldb/unittests/Target/PathMappingListTest.cpp
@@ -40,7 +40,7 @@
 map.RemapPath(ConstString(match.original.GetPath()), actual_remapped));
 EXPECT_EQ(FileSpec(actual_remapped.GetStringRef()), match.remapped);
 FileSpec unmapped_spec;
-EXPECT_TRUE(map.ReverseRemapPath(match.remapped, unmapped_spec));
+EXPECT_TRUE(map.ReverseRemapPath(match.remapped, unmapped_spec).hasValue());
 std::string unmapped_path = unmapped_spec.GetPath();
 EXPECT_EQ(unmapped_path, orig_normalized);
   }
Index: lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -8,6 +8,7 @@
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
+import json
 import os
 import side_effect
 
@@ -428,3 +429,84 @@
 
 bp_3 = target.FindBreakpointByID(bp_id_3)
 self.assertFalse(bp_3.IsValid(), "Didn't delete disabled breakpoint 3")
+
+
+def get_source_map_json(self, target):
+stream = lldb.SBStream()
+target.GetSourceMap().GetAsJSON(stream)
+return json.loads(stream.GetData())
+
+def verify_source_map_entry_pair(self, entry, first, second):
+self.assertEquals(entry["first"], first,
+"source map entry 'first' does not match")
+self.assertEquals(entry["second"], second,
+"source map entry 'second' does not match")
+
+@skipIf(oslist=["windows"])
+@no_debug_info_test
+def test_breakpoints_auto_deduce_source_map(self):
+"""
+Test that with target.auto-deduce-source-map settings.
+
+The "relative.yaml" contains a line table that is:
+
+Line table for a/b/c/main.cpp in `a.out
+0x00013f94: a/b/c/main.cpp:1
+0x00013fb0: a/b/c/main.cpp:2:3
+0x00013fb8: a/b/c/main.cpp:2:3
+"""
+src_dir = self.getSourceDir()
+yaml_path = os.path.join(src_dir, "relative.yaml")
+yaml

[Lldb-commits] [PATCH] D133038: Add GetSourceMap public API

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I'm a little sad that we don't yet have a way to read the current value of a 
setting into an SBStructuredData, so we do this piecemeal instead.  But that's 
a bigger project, so if you need this now, it doesn't seem fair to block you on 
that.  We should really name the output SBStructuredData keys more 
instructively, however.




Comment at: lldb/include/lldb/API/SBTarget.h:91
+  /// \return
+  /// A SBStructuredData with the source map entries collected.
+  lldb::SBStructuredData GetSourceMap();

You have to say what the structure is so people will know how to fetch the 
elements.



Comment at: lldb/source/API/SBTarget.cpp:222
+
+  std::string json_str =
+  llvm::formatv("{0:2}",

It seems a little round-about to convert the source map to JSON, then from JSON 
to an SBStructuredData.  You should be able to write the elements directly.  
I'm not sure how much that matters, however.



Comment at: lldb/source/Target/PathMappingList.cpp:138
+llvm::json::Object entry;
+entry.try_emplace("first", pair.first.GetStringRef().str());
+entry.try_emplace("second", pair.second.GetStringRef().str());

Can we call these something more instructive than "first" and "second"?  These 
are the "original" path and the "substitution" path, maybe those would be good 
keys?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133038

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

[Lldb-commits] [PATCH] D133045: Partial fix for handling backticks in commands and aliases

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added reviewers: JDevlieghere, clayborg.
Herald added a subscriber: jeroen.dobbelaere.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

At present, backtick substitution is a "preprocessor" operation, which is not, 
I think, what people expect.  I'm pretty sure you didn't mean to run an 
expression when you specified a file name that happened to have backticks in 
it, etc.  I think it makes a lot more sense that the substitution should be 
done when it is encountered as a quote character.  There was, in fact, some 
code in CommandObjectParsed that attempted to treat it this way, but it was 
defeated by the fact that we unconditionally do PreprocessCommand on the 
command string before parsing.  It was also not quite right since it assumed 
the quote would still be present in the argument value, when in fact it was 
just in the quote member of the ArgEntry...

I fixed that, and changed the CommandInterpreter code to only preprocess the 
whole string for raw commands.  I think really that Raw commands should handle 
their own preprocessing, for instance I don't think doing backtick substitution 
in the expression passed to `expr` is really what people expect...

When I did that a bunch of tests failed because we were not correctly 
preserving the backticks around arguments when we parsed them into a temporary 
argv array and then back out again, and because we didn't take them into 
account when stripping elements out of the incoming command when handling 
command aliases.

I also added a test that  explicitly tests command aliases with embedded 
backticks.  The test has a succeeding case and a failing case.  The latter 
shows an extant bug this patch doesn't fix: the CommandAlias::Desugar doesn't 
correctly handle the case where you alias an alias and the second alias fills 
in slots of the first alias.  It just pastes the second aliases options after 
the first aliases and then tries to evaluate them, which ends up with unfilled 
slots.  As I say, that's an extant bug so the test for it fails now.  I'll fix 
that one next chance I have to mess with this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133045

Files:
  lldb/include/lldb/Interpreter/CommandInterpreter.h
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/source/Interpreter/CommandAlias.cpp
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Interpreter/CommandObject.cpp
  lldb/source/Interpreter/Options.cpp
  lldb/source/Utility/Args.cpp
  lldb/test/API/commands/command/backticks/Makefile
  lldb/test/API/commands/command/backticks/TestBackticksInAlias.py
  lldb/test/API/commands/command/backticks/main.c

Index: lldb/test/API/commands/command/backticks/main.c
===
--- /dev/null
+++ lldb/test/API/commands/command/backticks/main.c
@@ -0,0 +1,5 @@
+int main (int argc, char const *argv[])
+{
+  return argc; // break here
+}
+
Index: lldb/test/API/commands/command/backticks/TestBackticksInAlias.py
===
--- /dev/null
+++ lldb/test/API/commands/command/backticks/TestBackticksInAlias.py
@@ -0,0 +1,32 @@
+"""
+Test that an alias can contain active backticks
+"""
+
+
+
+import lldb
+from lldbsuite.test.lldbtest import *
+import lldbsuite.test.lldbutil as lldbutil
+
+
+class TestBackticksInAlias(TestBase):
+NO_DEBUG_INFO_TESTCASE = True
+
+def test_backticks_in_alias(self):
+"""Test that an alias can contain active backtraces."""
+self.build()
+(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self, "break here", lldb.SBFileSpec("main.c"))
+interp = self.dbg.GetCommandInterpreter();
+result = lldb.SBCommandReturnObject()
+interp.HandleCommand('command alias _test-argv-cmd expression -Z \`argc\` -- argv', result)
+self.assertCommandReturn(result, "Made the alias")
+interp.HandleCommand("_test-argv-cmd", result)
+self.assertCommandReturn(result, "The alias worked")
+
+# Now try a harder case where we create this using an alias:
+interp.HandleCommand('command alias _test-argv-parray-cmd parray \`argc\` argv', result)
+self.assertCommandReturn(result, "Made the alias")
+interp.HandleCommand("_test-argv-parray-cmd", result)
+self.assertFalse(result.Succeeded(), "CommandAlias::Desugar currently fails if a alias substitutes %N arguments in another alias")
+
+
Index: lldb/test/API/commands/command/backticks/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/command/backticks/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
Index: lldb/source/Utility/Args.cpp
===
--- lldb/source

[Lldb-commits] [PATCH] D133046: [lldb] Make the `rumtimes` target a test dependency

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: fdeazeve, jingham.
Herald added a subscriber: mgorny.
Herald added a project: All.
JDevlieghere requested review of this revision.

Make the `rumtimes` target a test dependency


https://reviews.llvm.org/D133046

Files:
  lldb/test/CMakeLists.txt


Index: lldb/test/CMakeLists.txt
===
--- lldb/test/CMakeLists.txt
+++ lldb/test/CMakeLists.txt
@@ -107,6 +107,10 @@
 add_lldb_test_dependency(compiler-rt)
   endif()
 
+  if (TARGET runtimes)
+add_lldb_test_dependency(runtimes)
+  endif()
+
   if(APPLE AND NOT LLVM_TARGET_IS_CROSSCOMPILE_HOST)
 # FIXME: Standalone builds should import the cxx target as well.
 if(LLDB_BUILT_STANDALONE)


Index: lldb/test/CMakeLists.txt
===
--- lldb/test/CMakeLists.txt
+++ lldb/test/CMakeLists.txt
@@ -107,6 +107,10 @@
 add_lldb_test_dependency(compiler-rt)
   endif()
 
+  if (TARGET runtimes)
+add_lldb_test_dependency(runtimes)
+  endif()
+
   if(APPLE AND NOT LLVM_TARGET_IS_CROSSCOMPILE_HOST)
 # FIXME: Standalone builds should import the cxx target as well.
 if(LLDB_BUILT_STANDALONE)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133046: [lldb] Make the `rumtimes` target a test dependency

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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

LGTM!


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

https://reviews.llvm.org/D133046

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

[Lldb-commits] [PATCH] D133045: Partial fix for handling backticks in commands and aliases

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

The other bit of this patch was a cleanup I made as I was understanding the 
alias parsing where we were using specific tokens in the parsed alias, but the 
tokens were ad hoc strings, so I replaced them with constants.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133045

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

[Lldb-commits] [PATCH] D133046: [lldb] Make the `rumtimes` target a test dependency

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.

This is needed or the parts of the lldb testsuite that rely on the libcxx & 
libcxxabi will fail when you do `ninja check-lldb`.  Thanks for fixing this.


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

https://reviews.llvm.org/D133046

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

[Lldb-commits] [lldb] f328922 - [lldb] Make the rumtimes target a test dependency

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-08-31T14:25:07-07:00
New Revision: f328922f55fe62a587a3ec0cd7253861cadba85e

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

LOG: [lldb] Make the rumtimes target a test dependency

Make the rumtimes target a test dependency. This is needed or the parts
of the test suite that rely on the libcxx and libcxxabi.

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

Added: 


Modified: 
lldb/test/CMakeLists.txt

Removed: 




diff  --git a/lldb/test/CMakeLists.txt b/lldb/test/CMakeLists.txt
index 938420aea18ea..e2a0443499899 100644
--- a/lldb/test/CMakeLists.txt
+++ b/lldb/test/CMakeLists.txt
@@ -107,6 +107,10 @@ if(TARGET clang)
 add_lldb_test_dependency(compiler-rt)
   endif()
 
+  if (TARGET runtimes)
+add_lldb_test_dependency(runtimes)
+  endif()
+
   if(APPLE AND NOT LLVM_TARGET_IS_CROSSCOMPILE_HOST)
 # FIXME: Standalone builds should import the cxx target as well.
 if(LLDB_BUILT_STANDALONE)



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

[Lldb-commits] [PATCH] D133046: [lldb] Make the `rumtimes` target a test dependency

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf328922f55fe: [lldb] Make the rumtimes target a test 
dependency (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133046

Files:
  lldb/test/CMakeLists.txt


Index: lldb/test/CMakeLists.txt
===
--- lldb/test/CMakeLists.txt
+++ lldb/test/CMakeLists.txt
@@ -107,6 +107,10 @@
 add_lldb_test_dependency(compiler-rt)
   endif()
 
+  if (TARGET runtimes)
+add_lldb_test_dependency(runtimes)
+  endif()
+
   if(APPLE AND NOT LLVM_TARGET_IS_CROSSCOMPILE_HOST)
 # FIXME: Standalone builds should import the cxx target as well.
 if(LLDB_BUILT_STANDALONE)


Index: lldb/test/CMakeLists.txt
===
--- lldb/test/CMakeLists.txt
+++ lldb/test/CMakeLists.txt
@@ -107,6 +107,10 @@
 add_lldb_test_dependency(compiler-rt)
   endif()
 
+  if (TARGET runtimes)
+add_lldb_test_dependency(runtimes)
+  endif()
+
   if(APPLE AND NOT LLVM_TARGET_IS_CROSSCOMPILE_HOST)
 # FIXME: Standalone builds should import the cxx target as well.
 if(LLDB_BUILT_STANDALONE)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133045: Partial fix for handling backticks in commands and aliases

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

Sorry if these comments are not helpful! Everything looks great!




Comment at: lldb/source/Interpreter/Options.cpp:1026
+} else
+  option_to_insert = CommandInterpreter::g_no_argument;
+

Question: Could we drop the final `else` if we initialized `option_to_insert` 
to `CommandInterpreter::g_no_argument`? Just curious. 



Comment at: lldb/test/API/commands/command/backticks/TestBackticksInAlias.py:16
+def test_backticks_in_alias(self):
+"""Test that an alias can contain active backtraces."""
+self.build()

Nit: Typo. backtraces => backticks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133045

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

[Lldb-commits] [PATCH] D133069: Fix inconsistent target arch when attaching to arm64 binaries on arm64e platforms.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl created this revision.
aprantl added reviewers: JDevlieghere, jasonmolenda.
Herald added subscribers: omjavaid, kristof.beyls.
Herald added a project: All.
aprantl requested review of this revision.

On arm64e-capable Apple platforms, the system libraries are always arm64e, but 
applications often are arm64. When a target is created from file, LLDB 
recognizes it as an arm64 target, but debugserver will still (technically 
correct) report the process as being arm64e. For consistency, set the target to 
arm64 here.

rdar://92248684


https://reviews.llvm.org/D133069

Files:
  lldb/include/lldb/Target/Target.h
  lldb/packages/Python/lldbsuite/test/gdbclientutils.py
  lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestDynamicLoaderDarwin.py

Index: lldb/test/API/functionalities/gdb_remote_client/TestDynamicLoaderDarwin.py
===
--- /dev/null
+++ lldb/test/API/functionalities/gdb_remote_client/TestDynamicLoaderDarwin.py
@@ -0,0 +1,143 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test.gdbclientutils import *
+from lldbsuite.test.lldbgdbclient import *
+
+images = """
+{"images":[
+{"load_address":4370792448,
+ "mod_date":0,
+ "pathname":"/usr/lib/dyld",
+ "uuid":"75627683-A780-32AD-AE34-CF86DD23A26B",
+ "min_version_os_name":"macosx",
+ "min_version_os_sdk":"12.5",
+ "mach_header":{
+ "magic":4277009103,
+ "cputype":16777228,
+ "cpusubtype":2,
+ "filetype":7,
+ "flags":133},
+"segments":[
+{"name":"__TEXT",
+ "vmaddr":0,
+ "vmsize":393216,
+ "fileoff":0,
+ "filesize":393216,
+ "maxprot":5},
+{"name":"__DATA_CONST",
+ "vmaddr":393216,
+ "vmsize":98304,
+ "fileoff":393216,
+ "filesize":98304,
+ "maxprot":3},
+{"name":"__DATA",
+ "vmaddr":491520,
+ "vmsize":16384,
+ "fileoff":491520,
+ "filesize":16384,
+ "maxprot":3},
+{"name":"__LINKEDIT",
+ "vmaddr":507904,
+ "vmsize":229376,
+ "fileoff":507904,
+ "filesize":227520,
+ "maxprot":1}
+]
+},
+{"load_address":4369842176,
+ "mod_date":0,
+ "pathname":"/tmp/a.out",
+ "uuid":"536A0A09-792A-377C-BEBA-FFB00A787C38",
+ "min_version_os_name":"macosx",
+ "min_version_os_sdk":"12.0",
+ "mach_header":{
+ "magic":4277009103,
+ "cputype":16777228,
+ "cpusubtype":%s,
+ "filetype":2,
+ "flags":2097285
+ },
+ "segments":[
+ {"name":"__PAGEZERO",
+  "vmaddr":0,
+  "vmsize":4294967296,
+  "fileoff":0,
+  "filesize":0,
+  "maxprot":0},
+ {"name":"__TEXT",
+  "vmaddr":4294967296,
+  "vmsize":16384,
+  "fileoff":0,
+  "filesize":16384,
+  "maxprot":5},
+ {"name":"__DATA_CONST",
+  "vmaddr":4294983680,
+  "vmsize":16384,
+  "fileoff":16384,
+  "filesize":16384,
+  "maxprot":3},
+ {"name":"__LINKEDIT",
+  "vmaddr":429564,
+  "vmsize":32768,
+  "fileoff":32768,
+  "filesize":19488,
+  "maxprot":1}]
+}
+]
+}
+"""
+
+arm64_binary = "cffaedfe0c0102001000e80285002000190048005f5f504147455a45524f01001900e8005f5f54455854014000400500050002005f5f746578745f5f54455854b03f01000800b03f020400805f5f756e77696e645f696e666f005f5f54455854b83f01004800b83f0200190048005f5f4c494e4b45444954004001400040b801010001003480104038003380100038403200180070400100804018000b00510001000e002c002f7573722f6c69622f64796c64001b001800a9981092eb3632f4afd9957e769160d9320021000c050c000100030633032a00100028801800b03f0c0038001800020001781f0501002f7573722f6c69622f6c696253797374656d2e422e64796c69622600100068400800290010007040

[Lldb-commits] [PATCH] D133069: Fix inconsistent target arch when attaching to arm64 binaries on arm64e platforms.

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/include/lldb/Target/Target.h:1012
   ///
+  /// \param[in] set_platform
+  /// If true, arch_spec is merged with the current

typo ?


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

https://reviews.llvm.org/D133069

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

[Lldb-commits] [lldb] c7511b4 - [lldb] Correctly add runtime test dependencies

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-08-31T16:37:35-07:00
New Revision: c7511b4ecf45c174c43d9177b843f15e2f3bd68b

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

LOG: [lldb] Correctly add runtime test dependencies

When a runtime is enabled through LLVM_ENABLE_RUNTIMES, it is loaded
after other projects (i.e. after LLDB). This means that the
corresponding targets don't exist when LLDB is configured. To support
runtimes being enable this way, we need to check if they're listed in
LLVM_ENABLE_RUNTIMES. For runtimes being enabled as projects (i.e.
through LLVM_ENABLE_RUNTIMES) we need to check for the target.

This patch unifies things and correctly adds compiler-rt and libcxx as
test dependencies in both scenarios.

Added: 


Modified: 
lldb/test/CMakeLists.txt

Removed: 




diff  --git a/lldb/test/CMakeLists.txt b/lldb/test/CMakeLists.txt
index e2a044349989..64a9082df52e 100644
--- a/lldb/test/CMakeLists.txt
+++ b/lldb/test/CMakeLists.txt
@@ -95,22 +95,16 @@ endif()
 if(TARGET clang)
   add_lldb_test_dependency(clang)
 
-  if(TARGET asan)
-add_lldb_test_dependency(asan)
-  endif()
-
-  if(TARGET tsan)
-add_lldb_test_dependency(tsan)
+  if (TARGET libcxx OR ("libcxx" IN_LIST LLVM_ENABLE_RUNTIMES))
+set(LLDB_HAS_LIBCXX ON)
+add_lldb_test_dependency(cxx)
   endif()
 
-  if (TARGET compiler-rt)
+  if (TARGET compiler-rt OR "compiler-rt" IN_LIST LLVM_ENABLE_RUNTIMES)
+set(LLDB_HAS_COMPILER_RT ON)
 add_lldb_test_dependency(compiler-rt)
   endif()
 
-  if (TARGET runtimes)
-add_lldb_test_dependency(runtimes)
-  endif()
-
   if(APPLE AND NOT LLVM_TARGET_IS_CROSSCOMPILE_HOST)
 # FIXME: Standalone builds should import the cxx target as well.
 if(LLDB_BUILT_STANDALONE)
@@ -122,7 +116,7 @@ if(TARGET clang)
 else()
   # We require libcxx for the test suite, so if we aren't building it,
   # provide a helpful error about how to resolve the situation.
-  if(NOT TARGET cxx AND NOT libcxx IN_LIST LLVM_ENABLE_RUNTIMES)
+  if(NOT LLDB_HAS_LIBCXX)
 message(FATAL_ERROR
   "LLDB test suite requires libc++, but it is currently disabled. "
   "Please add `libcxx` to `LLVM_ENABLE_RUNTIMES` or disable tests via "
@@ -130,15 +124,6 @@ if(TARGET clang)
   endif()
 endif()
   endif()
-
-  # Add libc++ dependency for libc++-specific tests. This is an optional
-  # dependency as it's also possible to run the libc++ tests against the libc++
-  # installed on the system.
-  if (TARGET cxx)
-set(LLDB_HAS_LIBCXX ON)
-add_lldb_test_dependency(cxx)
-  endif()
-
 endif()
 
 if (LLDB_BUILT_STANDALONE)



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

[Lldb-commits] [PATCH] D133069: Fix inconsistent target arch when attaching to arm64 binaries on arm64e platforms.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: 
lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp:584
+triple.setArchName("arm64");
+target_arch.SetTriple(triple);
+target.SetArchitecture(target_arch, /*set_platform=*/false,

Why can't you set the `target_arch` to 
`exe_module_sp->GetArchitecture().GetTriple()`? 


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

https://reviews.llvm.org/D133069

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

[Lldb-commits] [PATCH] D133075: Fix lldb-dotest when dotest_args_str is empty

[Jim Ingham via Phabricator via lldb-commits]

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

When - after substitution - the dotest_args_str is empty, then we were still 
inserting the blank string into the args.  Later on, a blank arg is taken to be 
a directory path and gets added to the CWD (which is generally in the build 
directory) and then none of the actual test directories match that, so we find 
no tests.

This patch fixes that by not adding blank arguments in lldb-dotest.

It could also be fixed by having the dotest implementation discard empty 
arguments, but I wasn't sure that was always correct, maybe you are somewhere 
in the test suite already and want to pass "" to dial up the current directory? 
 So I went with fixing it where we were getting it wrong instead.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133075

Files:
  lldb/utils/lldb-dotest/lldb-dotest.in


Index: lldb/utils/lldb-dotest/lldb-dotest.in
===
--- lldb/utils/lldb-dotest/lldb-dotest.in
+++ lldb/utils/lldb-dotest/lldb-dotest.in
@@ -20,7 +20,10 @@
 # Build dotest.py command.
 cmd = [sys.executable, dotest_path]
 cmd.extend(['--arch', arch])
-cmd.extend(dotest_args)
+# Don't stick empty args into the command, they will get interpreted as
+# test path directories and override the default searching.
+if len(dotest_args) > 0:
+cmd.extend(dotest_args)
 cmd.extend(['--build-dir', lldb_build_dir])
 cmd.extend(['--executable', executable])
 cmd.extend(['--compiler', compiler])


Index: lldb/utils/lldb-dotest/lldb-dotest.in
===
--- lldb/utils/lldb-dotest/lldb-dotest.in
+++ lldb/utils/lldb-dotest/lldb-dotest.in
@@ -20,7 +20,10 @@
 # Build dotest.py command.
 cmd = [sys.executable, dotest_path]
 cmd.extend(['--arch', arch])
-cmd.extend(dotest_args)
+# Don't stick empty args into the command, they will get interpreted as
+# test path directories and override the default searching.
+if len(dotest_args) > 0:
+cmd.extend(dotest_args)
 cmd.extend(['--build-dir', lldb_build_dir])
 cmd.extend(['--executable', executable])
 cmd.extend(['--compiler', compiler])
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133075: Fix lldb-dotest when dotest_args_str is empty

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Interesting, I would have expected `array.extend([])` be a noop?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133075

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

[Lldb-commits] [PATCH] D133075: Fix lldb-dotest when dotest_args_str is empty

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Or really, we split the string, resulting in an empty array, and apparently 
`cmd.extend([])` adds an empty argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133075

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

[Lldb-commits] [PATCH] D133075: Fix lldb-dotest when dotest_args_str is empty

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D133075#3762869 , @aprantl wrote:

> Interesting, I would have expected `array.extend([])` be a noop?

Me too.  Too bad the Python folks don't check with us more often...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133075

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

[Lldb-commits] [PATCH] D133075: Fix lldb-dotest when dotest_args_str is empty

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D133075#3762872 , @jingham wrote:

> In D133075#3762869 , @aprantl wrote:
>
>> Interesting, I would have expected `array.extend([])` be a noop?
>
> Me too.  Too bad the Python folks don't check with us more often...

Ah, it's not that, rather:

>>> string = ""
>>> arr = string.split(";")
>>> arr

['']


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133075

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

[Lldb-commits] [PATCH] D133075: Fix lldb-dotest when dotest_args_str is empty

[Jim Ingham via Phabricator via lldb-commits]

jingham updated this revision to Diff 457138.
jingham added a comment.

This one actually works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133075

Files:
  lldb/utils/lldb-dotest/lldb-dotest.in


Index: lldb/utils/lldb-dotest/lldb-dotest.in
===
--- lldb/utils/lldb-dotest/lldb-dotest.in
+++ lldb/utils/lldb-dotest/lldb-dotest.in
@@ -16,7 +16,11 @@
 
 if __name__ == '__main__':
 wrapper_args = sys.argv[1:]
-dotest_args = dotest_args_str.split(';')
+dotest_args = []
+# split on an empty string will produce [''] and if you
+# add that to the command, it will be treated as a directory...
+if len(dotest_args_str) > 0:
+dotest_args = dotest_args_str.split(';')
 # Build dotest.py command.
 cmd = [sys.executable, dotest_path]
 cmd.extend(['--arch', arch])


Index: lldb/utils/lldb-dotest/lldb-dotest.in
===
--- lldb/utils/lldb-dotest/lldb-dotest.in
+++ lldb/utils/lldb-dotest/lldb-dotest.in
@@ -16,7 +16,11 @@
 
 if __name__ == '__main__':
 wrapper_args = sys.argv[1:]
-dotest_args = dotest_args_str.split(';')
+dotest_args = []
+# split on an empty string will produce [''] and if you
+# add that to the command, it will be treated as a directory...
+if len(dotest_args_str) > 0:
+dotest_args = dotest_args_str.split(';')
 # Build dotest.py command.
 cmd = [sys.executable, dotest_path]
 cmd.extend(['--arch', arch])
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133075: Fix lldb-dotest when dotest_args_str is empty

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Actually, I confused myself because I still had the other fix in place.  
G...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133075

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

[Lldb-commits] [PATCH] D133075: Fix lldb-dotest when dotest_args_str is empty

[Adrian Prantl via Phabricator via lldb-commits]

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

Thanks, that makes more sense!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133075

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

[Lldb-commits] [lldb] a2670b9 - Fix a bug in lldb-dotest that was uncovered by setting no value for dotest_args_str.

[Jim Ingham via lldb-commits]

Author: Jim Ingham
Date: 2022-08-31T18:00:18-07:00
New Revision: a2670b92a2542a9bb889db81ba0cf5a21b3240ee

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

LOG: Fix a bug in lldb-dotest that was uncovered by setting no value for 
dotest_args_str.
We were splitting the string, and adding that array to cmd.  But split generated
[''] which shows up later on as an empty "test directory search path".  That got
extended to the CWD + "" which generally doesn't have any tests, so

lldb-dotest -p SomeRealTest.py

would fail with a no matching tests error.

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

Added: 


Modified: 
lldb/utils/lldb-dotest/lldb-dotest.in

Removed: 




diff  --git a/lldb/utils/lldb-dotest/lldb-dotest.in 
b/lldb/utils/lldb-dotest/lldb-dotest.in
index f6b5e0d01dcfa..896cc13913539 100755
--- a/lldb/utils/lldb-dotest/lldb-dotest.in
+++ b/lldb/utils/lldb-dotest/lldb-dotest.in
@@ -16,7 +16,11 @@ llvm_tools_dir = "@LLVM_TOOLS_DIR_CONFIGURED@"
 
 if __name__ == '__main__':
 wrapper_args = sys.argv[1:]
-dotest_args = dotest_args_str.split(';')
+dotest_args = []
+# split on an empty string will produce [''] and if you
+# add that to the command, it will be treated as a directory...
+if len(dotest_args_str) > 0:
+dotest_args = dotest_args_str.split(';')
 # Build dotest.py command.
 cmd = [sys.executable, dotest_path]
 cmd.extend(['--arch', arch])



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

[Lldb-commits] [PATCH] D133075: Fix lldb-dotest when dotest_args_str is empty

[Jim Ingham via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa2670b92a254: Fix a bug in lldb-dotest that was uncovered by 
setting no value for… (authored by jingham).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133075

Files:
  lldb/utils/lldb-dotest/lldb-dotest.in


Index: lldb/utils/lldb-dotest/lldb-dotest.in
===
--- lldb/utils/lldb-dotest/lldb-dotest.in
+++ lldb/utils/lldb-dotest/lldb-dotest.in
@@ -16,7 +16,11 @@
 
 if __name__ == '__main__':
 wrapper_args = sys.argv[1:]
-dotest_args = dotest_args_str.split(';')
+dotest_args = []
+# split on an empty string will produce [''] and if you
+# add that to the command, it will be treated as a directory...
+if len(dotest_args_str) > 0:
+dotest_args = dotest_args_str.split(';')
 # Build dotest.py command.
 cmd = [sys.executable, dotest_path]
 cmd.extend(['--arch', arch])


Index: lldb/utils/lldb-dotest/lldb-dotest.in
===
--- lldb/utils/lldb-dotest/lldb-dotest.in
+++ lldb/utils/lldb-dotest/lldb-dotest.in
@@ -16,7 +16,11 @@
 
 if __name__ == '__main__':
 wrapper_args = sys.argv[1:]
-dotest_args = dotest_args_str.split(';')
+dotest_args = []
+# split on an empty string will produce [''] and if you
+# add that to the command, it will be treated as a directory...
+if len(dotest_args_str) > 0:
+dotest_args = dotest_args_str.split(';')
 # Build dotest.py command.
 cmd = [sys.executable, dotest_path]
 cmd.extend(['--arch', arch])
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D133069: Fix inconsistent target arch when attaching to arm64 binaries on arm64e platforms.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: 
lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp:584
+triple.setArchName("arm64");
+target_arch.SetTriple(triple);
+target.SetArchitecture(target_arch, /*set_platform=*/false,

JDevlieghere wrote:
> Why can't you set the `target_arch` to 
> `exe_module_sp->GetArchitecture().GetTriple()`? 
That would be shorter. I was worried that could undo any mac catalyst versus 
macos fixups we did in DynamicLinkerDarwin before.


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

https://reviews.llvm.org/D133069

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

[Lldb-commits] [PATCH] D133069: Fix inconsistent target arch when attaching to arm64 binaries on arm64e platforms.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: 
lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp:584
+triple.setArchName("arm64");
+target_arch.SetTriple(triple);
+target.SetArchitecture(target_arch, /*set_platform=*/false,

aprantl wrote:
> JDevlieghere wrote:
> > Why can't you set the `target_arch` to 
> > `exe_module_sp->GetArchitecture().GetTriple()`? 
> That would be shorter. I was worried that could undo any mac catalyst versus 
> macos fixups we did in DynamicLinkerDarwin before.
Fair enough. Can we at least take the architecture from the triple then and 
pass that instead of hardcoding `arm64`? 


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

https://reviews.llvm.org/D133069

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