Relationship between FEATURE_EVENT_TRACE and FEATURE_EVENTSOURCE_XPLAT

[am11]

There are three related (but maybe not related?) macros:

• FEATURE_EVENT_TRACE
• FEATURE_PERFTRACING
• FEATURE_EVENTSOURCE_XPLAT

Is there a situation where we want them to be mutually exclusive? If not, we can use just FEATURE_EVENT_TRACE at least in the native build (starting with clrdefinitions), in order to align with cross toolchains setup, e.g. https://github.com/dotnet/arcade/blob/89806f0b9e93ad2bbe32c654412835c0801a2032/eng/common/cross/toolchain.cmake#L82.

[am11]

cc @sywhang, @josalem

It has broken again for all non-Linux and non-OSX platforms. The build does not fail (since we have no benign build-time validation check for it), and it causes a cryptic missing ecall assertion failur on run-time (id != 0 without the method name) with debug artifacts, and sigsegv with release ones.

I have a fix for it, but I wanted to confirm if we really want to carry three macros for some reason.

[josalem]

Can you direct me to one of these failures? I haven't seen anything breaking in CI. I don't believe any of these are mutually exclusive, but I would need to check EventSource which has some features ifdef'd out since it ships out of band for old frameworks. I'm not opposed to merging these into one feature flag.

CC @noahfalk

[am11]

@josalem, the CI does not break because we don't run unit tests on any Unix-like OS beyond Linux and macOS.

runtime/src/coreclr/clrdefinitions.cmake

Lines 123 to 125 in 52262b8

	if(CLR_CMAKE_TARGET_LINUX)
	add_definitions(-DFEATURE_EVENTSOURCE_XPLAT)
	endif(CLR_CMAKE_TARGET_LINUX)

and

runtime/src/coreclr/clr.featuredefines.props

Line 17 in 52262b8

<FeatureXplatEventSource Condition="'$(TargetOS)'!='OSX'">true</FeatureXplatEventSource>

these two conditions cover the officially supported Linux and macOS. For the rest, it builds successfully without those ecalls in native binary, but allowing managed assemblies to still call these methods. On run-time as soon as application will call System.Diagnostics.Tracing.XplatEventLogger::IsEventSourceLoggingEnabled, this assertion fails:

runtime/src/coreclr/vm/ecall.cpp

Line 662 in 57bfe47

_ASSERTE(id != 0);

It would nicely scale if we just control it with FEATURE_EVENT_TRACE.

[josalem]

I think I'm grokking what your issue is.

Let me amend my previous statement as I spoke too soon. No, we can't merge all 3. The FEATURE_EVENTSOURCE_XPLAT is a Linux specific feature flag that enables LTTng collection of managed events.

I need to review whether we could merge FEATURE_PERFTRACING and FEATURE_EVENT_TRACE. PERFTRACING maps roughly to the EventPipe feature while EVENT_TRACE wraps most other eventing related features.

Does your patch fix your issue without merging these feature flags? Can you point me to the faulting locations? It's possible we missed some ifdefs that should be included.

[am11]

Right, the reason we have FEATURE_EVENT_TRACE on Android disabled is because lttng-ust package is missing on that platform (termux didn't had a package for it). However, lttng-ust's readme suggests it supports variety of platforms beyond Linux.

If both FEATURE_EVENTSOURCE_XPLAT and FEATURE_EVENT_TRACE are tied to the availability of (or lack thereof) lttng-ust (to select appropriate provider, lttng based or the dummy one), can FEATURE_EVENTSOURCE_XPLAT depend on FEATURE_EVENT_TRACE?

In case these flags cannot depend on eachother, then the the patch which fixes it for me is to simply align how we have defined the xplat flag in clrdefinitions.cmake:

--- a/src/coreclr/clr.featuredefines.props
+++ b/src/coreclr/clr.featuredefines.props
@@ -14,7 +14,7 @@
     </PropertyGroup>
 
     <PropertyGroup Condition="'$(TargetsUnix)' == 'true'">
-        <FeatureXplatEventSource Condition="'$(TargetOS)'!='OSX'">true</FeatureXplatEventSource>
+        <FeatureXplatEventSource Condition="'$(TargetOS)' == 'Linux'">true</FeatureXplatEventSource>
 
         <FeatureArrayStubAsIL>true</FeatureArrayStubAsIL>
         <FeatureMulticastStubAsIL>true</FeatureMulticastStubAsIL>

I was testing it on OpenIndiana (SunOS-like):

$ cat /etc/*release
             OpenIndiana Hipster 2021.04 (powered by illumos)
        OpenIndiana Project, part of The Illumos Foundation (C) 2010-2020
                        Use is subject to license terms.
                           Assembled 30 April 2021

$ COMPlus_ConsistencyCheck=1 /dotnet-6.0-debug/dotnet app.dll

CheckExtended: Unable to find internalcall implementation: System.Diagnostics.Tracing.XplatEventLogger::IsEventSourceLoggingEnabled
CheckExtended: Unable to find internalcall implementation: System.Diagnostics.Tracing.XplatEventLogger::LogEventSource
CheckCoreLibExtended: Unused ecall found: System.ArgIterator::.ctor
CheckCoreLibExtended: Unused ecall found: System.ArgIterator::.ctor
CheckCoreLibExtended: Unused ecall found: System.ArgIterator::GetRemainingCount
CheckCoreLibExtended: Unused ecall found: System.ArgIterator::_GetNextArgType
CheckCoreLibExtended: Unused ecall found: System.ArgIterator::FCallGetNextArg
CheckCoreLibExtended: Unused ecall found: System.ArgIterator::InternalGetNextArg
CheckCoreLibExtended: Unused type found: System.ArgIterator
CheckCoreLibExtended: Unused ecall found: System.CLRConfig::GetConfigBoolValue
CheckCoreLibExtended: Unused type found: System.CLRConfig
CheckCoreLibExtended: Unused ecall found: System.GC::GetMemoryLoad
CheckCoreLibExtended: Unused ecall found: .Kernel32::OpenEvent
CheckCoreLibExtended: Unused ecall found: .Kernel32::OpenSemaphore
CheckCoreLibExtended: Unused ecall found: .Kernel32::OutputDebugString
CheckCoreLibExtended: Unused ecall found: System.Runtime.InteropServices.Marshal::IsBuiltInComSupportedInternal
CheckCoreLibExtended: Unused ecall found: System.Math::FMod
CheckCoreLibExtended: Unused ecall found: System.MathF::FMod
CheckCoreLibExtended: Unused ecall found: System.Type::GetTypeFromHandleUnsafe
CheckExtended: completed without exception.

Assert failure(PID 794 [0x0000031a], Thread: 1 [0x0001]): id != 0
    File: /runtime/src/coreclr/vm/ecall.cpp Line: 662
    Image: /dotnet-6.0-debug/dotnet

I applied this patch:

diff --git a/src/coreclr/vm/ecall.cpp b/src/coreclr/vm/ecall.cpp
index c6d1b6d2f1e..3879a98410c 100644
--- a/src/coreclr/vm/ecall.cpp
+++ b/src/coreclr/vm/ecall.cpp
@@ -659,6 +659,13 @@ LPVOID ECall::GetQCallImpl(MethodDesc * pMD)
     if (id == 0)
     {
         id = ECall::GetIDForMethod(pMD);
+
+#ifdef _DEBUG
+    CONSISTENCY_CHECK_MSGF(id != 0,
+        ("%s::%s is not registered in ecall.cpp",
+        pMD->m_pszDebugClassName, pMD->m_pszDebugMethodName));
+#endif
+
         _ASSERTE(id != 0);
 
         // Cache the id

to find the name of offending ecall:

Assert failure(PID 904 [0x00000388], Thread: 1 [0x0001]): Consistency check failed: System.Diagnostics.Tracing.XplatEventLogger::IsEventSourceLoggingEnabled is not registered in ecall.cppFAILED: id != 0
    File: /runtime/src/coreclr/vm/ecall.cpp Line: 666
    Image: /dotnet-6.0-debug/dotnet

[josalem]

That patch seems reasonable if it unblocks your work. Feel free to open the PR and I can review it 😄

[noahfalk]

@josalem - a slight sidebar but I do think it would be helpful if we renamed these defines to clarify what their scope is supposed to be. I'm not sure if we've used them correctly everywhere, but in my ideal world the defines might look like this:

FEATURE_EVENT_TRACE // enabled everywhere
FEATURE_EVENT_TRACE_ETW // enabled on windows
FEATURE_EVENT_TRACE_EVENT_PIPE // probably enabled everywhere?
FEATURE_EVENT_TRACE_LTTNG // enabled on Linux

[josalem]

100% agree, Noah. The names organically grew to contain features with different names. We should match them all up.

[am11]

Agree with @noahfalk too.

FEATURE_EVENT_TRACE_LTTNG

On $other platforms where we have FEATURE_EVENT_TRACE_LTTNG=false, can we still use dummy provider (which is anyway generated by our python scripts IIRC)? If the answer is yes, then do we still need to define the constant in src/coreclr/clr.featuredefines.props and restrict managed code to abide by it? i.e. all platforms will surely have one of three providers: ETW, LTTNG or Dummy.

// enabled on Linux

lttng-ust package is also available on FreeBSD.