-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
System.Linq.Expressions tests failing on some PRs #47374
Comments
Tagging subscribers to this area: @cston Issue DetailsBlocking clean CI on #45022. See here for the full log. C:\h\w\A13D08CF\w\B433097F\e>"C:\h\w\A13D08CF\p\dotnet.exe" exec --runtimeconfig System.Linq.Expressions.Tests.runtimeconfig.json --depsfile System.Linq.Expressions.Tests.deps.json xunit.console.dll System.Linq.Expressions.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing
Discovering: System.Linq.Expressions.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Linq.Expressions.Tests (found 5646 of 5656 test cases)
Starting: System.Linq.Expressions.Tests (parallel test collections = on, max threads = 2)
System.Linq.Expressions.Tests.ConvertTests.ConvertFloatToUIntTest(useInterpreter: False) [FAIL]
Assert.Equal() Failure
Expected: 0
Actual: 4294967295
Stack Trace:
/_/src/libraries/System.Linq.Expressions/tests/Convert/ConvertTests.cs(11424,0): at System.Linq.Expressions.Tests.ConvertTests.VerifyFloatToUInt(Single value, Boolean useInterpreter)
/_/src/libraries/System.Linq.Expressions/tests/Convert/ConvertTests.cs(3099,0): at System.Linq.Expressions.Tests.ConvertTests.ConvertFloatToUIntTest(Boolean useInterpreter)
System.Linq.Expressions.Tests.ConvertTests.ConvertFloatToNullableUIntTest(useInterpreter: False) [FAIL]
Assert.Equal() Failure
Expected: 0
Actual: 4294967295
Stack Trace:
/_/src/libraries/System.Linq.Expressions/tests/Convert/ConvertTests.cs(11435,0): at System.Linq.Expressions.Tests.ConvertTests.VerifyFloatToNullableUInt(Single value, Boolean useInterpreter)
/_/src/libraries/System.Linq.Expressions/tests/Convert/ConvertTests.cs(3108,0): at System.Linq.Expressions.Tests.ConvertTests.ConvertFloatToNullableUIntTest(Boolean useInterpreter)
Finished: System.Linq.Expressions.Tests
=== TEST EXECUTION SUMMARY ===
System.Linq.Expressions.Tests Total: 35233, Errors: 0, Failed: 2, Skipped: 0, Time: 24,631s
|
Leaving a note that this is likely to have been caused by my #47133. I have confirmed locally that it does hit the new path on |
@SingleAccretion the problem it seems in this transformation in
|
Honestly, looks more like a Roslyn bug(or undefined behavior) to me, run the following: using System;
using System.Runtime.CompilerServices;
class Program
{
[MethodImpl(MethodImplOptions.NoInlining)]
static float Foo(float x) => x;
static void Main()
{
// convert float.MaxValue to int
Console.WriteLine(unchecked((int)float.MaxValue)); // Roslyn emits `ldc.i4.0` here
// convert float.MaxValue to int via a call:
Console.WriteLine((int)Foo(float.MaxValue)); // vcvttss2si returns -1
Console.ReadKey();
}
} /cc @tannergooding PS: clang and MSVC also fold it to 0 (unlike GCC): https://godbolt.org/z/1hP5rT |
We can temporarily remove |
I am afraid that I won't be able to do it today (it being 1:20 AM and all). Thank you Egor for your analysis! I am yet to realize what would the proper fix here be. |
It's UB according to the C# spec. So either the spec needs to change or these tests need to ignore non-finite values (NaN, +-inf) |
I guess appropriate test would be that non-finite cast doesn't throw in |
I think the issue with the tests is that they use Or could just change the expected values to the actual values that it shows on the failure for now, unless the actual value is random and changes all the time. |
This was likely fixed in Roslyn as part of the general determinism fixes around: dotnet/roslyn#37527 You are then only left with the existing |
I believe there was some special handling added for |
After thinking about this for a bit, I suspect there are a couple of issues at play here. The tests that fail assert that the result of the expression The tests have only very recently started failing. This seems fairly mysterious, as the logic for folding in the Jit has been there for a very long time, and it always folded things like
This is either a subtle change in the C++ compiler's behavior (very unlikely), or a latent bug in the folding logic. I am yet to reproduce this locally. More broadly, it seems like the time that this comment talks about has come, with Crossgen2 now entering wide use with its excellent support for cross-compilation. You can construct some very "interesting" scenarios if you mix R2R, inlining, tiered compilation and PGO with constant folding that's different between the AOT compiler and the Jit. Stepping back, the immediate action here would be, in my opinion, to get rid of testing in How to proceed requires a decision from the maintainers. |
@SingleAccretion I suspect it's MSVC x86 specific actually, a small repro: #include <iostream>
__declspec(noinline) float FloatMaxValue() {
return 3.40282346638528859e+38;
}
int main() {
std::cout << (unsigned)FloatMaxValue();
}
Fix #47380 |
Is there a volunteer to disable the affected tests meantime? |
* Enhance the FloatOvfToInt2 test to exercise VN paths It is not very effective right now because the "bad" combinations of types are morphed into helpers too early, but it will be helpful when we enable folding outside of the importer for those cases too. * Enhance the FloatOvfToInt2 test to cover importer It now fails on Windows x86, where many of the previous similar failures were observed. * Disable the test on Mono * Re-enable tests disabled against #13651 * Re-enable tests disabled against #51346 * Re-enable tests disabled against #47374 * Disable folding in gtFoldExprConst * Disable folding in VN * Temporarily promote the test to Pri0 * Reword the comment * Move the tests back to Pri1 Where they originally were.
Blocking clean CI on #45022. See here for the full log.
Runfo Tracking Issue: system.linq.expressions.tests.converttests.convertfloattonullableuinttest
Displaying 100 of 103 results
Build Result Summary
The text was updated successfully, but these errors were encountered: