This is an archive of the discontinued LLVM Phabricator instance.

Don't use CopyContext in RegisterContextWin
ClosedPublic

Authored by amccarth on Feb 11 2015, 3:26 PM.

Details

Summary

Bug 22410

Eliminate CopyContext and related functions to allow for debugging on Vista.

CopyContext is necessary to safely get the XState, but LLDB doesn't currently
use the XState. CopyContext is available as of Windows 7 SP1, so it can't be
used on Vista.

I left the structure of the code to continue to ensure that we're handling
the CONTEXT alignment and so that we can switch back to CopyContext in the
future.

I went to lldb/Host/windows.h to lower NTDDI_VERSION, which specifies the minimum
version of the SDK we use, and found it was already set to NTDDI_VISTA. This
was surprising since the CopyContext function was defined even though it's
inside a #if (NTDDI_VERSION >= NTDDI_WIN7SP1) block in <WinBase.h>. It turns out
that the SDK doesn't define NTDDI_WIN7SP1, so the statement boiled down to

While I was in lldb/Host/windows.h, I eliminated two redundant #undefs.

Diff Detail

Repository
rL LLVM

Event Timeline

amccarth updated this revision to Diff 19788.Feb 11 2015, 3:26 PM
amccarth retitled this revision from to Don't use CopyContext in RegisterContextWin.
amccarth updated this object.
amccarth edited the test plan for this revision. (Show Details)
amccarth added a reviewer: zturner.
amccarth added a subscriber: Unknown Object (MLST).
amccarth updated this object.Feb 11 2015, 3:31 PM
zturner added inline comments.Feb 11 2015, 3:36 PM
include/lldb/Host/windows/windows.h
22–23 ↗(On Diff #19788)

What's this part for?

source/Plugins/Process/Windows/x86/RegisterContextWindows_x86.cpp
335 ↗(On Diff #19788)

Is alignment to 16 necessary? I think that's only if you use CopyContext() and InitializeContext(). If you're not using any of the AVX stuff, I don't think it has special alignment requirements. We might be able to get rid of this whole function, and all the cruft associated with the heap buffer, and just store the CONTEXT directly in the class. Note that CONTEXT in WinNT.h is declared with DECLSPEC_ALIGN(8), so I think everything is already taken care of.

337–338 ↗(On Diff #19788)

checkout llvm/Support/MathExtras.h. There are functions to do this kind of math for you. (This only applies though if it's not already aligned correctly. If it is all this code goes away)

amccarth updated this revision to Diff 19859.Feb 12 2015, 2:59 PM

Addressed review comments.

amccarth added inline comments.Feb 12 2015, 4:20 PM
include/lldb/Host/windows/windows.h
22–23 ↗(On Diff #19788)

Removed duplicate lines (see lines 19-20). I was in here to roll NTDDI_VERSION back to Vista, but it was already Vista. The notes explain how this "worked".

source/Plugins/Process/Windows/x86/RegisterContextWindows_x86.cpp
335 ↗(On Diff #19788)

One definition of CONTEXT in winnt.h is tagged with __declspec(align(16)), so I assumed it was. Another is tagged 8, and one is untagged. The heap allocation seemed to be returning blocks aligned only to 4-byte boundaries (not even 8!?).

Do you know how to tell definitively which CONTEXT applies?

337–338 ↗(On Diff #19788)

Cool. I've switched to llvm::alignAddr.

But I'll remove this altogether if the alignment isn't needed.

amccarth updated this revision to Diff 19907.Feb 13 2015, 10:47 AM

More aggressive simplification of CONTEXT handling on Windows.

zturner accepted this revision.Feb 13 2015, 10:56 AM
zturner edited edge metadata.

Looks very nice. I will commit later this afternoon. No issues popped up while running the test suite I assume? Same number of failures before and after the patch?

I would try setting a breakpoint by line number, then doing a "source list" and a "frame info" (I think that's the command anyway to see the stack frame), and a "disasm" and make sure all the values look sane. Of course normally the test suite would catch all this, but since it still fails for many tests I usually like to do this sanity check first when messign with the register stuff (in addition to running the tests obviously)

This revision is now accepted and ready to land.Feb 13 2015, 10:56 AM

Don't commit yet.

ninja check-lldb was giving me good results last night, but now it never
finishes. Looks like I get about 15 python_d.exe processes and 3 a.out
processes that hang. All bug one Python process are consuming no CPU, so
it looks like those are deadlocked. I'm investigating.

Hmm. Could you try memsetting the CONTEXT to 0 before calling GetThreadContext()? We already do it in WriteAllRegisterValues before calling *SetThreadContext*, but we don't do it before calling GetThreadContext(). In the previous code, we were allocating a new DataBufferHeap, which I think will 0-initialize it.

amccarth updated this revision to Diff 19928.Feb 13 2015, 1:47 PM
amccarth edited edge metadata.

Restored initialization of the ContextFlags in the context.

Did that fix the deadlocks?

Yes. I'm getting the same number of successful tests as before. The
interactive tests worked as well, except for source list, which shows
nothing.

Did the source list work before?

This revision was automatically updated to reflect the committed changes.