Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make LLVMRustGetOrInsertGlobal always return a GlobalVariable #91070

Merged
merged 3 commits into from
Nov 21, 2021

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Nov 20, 2021

Module::getOrInsertGlobal returns a Constant*, which is a super
class of GlobalVariable, but if the given type doesn't match an
existing declaration, it returns a bitcast of that global instead.
This causes UB when we pass that to LLVMGetVisibility which
unconditionally casts the opaque argument to a GlobalValue*.

Instead, we can do our own get-or-insert without worrying whether
existing types match exactly. It's not relevant when we're just trying
to get/set the linkage and visibility, and if types are needed we can
bitcast or error nicely from rustc_codegen_llvm instead.

Fixes #91050, fixes #87933, fixes #87813.

`Module::getOrInsertGlobal` returns a `Constant*`, which is a super
class of `GlobalVariable`, but if the given type doesn't match an
existing declaration, it returns a bitcast of that global instead.
This causes UB when we pass that to `LLVMGetVisibility` which
unconditionally casts the opaque argument to a `GlobalValue*`.

Instead, we can do our own get-or-insert without worrying whether
existing types match exactly. It's not relevant when we're just trying
to get/set the linkage and visibility, and if types are needed we can
bitcast or error nicely from `rustc_codegen_llvm` instead.
@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 20, 2021
@cuviper
Copy link
Member Author

cuviper commented Nov 20, 2021

cc @datdenkikniet @haraldh -- if you could confirm that this fixes your bugs, that would be great. I'll start a build:

@bors try

@bors
Copy link
Contributor

bors commented Nov 20, 2021

⌛ Trying commit 023cc96 with merge 04339b3fab9c798bce7fed137e7f5ac81ab52509...

@cuviper
Copy link
Member Author

cuviper commented Nov 20, 2021

cc @hellow554 was also testing those bugs.

@bors
Copy link
Contributor

bors commented Nov 20, 2021

☀️ Try build successful - checks-actions
Build commit: 04339b3fab9c798bce7fed137e7f5ac81ab52509 (04339b3fab9c798bce7fed137e7f5ac81ab52509)

@nagisa

This comment has been minimized.

@bors

This comment has been minimized.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 20, 2021
@nagisa

This comment has been minimized.

@nagisa
Copy link
Member

nagisa commented Nov 20, 2021

Ah, actually, I do have a concern about what would happen with the code of the following shape (i.e. actual uses of the differently typed globals exist):

pub mod before {
    extern "C" {
        pub static GLOBAL1: [u8; 1];
    }

    pub unsafe fn do_something_with_array() -> u8 {
        GLOBAL1[0]
    }
}

pub mod inner {
    extern "C" {
        pub static GLOBAL1: u8;
    }

    pub unsafe fn call() -> u8 {
        GLOBAL1 + 42
    }
}

can we add a test like that too?


As I was busy coming up with the test case I built a compiler with this change, and indeed this case is problematic, building it with the following flags produces:

$ rustc test.rs --crate-type=rlib --emit=llvm-ir -Cno-prepopulate-passes
LLVM ERROR: Invalid LLVMRustVisibility value!

@bors r- for now

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 20, 2021
@cuviper
Copy link
Member Author

cuviper commented Nov 21, 2021

As I was busy coming up with the test case I built a compiler with this change, and indeed this case is problematic, building it with the following flags produces:

$ rustc test.rs --crate-type=rlib --emit=llvm-ir -Cno-prepopulate-passes
LLVM ERROR: Invalid LLVMRustVisibility value!

Are you sure your rustc command used my change? I can reproduce that error with nightly, but my local build works.

edit: it's also fine with the try build 04339b3fab9c798bce7fed137e7f5ac81ab52509 above.

Co-authored-by: Simonas Kazlauskas <git@kazlauskas.me>
@cuviper
Copy link
Member Author

cuviper commented Nov 21, 2021

@nagisa I added your test variant.

@nagisa
Copy link
Member

nagisa commented Nov 21, 2021

Are you sure your rustc command used my change?

/me facepalms. rustc was the rustup rustc and not ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc.

@bors r+ Thanks!

@bors
Copy link
Contributor

bors commented Nov 21, 2021

📌 Commit 3b2cfa5 has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 21, 2021
@rust-log-analyzer

This comment has been minimized.

@cuviper
Copy link
Member Author

cuviper commented Nov 21, 2021

Moved the tests to appease tidy.

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Nov 21, 2021

📌 Commit 3aa1954 has been approved by nagisa

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 21, 2021
…askrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#91008 (Adds IEEE 754-2019 minimun and maximum functions for f32/f64)
 - rust-lang#91070 (Make `LLVMRustGetOrInsertGlobal` always return a `GlobalVariable`)
 - rust-lang#91097 (Add spaces in opaque `impl Trait` with more than one trait)
 - rust-lang#91098 (Don't suggest certain fixups (`.field`, `.await`, etc) when reporting errors while matching on arrays )

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@datdenkikniet
Copy link

datdenkikniet commented Nov 21, 2021

A little late to the party perhaps, but this PR does indeed seem to fix the issues we faced in #87933

It's slightly difficult to tell because the error was spurious, but I've cleaned/built about 10 times now and it hasn't produced an error once, which it did do relatively consistently before these changes.

@bors bors merged commit df552b3 into rust-lang:master Nov 21, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 21, 2021
@glandium
Copy link
Contributor

Is it too late to get this in 1.57?

@cuviper
Copy link
Member Author

cuviper commented Nov 26, 2021

It is very late for 1.57, but possibly not too late -- @Mark-Simulacrum what do you think?

@nagisa
Copy link
Member

nagisa commented Nov 26, 2021

Given that it is fixing a relatively longstanding regression I would personally prefer this to ride the 🚆.

@Mark-Simulacrum
Copy link
Member

If it's nominated/accepted by beta backport quite soon (next ~72 hours), we can get it in, but not otherwise. I have no strong opinion or sense of the exact possibility for problems caused by this patch, though.

@cuviper
Copy link
Member Author

cuviper commented Nov 26, 2021

I'm OK with @nagisa's opinion to let it wait through the normal release process, but I might still backport in Fedora for its bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
10 participants