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.

Diff Detail

Repository
rL LLVM

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 #72823)

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
1218 ↗(On Diff #72823)

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 #72823)

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
1218 ↗(On Diff #72823)

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
1218 ↗(On Diff #72823)

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.