Remove unwrap() from cabi_x86_64.rs #47378
Conversation
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
|
Added a test for #38763. |
kennytm
added
the
S-waiting-on-review
src/librustc_trans/cabi_x86_64.rs
Outdated
| if let Ok(cls) = cls.as_ref() { | ||
| if let Some(target) = cast_target(cls, size) { | ||
| arg.cast_to(target); | ||
| } |
There was a problem hiding this comment.
Are the tests now passing because of the if let Ok(cls) or the if let Some(target)? I feel like instead of silently passing this should fail with a diagnostic error, but I'm not knowledgeable enough about this part fo the language. Otherwise the code seems ready to merge.
@eddyb could you take a look?
There was a problem hiding this comment.
See #38763 (comment) - I think this needs further investigation.
| let offset = Size::from_bytes(8) * (i as u64); | ||
| let target = if size <= offset { | ||
| CastTarget::from(lo) | ||
| } else { | ||
| let hi = reg_component(cls, &mut i, size - offset).unwrap(); | ||
| let hi = reg_component(cls, &mut i, size - offset)?; |
There was a problem hiding this comment.
These don't seem right - if the unwrap can fail that means there's a logic error somewhere within this file, which this seems to hide.
There was a problem hiding this comment.
Updated to call bug!() when this failed.
Add a test for rust-lang#38763. Update a test for rust-lang#45662.
topecongiro
force-pushed
the
issue-45662
branch
from
785910b to
b4ce007
Compare
|
@topecongiro I'm still not happy - I haven't seen any explanation for what's happening. |
|
I am sorry for being lazy about explaining what I was working on. Let me clarify a bit. I first tried to compile the following file using 83ba46f: // test.rs
#![feature(repr_align)]
#![feature(attr_literals)]
#[repr(align(16))]
pub struct Foo(i32);
#[no_mangle]
pub extern "C" fn foo(x: Foo) {}
fn main() {}and got the following error (an irrelevant part is omitted): Here, the content of
The new commits do not paper over errors, they will call |
Yes, because it should never fail. I think I see what's wrong here, we assumed primitives would fit into EDIIT: I mean, specifically when |
|
Cool, I will close this PR in favor of those two. @eddyb Thank you very much for your help! |
|
@topecongiro thank you for taking the time to dig into this in the first place! |
This PR is a follow-up of #47014. I am not certain whether simply removing
unwrap()suffices, but at least no test failed.Closes #38763. Closes #45662.