is this a correct approach?

It does look correct but I'm not convinced that using "forceinline" to control stacktraces is the right thing to do.

It normally works the other way around - you use "no inline" to ensure that a particular method does show up in the stack trace. All the time. That's because "no inline" can normally be relied on, it always works unless the compiler is dumb.

That's not the case with "forceinline", it is an optimization hint and there's no guarantee that the compiler will respect it. When an optimization is not performed the "normal" outcome is a drop in performance, not a change in observable behavior.

I'm not very familiar with ExceptionDispatchInfo but its design seems questionable. It takes it upon itself to throw the exception instead of allowing the user to retrieve the exception object and do whatever it wants with it - including throwing it.

I'm not very familiar with ExceptionDispatchInfo but its design seems questionable. It takes it upon itself to throw the exception instead of allowing the user to retrieve the exception object and do whatever it wants with it - including throwing it.

Line numbers go off if you separate the restore and throw #14564 (comment)

Though ideally its what I'd like to do; however it should still tend to inline for upstack (would need api review for exposing the exception and its behaviour fixed)

That's not the case with "forceinline", it is an optimization hint and there's no guarantee that the compiler will respect it.

I think that's fine; the issue is currently "no return" is the equivalent of "no inline"; so its more a way of removing the "no inline", without changing the method to return something or making it more complicated so it drops the "no return" - at which point it probably wouldn't inline anyway :)

Agree with @mikedn -- not convinced this particular use case makes sense. The runtime should have various bits of magic to make exceptions appear "as if" they were raised in the caller frame which might be more applicable here.

Also somewhere down the road we might get around to creating synthetic frames for inlined methods. The jit has most of this info today, but has no way to pass it back to the runtime.

At any rate, the idea of allowing force inline to override the inliner's "profitability" checks is sound; the aggressive inline annotation should basically mean "inline if at all possible".

I generally don't like to suppress observations, but instead suppress their effects. So I would have implemented this by modifying EnhancedLegacyPolicy::NoteInt to have an extra clause:

        case InlineObservation::CALLEE_NUMBER_OF_BASIC_BLOCKS:
            ...
            if (m_IsNoReturn && (basicBlockCount == 1))  // && !m_IsForceInline
            {
                SetNever(InlineObservation::CALLEE_DOES_NOT_RETURN);
            }

I think that's fine

Depends on the context. It looks like you are trying to cleanup some stacktraces and you want to rely on a mechanism (inlining) that works only when optimizations are enabled. So in Debug builds you get messy stacktraces and in Release builds you get clean stacktraces. That's sort of upside down.

FWIW I think the long term solution to this class of issue is to modify how we generate the textual form of stacktraces that developers read rather than imposing constraints on codegen. However as an interim step I don't see anything wrong with adjusting the underlying code if those changes aren't causing concerns in other aspects of design/performance/etc. More discussion is in #14564 if you want to see it.

not convinced this particular use case makes sense.

I don't think its the long term solution; however EDI is used quite a lot by aspnet and they and users are already having issue with the async stack trace length and the "no return" change adds another entry in each step

So in Debug builds you get messy stacktraces and in Release builds you get clean stacktraces. That's sort of upside down.

Its a compromise, in Debug you are dealing with one stack trace at a time; in Release you might be logging thousands or hundreds of thousands; increasing data volumes and making it harder to spot patterns in aggregate.

Will add comment that its a temporary work around. EDI aside; not sure the non-specified "no-return" should take precedence over the specified "aggressive inline"; i.e. have no opt out.

So I would have implemented this by modifying EnhancedLegacyPolicy::NoteInt to have an extra clause:

m_IsForceInline set prior to the observation? I'm guessing so as is attribute vs method body processing

Will change, is much better

however EDI is used quite a lot by aspen

This change is unlikely to fix aspnet cases for most users. aspnet should be R2R precompiled in most cases. The R2R precompilation prohibits cross-module inlining.

Formatting fix #14602

The R2R precompilation prohibits cross-module inlining.

Most of the aspnet stack traces would be cleaned up by #14564

Have removed the EDI change from this PR - is a separate issue

So in Debug builds you get messy stacktraces

Added change for StackTraces that also works for Debug mode #14608

Rebased

Is this still something you're interested in pursuing? As noted above I'd rather not suppress observations as this limits the ability to experiment with policy changes -- instead the policy should decide whether or not an observation implies the inline should fail.

So the change should be just a simple edit to the CALLEE_NUMBER_OF_BASIC_BLOCKS case in DefaultPolicy::NoteInt.

            if (!m_IsForceInline && m_IsNoReturn && (basicBlockCount == 1))
            {
                SetNever(InlineObservation::CALLEE_DOES_NOT_RETURN);
            }

Is this still something you're interested in pursuing?

Yes, more for the principle at this stage rather than any particular use case, now 😄

Should be able to opt-out in some way. Made the change

Centos failure

Two system times were tested, with a sleep of 3 seconds between.  
The time passed should have been at least 3 seconds.  
But, it was less according to the function.
18:52:34 
18:52:34 FAILED: file_io/GetSystemTimeAsFileTime
   /test1/paltest_getsystemtimeasfiletime_test1. Exit code: 1

@dotnet-bot test CentOS7.1 x64 Checked Innerloop Build and Test

@benaadams thanks!