[release/5.0] Fix silent bad codegen VSD possible tailcall converted to normal call

[BruceForstall]

Customer Impact

In the situation described below, the JIT generates incorrect (silent bad) code. Generated code will misbehave, leading to a crash, data corruption, or similar. It is customer reported (see #49078). It's likely a very rare situation, but customers have no way of knowing if they've hit this situation. Basic code would be "return Foo(args)" for some generic interface call Foo returning a struct, plus a number of additional conditions.

Regression?

Yes.

Risk (see taxonomy)

Low.

Is there a packaging impact?

No.

Full description

The problem was when a VSD interface call returning a multi-byte struct
in registers was initially considered to be a tailcall, but the tailcall
was abandoned in morph due to not enough stack space to store outgoing
arguments, in which case we create a new call return local to store the
return struct, and re-morph the call. In doing so, we forget that we had
already added VSD non-standard args, and re-add them, leaving the originally
added arg as a "normal" arg that shouldn't be there.

So, in summary, for a call A->B, to see this failure, we need:

1. The call is considered a potential tailcall (by the importer)
2. The call requires non-standard arguments that add call argument IR in
   fgInitArgInfo() (e.g., VSD call -- in this case, a generic interface call)
3. We reject the tailcall in fgMorphPotentialTailCall() (e.g., not enough
   incoming arg stack space in A to store B's outgoing args), in this case
   because the first arg is a large struct. We can't reject it earlier,
   due to things like address exposed locals -- we must get far enough
   through the checks to have called fgInitArgInfo() to add the extra
   non-standard arg.
4. B returns a struct in multiple registers (e.g., a 16-byte struct in
   Linux x64 ABI)

The fix is to remove the previously-added non-standard VSD argument IR when
we are preparing to re-morph a call.

There was one other location that was already doing this. I'm a little
worried that there are other places where the non-standard added IR is
not being considered when we clear out the arg info before remorphing.
It seems like there is some risk here. Probably, we should consider a
better way of handling the non-standard arg IR given the need in some
cases to re-morph calls.

I commented out a few cases of:

assert(!fgCanFastTailCall(call, nullptr));

because this function can call fgInitArgInfo which can alter the IR.
That seems dangerous in an assert, which should have any side-effects
like modifying the IR.

Fixes #49078

No SPMI asm diffs.

[BruceForstall]

@sandreenko @dotnet/jit-contrib PTAL

[jeffschwMSFT]

Approved. @JulieLeeMSFT can you take this issue for consideration in 5.0.x?

[JulieLeeMSFT]

Approved. @JulieLeeMSFT can you take this issue for consideration in 5.0.x?

Will do.

[BruceForstall]

@JulieLeeMSFT I've lost track of the servicing process. When should I expect this will get approved and merged?

[jeffschwMSFT]

@BruceForstall we will take it this week for consideration. If approved it will be merged for us once the branch is open for changes.

[Anipik]

@BruceForstall are the outerloop failures expected ?

[BruceForstall]

@Anipik The outerloop failures are unrelated to my change. I had triggered that pipeline to get more testing on this change. However, it appears that the pipeline is not maintained properly in the release/5.0 branch, so, unfortunately, I don't think there is currently any value there. That's a problem, IMO, but it shouldn't block this change.

[Anipik]

known failure #48091