[Lldb-commits] [PATCH] D89193: [lldb] [Process/FreeBSDRemote] Support YMM reg via PT_*XSTATE

[Michał Górny via Phabricator via lldb-commits]

mgorny marked 4 inline comments as done.
mgorny added inline comments.


================
Comment at: lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.cpp:448-449
+
+    assert(info.xsave_mask & XFEATURE_ENABLED_X87);
+    assert(info.xsave_mask & XFEATURE_ENABLED_SSE);
+
----------------
krytarowski wrote:
> emaste wrote:
> > mgorny wrote:
> > > mgorny wrote:
> > > > emaste wrote:
> > > > > I wonder if these should be an error rather than assertion?
> > > > I suppose the question is if they ever happen in real use. If they do, we should probably handle them gracefully. Otherwise, assertion should be sufficient.
> > > I can actually answer the first question myself. According to Intel's manual, it is impossible to disable x87 bit. IIRC attempt to unset it on XCR0 will raise some fault.
> > > 
> > > The second question is basically whether under any circumstances can FreeBSD kernel disable SSE on XCR0 (this code is only used on systems supporting XSAVE).
> > I guess my point is that having these bits unset would indicate a kernel issue or bug, or maybe hardware issue, but never indicate an error or invalid operation in lldb itself.
> > 
> > Either way I think there is no practical impact, it's not actually going to happen.
> If we assert on this code we more trigger software bug in lldb rather than a hardware specifics.
I think that's roughly the purpose assertions serve. If something is terribly broken, it will blow up on the developers but it will be optimized away in normal work.


================
Comment at: lldb/source/Plugins/Process/FreeBSDRemote/NativeRegisterContextFreeBSD_x86_64.cpp:479
+  case XSaveRegSet:
+    // TODO: can WriteRegisterSet() ever be called without ReadRegisterSet()?
+    assert(m_xsave.size() > 0);
----------------
labath wrote:
> mgorny wrote:
> > @labath, is this a safe assumption to make? Generally, if `ReadRegisterSet()` is not called, `m_xsave` will be zero-sized.
> Interesting question. I don't really have an answer to that. Somebody obviously should fill out `m_xsave` before calling this function, but maybe it doesn't have to be `ReadRegisterSet()`? What will happen if the client sends a `G` packet before reading any registers?
> 
> That said, I'm not particularly troubled by this assertion. I wouldn't be surprised if we already had code making that assumption without even realizing it.
Actually, I didn't realize `WriteRegisterSet()` is local to the plugin. In this case, I can easily verify it's never called like this, so I'll just update the comment and leave the assert so that we don't break it in the future.


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

https://reviews.llvm.org/D89193