[Lldb-commits] [PATCH] D126367: [lldb] Add gnu-debuglink support for Windows PE/COFF

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added inline comments.



Comment at: lldb/source/Plugins/SymbolVendor/PECOFF/SymbolVendorPECOFF.cpp:119
+  };
+  for (SectionType section_type : g_sections) {
+if (SectionSP section_sp =

I'm curious - this adds new logic (copied from SymbolVendorELF afaik?) for 
iterating over sections and finding the ones that contain the dwarf debug info. 
Prior to this change, finding debug info within the executable itself has 
worked just fine.

What codepath and where has handled that? Has it fallen back on SymbolVendorELF 
so far?

(If that used to work, why is a specific plugin for PECOFF needed at this point 
for debuglink?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126367

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

[Lldb-commits] [PATCH] D126657: [lldb] Fix loading DLL from some ramdisk on Windows

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a reviewer: labath.
mstorsjo added a comment.

The implementation looks reasonable to me (I didn't investigate alternative 
ways of doing it but trust the reasoning that this is the most reasonable way 
of finding the pathname of an open handle with this filesystem driver).




Comment at: lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp:427
+
+  auto view_deleter = [](void *pMem) { ::UnmapViewOfFile(pMem); };
+  std::unique_ptr pMem(

Do you need a check against `NULL` here in `view_deleter`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126657

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

[Lldb-commits] [PATCH] D126657: [lldb] Fix loading DLL from some ramdisk on Windows

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added inline comments.



Comment at: lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp:427
+
+  auto view_deleter = [](void *pMem) { ::UnmapViewOfFile(pMem); };
+  std::unique_ptr pMem(

mstorsjo wrote:
> Do you need a check against `NULL` here in `view_deleter`?
I think `unique_ptr` guarantees the deleter won't be called if the pointer is 
null, so I didn't put a null check here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126657

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

[Lldb-commits] [PATCH] D126657: [lldb] Fix loading DLL from some ramdisk on Windows

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added inline comments.



Comment at: lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp:427
+
+  auto view_deleter = [](void *pMem) { ::UnmapViewOfFile(pMem); };
+  std::unique_ptr pMem(

alvinhochun wrote:
> mstorsjo wrote:
> > Do you need a check against `NULL` here in `view_deleter`?
> I think `unique_ptr` guarantees the deleter won't be called if the pointer is 
> null, so I didn't put a null check here.
Sounds plausible - I tried browsing the documentation for that but didn't find 
it spelled out explicitly (in the couple minutes I was browsing at least).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126657

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

[Lldb-commits] [PATCH] D126367: [lldb] Add gnu-debuglink support for Windows PE/COFF

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added inline comments.



Comment at: lldb/source/Plugins/SymbolVendor/PECOFF/SymbolVendorPECOFF.cpp:119
+  };
+  for (SectionType section_type : g_sections) {
+if (SectionSP section_sp =

mstorsjo wrote:
> I'm curious - this adds new logic (copied from SymbolVendorELF afaik?) for 
> iterating over sections and finding the ones that contain the dwarf debug 
> info. Prior to this change, finding debug info within the executable itself 
> has worked just fine.
> 
> What codepath and where has handled that? Has it fallen back on 
> SymbolVendorELF so far?
> 
> (If that used to work, why is a specific plugin for PECOFF needed at this 
> point for debuglink?)
The SymbolVendorELF seems to only handle loading the external debug info 
specified by the `.gnu_debuglink` section. The dwarf debug info already 
embedded in the main executable should be handled somewhere else, which has 
worked fine for both ELF and PE/COFF. Though I don't know where specifically 
this was handled.

SymbolVendorELF tries to downcast the object file to `ObjectFileELF *`, so it 
could not have worked for PE/COFF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126367

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

[Lldb-commits] [PATCH] D126367: [lldb] Add gnu-debuglink support for Windows PE/COFF

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added inline comments.



Comment at: lldb/source/Plugins/SymbolVendor/PECOFF/SymbolVendorPECOFF.cpp:119
+  };
+  for (SectionType section_type : g_sections) {
+if (SectionSP section_sp =

alvinhochun wrote:
> mstorsjo wrote:
> > I'm curious - this adds new logic (copied from SymbolVendorELF afaik?) for 
> > iterating over sections and finding the ones that contain the dwarf debug 
> > info. Prior to this change, finding debug info within the executable itself 
> > has worked just fine.
> > 
> > What codepath and where has handled that? Has it fallen back on 
> > SymbolVendorELF so far?
> > 
> > (If that used to work, why is a specific plugin for PECOFF needed at this 
> > point for debuglink?)
> The SymbolVendorELF seems to only handle loading the external debug info 
> specified by the `.gnu_debuglink` section. The dwarf debug info already 
> embedded in the main executable should be handled somewhere else, which has 
> worked fine for both ELF and PE/COFF. Though I don't know where specifically 
> this was handled.
> 
> SymbolVendorELF tries to downcast the object file to `ObjectFileELF *`, so it 
> could not have worked for PE/COFF.
Ok, so what I misunderstood is that this function doesn't seem to handle dwarf 
debug info in the main executable after all - this only tries to dig up dwarf 
sections from `GetSymbolFileFileSpec()` or `GetDebugLink()`. So the existing 
code that locates dwarf sections in the executables themselves still runs as 
before.

So then this seems reasonable.

So essentially, if `GetDebugLink()` would be a virtual method, both 
SymbolVendorELF and SymbolVendorPECOFF could theoretically be merged into one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126367

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

[Lldb-commits] [PATCH] D126367: [lldb] Add gnu-debuglink support for Windows PE/COFF

[Alvin Wong via Phabricator via lldb-commits]

alvinhochun added inline comments.



Comment at: lldb/source/Plugins/SymbolVendor/PECOFF/SymbolVendorPECOFF.cpp:119
+  };
+  for (SectionType section_type : g_sections) {
+if (SectionSP section_sp =

mstorsjo wrote:
> alvinhochun wrote:
> > mstorsjo wrote:
> > > I'm curious - this adds new logic (copied from SymbolVendorELF afaik?) 
> > > for iterating over sections and finding the ones that contain the dwarf 
> > > debug info. Prior to this change, finding debug info within the 
> > > executable itself has worked just fine.
> > > 
> > > What codepath and where has handled that? Has it fallen back on 
> > > SymbolVendorELF so far?
> > > 
> > > (If that used to work, why is a specific plugin for PECOFF needed at this 
> > > point for debuglink?)
> > The SymbolVendorELF seems to only handle loading the external debug info 
> > specified by the `.gnu_debuglink` section. The dwarf debug info already 
> > embedded in the main executable should be handled somewhere else, which has 
> > worked fine for both ELF and PE/COFF. Though I don't know where 
> > specifically this was handled.
> > 
> > SymbolVendorELF tries to downcast the object file to `ObjectFileELF *`, so 
> > it could not have worked for PE/COFF.
> Ok, so what I misunderstood is that this function doesn't seem to handle 
> dwarf debug info in the main executable after all - this only tries to dig up 
> dwarf sections from `GetSymbolFileFileSpec()` or `GetDebugLink()`. So the 
> existing code that locates dwarf sections in the executables themselves still 
> runs as before.
> 
> So then this seems reasonable.
> 
> So essentially, if `GetDebugLink()` would be a virtual method, both 
> SymbolVendorELF and SymbolVendorPECOFF could theoretically be merged into one?
> So essentially, if GetDebugLink() would be a virtual method, both 
> SymbolVendorELF and SymbolVendorPECOFF could theoretically be merged into one?

Yes, they may very well be merged if no other platform-specific logic is 
needed. There are some small differences now: SymbolVendorELF checks that 
`obj_file->GetUUID()` is valid, but this fails the test I added so I removed 
this check in SymbolVendorPECOFF. I also removed a few entries (those that 
doesn't show up in `ObjectFilePECOFF`) from the `g_sections` list, but this 
change is probably not necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126367

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

[Lldb-commits] [PATCH] D126702: [lldb] Fix TCPSocket::Connect when getaddrinfo returns multiple addrs

[Daniele Di Proietto via Phabricator via lldb-commits]

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

TCPSocket::Connect() calls SocketAddress::GetAddressInfo() and tries to
connect any of them (in a for loop).

This used to work before commit 4f6d3a376c9f 
("[LLDB] 
Fix setting of
success in Socket::Close()") https://reviews.llvm.org/D116768.

As a side effect of that commit, TCPSocket can only connect to the first
address returned by SocketAddress::GetAddressInfo().

1. If the attempt to connect to the first address fails, TCPSocket::Connect(), 
calls CLOSE_SOCKET(GetNativeSocket()), which closes the fd, but DOES NOT set 
m_socket to kInvalidSocketValue.
2. On the second attempt, TCPSocket::CreateSocket() calls Socket::Close().
3. Socket::Close() proceeds, because IsValid() is true (m_socket was not reset 
on step 1).
4. Socket::Close() calls ::close(m_socket), which fails
5. Since commit 4f6d3a376c9f 
("[LLDB] 
Fix setting of success in Socket::Close()"), this is error is detected. 
Socket::Close() returns an error.
6. TCPSocket::CreateSocket() therefore returns an error.
7. TCPSocket::Connect() detects the error and continues, skipping the second 
(and the third, fourth...) address.

This commit fixes the problem by changing step 1: by calling
Socket::Close, instead of directly calling close(m_socket), m_socket is
also se to kInvalidSocketValue. On step 3, Socket::Close() is going to
return immediately and, on step 6, TCPSocket::CreateSocket() does not 
fail.

How to reproduce this problem:

On my system, getaddrinfo() resolves "localhost" to "::1" (first) and to
"127.0.0.1" (second).

Start a gdbserver that only listens on 127.0.0.1:

  gdbserver 127.0.0.1:2159 /bin/cat
  Process /bin/cat created; pid = 2146709
  Listening on port 2159

Start lldb and make it connect to "localhost:2159"

  ./bin/lldb
  (lldb) gdb-remote localhost:2159

Before 4f6d3a376c9f 
("[LLDB] 
Fix setting of success in Socket::Close()"),
this used to work. After that commit, it stopped working. This commit
fixes the problem.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126702

Files:
  lldb/source/Host/common/TCPSocket.cpp


Index: lldb/source/Host/common/TCPSocket.cpp
===
--- lldb/source/Host/common/TCPSocket.cpp
+++ lldb/source/Host/common/TCPSocket.cpp
@@ -170,7 +170,7 @@
 if (-1 == llvm::sys::RetryAfterSignal(-1, ::connect, GetNativeSocket(),
   &address.sockaddr(),
   address.GetLength())) {
-  CLOSE_SOCKET(GetNativeSocket());
+  Close();
   continue;
 }
 


Index: lldb/source/Host/common/TCPSocket.cpp
===
--- lldb/source/Host/common/TCPSocket.cpp
+++ lldb/source/Host/common/TCPSocket.cpp
@@ -170,7 +170,7 @@
 if (-1 == llvm::sys::RetryAfterSignal(-1, ::connect, GetNativeSocket(),
   &address.sockaddr(),
   address.GetLength())) {
-  CLOSE_SOCKET(GetNativeSocket());
+  Close();
   continue;
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D126464: [lldb] Add support to load object files from thin archives

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D126464#3545142 , @PRESIDENT810 
wrote:

> I have refactored my code so it should looks cleaner now, but I'm not sure 
> how to add a test. It seems that adding a test for thin archive on macOS 
> platforms can be not so straightforward. I see that in 
> lldb/test/API/functionalities/archives/Makefile, the test suite is using 
> libtool instead of ar to create archives on macOS platforms, and I don't know 
> whether I can produce a thin archive with that. Also, ld on macOS platforms 
> seems to be unable to identify thin archives (I'm using ld64.lld when testing 
> my code locally).
>
> I would really appreciate it if someone can provide some advice about how to 
> implement such a test case here, thank you!

Does LLVM have the ability to generate a "llvm-ar" tool that can do the 
packaging? If so, an easy test would be to create a thin archive and then run 
the following API from within python:

  static SBModuleSpecList SBModuleSpecList::GetModuleSpecifications(const char 
*path);

If you run this API on a thin archive without your modifications, then you will 
probably get an empty SBModuleSpecList object. So the flow would be:

- make sure "llvm-ar" is built if it already isn't with the "check-lldb" target 
(might need to add it as a depedency
- use the build "llvm-ar" to make an archive that points to two .o files that 
you build from sources
- then run some python in your test case like:

  # Make the Makefile that uses the built "llvm-ar"  one two source files that 
have .o created for them, and then it runs "llvm-ar" and will produce the 
"thin.a" in the output directory
  
  # This line will point to the "thin.a" that was just built
  archive_path = self.getBuildArtifact("thin.a")
  
  # Get the module specs from the thin archive
  module_specs = lldb.SBModuleSpecList.GetModuleSpecifications(archive_path)
  num_specs = module_specs.GetSize()
  self.assertEqual(num_specs, 2)
  
  # For extra credit you can verify that you get the right file path from each 
file spec
  obj_path_1 = self.getBuildArtifact("foo.o")
  obj_path_2 = self.getBuildArtifact("bar.o")
  self.assertEqual(module_specs.GetSpecAtIndex(0).GetObjectName(), obj_path_1)
  self.assertEqual(module_specs.GetSpecAtIndex(1).GetObjectName(), obj_path_2)


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

https://reviews.llvm.org/D126464

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

[Lldb-commits] [PATCH] D126730: Fix a bug where `break com add -s py -o "some_python" BKPT_NAME` only added the command to the first breakpoint

[Jim Ingham via Phabricator via lldb-commits]

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

This was a "probably should have copy-pasted" error.  A "!" was omitted in 
copying over the code that added the command for a python function to the code 
that did the same for a python one-liner.  I also added a test for both adding 
python functions and one-liners.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126730

Files:
  lldb/source/Interpreter/ScriptInterpreter.cpp
  
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py


Index: 
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- 
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ 
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -283,6 +283,34 @@
 self.assertEqual(com_list.GetStringAtIndex(1), "thread list", "Next 
thread list")
 self.assertEqual(com_list.GetStringAtIndex(2), "continue", "Last 
continue")
 
+def test_add_commands_by_breakpoint_name(self):
+"""Make sure that when you specify a breakpoint name to "break command 
add"
+   it gets added to all the breakpoints marked with that name."""
+self.build()
+target = self.createTestTarget()
+
+bp_ids = []
+bp_names = ["main", "not_here", "main"]
+for bp_name in bp_names:
+bp = target.BreakpointCreateByName(bp_name)
+bp.AddName("MyBKPTS")
+bp_ids.append(bp.GetID())
+# First do it with a script one-liner:
+self.runCmd("breakpoint command add -s py -o 'print(\"some command\")' 
MyBKPTS")
+for id in bp_ids:
+self.expect("breakpoint command list {0}".format(id),
+patterns=["some command"])
+# Now do the same thing with a python function:
+import side_effect
+self.runCmd("command script import --allow-reload ./bktptcmd.py")
+
+self.runCmd("breakpoint command add --python-function 
bktptcmd.function MyBKPTS")
+for id in bp_ids:
+self.expect("breakpoint command list {0}".format(id),
+patterns=["bktptcmd.function"])
+
+
+
 def test_breakpoint_delete_disabled(self):
 """Test 'break delete --disabled' works"""
 self.build()
Index: lldb/source/Interpreter/ScriptInterpreter.cpp
===
--- lldb/source/Interpreter/ScriptInterpreter.cpp
+++ lldb/source/Interpreter/ScriptInterpreter.cpp
@@ -109,13 +109,13 @@
 Status ScriptInterpreter::SetBreakpointCommandCallback(
 std::vector> &bp_options_vec,
 const char *callback_text) {
-  Status return_error;
+  Status error;
   for (BreakpointOptions &bp_options : bp_options_vec) {
-return_error = SetBreakpointCommandCallback(bp_options, callback_text);
-if (return_error.Success())
+error = SetBreakpointCommandCallback(bp_options, callback_text);
+if (!error.Success())
   break;
   }
-  return return_error;
+  return error;
 }
 
 Status ScriptInterpreter::SetBreakpointCommandCallbackFunction(


Index: lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -283,6 +283,34 @@
 self.assertEqual(com_list.GetStringAtIndex(1), "thread list", "Next thread list")
 self.assertEqual(com_list.GetStringAtIndex(2), "continue", "Last continue")
 
+def test_add_commands_by_breakpoint_name(self):
+"""Make sure that when you specify a breakpoint name to "break command add"
+   it gets added to all the breakpoints marked with that name."""
+self.build()
+target = self.createTestTarget()
+
+bp_ids = []
+bp_names = ["main", "not_here", "main"]
+for bp_name in bp_names:
+bp = target.BreakpointCreateByName(bp_name)
+bp.AddName("MyBKPTS")
+bp_ids.append(bp.GetID())
+# First do it with a script one-liner:
+self.runCmd("breakpoint command add -s py -o 'print(\"some command\")' MyBKPTS")
+for id in bp_ids:
+self.expect("breakpoint command list {0}".format(id),
+patterns=["some command"])
+# Now do the same thing with a python function:
+import side_effect
+self.runCmd("command script import --allow-reload ./bktptcmd.py")
+
+self.runCmd("breakpoint command add --python-function bktptcmd.function MyBKPTS

[Lldb-commits] [PATCH] D126730: Fix a bug where `break com add -s py -o "some_python" BKPT_NAME` only added the command to the first breakpoint

[Dave Lee via Phabricator via lldb-commits]

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

thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126730

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

[Lldb-commits] [lldb] ca73de4 - Adapt LLDB for D120540.

[Adrian Prantl via lldb-commits]

Author: Adrian Prantl
Date: 2022-05-31T16:56:37-07:00
New Revision: ca73de43744503a557b1f3709c0ff4751798702f

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

LOG: Adapt LLDB for D120540.

In https://reviews.llvm.org/D120540 the -fcxx-modules flag changed
semantics and specifying it explicitly seems to no longer be what we
want here.

Added: 


Modified: 
lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
index 38dd55bc76d36..a75671049f61d 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
@@ -604,7 +604,6 @@ ClangModulesDeclVendor::Create(Target &target) {
   "clang",
   "-fmodules",
   "-fimplicit-module-maps",
-  "-fcxx-modules",
   "-fsyntax-only",
   "-femit-all-decls",
   "-target",



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

[Lldb-commits] [lldb] d92f7f7 - Fix a copy-paste error in "br com add -s py -o 'some_python' BKPT_NAME"

[Jim Ingham via lldb-commits]

Author: Jim Ingham
Date: 2022-05-31T17:24:14-07:00
New Revision: d92f7f790c8e74bf796a0313cb296d726628142e

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

LOG: Fix a copy-paste error in "br com add -s py -o 'some_python' BKPT_NAME"

The function that was supposed to iterate over all the breakpoints sharing
BKPT_NAME stopped after the first one because of a reversed "if success"
condition.

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

Added: 


Modified: 
lldb/source/Interpreter/ScriptInterpreter.cpp

lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py

Removed: 




diff  --git a/lldb/source/Interpreter/ScriptInterpreter.cpp 
b/lldb/source/Interpreter/ScriptInterpreter.cpp
index fbdcbb8da868..bc8a542afc87 100644
--- a/lldb/source/Interpreter/ScriptInterpreter.cpp
+++ b/lldb/source/Interpreter/ScriptInterpreter.cpp
@@ -109,13 +109,13 @@ ScriptInterpreter::StringToLanguage(const llvm::StringRef 
&language) {
 Status ScriptInterpreter::SetBreakpointCommandCallback(
 std::vector> &bp_options_vec,
 const char *callback_text) {
-  Status return_error;
+  Status error;
   for (BreakpointOptions &bp_options : bp_options_vec) {
-return_error = SetBreakpointCommandCallback(bp_options, callback_text);
-if (return_error.Success())
+error = SetBreakpointCommandCallback(bp_options, callback_text);
+if (!error.Success())
   break;
   }
-  return return_error;
+  return error;
 }
 
 Status ScriptInterpreter::SetBreakpointCommandCallbackFunction(

diff  --git 
a/lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
 
b/lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
index 508a8d9eec32..7a47062de985 100644
--- 
a/lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ 
b/lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -283,6 +283,34 @@ def breakpoint_commands_on_creation(self):
 self.assertEqual(com_list.GetStringAtIndex(1), "thread list", "Next 
thread list")
 self.assertEqual(com_list.GetStringAtIndex(2), "continue", "Last 
continue")
 
+def test_add_commands_by_breakpoint_name(self):
+"""Make sure that when you specify a breakpoint name to "break command 
add"
+   it gets added to all the breakpoints marked with that name."""
+self.build()
+target = self.createTestTarget()
+
+bp_ids = []
+bp_names = ["main", "not_here", "main"]
+for bp_name in bp_names:
+bp = target.BreakpointCreateByName(bp_name)
+bp.AddName("MyBKPTS")
+bp_ids.append(bp.GetID())
+# First do it with a script one-liner:
+self.runCmd("breakpoint command add -s py -o 'print(\"some command\")' 
MyBKPTS")
+for id in bp_ids:
+self.expect("breakpoint command list {0}".format(id),
+patterns=["some command"])
+# Now do the same thing with a python function:
+import side_effect
+self.runCmd("command script import --allow-reload ./bktptcmd.py")
+
+self.runCmd("breakpoint command add --python-function 
bktptcmd.function MyBKPTS")
+for id in bp_ids:
+self.expect("breakpoint command list {0}".format(id),
+patterns=["bktptcmd.function"])
+
+
+
 def test_breakpoint_delete_disabled(self):
 """Test 'break delete --disabled' works"""
 self.build()



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

[Lldb-commits] [PATCH] D126730: Fix a bug where `break com add -s py -o "some_python" BKPT_NAME` only added the command to the first breakpoint

[Jim Ingham via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd92f7f790c8e: Fix a copy-paste error in "br com add -s 
py -o 'some_python' BKPT_NAME" (authored by jingham).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126730

Files:
  lldb/source/Interpreter/ScriptInterpreter.cpp
  
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py


Index: 
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- 
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ 
lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -283,6 +283,34 @@
 self.assertEqual(com_list.GetStringAtIndex(1), "thread list", "Next 
thread list")
 self.assertEqual(com_list.GetStringAtIndex(2), "continue", "Last 
continue")
 
+def test_add_commands_by_breakpoint_name(self):
+"""Make sure that when you specify a breakpoint name to "break command 
add"
+   it gets added to all the breakpoints marked with that name."""
+self.build()
+target = self.createTestTarget()
+
+bp_ids = []
+bp_names = ["main", "not_here", "main"]
+for bp_name in bp_names:
+bp = target.BreakpointCreateByName(bp_name)
+bp.AddName("MyBKPTS")
+bp_ids.append(bp.GetID())
+# First do it with a script one-liner:
+self.runCmd("breakpoint command add -s py -o 'print(\"some command\")' 
MyBKPTS")
+for id in bp_ids:
+self.expect("breakpoint command list {0}".format(id),
+patterns=["some command"])
+# Now do the same thing with a python function:
+import side_effect
+self.runCmd("command script import --allow-reload ./bktptcmd.py")
+
+self.runCmd("breakpoint command add --python-function 
bktptcmd.function MyBKPTS")
+for id in bp_ids:
+self.expect("breakpoint command list {0}".format(id),
+patterns=["bktptcmd.function"])
+
+
+
 def test_breakpoint_delete_disabled(self):
 """Test 'break delete --disabled' works"""
 self.build()
Index: lldb/source/Interpreter/ScriptInterpreter.cpp
===
--- lldb/source/Interpreter/ScriptInterpreter.cpp
+++ lldb/source/Interpreter/ScriptInterpreter.cpp
@@ -109,13 +109,13 @@
 Status ScriptInterpreter::SetBreakpointCommandCallback(
 std::vector> &bp_options_vec,
 const char *callback_text) {
-  Status return_error;
+  Status error;
   for (BreakpointOptions &bp_options : bp_options_vec) {
-return_error = SetBreakpointCommandCallback(bp_options, callback_text);
-if (return_error.Success())
+error = SetBreakpointCommandCallback(bp_options, callback_text);
+if (!error.Success())
   break;
   }
-  return return_error;
+  return error;
 }
 
 Status ScriptInterpreter::SetBreakpointCommandCallbackFunction(


Index: lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
===
--- lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
+++ lldb/test/API/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py
@@ -283,6 +283,34 @@
 self.assertEqual(com_list.GetStringAtIndex(1), "thread list", "Next thread list")
 self.assertEqual(com_list.GetStringAtIndex(2), "continue", "Last continue")
 
+def test_add_commands_by_breakpoint_name(self):
+"""Make sure that when you specify a breakpoint name to "break command add"
+   it gets added to all the breakpoints marked with that name."""
+self.build()
+target = self.createTestTarget()
+
+bp_ids = []
+bp_names = ["main", "not_here", "main"]
+for bp_name in bp_names:
+bp = target.BreakpointCreateByName(bp_name)
+bp.AddName("MyBKPTS")
+bp_ids.append(bp.GetID())
+# First do it with a script one-liner:
+self.runCmd("breakpoint command add -s py -o 'print(\"some command\")' MyBKPTS")
+for id in bp_ids:
+self.expect("breakpoint command list {0}".format(id),
+patterns=["some command"])
+# Now do the same thing with a python function:
+import side_effect
+self.runCmd("command script import --allow-reload ./bktptcmd.py")
+
+self.runCmd("breakpoint command add --python-function bktptcmd.function MyBKPTS")
+for id in bp_ids:
+self.expect("breakpoint command list {0}".format(id),
+patterns=["bktptcmd.function"])
+
+
+
 def test_breakpoint_delete_disabled(self):
   

[Lldb-commits] [PATCH] D125943: [trace][intelpt] Support system-wide tracing [11] - Read warnings and perf conversion in the client

[Jakob Johnson via Phabricator via lldb-commits]

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

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125943

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

[Lldb-commits] [PATCH] D126015: [trace][intelpt] Support system-wide tracing [12] - Support multi-core trace load and save

[Jakob Johnson via Phabricator via lldb-commits]

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

feedback-v1




Comment at: lldb/include/lldb/Target/Trace.h:520
+  /// core id -> data kind -> size
+  llvm::DenseMap>
+  m_live_core_data;

Would this work instead? I noticed that the several other maps around this code 
also use unordered_map and string, maybe we could update those too at some 
point.



Comment at: lldb/include/lldb/Target/Trace.h:538
+  /// core id -> data kind -> file
+  llvm::DenseMap>
+  m_postmortem_core_data;

same as comment above related to using LLVM types rather than std types 
wherever possible.



Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:193
 
-Expected>
-IntelPTMultiCoreTrace::GetBinaryData(const TraceGetBinaryDataRequest &request) 
{
-  return createStringError(inconvertibleErrorCode(), "Unimplemented");
+Expected>>
+IntelPTMultiCoreTrace::TryGetBinaryData(

`Expected>` feels weird. Are both of these "layers" needed?
I noticed this in a couple places but I'm just leaving a single comment here.



Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:191
+   * with respect to their definition of head pointer. In the case
+   * of Aux data buffer the head always wraps around the aux buffer
+   * and we don't need to care about it, whereas the data_head keeps

Be careful with the "strong" language here. The AUX head only wraps around when 
the AUX buffer is mmaped with read-only, if it is mmapped with write perms, 
then the AUX head behaves like the data head (it doesn't auto wrap)

See this snipped of kernel code, the overwrite flag is set based on the mmap 
PROT and that overwrite flag is passed to the intelpt code.
https://github.com/torvalds/linux/blob/df0cc57e/kernel/events/ring_buffer.c#L670-L733


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126015

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