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

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

Merged
merged 1 commit into from Apr 28, 2018

Conversation

Projects
None yet
7 participants
@nox
Copy link
Contributor

commented Apr 22, 2018

No description provided.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 22, 2018

r? @estebank

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

@@ -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 comment has been minimized.

This comment has been minimized.

Copy link
@nox

nox Apr 22, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@nox

nox Apr 22, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@rkruppe

rkruppe Apr 22, 2018

Member

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?

This comment has been minimized.

Copy link
@nox

nox Apr 22, 2018

Author Contributor

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

I'll add some comments.

This comment has been minimized.

Copy link
@rkruppe

rkruppe Apr 22, 2018

Member

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.

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

commented Apr 22, 2018

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:33:56]    Compiling pulldown-cmark v0.1.2
[00:33:56]    Compiling libc v0.2.40
[00:33:56]    Compiling bitflags v0.9.1
[00:33:56]    Compiling remove_dir_all v0.5.1
m        major |= (dev & 0xfffff00000000000) >> 32;
[00:33:58]      |                                             ^^ expected u64, found i32
[00:33:58] 
[00:33:58] error[E0271]: type mismatch resolving `<u64 as std::ops::Shr<i32>>::Output == i32`
[00:33:58]     --> /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.40/src/unix/notbsd/linux/mod.rs:1437:45
[00:33:58]      |
[00:33:58] 1437 |         minor |= (dev & 0x00000000000000ff) >> 0;
[00:33:58]      |                                             ^^ expected u64, found i32
[00:33:58] 
[00:33:58] error[E0271]: type mismatch resolving `<u64 as std::ops::Shr<i32>>::Output == i32`
[00:33:58]     --> /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.40/src/unix/notbsd/linux/mod.rs:1438:45
[00:33:58]      |
[00:33:58] 1438 |         minor |= (dev & 0x00000ffffff00000) >> 12;
[00:33:58]      |                                             ^^ expected u64, found i32
[00:33:58] 
[00:33:58] error[E0271]: type mismatch resolving `<u64 as std::ops::Shl<i32>>::Output == i32`
[00:33:58]     --> /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.40/src/unix/notbsd/linux/mod.rs:1446:37
[00:33:58]      |
[00:33:58] 1446 |         dev |= (major & 0x00000fff) << 8;
[00:33:58]      |                                     ^^ expected u64, found i32
[00:33:58] 
[00:33:58] error[E0271]: type mismatch resolving `<u64 as std::ops::Shl<i32>>::Output == i32`
[00:33:58]     --> /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.40/src/unix/notbsd/linux/mod.rs:1447:37
[00:33:58]      |
[00:33:58] 1447 |         dev |= (major & 0xfffff000) << 32;
[00:33:58]      |                                     ^^ expected u64, found i32
[00:33:58] 
[00:33:58] error[E0271]: type mismatch resolving `<u64 as std::ops::Shl<i32>>::Output == i32`
[00:33:58]     --> /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.40/src/unix/notbsd/linux/mod.rs:1448:37
[00:33:58]      |
[00:33:58] 1448 |         dev |= (minor & 0x000000ff) << 0;
[00:33:58]      or 1
[00:33:58] Makefile:28: recipe for target 'all' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:11bf7d43
$ 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)

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

