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

Remove structure packing where not needed #15

Open
hnrose opened this issue Apr 23, 2019 · 4 comments
Open

Remove structure packing where not needed #15

hnrose opened this issue Apr 23, 2019 · 4 comments

Comments

@hnrose
Copy link
Contributor

hnrose commented Apr 23, 2019

[jgunthorpe wrote:]
The reason you can't take the address of a packed member is because it is not aligned, it is simply an error and you shouldn't ever do it - it will crash at runtime on ARM. If the member is actually aligned then don't use packed, but use the proper attribute aligned to tell the compiler what is happening and it won't complain.

605osm_vendor_ibumad.c:409:41: error: taking address of packed member 'trans_id' of
606 class or structure '_ib_mad' may result in an unaligned pointer value
607 [-Werror,-Waddress-of-packed-member]
608 if (!(p_req_madw = get_madw(p_vend, &p_mad->trans_id,
609 ^~~~~~~~~~~~~~~
610osm_vendor_ibumad.c:437:35: error: taking address of packed member 'trans_id' of
611 class or structure '_ib_mad' may result in an unaligned pointer value
612 [-Werror,-Waddress-of-packed-member]
613 p_req_madw = get_madw(p_vend, &p_mad->trans_id,
614 ^~~~~~~~~~~~~~~
615osm_vendor_ibumad.c:1211:22: error: taking address of packed member 'trans_id'
616 of class or structure '_ib_mad' may result in an unaligned pointer value
617 [-Werror,-Waddress-of-packed-member]
618 get_madw(p_vend, &p_mad->trans_id,
619 ^~~~~~~~~~~~~~~
6203 errors generated.

The above occurred in libvendor/osm_vendor_ibumad.c where p_mad->trans_id was being accessed and p_mad is a pointer to ib_mad_t.

This is mainly issue in ib_types.h

@hnrose
Copy link
Contributor Author

hnrose commented Apr 23, 2019

[jgunthorpe wrote:]
Generally all MAD structures are aligned to 4 bytes, so what we did for srp_daemon/etc is to increase the alignment and use pahole & static_assert to validate the struct layout didn't change.

@hnrose
Copy link
Contributor Author

hnrose commented Apr 24, 2019

[nmorey wrote:]
I did a quick check with pahole. A lot fo struct just change size because they get 4 or 8B aligned which should not be an issue. But some get some internal padding between fields so we'll have deal carefully here.

@hnrose
Copy link
Contributor Author

hnrose commented Apr 24, 2019

@nmorey Most MAD attributes in IBA were spec'd to follow natural alignment but there are a small but significant number which do not. AFAIR NodeIndo is one of those because the GUIDs are not 64 bit aligned. There are others I've run across over time. Do you have a list of the ones which pahole found ?

@hnrose
Copy link
Contributor Author

hnrose commented Apr 24, 2019

[jgunthorpe wrote:]
Generally the MADs have a natural alignment of 4 bytes and 64 bit values are only aligned to 4 bytes, not 8. This is why we ended up defining umad_gid as aligned(4) so it was compatible with MAD structures that have 4 byte GID alignment.

When we did srp_daemon it took some fussing with attributes and other adjustments to make the structs have the same layout with a higher alignment than packed. But pahole is reliable and if it says the struct has the same layout, then it does.

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

No branches or pull requests

1 participant