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

graphene: fix alignment of graphene_simd4f_t on armhf and s390x #1105

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

schopin-pro
Copy link

On systems with the armhf ABI as used by Debian and Ubuntu (amdv7 + VFP, but without NEON), graphene falls back to the scalar definition of graphene_simd4f_t, which has the same alignment as its base type, float.

On s390x, the underlying type is a generic float
attribute((vector_size(16)). Surprinsingly, the s390x ABI does not mandate alignment bigger than 8 for the vector types.

Without these patches, the layout tests fail on those architectures. The armhf case is notable because there are a fair number of Raspberry Pis out there that use that ABI, either because their SoC isn't 64-bit ready or just because they have low memory.

Ideally, we could specifiy this in the Gir.toml file, but it doesn't seem like the tool is flexible enough to express this kind of nuances,

On systems with the armhf ABI as used by Debian and Ubuntu (amdv7 + VFP,
but without NEON), graphene falls back to the scalar definition of
graphene_simd4f_t, which has the same alignment as its base type, float.

On s390x, the underlying type is a generic float
__attribute__((vector_size(16)). Surprinsingly, the s390x ABI does not
mandate alignment bigger than 8 for the vector types.

Without these patches, the layout tests fail on those architectures. The
armhf case is notable because there are a fair number of Raspberry Pis
out there that use that ABI, either because their SoC isn't 64-bit ready
or just because they have low memory.

Ideally, we could specifiy this in the Gir.toml file, but it doesn't
seem like the tool is flexible enough to express this kind of nuances,
@sdroege
Copy link
Member

sdroege commented May 29, 2023

Based on #1107 there is also a problem on armel and i386 in addition.

The main problem here is that src/lib.rs is autogenerated, so you can't just change it like this. It would have to be marked as manual = true in the Gir.toml and then we could implement it manually in src/manual.rs with whatever complicated alignment configuration is correct here.

See e.g. gio/sys or glib/sys as an example for this. Can you update it accordingly?

@sdroege
Copy link
Member

sdroege commented May 29, 2023

From that issue it sounds like graphene_simd4f_t is only wrong for i386, and graphene_simd4x4f_t is wrong for everything else. But here you modify only graphene_simd4f_t for s390x which is probably unnecessary?

@sdroege
Copy link
Member

sdroege commented Aug 7, 2023

Any updates here?

@schopin-pro
Copy link
Author

schopin-pro commented Aug 7, 2023 via email

@bilelmoussaoui
Copy link
Member

@schopin-pro any updates on this one?

@schopin-pro
Copy link
Author

schopin-pro commented Oct 27, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] 32bit arch tests fail for graphene-sys
3 participants