Capture stackalloc size expression only if needed by alrz · Pull Request #24183 · dotnet/roslyn

alrz · 2018-01-11T19:13:30Z

Fixes #24174

VSadov

LGTM
Nice change!

VSadov · 2018-01-11T22:12:03Z

@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 · 2018-01-11T22:39:41Z

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 · 2018-01-12T08:41:36Z

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 · 2018-01-12T19:24:43Z

@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 · 2018-01-12T19:34:10Z

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

VSadov · 2018-01-12T20:42:00Z

@dotnet-bot test windows_release_unit64_prtest please

VSadov · 2018-01-12T20:51:52Z

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

alrz · 2018-01-14T18:33:39Z

@dotnet-bot test windows_release_unit64_prtest please

alrz · 2018-01-14T18:34:06Z

@dotnet-bot test windows_debug_unit64_prtest please

VSadov · 2018-01-16T18:43:11Z

@dotnet-bot test windows_release_unit64_prtest please

VSadov · 2018-01-16T18:43:26Z

@dotnet-bot test windows_debug_unit64_prtest please

VSadov · 2018-01-17T05:20:24Z

there are actually real failures.

tests are failing with StackOverflowException

VSadov · 2018-01-19T18:45:07Z

@dotnet-bot test this please

VSadov · 2018-01-19T18:45:45Z

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

alrz · 2018-01-20T18:32:00Z

looking at test results,

alrz · 2018-01-20T18:32:33Z

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

jaredpar · 2018-01-22T16:48:49Z

@alrz are the tests passing locally?

alrz · 2018-01-22T19:03:03Z

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

jcouv · 2018-01-22T20:11:05Z

@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 · 2018-01-22T20:58:48Z

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 · 2018-01-25T17:47:51Z

I wonder if all required fixes have reached the branch

VSadov · 2018-01-25T18:36:00Z

+        {
+            M();
+        }
+        catch (OverflowException)


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

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

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.

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

or should I skip it for now?

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

I think build -test64 might do it.

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

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 · 2018-01-25T20:12:50Z

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 · 2018-01-25T20:13:07Z

@dotnet-bot test windows_release_unit64_prtest please

VSadov · 2018-01-25T20:47:40Z

Thanks!!

Capture stackalloc size expression only if needed

fbb5451

alrz requested a review from a team as a code owner January 11, 2018 19:13

Address test failure

0179b41

VSadov approved these changes Jan 11, 2018

View reviewed changes

VSadov requested a review from OmarTawfik January 11, 2018 22:11

jcouv approved these changes Jan 11, 2018

View reviewed changes

OmarTawfik approved these changes Jan 11, 2018

View reviewed changes

jcouv added this to the 15.8 milestone Jan 11, 2018

jcouv added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jan 11, 2018

Add additional tests

90f51df

AlekseyTs approved these changes Jan 12, 2018

View reviewed changes

alrz closed this Jan 12, 2018

alrz reopened this Jan 12, 2018

VSadov closed this Jan 12, 2018

VSadov reopened this Jan 12, 2018

VSadov closed this Jan 12, 2018

VSadov reopened this Jan 12, 2018

VSadov closed this Jan 17, 2018

VSadov reopened this Jan 17, 2018

VSadov closed this Jan 25, 2018

VSadov reopened this Jan 25, 2018

VSadov reviewed Jan 25, 2018

View reviewed changes

Do not run the StackAllocSpanLengthOverflow test on x64 platform

59bb3fd

VSadov merged commit a68d632 into dotnet:master Jan 25, 2018

alrz deleted the stackalloc-fix branch January 25, 2018 20:48

VSadov mentioned this pull request Mar 16, 2018

Merge dev15.7.x to master #25503

Merged

alrz mentioned this pull request Mar 31, 2018

Add suggested stackalloc initializer tests #25656

Merged

Conversation

alrz commented Jan 11, 2018

Uh oh!

VSadov left a comment

Choose a reason for hiding this comment

Uh oh!

VSadov commented Jan 11, 2018

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

OmarTawfik left a comment

Choose a reason for hiding this comment

Uh oh!

VSadov commented Jan 11, 2018

Uh oh!

alrz commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

VSadov commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alrz commented Jan 12, 2018

Uh oh!

VSadov commented Jan 12, 2018

Uh oh!

VSadov commented Jan 12, 2018

Uh oh!

alrz commented Jan 14, 2018

Uh oh!

alrz commented Jan 14, 2018

Uh oh!

VSadov commented Jan 16, 2018

Uh oh!

VSadov commented Jan 16, 2018

Uh oh!

VSadov commented Jan 17, 2018

Uh oh!

VSadov commented Jan 19, 2018

Uh oh!

VSadov commented Jan 19, 2018

Uh oh!

alrz commented Jan 20, 2018

Uh oh!

alrz commented Jan 20, 2018

Uh oh!

jaredpar commented Jan 22, 2018

Uh oh!

alrz commented Jan 22, 2018

Uh oh!

jcouv commented Jan 22, 2018

Uh oh!

alrz commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VSadov commented Jan 25, 2018

Uh oh!

VSadov Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

VSadov Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

VSadov Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

alrz Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alrz Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

VSadov Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

VSadov Jan 25, 2018

alrz commented Jan 12, 2018 •

edited

Loading

VSadov commented Jan 12, 2018 •

edited

Loading

alrz commented Jan 22, 2018 •

edited

Loading

alrz Jan 25, 2018 •

edited

Loading

VSadov commented Jan 25, 2018 •

edited

Loading