diff --git a/profiler/src/Demos/Samples.Computer01/GlobalStruct.cs b/profiler/src/Demos/Samples.Computer01/GlobalStruct.cs new file mode 100644 index 000000000000..62dcb6c1ba7f --- /dev/null +++ b/profiler/src/Demos/Samples.Computer01/GlobalStruct.cs @@ -0,0 +1,17 @@ +// +// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2022 Datadog, Inc. +// + +#pragma warning disable SA1401 // Fields should be private + + +// This value type is deliberately declared outside of any namespace: when it is given as the type +// argument of a generic method, the |fg: part of the frame must not be prefixed by a '.' standing +// for the empty namespace. +internal struct GlobalStruct +{ + public int Member; +} + +#pragma warning restore SA1401 // Fields should be private diff --git a/profiler/src/Demos/Samples.Computer01/MethodsSignature.cs b/profiler/src/Demos/Samples.Computer01/MethodsSignature.cs index 2c5b088bf773..e221780bb434 100644 --- a/profiler/src/Demos/Samples.Computer01/MethodsSignature.cs +++ b/profiler/src/Demos/Samples.Computer01/MethodsSignature.cs @@ -7,6 +7,7 @@ using System.Collections.Generic; using System.Runtime.CompilerServices; using System.Threading; +using System.Threading.Tasks; #pragma warning disable SA1402 // File may only contain a single type #pragma warning disable SA1401 // Fields should be private @@ -22,7 +23,58 @@ public override void OnProcess() // because at least one of each type will be sampled. // Allocations beyond 100 KB would be fine except that we could not test // the .NET Framework runtime - TriggerExceptions(); + WithResult().GetAwaiter().GetResult(); + } + + // Same shape as BuggyBits ProductsController.WithResult() / GetSyncOverAsync(): the exception + // is thrown from the synchronous segment of an async method, so the compiler-generated state + // machines and AsyncMethodBuilderCore.Start are on the stack when it is + // captured. Each state machine is a type nested in MethodsSignature: its name must keep both + // the namespace and the enclosing type. + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)] + private async Task WithResult() + { + return await GetSyncOverAsync(); + } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)] + private async Task GetSyncOverAsync() + { + // nothing is awaited before this call: the whole chain below runs in the synchronous + // segment of the state machine, i.e. under AsyncMethodBuilderCore.Start<...> + RunStateMachine(); + + await Task.Yield(); + return 42; + } + + // The state machines generated by the compiler for the two methods above are structs in release + // builds but classes in debug builds. In the latter case, the shared canonical instantiation of + // Start is used, so the exact state machine type is not available to the profiler. + // Driving our own state machine, always a struct, gives the same frames in both configurations. + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)] + private void RunStateMachine() + { + var builder = AsyncTaskMethodBuilder.Create(); + var stateMachine = new SyncOverAsyncStateMachine { Owner = this }; + + // no need to complete the builder: nothing awaits its task and the exceptions thrown by + // the chain below are caught before they could escape MoveNext() + builder.Start(ref stateMachine); + } + + private struct SyncOverAsyncStateMachine : IAsyncStateMachine + { + public MethodsSignature Owner; + + public void MoveNext() + { + Owner.TriggerExceptions(); + } + + public void SetStateMachine(IAsyncStateMachine stateMachine) + { + } } // call methods with different signatures @@ -132,6 +184,13 @@ private void ThrowGenericMethod1(T element) } else if (element is MyStruct) + { + GlobalStruct gs; + gs.Member = 42; + ThrowGenericMethod1(gs); + } + else + if (element is GlobalStruct) { ThrowGenericMethod1("this"); } diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/FrameStore.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/FrameStore.cpp index 3069e10e55cd..0e90a7bbb84a 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/FrameStore.cpp +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/FrameStore.cpp @@ -609,12 +609,13 @@ std::tuple FrameStore::GetMethodName } else // normal namespace.type case { + // a type declared outside of any namespace has no namespace: don't prefix it with a '.' if (!ns.empty()) { - builder << ns; + builder << ns << "."; } - builder << "." << typeName; + builder << typeName; } if (i < genericParametersCount - 1) @@ -1117,30 +1118,17 @@ std::pair FrameStore::GetManagedTypeName(ICorProfilerI return std::make_pair("", "T"); } - IMetaDataImport2* pMetadata; - hr = pInfo->GetModuleMetaData(moduleId, ofRead, IID_IMetaDataImport2, reinterpret_cast(&pMetadata)); + ComPtr pMetadata; + hr = pInfo->GetModuleMetaData(moduleId, ofRead, IID_IMetaDataImport2, reinterpret_cast(pMetadata.GetAddressOf())); if (FAILED(hr)) { return std::make_pair("", "T"); } - std::string typeName = GetTypeNameFromMetadata(pMetadata, mdTypeToken); - pMetadata->Release(); - if (typeName.empty()) - { - return std::make_pair("", "T"); - } - - // look for the namespace - auto const pos = typeName.find_last_of('.'); - if (pos == std::string::npos) - { - // no namespace - return std::make_pair("", std::move(typeName)); - } - - // need to split to get the namespace and type name - return std::make_pair(typeName.substr(0, pos), typeName.substr(pos + 1)); + // the namespace and the enclosing types are not part of the metadata name of a nested type + // (such as the state machine generated for an async method): GetTypeWithNamespace() rebuilds + // them and, for a type that is not nested, splits the namespace from the type name + return GetTypeWithNamespace(pMetadata.Get(), mdTypeToken); } // use Peter Sollich way in ClrProfiler to parse the binary signature diff --git a/profiler/test/Datadog.Profiler.IntegrationTests/Helpers/EnvironmentHelper.cs b/profiler/test/Datadog.Profiler.IntegrationTests/Helpers/EnvironmentHelper.cs index a5086f3dc822..cde9fb5fa756 100644 --- a/profiler/test/Datadog.Profiler.IntegrationTests/Helpers/EnvironmentHelper.cs +++ b/profiler/test/Datadog.Profiler.IntegrationTests/Helpers/EnvironmentHelper.cs @@ -371,7 +371,7 @@ private string GetNativeLoaderPath() if (!IsRunningInCi()) { // native loader output folder - var binFolder = Path.Combine(GetSolutionDirectory(), "native-bin", "Datadog.Trace.ClrProfiler.Native", "bin"); + var binFolder = Path.Combine(GetSolutionDirectory(), "artifacts", "native-bin", "Datadog.Trace.ClrProfiler.Native"); return GetOS() switch { diff --git a/profiler/test/Datadog.Profiler.IntegrationTests/Helpers/StackFrame.cs b/profiler/test/Datadog.Profiler.IntegrationTests/Helpers/StackFrame.cs index e4a19beca5d6..4fc798285e2a 100644 --- a/profiler/test/Datadog.Profiler.IntegrationTests/Helpers/StackFrame.cs +++ b/profiler/test/Datadog.Profiler.IntegrationTests/Helpers/StackFrame.cs @@ -28,7 +28,7 @@ public StackFrame(string rawStackFrame) public StackFrame(string rawStackFrame, string filename, long line) { // |lm:Datadog.Demos.ExceptionGenerator |ns:ExceptionGenerator |ct:ExceptionsProfilerTestScenario |fn:Throw1_2 - var match = Regex.Match(rawStackFrame, @"^\|lm:(?.*) \|ns:(?.*) \|ct:(?.*) \|cg:(?.*) \|fn:(?.*) \|fg:(?.*) \|sg:(?.*)$"); + var match = Regex.Match(rawStackFrame, @"^\|lm:(?.*) \|ns:(?.*) \|ct:(?.*) \|cg:(?.*) \|fn:(?.*) \|fg:(?.*) \|sg:(?.*)$"); if (!match.Success) { diff --git a/profiler/test/Datadog.Profiler.IntegrationTests/Signature/SignatureTest.cs b/profiler/test/Datadog.Profiler.IntegrationTests/Signature/SignatureTest.cs index a0eac153963f..d49a9635c34f 100644 --- a/profiler/test/Datadog.Profiler.IntegrationTests/Signature/SignatureTest.cs +++ b/profiler/test/Datadog.Profiler.IntegrationTests/Signature/SignatureTest.cs @@ -23,6 +23,17 @@ public SignatureTest(ITestOutputHelper output) [TestAppFact("Samples.Computer01", new[] { "net48", "netcoreapp3.1", "net6.0", "net8.0", })] // FIXME: .NET 9 skipping .NET 9 for now public void ValidateSignatures(string appName, string framework, string appAssembly) + { + CheckExceptionsInProfiles(framework, GetExceptionSamples(appName, framework, appAssembly)); + } + + [TestAppFact("Samples.Computer01", new[] { "net48", "netcoreapp3.1", "net6.0", "net8.0", })] // FIXME: .NET 9 skipping .NET 9 for now + public void ValidateAsyncStateMachineSignatures(string appName, string framework, string appAssembly) + { + CheckAsyncStateMachineFrames(GetExceptionSamples(appName, framework, appAssembly)); + } + + private (string Type, string Message, long Count, StackTrace Stacktrace)[] GetExceptionSamples(string appName, string framework, string appAssembly) { var runner = new TestApplicationRunner(appName, framework, appAssembly, _output, commandLine: "--scenario 20"); EnvironmentHelper.DisableDefaultProfilers(runner); @@ -33,8 +44,47 @@ public void ValidateSignatures(string appName, string framework, string appAssem runner.Run(agent); Assert.True(agent.NbCallsOnProfilingEndpoint > 0); - var exceptionSamples = SamplesHelper.ExtractExceptionSamples(runner.Environment.PprofDir).ToArray(); - CheckExceptionsInProfiles(framework, exceptionSamples); + return SamplesHelper.ExtractExceptionSamples(runner.Environment.PprofDir).ToArray(); + } + + private static void CheckAsyncStateMachineFrames((string Type, string Message, long Count, StackTrace Stacktrace)[] exceptionSamples) + { + // A state machine is a type nested in the type declaring the async method: the generic + // parameter of Start must carry its namespace and its enclosing type, exactly + // like the signature already does. The frame is AsyncMethodBuilderCore.Start on .NET Core + // and AsyncTaskMethodBuilder.Start on .NET Framework. + var frames = exceptionSamples + .SelectMany(sample => Enumerable.Range(0, sample.Stacktrace.FramesCount).Select(i => sample.Stacktrace[i])) + .Distinct() + .ToArray(); + + // split code to debug more easily outside of LINQ + var startFrames = frames + .Where(frame => frame.Function == "Start") + .Distinct() + .ToArray(); + + var startAdornments = startFrames + .Select(frame => frame.FunctionAdornment) + .Distinct() + .ToArray(); + +#if DEBUG + // MethodsSignature drives its own state machine: a struct is never shared between + // instantiations so its exact type is always known, whatever the configuration used to + // build the sample + startAdornments.Should().Contain(""); + + // the state machines generated by the compiler are structs in release builds but classes in + // debug builds: in the latter case, only the shared canonical instantiation is known () + startAdornments.Should().Contain(""); +#else + startAdornments.Should().Contain("d__2>"); + startAdornments.Should().Contain("d__1>"); +#endif + + // regression test to check the "type with empty namespace" bug is fixed + frames.Should().OnlyContain(frame => !frame.FunctionAdornment.StartsWith("<.")); } private static void CheckExceptionsInProfiles(string framework, (string Type, string Message, long Count, StackTrace Stacktrace)[] exceptionSamples) @@ -56,6 +106,7 @@ private static void CheckExceptionsInProfiles(string framework, (string Type, st new StackFrame("|lm:Samples.Computer01 |ns:Samples.Computer01 |ct:GenericClassForValueTypeTest |cg: |fn:ThrowOneGenericFromType |fg: |sg:(TKey value)"), new StackFrame("|lm:Samples.Computer01 |ns:Samples.Computer01 |ct:MethodsSignature |cg: |fn:ThrowGenericMethod2 |fg: |sg:(T0 key1, System.Int32 value1, System.Int32 value2, T2 key2, T3 key3, System.Collections.Generic.List listOfTValue)"), new StackFrame("|lm:Samples.Computer01 |ns:Samples.Computer01 |ct:MethodsSignature |cg: |fn:ThrowGenericMethod1 |fg: |sg:(T0 element)"), + new StackFrame("|lm:Samples.Computer01 |ns:Samples.Computer01 |ct:MethodsSignature |cg: |fn:ThrowGenericMethod1 |fg: |sg:(GlobalStruct element)"), new StackFrame("|lm:Samples.Computer01 |ns:Samples.Computer01 |ct:MethodsSignature |cg: |fn:ThrowGenericMethod1 |fg: |sg:(Samples.Computer01.MyStruct element)"), new StackFrame("|lm:Samples.Computer01 |ns:Samples.Computer01 |ct:MethodsSignature |cg: |fn:ThrowGenericMethod1 |fg: |sg:(T0 element)"), new StackFrame("|lm:Samples.Computer01 |ns:Samples.Computer01 |ct:MethodsSignature |cg: |fn:ThrowGenericMethod1 |fg: |sg:(System.Boolean element)"), @@ -80,6 +131,7 @@ private static void CheckExceptionsInProfiles(string framework, (string Type, st new StackFrame("|lm:Samples.Computer01 |ns:Samples.Computer01 |ct:GenericClassForValueTypeTest |cg: |fn:ThrowOneGenericFromType |fg: |sg:(TVal value)"), new StackFrame("|lm:Samples.Computer01 |ns:Samples.Computer01 |ct:MethodsSignature |cg: |fn:ThrowGenericMethod2 |fg: |sg:(T0 key1, System.Int32 value1, System.Int32 value2, T2 key2, T3 key3, System.Collections.Generic.List listOfTValue)"), new StackFrame("|lm:Samples.Computer01 |ns:Samples.Computer01 |ct:MethodsSignature |cg: |fn:ThrowGenericMethod1 |fg: |sg:(T0 element)"), + new StackFrame("|lm:Samples.Computer01 |ns:Samples.Computer01 |ct:MethodsSignature |cg: |fn:ThrowGenericMethod1 |fg: |sg:(GlobalStruct element)"), new StackFrame("|lm:Samples.Computer01 |ns:Samples.Computer01 |ct:MethodsSignature |cg: |fn:ThrowGenericMethod1 |fg: |sg:(Samples.Computer01.MyStruct element)"), new StackFrame("|lm:Samples.Computer01 |ns:Samples.Computer01 |ct:MethodsSignature |cg: |fn:ThrowGenericMethod1 |fg: |sg:(T0 element)"), new StackFrame("|lm:Samples.Computer01 |ns:Samples.Computer01 |ct:MethodsSignature |cg: |fn:ThrowGenericMethod1 |fg: |sg:(System.Boolean element)"),