This is an archive of the discontinued LLVM Phabricator instance.

Differential D35789

[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

Reviewers
kpw
pelikan
dblaikie
Commits
rG5ca19556270f: [XRay][compiler-rt] Do not print the warning when the binary is not XRay…
rCRT309534: [XRay][compiler-rt] Do not print the warning when the binary is not XRay…
rL309534: [XRay][compiler-rt] Do not print the warning when the binary is not XRay…
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.

Diff Detail

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
Herald added a subscriber: mgorny. · View Herald TranscriptJul 26 2017, 6:56 PM
Harbormaster completed remote builds in B8632: Diff 108402.Jul 26 2017, 6:56 PM
dberris added a subscriber: dblaikie.Jul 26 2017, 6:59 PM

@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 a comment.Jul 26 2017, 8:30 PM
In D35789#822323, @dberris wrote:

@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.

dberris added a comment.Jul 27 2017, 4:41 AM
In D35789#822349, @dblaikie wrote:
In D35789#822323, @dberris wrote:

@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 added a comment.Jul 28 2017, 12:21 AM

PTAL @dblaikie ?

Harbormaster completed remote builds in B8675: Diff 108591.Jul 28 2017, 12:23 AM
dberris updated this revision to Diff 108594.Jul 28 2017, 12:26 AM
  • fixup: use Verbosity() instead
Harbormaster completed remote builds in B8676: Diff 108594.Jul 28 2017, 12:26 AM
dblaikie added a comment.Jul 29 2017, 10:35 AM

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)

dblaikie added a comment.Jul 30 2017, 6:39 PM

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.

Closed by commit rL309534: [XRay][compiler-rt] Do not print the warning when the binary is not XRay… (authored by dberris). · Explain WhyJul 30 2017, 10:17 PM
This revision was automatically updated to reflect the committed changes.
dberris marked an inline comment as done.
dberris mentioned this in rL309538: [XRay][compiler-rt] Require build-in-tree and x86_64-linux..Jul 30 2017, 11:10 PM
dberris mentioned this in rL309540: [XRay][compiler-rt] Fix typo for REQUIRES..Jul 30 2017, 11:22 PM
dberris mentioned this in rL309543: [XRay][compiler-rt] Fix test to not be too strict with output order..Jul 30 2017, 11:58 PM

Revision Contents

PathSize
lib/
xray/
3 lines
test/
xray/
TestCases/
Linux/
19 lines
5 lines

Diff 108855

lib/xray/xray_init.cc

Show First 20 Lines • Show All 43 Lines • ▼ Show 20 Lines
__sanitizer::SpinMutex XRayInstrMapMutex; __sanitizer::SpinMutex XRayInstrMapMutex;
XRaySledMap XRayInstrMap; XRaySledMap XRayInstrMap;
// __xray_init() will do the actual loading of the current process' memory map // __xray_init() will do the actual loading of the current process' memory map
// and then proceed to look for the .xray_instr_map section/segment. // and then proceed to look for the .xray_instr_map section/segment.
void __xray_init() XRAY_NEVER_INSTRUMENT { void __xray_init() XRAY_NEVER_INSTRUMENT {
initializeFlags(); initializeFlags();
if (__start_xray_instr_map == nullptr) { if (__start_xray_instr_map == nullptr) {
if (Verbosity())
Report("XRay instrumentation map missing. Not initializing XRay.\n"); Report("XRay instrumentation map missing. Not initializing XRay.\n");
return; return;
} }
{ {
__sanitizer::SpinMutexLock Guard(&XRayInstrMapMutex); __sanitizer::SpinMutexLock Guard(&XRayInstrMapMutex);
XRayInstrMap.Sleds = __start_xray_instr_map; XRayInstrMap.Sleds = __start_xray_instr_map;
XRayInstrMap.Entries = __stop_xray_instr_map - __start_xray_instr_map; XRayInstrMap.Entries = __stop_xray_instr_map - __start_xray_instr_map;
XRayInstrMap.SledsIndex = __start_xray_fn_idx; XRayInstrMap.SledsIndex = __start_xray_fn_idx;
Show All 11 Lines

test/xray/TestCases/Linux/quiet-start.cc

  • This file was added.
// Ensure that we have a quiet startup when we don't have the XRay
// instrumentation sleds.
//
// RUN: %clangxx -std=c++11 %s -o %t %xraylib
// RUN: XRAY_OPTIONS="patch_premain=true verbosity=1" %run %t 2>&1 | \
// RUN: FileCheck %s --check-prefix NOISY
// RUN: XRAY_OPTIONS="patch_premain=true verbosity=0" %run %t 2>&1 | \
// RUN: FileCheck %s --check-prefix QUIET
dblaikieUnsubmitted
Done
ReplyInline Actions

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))

dblaikie: Does this test the default (the comment says it checks what happens when there are no XRay…
dberrisAuthorUnsubmitted
Not Done
ReplyInline Actions

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

dberris: Added a test that will use an empty XRAY_OPTIONS environment variable, so that we're testing…
#include <iostream>
using namespace std;
int main(int, char**) {
// NOISY: {{.*}}XRay instrumentation map missing. Not initializing XRay.
// QUIET-NOT: {{.*}}XRay instrumentation map missing. Not initializing XRay.
cout << "Hello, XRay!" << endl;
// NOISY-NEXT: Hello, XRay!
// QUIET: Hello, XRay!
}

test/xray/lit.cfg

Show All 25 Linesconfig.substitutions.append(
('%clangxx ', ('%clangxx ',
build_invocation(config.cxx_mode_flags + [config.target_cflags]))) build_invocation(config.cxx_mode_flags + [config.target_cflags])))
config.substitutions.append( config.substitutions.append(
('%clang_xray ', build_invocation(clang_xray_cflags))) ('%clang_xray ', build_invocation(clang_xray_cflags)))
config.substitutions.append( config.substitutions.append(
('%clangxx_xray', build_invocation(clang_xray_cxxflags))) ('%clangxx_xray', build_invocation(clang_xray_cxxflags)))
config.substitutions.append( config.substitutions.append(
('%llvm_xray', llvm_xray)) ('%llvm_xray', llvm_xray))
config.substitutions.append(
('%xraylib',
('-lm -lpthread -ldl -lrt -L%s '
'-Wl,-whole-archive -lclang_rt.xray-%s -Wl,-no-whole-archive')
% (config.compiler_rt_libdir, config.host_arch)))
# Default test suffixes. # Default test suffixes.
config.suffixes = ['.c', '.cc', '.cpp'] config.suffixes = ['.c', '.cc', '.cpp']
if config.host_os not in ['Linux']: if config.host_os not in ['Linux']:
config.unsupported = True config.unsupported = True
elif '64' not in config.host_arch: elif '64' not in config.host_arch:
if 'arm' in config.host_arch: if 'arm' in config.host_arch:
Show All 9 Lines