This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Implement CrashReason using UnixSignals
ClosedPublic

Authored by DavidSpickett on Mar 14 2023, 6:37 AM.

Details

Summary

By adding signal codes to UnixSignals and adding a new function
where you can get a string with optional address and bounds.

Added signal codes to the Linux, FreeBSD and NetBSD signal sets.
I've checked the numbers against the relevant sources.

Each signal code has a code number, description and printing options.
By default you just get the descripton, you can opt into adding either
a fault address or bounds information.

Bounds signals we'll use the description, unless we have the bounds
values in which case we say whether it is an upper or lower bound
issue.

GetCrashReasonString remains in CrashReason because we need it to
be compiled only for platforms with siginfo_t. Ideally it would
move into NativeProcessProtocol, but that is also used
by NativeRegisterContextWindows, where there would be no siginfo_t.

Diff Detail

Event Timeline

DavidSpickett created this revision.Mar 14 2023, 6:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 6:37 AM
DavidSpickett requested review of this revision.Mar 14 2023, 6:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 6:37 AM

@emaste , @mgorny maybe you want to glance at the numbers for the BSDs. I got the codes from the FreeBSD/NetBSD sources, and the signal numbers are as before, using the Darwin numbers.

lldb/source/Plugins/Process/Utility/FreeBSDSignals.cpp
38

This one is only in FreeBSD.

emaste added inline comments.Mar 14 2023, 6:54 AM
lldb/source/Plugins/Process/Utility/FreeBSDSignals.cpp
20

FreeBSD comment uses "illegal opcode" fwiw

29

looks like the first two are swapped?

/* codes for SIGFPE */
#define FPE_INTOVF      1       /* Integer overflow.                    */
#define FPE_INTDIV      2       /* Integer divide by zero.              */
...

Corect FreeBSD SIGFPE integer overflow/divide by zero order.

FreeBSD is overflow then divide by zero.
NetBSD is divide by zero then overflow.

DavidSpickett marked an inline comment as done.Mar 14 2023, 7:04 AM

Update the descriptions to match the platform's.

Add a couple of FreeBSD signals I missed the first time.

We're missing a bunch of Linux ones too but since that file
already existed, I don't want to make the diff more messy.
I'll add them later.

DavidSpickett marked an inline comment as done.Mar 14 2023, 7:35 AM
DavidSpickett added inline comments.
lldb/source/Plugins/Process/Utility/FreeBSDSignals.cpp
20

Turns out, so does Linux.

FreeBSD LGTM

It's not exactly what I had in mind, but I kinda like it :)

lldb/include/lldb/Target/UnixSignals.h
128

I think we should just make strings out of these, particularly since we now also have a map here. It's not like this is extremely performance-sensitive code.

lldb/source/Plugins/Process/POSIX/CrashReason.cpp
25

I think this function should really be called GetSignalDescription, or something similar

JDevlieghere accepted this revision.Mar 14 2023, 11:07 AM

LGTM with Pavel's comments.

lldb/unittests/Signals/UnixSignalsTest.cpp
132–134

Nit: If these all need to pass maybe make these ASSERT_EQ so you don't get an "expected" failure below.

This revision is now accepted and ready to land.Mar 14 2023, 11:07 AM
DavidSpickett marked an inline comment as done.Mar 15 2023, 2:24 AM
DavidSpickett added inline comments.
lldb/include/lldb/Target/UnixSignals.h
128

Make strings in what way exactly. I used an enum just so it was harder to make a typo and not realise.

Use ASSERT_EQ, change name of new function.

DavidSpickett marked 2 inline comments as done.Mar 15 2023, 2:54 AM

Minor suggestions, feel free to ignore.

lldb/source/Plugins/Process/Utility/FreeBSDSignals.cpp
14

I wonder if it would make sense to do something like the following to ensure we use the same numbers as defined in FreeBSD (compile-time checks when building on a FreeBSD host). The it would be possible to use
FREEBSD_ILL_ILLOPC instead of 1 /*ILL_ILLOPC*/ below

57

Not sure if it's worth adding here, but CheriBSD (a fork of FreeBSD with support for CHERI) uses 34 for "SIGPROT": https://github.com/CTSRD-CHERI/cheribsd/blob/8993e1c3bba52a2610f863d206b319904c22e121/sys/sys/signal.h#L134

DavidSpickett marked an inline comment as done.Mar 15 2023, 4:04 AM
DavidSpickett added inline comments.
lldb/source/Plugins/Process/Utility/FreeBSDSignals.cpp
14

This is a cool idea, I'll give this a go (maybe as a follow up change).

57

Given that we don't support CheriBSD I'll leave it out for now. I think there is a Cheri llvm branch somewhere though? Worth noting to whomever does the merge there that they need to carry it over if it's in CrashReason there already.

DavidSpickett marked 2 inline comments as done.Mar 16 2023, 5:47 AM
DavidSpickett added inline comments.
lldb/source/Plugins/Process/Utility/FreeBSDSignals.cpp
14
DavidSpickett added inline comments.Mar 20 2023, 4:32 AM
lldb/include/lldb/Target/UnixSignals.h
128

@labath I'm going to start landing changes. If you still want this changed I'll do that in a later commit.

This revision was automatically updated to reflect the committed changes.