Hello Alex, On Tue, 25 Aug 2020 at 13:34, Alejandro Colomar <colomar.6.4.3@xxxxxxxxx> wrote: > > 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. Okay. Don't do them all at once, since we may change strategy while discussing the first few patches. > >> - 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). Thanks for the lesson in C basics. I clearly did need a refresher :-}. You and Jakub are of course correct. If you send the patches in the order (as I numbered in my previous reply): (1) (3) (2) as multiple pieces that would be best, since the first two patches should obviously be applied, and then we can discuss the last patch(es) case by case. Thanks, Michael