commented Apr 22, 2018

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:37:14]    Compiling rustc_trans v0.0.0 (file:///checkout/src/librustc_trans)
[00:37:14]    Compiling cc v1.0.10
[00:37:14]    Compiling num_cpus v1.8.0
[00:37:16]    Compiling rustc_llvm v0.0.0 (file:///checkout/src/librustc_llvm)
ismatch resolving `<u64 as std::ops::Shl<i32>>::Output == i32`
[00:38:08]     --> /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.40/src/unix/notbsd/linux/mod.rs:1447:37
[00:38:08]      |
[00:38:08] 1447 |         dev |= (major & 0xfffff000) << 32;
[00:38:08]      |                                     ^^ expected u64, found i32
[00:38:08] 
[00:38:08] error[E0271]: type mismatch resolving `<u64 as std::ops::Shl<i32>>::Output == i32`
[00:38:08]     --> /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.40/src/unix/notbsd/linux/mod.rs:1448:37
[00:38:08]      |
[00:38:08] 1448 |         dev |= (minor & 0x000000ff) << 0;
[00:38:08]      |                                     ^^ expected u64, found i32
[00:38:08] 
[00:38:08] error[E0271]: type mismatch resolving `<u64 as std::ops::Shl<i32>>::Output == i32`
[00:38:08]     --> /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.40/src/unix/notbsd/linux/mod.rs:1449:37
[00:38:08]      |
[00:38:08] 1449 |         dev |= (minor & 0xffffff00Sun Apr 22 23:36:24 UTC 2018
travis_time:end:03d2e3fd:start=1524440184085647645,finish=1524440184141966552,duration=56318907

The command "date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
" exited with 0.

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)

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

commented Apr 23, 2018

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:34:46]    Compiling pulldown-cmark v0.1.2
[00:34:46]    Compiling libc v0.2.40
[00:34:46]    Compiling remove_dir_all v0.5.1
[00:34:46]    Compiling bitflags v0.9.1
    const OPTION_ENABLE_FOOTNOTES = 1 << 2;
[00:34:47]     |                                           ^^ expected u32, found i32
[00:34:47] error: aborting due to 3 previous errors
[00:34:47] 
[00:34:47] For more information about this error, try `rustc --explain E0271`.
[00:34:47] For more information about this error, try `rustc --explain E0271`.
[00:34:47] error: Could not compile `pulldown-cmark`.
[00:34:47] warning: build failed, waiting for other jobs to finish...
[00:34:48] error[E0271]: type mismatch resolving `<u64 as std::ops::Shr<i32>>::Output == i32`
[00:34:48]     --> /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.40/src/unix/notbsd/linux/mod.rs:1430:45
[00:34:48]      |
[00:34:48] 1430 |         major |= (dev & 0x00000000000fff00) >> 8;
[00:34:48]      |                                             ^^ expected u64, found i32
[00:34:48] 
[00:34:48] error[E0271]: type mismatch resolving `<u64 as std::ops::Shr<i32>>::Output == i32`
[00:34:48]     --> /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.40/src/unix/notbsd/linux/mod.rs:1431:45
[00:34:48]      |
[00:34:48] 1431 |         major |= (dev & 0xfffff00000000000) >> 32;
[00:34:48]      |                                             ^^ expected u64, found i32
[00:34:48] 
[00:34:48] error[E0271]: type mismatch resolving `<u64 as std::ops::Shr<i32>>::Output == i32`
[00:34:48]     --> /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.40/src/unix/notbsd/linux/mod.rs:1437:45
[00:34:48]      |
[00:34:48] 1437 |         minor |= (dev & 0x00000000000000ff) >> 0;
[00:34:48]      |                                             ^^ expected u64, found i32
[00:34:48] 
[00:34:48] error[E0271]: type mismatch resolving `<u64 as std::ops::Shr<i32>>::Output == i32`
[00:34:48]     --> /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.40/src/unix/notbsd/linux/mod.rs:1438:45
[00:34:48]      |
[00:34:48] 1438 |         minor |= (dev & 0x00000ffffff00000) >> 12;
[00:34:48]      |                                             ^^ expected u64, found i32
[00:34:48] 
[00:34:48] error[E0271]: type mismatch resolving `<u64 as std::ops::Shl<i32>>::Output == i32`
[00:34:48]     --> /cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.40/src/unix/notbsd/linux/mod.rs:1446:37
[00:34:48]      |
[00:34:48] 1446 |         dev |= (major & 0x00000fff) << 8;
[00:34:48]      |                                     ^^ expected u64, found i32
[00:34:48] 
[00:34:48] error[E0271]: type m grep ^Date: | sed 's/Date: //g' || true)
travis_fold:start:after_failure.1
travis_time:start:15532440

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)

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

This comment has been minimized.

Copy link
@dotdash

dotdash Apr 24, 2018

Contributor

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

This comment has been minimized.

Copy link
@nox

nox Apr 24, 2018

Author Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

@bors-servo try

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

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2018

⌛️ Trying commit c7c292b with merge c4ba253...

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

Auto merge of #50164 - nox:rval-range-metadata, r=<try>
Emit range metadata on calls returning scalars (fixes #50157)
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2018

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

@@ -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>> {

This comment has been minimized.

Copy link
@eddyb

eddyb Apr 24, 2018

Member

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.

This comment has been minimized.

Copy link
@nox

nox Apr 25, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@eddyb

eddyb Apr 25, 2018

Member

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

This comment has been minimized.

Copy link
@nox

nox Apr 25, 2018

Author Contributor

Done!

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.

This comment has been minimized.

Copy link
@eddyb

eddyb Apr 24, 2018

Member

We should really retire LLVM 3.9, sigh.

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

This comment has been minimized.

Copy link
@eddyb

eddyb Apr 24, 2018

Member

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

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2018

r? @eddyb

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

/// 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> {

This comment has been minimized.

Copy link
@eddyb

eddyb Apr 25, 2018

Member

Please name this valid_range_exclusive.

This comment has been minimized.

Copy link
@nox

nox Apr 25, 2018

Author Contributor

Renamed and squashed everything.

@nox nox force-pushed the nox:rval-range-metadata branch from ee049eb to d4c8f37 Apr 25, 2018

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

This comment has been minimized.

Copy link
@eddyb

eddyb Apr 25, 2018

Member

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

This comment has been minimized.

Copy link
@nox

nox Apr 25, 2018

Author Contributor

Good question! Fixed.

@nox nox force-pushed the nox:rval-range-metadata branch from d4c8f37 to cc77dc4 Apr 25, 2018

@eddyb

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

📌 Commit cc77dc4 has been approved by eddyb

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 25, 2018

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

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

@bors r-

@nox nox force-pushed the nox:rval-range-metadata branch 4 times, most recently from 682ef23 to 55d1cf4 Apr 25, 2018

@eddyb

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

📌 Commit 55d1cf4 has been approved by eddyb

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2018

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

@nox nox force-pushed the nox:rval-range-metadata branch from 55d1cf4 to 9065644 Apr 26, 2018

@eddyb

This comment has been minimized.

Copy link
Member

commented Apr 26, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2018

📌 Commit 9065644 has been approved by eddyb

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2018

⌛️ Testing commit 9065644 with merge 68a09fc...

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

Auto merge of #50164 - nox:rval-range-metadata, r=eddyb
Emit range metadata on calls returning scalars (fixes #50157)
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2018

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

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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@nox nox deleted the nox:rval-range-metadata branch Apr 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.