Hello Michael & Jakub, On 8/25/20 12:29 PM, Michael Kerrisk (man-pages) wrote: > I would really have preferred three patches here, since: I can do that. > >> - Never use a space after ``sizeof``, and always use parentheses >> instead. >> >> Rationale: >> >> https://www.kernel.org/doc/html/v5.8/process/coding-style.html#spaces > > (1) This is completely unproblematic from my point of view. Actually there was only one appearance of that (and another one that used a space before the parentheses). It's unproblematic, but it's so minor that it can be fixed easily. >> - Use the name of the variable instead of the type as argument >> for ``sizeof``, wherever possible. >> >> Rationale: >> >> https://www.kernel.org/doc/html/v5.8/process/coding-style.html#allocating-memory > > (2) This one is up for debate. In many cases it makes sense to do > this. However, there are cases where I think that using the struct > name can actually help readability. And when I grep through the kernel > source, of around 139k lines that use "sizeof", some 37k take a 'struct type' > as an argument. SI, I think this kind of change may need to be considered on > a case by case basis, rather than as a blanket change. Ok. I can send a set of patches with a patch for each page. > >> - When the result of ``sizeof`` is multiplied (or otherwise modified), >> write ``sizeof`` in the first place. >> >> Rationale: >> >> ``(sizeof(x) * INT_MAX * 2)`` doesn't overflow. >> >> ``(INT_MAX * 2 * sizeof(x))`` overflows, giving incorrect >> results. > > (3) Is this true? "gcc -Wall" does not complain about this. And, I > thought that in both cases, all operands in the expression > would be promoted to the largest type. And, on my x86-64 system, > > sizeof((sizeof(x) * INT_MAX * 2)) == 8 > sizeof(INT_MAX * 2 * sizeof(x)) == 8 > > But, I will say tht I'm not a language lawyer, and C still > sometimes has surprises for me. At the least, I'd like to know > more about this point. Well, when I said the first one doesn't overflow, I meant it's much less likely. In C, successive multiplications are evaluated left to right (*), and therefore here is what happens: ``(sizeof(x) * INT_MAX * 2)``: 1) sizeof(x) * INT_MAX (the type is the largest of both, which is size_t (unsigned long; uint64_t)). 2) ANS * 2 (the type is again the largest: size_t) ``(INT_MAX * 2 * sizeof(x))``: 1) INT_MAX * 2 (the type is the largest of both, which is int as both are int (int; int32_t), so the result is already truncated as it doesn't fit an int; at this point, the intermediate result will be 2^32 - 2 (``INT_MAX - 1``) (if I did the math right)). 2) ANS * 2 (the type is again the largest of both: size_t; however, ANS was already incorrect, so the result will be an incorrect size_t value) > sizeof((sizeof(x) * INT_MAX * 2)) == 8 Here you were overflowing a uint64_t (if x were a char, it would not overflow, and the result would be close to UINT64_MAX). > sizeof(INT_MAX * 2 * sizeof(x)) == 8 Here you were overflowing int32_t, and it would overflow regardless of sizeof(x). I wrote an extreme case, but we can agree that 32 bit is much easier to overflow than 64. (*): https://en.cppreference.com/w/c/language/operator_precedence On 8/25/20 1:19 PM, Jakub Wilk wrote: > * Michael Kerrisk <mtk.manpages@xxxxxxxxx>, 2020-08-25, 12:29: >>> ``(sizeof(x) * INT_MAX * 2)`` doesn't overflow. >>> >>> ``(INT_MAX * 2 * sizeof(x))`` overflows, giving incorrect >>> results. >> >> (3) Is this true? "gcc -Wall" does not complain about this. > > My GCC (10.2.0) does, even without -Wall: > > $ gcc test-overflow.c > test-overflow.c: In function 'main': > test-overflow.c:8:52: warning: integer overflow in expression of type > 'int' results in '-2' [-Woverflow] > 8 | printf("INT_MAX * 2 * sizeof(x) = %zu\n", INT_MAX * 2 * > sizeof(x)); > | ^ I guess it will only complain for the first one, doesn't it? > >> sizeof((sizeof(x) * INT_MAX * 2)) == 8 >> sizeof(INT_MAX * 2 * sizeof(x)) == 8 > > Hmm? If there was no overflow, surely you should get a number larger > than INT_MAX... > INT_MAX is INT32_MAX, and given that size_t is 64 bit, sure we can, but only in the first case.
