Make the `type_name` intrinsic deterministic #60166

oli-obk · 2019-04-22T11:57:12Z

cc @eddyb for the printing infrastructure
cc @Centril for the deterministic (coherent?) output

src/librustc_codegen_ssa/traits/intrinsic/type_name.rs

eddyb · 2019-04-22T15:25:35Z

I don't want to hold this back too much, but I'm not sure the printer as written is a good idea.
You might want to take a look at my "demangler expectation" one, especially print_type, but probably ignore all the higher-ranked lifetime stuff (and never put any lifetimes into type_name output, making it perma-unsound to use as a TypeId replacement - which I would hope we want).

Also, the implementation of the intrinsic should be in miri IMO (like all the other const fn intrinsics), and codegen backends should just get it from there.

oli-obk · 2019-04-22T15:51:04Z

I'm not sure the printer as written is a good idea.

Can you elaborate on that? I was inferring how to use the Printer from impls and chose the one that shares the most code with the rest of the printing infrastructure. The demangler printer looks to me like I'll essentially need to duplicate all the type printing logic.

ignore all the higher-ranked lifetime stuff (and never put any lifetimes into type_name output, making it perma-unsound to use as a TypeId replacement - which I would hope we want).

Yes definitely, I took care not to have any lifetime printing

Also, the implementation of the intrinsic should be in miri IMO (like all the other const fn intrinsics), and codegen backends should just get it from there.

That makes much more sense than what I'm doing right now. I'll do that

rust-highfive · 2019-04-22T15:54:28Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:006c93cc:start=1555943989932350954,finish=1555943991974953965,duration=2042603011
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[01:09:25] .................................................................................................... 900/2961
[01:09:41] .................................................................................................... 1000/2961
[01:09:54] .................................................................................................... 1100/2961
[01:10:04] .................................................................................................... 1200/2961
[01:10:14] ............F....................................................................................... 1300/2961
[01:10:39] .................................................................................................... 1500/2961
[01:10:48] .................................................................................i.................. 1600/2961
[01:11:01] .................................................................................................... 1700/2961
[01:11:15] .................................................................................................... 1800/2961
---
[01:14:19] ------------------------------------------
[01:14:19] stderr:
[01:14:19] ------------------------------------------
[01:14:19] thread 'main' panicked at 'assertion failed: `(left == right)`
[01:14:19]   left: `("[u8]", "str", "dyn core::markerSend", "issue_21058::NT", "issue_21058::DST")`,
[01:14:19]  right: `("[u8]", "str", "dyn core::marker::Send", "issue_21058::NT", "issue_21058::DST")`', /checkout/src/test/run-pass/issues/issue-21058.rs:10:5
[01:14:19] 
[01:14:19] ------------------------------------------
[01:14:19] 
[01:14:19] 
---
[01:14:19] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:517:22
[01:14:19] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:14:19] 
[01:14:19] 
[01:14:19] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/run-pass" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "run-pass" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:14:19] 
[01:14:19] 
[01:14:19] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:14:19] Build completed unsuccessfully in 0:11:01
[01:14:19] Build completed unsuccessfully in 0:11:01
[01:14:19] make: *** [check] Error 1
[01:14:19] Makefile:48: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:08225a60
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Mon Apr 22 15:54:22 UTC 2019
---
travis_time:end:1b0d045c:start=1555948464223013275,finish=1555948464227402371,duration=4389096
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:008c29d4
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:00839e79
travis_time:start:00839e79
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:08e6c818
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

sfackler · 2019-04-22T16:07:11Z

The test output changes seem reasonable, to me, but I don't think that I have enough expertise in this bit of the compiler to review.

r? @eddyb

src/test/run-pass/issues/issue-21058.rs

oli-obk · 2019-04-24T09:08:04Z

src/test/run-pass/issues/issue-21058.rs

+        // tuple
+        std::intrinsics::type_name::<(i32, u32)>(),
+        // impl trait
+        type_name_of_val(foo()),


impl trait prints the concrete type, so this will violate privacy

src/librustc_mir/interpret/intrinsics/type_name.rs

