[SE-0283] Implement Equatable, Comparable, and Hashable conformance for Tuples

[Azoy]

From: https://forums.swift.org/t/hacking-equatable-conformance-on-void/25975

The tests right now are kind of lackluster, what else can I be testing with this? What else can be improved upon in this implementation? Is there an edge case that I hadn't thought about?

cc: @jckarter @rjmccall

[Azoy]

@rjmccall I've updated the implementation to have IRGen emit conformance descriptors for the stdlib. Let me know if this is what you had in mind.

[rjmccall]

Yes, this is basically what I was asking for, thank you.

As a general matter, I wonder if it wouldn't be better to just go a step further and make this a conditional conformance for all tuple types. That would be a big request except that you've obviously already figured out how to do some quite sophisticated stuff.

[Azoy]

I'll look into doing this conditionally for all tuples to see what work is needed.

[Azoy]

@rjmccall @jckarter I've gone ahead and implemented Equatable conformance for all tuples. Let me know what other tests you would like and any improvements I can make to the implementation. This is pretty exciting!

[Azoy]

Currently this initializes a new witness table everytime we need one, but it should be possible to have a single witness table in the runtime. Hmm...
Edit: Although maybe not because a conformance descriptor's witness table pattern is relative direct, and the address of the witness table I'm getting from the conformance at runtime does not match up to the address of the witness table in the runtime...

[jckarter]

The conformance descriptor format isn't absolutely important. Fundamentally, we just need something the witness table can point to that uniquely identifies the conformance. It seems to me like we should be able to have one descriptor and one witness table that works for all tuples.

[Azoy]

@jckarter Sorry I was being dumb, does this look ok? Single descriptor and witness table now!

[jckarter]

I haven't had time yet to look at this PR in depth, but I hope to soon. Sorry!

[Azoy]

@jckarter I've hopefully added backward compatibility for these conformances for both 5.0 and 5.1 runtimes. I'm not too versed with how the compatibility library works, but let me know if I made a mistake or something of the like. The test for compatibility should be exercised through the interpreter test I added in earlier commits, correct? I remember reading something from here: #25030 (comment)

[Azoy]

Does it make sense to also implement comparable and hashable in this pr, or should those be in separate prs?

[jckarter]

@Azoy It might be easier to review to break them out into their own PRs. Thanks!

[benrimmington]

@swift-ci Please test Windows platform

[jckarter]

@Azoy Have you asked for commit access yet? If you have commit access then you ought to be able to trigger CI by yourself.

[benrimmington]

All checks have passed. Should we test source compatibility again?

[Azoy]

I would say yes because they failed last time due to the macOS failure.

[benrimmington]

@swift-ci Please test source compatibility

[xwu]

@Azoy, this is fantastic. I guess it's ready to go?

[Azoy]

@xwu I believe so! @jckarter are we clear for takeoff?

[jckarter]

Should be good to go. Thanks @Azoy !

[drodriguez]

Has an internal crash in clang appeared during development?

It seems to have started around the introduction of these changes. The full logs can be seen in https://ci-external.swift.org/job/oss-swift-RA-linux-ubuntu-16.04-android-arm64/6614/console.

The failed assertion seems to be

clang++: /home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android-arm64/llvm-project/llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp:120: virtual unsigned int (anonymous namespace)::AArch64ELFObjectWriter::getRelocType(llvm::MCContext &, const llvm::MCValue &, const llvm::MCFixup &, bool) const: Assertion `(!Target.getSymA() || Target.getSymA()->getKind() == MCSymbolRefExpr::VK_None || Target.getSymA()->getKind() == MCSymbolRefExpr::VK_PLT) && "Should only be expression-level modifiers here"' failed.

In the case of the Android ARM32, the error is slightly different and might give someone a clue:

<inline asm>:1:41: error: expected STT_<TYPE_IN_UPPER_CASE>, '#<type>', '%<type>' or "<type>"
  .type __swift_tupleEquatable_private, @object
                                        ^
<inline asm>:5:37: error: expected STT_<TYPE_IN_UPPER_CASE>, '#<type>', '%<type>' or "<type>"
  .type _swift_tupleEquatable_conf, @object
                                    ^
<inline asm>:23:58: error: expected STT_<TYPE_IN_UPPER_CASE>, '#<type>', '%<type>' or "<type>"
  .type "associated conformance _swift_tupleComparable", @object
                                                         ^
<inline asm>:33:42: error: expected STT_<TYPE_IN_UPPER_CASE>, '#<type>', '%<type>' or "<type>"
  .type __swift_tupleComparable_private, @object
                                         ^
<inline asm>:37:38: error: expected STT_<TYPE_IN_UPPER_CASE>, '#<type>', '%<type>' or "<type>"
  .type _swift_tupleComparable_conf, @object
                                     ^
<inline asm>:62:40: error: expected STT_<TYPE_IN_UPPER_CASE>, '#<type>', '%<type>' or "<type>"
  .type __swift_tupleHashable_private, @object
                                       ^
<inline asm>:66:36: error: expected STT_<TYPE_IN_UPPER_CASE>, '#<type>', '%<type>' or "<type>"
  .type _swift_tupleHashable_conf, @object

There's some __asm pieces in the code, but I cannot match the reported errors to any of them.

Does someone has any idea what might be going on?

[drodriguez]

I found the problematic assembly. It was collapsed by GitHub.

Seems that @ is a comment symbol in AArch32 according to https://reviews.llvm.org/D10651. __ELF__ is probably exercised only in Linux, and not in ARM. ARM has probably checked in __MACH__, but not others.

Does anybody know which symbol can be used in front of object so it works both in every ELF platform (or at least in x86 and ARM)?

[Azoy]

@drodriguez for ARM32 on Android, it looks like it wants %object which should be a quick fix, but the ARM64 android looks like it doesn't support @GOTPCREL relocation from the error you gave (either nothing or @PLT which is different iirc) which I'm assuming will be an issue with ARM32. @jckarter I think a lot of these issues could be avoided if we manually created got equivalent structures for linux platforms, but that'll increase the amount of assembly for this dramatically. Maybe we can move this to an .S?

[drodriguez]

Thanks for the answer. I found that the Linux AArch64 testing seems to also had failed with a similar error: https://ci-external.swift.org/job/oss-swift-RA-linux-ubuntu-18.04-aarch64/2177/console. I will try to double check tonight about using the percent sign instead of the at-symbol.

[jckarter]

@Azoy The ELF syntax is @GOT instead of @GOTPCREL, IIRC.

[aschwaighofer]

Looks likely this broke: https://ci.swift.org/job/oss-swift_tools-RA_stdlib-DA_test-device-non_executable/250/

[aschwaighofer]

Temporary disabled the test here: #34480

[aschwaighofer]

@Azoy Can you take a look, I expect that the test just needs updating.

[tbkka]

We're also seeing some tests fail with errors about invalid relative offsets in a conformance. I don't know for sure that it was this PR, but the timing is suspicious. I'm trying to get more details.

[tbkka]

@rjmccall @jckarter -- Do you see a way that @Azoy could declare these conformances without resorting to assembly?

[Azoy]

@tbkka we're discussing writing them in LLVM IR instead over here: #34457

[jckarter]

@Azoy would it be OK if you reverted this? Another issue we're seeing is that it appears that executables and dylibs that link the compatibility versions of the tuple conformance implementation end up reexporting the __swift_tuple* symbols. The compatibility implementation ought to be private to the binary it gets linked in, so we'll want to make sure they have hidden visibility in the compatibility .a files (and it'd probably be good to give them different symbol names like __swift50_tuple* so they don't collide with the real runtime implementations in newer OSes).

I wonder if the issues @tbkka alluded to about invalid conformance addresses might be related to the compatibility libraries as well—if we link the compatibility libraries into a binary, but run with a new runtime that contains the real implementation of tuple conformance, that real implementation needs to take precedence, and the compatibility implementation would be invalid from the host runtime's perspective. Binaries that relies on the compatibility library for backward deployment might need to use a stub function that picks either the compatibility or host runtime version of the symbols to resolve the address of these symbols, instead of referencing the symbols directly.

[airspeedswift]

Reverted in #34492