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

transpile: emit zeroed array expressions as [0; N] #1170

Merged

Conversation

folkertdev
Copy link
Contributor

an input like

int foo() {
    int x[16] = {};
    return 0;
}

was emitted as

    let mut bar: [libc::c_int; 16] = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0];

but now emits

    let mut bar: [libc::c_int; 16] = [0; 16];

The current version only supports floating and integral types: in more recent rust versions it would be easy to make this work for e.g. structs: just use a const { ... } block. But c2rust targets older versions, and there (without introducing extra constants) the expression's type must be copy, and I could not find a convenient way to tell whether a CTypeId will derive/implement Copy.

this came up when working with the output of creduce, which loves zeroing arrays.

@thedataking
Copy link
Contributor

Thanks Folkert, I like this idea. Your comment indicates the output Rust is likely correct – but not necessarily so. Any examples where this would fail? If so, should this be guarded by a flag like other risky/experimental features?

Also, it would be great if you could add a test or two.

@folkertdev folkertdev force-pushed the use-repeat-for-zeroed-arrays branch from a56b3ee to e3405fd Compare November 28, 2024 09:33
@folkertdev
Copy link
Contributor Author

I believe the current change is correct, across rust versions. It just isn't as general as it could be.

For a "repeat" expression [E; N], the expression E must be either

  • a value of a type that is Copy
  • a constant value (so a path to a const or, in newer versions, a const { ... } expression)

The current versions restricts the type to be numeric ty.is_integral_type() || ty.is_floating_type(), because we know that all of those numeric types impl Copy, and hence will work.

But e.g. a typedef struct { int x, int y } Point still gets emitted as a long array literal right now.

For such a struct, c2rust will actually emit a #[derive(Clone, Copy)] down the line, so if that information were available then an [E; N] style array could be emitted for any type that is impl Copy. That "does this impl Copy?" information must be somewhere, I just could not find it.

Also, it would be great if you could add a test or two.

I added something basic, what else do you want to test and where?

@kkysen
Copy link
Contributor

kkysen commented Nov 30, 2024

@folkertdev, thanks for this improvement! It seems like a sound transformation to me.

For such a struct, c2rust will actually emit a #[derive(Clone, Copy)] down the line, so if that information were available then an [E; N] style array could be emitted for any type that is impl Copy. That "does this impl Copy?" information must be somewhere, I just could not find it.

I'm pretty sure that all C types are implicitly Copy, so c2rust should mark all such transpiled types #[derive(Clone, Copy)]. I think we can include these, not just primitives.

That said, the CI is failing:

error[E0599]: no method named `is_integral_type` found for struct `c_ast::CTypeId` in the current scope
   --> c2rust-transpile/src/translator/literals.rs:193:48
    |
193 |                 } else if ids.len() == 0 && ty.is_integral_type() || ty.is_floating_type() {
    |                                                ^^^^^^^^^^^^^^^^ method not found in `c_ast::CTypeId`
    |
   ::: c2rust-transpile/src/c_ast/mod.rs:15:1
    |
15  | pub struct CTypeId(pub u64);
    | ------------------ method `is_integral_type` not found for this struct

error[E0599]: no method named `is_floating_type` found for struct `c_ast::CTypeId` in the current scope
   --> c2rust-transpile/src/translator/literals.rs:193:73
    |
193 |                 } else if ids.len() == 0 && ty.is_integral_type() || ty.is_floating_type() {
    |                                                                         ^^^^^^^^^^^^^^^^ method not found in `c_ast::CTypeId`
    |
   ::: c2rust-transpile/src/c_ast/mod.rs:15:1
    |
15  | pub struct CTypeId(pub u64);
    | ------------------ method `is_floating_type` not found for this struct

Did you see this?

@folkertdev folkertdev force-pushed the use-repeat-for-zeroed-arrays branch from e3405fd to e86322b Compare November 30, 2024 13:04
@folkertdev
Copy link
Contributor Author

folkertdev commented Nov 30, 2024

C data types should be Copy, though I did find this case, which creates a heap-allocated vector (i.e. that can´t be Copy)

} else if let &CTypeKind::VariableArray(elt, _) = resolved_ty {
// Variable length arrays unnested and implemented as a flat array of the underlying
// element type.
// Find base element type of potentially nested arrays
let inner = self.variable_array_base_type(elt);
let count = self.compute_size_of_expr(ty_id).unwrap();
Ok(self
.implicit_default_expr(inner, is_static)?
.map(|val| vec_expr(val, count)))

I think that would not come up here because we're working with a CTypeKind::ConstantArray, but I'm not 100% sure.

CI should work now

Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's a good find. I didn't think about VLAs and unsized types.

I think that would not come up here because we're working with a CTypeKind::ConstantArray, but I'm not 100% sure.

I would think so, too. If the C compiles, an unsized type shouldn't be allowed in an array, and therefore the array's element type should be Copy.

Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now, but could someone else take a quite look to make sure there's not a case I'm overlooking where this might not work? @fw-immunant maybe?

@kkysen kkysen changed the title emit zeroed array expressions as [0; N] transpile: emit zeroed array expressions as [0; N] Dec 2, 2024
@kkysen kkysen changed the title transpile: emit zeroed array expressions as [0; N] transpile: emit zeroed array expressions as [0; N] Dec 2, 2024
@fw-immunant
Copy link
Contributor

I think this is fine, but I would want to test it against a case like struct {char* x; int y;} x[1] = {};.

an array like `int x[16] = {}` was emitted as `[0, 0, 0, 0, ...]`,
but is not emitted as `[0; 16]`.
@folkertdev folkertdev force-pushed the use-repeat-for-zeroed-arrays branch from e86322b to 9e5a1c0 Compare December 4, 2024 10:00
@folkertdev
Copy link
Contributor Author

I added that test case

@thedataking thedataking merged commit 6df47da into immunant:master Dec 6, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants