[Lldb-commits] [PATCH] D116372: [lldb-server/linux] Fix waitpid for multithreaded forks

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1909
+ wait_status);
+tid_events.try_emplace(thread_up->GetID(), wait_status);
+  }

mgorny wrote:
> Hmm, I'm a bit confused here. Why would the key already exist? I mean, you 
> start with empty `tid_events` and catch only one event for every thread, 
> correct?
It doesn't exist. It's just how the api is named. c++ std::map offers both 
emplace and try_emplace (which, ironically, don't differ in "trying" -- they 
only differ in the way they construct the pair). DenseMap offers only 
try_emplace (maybe for that reason).



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1918
+} else {
+  // This can happen if one of the events is an main thread exit.
+  LLDB_LOG(log, "... but the thread has disappeared");

mgorny wrote:
> Are we talking about some kind of race here? Or some thread that appears in 
> `m_threads` but is not returned by `GetThreadByID()`?
> 
> I was wondering if you could use thread pointers as keys.
The problem is when a thread disappears. This can happen in case of a main 
thread exit or an execve, in which case we remove all non-main threads from the 
list. However, we can still have some pending events for the other threads. 
Now, I haven't managed to reproduce this in my experiments, but the manpage is 
adamant that a SIGKILL should immediately terminate a process. In my (limited) 
tests the debugger always got a PTRACE_EVENT_EXIT stop for each threads (which 
is again something that the manpage says should not happen), so we 
theoretically (with careful management of thread lifetimes) might ensure that a 
thread with pending events does not disappear, but depending on it doesn't seem 
like a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116372

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

[Lldb-commits] [PATCH] D116255: [lldb] [Process/FreeBSDKernel] Support finding all processes

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Can we make the number of threads smaller? Perhaps by patching the linked lists 
to skip the uninteresting ones? 600 threads is way too much, all we really need 
is one thread from each of the three categories. Then you can meaningfully 
assert the names and registers of the existing threads (since their layout is 
the same, there's probably no point in asserting all registers, but it'd be 
good to check one or two, just to make sure the starting offset is right).


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

https://reviews.llvm.org/D116255

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

[Lldb-commits] [PATCH] D116255: [lldb] [Process/FreeBSDKernel] Support finding all processes

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

mgorny added a comment.

That would be a lot of work, and it would need to be repeated whenever the test 
data needs to be regenerated. How about leaving the threads as-is but just 
testing the few selected ones from the list?


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

https://reviews.llvm.org/D116255

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

[Lldb-commits] [PATCH] D116372: [lldb-server/linux] Fix waitpid for multithreaded forks

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

mgorny added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1918
+} else {
+  // This can happen if one of the events is an main thread exit.
+  LLDB_LOG(log, "... but the thread has disappeared");

labath wrote:
> mgorny wrote:
> > Are we talking about some kind of race here? Or some thread that appears in 
> > `m_threads` but is not returned by `GetThreadByID()`?
> > 
> > I was wondering if you could use thread pointers as keys.
> The problem is when a thread disappears. This can happen in case of a main 
> thread exit or an execve, in which case we remove all non-main threads from 
> the list. However, we can still have some pending events for the other 
> threads. Now, I haven't managed to reproduce this in my experiments, but the 
> manpage is adamant that a SIGKILL should immediately terminate a process. In 
> my (limited) tests the debugger always got a PTRACE_EVENT_EXIT stop for each 
> threads (which is again something that the manpage says should not happen), 
> so we theoretically (with careful management of thread lifetimes) might 
> ensure that a thread with pending events does not disappear, but depending on 
> it doesn't seem like a good idea.
Hmm, so it could disappear while `MonitorCallback()` is executing; do I 
understand correctly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116372

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

[Lldb-commits] [PATCH] D116255: [lldb] [Process/FreeBSDKernel] Support finding all processes

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

That would deal with the code coverage, but it still leaves us with a fairly 
large core file, and a lot of uninteresting tests obscuring the output.

How do you generate these core files? Do you actually create a fresh core dump 
or you just recompute the "interesting" portions from a master file you have 
around? If you make the changes the the master core file, then they would get 
automatically picked up during the recomputation.

Alternatively, maybe there is a way to capture a core file without so many 
processes. Either killing off everything before the core file is written, or by 
making sure the other processes are never started (something like 
`init=/bin/bash` on linux)?


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

https://reviews.llvm.org/D116255

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

[Lldb-commits] [PATCH] D116372: [lldb-server/linux] Fix waitpid for multithreaded forks

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1918
+} else {
+  // This can happen if one of the events is an main thread exit.
+  LLDB_LOG(log, "... but the thread has disappeared");

mgorny wrote:
> labath wrote:
> > mgorny wrote:
> > > Are we talking about some kind of race here? Or some thread that appears 
> > > in `m_threads` but is not returned by `GetThreadByID()`?
> > > 
> > > I was wondering if you could use thread pointers as keys.
> > The problem is when a thread disappears. This can happen in case of a main 
> > thread exit or an execve, in which case we remove all non-main threads from 
> > the list. However, we can still have some pending events for the other 
> > threads. Now, I haven't managed to reproduce this in my experiments, but 
> > the manpage is adamant that a SIGKILL should immediately terminate a 
> > process. In my (limited) tests the debugger always got a PTRACE_EVENT_EXIT 
> > stop for each threads (which is again something that the manpage says 
> > should not happen), so we theoretically (with careful management of thread 
> > lifetimes) might ensure that a thread with pending events does not 
> > disappear, but depending on it doesn't seem like a good idea.
> Hmm, so it could disappear while `MonitorCallback()` is executing; do I 
> understand correctly?
Yes. I meant disappearing from the list we maintain, not from the system.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116372

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

[Lldb-commits] [lldb] d7dbe2c - [lldb] Remove lldbtest.getBuildFlags

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2021-12-30T12:19:24+01:00
New Revision: d7dbe2c4a00ba2abd998328ad6b8023637bc71d9

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

LOG: [lldb] Remove lldbtest.getBuildFlags

It was being used only in some very old tests (which pass even without
it) and its implementation is highly questionable.

These days we have different mechanisms for requesting a build with a
particular kind of c++ library (USE_LIB(STD)CPP in the makefile).

Added: 


Modified: 
lldb/packages/Python/lldbsuite/test/lldbtest.py
lldb/test/API/android/platform/TestDefaultCacheLineSize.py

lldb/test/API/functionalities/dynamic_value_child_count/TestDynamicValueChildCount.py
lldb/test/API/functionalities/thread/backtrace_all/TestBacktraceAll.py
lldb/test/API/functionalities/thread/break_after_join/TestBreakAfterJoin.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentBreakpointDelayBreakpointOneSignal.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentBreakpointOneDelayBreakpointThreads.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentBreakpointsDelayedBreakpointOneWatchpoint.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentCrashWithBreak.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentCrashWithSignal.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentCrashWithWatchpoint.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentCrashWithWatchpointBreakpointSignal.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentDelaySignalBreak.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentDelaySignalWatch.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentDelayWatchBreak.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentDelayedCrashWithBreakpointSignal.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentDelayedCrashWithBreakpointWatchpoint.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentManyBreakpoints.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentManyCrash.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentManySignals.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentManyWatchpoints.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentNWatchNBreak.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalBreak.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalDelayBreak.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalDelayWatch.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalNWatchNBreak.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatch.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentSignalWatchBreak.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoBreakpointThreads.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoBreakpointsOneDelaySignal.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoBreakpointsOneSignal.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoBreakpointsOneWatchpoint.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointThreads.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneBreakpoint.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneDelayBreakpoint.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentTwoWatchpointsOneSignal.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentWatchBreak.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentWatchBreakDelay.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentWatchpointDelayWatchpointOneBreakpoint.py

lldb/test/API/functionalities/thread/concurrent_events/TestConcurrentWatchpointWithDelayWatchpointThreads.py

lldb/test/API/functionalities/thread/crash_during_step/TestCrashDuringStep.py

lldb/test/API/functionalities/thread/create_after_attach/TestCreateAfterAttach.py

lldb/test/API/functionalities/thread/create_during_step/TestCreateDuringStep.py

lldb/test/API/functionalities/thread/exit_during_break/TestExitDuringBreak.py
lldb/test/API/functionalities/thread/exit_during_step/TestExitDuringStep.py
lldb/test/API/functionalities/thread/jump/TestThreadJump.py
lldb/test/API/functionalities/thread/m

[Lldb-commits] [PATCH] D116255: [lldb] [Process/FreeBSDKernel] Support finding all processes

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

mgorny added a comment.

In D116255#3213612 , @labath wrote:

> That would deal with the code coverage, but it still leaves us with a fairly 
> large core file, and a lot of uninteresting tests obscuring the output.
>
> How do you generate these core files? Do you actually create a fresh core 
> dump or you just recompute the "interesting" portions from a master file you 
> have around? If you make the changes the the master core file, then they 
> would get automatically picked up during the recomputation.

I do recompute them from master copies (which I finally need to upload 
somewhere) but the recomputation is really dumb and unaware of the file format. 
I suppose I could just hack LLDB to stop after grabbing the first N threads 
but… the first non-dump on-CPU thread is no. 200. Finishing on that will 
probably make the core smaller but not sure how much smaller. I'll try in a 
minute.

> Alternatively, maybe there is a way to capture a core file without so many 
> processes. Either killing off everything before the core file is written, or 
> by making sure the other processes are never started (something like 
> `init=/bin/bash` on linux)?

The vast majority of threads are kernel threads. At a quick glance, only about 
20 threads look like userspace.


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

https://reviews.llvm.org/D116255

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

[Lldb-commits] [PATCH] D116255: [lldb] [Process/FreeBSDKernel] Support finding all processes

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

mgorny added a comment.

Ok, so limiting to 200-ish threads halves the data size:

  -rw-r--r-- 1 mgorny mgorny 76K 12-30 14:16 /tmp/vmcore.bz2


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

https://reviews.llvm.org/D116255

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

[Lldb-commits] [PATCH] D116255: [lldb] [Process/FreeBSDKernel] Support finding all processes

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D116255#3213665 , @mgorny wrote:

> In D116255#3213612 , @labath wrote:
>
>> That would deal with the code coverage, but it still leaves us with a fairly 
>> large core file, and a lot of uninteresting tests obscuring the output.
>>
>> How do you generate these core files? Do you actually create a fresh core 
>> dump or you just recompute the "interesting" portions from a master file you 
>> have around? If you make the changes the the master core file, then they 
>> would get automatically picked up during the recomputation.
>
> I do recompute them from master copies (which I finally need to upload 
> somewhere) but the recomputation is really dumb and unaware of the file 
> format.

I am aware how the recomputation works. My point is that if you hack the 
"master" copy (quotes because you probably still want to keep the original 
master around) then you only need to redo the hacks after when you're 
regenerating the master. At that point you'll also need to update all the test 
expectations, so it doesn't seem like too much extra work. (In fact, I wouldn't 
be surprised if we just end up adding a new core file instead of updating the 
old one).

> I suppose I could just hack LLDB to stop after grabbing the first N threads 
> but… the first non-dump on-CPU thread is no. 200. Finishing on that will 
> probably make the core smaller but not sure how much smaller. I'll try in a 
> minute.

You may still need to do some hex editing to avoid ending the list with a 
dangling pointer...

>> Alternatively, maybe there is a way to capture a core file without so many 
>> processes. Either killing off everything before the core file is written, or 
>> by making sure the other processes are never started (something like 
>> `init=/bin/bash` on linux)?
>
> The vast majority of threads are kernel threads. At a quick glance, only 
> about 20 threads look like userspace.

yikes. And I thought linux was bad..


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

https://reviews.llvm.org/D116255

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

[Lldb-commits] [lldb] 9b8f9d3 - [lldb/qemu] More flexible emulator specification

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2021-12-30T15:14:41+01:00
New Revision: 9b8f9d33dbbcd6525ab4d582cb9abb6f98e3601c

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

LOG: [lldb/qemu] More flexible emulator specification

This small patch adds two useful improvements:
- allows one to specify the emulator path as a bare filename, and have
  it be looked up in the PATH
- allows one to leave the path empty and have the filename be derived
  from the architecture.

Added: 


Modified: 
lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
lldb/source/Plugins/Platform/QemuUser/PlatformQemuUserProperties.td
lldb/test/API/qemu/TestQemuLaunch.py

