This is an archive of the discontinued LLVM Phabricator instance.

[XRay][compiler-rt] Do not print the warning when the binary is not XRay instrumented.
ClosedPublic

Authored by dberris on Jul 24 2017, 1:50 AM.

Details

Summary

Currently when the XRay runtime is linked into a binary that doesn't
have the instrumentation map, we print a warning unconditionally. This
change attempts to make this behaviour more quiet by tying it to the
Verbosity setting.

We only print the error when Verbosity is set to anything other than 0.

The Verbosity setting is parsed by the sanitizer_common library, which defines
a common set of flags for the sanitizers. The default verbosity set by
sanitizer_common is 0. XRay already depends on this library and the flags
defined there.

Event Timeline

dberris created this revision.Jul 24 2017, 1:50 AM
pelikan accepted this revision.Jul 24 2017, 2:05 PM

With my "instrument my whole Gentoo box with XRay" hat on, I highly approve of this change.

This revision is now accepted and ready to land.Jul 24 2017, 2:05 PM
dberris updated this revision to Diff 108402.Jul 26 2017, 6:56 PM
  • Fixup: address comments
  • Fixup: required testing artefacts

@dblaikie -- Added the test, and in the process learned that we haven't been building/running the unit tests because of some configuration problems. I've added the required changes here too.

@kpw -- Good catch! Done now.

@dblaikie -- Added the test, and in the process learned that we haven't been building/running the unit tests because of some configuration problems. I've added the required changes here too.

I'm guessing a bunch of the changes in here are fixes for tests that were broken but never running? Probably best to separate out the "make the tests run (& make them pass)" from the rest of this and commit it ahead of the rest. In case the functionality added has issues, needs to be reverted or dealt with in other ways, etc.

@dblaikie -- Added the test, and in the process learned that we haven't been building/running the unit tests because of some configuration problems. I've added the required changes here too.

I'm guessing a bunch of the changes in here are fixes for tests that were broken but never running? Probably best to separate out the "make the tests run (& make them pass)" from the rest of this and commit it ahead of the rest. In case the functionality added has issues, needs to be reverted or dealt with in other ways, etc.

Yes, although it turns out that the unit tests were also printing the error I was fixing too. :/

I'll work a little bit more to see whether there's a clean way to introduce just the test for this change and the requisite changes to the lit configs.

Thanks!

dberris updated this revision to Diff 108591.Jul 28 2017, 12:20 AM
  • fixup: reduce to minimum viable change
dberris updated this revision to Diff 108594.Jul 28 2017, 12:26 AM
  • fixup: use Verbosity() instead

Testing looks OK - but the change doesn't seem to match the description. I'm guessing there's existing code to SetVerbosity depending on whether the binary is instrumented or not? & there are tests for that?

If so, then maybe this could use a different description that better matches the code change? (Don't print warning about missing instrumentation when verbosity is zero?)

dberris updated this revision to Diff 108848.Jul 30 2017, 6:27 PM
dberris edited the summary of this revision. (Show Details)

(reword)

Not sure the description captures how the change addresses the subject, does it? (perhaps I'm misreading/not following somehow - it says "tie it to the sanitizer verbosity setting", but doesn't explain why this addresses the "don't do it when it's not instrumented" goal?)

dberris updated this revision to Diff 108855.Jul 30 2017, 9:39 PM
dberris edited the summary of this revision. (Show Details)

(reword, say that the default verbosity is 0)

dblaikie accepted this revision.Jul 30 2017, 9:59 PM

Phrasing looks like it could still use some polish/consistency, but all good in any case.

test/xray/TestCases/Linux/quiet-start.cc
5–8

Does this test the default (the comment says it checks what happens when there are no XRay instrumentation sleds - but the code seems to explicitly specify verbosity, rather than relying on any default based on the presence/absence of sleds? (unless that would override the XRAY_OPTIONS?))

Seems like it might be better to phrase this whole change, etc in terms of tying it into the verbosity level & mention as an aside that this means it defaults to off. (& I don't think it's necessary to test that default if the compiler-rt verbosity work already handles the default, tests & documents it, etc))

dberris updated this revision to Diff 108856.Jul 30 2017, 10:06 PM
  • fixup: add a default case test
dberris marked an inline comment as done.Jul 30 2017, 10:07 PM
dberris added inline comments.
test/xray/TestCases/Linux/quiet-start.cc
5–8

Added a test that will use an empty XRAY_OPTIONS environment variable, so that we're testing all the defaults.

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