| Message ID | 20200430213101.135134-14-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | gcc-10 warning fixes | expand |
On 30/04/2020 22:30, Arnd Bergmann wrote: > Two files access the zero-length resp_data[] array, which now > causes a compiler warning: > > drivers/scsi/aic94xx/aic94xx_tmf.c: In function 'asd_get_tmf_resp_tasklet': > drivers/scsi/aic94xx/aic94xx_tmf.c:291:22: warning: array subscript 3 is outside the bounds of an interior zero-length array 'u8[0]' {aka 'unsigned char[0]'} [-Wzero-length-bounds] > 291 | res = ru->resp_data[3]; > | ~~~~~~~~~~~~~^~~ > In file included from include/scsi/libsas.h:15, > from drivers/scsi/aic94xx/aic94xx.h:16, > from drivers/scsi/aic94xx/aic94xx_tmf.c:11: > include/scsi/sas.h:557:9: note: while referencing 'resp_data' > 557 | u8 resp_data[0]; > | ^~~~~~~~~ > drivers/scsi/libsas/sas_task.c: In function 'sas_ssp_task_response': > drivers/scsi/libsas/sas_task.c:21:30: warning: array subscript 3 is outside the bounds of an interior zero-length array 'u8[0]' {aka 'unsigned char[0]'} [-Wzero-length-bounds] > 21 | tstat->stat = iu->resp_data[3]; > | ~~~~~~~~~~~~~^~~ > In file included from include/scsi/scsi_transport_sas.h:8, > from drivers/scsi/libsas/sas_internal.h:14, > from drivers/scsi/libsas/sas_task.c:3: > include/scsi/sas.h:557:9: note: while referencing 'resp_data' > 557 | u8 resp_data[0]; > | ^~~~~~~~~ > > This should really be a flexible-array member, but the structure > already has such a member, swapping it out with sense_data[] would > cause many more warnings elsewhere. > Hi Arnd, If we really prefer flexible-array members over zero-length array members, then could we have a union of flexible-array members? I'm not sure if that's a good idea TBH (or even permitted), as these structures are defined by the SAS spec and good practice to keep as consistent as possible, but just wondering. Apart from that: Reviewed-by: John Garry <john.garry@huawei.com> > As a workaround, add a temporary pointer that can be accessed without > a warning. > > Fixes: 2908d778ab3e ("[SCSI] aic94xx: new driver") > Fixes: 366ca51f30de ("[SCSI] libsas: abstract STP task status into a function") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/scsi/aic94xx/aic94xx_tmf.c | 4 +++- > drivers/scsi/libsas/sas_task.c | 3 ++- > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/aic94xx/aic94xx_tmf.c b/drivers/scsi/aic94xx/aic94xx_tmf.c > index f814026f26fa..a3139f9766c8 100644 > --- a/drivers/scsi/aic94xx/aic94xx_tmf.c > +++ b/drivers/scsi/aic94xx/aic94xx_tmf.c > @@ -269,6 +269,7 @@ static int asd_get_tmf_resp_tasklet(struct asd_ascb *ascb, > struct ssp_frame_hdr *fh; > struct ssp_response_iu *ru; > int res = TMF_RESP_FUNC_FAILED; > + u8 *resp; > > ASD_DPRINTK("tmf resp tasklet\n"); > > @@ -287,8 +288,9 @@ static int asd_get_tmf_resp_tasklet(struct asd_ascb *ascb, > fh = edb->vaddr + 16; > ru = edb->vaddr + 16 + sizeof(*fh); > res = ru->status; > + resp = ru->resp_data; > if (ru->datapres == 1) /* Response data present */ > - res = ru->resp_data[3]; > + res = resp[3]; > #if 0 > ascb->tag = fh->tag; > #endif > diff --git a/drivers/scsi/libsas/sas_task.c b/drivers/scsi/libsas/sas_task.c > index e2d42593ce52..4cd2f9611c4a 100644 > --- a/drivers/scsi/libsas/sas_task.c > +++ b/drivers/scsi/libsas/sas_task.c > @@ -12,13 +12,14 @@ void sas_ssp_task_response(struct device *dev, struct sas_task *task, > struct ssp_response_iu *iu) > { > struct task_status_struct *tstat = &task->task_status; > + u8 *resp = iu->resp_data; > > tstat->resp = SAS_TASK_COMPLETE; > > if (iu->datapres == 0) > tstat->stat = iu->status; > else if (iu->datapres == 1) > - tstat->stat = iu->resp_data[3]; > + tstat->stat = resp[3]; > else if (iu->datapres == 2) { > tstat->stat = SAM_STAT_CHECK_CONDITION; > tstat->buf_valid_size = >
On Fri, May 1, 2020 at 9:48 AM John Garry <john.garry@huawei.com> wrote: > On 30/04/2020 22:30, Arnd Bergmann wrote: > > This should really be a flexible-array member, but the structure > > already has such a member, swapping it out with sense_data[] would > > cause many more warnings elsewhere. > > > > > Hi Arnd, > > If we really prefer flexible-array members over zero-length array > members, then could we have a union of flexible-array members? I'm not > sure if that's a good idea TBH (or even permitted), as these structures > are defined by the SAS spec and good practice to keep as consistent as > possible, but just wondering. gcc does not allow flexible-array members inside of a union, or more than one flexible-array member at the end of a structure. I found one hack that would work, but I think it's too ugly and likely not well-defined either: struct ssp_response_iu { ... struct { u8 dummy[0]; /* a struct must have at least one non-flexible member */ u8 resp_data[]; /* allowed here because it's at the one of a struct */ }; u8 sense_data[]; } __attribute__ ((packed)); > Apart from that: > > Reviewed-by: John Garry <john.garry@huawei.com> Thanks! Arnd
On Fri, 2020-05-01 at 09:54 +0200, Arnd Bergmann wrote: > On Fri, May 1, 2020 at 9:48 AM John Garry <john.garry@huawei.com> > wrote: > > On 30/04/2020 22:30, Arnd Bergmann wrote: > > > This should really be a flexible-array member, but the structure > > > already has such a member, swapping it out with sense_data[] > > > would cause many more warnings elsewhere. > > > > > > > > > Hi Arnd, > > > > If we really prefer flexible-array members over zero-length array > > members, then could we have a union of flexible-array members? I'm > > not sure if that's a good idea TBH (or even permitted), as these > > structures are defined by the SAS spec and good practice to keep as > > consistent as possible, but just wondering. > > gcc does not allow flexible-array members inside of a union, or more > than one flexible-array member at the end of a structure. > > I found one hack that would work, but I think it's too ugly and > likely not well-defined either: > > struct ssp_response_iu { > ... > struct { > u8 dummy[0]; /* a struct must have at least one > non-flexible member */ If gcc is now warning about zero length members, why isn't it warning about this one ... are unions temporarily excluded? > u8 resp_data[]; /* allowed here because it's at > the one of a struct */ > }; > u8 sense_data[]; > } __attribute__ ((packed)); Let's go back to what the standard says: we want the data beyond the ssp_response_iu to be addressable either as sense_data if it's an error return or resp_data if it's a real response. What about trying to use an alias attribute inside the structure ... will that work on gcc-10? James
On Fri, May 1, 2020 at 4:53 PM James Bottomley <jejb@linux.ibm.com> wrote: > On Fri, 2020-05-01 at 09:54 +0200, Arnd Bergmann wrote: > > On Fri, May 1, 2020 at 9:48 AM John Garry <john.garry@huawei.com> > > wrote: > > I found one hack that would work, but I think it's too ugly and > > likely not well-defined either: > > > > struct ssp_response_iu { > > ... > > struct { > > u8 dummy[0]; /* a struct must have at least one > > non-flexible member */ > > If gcc is now warning about zero length members, why isn't it warning > about this one ... are unions temporarily excluded? It does not warn about all zero-length arrays, but it does warn when you try to access an array with an out-of-range index, and this apparently got extended in gcc-10 to any index for zero-length arrays. > > u8 resp_data[]; /* allowed here because it's at > > the one of a struct */ > > }; > > u8 sense_data[]; > > } __attribute__ ((packed)); > > Let's go back to what the standard says: we want the data beyond the > ssp_response_iu to be addressable either as sense_data if it's an error > return or resp_data if it's a real response. What about trying to use > an alias attribute inside the structure ... will that work on gcc-10? I think alias attributes only work for functions and variables, but not for struct members. A "#define sense_data resp_data" would obviously work, but it's rather error-prone when other code uses the same identifiers. Another option would be an inline helper like static inline u8 *ssp_response_data(struct ssp_response_iu *iu) { return iu.resp_data; } Arnd
diff --git a/drivers/scsi/aic94xx/aic94xx_tmf.c b/drivers/scsi/aic94xx/aic94xx_tmf.c index f814026f26fa..a3139f9766c8 100644 --- a/drivers/scsi/aic94xx/aic94xx_tmf.c +++ b/drivers/scsi/aic94xx/aic94xx_tmf.c @@ -269,6 +269,7 @@ static int asd_get_tmf_resp_tasklet(struct asd_ascb *ascb, struct ssp_frame_hdr *fh; struct ssp_response_iu *ru; int res = TMF_RESP_FUNC_FAILED; + u8 *resp; ASD_DPRINTK("tmf resp tasklet\n"); @@ -287,8 +288,9 @@ static int asd_get_tmf_resp_tasklet(struct asd_ascb *ascb, fh = edb->vaddr + 16; ru = edb->vaddr + 16 + sizeof(*fh); res = ru->status; + resp = ru->resp_data; if (ru->datapres == 1) /* Response data present */ - res = ru->resp_data[3]; + res = resp[3]; #if 0 ascb->tag = fh->tag; #endif diff --git a/drivers/scsi/libsas/sas_task.c b/drivers/scsi/libsas/sas_task.c index e2d42593ce52..4cd2f9611c4a 100644 --- a/drivers/scsi/libsas/sas_task.c +++ b/drivers/scsi/libsas/sas_task.c @@ -12,13 +12,14 @@ void sas_ssp_task_response(struct device *dev, struct sas_task *task, struct ssp_response_iu *iu) { struct task_status_struct *tstat = &task->task_status; + u8 *resp = iu->resp_data; tstat->resp = SAS_TASK_COMPLETE; if (iu->datapres == 0) tstat->stat = iu->status; else if (iu->datapres == 1) - tstat->stat = iu->resp_data[3]; + tstat->stat = resp[3]; else if (iu->datapres == 2) { tstat->stat = SAM_STAT_CHECK_CONDITION; tstat->buf_valid_size =
Two files access the zero-length resp_data[] array, which now causes a compiler warning: drivers/scsi/aic94xx/aic94xx_tmf.c: In function 'asd_get_tmf_resp_tasklet': drivers/scsi/aic94xx/aic94xx_tmf.c:291:22: warning: array subscript 3 is outside the bounds of an interior zero-length array 'u8[0]' {aka 'unsigned char[0]'} [-Wzero-length-bounds] 291 | res = ru->resp_data[3]; | ~~~~~~~~~~~~~^~~ In file included from include/scsi/libsas.h:15, from drivers/scsi/aic94xx/aic94xx.h:16, from drivers/scsi/aic94xx/aic94xx_tmf.c:11: include/scsi/sas.h:557:9: note: while referencing 'resp_data' 557 | u8 resp_data[0]; | ^~~~~~~~~ drivers/scsi/libsas/sas_task.c: In function 'sas_ssp_task_response': drivers/scsi/libsas/sas_task.c:21:30: warning: array subscript 3 is outside the bounds of an interior zero-length array 'u8[0]' {aka 'unsigned char[0]'} [-Wzero-length-bounds] 21 | tstat->stat = iu->resp_data[3]; | ~~~~~~~~~~~~~^~~ In file included from include/scsi/scsi_transport_sas.h:8, from drivers/scsi/libsas/sas_internal.h:14, from drivers/scsi/libsas/sas_task.c:3: include/scsi/sas.h:557:9: note: while referencing 'resp_data' 557 | u8 resp_data[0]; | ^~~~~~~~~ This should really be a flexible-array member, but the structure already has such a member, swapping it out with sense_data[] would cause many more warnings elsewhere. As a workaround, add a temporary pointer that can be accessed without a warning. Fixes: 2908d778ab3e ("[SCSI] aic94xx: new driver") Fixes: 366ca51f30de ("[SCSI] libsas: abstract STP task status into a function") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/scsi/aic94xx/aic94xx_tmf.c | 4 +++- drivers/scsi/libsas/sas_task.c | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-)