Page MenuHomePhabricator

Fix a problem where lldb-mi would not stop the debuggee after -exec-interrupt command.
ClosedPublic

Authored by abidh on Feb 20 2015, 3:26 AM.

Details

Summary

This revision fixes a problem where lldb-mi would not stop the execution after exec-interrupt call.
On Linux, SIGSTOP is used to stop the debuggee process. LLDB stopped the debuggee alright. But when
lldb-mi received the notification of stopping with reason as SIGSTOP, it would resume the process.
This was heppening in CMICmnLLDBDebuggerHandleEvents::HandleProcessEventStopSignal. This function aslo
used hard coded numbers for signal istead of symbolic names.

This revision changes code to treat SIGSTOP reason as SIGINT. Also used symbolic names for signals
instead of numbers.

Diff Detail

Event Timeline

abidh updated this revision to Diff 20384.Feb 20 2015, 3:26 AM
abidh retitled this revision from to Fix a problem where lldb-mi would not stop the debuggee after -exec-interrupt command..
abidh updated this object.
abidh edited the test plan for this revision. (Show Details)
abidh added reviewers: ki.stfu, clayborg.
abidh added a subscriber: Unknown Object (MLST).
ki.stfu edited edge metadata.Feb 20 2015, 3:42 AM

Can you provide test for it?

tools/lldb-mi/MICmnLLDBDebuggerHandleEvents.cpp
884

What is it? SIGSTOP?

Can you provide test for it?

I'll add this test in D7762.

abidh added a comment.Feb 20 2015, 7:57 AM

Can you provide test for it?

I'll add this test in D7762.

Thanks. I will wait for the test.

tools/lldb-mi/MICmnLLDBDebuggerHandleEvents.cpp
884

Yes it is SIGSTOP.

Does this work on windows? I see reference to some signal constants?

Does this work on windows? I see reference to some signal constants?

It can be done like this (right?):

#ifdef _WIN32
#define SIGINT 2
#define SIGSTOP 19
...
#endif // _WIN32
abidh added a comment.Feb 20 2015, 8:43 AM

Does this work on windows? I see reference to some signal constants?

I dont think this is a problem for Windows as I think SIGSTOP is not used to stop the process there. OTOH, I see that SIGSTOP is defined to a 2 different values within lldb.
#define SIGSTOP 20 win32.h
#define SIGSTOP 23
Platform.h

So probably this needs to be corrected.

As long as it compiles it's probably ok for now. But yes, fixing the signal
values would be nice.

In D7783#127438, @abidh wrote:

Can you provide test for it?

I'll add this test in D7762.

Thanks. I will wait for the test.

On OS X 2 tests failed: test_lldbmi_stopped_when_stopatentry_local / test_lldbmi_stopped_when_stopatentry_remote. I did some fix for it:

  • Add thread-id and stopped-threads in SIGINT message:
@@ -852,11 +853,18 @@
             bOk = bOk && MiHelpGetCurrentThreadFrame(miValueTuple);
             const CMICmnMIValueResult miValueResult5("frame", miValueTuple);
             bOk = bOk && miOutOfBandRecord.Add(miValueResult5);
+            const CMIUtilString strThreadId(CMIUtilString::Format("%d", sbProcess.GetSelectedThread().GetIndexID()));
+            const CMICmnMIValueConst miValueConst6(strThreadId);
+            const CMICmnMIValueResult miValueResult6("thread-id", miValueConst6);
+            bOk = bOk && miOutOfBandRecord.Add(miValueResult6);
+            const CMICmnMIValueConst miValueConst7("all");
+            const CMICmnMIValueResult miValueResult7("stopped-threads", miValueConst7);
+            bOk = bOk && miOutOfBandRecord.Add(miValueResult7);
             bOk = bOk && MiOutOfBandRecordToStdout(miOutOfBandRecord);
             bOk = bOk && TextToStdout("(gdb)");
         }
         break;
  • Fix signal tests:
Index: test/tools/lldb-mi/signal/TestMiSignal.py
===================================================================
--- test/tools/lldb-mi/signal/TestMiSignal.py   (revision 230022)
+++ test/tools/lldb-mi/signal/TestMiSignal.py   (working copy)
@@ -64,10 +64,7 @@
         self.expect("\^done")

         # Test that *stopped is printed