Removed: 




diff  --git a/lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp 
b/lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
index 67c9484680a42..572a5b39985ec 100644
--- a/lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
+++ b/lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
@@ -162,7 +162,10 @@ lldb::ProcessSP 
PlatformQemuUser::DebugProcess(ProcessLaunchInfo &launch_info,
Target &target, Status &error) {
   Log *log = GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PLATFORM);
 
-  std::string qemu = GetGlobalProperties().GetEmulatorPath().GetPath();
+  FileSpec qemu = GetGlobalProperties().GetEmulatorPath();
+  if (!qemu)
+qemu.SetPath(("qemu-" + GetGlobalProperties().GetArchitecture()).str());
+  FileSystem::Instance().ResolveExecutableLocation(qemu);
 
   llvm::SmallString<0> socket_model, socket_path;
   HostInfo::GetProcessTempDir().GetPath(socket_model);
@@ -171,7 +174,7 @@ lldb::ProcessSP 
PlatformQemuUser::DebugProcess(ProcessLaunchInfo &launch_info,
 llvm::sys::fs::createUniquePath(socket_model, socket_path, false);
   } while (FileSystem::Instance().Exists(socket_path));
 
-  Args args({qemu, "-g", socket_path});
+  Args args({qemu.GetPath(), "-g", socket_path});
   args.AppendArguments(GetGlobalProperties().GetEmulatorArgs());
   args.AppendArgument("--");
   args.AppendArgument(launch_info.GetExecutableFile().GetPath());

diff  --git 
a/lldb/source/Plugins/Platform/QemuUser/PlatformQemuUserProperties.td 
b/lldb/source/Plugins/Platform/QemuUser/PlatformQemuUserProperties.td
index 4e8fbcfd67609..c7ec4bbc6e786 100644
--- a/lldb/source/Plugins/Platform/QemuUser/PlatformQemuUserProperties.td
+++ b/lldb/source/Plugins/Platform/QemuUser/PlatformQemuUserProperties.td
@@ -8,7 +8,7 @@ let Definition = "platformqemuuser" in {
   def EmulatorPath: Property<"emulator-path", "FileSpec">,
 Global,
 DefaultStringValue<"">,
-Desc<"Path to the emulator binary.">;
+Desc<"Path to the emulator binary. If the path does not contain a 
directory separator, the filename is looked up in the PATH environment 
variable. If empty, the filename is derived from the architecture setting.">;
   def EmulatorArgs: Property<"emulator-args", "Args">,
 Global,
 DefaultStringValue<"">,

diff  --git a/lldb/test/API/qemu/TestQemuLaunch.py 
b/lldb/test/API/qemu/TestQemuLaunch.py
index 2e817ede4154d..01c4143c9e77e 100644
--- a/lldb/test/API/qemu/TestQemuLaunch.py
+++ b/lldb/test/API/qemu/TestQemuLaunch.py
@@ -154,6 +154,24 @@ def test_stdio_redirect(self):
 state = json.load(s)
 self.assertEqual(state["stdin"], "STDIN CONTENT")
 
+def test_find_in_PATH(self):
+emulator = self.getBuildArtifact("qemu-" + self.getArchitecture())
+os.rename(self.getBuildArtifact("qemu.py"), emulator)
+self.set_emulator_setting("emulator-path", "''")
+
+original_path = os.environ["PATH"]
+os.environ["PATH"] = (self.getBuildDir() +
+self.platformContext.shlib_path_separator + original_path)
+def cleanup():
+os.environ["PATH"] = original_path
+
+self.addTearDownHook(cleanup)
+state = self._run_and_get_state()
+
+self.assertEqual(state["program"], self.getBuildArtifact())
+self.assertEqual(state["args"],
+["dump:" + self.getBuildArtifact("state.log")])
+
 def test_bad_emulator_path(self):
 self.set_emulator_setting("emulator-path",
 self.getBuildArtifact("nonexistent.file"))



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

[Lldb-commits] [PATCH] D116419: [lldb] Display MachO seg, sec of memory region

[Dave Lee via Phabricator via lldb-commits]

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

`memory region` displays the top-level section name, which for MachO is the 
segment name. The segment name alone is not much use, it's very course grained. 
This changes `memory region` to display the segment and section names for MachO 
images. For example: `__DATA,__const`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116419

Files:
  lldb/source/Commands/CommandObjectMemory.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
@@ -62,3 +62,21 @@
 interp.HandleCommand("memory region", result)
 self.assertFalse(result.Succeeded())
 self.assertRegexpMatches(result.GetError(), "Usage: memory region 
ADDR")
+
+@skipUnlessDarwin
+def test_macho(self):
+self.build()
+
+# Set breakpoint in main and run
+self.runCmd("file " + self.getBuildArtifact("a.out"), 
CURRENT_EXECUTABLE_SET)
+lldbutil.run_break_set_by_symbol(
+self, "main", sym_exact=True, num_expected_locations=1
+)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+interp = self.dbg.GetCommandInterpreter()
+result = lldb.SBCommandReturnObject()
+
+interp.HandleCommand("memory region $pc", result)
+self.assertRegexpMatches(result.GetOutput(), r"\b__TEXT,__text\b")
Index: lldb/source/Commands/CommandObjectMemory.cpp
===
--- lldb/source/Commands/CommandObjectMemory.cpp
+++ lldb/source/Commands/CommandObjectMemory.cpp
@@ -34,6 +34,7 @@
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/DataBufferLLVM.h"
 #include "lldb/Utility/StreamString.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/MathExtras.h"
 #include 
 #include 
@@ -1673,13 +1674,26 @@
   lldb_private::Address addr;
   ConstString name = range_info.GetName();
   ConstString section_name;
-  if (process_sp->GetTarget().ResolveLoadAddress(load_addr, addr)) {
+  auto &target = process_sp->GetTarget();
+  if (target.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();
+  if (target.GetArchitecture().GetTriple().isOSBinFormatMachO()) {
+// Display the conventional Mach-O format: __SEG,__SECT
+if (auto segment_sp = section_sp->GetParent()) {
+  auto segment_name = segment_sp->GetName();
+  auto section_name_ = section_sp->GetName();
+  section_name = ConstString(
+  llvm::formatv("{0},{1}", segment_name, section_name_).str());
+}
+  }
+
+  if (section_name.IsEmpty()) {
+// Got the top most section, not the deepest section
+while (section_sp->GetParent())
+  section_sp = section_sp->GetParent();
+section_name = section_sp->GetName();
+  }
 }
   }
 


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
@@ -62,3 +62,21 @@
 interp.HandleCommand("memory region", result)
 self.assertFalse(result.Succeeded())
 self.assertRegexpMatches(result.GetError(), "Usage: memory region ADDR")
+
+@skipUnlessDarwin
+def test_macho(self):
+self.build()
+
+# Set breakpoint in main and run
+self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+lldbutil.run_break_set_by_symbol(
+self, "main", sym_exact=True, num_expected_locations=1
+)
+
+self.runCmd("run", RUN_SUCCEEDED)
+
+interp = self.dbg.GetCommandInterpreter()
+result = lldb.SBCommandReturnObject()
+
+interp.HandleCommand("memory region $pc", result)
+self.assertRegexpMatches(result.GetOutput(), r"\b__TEXT,__text\b")
Index: lldb/source/Commands/CommandObjectMemory.cpp
===
--- lldb/source/Commands/CommandObjectMemory.cpp
+++ lldb/source/Commands/CommandObjectMemory.cpp
@@ -34,6 +34,7 @@
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/DataBuf

[Lldb-commits] [PATCH] D115662: [lldb][DWARF] Remove duplicate DIE type assignment

[Luís Ferreira via Phabricator via lldb-commits]

ljmf00 planned changes to this revision.
ljmf00 added a comment.

In D115662#3211312 , @labath wrote:

> I don't have any issues with this, if everything still works. I hoped there 
> would be more of a discussion on this (I was hoping I'd maybe learn something 
> from it). That obviously didn't happen, but I don't think it needs to hold 
> this back.

I understand your point and I'm fine with discussing this further since this is 
not a blocker for me :)

I did this as an improvement as I saw this as a duplicate and apparently more 
duplicates of this assignment (see here 
https://github.com/llvm/llvm-project/blob/ed6c757d5c5926a72183cdd15a5b1efc54111db0/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp#L1552
 ) while reading the code.

To give more context, I'm in the process of generalizing 
`UpdateSymbolContextScopeForType` to be able to use it on other non-Clang 
language plugins (I'm creating a D lldb plugin), see D115201 
. Both the `TypeList` and the DIEToType 
mapping are/were duplicated on `ParseTypeFromClangModule`. That said, 
`UpdateSymbolContextScopeForType` included both and it is useful to do that 
there due to generalization, although I also understand that 
`UpdateSymbolContextScopeForType` is targeted for different tasks.

I propose to either:

1. change the name of `UpdateSymbolContextScopeForType` to accommodate both 
insertions (reverting D115308  and removing 
both assignments on `ParseTypeFromClangModule` and elsewhere)
2. create a separate routine to perform  both insertions (removing it from 
`UpdateSymbolContextScopeForType` and elsewhere)
3. simply delete them from `UpdateSymbolContextScopeForType` and keep it on 
`ParseTypeFromClangModule` and other places.

This patch is actually doing none of that since I didn't find the other 
duplicate assignments at the time of writing it.

I don't have the full picture of this, but this seems to be code that is needed 
independent of the DWARFASTParser. Looking at older implementations of language 
plugins (Go, for example), they copy and pasted 
`UpdateSymbolContextScopeForType` in their implementation (see 
https://github.com/llvm/llvm-project/commit/77198bc79b54267f2ce981c3a6c9c0d6384cac01#diff-0cf0246cf7e5ca0f0d43abe83aff7040786afb07703394799a5122cb6b3a18e7L428
 ).

Note: I don't have active guidance on LLDB codebase, so I try to learn from the 
documentation and the actual codebase by myself. Maybe @zequanwu or @labath 
have more knowledge of the code and they can share their thoughts on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115662

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