[Lldb-commits] [PATCH] D91504: [lldb] Use translated full ftag values [WIP]

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Looks reasonable to me.



================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:534
+  // TODO
+  if (reg_info->kinds[lldb::eRegisterKindLLDB] == lldb_ftag_x86_64) {
+    uint8_t abridged_tw = *(uint8_t *)src;
----------------
mgorny wrote:
> @labath, any suggestion what kind of test to use here? Maybe against `&m_xstate->fxsave.ftag` address?
You mean, like, how to detect that we're dealing with the ftag register?

This approach seems reasonable to me..


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContext_x86.h:381
 
+inline uint16_t AbridgedToFullTagWord(uint8_t abridged_tw, uint16_t sw, MMSReg *st_regs) {
+  // Tag word is using internal FPU register numbering rather than ST(i).
----------------
This is probably complex enough to deserve creating a cpp file (and a header comment explaining the high-level purpose of the function).


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContext_x86.h:381
 
+inline uint16_t AbridgedToFullTagWord(uint8_t abridged_tw, uint16_t sw, MMSReg *st_regs) {
+  // Tag word is using internal FPU register numbering rather than ST(i).
----------------
labath wrote:
> This is probably complex enough to deserve creating a cpp file (and a header comment explaining the high-level purpose of the function).
Also, probably ArrayRef<MMSReg> instead of the raw pointer.


================
Comment at: lldb/unittests/Process/Utility/RegisterContextTest.cpp:31
+
+std::array<MMSReg, 8> st_regs = {
+	st_from_comp(0x8000000000000000, 0x4000), // +2.0
----------------
Add const, for good measure (I see that will result in additional const qualifications in a bunch of places.


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

https://reviews.llvm.org/D91504