Less conservative uninhabitedness check #54125

varkor · 2018-09-11T12:05:36Z

Extends the uninhabitedness check to structs, non-empty enums, tuples and arrays.

Pulled out of #47291 and #50262.

Fixes #54586.

r? @nikomatsakis

src/librustc/ty/sty.rs

src/librustc_codegen_llvm/mir/block.rs

src/librustc_codegen_llvm/debuginfo/mod.rs

src/librustc_codegen_llvm/declare.rs

eddyb · 2018-09-14T19:01:47Z

I'm not sure I understand why places which checked .abi == Abi::Uninhabited where changed - an user-facing type-check-level "weak equivalence" to ! is not the same as the lower-level "whole values of this layout do not exist" (that optimizations can assume).

Ideally we'd have some implication between them, e.g. we can make a query for conservative_is_uninhabited (with the proper substitution and normalization that layout code does) and layout could assert that Abi::Uninhabited implies conservative_is_uninhabited as well.

nikomatsakis · 2018-09-14T20:57:52Z

I'm not sure I understand why places which checked .abi == Abi::Uninhabited where changed - an user-facing type-check-level "weak equivalence" to ! is not the same as the lower-level "whole values of this layout do not exist" (that optimizations can assume).

It is also not clear to me if we should change this code, but I think a possible justification arises from this stuff I wrote earlier:

In the terms from @RalfJung's Two Kinds of Invariants blog post, the justification for optimizations around uninhabitedness arises from the validity invariant (to a large extent). All values that the compiler thinks are initialized must meet this invariant: presumably this includes the return value. This means that, yes, if we find a ! embedded within a struct or tuple, we can justify saying that the fn never returns (etc).

But the validity invariant for &! is different, and doesn't require that the pointer is valid. The same is true of unions.

We might wind up requiring a stronger invariant of fn return values, mind you, that would make returning &! UB, but I think this is unlikely and certainly not decided.

But the validity invariant for &! is different, and doesn't require that the pointer is valid. The same is true of unions.

That said, I'm not entirely sure how to think about unions, and I'm wondering if what I wrote above is strictly correct. =) In particular, the compiler will (I think) get with you if you don't initialization the union somehow...? You might though be able to use a MaybeInit or something to read it out. I feel like we haven't quite settled enough of the rules to crisply call that illegal.
We might wind up requiring a stronger invariant of fn return values, mind you, that would make returning &! UB, but I think this is unlikely and certainly not decided.

varkor · 2018-09-14T21:40:08Z

I'm not sure I understand why places which checked .abi == Abi::Uninhabited where changed - an user-facing type-check-level "weak equivalence" to ! is not the same as the lower-level "whole values of this layout do not exist" (that optimizations can assume).

I'll admit I mostly copied these over from the original PR — and I think at the time they were made, Abi::Uninhabited wasn't as precise as it is now (I could be mistaken about that, though). It shouldn't make much difference in these particular places, but I'll revert the changes.

I feel like we haven't quite settled enough of the rules to crisply call that illegal.

It's safer to treat them as possibly-inhabited for now.

RalfJung · 2018-09-18T09:26:20Z

Extends the uninhabitedness check to [...] unions

Uh, please don't. We want to have some proper discussion before we make any assumption about union contents.

src/librustc/ty/sty.rs

varkor · 2018-09-18T11:22:31Z

Uh, please don't. We want to have some proper discussion before we make any assumption about union contents.

Ah, I removed the union handling, but forgot to update the comment. Unions are now currently never treated as uninhabited.

nikomatsakis · 2018-09-18T17:39:55Z

@varkor

Ah, I removed the union handling, but forgot to update the comment. Unions are now currently never treated as uninhabited.

Maybe you forgot to push? The code I am looking at doesn't seem to check for unions?

varkor · 2018-09-18T17:44:00Z

Maybe you forgot to push? The code I am looking at doesn't seem to check for unions?

It's just above the main ADT handling:
https://github.com/rust-lang/rust/pull/54125/files#diff-b180e2fcc6ac765371cbd5b1dc95c914R1481

RalfJung · 2018-09-19T07:39:59Z

Btw, #53508 also introduces an is_uninhabited function for layouts. Seems like there is some overlap?

Cc @japaric

eddyb · 2018-09-19T08:32:21Z

@RalfJung that's just a helper - you can't always compute the layout, even in cases where you determine separately from the layout, that the type is uninhabited.

nikomatsakis · 2018-09-20T19:02:56Z

@varkor

The comment about &T is still wrong. The current comment is definitely wrong. It says that null references are permitted in unsafe code, but in fact they are not (else the Option<&T> layout optimization is incorrect). What would be permitted is "non-null references to uninitialized memory".

nikomatsakis · 2018-09-20T19:03:58Z

I am removing the S-blocked annotation since there was a deadlock:

This PR claimed to be blocked on #54123, but #54123 contained an annotation that it's better to review this one.

nikomatsakis · 2018-09-20T19:04:17Z

I changed to S-waiting-on-author because we need to at minimum change that comment. Other than that, I don't remember if there are any blocking concerns here?

varkor · 2018-12-11T12:20:56Z

I'd like to at least have a mir-opt testcase for

I've added a mir-opt test, though with the reversion of the destination uninhabitedness checking (see below), they currently do result in slightly different MIR.

Regarding the state of the PR, I think it's ready. I've reverted the change in into.rs, since we seem to be unable to get the current module for visibility-based uninhabitedness checking there (#54125 (comment)) currently, so I've left it as a FIXME. It'd be good to get at least these changes approved and then we can make it more flexible in the future.

nikomatsakis · 2018-12-18T16:41:33Z

@bors r+

@varkor thanks for sticking with this for so long

bors · 2018-12-18T16:41:34Z

📌 Commit 2ba3e66 has been approved by nikomatsakis

bors · 2018-12-19T04:34:13Z

⌛ Testing commit 2ba3e66 with merge bb22fc863da8536d7f6c3a6b53eb0357d7384d0b...

bors · 2018-12-19T05:38:08Z

💔 Test failed - status-travis

rust-highfive · 2018-12-19T05:38:10Z

The job x86_64-gnu 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.

[00:59:49] test [debuginfo-both] debuginfo/vec.rs ... ignored
[00:59:49] 
[00:59:49] failures:
[00:59:49] 
[00:59:49] ---- [debuginfo-both] debuginfo/nil-enum.rs stdout ----
[00:59:49] NOTE: compiletest thinks it is using GDB with native rust support
[00:59:49] NOTE: compiletest thinks it is using GDB version 8002000
[00:59:49] error: gdb failed to execute
[00:59:49] status: signal: 6
[00:59:49] status: signal: 6
[00:59:49] command: "/usr/bin/gdb" "-quiet" "-batch" "-nx" "-command=/checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo/nil-enum/nil-enum.debugger.script"
[00:59:49] ------------------------------------------
[00:59:49] ------------------------------------------
[00:59:49] GNU gdb (Ubuntu 8.2-0ubuntu1) 8.2
[00:59:49] Copyright (C) 2018 Free Software Foundation, Inc.
[00:59:49] License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
[00:59:49] This is free software: you are free to change and redistribute it.
[00:59:49] There is NO WARRANTY, to the extent permitted by law.
[00:59:49] Type "show copying" and "show warranty" for details.
[00:59:49] This GDB was configured as "x86_64-linux-gnu".
[00:59:49] Type "show configuration" for configuration details.
[00:59:49] For bug reporting instructions, please see:
[00:59:49] <http://www.gnu.org/software/gdb/bugs/>.
[00:59:49] Find the GDB manual and other documentation resources online at:
[00:59:49]     <http://www.gnu.org/software/gdb/documentation/>.
[00:59:49] 
[00:59:49] For help, type "help".
[00:59:49] Type "apropos word" to search for commands related to "word".
[00:59:49] Breakpoint 1 at 0x121a: file /checkout/src/test/debuginfo/nil-enum.rs, line 21.
[00:59:49] [Thread debugging using libthread_db enabled]
[00:59:49] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[00:59:49] 
[00:59:49] Breakpoint 1, nil_enum::main () at /checkout/src/test/debuginfo/nil-enum.rs:21
[00:59:49] 21     zzz(); // #break
[00:59:49] $1 = /build/gdb-CJziqM/gdb-8.2/gdb/valops.c:2270: internal-error: int value_union_variant(type*, const gdb_byte*): Assertion `discriminant_prop != nullptr' failed.
[00:59:49] A problem internal to GDB has been detected,
[00:59:49] further debugging may prove unreliable.
[00:59:49] Quit this debugging session? (y or n) [answered Y; input not from terminal]
[00:59:49] /build/gdb-CJziqM/gdb-8.2/gdb/valops.c:2270: internal-error: int value_union_variant(type*, const gdb_byte*): Assertion `discriminant_prop != nullptr' failed.
[00:59:49] A problem internal to GDB has been detected,
[00:59:49] further debugging may prove unreliable.
[00:59:49] Create a core file of GDB? (y or n) [answered Y; input not from terminal]
[00:59:49] ------------------------------------------
[00:59:49] stderr:
[00:59:49] ------------------------------------------
[00:59:49] 
[00:59:49] 
[00:59:49] This is a bug, please report it.  For instructions, see:
[00:59:49] <http://www.gnu.org/software/gdb/bugs/>.
[00:59:49] 
[00:59:49] ------------------------------------------
[00:59:49] 
[00:59:49] thread '[debuginfo-both] debuginfo/nil-enum.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3252:9
---
[00:59:49] test result: FAILED. 83 passed; 1 failed; 35 ignored; 0 measured; 0 filtered out
[00:59:49] 
[00:59:49] 
[00:59:49] 
[00:59:49] 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/debuginfo" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/debuginfo" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "debuginfo-both" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/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" "--llvm-version" "8.0.0svn\n" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:59:49] 
[00:59:49] 
[00:59:49] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:59:49] Build completed unsuccessfully in 0:58:00
---
travis_time:end:0cd2c81c:start=1545197773913751409,finish=1545197773922101624,duration=8350215
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0b21dffc
$ 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:006401f0
travis_time:start:006401f0
$ 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:041685d7
$ 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)

varkor · 2018-12-19T10:30:09Z

The test src/test/debuginfo/nil-enum.rs (https://github.com/rust-lang/rust/pull/54125/files#diff-61f322a0990681b9dd5aad4259423e2f), which explicitly does things that shouldn't be done (printing values of uninhabited types), is failing. It seems reasonable just to remove — I don't expect what it's testing to work. Is there a good reason not to?

// This test relies on gdbg printing the string "{<No data fields>}" for empty
// structs (which may change some time)

varkor · 2018-12-20T19:26:20Z

I've removed the nil-enum.rs test under the assumption that relying on undefined debugger behavior isn't sensible anyway. If anyone has any objections, we can think about a better way to test this behaviour as a follow up.

@bors r=nikomatsakis

bors · 2018-12-20T19:26:20Z

📌 Commit 0a8b696 has been approved by nikomatsakis

RalfJung · 2018-12-20T19:41:22Z

That test was checking how gdb and debuginfo cope with UB, which does make some sense.

Cc @TimNN @japaric @alexcrichton see #54125 (comment)

bors · 2018-12-20T22:04:28Z

⌛ Testing commit 0a8b696 with merge 3f7c718...

@nikomatsakis

…, r=nikomatsakis Less conservative uninhabitedness check Extends the uninhabitedness check to structs, non-empty enums, tuples and arrays. Pulled out of #47291 and #50262. Fixes #54586. r? @nikomatsakis

bors · 2018-12-21T00:37:47Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 3f7c718 to master...

rust-highfive assigned nikomatsakis Sep 11, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 11, 2018

varkor added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2018

varkor mentioned this pull request Sep 11, 2018

Check for uninhabitedness instead of never #54123

Closed

This comment has been minimized.

Sign in to view

varkor mentioned this pull request Sep 11, 2018

Lint uncallable function #54127

Closed

nikomatsakis reviewed Sep 14, 2018

View reviewed changes

src/librustc/ty/sty.rs Show resolved Hide resolved