Bug 25050: X87 FPU Special Purpose Registers

[abhishek.aggarwal]

Summary

• For x86_64-FreeBSD Platform:
  • LLDB now provides correct values of X87 FPU Special Purpose Registers like fstat, ftag, fctrl etc..

Signed-off-by: Abhishek Aggarwal <[e-mail]>

[clayborg]

Looks good as long as all test suite tests are happy. We should probably add tests for specific architectures to load registers with known values and verify they are correct, but that can be done in a later patch.

[abhishek.aggarwal]

A test was laready present where registers were loaded from lldb command terminal with specific values and then read to compare the values. However, the test still passed because lldb wrote the register values at wrong offsets, but then it read the register values from the same wrong offsets also.

To expose this, I had submitted a new test where the inferior itself pushes some values in these registers and lldb just reads these values for comparison. This way the wrong offset bug was resolved. But it is specific to IA architecture only. For other architectures, we may have to include a similar test.

[emaste]

Thank you for fixing this! Sorry I didn't have a chance to review or test - I left for a conference on the 5th and did not spot it until returning today.

[abhishek.aggarwal]

I thought that the patch can be landed even if one of the reviewers accepts it. Is it so that the patch requires approval from all the reviewers? If that is the case then I am extremely sorry to land it without your approval.

[emaste]

In D13434#260701, @abhishek.aggarwal wrote:

I thought that the patch can be landed even if one of the reviewers accepts it. Is it so that the patch requires approval from all the reviewers? If that is the case then I am extremely sorry to land it without your approval.

No, landing it after @clayborg approved is absolutely fine. I didn't mean to imply there's any trouble with you landing the change.

I just try to keep up with FreeBSD changes (reviewing and testing) to help ensure they make it into the tree in a timely manner.

source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_x86.h

protected:
     SetHardwareWatchpointWithIndex(lldb::addr_t addr, size_t size, bool read, bool write, uint32_t hw_index);
 
     uint32_t
     NumSupportedHardwareWatchpoints();
 
 private:
     ProcessMonitor &
     GetMonitor();
+    uint32_t m_fctrl_offset_in_userarea;  // Offset of 'fctrl' in 'UserArea' Structure
 };
 
 #endif

source/Plugins/Process/FreeBSD/RegisterContextPOSIXProcessMonitor_x86.cpp

size_and_rw_bits(size_t size, bool read, bool write)
     }
 }
 
 RegisterContextPOSIXProcessMonitor_x86_64::RegisterContextPOSIXProcessMonitor_x86_64(Thread &thread,
                                                                                      uint32_t concrete_frame_idx,
                                                                                      lldb_private::RegisterInfoInterface *register_info)
     : RegisterContextPOSIX_x86(thread, concrete_frame_idx, register_info)
 {
+    // Store byte offset of fctrl (i.e. first register of FPR) wrt 'UserArea'
+    const RegisterInfo *reg_info_fctrl = GetRegisterInfoByName("fctrl");
+    m_fctrl_offset_in_userarea = reg_info_fctrl->byte_offset;
 }
 
 ProcessMonitor &
 RegisterContextPOSIXProcessMonitor_x86_64::GetMonitor()
 {
     ProcessSP base = CalculateProcess();
     ProcessFreeBSD *process = static_cast<ProcessFreeBSD*>(base.get());
     return process->GetMonitor();
if (reg_info->encoding == eEncodingVector)
                     return false;
             }
             return value.GetType() == RegisterValue::eTypeBytes;
         }
         return false;
     }
 
     // Get pointer to m_fpr.xstate.fxsave variable and set the data from it.
-    assert (reg_info->byte_offset < sizeof(m_fpr));
-    uint8_t *src = (uint8_t *)&m_fpr + reg_info->byte_offset; 
+    // Byte offsets of all registers are calculated wrt 'UserArea' structure.
+    // However, ReadFPR() reads fpu registers {using ptrace(PT_GETFPREGS,..)}
+    // and stores them in 'm_fpr' (of type FPR structure). To extract values of fpu
+    // registers, m_fpr should be read at byte offsets calculated wrt to FPR structure.
+
+    // Since, FPR structure is also one of the member of UserArea structure.
+    // byte_offset(fpu wrt FPR) = byte_offset(fpu wrt UserArea) - byte_offset(fctrl wrt UserArea)
+    assert ( (reg_info->byte_offset - m_fctrl_offset_in_userarea) < sizeof(m_fpr));
+    uint8_t *src = (uint8_t *)&m_fpr + reg_info->byte_offset - m_fctrl_offset_in_userarea;
     switch (reg_info->byte_size)
     {
         case 2:
             value.SetUInt16(*(uint16_t *)src);
             return true;
         case 4:
             value.SetUInt32(*(uint32_t *)src);
             return true;
if (IsFPR(reg, GetFPRType()))
                ::memcpy (m_ymm_set.ymm[reg - m_reg_info.first_ymm].bytes, value.GetBytes(), value.GetByteSize());
                if (false == CopyYMMtoXSTATE(reg, GetByteOrder()))
                    return false;
             }
         }
         else
         {
             // Get pointer to m_fpr.xstate.fxsave variable and set the data to it.
-            assert (reg_info->byte_offset < sizeof(m_fpr));
-            uint8_t *dst = (uint8_t *)&m_fpr + reg_info->byte_offset; 
+            // Byte offsets of all registers are calculated wrt 'UserArea' structure.
+            // However, WriteFPR() takes m_fpr (of type FPR structure) and writes only fpu
+            // registers using ptrace(PT_SETFPREGS,..) API. Hence fpu registers should
+            // be written in m_fpr at byte offsets calculated wrt FPR structure.
+
+            // Since, FPR structure is also one of the member of UserArea structure.
+            // byte_offset(fpu wrt FPR) = byte_offset(fpu wrt UserArea) - byte_offset(fctrl wrt UserArea)
+            assert ( (reg_info->byte_offset - m_fctrl_offset_in_userarea) < sizeof(m_fpr));
+            uint8_t *dst = (uint8_t *)&m_fpr + reg_info->byte_offset - m_fctrl_offset_in_userarea;
             switch (reg_info->byte_size)
             {
                 case 2:
                     *(uint16_t *)dst = value.GetAsUInt16();
                     break;
                 case 4:
                     *(uint32_t *)dst = value.GetAsUInt32();
                     break;