[Lldb-commits] [PATCH] D88792: [lldb/test] Catch invalid calls to expect()

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2440-2441
+assert "patterns must be a collection of strings" and False
+if isinstance(substrs, six.string_types):
+assert "substrs must be a collection of strings" and False
+

`assert isinstance(substrs, six.string_types), "substrs must be a collection of 
strings"` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88792

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

[Lldb-commits] [PATCH] D88795: [lldb] [test/Register] Attempt to fix x86-fp-read.test on Darwin

[Pavel Labath via Phabricator via lldb-commits]

labath added a subscriber: jasonmolenda.
labath added a comment.

BTW, what's the reason for darwin using the name `stmm`? It would be nice if 
the register names were consistent across a single architecture. We could keep 
the "stmm" variants as the "alternate" names on darwin for a while, to avoid 
breaking any scripts or so...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88795

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

[Lldb-commits] [PATCH] D82064: [ARM64] Add QEMU testing environment setup guide for SVE testing

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

Ping!

LGTM or any further comments or suggestions?

PS: I have been on holidays so apologies for late responses in past few week.


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

https://reviews.llvm.org/D82064

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

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

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 296106.
wallace added a comment.
Herald added a subscriber: dexonsmith.

Address comments:

- Created a basic ThreadTrace class instead of using HistoryThread. I'll modify 
this class later when I add the decoding functionality and refactor the json 
file parsing logic.
- Renamed offset to start_position, and added a clearer documentation about it. 
We'll have a current_position in each thread, and we'll print instructions in 
reverse order chronologically starting at current_position, unless 
start_position is specified. This matches gdb's implementation and works well 
with repeat commands, e.g. the user types "thread trace dump instructions" 
repeatedly showing a continous list of instructions from most recent to oldest.
- Now using DidAttach instead of LoadCore for setting the newly created 
procesess state to stopped.
- Moved the dump implementation from Target to Thread.
- Fixed the CanDebug function.
- Added a test for "thread trace dump instructions " and 
now using the long options instead of the short ones in the tests.

Some notes:

- I think that TraceSettingsParser should be used only when constructing a 
trace from a json file. In the case of a live process, we can skip it and 
directly create the trace objects accordingly. So the json parser can assume 
that the process is defunct.
- I haven't thought of making "thread trace dump" do anything. I'm thinking of 
having

thread trace info (for general information, including trace size, for example)
thread trace dump instructions
thread trace dump functions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88769

Files:
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Target/Thread.h
  lldb/include/lldb/Target/Trace.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Process/CMakeLists.txt
  lldb/source/Plugins/Process/Trace/CMakeLists.txt
  lldb/source/Plugins/Process/Trace/ProcessTrace.cpp
  lldb/source/Plugins/Process/Trace/ProcessTrace.h
  lldb/source/Plugins/Process/Trace/ThreadTrace.cpp
  lldb/source/Plugins/Process/Trace/ThreadTrace.h
  lldb/source/Target/Target.cpp
  lldb/source/Target/Thread.cpp
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceSettingsParser.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceLoad.py

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -35,6 +35,10 @@
 
 self.assertEqual("6AA9A4E2-6F28-2F33-377D-59FECE874C71-5B41261A", module.GetUUIDString())
 
+# check that the Process and Thread objects were created correctly
+self.expect("thread info", substrs=["tid = 3842849"])
+self.expect("thread list", substrs=["Process 1234 stopped", "tid = 3842849"])
+
 
 def testLoadInvalidTraces(self):
 src_dir = self.getSourceDir()
Index: lldb/test/API/commands/trace/TestTraceDumpInstructions.py
===
--- /dev/null
+++ lldb/test/API/commands/trace/TestTraceDumpInstructions.py
@@ -0,0 +1,56 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+
+class TestTraceDumpInstructions(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+def setUp(self):
+TestBase.setUp(self)
+if 'intel-pt' not in configuration.enabled_plugins:
+self.skipTest("The intel-pt test plugin is not enabled")
+
+def testErrorMessages(self):
+# We first check the output when there are no targets
+self.expect("thread trace dump instructions",
+substrs=["error: invalid target, create a target using the 'target create' command"],
+error=True)
+
+# We now check the output when there's a non-running target
+self.expect("target create " + os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
+
+self.expect("thread trace dump instructions",
+substrs=["error: invalid process"],
+error=True)
+
+# Now we check the output when there's a running target without a trace
+self.expect("b main")
+self.expect("run")
+
+self.expect("thread trace dump instructions",
+substrs=["error: no trace in this target"])
+
+def testDumpInstructions(self):
+self.expect("trace load -v " + os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"),
+substrs=["intel-pt"])
+
+self.expect("thread trace dump instructions",
+substrs=['thread #1: tid = 3842849',
+ 'would print 10 instructions from position 0'])
+
+# We c

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

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

@labath
I am just back from holiday and was hoping for Jason to give his advice in 
this. I am wondering how we can move forward on this.
I have added a some information from GDB inline.

In D82863#2254423 , @labath wrote:

> I'd like to also pull in Jason for this, since this is really the trickiest 
> part of the whole patchset.
>
> What this patch essentially does is bake in the knowledge of the arm sve 
> registers and their various configurations into the lldb client (and the 
> `DynamicRegisterInfo` class in particular). Before we go into the 
> implementation details, we should answer the question whether we are ok with 
> that.
>
> I am personally fine with that (if it gets encapsulated a bit better), 
> because we can't avoid knowing about the registers of some architecture for 
> various other reasons (core file debugging, eh_frame parsing, stubs which 
> don't support qRegisterInfo, etc.). In fact, I'd be very happy if this got us 
> closer towards a state where the stub does not need to send eh_frame and 
> other register numbers over. Also, hardcoding may be the only way to get 
> reasonable performance out of this, since a fully generic solution of asking 
> about the register information after every stop would likely be prohibitively 
> slow (well.. I suppose we could come up with some way of letting the stub to 
> notify us when the register configuration has changed).
>
> I believe Jason is also ok some hardcoding, in principle. However,
> a) I don't want to speak for him,
> b) there's still a lot of details to be worked out.
>
> For example, one of the ideas floated around previously was based on the 
> client and server communicating 4 things about each register:
>
> - name
> - the register number (for `p` packets, etc.)
> - size
> - offset (for e.g. `g` packet)
>
> The register name and number are fine, but the other two things are sort of 
> variable with SVE. The register size was mainly there as a cross-check, and 
> so we could conceivably just drop it from the list (or use the max value or 
> some other placeholder for sve registers in particular). But that still 
> leaves the question of the `g` offsets...

There are two different things we are looking at in this scenario:

1. Exchange of register information via RegisterInfo or XML packet at inferior 
creation
2. Register size/offset update mechanism during execution on every stop.

Reaching an optimal solution dropping extra information in both above cases is 
quite a challenge. I have been thinking about ways to drop offset and only way 
right now is to make LLDB register numbers act as sequence numbers and offset 
can be calculated based on the size of next in line register and previous value 
of offset. However challenge in this case (we discussed before during review of 
an earilier patch) was dynamic selection of register sets. Some register sets 
may be active some may not be active and it will make things quite tricky.
If we look at the eh_frame, dwarf reg numbers implementation in GDB its a 
similar mess there as well although its not kept inside register information 
but its being dealt with hardcoded in various areas of code. GDB also supports 
complex register type description as part of target XML which luckily we are 
not doing at the moment.

> It might be interesting data point to look at how the sve registers are 
> described in gdb's target.xml descriptions.

GDB uses dynamically generated target XMLs now which were previously static. 
Target XML is generated based on the value of scale, which is no of chunks of 
128 Bits in a Z register.
https://sourceware.org/git?p=binutils-gdb.git;a=blob;f=gdb/features/aarch64-sve.c;h=e4361b07a4d47282c5515846896638cc4caf97f5;hb=HEAD#l25

Furthermore, GDB keeps a list of target descriptions for every combination of 
(vq, pauth) where pauth is flag telling if pointer authentication feature is 
supported while vq is vector length quad (multiples of 128bits). 
https://sourceware.org/git?p=binutils-gdb.git;a=blob;f=gdb/aarch64-tdep.c;h=cbc7038dbba9472a12a3ae927bbb0937b10b2bdd;hb=HEAD#l3284

On gdbserver side similar to what is proposed for LLDB, GDB also exchanges 
vector length information using expedited registers list and updates target 
descriptions based on vg (vector granule register) value.
https://sourceware.org/git?p=binutils-gdb.git;a=blob;f=gdbserver/linux-aarch64-tdesc.cc;h=897fbb43bd28ddf44c69d4162dda43c2589b060f;hb=HEAD#l35


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

https://reviews.llvm.org/D82863

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

[Lldb-commits] [PATCH] D82064: [ARM64] Add QEMU testing environment setup guide for SVE testing

[Pavel Labath via Phabricator via lldb-commits]

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

This looks fine to me. Please wait a bit to give Jonas the opportunity to 
comment. (FWIW, I think these are fine as shell scripts right now, but that may 
change if windows comes into play.)


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

https://reviews.llvm.org/D82064

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

[Lldb-commits] [PATCH] D88792: [lldb/test] Catch invalid calls to expect()

[Raphael Isemann via Phabricator via lldb-commits]

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

Thanks for fixing all of this! Beside the assertion this LGTM.




Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2440-2441
+assert "patterns must be a collection of strings" and False
+if isinstance(substrs, six.string_types):
+assert "substrs must be a collection of strings" and False
+

labath wrote:
> `assert isinstance(substrs, six.string_types), "substrs must be a collection 
> of strings"` ?
Or we could do `assertNotIsInstance` for a better error message that should 
show the actual type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88792

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

[Lldb-commits] [PATCH] D88796: [lldb] Initial version of FreeBSD remote process plugin

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Looks pretty straigh-forward. I'm getting a lot of deja-vu when looking at this 
code, though it's not clear to me whether its identical of just similar. Have 
you investigated the possibility of factoring some parts of this && the netbsd 
code (and maybe linux too) into some common place?




Comment at: lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp:269-375
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PLATFORM));
+  LLDB_LOG(log, "target {0}", target);
+
+  // If we're a remote host, use standard behavior from parent class.
+  if (!IsHost()) {
+printf("pare\n");
+return PlatformPOSIX::DebugProcess(launch_info, debugger, target, error);

I think it's time for a switcheroo -- move this code into 
PlatformPOSIX::DebugProcess, and move that function into PlatformDarwin.



Comment at: lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp:274
+  if (!IsHost()) {
+printf("pare\n");
+return PlatformPOSIX::DebugProcess(launch_info, debugger, target, error);

?



Comment at: lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp:82
 void ProcessFreeBSD::Initialize() {
-  static llvm::once_flag g_once_flag;
+  if (!getenv("FREEBSD_REMOTE_PLUGIN")) {
+static llvm::once_flag g_once_flag;

I would expect that the check above is sufficient, is it not?



Comment at: 
lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp:22
+
+//#include "Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD.h"
+#include "Plugins/Process/POSIX/ProcessPOSIXLog.h"

krytarowski wrote:
> mgorny wrote:
> > krytarowski wrote:
> > > Why this line?
> > Because otherwise it fails to compile? ;-)
> OK, you have removed the unused include.
Bonus cookies for anyone who makes it possible to remove that line.



Comment at: 
lldb/source/Plugins/Process/FreeBSDRemote/NativeThreadFreeBSD.cpp:9-19
+#include "NativeThreadFreeBSD.h"
+#include "NativeRegisterContextFreeBSD.h"
+
+#include "NativeProcessFreeBSD.h"
+
+#include "Plugins/Process/POSIX/CrashReason.h"
+#include "Plugins/Process/POSIX/ProcessPOSIXLog.h"

This grouping is very random. Normal llvm style is to just put all includes in 
one block and let clang-format sort that out.


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

https://reviews.llvm.org/D88796

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

[Lldb-commits] [PATCH] D88229: Reland "[lldb] Don't send invalid region addresses to lldb server"

[David Spickett via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG71cf97e95b8c: Reland "[lldb] Don't send invalid 
region addresses to lldb server" (authored by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88229

Files:
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
  lldb/test/API/functionalities/memory-region/TestMemoryRegion.py

Index: lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
===
--- lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
+++ lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
@@ -41,6 +41,12 @@
 self.assertFalse(result.Succeeded())
 self.assertRegexpMatches(result.GetError(), "Usage: memory region ADDR")
 
+# Test that when the address fails to parse, we show an error and do not continue
+interp.HandleCommand("memory region not_an_address", result)
+self.assertFalse(result.Succeeded())
+self.assertEqual(result.GetError(),
+"error: invalid address argument \"not_an_address\": address expression \"not_an_address\" evaluation failed\n")
+
 # Now let's print the memory region starting at 0 which should always work.
 interp.HandleCommand("memory region 0x0", result)
 self.assertTrue(result.Succeeded())
Index: lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
===
--- lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
+++ lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
@@ -405,7 +405,8 @@
   MEMORY_BASIC_INFORMATION mem_info = {};
   SIZE_T result = ::VirtualQueryEx(handle, addr, &mem_info, sizeof(mem_info));
   if (result == 0) {
-if (::GetLastError() == ERROR_INVALID_PARAMETER) {
+DWORD last_error = ::GetLastError();
+if (last_error == ERROR_INVALID_PARAMETER) {
   // ERROR_INVALID_PARAMETER is returned if VirtualQueryEx is called with
   // an address past the highest accessible address. We should return a
   // range from the vm_addr to LLDB_INVALID_ADDRESS
@@ -417,7 +418,7 @@
   info.SetMapped(MemoryRegionInfo::eNo);
   return error;
 } else {
-  error.SetError(::GetLastError(), eErrorTypeWin32);
+  error.SetError(last_error, eErrorTypeWin32);
   LLDB_LOG(log,
"VirtualQueryEx returned error {0} while getting memory "
"region info for address {1:x}",
@@ -460,7 +461,6 @@
 info.SetMapped(MemoryRegionInfo::eNo);
   }
 
-  error.SetError(::GetLastError(), eErrorTypeWin32);
   LLDB_LOGV(log,
 "Memory region info for address {0}: readable={1}, "
 "executable={2}, writable={3}",
Index: lldb/source/Commands/CommandObjectMemory.cpp
===
--- lldb/source/Commands/CommandObjectMemory.cpp
+++ lldb/source/Commands/CommandObjectMemory.cpp
@@ -1687,63 +1687,66 @@
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
 ProcessSP process_sp = m_exe_ctx.GetProcessSP();
-if (process_sp) {
-  Status error;
-  lldb::addr_t load_addr = m_prev_end_addr;
+if (!process_sp) {
   m_prev_end_addr = LLDB_INVALID_ADDRESS;
+  result.AppendError("invalid process");
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}
+
+Status error;
+lldb::addr_t load_addr = m_prev_end_addr;
+m_prev_end_addr = LLDB_INVALID_ADDRESS;
 
-  const size_t argc = command.GetArgumentCount();
-  if (argc > 1 || (argc == 0 && load_addr == LLDB_INVALID_ADDRESS)) {
-result.AppendErrorWithFormat("'%s' takes one argument:\nUsage: %s\n",
- m_cmd_name.c_str(), m_cmd_syntax.c_str());
+const size_t argc = command.GetArgumentCount();
+if (argc > 1 || (argc == 0 && load_addr == LLDB_INVALID_ADDRESS)) {
+  result.AppendErrorWithFormat("'%s' takes one argument:\nUsage: %s\n",
+   m_cmd_name.c_str(), m_cmd_syntax.c_str());
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}
+
+if (argc == 1) {
+  auto load_addr_str = command[0].ref();
+  load_addr = OptionArgParser::ToAddress(&m_exe_ctx, load_addr_str,
+ LLDB_INVALID_ADDRESS, &error);
+  if (error.Fail() || load_addr == LLDB_INVALID_ADDRESS) {
+result.AppendErrorWithFormat("invalid address argument \"%s\": %s\n",
+ command[0].c_str(), error.AsCString());
 result.SetStatus(eReturnStatusFailed);
-  } else {
-if (command.GetArgumentCount() == 1) {
-  auto load_addr_str =

[Lldb-commits] [lldb] 71cf97e - Reland "[lldb] Don't send invalid region addresses to lldb server"

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2020-10-05T11:50:29+01:00
New Revision: 71cf97e95b8c888367284d1d12925f79b38034eb

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

LOG: Reland "[lldb] Don't send invalid region addresses to lldb server"

This reverts commit c65627a1fe3be7521fc232d633bb6df577f55269.

The test immediately after the new invalid symbol test was
failing on Windows. This was because when we called
VirtualQueryEx to get the region info for 0x0,
even if it succeeded we would call GetLastError.

Which must have picked up the last error that was set while
trying to lookup "not_an_address". Which happened to be 2.
("The system cannot find the file specified.")

To fix this only call GetLastError when we know VirtualQueryEx
has failed. (when it returns 0, which we were also checking for anyway)

Also convert memory region to an early return style
to make the logic clearer.

Reviewed By: labath, stella.stamenova

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

Added: 


Modified: 
lldb/source/Commands/CommandObjectMemory.cpp
lldb/source/Plugins/Process/Windows/Common/ProcessDebugger.cpp
lldb/test/API/functionalities/memory-region/TestMemoryRegion.py

Removed: 




diff  --git a/lldb/source/Commands/CommandObjectMemory.cpp 
b/lldb/source/Commands/CommandObjectMemory.cpp
index 474c37710149..d4c2808dc159 100644
--- a/lldb/source/Commands/CommandObjectMemory.cpp
+++ b/lldb/source/Commands/CommandObjectMemory.cpp
@@ -1687,63 +1687,66 @@ class CommandObjectMemoryRegion : public 
CommandObjectParsed {
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
 ProcessSP process_sp = m_exe_ctx.GetProcessSP();
-if (process_sp) {
-  Status error;
-  lldb::addr_t load_addr = m_prev_end_addr;
+if (!process_sp) {
   m_prev_end_addr = LLDB_INVALID_ADDRESS;
+  result.AppendError("invalid process");
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}
+
+Status error;
+lldb::addr_t load_addr = m_prev_end_addr;
+m_prev_end_addr = LLDB_INVALID_ADDRESS;
 
-  const size_t argc = command.GetArgumentCount();
-  if (argc > 1 || (argc == 0 && load_addr == LLDB_INVALID_ADDRESS)) {
-result.AppendErrorWithFormat("'%s' takes one argument:\nUsage: %s\n",
- m_cmd_name.c_str(), m_cmd_syntax.c_str());
+const size_t argc = command.GetArgumentCount();
+if (argc > 1 || (argc == 0 && load_addr == LLDB_INVALID_ADDRESS)) {
+  result.AppendErrorWithFormat("'%s' takes one argument:\nUsage: %s\n",
+   m_cmd_name.c_str(), m_cmd_syntax.c_str());
+  result.SetStatus(eReturnStatusFailed);
+  return false;
+}
+
+if (argc == 1) {
+  auto load_addr_str = command[0].ref();
+  load_addr = OptionArgParser::ToAddress(&m_exe_ctx, load_addr_str,
+ LLDB_INVALID_ADDRESS, &error);
+  if (error.Fail() || load_addr == LLDB_INVALID_ADDRESS) {
+result.AppendErrorWithFormat("invalid address argument \"%s\": %s\n",
+ command[0].c_str(), error.AsCString());
 result.SetStatus(eReturnStatusFailed);
-  } else {
-if (command.GetArgumentCount() == 1) {
-  auto load_addr_str = command[0].ref();
-  load_addr = OptionArgParser::ToAddress(&m_exe_ctx, load_addr_str,
- LLDB_INVALID_ADDRESS, &error);
-  if (error.Fail() || load_addr == LLDB_INVALID_ADDRESS) {
-result.AppendErrorWithFormat(
-"invalid address argument \"%s\": %s\n", command[0].c_str(),
-error.AsCString());
-result.SetStatus(eReturnStatusFailed);
-  }
-}
+return false;
+  }
+}
 
-lldb_private::MemoryRegionInfo range_info;
-error = process_sp->GetMemoryRegionInfo(load_addr, range_info);
-if (error.Success()) {
-  lldb_private::Address addr;
-  ConstString name = range_info.GetName();
-  ConstString section_name;
-  if (process_sp->GetTarget().ResolveLoadAddress(load_addr, addr)) {
-SectionSP section_sp(addr.GetSection());
-if (section_sp) {
-  // Got the top most section, not the deepest section
-  while (section_sp->GetParent())
-section_sp = section_sp->GetParent();
-  section_name = section_sp->GetName();
-}
-  }
-  result.AppendMessageWithFormatv(
-  "[{0:x16}-{1:x16}) {2:r}{3:w}{4:x}{5}{6}{7}{8}\n",
-  range_info.GetRange().GetRangeBase(),
-  range_info.GetRange().GetRangeEnd(), range_info.GetReadable(),
-  

[Lldb-commits] [PATCH] D88792: [lldb/test] Catch invalid calls to expect()

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

Does it make sense to add a few tests for this to 
`lldb/test/API/assert_messages_test/TestAssertMessages.py`? (this is only 
checking expect() messages, despite the generic name)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88792

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

[Lldb-commits] [PATCH] D88753: Fix raciness in the check for whether a stop hook has run the target

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

This explains the things I saw in the log file. Thanks for fixing it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88753

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

[Lldb-commits] [PATCH] D88728: [lldb] Check for and use ptsname_r if available

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Host/common/PseudoTerminal.cpp:149
+  int r = ptsname_r(m_primary_fd, buf, sizeof(buf));
+  assert(r == 0);
+  return buf;

MaskRay wrote:
> MaskRay wrote:
> > labath wrote:
> > > mgorny wrote:
> > > > labath wrote:
> > > > > mgorny wrote:
> > > > > > I would really feel better with a real error handling here. It 
> > > > > > shouldn't be hard to use `ErrorOr` here.
> > > > > Yeah, but what are you going to do with that value? Pass it to the 
> > > > > caller? The existing callers are ignoring the error return anyway, 
> > > > > and I don't want to add error handling everywhere as afaict, this 
> > > > > function can't fail unless the user messes up the master state (which 
> > > > > is not something I want to support).
> > > > I get your point but I've literally wasted days because of missing 
> > > > error handling, so I'd really preferred if we wouldn't make it even 
> > > > worse. Though I guess `assert` is good enough.
> > > In some ways it's even better because it will point you straight to the 
> > > place where the assumption is violated, whereas a propagated logic error 
> > > can manifest itself much farther away (or not at all). :)
> > If `ptsname/ptsname_r` fails, buf will be uninitialized and trigger a 
> > use-of-uninitialized-value error.
> ... in a -DLLVM_ENABLE_ASSERTIONS=off build.
> 
> This probably still needs some protection.
What kind of protection did you have it mind? Initialize the buffer to an empty 
string?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88728

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

[Lldb-commits] [PATCH] D88796: [lldb] Initial version of FreeBSD remote process plugin

[Michał Górny via Phabricator via lldb-commits]

mgorny marked an inline comment as done.
mgorny added a comment.

Indeed I've been wondering how we could dedupe this. At least large part of hw 
watchpoint handling should be reusable.




Comment at: lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp:274
+  if (!IsHost()) {
+printf("pare\n");
+return PlatformPOSIX::DebugProcess(launch_info, debugger, target, error);

labath wrote:
> ?
Nów everyone knows I've been doing printf debugging! 



Comment at: lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp:82
 void ProcessFreeBSD::Initialize() {
-  static llvm::once_flag g_once_flag;
+  if (!getenv("FREEBSD_REMOTE_PLUGIN")) {
+static llvm::once_flag g_once_flag;

labath wrote:
> I would expect that the check above is sufficient, is it not?
Could you be more specific? I'm pretty sure I had to do this or otherwise the 
old plugin ended up being used. This is also how windows does it. 


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

https://reviews.llvm.org/D88796

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

[Lldb-commits] [PATCH] D88796: [lldb] Initial version of FreeBSD remote process plugin

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

Also a potentially interesting concept would be to unify reg constants between 
platforms.


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

https://reviews.llvm.org/D88796

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

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Before going into the code details, could you give a high-level overview of the 
intended interactions between the Trace and ProcessTrace classes?

I am thinking about how to organize this so that we don't have too many 
criss-cross dependencies. My main question is whether it's expected that each 
"trace" plugin will also create its own ProcessTrace subclass.




Comment at: lldb/source/Target/TraceSettingsParser.cpp:13
 
-#include "Plugins/Process/Utility/HistoryThread.h"
+#include "Plugins/Process/Trace/ThreadTrace.h"
 #include "lldb/Core/Debugger.h"

This is a layering violation. We shouldn't have non-plugin code depend on 
plugin code. (I know there are some existing usages like that, but let's not 
add new ones -- people have been working for a long time to remove these.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88769

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

[Lldb-commits] [PATCH] D88796: [lldb] Initial version of FreeBSD remote process plugin

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

BTW, are you running the lldb-server test suite on this? How's it shaping up?

In D88796#2311594 , @mgorny wrote:

> Also a potentially interesting concept would be to unify reg constants 
> between platforms.

Which constants do you have in mind?




Comment at: lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp:82
 void ProcessFreeBSD::Initialize() {
-  static llvm::once_flag g_once_flag;
+  if (!getenv("FREEBSD_REMOTE_PLUGIN")) {
+static llvm::once_flag g_once_flag;

mgorny wrote:
> labath wrote:
> > I would expect that the check above is sufficient, is it not?
> Could you be more specific? I'm pretty sure I had to do this or otherwise the 
> old plugin ended up being used. This is also how windows does it. 
Hmm... interesting. But windows does not have the check in PlatformWindows. 
Does that mean that check is redundant? I find it strange that we need to check 
this at two places..


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

https://reviews.llvm.org/D88796

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

[Lldb-commits] [PATCH] D87442: [lldb] Show flags for memory regions

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> The thing I'm not so sure about is the verbatim passing of the os-specific 
> flags. That's nice for the purpose of displaying them to the user, but it can 
> make things messy for programatic use, particularly if multiple platforms 
> start using these. What's do you intend to do with these flags? Like how many 
> of them are you going to actually use, and for what purpose?

At this point I only need to get the "mt" flag that shows whether a region has 
memory tagging enabled. This will be used by lldb internally and anyone 
checking for memory tagging with "memory region".

Some background on the design...

With the current implementation:

- We don't have to translate the flags per OS to show them.
- Has() needs to check for OS specific names, which balances things out. 
(you've got an OS specific switch either way)
- The gdb protocol packet just contains the names as strings.
- OSes that don't present flags as strings directly in their API have to 
provide string versions.

At first I did have a generically named bitset of flags internally. This means 
that:

- In the "memory region" output the user can't tell the difference between an 
unrecognised flag, or a flag that's not enabled. Since we'd need to update lldb 
to handle the new flag. (a rare occurrence now that I think about it)
- When "memory region" prints we'd need to translate those flags to OS specific 
names, or use long generic names like "memory tagging: enabled". (I prefer the 
former so you could compare it directly to the native tools)
- A region object's Has() is the same code regardless of OS.
- The gdb protocol packet would contain generic versions of the flag names.

Now that I wrote that the second idea does look cleaner. New flags are going to 
be infrequent and internal calls won't have to care about what OS they're on to 
query flags.

Is that the direction you were thinking of? If so I'll rework this to do that 
instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87442

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

[Lldb-commits] [PATCH] D88796: [lldb] Initial version of FreeBSD remote process plugin

[Kamil Rytarowski via Phabricator via lldb-commits]

krytarowski added inline comments.



Comment at: lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp:269-375
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PLATFORM));
+  LLDB_LOG(log, "target {0}", target);
+
+  // If we're a remote host, use standard behavior from parent class.
+  if (!IsHost()) {
+printf("pare\n");
+return PlatformPOSIX::DebugProcess(launch_info, debugger, target, error);

labath wrote:
> I think it's time for a switcheroo -- move this code into 
> PlatformPOSIX::DebugProcess, and move that function into PlatformDarwin.
Please defer any refactoring to a separate, follow up commit. Here is a lot of 
room for code deduplication, at least for ELF platforms.



Comment at: 
lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp:491
+
+  len = len * 4 / 3;
+  std::unique_ptr buf =

Assuming that the process is always stopped, we don't need to increment `len`.


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

https://reviews.llvm.org/D88796

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

[Lldb-commits] [PATCH] D88792: [lldb/test] Catch invalid calls to expect()

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Oh wow — that paid off!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88792

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

[Lldb-commits] [PATCH] D88792: [lldb/test] Catch invalid calls to expect()

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/test/API/types/TestRecursiveTypes.py:54
+self.expect("print tpi")
+self.expect("print *tpi")

I'm not not sure I understand this change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88792

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

[Lldb-commits] [PATCH] D88792: [lldb/test] Catch invalid calls to expect()

[Dave Lee via Phabricator via lldb-commits]

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

Resyntax the isinstance asserts; Add expect() tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88792

Files:
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/test/API/assert_messages_test/TestAssertMessages.py
  lldb/test/API/commands/frame/diagnose/bad-reference/TestBadReference.py
  
lldb/test/API/commands/frame/diagnose/complicated-expression/TestComplicatedExpression.py
  
lldb/test/API/commands/frame/diagnose/dereference-argument/TestDiagnoseDereferenceArgument.py
  
lldb/test/API/commands/frame/diagnose/dereference-this/TestDiagnoseDereferenceThis.py
  lldb/test/API/commands/frame/diagnose/inheritance/TestDiagnoseInheritance.py
  lldb/test/API/commands/frame/diagnose/local-variable/TestLocalVariable.py
  
lldb/test/API/commands/frame/diagnose/virtual-method-call/TestDiagnoseDereferenceVirtualMethodCall.py
  lldb/test/API/commands/settings/TestSettings.py
  lldb/test/API/driver/batch_mode/TestBatchMode.py
  
lldb/test/API/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-objc/cmtime/TestDataFormatterCMTime.py
  lldb/test/API/lang/cpp/constructors/TestCppConstructors.py
  lldb/test/API/macosx/macCatalyst/TestMacCatalyst.py
  lldb/test/API/types/TestRecursiveTypes.py

Index: lldb/test/API/types/TestRecursiveTypes.py
===
--- lldb/test/API/types/TestRecursiveTypes.py
+++ lldb/test/API/types/TestRecursiveTypes.py
@@ -50,5 +50,5 @@
 
 self.runCmd("run", RUN_SUCCEEDED)
 
-self.expect("print tpi", RUN_SUCCEEDED)
-self.expect("print *tpi", RUN_SUCCEEDED)
+self.expect("print tpi")
+self.expect("print *tpi")
Index: lldb/test/API/macosx/macCatalyst/TestMacCatalyst.py
===
--- lldb/test/API/macosx/macCatalyst/TestMacCatalyst.py
+++ lldb/test/API/macosx/macCatalyst/TestMacCatalyst.py
@@ -24,8 +24,8 @@
 self.expect("image list -t -b",
 patterns=[self.getArchitecture() +
   r'.*-apple-ios.*-macabi a\.out'])
-self.expect("fr v s", "Hello macCatalyst")
-self.expect("p s", "Hello macCatalyst")
+self.expect("fr v s", substrs=["Hello macCatalyst"])
+self.expect("p s", substrs=["Hello macCatalyst"])
 self.check_debugserver(log)
 
 def check_debugserver(self, log):
Index: lldb/test/API/lang/cpp/constructors/TestCppConstructors.py
===
--- lldb/test/API/lang/cpp/constructors/TestCppConstructors.py
+++ lldb/test/API/lang/cpp/constructors/TestCppConstructors.py
@@ -19,7 +19,7 @@
 self.expect_expr("ClassWithDeletedDefaultCtor(7).value", result_type="int", result_value="7")
 
 # FIXME: It seems we try to call the non-existent default constructor here which is wrong.
-self.expect("expr ClassWithDefaultedCtor().foo()", error=True, substrs="Couldn't lookup symbols:")
+self.expect("expr ClassWithDefaultedCtor().foo()", error=True, substrs=["Couldn't lookup symbols:"])
 
 # FIXME: Calling deleted constructors should fail before linking.
 self.expect("expr ClassWithDeletedCtor(1).value", error=True, substrs=["Couldn't lookup symbols:"])
Index: lldb/test/API/functionalities/data-formatter/data-formatter-objc/cmtime/TestDataFormatterCMTime.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-objc/cmtime/TestDataFormatterCMTime.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-objc/cmtime/TestDataFormatterCMTime.py
@@ -48,6 +48,6 @@
 self.expect(
 'frame variable t4',
 substrs=['10 seconds', 'value = 10', 'timescale = 1', 'epoch = 0'])
-self.expect('frame variable t5', '-oo')
-self.expect('frame variable t6', '+oo')
-self.expect('frame variable t7', 'indefinite')
+self.expect('frame variable t5', substrs=['+oo'])
+self.expect('frame variable t6', substrs=['-oo'])
+self.expect('frame variable t7', substrs=['indefinite'])
Index: lldb/test/API/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py
===
--- lldb/test/API/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py
+++ lldb/test/API/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py
@@ -21,7 +21,7 @@
 main_c = lldb.SBFileSpec("main.c")
 _, _, _, breakpoint = lldbutil.run_to_line_breakpoint(self,
   main_c, 11, 50)
- 

[Lldb-commits] [PATCH] D88792: [lldb/test] Catch invalid calls to expect()

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2440-2441
+assert "patterns must be a collection of strings" and False
+if isinstance(substrs, six.string_types):
+assert "substrs must be a collection of strings" and False
+

teemperor wrote:
> labath wrote:
> > `assert isinstance(substrs, six.string_types), "substrs must be a 
> > collection of strings"` ?
> Or we could do `assertNotIsInstance` for a better error message that should 
> show the actual type.
It turns out that in practice `assertNotIsInstance` isn't a good match for this 
case because `six.string_types` is a tuple, and the formatted string becomes 
`"… is an instance of (,)"`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88792

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

[Lldb-commits] [PATCH] D88792: [lldb/test] Catch invalid calls to expect()

[Dave Lee via Phabricator via lldb-commits]

kastiglione added inline comments.



Comment at: lldb/test/API/types/TestRecursiveTypes.py:54
+self.expect("print tpi")
+self.expect("print *tpi")

aprantl wrote:
> I'm not not sure I understand this change?
The second parameter, `msg`, is only valid if there are other "matchers", like 
`substrs` or `startstr`. If not, then the `msg` isn't used and so shouldn't 
need to be passed. By preventing a `msg` here, it allows the precondition to 
distinguish between cases where the `msg` is valid, and where it's not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88792

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

[Lldb-commits] [PATCH] D88792: [lldb/test] Catch invalid calls to expect()

[Dave Lee via Phabricator via lldb-commits]

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

"missing a matcher" -> "missing a matcher argument"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88792

Files:
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/test/API/assert_messages_test/TestAssertMessages.py
  lldb/test/API/commands/frame/diagnose/bad-reference/TestBadReference.py
  
lldb/test/API/commands/frame/diagnose/complicated-expression/TestComplicatedExpression.py
  
lldb/test/API/commands/frame/diagnose/dereference-argument/TestDiagnoseDereferenceArgument.py
  
lldb/test/API/commands/frame/diagnose/dereference-this/TestDiagnoseDereferenceThis.py
  lldb/test/API/commands/frame/diagnose/inheritance/TestDiagnoseInheritance.py
  lldb/test/API/commands/frame/diagnose/local-variable/TestLocalVariable.py
  
lldb/test/API/commands/frame/diagnose/virtual-method-call/TestDiagnoseDereferenceVirtualMethodCall.py
  lldb/test/API/commands/settings/TestSettings.py
  lldb/test/API/driver/batch_mode/TestBatchMode.py
  
lldb/test/API/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-objc/cmtime/TestDataFormatterCMTime.py
  lldb/test/API/lang/cpp/constructors/TestCppConstructors.py
  lldb/test/API/macosx/macCatalyst/TestMacCatalyst.py
  lldb/test/API/types/TestRecursiveTypes.py

Index: lldb/test/API/types/TestRecursiveTypes.py
===
--- lldb/test/API/types/TestRecursiveTypes.py
+++ lldb/test/API/types/TestRecursiveTypes.py
@@ -50,5 +50,5 @@
 
 self.runCmd("run", RUN_SUCCEEDED)
 
-self.expect("print tpi", RUN_SUCCEEDED)
-self.expect("print *tpi", RUN_SUCCEEDED)
+self.expect("print tpi")
+self.expect("print *tpi")
Index: lldb/test/API/macosx/macCatalyst/TestMacCatalyst.py
===
--- lldb/test/API/macosx/macCatalyst/TestMacCatalyst.py
+++ lldb/test/API/macosx/macCatalyst/TestMacCatalyst.py
@@ -24,8 +24,8 @@
 self.expect("image list -t -b",
 patterns=[self.getArchitecture() +
   r'.*-apple-ios.*-macabi a\.out'])
-self.expect("fr v s", "Hello macCatalyst")
-self.expect("p s", "Hello macCatalyst")
+self.expect("fr v s", substrs=["Hello macCatalyst"])
+self.expect("p s", substrs=["Hello macCatalyst"])
 self.check_debugserver(log)
 
 def check_debugserver(self, log):
Index: lldb/test/API/lang/cpp/constructors/TestCppConstructors.py
===
--- lldb/test/API/lang/cpp/constructors/TestCppConstructors.py
+++ lldb/test/API/lang/cpp/constructors/TestCppConstructors.py
@@ -19,7 +19,7 @@
 self.expect_expr("ClassWithDeletedDefaultCtor(7).value", result_type="int", result_value="7")
 
 # FIXME: It seems we try to call the non-existent default constructor here which is wrong.
-self.expect("expr ClassWithDefaultedCtor().foo()", error=True, substrs="Couldn't lookup symbols:")
+self.expect("expr ClassWithDefaultedCtor().foo()", error=True, substrs=["Couldn't lookup symbols:"])
 
 # FIXME: Calling deleted constructors should fail before linking.
 self.expect("expr ClassWithDeletedCtor(1).value", error=True, substrs=["Couldn't lookup symbols:"])
Index: lldb/test/API/functionalities/data-formatter/data-formatter-objc/cmtime/TestDataFormatterCMTime.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-objc/cmtime/TestDataFormatterCMTime.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-objc/cmtime/TestDataFormatterCMTime.py
@@ -48,6 +48,6 @@
 self.expect(
 'frame variable t4',
 substrs=['10 seconds', 'value = 10', 'timescale = 1', 'epoch = 0'])
-self.expect('frame variable t5', '-oo')
-self.expect('frame variable t6', '+oo')
-self.expect('frame variable t7', 'indefinite')
+self.expect('frame variable t5', substrs=['+oo'])
+self.expect('frame variable t6', substrs=['-oo'])
+self.expect('frame variable t7', substrs=['indefinite'])
Index: lldb/test/API/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py
===
--- lldb/test/API/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py
+++ lldb/test/API/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py
@@ -21,7 +21,7 @@
 main_c = lldb.SBFileSpec("main.c")
 _, _, _, breakpoint = lldbutil.run_to_line_breakpoint(self,
   main_c, 11, 50)
- 

[Lldb-commits] [PATCH] D87868: [RFC] When calling the process mmap try to call all found instead of just the first one

[António Afonso via Phabricator via lldb-commits]

aadsm updated this revision to Diff 296220.
aadsm added a comment.

I explored Greg's suggestion of checking if the functions were external 
symbols. As suspected the overriden mmap is not external, so we can use this 
check to avoid calling it!

It does look to me that Pavel's suggestion is the way to go in the future but I 
don't have the knowledge to implement the suggestion nor (unfortunately) the 
bandwidth to gain that knowledge right. In the meanwhile I was hoping we could 
go with this new version of the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87868

Files:
  lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp


Index: lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp
===
--- lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp
+++ lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp
@@ -48,9 +48,10 @@
   ConstString("mmap"), eFunctionNameTypeFull, include_symbols,
   include_inlines, sc_list);
   const uint32_t count = sc_list.GetSize();
-  if (count > 0) {
+  for (uint32_t i = 0; i < count; i++) {
 SymbolContext sc;
-if (sc_list.GetContextAtIndex(0, sc)) {
+if (sc_list.GetContextAtIndex(i, sc) &&
+(sc.symbol->IsExternal() || sc.symbol->IsWeak())) {
   const uint32_t range_scope =
   eSymbolContextFunction | eSymbolContextSymbol;
   const bool use_inline_block_range = false;


Index: lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp
===
--- lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp
+++ lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp
@@ -48,9 +48,10 @@
   ConstString("mmap"), eFunctionNameTypeFull, include_symbols,
   include_inlines, sc_list);
   const uint32_t count = sc_list.GetSize();
-  if (count > 0) {
+  for (uint32_t i = 0; i < count; i++) {
 SymbolContext sc;
-if (sc_list.GetContextAtIndex(0, sc)) {
+if (sc_list.GetContextAtIndex(i, sc) &&
+(sc.symbol->IsExternal() || sc.symbol->IsWeak())) {
   const uint32_t range_scope =
   eSymbolContextFunction | eSymbolContextSymbol;
   const bool use_inline_block_range = false;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88840: [dotest] Simplify logic to find the Python path

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: rupprecht, labath, mgorny, teemperor.
JDevlieghere requested review of this revision.

Simplify the logic of parsing the `lldb -P` output to find the python path. 
This removes the special handling for the LLDB.framework and instead of pattern 
matching known errors focus on finding a directory path that contains an 
`__init__.py`.


https://reviews.llvm.org/D88840

Files:
  lldb/packages/Python/lldbsuite/test/dotest.py

Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -545,68 +545,38 @@
 configuration.skip_categories.append("lldb-vscode")
 
 lldbPythonDir = None  # The directory that contains 'lldb/__init__.py'
-if configuration.lldb_framework_path:
-lldbtest_config.lldb_framework_path = configuration.lldb_framework_path
-candidatePath = os.path.join(
-configuration.lldb_framework_path, 'Resources', 'Python')
-if os.path.isfile(os.path.join(candidatePath, 'lldb/__init__.py')):
-lldbPythonDir = candidatePath
-if not lldbPythonDir:
-print(
-'Resources/Python/lldb/__init__.py was not found in ' +
-configuration.lldb_framework_path)
-sys.exit(-1)
-else:
-# If our lldb supports the -P option, use it to find the python path:
-init_in_python_dir = os.path.join('lldb', '__init__.py')
 
-lldb_dash_p_result = subprocess.check_output(
-[lldbtest_config.lldbExec, "-P"], stderr=subprocess.STDOUT, universal_newlines=True)
-
-if lldb_dash_p_result and not lldb_dash_p_result.startswith(
-("<", "lldb: invalid option:")) and not lldb_dash_p_result.startswith("Traceback"):
+# If our lldb supports the -P option, use it to find the python path:
+with open(os.devnull, 'w') as DEVNULL:
+lldb_dash_p_result = subprocess.check_output([lldbtest_config.lldbExec, "-P"], stderr=DEVNULL, universal_newlines=True)
+if lldb_dash_p_result and not lldb_dash_p_result.startswith(("<", "lldb: invalid option:")):
 lines = lldb_dash_p_result.splitlines()
+for line in lines:
+if os.path.isdir(line) and os.path.exists(os.path.join(line, 'lldb', '__init__.py')):
+lldbPythonDir = line
+break
 
-# Workaround for readline vs libedit issue on FreeBSD.  If stdout
-# is not a terminal Python executes
-# rl_variable_bind ("enable-meta-key", "off");
-# This produces a warning with FreeBSD's libedit because the
-# enable-meta-key variable is unknown.  Not an issue on Apple
-# because cpython commit f0ab6f9f0603 added a #ifndef __APPLE__
-# around the call.  See http://bugs.python.org/issue19884 for more
-# information.  For now we just discard the warning output.
-if len(lines) >= 1 and lines[0].startswith(
-"bind: Invalid command"):
-lines.pop(0)
-
-# Taking the last line because lldb outputs
-# 'Cannot read termcap database;\nusing dumb terminal settings.\n'
-# before the path
-if len(lines) >= 1 and os.path.isfile(
-os.path.join(lines[-1], init_in_python_dir)):
-lldbPythonDir = lines[-1]
-if "freebsd" in sys.platform or "linux" in sys.platform:
-os.environ['LLDB_LIB_DIR'] = os.path.join(
-lldbPythonDir, '..', '..')
-
-if not lldbPythonDir:
-print(
-"Unable to load lldb extension module.  Possible reasons for this include:")
-print("  1) LLDB was built with LLDB_ENABLE_PYTHON=0")
-print(
-"  2) PYTHONPATH and PYTHONHOME are not set correctly.  PYTHONHOME should refer to")
-print(
-" the version of Python that LLDB built and linked against, and PYTHONPATH")
-print(
-" should contain the Lib directory for the same python distro, as well as the")
-print(" location of LLDB\'s site-packages folder.")
-print(
-"  3) A different version of Python than that which was built against is exported in")
-print(" the system\'s PATH environment variable, causing conflicts.")
-print(
-"  4) The executable '%s' could not be found.  Please check " %
-lldbtest_config.lldbExec)
-print(" that it exists and is executable.")
+if lldbPythonDir and "freebsd" in sys.platform or "linux" in sys.platform:
+os.environ['LLDB_LIB_DIR'] = os.path.join(lldbPythonDir, '..', '..')
+
+if n

[Lldb-commits] [PATCH] D88840: [dotest] Simplify logic to find the Python path

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 296229.

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

https://reviews.llvm.org/D88840

Files:
  lldb/packages/Python/lldbsuite/test/dotest.py

Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -545,68 +545,35 @@
 configuration.skip_categories.append("lldb-vscode")
 
 lldbPythonDir = None  # The directory that contains 'lldb/__init__.py'
-if configuration.lldb_framework_path:
-lldbtest_config.lldb_framework_path = configuration.lldb_framework_path
-candidatePath = os.path.join(
-configuration.lldb_framework_path, 'Resources', 'Python')
-if os.path.isfile(os.path.join(candidatePath, 'lldb/__init__.py')):
-lldbPythonDir = candidatePath
-if not lldbPythonDir:
-print(
-'Resources/Python/lldb/__init__.py was not found in ' +
-configuration.lldb_framework_path)
-sys.exit(-1)
-else:
-# If our lldb supports the -P option, use it to find the python path:
-init_in_python_dir = os.path.join('lldb', '__init__.py')
 
-lldb_dash_p_result = subprocess.check_output(
-[lldbtest_config.lldbExec, "-P"], stderr=subprocess.STDOUT, universal_newlines=True)
-
-if lldb_dash_p_result and not lldb_dash_p_result.startswith(
-("<", "lldb: invalid option:")) and not lldb_dash_p_result.startswith("Traceback"):
+# If our lldb supports the -P option, use it to find the python path:
+with open(os.devnull, 'w') as DEVNULL:
+lldb_dash_p_result = subprocess.check_output([lldbtest_config.lldbExec, "-P"], stderr=DEVNULL, universal_newlines=True)
+if lldb_dash_p_result and not lldb_dash_p_result.startswith(("<", "lldb: invalid option:")):
 lines = lldb_dash_p_result.splitlines()
+for line in lines:
+if os.path.isdir(line) and os.path.exists(os.path.join(line, 'lldb', '__init__.py')):
+lldbPythonDir = line
+break
 
-# Workaround for readline vs libedit issue on FreeBSD.  If stdout
-# is not a terminal Python executes
-# rl_variable_bind ("enable-meta-key", "off");
-# This produces a warning with FreeBSD's libedit because the
-# enable-meta-key variable is unknown.  Not an issue on Apple
-# because cpython commit f0ab6f9f0603 added a #ifndef __APPLE__
-# around the call.  See http://bugs.python.org/issue19884 for more
-# information.  For now we just discard the warning output.
-if len(lines) >= 1 and lines[0].startswith(
-"bind: Invalid command"):
-lines.pop(0)
-
-# Taking the last line because lldb outputs
-# 'Cannot read termcap database;\nusing dumb terminal settings.\n'
-# before the path
-if len(lines) >= 1 and os.path.isfile(
-os.path.join(lines[-1], init_in_python_dir)):
-lldbPythonDir = lines[-1]
-if "freebsd" in sys.platform or "linux" in sys.platform:
-os.environ['LLDB_LIB_DIR'] = os.path.join(
-lldbPythonDir, '..', '..')
-
-if not lldbPythonDir:
-print(
-"Unable to load lldb extension module.  Possible reasons for this include:")
-print("  1) LLDB was built with LLDB_ENABLE_PYTHON=0")
-print(
-"  2) PYTHONPATH and PYTHONHOME are not set correctly.  PYTHONHOME should refer to")
-print(
-" the version of Python that LLDB built and linked against, and PYTHONPATH")
-print(
-" should contain the Lib directory for the same python distro, as well as the")
-print(" location of LLDB\'s site-packages folder.")
-print(
-"  3) A different version of Python than that which was built against is exported in")
-print(" the system\'s PATH environment variable, causing conflicts.")
-print(
-"  4) The executable '%s' could not be found.  Please check " %
-lldbtest_config.lldbExec)
-print(" that it exists and is executable.")
+if not lldbPythonDir:
+print(
+"Unable to load lldb extension module.  Possible reasons for this include:")
+print("  1) LLDB was built with LLDB_ENABLE_PYTHON=0")
+print(
+"  2) PYTHONPATH and PYTHONHOME are not set correctly.  PYTHONHOME should refer to")
+print(
+" the version of Python that LLDB built and linked against, and PYTHONPATH")
+print(
+" should contai

[Lldb-commits] [PATCH] D88796: [lldb] Initial version of FreeBSD remote process plugin

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 296238.
mgorny marked 2 inline comments as done.
mgorny added a comment.

For a start, removed leftover printf and len increment.


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

https://reviews.llvm.org/D88796

Files:
  lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
  lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.h
  lldb/source/Plugins/Process/CMakeLists.txt
  lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
  lldb/source/Plugins/Process/FreeBSDRemote/CMakeLists.txt
  lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
  lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.h
  lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD.cpp
  lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD.h
  
lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.cpp
  
lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.h
  lldb/source/Plugins/Process/FreeBSDRemote/NativeThreadFreeBSD.cpp
  lldb/source/Plugins/Process/FreeBSDRemote/NativeThreadFreeBSD.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  lldb/test/Shell/lit.cfg.py
  lldb/tools/lldb-server/CMakeLists.txt
  lldb/tools/lldb-server/lldb-gdbserver.cpp

Index: lldb/tools/lldb-server/lldb-gdbserver.cpp
===
--- lldb/tools/lldb-server/lldb-gdbserver.cpp
+++ lldb/tools/lldb-server/lldb-gdbserver.cpp
@@ -38,6 +38,8 @@
 
 #if defined(__linux__)
 #include "Plugins/Process/Linux/NativeProcessLinux.h"
+#elif defined(__FreeBSD__)
+#include "Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.h"
 #elif defined(__NetBSD__)
 #include "Plugins/Process/NetBSD/NativeProcessNetBSD.h"
 #elif defined(_WIN32)
@@ -61,6 +63,8 @@
 namespace {
 #if defined(__linux__)
 typedef process_linux::NativeProcessLinux::Factory NativeProcessFactory;
+#elif defined(__FreeBSD__)
+typedef process_freebsd::NativeProcessFreeBSD::Factory NativeProcessFactory;
 #elif defined(__NetBSD__)
 typedef process_netbsd::NativeProcessNetBSD::Factory NativeProcessFactory;
 #elif defined(_WIN32)
Index: lldb/tools/lldb-server/CMakeLists.txt
===
--- lldb/tools/lldb-server/CMakeLists.txt
+++ lldb/tools/lldb-server/CMakeLists.txt
@@ -4,6 +4,12 @@
   list(APPEND LLDB_PLUGINS lldbPluginProcessLinux)
 endif()
 
+if(CMAKE_SYSTEM_NAME MATCHES "FreeBSD")
+  list(APPEND LLDB_PLUGINS
+lldbPluginProcessFreeBSDRemote
+lldbPluginProcessFreeBSD)
+endif()
+
 if(CMAKE_SYSTEM_NAME MATCHES "NetBSD")
   list(APPEND LLDB_PLUGINS lldbPluginProcessNetBSD)
 endif()
Index: lldb/test/Shell/lit.cfg.py
===
--- lldb/test/Shell/lit.cfg.py
+++ lldb/test/Shell/lit.cfg.py
@@ -133,3 +133,6 @@
 can_set_dbregs = False
 if can_set_dbregs:
 config.available_features.add('dbregs-set')
+
+# pass control variable through
+llvm_config.with_system_environment('FREEBSD_REMOTE_PLUGIN')
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
@@ -843,7 +843,7 @@
   response.PutCString(";QListThreadsInStopReply+");
   response.PutCString(";qEcho+");
   response.PutCString(";qXfer:features:read+");
-#if defined(__linux__) || defined(__NetBSD__)
+#if defined(__linux__) || defined(__NetBSD__) || defined(__FreeBSD__)
   response.PutCString(";QPassSignals+");
   response.PutCString(";qXfer:auxv:read+");
   response.PutCString(";qXfer:libraries-svr4:read+");
Index: lldb/source/Plugins/Process/FreeBSDRemote/NativeThreadFreeBSD.h
===
--- /dev/null
+++ lldb/source/Plugins/Process/FreeBSDRemote/NativeThreadFreeBSD.h
@@ -0,0 +1,83 @@
+//===-- NativeThreadFreeBSD.h - -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef liblldb_NativeThreadFreeBSD_H_
+#define liblldb_NativeThreadFreeBSD_H_
+
+#include "lldb/Host/common/NativeThreadProtocol.h"
+
+#include "Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD.h"
+
+#include 
+#include 
+#include 
+
+namespace lldb_private {
+namespace process_freebsd {
+
+class NativeProcessFreeBSD;
+
+class NativeThreadFreeBSD : public NativeThreadProtocol {
+  friend class NativeProcessFreeBSD;
+
+public:
+  NativeThreadFreeBSD(NativeProcessFreeBSD &process, lldb::tid_t tid);
+
+  // NativeThreadPr

[Lldb-commits] [PATCH] D88840: [dotest] Simplify logic to find the Python path

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 296239.

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

https://reviews.llvm.org/D88840

Files:
  lldb/packages/Python/lldbsuite/test/dotest.py

Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -545,68 +545,35 @@
 configuration.skip_categories.append("lldb-vscode")
 
 lldbPythonDir = None  # The directory that contains 'lldb/__init__.py'
-if configuration.lldb_framework_path:
-lldbtest_config.lldb_framework_path = configuration.lldb_framework_path
-candidatePath = os.path.join(
-configuration.lldb_framework_path, 'Resources', 'Python')
-if os.path.isfile(os.path.join(candidatePath, 'lldb/__init__.py')):
-lldbPythonDir = candidatePath
-if not lldbPythonDir:
-print(
-'Resources/Python/lldb/__init__.py was not found in ' +
-configuration.lldb_framework_path)
-sys.exit(-1)
-else:
-# If our lldb supports the -P option, use it to find the python path:
-init_in_python_dir = os.path.join('lldb', '__init__.py')
 
-lldb_dash_p_result = subprocess.check_output(
-[lldbtest_config.lldbExec, "-P"], stderr=subprocess.STDOUT, universal_newlines=True)
-
-if lldb_dash_p_result and not lldb_dash_p_result.startswith(
-("<", "lldb: invalid option:")) and not lldb_dash_p_result.startswith("Traceback"):
+# If our lldb supports the -P option, use it to find the python path:
+with open(os.devnull, 'w') as DEVNULL:
+lldb_dash_p_result = subprocess.check_output([lldbtest_config.lldbExec, "-P"], stderr=DEVNULL, universal_newlines=True)
+if lldb_dash_p_result:
 lines = lldb_dash_p_result.splitlines()
+for line in lines:
+if os.path.isdir(line) and os.path.exists(os.path.join(line, 'lldb', '__init__.py')):
+lldbPythonDir = line
+break
 
-# Workaround for readline vs libedit issue on FreeBSD.  If stdout
-# is not a terminal Python executes
-# rl_variable_bind ("enable-meta-key", "off");
-# This produces a warning with FreeBSD's libedit because the
-# enable-meta-key variable is unknown.  Not an issue on Apple
-# because cpython commit f0ab6f9f0603 added a #ifndef __APPLE__
-# around the call.  See http://bugs.python.org/issue19884 for more
-# information.  For now we just discard the warning output.
-if len(lines) >= 1 and lines[0].startswith(
-"bind: Invalid command"):
-lines.pop(0)
-
-# Taking the last line because lldb outputs
-# 'Cannot read termcap database;\nusing dumb terminal settings.\n'
-# before the path
-if len(lines) >= 1 and os.path.isfile(
-os.path.join(lines[-1], init_in_python_dir)):
-lldbPythonDir = lines[-1]
-if "freebsd" in sys.platform or "linux" in sys.platform:
-os.environ['LLDB_LIB_DIR'] = os.path.join(
-lldbPythonDir, '..', '..')
-
-if not lldbPythonDir:
-print(
-"Unable to load lldb extension module.  Possible reasons for this include:")
-print("  1) LLDB was built with LLDB_ENABLE_PYTHON=0")
-print(
-"  2) PYTHONPATH and PYTHONHOME are not set correctly.  PYTHONHOME should refer to")
-print(
-" the version of Python that LLDB built and linked against, and PYTHONPATH")
-print(
-" should contain the Lib directory for the same python distro, as well as the")
-print(" location of LLDB\'s site-packages folder.")
-print(
-"  3) A different version of Python than that which was built against is exported in")
-print(" the system\'s PATH environment variable, causing conflicts.")
-print(
-"  4) The executable '%s' could not be found.  Please check " %
-lldbtest_config.lldbExec)
-print(" that it exists and is executable.")
+if not lldbPythonDir:
+print(
+"Unable to load lldb extension module.  Possible reasons for this include:")
+print("  1) LLDB was built with LLDB_ENABLE_PYTHON=0")
+print(
+"  2) PYTHONPATH and PYTHONHOME are not set correctly.  PYTHONHOME should refer to")
+print(
+" the version of Python that LLDB built and linked against, and PYTHONPATH")
+print(
+" should contain the Lib directory for the same python distro, as well as the")
+

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

[walter erquinigo via Phabricator via lldb-commits]

wallace created this revision.
wallace added reviewers: clayborg, labath.
Herald added subscribers: lldb-commits, mgorny.
Herald added a reviewer: JDevlieghere.
Herald added a project: LLDB.
wallace requested review of this revision.

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

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

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

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

With the current set of targets, we have the following

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88841

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

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -40,7 +40,7 @@
 src_dir = self.getSourceDir()
 # We test first an invalid type
 self.expect("trace load -v " + os.path.join(src_dir, "intelpt-trace", "trace_bad.json"), error=True,
-  substrs=['''error: expected object at settings.processes[0]
+  substrs=['''error: expected object at traceSession.processes[0]
 
 Context:
 {
@@ -53,7 +53,7 @@
 
 Schema:
 {
- "trace": {
+  "trace": {
 "type": "intel-pt",
 "pt_cpu": {
   "vendor": "intel" | "unknown",
@@ -63,32 +63,35 @@
 }
   },'''])
 
-# Now we test a missing field in the global settings
+# Now we test a missing field in the global session file
 self.expect("trace load -v " + os.path.join(src_dir, "intelpt-trace", "trace_bad2.json"), error=True,
-substrs=['error: missing value at settings.processes[1].triple', "Context", "Schema"])
+substrs=['error: missing value at traceSession.processes[1].triple', "Context", "Schema"])
 
 # Now we test a missing field in the intel-pt settings
 self.expect("trace load -v " + os.path.join(src_dir, "intelpt-trace", "trace_bad4.json"), error=True,
-substrs=['''error: missing value at settings.trace.pt_cpu.family
+substrs=['''error: missing value at traceSession.trace.pt_cpu.family
 
 Context:
 {
-  "pt_cpu": /* error: missing value */ {
-"model": 79,
-"stepping": 1,
-"vendor": "intel"
-  },
-  "type": "intel-pt"
+  "processes": [],
+  "trace": {
+"pt_cpu": /* error: missing value */ {
+  "model": 79,
+  "stepping": 1,
+  "vendor": "intel"
+},
+"type": "intel-pt"
+  }
 }''', "Schema"])
 
 # N

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

[walter erquinigo via Phabricator via lldb-commits]

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

Rebased on top of https://reviews.llvm.org/D88841

- Comments on the architecture of classes are in that diff
- I was able to get rid of cross-plug-in dependencies


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88769

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

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -35,6 +35,10 @@
 
 self.assertEqual("6AA9A4E2-6F28-2F33-377D-59FECE874C71-5B41261A", module.GetUUIDString())
 
+# check that the Process and Thread objects were created correctly
+self.expect("thread info", substrs=["tid = 3842849"])
+self.expect("thread list", substrs=["Process 1234 stopped", "tid = 3842849"])
+
 
 def testLoadInvalidTraces(self):
 src_dir = self.getSourceDir()
Index: lldb/test/API/commands/trace/TestTraceDumpInstructions.py
===
--- /dev/null
+++ lldb/test/API/commands/trace/TestTraceDumpInstructions.py
@@ -0,0 +1,56 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+
+class TestTraceDumpInstructions(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+def setUp(self):
+TestBase.setUp(self)
+if 'intel-pt' not in configuration.enabled_plugins:
+self.skipTest("The intel-pt test plugin is not enabled")
+
+def testErrorMessages(self):
+# We first check the output when there are no targets
+self.expect("thread trace dump instructions",
+substrs=["error: invalid target, create a target using the 'target create' command"],
+error=True)
+
+# We now check the output when there's a non-running target
+self.expect("target create " + os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
+
+self.expect("thread trace dump instructions",
+substrs=["error: invalid process"],
+error=True)
+
+# Now we check the output when there's a running target without a trace
+self.expect("b main")
+self.expect("run")
+
+self.expect("thread trace dump instructions",
+substrs=["error: no trace in this target"])
+
+def testDumpInstructions(self):
+self.expect("trace load -v " + os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"),
+substrs=["intel-pt"])
+
+self.expect("thread trace dump instructions",
+substrs=['thread #1: tid = 3842849',
+ 'would print 10 instructions from position 0'])
+
+# We check if we can pass count and offset
+self.expect("thread trace dump instructions --count 5 --start-position 10",
+substrs=['thread #1: tid = 3842849',
+ 'would print 5 instructions from position 10'])
+
+# We check if we can access the thread by index id
+self.expect("thread trace dump instructions 1",
+substrs=['thread #1: tid = 3842849',
+ 'would print 10 instructions from position 0'])
+
+# We check that we get an error when using an invalid thread index id
+self.expect("thread trace dump instructions 10", error=True,
+substrs=['error: no thread with index: "10"'])
Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -1614,6 +1614,17 @@
   return Status();
 }
 
+void Thread::DumpTraceInstructions(Stream &s, size_t count,
+   size_t start_position) const {
+  if (!GetProcess()->GetTarget().GetTrace()) {
+s << "error: no trace in this target\n";
+return;
+  }
+  s.Printf("thread #%u: tid = %" PRIu64 "\n", GetIndexID(), GetID());
+  s.Printf("  would print %zu instructions from position %zu\n", count,
+   start_position);
+}
+
 void Thread::DumpUsingSettingsFormat(Stream &strm, uint32_t frame_idx,
  bool stop_format

[Lldb-commits] [lldb] 010d7a3 - [lldb/test] Catch invalid calls to expect()

[Dave Lee via lldb-commits]

Author: Dave Lee
Date: 2020-10-05T12:41:52-07:00
New Revision: 010d7a388b146cafaf4bc0b28b952d5852d62b6a

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

LOG: [lldb/test] Catch invalid calls to expect()

Add preconditions to `TestBase.expect()` that catch semantically invalid calls
that happen to succeed anyway. This also fixes the broken callsites caught by
these checks.

This prevents the following incorrect calls:

1. `self.expect("lldb command", "some substr")`
2. `self.expect("lldb command", "assert message", "some substr")`

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

Added: 


Modified: 
lldb/packages/Python/lldbsuite/test/lldbtest.py
lldb/test/API/assert_messages_test/TestAssertMessages.py
lldb/test/API/commands/frame/diagnose/bad-reference/TestBadReference.py

lldb/test/API/commands/frame/diagnose/complicated-expression/TestComplicatedExpression.py

lldb/test/API/commands/frame/diagnose/dereference-argument/TestDiagnoseDereferenceArgument.py

lldb/test/API/commands/frame/diagnose/dereference-this/TestDiagnoseDereferenceThis.py
lldb/test/API/commands/frame/diagnose/inheritance/TestDiagnoseInheritance.py
lldb/test/API/commands/frame/diagnose/local-variable/TestLocalVariable.py

lldb/test/API/commands/frame/diagnose/virtual-method-call/TestDiagnoseDereferenceVirtualMethodCall.py
lldb/test/API/commands/settings/TestSettings.py
lldb/test/API/driver/batch_mode/TestBatchMode.py

lldb/test/API/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py

lldb/test/API/functionalities/data-formatter/data-formatter-objc/cmtime/TestDataFormatterCMTime.py
lldb/test/API/lang/cpp/constructors/TestCppConstructors.py
lldb/test/API/macosx/macCatalyst/TestMacCatalyst.py
lldb/test/API/types/TestRecursiveTypes.py

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py 
b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index 2ee82295c553..2309b403cb99 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -2424,6 +2424,22 @@ def expect(
 set to False, the 'str' is treated as a string to be 
matched/not-matched
 against the golden input.
 """
+# Catch cases where `expect` has been miscalled. Specifically, prevent
+# this easy to make mistake:
+# self.expect("lldb command", "some substr")
+# The `msg` parameter is used only when a failed match occurs. A failed
+# match can only occur when one of `patterns`, `startstr`, `endstr`, or
+# `substrs` has been given. Thus, if a `msg` is given, it's an error to
+# not also provide one of the matcher parameters.
+if msg and not (patterns or startstr or endstr or substrs or error):
+assert False, "expect() missing a matcher argument"
+
+# Check `patterns` and `substrs` are not accidentally given as strings.
+assert not isinstance(patterns, six.string_types), \
+"patterns must be a collection of strings"
+assert not isinstance(substrs, six.string_types), \
+"substrs must be a collection of strings"
+
 trace = (True if traceAlways else trace)
 
 if exe:

diff  --git a/lldb/test/API/assert_messages_test/TestAssertMessages.py 
b/lldb/test/API/assert_messages_test/TestAssertMessages.py
index 6619a65ad69e..f8b6b33f297c 100644
--- a/lldb/test/API/assert_messages_test/TestAssertMessages.py
+++ b/lldb/test/API/assert_messages_test/TestAssertMessages.py
@@ -113,3 +113,20 @@ def test_expect(self):
 
Expecting start string: "cat" (was not found)
Reason for check goes here!""")
+
+# Verify expect() preconditions.
+# Both `patterns` and `substrs` cannot be of type string.
+self.assert_expect_fails_with("any command",
+dict(patterns="some substring"),
+"patterns must be a collection of strings")
+self.assert_expect_fails_with("any command",
+dict(substrs="some substring"),
+"substrs must be a collection of strings")
+# Prevent `self.expect("cmd", "substr")`
+self.assert_expect_fails_with("any command",
+dict(msg="some substring"),
+"expect() missing a matcher argument")
+# Prevent `self.expect("cmd", "msg", "substr")`
+self.assert_expect_fails_with("any command",
+dict(msg="a message", patterns="some substring"),
+"must be a collection of strings")

diff  --git 
a/lldb/test/API/commands/frame/diagnose/bad-reference/TestBadReference.py 
b/lldb/test/API/commands/frame/diagnose/bad-reference/TestBadReference.py
index 737b297ed76b..2ed

[Lldb-commits] [PATCH] D88792: [lldb/test] Catch invalid calls to expect()

[Dave Lee via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG010d7a388b14: [lldb/test] Catch invalid calls to expect() 
(authored by kastiglione).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88792

Files:
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/test/API/assert_messages_test/TestAssertMessages.py
  lldb/test/API/commands/frame/diagnose/bad-reference/TestBadReference.py
  
lldb/test/API/commands/frame/diagnose/complicated-expression/TestComplicatedExpression.py
  
lldb/test/API/commands/frame/diagnose/dereference-argument/TestDiagnoseDereferenceArgument.py
  
lldb/test/API/commands/frame/diagnose/dereference-this/TestDiagnoseDereferenceThis.py
  lldb/test/API/commands/frame/diagnose/inheritance/TestDiagnoseInheritance.py
  lldb/test/API/commands/frame/diagnose/local-variable/TestLocalVariable.py
  
lldb/test/API/commands/frame/diagnose/virtual-method-call/TestDiagnoseDereferenceVirtualMethodCall.py
  lldb/test/API/commands/settings/TestSettings.py
  lldb/test/API/driver/batch_mode/TestBatchMode.py
  
lldb/test/API/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-objc/cmtime/TestDataFormatterCMTime.py
  lldb/test/API/lang/cpp/constructors/TestCppConstructors.py
  lldb/test/API/macosx/macCatalyst/TestMacCatalyst.py
  lldb/test/API/types/TestRecursiveTypes.py

Index: lldb/test/API/types/TestRecursiveTypes.py
===
--- lldb/test/API/types/TestRecursiveTypes.py
+++ lldb/test/API/types/TestRecursiveTypes.py
@@ -50,5 +50,5 @@
 
 self.runCmd("run", RUN_SUCCEEDED)
 
-self.expect("print tpi", RUN_SUCCEEDED)
-self.expect("print *tpi", RUN_SUCCEEDED)
+self.expect("print tpi")
+self.expect("print *tpi")
Index: lldb/test/API/macosx/macCatalyst/TestMacCatalyst.py
===
--- lldb/test/API/macosx/macCatalyst/TestMacCatalyst.py
+++ lldb/test/API/macosx/macCatalyst/TestMacCatalyst.py
@@ -24,8 +24,8 @@
 self.expect("image list -t -b",
 patterns=[self.getArchitecture() +
   r'.*-apple-ios.*-macabi a\.out'])
-self.expect("fr v s", "Hello macCatalyst")
-self.expect("p s", "Hello macCatalyst")
+self.expect("fr v s", substrs=["Hello macCatalyst"])
+self.expect("p s", substrs=["Hello macCatalyst"])
 self.check_debugserver(log)
 
 def check_debugserver(self, log):
Index: lldb/test/API/lang/cpp/constructors/TestCppConstructors.py
===
--- lldb/test/API/lang/cpp/constructors/TestCppConstructors.py
+++ lldb/test/API/lang/cpp/constructors/TestCppConstructors.py
@@ -19,7 +19,7 @@
 self.expect_expr("ClassWithDeletedDefaultCtor(7).value", result_type="int", result_value="7")
 
 # FIXME: It seems we try to call the non-existent default constructor here which is wrong.
-self.expect("expr ClassWithDefaultedCtor().foo()", error=True, substrs="Couldn't lookup symbols:")
+self.expect("expr ClassWithDefaultedCtor().foo()", error=True, substrs=["Couldn't lookup symbols:"])
 
 # FIXME: Calling deleted constructors should fail before linking.
 self.expect("expr ClassWithDeletedCtor(1).value", error=True, substrs=["Couldn't lookup symbols:"])
Index: lldb/test/API/functionalities/data-formatter/data-formatter-objc/cmtime/TestDataFormatterCMTime.py
===
--- lldb/test/API/functionalities/data-formatter/data-formatter-objc/cmtime/TestDataFormatterCMTime.py
+++ lldb/test/API/functionalities/data-formatter/data-formatter-objc/cmtime/TestDataFormatterCMTime.py
@@ -48,6 +48,6 @@
 self.expect(
 'frame variable t4',
 substrs=['10 seconds', 'value = 10', 'timescale = 1', 'epoch = 0'])
-self.expect('frame variable t5', '-oo')
-self.expect('frame variable t6', '+oo')
-self.expect('frame variable t7', 'indefinite')
+self.expect('frame variable t5', substrs=['+oo'])
+self.expect('frame variable t6', substrs=['-oo'])
+self.expect('frame variable t7', substrs=['indefinite'])
Index: lldb/test/API/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py
===
--- lldb/test/API/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py
+++ lldb/test/API/functionalities/breakpoint/breakpoint_by_line_and_column/TestBreakpointByLineAndColumn.py
@@ -21,7 +21,7 @@
 main_c = lldb.SBFileSpec("main.c")
 _, _, _, breakpoint = lldbutil.run_to_line_breakpoint(self,
   

[Lldb-commits] [PATCH] D88852: [lldb] [Platform] Move common ::DebugProcess() to PlatformPOSIX

[Michał Górny via Phabricator via lldb-commits]

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

Move common ::DebugProcess() implementation shared by Linux and NetBSD
(and to be shared by FreeBSD shortly) into PlatformPOSIX, and move
the old base implementation used only by Darwin to PlatformDarwin.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88852

Files:
  lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
  lldb/source/Plugins/Platform/Linux/PlatformLinux.h
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h
  lldb/source/Plugins/Platform/NetBSD/PlatformNetBSD.cpp
  lldb/source/Plugins/Platform/NetBSD/PlatformNetBSD.h
  lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

Index: lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
===
--- lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
+++ lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
@@ -416,24 +416,115 @@
 Target *target, // Can be NULL, if NULL create a new
 // target, else use existing one
 Status &error) {
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PLATFORM));
+  LLDB_LOG(log, "target {0}", target);
+
   ProcessSP process_sp;
 
-  if (IsHost()) {
-// We are going to hand this process off to debugserver which will be in
-// charge of setting the exit status.  However, we still need to reap it
-// from lldb. So, make sure we use a exit callback which does not set exit
-// status.
-const bool monitor_signals = false;
-launch_info.SetMonitorProcessCallback(
-&ProcessLaunchInfo::NoOpMonitorCallback, monitor_signals);
-process_sp = Platform::DebugProcess(launch_info, debugger, target, error);
-  } else {
+  if (!IsHost()) {
 if (m_remote_platform_sp)
   process_sp = m_remote_platform_sp->DebugProcess(launch_info, debugger,
   target, error);
 else
   error.SetErrorString("the platform is not currently connected");
+return process_sp;
+  }
+
+  //
+  // For local debugging, we'll insist on having ProcessGDBRemote create the
+  // process.
+  //
+
+  // Make sure we stop at the entry point
+  launch_info.GetFlags().Set(eLaunchFlagDebug);
+
+  // We always launch the process we are going to debug in a separate process
+  // group, since then we can handle ^C interrupts ourselves w/o having to
+  // worry about the target getting them as well.
+  launch_info.SetLaunchInSeparateProcessGroup(true);
+
+  // Ensure we have a target.
+  if (target == nullptr) {
+LLDB_LOG(log, "creating new target");
+TargetSP new_target_sp;
+error = debugger.GetTargetList().CreateTarget(
+debugger, "", "", eLoadDependentsNo, nullptr, new_target_sp);
+if (error.Fail()) {
+  LLDB_LOG(log, "failed to create new target: {0}", error);
+  return process_sp;
+}
+
+target = new_target_sp.get();
+if (!target) {
+  error.SetErrorString("CreateTarget() returned nullptr");
+  LLDB_LOG(log, "error: {0}", error);
+  return process_sp;
+}
+  }
+
+  // Mark target as currently selected target.
+  debugger.GetTargetList().SetSelectedTarget(target);
+
+  // Now create the gdb-remote process.
+  LLDB_LOG(log, "having target create process with gdb-remote plugin");
+  process_sp =
+  target->CreateProcess(launch_info.GetListener(), "gdb-remote", nullptr);
+
+  if (!process_sp) {
+error.SetErrorString("CreateProcess() failed for gdb-remote process");
+LLDB_LOG(log, "error: {0}", error);
+return process_sp;
   }
+
+  LLDB_LOG(log, "successfully created process");
+  // Adjust launch for a hijacker.
+  ListenerSP listener_sp;
+  if (!launch_info.GetHijackListener()) {
+LLDB_LOG(log, "setting up hijacker");
+listener_sp =
+Listener::MakeListener("lldb.PlatformLinux.DebugProcess.hijack");
+launch_info.SetHijackListener(listener_sp);
+process_sp->HijackProcessEvents(listener_sp);
+  }
+
+  // Log file actions.
+  if (log) {
+LLDB_LOG(log, "launching process with the following file actions:");
+StreamString stream;
+size_t i = 0;
+const FileAction *file_action;
+while ((file_action = launch_info.GetFileActionAtIndex(i++)) != nullptr) {
+  file_action->Dump(stream);
+  LLDB_LOG(log, "{0}", stream.GetData());
+  stream.Clear();
+}
+  }
+
+  // Do the launch.
+  error = process_sp->Launch(launch_info);
+  if (error.Success()) {
+// Handle the hijacking of process events.
+if (listener_sp) {
+  const StateType state = process_sp->WaitForProcessToStop(
+  llvm::None, nullptr, false, listener_sp);
+
+  LLDB_LOG(log, "pid {0} state {0}", process_sp->GetID(), state);
+}
+
+// Hook up proce

[Lldb-commits] [PATCH] D88852: [lldb] [Platform] Move common ::DebugProcess() to PlatformPOSIX

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

@labath, I've tested it on Linux so far. I'm currently building on NetBSD.

@JDevlieghere, could you test this on Darwin? Alternatively, it's quite simple 
so I can wait for buildbots to complain after pushing ;-).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88852

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

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

[Greg Clayton via Phabricator via lldb-commits]

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



Comment at: lldb/include/lldb/Target/Trace.h:77
   ///
-  /// \param[in] settings
-  /// JSON object describing a trace.
+  /// \param[in] trace_session_file
+  /// The contents of the trace session file describing the trace session.

Is this the top level object from the JSON file? I think the old name of 
"settings" was better?



Comment at: lldb/include/lldb/Target/Trace.h:95
   ///
-  /// \param[in] debugger
-  ///   The debugger instance where the targets are created.
-  ///
-  /// \param[in] settings
-  /// JSON object describing a trace.
+  /// It should include at least these basic fields
   ///

Maybe some more comments here saying something about the fact that there are 
common settings required by all plug-ins and where to find this info, and also 
where this plug-in specific schema must appear within the JSON trace file? 
Maybe also mention that each trace plug-in can defined their own needed 
settings and that this information will appear in "bla bla" command if you type 
it so you can see the information required by each plug-in?



Comment at: lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.h:51
+  lldb_private::FileSpec m_trace_file;
+  pt_cpu m_pt_cpu;
+};

Can we have different "pt_cpu" values for each thread? Or are we just storing 
this here because we don't control the Process class that we will use when 
doing IntelPT when we have a live process? Do we need this in the thread? Could 
we not just fetch the trace from the target and cast it to TraceIntelPT and 
call an accessor if we need the pt_cpu?



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:40
+  ///
+  /// \param[in] trace_session_file
+  /// The contents of the trace session file. See \a Trace::FindPlugin.

I still think "settings" is a better word for this argument.



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:58
+  static lldb::TraceSP
+  CreateInstance(const llvm::json::Value *trace_session_file,
+ llvm::StringRef *session_file_dir,

"const llvm::json::Value &" instead of a pointer? This can't ever be NULL and 
it would be nice to not each plug-in think it might need to check for NULL?



Comment at: 
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionParser.cpp:31-50
+  "processes": [
+{
+  "pid": integer,
+  "triple": string, // llvm-triple
+  "threads": [
+{
+  "tid": integer,

This common information should still be parsed by code in Trace.cpp.



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionParser.h:21-42
+  struct JSONAddress {
+lldb::addr_t value;
+  };
+
+  struct JSONModule {
+std::string system_path;
+llvm::Optional file;

Seems like we should still parse this in Trace.cpp. Any common information 
shouldn't be duplicated or represented differently in different trace plug-ins 
IMHO. 



Comment at: lldb/source/Target/Trace.cpp:80-82
+TraceSP instance = create_callback(/*trace_session_file*/ nullptr,
+   /*session_file_dir*/ nullptr,
+   /*debugger*/ nullptr, error);

Why have the create_callback take an of these arguments if they are always null?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88841

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

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

[Greg Clayton via Phabricator via lldb-commits]

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



Comment at: lldb/source/Commands/CommandObjectThread.cpp:2231-2240
+  CommandObjectTraceDumpInstructions(CommandInterpreter &interpreter)
+  : CommandObjectIterateOverThreads(
+interpreter, "thread trace dump instructions",
+"Dump the traced instructions for one or more threads.  If no "
+"threads are specified, show the current thread.  Use the "
+"thread-index \"all\" to see all threads.",
+nullptr,

This command does seem hard to abstract over all flavors of trace. Would it be 
better to have the trace plug-ins vend a command objects for "thread trace 
dump"? The idea would be something like "hey trace plug-in, do you have any 
commands you support for the 'thread trace dump' multiword command?". It could 
say "yes, I have one called 'instructions' and here is the CommandObjectSP



Comment at: lldb/source/Commands/CommandObjectThread.cpp:2245
+  Options *GetOptions() override { return &m_options; }
+
+protected:

We should override the repeat command method here:

```
const char *GetRepeatCommand(Args ¤t_command_args, uint32_t index) 
override;
```

This way if you type:
```
(lldb) thread trace dump instruction --offset 0 --count 32
```
And then hit enter, the next command it should run is:
```
(lldb) thread trace dump instruction --offset 32 --count 32
```
That you can keep dumping more instructions by just hitting ENTER.



Comment at: lldb/source/Commands/Options.td:1008
 
+let Command = "thread trace dump instructions" in {
+  def thread_trace_dump_instructions_count : Option<"count", "c">, Group<1>,

Do we add a "--summary" option to do this command so that when this option is 
specified would dump the number of instructions that any threads from the might 
have? Like:
```
(lldb) thread trace dump instructions --summary
thread #1 has 4096 instructions
thread #2 has 234 instructions
```



Comment at: lldb/source/Commands/Options.td:1017-1020
+Desc<"The position of the first instruction to print. Defaults to the "
+"current position. The instructions are indexed in reverse order, which "
+"means that a start position of 0 refers to the last instruction "
+"chronologically.">;

Is there a "current position" that is maintained? Or is the current position 
where TraceThread currently is stopped?



Comment at: lldb/source/Plugins/Process/Trace/ProcessTrace.h:18
+
+class ProcessTrace : public lldb_private::Process {
+public:

So one issue is how do we eventually deal with debugging a live process that 
enables tracing. In that case we already have a real process class: 
ProcessGDBRemote most likely. We should avoid putting anything custom that is 
required from a process in this ProcessTrace class for when we actually have a 
real process class already. If we need to add anything, we will need to have 
virtual functions on the lldb_private::Process class that can call through to 
the Trace plug-in via its virtual functions as well to implement any 
functionality we might need.

Is this class solely going to be used for "trace load"?



Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:46
+self.expect("thread trace dump instructions --count 5 --start-position 
10",
+substrs=['thread #1: tid = 3842849',
+ 'would print 5 instructions from position 10'])

need to test the repeat command stuff here if we add support for:
```
const char *GetRepeatCommand(Args ¤t_command_args, uint32_t index) 
override {
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88769

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

[Lldb-commits] [PATCH] D87868: [RFC] When calling the process mmap try to call all found instead of just the first one

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Glad the other mmap was not marked external, that makes this much easier.




Comment at: lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp:54
+if (sc_list.GetContextAtIndex(i, sc) &&
+(sc.symbol->IsExternal() || sc.symbol->IsWeak())) {
   const uint32_t range_scope =

Why are we checking "IsWeak()" here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87868

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

[Lldb-commits] [PATCH] D88753: Fix raciness in the check for whether a stop hook has run the target

[Jim Ingham 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 rGbe66987e2047: Fix raciness in the StopHook check for 
"has the target run". (authored by jingham).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88753

Files:
  lldb/include/lldb/Target/Target.h
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py

Index: lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
===
--- lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
+++ lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
@@ -71,8 +71,6 @@
 """Test that the returning False from a stop hook works"""
 self.do_test_auto_continue(True)
 
-# Test is flakey on Linux.
-@skipIfLinux
 def do_test_auto_continue(self, return_true):
 """Test that auto-continue works."""
 # We set auto-continue to 1 but the stop hook only applies to step_out_of_me,
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2541,25 +2541,26 @@
   }
 }
 
-void Target::RunStopHooks() {
+bool Target::RunStopHooks() {
   if (m_suppress_stop_hooks)
-return;
+return false;
 
   if (!m_process_sp)
-return;
+return false;
 
   // Somebody might have restarted the process:
+  // Still return false, the return value is about US restarting the target.
   if (m_process_sp->GetState() != eStateStopped)
-return;
+return false;
 
   //  make sure we check that we are not stopped
   // because of us running a user expression since in that case we do not want
   // to run the stop-hooks
   if (m_process_sp->GetModIDRef().IsLastResumeForUserExpression())
-return;
+return false;
 
   if (m_stop_hooks.empty())
-return;
+return false;
 
   // If there aren't any active stop hooks, don't bother either.
   bool any_active_hooks = false;
@@ -2570,7 +2571,7 @@
 }
   }
   if (!any_active_hooks)
-return;
+return false;
 
   std::vector exc_ctx_with_reasons;
 
@@ -2588,7 +2589,7 @@
   // If no threads stopped for a reason, don't run the stop-hooks.
   size_t num_exe_ctx = exc_ctx_with_reasons.size();
   if (num_exe_ctx == 0)
-return;
+return false;
 
   StreamSP output_sp = m_debugger.GetAsyncOutputStream();
 
@@ -2636,22 +2637,27 @@
 output_sp->Printf("-- Thread %d\n",
   exc_ctx.GetThreadPtr()->GetIndexID());
 
-  bool this_should_stop = cur_hook_sp->HandleStop(exc_ctx, output_sp);
-  // If this hook is set to auto-continue that should override the
-  // HandleStop result...
-  if (cur_hook_sp->GetAutoContinue())
-this_should_stop = false;
+  StopHook::StopHookResult this_result =
+  cur_hook_sp->HandleStop(exc_ctx, output_sp);
+  bool this_should_stop = true;
 
-  // If anybody wanted to stop, we should all stop.
-  if (!should_stop)
-should_stop = this_should_stop;
+  switch (this_result) {
+  case StopHook::StopHookResult::KeepStopped:
+// If this hook is set to auto-continue that should override the
+// HandleStop result...
+if (cur_hook_sp->GetAutoContinue())
+  this_should_stop = false;
+else
+  this_should_stop = true;
 
-  // We don't have a good way to prohibit people from restarting the target
-  // willy nilly in a stop hook.  So see if the private state is running
-  // here and bag out if it is.
-  // FIXME: when we are doing non-stop mode for realz we'll have to instead
-  // track each thread, and only bag out if a thread is set running.
-  if (m_process_sp->GetPrivateState() != eStateStopped) {
+break;
+  case StopHook::StopHookResult::RequestContinue:
+this_should_stop = false;
+break;
+  case StopHook::StopHookResult::AlreadyContinued:
+// We don't have a good way to prohibit people from restarting the
+// target willy nilly in a stop hook.  If the hook did so, give a
+// gentle suggestion here and bag out if the hook processing.
 output_sp->Printf("\nAborting stop hooks, hook %" PRIu64
   " set the program running.\n"
   "  Consider using '-G true' to make "
@@ -2660,16 +2666,42 @@
 somebody_restarted = true;
 break;
   }
+  // If we're already restarted, stop processing stop hooks.
+  // FIXME: if we are doing non-stop mode for real, we would have to
+  // check that OUR thread was restarted, otherwise we should keep
+  // processing stop hooks.
+  if (somebody_restarted)
+break;
+
+  // If a

[Lldb-commits] [lldb] be66987 - Fix raciness in the StopHook check for "has the target run".

[Jim Ingham via lldb-commits]

Author: Jim Ingham
Date: 2020-10-05T15:44:28-07:00
New Revision: be66987e2047636d9ed9d2a4d88b762d59ae88f2

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

LOG: Fix raciness in the StopHook check for "has the target run".

This was looking at the privateState, but it's possible that
the actual process has started up and then stopped again by the
time we get to the check, which would lead us to get out of running
the stop hooks too early.

Instead we need to track the intention of the stop hooks directly.

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

Added: 


Modified: 
lldb/include/lldb/Target/Target.h
lldb/source/Target/Process.cpp
lldb/source/Target/Target.cpp
lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py

Removed: 




diff  --git a/lldb/include/lldb/Target/Target.h 
b/lldb/include/lldb/Target/Target.h
index 94c6ebeac10d..7ee27a9776d5 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1145,6 +1145,11 @@ class Target : public 
std::enable_shared_from_this,
 virtual ~StopHook() = default;
 
 enum class StopHookKind  : uint32_t { CommandBased = 0, ScriptBased };
+enum class StopHookResult : uint32_t {
+  KeepStopped = 0,
+  RequestContinue,
+  AlreadyContinued
+};
 
 lldb::TargetSP &GetTarget() { return m_target_sp; }
 
@@ -1160,8 +1165,8 @@ class Target : public 
std::enable_shared_from_this,
 // with a reason" thread.  It should add to the stream whatever text it
 // wants to show the user, and return False to indicate it wants the target
 // not to stop.
-virtual bool HandleStop(ExecutionContext &exe_ctx,
-lldb::StreamSP output) = 0;
+virtual StopHookResult HandleStop(ExecutionContext &exe_ctx,
+  lldb::StreamSP output) = 0;
 
 // Set the Thread Specifier.  The stop hook will own the thread specifier,
 // and is responsible for deleting it when we're done.
@@ -1201,8 +1206,8 @@ class Target : public 
std::enable_shared_from_this,
 void SetActionFromString(const std::string &strings);
 void SetActionFromStrings(const std::vector &strings);
 
-bool HandleStop(ExecutionContext &exc_ctx,
-lldb::StreamSP output_sp) override;
+StopHookResult HandleStop(ExecutionContext &exc_ctx,
+  lldb::StreamSP output_sp) override;
 void GetSubclassDescription(Stream *s,
 lldb::DescriptionLevel level) const override;
 
@@ -1219,7 +1224,8 @@ class Target : public 
std::enable_shared_from_this,
   class StopHookScripted : public StopHook {
   public:
 virtual ~StopHookScripted() = default;
-bool HandleStop(ExecutionContext &exc_ctx, lldb::StreamSP output) override;
+StopHookResult HandleStop(ExecutionContext &exc_ctx,
+  lldb::StreamSP output) override;
 
 Status SetScriptCallback(std::string class_name,
  StructuredData::ObjectSP extra_args_sp);
@@ -1254,7 +1260,9 @@ class Target : public 
std::enable_shared_from_this,
   /// remove the stop hook, as it will also reset the stop hook counter.
   void UndoCreateStopHook(lldb::user_id_t uid);
 
-  void RunStopHooks();
+  // Runs the stop hooks that have been registered for this target.
+  // Returns true if the stop hooks cause the target to resume.
+  bool RunStopHooks();
 
   size_t GetStopHookSize();
 

diff  --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index c602511daedc..490ca45bfee2 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -4178,8 +4178,7 @@ void Process::ProcessEventData::DoOnRemoval(Event 
*event_ptr) {
   // public (or SyncResume) broadcasters.  StopHooks are just for
   // real public stops.  They might also restart the target,
   // so watch for that.
-  process_sp->GetTarget().RunStopHooks();
-  if (process_sp->GetPrivateState() == eStateRunning)
+  if (process_sp->GetTarget().RunStopHooks())
 SetRestarted(true);
 }
   }

diff  --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index a5250ddcef74..49af6c297cbc 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -2541,25 +2541,26 @@ void Target::SetAllStopHooksActiveState(bool 
active_state) {
   }
 }
 
-void Target::RunStopHooks() {
+bool Target::RunStopHooks() {
   if (m_suppress_stop_hooks)
-return;
+return false;
 
   if (!m_process_sp)
-return;
+return false;
 
   // Somebody might have restarted the process:
+  // Still return false, the return value is about US restarting the target.
   if (m_process_sp->GetState() != eStateStopped)
-retu

[Lldb-commits] [PATCH] D87868: [RFC] When calling the process mmap try to call all found instead of just the first one

[António Afonso via Phabricator via lldb-commits]

aadsm added inline comments.



Comment at: lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp:54
+if (sc_list.GetContextAtIndex(i, sc) &&
+(sc.symbol->IsExternal() || sc.symbol->IsWeak())) {
   const uint32_t range_scope =

clayborg wrote:
> Why are we checking "IsWeak()" here?
A weak symbol is also an external symbol, but it's weak in the sense that 
another external symbol with the same name will take precedence over it (as far 
as I understood).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87868

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

[Lldb-commits] [PATCH] D88840: [dotest] Simplify logic to find the Python path

[Jordan Rupprecht via Phabricator via lldb-commits]

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



Comment at: lldb/packages/Python/lldbsuite/test/dotest.py:550-551
+# If our lldb supports the -P option, use it to find the python path:
+with open(os.devnull, 'w') as DEVNULL:
+lldb_dash_p_result = 
subprocess.check_output([lldbtest_config.lldbExec, "-P"], stderr=DEVNULL, 
universal_newlines=True)
+if lldb_dash_p_result:

Is DEVNULL necessary? I think `stderr=None` might work just as well.

FWIW, subprocess.DEVNULL exists as of python 3.3, but I guess we still have 
python2 users.


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

https://reviews.llvm.org/D88840

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

[Lldb-commits] [PATCH] D88840: [dotest] Simplify logic to find the Python path

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked an inline comment as done.
JDevlieghere added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/dotest.py:550-551
+# If our lldb supports the -P option, use it to find the python path:
+with open(os.devnull, 'w') as DEVNULL:
+lldb_dash_p_result = 
subprocess.check_output([lldbtest_config.lldbExec, "-P"], stderr=DEVNULL, 
universal_newlines=True)
+if lldb_dash_p_result:

rupprecht wrote:
> Is DEVNULL necessary? I think `stderr=None` might work just as well.
> 
> FWIW, subprocess.DEVNULL exists as of python 3.3, but I guess we still have 
> python2 users.
Neat, `None` is the default argument so I'll just omit it. 


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

https://reviews.llvm.org/D88840

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

[Lldb-commits] [PATCH] D88866: [lldb] Add another fallback to GetXcodeSDK

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added a reviewer: aprantl.
JDevlieghere requested review of this revision.

If we tried getting the SDK from `xcrun` with the developer dir set and it 
failed, try without the developer dir.


https://reviews.llvm.org/D88866

Files:
  lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm


Index: lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
===
--- lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
+++ lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
@@ -373,26 +373,13 @@
 static std::string GetXcodeSDK(XcodeSDK sdk) {
   XcodeSDK::Info info = sdk.Parse();
   std::string sdk_name = XcodeSDK::GetCanonicalName(info);
-  auto find_sdk = [](std::string sdk_name) -> std::string {
-std::string xcrun_cmd;
-std::string developer_dir = GetEnvDeveloperDir();
-if (developer_dir.empty())
-  if (FileSpec fspec = HostInfo::GetShlibDir())
-if (FileSystem::Instance().Exists(fspec)) {
-  FileSpec path(
-  XcodeSDK::FindXcodeContentsDirectoryInPath(fspec.GetPath()));
-  if (path.RemoveLastPathComponent())
-developer_dir = path.GetPath();
-}
-if (!developer_dir.empty())
-  xcrun_cmd = "/usr/bin/env DEVELOPER_DIR=\"" + developer_dir + "\" ";
-xcrun_cmd += "xcrun --show-sdk-path --sdk " + sdk_name;
 
+  auto xcrun = [](llvm::StringRef xcrun_cmd) -> std::string {
 int status = 0;
 int signo = 0;
 std::string output_str;
 lldb_private::Status error =
-Host::RunShellCommand(xcrun_cmd.c_str(), FileSpec(), &status, &signo,
+Host::RunShellCommand(xcrun_cmd, FileSpec(), &status, &signo,
   &output_str, std::chrono::seconds(15));
 
 // Check that xcrun return something useful.
@@ -414,6 +401,32 @@
 return output.str();
   };
 
+  auto find_sdk = [&](std::string sdk_name) -> std::string {
+std::string developer_dir = GetEnvDeveloperDir();
+if (developer_dir.empty())
+  if (FileSpec fspec = HostInfo::GetShlibDir())
+if (FileSystem::Instance().Exists(fspec)) {
+  FileSpec path(
+  XcodeSDK::FindXcodeContentsDirectoryInPath(fspec.GetPath()));
+  if (path.RemoveLastPathComponent())
+developer_dir = path.GetPath();
+}
+
+std::string xcrun_cmd = "xcrun --show-sdk-path --sdk " + sdk_name;
+
+// If we have a developer_dir try that first.
+if (!developer_dir.empty()) {
+  std::string env_xcrun_cmd =
+  "/usr/bin/env DEVELOPER_DIR=\"" + developer_dir + "\" " + xcrun_cmd;
+  std::string output_str = xcrun(env_xcrun_cmd);
+  if (!output_str.empty())
+return output_str;
+}
+
+// If we didn't find the SDK with the developer dir set, try without.
+return xcrun(xcrun_cmd);
+  };
+
   std::string path = find_sdk(sdk_name);
   while (path.empty()) {
 // Try an alternate spelling of the name ("macosx10.9internal").


Index: lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
===
--- lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
+++ lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
@@ -373,26 +373,13 @@
 static std::string GetXcodeSDK(XcodeSDK sdk) {
   XcodeSDK::Info info = sdk.Parse();
   std::string sdk_name = XcodeSDK::GetCanonicalName(info);
-  auto find_sdk = [](std::string sdk_name) -> std::string {
-std::string xcrun_cmd;
-std::string developer_dir = GetEnvDeveloperDir();
-if (developer_dir.empty())
-  if (FileSpec fspec = HostInfo::GetShlibDir())
-if (FileSystem::Instance().Exists(fspec)) {
-  FileSpec path(
-  XcodeSDK::FindXcodeContentsDirectoryInPath(fspec.GetPath()));
-  if (path.RemoveLastPathComponent())
-developer_dir = path.GetPath();
-}
-if (!developer_dir.empty())
-  xcrun_cmd = "/usr/bin/env DEVELOPER_DIR=\"" + developer_dir + "\" ";
-xcrun_cmd += "xcrun --show-sdk-path --sdk " + sdk_name;
 
+  auto xcrun = [](llvm::StringRef xcrun_cmd) -> std::string {
 int status = 0;
 int signo = 0;
 std::string output_str;
 lldb_private::Status error =
-Host::RunShellCommand(xcrun_cmd.c_str(), FileSpec(), &status, &signo,
+Host::RunShellCommand(xcrun_cmd, FileSpec(), &status, &signo,
   &output_str, std::chrono::seconds(15));
 
 // Check that xcrun return something useful.
@@ -414,6 +401,32 @@
 return output.str();
   };
 
+  auto find_sdk = [&](std::string sdk_name) -> std::string {
+std::string developer_dir = GetEnvDeveloperDir();
+if (developer_dir.empty())
+  if (FileSpec fspec = HostInfo::GetShlibDir())
+if (FileSystem::Instance().Exists(fspec)) {
+  FileSpec path(
+  XcodeSDK::FindXcodeContentsDirectoryInPath(fspec.GetPath()));
+  if (path.RemoveLastPathComponent())
+developer_dir = path.GetPa

[Lldb-commits] [lldb] f22496a - [dotest] Simplify logic to find the Python path

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-10-05T19:04:33-07:00
New Revision: f22496a9f4cabb97e735314b62731fedb2e01e50

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

LOG: [dotest] Simplify logic to find the Python path

Simplify the logic of parsing the lldb -P output to find the python
path. This removes the special handling for the LLDB.framework case and
instead of pattern matching known errors focus on finding a directory
path that contains an __init__.py.

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

Added: 


Modified: 
lldb/packages/Python/lldbsuite/test/dotest.py

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/dotest.py 
b/lldb/packages/Python/lldbsuite/test/dotest.py
index b4eddda91403..922d7c9377ee 100644
--- a/lldb/packages/Python/lldbsuite/test/dotest.py
+++ b/lldb/packages/Python/lldbsuite/test/dotest.py
@@ -545,68 +545,33 @@ def setupSysPath():
 configuration.skip_categories.append("lldb-vscode")
 
 lldbPythonDir = None  # The directory that contains 'lldb/__init__.py'
-if configuration.lldb_framework_path:
-lldbtest_config.lldb_framework_path = configuration.lldb_framework_path
-candidatePath = os.path.join(
-configuration.lldb_framework_path, 'Resources', 'Python')
-if os.path.isfile(os.path.join(candidatePath, 'lldb/__init__.py')):
-lldbPythonDir = candidatePath
-if not lldbPythonDir:
-print(
-'Resources/Python/lldb/__init__.py was not found in ' +
-configuration.lldb_framework_path)
-sys.exit(-1)
-else:
-# If our lldb supports the -P option, use it to find the python path:
-init_in_python_dir = os.path.join('lldb', '__init__.py')
-
-lldb_dash_p_result = subprocess.check_output(
-[lldbtest_config.lldbExec, "-P"], stderr=subprocess.STDOUT, 
universal_newlines=True)
-
-if lldb_dash_p_result and not lldb_dash_p_result.startswith(
-("<", "lldb: invalid option:")) and not 
lldb_dash_p_result.startswith("Traceback"):
-lines = lldb_dash_p_result.splitlines()
-
-# Workaround for readline vs libedit issue on FreeBSD.  If stdout
-# is not a terminal Python executes
-# rl_variable_bind ("enable-meta-key", "off");
-# This produces a warning with FreeBSD's libedit because the
-# enable-meta-key variable is unknown.  Not an issue on Apple
-# because cpython commit f0ab6f9f0603 added a #ifndef __APPLE__
-# around the call.  See http://bugs.python.org/issue19884 for more
-# information.  For now we just discard the warning output.
-if len(lines) >= 1 and lines[0].startswith(
-"bind: Invalid command"):
-lines.pop(0)
-
-# Taking the last line because lldb outputs
-# 'Cannot read termcap database;\nusing dumb terminal settings.\n'
-# before the path
-if len(lines) >= 1 and os.path.isfile(
-os.path.join(lines[-1], init_in_python_dir)):
-lldbPythonDir = lines[-1]
-if "freebsd" in sys.platform or "linux" in sys.platform:
-os.environ['LLDB_LIB_DIR'] = os.path.join(
-lldbPythonDir, '..', '..')
-
-if not lldbPythonDir:
-print(
-"Unable to load lldb extension module.  Possible reasons for 
this include:")
-print("  1) LLDB was built with LLDB_ENABLE_PYTHON=0")
-print(
-"  2) PYTHONPATH and PYTHONHOME are not set correctly.  
PYTHONHOME should refer to")
-print(
-" the version of Python that LLDB built and linked 
against, and PYTHONPATH")
-print(
-" should contain the Lib directory for the same python 
distro, as well as the")
-print(" location of LLDB\'s site-packages folder.")
-print(
-"  3) A 
diff erent version of Python than that which was built against is exported in")
-print(" the system\'s PATH environment variable, causing 
conflicts.")
-print(
-"  4) The executable '%s' could not be found.  Please check " %
-lldbtest_config.lldbExec)
-print(" that it exists and is executable.")
+
+# If our lldb supports the -P option, use it to find the python path:
+lldb_dash_p_result = subprocess.check_output([lldbtest_config.lldbExec, 
"-P"], universal_newlines=True)
+if lldb_dash_p_result:
+for line in lldb_dash_p_result.splitlines():
+if os.path.isdir(line) and os.path.exists(os.path.joi

[Lldb-commits] [PATCH] D88840: [dotest] Simplify logic to find the Python path

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
JDevlieghere marked an inline comment as done.
Closed by commit rGf22496a9f4ca: [dotest] Simplify logic to find the Python 
path (authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D88840?vs=296239&id=296345#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88840

Files:
  lldb/packages/Python/lldbsuite/test/dotest.py

Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -545,68 +545,33 @@
 configuration.skip_categories.append("lldb-vscode")
 
 lldbPythonDir = None  # The directory that contains 'lldb/__init__.py'
-if configuration.lldb_framework_path:
-lldbtest_config.lldb_framework_path = configuration.lldb_framework_path
-candidatePath = os.path.join(
-configuration.lldb_framework_path, 'Resources', 'Python')
-if os.path.isfile(os.path.join(candidatePath, 'lldb/__init__.py')):
-lldbPythonDir = candidatePath
-if not lldbPythonDir:
-print(
-'Resources/Python/lldb/__init__.py was not found in ' +
-configuration.lldb_framework_path)
-sys.exit(-1)
-else:
-# If our lldb supports the -P option, use it to find the python path:
-init_in_python_dir = os.path.join('lldb', '__init__.py')
-
-lldb_dash_p_result = subprocess.check_output(
-[lldbtest_config.lldbExec, "-P"], stderr=subprocess.STDOUT, universal_newlines=True)
-
-if lldb_dash_p_result and not lldb_dash_p_result.startswith(
-("<", "lldb: invalid option:")) and not lldb_dash_p_result.startswith("Traceback"):
-lines = lldb_dash_p_result.splitlines()
-
-# Workaround for readline vs libedit issue on FreeBSD.  If stdout
-# is not a terminal Python executes
-# rl_variable_bind ("enable-meta-key", "off");
-# This produces a warning with FreeBSD's libedit because the
-# enable-meta-key variable is unknown.  Not an issue on Apple
-# because cpython commit f0ab6f9f0603 added a #ifndef __APPLE__
-# around the call.  See http://bugs.python.org/issue19884 for more
-# information.  For now we just discard the warning output.
-if len(lines) >= 1 and lines[0].startswith(
-"bind: Invalid command"):
-lines.pop(0)
-
-# Taking the last line because lldb outputs
-# 'Cannot read termcap database;\nusing dumb terminal settings.\n'
-# before the path
-if len(lines) >= 1 and os.path.isfile(
-os.path.join(lines[-1], init_in_python_dir)):
-lldbPythonDir = lines[-1]
-if "freebsd" in sys.platform or "linux" in sys.platform:
-os.environ['LLDB_LIB_DIR'] = os.path.join(
-lldbPythonDir, '..', '..')
-
-if not lldbPythonDir:
-print(
-"Unable to load lldb extension module.  Possible reasons for this include:")
-print("  1) LLDB was built with LLDB_ENABLE_PYTHON=0")
-print(
-"  2) PYTHONPATH and PYTHONHOME are not set correctly.  PYTHONHOME should refer to")
-print(
-" the version of Python that LLDB built and linked against, and PYTHONPATH")
-print(
-" should contain the Lib directory for the same python distro, as well as the")
-print(" location of LLDB\'s site-packages folder.")
-print(
-"  3) A different version of Python than that which was built against is exported in")
-print(" the system\'s PATH environment variable, causing conflicts.")
-print(
-"  4) The executable '%s' could not be found.  Please check " %
-lldbtest_config.lldbExec)
-print(" that it exists and is executable.")
+
+# If our lldb supports the -P option, use it to find the python path:
+lldb_dash_p_result = subprocess.check_output([lldbtest_config.lldbExec, "-P"], universal_newlines=True)
+if lldb_dash_p_result:
+for line in lldb_dash_p_result.splitlines():
+if os.path.isdir(line) and os.path.exists(os.path.join(line, 'lldb', '__init__.py')):
+lldbPythonDir = line
+break
+
+if not lldbPythonDir:
+print(
+"Unable to load lldb extension module.  Possible reasons for this include:")
+print("  1) LLDB was built with LLDB_ENABLE_PYTHON=0")
+print(
+"  2) PYTHONPATH and PYTHONHOME ar

[Lldb-commits] [PATCH] D88840: [dotest] Simplify logic to find the Python path

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

About `-P`, the man page for `lldb` and the driver's `Options.td` say it:

> Prints out the path to the lldb.py file for this version of lldb.

Should it do just that? If so this can be simplified further.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88840

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

[Lldb-commits] [PATCH] D88866: [lldb] Add another fallback to GetXcodeSDK

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

So this is adding a fallback that ignores DEVELOPER_DIR if it is set, but 
doesn't exist? Is that really desirable, or am I misunderstanding the patch? I 
would expect LLDB to fail hard if the DEVELOPER_DIR in the environment is wrong 
and not silently ignore it...




Comment at: lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm:389
 
 // Convert to a StringRef so we can manipulate the string without modifying
 // the underlying data.

Can we delete this comment? I find it more confusing than helpful...


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

https://reviews.llvm.org/D88866

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

[Lldb-commits] [PATCH] D88866: [lldb] Add another fallback to GetXcodeSDK

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D88866#2313371 , @aprantl wrote:

> So this is adding a fallback that ignores DEVELOPER_DIR if it is set, but 
> doesn't exist? Is that really desirable, or am I misunderstanding the patch? 
> I would expect LLDB to fail hard if the DEVELOPER_DIR in the environment is 
> wrong and not silently ignore it...

It's slightly more subtle than that, it falls back to running `xcrun` without a 
developer dir if we couldn't find the desired SDK with the developer dir set. 
It doesn't mean the developer dir is invalid or doesn't exist, it just means we 
couldn't find the SDK there. Imagine for the sake of argument that you're 
running an Xcode that has no SDKs and that this Xcode is not `xcode-select`ed. 
Based on lldb's location in LLDB.framework it will do an `xcrun` with the 
developer set to the Xcode without any SDKs and that will fail. With this 
patch, when that happens, we'll fall back to trying the `xcode-select`ed by 
running `xcrun` without a developer dir specified. I know it sounds like a 
contrived example but that's exactly what I was running into...


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

https://reviews.llvm.org/D88866

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

[Lldb-commits] [PATCH] D88870: [lldb][NFC] remove unused local variable uuid

[Zequan Wu via Phabricator via lldb-commits]

zequanwu created this revision.
zequanwu added reviewers: amccarth, labath.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
zequanwu requested review of this revision.
Herald added a subscriber: JDevlieghere.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88870

Files:
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp


Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -187,9 +187,6 @@
 
   ModuleSpec module_spec(file);
   ArchSpec &spec = module_spec.GetArchitecture();
-  lldb_private::UUID &uuid = module_spec.GetUUID();
-  if (!uuid.IsValid())
-uuid = GetCoffUUID(*COFFObj);
 
   switch (COFFObj->getMachine()) {
   case MachineAmd64:


Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -187,9 +187,6 @@
 
   ModuleSpec module_spec(file);
   ArchSpec &spec = module_spec.GetArchitecture();
-  lldb_private::UUID &uuid = module_spec.GetUUID();
-  if (!uuid.IsValid())
-uuid = GetCoffUUID(*COFFObj);
 
   switch (COFFObj->getMachine()) {
   case MachineAmd64:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D87868: [RFC] When calling the process mmap try to call all found instead of just the first one

[Greg Clayton via Phabricator via lldb-commits]

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

We probably don't need to check for IsWeak() as we wouldn't want an internal + 
weak symbol (if there is such a thing) to match and be used.




Comment at: lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp:36
 
 bool lldb_private::InferiorCallMmap(Process *process, addr_t &allocated_addr,
 addr_t addr, addr_t length, unsigned prot,

Might be nice to return a llvm::Error here instead of pool? Maybe to help 
explain what went wrong? like any of:

"thread required to call mmap"
"couldn't find any symbols named 'mmap'"
"no external symbols named 'mmap' were found"
"mmap call failed" (for when it returns UINT32_MAX for 4 byte addresses or 
UINT64_MAX for 8 byte addresses"

It have been nice to see a nice error message during the expression evaluation 
prior to this fix. At the very least we should log to the expression log 
channel that mmap failed if we choose not to return an error.



Comment at: lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp:54
+if (sc_list.GetContextAtIndex(i, sc) &&
+(sc.symbol->IsExternal() || sc.symbol->IsWeak())) {
   const uint32_t range_scope =

aadsm wrote:
> clayborg wrote:
> > Why are we checking "IsWeak()" here?
> A weak symbol is also an external symbol, but it's weak in the sense that 
> another external symbol with the same name will take precedence over it (as 
> far as I understood).
I think we only need to check for external here. Any weak symbol will also need 
to be external, but if it isn't we don't want that symbol.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87868

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