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

Returning [i128::MIN; 1] from a function actually returns [0i128; 1] #101585

Closed
cppforliving opened this issue Sep 8, 2022 · 7 comments · Fixed by #101612
Closed

Returning [i128::MIN; 1] from a function actually returns [0i128; 1] #101585

cppforliving opened this issue Sep 8, 2022 · 7 comments · Fixed by #101612
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority

Comments

@cppforliving
Copy link

Array created using repeat expression [i128::MIN; 1] and returned from a function actually returns [0i128; 1].
Interestingly the problem does not happen when using the list syntax [i128::MIN] to create the array.

I tried this code:

fn main() {
    fn min_array_ok() -> [i128; 1] {
        [i128::MIN]
    }
    assert_eq!(min_array_ok(), [-170141183460469231731687303715884105728i128]);
    
    fn min_array_nok() -> [i128; 1] {
        [i128::MIN; 1]
    }
    assert_eq!(min_array_nok(), [-170141183460469231731687303715884105728i128]); // panic!
}

I expected to see this happen: the second assert_eq should pass

Instead, this happened: the second assert_eq fails screaming that

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `[0]`,
 right: `[-170141183460469231731687303715884105728]`', src/main.rs:10:5

Meta

Problem exists on stable, beta and nightly

rustc --version --verbose:

rustc 1.63.0 (4b91a6ea7 2022-08-08)
binary: rustc
commit-hash: 4b91a6ea7258a947e59c6522cd5898e7c0a6a88f
commit-date: 2022-08-08
host: x86_64-unknown-linux-gnu
release: 1.63.0
LLVM version: 14.0.5
Backtrace

   0: rust_begin_unwind
             at /rustc/289279de116707f28cf9c18e4bbb8c6ec84ad75b/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/289279de116707f28cf9c18e4bbb8c6ec84ad75b/library/core/src/panicking.rs:142:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
             at /rustc/289279de116707f28cf9c18e4bbb8c6ec84ad75b/library/core/src/panicking.rs:181:5
   4: playground::main
             at ./[src/main.rs:10](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021#):5
   5: core::ops::function::FnOnce::call_once
             at /rustc/289279de116707f28cf9c18e4bbb8c6ec84ad75b/library/core/src/ops/function.rs:248:5

@cppforliving cppforliving added the C-bug Category: This is a bug. label Sep 8, 2022
@tmiasko tmiasko added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-codegen Area: Code generation labels Sep 8, 2022
@nikic nikic added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Sep 8, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 8, 2022
@5225225
Copy link
Contributor

5225225 commented Sep 8, 2022

I wonder if

if bx.cx().const_to_opt_uint(v) == Some(0) {
let fill = bx.cx().const_u8(0);
bx.memset(start, fill, size, dest.align, MemFlags::empty());
return bx;
}
is at fault here.

It does look to be inappropriately turning the repeat into a memset (when looking at the llvm IR)

@saethlin
Copy link
Member

saethlin commented Sep 8, 2022

https://godbolt.org/z/4xz1d4Tz9
Compiling this:

pub fn min_array_ok() -> [i128; 1] {
    [i128::MIN]
}

pub fn min_array_nok() -> [i128; 1] {
    [i128::MIN; 1]
}

with -Cno-prepopulate-passes -Copt-level=0 --emit=llvm-ir
Yields:

define void @_ZN7example12min_array_ok17h64ef6889d5bfe023E(ptr sret([1 x i128]) %0) unnamed_addr #0 !dbg !6 {
start:
  %1 = getelementptr inbounds [1 x i128], ptr %0, i64 0, i64 0, !dbg !11
  store i128 -170141183460469231731687303715884105728, ptr %1, align 8, !dbg !11
  ret void, !dbg !12
}

define void @_ZN7example13min_array_nok17h5f347fb6a42ac96fE(ptr sret([1 x i128]) %0) unnamed_addr #0 !dbg !13 {
start:
  %1 = getelementptr inbounds [1 x i128], ptr %0, i64 0, i64 0, !dbg !14
  call void @llvm.memset.p0.i64(ptr align 8 %1, i8 0, i64 16, i1 false), !dbg !14
  ret void, !dbg !15
}

Which does not seem right. We shouldn't be emitting a memset to 0 there.

@saethlin
Copy link
Member

saethlin commented Sep 8, 2022

This program hits an LLVM assertion if compiled with an LLVM that has assertions enabled:

╰ ➤ cargo +master-stage1 run
   Compiling scratch v0.1.0 (/tmp/scratch)
rustc: /checkout/src/llvm-project/llvm/include/llvm/ADT/APInt.h:1469: uint64_t llvm::APInt::getZExtValue() const: Assertion `getActiveBits() <= 64 && "Too many bits for uint64_t"' failed.
error: could not compile `scratch`

Caused by:
  process didn't exit successfully: `rustc --crate-name scratch --edition=2021 src/main.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type bin --emit=dep-info,link -C embed-bitcode=no -C debuginfo=2 -C metadata=604a9a6ba3499cae -C extra-filename=-604a9a6ba3499cae --out-dir /tmp/scratch/target/debug/deps -C linker=clang -C incremental=/tmp/scratch/target/debug/incremental -L dependency=/tmp/scratch/target/debug/deps -C link-arg=-fuse-ld=mold` (signal: 6, SIGABRT: process abort signal)

@apiraino
Copy link
Contributor

apiraino commented Sep 9, 2022

This appeared in 1.43.0. Until 1.42 the sample would code emit a compile error (godbolt link), maybe that would shadow the error?

A bisection between those two stable releases points to a group of old PR but I can't really pinpoint one :/

  commit[0] 2020-01-29UTC: Auto merge of #68634 - JohnTitor:clippy-up, r=JohnTitor
  commit[1] 2020-01-29UTC: Auto merge of #67688 - cjgillot:passes, r=Zoxc
  commit[2] 2020-01-30UTC: Auto merge of #68659 - Dylan-DPC:rollup-zo7zi9f, r=Dylan-DPC
  commit[3] 2020-01-30UTC: Auto merge of #68325 - faern:move-numeric-consts-to-associated-consts-step1, r=LukasKalbertodt
  commit[4] 2020-01-30UTC: Auto merge of #68661 - nnethercote:rm-unused-read_uleb128-param, r=Mark-Simulacrum

@5225225
Copy link
Contributor

5225225 commented Sep 9, 2022

pub fn min_array_nok() -> [i128; 1] {
    [std::i128::MIN; 1]
}

fails even under 1.26. And before that, i128 didn't even exist. So this has been a problem since 128 bit ints were introduced, as far as I can see.

@cppforliving
Copy link
Author

Thank you for the analysis and the fix :)

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants