[lldb] Show signal number description #164176
Conversation
when the user types `process handle` show that the signal is used for.
da-viper
requested review from
DavidSpickett
and removed request for
JDevlieghere
|
@llvm/pr-subscribers-lldb Author: Ebuka Ezike (da-viper) Changesshow information about the signal when the user presses (lldb) process handle SIGWINCH
NAME PASS STOP NOTIFY DESCRIPTION
=========== ===== ===== ====== ===================
SIGWINCH true false false window size changesWanted to use the existing llvm-project/lldb/source/Target/StopInfo.cpp Lines 1192 to 1195 in 65c895d Full diff: https://github.com/llvm/llvm-project/pull/164176.diff 4 Files Affected:
diff --git a/lldb/include/lldb/Target/UnixSignals.h b/lldb/include/lldb/Target/UnixSignals.h
index a1807d69f329b..590e4d1aa5208 100644
--- a/lldb/include/lldb/Target/UnixSignals.h
+++ b/lldb/include/lldb/Target/UnixSignals.h
@@ -31,6 +31,8 @@ class UnixSignals {
llvm::StringRef GetSignalAsStringRef(int32_t signo) const;
+ llvm::StringRef GetSignalNumberDescription(int32_t signo) const;
+
std::string
GetSignalDescription(int32_t signo,
std::optional<int32_t> code = std::nullopt,
diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp
index 7d326404a5503..1dc9d12580cbe 100644
--- a/lldb/source/Commands/CommandObjectProcess.cpp
+++ b/lldb/source/Commands/CommandObjectProcess.cpp
@@ -1603,8 +1603,8 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
Options *GetOptions() override { return &m_options; }
void PrintSignalHeader(Stream &str) {
- str.Printf("NAME PASS STOP NOTIFY\n");
- str.Printf("=========== ===== ===== ======\n");
+ str.Printf("NAME PASS STOP NOTIFY DESCRIPTION\n");
+ str.Printf("=========== ===== ===== ====== ===================\n");
}
void PrintSignal(Stream &str, int32_t signo, llvm::StringRef sig_name,
@@ -1616,8 +1616,15 @@ class CommandObjectProcessHandle : public CommandObjectParsed {
str.Format("{0, -11} ", sig_name);
if (signals_sp->GetSignalInfo(signo, suppress, stop, notify)) {
bool pass = !suppress;
- str.Printf("%s %s %s", (pass ? "true " : "false"),
+ str.Printf("%s %s %s ", (pass ? "true " : "false"),
(stop ? "true " : "false"), (notify ? "true " : "false"));
+
+ llvm::StringRef sig_description =
+ signals_sp->GetSignalNumberDescription(signo);
+ if (!sig_description.empty()) {
+ str.PutCString(" ");
+ str.PutCString(sig_description);
+ }
}
str.Printf("\n");
}
diff --git a/lldb/source/Target/UnixSignals.cpp b/lldb/source/Target/UnixSignals.cpp
index 6113c6648817c..881431f4631e5 100644
--- a/lldb/source/Target/UnixSignals.cpp
+++ b/lldb/source/Target/UnixSignals.cpp
@@ -137,6 +137,13 @@ llvm::StringRef UnixSignals::GetSignalAsStringRef(int32_t signo) const {
return pos->second.m_name;
}
+llvm::StringRef UnixSignals::GetSignalNumberDescription(int32_t signo) const {
+ const auto pos = m_signals.find(signo);
+ if (pos == m_signals.end())
+ return {};
+ return pos->second.m_description;
+}
+
std::string UnixSignals::GetSignalDescription(
int32_t signo, std::optional<int32_t> code,
std::optional<lldb::addr_t> addr, std::optional<lldb::addr_t> lower,
diff --git a/lldb/unittests/Signals/UnixSignalsTest.cpp b/lldb/unittests/Signals/UnixSignalsTest.cpp
index 582e441556067..3bd4aedd600a3 100644
--- a/lldb/unittests/Signals/UnixSignalsTest.cpp
+++ b/lldb/unittests/Signals/UnixSignalsTest.cpp
@@ -148,6 +148,18 @@ TEST(UnixSignalsTest, GetAsString) {
signals.GetSignalDescription(16, 3, 0x1233, 0x1234, 0x5678));
}
+TEST(UnixSignalsTest, GetNumberDescription) {
+ TestSignals signals;
+
+ ASSERT_EQ("DESC2", signals.GetSignalNumberDescription(2));
+ ASSERT_EQ("DESC4", signals.GetSignalNumberDescription(4));
+ ASSERT_EQ("DESC8", signals.GetSignalNumberDescription(8));
+ ASSERT_EQ("DESC16", signals.GetSignalNumberDescription(16));
+
+ // Unknown signal number.
+ ASSERT_EQ("", signals.GetSignalNumberDescription(100));
+}
+
TEST(UnixSignalsTest, VersionChange) {
TestSignals signals;
|
There was a problem hiding this comment.
This is a good idea, one less reason to leave the debugger.
Took me a while to figure out exactly why you couldn't use the existing method, but that's not because you didn't say so, it's because we have too much overlapping terminology in the existing code.
It's like GetSignalDescription is the __repr__ of the signal, and so should try as hard as it can to return something, sometimes just the name. You specifically want the description part of the information instead.
It's not for this PR but it feels like GetSignalDescription should be GetSignalAsString or FormatSignal or whatever, and GetSignalAsStringRef should be GetSignalName to match your new method.
Too many things say "signal" when they mean just the signal number but also other things say signal when they mean the whole signal struct.
If you want to have a go at improving that, please do.
Your new method does what it says though so it's ok. If you do change the other names, it could be "GetSignalDescription" as you'd have liked it to be.
| str.Printf("NAME PASS STOP NOTIFY\n"); | ||
| str.Printf("=========== ===== ===== ======\n"); | ||
| str.Printf("NAME PASS STOP NOTIFY DESCRIPTION\n"); | ||
| str.Printf("=========== ===== ===== ====== ===================\n"); |
There was a problem hiding this comment.
Also we don't have any tests for this part of the command. There is lldb/test/API/commands/process/handle/TestProcessHandle.py but it's only changing the settings.
Not all platforms will have all signals but we don't want to check them all anyway. The test uses SIGSEGV already. So maybe you can just add :
- one call prior to loading the test binary, that just gets the header
- one later that expects the header and some line for SIGSEGV
The description will change between platforms (though they all mean the same thing), so it would be ok to regex match that there is some string after the last bool.
We don't need to check for the exact string because we trust that the unittests prove that the right description will be used.
| if (signals_sp->GetSignalInfo(signo, suppress, stop, notify)) { | ||
| bool pass = !suppress; | ||
| str.Printf("%s %s %s", (pass ? "true " : "false"), | ||
| str.Printf("%s %s %s ", (pass ? "true " : "false"), |
There was a problem hiding this comment.
What's the logic with adding spaces here but also below?
After years of staring at Python test failures due to whitespace differences, I always try to add as few of them as possible, but that's just a personal preference most of the time.
Could you add no spaces here and then add 3 later if there is a description?
3 participants
show information about the signal when the user presses
process handle <unix-signal>i.eWanted to use the existing
GetSignalDescriptionbut it is expected behaviour to return the signal name if no signal code is passed. It is used in stop info.llvm-project/lldb/source/Target/StopInfo.cpp
Lines 1192 to 1195 in 65c895d