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

[SR-4393] Miscompilation: object modified after being freed (Swift 3.1 regression) #46972

Closed
NachoSoto opened this issue Mar 28, 2017 · 14 comments
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself optimized only Flag: An issue whose reproduction requires optimized compilation

Comments

@NachoSoto
Copy link
Contributor

Previous ID SR-4393
Radar rdar://problem/31482288
Original Reporter @NachoSoto
Type Bug
Status Resolved
Resolution Done

Attachment: Download

Environment

Apple Swift version 3.1 (swiftlang-802.0.48 clang-802.0.38)
Target: x86_64-apple-macosx10.9

Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Bug, 3.1Regression, OptimizedOnly
Assignee @eeckstein
Priority Medium

md5: f6a66d588871a047e6ed660ea42d4841

is duplicated by:

  • SR-4497 Memory issue causes crash when application built with optimization turned on.
  • SR-4569 Crash due to memory issue while using whole optimization
  • SR-4579 String.append method causes malloc error, but appending by "+=" does not

Issue Description:

main.swift

import Test
import Foundation

enum T: String {
    case a = "a"
    case b = "b"
}

func f(_ components: URLComponents) -> [T : String] {
    return components.queryItems?
        .flatMap { item -> (T, String)? in
            if let parameter = T(rawValue: item.name) {
                return (parameter, item.value ?? "")
            } else {
                return nil
            }
        }
        .uniqueIndex(allowDuplicateKeys: true) { $0.0 }
        .mapValues { $0.1 }
        ?? [:]
}

let c: URLComponents = {
    var c = URLComponents()
    c.queryItems = [
        URLQueryItem(name: T.a.rawValue, value: "1"),
        URLQueryItem(name: T.a.rawValue, value: "2"),
        URLQueryItem(name: T.b.rawValue, value: "1"),
        URLQueryItem(name: "test", value: "1")
    ]
    
    return c
}()
let x = f(c)

print(x)

lib.swift

public extension Sequence {
    func uniqueIndex<K: Hashable>(
        allowDuplicateKeys: Bool = false,
        _ key: (Iterator.Element) -> K
    ) -> [K : Iterator.Element] {
        var result: [K : Iterator.Element] = [:]
        
        for item in self {
            let key = key(item)
            
            guard result.updateValue(item, forKey: key) == nil || allowDuplicateKeys else {
                fatalError("Duplicated key found: \(key)")
            }
        }
        
        return result
    }
}

public extension Dictionary {
    func mapValues<NewValue>(_ transformer: (Value) -> NewValue) -> [Key : NewValue] {
        var result = [Key: NewValue](minimumCapacity: self.count)
        
        for (key, value) in self {
            result[key] = transformer(value)
        }
        
        return result
    }
}

xcrun -sdk macosx swiftc -module-name Test -emit-module -emit-library -O -wmo lib.swift
xcrun swiftc -target x86_64-apple-macosx10.10 -sdk $(xcrun --show-sdk-path --sdk macosx) -I "." -L "." -lTest -O main.swift

This is a reduced version of the real crash, which I ran with the address sanitizer, and this is the full output: https://gist.github.com/NachoSoto/8d991d1dec478eb0e1fc4f2b294e2cba.

@jckarter
Copy link
Member

@swift-ci create

@adellibovi
Copy link
Contributor

Looks like that what cause the crash is something related with

return (parameter, item.value ?? "")

Workaround:

if let value = item.value {
  return (parameter, value)
} else {
  return (parameter, "")
}

@adellibovi
Copy link
Contributor

If it could help this is a shorter version of (probably) the same crash:

import Foundation

enum T: String {
  case a
}

var c = URLComponents()
c.queryItems = [
  URLQueryItem(name: "a", value: "1"),
  URLQueryItem(name: "a", value: "1")
]

c.queryItems?.flatMap { item -> (T, String)? in
  if let parameter = T(rawValue: item.name) {
    return (parameter, item.value ?? "")
  } else {
    return nil
  }
}

@atrick
Copy link
Member

atrick commented Apr 1, 2017

This is a bug in RedundantLoadElimination. It's eliminating loads of the array storage pointer across reallocation of the array. I've been working on the general debug-ability of this pass.

@atrick
Copy link
Member

atrick commented Apr 5, 2017

The problem may be in escape analysis.

We have an Array copy method taking a local array %5 as its argument.

  // function_ref specialized Array._copyToNewBuffer(oldCount : Int) -> ()
  %172 = function_ref 
  %173 = apply %172(%160, %5)
       : $@convention(method) (Int, @inout Array<(StringEnum, String)>) -> ()

We then have an address-type block argument %151:

bb22(%151 : $*Builtin.BridgeObject)

Which is derived from the same local array:

  %99 = struct_element_addr %5
      : $*Array<(StringEnum, String)>, #Array._buffer
  %100 = struct_element_addr %99
      : $*_ArrayBuffer<(StringEnum, String)>, #_ArrayBuffer._storage
  %101 = struct_element_addr %100
      : $*_BridgeStorage<_ContiguousArrayStorageBase, _NSArrayCore>, #_BridgeStorage.rawValue

  br bb21(%101 : $*Builtin.BridgeObject)

bb21(%137 : $*Builtin.BridgeObject)

  br bb22(%137 : $*Builtin.BridgeObject)

bb22(%151 : $*Builtin.BridgeObject)

However, the connection graph does not think apply is a use point.

I'll work on hand-coding a SIL test case.

@atrick
Copy link
Member

atrick commented Apr 5, 2017

Helpful RLE debugging support should be merged in soon:
#8568

I've reduced this to a .sil test case. See testrle.sil. It could
be reduced further but I didn't want to change any of the
callee effects until we definitely have a fix.

See the RLE output in testrle.out.sil.

These two loads are being eliminated:

bb12:                                             // Preds: bb11 bb10
  %63 = load %49 : $*Builtin.BridgeObject
  %64 = load %49 : $*Builtin.BridgeObject
  cond_br undef, bb13, bb2

FORWARD   %63 = argument of bb12 : $Builtin.BridgeObject
  to  %64 = load %49 : $*Builtin.BridgeObject
FORWARD   %63 = argument of bb12 : $Builtin.BridgeObject
  to  %65 = load %49 : $*Builtin.BridgeObject

...even though bb11 overwrites the memory during Array.copyToNewBuffer.

The current suspect is:
EscapeAnalysis::canObjectOrContentEscapeTo(SILValue V, FullApplySite FAS)

It seems the the apply should be a use point of the local Array object, but it is not.

I'm waiting for a build before I can debug further. Meanwhile I'll ask Erik to take a look.

@atrick
Copy link
Member

atrick commented Apr 5, 2017

Reproducer:

sil-opt ./testrle.sil -assume-parsing-unqualified-ownership-sil -redundant-load-elim -debug-only=sil-redundant-load-elim

@NachoSoto
Copy link
Contributor Author

Are you adding the Swift test case as well? Otherwise this could regress for a different reason in the future.

@eeckstein
Copy link
Member

Thanks for the sil file.
I'll take a look

@atrick
Copy link
Member

atrick commented Apr 6, 2017

I verified this is fixed by

commit 2fce87c
Author: Erik Eckstein <eeckstein@apple.com>
Date: Wed Apr 5 17:02:29 2017

EscapeAnalysis: fix a wrong use-point detection.

This caused redundant load elimination to remove a load although the value is overwritten in a called function.
Most likely this could only occur if the load address is a block argument.

fixes SR-4393

It also fixes SR-4497, and is likely the cause of other recent bugs.

I actually cannot reproduce crash from the original test case; at
least not after upgrading my host OS. However, I can reproduce the
problem with a reduced test case that does not depend on
Foundation. See reduced.swift.

@eeckstein
Copy link
Member

done in 2fce87c

@NachoSoto
Copy link
Contributor Author

You ignored my previous comment: why is this reduced Swift code not added as a test case? You fixed that SIL issue, but the same Swift code could regress again in a future version of Swift since it's not part of the test suite, and nobody would notice until getting it released.
The fact that this crashed due to that SIL bug is an implement detail, and the higher level code I attached here still has no test coverage.

@atrick
Copy link
Member

atrick commented Apr 6, 2017

I actually tried for several hours yesterday to reproduce with the original test case unsuccessfully. Instead, I attached a new `reduced.swift` test case that exposes the same problem the same way but doesn't depend on Foundation. However, there's nothing special about that swift code. The same bug is now showing up in multiple Swift test cases. Reproducing it has more to do with the stdlib code than the test code. My swift test case is unlikely to expose the same issue in future versions of stdlib/compiler. So while I agree with the motivation for source-level test, I don't really see an interesting .swift test case here and don't have time to invest in a stdlib-independent version of the source test.

That said, there should be a process for adding tests derived from external source if you think it's an interesting test. Erik will follow up on that.

@eeckstein
Copy link
Member

If you want your swift code to be added to the compiler tests, I would recommend that you create a pull request for https://github.com/apple/swift
A good place to add the test is in validation-test/execution. There you can find some other tests you can use as templates.

If you need any help with that, please let me know.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
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. compiler The Swift compiler in itself optimized only Flag: An issue whose reproduction requires optimized compilation
Projects
None yet
Development

No branches or pull requests

5 participants