[SR-2814] Swift does not correctly pass in multiple optional strings to C function #45418

swift-ci · 2016-10-01T16:20:41Z


Previous ID	SR-2814
Radar	None
Original Reporter	Aquis (JIRA User)
Type	Bug

Environment

Tested on macOS 10.12 (16A323) with Swift 3.0 running on Xcode 8.0 (8A218a).

Additional Detail from JIRA


Votes	2
Component/s	Compiler
Labels	Bug, SILGen
Assignee	None
Priority	Medium

md5: cb58a1980dcaff3969ab4708cb4038fa

relates to:

SR-3231 Generated C string doesn't survive until called C function returns

Issue Description:

In a C function taking in multiple strings (a.k.a. `const char *`s), passing in optional strings in Swift (a.k.a. `String?`) causes the last passed in string to appear for all arguments.

Consider the following C function:

void multiString(const char *arg0, const char *arg1, const char *arg2, const char *arg3) {
    printf("%p: %c %c %c\n", arg0, arg0[0], arg0[1], arg0[2]);
    printf("%p: %c %c %c\n", arg1, arg1[0], arg1[1], arg1[2]);
    printf("%p: %c %c %c\n", arg2, arg2[0], arg2[1], arg2[2]);
    printf("%p: %c %c %c\n", arg3, arg3[0], arg3[1], arg3[2]);
}

Calling it from Swift with the following input:

let input0: String? = "Zero"
let input1: String? = "One"
let input2: String? = "Two"
let input3: String? = "Three"

multiString(input0, input1, input2, input3)

Results in an output like:

0x101003170: T h r
0x101003170: T h r
0x101003170: T h r
0x101003170: T h r

In the correct output, the pointers would all be different and the characters would correspond to the correct input strings. Passing in non-optional strings results in the correct behaviour, as does a C function with a single string type.

The text was updated successfully, but these errors were encountered:

belkadan · 2016-10-03T18:30:50Z

Some problem with the optional-string-to-pointer conversion?

belkadan · 2016-10-03T19:21:22Z

It looks like we just throw away the buffer owner instead of keeping it alive until we're done with the string. Yikes.

bb1:                                              // Preds: bb0
  %58 = unchecked_enum_data %52 : $Optional<String>, #Optional.some!enumelt.1, loc "<stdin>":6:13, scope 1 // user: %61
  // function_ref _convertConstStringToUTF8PointerArgument<A where ...> (String) -> (AnyObject?, A)
  %59 = function_ref @_TFs40_convertConstStringToUTF8PointerArgumentuRxs8_PointerrFSSTGSqPs9AnyObject__x_ : $@convention(thin) <τ_0_0 where τ_0_0 : _Pointer> (@owned String) -> (@owned Optional<AnyObject>, @out τ_0_0), loc "<stdin>":6:13, scope 1 // user: %61
  %60 = alloc_stack $UnsafePointer<Int8>, loc "<stdin>":6:13, scope 1 // users: %65, %62, %61
  %61 = apply %59<UnsafePointer<Int8>>(%60, %58) : $@convention(thin) <τ_0_0 where τ_0_0 : _Pointer> (@owned String) -> (@owned Optional<AnyObject>, @out τ_0_0), loc "<stdin>":6:13, scope 1 // user: %64
  %62 = load %60 : $*UnsafePointer<Int8>, loc "<stdin>":6:13, scope 1 // user: %63
  %63 = enum $Optional<UnsafePointer<Int8>>, #Optional.some!enumelt.1, %62 : $UnsafePointer<Int8>, loc "<stdin>":6:13, scope 1 // user: %66
  release_value %61 : $Optional<AnyObject>, loc "<stdin>":6:13, scope 1 // id: %64
  dealloc_stack %60 : $*UnsafePointer<Int8>, loc "<stdin>":6:13, scope 1 // id: %65
  br bb2(%63 : $Optional<UnsafePointer<Int8>>), loc "<stdin>":6:13, scope 1 // id: %66

modocache · 2017-01-09T22:37:08Z

I think attempting to fix this will be a good learning opportunity for me. If someone wants to fix it quickly, or if I don't submit a pull request by Jan 22, they should feel free to snatch this task away from me.

The problem can be reduced further, by using a function that takes only two parameters instead of four:

// MyCFunction.h

#include <stdio.h>

void multiString(const char *arg0, const char *arg1) {
  printf("%p: %c %c %c\n", arg0, arg0[0], arg0[1], arg0[2]);
  printf("%p: %c %c %c\n", arg1, arg1[0], arg1[1], arg1[2]);
}

// MySwift.swift

let input0: String? = "Zero"
let input1: String? = "One"
multiString(input0, input1)

