Fix incorrect LLVM Linkage enum #36200

mattico · 2016-09-01T19:12:55Z

Followup of #33994 to actually work.

The Linkage enum in librustc_llvm got out of sync with the version in LLVM and it caused two variants of the #[linkage=""] attribute to break.

This adds the functions LLVMRustGetLinkage and LLVMRustSetLinkage which convert between the Rust Linkage enum and the LLVM one, which should stop this from breaking every time LLVM changes it.

Possible remaining concerns:

There could be a codegen test to make sure that the attributes are applied correctly (I don't know how to do this).
The test does not exercise the appending linkage. I can't figure out how to make a global static raw pointer to an array. This might not even be possible? If not we should probably remove appending linkage as its unusable in rust. Appending linkage is not 'emittable' anyway.
The test only runs on Linux.

Fixes #33992

r? @alexcrichton

rust-highfive · 2016-09-01T19:13:09Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

alexcrichton · 2016-09-02T01:24:20Z

@bors: r+ 5897dcc8bba54ebd382a0fbf7afa20ea7b790963

Thanks!

mattico · 2016-09-02T19:57:23Z

4ce90a2 - Fixed tidy

alexcrichton · 2016-09-02T20:11:21Z

@bors: r+

bors · 2016-09-02T20:11:22Z

📌 Commit 4ce90a2 has been approved by alexcrichton

@alexcrichton

…chton Fix incorrect LLVM Linkage enum Followup of rust-lang#33994 to actually work. The `Linkage` enum in librustc_llvm got out of sync with the version in LLVM and it caused two variants of the `#[linkage=""]` attribute to break. This adds the functions `LLVMRustGetLinkage` and `LLVMRustSetLinkage` which convert between the Rust Linkage enum and the LLVM one, which should stop this from breaking every time LLVM changes it. Possible remaining concerns: 1. There could be a codegen test to make sure that the attributes are applied correctly (I don't know how to do this). 2. The test does not exercise the `appending` linkage. I can't figure out how to make a global static raw pointer to an array. This might not even be possible? If not we should probably remove appending linkage as its unusable in rust. 3. The test only runs on Linux. Fixes rust-lang#33992 r? @alexcrichton

Rollup of 7 pull requests - Successful merges: #36070, #36132, #36200, #36212, #36225, #36231, #36234 - Failed merges:

TimNN · 2016-09-04T08:11:40Z

This failed as part of the rollup:

---- [run-pass] run-pass/issue-33992.rs stdout ----

error: compilation failed!
status: signal: 6
command: /buildslave/rust-buildbot/slave/auto-linux-64-opt-rustbuild/build/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc /buildslave/rust-buildbot/slave/auto-linux-64-opt-rustbuild/build/src/test/run-pass/issue-33992.rs -L /buildslave/rust-buildbot/slave/auto-linux-64-opt-rustbuild/build/obj/build/x86_64-unknown-linux-gnu/test/run-pass --target=x86_64-unknown-linux-gnu --error-format json -L /buildslave/rust-buildbot/slave/auto-linux-64-opt-rustbuild/build/obj/build/x86_64-unknown-linux-gnu/test/run-pass/issue-33992.stage2-x86_64-unknown-linux-gnu.run-pass.libaux -C prefer-dynamic -o /buildslave/rust-buildbot/slave/auto-linux-64-opt-rustbuild/build/obj/build/x86_64-unknown-linux-gnu/test/run-pass/issue-33992.stage2-x86_64-unknown-linux-gnu -Crpath -O -Lnative=/buildslave/rust-buildbot/slave/auto-linux-64-opt-rustbuild/build/obj/build/x86_64-unknown-linux-gnu/rust-test-helpers
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
Should never emit this
UNREACHABLE executed at /buildslave/rust-buildbot/slave/auto-linux-64-opt-rustbuild/build/src/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:336!

------------------------------------------

arielb1 · 2016-09-04T08:19:06Z

But LLVMLinkage is a part of the supposedly backwards-compatible C ABI! Should we wrap everything?

TimNN · 2016-09-04T08:19:35Z

To quote the llvm source file AsmPrinter.cpp:336:

  case GlobalValue::ExternalWeakLinkage:
    llvm_unreachable("Should never emit this");
  }

So it's probably just that one linkage type which needs to be removed from the test.

arielb1 · 2016-09-04T08:19:59Z

src/rustllvm/RustWrapper.cpp

+
+extern "C" unsigned LLVMRustGetLinkage(LLVMValueRef V) {
+    switch (LLVMGetLinkage(V)) {
+        case LLVMExternalLinkage:


Use an enum (see LLVMRustDiagnosticKind).

Rollup of 7 pull requests - Successful merges: #36070, #36132, #36200, #36212, #36225, #36231, #36234 - Failed merges:

mattico · 2016-09-04T14:17:53Z

@TimNN AppendingLinkage is part of the same case and should also be removed, I guess.

mattico · 2016-09-04T16:10:58Z

Updated to address issues.

arielb1 · 2016-09-04T16:36:04Z

src/librustc_llvm/ffi.rs

@@ -49,6 +49,10 @@ pub enum CallConv {

 /// LLVMLinkage
 ///
+/// This does not attempt to mirror the similar enum in LLVM, as this is fragile


nit: comment should be just /// LLVMRustLinkage - see e.g. LLVMRustAsmDialect. While you're at it, maybe also convert the other non-LLVMRust enums before they bite us?

The `Linkage` enum in librustc_llvm got out of sync with the version in LLVM and it caused two variants of the #[linkage=""] attribute to break. This adds the functions `LLVMRustGetLinkage` and `LLVMRustSetLinkage` which convert between the Rust Linkage enum and the LLVM one, which should stop this from breaking every time LLVM changes it. Fixes rust-lang#33992

mattico · 2016-09-04T21:47:25Z

Nits addressed, tests fixed, tidy passed, travis works! 🍀

arielb1 · 2016-09-05T12:39:48Z

@bors r+

bors · 2016-09-05T12:39:49Z

📌 Commit b9a8c1a has been approved by arielb1

bors · 2016-09-05T15:13:33Z

⌛ Testing commit b9a8c1a with merge 3f50b63...

@alexcrichton

Fix incorrect LLVM Linkage enum Followup of #33994 to actually work. The `Linkage` enum in librustc_llvm got out of sync with the version in LLVM and it caused two variants of the `#[linkage=""]` attribute to break. This adds the functions `LLVMRustGetLinkage` and `LLVMRustSetLinkage` which convert between the Rust Linkage enum and the LLVM one, which should stop this from breaking every time LLVM changes it. Possible remaining concerns: 1. There could be a codegen test to make sure that the attributes are applied correctly (I don't know how to do this). 2. ~~The test does not exercise the `appending` linkage. I can't figure out how to make a global static raw pointer to an array. This might not even be possible? If not we should probably remove appending linkage as its unusable in rust.~~ Appending linkage is not 'emittable' anyway. 3. The test only runs on Linux. Fixes #33992 r? @alexcrichton

bors · 2016-09-05T18:28:34Z

rust-highfive assigned alexcrichton Sep 1, 2016

mattico force-pushed the fix-llvm-linkage branch from 5897dcc to 4ce90a2 Compare September 2, 2016 19:57

Manishearth mentioned this pull request Sep 4, 2016

Rollup of 7 pull requests #36255

Merged

bors added a commit that referenced this pull request Sep 4, 2016

Auto merge of #36255 - Manishearth:rollup, r=Manishearth

0e5c96b

Rollup of 7 pull requests - Successful merges: #36070, #36132, #36200, #36212, #36225, #36231, #36234 - Failed merges:

arielb1 reviewed Sep 4, 2016
View reviewed changes

bors added a commit that referenced this pull request Sep 4, 2016

Auto merge of #36255 - Manishearth:rollup, r=Manishearth

987b475

Rollup of 7 pull requests - Successful merges: #36070, #36132, #36200, #36212, #36225, #36231, #36234 - Failed merges:

mattico force-pushed the fix-llvm-linkage branch from 4ce90a2 to cd6cad1 Compare September 4, 2016 16:08

arielb1 reviewed Sep 4, 2016
View reviewed changes

mattico force-pushed the fix-llvm-linkage branch from cd6cad1 to 71c9d11 Compare September 4, 2016 17:19

mattico force-pushed the fix-llvm-linkage branch from 71c9d11 to b9a8c1a Compare September 4, 2016 21:12

bors merged commit b9a8c1a into rust-lang:master Sep 5, 2016

mattico deleted the fix-llvm-linkage branch September 5, 2016 18:30

Zalathar mentioned this pull request Oct 26, 2024

Replace some LLVMRust wrappers with calls to the LLVM C API #132167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect LLVM Linkage enum #36200

Fix incorrect LLVM Linkage enum #36200

mattico commented Sep 1, 2016 •

edited

Loading

rust-highfive commented Sep 1, 2016

alexcrichton commented Sep 2, 2016

mattico commented Sep 2, 2016

alexcrichton commented Sep 2, 2016

bors commented Sep 2, 2016

TimNN commented Sep 4, 2016 •

edited

Loading

arielb1 commented Sep 4, 2016

TimNN commented Sep 4, 2016 •

edited

Loading

arielb1 Sep 4, 2016

mattico commented Sep 4, 2016

mattico commented Sep 4, 2016

arielb1 Sep 4, 2016

mattico commented Sep 4, 2016

arielb1 commented Sep 5, 2016

bors commented Sep 5, 2016

bors commented Sep 5, 2016

bors commented Sep 5, 2016

Fix incorrect LLVM Linkage enum #36200

Fix incorrect LLVM Linkage enum #36200

Conversation

mattico commented Sep 1, 2016 • edited Loading

rust-highfive commented Sep 1, 2016

alexcrichton commented Sep 2, 2016

mattico commented Sep 2, 2016

alexcrichton commented Sep 2, 2016

bors commented Sep 2, 2016

TimNN commented Sep 4, 2016 • edited Loading

arielb1 commented Sep 4, 2016

TimNN commented Sep 4, 2016 • edited Loading

arielb1 Sep 4, 2016

Choose a reason for hiding this comment

mattico commented Sep 4, 2016

mattico commented Sep 4, 2016

arielb1 Sep 4, 2016

Choose a reason for hiding this comment

mattico commented Sep 4, 2016

arielb1 commented Sep 5, 2016

bors commented Sep 5, 2016

bors commented Sep 5, 2016

bors commented Sep 5, 2016

mattico commented Sep 1, 2016 •

edited

Loading

TimNN commented Sep 4, 2016 •

edited

Loading

TimNN commented Sep 4, 2016 •

edited

Loading