src/test/run-pass/issues/issue-21058.rs

src/test/run-pass/tydesc-name.rs

oli-obk · 2019-05-07T14:10:11Z

Addressed review comments

eddyb · 2019-05-14T17:06:24Z

src/librustc_mir/interpret/intrinsics/type_name.rs

+            | ty::Opaque(did, substs)
+            => self.print_value_path(did, substs),
+            ty::Closure(did, substs) => self.print_value_path(did, substs.substs),
+            ty::Generator(did, substs, _) => self.print_value_path(did, substs.substs),


I'm a bit surprised all of these are using print_value_path, I would've expected perhaps only FnDef to?

You said "everything with a DefId" :D

Generators and Closures are now platform independent (they printed their file and line info before)

Adt, Foreign, FnDef and Opaque are probably fine via the pretty_print_type path.

bors · 2019-05-20T06:39:05Z

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

bors · 2019-05-28T20:36:45Z

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

eddyb · 2019-05-28T22:27:46Z

r=me after a rebase

oli-obk · 2019-05-30T09:20:13Z

@bors r=eddyb

bors · 2019-05-30T09:20:14Z

📌 Commit 5b98489 has been approved by eddyb

@eddyb

Make the `type_name` intrinsic deterministic cc @eddyb for the printing infrastructure cc @Centril for the deterministic (coherent?) output r? @sfackler

@eddyb

Make the `type_name` intrinsic deterministic cc @eddyb for the printing infrastructure cc @Centril for the deterministic (coherent?) output r? @sfackler

bors · 2019-05-31T10:42:39Z

⌛ Testing commit 5b98489 with merge db4c783...

@eddyb

Make the `type_name` intrinsic deterministic cc @eddyb for the printing infrastructure cc @Centril for the deterministic (coherent?) output r? @sfackler

bors · 2019-05-31T13:23:52Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: eddyb
Pushing db4c783 to master...

RalfJung · 2019-05-31T15:50:09Z

src/librustc_mir/interpret/intrinsics.rs

@@ -14,6 +14,9 @@ use super::{
    Machine, PlaceTy, OpTy, InterpretCx,
 };

+mod type_name;
+
+pub use type_name::*;


Seems a bit funny to have that here when the intrinsic is not even available in CTFE?^^

RalfJung · 2019-05-31T15:50:36Z

src/librustc_codegen_llvm/intrinsic.rs

-                let ty_name = LocalInternedString::intern(&tp_ty.to_string());
-                self.const_str_slice(ty_name)
+                let ty_name = rustc_mir::interpret::type_name(self.tcx, tp_ty);
+                OperandRef::from_const(self, ty_name).immediate_or_packed_pair(self)


@oli-obk I assume Miri needs the same change then for its implementation of the intrinsic? Can you submit a PR?

I submitted a PR for this.

Uh, we can also just wait for #61399 to get merged, then miri will be able to just remove its implementation

rust-highfive assigned sfackler Apr 22, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 22, 2019

This comment has been minimized.

Sign in to view

eddyb reviewed Apr 22, 2019

View reviewed changes

src/librustc_codegen_ssa/traits/intrinsic/type_name.rs Outdated Show resolved Hide resolved

rust-highfive assigned eddyb and unassigned sfackler Apr 22, 2019

oli-obk mentioned this pull request Apr 23, 2019

"first" element check does nothing rust-lang/rustfmt#3519

Merged

This comment has been minimized.

Sign in to view

oli-obk commented Apr 24, 2019

View reviewed changes

src/test/run-pass/issues/issue-21058.rs Outdated Show resolved Hide resolved

oli-obk commented Apr 24, 2019

View reviewed changes

eddyb reviewed Apr 30, 2019

View reviewed changes

src/librustc_mir/interpret/intrinsics/type_name.rs Outdated Show resolved Hide resolved

eddyb reviewed Apr 30, 2019

View reviewed changes

src/test/run-pass/issues/issue-21058.rs Outdated Show resolved Hide resolved

eddyb reviewed Apr 30, 2019

View reviewed changes

src/test/run-pass/tydesc-name.rs Show resolved Hide resolved

