Use the aligned size for alloca at args/ret when the pass mode is cast #127168

DianQK · 2024-06-30T13:53:45Z

The load and store instructions in LLVM access the aligned size. For example, load { i64, i32 } accesses 16 bytes on x86_64: https://alive2.llvm.org/ce/z/n8CHAp.

BTW, this example is expected to be optimized to immediate UB by Alive2: https://rust.godbolt.org/z/b7xK7hv1c and https://alive2.llvm.org/ce/z/vZDtZH.

r? compiler

workingjubilee · 2024-06-30T23:18:48Z

@DianQK what's the LLVMIR of the example after this patch?

workingjubilee · 2024-06-30T23:59:39Z

The LLVMIR after this PR now seems to be this:

; ModuleID = 'miscompiled_arg.4dadb824ae4952c2-cgu.0'
source_filename = "miscompiled_arg.4dadb824ae4952c2-cgu.0"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

; Function Attrs: nounwind nonlazybind uwtable
define { i64, i32 } @foo({ i64, i32 } %0) unnamed_addr #0 {
start:
  %_0 = alloca [12 x i8], align 4
  %1 = alloca [16 x i8], align 8
  %arg = alloca [12 x i8], align 4
  store { i64, i32 } %0, ptr %1, align 8
  call void @llvm.memcpy.p0.p0.i64(ptr align 4 %arg, ptr align 8 %1, i64 12, i1 false)
  call void @llvm.memcpy.p0.p0.i64(ptr align 4 %_0, ptr align 4 %arg, i64 12, i1 false)
  %2 = load { i64, i32 }, ptr %_0, align 4
  ret { i64, i32 } %2
}

; Function Attrs: nocallback nofree nounwind willreturn memory(argmem: readwrite)
declare void @llvm.memcpy.p0.p0.i64(ptr noalias nocapture writeonly, ptr noalias nocapture readonly, i64, i1 immarg) #1

attributes #0 = { nounwind nonlazybind uwtable "probe-stack"="inline-asm" "target-cpu"="x86-64" }
attributes #1 = { nocallback nofree nounwind willreturn memory(argmem: readwrite) }

!llvm.module.flags = !{!0, !1}
!llvm.ident = !{!2}

!0 = !{i32 8, !"PIC Level", i32 2}
!1 = !{i32 2, !"RtLibUseGOT", i32 1}
!2 = !{!"rustc version 1.81.0-dev"}

But Alive2 still flags this as UB, and what needs to happen is this:

-  %_0 = alloca [12 x i8], align 4
+  %_0 = alloca [16 x i8], align 4

DianQK · 2024-07-01T00:46:37Z

What was I thinking? @.@
I believe this must be a test case in tests/codegen/cast-target-abi.rs. I will update this PR tonight.

@rustbot author

workingjubilee · 2024-07-01T01:59:05Z

Thank you for your work on this, and please don't feel bad about making a mistake. 💖 Code review is precisely for checking for errors, sometimes silly-seeming ones, and this is... certainly not the easiest part of rustc to work with.

DianQK · 2024-07-01T02:13:34Z

Thank you for your work on this, and please don't feel bad about making a mistake. 💖 Code review is precisely for checking for errors, sometimes silly-seeming ones, and this is... certainly not the easiest part of rustc to work with.

Don't worry. I'm just complaining about I show a bug that I'm fixing in the PR. Probably because I simplified the code while debugging. :3

DianQK · 2024-07-01T14:51:37Z

~~Edit: Perhaps I should put the commit that updates the test cases at first. I'll consider updating it tomorrow. Time for bed.~~

I split the changes into three commits for review. The latest commit includes updates to the return value.

@rustbot ready

The `load` and `store` instructions in LLVM access the aligned size.

workingjubilee · 2024-07-02T01:41:58Z

Everything seems in order. Confirmed the codegen passes Alive2 after pulling and building. If this PR needs further fixes, I think we can do it as a followup.

@bors r+

bors · 2024-07-02T01:42:00Z

📌 Commit 2ef8280 has been approved by workingjubilee

It is now in the queue for this repository.

Use the aligned size for alloca at args/ret when the pass mode is cast Fixes rust-lang#75839. Fixes rust-lang#121028. The `load` and `store` instructions in LLVM access the aligned size. For example, `load { i64, i32 }` accesses 16 bytes on x86_64: https://alive2.llvm.org/ce/z/n8CHAp. BTW, this example is expected to be optimized to immediate UB by Alive2: https://rust.godbolt.org/z/b7xK7hv1c and https://alive2.llvm.org/ce/z/vZDtZH. r? compiler

bors · 2024-07-02T02:50:23Z

⌛ Testing commit 2ef8280 with merge 7a59bfd...

bors · 2024-07-02T02:52:56Z

💔 Test failed - checks-actions

compiler-errors · 2024-07-02T03:02:11Z

@bors retry

rust-log-analyzer · 2024-07-02T03:06:31Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

#14 0.422 
#14 0.422 gzip: stdin: not in gzip format
#14 0.423 tar: Child returned status 1
#14 0.423 tar: Error is not recoverable: exiting now
#14 ERROR: process "/bin/sh -c env HOST_CFLAGS=\"-O -fcommon\" /tmp/build-netbsd-toolchain.sh" did not complete successfully: exit code: 2
 > [6/8] RUN env HOST_CFLAGS="-O -fcommon" /tmp/build-netbsd-toolchain.sh:
0.035 + mkdir netbsd

100   245  100   245    0     0    647      0 --:--:-- --:--:-- --:--:--   648
---
  11 | >>> RUN env HOST_CFLAGS="-O -fcommon" /tmp/build-netbsd-toolchain.sh
  12 |     
  13 |     COPY scripts/sccache.sh /scripts/
--------------------
ERROR: failed to solve: process "/bin/sh -c env HOST_CFLAGS=\"-O -fcommon\" /tmp/build-netbsd-toolchain.sh" did not complete successfully: exit code: 2
#0 building with "jolly_gagarin" instance using docker-container driver

#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 988B done
---
#11 0.182 
#11 0.182 gzip: stdin: not in gzip format
#11 0.182 tar: Child returned status 1
#11 0.182 tar: Error is not recoverable: exiting now
#11 ERROR: process "/bin/sh -c env HOST_CFLAGS=\"-O -fcommon\" /tmp/build-netbsd-toolchain.sh" did not complete successfully: exit code: 2
 > [6/8] RUN env HOST_CFLAGS="-O -fcommon" /tmp/build-netbsd-toolchain.sh:
:--:--  1737
0.182 
0.182 gzip: stdin: not in gzip format
---
  11 | >>> RUN env HOST_CFLAGS="-O -fcommon" /tmp/build-netbsd-toolchain.sh
  12 |     
  13 |     COPY scripts/sccache.sh /scripts/
--------------------
ERROR: failed to solve: process "/bin/sh -c env HOST_CFLAGS=\"-O -fcommon\" /tmp/build-netbsd-toolchain.sh" did not complete successfully: exit code: 2
#0 building with "jolly_gagarin" instance using docker-container driver

#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 988B done
---
#11 0.453 
#11 0.453 gzip: stdin: not in gzip format
#11 0.453 tar: Child returned status 1
#11 0.453 tar: Error is not recoverable: exiting now
#11 ERROR: process "/bin/sh -c env HOST_CFLAGS=\"-O -fcommon\" /tmp/build-netbsd-toolchain.sh" did not complete successfully: exit code: 2
 > [6/8] RUN env HOST_CFLAGS="-O -fcommon" /tmp/build-netbsd-toolchain.sh:

100   245  100   245    0     0    592      0 --:--:-- --:--:-- --:--:--   593
0.453 
---
  11 | >>> RUN env HOST_CFLAGS="-O -fcommon" /tmp/build-netbsd-toolchain.sh
  12 |     
  13 |     COPY scripts/sccache.sh /scripts/
--------------------
ERROR: failed to solve: process "/bin/sh -c env HOST_CFLAGS=\"-O -fcommon\" /tmp/build-netbsd-toolchain.sh" did not complete successfully: exit code: 2
#0 building with "jolly_gagarin" instance using docker-container driver

#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 988B done
---
#11 0.423 
#11 0.423 gzip: stdin: not in gzip format
#11 0.424 tar: Child returned status 1
#11 0.424 tar: Error is not recoverable: exiting now
#11 ERROR: process "/bin/sh -c env HOST_CFLAGS=\"-O -fcommon\" /tmp/build-netbsd-toolchain.sh" did not complete successfully: exit code: 2
 > [6/8] RUN env HOST_CFLAGS="-O -fcommon" /tmp/build-netbsd-toolchain.sh:

100   245  100   245    0     0    639      0 --:--:-- --:--:-- --:--:--   639
0.423 
---
  11 | >>> RUN env HOST_CFLAGS="-O -fcommon" /tmp/build-netbsd-toolchain.sh
  12 |     
  13 |     COPY scripts/sccache.sh /scripts/
--------------------
ERROR: failed to solve: process "/bin/sh -c env HOST_CFLAGS=\"-O -fcommon\" /tmp/build-netbsd-toolchain.sh" did not complete successfully: exit code: 2
#0 building with "jolly_gagarin" instance using docker-container driver

#1 [internal] load build definition from Dockerfile
#1 transferring dockerfile: 988B done
---
#11 0.344 
#11 0.344 gzip: stdin: not in gzip format
#11 0.344 tar: Child returned status 1
#11 0.344 tar: Error is not recoverable: exiting now
#11 ERROR: process "/bin/sh -c env HOST_CFLAGS=\"-O -fcommon\" /tmp/build-netbsd-toolchain.sh" did not complete successfully: exit code: 2
 > [6/8] RUN env HOST_CFLAGS="-O -fcommon" /tmp/build-netbsd-toolchain.sh:

100   245  100   245    0     0    805      0 --:--:-- --:--:-- --:--:--   808
0.344 
---
  11 | >>> RUN env HOST_CFLAGS="-O -fcommon" /tmp/build-netbsd-toolchain.sh
  12 |     
  13 |     COPY scripts/sccache.sh /scripts/
--------------------
ERROR: failed to solve: process "/bin/sh -c env HOST_CFLAGS=\"-O -fcommon\" /tmp/build-netbsd-toolchain.sh" did not complete successfully: exit code: 2
##[error]Process completed with exit code 1.
Post job cleanup.

nikic · 2024-07-02T07:04:24Z

I don't think that this is the right way to fix the issue. Instead of changing the alloca size, we should remove the use of aggregate loads/stores (load/store member-wise instead).

DianQK · 2024-07-02T07:37:08Z

I don't think that this is the right way to fix the issue. Instead of changing the alloca size, we should remove the use of aggregate loads/stores (load/store member-wise instead).

But we always need to return a { i64, i32 }, so do we still need this alloca { i64, i32 }? It looks like clang generates similar IR: https://llvm.godbolt.org/z/bxbbrhjeY. Or I guess you mean insertvalue?

nikic · 2024-07-02T07:41:24Z

Yes, I mean using insertvalue. LLVM will ultimately convert it to that form, which is why this is not a problem with optimizations. But we should directly generate that instead of going through an aggregate load in the first place.

DianQK · 2024-07-02T07:58:37Z

Yes, I mean using insertvalue. LLVM will ultimately convert it to that form, which is why this is not a problem with optimizations. But we should directly generate that instead of going through an aggregate load in the first place.

This makes sense to me. I think that as a bugfix PR, the current changes can make the sanitizers work correctly and LLVM can also optimize most of it with insertvalue. I prefer to treat insertvalue as a separate PR for improving compilation time.

…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#126883 (Parenthesize break values containing leading label) - rust-lang#127136 (Fix `FnMut::call_mut`/`Fn::call` shim for async closures that capture references) - rust-lang#127146 (Uplift fast rejection to new solver) - rust-lang#127152 (Bootstrap: Try renaming the file if removing fails) - rust-lang#127168 (Use the aligned size for alloca at args/ret when the pass mode is cast) - rust-lang#127203 (Fix import suggestion error when path segment failed not from starting) - rust-lang#127212 (Update books) - rust-lang#127224 (Make `FloatTy` checks exhaustive in pretty print) - rust-lang#127230 (chore: remove duplicate words) - rust-lang#127243 (Add test for adt_const_params) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#127168 - DianQK:cast-size, r=workingjubilee Use the aligned size for alloca at args/ret when the pass mode is cast Fixes rust-lang#75839. Fixes rust-lang#121028. The `load` and `store` instructions in LLVM access the aligned size. For example, `load { i64, i32 }` accesses 16 bytes on x86_64: https://alive2.llvm.org/ce/z/n8CHAp. BTW, this example is expected to be optimized to immediate UB by Alive2: https://rust.godbolt.org/z/b7xK7hv1c and https://alive2.llvm.org/ce/z/vZDtZH. r? compiler

workingjubilee · 2024-07-03T00:55:12Z

yes, I assumed that we already intended to remove our aggregate load/stores and that this not doing so is because fixing "emitting LLVMIR with UB" is a smaller patch worth independently addressing.

DianQK · 2024-07-03T01:15:55Z

I'll give it a try. :)
This is a P-high issue and the only known real problem is ASAN, but I'm worried that this might cause runtime errors in some edge cases.

@rustbot label +beta-nominated

