Fix oversized loads on x86_64 SysV FFI calls #47614

dotdash · 2018-01-20T16:14:22Z

The x86_64 SysV ABI should use exact sizes for small structs passed in
registers, i.e. a struct that occupies 3 bytes should use an i24,
instead of the i32 it currently uses.

Refs #45543

rust-highfive · 2018-01-20T16:14:26Z

r? @petrochenkov

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

eddyb · 2018-01-20T18:12:17Z

r? @eddyb

eddyb · 2018-01-21T12:48:59Z

Should casts ever cause oversized loads, or should we not create CastTargets larger than the type? Maybe we can redesign the CastTarget API to make sure the final size matches the original layout?

dotdash · 2018-01-21T12:58:30Z

Not sure what you mean by the final size matching the original size, bit the proper solution would be to have the store happen into an alloca of the type that you want to pass/return and do the proper bitcast or memcpy there. And then do a plain load from there. Am 21.01.2018 13:49 schrieb "Eduard-Mihai Burtescu" < [email protected]>:

…

Should casts ever cause oversized loads, or should we not create CastTargets larger than the type? Maybe we can redesign the CastTarget API to make sure the final size matches the original layout? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#47614 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOGMl7Yxb-diqddZTb5R017316MyYY8ks5tMzJOgaJpZM4RlfSl> .

eddyb · 2018-01-21T13:12:19Z

I mean that either we should always be using i24 for 3-byte structs or handle the i32 correctly. Which one does clang do?

dotdash · 2018-01-21T13:32:22Z

Uses i24 for x86_64 but i32 for sparc64. In either case it does what I described above though. Am 21.01.2018 14:12 schrieb "Eduard-Mihai Burtescu" < [email protected]>:

…

I mean that either we should always be using i24 for 3-byte structs or handle the i32 correctly. Which one does clang do? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#47614 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOGMjetk2St-3hXw-HIh9whcD6hbgJnks5tMzfJgaJpZM4RlfSl> .

shepmaster · 2018-01-26T18:34:13Z

Ping from triage for you, @eddyb !

eddyb · 2018-01-26T21:13:12Z

@bors r+

bors · 2018-01-26T21:13:13Z

📌 Commit a0aa101 has been approved by eddyb

bors · 2018-01-27T04:12:23Z

⌛ Testing commit a0aa101e458735d6282744801416dab4904592e4 with merge e8ece949ff4a8df4ca2ecc922b2c2816c0dbbb44...

bors · 2018-01-27T05:14:13Z

💔 Test failed - status-travis

kennytm · 2018-01-27T08:01:18Z

codegen/repr-transparent.rs needs to be updated...

--- a/src/test/codegen/repr-transparent.rs
+++ b/src/test/codegen/repr-transparent.rs
@@ -145,7 +145,7 @@ pub struct Rgb8 { r: u8, g: u8, b: u8 }
 pub struct Rgb8Wrap(Rgb8);
 
 // NB: closing parenthesis is missing because sometimes the argument has a name and sometimes not
-// CHECK: define i32 @test_Rgb8Wrap(i32
+// CHECK: define i24 @test_Rgb8Wrap(i32
 #[no_mangle]
 #[cfg(all(target_arch="x86_64", target_os="linux"))]
 pub extern fn test_Rgb8Wrap(_: Rgb8Wrap) -> Rgb8Wrap { loop {} }

