Page MenuHomePhabricator

[lldb] [Process/NetBSD] Use XStateRegSet for all FPU registers
ClosedPublic

Authored by mgorny on Oct 23 2020, 5:40 AM.

Details

Summary

Unify the x86 regset API to use XStateRegSet for all FPU registers,
therefore eliminating the legacy API based on FPRegSet. This makes
the code a little bit simpler but most notably, it provides future
compatibility for register caching.

Since the NetBSD kernel takes care of providing compatibility with
pre-XSAVE processors, PT_{G,S}ETXSTATE can be used on systems supporting
only FXSAVE or even plain FSAVE (and unlike PT_{G,S}ETXMMREGS, it
clearly indicates that XMM registers are not supported).

Diff Detail

Event Timeline

mgorny created this revision.Oct 23 2020, 5:40 AM
mgorny requested review of this revision.Oct 23 2020, 5:40 AM

I don't know much about netbd, but it definitely seems cleaner...

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

This is in an anonymous namespace, so static is superfluous. However, this anonymous namespace is against the coding standards so you might as well finish adding statics, and remove it. :)

989–991

Is this code actually reachable? Ideally, I'd expect this to be checked immediately after program startup, so that we can avoid even reporting the existence of [xy]mm registers..

mgorny updated this revision to Diff 300277.Oct 23 2020, 6:58 AM
mgorny marked an inline comment as done.

Removed anonymous namespace and obsolete #define.

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
989–991

Yes, it is. I've just run it inside qemu with an emulated non-AVX CPU, and the code definitely gets run. The user-visible result is apparently that the registers are reported as unavailable.

I suppose we could use sysctl (or cpuid) to check for supported instruction sets in constructor but I don't really know how to make LLDB consider them non-existing.

In any case, I think that's material for a separate patch.

krytarowski accepted this revision.Oct 23 2020, 11:08 AM
This revision is now accepted and ready to land.Oct 23 2020, 11:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2020, 12:18 AM
labath added inline comments.Oct 26 2020, 3:00 AM
lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
989–991

I think you can make them non-existent by returning a smaller number via GetUserRegisterCount. Linux code has something like that, though I'm not sure that it is entirely correct.

Yeah, this is for a separate patch, though it has some implications for the caching code. The current linux code works by assuming that each "register set" is either entirely present or entirely absent (a reasonable assumption I'd say). That might make things tricky of one wants to have a single register set for reading/caching all of xstate data.

I'm wondering if register sets (as presented to the user) and register sets for the purposes of caching (maybe these should have a different name) shouldn't be distinct concepts...

mgorny added inline comments.Oct 26 2020, 3:48 AM
lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
989–991

I've been thinking about having two levels of 'register sets' but I've came to the conclusion that the benefit is not worth the added complexity.

I'm not sure I agree with that. Given the ever-growing number of registers (which are likely going to all end up in the "xstate" block), I think it is important to be able to add some structure to the list of registers. Also, having the ability to group registers which can come or go together will probably make it easier to implement the "disappearance" of unsupported registers. Given how xstate works, I think the individual register sets are a better match (more or less) for the individual "features" of that state rather than for the block as a whole.

I don't think it should be that hard to come up with a solution that allows caching at xstate level, but still keeps finer-grained groups. One way, which does not require the explicit concept of a "second order group", is to allow the cache reading/writing function to mark multiple sets as read/written. In this world it wouldn't be the generic code which manages the state of each set, but the individual writing function instead. This way the reading/writing function could mark any number of registers sets as read/written. The generic code could just assert that the set that it cares about has been processed correctly.

I wonder if 'register sets' shouldn't become shared LLVM concept rather than per-plugin.

I wonder if 'register sets' shouldn't become shared LLVM concept rather than per-plugin.

I'm totally down with that. In fact there's been some movement on that front recently, with the introduction of RegisterInfoAndSetInterface class (currently only used for arm).