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.

Sign up for GitHub

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

Jump to bottom

fix static_ptr_ty for foreign statics #78101

Merged
merged 3 commits into from
Oct 21, 2020
Merged

fix static_ptr_ty for foreign statics #78101

merged 3 commits into from
Oct 21, 2020

Conversation

RalfJung
Copy link
Member

Cc #74840

This does not fix that issue but fixes a problem in static_ptr_ty that we noticed while discussing that issue. I also added and updated a few comments. The one about internal locals being ignored does not seem to have been true even in the commit that introduced it.

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 19, 2020
@RalfJung
Copy link
Member Author

However, the test suite currently fails with an ICE:

---- [ui] ui/threads-sendsync/thread-local-extern-static.rs stdout ----

error: test compilation failed although it shouldn't!
status: exit code: 101
command: "/home/r/src/rust/rustc.2/build/x86_64-unknown-linux-gnu/stage1/bin/rustc" "/home/r/src/rust/rustc.2/src/test/ui/threads-sendsync/thread-local-extern-static.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-C" "prefer-dynamic" "-o" "/home/r/src/rust/rustc.2/build/x86_64-unknown-linux-gnu/test/ui/threads-sendsync/thread-local-extern-static/a" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/home/r/src/rust/rustc.2/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/home/r/src/rust/rustc.2/build/x86_64-unknown-linux-gnu/test/ui/threads-sendsync/thread-local-extern-static/auxiliary"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
error: internal compiler error: broken MIR in DefId(0:7 ~ thread_local_extern_static[317d]::main) (_6 = &/*tls*/ FOO): bad assignment (*const std::cell::Cell<u32> = &'static std::cell::Cell<u32>): NoSolution
  --> /home/r/src/rust/rustc.2/src/test/ui/threads-sendsync/thread-local-extern-static.rs:22:20
   |
LL |         assert_eq!(FOO.get(), 3);
   |                    ^^^
   |
   = note: delayed at compiler/rustc_mir/src/borrow_check/type_check/mod.rs:253:27

error: internal compiler error: broken MIR in Item(WithOptConstParam { did: DefId(0:7 ~ thread_local_extern_static[317d]::main), const_param_did: None }) (end of phase Optimization) at bb0[5]:
encountered `Assign((_5, &/*tls*/ FOO))` with incompatible types:
left-hand side has type: *const Cell<u32>
right-hand side has type: &'static Cell<u32>
  --> /home/r/src/rust/rustc.2/src/test/ui/threads-sendsync/thread-local-extern-static.rs:22:20
   |
LL |         assert_eq!(FOO.get(), 3);
   |                    ^^^
   |
   = note: delayed at compiler/rustc_mir/src/transform/validate.rs:156:36

Looks like something somewhere is not using static_ptr_ty to determine the type of a thread-local extern static and thus causes different types to be used on the two sides.

@RalfJung
Copy link
Member Author

Or maybe it is the other way around -- convert_path_expr now uses raw pointers for extern thread-local refs but something else still expects that to be a normal reference. Thread-local refs have their own ExprKind and I don't know how that works.

@RalfJung
Copy link
Member Author

I think I found where the wrong type was coming from.

oli-obk
oli-obk reviewed Oct 20, 2020
View reviewed changes
compiler/rustc_middle/src/mir/tcx.rs
Comment on lines +155 to +162
let static_ty = tcx.type_of(did);
if tcx.is_mutable_static(did) {
tcx.mk_mut_ptr(tcx.type_of(did))
tcx.mk_mut_ptr(static_ty)
} else if tcx.is_foreign_item(did) {
tcx.mk_imm_ptr(static_ty)
} else {
tcx.mk_imm_ref(tcx.lifetimes.re_static, tcx.type_of(did))
// FIXME: These things don't *really* have 'static lifetime.
tcx.mk_imm_ref(tcx.lifetimes.re_static, static_ty)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be deduplicated with the other site by putting it in a function. I see no danger in eagerly normalizing static items' types (so doing it here, too).

Copy link
Member Author

@RalfJung RalfJung Oct 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that at first, but the lifetime differs. That lead to ICEs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uff, right

@oli-obk
Copy link
Contributor

oli-obk commented Oct 20, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 20, 2020

📌 Commit 153e843 has been approved by oli-obk

@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 Oct 20, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 21, 2020
@JohnTitor
Rollup merge of rust-lang#78101 - RalfJung:foreign-static, r=oli-obk
dbb36ab 
fix static_ptr_ty for foreign statics

Cc rust-lang#74840

This does not fix that issue but fixes a problem in `static_ptr_ty` that we noticed while discussing that issue. I also added and updated a few comments. The one about `internal` locals being ignored does not seem to have been true [even in the commit that introduced it](https://github.com/rust-lang/rust/pull/44700/files#diff-ae2f3c7e2f9744f7ef43e96072b10e98d4e3fe74a3a399a3ad8a810fbe56c520R139).

r? @oli-obk
@JohnTitor JohnTitor mentioned this pull request Oct 21, 2020
Closed
@JohnTitor JohnTitor mentioned this pull request Oct 21, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 21, 2020
@bors
Auto merge of rust-lang#78178 - JohnTitor:rollup-dslazzj, r=JohnTitor
1935645 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#77726 (Add Pin::static_ref, static_mut.)
 - rust-lang#78002 (Tweak "object unsafe" errors)
 - rust-lang#78056 (BTreeMap: split off most code of remove and split_off)
 - rust-lang#78063 (Improve wording of "cannot multiply" type error)
 - rust-lang#78094 (rustdoc: Show the correct source filename in page titles, without `.html`)
 - rust-lang#78101 (fix static_ptr_ty for foreign statics)
 - rust-lang#78118 (Inline const followups)

Failed merges:

r? `@ghost`
@bors bors merged commit 83f126b into rust-lang:master Oct 21, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 21, 2020
@RalfJung RalfJung deleted the foreign-static branch October 24, 2020 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.49.0
Development

Successfully merging this pull request may close these issues.
5 participants
@RalfJung @oli-obk @bors @rust-highfive @rustbot