This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][MIPS] fix Floating point register read/write for big endian
ClosedPublic

Authored by nitesh.jain on Sep 15 2016, 2:48 AM.

Diff Detail

Event Timeline

nitesh.jain retitled this revision from to [LLDB][MIPS] fix Floating point register read/write for big endian.
nitesh.jain updated this object.
nitesh.jain added reviewers: clayborg, labath, jaydeep.
labath added inline comments.Sep 15 2016, 4:55 AM
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
1183

Why do we need to do this remapping? Couldn't we structure the register infos in a way that reg_info->byte_offset is exactly the offset that ptrace expects?

Or are you saying that ptrace does not accept register offsets, but some random register numbers instead? (I cannot tell, as the comment above is confusing.)

1222

This seems like a pretty complicated way to write assert(!reg_info->invalidate_regs);

nitesh.jain added inline comments.Sep 15 2016, 7:43 AM
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
1183

In case of MIPS, ptrace request PTRACE_PEEKUSER/PTRACE_POKEUSER accept register number as an offset. We used reg_info->byte_offset to find register value in the struct GPR_linux_mips. The struct GPR_linux_mips is same for 32 and 64 bit since ptrace always return 64 bit value irrespective of Arch (32 and 64) . Hence we can't modify reg_info->byte_offset to match exactly the offset that ptrace expects.

Updated diff as per suggestion.

nitesh.jain marked an inline comment as done.Sep 15 2016, 8:01 AM
labath added inline comments.Sep 15 2016, 8:14 AM
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
1161

Ok I think I understand this now.
s/has/as/

1183

Ok, I see what you mean. byte_offset is out in that case.
However, I am wondering if we couldn't tweak one of the existing numbering schemes to fit that one. For example, the LLDB numbering scheme is completely arbitrary, and under our control, so what if we just reordered that a bit to match what ptrace expects? I feel that we have enough numbering schemes already, so it should be possible to find one that works without introducing a new one.

This is not that bad, as the "scheme" is confined to the single file, but still you have to admit that this function looks very odd: sometimes you return a register index, sometimes a register offset and sometimes a completely made up number...

1207

Isn't it possible to match the "config5" register by one of the numbering schemes, instead of by name?

nitesh.jain added inline comments.Sep 21 2016, 2:18 AM
source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp
1183

Changing LLDB numbering scheme is not feasible since there are lots of changes both at host and target site. Should I add new numbering scheme ?

labath accepted this revision.Sep 21 2016, 6:21 AM
labath edited edge metadata.

Ok, lets leave that as-is then.. the issue seem s pretty contained for now.

This revision is now accepted and ready to land.Sep 21 2016, 6:21 AM
clayborg requested changes to this revision.Sep 21 2016, 10:01 AM
clayborg edited edge metadata.

So it seems like this can be fixed by doing a few things:
1 - just set the RegisterInfo.offset to the actual offset in the buffer and don't use the offset as the ptrace key used to grab the register
2 - modify the RegisterInfo.kinds[eRegisterKindProcessPlugin] to be the offset to you to use for ptrace

There should be no need what so ever to do manual bit twiddling with src and dst.

This revision now requires changes to proceed.Sep 21 2016, 10:01 AM
nitesh.jain edited edge metadata.

Updated patch as per suggestion.

So it seems like this can be fixed by doing a few things:
1 - just set the RegisterInfo.offset to the actual offset in the buffer and don't use the offset as the ptrace key used to grab the register
2 - modify the RegisterInfo.kinds[eRegisterKindProcessPlugin] to be the offset to you to use for ptrace

There should be no need what so ever to do manual bit twiddling with src and dst.

The floating point register size can be dynamically changes . Hence following cases are possible when we read registers from ptrace in FPR_linux_mips buffer (In case of MIPS, ptrace always return value in 64 bit container irrespective of Arch).

  1. Floating point register size is 32 (SR.FR = 0).

    a) In case of big endian system, the FPR_linux_mips.f0 will contains two floating point registers . The $f0 register will be store in upper 32 bit and $f1 register in lower 32 bit of FPR_linux_mips.f0. The FPR_linux_mips.f1 will contain don't care value and similarly for all other registers. Thus each even buffer will contain valid value and represent two registers.

    b) In case of liitle endian , the $f0 will be store in lower 32 bit and $f1 in upper 32bit of FPR_linux_mips.f0. The FPR_linux_mips.f1 will contain don't care value and similarly for all other registers.
  1. Floating point register size is 64 (SR.FR = 1). In these case all buffer will have valid value.

Hence we need bit twiddling with src and dst (in case of SR.FR = 0) so that each buffer will represent value for single register.

clayborg accepted this revision.Sep 27 2016, 10:19 AM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Sep 27 2016, 10:19 AM

Please comment why you are manually bit twiddling due to the size of the register that can change. We don't want anyone else copying this kind of code. Another solution would be to update the offset of the register when you change the byte size so none of this would need to be done. We are already updating the byte size of the register in the RegisterInfo, so why not do the same for the offset. You could store the original offset in the RegisterInfo.kinds[eRegisterKindProcessPlugin] as this field is only for the process plug-in's use.

nitesh.jain updated this revision to Diff 73278.Oct 3 2016, 7:35 AM
nitesh.jain edited edge metadata.

These diff remove manually bit twiddling due to the size of the floating point register that can change. We use register offset to get floating point register data based on endianess and it's size. We have not remove bit twiddling for MSA register since it's need to be tested( require support in ptrace).

Thanks

Hi Greg,

The diff is update as per suggestion so should I commit this ?

Thanks

Commit anytime.

Commit anytime.

Thanks

This revision was automatically updated to reflect the committed changes.