-Wsizeof-array-div in mm/hugetlb.c

-Wsizeof-array-div in mm/hugetlb.c

[Nathan Chancellor]

Hi all,

Clang recently added a new diagnostic in r371605, -Wsizeof-array-div,
that tries to warn when sizeof(X) / sizeof(Y) does not compute the
number of elements in an array X (i.e., sizeof(Y) is wrong). See that
commit for more details:

https://github.com/llvm/llvm-project/commit/3240ad4ced0d3223149b72a4fc2a4d9b67589427

There is a warning in mm/hugetlb.c in hugetlb_fault_mutex_hash:

mm/hugetlb.c:4055:40: warning: expression does not compute the number of
elements in this array; element type is 'unsigned long', not 'u32' (aka
'unsigned int') [-Wsizeof-array-div]
        hash = jhash2((u32 *)&key, sizeof(key)/sizeof(u32), 0);
                                          ~~~ ^
mm/hugetlb.c:4049:16: note: array 'key' declared here
        unsigned long key[2];
                      ^
1 warning generated.

Should this warning be silenced? What is the reasoning behind having key
be an array of unsigned longs but representing it as an array of u32s?
Would it be better to avoid the cast and have it just be an array of
u32s directly? I am not familiar with this code so I may be naive for
asking such questions but we'd like to get these warnings cleaned up so
that this warning can be useful down the road.

Cheers,
Nathan

Re: -Wsizeof-array-div in mm/hugetlb.c

[Mike Kravetz]

On 9/17/19 12:34 AM, Nathan Chancellor wrote:
> Hi all,
> 
> Clang recently added a new diagnostic in r371605, -Wsizeof-array-div,
> that tries to warn when sizeof(X) / sizeof(Y) does not compute the
> number of elements in an array X (i.e., sizeof(Y) is wrong). See that
> commit for more details:
> 
> https://github.com/llvm/llvm-project/commit/3240ad4ced0d3223149b72a4fc2a4d9b67589427
> 
> There is a warning in mm/hugetlb.c in hugetlb_fault_mutex_hash:
> 
> mm/hugetlb.c:4055:40: warning: expression does not compute the number of
> elements in this array; element type is 'unsigned long', not 'u32' (aka
> 'unsigned int') [-Wsizeof-array-div]
>         hash = jhash2((u32 *)&key, sizeof(key)/sizeof(u32), 0);
>                                           ~~~ ^
> mm/hugetlb.c:4049:16: note: array 'key' declared here
>         unsigned long key[2];
>                       ^
> 1 warning generated.
> 
> Should this warning be silenced? What is the reasoning behind having key
> be an array of unsigned longs but representing it as an array of u32s?

Well, the second argument to jhash2 is "the number of u32's in the key".
This is the reason for the sizeof(key)/sizeof(u32) calculation.  It certainly
is not trying to calculate the number of elements in the array as suggested by
the warning.

> Would it be better to avoid the cast and have it just be an array of
> u32s directly?

I did not write this code, but it is much easier to do the assignments (below)
to build the key if the array is unsigned long as opposed to u32.

struct address_space *mapping;
pgoff_t idx;
unsigned long key[2];

        key[0] = (unsigned long) mapping;
        key[1] = idx;

> u32s directly? I am not familiar with this code so I may be naive for
> asking such questions but we'd like to get these warnings cleaned up so
> that this warning can be useful down the road.

I suppose it would be possible to change 'key' to be something else besides
an array (such as struct or union) to eliminate the warning.  But, I would
prefer to have some type of directive to indicate the code is ok as is.  It
is not trying to calculate the number of elements in the array as suspected
by the clang diagnostic.

-- 
Mike Kravetz

Re: -Wsizeof-array-div in mm/hugetlb.c

[Dávid Bolvanský]

[-- Attachment #1: Type: text/plain, Size: 2507 bytes --]

You can use extra parens.

size(arr) / (size(int))

ut 17. 9. 2019 o 23:06 Mike Kravetz <mike.kravetz@oracle.com> napísal(a):

> On 9/17/19 12:34 AM, Nathan Chancellor wrote:
> > Hi all,
> >
> > Clang recently added a new diagnostic in r371605, -Wsizeof-array-div,
> > that tries to warn when sizeof(X) / sizeof(Y) does not compute the
> > number of elements in an array X (i.e., sizeof(Y) is wrong). See that
> > commit for more details:
> >
> >
> https://github.com/llvm/llvm-project/commit/3240ad4ced0d3223149b72a4fc2a4d9b67589427
> >
> > There is a warning in mm/hugetlb.c in hugetlb_fault_mutex_hash:
> >
> > mm/hugetlb.c:4055:40: warning: expression does not compute the number of
> > elements in this array; element type is 'unsigned long', not 'u32' (aka
> > 'unsigned int') [-Wsizeof-array-div]
> >         hash = jhash2((u32 *)&key, sizeof(key)/sizeof(u32), 0);
> >                                           ~~~ ^
> > mm/hugetlb.c:4049:16: note: array 'key' declared here
> >         unsigned long key[2];
> >                       ^
> > 1 warning generated.
> >
> > Should this warning be silenced? What is the reasoning behind having key
> > be an array of unsigned longs but representing it as an array of u32s?
>
> Well, the second argument to jhash2 is "the number of u32's in the key".
> This is the reason for the sizeof(key)/sizeof(u32) calculation.  It
> certainly
> is not trying to calculate the number of elements in the array as
> suggested by
> the warning.
>
> > Would it be better to avoid the cast and have it just be an array of
> > u32s directly?
>
> I did not write this code, but it is much easier to do the assignments
> (below)
> to build the key if the array is unsigned long as opposed to u32.
>
> struct address_space *mapping;
> pgoff_t idx;
> unsigned long key[2];
>
>         key[0] = (unsigned long) mapping;
>         key[1] = idx;
>
> > u32s directly? I am not familiar with this code so I may be naive for
> > asking such questions but we'd like to get these warnings cleaned up so
> > that this warning can be useful down the road.
>
> I suppose it would be possible to change 'key' to be something else besides
> an array (such as struct or union) to eliminate the warning.  But, I would
> prefer to have some type of directive to indicate the code is ok as is.  It
> is not trying to calculate the number of elements in the array as suspected
> by the clang diagnostic.
>
> --
> Mike Kravetz
>

[-- Attachment #2: Type: text/html, Size: 3251 bytes --]