These files, compiled with swiftc -sdk `xcrun --show-sdk-path` MySwift.swift -import-objc-header MyCFunction.h, produce similarly incorrect output:

0x7fc62c40b7a0: O n e
0x7fc62c40b7a0: O n e

(It's important to note, however, that it's not 100% guaranteed the above incorrect output will be produced. I noticed that around 1 in 10 invocations actually produced correct output. I guess this isn't entirely deterministic right now.)

I emitted SIL using swiftc -sdk `xcrun --show-sdk-path` MySwift.swift -import-objc-header MyCFunction.h -emit-sil, and found the basic block that @belkadan mentioned above. To test his hypothesis that the problem is due to a use after free, I wanted to try modifying the SIL code, then passing it into the compiler. Unfortunately, I hit an assertion failure:

swiftc -sdk `xcrun --show-sdk-path` MySwift.sil -import-objc-header MyCFunction.h -emit-sil
Assertion failed: ((Qualifier != StoreOwnershipQualifier::Unqualified) || F.hasUnqualifiedOwnership() && "Unqualified inst in qualified function"), function createStore, file /Users/bgesiak/GitHub/apple/swift/include/swift/SIL/SILBuilder.h, line 500.
Stack dump:
0.  Program arguments: /Users/bgesiak/GitHub/apple/build/Ninja-ReleaseAssert+swift-DebugAssert/swift-macosx-x86_64/bin/swift -frontend -emit-sil -primary-file MySwift-mangled.sil -target x86_64-apple-macosx10.9 -enable-objc-interop -sdk /Applications/Xcode_8.1_fb.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk -import-objc-header MyCFunction.h -color-diagnostics -module-name main -o -
1.  With parser at source location: MySwift-mangled.sil:35:3
[1]    55375 abort       -sdk `xcrun --show-sdk-path` MySwift-mangled.sil -import-objc-header

Not being able to modify the SIL directly, I tried modifying the lib/SIL source code. The following change "fixes" the problem:

diff --git a/lib/SIL/TypeLowering.cpp b/lib/SIL/TypeLowering.cpp
index 3c7379e..bde9ead 100644
--- a/lib/SIL/TypeLowering.cpp
+++ b/lib/SIL/TypeLowering.cpp
@@ -872,7 +872,7 @@ namespace {
         B.createDestroyValue(loc, value);
         return;
       }
-      B.emitReleaseValueAndFold(loc, value);
+      // B.emitReleaseValueAndFold(loc, value);
     }

     void emitLoweredDestroyValue(SILBuilder &B, SILLocation loc, SILValue value,

After having made the above modification, swiftc -sdk `xcrun --show-sdk-path` MySwift.swift -import-objc-header MyCFunction.h && ./MySwift deterministically prints:

0x7fe8f3d0bbc0: Z e r
0x7fe8f3d0b610: O n e

The SIL was identical, except the calls to release_value in bb1 and bb3 (the basic blocks that convert the Swift.String into C const char * strings) are not present in the "fixed" version.

Of course, the "fix" above probably leaks memory like crazy. Next, I'll read the source code to learn about the reasoning behind emitting release_value at that location when converting Swift strings to C strings, and determine a way to keep it from happening until after the strings will no longer be used.

modocache · 2017-01-11T02:05:38Z

Some updates:

I noticed that -emit-silgen produced SIL that indicated that the same error was occurring. Instead of immediately calling release_value, it called destroy_value. (According to Twitter, the two are the same, but destroy_value is "lowered" as release_value because lib/IRGen expects that symbol.) So I deduced that the problem is in the generation of "raw" SIL, as opposed to it existing in the SIL optimization passes or "lowering" to canonical SIL.

The destroy_value is emitted in LoadableEnumTypeLowering::emitDestroyValue. I placed a breakpoint there, and it fired once for each of the two basic blocks I mention in my last comment.

I find it helpful to read through the backtrace, reversed, to see what's going on and how the Swift compiler gets from int main() to the area I'm looking at. Here's the trace:

main(argc_=12, argv_=...) + 4811 at driver.cpp:160
swift::performFrontend(Args=ArrayRef<const char *> @ ..., Argv0="/Users/bgesiak/GitHub/apple/build/Ninja-ReleaseAssert+swift-DebugAssert/swift-macosx-x86_64/bin/swift", MainAddr=..., observer=...) + 13279 at FrontendTool.cpp:969
performCompile(Instance=..., Invocation=..., Args=ArrayRef<const char *> @ ..., ReturnValue=..., observer=...) + 8926 at FrontendTool.cpp:523
swift::performSILGeneration(sf=..., options=..., startElem=Optional<unsigned int> @ ..., makeModuleFragile=false) + 135 at SILGen.cpp:1509
swift::SILModule::constructSIL(mod=..., options=..., SF=..., startElem=Optional<unsigned int> @ ..., makeModuleFragile=false, isWholeModule=false) + 1037 at SILGen.cpp:1445
swift::Lowering::SILGenModule::emitSourceFile(this=..., sf=..., startElem=0) + 263 at SILGen.cpp:1409
swift::ASTVisitor<swift::Lowering::SILGenModule, void, void, void, void, void, void>::visit(this=..., D=...) + 188 at DeclNodes.def:93
swift::Lowering::SILGenModule::visitTopLevelCodeDecl(this=..., td=...) + 668 at SILGen.cpp:1219
swift::Lowering::SILGenFunction::emitIgnoredExpr(this=..., E=...) + 887 at SILGenExpr.cpp:3622
swift::Lowering::SILGenFunction::emitRValue(this=..., E=..., C=SGFContext @ ...) + 201 at SILGenExpr.cpp:3572
swift::ASTVisitor<RValueEmitter, swift::Lowering::RValue, void, void, void, void, void, swift::Lowering::SGFContext>::visit(this=..., E=..., AA=SGFContext @ ...) + 3549 at ExprNodes.def:112
swift::ASTVisitor<RValueEmitter, swift::Lowering::RValue, void, void, void, void, void, swift::Lowering::SGFContext>::visitCallExpr(this=..., E=..., AA=SGFContext @ ...) + 74 at ExprNodes.def:112
RValueEmitter::visitApplyExpr(this=..., E=..., C=SGFContext @ ...) + 66 at SILGenExpr.cpp:334
swift::Lowering::SILGenFunction::emitApplyExpr(this=..., e=..., c=SGFContext @ ...) + 132 at SILGenApply.cpp:4597
CallEmission::apply(this=..., C=SGFContext @ ...) + 6953 at SILGenApply.cpp:4376
CallSite::emit(this=..., gen=..., origParamType=AbstractionPattern @ ..., lowering=..., args=..., inoutArgs=..., foreignError=..., foreignSelf=...)::ParamLowering&, llvm::SmallVectorImpl<swift::Lowering::ManagedValue>&, llvm::SmallVectorImpl<std::__1::pair<swift::Lowering::LValue, swift::SILLocation> >&, llvm::Optional<swift::ForeignErrorConvention> const&, swift::ImportAsMemberStatus const&) && + 666 at SILGenApply.cpp:4063
ArgEmitter::emitTopLevel(this=..., arg=..., origParamType=AbstractionPattern @ ...) + 128 at SILGenApply.cpp:2866
ArgEmitter::emit(this=..., arg=..., origParamType=AbstractionPattern @ ...) + 179 at SILGenApply.cpp:2876
ArgEmitter::emitExpanded(this=..., arg=..., origParamType=AbstractionPattern @ ...) + 1935 at SILGenApply.cpp:3016
ArgEmitter::emit(this=..., arg=..., origParamType=AbstractionPattern @ ...) + 1779 at SILGenApply.cpp:2951
ArgEmitter::emitDirect(this=..., arg=..., loweredSubstArgType=SILType @ ..., origParamType=AbstractionPattern @ ..., param=SILParameterInfo @ ...) + 650 at SILGenApply.cpp:3141
swift::Lowering::ArgumentSource::getAsSingleValue(this=..., gen=..., C=SGFContext @ ...) && + 631 at ArgumentSource.cpp:117
swift::Lowering::SILGenFunction::emitRValueAsSingleValue(this=..., E=..., C=SGFContext @ ...) + 82 at SILGenExpr.cpp:3628
swift::Lowering::SILGenFunction::emitRValue(this=..., E=..., C=SGFContext @ ...) + 201 at SILGenExpr.cpp:3572
swift::ASTVisitor<RValueEmitter, swift::Lowering::RValue, void, void, void, void, void, swift::Lowering::SGFContext>::visit(this=..., E=..., AA=SGFContext @ ...) + 3249 at ExprNodes.def:107
RValueEmitter::visitOptionalEvaluationExpr(this=..., E=..., C=SGFContext @ ...) + 1948 at SILGenExpr.cpp:3073
swift::Lowering::Scope::pop(this=...) + 128 at Scope.h:58
swift::Lowering::Scope::popImpl(this=...) + 303 at Scope.h:41
swift::Lowering::CleanupManager::endScope(this=..., depth=(Depth = 0), l=CleanupLocation @ ...) + 222 at Cleanup.cpp:94
swift::Lowering::CleanupManager::emitCleanups(this=..., depth=(Depth = 0), l=CleanupLocation @ ..., popCleanups=true) + 452 at Cleanup.cpp:75
ReleaseValueCleanup::emit(this=..., gen=..., l=CleanupLocation @ ...) + 266 at SILGenDecl.cpp:229
swift::SILBuilder::emitDestroyValueOperation(this=..., Loc=SILLocation @ ..., v=SILValue @ ...) + 267 at SILBuilder.h:1586
LoadableEnumTypeLowering::emitDestroyValue(this=..., B=..., loc=SILLocation @ ..., value=SILValue @ ...) const + 16 at TypeLowering.cpp:871

