Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

Fix sizeof(http_parser) assert #510

Merged
merged 0 commits into from
Jul 10, 2020
Merged

Fix sizeof(http_parser) assert #510

merged 0 commits into from
Jul 10, 2020

Conversation

bnoordhuis
Copy link
Member

The result should be 32 on both 32 bits and 64 bits architectures
because of struct padding.

Fixes: #507

@bnoordhuis
Copy link
Member Author

@indutny Can I get a quick LGTM?

@mbakke
Copy link

mbakke commented May 21, 2020

@bnoordhuis This works on ARMv7, but fails on i686:

$ ./test_g 
http_parser v2.9.4 (0x020904)
sizeof(http_parser) = 28
test_g: test.c:4240: main: Assertion `sizeof(http_parser) == 32' failed.
Aborted

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

RSLGTM

@thefloweringash
Copy link

Turns out the comment from stack overflow I read in #507 (comment) was not sufficient. The actual rule for this case is detailed in The Lost Art of Structure Packing.

In general, a struct instance will have the alignment of its widest scalar member

Which is not the same as being aligned to the size of the widest member. Apparently x86 and arm differ on the alignment requirements for 64-bit values, where arm requires 8-byte alignment and x86 requires 4-byte alignment. This particular case is covered by a note about arm and x86 compatibility from intel:

The 8-byte (64-bit) mVar2 results in different layout for TestStruct. This is because ARM requires 8-byte alignment for 64-bit variables like mVar2.

So it seems this test needs to encode the abi requirements.

@delroth
Copy link

delroth commented May 17, 2022

As mentioned by #510 (comment) the fix which was merged here breaks testing on i686-linux (where sizeof == 28, not 32). It seems like the only net effect was trading i686 compat for armv7 compat.

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

Successfully merging this pull request may close these issues.

armv7hl: Assertion `sizeof(http_parser) == 4 + 4 + 8 + 2 + 2 + 4 + sizeof(void *)' failed
5 participants