This is an archive of the discontinued LLVM Phabricator instance.

[XRay][compiler-rt] Remove use of std::mutex and std::shared_ptr from global scope.
ClosedPublic

Authored by dberris on Jul 31 2017, 2:13 AM.

Details

Summary

This change attempts to remove all the dependencies we have on
std::mutex and any std::shared_ptr construction in global variables. We
instead use raw pointers to these objects, and construct them on the
heap. In cases where it's possible, we lazily initialize these pointers.

While we do not have a replacement for std::shared_ptr yet in
compiler-rt, we use this work-around to avoid having to statically
initialize the objects as globals. Subsequent changes should allow us to
completely remove our dependency on std::shared_ptr and instead have our
own implementation of the std::shared_ptr and std::weak_ptr semantics
(or completely rewrite the implementaton to not need these
standard-library provided abstractions).

Event Timeline

dberris created this revision.Jul 31 2017, 2:13 AM
dblaikie added inline comments.Jul 31 2017, 9:17 AM
lib/xray/xray_fdr_logging.cc
152

->

275

Could drop the nullptr here

lib/xray/xray_fdr_logging_impl.h
173

and here

471

Might be a good opportunity to change releaseThreadLocalBuffer take the BufferQueeu by mutable reference, since it's required to be nonnull anyway? Then this parameter would be '**LocalBQ' (which at least I treat with less suspicion than a function called 'release' that takes a raw pointer to data that's owned by a smart pointer... )

lib/xray/xray_inmemory_log.cc
46

Might be worth separating these two changes (the mutex from the shared-ptr) - they seem pretty independent & this one looks like it's good forward progress to move away from standard library dependencies as is required by compiler-rt, the other's more of a stop-gap (or at least neutral on the progress in that direction).

86–87

unnecessary reformatting? or did something change in this line that I can't quite spot.

dberris updated this revision to Diff 109043.Jul 31 2017, 7:17 PM
dberris marked 5 inline comments as done.

Address some comments.

lib/xray/xray_inmemory_log.cc
46

Thought about it, but for efficiency's sake, I suspect I'd like to make both changes as a group. They're both trying to achieve the same thing (no complex/atomic initialisation at init-time) and are working towards the same (partial) goal.

86–87

Looks like this wasn't formatted correctly before, and clang-format did the needful for me. :)

dberris updated this revision to Diff 109044.Jul 31 2017, 7:27 PM

Fix build break.

kpw edited edge metadata.Jul 31 2017, 10:13 PM

What's the primary motivation? Are we trying to avoid the upfront cost of global initialization? The unspecified destructor ordering with multithreaded programs?

In D36078#826962, @kpw wrote:

What's the primary motivation? Are we trying to avoid the upfront cost of global initialization? The unspecified destructor ordering with multithreaded programs?

The primary motivation is to avoid the initialization of atomics at initialization time. This has caused some issues in the past with dynamic linking and relocations.

It also allows us to start removing the dependencies on the C++ standard library in the implementation. Using raw pointers gets us there, and not using std::mutex gets us there closer.

kpw accepted this revision.Aug 1 2017, 10:04 AM

Thanks for the clarification of intent Dean. LGTM, but I'd like to clear up one source of confusion with name collisions.

lib/xray/xray_fdr_logging_impl.h
172–174

Not a big deal, but earlier you declare "shared_ptr<T>* name" and here you have "shared_ptr<T> *name".

Not sure which is preferred in LLVM.

497

Here LocalBQ is a shared_ptr& that shadows the thread local shared_ptr* of the same name. The shadowing is not new, but the types used to match.

I find this confusing. Can we rename the param LocalBQ to ParamBQ? Although I don't think dblakie's suggestion to pass \*\*LocalBQ from the call sites applies here too because \*\*LocalBQ is allowed to be nullptr if RecordPtr == nullptr or xray status is XRAY_LOG_INITIALIZING.

508

Changing .get()s to *s for the smart ptr parameter is unnecessary and contributes to the confusion caused by the shadowing.

This revision is now accepted and ready to land.Aug 1 2017, 10:04 AM
dberris updated this revision to Diff 109270.Aug 1 2017, 9:34 PM
dberris marked 3 inline comments as done.
  • fixup: avoid shadowing global variable

Thanks @kpw!

lib/xray/xray_fdr_logging_impl.h
172–174

The former is LLVM preferred. Somehow my local formatter keeps ignoring the LLVM style. :(

497

Good point. Renamed to LBQ instead in this function.

This revision was automatically updated to reflect the committed changes.