Page MenuHomePhabricator

[NativeProcessLinux] Fix a bug in instruction-stepping over thread creation
ClosedPublic

Authored by labath on Aug 18 2015, 7:56 AM.

Details

Summary

There was a bug in NativeProcessLinux, where doing an instruction-level single-step over the
thread-creation syscall resulted in loss of control over the inferior. This happened because
after the inferior entered the thread-creation maintenance stop, we unconditionally performed a
PTRACE_CONT, even though the original intention was to do a PTRACE_SINGLESTEP. This is fixed by
storing the original state of the thread before the stop (stepping or running) and then
performing the appropriate action when resuming.

I also get rid of the callback in the ThreadContext structure, which stored the lambda used to
resume the thread, but which was not used consistently.

A test verifying the correctness of the new behavior is included.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 32415.Aug 18 2015, 7:56 AM
labath retitled this revision from to [NativeProcessLinux] Fix a bug in instruction-stepping over thread creation.
labath updated this object.
labath added reviewers: ovyalov, tberghammer.
labath added a subscriber: lldb-commits.
ovyalov edited edge metadata.Aug 18 2015, 4:28 PM

Please see my comments.

source/Plugins/Process/Linux/NativeProcessLinux.cpp
1457 ↗(On Diff #32415)

Since AddThread is private method of NPL we can make it to return NativeThreadLinuxSP.

2822 ↗(On Diff #32415)

It seems the comment is no longer actual.

3056 ↗(On Diff #32415)

Should it be eStateStepping?

3060 ↗(On Diff #32415)

Could you print state value here?

source/Plugins/Process/Linux/NativeProcessLinux.h
314 ↗(On Diff #32415)

Please update the comment.

source/Plugins/Process/Linux/NativeThreadLinux.h
115 ↗(On Diff #32415)

Nit - include <memory> for shared_ptr?

labath updated this revision to Diff 32526.Aug 19 2015, 2:57 AM
labath marked 4 inline comments as done.
labath edited edge metadata.
  • Address review comments
source/Plugins/Process/Linux/NativeProcessLinux.cpp
1457 ↗(On Diff #32415)

I've been wanting to do this as well, but I'd prefer to do this as a separate commit, if possible.

3056 ↗(On Diff #32415)

That's a good question. Previously, the state of the whole process was indeed set to eStateStepping, and I have (accidentally) failed to preserve that while refactoring.

However, now that I think about it, this had the problem that the state of the process was nondeterministic and depended on the order in which you resume threads (If you are continuing one thread and stepping another, the process state would depend on which thread you resumed last). The test suite passes, so i think we can assume no one is depending on this stepping state now, but to avoid introducing nondeterministic bugs in the future, I think we should just abolish the stepping state and say that the process is running if any of its threads are stepping or running.

What do you think?

labath updated this revision to Diff 32545.Aug 19 2015, 6:42 AM

Fix test on android arm - skip single-stepping over some functions

tberghammer added inline comments.Aug 19 2015, 8:35 AM
source/Plugins/Process/Linux/NativeProcessLinux.cpp
1049–1051 ↗(On Diff #32545)

(nit): You don't need this cast (the value is never used).

1148 ↗(On Diff #32545)

You call SetStoppedWithNoReason before almost all ResumeThread but as far as I see it isn't used in ResumeThread (except checked in an assert) and ResumeThread overwrites it. I would prefer to remove these as for me they complicate the code without any reason, but I might miss their purpose.

test/functionalities/thread/create_during_instruction_step/TestCreateDuringInstructionStep.py
60 ↗(On Diff #32545)

Please make this list architecture specific or add a comment for each function with the architecture where it imposes an issue

labath updated this revision to Diff 32571.Aug 19 2015, 10:22 AM
labath marked an inline comment as done.

Address review comments

ovyalov accepted this revision.Aug 19 2015, 7:42 PM
ovyalov edited edge metadata.

LGTM

source/Plugins/Process/Linux/NativeProcessLinux.cpp
3051 ↗(On Diff #32571)

Let's go ahead with eStateRunning instead of eStateStepping - I believe it sounds reasonable.

This revision is now accepted and ready to land.Aug 19 2015, 7:42 PM
tberghammer accepted this revision.Aug 20 2015, 2:06 AM
tberghammer edited edge metadata.
This revision was automatically updated to reflect the committed changes.