On Mon, Sep 13, 2021 at 9:53 PM 'Nick Desaulniers' via Clang Built Linux clang-built-linux@googlegroups.com wrote:

On Mon, Sep 13, 2021 at 11:39 AM Nick Desaulniers ndesaulniers@google.com wrote:

...

On Mon, Sep 13, 2021 at 10:58 AM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:

...

On Mon, Sep 13, 2021 at 09:52:33PM +0530, Naresh Kamboju wrote:

...

[PATCH 00/10] raise minimum GCC version to 5.1 https://lore.kernel.org/lkml/20210910234047.1019925-1-ndesaulniers@google.co...

Has anyone submitted a fix for this upstream yet? I can't seem to find one :(

That lore link has a series to address this, though that's maybe something we don't want to backport to stable.

I thought about this all weekend; I think I might be able to work around the one concern I had with my other approach, using __builtin_choose_expr().

There's an issue with my alternative approach (https://gist.github.com/nickdesaulniers/2479818f4983bbf2d688cebbab435863) with declaring the local variable z in div_64() since either operand could be 64b, which result in an unwanted truncation if the dividend is 32b (or less, and divisor is 64b). I think (what I realized this weekend) is that we might be able to replace the `if` with `__builtin_choose_expr`, then have that whole expression be the final statement and thus the "return value" of the statement expression.

Christ...that...works? Though, did Linus just merge my patches for gcc 5.1?

"Merge branch 'gcc-min-version-5.1' (make gcc-5.1 the minimum version)"

- Sedat -

https://git.kernel.org/linus/316346243be6df12799c0b64b788e06bad97c30b

Anyways, I'll send something like this for stable:

diff --git a/include/linux/math64.h b/include/linux/math64.h index 2928f03d6d46..e9ab8c25f8d3 100644 --- a/include/linux/math64.h +++ b/include/linux/math64.h @@ -11,6 +11,9 @@

#define div64_long(x, y) div64_s64((x), (y)) #define div64_ul(x, y) div64_u64((x), (y)) +#ifndef is_signed_type +#define is_signed_type(type) (((type)(-1)) < (type)1) +#endif

/**

• div_u64_rem - unsigned 64bit divide with 32bit divisor with remainder

@@ -112,6 +115,15 @@ extern s64 div64_s64(s64 dividend, s64 divisor);

#endif /* BITS_PER_LONG */

+#define div64_x64(dividend, divisor) ({ \

•   BUILD_BUG_ON_MSG(sizeof(dividend) < sizeof(u64),\
  

•                    "prefer div_x64");             \
  

•   __builtin_choose_expr(                          \
  

•           is_signed_type(typeof(dividend)),       \
  

•           div64_s64(dividend, divisor),           \
  

•           div64_u64(dividend, divisor));          \
  

+})