-        # Note that message is different in Darwin and Linux:
-        # Darwin: "*stopped,reason=\"signal-received\",signal=\"17\",thread-id=\"1\",stopped-threads=\"all\""
-        # Linux:  "*stopped,reason=\"end-stepping-range\",frame={addr=\"0x[0-9a-f]+\",func=\"??\",args=\[\],file=\"??\",fullname=\"??\",line=\"-1\"},thread-id=\"1\",stopped-threads=\"all\"
-        self.expect("\*stopped,reason=\"(signal-received|end-stepping-range)\",.+,thread-id=\"1\",stopped-threads=\"all\"")
+        self.expect("\*stopped,reason=\"signal-received\",signal-name=\"SIGINT\",signal-meaning=\"Interrupt\",.*thread-id=\"1\",stopped-threads=\"all\"")

         # Run to main to make sure we have not exited the application
         self.runCmd("-break-insert -f main")
@@ -112,7 +109,7 @@
             self.expect("\^done")

             # Test that *stopped is printed
-            self.expect("\*stopped,reason=\"signal-received\",signal=\"17\",thread-id=\"1\",stopped-threads=\"all\"")
+            self.expect("\*stopped,reason=\"signal-received\",signal-name=\"SIGINT\",signal-meaning=\"Interrupt\",.*thread-id=\"1\",stopped-threads=\"all\"")

             # Exit
             self.runCmd("-gdb-exit")
clayborg requested changes to this revision.Feb 20 2015, 10:06 AM
clayborg edited edge metadata.

It is ok to rely on signals here because CMICmnLLDBDebuggerHandleEvents::HandleProcessEventStateStopped() already determined that a signal was reported as the stop reason and windows just can avoid saying it stopped due to a signal, so no work needs to be done to work around this.

You do need to ask the process about its signals and you can't use hard coded values, you need to look them up by name through the SBUnixSignals object that you can get from a live SBProcess instance. See inlined comments above.

tools/lldb-mi/MICmnLLDBDebuggerHandleEvents.cpp
845–846

You need to ask the process about its unix signal and you can't use hard coded values. You can make some member variables in CMICmnLLDBDebuggerHandleEvents and then initialize them when you have a process. The process must be up and running, you can't ask the process before it is stopped at its first stop. So when you get a eStateStopped event, see if it is the first one and then initialize your ivars with something like:

void
CMICmnLLDBDebuggerHandleEvents::InitializeSignals()
{
    SBUnixSignals unix_signals = sbProcess.GetUnixSignals();
    m_SIGINT = unix_signals.GetSignalNumberFromName("SIGINT");
    m_SIGSTOP = unix_signals.GetSignalNumberFromName("SIGSTOP");
    m_SIGSEGV = unix_signals.GetSignalNumberFromName("SIGSEGV");
    m_SIGTRAP = unix_signals.GetSignalNumberFromName("SIGTRAP");
}
This revision now requires changes to proceed.Feb 20 2015, 10:06 AM

That's fine, I was mostly just concerned that it wouldn't compile. Windows
has only a very small subset of SIG_XXX constants defined in its system
headers. lldb and lldb-mi define a few more for Windows that it needs, I
just didn't have code in front of me so I wanted to make sure that the
#defines being used here are defined somewhere on Windows so that it would
compile.

abidh updated this revision to Diff 20506.Feb 23 2015, 4:32 AM
abidh edited edge metadata.

Obtained values of the signal from Process instead of using hard-coded values as suggested in review.
Patches looks bigger due to indentation change after replacing switch with if.

All tests pass on Linux and it build ok on Windows.

ki.stfu accepted this revision.Feb 23 2015, 4:57 AM
ki.stfu edited edge metadata.

Looks good. I'll check it on OS X tomorrow.

Don't wait me. I'll fix tests if something is wrong.

clayborg accepted this revision.Feb 23 2015, 9:51 AM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Feb 23 2015, 9:51 AM
abidh closed this revision.Feb 23 2015, 10:29 AM