Fix build with disabled event trace/perftracing#130305
Conversation
dotnet-policy-service
Bot
added
the
community-contribution
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
| #endif // FEATURE_EVENT_TRACE | ||
|
|
||
| FireEtwGCSuspendEEBegin_V1(Info.SuspendEE.Reason, Info.SuspendEE.GcCount, GetClrInstanceId()); | ||
| #endif // FEATURE_EVENT_TRACE |
There was a problem hiding this comment.
That's actually line which catch my attention. Other changes is from attempt to build things with disabled features
There was a problem hiding this comment.
This should not actually be needed. There's another FireEtw 6 lines below that compiles just fine. I think these are #defined to nothing when FEATURE_EVENT_TRACE is not set.
| }; | ||
| }; | ||
|
|
||
| #ifndef FEATURE_EVENT_TRACE |
There was a problem hiding this comment.
This is also interesting thing. This looks like implemented in the disabledeventtrace.cpp or if I guess incorrectly, then it should be removed from both implementations eventtrace.cpp and disabledeventtrace.cpp
There was a problem hiding this comment.
Yep, this looks unnecessary. The inline prevents this from being a duplicate symbol error, but looks duplicated.
There was a problem hiding this comment.
actually either by order of inclusion, or by something else, inline does not prevent it from being build error for me. that's how I discover this.
MichalStrehovsky
left a comment
MichalStrehovsky
left a comment
There was a problem hiding this comment.
There are build breaks.
| } | ||
|
|
||
| void ETW::GCLog::FireGcStart(ETW_GC_INFO * pGcInfo) { } | ||
| #if FEATURE_EVENT_TRACE |
There was a problem hiding this comment.
Why is this one different?
There was a problem hiding this comment.
That's from
set(FEATURE_PERFTRACING 1)
set(FEATURE_EVENT_TRACE 0)
configuration probably, so given that we don't care about it. I remove it.
There was a problem hiding this comment.
PERFTRACING is basically event pipe. EVENT_TRACE is ETW. Changes here are "allowing" to build PERFTRACING=1 EVENT_TRACE=0, but not really because we're not actually building event pipe.
I would undo the changes under /eventpipe/ and declare PERFTRACING=1 EVENT_TRACE=0 non-sensical.
| void EventPipe_Shutdown(); | ||
| bool DiagnosticServer_Shutdown(); | ||
|
|
||
| #ifdef FEATURE_EVENT_TRACE |
There was a problem hiding this comment.
I would undo this one too for the same reason as /eventpipe/
There was a problem hiding this comment.
Same from
set(FEATURE_PERFTRACING 1)
set(FEATURE_EVENT_TRACE 0)
Removed.
| static HRESULT ForceGCForDiagnostics(); | ||
| static void ForceGC(LONGLONG l64ClientSequenceNumber); | ||
| static void FireGcStart(ETW_GC_INFO * pGcInfo); | ||
| #if defined(FEATURE_EVENT_TRACE) |
| }; | ||
| }; | ||
|
|
||
| #ifndef FEATURE_EVENT_TRACE |
There was a problem hiding this comment.
Yep, this looks unnecessary. The inline prevents this from being a duplicate symbol error, but looks duplicated.
| #endif // FEATURE_EVENT_TRACE | ||
|
|
||
| FireEtwGCSuspendEEBegin_V1(Info.SuspendEE.Reason, Info.SuspendEE.GcCount, GetClrInstanceId()); | ||
| #endif // FEATURE_EVENT_TRACE |
There was a problem hiding this comment.
This should not actually be needed. There's another FireEtw 6 lines below that compiles just fine. I think these are #defined to nothing when FEATURE_EVENT_TRACE is not set.
2 participants
That's changes only for NativeAOT and I did test only these configuration
Also disable EventSourceSupport in the AotCompilerCommon.props