/**

• div_u64 - unsigned 64bit divide with 32bit divisor
• @dividend: unsigned 64bit dividend

@@ -142,6 +154,28 @@ static inline s64 div_s64(s64 dividend, s32 divisor) } #endif

+#define div_x64(dividend, divisor) ({ \

•   BUILD_BUG_ON_MSG(sizeof(dividend) > sizeof(u32),\
  

•                    "prefer div64_x64");           \
  

•   __builtin_choose_expr(                          \
  

•           is_signed_type(typeof(dividend)),       \
  

•           div_s64(dividend, divisor),             \
  

•           div_u64(dividend, divisor));            \
  

+})

+// TODO: what if divisor is 128b? +#define div_64(dividend, divisor) ({ \

•   __builtin_choose_expr(
     \
  

•           __builtin_types_compatible_p(typeof(dividend), s64) ||
     \
  

•           __builtin_types_compatible_p(typeof(dividend), u64),
     \
  

•           __builtin_choose_expr(
     \
  

•                   __builtin_types_compatible_p(typeof(divisor),
  

s64) || \

•                   __builtin_types_compatible_p(typeof(divisor),
  

u64), \

•                   div64_x64(dividend, divisor),
     \
  

•                   div_x64(dividend, divisor)),
     \
  

•           dividend / divisor);
     \
  

+})

u32 iter_div_u64_rem(u64 dividend, u32 divisor, u64 *remainder);

#ifndef mul_u32_u32

-- Thanks, ~Nick Desaulniers

-- You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/CAKwvOdmCS5Q7AzUL5nziYVU....

On Mon, Sep 13, 2021 at 2:13 PM Nick Desaulniers ndesaulniers@google.com wrote:

On Mon, Sep 13, 2021 at 1:50 PM Nick Desaulniers ndesaulniers@google.com wrote:

...

On Mon, Sep 13, 2021 at 1:42 PM Linus Torvalds torvalds@linux-foundation.org wrote:

...

On Mon, Sep 13, 2021 at 1:16 PM Nick Desaulniers ndesaulniers@google.com wrote:

...

Do we have access to _Generic in GCC 4.9?

We've ended up using it unconditionally since last year, so yes.

Sorry, grepping would have taken < 1s. I'm very lazy. http://threevirtues.com/

...

In fact, the compiler version tests got removed when we raised the gcc version requirement to 4.9 in commit 6ec4476ac825 ("Raise gcc version requirement to 4.9"):

"In particular, raising the minimum to 4.9 means that we can now just assume _Generic() exists, which is likely the much better replacement for a lot of very convoluted built-time magic with conditionals on sizeof and/or __builtin_choose_expr() with same_type() etc"

but we haven't used it much since.

The "seqprop" code for picking the right lock for seqlock is perhaps the main example, and staring at that code will make you go blind, so look away.

Looking at my patch: https://lore.kernel.org/stable/20210913203201.1844253-1-ndesaulniers@google.... I don't think _Generic helps us in the case of dispatching based on the result of is_signed_type() (the operands could undergo type promotion, so we'd need lots of cases that are more concisely covered by is_signed_type()). It could replace the nested checks in div_64 with nested _Generics, I think. Not sure it's a huge win for readability. Maybe cut the number of expansions of the parameters in half though. Let me give it a try just to see what it looks like.

Is this more readable? Same line count. I'm not sure if there's such a thing as "fallthrough" between the "cases" of _Generic to minimize duplication, or whether this could be factored further. Needs lots more () around macro param use and tab'ed out line endings...

Sorry wrong diff: diff --git a/include/linux/math64.h b/include/linux/math64.h index bc9c12c168d0..8fc4d56a45b9 100644 --- a/include/linux/math64.h +++ b/include/linux/math64.h @@ -162,18 +162,18 @@ static inline s64 div_s64(s64 dividend, s32 divisor) div_u64(dividend, divisor)); \ })

+#define __div_64(dividend, divisor) _Generic((divisor), \ + s64: div64_x64(dividend, divisor), \ + u64: div64_x64(dividend, divisor), \ + default: div_x64(dividend, divisor)) + #define div_64(dividend, divisor) ({ \ BUILD_BUG_ON_MSG(sizeof(dividend) > sizeof(u64), \ "128b div unsupported"); \ - __builtin_choose_expr( \ - __builtin_types_compatible_p(typeof(dividend), s64) || \ - __builtin_types_compatible_p(typeof(dividend), u64), \ - __builtin_choose_expr( \ - __builtin_types_compatible_p(typeof(divisor), s64) || \ - __builtin_types_compatible_p(typeof(divisor), u64), \ - div64_x64(dividend, divisor), \ - div_x64(dividend, divisor)), \ - dividend / divisor); \ + _Generic((dividend), \ + s64: __div_64(dividend, divisor), \ + u64: __div_64(dividend, divisor), \ + default: dividend / divisor); \ })

On Mon, Sep 13, 2021 at 7:13 PM libaokun (A) libaokun1@huawei.com wrote:

在 2021/9/14 7:23, Nick Desaulniers 写道:

...

On Mon, Sep 13, 2021 at 4:00 PM Linus Torvalds torvalds@linux-foundation.org wrote:

...

On Mon, Sep 13, 2021 at 2:15 PM Nick Desaulniers ndesaulniers@google.com wrote:

...

Sorry wrong diff:

Well, this second diff was seriously whitespace-damaged and hard to read, but while it seems to be the same number of lines, it sure looks a lot more readable in this format.

Except I think that

             default: dividend / divisor);

should really have parentheses around both of those macro arguments.

That's a preexisting problem, but it should be fixed while at it.

Ok, I'll send a revised v2 based on _Generic; Rasmus can help review when he's awake.

...

I'm also not sure why that (again, preexisting) BUILD_BUG_ON_MSG() only checks the size of the dividend, not the divisor. Very strange. But probably not worth worrying about.

I sent a not-yet-applied diff of my not-yet-applied diff. I was playing with this last week, and IIRC we had divisors that were less than 32b being promoted to int. But I'll test it some more.

How about deleting the check_mul_overflow in the __nbd_ioctl as follows?

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index 5170a630778d..f404e0540476 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -1393,7 +1393,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, unsigned int cmd, unsigned long arg) { struct nbd_config *config = nbd->config;

•   loff_t bytesize;
  
     switch (cmd) {
     case NBD_DISCONNECT:
  

@@ -1408,9 +1407,10 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, case NBD_SET_SIZE: return nbd_set_size(nbd, arg, config->blksize); case NBD_SET_SIZE_BLOCKS:

•           if (check_mul_overflow((loff_t)arg, config->blksize,
  

&bytesize))

•           if (arg && (LLONG_MAX / arg <= config->blksize))
                     return -EINVAL;
  

64b division is going to need do_div(), yeah? Besides, this is likely to pop up again for other callers of check_mul_overflow(), might as well fix it.

•           return nbd_set_size(nbd, bytesize, config->blksize);
  

•           return nbd_set_size(nbd, arg * config->blksize,
  

•                               config->blksize);
     case NBD_SET_TIMEOUT:
             nbd_set_cmd_timeout(nbd, arg);
             return 0;
  

-- 2.31.1

-- With Best Regards, Baokun Li