IRGen: ensure that helper functions are COMDAT'ed #20059

compnerd · 2018-10-26T00:52:32Z

The linkonce_odr semantics in LLVM have changed. They no longer create
a COMDAT group. This means that the helper functions that we are
generating inline globally are not collected and uniqued. This results
in multiple definitions, which is a hard error in PE/COFF. ELF would
get away with it due to the weak linkage semantics that it provides.
Explicitly mark the helpers for COMDAT. This should enable building the
swift SDK overlay for libdispatch on Windows again.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

compnerd · 2018-10-26T00:54:07Z

CC: @aschwaighofer
@eeckstein, @gottesmm you might care about this as this impacts size (on non-MachO though)

Yes, I know, I should go clean up the other cases, but this is sufficient to unblock me. I'll try to get to the other cases in a follow up.

compnerd · 2018-10-26T00:54:16Z

@swift-ci please test

swift-ci · 2018-10-26T01:47:05Z

Build failed
Swift Test Linux Platform
Git Sha - 1cc2e25f7028a4aea487969c5165da9f8c03fa30

swift-ci · 2018-10-26T03:38:29Z

Build failed
Swift Test OS X Platform
Git Sha - 1cc2e25f7028a4aea487969c5165da9f8c03fa30

eeckstein · 2018-10-26T15:36:20Z

Fine with me

jrose-apple · 2018-10-26T16:21:09Z

test/IRGen/alloc.sil

-// CHECK: define linkonce_odr hidden i8* @__swift_memcpy4097_8(i8*, i8*, %swift.type*)
+// CHECK-COMDAT: define linkonce_odr hidden i8* @__swift_memcpy4097_8(i8*, i8*, %swift.type*)
+// CHECK-COMDAT-SAME: comdat
+// CHECK-NOCOMDAT-NOT:comdat


This will only check that "comdat" isn't written here, not that it isn't written at all. Did you mean to leave the first line as CHECK instead of CHECK-COMDAT?

Oops, yeah, the first line should be CHECK.

compnerd · 2018-10-27T02:02:00Z

This uncovered a bug in LLVM (dwarfdump was being too aggressive about the verification on object files, I've fixed that in LLVM and will pull that into the stable branch).

compnerd · 2018-10-28T10:22:55Z

@swift-ci please test

swift-ci · 2018-10-28T10:24:43Z

Build failed
Swift Test Linux Platform
Git Sha - 1cc2e25f7028a4aea487969c5165da9f8c03fa30

swift-ci · 2018-10-28T10:27:08Z

Build failed
Swift Test OS X Platform
Git Sha - 1cc2e25f7028a4aea487969c5165da9f8c03fa30

compnerd · 2018-10-29T20:34:00Z

@swift-ci please test

swift-ci · 2018-10-29T20:37:18Z

Build failed
Swift Test OS X Platform
Git Sha - 649f6d38cb052f10c402e064a6a51c981704c3ad

swift-ci · 2018-10-29T20:40:19Z

Build failed
Swift Test Linux Platform
Git Sha - 649f6d38cb052f10c402e064a6a51c981704c3ad

compnerd · 2018-10-29T21:45:50Z

@swift-ci please test

swift-ci · 2018-10-29T21:48:13Z

Build failed
Swift Test Linux Platform
Git Sha - 2556e2ccd4e578af8f8d54aff3b04d2ba027742a

swift-ci · 2018-10-29T21:49:35Z

Build failed
Swift Test OS X Platform
Git Sha - 2556e2ccd4e578af8f8d54aff3b04d2ba027742a

rjmccall · 2018-10-29T21:56:14Z

@compnerd Can you point out where this change happened in LLVM? It wasn't well-communicated.

jckarter · 2018-10-29T21:56:33Z

Are you sure this was an intentional LLVM change? It seems like a pretty severe break if linkonce_odr doesn't guarantee coalescing by itself anymore.

compnerd · 2018-10-29T22:11:56Z

@rjmccall, @jckarter - yes, this was intentional :-( ... I just have it on what Reid Klecker mentioned to me. @jckarter - it still can coalesce on ELF - because of weak symbols. This definitely does not work on COFF where you do not have weak symbols. By creating the COMDAT group we ensure that there is a single definition.

compnerd · 2018-10-29T22:20:11Z

CC: @rnk

gottesmm · 2018-10-29T22:20:12Z

@rnk Do you remember what changed here?

rnk · 2018-10-29T22:35:47Z

Yeah, like everything. You gotta use comdats now if you want comdats. That happened back in 2014. It looks like the final change was r226467, so I guess 2015. Unfortunately, I have not been able to find a good llvm-dev discussion summarizing the change, but I remember there was a lot of discussion.

You know, the plans to bulldoze linkonce_odr semantics were on display down at the planning office for months. =P

compnerd · 2018-10-29T23:00:30Z

😢

Failing Tests (5):
Swift(linux-x86_64) :: Reflection/typeref_decoding_asan.swift
Swift(linux-x86_64) :: Reflection/typeref_decoding.swift
Swift(linux-x86_64) :: Reflection/typeref_lowering.swift
Swift(linux-x86_64) :: Reflection/typeref_decoding_imported.swift
Swift(linux-x86_64) :: DebugInfo/local-vars.swift.gyb

The DebugInfo failure is due to the DIE check being invalid. That was relaxed in LLVM (by me). I'm not sure why the Reflection tests are failing after this change.

rjmccall · 2018-10-29T23:11:11Z

I feel like the old semantics were a useful common core across all targets, which is all that was necessary for the vast majority of frontend use cases. But PE/COFF does some other things in IR pointlessly differently, so I guess this is of a piece.

compnerd · 2018-11-02T21:35:37Z

CC: @slavapestov

compnerd · 2018-11-04T05:26:48Z

sigh okay, binaries built with lld work but fail with gold? This is starting to smell like a linker issue :-(. I am amused at the number of linker issues we are finding with swift code.

jckarter · 2018-11-04T16:06:47Z

Alas, the more you deviate from what C compilers do, the more linker breakage you overturn. Can we stick to using COMDAT only for COFF, and continue to rely on weak definitions on ELF?

compnerd · 2018-11-04T17:54:53Z

@jckarter that definitely is what I have in mind, but this does come at a cost, we should definitely consider this. We already know that the BFD linker doesn't work for swift. Perhaps it is time to switch from gold to lld.

With COMDAT:
text data bss dec hex filename
5252178 328781 128424 5709383 571e47 libswiftCore.so

Weak:
text data bss dec hex filename
5405803 175220 128424 5709447 571e87 libswiftCore.so

jckarter · 2018-11-04T18:03:40Z

We dropped BFD support because they broke protected linkage in favor of copy relocations, and don't seem interested in fixing it (https://www.sourceware.org/ml/binutils/2016-03/msg00312.html). gold would probably be receptive to fixing bugs in their comdat handling. The code size win is nice, but I think we should fix the immediate issue with Windows support first without regressing Linux support in the meantime.

In order to handle LinkOnceODR semantics correctly across various object formats, introduce a new helper ApplyIRLinkage. This abstracts the need to create a COMDAT group and set it on the GlobalValue. Adjust all sites where we set the IR linkage attributes to use this mechanism instead to avoid having to track down symbols not being added to a COMDAT group.

compnerd · 2018-11-04T18:20:11Z

@swift-ci please test

swift-ci · 2018-11-04T18:22:12Z

Build failed
Swift Test OS X Platform
Git Sha - c4e6514e17fbb5eedd7c3b259f545a61f2aa0f71

swift-ci · 2018-11-04T18:22:13Z

Build failed
Swift Test Linux Platform
Git Sha - c4e6514e17fbb5eedd7c3b259f545a61f2aa0f71

rnk · 2018-11-05T18:58:20Z

Alas, the more you deviate from what C compilers do, the more linker breakage you overturn. Can we stick to using COMDAT only for COFF, and continue to rely on weak definitions on ELF?

Uh, no, C++ compilers use comdat and weak on ELF. I would suggest setting both comdat and linkonce_odr linkage everywhere to more closely match what clang does.

I feel like the old semantics were a useful common core across all targets, which is all that was necessary for the vast majority of frontend use cases. But PE/COFF does some other things in IR pointlessly differently, so I guess this is of a piece.

There were nice things about the old way of doing things, but as usual, people wanted to access lower-level features that LLVM was hiding from them. This isn't PE/COFF being weirdly different for its own sake, though. ELF has comdats too, and you need to use them if you want to generate vague linkage things the same way that clang does.

Here's what the main object file formats support (ignoring wasm):
macho: has weak
elf: has weak, has comdats
pe/coff: has only comdats, but GCC implements attribute((weak)) with stubs if you're game (not appropriate for the use case of vague linkage)

All of the weak, linkonce, and ODR linkages used to enable weak and comdat, depending on what the object file format supported. For ELF, weak and comdat are kind of redundant. If all symbol definitions are weak, the first one will prevail, otherwise the one and only strong one prevails. Adding a comdat group basically does a first pass to drop duplicate symbols before doing that weak vs. strong resolution.

jckarter · 2018-11-05T19:58:48Z

Alas, the more you deviate from what C compilers do, the more linker breakage you overturn. Can we stick to using COMDAT only for COFF, and continue to rely on weak definitions on ELF?

Uh, no, C++ compilers use comdat and weak on ELF. I would suggest setting both comdat and linkonce_odr linkage everywhere to more closely match what clang does.

I'm aware of that, but there could be interactions with comdat and other things Swift does that C doesn't that gold doesn't handle correctly. (Particularly, we've had persistent problems with ld64 on Darwin not handling subtractions for relative references correctly with symbols in coalesced sections; doesn't seem totally out of the question gold might have similar latent phase-order bugs.)

compnerd · 2018-11-06T01:27:37Z

@swift-ci please clean test macOS platform

compnerd · 2018-11-06T04:56:16Z

I've filed SR-9197 to track the ELF side of things. This currently is limited to COFF for the COMDAT group construction. We should work out the bug in gold and enable this for ELF too.

compnerd · 2018-11-06T05:01:56Z

@jckarter @rjmccall - okay to merge?

rjmccall · 2018-11-06T05:53:09Z

lib/IRGen/GenDecl.cpp

-  alias->setVisibility(link.getVisibility());
-  alias->setDLLStorageClass(link.getDLLStorage());
+  ApplyIRLinkage({link.getLinkage(), link.getVisibility(), link.getDLLStorage()})
+      .to(alias);


This pattern — repeated over and over again — where we create a declaration and then immediately override half the values from it with the values of an IRLinkage is quite unfortunate. I like the idea of encapsulating IRLinkage; perhaps it ought to have functions on it to actually create these globals?

I disliked that we were setting the properties for the linkage everywhere but didn't manage to do it very uniformly. So, the application of the IRLinkage at least ensures we set the properties more uniformly. But, I can try to sink this into createVariable and createFunction. I'm happy to try to do that in a follow up.

I'm fine with doing it in multiple stages. Some other suggestions:

As mentioned, please add functions to create variables and functions.

Please look around IRGen for other places that create variables and functions that could use this. LLVM's API for creating globals is really a mess, so it shouldn't be hard to create something that's friendlier and therefore encourages people to use it (and thus implicitly get the correctness benefits).

Please add static factories (like shared() or internal()) for the common patterns.

Consider incorporating unnamed_addr into this, and maybe an optional section name. But constant probably ought to be a mandatory argument to the function to create a global, not part of the linkage.

Yeah, unnamed_addr and dso_local are other attributes that I think that we should grow into this. But, this unblocks Windows builds and improves things.

jrose-apple reviewed Oct 26, 2018

View reviewed changes

compnerd force-pushed the comdat-helpers branch 2 times, most recently from c72ea22 to 649f6d3 Compare October 28, 2018 10:21

compnerd force-pushed the comdat-helpers branch from 649f6d3 to 2556e2c Compare October 29, 2018 20:30

compnerd force-pushed the comdat-helpers branch from 2556e2c to c4e6514 Compare October 29, 2018 21:45

compnerd mentioned this pull request Nov 2, 2018

IRGen: always use runtime initialized VWT on PE/COFF #20233

Closed

compnerd force-pushed the comdat-helpers branch from c4e6514 to 86e5b25 Compare November 4, 2018 18:14

rjmccall reviewed Nov 6, 2018

View reviewed changes

compnerd merged commit 24b86d8 into swiftlang:master Nov 6, 2018

compnerd deleted the comdat-helpers branch November 6, 2018 16:53

compnerd mentioned this pull request Oct 29, 2018

[SR-9117] LinkOnceODR functions need to be COMDAT'ed #51614

Closed

IRGen: ensure that helper functions are COMDAT'ed #20059

IRGen: ensure that helper functions are COMDAT'ed #20059

Uh oh!

Conversation

compnerd commented Oct 26, 2018

Uh oh!

compnerd commented Oct 26, 2018

Uh oh!

compnerd commented Oct 26, 2018

Uh oh!

swift-ci commented Oct 26, 2018

Uh oh!

swift-ci commented Oct 26, 2018

Uh oh!

eeckstein commented Oct 26, 2018

Uh oh!

jrose-apple Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

compnerd Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

compnerd commented Oct 27, 2018

Uh oh!

compnerd commented Oct 28, 2018

Uh oh!

swift-ci commented Oct 28, 2018

Uh oh!

swift-ci commented Oct 28, 2018

Uh oh!

compnerd commented Oct 29, 2018

Uh oh!

swift-ci commented Oct 29, 2018

Uh oh!

swift-ci commented Oct 29, 2018

Uh oh!

compnerd commented Oct 29, 2018

Uh oh!

swift-ci commented Oct 29, 2018

Uh oh!

swift-ci commented Oct 29, 2018

Uh oh!

rjmccall commented Oct 29, 2018

Uh oh!

jckarter commented Oct 29, 2018

Uh oh!

compnerd commented Oct 29, 2018

Uh oh!

compnerd commented Oct 29, 2018

Uh oh!

gottesmm commented Oct 29, 2018

Uh oh!

rnk commented Oct 29, 2018

Uh oh!

compnerd commented Oct 29, 2018

Uh oh!

rjmccall commented Oct 29, 2018

Uh oh!

compnerd commented Nov 2, 2018

Uh oh!

compnerd commented Nov 4, 2018

Uh oh!

jckarter commented Nov 4, 2018

Uh oh!

compnerd commented Nov 4, 2018

Uh oh!

jckarter commented Nov 4, 2018

Uh oh!

compnerd commented Nov 4, 2018

Uh oh!

swift-ci commented Nov 4, 2018

Uh oh!

swift-ci commented Nov 4, 2018

Uh oh!

rnk commented Nov 5, 2018

Uh oh!

jckarter commented Nov 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

compnerd commented Nov 6, 2018

jckarter commented Nov 5, 2018 •

edited

Loading