[00:58:30] ---- [codegen] codegen/repr-transparent.rs stdout ----
[00:58:30] 	
[00:58:30] error: verification with 'FileCheck' failed
[00:58:30] status: exit code: 1
[00:58:30] command: "/usr/lib/llvm-3.9/bin/FileCheck" "--input-file" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/repr-transparent.ll" "/checkout/src/test/codegen/repr-transparent.rs"
[00:58:30] stdout:
[00:58:30] ------------------------------------------
[00:58:30] 
[00:58:30] ------------------------------------------
[00:58:30] stderr:
[00:58:30] ------------------------------------------
[00:58:30] /checkout/src/test/codegen/repr-transparent.rs:148:11: error: expected string not found in input
[00:58:30] // CHECK: define i32 @test_Rgb8Wrap(i32
[00:58:30]           ^
[00:58:30] /checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/repr-transparent.ll:120:44: note: scanning from here
[00:58:30] define float @test_Projection(float %arg0) unnamed_addr #0 {
[00:58:30]                                            ^
[00:58:30] /checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/repr-transparent.ll:129:1: note: possible intended match here
[00:58:30] define i24 @test_Rgb8Wrap(i24) unnamed_addr #0 {
[00:58:30] ^
[00:58:30] 
[00:58:30] ------------------------------------------
[00:58:30] 
[00:58:30] thread '[codegen] codegen/repr-transparent.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:2883:9
[00:58:30] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:58:30] 
[00:58:30] 
[00:58:30] failures:
[00:58:30]     [codegen] codegen/repr-transparent.rs
[00:58:30] 
[00:58:30] test result: �[31mFAILED�(B�[m. 50 passed; 1 failed; 16 ignored; 0 measured; 0 filtered out

dotdash · 2018-01-28T12:34:07Z

@bors r=eddyb

bors · 2018-01-28T12:34:07Z

📌 Commit 8ecadd2 has been approved by eddyb

hanna-kruppe · 2018-01-28T12:57:52Z

src/test/codegen/repr-transparent.rs

@@ -145,7 +145,7 @@ pub struct Rgb8 { r: u8, g: u8, b: u8 }
 pub struct Rgb8Wrap(Rgb8);

 // NB: closing parenthesis is missing because sometimes the argument has a name and sometimes not
-// CHECK: define i32 @test_Rgb8Wrap(i32
+// CHECK: define i24 @test_Rgb8Wrap(i32


This will fail on non-x86_64 linux. Probably best to move this test into a separate file with appropriate only-*/ignore-* lines.

hanna-kruppe · 2018-01-28T12:58:02Z

@bors r- The repr(transparent) test change will fail (see above).

dotdash · 2018-01-28T13:24:41Z

Haha, I got the logic for the tests backwards, and wondered why the code already seemed prepared for this patch.

pietroalbini · 2018-02-05T09:51:21Z

Hi @dotdash, just checking if the PR is still worked on! Could you fix the test as per #47614 (comment)?

dotdash · 2018-02-06T12:32:10Z

@rkruppe Like that?

hanna-kruppe · 2018-02-06T14:46:36Z

src/test/codegen/repr-transparent-x86_64.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// only-x86_64


I'm not sure but this might need more restrictions, since not all x86_64 targets use the SysV ABI. Or alternatively, make the function below extern "sysv64" as in the other codegen test.

Right, I forgot to add the other only-XXX line that I meant to add, but I think forcing sysv ABI is even better here, so I'll go with that. Thanks for the suggestion

hanna-kruppe · 2018-02-06T14:47:38Z

src/test/codegen/repr-transparent-x86_64.rs

+#[repr(transparent)]
+pub struct Rgb8Wrap(Rgb8);
+
+// NB: closing parenthesis is missing because sometimes the argument has a name and sometimes not


Aside: I think this was an artifact of the previous hack and shouldn't be necessary any more. Not that important, though.

That shouldn't have made a difference. The only thing that should make a difference here is whether e.g. --emit llvm-ir is given, making LLVM preserve names.

I'm pretty sure the difference was due to declaring the function as taking an aggregate vs taking a primitive, and possibly also platform dependent. But whatever, it should definitely be consistent now.

Oh, you're right, I botched my local test and passed the wrong struct.

hanna-kruppe · 2018-02-06T14:56:39Z

src/test/codegen/repr-transparent.rs

-#[cfg(not(all(target_arch="x86_64", target_os="linux")))]
-#[no_mangle]
-pub extern fn test_Rgb8Wrap(_: u32) -> u32 { loop {} }
-
 // Same as with the small struct above: ABI-dependent, we only test the interesting case


This comment and the one above doesn't quite fit any more. I'd move the union FloatBits test below into repr-transparent-x86_64.rs (perhaps better called repr-transparent-sysv64.rs?) and replace everything from line 126 onwards with:

// All that remains to be tested are aggregates. They are tested in separate files called repr- // transparent-*.rs with `only-*` or `ignore-*` directives, because the expected LLVM IR // function signatures vary so much that it's not reasonably possible to cover all of them with a // single CHECK line. // // You may be wondering why we don't just compare the return types and argument types for equality // with FileCheck regex captures. Well, rustc doesn't perform newtype unwrapping on newtypes // containing aggregates. This is OK on all ABIs we support, but because LLVM has not gotten rid of // pointee types yet, the IR function signature will be syntactically different (%Foo* vs // %FooWrapper*).

hanna-kruppe · 2018-02-06T19:39:09Z

src/test/codegen/repr-transparent-sysv64.rs

+
+// CHECK: define i32 @test_SmallUnion(i32)
+#[no_mangle]
+#[cfg(all(target_arch="x86_64", target_os="linux"))]


This cfg is not necessary any more, and in fact as written will fail the test on non-linux targets.

sigh I shouldn't try to squeeze things in between other tasks.

The x86_64 SysV ABI should use exact sizes for small structs passed in registers, i.e. a struct that occupies 3 bytes should use an i24, instead of the i32 it currently uses. Refs rust-lang#45543

dotdash · 2018-02-08T12:52:08Z

I hope I didn't miss anything this time around...

hanna-kruppe · 2018-02-08T13:50:48Z

Tests look good now. Let's wait for travis though, since these tests only run on the one target travis actually tests.

hanna-kruppe · 2018-02-11T09:57:42Z

@bors r=eddyb

bors · 2018-02-11T09:57:43Z

📌 Commit 5f3dc8b has been approved by eddyb

bors · 2018-02-11T11:30:21Z

⌛ Testing commit 5f3dc8b with merge 2c77443...

Fix oversized loads on x86_64 SysV FFI calls The x86_64 SysV ABI should use exact sizes for small structs passed in registers, i.e. a struct that occupies 3 bytes should use an i24, instead of the i32 it currently uses. Refs #45543

bors · 2018-02-11T13:24:10Z

💔 Test failed - status-travis

kennytm · 2018-02-11T14:35:12Z

@bors retry #48116

bors · 2018-02-11T15:10:52Z

⌛ Testing commit 5f3dc8b with merge 0196b20...

Fix oversized loads on x86_64 SysV FFI calls The x86_64 SysV ABI should use exact sizes for small structs passed in registers, i.e. a struct that occupies 3 bytes should use an i24, instead of the i32 it currently uses. Refs #45543

bors · 2018-02-11T18:10:57Z

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

rust-highfive assigned petrochenkov Jan 20, 2018

rust-highfive assigned eddyb and unassigned petrochenkov Jan 20, 2018

shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 20, 2018

kennytm 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 Jan 27, 2018

dotdash force-pushed the x86_64_sysv_ffi branch from a0aa101 to 8ecadd2 Compare January 28, 2018 12:32

hanna-kruppe reviewed Jan 28, 2018

View reviewed changes

dotdash force-pushed the x86_64_sysv_ffi branch 2 times, most recently from 3771c77 to f08883e Compare February 6, 2018 10:31

hanna-kruppe reviewed Feb 6, 2018

View reviewed changes

dotdash force-pushed the x86_64_sysv_ffi branch from f08883e to a0e4dd7 Compare February 6, 2018 16:00

hanna-kruppe reviewed Feb 6, 2018

View reviewed changes

Fix oversized loads on x86_64 SysV FFI calls

5f3dc8b

The x86_64 SysV ABI should use exact sizes for small structs passed in registers, i.e. a struct that occupies 3 bytes should use an i24, instead of the i32 it currently uses. Refs rust-lang#45543

dotdash force-pushed the x86_64_sysv_ffi branch from a0e4dd7 to 5f3dc8b Compare February 8, 2018 12:50

kennytm 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 Feb 8, 2018

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 Feb 11, 2018

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 Feb 11, 2018

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 Feb 11, 2018

kennytm mentioned this pull request Feb 11, 2018

Spurious (?) error in "compile-fail\rfc-2126-extern-in-paths\single-segment.rs" etc #48116

Closed

bors merged commit 5f3dc8b into rust-lang:master Feb 11, 2018

dotdash deleted the x86_64_sysv_ffi branch February 10, 2022 15:12

Fix oversized loads on x86_64 SysV FFI calls #47614

Fix oversized loads on x86_64 SysV FFI calls #47614

Conversation

dotdash commented Jan 20, 2018

rust-highfive commented Jan 20, 2018

eddyb commented Jan 20, 2018

eddyb commented Jan 21, 2018

dotdash commented Jan 21, 2018 via email

eddyb commented Jan 21, 2018

dotdash commented Jan 21, 2018 via email

shepmaster commented Jan 26, 2018

eddyb commented Jan 26, 2018

bors commented Jan 26, 2018

bors commented Jan 27, 2018

bors commented Jan 27, 2018

kennytm commented Jan 27, 2018

dotdash commented Jan 28, 2018

bors commented Jan 28, 2018

Choose a reason for hiding this comment

hanna-kruppe commented Jan 28, 2018

dotdash commented Jan 28, 2018

pietroalbini commented Feb 5, 2018

dotdash commented Feb 6, 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

hanna-kruppe Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotdash commented Feb 8, 2018

hanna-kruppe commented Feb 8, 2018

hanna-kruppe commented Feb 11, 2018

bors commented Feb 11, 2018

bors commented Feb 11, 2018

bors commented Feb 11, 2018

kennytm commented Feb 11, 2018

bors commented Feb 11, 2018

bors commented Feb 11, 2018

hanna-kruppe Feb 6, 2018 •

edited

Loading