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

What should we guarantee regarding "sort-of unused" extern statics #54388

Closed
pnkfelix opened this issue Sep 20, 2018 · 7 comments
Closed

What should we guarantee regarding "sort-of unused" extern statics #54388

pnkfelix opened this issue Sep 20, 2018 · 7 comments
Assignees
Labels
A-linkage Area: linking into static, shared libraries and binaries T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Sep 20, 2018

Spawned off of #54188 (comment)

Right now we have a "test" that is auto-generated (via rustfix) that looks like this:

// compile-flags: -Z continue-parse-after-error
extern "C" {
static C: u8; //~ ERROR extern items cannot be `const`
}
fn main() {
// We suggest turning the (illegal) extern `const` into an extern `static`,
// but this also requires `unsafe` (a deny-by-default lint at comment time,
// future error; Issue #36247)
unsafe {
let _x = C;
}
}

Note that there aren't any definitions provided for that extern "C" { static C: u8; }; we haven't linked in any code that gives a definition for it.

Today, rustc will compile that code without complaint (as long as you don't add the -g to the command line). And you can even run it.

Likewise, cargo build --release will also compile without complaint: play

but cargo build (developer aka debug mode) says this (play):

   Compiling extern_const v0.1.0 (/tmp/extern_const)
     Running `rustc --edition=2018 --crate-name extern_const src/main.rs --color never --crate-type bin --emit=dep-info,link -C codegen-units=1 -C debuginfo=2 -C metadata=85ca601d97244906 -C extra-filename=-85ca601d972449\
06 --out-dir /tmp/extern_const/target/debug/deps -C incremental=/tmp/extern_const/target/debug/incremental -L dependency=/tmp/extern_const/target/debug/deps`
error: linking with `cc` failed: exit code: 1
  |
  = note: "cc" "-Wl,--as-needed" "-Wl,-z,noexecstack" "-m64" "-L" "/home/pnkfelix/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "/tmp/extern_const/target/debug/deps/extern_c\
onst-85ca601d97244906.1grotb0ctcuur2a3.rcgu.o" "/tmp/extern_const/target/debug/deps/extern_const-85ca601d97244906.2nfhy5xytludulsp.rcgu.o" "/tmp/extern_const/target/debug/deps/extern_const-85ca601d97244906.4otyorm6idcpq6f\
0.rcgu.o" "/tmp/extern_const/target/debug/deps/extern_const-85ca601d97244906.5faf0o72xf8hr32x.rcgu.o" "/tmp/extern_const/target/debug/deps/extern_const-85ca601d97244906.9rjqxqcpfdxtjec.rcgu.o" "/tmp/extern_const/target/de\
bug/deps/extern_const-85ca601d97244906.uxgx6titm1bphcw.rcgu.o" "-o" "/tmp/extern_const/target/debug/deps/extern_const-85ca601d97244906" "/tmp/extern_const/target/debug/deps/extern_const-85ca601d97244906.2q6157o69h749896.r\
cgu.o" "-Wl,--gc-sections" "-pie" "-Wl,-zrelro" "-Wl,-znow" "-nodefaultlibs" "-L" "/tmp/extern_const/target/debug/deps" "-L" "/home/pnkfelix/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-l\
inux-gnu/lib" "-Wl,--start-group" "-Wl,-Bstatic" "/home/pnkfelix/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd-3eb03432d1a557ee.rlib" "/home/pnkfelix/.rustup/toolchain\
s/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libpanic_unwind-77521309be1d38a9.rlib" "/home/pnkfelix/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/l\
ib/liballoc_jemalloc-cba7d140ba86aeeb.rlib" "/home/pnkfelix/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libunwind-2e1ca0447d72aed7.rlib" "/home/pnkfelix/.rustup/toolchains/\
nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liballoc_system-ad6b32d9e8e7bb10.rlib" "/home/pnkfelix/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib\
/liblibc-fb58cc2ace508689.rlib" "/home/pnkfelix/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liballoc-5f62eb2f1b8adf62.rlib" "/home/pnkfelix/.rustup/toolchains/nightly-x86_6\
4-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcore-8c2c57d12c24c7a1.rlib" "-Wl,--end-group" "/home/pnkfelix/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/li\
bcompiler_builtins-2d2930074aa4f43b.rlib" "-Wl,-Bdynamic" "-ldl" "-lrt" "-lpthread" "-lpthread" "-lgcc_s" "-lc" "-lm" "-lrt" "-lpthread" "-lutil" "-lutil"
  = note: /tmp/extern_const/target/debug/deps/extern_const-85ca601d97244906.9rjqxqcpfdxtjec.rcgu.o: In function `extern_const::main':
          /tmp/extern_const/src/main.rs:5: undefined reference to `C'
          collect2: error: ld returned 1 exit status

So my hypothesis is that LLVM is managing to optimize the code enough that it doesn't notice the missing extern symbol definiton.

(But for some reason, after applying #54188, in some scenarios it does start to notice the missing extern symbol definition.)

@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-linkage Area: linking into static, shared libraries and binaries labels Sep 20, 2018
@pnkfelix
Copy link
Member Author

pnkfelix commented Sep 20, 2018

My current assumptions:

  • we cannot actually make any guarantees about catching all cases like this,
  • but this test as written (the rustfix generated one) is just broken and we should not be attempting to link it.
  • The rustfix variant might qualify for // compile-pass (or perhaps it really should be // skip-trans, once support for that lands in the ui suite)
    • (But I don't think the // run-rustfix architecture supports that sort of .fixed-conditionalized property)
    • Another, perhaps simpler solution: add the necessary definition of the extern u8 C; to an auxiliary file, and leave the original as a normal (i.e. implicitly compile-fail) ui test.

@pnkfelix
Copy link
Member Author

nominating for discussion at compiler team meeting.

@nagisa
Copy link
Member

nagisa commented Sep 20, 2018

IMO not-a-bug.

It is possible to make this sort of case behave consistently regardless of the compilation mode (to make it consistently succeed, a weak definition could be provided, but that seems kind of insane thing to do; to make it consistently fail a non-optimisable reference to the declaration would have to be generated).

However, the current behaviour is already fairly consistent across all the toolchains I’ve used so far and has its own benefits as well (not having to generate extra code for example)

@pnkfelix
Copy link
Member Author

discussed at T-compiler meeting

But it was at the end of the meeting and I decided, based on the opinions put forward, we should not attempt to reach any final conclusions in a five minute discussion

@pnkfelix
Copy link
Member Author

@nagisa I just want to clarify: when you say "not-a-bug", you are saying the current behavior of rustc is not a bug in rustc, right?

(As opposed to, e.g., "the test is not buggy")

That is my reading of the paragraphs you wrote after the "not-a-bug", but I just wanted to double-check.

@nagisa
Copy link
Member

nagisa commented Sep 20, 2018 via email

@pnkfelix
Copy link
Member Author

based on the radio silence over past month, I'm concluding that the compiler's behavior here is not-buggy, and that the test itself should be revised in some manner (perhaps as outlined above) to avoid tripping on this.

@pnkfelix pnkfelix self-assigned this Oct 25, 2018
pnkfelix added a commit to pnkfelix/rust that referenced this issue Nov 6, 2018
As a drive-by, added `-g` to the compile-flags so that the test more
reliably fails to compile when the extern static in question is *not*
provided. (I.e. this is making the test more robust in the face of
potential future revisions.)

Fix rust-lang#54388.
kennytm added a commit to kennytm/rust that referenced this issue Nov 8, 2018
…error-from-rustfixed-code, r=alexcrichton

Sidestep link error from rustfix'ed code by using a *defined* static.

As a drive-by, added `-g` to the compile-flags so that the test more
reliably fails to compile when the extern static in question is *not*
provided. (I.e. this is making the test more robust in the face of
potential future revisions.)

Fix rust-lang#54388.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Nov 9, 2018
…error-from-rustfixed-code, r=alexcrichton

Sidestep link error from rustfix'ed code by using a *defined* static.

As a drive-by, added `-g` to the compile-flags so that the test more
reliably fails to compile when the extern static in question is *not*
provided. (I.e. this is making the test more robust in the face of
potential future revisions.)

Fix rust-lang#54388.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants