rust_trans: struct argument over ffi were passed incorrectly in some situations. Change cabi_x86_64 to better model the sysv abi. #27017
Conversation
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
luqmana
force-pushed
the
25594-sysv-abi-ffi
branch
from
9488b75 to
1e1f991
Compare
| assert(c == 3); | ||
| assert(d == 4); | ||
| assert(e == 5); | ||
| assert(s.a == 1); |
There was a problem hiding this comment.
I would prefer to use different numeric values here (and in all other tests).
|
Nice fixes and good test coverage. Thanks @luqmana |
luqmana
force-pushed
the
25594-sysv-abi-ffi
branch
from
1e1f991 to
b399fd0
Compare
| let ty = x86_64_ty(ccx, *t, |cls| cls.is_pass_byval(), Attribute::ByVal); | ||
| arg_tys.push(ty); | ||
| } | ||
| let mut avail_regs = 6; // RDI, RSI, RDX, RCX, R8, R9 |
There was a problem hiding this comment.
The ABI rules require counting both integer and floating-point registers.
There was a problem hiding this comment.
Yes but, this is a kinda simplistic approach similar to what clang does since we need to explicitly tell LLVM whether a struct is passed via some hidden pointer (marked byval) or split up/passed as a FCA. The actual specifics of which register or whether it goes on the stack is handled by LLVM.
There was a problem hiding this comment.
You still need to track them separately... suppose you have a struct which contains a long long and a double. If you run out of xmm registers, the struct needs to be explicitly byval instead of being split up: otherwise, the integer part of the struct ends up in a register. (clang has similar logic; see https://github.com/llvm-mirror/clang/blob/335cc9e22d8b977d83a12297fc85b6f6be965689/lib/CodeGen/TargetInfo.cpp#L2804 .)
There was a problem hiding this comment.
Right, forget about that. Updated, thanks!
luqmana
force-pushed
the
25594-sysv-abi-ffi
branch
2 times, most recently
from
b509c2b to
1ed9251
Compare
| }, Attribute::ByVal); | ||
| arg_tys.push(ty); | ||
|
|
||
| // An integer or pointer parameter |
There was a problem hiding this comment.
This comment needs to be updated.
luqmana
force-pushed
the
25594-sysv-abi-ffi
branch
2 times, most recently
from
2d6d4a8 to
d4ecbde
Compare
|
r? @rust-lang/compiler |
| #include <assert.h> | ||
|
|
||
| struct Rect { | ||
| int a; |
There was a problem hiding this comment.
Since Rust counterpart uses explicitly sized integers, could this use int32_t rather than int?
|
I would also like to see a test with a struct which is so big it doesn’t fit into the registers (i.e. 7 integers and/or 9 floats). Other than that, I don’t see anything wrong with this patch. |
luqmana
force-pushed
the
25594-sysv-abi-ffi
branch
2 times, most recently
from
e0e8590 to
b4dbbc4
Compare
|
@nagisa Added a big struct case. |
luqmana
force-pushed
the
25594-sysv-abi-ffi
branch
2 times, most recently
from
0978328 to
f0c86cc
Compare
|
@bors r+ Nice test coverage. Thanks! |
|
@bors r+ Not sure why @bors ignored @bkoropoff |
|
📌 Commit f0c86cc has been approved by |
|
⌛ Testing commit f0c86cc with merge 1e1d32a... |
|
💔 Test failed - auto-win-gnu-64-opt |
luqmana
force-pushed
the
25594-sysv-abi-ffi
branch
from
f0c86cc to
340bdee
Compare
luqmana
force-pushed
the
25594-sysv-abi-ffi
branch
from
340bdee to
83dda9d
Compare
|
What happened to this? http://buildbot.rust-lang.org/builders/auto-win-gnu-64-opt/builds/912 is now a 404 error. |
luqmana
force-pushed
the
25594-sysv-abi-ffi
branch
from
83dda9d to
670b810
Compare
|
Rebased and #29012 should address the windows failure. Updated the test comments and the test makefile to use not call gcc and friends directly but instead use the utilities that abstract platform differences. |
|
@bors: r=bkoropoff |
|
📌 Commit 670b810 has been approved by |
|
⌛ Testing commit 670b810 with merge 654ba19... |
|
💔 Test failed - auto-win-gnu-32-nopt-t |
luqmana
force-pushed
the
25594-sysv-abi-ffi
branch
from
670b810 to
5eb4de1
Compare
|
@bors r=bkoropoff |
|
📌 Commit 5eb4de1 has been approved by |
Fixes #25594.