| Message ID | 20170306123150.99386-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 06.03.17 at 13:31, <roger.pau@citrix.com> wrote: > Clang 4.0 has added a new check that triggers when taking the address of a > field of a packed structure. I've explained our situation to the llvm/clang > folks, but nobody seem to care: > > https://reviews.llvm.org/D20561 > > So simply disable that check if present. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Acked-by: Jan Beulich <jbeulich@suse.com>
On 06/03/17 12:31, Roger Pau Monne wrote: > Clang 4.0 has added a new check that triggers when taking the address of a > field of a packed structure. I've explained our situation to the llvm/clang > folks, but nobody seem to care: > > https://reviews.llvm.org/D20561 > > So simply disable that check if present. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> In this case, I'd prefer that we drop the unnecessary packed on segment_attributes and segment_register, perhaps adding some BUILD_BUG_ON()'s in the SVM code to check that the structure still matches hardwares representation. Or were there other structures which ended up in a similar situation? ~Andrew
>>> On 06.03.17 at 13:31, <roger.pau@citrix.com> wrote: > --- a/Config.mk > +++ b/Config.mk > @@ -216,6 +216,7 @@ $(call cc-option-add,HOSTCFLAGS,HOSTCC,-Wdeclaration-after-statement) > $(call cc-option-add,CFLAGS,CC,-Wdeclaration-after-statement) > $(call cc-option-add,CFLAGS,CC,-Wno-unused-but-set-variable) > $(call cc-option-add,CFLAGS,CC,-Wno-unused-local-typedefs) > +$(call cc-option-add,CFLAGS,CC,-Wno-address-of-packed-member) Actually, having thought some more about this, the warning should be suppressed only for x86 imo. ARM wants aligned accesses after all. Jan
On 06/03/17 13:58, Jan Beulich wrote: >>>> On 06.03.17 at 13:31, <roger.pau@citrix.com> wrote: >> --- a/Config.mk >> +++ b/Config.mk >> @@ -216,6 +216,7 @@ $(call cc-option-add,HOSTCFLAGS,HOSTCC,-Wdeclaration-after-statement) >> $(call cc-option-add,CFLAGS,CC,-Wdeclaration-after-statement) >> $(call cc-option-add,CFLAGS,CC,-Wno-unused-but-set-variable) >> $(call cc-option-add,CFLAGS,CC,-Wno-unused-local-typedefs) >> +$(call cc-option-add,CFLAGS,CC,-Wno-address-of-packed-member) > > Actually, having thought some more about this, the warning > should be suppressed only for x86 imo. ARM wants aligned > accesses after all. Looking at Roger's complaint, it appears that the warning is issued even if the member actually is aligned, if *on some unknown system*, it might someday be un-aligned. -George
At 14:36 +0000 on 06 Mar (1488811016), George Dunlap wrote: > On 06/03/17 13:58, Jan Beulich wrote: > >>>> On 06.03.17 at 13:31, <roger.pau@citrix.com> wrote: > >> --- a/Config.mk > >> +++ b/Config.mk > >> @@ -216,6 +216,7 @@ $(call cc-option-add,HOSTCFLAGS,HOSTCC,-Wdeclaration-after-statement) > >> $(call cc-option-add,CFLAGS,CC,-Wdeclaration-after-statement) > >> $(call cc-option-add,CFLAGS,CC,-Wno-unused-but-set-variable) > >> $(call cc-option-add,CFLAGS,CC,-Wno-unused-local-typedefs) > >> +$(call cc-option-add,CFLAGS,CC,-Wno-address-of-packed-member) > > > > Actually, having thought some more about this, the warning > > should be suppressed only for x86 imo. ARM wants aligned > > accesses after all. > > Looking at Roger's complaint, it appears that the warning is issued even > if the member actually is aligned, if *on some unknown system*, it might > someday be un-aligned. AIUI the complaint is (based on the simplified example from the ticket): struct __attribute__((__packed__)) bar { uint16_t x1; uint16_t x2; } b; &b.x2; Because the struct is packed, it has alignment 1, and so do its fields. &b.x2 is a pointer to a uint16_t, but it _isn't_ 16-bit aligned (because the whole struct is only byte-aligned). So I guess that one fix would be to declare that the struct has appropriate alignment? I don't know whether that would suppress the warning, but the clang devs might be more receptive to seeing it as a false positive. Tim.
On 06/03/17 15:16, Tim Deegan wrote: > At 14:36 +0000 on 06 Mar (1488811016), George Dunlap wrote: >> On 06/03/17 13:58, Jan Beulich wrote: >>>>>> On 06.03.17 at 13:31, <roger.pau@citrix.com> wrote: >>>> --- a/Config.mk >>>> +++ b/Config.mk >>>> @@ -216,6 +216,7 @@ $(call cc-option-add,HOSTCFLAGS,HOSTCC,-Wdeclaration-after-statement) >>>> $(call cc-option-add,CFLAGS,CC,-Wdeclaration-after-statement) >>>> $(call cc-option-add,CFLAGS,CC,-Wno-unused-but-set-variable) >>>> $(call cc-option-add,CFLAGS,CC,-Wno-unused-local-typedefs) >>>> +$(call cc-option-add,CFLAGS,CC,-Wno-address-of-packed-member) >>> Actually, having thought some more about this, the warning >>> should be suppressed only for x86 imo. ARM wants aligned >>> accesses after all. >> Looking at Roger's complaint, it appears that the warning is issued even >> if the member actually is aligned, if *on some unknown system*, it might >> someday be un-aligned. > AIUI the complaint is (based on the simplified example from the ticket): > > struct __attribute__((__packed__)) bar { > uint16_t x1; > uint16_t x2; > } b; > > &b.x2; > > Because the struct is packed, it has alignment 1, and so do its > fields. &b.x2 is a pointer to a uint16_t, but it _isn't_ 16-bit > aligned (because the whole struct is only byte-aligned). > > So I guess that one fix would be to declare that the struct has > appropriate alignment? I don't know whether that would suppress the > warning, but the clang devs might be more receptive to seeing it as > a false positive. The structs in question (segment_attributes and segment_register) have proper natural alignment with no padding anyway, so don't need to be __packed__ to be correct. It would be better to remove the unnecessary __packed__. ~Andrew
diff --git a/Config.mk b/Config.mk index 81550a7..e9f60c7 100644 --- a/Config.mk +++ b/Config.mk @@ -216,6 +216,7 @@ $(call cc-option-add,HOSTCFLAGS,HOSTCC,-Wdeclaration-after-statement) $(call cc-option-add,CFLAGS,CC,-Wdeclaration-after-statement) $(call cc-option-add,CFLAGS,CC,-Wno-unused-but-set-variable) $(call cc-option-add,CFLAGS,CC,-Wno-unused-local-typedefs) +$(call cc-option-add,CFLAGS,CC,-Wno-address-of-packed-member) LDFLAGS += $(foreach i, $(EXTRA_LIB), -L$(i)) CFLAGS += $(foreach i, $(EXTRA_INCLUDES), -I$(i))
Clang 4.0 has added a new check that triggers when taking the address of a field of a packed structure. I've explained our situation to the llvm/clang folks, but nobody seem to care: https://reviews.llvm.org/D20561 So simply disable that check if present. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wei.liu2@citrix.com> --- Config.mk | 1 + 1 file changed, 1 insertion(+)