NFC: Add `GenericSignature::getCanonicalSignature`. #29105

dan-zheng · 2020-01-09T22:53:37Z

Motivation: GenericSignatureImpl::getCanonicalSignature crashes for
GenericSignature with underlying nullptr. This led to verbose workarounds
when computing CanGenericSignature from GenericSignature:

// Workaround:
CanGenericSignature canGenSig;
if (genSig)
  canGenSig = genSig->getCanonicalSignature();

// Workaround:
auto canGenSig =
    genSig ? genSig->getCanonicalSignature() : CanGenericSignature();

Solution: GenericSignature::getCanonicalSignature is a wrapper around
GenericSignatureImpl::getCanonicalSignature that returns the canonical
signature, or nullptr if the underlying pointer is nullptr.

// Now:
auto canGenSig = getSig.getCanonicalSignature();

Rewrite all verbose workarounds using GenericSignature::getCanonicalSignature.

Related forum discussion.
For reference: motivated by #29099.

Similarly, Type::getCanonicalType can be added, forwarding to
TypeBase::getCanonicalType.

Motivation: `GenericSignatureImpl::getCanonicalSignature` crashes for `GenericSignature` with underlying `nullptr`. This led to verbose workarounds when computing `CanGenericSignature` from `GenericSignature`. Solution: `GenericSignature::getCanonicalSignature` is a wrapper around `GenericSignatureImpl::getCanonicalSignature` that returns the canonical signature, or `nullptr` if the underlying pointer is `nullptr`. Rewrite all verbose workarounds using `GenericSignature::getCanonicalSignature`.

dan-zheng · 2020-01-09T23:16:17Z

One concern is that genSig.getCanonicalSignature and genSig->getCanonicalSignature may be easily confused.

GenericSignature sig;
auto canSig1 = sig.getCanonicalSignature(); // newly added, returns `nullptr`
auto canSig2 = sig->getCanonicalSignature(); // crashes

We could avoid this confusion by removing GenericSignatureImpl *GenericSignature::operator->. That aligns with @jckarter's reply:

Eventually, most of the important API for GenericSignature should probably live on GenericSignature , and provide reasonable default behavior for empty generic signatures, so that GenericSignatureImpl can be hidden.

jckarter

Looks awesome to me!

dan-zheng · 2020-01-09T23:20:14Z

@swift-ci Please test

CodaFi · 2020-01-10T00:00:39Z

Yes. We need to migrate most of the API up and out of the Impl type. This state of affairs only exists because there's some weird header dependency between Type.h and Types.h that goes through GenericSignature.h or somesuch nonsense.

swift-ci · 2020-01-10T00:54:31Z

Build failed
Swift Test Linux Platform
Git Sha - 26e142c

slavapestov

Nice cleanup.

lib/SIL/SILVerifier.cpp

lib/SILOptimizer/Utils/Generics.cpp

dan-zheng · 2020-01-10T03:03:08Z

Linux CI fails for validation-test/stdlib/Arrays.swift.gyb:

16:54:26 Failing Tests (1):
16:54:26     Swift(linux-x86_64) :: stdlib/Arrays.swift.gyb

Details

/home/danielzheng/swift-master/build/Ninja-ReleaseAssert+stdlib-Release/swift-linux-x86_64/validation-test-linux-x86_64/stdlib/Output/Arrays.swift.gyb.tmp/Arrays
[ RUN      ] CopyToNativeArrayBufferTests.Sequence._copyToContiguousArray()
[       OK ] CopyToNativeArrayBufferTests.Sequence._copyToContiguousArray()
[ RUN      ] CopyToNativeArrayBufferTests.Collection._copyToContiguousArray()
[       OK ] CopyToNativeArrayBufferTests.Collection._copyToContiguousArray()
CopyToNativeArrayBufferTests: All tests passed
[ RUN      ] Array.sizeof
[       OK ] Array.sizeof
[ RUN      ] Array.valueDestruction
[       OK ] Array.valueDestruction
[ RUN      ] Array.capacity/reserveCapacity(_:)
[       OK ] Array.capacity/reserveCapacity(_:)
[ RUN      ] Array.init(arrayLiteral:)
[       OK ] Array.init(arrayLiteral:)
[ RUN      ] Array.init(repeating:count:)
[       OK ] Array.init(repeating:count:)
[ RUN      ] Array.Hashable
[       OK ] Array.Hashable
[ RUN      ] Array.mutationDoesNotAffectIterator/subscript/store
[       OK ] Array.mutationDoesNotAffectIterator/subscript/store
[ RUN      ] Array.mutationDoesNotAffectIterator/removeAt,1
[       OK ] Array.mutationDoesNotAffectIterator/removeAt,1
[ RUN      ] Array.mutationDoesNotAffectIterator/removeAt,all
[       OK ] Array.mutationDoesNotAffectIterator/removeAt,all
[ RUN      ] Array.mutationDoesNotAffectIterator/removeAll,keepingCapacity=false
[       OK ] Array.mutationDoesNotAffectIterator/removeAll,keepingCapacity=false
[ RUN      ] Array.mutationDoesNotAffectIterator/removeAll,keepingCapacity=true
[       OK ] Array.mutationDoesNotAffectIterator/removeAll,keepingCapacity=true
[ RUN      ] Array.Native/count/empty
[       OK ] Array.Native/count/empty
[ RUN      ] Array.Native/count
[       OK ] Array.Native/count
[ RUN      ] Array.Native/isEmpty/empty
[       OK ] Array.Native/isEmpty/empty
[ RUN      ] Array.Native/isEmpty
[       OK ] Array.Native/isEmpty
[ RUN      ] Array.Array/popLast
[       OK ] Array.Array/popLast
[ RUN      ] Array.ContiguousArray/popLast
stderr>>> CRASHED: SIGILL
the test crashed unexpectedly
[     FAIL ] Array.ContiguousArray/popLast
[ RUN      ] Array.ArraySlice/popLast
[       OK ] Array.ArraySlice/popLast
[ RUN      ] Array.COW.Smoke
[       OK ] Array.COW.Smoke
[ RUN      ] Array.COW.Fast.SubscriptWithIndexDoesNotReallocate
[       OK ] Array.COW.Fast.SubscriptWithIndexDoesNotReallocate
[ RUN      ] Array.COW.Slow.SubscriptWithIndexDoesNotReallocate
[       OK ] Array.COW.Slow.SubscriptWithIndexDoesNotReallocate
[ RUN      ] Array.COW.Fast.RemoveAtDoesNotReallocate
[       OK ] Array.COW.Fast.RemoveAtDoesNotReallocate
[ RUN      ] Array.COW.Slow.RemoveAtDoesNotReallocate
[       OK ] Array.COW.Slow.RemoveAtDoesNotReallocate
[ SKIP     ] Array.COW.Fast.RemoveAllDoesNotReallocate (skip: [linuxAny(*, reason: rdar://problem/34268868)])
[ SKIP     ] Array.COW.Slow.RemoveAllDoesNotReallocate (skip: [linuxAny(*, reason: rdar://problem/34268868)])
[ RUN      ] Array.COW.Fast.CountDoesNotReallocate
[       OK ] Array.COW.Fast.CountDoesNotReallocate
[ RUN      ] Array.COW.Slow.CountDoesNotReallocate
[       OK ] Array.COW.Slow.CountDoesNotReallocate
[ RUN      ] Array.COW.Fast.GenerateDoesNotReallocate
[       OK ] Array.COW.Fast.GenerateDoesNotReallocate
[ RUN      ] Array.COW.Slow.GenerateDoesNotReallocate
[       OK ] Array.COW.Slow.GenerateDoesNotReallocate
[ RUN      ] Array.COW.Fast.EqualityTestDoesNotReallocate
[       OK ] Array.COW.Fast.EqualityTestDoesNotReallocate
[ RUN      ] Array.COW.Slow.EqualityTestDoesNotReallocate
[       OK ] Array.COW.Slow.EqualityTestDoesNotReallocate
[ RUN      ] Array.ArrayIndexTests
[       OK ] Array.ArrayIndexTests
[ RUN      ] Array.ArrayIndexTests
[       OK ] Array.ArrayIndexTests
[ RUN      ] Array.ArrayIndexTests
[       OK ] Array.ArrayIndexTests
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilGrowth/Infrastructure/EvilSequence
[       OK ] Array.MemorySafety/BoundsChecked/EvilGrowth/Infrastructure/EvilSequence
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilGrowth/Infrastructure/EvilCollection
stderr>>> OK: saw expected "crashed: sigill"
[       OK ] Array.MemorySafety/BoundsChecked/EvilGrowth/Infrastructure/EvilCollection
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilGrowth/Construction
[       OK ] Array.MemorySafety/BoundsChecked/EvilGrowth/Construction
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilGrowth/replaceSubrange/ShrinkUnique
[       OK ] Array.MemorySafety/BoundsChecked/EvilGrowth/replaceSubrange/ShrinkUnique
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilGrowth/replaceSubrange/ShrinkNonUnique
[       OK ] Array.MemorySafety/BoundsChecked/EvilGrowth/replaceSubrange/ShrinkNonUnique
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilGrowth/replaceSubrange/GrowUnique
[       OK ] Array.MemorySafety/BoundsChecked/EvilGrowth/replaceSubrange/GrowUnique
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilGrowth/replaceSubrange/GrowNonUnique
[       OK ] Array.MemorySafety/BoundsChecked/EvilGrowth/replaceSubrange/GrowNonUnique
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilGrowth/SequenceMap
[       OK ] Array.MemorySafety/BoundsChecked/EvilGrowth/SequenceMap
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilGrowth/CollectionMap
[       OK ] Array.MemorySafety/BoundsChecked/EvilGrowth/CollectionMap
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilGrowth/FilterAll
[       OK ] Array.MemorySafety/BoundsChecked/EvilGrowth/FilterAll
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilGrowth/FilterNone
[       OK ] Array.MemorySafety/BoundsChecked/EvilGrowth/FilterNone
[ RUN      ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/Infrastructure/EvilSequence
[       OK ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/Infrastructure/EvilSequence
[ RUN      ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/Infrastructure/EvilCollection
[       OK ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/Infrastructure/EvilCollection
[ RUN      ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/Construction
stderr>>> OK: saw expected "crashed: sigill"
[       OK ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/Construction
[ RUN      ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/replaceSubrange/ShrinkUnique
[       OK ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/replaceSubrange/ShrinkUnique
[ RUN      ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/replaceSubrange/ShrinkNonUnique
[       OK ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/replaceSubrange/ShrinkNonUnique
[ RUN      ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/replaceSubrange/GrowUnique
[       OK ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/replaceSubrange/GrowUnique
[ RUN      ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/replaceSubrange/GrowNonUnique
[       OK ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/replaceSubrange/GrowNonUnique
[ RUN      ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/SequenceMap
stderr>>> OK: saw expected "crashed: sigill"
[       OK ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/SequenceMap
[ RUN      ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/CollectionMap
[       OK ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/CollectionMap
[ RUN      ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/FilterAll
[       OK ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/FilterAll
[ RUN      ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/FilterNone
[       OK ] Array.MemorySafety/NoBoundsCheck/EvilShrinkage/FilterNone
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilShrinkage/Infrastructure/EvilSequence
[       OK ] Array.MemorySafety/BoundsChecked/EvilShrinkage/Infrastructure/EvilSequence
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilShrinkage/Infrastructure/EvilCollection
stderr>>> OK: saw expected "crashed: sigill"
[       OK ] Array.MemorySafety/BoundsChecked/EvilShrinkage/Infrastructure/EvilCollection
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilShrinkage/Construction
stderr>>> OK: saw expected "crashed: sigill"
[       OK ] Array.MemorySafety/BoundsChecked/EvilShrinkage/Construction
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilShrinkage/replaceSubrange/ShrinkUnique
stderr>>> OK: saw expected "crashed: sigill"
[       OK ] Array.MemorySafety/BoundsChecked/EvilShrinkage/replaceSubrange/ShrinkUnique
[ RUN      ] Array.MemorySafety/BoundsChecked/EvilS

I'm not sure what causes this failure, but I can reproduce it locally on Linux.
I wonder if it's introduced in this PR or a different one. I'll investigate further.

Simplify `GenericSignature::getCanonicalSignature` callers.

…ature-fix

dan-zheng · 2020-01-10T22:09:41Z

Linux CI fails for validation-test/stdlib/Arrays.swift.gyb:

I tried merging master into this PR, but it didn't fix the test failure.
I didn't find the underlying issue, but I minimized the reproducer:

// Note: the `do` statement is important to reproduce the crash.
do {
  var a: ContiguousArray = [0]
  while let element = a.popLast() {
  }
}

$ swift -O crash.swift
crash.swift:4:13: warning: value 'element' was defined but never used; consider replacing with boolean test
  while let element = a.popLast() {
        ~~~~^~~~~~~~~~
                                  != nil
Stack dump:
0.	Program arguments: /home/danielzheng/swift-master/build/Ninja-ReleaseAssert+stdlib-Release/swift-linux-x86_64/bin/swift -frontend -interpret crash.swift -disable-objc-interop -color-diagnostics -O -module-name crash
1.	Swift version 5.2-dev
2.	While running user code "crash.swift"
/home/danielzheng/swift-master/build/Ninja-ReleaseAssert+stdlib-Release/swift-linux-x86_64/bin/swift[0x4b81b24]
/home/danielzheng/swift-master/build/Ninja-ReleaseAssert+stdlib-Release/swift-linux-x86_64/bin/swift[0x4b7f6fe]
/home/danielzheng/swift-master/build/Ninja-ReleaseAssert+stdlib-Release/swift-linux-x86_64/bin/swift[0x4b81de6]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x12890)[0x7f950542c890]
[0x7f950585d0ce]
/home/danielzheng/swift-master/build/Ninja-ReleaseAssert+stdlib-Release/swift-linux-x86_64/bin/swift[0x507059]
/home/danielzheng/swift-master/build/Ninja-ReleaseAssert+stdlib-Release/swift-linux-x86_64/bin/swift[0x50b500]
/home/danielzheng/swift-master/build/Ninja-ReleaseAssert+stdlib-Release/swift-linux-x86_64/bin/swift[0x4f8730]
/home/danielzheng/swift-master/build/Ninja-ReleaseAssert+stdlib-Release/swift-linux-x86_64/bin/swift[0x4edb72]
/home/danielzheng/swift-master/build/Ninja-ReleaseAssert+stdlib-Release/swift-linux-x86_64/bin/swift[0x4e260c]
/home/danielzheng/swift-master/build/Ninja-ReleaseAssert+stdlib-Release/swift-linux-x86_64/bin/swift[0x4df413]
/home/danielzheng/swift-master/build/Ninja-ReleaseAssert+stdlib-Release/swift-linux-x86_64/bin/swift[0x473f85]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xe7)[0x7f9503872b97]
/home/danielzheng/swift-master/build/Ninja-ReleaseAssert+stdlib-Release/swift-linux-x86_64/bin/swift[0x473bca]
[1]    941 illegal hardware instruction (core dumped)  $SWIFT_BIN/swift -O crash.swift

Edit: others say this test failure was caused by an unrelated PR, and has since been fixed/reverted. Pulling from master and rerunning CI fixed the failure.

…ature-fix

dan-zheng · 2020-01-12T06:15:20Z

@swift-ci Please test

swift-ci · 2020-01-12T06:17:20Z

Build failed
Swift Test Linux Platform
Git Sha - 26e142c

swift-ci · 2020-01-12T06:17:29Z

Build failed
Swift Test OS X Platform
Git Sha - 26e142c

dan-zheng added 2 commits January 9, 2020 22:51

Run git clang-format.

26e142c

dan-zheng requested a review from jckarter January 9, 2020 22:53

dan-zheng requested a review from CodaFi January 9, 2020 23:16

jckarter approved these changes Jan 9, 2020

View reviewed changes

dan-zheng mentioned this pull request Jan 10, 2020

[AutoDiff upstream] Define AutoDiffConfig. #29099

Merged

slavapestov approved these changes Jan 10, 2020

View reviewed changes

lib/SIL/SILVerifier.cpp Outdated Show resolved Hide resolved

lib/SILOptimizer/Utils/Generics.cpp Outdated Show resolved Hide resolved

lib/SILOptimizer/Utils/Generics.cpp Outdated Show resolved Hide resolved

dan-zheng added 2 commits January 10, 2020 06:43

Address review feedback.

7a10f01

Simplify `GenericSignature::getCanonicalSignature` callers.

Merge branch 'master' of github.com:apple/swift into can-generic-sign…

b1aa154

…ature-fix

CodaFi approved these changes Jan 10, 2020

View reviewed changes

Merge branch 'master' of github.com:apple/swift into can-generic-sign…

4701696

…ature-fix

dan-zheng merged commit 1486d6b into swiftlang:master Jan 12, 2020

dan-zheng deleted the can-generic-signature-fix branch January 12, 2020 20:17

dan-zheng mentioned this pull request Mar 22, 2020

[AutoDiff upstream] Add derivative function SILDeclRefs. #30564

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NFC: Add `GenericSignature::getCanonicalSignature`. #29105

NFC: Add `GenericSignature::getCanonicalSignature`. #29105

dan-zheng commented Jan 9, 2020 •

edited

Loading

dan-zheng commented Jan 9, 2020

jckarter left a comment

dan-zheng commented Jan 9, 2020

CodaFi commented Jan 10, 2020

swift-ci commented Jan 10, 2020

slavapestov left a comment

dan-zheng commented Jan 10, 2020

dan-zheng commented Jan 10, 2020 •

edited

Loading

dan-zheng commented Jan 12, 2020

swift-ci commented Jan 12, 2020

swift-ci commented Jan 12, 2020

NFC: Add GenericSignature::getCanonicalSignature. #29105

NFC: Add GenericSignature::getCanonicalSignature. #29105

Conversation

dan-zheng commented Jan 9, 2020 • edited Loading

dan-zheng commented Jan 9, 2020

jckarter left a comment

Choose a reason for hiding this comment

dan-zheng commented Jan 9, 2020

CodaFi commented Jan 10, 2020

swift-ci commented Jan 10, 2020

slavapestov left a comment

Choose a reason for hiding this comment

dan-zheng commented Jan 10, 2020

dan-zheng commented Jan 10, 2020 • edited Loading

dan-zheng commented Jan 12, 2020

swift-ci commented Jan 12, 2020

swift-ci commented Jan 12, 2020

NFC: Add `GenericSignature::getCanonicalSignature`. #29105

NFC: Add `GenericSignature::getCanonicalSignature`. #29105

dan-zheng commented Jan 9, 2020 •

edited

Loading

dan-zheng commented Jan 10, 2020 •

edited

Loading