| Message ID | 20201023200645.1055-3-dbuono@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for Control-Flow Integrity | expand |
On 23/10/2020 22.06, Daniele Buono wrote: > Clang 11 finds a couple of spots in the code that trigger new warnings: > > ../qemu-base/hw/usb/dev-uas.c:157:31: error: field 'status' with variable sized type 'uas_iu' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] > uas_iu status; > ^ > 1 error generated. > > The data structure is UASStatus, which must end with a QTAILQ_ENTRY, so > I believe we cannot have uas_iu at the end. Since this is a gnu > extension but CLANG supports it, just add > -Wno-gnu-variable-sized-type-not-at-end > to remove the warning. > > ../qemu-base/target/s390x/cpu_models.c:985:21: error: cast to smaller integer type 'S390Feat' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast] > S390Feat feat = (S390Feat) opaque; > ^~~~~~~~~~~~~~~~~ > ../qemu-base/target/s390x/cpu_models.c:1002:21: error: cast to smaller integer type 'S390Feat' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast] > S390Feat feat = (S390Feat) opaque; > ^~~~~~~~~~~~~~~~~ > ../qemu-base/target/s390x/cpu_models.c:1036:27: error: cast to smaller integer type 'S390FeatGroup' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast] > S390FeatGroup group = (S390FeatGroup) opaque; > ^~~~~~~~~~~~~~~~~~~~~~ > ../qemu-base/target/s390x/cpu_models.c:1057:27: error: cast to smaller integer type 'S390FeatGroup' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast] > S390FeatGroup group = (S390FeatGroup) opaque; > ^~~~~~~~~~~~~~~~~~~~~~ > 4 errors generated. > > These are void * that get casted to enums, which are (or can be) > smaller than a 64bit pointer. > A code reorg may be better on the long term, but for now will > fix this adding > -Wno-void-pointer-to-enum-cast Compiling all code with -Wno-void-pointer-to-enum-cast sounds like the wrong approach to me, since this might hide some real bugs in other spots instead. Could you please try to cast the value through (uintptr_t) first, e.g. : S390Feat feat = (S390Feat)(uintptr_t) opaque; It's a little bit ugly, but still better than to disable the warning globally, I think. Thomas > Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com> > --- > configure | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/configure b/configure > index e6754c1e87..9dc05cfb8a 100755 > --- a/configure > +++ b/configure > @@ -2000,6 +2000,8 @@ add_to nowarn_flags -Wno-shift-negative-value > add_to nowarn_flags -Wno-string-plus-int > add_to nowarn_flags -Wno-typedef-redefinition > add_to nowarn_flags -Wno-tautological-type-limit-compare > +add_to nowarn_flags -Wno-gnu-variable-sized-type-not-at-end > +add_to nowarn_flags -Wno-void-pointer-to-enum-cast > add_to nowarn_flags -Wno-psabi > > gcc_flags="$warn_flags $nowarn_flags" >
On 10/24/2020 1:17 AM, Thomas Huth wrote: > Compiling all code with -Wno-void-pointer-to-enum-cast sounds like the wrong > approach to me, since this might hide some real bugs in other spots instead. > > Could you please try to cast the value through (uintptr_t) first, e.g. : > > S390Feat feat = (S390Feat)(uintptr_t) opaque; > > It's a little bit ugly, but still better than to disable the warning > globally, I think. > > Thomas Hi Thomas, I did a quick check and casting to a uintptr_t seem to be a working solution to the issue. I will fix this in v3 Thanks, Daniele
On 23/10/20 22:06, Daniele Buono wrote: > 1 error generated. > > The data structure is UASStatus, which must end with a QTAILQ_ENTRY, so > I believe we cannot have uas_iu at the end. Since this is a gnu > extension but CLANG supports it, just add > -Wno-gnu-variable-sized-type-not-at-end This is potentially a real bug, in this case it works only because UASStatus's packet is never uas_iu_command (which has the variable sized type). The QTAILQ_ENTRY need not be at the end, please rearrange UASStatus's field so that the "usb_ui status" field is the last. Thanks, Paolo
Hi Paolo,
I reorganized UASStatus to put uas_iu at the end and it works fine.
Unfortunately, this uncovered another part of the code with a similar
issue (variable sized type not at the end of the struct), here:
In file included from ../qemu-cfi-v3/target/s390x/diag.c:21:
../qemu-cfi-v3/hw/s390x/ipl.h:161:23: error: field 'iplb' with variable
sized type 'IplParameterBlock' (aka 'union IplParameterBlock') not at
the end of a struct or class is a GNU extension
[-Werror,-Wgnu-variable-sized-type-not-at-end]
IplParameterBlock iplb;
^
../qemu-cfi-v3/hw/s390x/ipl.h:162:23: error: field 'iplb_pv' with
variable sized type 'IplParameterBlock' (aka 'union IplParameterBlock')
not at the end of a struct or class is a GNU extension
[-Werror,-Wgnu-variable-sized-type-not-at-end]
IplParameterBlock iplb_pv;
My understanding is that each of these IplParameterBlock may contain
either a IPLBlockPV or a IplBlockFcp, which both end with a variable
sized field (an array).
Adding maintainers of s390x to see if they have a suggested solution to
avoid disabling the warning.
On 10/26/2020 5:50 AM, Paolo Bonzini wrote:
> On 23/10/20 22:06, Daniele Buono wrote:
>> 1 error generated.
>>
>> The data structure is UASStatus, which must end with a QTAILQ_ENTRY, so
>> I believe we cannot have uas_iu at the end. Since this is a gnu
>> extension but CLANG supports it, just add
>> -Wno-gnu-variable-sized-type-not-at-end
>
> This is potentially a real bug, in this case it works only because
> UASStatus's packet is never uas_iu_command (which has the variable sized
> type).
>
> The QTAILQ_ENTRY need not be at the end, please rearrange UASStatus's
> field so that the "usb_ui status" field is the last.
>
> Thanks,
>
> Paolo
>
On 26/10/20 16:03, Daniele Buono wrote: > Hi Paolo, > I reorganized UASStatus to put uas_iu at the end and it works fine. > Unfortunately, this uncovered another part of the code with a similar > issue (variable sized type not at the end of the struct), here: > > In file included from ../qemu-cfi-v3/target/s390x/diag.c:21: > ../qemu-cfi-v3/hw/s390x/ipl.h:161:23: error: field 'iplb' with variable > sized type 'IplParameterBlock' (aka 'union IplParameterBlock') not at > the end of a struct or class is a GNU extension > [-Werror,-Wgnu-variable-sized-type-not-at-end] > IplParameterBlock iplb; > ^ > ../qemu-cfi-v3/hw/s390x/ipl.h:162:23: error: field 'iplb_pv' with > variable sized type 'IplParameterBlock' (aka 'union IplParameterBlock') > not at the end of a struct or class is a GNU extension > [-Werror,-Wgnu-variable-sized-type-not-at-end] > IplParameterBlock iplb_pv; > > My understanding is that each of these IplParameterBlock may contain > either a IPLBlockPV or a IplBlockFcp, which both end with a variable > sized field (an array). > > Adding maintainers of s390x to see if they have a suggested solution to > avoid disabling the warning. This one seems okay because the union constrains the size to 4K. If "[0]" is enough to shut up the compiler, I'd say do that. Paolo
Using an array of length 0 seems to be enough to avoid the warning Will use that solution in v3. Thanks, Daniele On 10/26/2020 11:12 AM, Paolo Bonzini wrote: > On 26/10/20 16:03, Daniele Buono wrote: >> Hi Paolo, >> I reorganized UASStatus to put uas_iu at the end and it works fine. >> Unfortunately, this uncovered another part of the code with a similar >> issue (variable sized type not at the end of the struct), here: >> >> In file included from ../qemu-cfi-v3/target/s390x/diag.c:21: >> ../qemu-cfi-v3/hw/s390x/ipl.h:161:23: error: field 'iplb' with variable >> sized type 'IplParameterBlock' (aka 'union IplParameterBlock') not at >> the end of a struct or class is a GNU extension >> [-Werror,-Wgnu-variable-sized-type-not-at-end] >> IplParameterBlock iplb; >> ^ >> ../qemu-cfi-v3/hw/s390x/ipl.h:162:23: error: field 'iplb_pv' with >> variable sized type 'IplParameterBlock' (aka 'union IplParameterBlock') >> not at the end of a struct or class is a GNU extension >> [-Werror,-Wgnu-variable-sized-type-not-at-end] >> IplParameterBlock iplb_pv; >> >> My understanding is that each of these IplParameterBlock may contain >> either a IPLBlockPV or a IplBlockFcp, which both end with a variable >> sized field (an array). >> >> Adding maintainers of s390x to see if they have a suggested solution to >> avoid disabling the warning. > > This one seems okay because the union constrains the size to 4K. If > "[0]" is enough to shut up the compiler, I'd say do that. > > Paolo >
On Mon, 26 Oct 2020 at 15:13, Paolo Bonzini <pbonzini@redhat.com> wrote: > This one seems okay because the union constrains the size to 4K. If > "[0]" is enough to shut up the compiler, I'd say do that. Do you mean converting a C flexible array member (declared as "foo[]") into a gcc zero-length array (declared as "foo[0]") ? That seems like it would be going backwards from the direction we started in with commits f7795e4096d8bd1c and 880a7817c1a82 of preferring flexible arrays to the GNU extension, so it's maybe not ideal, but I guess it's expedient. thanks -- PMM
On 26/10/2020 16.12, Paolo Bonzini wrote: > On 26/10/20 16:03, Daniele Buono wrote: >> Hi Paolo, >> I reorganized UASStatus to put uas_iu at the end and it works fine. >> Unfortunately, this uncovered another part of the code with a similar >> issue (variable sized type not at the end of the struct), here: >> >> In file included from ../qemu-cfi-v3/target/s390x/diag.c:21: >> ../qemu-cfi-v3/hw/s390x/ipl.h:161:23: error: field 'iplb' with variable >> sized type 'IplParameterBlock' (aka 'union IplParameterBlock') not at >> the end of a struct or class is a GNU extension >> [-Werror,-Wgnu-variable-sized-type-not-at-end] >> IplParameterBlock iplb; >> ^ >> ../qemu-cfi-v3/hw/s390x/ipl.h:162:23: error: field 'iplb_pv' with >> variable sized type 'IplParameterBlock' (aka 'union IplParameterBlock') >> not at the end of a struct or class is a GNU extension >> [-Werror,-Wgnu-variable-sized-type-not-at-end] >> IplParameterBlock iplb_pv; >> >> My understanding is that each of these IplParameterBlock may contain >> either a IPLBlockPV or a IplBlockFcp, which both end with a variable >> sized field (an array). >> >> Adding maintainers of s390x to see if they have a suggested solution to >> avoid disabling the warning. > > This one seems okay because the union constrains the size to 4K. If > "[0]" is enough to shut up the compiler, I'd say do that. The "IplBlockFcp fcp" part seems to be completely unused, so I think you could even remove that IplBlockFcp struct. For IPLBlockPV I agree with Paolo, it's likely easiest to use [0] for that struct. Thomas
On Tue, 27 Oct 2020 12:26:21 +0100 Thomas Huth <thuth@redhat.com> wrote: > On 26/10/2020 16.12, Paolo Bonzini wrote: > > On 26/10/20 16:03, Daniele Buono wrote: > >> Hi Paolo, > >> I reorganized UASStatus to put uas_iu at the end and it works fine. > >> Unfortunately, this uncovered another part of the code with a similar > >> issue (variable sized type not at the end of the struct), here: > >> > >> In file included from ../qemu-cfi-v3/target/s390x/diag.c:21: > >> ../qemu-cfi-v3/hw/s390x/ipl.h:161:23: error: field 'iplb' with variable > >> sized type 'IplParameterBlock' (aka 'union IplParameterBlock') not at > >> the end of a struct or class is a GNU extension > >> [-Werror,-Wgnu-variable-sized-type-not-at-end] > >> IplParameterBlock iplb; > >> ^ > >> ../qemu-cfi-v3/hw/s390x/ipl.h:162:23: error: field 'iplb_pv' with > >> variable sized type 'IplParameterBlock' (aka 'union IplParameterBlock') > >> not at the end of a struct or class is a GNU extension > >> [-Werror,-Wgnu-variable-sized-type-not-at-end] > >> IplParameterBlock iplb_pv; > >> > >> My understanding is that each of these IplParameterBlock may contain > >> either a IPLBlockPV or a IplBlockFcp, which both end with a variable > >> sized field (an array). > >> > >> Adding maintainers of s390x to see if they have a suggested solution to > >> avoid disabling the warning. > > > > This one seems okay because the union constrains the size to 4K. If > > "[0]" is enough to shut up the compiler, I'd say do that. > > The "IplBlockFcp fcp" part seems to be completely unused, so I think you > could even remove that IplBlockFcp struct. For IPLBlockPV I agree with > Paolo, it's likely easiest to use [0] for that struct. The fcp block had probably been added for completeness' sake, but we do not support list-directed IPL anyway. Not sure if we actually want it, as we use a different mechanism for IPLing from SCSI devices. So yes, maybe we should just drop it.
So what I would do for the warning in IplParameterBlock is the following (if I got the comments right): - Remove IplBlockFcp from IplParameterBlock - Keep IPLBlockPV and, in its declaration, use struct IPLBlockPVComp components[0]; Now for the IplBlockFcp struct declaration, it does not seem to be used anywhere now. I could either keep it as it was before (with the variable-size array) or remove it entirely. I guess this is more a question for the maintainers, what is your preference here? Daniele On 10/27/2020 7:38 AM, Cornelia Huck wrote: > On Tue, 27 Oct 2020 12:26:21 +0100 > Thomas Huth <thuth@redhat.com> wrote: > >> On 26/10/2020 16.12, Paolo Bonzini wrote: >>> On 26/10/20 16:03, Daniele Buono wrote: >>>> Hi Paolo, >>>> I reorganized UASStatus to put uas_iu at the end and it works fine. >>>> Unfortunately, this uncovered another part of the code with a similar >>>> issue (variable sized type not at the end of the struct), here: >>>> >>>> In file included from ../qemu-cfi-v3/target/s390x/diag.c:21: >>>> ../qemu-cfi-v3/hw/s390x/ipl.h:161:23: error: field 'iplb' with variable >>>> sized type 'IplParameterBlock' (aka 'union IplParameterBlock') not at >>>> the end of a struct or class is a GNU extension >>>> [-Werror,-Wgnu-variable-sized-type-not-at-end] >>>> IplParameterBlock iplb; >>>> ^ >>>> ../qemu-cfi-v3/hw/s390x/ipl.h:162:23: error: field 'iplb_pv' with >>>> variable sized type 'IplParameterBlock' (aka 'union IplParameterBlock') >>>> not at the end of a struct or class is a GNU extension >>>> [-Werror,-Wgnu-variable-sized-type-not-at-end] >>>> IplParameterBlock iplb_pv; >>>> >>>> My understanding is that each of these IplParameterBlock may contain >>>> either a IPLBlockPV or a IplBlockFcp, which both end with a variable >>>> sized field (an array). >>>> >>>> Adding maintainers of s390x to see if they have a suggested solution to >>>> avoid disabling the warning. >>> >>> This one seems okay because the union constrains the size to 4K. If >>> "[0]" is enough to shut up the compiler, I'd say do that. >> >> The "IplBlockFcp fcp" part seems to be completely unused, so I think you >> could even remove that IplBlockFcp struct. For IPLBlockPV I agree with >> Paolo, it's likely easiest to use [0] for that struct. > > The fcp block had probably been added for completeness' sake, but we do > not support list-directed IPL anyway. Not sure if we actually want it, > as we use a different mechanism for IPLing from SCSI devices. So yes, > maybe we should just drop it. > >
diff --git a/configure b/configure index e6754c1e87..9dc05cfb8a 100755 --- a/configure +++ b/configure @@ -2000,6 +2000,8 @@ add_to nowarn_flags -Wno-shift-negative-value add_to nowarn_flags -Wno-string-plus-int add_to nowarn_flags -Wno-typedef-redefinition add_to nowarn_flags -Wno-tautological-type-limit-compare +add_to nowarn_flags -Wno-gnu-variable-sized-type-not-at-end +add_to nowarn_flags -Wno-void-pointer-to-enum-cast add_to nowarn_flags -Wno-psabi gcc_flags="$warn_flags $nowarn_flags"
Clang 11 finds a couple of spots in the code that trigger new warnings: ../qemu-base/hw/usb/dev-uas.c:157:31: error: field 'status' with variable sized type 'uas_iu' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] uas_iu status; ^ 1 error generated. The data structure is UASStatus, which must end with a QTAILQ_ENTRY, so I believe we cannot have uas_iu at the end. Since this is a gnu extension but CLANG supports it, just add -Wno-gnu-variable-sized-type-not-at-end to remove the warning. ../qemu-base/target/s390x/cpu_models.c:985:21: error: cast to smaller integer type 'S390Feat' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast] S390Feat feat = (S390Feat) opaque; ^~~~~~~~~~~~~~~~~ ../qemu-base/target/s390x/cpu_models.c:1002:21: error: cast to smaller integer type 'S390Feat' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast] S390Feat feat = (S390Feat) opaque; ^~~~~~~~~~~~~~~~~ ../qemu-base/target/s390x/cpu_models.c:1036:27: error: cast to smaller integer type 'S390FeatGroup' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast] S390FeatGroup group = (S390FeatGroup) opaque; ^~~~~~~~~~~~~~~~~~~~~~ ../qemu-base/target/s390x/cpu_models.c:1057:27: error: cast to smaller integer type 'S390FeatGroup' from 'void *' [-Werror,-Wvoid-pointer-to-enum-cast] S390FeatGroup group = (S390FeatGroup) opaque; ^~~~~~~~~~~~~~~~~~~~~~ 4 errors generated. These are void * that get casted to enums, which are (or can be) smaller than a 64bit pointer. A code reorg may be better on the long term, but for now will fix this adding -Wno-void-pointer-to-enum-cast Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com> --- configure | 2 ++ 1 file changed, 2 insertions(+)