Skip to content
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

Fix assertion at System.Numerics.BigInteger.Parse #96746

Closed
wants to merge 3 commits into from

Conversation

kzrnm
Copy link
Contributor

@kzrnm kzrnm commented Jan 10, 2024

I fixed the following assertions.

  • BigInteger.Parse(new string('1',33))
    • I changed buffer reading in TryParseBigIntegerBinaryNumberStyle to be the same as in TryParseBigIntegerHexNumberStyle.
  • BigInteger.TryParse(null, out _)
    • null check

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 10, 2024
@kzrnm kzrnm force-pushed the fix/BigIntegerParse branch from 0459b21 to 49bb7c4 Compare January 10, 2024 09:47
@ghost
Copy link

ghost commented Jan 10, 2024

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

I fixed the following assertions.

  • 0459b21c93b BigInteger.Parse(new string('1',33))
    • I changed buffer reading in TryParseBigIntegerBinaryNumberStyle to be the same as in TryParseBigIntegerHexNumberStyle.
  • 3050630a6c8 BigInteger.TryParse(null, out _)
    • null check
Author: kzrnm
Assignees: -
Labels:

area-System.Numerics, community-contribution

Milestone: -

@huoyaoyuan
Copy link
Member

This conflicts with #95543. Can you test if the new implementation handles these cases correctly?

@kzrnm kzrnm force-pushed the fix/BigIntegerParse branch 2 times, most recently from a843699 to e9a2207 Compare January 10, 2024 10:31
Copy link
Contributor Author

@kzrnm kzrnm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@huoyaoyuan
I merged this branch with #95543 and tested it. Then, it failed in the part that had not been changed.

https://github.com/kzrnm/dotnet-runtime/commits/4285bf8eff739dce77735e58839f73661e6513b8

dotnet build /t:Test /p:XUnitOptions="-class System.Numerics.Tests.parseTest" /p:Outerloop=true
$ git rev-parse HEAD
4285bf8eff739dce77735e58839f73661e6513b8
$ dotnet build /t:Test /p:XUnitOptions="-class System.Numerics.Tests.parseTest" /p:Outerloop=true
MSBuild のバージョン 17.8.3+195e7f5a3 (.NET)
  復元対象のプロジェクトを決定しています...
  復元対象のすべてのプロジェクトは最新です。
  TestUtilities -> /repo/dotnet/runtime/artifacts/bin/TestUtilities/Debug/net8.0/TestUtilities.dll
  ILLink.RoslynAnalyzer -> /repo/dotnet/runtime/artifacts/bin/ILLink.RoslynAnalyzer/Debug/netstandard2.0/ILLink.RoslynAnalyzer.dll
  ILLink.CodeFixProvider -> /repo/dotnet/runtime/artifacts/bin/ILLink.CodeFixProvider/Debug/netstandard2.0/ILLink.CodeFixProvider.dll
  Mono.Linker -> /repo/dotnet/runtime/artifacts/bin/Mono.Linker/ref/Debug/net8.0/illink.dll
  Mono.Linker -> /repo/dotnet/runtime/artifacts/bin/Mono.Linker/Debug/net8.0/illink.dll
  ILLink.Tasks -> /repo/dotnet/runtime/artifacts/bin/ILLink.Tasks/Debug/net8.0/ILLink.Tasks.dll
  System.Runtime -> /repo/dotnet/runtime/artifacts/bin/System.Runtime/ref/Debug/net9.0/System.Runtime.dll
  System.Runtime.Numerics -> /repo/dotnet/runtime/artifacts/bin/System.Runtime.Numerics/ref/Debug/net9.0/System.Runtime.Numerics.dll
  System.Runtime.Numerics -> /repo/dotnet/runtime/artifacts/bin/System.Runtime.Numerics/Debug/net9.0/System.Runtime.Numerics.dll
  System.Runtime.Numerics.Tests -> /repo/dotnet/runtime/artifacts/bin/System.Runtime.Numerics.Tests/Debug/net9.0/System.Runtime.Numerics.Tests.dll
  ========================= Begin custom configuration settings ==============================
  export __TestArchitecture=arm64
  export __IsXUnitLogCheckerSupported=1
  ========================== End custom configuration settings ===============================
  ----- start Wed Jan 10 19:57:59 JST 2024 =============== To repro directly: =====================================================
  pushd /repo/dotnet/runtime/artifacts/bin/System.Runtime.Numerics.Tests/Debug/net9.0
  /repo/dotnet/runtime/artifacts/bin/testhost/net9.0-osx-Debug-arm64/dotnet exec --runtimeconfig System.Runtime.Numerics.Tests.runtimeconfig.json --depsfile System.Runtime.Numerics.Tests.deps.json xunit.console.dll System.Runtime.Numerics.Tests.dll -xml testResults.xml -nologo -notrait category=failing -class System.Numerics.Tests.parseTest 
  popd
  ===========================================================================================================
  ~/workspace/dotnet/dotnet-runtime/artifacts/bin/System.Runtime.Numerics.Tests/Debug/net9.0 ~/workspace/dotnet/dotnet-runtime/src/libraries/System.Runtime.Numerics/tests
    Discovering: System.Runtime.Numerics.Tests (method display = ClassAndMethod, method display options = None)
    Discovered:  System.Runtime.Numerics.Tests (found 5 of 564 test cases)
    Starting:    System.Runtime.Numerics.Tests (parallel test collections = on, max threads = 8)
      System.Numerics.Tests.parseTest.RunParseToStringTests(culture: ) [FAIL]
        Assert.Equal() Failure: Strings differ
                        ↓ (pos 5)
        Expected: "69910581945023274333765593742327408845247"···
        Actual:   "69910993429426825567413059740872573889186"···
                        ↑ (pos 5)
        Stack Trace:
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(1124,0): at System.Numerics.Tests.parseTest.Eval(BigInteger x, String expected)
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(673,0): at System.Numerics.Tests.parseTest.VerifyParseToString(String num1, NumberStyles ns, Boolean failureNotExpected, String expected)
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(636,0): at System.Numerics.Tests.parseTest.VerifyParseToString(String num1, NumberStyles ns, Boolean failureNotExpected)
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(356,0): at System.Numerics.Tests.parseTest.VerifyBinaryNumberStyles(NumberStyles ns, Random random)
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(61,0): at System.Numerics.Tests.parseTest.<>c__DisplayClass4_0.<RunParseToStringTests>g__Test|0()
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(39,0): at System.Numerics.Tests.parseTest.RunParseToStringTests(CultureInfo culture)
             at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
          /repo/dotnet/runtime/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.CoreCLR.cs(36,0): at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
          /repo/dotnet/runtime/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.cs(178,0): at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)
        Assert.Equal() Failure: Strings differ
                             ↓ (pos 10)
        Expected: "47415714611880159672943727301900602132674"···
        Actual:   "47415714615699526003274165560488192668529"···
                             ↑ (pos 10)
      System.Numerics.Tests.parseTest.RunParseToStringTests(culture: uk-UA) [FAIL]
        Stack Trace:
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(1124,0): at System.Numerics.Tests.parseTest.Eval(BigInteger x, String expected)
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(673,0): at System.Numerics.Tests.parseTest.VerifyParseToString(String num1, NumberStyles ns, Boolean failureNotExpected, String expected)
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(636,0): at System.Numerics.Tests.parseTest.VerifyParseToString(String num1, NumberStyles ns, Boolean failureNotExpected)
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(356,0): at System.Numerics.Tests.parseTest.VerifyBinaryNumberStyles(NumberStyles ns, Random random)
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(61,0): at System.Numerics.Tests.parseTest.<>c__DisplayClass4_0.<RunParseToStringTests>g__Test|0()
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(39,0): at System.Numerics.Tests.parseTest.RunParseToStringTests(CultureInfo culture)
             at InvokeStub_parseTest.RunParseToStringTests(Object, Span`1)
             at System.Reflection.MethodBaseInvoker.InvokeWithOneArg(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
        Assert.Equal() Failure: Strings differ
                    ↓ (pos 1)
        Expected: "42227368757185021250022641083366759512028"···
        Actual:   "43610323456904365678486163459101075441883"···
                    ↑ (pos 1)
        Stack Trace:
      System.Numerics.Tests.parseTest.RunParseToStringTests(culture: en-US) [FAIL]
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(1124,0): at System.Numerics.Tests.parseTest.Eval(BigInteger x, String expected)
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(673,0): at System.Numerics.Tests.parseTest.VerifyParseToString(String num1, NumberStyles ns, Boolean failureNotExpected, String expected)
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(636,0): at System.Numerics.Tests.parseTest.VerifyParseToString(String num1, NumberStyles ns, Boolean failureNotExpected)
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(356,0): at System.Numerics.Tests.parseTest.VerifyBinaryNumberStyles(NumberStyles ns, Random random)
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(61,0): at System.Numerics.Tests.parseTest.<>c__DisplayClass4_0.<RunParseToStringTests>g__Test|0()
          /repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/BigInteger/parse.cs(39,0): at System.Numerics.Tests.parseTest.RunParseToStringTests(CultureInfo culture)
             at InvokeStub_parseTest.RunParseToStringTests(Object, Span`1)
             at System.Reflection.MethodBaseInvoker.InvokeWithOneArg(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    Finished:    System.Runtime.Numerics.Tests
  === TEST EXECUTION SUMMARY ===
     System.Runtime.Numerics.Tests  Total: 18, Errors: 0, Failed: 3, Skipped: 0, Time: 15.547s
  ~/workspace/dotnet/dotnet-runtime/src/libraries/System.Runtime.Numerics/tests
  ----- end Wed Jan 10 19:58:25 JST 2024 ----- exit code 1 ----------------------------------------------------------
  ulimit -c value: 0
/repo/dotnet/runtime/eng/testing/tests.targets(198,5): error : One or more tests failed while running tests from 'System.Runtime.Numerics.Tests'. Please check /repo/dotnet/runtime/artifacts/bin/System.Runtime.Numerics.Tests/Debug/net9.0/testResults.xml for details! [/repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/System.Runtime.Numerics.Tests.csproj]

ビルドに失敗しました。

/repo/dotnet/runtime/eng/testing/tests.targets(198,5): error : One or more tests failed while running tests from 'System.Runtime.Numerics.Tests'. Please check /repo/dotnet/runtime/artifacts/bin/System.Runtime.Numerics.Tests/Debug/net9.0/testResults.xml for details! [/repo/dotnet/runtime/src/libraries/System.Runtime.Numerics/tests/System.Runtime.Numerics.Tests.csproj]
    0 個の警告
    1 エラー

経過時間 00:00:30.34

I added comment to #95543 (comment)

@kzrnm kzrnm force-pushed the fix/BigIntegerParse branch from e9a2207 to d1924de Compare January 10, 2024 14:23
@tannergooding
Copy link
Member

#95543 has had another update to resolve the case you called out, is there any others you've found that aren't handled correctly or can this PR be closed in favor of the other since it is also a perf improvement?

@kzrnm kzrnm force-pushed the fix/BigIntegerParse branch from d1924de to 70f4d9b Compare January 13, 2024 03:24
@kzrnm kzrnm force-pushed the fix/BigIntegerParse branch 2 times, most recently from c009000 to 9529565 Compare January 13, 2024 03:30
@kzrnm
Copy link
Contributor Author

kzrnm commented Jan 13, 2024

I think the null check in TryParse is a different issue than the change in #95543. I will try to create another PR on that part.

@kzrnm kzrnm closed this Jan 13, 2024
@kzrnm kzrnm deleted the fix/BigIntegerParse branch January 13, 2024 03:31
@github-actions github-actions bot locked and limited conversation to collaborators Feb 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Numerics community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants