[Lldb-commits] [PATCH] D90034: [lldb] [Process/NetBSD] Use XStateRegSet for all FPU registers

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.
Herald added a subscriber: JDevlieghere.


================
Comment at: lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp:994-995
         !(m_xstate.xs_rfbm & XCR0_YMM_Hi128)) {
-      error.SetErrorStringWithFormat("register \"%s\" not supported by CPU/kernel",
-                                     reg_info->name);
+      error.SetErrorStringWithFormat(
+          "register \"%s\" not supported by CPU/kernel", reg_info->name);
     } else {
----------------
mgorny wrote:
> labath wrote:
> > 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..
> 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.
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...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90034/new/

https://reviews.llvm.org/D90034