This is an archive of the discontinued LLVM Phabricator instance.

[XRay][tools] Fix an accounting bug in llvm-xray account
ClosedPublic

Authored by dberris on Apr 9 2017, 10:04 PM.

Details

Summary

Before this patch, llvm-xray account will assume that thread stacks will
not be empty. Unfortunately there are cases where an instrumented
function will see a call to fork() which will cause the child process
to not see the start of the function, but only see the end of the
function. The tooling cannot assume that threads will always have
perfect stacks, and so we change it to support this reality.

Event Timeline

dberris created this revision.Apr 9 2017, 10:04 PM
dblaikie added inline comments.Apr 10 2017, 11:13 AM
test/tools/llvm-xray/X86/account-empty-stack-error.yaml
15–16

Is the "keep going" functionality sufficiently tested? (Does this new error path present an interesting/new case of "keep going" that's worth testing separately - or is the functionality sufficiently generic that the existing testing covers enough here)

dberris added inline comments.Apr 10 2017, 8:40 PM
test/tools/llvm-xray/X86/account-empty-stack-error.yaml
15–16

We have a dedicated test that uses -k for account. This particular case isn't so interesting, because we're not attempting to recover or handle it (yet). In the future we may want to indicate in the accounting how many records we've discarded, how many we've deduced tail calls for, etc. -- but that's not so important at the moment than not crashing when we have this situation.

dblaikie edited edge metadata.May 9 2017, 10:24 PM

Please read the mailing list - Phab is not authoritative and doesn't always reflect replies/content on the mailing list.

dberris updated this revision to Diff 111919.Aug 20 2017, 5:54 PM

Update test with the keep-going path along with the non-keep-going test.

dberris marked 2 inline comments as done.Aug 20 2017, 5:55 PM
dberris added inline comments.
test/tools/llvm-xray/X86/account-empty-stack-error.yaml
15–16

I added a test for the keep-going path as well, making sure it works in this path as well.

dberris marked an inline comment as done.Aug 28 2017, 5:23 PM

ping @dblaikie -- I think I've addressed the question you had about the -keep-going mode, with an additional test.

dberris added subscribers: kpw, eizan.

Adding @kpw and @eizan for a couple more sets of eyes to see whether this makes a bit more sense.

dblaikie accepted this revision.Aug 30 2017, 11:31 AM

Looks fine to me

This revision is now accepted and ready to land.Aug 30 2017, 11:31 AM
eizan accepted this revision.Aug 30 2017, 5:15 PM
eizan added inline comments.
tools/llvm-xray/xray-account.cc
412

This struct doesn't appear to be used anywhere. Do we need it?

dberris marked an inline comment as done.Aug 30 2017, 5:57 PM
dberris added inline comments.
tools/llvm-xray/xray-account.cc
412

Ah, yes -- this is a specialization of a type used by the llvm::format(...) library for specifically the RecordTypes enum class.

This revision was automatically updated to reflect the committed changes.
dberris marked an inline comment as done.