apiraino · 2024-07-04T14:33:56Z

Beta backport declined as per compiler team on Zulip, since the issue fixed is quite old and ASAN is not stable.

@rustbot label -beta-nominated

DianQK · 2024-08-11T13:10:30Z

Follow-up PR: #128969.

rustbot assigned wesleywiser Jun 30, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 30, 2024

DianQK changed the title ~~Use the aligned size for alloca.~~ Use the aligned size for alloca Jun 30, 2024

rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 1, 2024

DianQK force-pushed the cast-size branch from c8ceff2 to edb1ee5 Compare July 1, 2024 14:46

DianQK changed the title ~~Use the aligned size for alloca~~ Use the aligned size for alloca at args/ret when the pass mode is cast Jul 1, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 1, 2024

DianQK added 3 commits July 2, 2024 06:31

Add the definition for extern "C" at cast-target-abi.rs.

09e0abb

Use the aligned size for alloca at args when the pass mode is cast.

c453dcd

The `load` and `store` instructions in LLVM access the aligned size.

Use the aligned size for alloca at ret when the pass mode is cast.

2ef8280

DianQK force-pushed the cast-size branch from edb1ee5 to 2ef8280 Compare July 1, 2024 22:34

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 Jul 2, 2024

workingjubilee mentioned this pull request Jul 2, 2024

sanitizers: Stabilize AddressSanitizer and LeakSanitizer for the Tier 1 targets #123617

Open

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 2, 2024

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 Jul 2, 2024

matthiaskrgr mentioned this pull request Jul 2, 2024

Rollup of 10 pull requests #127244

Merged

bors merged commit 33b0238 into rust-lang:master Jul 2, 2024
6 of 7 checks passed

rustbot added this to the 1.81.0 milestone Jul 2, 2024

DianQK deleted the cast-size branch July 2, 2024 22:30

rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 3, 2024

apiraino mentioned this pull request Jul 3, 2024

llvm lint: buffer overflow in src/test/ui/foreign/foreign-truncated-arguments.rs #75839

Closed

rustbot removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the aligned size for alloca at args/ret when the pass mode is cast #127168

Use the aligned size for alloca at args/ret when the pass mode is cast #127168

DianQK commented Jun 30, 2024 •

edited

Loading

workingjubilee commented Jun 30, 2024

workingjubilee commented Jun 30, 2024

DianQK commented Jul 1, 2024 •

edited

Loading

workingjubilee commented Jul 1, 2024

DianQK commented Jul 1, 2024

DianQK commented Jul 1, 2024 •

edited

Loading

workingjubilee commented Jul 2, 2024

bors commented Jul 2, 2024

bors commented Jul 2, 2024

bors commented Jul 2, 2024

compiler-errors commented Jul 2, 2024

rust-log-analyzer commented Jul 2, 2024

nikic commented Jul 2, 2024

DianQK commented Jul 2, 2024 •

edited

Loading

nikic commented Jul 2, 2024

DianQK commented Jul 2, 2024

workingjubilee commented Jul 3, 2024

DianQK commented Jul 3, 2024

apiraino commented Jul 4, 2024

DianQK commented Aug 11, 2024

Use the aligned size for alloca at args/ret when the pass mode is cast #127168

Use the aligned size for alloca at args/ret when the pass mode is cast #127168

Conversation

DianQK commented Jun 30, 2024 • edited Loading

workingjubilee commented Jun 30, 2024

workingjubilee commented Jun 30, 2024

DianQK commented Jul 1, 2024 • edited Loading

workingjubilee commented Jul 1, 2024

DianQK commented Jul 1, 2024

DianQK commented Jul 1, 2024 • edited Loading

workingjubilee commented Jul 2, 2024

bors commented Jul 2, 2024

bors commented Jul 2, 2024

bors commented Jul 2, 2024

compiler-errors commented Jul 2, 2024

rust-log-analyzer commented Jul 2, 2024

nikic commented Jul 2, 2024

DianQK commented Jul 2, 2024 • edited Loading

nikic commented Jul 2, 2024

DianQK commented Jul 2, 2024

workingjubilee commented Jul 3, 2024

DianQK commented Jul 3, 2024

apiraino commented Jul 4, 2024

DianQK commented Aug 11, 2024

DianQK commented Jun 30, 2024 •

edited

Loading

DianQK commented Jul 1, 2024 •

edited

Loading

DianQK commented Jul 1, 2024 •

edited

Loading

DianQK commented Jul 2, 2024 •

edited

Loading