This repository was archived by the owner on Jan 23, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Fix LTTng build for build environments with older liblttng-ust-dev #27273
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brianrob
reviewed
Oct 17, 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sywhang, this looks good for 3.0 and 3.1 (modulo comment below), but for 5.0, we really should upgrade and prereq a UST that supports tracepoint_enabled. Then we can remove the #ifndef
.
josalem
reviewed
Oct 17, 2019
I agree. Once I am finished with this and the backports, I will file an issue to keep track of this. |
noahfalk
reviewed
Oct 18, 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
sywhang
added a commit
to sywhang/coreclr
that referenced
this pull request
Oct 18, 2019
…otnet#27273) * Fix macro redefinition to use XplatEventLogger instead of simply writing FALSE * Fix linker error * undo newline changes * Some changes to comment * Move wrapper export from eventpipe.cpp to eventtrace.cpp
sywhang
added a commit
to sywhang/coreclr
that referenced
this pull request
Oct 18, 2019
…otnet#27273) * Fix macro redefinition to use XplatEventLogger instead of simply writing FALSE * Fix linker error * undo newline changes * Some changes to comment * Move wrapper export from eventpipe.cpp to eventtrace.cpp
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes https://github.com/dotnet/coreclr/issues/27024.
For all of our event sinks for LTTng, we wrap the call to fire events with
tracepoint_enabled
, which is a macro defined in a header file liblttng-ust-dev. This macro checks whether a particular probe point is enabled before we fire an event, so that we're not unnecessarily making the call to fire events through LTTng when nobody is listening to it.For environments that don't want to build using LTTng, we have a macro definition for
tracepoint_enabled
, which used to be defined as:#define tracepoint_enabled(provider, name) TRUE
.This meant that for custom CoreCLR builds that were produced without LTTng library enabled, the runtime will always try to fire all events only to find out nobody is listening. So this behavior was changed to
#define tracepoint_enabled(provider, name) FALSE
.Unfortunately this caused the compiler to optimize out the bodies for all the event sinks when such thing happens. This would be fine for custom builds that didn't want LTTng since they become no-ops. However, some build environments that we still use, we use old enough distributions that have old LTTng libraries, specifically CentOS 6 which uses liblttng-ust-dev version 2.4. This particular version of the library doesn't have the definition of
tracepoint_enabled
in its header, so we hit this bad behavior in builds produced in CentOS 6 container.Even more unfortunately, CentOS 6 is the distro we use for our official SDK build containers for Linux, so this caused LTTng support in CoreCLR to regress in the official builds.
This fix changes the macro definition of
tracepoint_enabled
to useXplatEventLogger::IsEventLoggingEnabled
method, which checks if the environment variable we require for using LTTng tracing (COMPlus_EnableEventLog
) is enabled.The long term fix for this would be to change the official build containers to build and install liblttng-ust-dev versions 2.5 or newer, but this is the short-term fix until that happens.
The fix will be ported to 3.0 and 3.1 once merged.
cc @tommcdon