Page MenuHomePhabricator

Bug 24457 - X87 FPU Special Purpose Registers
ClosedPublic

Authored by abhishek.aggarwal on Sep 3 2015, 2:53 AM.

Details

Summary
  • For 'register read --all' command on x86_64-Linux Platform:
    • Provide correct values of X87 FPU Special Purpose Registers
    • Both 32-bit & 64-bit inferiors give correct values on this Platform
  • Added a Test Vector:
    • To verify the expected behaviour of the command

Signed-off-by: Abhishek Aggarwal <abhishek.a.aggarwal@intel.com>

Diff Detail

Repository
rL LLVM

Event Timeline

abhishek.aggarwal retitled this revision from to Bug 24457 - X87 FPU Special Purpose Registers.
abhishek.aggarwal updated this object.
tfiala edited edge metadata.Sep 3 2015, 8:05 AM

Overall LGTM. I'd put the offset calculation into an inline function.

It sounds like you gave it a test on a 32-bit process running against a 64-bit OS? (I'm not sure if we're still using the x86_64 register contexts on 32-bit processes running on a 64-bit host these days, but it would be a good check).

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
640–642 ↗(On Diff #33920)

It might be worth putting the offset calculation into either a macro or an inlined function since it looks like we're using it in two places.

clayborg requested changes to this revision.Sep 3 2015, 11:59 AM
clayborg edited edge metadata.

I would suggest making a new member variable: "uint32_t m_fpr_offset;" and filling it in once and using as mentioned in the inline comments.

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
563–572 ↗(On Diff #33920)

Seems like the FPR offset could be stored as a member variable and computed once? Then this would become:

uint8_t *src = (uint8_t *)&m_fpr + reg_info->byte_offset - m_fpr_offset;
640–642 ↗(On Diff #33920)

Seems like the FPR offset could be stored as a member variable and computed once? Then this would become:

uint8_t *dst = (uint8_t *)&m_fpr + reg_info->byte_offset - m_fpr_offset;
This revision now requires changes to proceed.Sep 3 2015, 11:59 AM
abhishek.aggarwal edited edge metadata.

Created a data member in the class to store byte offset of fctrl

Changed the implementation accordingly

Overall LGTM. I'd put the offset calculation into an inline function.

It sounds like you gave it a test on a 32-bit process running against a 64-bit OS? (I'm not sure if we're still using the x86_64 register contexts on 32-bit processes running on a 64-bit host these days, but it would be a good check).

Yes. I tested 64-bit inferiors as well as 32-bit inferiors on 64-bit Linux OS. For 32-bit inferior case, we initialize the register offsets with i386 register offsets calculations but we overwrite them with x86_64 register offsets as soon as we figure out that inferiors will be running on x86_64 target system. Hence, this works fine there also.

tfiala accepted this revision.Sep 4 2015, 8:07 AM
tfiala edited edge metadata.

LGTM. Thanks for tracking that down!

clayborg accepted this revision.Sep 4 2015, 10:00 AM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Sep 4 2015, 10:00 AM
This revision was automatically updated to reflect the committed changes.