Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Inject startup hook preferentially into static constructor when available #6154

Merged
merged 2 commits into from
Oct 29, 2024
Merged

Inject startup hook preferentially into static constructor when available #6154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25f8eb4
Skip injecting entrypoint when have a static constructor
andrewlock Oct 15, 2024
6fbd4da
Add static constructor to manual instrumentation program
andrewlock Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions tracer/src/Datadog.Tracer.Native/cor_profiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1567,11 +1567,19 @@ HRESULT STDMETHODCALLTYPE CorProfiler::JITCompilationStarted(FunctionID function
// Note: This check must only run on desktop because it is possible (and the default) to host
// ASP.NET Core in-process, so a new .NET Core runtime is instantiated and run in the same w3wp.exe process
auto valid_startup_hook_callsite = true;
// In some cases we may choose to defer instrumenting a valid startup hook callsite.
// For example, if we're instrumenting the entrypoint, but the Program.Main() implementing type
// has a static constructor, then the JIT inserts a call to the static constructor at the start of the method.
// If we insert the startup hook at the start of the method, we'll miss the static constructor call. This is
// particularly problematic if there's any "one time setup" happening in that constructor, e.g. usages of
// Datadog.Trace.Manual instrumentation. This behaviour only occurs on .NET Core, so limit the behaviour to there.
auto can_skip_startup_hook_callsite = runtime_information_.is_core();
if (is_desktop_iis)
{
valid_startup_hook_callsite = module_metadata->assemblyName == WStr("System.Web") &&
caller.type.name == WStr("System.Web.Compilation.BuildManager") &&
caller.name == WStr("InvokePreStartInitMethods");
can_skip_startup_hook_callsite = false;
}
else if (module_metadata->assemblyName == WStr("System") ||
module_metadata->assemblyName == WStr("System.Net.Http") ||
Expand Down Expand Up @@ -1628,6 +1636,45 @@ HRESULT STDMETHODCALLTYPE CorProfiler::JITCompilationStarted(FunctionID function
return S_OK;
}

// *********************************************************************
// Checking if the caller has an explicit static constructor.
// If it does, we delay instrumenting this and let the static constructor get instrumented instead.
// Bypassing for calls that we explicitly want to instrument (e.g. IIS startup hook)
// *********************************************************************
if (can_skip_startup_hook_callsite && caller.name != WStr(".cctor"))
{
mdMethodDef memberDef;
hr = metadataImport->FindMethod(caller.type.id, WStr(".cctor"), 0, 0, &memberDef);
if (FAILED(hr))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember Kevin saying that FAILED and SUCEEDED macros are unreliable, should we do hr != S_OK then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All our existing usages of FindMethod() use Failed(hr), which is why I did here, but it's a good question, @kevingosse?
image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline

In the code it doesn't look like it can return S_FALSE
And there are other places where we use it with if (FAILED(hr)), so probably fine

{
Logger::Debug("JITCompilationStarted: No .cctor found for type ", caller.type.name);
}
else
{
// we found a static constructor, so now we need to work out if it's an explicit or implicit
// constructor, because we won't be able to inject into an implicit static constructor, so
// would inject too late
DWORD typeDefFlags;
hr = metadataImport->GetTypeDefProps(caller.type.id, nullptr, 0, nullptr, &typeDefFlags, nullptr);
if (FAILED(hr))
{
Logger::Debug("JITCompilationStarted: Error calling GetTypeDefProps for type ", caller.type.name,
", allowing injection into ", caller.name, "()");
}
else if(typeDefFlags & tdBeforeFieldInit)
{
Logger::Debug("JITCompilationStarted: Found .cctor for type ", caller.type.name,
" but allowing startup hook injection as tdBeforeFieldInit indicates an implicit static constructor");
}
else
{
Logger::Debug("JITCompilationStarted: Startup hook skipped from ", caller.type.name, ".",
caller.name, "() as found .cctor");
return S_OK;
}
}
}

// *********************************************************************

bool domain_neutral_assembly = runtime_information_.is_desktop() && corlib_module_loaded &&
Expand Down
Loading
Loading