| Message ID | 1441305296.9666.14.camel@perches.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 3 Sep 2015, Joe Perches wrote: > On Thu, 2015-09-03 at 11:22 -0700, Linus Torvalds wrote: > > On Thu, Sep 3, 2015 at 10:40 AM, David Miller <davem@davemloft.net> wrote: > > > > > > Linus, what GCC version are you using and what does the warning look > > > like? > > > > I'm on whatever is in F22. gcc -v says > > > > gcc version 5.1.1 20150618 (Red Hat 5.1.1-4) (GCC) > > > > and the warning looks like so: > > > > net/mac80211/rate.c: In function ‘rate_control_cap_mask’: > > net/mac80211/rate.c:719:25: warning: ‘sizeof’ on array function > > parameter ‘mcs_mask’ will return size of ‘u8 * {aka unsigned char *}’ > > [-Wsizeof-array-argument] > > for (i = 0; i < sizeof(mcs_mask); i++) > > ^ > > > > (note the lack of warning about the use of an array in the function > > definition parameter list - I tried to find if there's any way to > > enable such a warning, but couldn't find anything. Maybe my google-fu > > is weak, but more probably that just doesn't exist). I find 518 occurrences of a function parameter declaration that contains an explicit size. But only the sizeof(mcs_mask) where there is a sizeof on such a parameter. I also checked for ARRAY_SIZE on such parameters, and didn't find any occurrences of that either. julia > Coccinelle might be a better tool for this but > a possible checkpatch patch is below: > > It produces output like: > > $ ./scripts/checkpatch.pl -f net/iucv/iucv.c --types=sized_array_argument > WARNING: Avoid sized array arguments > #716: FILE: net/iucv/iucv.c:716: > +static int iucv_sever_pathid(u16 pathid, u8 userdata[16]) > +{ > > WARNING: Avoid sized array arguments > #878: FILE: net/iucv/iucv.c:878: > +int iucv_path_accept(struct iucv_path *path, struct iucv_handler *handler, > + u8 userdata[16], void *private) > +{ > > WARNING: Avoid sized array arguments > #925: FILE: net/iucv/iucv.c:925: > +int iucv_path_connect(struct iucv_path *path, struct iucv_handler *handler, > + u8 userid[8], u8 system[8], u8 userdata[16], > + void *private) > +{ > > WARNING: Avoid sized array arguments > #988: FILE: net/iucv/iucv.c:988: > +int iucv_path_quiesce(struct iucv_path *path, u8 userdata[16]) > +{ > > WARNING: Avoid sized array arguments > #1020: FILE: net/iucv/iucv.c:1020: > +int iucv_path_resume(struct iucv_path *path, u8 userdata[16]) > +{ > > WARNING: Avoid sized array arguments > #1050: FILE: net/iucv/iucv.c:1050: > +int iucv_path_sever(struct iucv_path *path, u8 userdata[16]) > +{ > > total: 0 errors, 6 warnings, 0 checks, 2119 lines checked > --- > scripts/checkpatch.pl | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index e14dcdb..747b164 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -5422,6 +5422,24 @@ sub process { > "externs should be avoided in .c files\n" . $herecurr); > } > > +# check for function arguments using arg[SIZE] > + if ($^V && $^V ge 5.10.0 && > + defined $stat && > + $stat =~ /^.\s*(?:$Declare|$DeclareMisordered)\s*$Ident\s*($balanced_parens)\s*\{/s) { > + my $func_args = $1; > + if ($func_args =~ /(.*)\[\s*(?:$Constant|[A-Z0-9_]+)\s*\]/ && (!defined($1) || $1 !~ /\[\s*\]\s*$/)) { > + my $ctx = ''; > + my $herectx = $here . "\n"; > + my $cnt = statement_rawlines($stat); > + for (my $n = 0; $n < $cnt; $n++) { > + $herectx .= raw_line($linenr, $n) . "\n"; > + $n = $cnt if ($herectx =~ /{/); > + } > + WARN("SIZED_ARRAY_ARGUMENT", > + "Avoid sized array arguments\n" . $herectx); > + } > + } > + > # checks for new __setup's > if ($rawline =~ /\b__setup\("([^"]*)"/) { > my $name = $1; > > >
On Thu, Sep 3, 2015 at 12:32 PM, Julia Lawall <julia.lawall@lip6.fr> wrote: > > I find 518 occurrences of a function parameter declaration that contains > an explicit size. But only the sizeof(mcs_mask) where there is a sizeof > on such a parameter. I also checked for ARRAY_SIZE on such parameters, > and didn't find any occurrences of that either. Are there any cases of multi-dimensional arrays? Because those actually have semantic meaning outside of sizeof(), just in things like adding offsets. Eg something like int fn(int a[][10]) ends up being equivalent to something like int fn(int (*a)[10]) and "a+1" is actually 40 bytes ahead of "a", so it does *not* act like an "int *". (And I might have screwed that up mightily - C multidimensional arrays and the conversions to pointers are really easy to get confused about. Which is why I hope we don't have them) Linus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 3 Sep 2015, Linus Torvalds wrote: > On Thu, Sep 3, 2015 at 12:32 PM, Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > I find 518 occurrences of a function parameter declaration that contains > > an explicit size. But only the sizeof(mcs_mask) where there is a sizeof > > on such a parameter. I also checked for ARRAY_SIZE on such parameters, > > and didn't find any occurrences of that either. > > Are there any cases of multi-dimensional arrays? Because those > actually have semantic meaning outside of sizeof(), just in things > like adding offsets. > > Eg something like > > int fn(int a[][10]) > > ends up being equivalent to something like > > int fn(int (*a)[10]) > > and "a+1" is actually 40 bytes ahead of "a", so it does *not* act like > an "int *". > > (And I might have screwed that up mightily - C multidimensional arrays > and the conversions to pointers are really easy to get confused about. > Which is why I hope we don't have them) There are 32 2-dimensional arrays in function parameters, and 1 3-dimensional array. No 4-dimensional arrays. I didn't check past that. None of these has a sizeof or ARRAY_SIZE. The three dimensional array is here: drivers/media/dvb-frontends/stv0367.c static int stv0367ter_filt_coeff_init(struct stv0367_state *state, u16 CellsCoeffs[3][6][5], u32 DemodXtal) It is used as follows: stv0367_writereg(state, (R367TER_IIRCX_COEFF1_MSB + 2 * (j - 1)), MSB(CellsCoeffs[k][i-1][j-1])); stv0367_writereg(state, (R367TER_IIRCX_COEFF1_LSB + 2 * (j - 1)), LSB(CellsCoeffs[k][i-1][j-1])); The value of this parameter is one of three locally defined static global arrays. julia -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 3, 2015 at 1:55 PM, Julia Lawall <julia.lawall@lip6.fr> wrote: > > There are 32 2-dimensional arrays in function parameters, and 1 > 3-dimensional array. No 4-dimensional arrays. I didn't check past that. > None of these has a sizeof or ARRAY_SIZE. > > The three dimensional array is here: drivers/media/dvb-frontends/stv0367.c Ok. That actually looks like a valid use of the C function argument array passing semantics. It's rather much simpler than exposing the pointers. So I guess we don't really end up wanting to disallow this, and the new gcc array sizeof warning is good enough. Thanks for running the analysis so that I didn't have to look at it ;) Linus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 3 Sep 2015, Linus Torvalds wrote: > On Thu, Sep 3, 2015 at 1:55 PM, Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > There are 32 2-dimensional arrays in function parameters, and 1 > > 3-dimensional array. No 4-dimensional arrays. I didn't check past that. > > None of these has a sizeof or ARRAY_SIZE. > > > > The three dimensional array is here: drivers/media/dvb-frontends/stv0367.c > > Ok. That actually looks like a valid use of the C function argument > array passing semantics. It's rather much simpler than exposing the > pointers. > > So I guess we don't really end up wanting to disallow this, and the > new gcc array sizeof warning is good enough. > > Thanks for running the analysis so that I didn't have to look at it ;) The double arrays also look OK - the uses are also explicit double array references. julia -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index e14dcdb..747b164 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5422,6 +5422,24 @@ sub process { "externs should be avoided in .c files\n" . $herecurr); } +# check for function arguments using arg[SIZE] + if ($^V && $^V ge 5.10.0 && + defined $stat && + $stat =~ /^.\s*(?:$Declare|$DeclareMisordered)\s*$Ident\s*($balanced_parens)\s*\{/s) { + my $func_args = $1; + if ($func_args =~ /(.*)\[\s*(?:$Constant|[A-Z0-9_]+)\s*\]/ && (!defined($1) || $1 !~ /\[\s*\]\s*$/)) { + my $ctx = ''; + my $herectx = $here . "\n"; + my $cnt = statement_rawlines($stat); + for (my $n = 0; $n < $cnt; $n++) { + $herectx .= raw_line($linenr, $n) . "\n"; + $n = $cnt if ($herectx =~ /{/); + } + WARN("SIZED_ARRAY_ARGUMENT", + "Avoid sized array arguments\n" . $herectx); + } + } + # checks for new __setup's if ($rawline =~ /\b__setup\("([^"]*)"/) { my $name = $1;