Page MenuHomePhabricator

LLDB qXfer:features:read support
ClosedPublic

Authored by ADodds on Apr 13 2015, 4:47 AM.

Details

Summary

This is a preliminary patch that adds lldb support for querying the register mapping from gdbserver remote targets using qXfer:features:read packet.
I have been testing this patch using x86 gdbserver running on the android emulator however I will be trying more gdbserver flavours (arm/mips) in the next few days.
Since this patch ties into a range of place in LLDB I thought It would be best to submit it early to get some feedback.

With this patch, upon connection to a remote, if lldb has no register information loaded and it sees the target supports qXfer:features:read, it will try to extract register info at that point.
It will parse all scalar registers, and translates this directly into gdb remote dynamic register info. Still to be implemented are vector register and flag register layout parsing.

This is currently enough to enable basic debugging via gdbserver from lldb with no prior knowledge of the target registers.

C&C will be appreciated.

A quick note on how I compiled libxml2 on windows and linked lldb against it:

I downloaded the latest libxml2 snapshot:
ftp://xmlsoft.org/libxml2/libxml2-git-snapshot.tar.gz

After unpacking I navigated to libxml2-2.9.2\win32.

I ran a configuration using the following command:
D:\libxml2\libxml2-2.9.2\win32>cscript configure.js compiler=msvc prefix=c:\opt include=c:\opt\include lib=c:\opt\lib debug=yes iconv=no

use [vcvars32.bat for 32bit] build, or [vcvars64.bat for 64bit] build

to kickstart compilation run:
D:\libxml2\libxml2-2.9.2\win32>nmake /f Makefile.msvc

The libs can be found in the following path:
D:\libxml2\libxml2-2.9.2\win32\bin.msvc

These libs were then supplied when running CMake for llvm:
  LIBXML2_INCLUDE_DIR=D:/libxml2/include
  LIBXML2_LIBRARIES=D:/libxml2/win32/bin.msvc/libxml2.lib

Also, here is a quick view of the initial gdb-rsp transcript for quick reference:

(lldb) log enable gdb-remote packets
(lldb) target create d:/test.so
Current executable set to 'd:/test.so' (i386).
(lldb) gdb-remote 1234
<   1> send packet: +
history[1] tid=0x1dac <   1> send packet: +
<  19> send packet: $QStartNoAckMode#b0
<   1> read packet: +
<   6> read packet: $OK#9a
<   1> send packet: +
<  41> send packet: $qSupported:xmlRegisters=i386,arm,mips#12
< 542> read packet: $PacketSize=3fff;QPassSignals+;QProgramSignals+;qXfer:libraries-svr4:read+;qXfer:auxv:read+;qXfer:spu:read+;qXfer:spu:wr
ite+;qXfer:siginfo:read+;qXfer:siginfo:write+;qXfer:features:read+;QStartNoAckMode+;qXfer:osdata:read+;multiprocess+;QNonStop+;qXfer:threads
:read+;ConditionalTracepoints+;TraceStateVariables+;TracepointSource+;DisconnectedTracing+;StaticTracepoints+;InstallInTrace+;qXfer:statictr
ace:read+;qXfer:traceframe-info:read+;EnableDisableTracepoints+;QTBuffer:size+;tracenz+;ConditionalBreakpoints+;BreakpointCommands+;QAgent+#
9f
<  40> send packet: $qXfer:features:read:target.xml:0,fff#7d
< 582> read packet: $6c3c3f786d6c2076657273696f6e3d22312e30223f3e0a3c212d2d20436f707972696768742028432920323031302d32303133204672656520536f6
6747761726520466f756e646174696f6e2c20496e632e0a0a202a21436f7079696e6720616e6420646973747269627574696f6e206f6620746869732066696c652c207769746
8206f7220776974686f7574206d6f64696669636174696f6e2c0a202a21617265207065726d697474656420696e20616e79206d656469756d20776974686f757420726f79616
c74792070726f76696465642074686520636f707972696768740a202a216e6f7469636520616e642074686973206e6f7469636520617265207072657365727665642e20202d2
d3e0a0a3c212d2d2049333836207769746820535345202d20496e636c75646573204c696e75782d6f6e6c79207370656369616c20227265676973746572222e20202d2d3e0a0
a3c21444f4354595045207461726765742053595354454d20226764622d7461726765742e647464223e0a3c7461726765743e0a20203c6172636869746563747572653e69333
8363c2f6172636869746563747572653e0a20203c6f736162693e474e552f4c696e75783c2f6f736162693e0a20203c78693a696e636c75646520687265663d2233326269742
d636f72652e786d6c222f3e0a20203c78693a696e636c75646520687265663d2233326269742d6c696e75782e786d6c222f3e0a20203c78693a696e636c75646520687265663
d2233326269742d7373652e786d6c222f3e0a3c2f7461726765743e0a#75
noname.xml:14: namespace error : Namespace prefix xi on include is not defined
  <xi:include href="32bit-core.xml"/>
                                   ^
noname.xml:15: namespace error : Namespace prefix xi on include is not defined
  <xi:include href="32bit-linux.xml"/>
                                    ^
noname.xml:16: namespace error : Namespace prefix xi on include is not defined
  <xi:include href="32bit-sse.xml"/>
                                  ^
<  44> send packet: $qXfer:features:read:32bit-core.xml:0,fff#70
<2736> read packet: $6c3c3f786d6c2076657273696f6e3d22312e30223f3e0a3c212d2d20436f707972696768742028432920323031302d32303133204672656520536f6
6747761726520466f756e646174696f6e2c20496e632e0a0a202a21436f7079696e6720616e6420646973747269627574696f6e206f6620746869732066696c652c207769746
8206f7220776974686f7574206d6f64696669636174696f6e2c0a202a21617265207065726d697474656420696e20616e79206d656469756d20776974686f757420726f79616
c74792070726f76696465642074686520636f707972696768740a202a216e6f7469636520616e642074686973206e6f7469636520617265207072657365727665642e20202d2
d3e0a0a3c21444f435459504520666561747572652053595354454d20226764622d7461726765742e647464223e0a3c66656174757265206e616d653d226f72672e676e752e6
764622e693338362e636f7265223e0a20203c666c6167732069643d22693338365f65666c616773222073697a653d2234223e0a202a203c6669656c64206e616d653d2243462
...

Unfortunately, by default libxml2 outputs warnings on stdout as it parses which is why the following messages show up. In a revised patch I will suppress this completely or pipe it into one of the logging channels.

noname.xml:16: namespace error : Namespace prefix xi on include is not defined
  <xi:include href="32bit-sse.xml"/>
                                  ^

Diff Detail

Repository
rL LLVM

Event Timeline

ADodds updated this revision to Diff 23670.Apr 13 2015, 4:47 AM
ADodds retitled this revision from to LLDB qXfer:features:read support.
ADodds updated this object.
ADodds edited the test plan for this revision. (Show Details)
ADodds added reviewers: zturner, clayborg.
ADodds set the repository for this revision to rL LLVM.
ADodds added a subscriber: Unknown Object (MLST).
ADodds updated this object.Apr 13 2015, 4:49 AM
ADodds updated this object.Apr 13 2015, 5:44 AM
ADodds updated this object.Apr 13 2015, 5:47 AM
ADodds updated this object.Apr 13 2015, 5:50 AM
ADodds updated this revision to Diff 23677.Apr 13 2015, 8:36 AM

This change adds support for gdbserver running on android Arm. It also fixes a bug where register discovery would bomb out if the osabi name wasnt given in target.xml. I have moved gdbserver register discovery inside of BuildDynamicRegisterInfo() so it will only be used when registers dont already exist (as intended).

clayborg requested changes to this revision.Apr 13 2015, 11:28 AM
clayborg edited edge metadata.

Pretty close, just a few things to fix. We don't need the "gdbserver-compatibility" setting. We should always use the qRegisterInfo packets first, if those aren't supported, then fall back to the "qXfer:features:", and if that fails, then fall back to the target definition python file.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
380–395 ↗(On Diff #23677)

No one else is using this function, are you going to need it elsewhere? If not, just inline the contents in the one function that calls it. Otherwise this is fine.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
110 ↗(On Diff #23677)

Why do we need a setting for this? We should always try the qRegisterInfo first and then fall back to using the qXfer:features: stuff.

118 ↗(On Diff #23677)

Do we need this? See comment on line 110 of ProcessGDBRemote.cpp.

164–169 ↗(On Diff #23677)

Do we need this? See comment on line 110 of ProcessGDBRemote.cpp.

401–404 ↗(On Diff #23677)

This code should be moved to just after the qRegisterInfo packets are called and it should only be executed if the qRegisterInfo packets are not supported. See non-inlined comments for more info.

1680 ↗(On Diff #23677)

We should call this first and always try to use the qRegisterInfo packet, and if this fails, fall back to the qXfer:features: and then if that fails, use the target definition file.

This revision now requires changes to proceed.Apr 13 2015, 11:28 AM
ADodds updated this revision to Diff 23736.Apr 14 2015, 9:16 AM
ADodds added a reviewer: domipheus.

Thanks for having a look at my previous patch Greg. I have made a revision that hopefully addresses all the points you had raised earlier. Any other C&C is welcome.

A few things to fix:

  • remove the unused ePropertyGdbServerCompatibility enum
  • Use StringStream instead of std::stringstream to keep things consistent with all other send packets

This:

+ // build the qSupported packet
+ std::vector<std::string> features = {"xmlRegisters=i386,arm,mips"};
+ std::stringstream packet;
+ packet << "qSupported";
+ for ( uint32_t i = 0; i < features.size( ); i++ )
+ packet << (i==0 ? ":" : ";") << features[i];

can become

std::vector<std::string> features = {"xmlRegisters=i386,arm,mips"};
StringStream packet;
packet.PutCString("qSupported");
for (const auto& feature: features)
{

packet.Printf("%c%s", (i==0 ? ':' : ';'), feature.c_str());

}

ADodds updated this revision to Diff 23783.Apr 15 2015, 10:25 AM
ADodds edited edge metadata.

Thanks again Greg.

I have hopefully addressed the things you pointed out, apart from one issue with the for loop. I would have liked to use a for each but unfortunately I need to append a different character on the first itteration of the loop, which makes it a little awkward. It seemed to me like the original for loop was less awkward then using a for each and having a new variable to track the first itteration.

clayborg accepted this revision.Apr 15 2015, 11:41 AM
clayborg edited edge metadata.

Very nice. All changes look good. Thanks for adding this and improving LLDB's GDB server support!

This revision is now accepted and ready to land.Apr 15 2015, 11:41 AM
This revision was automatically updated to reflect the committed changes.
dawn added a subscriber: dawn.Jun 11 2015, 12:15 PM
This comment was removed by dawn.
dawn added a comment.Jun 11 2015, 12:20 PM

Removed comment (was added to wrong patch - oops)

zturner edited edge metadata.Jun 11 2015, 12:57 PM

I will to have a look, unfortunately after tomorrow I will be away for an
extended period of time and I need to finish something else up before then
too. That being said, I suspect this is just an issue of the number that
is being printed, and not of which tests are actually being run. I didn't
change anything related to what tests run, only how we display the number.
I'm betting those 548 tests are either skipped tests, xfail'ed tests, or
both.