Translate between abridged and full ftag values in order to expose
the latter in the gdb-remote protocol while the former are used by
FXSAVE/XSAVE... This matches the gdb behavior.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Looks reasonable to me.
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp | ||
---|---|---|
535 | You mean, like, how to detect that we're dealing with the ftag register? This approach seems reasonable to me.. | |
lldb/source/Plugins/Process/Utility/RegisterContext_x86.h | ||
386 | This is probably complex enough to deserve creating a cpp file (and a header comment explaining the high-level purpose of the function). | |
386 | Also, probably ArrayRef<MMSReg> instead of the raw pointer. | |
lldb/unittests/Process/Utility/RegisterContextTest.cpp | ||
32 | Add const, for good measure (I see that will result in additional const qualifications in a bunch of places. |
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp | ||
---|---|---|
535 | This = checking register number like in the code (in which I suppose we'd have to move distinguishing amd64/i386 to ctor like first_fpr? Or checking for actual pointer as I suggested in the comment? |
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp | ||
---|---|---|
535 | I originally meant the register number approach, but I see what you mean now. I think that going off of the address is perfectly fine... |
Updated Linux to use pointer comparison and FreeBSD. It's ready for review now, though I'd use a better suggestion for FreeBSD.
Looks good. Some minor questions inline.
lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.cpp | ||
---|---|---|
521 | if you make data a void*, then you can remove the reinterpret_cast two lines below. | |
526 | reuse data from above | |
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp | ||
537 | Is this really needed? I would have hoped that just passing m_xstate->fxsave.stmm would be enough and that the c array constructor of ArrayRef would do the right thing... | |
lldb/unittests/Process/Utility/RegisterContextTest.cpp | ||
56 | add SCOPED_TRACE macro to make it clear which test case failed, when it fails. You can use llvm::enumerate to produce sequence numbers for the test cases... |
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp | ||
---|---|---|
537 | Indeed. I have missed that ArrayRef can figure the size of a C-style array out. |
Simplified FreeBSD & Linux code as @labath suggested. Also added the scope thingy to tests.
Declare MMSRegComp type explicitly to avoid warnings:
In file included from /home/mgorny/llvm-project/llvm/tools/lldb/source/Plugins/Process/Utility/RegisterContextLinux_x86_64.cpp:11: In file included from /home/mgorny/llvm-project/llvm/tools/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_x86.h:12: /home/mgorny/llvm-project/llvm/tools/lldb/source/Plugins/Process/Utility/RegisterContext_x86.h:247:5: warning: anonymous types declared in an anonymous union are an extension [-Wnested-anon-types] struct { ^ 1 warning generated.
Hi @mgorny! http://lab.llvm.org:8011/#/builders/68/builds/2298 build failed. Could you please fix the test?
Does this change to TestRegister.py look good? It makes the test pass for me (and seems logically correct).
Well... I think that will make the test fail on mac, as debugserver hasn't been updated.
This test comes from D12592, and is actually serves a very similar purpose to the test you just wrote. Normally, I would say we don't need both of them, but since we still have debugserver with the old behavior, and your new test is consistent with how we've (you've) been writing other register tests, maybe we could just slap @skipUnlessDarwin on TestRegisters.py, and put some comments (on both tests) that explain the situation and cross-reference each other (?)
I could do that. I suppose it'd also be reasonably easy to make the Python test support both platforms with an if.
Skipped python test on non-Darwin, xfailed shell test on Darwin, added respective comments.
if you make data a void*, then you can remove the reinterpret_cast two lines below.