Capture stackalloc size expression only if needed

[alrz]

Fixes #24174

[VSadov]

LGTM
Nice change!

[VSadov]

@dotnet/roslyn-compiler - please review

[jcouv]

LGTM Thanks!

[OmarTawfik]

LGTM. Can you please add a test that makes sure we optimize more complex constants as well? stackalloc int[5+6]
Thanks @alrz!

[VSadov]

Also, what happens if it does not fit? Like Span<int> s = stackalloc int[int.MaxValue]

I would expect it to still emit a checked mul and throw at run time.
Can we have a test for this to be sure it does not regress?

[alrz]

I would expect it to still emit a checked mul and throw at run time.

Shouldn't that produce a warning then (for constants)? or an error since it always throw (similar to #8456).

[AlekseyTs]

LGTM (iteration 3)

[VSadov]

@alrz - giving a warning should be a separate issue/workitem

A general approach is that compiler must handle corner cases predictably even if such corner cases are unlikely. This change keeps such behavior and with better IL, so everyone is 👍

Some corner cases clearly indicate that the code contains an error. In such cases a warning may be considered. That is not as simple. The questions that must be asked are:

• is the case likely to happen? (i.e. no point to add code to detect cases that will never happen in reality)
• is there a chance the code is intentional? (then we should not warn)
• introducing a new warning is breaking when /warnaserror is used and that is very common. Are the potential benefits worth the potential grief when code that used to compile no longer does.

IMO the chance of breaking existing code would be low, but the benefits are also not extremely high.

It seems worth logging an issue about the warning, but whether it will be accepted/rejected - hard to tell.

[alrz]

Filed dotnet/roslyn-analyzers#6232 for a warning candidate in warning waves

[VSadov]

@dotnet-bot test windows_release_unit64_prtest please

[VSadov]

runs just keep timing out for no reasons (other runs too) will reopen

[alrz]

@dotnet-bot test windows_release_unit64_prtest please

[alrz]

@dotnet-bot test windows_debug_unit64_prtest please

[VSadov]

@dotnet-bot test windows_release_unit64_prtest please

[VSadov]

@dotnet-bot test windows_debug_unit64_prtest please

[VSadov]

there are actually real failures.

tests are failing with StackOverflowException

[VSadov]

@dotnet-bot test this please

[VSadov]

I do not see anything in this change that could cause SO. Perhaps it was a problem in the branch..

[alrz]

looking at test results,

[alrz]

this is driving me nuts, anything I can do locally to make sure things are ok?

[jaredpar]

@alrz are the tests passing locally?

[alrz]

On my machine just the ones that require ilasm tool. No stackoverflow.

[jcouv]

@alrz To repro CI-only problems, we have commands that are the same that CI uses. Look at build\scripts\cibuild.cmd. You can use that command with additional parameters (such as -testDesktop) to run tests.

[alrz]

OK, all green except for tests fixed in #24323.

local:

Roslyn.Compilers.CSharp.Emit.UnitTests.dll.2                           FAILED 00:02:04.8871981
Roslyn.Compilers.VisualBasic.Semantic.UnitTests.dll                    FAILED 00:05:08.1064356

CI (excluding timeouts):

Roslyn.Compilers.CSharp.Emit.UnitTests.dll.3                           FAILED 00:02:30.0768996

The VB failure doesn't appear in CI results though so I'm not sure if that's the cause. probably worth to wait for #24323 to reach master first.

[VSadov]

I wonder if all required fixes have reached the branch

[VSadov]

I think you might not be able to catch this when it actually runs.

[VSadov]

Ah, no it is a regular overflow, not a StackOverflow.

[VSadov]

Actually this is the problem. On 64bit this is not an overflow since the native int can take this value, but then it becomes SO when we localloc.

I guess on 64bit we should not run the compiled code, only validate the IL.

[alrz]

How can I repro locally? Does ReleaseExe.WithPlatform(Platform.X86) help here?

[alrz]

or should I skip it for now?

[VSadov]

both build and test utilities in the repo root take -test64 switch.

I think build -test64 might do it.

[VSadov]

nope :-), just tried build -test64 and it only did a build. Need to run test -test64 after that.

Normally our test failures are not platform specific, so we just rely on CI

[VSadov]

I would just make a fix and push the change to the branch.
It should really be something like:

var expectedIL = "....";


if (IntPtr.Size == 4)
{
       CompileAndVerify(comp, expectedOutput: "10", verify: Verification.Fails)
       . . . Verify (expectedIL):...
}
else
{
      // on 64bit the native int does not overflow, so we get StackOverflow instead
      // therefore we will just check the IL
      CompileAndVerify(comp, verify: Verification.Fails, options:TestOptions.ReleaseDll)
       . . . Verify (expectedIL):...
}

[VSadov]

Microsoft.CodeAnalysis.Editor.UnitTests.Structure.StructureTaggerTests.OutliningTaggerTooltipText [FAIL]

seems like a flaky test

https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_release_unit64_prtest/11891/

[VSadov]

@dotnet-bot test windows_release_unit64_prtest please

[VSadov]

Thanks!!