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

librustc_trans: function parameters should be annotated with zeroext/signext #30841

Closed
antonblanchard opened this issue Jan 12, 2016 · 9 comments
Assignees
Labels
A-codegen Area: Code generation P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@antonblanchard
Copy link
Contributor

type_of -> Type::uint_from_ty/Type::int_from_ty converts to LLVM types which have no sign. This is then passed to the architecture specific code in compute_abi_info().

PowerPC needs to know whether to sign or zero extend function call parameters, but it doesn't have enough information in the LLVM types.

@Aatch
Copy link
Contributor

Aatch commented Jan 15, 2016

I don't think it's compute_abi_info() that needs it really, it's just that we should be adding zeroext or signext parameter attributes where appropriate. Looking at the output of clang it always adds them for types with a size less than 32-bits.

I'm curious if you have a PowerPC-specific example, as we don't add these for other platforms and it hasn't caused a problem there. Does PowerPC do something different that means it matters more here?

@Aatch Aatch added the A-codegen Area: Code generation label Jan 15, 2016
@antonblanchard
Copy link
Contributor Author

This came up while reviewing #30776. I haven't seen a specific issue, but @eefriedman pointed out that I wasn't modelling the PowerPC64 ABI completely. The test case in C was:

void f(int); void g(unsigned long long x) { f(x); }

I'm not sure why x86_64 isn't annotating the LLVM IR with signext here.

@Aatch
Copy link
Contributor

Aatch commented Jan 15, 2016

Well we're not annotating the parameters properly for any platform, which at least suggests that is relatively low priority in terms of actual problems caused. The reason being that most of the time you're calling out to C code that will itself only use the parts of the register for the size of the appropriate type. What I mean is that it doesn't matter if the value is sign- or zero-extended if code never actually relies on the extended portion of the value.

Anyway, I think the PowerPC aspect here is a bit of a red herring and we should be annotating all parameters that need the annotations,

@antonblanchard antonblanchard changed the title librustc_trans: compute_abi_info() needs signedness info for PowerPC targets librustc_trans: function parameters should be annotated with zeroext/signext Jan 17, 2016
@antonblanchard
Copy link
Contributor Author

Agreed, changing the title to match.

@Aatch
Copy link
Contributor

Aatch commented Jan 20, 2016

Ah, I just figured out/found out why PowerPC is a problem specifically. PowerPC apparently doesn't support 8-bit or 16-bit operations natively and therefore you can't tell it to just add (for example) together to lowest byte/word in each register.

I'm surprised this hasn't caused any issues with calling out to C code, a quick test suggests that it could cause issues if you have a C function accepting signed bytes, Rust will just zero-extend them to the register size. This is on x86 too, so it's definitely a not a PPC-specific issue, and it actually much more serious than I first though.

@Aatch Aatch added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 20, 2016
@nikomatsakis
Copy link
Contributor

triage: P-medium

Good catch reporting this bug! This should definitely be fixed, but PowerPC is not a "tier 1" platform, so it's not a high priority (drop everything) sort of bug.

@rust-highfive rust-highfive added P-medium Medium priority and removed I-nominated labels Jan 21, 2016
@Aatch
Copy link
Contributor

Aatch commented Jan 26, 2016

@nikomatsakis this does affect other platforms though. Clang will assume that char function arguments have been sign-extended when generating x86 code, but we won't do so which means it could cause an issue.

I'm not disagreeing with the priority, I think medium is fine, just clarifying that this is a general FFI issue, not just a PPC issue.

@nikomatsakis
Copy link
Contributor

thanks for the clarification

On Mon, Jan 25, 2016 at 08:37:23PM -0800, James Miller wrote:

@nikomatsakis this does affect other platforms though. Clang will assume that char function arguments have been sign-extended when generating x86 code, but we won't do so which means it could cause an issue.

I'm not disagreeing with the priority, I think medium is fine, just clarifying that this is a general FFI issue, not just a PPC issue.


Reply to this email directly or view it on GitHub:
#30841 (comment)

@sanxiyn
Copy link
Member

sanxiyn commented May 8, 2016

This seems fixed by #32732.

@sanxiyn sanxiyn closed this as completed May 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants