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

[release/7.0-rc2] Fixing SpanHelpers.LastIndexOfAnyValueType to no longer create out of bounds GC refs #75885

Merged
merged 3 commits into from
Sep 20, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 20, 2022

Backport of #75857 to release/7.0-rc2

/cc @tannergooding

Customer Impact

Customers utilizing the Span<T>.LastIndexOf APIs may get invalid results. This is due to a "GC Hole" in the algorithm caused by creating a byref that points to before the managed object it was originally meant to be tracking.

Testing

In addition to the existing tests, additional manual testing was done to validate the logic no longer causes a crash under GCStress=0xC

Risk

The new logic avoids the GC hole by tracking a byref + offset rather than mutating the byref each iteration. This ensures we don't end up with an exterior byref and thus avoids the GC hole entirely.

@ghost
Copy link

ghost commented Sep 20, 2022

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

Issue Details

Backport of #75857 to release/7.0-rc2

/cc @tannergooding

Customer Impact

Testing

Risk

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Memory

Milestone: -

@tannergooding tannergooding added the Servicing-consider Issue for next servicing release review label Sep 20, 2022
@tannergooding tannergooding added this to the 7.0.0 milestone Sep 20, 2022
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 20, 2022
@danmoseley
Copy link
Member

@rainersigwald does the stack below simply mean that a child process couldn't start for some reason (not necessarily MSBuild issue)?

      Unhandled exception: System.IO.IOException: Connection timed out : 'Global\msbuild-server-launch-MmOQLObCTwIYk+VOCXULoCbHQULmwZ_RzAzo+dJyqpI'
         at System.Threading.Mutex.CreateMutexCore(Boolean initiallyOwned, String name, Boolean& createdNew)
         at Microsoft.Build.Experimental.MSBuildClient.TryLaunchServer()
         at Microsoft.Build.Experimental.MSBuildClient.Execute(CancellationToken cancellationToken)
         at Microsoft.Build.CommandLine.MSBuildClientApp.Execute(String[] commandLine, String msbuildLocation, CancellationToken cancellationToken)
         at Microsoft.Build.CommandLine.MSBuildApp.Main(String[] args)
         at Microsoft.DotNet.Cli.Utils.MSBuildForwardingAppWithoutLogging.ExecuteInProc(String[] arguments)

@rainersigwald
Copy link
Member

That is a good question. @rokonec / @AR-May / @MichalPavlik?

I'm not super familiar with this area but it looks to me like this is before we even try to create a new process. I don't know why creating a mutex would fail with a timeout--that's not one of the common failures in the docs.

@carlossanlop
Copy link
Member

cc @mmitche - We still have the above failure to figure out. This is the last RC2 PR backport to merge today.

@rainersigwald
Copy link
Member

Is the failure happening consistently? And is that MSBuild running on a just-built runtime, or the "LKG" one from the SDK?

@carlossanlop
Copy link
Member

carlossanlop commented Sep 20, 2022

I just remembered that the failure mentioned above in MSBuild is happening in other PRs. I had already opened this issue yesterday: #75867

So we can consider it unrelated to this change.

      Unhandled exception: System.IO.IOException: Connection timed out : 'Global\msbuild-server-launch-MmOQLObCTwIYk+VOCXULoCbHQULmwZ_RzAzo+dJyqpI'
         at System.Threading.Mutex.CreateMutexCore(Boolean initiallyOwned, String name, Boolean& createdNew)
         at Microsoft.Build.Experimental.MSBuildClient.TryLaunchServer()
         at Microsoft.Build.Experimental.MSBuildClient.Execute(CancellationToken cancellationToken)
         at Microsoft.Build.CommandLine.MSBuildClientApp.Execute(String[] commandLine, String msbuildLocation, CancellationToken cancellationToken)
         at Microsoft.Build.CommandLine.MSBuildApp.Main(String[] args)
         at Microsoft.DotNet.Cli.Utils.MSBuildForwardingAppWithoutLogging.ExecuteInProc(String[] arguments)

@carlossanlop
Copy link
Member

carlossanlop commented Sep 20, 2022

This other failure is also unrelated to this change. It's being tracked in this issue: #75391

