[Runtime] Define GOT equivalents for Linux for BuiltinProtocolConformances #34457

Azoy · 2020-10-27T06:24:22Z

Some Linux archs+platforms don't support relocations like @GOTPCREL which is what we defaulted to using on ELF platforms. Create a GOT equivalent for indirect symbols we reference for ELF (mirroring what the Swift compiler currently does). This should fix external CI testing platforms. Joe, I remember we mentioned that we shouldn't need to have to do this, but it seems beneficial to go ahead and create these for Linux because of the wide support range of arch requirements when it comes to relocations.

…ances

Azoy · 2020-10-27T06:24:42Z

@swift-ci Please test Edit: Seems CI access hasn't kicked in yet.

xwu · 2020-10-27T10:28:20Z

@swift-ci Please test

swift-ci · 2020-10-27T12:57:03Z

Build failed
Swift Test OS X Platform
Git Sha - cb67bda

xwu · 2020-10-27T14:33:16Z

@swift-ci Please test OS X platform

gottesmm · 2020-10-27T16:26:21Z

@lhames what are your thoughts on this?

jckarter · 2020-10-27T17:26:35Z

ELF assemblers do support GOT relative references, but the syntax may be different. @GOT should work IIRC.

jckarter · 2020-10-27T17:36:59Z

Maybe it would be more portable to write the assembly as a .ll file? LLVM ought to be able to generate the proper relocations for all of our target platforms.

drodriguez · 2020-10-27T22:33:30Z

stdlib/public/runtime/BuiltinProtocolConformances.cpp

+#define P2ALIGN_FOR_GOT "  .p2align 3\n"
+#else
+#define POINTER_FOR_GOT(SYMBOL) \
+  "  .long (" SYMBOl ")\n" \


Minor: lowercase l at the end of SYMBOl.

lhames · 2020-10-27T23:01:28Z

@jckarter @Azoy I thought ELF had syntax for this too, but don't recall it off the top of my head. Does @got work?

If there is any suitable native ELF syntax I would use that. Otherwise IR seems like a good option.

drodriguez · 2020-10-27T23:13:50Z

So with this patch applied, and the fix for the lowercase l, I get to the following error, which seems related:

/usr/bin/ld.gold: error: stdlib/public/runtime/CMakeFiles/swiftRuntime-linux-x86_64.dir/BuiltinProtocolConformances.cpp.o: cannot make copy relocation for protected symbol '$sSQ2eeoiySbx_xtFZTq', defined in lib/swift/linux/x86_64/libswiftCore.so
/usr/bin/ld.gold: error: stdlib/public/runtime/CMakeFiles/swiftRuntime-linux-x86_64.dir/BuiltinProtocolConformances.cpp.o: cannot make copy relocation for protected symbol '$sSHSQTb', defined in lib/swift/linux/x86_64/libswiftCore.so
/usr/bin/ld.gold: error: stdlib/public/runtime/CMakeFiles/swiftRuntime-linux-x86_64.dir/BuiltinProtocolConformances.cpp.o: cannot make copy relocation for protected symbol '$sSH9hashValueSivgTq', defined in lib/swift/linux/x86_64/libswiftCore.so
/usr/bin/ld.gold: error: stdlib/public/runtime/CMakeFiles/swiftRuntime-linux-x86_64.dir/BuiltinProtocolConformances.cpp.o: cannot make copy relocation for protected symbol '$sSH4hash4intoys6HasherVz_tFTq', defined in lib/swift/linux/x86_64/libswiftCore.so
/usr/bin/ld.gold: error: stdlib/public/runtime/CMakeFiles/swiftRuntime-linux-x86_64.dir/BuiltinProtocolConformances.cpp.o: cannot make copy relocation for protected symbol '$sSH13_rawHashValue4seedS2i_tFTq', defined in lib/swift/linux/x86_64/libswiftCore.so
/usr/bin/ld.gold: error: stdlib/public/runtime/CMakeFiles/swiftRuntime-linux-x86_64.dir/BuiltinProtocolConformances.cpp.o: cannot make copy relocation for protected symbol '$sSLSQTb', defined in lib/swift/linux/x86_64/libswiftCore.so
/usr/bin/ld.gold: error: stdlib/public/runtime/CMakeFiles/swiftRuntime-linux-x86_64.dir/BuiltinProtocolConformances.cpp.o: cannot make copy relocation for protected symbol '$sSL1loiySbx_xtFZTq', defined in lib/swift/linux/x86_64/libswiftCore.so
/usr/bin/ld.gold: error: stdlib/public/runtime/CMakeFiles/swiftRuntime-linux-x86_64.dir/BuiltinProtocolConformances.cpp.o: cannot make copy relocation for protected symbol '$sSL2leoiySbx_xtFZTq', defined in lib/swift/linux/x86_64/libswiftCore.so
/usr/bin/ld.gold: error: stdlib/public/runtime/CMakeFiles/swiftRuntime-linux-x86_64.dir/BuiltinProtocolConformances.cpp.o: cannot make copy relocation for protected symbol '$sSL2geoiySbx_xtFZTq', defined in lib/swift/linux/x86_64/libswiftCore.so
/usr/bin/ld.gold: error: stdlib/public/runtime/CMakeFiles/swiftRuntime-linux-x86_64.dir/BuiltinProtocolConformances.cpp.o: cannot make copy relocation for protected symbol '$sSL1goiySbx_xtFZTq', defined in lib/swift/linux/x86_64/libswiftCore.so

> swift-demangle
$sSQ2eeoiySbx_xtFZTq
method descriptor for static Swift.Equatable.== infix(A, A) -> Swift.Bool
$sSHSQTb
base conformance descriptor for Swift.Hashable: Swift.Equatable
$sSH9hashValueSivgTq
method descriptor for Swift.Hashable.hashValue.getter : Swift.Int
$sSH4hash4intoys6HasherVz_tFTq
method descriptor for Swift.Hashable.hash(into: inout Swift.Hasher) -> ()
$sSH13_rawHashValue4seedS2i_tFTq
method descriptor for Swift.Hashable._rawHashValue(seed: Swift.Int) -> Swift.Int
$sSLSQTb
base conformance descriptor for Swift.Comparable: Swift.Equatable
$sSL1loiySbx_xtFZTq
method descriptor for static Swift.Comparable.< infix(A, A) -> Swift.Bool
$sSL2leoiySbx_xtFZTq
method descriptor for static Swift.Comparable.<= infix(A, A) -> Swift.Bool
$sSL2geoiySbx_xtFZTq
method descriptor for static Swift.Comparable.>= infix(A, A) -> Swift.Bool
$sSL1goiySbx_xtFZTq
method descriptor for static Swift.Comparable.> infix(A, A) -> Swift.Bool

(the usage of ld.gold is because it is the one available in the Android NDK, which is the setup the Android CI builders use).

I will try AArch64 next to see if the same error is generated. The fix seems to at least avoid the compilation problem. Thanks a lot for it. I would not have been able to figure out the changes necessary.

Update: same problem in AArch64, but I just realized that it is failing in the Linux x86_64 runtime, not the Android one.

Update 2: checking in AArch64, but without this patch. I changed the @object to %object and seems to trigger the same assert as before (expected, since @object wasn't a problem before). I tried changing @GOTPCREL for just @GOT and the same assert happens. If one uses @PLT the assert goes away. Every non-device test seems to pass (except AutoDiff/compiler_crashers_fixed/sr13411-tangent-value-category-mismatch.swift, but I haven't looked into why). I haven't run the device tests, but I would imagine that changing the relocation type might show problems in runtime, not during compilation. One thing about @PLT is that it has 32 bit maximum, so the code size is "limited", if I understand correctly.

Azoy · 2020-10-28T02:04:41Z

Can we even use LLVM IR though? From a quick search it seems that we don't always build with clang, sometimes we build with msvc and I don't believe it can compile ll files (maybe that's just for when building clang? I'm not sure).

Yeah I've been looking at LLVM source and found the assert when its checking the relocation for aarch64 ELF, and it seems to only support an absolute symbol or a @PLT relocation which I don't believe is the same thing. With the new issue you're seeing @drodriguez I found that this seems to be an issue with gold and the fact that we can't PC-relative reference stdlib symbols in the runtime at compile time (which is why we have to go through GOT for these symbols in the first place). With device tests, does that include Interpreter tests? If it doesn't that means the runtime test for this feature passed using @PLT no? Also, its interesting that method descriptors and base conformance descriptors are marked as protected on linux whereas the protocol descriptors themselves are not (no linker errors for those symbols).

drodriguez · 2020-10-28T15:45:34Z

Can we even use LLVM IR though?

The Windows build prefers MSVC for compiling (it's faster on Windows), but builds large parts of LLVM. I don't see llc explicitly listed in https://github.com/apple/swift/blob/main/utils/build-windows.bat#L199, but it might be possible to change the build slightly to compile and use that tool. I suppose that @compnerd might know if there's a chance of a .ll file working for this problem across platforms.

With device tests, does that include Interpreter tests?

I don't think Interpreter tests are possible for cross-compiled targets, or I think they never have run in Android. I might be wrong. What I mean by "device tests" is those tests marked as executable_test, which need to execute the results of a compilation into the target device.

If it doesn't that means the runtime test for this feature passed using @plt no?

Yes. That's what I wanted to say with that disclaimer. I only run half of the tests. I would expect @PLT to make some noise in the device tests. Setting those up is a little bit complicated and takes long time. That's why I didn't do it.

jckarter · 2020-10-28T17:33:14Z

Can we even use LLVM IR though? From a quick search it seems that we don't always build with clang, sometimes we build with msvc and I don't believe it can compile ll files (maybe that's just for when building clang? I'm not sure).

The runtime can only be built with the just-built Clang, because upstream clang and msvc do not support the Swift calling convention. It should be fine to use LLVM IR.

compnerd · 2020-10-29T15:56:24Z

@jckarter - it can also be built with a host compiler as long as the host compiler supports the SwiftCC.

compnerd · 2020-10-29T15:56:48Z

stdlib/public/runtime/BuiltinProtocolConformances.cpp

+#define INDIRECT_RELREF_GOTPCREL(SYMBOL) ".Lgot." SYMBOL " - . + 1"
+
+// Helper for defining the .type on linux.
+#if defined(__arm__) && !defined(__aarch64__)


PPC uses the % sigil as well

compnerd · 2020-10-29T15:57:29Z

stdlib/public/runtime/BuiltinProtocolConformances.cpp

+#if defined(__x86_64__) || defined(__aarch64__)
+#define POINTER_FOR_GOT(SYMBOL) \
+  "  .quad (" SYMBOL ")\n" \
+  "  .size .Lgot." SYMBOL ", 8\n"


I think you might want to have a level of indirection here. The .L not guaranteed to the assembler local prefix.

compnerd · 2020-10-29T15:57:39Z

stdlib/public/runtime/BuiltinProtocolConformances.cpp

+#endif
+
+// Helper to get the pointer sized pointer to symbol and the right alignment.
+#if defined(__x86_64__) || defined(__aarch64__)


compnerd · 2020-10-29T15:58:02Z

stdlib/public/runtime/BuiltinProtocolConformances.cpp

+#else
+#define POINTER_FOR_GOT(SYMBOL) \
+  "  .long (" SYMBOl ")\n" \
+  "  .size .Lgot." SYMBOL ", 4\n"


Similar here to the 8-byte case.

compnerd · 2020-10-29T15:59:05Z

stdlib/public/runtime/BuiltinProtocolConformances.cpp

+// equivalent variables for linux.
+#define GOT_EQUIVALENT(SYMBOL) \
+  __asm( \
+    "  .type .Lgot." SYMBOL ", " TYPE_OBJECT "\n" \


I think that you should create another macro to generate the name rather than use the .Lgot." SYMBOL" in multiple locations.

compnerd · 2020-10-29T16:31:30Z

The copy relocation against a protected symbol has had issues in the past. Dropping the visibility to default should work, or alternatively, I think that looking into R_*_GOTPCRELX might be useful. Depending on the context in which it is used, we should be able to use the GOTPCREL variant of the symbol.

jckarter · 2020-10-29T17:22:36Z

@compnerd We should never copy Swift symbols, and we really do want them to be protected. If something is trying to emit a copy relocation of a Swift symbol, we need to fix that side.

jckarter · 2020-10-29T17:25:46Z

@jckarter - it can also be built with a host compiler as long as the host compiler supports the SwiftCC.

AFAIK that still implies some variation of Clang. If we tweak the swiftcc lowering, or the way the swiftcc maps into runtime C++, we're going to want to make the change to swift-clang in lockstep with swift itself, so using any other compiler is going to be brittle. However, we could use llc from the just built LLVM to compile .ll files if we don't want to rely on the runtime compiler being the just-built Clang.

compnerd · 2020-10-29T17:40:21Z

Oh, assuming that the compiler that is used for building the runtime supports the correct thing is reasonable. I don't think that we should be relying on llc as upstream officially deems it as an internal tool and provides no guarantees on it. I think that just using out-of-line assembly might really be a significantly easier to maintain solution here.

jckarter · 2020-10-29T18:10:19Z

I thought so too, but it looks like the correct assembly syntax for emitting the kinds of relocations we want is pretty tempermental and varies between platforms quite a bit. As far as portable options, C can't express relative references, but LLVM IR can.

If we don't want to ship a .ll file, or rely on llc or clang being part of the build toolchain, another kinda-gross option might be to just add code to IRGen that generates the necessary LLVM IR while building the standard library.

Azoy · 2020-10-29T18:12:17Z

I was just about to mention moving this back to IRGen to solve lots of these issues. Is there any particular reason you consider that solution kinda-gross? IIRC IRGen does something similar with Builtin reflection information where it emits it for the standard library vs. being in the runtime.

jckarter · 2020-10-29T18:15:46Z

I guess it just seems kind of unfortunate for the compiler to have code in it that's only ever used to build the standard library to me.

[Runtime] Define got equivalents for Linux for BuiltinProtocolConform…

cb67bda

…ances

Azoy requested review from drodriguez and jckarter October 27, 2020 06:24

drodriguez reviewed Oct 27, 2020

View reviewed changes

Azoy mentioned this pull request Oct 29, 2020

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

Merged

compnerd requested changes Oct 29, 2020

View reviewed changes

Azoy closed this Feb 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Runtime] Define GOT equivalents for Linux for BuiltinProtocolConformances #34457

[Runtime] Define GOT equivalents for Linux for BuiltinProtocolConformances #34457

Azoy commented Oct 27, 2020 •

edited

Loading

Azoy commented Oct 27, 2020 •

edited

Loading

xwu commented Oct 27, 2020

swift-ci commented Oct 27, 2020

xwu commented Oct 27, 2020

gottesmm commented Oct 27, 2020

jckarter commented Oct 27, 2020 •

edited

Loading

jckarter commented Oct 27, 2020

drodriguez Oct 27, 2020

lhames commented Oct 27, 2020

drodriguez commented Oct 27, 2020 •

edited

Loading

Azoy commented Oct 28, 2020 •

edited

Loading

drodriguez commented Oct 28, 2020

jckarter commented Oct 28, 2020

compnerd commented Oct 29, 2020

compnerd Oct 29, 2020

compnerd Oct 29, 2020

compnerd Oct 29, 2020

compnerd Oct 29, 2020

compnerd Oct 29, 2020

compnerd commented Oct 29, 2020

jckarter commented Oct 29, 2020 •

edited

Loading

jckarter commented Oct 29, 2020

compnerd commented Oct 29, 2020

jckarter commented Oct 29, 2020

Azoy commented Oct 29, 2020 •

edited

Loading

jckarter commented Oct 29, 2020

[Runtime] Define GOT equivalents for Linux for BuiltinProtocolConformances #34457

[Runtime] Define GOT equivalents for Linux for BuiltinProtocolConformances #34457

Conversation

Azoy commented Oct 27, 2020 • edited Loading

Azoy commented Oct 27, 2020 • edited Loading

xwu commented Oct 27, 2020

swift-ci commented Oct 27, 2020

xwu commented Oct 27, 2020

gottesmm commented Oct 27, 2020

jckarter commented Oct 27, 2020 • edited Loading

jckarter commented Oct 27, 2020

drodriguez Oct 27, 2020

Choose a reason for hiding this comment

lhames commented Oct 27, 2020

drodriguez commented Oct 27, 2020 • edited Loading

Azoy commented Oct 28, 2020 • edited Loading

drodriguez commented Oct 28, 2020

jckarter commented Oct 28, 2020

compnerd commented Oct 29, 2020

compnerd Oct 29, 2020

Choose a reason for hiding this comment

compnerd Oct 29, 2020

Choose a reason for hiding this comment

compnerd Oct 29, 2020

Choose a reason for hiding this comment

compnerd Oct 29, 2020

Choose a reason for hiding this comment

compnerd Oct 29, 2020

Choose a reason for hiding this comment

compnerd commented Oct 29, 2020

jckarter commented Oct 29, 2020 • edited Loading

jckarter commented Oct 29, 2020

compnerd commented Oct 29, 2020

jckarter commented Oct 29, 2020

Azoy commented Oct 29, 2020 • edited Loading

jckarter commented Oct 29, 2020

Azoy commented Oct 27, 2020 •

edited

Loading

Azoy commented Oct 27, 2020 •

edited

Loading

jckarter commented Oct 27, 2020 •

edited

Loading

drodriguez commented Oct 27, 2020 •

edited

Loading

Azoy commented Oct 28, 2020 •

edited

Loading

jckarter commented Oct 29, 2020 •

edited

Loading

Azoy commented Oct 29, 2020 •

edited

Loading