Page MenuHomePhabricator

[LLDB][MIPS] Fix qProcessInfo to return correct pointer size based on ELF ABI
ClosedPublic

Authored by nitesh.jain on Sep 28 2016, 7:33 AM.

Details

Summary

In case of MIPS64, the pointer size depends on ELF abi. The MIPS64 currently support following abi's

  1. N32 : - The pointer size is 4 byte
  2. N64 :- The pointer size is 8 byte

This patch add one more key (eflags) in qProcessInfo which will be use to get correct pointer size based on abi.

Event Timeline

nitesh.jain retitled this revision from to [LLDB][MIPS] Fix qProcessInfo to return correct pointer size based on ELF ABI.
nitesh.jain updated this object.
nitesh.jain added reviewers: clayborg, labath.
nitesh.jain set the repository for this revision to rL LLVM.
labath edited edge metadata.Sep 28 2016, 7:49 AM

Looks good as far as I am concerned. Just a couple of nits here and there.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
1887 ↗(On Diff #73029)

Why you chose this name? eflags is the name of a i386 register. How about we use a different field name? archflags maybe ?

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
1217 ↗(On Diff #73029)

Why do we have the compilcated switch here. Can't we replace that with:
response.Printf("ptrsize:%d;", proc_arch.GetAddressByteSize()); for all sizes?

clayborg requested changes to this revision.Sep 28 2016, 8:59 AM
clayborg edited edge metadata.

We need to send each flag individually. See inlined comments.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
1887 ↗(On Diff #73029)

We should avoid trying pass ArchSpec specific flags over the wire. Seems like we should be abstracting each flag we want to send as a new key. How is some code that isn't based on LLDB going to fill in these flags? Please add a key/value pair for each flag you want to send. You can add a function to ArchSpec that gets a StructuredData::Dictionary from the ArchSpec and then uses that to populate the key/value pairs that you send back. Each architecture defines a set of unique flags. Another issue with sending e_flags directly is if someone changes the bit definitions of the flags and we have a new LLDB hook up to an older lldb-server, we will get the flags wrong.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
1217 ↗(On Diff #73029)

labath: the default ptr size for MIPS64 is 8. We would need to modify ArchSpec.cpp to look at the flags for MIPS64 and change the pointer size to 4 if the N32 is being used.

This revision now requires changes to proceed.Sep 28 2016, 8:59 AM
nitesh.jain added inline comments.Sep 28 2016, 11:34 PM
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
1217 ↗(On Diff #73029)

Greg: ArchSpec::GetAddressByteSize() will always return pointer size based on abi. So for N64 (default for MIPS64) abi it will return pointer size as 8
labath: We can replace switch with ArchSpec::GetAddressByteSize().

nitesh.jain edited edge metadata.
nitesh.jain removed rL LLVM as the repository for this revision.

We just require ABI information so that auxv vector is parse when lldb try to attach a process via "attach -p pid"

Hi Greg,

Please could you find some time to review this ?

Thanks

clayborg accepted this revision.Oct 10 2016, 12:57 PM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Oct 10 2016, 12:57 PM
This revision was automatically updated to reflect the committed changes.