Microsoft.DotNet.XUnitConsoleRunner v2.5.0 (64-bit .NET 7.0.0-rc.1.22423.16)
  Discovering: JIT.Math.XUnitWrapper (method display = ClassAndMethod, method display options = None)
  Discovered:  JIT.Math.XUnitWrapper (found 2 test cases)
  Starting:    JIT.Math.XUnitWrapper (parallel test collections = on, max threads = 2)
    JIT/Math/Functions/Functions_ro/Functions_ro.sh [FAIL]
      
      Return code:      1
      Raw output file:      /datadisks/disk1/work/B44709B0/w/A6DF0914/uploads/Reports/JIT.Math/Functions/Functions_ro/Functions_ro.output.txt
      Raw output:
      BEGIN EXECUTION
      MSBuild version 17.4.0-preview-22416-02+5d102ae37 for .NET
      /datadisks/disk1/work/B44709B0/p/wasm-test-runner/WasmTestRunner.proj : error : Failed to resolve SDK 'Microsoft.Build.NoTargets/3.5.0'. Package restore was successful but a package with the ID of "Microsoft.Build.NoTargets" was not installed.
      /datadisks/disk1/work/B44709B0/p/wasm-test-runner/WasmTestRunner.proj : error MSB4236: The SDK 'Microsoft.Build.NoTargets/3.5.0' specified could not be found.
      Test Harness Exitcode is : 1
      To run the test:
      > set CORE_ROOT=/datadisks/disk1/work/B44709B0/p
      > /datadisks/disk1/work/B44709B0/w/A6DF0914/e/JIT/Math/Functions/Functions_ro/Functions_ro.sh
      Expected: True
      Actual:   False
      Stack Trace:
           at JIT_Math._Functions_Functions_ro_Functions_ro_._Functions_Functions_ro_Functions_ro_sh()
           at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
           at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
      Output:
        
        Return code:      1
        Raw output file:      /datadisks/disk1/work/B44709B0/w/A6DF0914/uploads/Reports/JIT.Math/Functions/Functions_ro/Functions_ro.output.txt
        Raw output:
        BEGIN EXECUTION
        MSBuild version 17.4.0-preview-22416-02+5d102ae37 for .NET
        /datadisks/disk1/work/B44709B0/p/wasm-test-runner/WasmTestRunner.proj : error : Failed to resolve SDK 'Microsoft.Build.NoTargets/3.5.0'. Package restore was successful but a package with the ID of "Microsoft.Build.NoTargets" was not installed.
        /datadisks/disk1/work/B44709B0/p/wasm-test-runner/WasmTestRunner.proj : error MSB4236: The SDK 'Microsoft.Build.NoTargets/3.5.0' specified could not be found.
        Test Harness Exitcode is : 1
        To run the test:
        > set CORE_ROOT=/datadisks/disk1/work/B44709B0/p
        > /datadisks/disk1/work/B44709B0/w/A6DF0914/e/JIT/Math/Functions/Functions_ro/Functions_ro.sh
  Finished:    JIT.Math.XUnitWrapper
=== TEST EXECUTION SUMMARY ===
   JIT.Math.XUnitWrapper  Total: 2, Errors: 0, Failed: 1, Skipped: 0, Time: 4.675s

@carlossanlop
Copy link
Member

CI failures are unrelated, pre-existing: #75391
Signed off. Approved by Tactics. Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 62db96b into release/7.0-rc2 Sep 20, 2022
@carlossanlop carlossanlop deleted the backport/pr-75857-to-release/7.0-rc2 branch September 20, 2022 22:47
carlossanlop added a commit that referenced this pull request Sep 26, 2022
* Fix querying L3 cache size on osx-x64 (#75870)

Co-authored-by: Filip Navara <navara@emclient.com>

* KeyChar should be preserved for Ctrl+Letter (#75861)

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>

* Try re-enabling System.Transactions.Local tests on Arm64 (#75703)

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

* Updating inbox source generators to Roslyn 4.4 and removing polyfill approach (#75717) (#75875)

* Removed internalProperties group from proxy and tests. (#75906)

Co-authored-by: Ilona Tomkowicz <itomkowicz@microsoft.com>

* [release/7.0-rc2] Fixing SpanHelpers.LastIndexOfAnyValueType to no longer create out of bounds GC refs (#75885)

* Fixing SpanHelpers.LastIndexOfAnyValueType to no longer create out of bounds GC refs

* Apply suggestions from code review

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>

* Adjusting the comment as per PR review feedback

Co-authored-by: Tanner Gooding <tagoo@outlook.com>
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>

* Bump intellisense package for RC2 to include latest comments for Numerics (#75938)

Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com>

* [release/7.0-rc2] [Mono] Restore old code to solve the recent SpanHelpers regressions (#75996)

* bring back the old code...

* bring back more old code

* Use an ifdef around clr code instead of a separate file

* Delete SpanHelpers.Clr.cs

* Remove a remaining INumber<T> helper from mono

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Filip Navara <navara@emclient.com>
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: Jose Perez Rodriguez <joperezr@microsoft.com>
Co-authored-by: Ilona Tomkowicz <itomkowicz@microsoft.com>
Co-authored-by: Tanner Gooding <tagoo@outlook.com>
Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com>
Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com>
Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>
@ghost ghost locked as resolved and limited conversation to collaborators Oct 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Memory Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants