Runtime: Attribute to tag methods that implicitly depend on its caller

[abelbraaksma]

Description

The reference assemblies typically expose the same meta-info as implementation assemblies, but DynamicSecurityMethod, or reqsecobj is not exposed and only visible in the implementation assembly.

As an example, check GetCallingAssembly or GetExecutingAssembly.

Is this a deliberate limitation of the reference assemblies? It appears to me that this information should be present and the 70 or so BCL methods (see https://github.com/dotnet/runtime/issues/11698#issuecomment-449225852, but probably less now after https://github.com/dotnet/coreclr/pull/21825) that have reqsecobj should also set DynamicSecurityMethod in their reference assemblies.

If there is no backwards compat issue or other requirement that prevents inclusion of reqsecobj in the signatures, I propose we add it, so that compilers can rely on these signatures to be accurate.

Other information

This information is relevant for compilers, like here in F# (https://github.com/dotnet/fsharp/issues/9283#issuecomment-636574385), where we _should not_ inline GetExecutingAssembly, but since the compiler uses the reference assemblies as a guide, it cannot properly make that decision.

Note that this is an inlining decision at compile time, irrelevant on the inlining decision of the JIT, which doesn't have this problem as it'll see the implementation assembly.

A workaround was proposed by @dsyme (see https://github.com/dotnet/fsharp/issues/9283#issuecomment-684896970) to hard-code the BCL methods that have reqsecobj and/or StackCrawlMark so that

[jkotas]

DynamicSecurityMethod attribute was used for Code Access Security originally. CoreCLR reused it as an internal implementation detail for a different purpose that has nothing to do with security.

If we were to make it a public concept, I think we should give it a new name that captures the intent properly.

[jkotas]

There may be alternative designs possible: Add CallerObjectAttribute that can be applied to [Assembly/Type/MethodBase] argument. The compiler is then required to fill in the caller object, similar to how it fills in names for the existing CallerMemberNameAttribute and other similar types. This design would also fix the bad performance of these methods (e.g. https://github.com/dotnet/runtime/issues/41575).

[abelbraaksma]

@jkotas, I think you misunderstand my proposal here. This is not about making it available to the general public, nor to change its meaning.

It's about an inconsistency in the attribute not being present in the reference assemblies of dotnet, while the implementation assembly does carry it.

This pseudocustom attribute is emitted as a reserved method flag. While it was originally used for security, it has also been applied to BCL methods that require an intact stack, and therefore cannot, and should not be cross module optimized.

The absence of this flag in the reference assembly, as noted by @dsyme, leads to incorrect compiled IL by compilers that do compile time inlining like F#.

In short, I think all method flags in the reference assembly of runtime .NET Core libraries should be equal to the flags in the implementation assembly.

[abelbraaksma]

@jkotas, just to emphasize: this is not an api-suggestion, but a bug, or oversight in the current Base Class Libraries.

I can see two ways to fix this:

• Add NoInlining to the methods in BCL (both in reference and impl. assemblies) that should never be inlined (this is a no-op for JIT as these methods already aren't inlined because of reqsecobj present)
• Add reqsecobj to the reference assembly in places where it doesn't align with the impl. assemblies. Again, this won't be seen by JIT, and we can take care of this in the F# compiler by looking at the methodsig of the reference assembly as we currently do.

Example:

Take GetExecutingAssembly. The reference assembly, i.e. C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\3.1.0\ref\netcoreapp3.1\System.Runtime.dll has:

.method public hidebysig static class System.Reflection.Assembly 
        GetExecutingAssembly() cil managed
{
  // Code size       2 (0x2)
  .maxstack  8
  IL_0000:  ldnull
  IL_0001:  throw
} // end of method Assembly::GetExecutingAssembly

While the implementation assembly has:

.method public hidebysig reqsecobj static class System.Reflection.Assembly 
        GetExecutingAssembly () cil managed 
{
    // Method begins at RVA 0x159270
    // Code size 10 (0xa)
    .maxstack 1
    .locals (
        [0] valuetype System.Threading.StackCrawlMark
    )

    IL_0000: ldc.i4.1
    IL_0001: stloc.0
    IL_0002: ldloca.s 0
    IL_0004: call class System.Reflection.RuntimeAssembly System.Reflection.Assembly::GetExecutingAssembly(valuetype System.Threading.StackCrawlMark&)
    IL_0009: ret
}

Note the difference in the signature, the first misses the reqsecobj, which should not be inlined.

-.method public hidebysig static class System.Reflection.Assembly 
+.method public hidebysig reqsecobj static class System.Reflection.Assembly 

[jkotas]

It's about an inconsistency in the attribute not being present in the reference assemblies of dotnet, while the implementation assembly does carry it.

We have number of other attributes that are present in the implementation assemblies, but that are not present in reference assemblies.

this is not an api-suggestion

The DynamicSecurityMethod attribute that this is partially about is not in the public .NET 5 API surface. We treat all additions to the public surface as API suggestions.

[dsyme]

@abelbraaksma I agree with @jkotas - this is really an internal implementation flag.

There may be alternative designs possible: Add CallerObjectAttribute that can be applied to [Assembly/Type/MethodBase] argument. The compiler is then required to fill in the caller object, similar to how it fills in names for the existing CallerMemberNameAttribute and other similar types. This design would also fix the bad performance of these methods (e.g. #41575).

This is a good suggestion

[abelbraaksma]

The DynamicSecurityMethod attribute that this is partially about is not in the public .NET 5 API surface. We treat all additions to the public surface as API suggestions.

Aha, gotcha. But this one is currently present internally(but compiled as method flag, not attribute), and since this is a reserved method attribute, we don't need to add the attribute itself to a method in the reference assembly, but only the reqsecobj, method flag 0x8000. I don't mean to expose it.

https://github.com/dotnet/runtime/blob/6072e4d3a7a2a1493f514cdf4be75a3d56580e84/src/coreclr/src/System.Private.CoreLib/src/System/Reflection/Assembly.CoreCLR.cs#L68-L73

We have number of other attributes that are present in the implementation assemblies, but that are not present in reference assemblies.

I wasn't aware. But now I see that NoInlining is _also_ not in the reference assemblies... It starts to dawn on me ;).

[abelbraaksma]

This is a good suggestion

@dsyme, I'm not sure I follow, how would that help the current situation? If it does, that would be great, of course.

[dsyme]

@dsyme, I'm not sure I follow, how would that help the current situation? If it does, that would be great, of course.

It wouldn't, AFAIU it would be a new API in .NET

For F# we still need to do something about knowing what methods should not be inlined

[abelbraaksma]

Possibly relevant to this discussion is how CoreCLR currently fixed this for crossgen: https://github.com/dotnet/coreclr/pull/27050.

The issue for source-code inlining is similar, except that we don't have the same info as the CLR during compiling (as noted in the discussion above). This is not only about inlining, but also about tail-calling when done during the compile phase (vs the JIT phase).