This is an archive of the discontinued LLVM Phabricator instance.

[lldb] [Process/NetBSD] Support reading YMM registers via PT_*XSTATE
ClosedPublic

Authored by mgorny on Jun 19 2019, 5:53 AM.

Details

Summary

Provide a (conditional) support for the new PT_GETXSTATE
and PT_SETXSTATE ptrace() requests, and use them to implement getting
and setting YMM registers. The functions used for splitting
and recombining YMM register data are based on matching functions
in FreeBSD plugin, with some simplification and updates to match NetBSD
structures.

Diff Detail

Event Timeline

mgorny created this revision.Jun 19 2019, 5:53 AM
mgorny updated this revision to Diff 205763.Jun 20 2019, 2:40 AM

Rebased for changes in NativeRegisterContextNetBSD_x86_64::GetSetForNativeRegNum(), and removed the conditions there.

We have the same XSTATE<->YMM conversion functions in NativeProcessLinux. It would be nice to extract them to some common place (Plugins/Process/Utility, I guess :P). The rest seems pretty straight-forward, though you're repeating the patterns that I find really reduntant/annoying. Eg. I don't see the reason to check the null-ness of the XState buffer. It sounds like it should be a hard error for someone to call ReadXState without bothering to arrange for the storage buffer to exist.

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
251

Why would we ever want to do this?

mgorny marked an inline comment as done.Jun 20 2019, 3:56 AM

We have the same XSTATE<->YMM conversion functions in NativeProcessLinux. It would be nice to extract them to some common place (Plugins/Process/Utility, I guess :P).

Hmm, I guess that's doable if we pass the relevant struct fields as pointers.

The rest seems pretty straight-forward, though you're repeating the patterns that I find really reduntant/annoying. Eg. I don't see the reason to check the null-ness of the XState buffer. It sounds like it should be a hard error for someone to call ReadXState without bothering to arrange for the storage buffer to exist.

Yeah, I guess that's something I can fix globally first.

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
251

I was asking myself the exact same thing! But the code is present both on Linux and FreeBSD, so I presumed there is some fancy use case I have no clue about.

We have the same XSTATE<->YMM conversion functions in NativeProcessLinux. It would be nice to extract them to some common place (Plugins/Process/Utility, I guess :P).

Hmm, I guess that's doable if we pass the relevant struct fields as pointers.

Maybe have GetYMM(unsigned num, YMM& reg)/SetYMM(unsigned num, const YMM &reg) methods on the XSAVE struct ?

The rest seems pretty straight-forward, though you're repeating the patterns that I find really reduntant/annoying. Eg. I don't see the reason to check the null-ness of the XState buffer. It sounds like it should be a hard error for someone to call ReadXState without bothering to arrange for the storage buffer to exist.

Yeah, I guess that's something I can fix globally first.

Sounds great.

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
251

Yeah, I don't think that's a safe assumption around here. :)

We have the same XSTATE<->YMM conversion functions in NativeProcessLinux. It would be nice to extract them to some common place (Plugins/Process/Utility, I guess :P).

Hmm, I guess that's doable if we pass the relevant struct fields as pointers.

Maybe have GetYMM(unsigned num, YMM& reg)/SetYMM(unsigned num, const YMM &reg) methods on the XSAVE struct ?

We aren't using XSAVE struct. We are using NetBSD-specific struct xstate that has guaranteed fixed offsets.

Maybe have GetYMM(unsigned num, YMM& reg)/SetYMM(unsigned num, const YMM &reg) methods on the XSAVE struct ?

We aren't using XSAVE struct. We are using NetBSD-specific struct xstate that has guaranteed fixed offsets.

Ah, right, I forgot about that part. In that case, it's probably not worth doing that...

krytarowski added inline comments.Jun 20 2019, 9:16 AM
lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
251

Right now x86 supports only LE on NetBSD.

mgorny updated this revision to Diff 205998.Jun 21 2019, 7:50 AM

Updated to use new XState conversion methods.

mgorny updated this revision to Diff 205999.Jun 21 2019, 7:51 AM

…and removed stale declarations.

mgorny retitled this revision from [lldb] [Process/NetBSD] Support reading YMM registers via PT_*XSTATE [DO NOT MERGE] to [lldb] [Process/NetBSD] Support reading YMM registers via PT_*XSTATE.Jun 27 2019, 12:18 PM
mgorny edited the summary of this revision. (Show Details)

This is ready to be reviewed now.

labath accepted this revision.Jul 1 2019, 2:39 AM

I don't know whether the code is correct for NetBSD, but it looks ok from a general readability perspective.

This revision is now accepted and ready to land.Jul 1 2019, 2:39 AM
krytarowski accepted this revision.Jul 1 2019, 7:39 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2019, 8:11 AM