A function calling Assembly.GetExecutingAssembly() gets inlined across assembly boundaries #9283
Comments
ForNeVeR
changed the title
|
I think this is a feature and not a bug. In my local logging framework I exploit this behavior to get the executing assembly from the call to the log, not the log assembly itself. Changing this behavior would break that code and possibly others that use similar techniques. You probably want to use |
No, in this case, the function is inlined by the F# compiler; I was able to confirm this hypothesis by reading the decompiled code of my assembly. The entry point looks like this: public static int main(string[] argv)
{
PrintfFormat<Unit, TextWriter, Unit, Unit> printfFormat = new PrintfFormat<Unit, TextWriter, Unit, Unit, Unit>("Hello World from F#!");
PrintfModule.PrintFormatLineToTextWriter<Unit>(Console.Out, printfFormat);
PrintfFormat<FSharpFunc<AssemblyName, Unit>, TextWriter, Unit, Unit> printfFormat2 = new PrintfFormat<FSharpFunc<AssemblyName, Unit>, TextWriter, Unit, Unit, AssemblyName>("ProjectA: %A");
PrintfModule.PrintFormatLineToTextWriter<FSharpFunc<AssemblyName, Unit>>(Console.Out, printfFormat2).Invoke(Assembly.GetExecutingAssembly().GetName());
printfFormat2 = new PrintfFormat<FSharpFunc<AssemblyName, Unit>, TextWriter, Unit, Unit, AssemblyName>("ProjectB: %A");
PrintfModule.PrintFormatLineToTextWriter<FSharpFunc<AssemblyName, Unit>>(Console.Out, printfFormat2).Invoke(Assembly.GetExecutingAssembly().GetName());
return 0;
}Here, the first call to Also, I believe that
It may or may not be a problem that I wasn't able to find I mean, if F# compiler starts respecting this attribute at some point (currently it seems it doesn't), its absence from the ref assemblies may impose a problem.
How do you work around the fact that this approach doesn't work in Debug mode?
You probably meant To conclude, currently I believe this is, in fact, a bug and not a feature. F# compiler inlines the code that shouldn't be inlined, as described by Not a big deal? Probably. A feature? Nope. |
|
@ForNeVeR Try this: Produces this output on retail build: |
@ForNeVeR, I was wrong, the code uses All these methods are marked debug-only, so I'm doing the inverse: when in debug mode, I know certain functions are not erased or inlined (unless Sorry for the clutter, as I deliberately wrote in my code not to use
Totally in agreement there now, thanks for the insights! |
|
@KevinRansom, thanks, I confirm that this workaround works (and already mentioned it in my opening post). I do still believe the current behavior is an issue, but I don't think it's a critical one. |
|
If you don't mind, I'm going to close this, everything is working as it is intended. Thanks for the report. |
|
@KevinRansom, I don't think it is working as expected. You wrote:
But in this case, the method [MethodImpl(MethodImplOptions.NoInlining)]
[SecuritySafeCritical]
[__DynamicallyInvokable]
public static Assembly GetExecutingAssembly()
{
StackCrawlMark stackMark = StackCrawlMark.LookForMyCaller;
return RuntimeAssembly.GetExecutingAssembly(ref stackMark);
}And: // DynamicSecurityMethodAttribute:
// All methods that use StackCrawlMark should be marked with this attribute. This attribute
// disables inlining of the calling method to allow stackwalking to find the exact caller.
//
// This attribute used to indicate that the target method requires space for a security object
// to be allocated on the callers stack. It is not used for this purpose anymore because of security
// stackwalks are not ever done in CoreCLR.And I can confirm, by looking at the generated IL from the FSC compiler that @ForNeVeR is correct that the method that uses Yes, the workaround is to add |
|
I agree with the previous comment. It looks like this is a compiler bug (or, let's say, an open issue in compiler), even if a minor one. I think it should be reopened, if there's no policy to close minor/archived/postponed bugs. |
|
Yeah, this looks like a bug. |
|
@abelbraaksma, I have a comment on your understanding of
|
|
@ForNeVeR Ok, reading again al comments, trying to understand. You wrote:
But Then there's If it's not transitive, and the proper security attribute is not present on This is my educated analysis, being on mobile I didn't check this with any spec. |
|
@abelbraaksma, I can see your logic, but it looks like we're looking at different sources/variants of For definition of (which could or could not be the real sources used by runtime) These more or less correspond to the sources I get from the source server when looking for And where did you get the sources without this attribute, but with different attribute set? |
You were right. The quote for the source of .method public hidebysig static
class System.Reflection.Assembly GetExecutingAssembly () cil managed noinlining
{
.custom instance void System.Security.SecuritySafeCriticalAttribute::.ctor() = (
01 00 00 00
)
.custom instance void __DynamicallyInvokableAttribute::.ctor() = (
01 00 00 00
)
// Method begins at RVA 0xf6184
// Code size 10 (0xa)
.maxstack 1
.locals init (
[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.RuntimeAssembly::GetExecutingAssembly(valuetype System.Threading.StackCrawlMark&)
IL_0009: ret
} Contrast that with .NET Core 3.1, which has this (from ILSpy): .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
}
I created a little test solution to compare, this is from VS 2019, I didn't try Rider. It has two libraries, one for .NET Core and one for .NET Framework. And it has four console apps, two for each library in C# and F#. The library looks like this (same for Core and Framework, only namespace and module name differ, added a bunch of variants to compare consistency of inlining for different types of methods): namespace FSharpBug9283_NetCore
open System
open System.Reflection
open System.Runtime.CompilerServices
module Core =
module ModGetAsm =
let staticName = Assembly.GetExecutingAssembly().GetName().Name
let getName() =
let asm = Assembly.GetExecutingAssembly().GetName().Name
asm
type GetAsm() =
static let staticName = Assembly.GetExecutingAssembly().GetName().Name
let staticName = Assembly.GetExecutingAssembly().GetName().Name
let getName() =
let asm = Assembly.GetExecutingAssembly().GetName().Name
asm
static member GetName() =
let asm = Assembly.GetExecutingAssembly().GetName().Name
asm
member _.InstanceGetNameIndirect1() = getName()
member _.InstanceGetNameIndirect2() = staticName
static member GetNameIndirect3() = staticName
member _.InstanceGetName() =
let asm = Assembly.GetExecutingAssembly().GetName().Name
asmAnd this is the F# calling code: open FSharpBug9283_NetCore
[<EntryPoint>]
let main argv =
printfn "\n.NET Core: "
printfn "Module value: %s" Core.ModGetAsm.staticName
printfn "Module function: %s" (Core.ModGetAsm.getName())
printfn "Static class member: %s" (Core.GetAsm.GetName())
printfn "Static class member ind: %s" (Core.GetAsm.GetNameIndirect3())
printfn "Instance member: %s" (Core.GetAsm().InstanceGetName())
printfn "Instance member ind 1: %s" (Core.GetAsm().InstanceGetNameIndirect1())
printfn "Instance member ind 2: %s" (Core.GetAsm().InstanceGetNameIndirect2())And this is the C# calling code: using FSharpBug9283_NetCore;
using System;
namespace FSharpBug9283_NetCore_Caller_CSharp
{
class Program
{
static void Main(string[] args)
{
Console.WriteLine("\n.NET Core: ");
Console.WriteLine("Module value: {0}", Core.ModGetAsm.staticName);
Console.WriteLine("Module function: {0}", Core.ModGetAsm.getName());
Console.WriteLine("Static class member: {0}", Core.GetAsm.GetName());
Console.WriteLine("Static class member ind: {0}", Core.GetAsm.GetNameIndirect3());
Console.WriteLine("Instance member: {0}", new Core.GetAsm().InstanceGetName());
Console.WriteLine("Instance member ind 1: {0}", new Core.GetAsm().InstanceGetNameIndirect1());
Console.WriteLine("Instance member ind 2: {0}", new Core.GetAsm().InstanceGetNameIndirect2());
}
}
}If I run all combinations (F# vs C# vs Debug vs Release), this is the output (which backs your original report): I should add that if I add I'll add my concluding thoughts in the next post. |
|
Here's what I gathered from my attempt to find definitive™ references:
I also found additional information through this excellent blog post on stack-crawling: https://mattwarren.org/2019/01/21/Stackwalking-in-the-.NET-Runtime/. If I try to put all this scattered info together, I think that:
Whatever we choose to do, there's still the inconsistency between .NET versions... |
|
@abelbraaksma, thank you so much for your investigation. I have tried to inspect the code earlier using reflection, and was puzzled that I cannot find any trace of that
I can't help but to argue, or either clarify this a bit more: as I can see, security-criticality in only transitive one level down, e.g. a method calling a method calling a security-critical method shouldn't get any unusual properties, while a direct caller of a security-critical method should. (this is very important for ease of implementation, that's why I'm nitpicking here) Other than that, I completely agree with your analysis. And yes, there seem to be a problem with the reference assemblies that carry no such attribute. Probably Roslyn (or whatever party is used to generate reference assemblies) should preserve it? |
I actually had different arguments/conclusions before I wrote down that simple bullet point in the end, I expected a response there ;). If you look at how stuff for mscorlib is implemented, you can see that if a method sets I think you are right that this transitivity doesn't carry over to user-code, as I assumed somewhat in my 3rd point at the bottom. In which case indeed only one level of non-inlining needs to be preserved to keep the stack-crawl marks in the right place.
Yeah, same for me, it wasn't until I read the IL Assembler book sections that it became clear. You'd expect this to be in the docs, at least as a remark or something. There's surprisingly little info about this out there, on any platform.
Actually, the IL-disassembly I copied is not from the reference assemblies, that's from the resolved assemblies. But the difference between .NET Framework and .NET Core is real and I'm not sure what the best cause of action is. Probably @dsyme or @cartermp should chime in for that (both, see #9283 (comment) and further for the start of the more detailed analysis of this). |
|
@abelbraaksma Great analysis
Is this attribute present in the .NET Standard 2.0 reference assemblies? (What's in the implementation assemblies isn't of much help as the compiler doesn't see those). All I'm seeing is this (this is for In the absence of a usable attribute I guess we should just make a list of everything on the core libraries that is sensitive to its current caller and hardwire that into the compiler. |
dsyme
added
the
Impact-Medium
@dsyme It appears to be non-present. I tried a handful of Not sure this is an oversight of the runtime team or whether this omission is by design or not. |
|
@dsyme ---- let's not have a list of non-inlinable apis in the compiler. I think this falls into the camp of it hurts stop doing it. I can as easily imagine scenarios, where the inlinining is desired as I can imagine those where it is not desired. We should just ensure that the inlining behaviour is called out to developers. If we were the C# team we would probably say let's build an analyzer that spots this. An analyzer can have as much hired wired as it wants. So perhaps add a check to fslint? |
@KevinRansom, please note that
The C# team just recognizes The problem we are facing is that we don't "see" this because we link the reference assemblies during compilation, where this attribute is not exposed. I've reported it to .NET in case it's considered an oversight (any other meta information is available in the reference assemblies), see dotnet/runtime#41690. |
|
@abelbraaksma , we are most assuredly not violating the CLR spec, because we are inlining IL at source code compile time. The spec is referring to optimizing the native code that is generated at jit or native compilation time. It might be worth getting the BCL team to add the attribute to reference assemblies or otherwise decorate them so that we have a chance to deal with it, I am merely reluctant to add another list of "special types that we treat differently". The reason that C# doesn't do this, is because they rely on the jit for inlining operations and we don't. |
Aha, so you're basically saying that even though the JIT is not allowed to inline, we can, because we do it at compile time? I didn't look at it that way. But still, it creates the awkward situation as explained by the OP: if we do things the JIT cannot do, this subsequently leads to differences in runtime result. This is particularly painful for methods that heavily rely on an intact stack frame.
I fully agree, and hopefully @jkotas will give it some consideration, but it looks like there may be another solution (I just don't see how it helps us, yet): dotnet/runtime#41690 (comment) |
|
@abelbraaksma , in FSharp inline does exactly what you would expect. It inlines the code and inlines much bigger chunks of code than the jit does. If we want the compiler to respect a set of attributes that are only in the implementation assemblies, then the dotnet framework team need to ensure those methods are available to us at compile time. And then we need to implement the feature that enables it. It should be noted, that turning that feature on would be a breaking change. It should also be noted that just switching from debug to retail today would also change behaviour. If the developer wanted that Api to not behave as though it was not inlined, then placing inline on the method that included it is a bug. I may be looking at this too simplistically ... but I don't think so. Kevin |
|
@KevinRansom, this issue is not about using the Hence the observed difference of behavior, even though the method signatures used in F# vs C# were equal and no The problem arises when the body of such a method contains a method call that relies in the stack. When that stack gets erased, outside of the control of the user, it leads to surprises and undocumented behavior (as in, contrary to what the method is described to do). |
|
Then shove the no-inlining attribute on the method, and call it good. |
|
Indeed, that's mentioned above as workaround ;). But that's rather hard to diagnose, most people are not aware of this, so it's better to try to make the compiler behave "more correctly" in light of such methods. They are marked for a reason with the (little known) |
|
@abelbraaksma ... That API is marked with that attribute for jit compilation, I wonder what the source generators are going to do with this on the AOT scenarios, I imagine they will say to the bcl team decorate it in the reference assemblies and we will honour it somhow, also I suppose the source generator will be generating a new assembly, so inlining is less likely, although I can imagine doing it. The compile time assemblies don't have it, and until they do any other heuristic we pick is just that, a heuristic, and its likely to break over time. Step one is for the framework guys to decorate those assemblies. Heuristics and well known lists are particularly annoying, to me this week because when I ensured FSI and the compiler could support netstandard assemblies, I broke support for all older .net frameworks which didn't have the netstandard shim in their references assemblies, it's taken until now to show up, however it did eventually and so now I have to fix it and I am not sure how. Having said that, this feels pretty much a consequence of us opportunistically optimizing methods, that has other side effects for example when throwing an exception from an inlined function the stack frame doesn't include the function that was inlined only the call site to which it was inlined. I assert this specific issue is rare, it is not at all certain that a developer wants to report the name of the dll the code was inlined from. I would actually like the caller name in logging scenarios, of course in that case because I want it, I would certainly put inline on it to make it clear, and I would be really annoyed if it didn't do what I wanted. Kevin |
|
I think we agree. If While these don't show that information, unless there's another analytical method we can use (like using the implementation assemblies), we shouldn't hard code this in, as that's prone to maintenence nightmare. |
|
@abelbraaksma , we can't use the implementation assemblies, there is no guarantee that the flag will remain constant, and transfer across platforms. Although to be fair, for this example it is likely. Even if the above wasn't true it is still the case that modifying the behavior is still a breaking change. Or at least a change in behavior. It is just as likely I think that a developer would expect the behavior to not change in the future. If I wrote the logging api that depended on automagic inlining that I hypothesized before I would be annoyed that I had to change my code to now say inline on the code that had inlined perfectly well before. This is one of those changes that one would like to fix from a position of consistency or completeness, but the question is "Is it actually worth the change"? Right now the behavior is deterministic discoverable, and using inline and the dontinline attribute can even be made explicit. I strongly suggest using fslint to prompt developers to use either the dontinline attribute or the inline flag. We should also consider, including fslint and configuration property pages in VS and Ionide by default. |
|
For what it's worth, fsharplint can be configured through a json file and FSAC-based editors do pick that up. No nice UI, but json for these sorts of linting tools is pretty commonplace (eslint being the initial driver). |
And I was ready to create an issue about that...My opinion is that inlining functions at the compiler-level without the However, such automatic inlining looks OK for limited cases like small nested functions or active patterns. Automatically inlining functions across assembly boundaries, like what this issue is concerned with, is a terrible idea because it breaches the barrier of abstraction between assemblies and should never be performed without the |
|
@teo-tsirpanis -- I don't know that I agree that it is a terrible idea, however, it is often quite inconvenient, and makes the source code a lot harder to reason about. I think that having fslint tell developers this is an iffy construct and they should use the noinlining attribute is the best result we can do right now. For sure turning off automagic inlining would ensure that the performance characteristics of most code would change, and probably not for the better. |
|
I agree with Teo that automatic cross-assembly inlining is bad idea. Code tends to change and once correct code can cause Maybe compiler should have flag to control implicit inlining, e.g. |
|
You can disable cross-assembly inlining using Libraries can suppress cross-assembly inline information using |
|
The dotnet bug report led to a new API proposal that would allow to implicitly pass caller identity, which would ultimately be an alternative for the flawed behavior explained in this issue, see: dotnet/csharplang#4984. |
dsyme
added
Area-Compiler-Optimization
|
I am going to close this, there is nothing really for us to do. To address this a developer can add [<MethodImpl(MethodImplOptions.NoInlining)>], there's no reliable attribute for us to detect, and lists of well-known APIs are always difficult to reason about and maintain, and the developer can also suppress cross-assembly inlining when needed. Thanks for the discussion Kevin |
github-project-automation
bot
moved this from Not Planned
to Done
in F# Compiler and Tooling
@KevinRansom, the point of this whole discussion and issue was, that See this comment for a summary: #9283 (comment) and this:
I'm not saying it is wrong to close this, I consider it "not worth the effort" (however much this makes |
|
Even if we wanted to fix this behavior, not having It is sad, but looks like you are right in that there's nothing we can do here... |
|
C#, notably, "solved" it by not having inlining at all. |
Repro steps
let getName() = Assembly.GetExecutingAssembly().GetName(), and another one using that functionHere's a sample project with reproduction; just in case, I'll attach all the sources as a ZIP archive, too: fsharp-inlining-bug-master.zip
For ease of reading, here're the main source files:
Output:
Expected output: in both cases, the same:
Known workarounds
Mark a function containing a call to
Assembly.GetExecutingAssembly()as[<MethodImpl(MethodImplOptions.NoInlining)>].Related information
The text was updated successfully, but these errors were encountered: