swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.53k stars 10.35k forks source link

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

Open kyulee-com opened 5 months ago

kyulee-com commented 5 months ago

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 c0af25c0699f886) Target: x86_64-apple-macosx14.0

Additional information

No response

tbkka commented 5 months ago

CC: @drexin @aschwaighofer

aschwaighofer commented 5 months ago

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 commented 5 months ago

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 commented 4 months ago

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.