This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt][XRay] re-submitting r276117, with fixes for build breakage due to extraneous and missing dependencies and attempts to build on unsupported OSes
ClosedPublic

Authored by dberris on Jul 20 2016, 10:22 PM.

Details

Summary

This is a fixed-up version of D21612, to address failure identified post-commit.

Original commit description:

This patch implements the initialisation and patching routines for the XRay runtime, along with the necessary trampolines for function entry/exit handling. For now we only define the basic hooks for allowing an implementation to define a handler that gets run on function entry/exit. We expose a minimal API for controlling the behaviour of the runtime (patching, cleanup, and setting the handler to invoke when instrumenting).

Fixes include:

  • Gating XRay build to only Linux x86_64 and with the right dependencies in case it is the only library being built
  • Including <cstddef> to fix std::size_t issue

Diff Detail

Repository
rL LLVM

Event Timeline

dberris updated this revision to Diff 64821.Jul 20 2016, 10:22 PM
dberris retitled this revision from to [compiler-rt][XRay] Re-roll r276117, with fixes..
dberris updated this object.
dberris added reviewers: kcc, rnk, echristo.
dberris added a subscriber: llvm-commits.

This was tested locally with the stand-alone (i.e. not part of full llvm) build. Only tested on Linux (that it builds).

echristo edited edge metadata.Jul 20 2016, 11:42 PM

can you update the description to be a full commit message? "recommitting <blah> with a fix for <blah>"?

dberris retitled this revision from [compiler-rt][XRay] Re-roll r276117, with fixes. to [compiler-rt][XRay] Re-roll r276117, with fixes for broken builds..Jul 20 2016, 11:45 PM
dberris updated this object.
dberris edited edge metadata.
dberris retitled this revision from [compiler-rt][XRay] Re-roll r276117, with fixes for broken builds. to [compiler-rt][XRay] re-submitting r276117, with fixes for build breakage due to extraneous and missing dependencies and attempts to build on unsupported OSes.
dberris updated this revision to Diff 64829.Jul 20 2016, 11:55 PM
  • Clang-format
dberris updated this object.Jul 21 2016, 12:07 AM

One inline comment, otherwise I don't see any problems.

-eric

lib/CMakeLists.txt
61–68 ↗(On Diff #64829)

This looks unnecessarily complicated?

dberris added inline comments.Jul 21 2016, 12:14 AM
lib/CMakeLists.txt
61–68 ↗(On Diff #64829)

Right. The other option was to just lump XRay into the sanitizers above. But that doesn't allow us to just build XRay -- or at least, XRay isn't strictly a sanitizer even.

If we want to allow building just XRay which depends on sanitizer_common for the flag processing then we need to guard that somehow.

This is the Nth iteration for this that I can convince myself is succinct enough to convey the intent. It's really that:

  • if you want to build xray
    • and you can also build sanitizer common
      • if you didn't build any of the other sanitizers
        • add dependency to build sanitizer_common
      • build xray

Other more invasive change involves building sanitizer_common if either we build the sanitizers or we build xray.

I can go with that approach, which might make this part of the stuff less complicated.

WDYT?

dberris updated this revision to Diff 64830.Jul 21 2016, 12:23 AM
  • Simpilfy logic for build dependencies for XRay and Sanitizers

Actually, that "more invasive" version turns out to be simpler to explain and document. :)

echristo accepted this revision.Jul 21 2016, 12:40 AM
echristo edited edge metadata.

OK with one comment.

Thanks!

-eric

lib/CMakeLists.txt
7 ↗(On Diff #64830)

This definitely makes more sense, can you add a TODO to factor out the command line handling?

This revision is now accepted and ready to land.Jul 21 2016, 12:40 AM
dberris updated this revision to Diff 64831.Jul 21 2016, 12:44 AM
dberris marked an inline comment as done.
dberris edited edge metadata.
  • Add TODO to refactor sanitizer_common into smaller pieces
This revision was automatically updated to reflect the committed changes.