Skip to content
Merged
37 changes: 29 additions & 8 deletions tracer/src/Datadog.Tracer.Native/calltarget_tokens.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -358,9 +358,9 @@ mdMethodSpec CallTargetTokens::GetCallTargetDefaultValueMethodSpec(const TypeSig
}

HRESULT CallTargetTokens::ModifyLocalSig(ILRewriter* reWriter, TypeSignature* methodReturnValue,
std::vector<TypeSignature>* methodTypeArguments,
std::vector<TypeSignature>* methodTypeArguments, FunctionInfo* caller,
ULONG* callTargetStateIndex, ULONG* exceptionIndex,
ULONG* callTargetReturnIndex, ULONG* returnValueIndex,
ULONG* callTargetReturnIndex, ULONG* staticValueTypeIndex, ULONG* returnValueIndex,
mdToken* callTargetStateToken, mdToken* exceptionToken,
mdToken* callTargetReturnToken, std::vector<ULONG>& additionalLocalIndices, bool isAsyncMethod)
{
Expand Down Expand Up @@ -399,7 +399,12 @@ HRESULT CallTargetTokens::ModifyLocalSig(ILRewriter* reWriter, TypeSignature* me
}
constexpr int variableNumber = 3;
const auto additionalLocalsCount = GetAdditionalLocalsCount(*methodTypeArguments);
ULONG newLocalsCount = variableNumber + additionalLocalsCount;
bool isStaticValueType = !(caller->method_signature.CallingConvention() & IMAGE_CEE_CS_CALLCONV_HASTHIS) && caller->type.valueType;
ULONG newLocalsCount = variableNumber + additionalLocalsCount + (isStaticValueType ? 1 : 0);

// Gets the static value type buffer and size
unsigned staticValueTypeTypeRefBuffer;
auto staticValueTypeTypeRefSize = CorSigCompressToken(caller->type.id, &staticValueTypeTypeRefBuffer);

// Gets the calltarget state type buffer and size
unsigned callTargetStateTypeRefBuffer;
Expand Down Expand Up @@ -496,6 +501,13 @@ HRESULT CallTargetTokens::ModifyLocalSig(ILRewriter* reWriter, TypeSignature* me
newSignature.Append(ELEMENT_TYPE_VALUETYPE);
newSignature.Append(&callTargetReturnBuffer, callTargetReturnSize);
}

// Static method in a ValueType
if (isStaticValueType)
{
newSignature.Append(ELEMENT_TYPE_VALUETYPE);
newSignature.Append(&staticValueTypeTypeRefBuffer, staticValueTypeTypeRefSize);
}

// Add custom locals
AddAdditionalLocals(methodReturnValue, methodTypeArguments, newSignature, isAsyncMethod);
Expand All @@ -519,7 +531,7 @@ HRESULT CallTargetTokens::ModifyLocalSig(ILRewriter* reWriter, TypeSignature* me
*callTargetReturnToken = callTargetReturn;

const auto sizeOfOtherIndexes = additionalLocalIndices.size();
auto indexStart = variableNumber + additionalLocalsCount;
auto indexStart = variableNumber + additionalLocalsCount + (isStaticValueType ? 1 : 0);

if (returnSignatureType != nullptr)
{
Expand All @@ -533,6 +545,15 @@ HRESULT CallTargetTokens::ModifyLocalSig(ILRewriter* reWriter, TypeSignature* me
*exceptionIndex = newLocalsCount - indexStart--;
*callTargetReturnIndex = newLocalsCount - indexStart--;

if (isStaticValueType)
{
*staticValueTypeIndex = newLocalsCount - indexStart--;
}
else
{
*staticValueTypeIndex = static_cast<ULONG>(ULONG_MAX);
}

for (unsigned int i = 0; i < sizeOfOtherIndexes; i++)
{
additionalLocalIndices[i] = newLocalsCount - indexStart--;
Expand Down Expand Up @@ -870,9 +891,9 @@ mdAssemblyRef CallTargetTokens::GetCorLibAssemblyRef()
}

HRESULT CallTargetTokens::ModifyLocalSigAndInitialize(void* rewriterWrapperPtr, TypeSignature* methodReturnType,
std::vector<TypeSignature>* methodTypeArguments,
std::vector<TypeSignature>* methodTypeArguments, FunctionInfo* caller,
ULONG* callTargetStateIndex, ULONG* exceptionIndex,
ULONG* callTargetReturnIndex, ULONG* returnValueIndex,
ULONG* callTargetReturnIndex, ULONG* staticValueTypeIndex, ULONG* returnValueIndex,
mdToken* callTargetStateToken, mdToken* exceptionToken,
mdToken* callTargetReturnToken, ILInstr** firstInstruction,
std::vector<ULONG>& additionalLocalIndices, bool isAsyncMethod)
Expand All @@ -881,8 +902,8 @@ HRESULT CallTargetTokens::ModifyLocalSigAndInitialize(void* rewriterWrapperPtr,

// Modify the Local Var Signature of the method

auto hr = ModifyLocalSig(rewriterWrapper->GetILRewriter(), methodReturnType, methodTypeArguments,
callTargetStateIndex, exceptionIndex, callTargetReturnIndex,
auto hr = ModifyLocalSig(rewriterWrapper->GetILRewriter(), methodReturnType, methodTypeArguments, caller,
callTargetStateIndex, exceptionIndex, callTargetReturnIndex, staticValueTypeIndex,
returnValueIndex, callTargetStateToken, exceptionToken, callTargetReturnToken,
additionalLocalIndices, isAsyncMethod);

Expand Down
8 changes: 4 additions & 4 deletions tracer/src/Datadog.Tracer.Native/calltarget_tokens.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ class CallTargetTokens
mdMemberRef GetCallTargetReturnValueDefaultMemberRef(mdTypeSpec callTargetReturnTypeSpec);
mdMethodSpec GetCallTargetDefaultValueMethodSpec(const TypeSignature* methodArgument);

HRESULT ModifyLocalSig(ILRewriter* reWriter, TypeSignature* methodReturnValue, std::vector<TypeSignature>* methodTypeArguments,
ULONG* callTargetStateIndex, ULONG* exceptionIndex, ULONG* callTargetReturnIndex, ULONG* returnValueIndex,
HRESULT ModifyLocalSig(ILRewriter* reWriter, TypeSignature* methodReturnValue, std::vector<TypeSignature>* methodTypeArguments, FunctionInfo* caller,
ULONG* callTargetStateIndex, ULONG* exceptionIndex, ULONG* callTargetReturnIndex, ULONG* staticValueTypeIndex, ULONG* returnValueIndex,
mdToken* callTargetStateToken, mdToken* exceptionToken, mdToken* callTargetReturnToken,
std::vector<ULONG>& additionalLocalIndices, bool isAsyncMethod = false);

Expand Down Expand Up @@ -97,8 +97,8 @@ class CallTargetTokens

mdMethodDef GetCallTargetStateSkipMethodBodyMemberRef();

HRESULT ModifyLocalSigAndInitialize(void* rewriterWrapperPtr, TypeSignature* methodReturnType, std::vector<TypeSignature>* methodTypeArguments,
ULONG* callTargetStateIndex, ULONG* exceptionIndex, ULONG* callTargetReturnIndex, ULONG* returnValueIndex,
HRESULT ModifyLocalSigAndInitialize(void* rewriterWrapperPtr, TypeSignature* methodReturnType, std::vector<TypeSignature>* methodTypeArguments, FunctionInfo* caller,
ULONG* callTargetStateIndex, ULONG* exceptionIndex, ULONG* callTargetReturnIndex, ULONG* staticValueTypeIndex, ULONG* returnValueIndex,
mdToken* callTargetStateToken, mdToken* exceptionToken,
mdToken* callTargetReturnToken, ILInstr** firstInstruction, std::vector<ULONG>& additionalLocalIndices, bool
isAsyncMethod = false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2239,6 +2239,7 @@ HRESULT DebuggerMethodRewriter::Rewrite(RejitHandlerModule* moduleHandler,
ULONG callTargetStateIndex = static_cast<ULONG>(ULONG_MAX);
ULONG exceptionIndex = static_cast<ULONG>(ULONG_MAX);
ULONG callTargetReturnIndex = static_cast<ULONG>(ULONG_MAX);
ULONG staticValueTypeIndex = static_cast<ULONG>(ULONG_MAX);
ULONG returnValueIndex = static_cast<ULONG>(ULONG_MAX);
mdToken callTargetStateToken = mdTokenNil;
mdToken exceptionToken = mdTokenNil;
Expand Down Expand Up @@ -2275,8 +2276,8 @@ HRESULT DebuggerMethodRewriter::Rewrite(RejitHandlerModule* moduleHandler,
methodReturnType = caller->method_signature.GetReturnValue();
}
auto debuggerLocals = std::vector<ULONG>(debuggerTokens->GetAdditionalLocalsCount(methodArguments));
hr = debuggerTokens->ModifyLocalSigAndInitialize(&rewriterWrapper, &methodReturnType, &methodArguments, &callTargetStateIndex, &exceptionIndex,
&callTargetReturnIndex, &returnValueIndex, &callTargetStateToken,
hr = debuggerTokens->ModifyLocalSigAndInitialize(&rewriterWrapper, &methodReturnType, &methodArguments, caller, &callTargetStateIndex, &exceptionIndex,
&callTargetReturnIndex, &staticValueTypeIndex, &returnValueIndex, &callTargetStateToken,
&exceptionToken, &callTargetReturnToken, &firstInstruction, debuggerLocals, isAsyncMethod);

ULONG lineProbeCallTargetStateIndex = debuggerLocals[0];
Expand Down
90 changes: 62 additions & 28 deletions tracer/src/Datadog.Tracer.Native/tracer_method_rewriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,12 @@ HRESULT TracerMethodRewriter::Rewrite(RejitHandlerModule* moduleHandler, RejitHa
Current CallTarget Limitations:
===============================

1. Static methods in a ValueType (struct) cannot be instrumented.
2. Generic ValueTypes (struct) cannot be instrumented.
3. Nested ValueTypes (struct) inside a Generic parent type will not expose the type instance (the instance will
1. Generic ValueTypes (struct) cannot be instrumented.
2. Nested ValueTypes (struct) inside a Generic parent type will not expose the type instance (the instance will
be always null).
4. Nested types (reference types) inside a Generic parent type will not expose the type instance (the instance
3. Nested types (reference types) inside a Generic parent type will not expose the type instance (the instance
will be casted as an object type).
5. Methods in a Generic type will not expose the Generic type instance (the instance will be casted as a non
4. Methods in a Generic type will not expose the Generic type instance (the instance will be casted as a non
generic base type or object type).
*/

Expand Down Expand Up @@ -183,6 +182,7 @@ HRESULT TracerMethodRewriter::Rewrite(RejitHandlerModule* moduleHandler, RejitHa
ULONG callTargetStateIndex = static_cast<ULONG>(ULONG_MAX);
ULONG exceptionIndex = static_cast<ULONG>(ULONG_MAX);
ULONG callTargetReturnIndex = static_cast<ULONG>(ULONG_MAX);
ULONG staticValueTypeIndex = static_cast<ULONG>(ULONG_MAX);
ULONG returnValueIndex = static_cast<ULONG>(ULONG_MAX);

std::vector<ULONG> indexes(tracerTokens->GetAdditionalLocalsCount(methodArguments));
Expand All @@ -193,7 +193,7 @@ HRESULT TracerMethodRewriter::Rewrite(RejitHandlerModule* moduleHandler, RejitHa
auto returnType = caller->method_signature.GetReturnValue();

tracerTokens->ModifyLocalSigAndInitialize(
&reWriterWrapper, &returnType, &methodArguments, &callTargetStateIndex, &exceptionIndex, &callTargetReturnIndex,
&reWriterWrapper, &returnType, &methodArguments, caller, &callTargetStateIndex, &exceptionIndex, &callTargetReturnIndex, &staticValueTypeIndex,
&returnValueIndex, &callTargetStateToken, &exceptionToken, &callTargetReturnToken, &firstInstruction, indexes);

ULONG exceptionValueIndex = indexes[0];
Expand All @@ -209,17 +209,35 @@ HRESULT TracerMethodRewriter::Rewrite(RejitHandlerModule* moduleHandler, RejitHa
{
if (caller->type.valueType)
{
// Static methods in a ValueType can't be instrumented.
// In the future this can be supported by adding a local for the valuetype and initialize it to the default
// value. After the signature modification we need to emit the following IL to initialize and load into the
// stack.
// ldloca.s [localIndex]
// initobj [valueType]
// ldloc.s [localIndex]
Logger::Warn("*** CallTarget_RewriterCallback(): Static methods in a ValueType cannot be instrumented. ");
return S_FALSE;
reWriterWrapper.LoadLocalAddress(staticValueTypeIndex);
if (caller->type.type_spec != mdTypeSpecNil)
{

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid passing value-type instance when CallTarget falls back to object

For static methods on value types nested in generic parents, GetCurrentTypeRef falls back to mdTokenNil, so WriteBeginMethod/EndMethod specialize CallTarget with TTarget=object. This branch now always emits initobj + ldloc for any value type, so the stack holds an unboxed struct while the calltarget method spec expects object, which can produce invalid IL/JIT failures when instrumenting Outer<T>.Inner.StaticMethod. Consider checking the generic-parent case (same condition used in GetCurrentTypeRef) and using LoadNull or boxing instead of pushing the value type.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was useful as I didn't cover this edge case. Although we don't instrument this case today, in our CallTargetNativeTest program we can enable this scenario. With the latest set of commits, we correctly handle this and load a null value as the CallTarget "instance" so we can in fact instrument this scenario.

reWriterWrapper.InitObj(caller->type.type_spec);
}
else if (!caller->type.isGeneric)
{
reWriterWrapper.InitObj(caller->type.id);
}
else
{
// Generic struct instrumentation is not supported
// IMetaDataImport::GetMemberProps and IMetaDataImport::GetMemberRefProps returns
// The parent token as mdTypeDef and not as a mdTypeSpec
// that's because the method definition is stored in the mdTypeDef
// The problem is that we don't have the exact Spec of that generic
// We can't emit LoadObj or Box because that would result in an invalid IL.
// This problem doesn't occur on a class type because we can always relay in the
// object type.
Logger::Warn("*** CallTarget_RewriterCallback(): Generic struct (struct TypeName<T>) instrumentation is not supported.");
return S_FALSE;
}

reWriterWrapper.LoadLocal(staticValueTypeIndex);
}
else
{
reWriterWrapper.LoadNull();
}
reWriterWrapper.LoadNull();
}
else
{
Expand Down Expand Up @@ -507,19 +525,35 @@ HRESULT TracerMethodRewriter::Rewrite(RejitHandlerModule* moduleHandler, RejitHa
{
if (caller->type.valueType)
{
// Static methods in a ValueType can't be instrumented.
// In the future this can be supported by adding a local for the valuetype
// and initialize it to the default value. After the signature
// modification we need to emit the following IL to initialize and load
// into the stack.
// ldloca.s [localIndex]
// initobj [valueType]
// ldloc.s [localIndex]
Logger::Warn("CallTarget_RewriterCallback: Static methods in a ValueType cannot "
"be instrumented. ");
return S_FALSE;
endMethodTryStartInstr = reWriterWrapper.LoadLocalAddress(staticValueTypeIndex);
if (caller->type.type_spec != mdTypeSpecNil)
{
reWriterWrapper.InitObj(caller->type.type_spec);
}
else if (!caller->type.isGeneric)
{
reWriterWrapper.InitObj(caller->type.id);
}
else
{
// Generic struct instrumentation is not supported
// IMetaDataImport::GetMemberProps and IMetaDataImport::GetMemberRefProps returns
// The parent token as mdTypeDef and not as a mdTypeSpec
// that's because the method definition is stored in the mdTypeDef
// The problem is that we don't have the exact Spec of that generic
// We can't emit LoadObj or Box because that would result in an invalid IL.
// This problem doesn't occur on a class type because we can always relay in the
// object type.
Logger::Warn("*** CallTarget_RewriterCallback(): Generic struct (struct TypeName<T>) instrumentation is not supported.");
return S_FALSE;
}

reWriterWrapper.LoadLocal(staticValueTypeIndex);
}
else
{
endMethodTryStartInstr = reWriterWrapper.LoadNull();
}
endMethodTryStartInstr = reWriterWrapper.LoadNull();
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ public CallTargetNativeTests(ITestOutputHelper output)
: base(new EnvironmentHelper("CallTargetNativeTest", typeof(TestHelper), output, samplesDirectory: Path.Combine("test", "test-applications", "instrumentation"), prependSamplesToAppName: false), output)
{
SetServiceVersion("1.0.0");
#pragma warning disable CS0618 // Type or member is obsolete
EnableDebugMode();
#pragma warning restore CS0618 // Type or member is obsolete
Comment thread
zacharycmontoya marked this conversation as resolved.
Outdated
}

public static IEnumerable<object[]> MethodArgumentsData()
Expand Down Expand Up @@ -65,20 +68,20 @@ public async Task MethodArgumentsInstrumentation(int numberOfArguments, bool fas
{
// On number of arguments = 0 the throw exception on integrations async continuation runs.
// So we have 1 more case with an exception being reported from the integration.
Assert.Equal(180, beginMethodCount);
Assert.Equal(180, endMethodCount);
Assert.Equal(227, beginMethodCount);
Assert.Equal(227, endMethodCount);
Assert.Equal(44, exceptionCount);
}
else if (numberOfArguments == 1)
{
Assert.Equal(175, beginMethodCount);
Assert.Equal(175, endMethodCount);
Assert.Equal(222, beginMethodCount);
Assert.Equal(222, endMethodCount);
Assert.Equal(40, exceptionCount);
}
else
{
Assert.Equal(168, beginMethodCount);
Assert.Equal(168, endMethodCount);
Assert.Equal(215, beginMethodCount);
Assert.Equal(215, endMethodCount);
Assert.Equal(40, exceptionCount);
}

Expand Down
Loading
Loading