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

Incorrect nocapture attribute for modify accessor, causing miscompilation (runtime crash) #73926

Open
kyulee-com opened this issue May 27, 2024 · 4 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. crash Bug: A crash, i.e., an abnormal termination of software SILGen Area → compiler: The SIL generation stage

Comments

@kyulee-com
Copy link

Description

We experienced a runtime crash attributed to a hoisted load for utf8ToString in the SwiftProtobuf library. Initially suspecting a loop optimizer issue, I reported it here: LLVM Project Issue #93110. Based on the feedback, it seems that $ss7UnicodeO4UTF8O13ForwardParserVs10_UTFParserssAGP7_buffers11_UIntBufferVy8Encoding_8CodeUnitQZGvMTW returns a nocapture pointer. This is demangled as protocol witness for Swift._UTFParser._buffer.modify : Swift._UIntBuffer<A.Encoding.CodeUnit> in conformance Swift.Unicode.UTF8.ForwardParser : Swift._UTFParser in Swift.
The relevant code can be found in the Swift repository: UTF8.swift Code.

Reproduction

To isolate the issue, I used the following simplified code snippet and generated similar problematic code:

public struct ForwardParser: Sendable {
  public var _buffer: _UIntBuffer<UInt8>
}

Command used:

$ swift-frontend -emit-ir repro.swift -o -

Generated IR snippet:

...
; Function Attrs: noinline
define swiftcc { ptr, ptr } @"$s5repro13ForwardParserV7_buffers11_UIntBufferVys5UInt8VGvM"(ptr noalias dereferenceable(32) %0, ptr nocapture swiftself dereferenceable(5) %1) #2 {
entry: 
  %._buffer = getelementptr inbounds %T5repro13ForwardParserV, ptr %1, i32 0, i32 0
  %2 = insertvalue { ptr, ptr } poison, ptr @"$s5repro13ForwardParserV7_buffers11_UIntBufferVys5UInt8VGvM.resume.0", 0
  %3 = insertvalue { ptr, ptr } %2, ptr %._buffer, 1
  ret { ptr, ptr } %3
}

Stack dump

Can't provide a runtime crash trace on our app.

Expected behavior

The nocapture attribute on the passed argument %1 seems to be violated as it is saved to a struct and returned in %3, contradicting the nocapture semantics detailed here: LLVM Pointer Capture.
In the original issue reported LLVM Project Issue #93110, the problem likely occurs at the call site in the caller due to aggressive loop optimization, which misinterprets the nocapture attribute, leading to incorrect load hoisting.
It seems we may reconsider the nocapture annotation for the first property of the modify accessor, as its address might be aliased with the passed this pointer.

Environment

$ swift --version
Swift version 6.0-dev (LLVM e4f0051da337219, Swift c0af25c)
Target: x86_64-apple-macosx14.0

Additional information

No response

@kyulee-com kyulee-com added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. crash Bug: A crash, i.e., an abnormal termination of software triage needed This issue needs more specific labels labels May 27, 2024
@tbkka
Copy link
Contributor

tbkka commented May 29, 2024

CC: @drexin @aschwaighofer

@aschwaighofer
Copy link
Contributor

aschwaighofer commented May 29, 2024

Ah, interesting. Yes, the nocapture attribute is not valid after Coro splitting. Coro splitting will take values "live across" a suspension point and store them into the coroutine frame. Coro splitting will split functions at suspension point into multiple functions. If the original parameter value is "live" in the second function it was spilled to the coroutine frame and the attribute is no longer valid.

LLVM's Coro splitting pass should remove the attribute.

@aschwaighofer
Copy link
Contributor

In both splitAsyncCoroutine and splitRetconCoroutine we should iterate over the function's parameter attributes and remove the nocapture attribute.

https://github.com/llvm/llvm-project/blob/799316ff26cc82d60f276dc62c4a69b5bba1aef3/llvm/lib/Transforms/Coroutines/CoroSplit.cpp#L1677

https://github.com/llvm/llvm-project/blob/799316ff26cc82d60f276dc62c4a69b5bba1aef3/llvm/lib/Transforms/Coroutines/CoroSplit.cpp#L1772

@kyulee-com
Copy link
Author

In both splitAsyncCoroutine and splitRetconCoroutine we should iterate over the function's parameter attributes and remove the nocapture attribute.

https://github.com/llvm/llvm-project/blob/799316ff26cc82d60f276dc62c4a69b5bba1aef3/llvm/lib/Transforms/Coroutines/CoroSplit.cpp#L1677

https://github.com/llvm/llvm-project/blob/799316ff26cc82d60f276dc62c4a69b5bba1aef3/llvm/lib/Transforms/Coroutines/CoroSplit.cpp#L1772

I'm not familiar with this pass. Do you think we should conservatively remove the nocapture attribute from each parameter? I believe we also need to update the attributes of the arguments in the call instructions to these functions. Since this is a function pass, it seems we might require a module pass although we could simply update the users (call instructions) of the functions in place.

@hborla hborla added SILGen Area → compiler: The SIL generation stage and removed triage needed This issue needs more specific labels labels Jul 14, 2024
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. crash Bug: A crash, i.e., an abnormal termination of software SILGen Area → compiler: The SIL generation stage
Projects
None yet
Development

No branches or pull requests

4 participants