Emit range metadata on calls returning scalars (fixes #50157) #50164

nox · 2018-04-22T19:27:54Z

No description provided.

rust-highfive · 2018-04-22T19:27:56Z

(rust_highfive has picked a reviewer for you, use r? to override)

hanna-kruppe · 2018-04-22T19:30:16Z

src/librustc_trans/abi.rs

@@ -1055,6 +1055,27 @@ impl<'a, 'tcx> FnType<'tcx> {
            PassMode::Indirect(ref attrs) => apply(attrs),
            _ => {}
        }
+        if let layout::Abi::Scalar(ref scalar) = self.ret.layout.abi {


This duplicates https://github.com/rust-lang/rust/blob/master/src/librustc_trans/mir/place.rs#L93:L116 right?

Partially yes, there are two differences:

no nonnull metadata is ever emitted for calls, because that's illegal;

boolean scalars don't get range metadata because they end up as i1 with a
!{i1 false, i1 false} range otherwise.

I guess I could make the range extraction and masking its own method on Scalar, would you prefer that?

Good point about nonnull, but the calculation of the range (and the relevant comments) should still be de-duplicated.

The i1 part confuses me though. Surely the same problem applies to range metadata on loads? If it is a problem at all, that is: the max_next & mask != min & mask guard should rule that out already, I think? Did something break without the extra check for bools?

About the i1 part: https://botbot.me/mozilla/rustc/2018-04-22/?msg=99259759&page=3

I'll add some comments.

I guess I could make the range extraction and masking its own method on Scalar, would you prefer that?

I still don't know the layout code well enough for my opinion on this to carry much weight, but this is really an LLVM concept, so maybe it should be in rustc_trans. No real clue where, though.

Note that LLVM ranges with max_next & mask == min & mask are inherently invalid, so the helper function could return Option<Range<_>> to avoid duplicating that check as well.

dotdash · 2018-04-24T09:54:04Z

src/librustc_trans/mir/place.rs

+                    layout::Int(..) => {
+                        bx.range_metadata(load, range);
+                    }
+                    layout::Pointer if 0 < range.start && range.start < range.end => {


Not sure whether this matters, but this comparison changed. range.end is equal to the old max_next, not max

How many things can I inadvertently change in a single piece of code? I think this actually disables a bunch of nonnull metadata by mistake, but it shouldn't be causing that type checking failure. Will fix anyway.

nox · 2018-04-24T13:58:39Z

@bors-servo try

Let's see if this actually works for LLVM >3.9.

bors · 2018-04-24T13:58:51Z

⌛ Trying commit c7c292b with merge c4ba253...

Emit range metadata on calls returning scalars (fixes #50157)

bors · 2018-04-24T15:53:51Z

☀️ Test successful - status-travis
State: approved= try=True

eddyb · 2018-04-24T17:51:28Z

src/librustc/ty/layout.rs

@@ -648,6 +648,25 @@ impl Scalar {
            false
        }
    }
+
+    /// Returns a range suitable to be passed to LLVM for range metadata.
+    pub fn range_metadata<C: HasDataLayout>(&self, cx: C) -> Option<Range<u128>> {


First off, please do not put LLVM special-cases outside of librustc_trans. Secondly, could the check be performed by having a way to get the full range for the size of the primitive in question?

We should really assert, somewhere, that start & mask == start and end & mask == end, because niche computation relies on it - #49981 already fixed an instance of this invariant not being upheld.

Because then you can just go prim.value.full_valid_range(cx) == prim.valid_range and that's your check.

First off, please do not put LLVM special-cases outside of librustc_trans.

Where should I put that in librustc_trans, and how? A free-standing function? A method on some C defined in librustc_trans? A trait implemented for Scalar?

Secondly, could the check be performed by having a way to get the full range for the size of the primitive in question?

Nope, 0..=255 isn't equal to 1..=0, so I doubt this is a good idea.

Alright, could this function be named valid_range_exclusive instead, and have the Option replaced with a r.start == r.end in rustc_trans?

eddyb · 2018-04-24T17:51:59Z

src/librustc_trans/abi.rs

+                layout::Int(..) if !scalar.is_bool() => {
+                    if let Some(range) = scalar.range_metadata(bx.cx) {
+                        // FIXME(nox): This causes very weird type errors about
+                        // SHL operators in constants in stage 2 with LLVM 3.9.


We should really retire LLVM 3.9, sigh.

eddyb · 2018-04-24T17:55:13Z

src/librustc/ty/layout.rs

+        assert!(bits <= 128);
+        let mask = !0u128 >> (128 - bits);
+        let start = self.valid_range.start;
+        let end = self.valid_range.end.wrapping_add(1);


Oh, wait, this is turning it into an exclusive range. Whereas the storage uses inclusive. I wonder if we could store exclusive ranges throughout, but that sucks for wrap-around ranges because you end up with 0..0 - we should really have some wrap-around range type for layout specifically.

estebank · 2018-04-24T19:03:45Z

r? @eddyb

eddyb · 2018-04-25T11:34:36Z

src/librustc/ty/layout.rs

+    /// Returns the valid range as a `x..y` range.
+    ///
+    /// If `x` and `y` are equal, the range is full, not empty.
+    pub fn range_metadata_exclusive<C: HasDataLayout>(&self, cx: C) -> Range<u128> {


Please name this valid_range_exclusive.

Renamed and squashed everything.

eddyb · 2018-04-25T12:55:39Z

src/librustc_trans/mir/place.rs

                }
-                layout::Pointer if 0 < min && min < max => {
+                layout::Pointer
+                if (1..scalar.valid_range.end).contains(&scalar.valid_range.start) => {


Why not !scalar.valid_range.contains(&0)?

Good question! Fixed.

eddyb · 2018-04-25T13:17:39Z

@bors r+

bors · 2018-04-25T13:17:40Z

📌 Commit cc77dc4 has been approved by eddyb

rust-highfive · 2018-04-25T13:26:56Z

The job x86_64-gnu-llvm-3.9 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:24:37]    Compiling atty v0.2.8
[00:24:37]    Compiling backtrace-sys v0.1.16
[00:24:41]    Compiling miniz-sys v0.1.10
[00:24:41]    Compiling rustc_cratesio_shim v0.0.0 (file:///checkout/src/librustc_cratesio_shim)
[00:24:41] error: failed to run custom build command for `syntax v0.0.0 (file:///checkout/src/libsyntax)`
[00:24:41] process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/release/build/syntax-5f3de57df7d70275/build-script-build` (signal: 11, SIGSEGV: invalid memory reference)
[00:24:46] error: build failed
[00:24:46] error: build failed
[00:24:46] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" " jemalloc" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:24:46] expected success, got: exit code: 101
[00:24:46] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1091:9
[00:24:46] travis_fold:end:stage1-rustc

[00:24:46] travis_time:end:stage1-rustc:start=1524662791826013199,finish=1524662812586433926,duration=20760420727


[00:24:46] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:24:46] Build completed unsuccessfully in 0:19:52
[00:24:46] Makefile:28: recipe for target 'all' failed
[00:24:46] make: *** [all] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:02fafc58
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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)

eddyb · 2018-04-25T14:07:33Z

@bors r-

eddyb · 2018-04-25T16:27:14Z

@bors r+

bors · 2018-04-25T16:27:15Z

📌 Commit 55d1cf4 has been approved by eddyb

bors · 2018-04-26T18:35:20Z

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

eddyb · 2018-04-26T20:04:15Z

@bors r+

bors · 2018-04-26T20:04:16Z

📌 Commit 9065644 has been approved by eddyb

bors · 2018-04-28T17:59:32Z

⌛ Testing commit 9065644 with merge 68a09fc...

Emit range metadata on calls returning scalars (fixes #50157)

bors · 2018-04-28T20:10:15Z

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

rust-highfive assigned estebank Apr 22, 2018

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

hanna-kruppe reviewed Apr 22, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

dotdash reviewed Apr 24, 2018

View reviewed changes

bors added a commit that referenced this pull request Apr 24, 2018

Auto merge of #50164 - nox:rval-range-metadata, r=<try>

c4ba253

Emit range metadata on calls returning scalars (fixes #50157)

eddyb reviewed Apr 24, 2018

View reviewed changes

rust-highfive assigned eddyb and unassigned estebank Apr 24, 2018

eddyb reviewed Apr 25, 2018

View reviewed changes

nox force-pushed the rval-range-metadata branch from ee049eb to d4c8f37 Compare April 25, 2018 11:38

eddyb reviewed Apr 25, 2018

View reviewed changes

nox force-pushed the rval-range-metadata branch from d4c8f37 to cc77dc4 Compare April 25, 2018 12:58

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 Apr 25, 2018

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 Apr 25, 2018

nox force-pushed the rval-range-metadata branch 4 times, most recently from 682ef23 to 55d1cf4 Compare April 25, 2018 16:26

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 Apr 25, 2018

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 Apr 26, 2018

Emit range metadata on calls returning scalars (fixes rust-lang#50157)

9065644

nox force-pushed the rval-range-metadata branch from 55d1cf4 to 9065644 Compare April 26, 2018 20:02

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 Apr 26, 2018

bors added a commit that referenced this pull request Apr 28, 2018

Auto merge of #50164 - nox:rval-range-metadata, r=eddyb

68a09fc

Emit range metadata on calls returning scalars (fixes #50157)

bors merged commit 9065644 into rust-lang:master Apr 28, 2018

nox deleted the rval-range-metadata branch April 28, 2018 21:11

Emit range metadata on calls returning scalars (fixes #50157) #50164

Emit range metadata on calls returning scalars (fixes #50157) #50164

Conversation

nox commented Apr 22, 2018

rust-highfive commented Apr 22, 2018

Choose a reason for hiding this comment

nox Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nox commented Apr 24, 2018

bors commented Apr 24, 2018

bors commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Apr 25, 2018

bors commented Apr 25, 2018

rust-highfive commented Apr 25, 2018

eddyb commented Apr 25, 2018

eddyb commented Apr 25, 2018

bors commented Apr 25, 2018

bors commented Apr 26, 2018

eddyb commented Apr 26, 2018

bors commented Apr 26, 2018

bors commented Apr 28, 2018

bors commented Apr 28, 2018

nox Apr 22, 2018 •

edited

Loading