Use weak_odr linkage when reusing definitions across codegen units #32557

dotdash · 2016-03-28T16:01:06Z

When reuing a definition across codegen units, we obviously cannot use
internal linkage, but using external linkage means that we can end up
with multiple conflicting definitions of a single symbol across
multiple crates. Since the definitions should all be equal
semantically, we can use weak_odr linkage to resolve the situation.

Fixes #32518

r? @nikomatsakis

nikomatsakis · 2016-03-28T16:11:34Z

src/librustc_trans/monomorphize.rs

@@ -139,6 +139,8 @@ pub fn monomorphic_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,

            if trans_everywhere || is_first {
                trans_fn(ccx, decl, body, lldecl, psubsts, fn_node_id);
+            } else {
+                llvm::SetLinkage(lldecl, llvm::ExternalLinkage);


as we said on IRC, this is subtle -- comment would be good

nikomatsakis · 2016-03-28T16:11:39Z

r=me w/ comment

dotdash · 2016-03-28T16:19:32Z

@bors r=nikomatsakis

bors · 2016-03-28T16:19:33Z

📌 Commit fdaea82 has been approved by nikomatsakis

nikomatsakis · 2016-03-28T19:56:43Z

can't really see what travis is complaining about; looks spurious

michaelwoerister · 2016-03-28T20:22:29Z

The LLVM document says

Unreferenced linkonce globals are allowed to be discarded.

Could that be the problem for these undefined reference errors?

michaelwoerister · 2016-03-28T20:23:22Z

weak_odr linkage might be a better fit then...

dotdash · 2016-03-28T20:46:18Z

No, it's probably just that linkonce_odr must only be used for things that are actually translated in every crate, not everything. IOW update_linkage was the wrong place to do this.

michaelwoerister · 2016-03-28T20:50:21Z

base::internalize_symbols might also be something to look at.

dotdash · 2016-03-28T20:58:21Z

Actually, you're right about using weak_odr. The whole point is that with codegen units, we're not even translating generics in every unit. D'oh!

And I should adjust base::internalize_symbols as well. I already had that and thought I had undone it for a reason, but I can't think of anything, so probably an accident.

nikomatsakis · 2016-03-28T21:55:43Z

@bors r+ p=1

bors · 2016-03-28T21:55:43Z

📌 Commit 88cef8f has been approved by nikomatsakis

nikomatsakis · 2016-03-28T22:05:38Z

@bors r-

per conversation on IRC, this should have some sort of comment, since the boolean logic is pretty subtle, and the meaning of these flags not 100% obvious

nikomatsakis · 2016-03-28T23:06:59Z

much better :)

nikomatsakis · 2016-03-28T23:07:04Z

@bors r+ p=1

bors · 2016-03-28T23:07:04Z

📌 Commit 5fa4b63 has been approved by nikomatsakis

bors · 2016-03-29T00:25:43Z

⌛ Testing commit 5fa4b63 with merge 2aef71e...

bors · 2016-03-29T01:34:49Z

💔 Test failed - auto-win-gnu-64-nopt-t

alexcrichton · 2016-03-29T04:35:32Z

Hm I wonder if the weak odr linkage isn't supported on all platforms? It sounds like something that'd be Linux only unfortunately :(

dotdash · 2016-03-29T13:15:30Z

Turns out that you need to use comdat sections to make this work.

http://www.airs.com/blog/archives/52 has some details.

michaelwoerister · 2016-03-29T13:29:33Z

From the LLVM documentation on comdat sections:

Note that the Mach-O platform doesn’t support COMDATs

Do you know how this affects us? How do they do C++ templates there?

dotdash · 2016-03-29T13:34:49Z

@michaelwoerister The check in the LLVMRustSetComdat function takes care not to set comdat for Mach-O. Weak/Linkonce_ODR + Comdat is what Clang does. I suppose that weak_odr already does the trick for Mach-O as it does for ELF.

nikomatsakis · 2016-03-29T13:36:55Z

src/librustc_llvm/lib.rs

@@ -2149,6 +2151,12 @@ pub fn SetLinkage(global: ValueRef, link: Linkage) {
    }
 }

+pub fn SetUniqueComdat(llmod: ModuleRef, val: ValueRef) {
+    unsafe {
+        LLVMRustSetComdat(llmod, val, LLVMGetValueName(val));


this function needs a comment; at minimum, it should link to that blog post, but also: why does it make sense to use LLVMGetValueName here?

I would expect the string to be the symbol name or something similar -- does that happen to be what LLVMGetValueName is returning here?

Yes, the returned string is the name of the function.

nikomatsakis · 2016-03-29T13:52:09Z

@bors r+

bors · 2016-03-29T13:52:10Z

📌 Commit 0192e54 has been approved by nikomatsakis

bors · 2016-03-29T13:52:12Z

⌛ Testing commit 0192e54 with merge 5c50dff...

michaelwoerister · 2016-03-29T14:28:52Z

@dotdash OK, sounds good then!

dotdash · 2016-03-29T14:44:32Z

@bors r- Forgot to unset comdat in internalize_symbols

When reuing a definition across codegen units, we obviously cannot use internal linkage, but using external linkage means that we can end up with multiple conflicting definitions of a single symbol across multiple crates. Since the definitions should all be equal semantically, we can use weak_odr linkage to resolve the situation. Fixes rust-lang#32518

dotdash · 2016-03-29T14:45:27Z

@bors r=nikomatsakis

bors · 2016-03-29T14:45:27Z

📌 Commit 22f4587 has been approved by nikomatsakis

alexcrichton · 2016-03-29T15:00:38Z

Nice find! I've always wondered what comdats are...

bors · 2016-03-29T15:02:52Z

⌛ Testing commit 22f4587 with merge b3d940a...

bors · 2016-03-29T15:04:50Z

💔 Test failed - auto-mac-ios-opt

alexcrichton · 2016-03-29T15:10:23Z

@bors: retry

On Tue, Mar 29, 2016 at 8:04 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-ios-opt
http://buildbot.rust-lang.org/builders/auto-mac-ios-opt/builds/708

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#32557 (comment)

bors · 2016-03-29T18:10:40Z

⌛ Testing commit 22f4587 with merge 8f5c3f1...

@nikomatsakis

Use weak_odr linkage when reusing definitions across codegen units When reuing a definition across codegen units, we obviously cannot use internal linkage, but using external linkage means that we can end up with multiple conflicting definitions of a single symbol across multiple crates. Since the definitions should all be equal semantically, we can use weak_odr linkage to resolve the situation. Fixes #32518 r? @nikomatsakis

bors · 2016-03-29T21:27:42Z

michaelwoerister · 2016-03-29T21:30:14Z

💖

nikomatsakis · 2016-03-29T22:40:01Z

@dotdash you suggested on IRC that there may still be further problems in this area -- any update on that?

dotdash · 2016-03-29T22:52:35Z

I think that was just about not unsetting the comdat section when
internalizing after trans has finished.

I didn't notify about the fix for that on IRC, but it's on the merged
commit.
Am 30.03.2016 00:40 schrieb "Niko Matsakis" notifications@github.com:

@dotdash https://github.com/dotdash you suggested on IRC that there may
still be further problems in this area -- any update on that?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#32557 (comment)

brson · 2016-04-11T22:50:50Z

In my previous experiments weak_odr functions didn't optimize as well as internal functions so this may be a perf regression.

Edit: oh this is changing from external to weak_odr, not internal to weak_odr.

rust-highfive assigned nikomatsakis Mar 28, 2016

nikomatsakis reviewed Mar 28, 2016
View reviewed changes

dotdash force-pushed the issue-32518 branch from 7961ca3 to fdaea82 Compare March 28, 2016 16:18

michaelwoerister mentioned this pull request Mar 28, 2016

mk: Add -C metadata for compiling crates we ship #32560

Merged

dotdash force-pushed the issue-32518 branch from fdaea82 to dbbd921 Compare March 28, 2016 20:48

dotdash force-pushed the issue-32518 branch 2 times, most recently from 023a1b3 to 88cef8f Compare March 28, 2016 21:13

dotdash changed the title ~~Use linkonce_odr linkage when reusing definitions across codegen units~~ Use weak_odr linkage when reusing definitions across codegen units Mar 28, 2016

dotdash force-pushed the issue-32518 branch 3 times, most recently from 673b1d4 to 5fa4b63 Compare March 28, 2016 23:05

dotdash force-pushed the issue-32518 branch from 4c317f1 to 02959e8 Compare March 29, 2016 13:12

nikomatsakis reviewed Mar 29, 2016
View reviewed changes

dotdash force-pushed the issue-32518 branch from 02959e8 to 0192e54 Compare March 29, 2016 13:46

dotdash force-pushed the issue-32518 branch from 0192e54 to 22f4587 Compare March 29, 2016 14:45

bors merged commit 22f4587 into rust-lang:master Mar 29, 2016

dotdash deleted the issue-32518 branch May 17, 2016 03:44

Use weak_odr linkage when reusing definitions across codegen units #32557

Use weak_odr linkage when reusing definitions across codegen units #32557

Conversation

dotdash commented Mar 28, 2016

nikomatsakis Mar 28, 2016

Choose a reason for hiding this comment

nikomatsakis commented Mar 28, 2016

dotdash commented Mar 28, 2016

bors commented Mar 28, 2016

nikomatsakis commented Mar 28, 2016

michaelwoerister commented Mar 28, 2016

michaelwoerister commented Mar 28, 2016

dotdash commented Mar 28, 2016

michaelwoerister commented Mar 28, 2016

dotdash commented Mar 28, 2016

nikomatsakis commented Mar 28, 2016

bors commented Mar 28, 2016

nikomatsakis commented Mar 28, 2016

nikomatsakis commented Mar 28, 2016

nikomatsakis commented Mar 28, 2016

bors commented Mar 28, 2016

bors commented Mar 29, 2016

bors commented Mar 29, 2016

alexcrichton commented Mar 29, 2016

dotdash commented Mar 29, 2016

michaelwoerister commented Mar 29, 2016

dotdash commented Mar 29, 2016

nikomatsakis Mar 29, 2016

Choose a reason for hiding this comment

dotdash Mar 29, 2016

Choose a reason for hiding this comment

nikomatsakis commented Mar 29, 2016

bors commented Mar 29, 2016

bors commented Mar 29, 2016

michaelwoerister commented Mar 29, 2016

dotdash commented Mar 29, 2016

dotdash commented Mar 29, 2016

bors commented Mar 29, 2016

alexcrichton commented Mar 29, 2016

bors commented Mar 29, 2016

bors commented Mar 29, 2016

alexcrichton commented Mar 29, 2016

bors commented Mar 29, 2016

bors commented Mar 29, 2016

michaelwoerister commented Mar 29, 2016

nikomatsakis commented Mar 29, 2016

dotdash commented Mar 29, 2016

brson commented Apr 11, 2016