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

Memory Corruption on x86_64 with async closure #64966

Open
troughton opened this issue Apr 6, 2023 · 4 comments
Open

Memory Corruption on x86_64 with async closure #64966

troughton opened this issue Apr 6, 2023 · 4 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. concurrency Feature: umbrella label for concurrency language features memory safety Feature: memory safety

Comments

@troughton
Copy link
Contributor

troughton commented Apr 6, 2023

The root issue is a consistently reproducible crash which appears to be related to a concurrency/job-switching issue – details are at https://forums.swift.org/t/debugging-a-concurrency-crash/64179/.

The issue applies when building with Xcode 14.3 and targeting x86_64 macOS 13.3; the bug doesn't reproduce on ARM but that may also be because of behaviour differences in the test application. It was present in Xcode 14.2 but only when compiling without optimisations. I haven't been able to produce a reduced test case.

The application does use some unsafe constructs such as @_unsafeInheritExecutor (e.g. in methods that wrap with_Continuation), so it's possible the crash is a result of invalid/unsupported code rather than a compiler/runtime bug; however, if that's the case, it would still be useful to know the root cause if possible so I can work around it. I've tried eliminating the use of those attributes where possible and the application still crashes, so my assumption is that it's likely something else.

@troughton troughton added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. triage needed This issue needs more specific labels labels Apr 6, 2023
@troughton troughton changed the title Concurrency-Related Crash Concurrency-Related Memory Corruption Apr 12, 2023
@troughton
Copy link
Contributor Author

troughton commented Apr 12, 2023

Also posted on the forums:

I've managed to narrow down the problem, although I don't have an isolated test case; the bug seems to be fairly brittle. The issue appears to be memory corruption related to captures in an async closure (where I'm capturing a number of variables by value which were vars in the enclosing scope). Concretely, on x86_64 and not ARM, this crashes at the next await call that suspends the current task:

renderGraph.addDrawCallbackPass(renderTarget: renderTarget, 
                                reflection: ResolveAccumulatedTargetPassReflection.self, 
                                { [uvToNDC, imageUVToSceneUV, filmicToneMapUniforms, vignetteUniforms, colorGradedSceneRegionBackgroundColor] encoder in
    await Task.yield() // crash
    
}

Moving the imageUVToSceneUV to a tuple works around it (doesn't crash):

let arguments = (uvToNDC, imageUVToSceneUV)
renderGraph.addDrawCallbackPass(renderTarget: renderTarget, 
                                reflection: ResolveAccumulatedTargetPassReflection.self, 
                                { [arguments, filmicToneMapUniforms, vignetteUniforms, colorGradedSceneRegionBackgroundColor] encoder in
    let (uvToNDC, imageUVToSceneUV) = arguments
    await Task.yield() // no crash
                    
}

as does inserting a print statement for imageUVToSceneUV specifically:

renderGraph.addDrawCallbackPass(renderTarget: renderTarget, 
                                reflection: ResolveAccumulatedTargetPassReflection.self, 
                                { [uvToNDC, imageUVToSceneUV, filmicToneMapUniforms, vignetteUniforms, colorGradedSceneRegionBackgroundColor] encoder in
    print(imageUVToSceneUV)
    await Task.yield() // no crash
                   
}

but moving the print statement to after the first use of imageUVToSceneUV in the function does crash:

renderGraph.addDrawCallbackPass(renderTarget: renderTarget, 
                                reflection: ResolveAccumulatedTargetPassReflection.self, 
                                { [uvToNDC, imageUVToSceneUV, filmicToneMapUniforms, vignetteUniforms, colorGradedSceneRegionBackgroundColor] encoder in
    
    encoder.set1.uniforms = .init(clearColor: SIMD4(SIMD3(clearColor), 0.0),
                                  backgroundColor: SIMD4(colorGradedSceneRegionBackgroundColor),
                                  uvToSceneUVScale: SIMD2(imageUVToSceneUV.scale),
                                  uvToSceneUVOffset: SIMD2(imageUVToSceneUV.offset),
                                  uvToBlitTexture0UV: multiFrameAccumulationWeight < 1.0 ? previewBufferUVScale : .one,
                                  highlightColorMultiply: SIMD3(highlightColorMult),
                                  highlightColorAdditive: SIMD3(highlightColorAdd),
                                  toneMapParams: filmicToneMapUniforms,
                                  vignetteParams: vignetteUniforms)
    print(imageUVToSceneUV)
    await Task.yield() // crash     
           
}

uvToNDC and imageUVToSceneUV are RectTransform<Float>s, which are composed of two SIMD2<Float>s each; addDrawCallbackPass is defined at https://github.com/troughton/Substrate/blob/6f25f5b6606278e705144b3664a4fdfe111b3a53/Sources/Substrate/RenderGraph/RenderGraph.swift#L1000.

I can’t provide source but isolated SIL/LLVM IR may be possible.

@troughton
Copy link
Contributor Author

@ktoso Since it doesn’t seem like anyone has picked this up, would you be able to direct this issue to the right people? We have a workaround, so it’s not critical, but it also seems important that a runtime memory corruption bug like this gets addressed.

@ktoso ktoso added concurrency Feature: umbrella label for concurrency language features memory safety Feature: memory safety and removed triage needed This issue needs more specific labels labels Apr 18, 2023
@ktoso ktoso changed the title Concurrency-Related Memory Corruption Concurrency-Related Memory Corruption on x86_64 with async closure Apr 18, 2023
@ktoso
Copy link
Member

ktoso commented Apr 18, 2023

Let me sync this with a radar, but I can't comment on timelines or plans.
I've not wrapped my head around what's happening here either yet.

Thanks for the detective work so far!

rdar://108132090

@ktoso ktoso changed the title Concurrency-Related Memory Corruption on x86_64 with async closure Memory Corruption on x86_64 with async closure Apr 18, 2023
@troughton
Copy link
Contributor Author

troughton commented May 24, 2024

Just to update this (cc @ktoso): we saw this bug again today. The crash occurred only on x86_64, in both debug and release configurations, on Xcode 15.4/Swift 5.10 (swift-driver version: 1.90.11.1 Apple Swift version 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4)) and running on macOS 14.5. The fix was again to move an RGBAColor variable from a separate closure capture to be included in a tuple of arguments (i.e. [argA, argB, argC] to let arguments = (argA, argB, argC), [arguments]).

Basically, it seems like there's still a memory corruption bug not tied to the optimiser regarding multiple captures in an async closure on x86_64. The crash occurs some time after the closure has been executed when unwinding the stack out of nested async calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. concurrency Feature: umbrella label for concurrency language features memory safety Feature: memory safety
Projects
None yet
Development

No branches or pull requests

2 participants