The war of symbol and symbol (the PR dedicated to duplicate symbols breaking rustc in unexpected ways) #23011

nagisa · 2015-03-03T23:11:50Z

We provide tools to tell what exact symbols to emit for any fn or static, but
don’t quite check if that won’t cause any issues later on. Some of the issues
include LLVM mangling our names again and our names pointing to wrong locations,
us generating dumb foreign call wrappers, linker errors, extern functions
resolving to different symbols altogether (extern {fn fail();} fail(); in some
cases calling fail1()), etc.

Before the commit we had a function called note_unique_llvm_symbol, so it is
clear somebody was aware of the issue at some point, but the function was barely
used, mostly in irrelevant locations.

Along with working on it I took liberty to start refactoring trans/base into
a few smaller modules. The refactoring is incomplete and I hope I will find some
motivation to carry on with it.

This is possibly a [breaking-change] because it makes dumbly written code
properly invalid.

This fixes all those issues about incorrect use of #[no_mangle] being not reported/misreported/ICEd by the compiler.

NB. This PR does not attempt to tackle the parallel codegen issue that was mentioned in #22811, but I believe it should be very straightforward in a follow up PR by modifying trans::declare::get_defined_value to look at all the contexts.

cc @alexcrichton @huonw @nrc because you commented on the original RFC issue.

EDIT: wow, this became much bigger than I initially intended.

rust-highfive · 2015-03-03T23:12:01Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

nagisa · 2015-03-03T23:18:50Z

src/librustc_trans/trans/base.rs

NB: before the move of this block from finish_register_fn we reported “compilation successful” error before we created the entry wrapper or actually translated contents of the entry function.

alexcrichton · 2015-03-04T04:17:10Z

This seems like a fine refactoring to me, thanks @nagisa! I'm not trans hacker though so I'm not comfortable r+'ing

nagisa · 2015-03-04T09:56:56Z

Thanks for taking a look @alexcrichton!

arielb1 · 2015-03-05T22:24:57Z

src/librustc_trans/trans/attributes.rs

Why the double negative?

This function has only been moved and renamed from base::get_fn_llvm_attributes only. The body has seen no changes (other than variable renames, of course).

Nicely spotted, nevertheless. I will go ahead and fix this.

bors · 2015-03-12T22:49:23Z

☔ The latest upstream changes (presumably #23265) made this pull request unmergeable. Please resolve the merge conflicts.

pnkfelix · 2015-03-13T01:35:35Z

(i'm going to be out until wedneday; but I will try to get to this then. sorry for the absurd delay; maybe you can rebase in the meantime @nagisa )

bors · 2015-03-13T20:27:51Z

☔ The latest upstream changes (presumably #23337) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2015-03-17T23:59:19Z

☔ The latest upstream changes (presumably #23376) made this pull request unmergeable. Please resolve the merge conflicts.

nagisa · 2015-03-20T13:09:45Z

Rebased. I didn’t even notice the last breakage until now, didn’t get github mail.

bors · 2015-03-24T21:03:07Z

☔ The latest upstream changes (presumably #23654) made this pull request unmergeable. Please resolve the merge conflicts.

nagisa · 2015-03-26T18:51:03Z

Rebased.

pnkfelix · 2015-03-28T17:53:47Z

@nagisa my plan is to review this right after the beta-release, is that okay with you?

nagisa · 2015-03-28T17:55:41Z

Sure, been waiting for a month, can easily wait a few weeks more. I’ll have to rigorously review the patch for bitrot, though.

pnkfelix · 2015-03-28T18:36:08Z

@nagisa yeah, sorry about the delay...

bors · 2015-04-01T14:49:54Z

☔ The latest upstream changes (presumably #23936) made this pull request unmergeable. Please resolve the merge conflicts.

It emits the same symbol – `transmute` – from the same crate twice.

This discovers another class of mis-trans where we wrap multiple native functions into a single wrapper, which is wrong.

This commit causes no change in trans semantics, it just moves some functions around and deduplicates them.

These functions have only a single use and functionally belong to foreign and glue respectively anyway

We provide tools to tell what exact symbols to emit for any fn or static, but don’t quite check if that won’t cause any issues later on. Some of the issues include LLVM mangling our names again and our names pointing to wrong locations, us generating dumb foreign call wrappers, linker errors, extern functions resolving to different symbols altogether (extern {fn fail();} fail(); in some cases calling fail1()), etc. Before the commit we had a function called note_unique_llvm_symbol, so it is clear somebody was aware of the issue at some point, but the function was barely used, mostly in irrelevant locations. Along with working on it I took liberty to start refactoring trans/base into a few smaller modules. The refactoring is incomplete and I hope I will find some motivation to carry on with it. This is possibly a [breaking-change] because it makes dumbly written code properly invalid.

pnkfelix · 2015-04-12T01:26:51Z

@bors r+

bors · 2015-04-12T01:26:52Z

📌 Commit 000db38 has been approved by pnkfelix

bors · 2015-04-12T01:26:54Z

⌛ Testing commit 000db38 with merge 5afa270...

@alexcrichton

We provide tools to tell what exact symbols to emit for any fn or static, but don’t quite check if that won’t cause any issues later on. Some of the issues include LLVM mangling our names again and our names pointing to wrong locations, us generating dumb foreign call wrappers, linker errors, extern functions resolving to different symbols altogether (`extern {fn fail();} fail();` in some cases calling `fail1()`), etc. Before the commit we had a function called `note_unique_llvm_symbol`, so it is clear somebody was aware of the issue at some point, but the function was barely used, mostly in irrelevant locations. Along with working on it I took liberty to start refactoring trans/base into a few smaller modules. The refactoring is incomplete and I hope I will find some motivation to carry on with it. This is possibly a [breaking-change] because it makes dumbly written code properly invalid. This fixes all those issues about incorrect use of #[no_mangle] being not reported/misreported/ICEd by the compiler. NB. This PR does not attempt to tackle the parallel codegen issue that was mentioned in #22811, but I believe it should be very straightforward in a follow up PR by modifying `trans::declare::get_defined_value` to look at all the contexts. cc @alexcrichton @huonw @nrc because you commented on the original RFC issue. EDIT: wow, this became much bigger than I initially intended.

bors · 2015-04-12T04:28:54Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

rust-highfive assigned pnkfelix Mar 3, 2015

nagisa changed the title ~~The war of symbol and symbol~~ The war of symbol and symbol (the PR dedicated to duplicate symbols breaking rustc in unexpected ways) Mar 3, 2015

nagisa reviewed Mar 3, 2015
View reviewed changes

nagisa force-pushed the the-war-of-symbol-and-symbol branch from 4ac3765 to e1f7a6b Compare March 4, 2015 09:52

arielb1 reviewed Mar 5, 2015
View reviewed changes

nagisa force-pushed the the-war-of-symbol-and-symbol branch from 07e593c to caa5001 Compare March 13, 2015 11:10

nagisa force-pushed the the-war-of-symbol-and-symbol branch from caa5001 to e935301 Compare March 13, 2015 21:04

nagisa force-pushed the the-war-of-symbol-and-symbol branch from e935301 to 46b0b82 Compare March 20, 2015 13:09

nagisa force-pushed the the-war-of-symbol-and-symbol branch from 46b0b82 to 91e9f9d Compare March 26, 2015 18:50

nagisa added 3 commits April 3, 2015 15:37

Fix a broken test

b9a11f0

It emits the same symbol – `transmute` – from the same crate twice.

Validate export_name attribute

723ca4b

Wrap LLVM’s Module::getNamedValue

2198969

nagisa added 10 commits April 3, 2015 15:46

Abort creating wrapper fn for multiple inner fns

baa52ca

This discovers another class of mis-trans where we wrap multiple native functions into a single wrapper, which is wrong.

Add the tests for duplicate symbol errors

d36c4db

Extract attribute handling code into a module

c71970e

This commit causes no change in trans semantics, it just moves some functions around and deduplicates them.

Move get_extern_fn and get_res_dtor out of base

cb45703

These functions have only a single use and functionally belong to foreign and glue respectively anyway

Implement LLVMGetOrInsertGlobal wrapper

deb097a

Move a big attribute fn into trans::attributes

caea044

Update tests to match the messages

8943709

Address Alex’s comments

d8d59a9

Remove two cases of unecesary match branches

ea04cdf

nagisa force-pushed the the-war-of-symbol-and-symbol branch from 91e9f9d to 16881af Compare April 3, 2015 12:48

Rebase fallout

000db38

nagisa force-pushed the the-war-of-symbol-and-symbol branch from 16881af to 000db38 Compare April 3, 2015 21:22

bors merged commit 000db38 into rust-lang:master Apr 12, 2015

bors mentioned this pull request Apr 12, 2015

Refactor coercions/adjustments #24261

Merged

nagisa mentioned this pull request Apr 19, 2015

rustc crash when #[no_mangle] function is declared multiple times with different signatures. #24583

Closed

japaric mentioned this pull request Mar 19, 2016

ARM compilers always hit an LLVM assertion (if enabled) during codegen. #32360

Closed

nagisa mentioned this pull request Feb 23, 2020

Miscompilation with clashing symbol fn names of different types #69390

Closed

Uh oh!

The war of symbol and symbol (the PR dedicated to duplicate symbols breaking rustc in unexpected ways) #23011

The war of symbol and symbol (the PR dedicated to duplicate symbols breaking rustc in unexpected ways) #23011

Uh oh!

Conversation

nagisa commented Mar 3, 2015

Uh oh!

rust-highfive commented Mar 3, 2015

Uh oh!

nagisa Mar 3, 2015

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Mar 4, 2015

Uh oh!

nagisa commented Mar 4, 2015

Uh oh!

arielb1 Mar 5, 2015

Choose a reason for hiding this comment

Uh oh!

nagisa Mar 6, 2015

Choose a reason for hiding this comment

Uh oh!

bors commented Mar 12, 2015

Uh oh!

pnkfelix commented Mar 13, 2015

Uh oh!

bors commented Mar 13, 2015

Uh oh!

bors commented Mar 17, 2015

Uh oh!

nagisa commented Mar 20, 2015

Uh oh!

bors commented Mar 24, 2015

Uh oh!

nagisa commented Mar 26, 2015

Uh oh!

pnkfelix commented Mar 28, 2015

Uh oh!

nagisa commented Mar 28, 2015

Uh oh!

pnkfelix commented Mar 28, 2015

Uh oh!

bors commented Apr 1, 2015

Uh oh!

pnkfelix commented Apr 12, 2015

Uh oh!

bors commented Apr 12, 2015

Uh oh!

bors commented Apr 12, 2015

Uh oh!

bors commented Apr 12, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants