This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Move UnixSignals creation into Platform plugins
ClosedPublic

Authored by bulbazord on Mar 16 2023, 2:55 PM.

Details

Summary

The high level goal of this change is to remove lldbTarget's dependency
on lldbPluginProcessUtility. The reason for this existing dependency is
so that we can create the appropriate UnixSignals object based on an
ArchSpec. Instead of using the ArchSpec, we can instead take advantage
of the Platform associated with the current Target.

This is accomplished by adding a new method to Platform,
CreateUnixSignals, which will create the correct UnixSignals object for
us. We then can use Platform::GetUnixSignals and rely on that to give
us the correct signals as needed.

Diff Detail

Event Timeline

bulbazord created this revision.Mar 16 2023, 2:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 2:55 PM
Herald added a subscriber: emaste. · View Herald Transcript
bulbazord requested review of this revision.Mar 16 2023, 2:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 2:55 PM
JDevlieghere accepted this revision.Mar 20 2023, 12:44 PM

We're only using the OS part of the triple/ArchSpec so there should be a 1:1 mapping with the platform. I don't see any issue with this. LGTM.

This revision is now accepted and ready to land.Mar 20 2023, 12:44 PM
This revision was automatically updated to reflect the committed changes.

This seems to cause a regression on Linux where we no longer get the signal details.

Given an intentionally-crashy test binary, such as:

void crash() {
  // Allocate valid but non-accessible memory and attempt to write to it,
  // triggering a Segmentation Fault.
  int *ptr = static_cast<int *>(
      mmap(nullptr, sizeof(int), PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0));
  *ptr = 0x12345678;
  munmap(ptr, sizeof(int));
}

we used to see this:

$ bin/lldb crashy
(lldb) target create "crashy"
Current executable set to 'crashy' (x86_64).
(lldb) r
Process 2317185 launched: 'crashy' (x86_64)
Process 2317185 stopped
* thread #1, name = 'crashy', stop reason = signal SIGSEGV: invalid permissions for mapped object (fault address: 0x7ffff7fc5000)

but now the stop reason is just: stop reason = signal SIGSEGV (no details about the fault address)

Unrelated to the bug above: this also creates a circular dependency between gdb-server <-> gdb-remote packages (https://llvm.org/docs/CodingStandards.html#library-layering)

This seems to cause a regression on Linux where we no longer get the signal details.

Given an intentionally-crashy test binary, such as:

void crash() {
  // Allocate valid but non-accessible memory and attempt to write to it,
  // triggering a Segmentation Fault.
  int *ptr = static_cast<int *>(
      mmap(nullptr, sizeof(int), PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0));
  *ptr = 0x12345678;
  munmap(ptr, sizeof(int));
}

we used to see this:

$ bin/lldb crashy
(lldb) target create "crashy"
Current executable set to 'crashy' (x86_64).
(lldb) r
Process 2317185 launched: 'crashy' (x86_64)
Process 2317185 stopped
* thread #1, name = 'crashy', stop reason = signal SIGSEGV: invalid permissions for mapped object (fault address: 0x7ffff7fc5000)

but now the stop reason is just: stop reason = signal SIGSEGV (no details about the fault address)

Yep, definitely a regression... I suspect we're not creating the correct UnixSignals object anymore in this case. It would be nice if we had more testing here... I'll try to fix this, thanks for pointing it out.

Unrelated to the bug above: this also creates a circular dependency between gdb-server <-> gdb-remote packages (https://llvm.org/docs/CodingStandards.html#library-layering)

Ah, yeah that does happen too, doesn't it? Maybe the Signals should live in the Process plugins instead of the Platform plugins? I'm not married to this specific implementation.

rupprecht added inline comments.Mar 22 2023, 2:18 PM
lldb/source/Target/UnixSignals.cpp
39

I think the divergence might be happening here -- the host platform is null here, so we get UnixSignals instead of the subcode from LinuxSignals.

bulbazord added inline comments.Mar 22 2023, 2:20 PM
lldb/source/Target/UnixSignals.cpp
39

Why might the HostPlatform be nullptr? I assume you're debugging a binary built for some Linux distro on some Linux machine.

rupprecht added inline comments.Mar 22 2023, 2:41 PM
lldb/source/Target/UnixSignals.cpp
39

Yes, this is a local debugging session of a linux binary running on a linux machine. But it seems from my prodding around that lldb calls Platform::SetHostPlatform() (via PlatformLinux::Initialize(), but lldb-server isn't, and lldb-server is the thing that supposed to be returning the subcode details.

bulbazord added inline comments.Mar 22 2023, 3:44 PM
lldb/source/Target/UnixSignals.cpp
39
* frame #0: 0x0000aaaaab72dfe0 lldb-server`lldb_private::Platform::GetHostPlatform() at Platform.cpp:137:49
   frame #1: 0x0000aaaaab83f6e8 lldb-server`lldb_private::UnixSignals::CreateForHost() at UnixSignals.cpp:36:27
   frame #2: 0x0000aaaaab569590 lldb-server`GetCrashReasonString[abi:cxx11](info=0x0000ffffffffd9e0) at CrashReason.cpp:25:7
   frame #3: 0x0000aaaaab48e084 lldb-server`lldb_private::process_linux::NativeThreadLinux::SetStoppedBySignal(this=0x0000aaaab5255180, signo=11, info=0x0000ffffffffd9e0) at NativeThreadLinux.cpp:297:28

Yep, that makes sense to me. I'll work on this, thanks for letting me know about this.

bulbazord added inline comments.Mar 22 2023, 4:22 PM
lldb/source/Target/UnixSignals.cpp
39