This is an archive of the discontinued LLVM Phabricator instance.

[LLDB] Don't use signo as key in Signals DenseMap, as signo are not unique
ClosedPublic

Authored by mohit.bhakkad on Oct 12 2015, 3:25 AM.

Details

Summary

Looking at the <signo,description> keymap generated by Reset() in LinuxSignals.cpp, we can see that some signo maps to
more than one signals.

So for example, if one tries signal handle command on SIGCLD
(lldb) process handle SIGCLD -s 0 -p 1 -n 0
it says
error: Invalid signal name 'SIGCLD'

As a solution for this,I have tried:

  1. std::next() for iterating keymap, but It looks like, entries with duplicate signo are not getting added in map, so we get one signo only ones.
  2. Just like "short_name" field, add another field "alias", and add all signals with same signo in same entry, but looks like for some of such signals default (suppress, stop, notify) values are different, for example signo 16 in LinuxSignals.
  3. So I suggest to move signo in description part of keymap, and adding serial_no for each signal as unique key.

Let me know if this approach is correct, and I will provide patches for other functions related to signals.

Diff Detail

Repository
rL LLVM

Event Timeline

mohit.bhakkad retitled this revision from to [LLDB] Don't use signo as key in Signals DenseMap, as signo are not unique.
mohit.bhakkad updated this object.
mohit.bhakkad added a reviewer: clayborg.
mohit.bhakkad set the repository for this revision to rL LLVM.
mohit.bhakkad added subscribers: jaydeep, bhushan, slthakur and 2 others.
labath added a subscriber: labath.Oct 12 2015, 6:22 AM

The fact that LinuxSignals specifies different numbers and dispositions for SIGCHLD and SIGCLD is certainly a bug. If you fix that, then your alternative (2) becomes feasible. I think (2) is a better solution than the current proposed one.

clayborg requested changes to this revision.Oct 12 2015, 10:24 AM
clayborg edited edge metadata.

I would prefer to add an alias name instead of a serial number. If a signal has more than one possible name, I would rather see that documented up front. It also makes for clearer debugging when you get a specific signal. If a signal gets hits, we should show both names if the stop reply.

This revision now requires changes to proceed.Oct 12 2015, 10:24 AM

So I would prefer to have one entry per signal and have that structure describe everything for that one signal.

mohit.bhakkad edited edge metadata.

Changes in this revision:

  • Added alias as new argument for AddSignal.
  • removed shortname as hardcoded argument, as for alias we would need another shortname. Now generating shortname in run time.

This looks much better, I just have a couple of small remarks.

include/lldb/Target/UnixSignals.h
116 ↗(On Diff #37469)

nullptr

source/Plugins/Process/Utility/LinuxSignals.cpp
33 ↗(On Diff #37469)

It would be great to align these, so they don't get lost in the noise.

source/Plugins/Process/Utility/MipsLinuxSignals.cpp
33 ↗(On Diff #37469)

In LinuxSignals SIGABRT was the main name, while SIGIOT was an alias. Is this intentional? (as in, this signal is usually referred to as SIGIOT on mips platforms). If not, could you make it consistent?

clayborg requested changes to this revision.Oct 15 2015, 9:44 AM
clayborg edited edge metadata.

See inlined comments.

include/lldb/Target/UnixSignals.h
101–102 ↗(On Diff #37469)

Why are we doing this by name? shouldn't we do this with the signal number? And shouldn't this be named GetSignalAlias()?

source/Target/UnixSignals.cpp
177 ↗(On Diff #37469)

Should the name be GetAlias? Should the argument be a signal number?

196 ↗(On Diff #37469)

Call GetAlias and use signal number here?

This revision now requires changes to proceed.Oct 15 2015, 9:44 AM
mohit.bhakkad added inline comments.Oct 18 2015, 9:25 PM
include/lldb/Target/UnixSignals.h
101–102 ↗(On Diff #37469)

Hi @clayborg,

sorry, I didn't write proper description while submitting this patch.

Here alias and short_name are two different things. As eg. for signal SIGCHLD, SIGCLD is an alias and CHLD is the short_name.
For providing alias, I have added a default argument called "alias" in function AddSignal, which will be stored in variable m_alias.

Now for short_name, we have another variable short_name. But for signals with alias, like SIGCHLD, we will have 2 short_names, i.e. CHLD and CLD. But if we observe, we can see that short_name is just signal_name with first 3 letters removed, which can solve our problem. So instead of hardcoding short_names in <key,value> map, we can use this function to get it at run time.

clayborg accepted this revision.Oct 19 2015, 10:09 AM
clayborg edited edge metadata.

Ok, makes sense then.

This revision is now accepted and ready to land.Oct 19 2015, 10:09 AM
This revision was automatically updated to reflect the committed changes.

This looks much better, I just have a couple of small remarks.

@labath addressed these while committing.