This is an archive of the discontinued LLVM Phabricator instance.

[lldb] [Process/FreeBSDRemote] Modernize and simplify YMM logic
ClosedPublic

Authored by mgorny on Nov 11 2020, 12:31 PM.

Details

Summary

Eliminate the remaining swith-case code for register getters,
and migrate YMM registers to regset-oriented model. Since these
registers are recombined from XMM and YMM_Hi128 XSAVE blocks, while LLDB
gdb-server protocol transmits YMM registers whole, the offset-based
model will not work here. Nevertheless, some improvement was possible.

Replace generic 'XSaveRegSet' along with sub-sets for XSAVE components
with 'YMMRegSet' (and more regsets in the future as further components
are implemented). Create a helper GetYMMSplitReg() method that obtains
pointers to the appropriate XMM and YMM_Hi128 blocks to reduce code
duplication.

Diff Detail

Event Timeline

mgorny requested review of this revision.Nov 11 2020, 12:31 PM
mgorny created this revision.
labath accepted this revision.Nov 12 2020, 1:35 AM
labath added inline comments.
lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.cpp
633

When does this return false? When the ymm regs are not available?

lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.h
83

void *&

This revision is now accepted and ready to land.Nov 12 2020, 1:35 AM
mgorny added inline comments.Nov 12 2020, 1:41 AM
lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.cpp
633

Yes. Note that this change brings us closer to being able to disable whole regsets — I just need to figure out how to do it ;-).

lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.h
83

Are you sure about this? I feel like the whole & deal is quite confusing (read: shouldn't have happened). You don't know whether the method modifies the variables passed to it unless you look at the method prototype.

labath added inline comments.Nov 12 2020, 3:03 AM
lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.h
83

well.. there certainly are code styles which prohibit non-const reference arguments. However, llvm is not one of them. Also lldb has a issue with compulsive null checks, so I think it's important to communicate the fact the two arguments cannot be null, which reference arguments do.

Another possibility would be to just make this a real return value (pair<void *, void*> ?). The downside there is that it's not as self-documenting. Pick your poison...

mgorny updated this revision to Diff 304865.Nov 12 2020, 9:06 AM
mgorny marked 3 inline comments as done.

Changed prototype to use pointer referenec.

mgorny updated this revision to Diff 304990.Nov 12 2020, 4:07 PM

Use a struct-based return type.

labath accepted this revision.Nov 13 2020, 2:11 AM
labath added inline comments.
lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.cpp
633

Ok, got it.

lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.h
35–38

Declare this inside the class (next to the function returning it), since it's really a private thing.

83

Ok, that feels a bit over-engineered, but works too...

mgorny updated this revision to Diff 305090.Nov 13 2020, 3:51 AM

Split LLDB-visible regsets into separate AVX and MPX.

mgorny updated this revision to Diff 305113.Nov 13 2020, 5:56 AM

Perform offsetting in GetOffsetRegSetData() to avoid UB.

Perform offsetting in GetOffsetRegSetData() to avoid UB.

Shouldn't have the last update gone to D91411 instead ?

mgorny updated this revision to Diff 305125.Nov 13 2020, 6:49 AM

Upload the correct diff.

Perform offsetting in GetOffsetRegSetData() to avoid UB.

Shouldn't have the last update gone to D91411 instead ?

Indeed.

mgorny updated this revision to Diff 305130.Nov 13 2020, 7:04 AM

Restore old register set indices, to unbreak output.

(Note: I'm going to make that struct move soonish)

mgorny marked 3 inline comments as done.Nov 13 2020, 7:05 AM
mgorny updated this revision to Diff 305132.Nov 13 2020, 7:07 AM

Move YMMSplitPtr inside the class.

mgorny updated this revision to Diff 305158.Nov 13 2020, 8:18 AM

Rename AVX/MPX regsets to avoid matching strings in commands/register/register/register_command/TestRegisters.py that cause the test to wrongly assume that I have MPX on this machine.

labath accepted this revision.Nov 16 2020, 1:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2020, 4:03 AM