Unfortunately, since I'm not experienced with SIL in general, I'm not sure what part of this stacktrace is "incorrect." Is it the fact that SILGenFunction::emitIgnoredExpr is being called? Something "ignored" strikes me as the sort of thing that would be released immediately. Or is it something else?

As a next step, I plan on looking at what happens when multiString is invoked with non-optional strings, and what happens when multiString is implemented as a Swift function instead of in C. How does the generated SIL compare? How do the backtraces differ?

I'm on a bit of a wild goose chase at this point, so any hints would be helpful! Otherwise I'll keep plugging away.

jtbandes · 2017-01-11T07:58:39Z

Interesting investigation so far. I don't know much about SIL/Gen/Lowering et al. either, but here are some thoughts from looking at what you've written and reproducing it myself:

I find it easer to use the debugger when there's only one call to emitDestroyValue. The same SIL problem occurs with a 1-argument function, even if running the resulting program might not exhibit the nondeterministic symptoms.

I don't think emitIgnoredExpr is a problem; this seems to happen early in the call stack no matter what is being emitted. If you look at StmtEmitter::visitBraceStmt or SILGenModule::visitTopLevelCodeDecl, you'll see that emitIgnoredExpr is simply the alternative to visiting a statement. (I tried discarding the function result with _ = to see if that changed anything, and it didn't — it just adds another AST node, a DiscardAssignmentExpr.)

I was a bit surprised to find that OptionalEvaluationExpr appears in this expression given that there is no optional chaining or binding. I tried adding _Nullable to the function param in the header and that didn't change this either. OptionalEvaluationExpr is the one responsible for calling scope.pop() which results in emitting this destroy. One question worth answering is whether OptionalEvaluationExpr should even be there?

From a quick glance at the SIL when using a non-optional value let input0: String, the problem does not occur (the destroy happens after the function apply). There's also only one block and no branch instructions. I don't know what the branches are for but maybe they interact badly with this scope.pop() in the RValueEmitter.

gottesmm · 2017-01-11T22:11:44Z

My feeling is that the problem here is that temporaries that are not forwarded should live until the end of an apply. I think this probably has to do with argument emission.

That being said, I am going to be making changes in this area, so I would suggest either waiting or at least give me a heads up with what you are planning.

Another thing, emtiReleaseValueAndFold is no longer used by SILGen today. If it is, then it is a bug.

modocache · 2017-03-08T19:17:10Z

I don't have a ton of time to work on this anymore. Still, it was a fun way to learn more about SILGen, so thank you!

martinr448 · 2022-09-02T18:38:17Z

Has this been fixed in the meantime? I cannot reproduce the problem anymore with Xcode 13.4.1/Swift 5.6.1. The output of the test program is

0x100711740: Z e r
0x100712300: O n e
0x100712330: T w o
0x1007d4920: T h r

as one would expect.

swift-ci transferred this issue from apple/swift-issues Apr 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SR-2814] Swift does not correctly pass in multiple optional strings to C function #45418

[SR-2814] Swift does not correctly pass in multiple optional strings to C function #45418

swift-ci commented Oct 1, 2016

belkadan commented Oct 3, 2016

belkadan commented Oct 3, 2016

modocache mannequin commented Jan 9, 2017

modocache mannequin commented Jan 11, 2017

jtbandes commented Jan 11, 2017

gottesmm commented Jan 11, 2017

modocache mannequin commented Mar 8, 2017

martinr448 commented Sep 2, 2022

[SR-2814] Swift does not correctly pass in multiple optional strings to C function #45418

[SR-2814] Swift does not correctly pass in multiple optional strings to C function #45418

Comments

swift-ci commented Oct 1, 2016

belkadan commented Oct 3, 2016

belkadan commented Oct 3, 2016

modocache mannequin commented Jan 9, 2017

modocache mannequin commented Jan 11, 2017

jtbandes commented Jan 11, 2017

gottesmm commented Jan 11, 2017

modocache mannequin commented Mar 8, 2017

martinr448 commented Sep 2, 2022