Page MenuHomePhabricator

[XRay] convert FDR arg1 log entries

Authored by pelikan on May 3 2017, 5:14 PM.



A new FDR metadata record will support logging function arguments
using a 16-bit position (we currently only plan to support "1" for arg1 logging)
and a 64-bit value (useful for "this" pointers and synchronization objects).
If present, we put this argument to the function call "entry" record it belongs
to, and alter its type to notify the user of the first argument's presence.

Diff Detail


Event Timeline

pelikan created this revision.May 3 2017, 5:14 PM
dberris requested changes to this revision.May 3 2017, 5:49 PM
dberris added inline comments.
77–78 ↗(On Diff #97752)

For this API, I think it would make sense for us to use a std::vector<...> or even an llvm::SmallVector<uint64_t, 1> if we ever support more argument logging.

224–227 ↗(On Diff #97752)

If you supported multiple arguments today, you can make this error go away.

228 ↗(On Diff #97752)

Have you considered parsing the following metadata record greedily here, instead of trying to refer to the last record in the vector?

274–279 ↗(On Diff #97752)

Have you considered special handling for the "function with call arguments" instead of trying to handle the metadata records independently? Since this is already a greedy parser, it seems much simpler (and self-contained) to add another state for "we are parsing arguments". This way we can enforce that if we encounter records that indicate we are entering a function with arguments, that we should only look for metadata records following it that contain the arguments.

408–409 ↗(On Diff #97752)

Did clang-format do this?

This revision now requires changes to proceed.May 3 2017, 5:49 PM
dberris added a reviewer: kpw.May 3 2017, 6:44 PM
dberris added a subscriber: kpw.

Adding in @kpw who knows this bit of code a lot better, can probably provide more insights too.

pelikan updated this revision to Diff 104087.Jun 26 2017, 9:51 PM
pelikan edited edge metadata.
pelikan marked 5 inline comments as done.

after long discussions and long time on a different project, update to reflect comments

77–78 ↗(On Diff #97752)

Makes sense. Last discussion did clear up the intents of the API a lot.

224–227 ↗(On Diff #97752)

Changed everything as discussed.

228 ↗(On Diff #97752)

The rest of the code does 1 record at a time, it didn't look appealing to me to deviate. It also isn't required for performance, and it's a very rare case.

274–279 ↗(On Diff #97752)

Again, the rest of the code processes each record separately. It would look uglier IMO if only one case differed.

408–409 ↗(On Diff #97752)

No, I had a previous version of the code there and later erased it. clang-format wants it to be the old way :-(

Looking better. I'll ask for tests now. :)

kpw edited edge metadata.Jul 12 2017, 6:35 PM

Looks reasonable. Here's some suggestions on prior integration tests that can be cargo culted.

If you are OK playing with a hex editor, making a test similar to llvm/test/tools/llvm-xray/X86/convert-fdr-to-yaml.txt could be a quick sanity check.

llvm/projects/compiler-rt/test/xray/TestCases/Linux/ uses a REQUIRES: built-in-llvm-tree to be able to use llvm-xray convert within a lit test after instrumenting a binary as well. You might be able to do something clever to have the program emit the memory address and use lit to capture it and see that it matches the ENTER_ARG value.

228 ↗(On Diff #104087)

we do se -> we do use?

kpw added inline comments.Jul 18 2017, 2:45 PM
56 ↗(On Diff #104087)

I just realized that doing this definition would be unfortunate, because we'll drift out of sync with XRayEntryType defined in compiler-rt/include/xray/xray_interface.h

enum XRayEntryType {
  ENTRY = 0,
  EXIT = 1,
  TAIL = 2,

Could you either define TAIL or assign 3 to ENTER_ARG. Defining TAIL is likely better done in a separate patch that updates the llvm-xray subcommands to handle it.

kpw added inline comments.Jul 18 2017, 2:47 PM
56 ↗(On Diff #104087)

Ugh. These are also defined in AsmPrinter::SledKind.

Is there something else we need to do here?

pelikan updated this revision to Diff 115592.Sep 17 2017, 8:28 PM
pelikan marked 3 inline comments as done.

Rebased to HEAD, added a test. Not sure if it passes as I have to fix a local toolchain issue with some other tests.

pelikan added inline comments.Sep 17 2017, 8:30 PM
56 ↗(On Diff #104087)

Ugh indeed. I was under the impression this should be ignored as compiler-rt is "a different project". I've added the "= 3" there.

228 ↗(On Diff #104087)

Resolved in a different diff.

I'm adding support for tail exit records, can you wait on submitting this as this changes a number of similar places?

89 ↗(On Diff #115592)

What is this meant to do? Is it meant to force text wrapping? If so, please don't do that (it messes with the tests and the settings that can be defined on the YAML stream instead).

I will wait for the other diff to go in then. In the meantime I can sort out my new system's toolchain issues.

89 ↗(On Diff #115592)

It's meant to *remove* text wrapping. Since we only really support one argument in the list, we want it to look like "[ 1 ]", not like "[\n\t1,\n]\n" or whatever it thinks is in order. This way each log entry will only occupy one line, as they should (both for readability and parsability).

dberris requested changes to this revision.Sep 18 2017, 12:29 AM

I think you need to rebase this too. :)

89 ↗(On Diff #115592)

Isn't text wrapping controlled on the stream too? Or is this different?

I guess the question is whether this is actually required, or whether this is something controllable through the serialisation instead of being hard-coded on the type.

This revision now requires changes to proceed.Sep 18 2017, 12:29 AM
pelikan marked an inline comment as done.Sep 19 2017, 11:14 PM
pelikan added inline comments.
89 ↗(On Diff #115592)

This isn't *stream* wrapping, this is just wrapping for the (one element) list of arguments. Trust me, I had to re-read the entire YAML code base just to find this flag, which fortunately does precisely what we want. :-)

pelikan updated this revision to Diff 115963.Sep 19 2017, 11:14 PM
pelikan edited edge metadata.
pelikan marked an inline comment as done.

rebase to HEAD

dberris accepted this revision.Sep 20 2017, 12:26 AM

LGTM -- thanks @pelikan!

This revision is now accepted and ready to land.Sep 20 2017, 12:26 AM
pelikan updated this revision to Diff 116755.Sep 26 2017, 9:44 PM

rebase again; tests now pass

This revision was automatically updated to reflect the committed changes.