Page MenuHomeFreeBSD

Avoid taking the address of a packed struct member in mfiutil
ClosedPublic

Authored by dim on Jan 6 2017, 7:08 PM.

Details

Summary

Fix a clang 4.0.0 warning about taking the address of a packed member of
struct mfi_evt in mfiutil:

usr.sbin/mfiutil/mfi_evt.c:583:30: error: taking address of packed member 'members' of class or structure 'mfi_evt' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
                        if (parse_locale(optarg, &filter.members.locale) < 0) {
                                                  ^~~~~~~~~~~~~~~~~~~~~

Use a local variable instead, and copy that into the struct.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6596
Build 6814: arc lint + arc unit

Event Timeline

dim updated this revision to Diff 23667.Jan 6 2017, 7:08 PM
dim retitled this revision from to Avoid taking the address of a packed struct member in mfiutil.
dim updated this object.
dim edited the test plan for this revision. (Show Details)
dim added reviewers: emaste, jhb.
jhb edited edge metadata.Jan 6 2017, 7:19 PM

Ok, though the warning is actually a bit wrong. The structure is marked packed to match a firmware structure (so no padding sneaks in), but the structure itself is always going to be aligned and the field within it will be properly aligned. (This is more a gripe about the warning than your change.)

imp added a comment.Jan 6 2017, 7:34 PM
In D9069#187321, @jhb wrote:

Ok, though the warning is actually a bit wrong. The structure is marked packed to match a firmware structure (so no padding sneaks in), but the structure itself is always going to be aligned and the field within it will be properly aligned. (This is more a gripe about the warning than your change.)

Shouldn't we mark it as __aligned(4) or something then?

dim added a comment.Jan 6 2017, 11:26 PM
In D9069#187321, @jhb wrote:

Ok, though the warning is actually a bit wrong. The structure is marked packed to match a firmware structure (so no padding sneaks in), but the structure itself is always going to be aligned and the field within it will be properly aligned. (This is more a gripe about the warning than your change.)

Yes, this is more about the compiler not being able to ascertain that it will be aligned at this point. The warning was implemented upstream here: https://reviews.llvm.org/rL278483, where the commit message says:

This change adds a new warning to clang emitted when taking the address of a packed member. A packed member is either a field/data member declared as attribute((packed)) or belonging to a struct/class declared as such. The associated flag is -Waddress-of-packed-member. Conversions (either implicit or via a valid casting) to pointer types with lower or equal alignment requirements (e.g. void* or char*) will silence the warning.

So we could also work around this by explicit casting.

dim added a comment.Jan 8 2017, 3:03 PM

So what shall I do with this? Suppress the warning, or commit this workaround?

jhb added a comment.Jan 9 2017, 4:23 PM

I think this is fine to commit. It will get messier in the kernel perhaps in the driver code that examines firmware structures. At that point we might want to start noting the things are already aligned somehow.

This revision was automatically updated to reflect the committed changes.