oli-obk force-pushed the type_name branch from 9c8a09f to 499b2f8 Compare May 7, 2019 14:06

eddyb reviewed May 14, 2019

View reviewed changes

oli-obk force-pushed the type_name branch from 499b2f8 to b703a9e Compare May 26, 2019 23:04

eddyb approved these changes May 28, 2019

View reviewed changes

Symbol name predicate list printed + excessively

14178ef

oli-obk force-pushed the type_name branch from cedaf5e to 3029c8f Compare May 30, 2019 09:18

Make the type_name intrinsic's output deterministic

5b98489

oli-obk force-pushed the type_name branch from 3029c8f to 5b98489 Compare May 30, 2019 09:19

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 May 30, 2019

Centril mentioned this pull request May 30, 2019

Rollup of 3 pull requests #61354

Closed

Centril mentioned this pull request May 30, 2019

Rollup of 2 pull requests #61355

Closed

bors added a commit that referenced this pull request May 31, 2019

Auto merge of #60166 - oli-obk:type_name, r=eddyb

db4c783

Make the `type_name` intrinsic deterministic cc @eddyb for the printing infrastructure cc @Centril for the deterministic (coherent?) output r? @sfackler

bors added the merged-by-bors This PR was explicitly merged by bors. label May 31, 2019

bors merged commit 5b98489 into rust-lang:master May 31, 2019

This was referenced May 31, 2019

Introduce Rust symbol mangling scheme. #57967

Merged

Miri: give machine the chance to tag all allocations #61278

Merged

RalfJung reviewed May 31, 2019

View reviewed changes

This was referenced Jun 3, 2019

Remove _all_ codegen dependencies on rustc_mir 🎉 #61062

Merged

Tracking issue for RFC 1892, "Deprecate uninitialized in favor of a new MaybeUninit type" #53491

Closed

quininer mentioned this pull request Jun 16, 2019

type_name ICE #61894

Closed

oli-obk deleted the type_name branch March 16, 2021 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the `type_name` intrinsic deterministic #60166

Make the `type_name` intrinsic deterministic #60166

oli-obk commented Apr 22, 2019

This comment has been minimized.

eddyb commented Apr 22, 2019

oli-obk commented Apr 22, 2019

rust-highfive commented Apr 22, 2019

sfackler commented Apr 22, 2019

This comment has been minimized.

oli-obk Apr 24, 2019

oli-obk commented May 7, 2019

eddyb May 14, 2019

oli-obk May 14, 2019

bors commented May 20, 2019

bors commented May 28, 2019

eddyb commented May 28, 2019

oli-obk commented May 30, 2019

bors commented May 30, 2019

bors commented May 31, 2019

bors commented May 31, 2019

RalfJung May 31, 2019

RalfJung May 31, 2019

ecstatic-morse Jun 3, 2019

oli-obk Jun 3, 2019

Make the type_name intrinsic deterministic #60166

Make the type_name intrinsic deterministic #60166

Conversation

oli-obk commented Apr 22, 2019

This comment has been minimized.

eddyb commented Apr 22, 2019

oli-obk commented Apr 22, 2019

rust-highfive commented Apr 22, 2019

sfackler commented Apr 22, 2019

This comment has been minimized.

oli-obk Apr 24, 2019

Choose a reason for hiding this comment

oli-obk commented May 7, 2019

eddyb May 14, 2019

Choose a reason for hiding this comment

oli-obk May 14, 2019

Choose a reason for hiding this comment

bors commented May 20, 2019

bors commented May 28, 2019

eddyb commented May 28, 2019

oli-obk commented May 30, 2019

bors commented May 30, 2019

bors commented May 31, 2019

bors commented May 31, 2019

RalfJung May 31, 2019

Choose a reason for hiding this comment

RalfJung May 31, 2019

Choose a reason for hiding this comment

ecstatic-morse Jun 3, 2019

Choose a reason for hiding this comment

oli-obk Jun 3, 2019

Choose a reason for hiding this comment

Make the `type_name` intrinsic deterministic #60166

Make the `type_name` intrinsic deterministic #60166