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

Codegen backends should validate their assumed encodings of e.g. rustc layouts. #105282

Open
eddyb opened this issue Dec 5, 2022 · 3 comments
Open
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Dec 5, 2022

Specifically the thing that came to mind is that rustc_codegen_llvm could assert LLVMSizeOf matches the rustc Layout every time it builds a LLVM type from a layout.
(and this should hopefully be cheap enough that we could always have it enabled)
(I generally dislike debug asserts as they can't be relied upon to enforce soundness...)

The motivating example is a discussion around a #[repr(simd)] struct i32x3([i32; 3]); type having size 12:

  • non-power-of-2 Simd types have wrong size portable-simd#319 (comment)
  • but as I note in the comment linked above, LLVM's <3 x i32> is 16 bytes, so that would be illegal
    • (16 bytes at least in "stride", which LLVM doesn't seem to name as such - it doesn't read/write 16 bytes on load/store, so it definitely seems to have more than once concept of "size" even for primitives)
    • (and no, #[repr(packed)] doesn't seem to do anything today, in combination with #[repr(simd)], so you couldn't get the 12-byte layout without changing e.g. the vector_align algorithm in the compiler)

I'm unsure about the GCC backend but presumably it should also be able to do this? (cc @antoyo)

cc @rust-lang/wg-llvm @bjorn3

@eddyb eddyb added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-codegen Area: Code generation labels Dec 5, 2022
@bjorn3
Copy link
Member

bjorn3 commented Dec 5, 2022

Cranelift doesn't support non-power-of-two vector types, so those will need to be implemented using scalars, which makes it easy to follow the rust layout. As for composite types, Cranelift doesn't support them either. Basically the only relevant check for Cranelift is if the pointer type size matches, which I already check.

@nikic
Copy link
Contributor

nikic commented Dec 5, 2022

(16 bytes at least in "stride", which LLVM doesn't seem to name as such - it doesn't read/write 16 bytes on load/store, so it definitely seems to have more than once concept of "size" even for primitives)

LLVM has a type size, a type store size (type size rounded up to bytes) and a type alloc size, which is the type store size rounded up to alignment (presumably what you mean by "stride").

@antoyo
Copy link
Contributor

antoyo commented Dec 5, 2022

With libgccjit, I could easily have an assert for simple types, but not all types.
There might be a way to do it for all types, but that will require much more work.

@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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