Page MenuHomePhabricator

Add target.xml support for qXfer request.

Authored by PatriosTheGreat on Feb 7 2020, 6:05 AM.



Requesting registers one by one takes a while in our project.
We want to get rid of it by using target.xml.

Diff Detail

Event Timeline

PatriosTheGreat created this revision.Feb 7 2020, 6:05 AM
PatriosTheGreat set the repository for this revision to rG LLVM Github Monorepo.
PatriosTheGreat edited the summary of this revision. (Show Details)Feb 7 2020, 6:26 AM
jarin added inline comments.Feb 7 2020, 6:45 AM

As discussed offline, this should not be hard-coded, register context provides register count.


Fill all the relevant fields, please (everything that the qRegisterInfo handler provides).

labath added a comment.Feb 7 2020, 9:24 AM

Yeah, we should fill in the other attributes too. As that will probably noticeably increase the amount of code, it would be good to put this stuff into a separate function (or two..). For a test, you could create some kind of a lldb-server test (packages/Python/lldbsuite/test/tools/lldb-server/). I am imagining something that will check that this parses as valid xml, and then does some kind of basic validation on the contents.


"target.xml" would be a better buffer name than __FUNCTION__.

I think this is looking pretty good overall. I just have a bunch of stylistic remarks...


I don't think the choice of inferior really matters here. This code could just be like int main(){}


You can put this outside the #ifdef


These should return a llvm::StringRef to avoid copies, and take the argument as a reference to convey non-nullness.


Please move this into a separate function


This check could be factored to the front of the function (the svr4 branch does currently not have it, but it most likely should).


We generally don't put curly braces around statements which fit on a single line.


Make a utility function (lambda?) for this?

Just a couple of comments to make this code more llvm-y. Without those, we have just moved the place which constructs the temporary std::string. I haven't highlighted all the uses, but overall I'd recommend using the Format function or something similar instead of Printf, as the latter can be error-prone (because of the need to pass the right PRIx macro to get the integer size right) and/or inefficient (need to do the .str().c_str() dance on StringRefs).


Despite the name, PutCString actually takes a StringRef, so you can just drop the .str().c_str() thingy. In fact, I'd consider just writing response << "encoding:" << encoding << ';';


Similarly, response << "encoding='" << encoding << "' ", or response.Format("encoding='{0}'", encoding) would be shorter, and avoid string copying.


no braces

Just one more thing I didn't catch in the previous round :(.


Sorry I wasn't clear about this, but my idea was to make a _single_ utility function for this. If you pass in the uint32_t* to the function instead of the whole RegisterInfo struct, then you can use the same code in both places..

labath accepted this revision.Feb 12 2020, 10:58 PM

Looks good. Thanks for doing this.

This revision is now accepted and ready to land.Feb 12 2020, 10:58 PM

Hi Pavel,
Thank you for review.
Could you also commit this to master, since I don't have commit access?

This revision was automatically updated to reflect the committed changes.

Committed as aedc196101e33bd58f7443c5b93398418ce55edf. I've updated the commit message to include a better description of what is being changed.

omjavaid reopened this revision.Feb 17 2020, 9:10 PM
omjavaid added a subscriber: omjavaid.
This revision is now accepted and ready to land.Feb 17 2020, 9:10 PM
omjavaid requested changes to this revision.Feb 17 2020, 9:20 PM

I have reverted this change temporarily. Please look at the test failure. I ll also do the same at my end. Thanks!

This revision now requires changes to proceed.Feb 17 2020, 9:20 PM
jarin added inline comments.Feb 17 2020, 10:33 PM

I think this is not correct: target.xml expects this to be decimal. (This is different from qRegisterInfo, which expects hex.)

jarin added inline comments.Feb 17 2020, 10:42 PM

Why don't you test all the fields here?


Nit: Now it is a funny mixture of operator<<, Printfs and PutCString. Is there a reason not to use << for everything? (I guess PutHex8 can't be easily done with <<, but everything else can?)

jarin added inline comments.Feb 17 2020, 10:47 PM

I know this is copy&paste from qRegisterInfo, but should not this be if (reg_info->invalidate_regs && reg_info->invalidate_regs[0] != LLDB_INVALID_REGNUM)?

labath added inline comments.Feb 17 2020, 11:26 PM

Yeah, I'm all for using anything else than printf (operator<<, the Format function, etc.). I just didn't feel like raising this point at that time. :/

Hi Muhammad,

Thank you for finding this issue.
After I fix that, could I update current review or should I create new one?

Please update the current one to keep the context together.

In this revision I fix value_regnums and invalidate_regnums serialization in target.xml.
However I'm not so sure that this is the cause of problem with ARM tests.

From tests log it seems like there are some unexpected value in register.
Is it possible to get logs output from this test run that would help us a lot to fix it.

omjavaid accepted this revision.Feb 19 2020, 5:27 AM

I have tested this not sure why it actually failed on the buildbot. The test seems to pass independently. Lets see if the new version fixes the issue.

This revision is now accepted and ready to land.Feb 19 2020, 5:27 AM

Hi Muhammad,
Thank you for review.
Could you or Pavel commit this to master, since I don't have commit access?

Hi Muhammad,
Thank you for review.
Could you or Pavel commit this to master, since I don't have commit access?

Hi Patrios,

There was some problem with our buildbot and it went down overnight. I have put it back in place and will commit your patch today.

This revision was automatically updated to reflect the committed changes.