-Wunsequenced warnings after r359367 in Clang

[nathanchance]

r359367: llvm/llvm-project@31cfb31

Sample build log from arm multi_v7_defconfig: https://gist.github.com/24b1935db63d338891cfbd590febbe93

However, it happens with every single build I test: https://gist.github.com/6c3f376ad59190a9e0aaa87481052571

This is currently present in our CI as well, causing PowerPC failures because of -Werror: https://travis-ci.com/ClangBuiltLinux/continuous-integration/jobs/196132202

Bisect log:

git bisect start
# good: [4f71049a39dc7ebbb957dc701f77cd6191b20e3c] [ORC] Remove symbols from dependency lists when failing materialization.
git bisect good 4f71049a39dc7ebbb957dc701f77cd6191b20e3c
# bad: [03c4e2663ce4c3431afd075ae72e2357a7d085c4] Revert rL359389: [X86][SSE] Add support for <64 x i1> bool reduction
git bisect bad 03c4e2663ce4c3431afd075ae72e2357a7d085c4
# good: [60f5df948b0dd49349baf4326cebfb0b5e6bacf2] [asan_symbolize] Teach `asan_symbolize.py` to symbolicate using a module map
git bisect good 60f5df948b0dd49349baf4326cebfb0b5e6bacf2
# good: [2b29b432d206ee706538b8dad54806d7649fb378] [ScriptInterpreter] Move ownership into debugger (NFC)
git bisect good 2b29b432d206ee706538b8dad54806d7649fb378
# bad: [0f21545a3c9e57b7df4c68aaff92cb83ae7ce5d2] [sanitizer] Calculate SizeClassAllocator32::ByteMap type from Params::kSpaceSize and Params::kRegionSizeLog
git bisect bad 0f21545a3c9e57b7df4c68aaff92cb83ae7ce5d2
# good: [c1008e4d3dabfe62120549740decfe6637c2ae55] [NFC][Sanitizer] Remove GetRealFunctionAddress and replace usages
git bisect good c1008e4d3dabfe62120549740decfe6637c2ae55
# bad: [31cfb311c5cbd94963d21a76be8b7c3bec1419cc] Reinstate r359059, reverted in r359361, with a fix to properly prevent us emitting the operand of __builtin_constant_p if it has side-effects.
git bisect bad 31cfb311c5cbd94963d21a76be8b7c3bec1419cc
# good: [83c6d10b62a9575c19dfb0977ef675aeb996319a] [sanitizer] NFC: add static_assert to confirm that we use reasonable ByteMap type
git bisect good 83c6d10b62a9575c19dfb0977ef675aeb996319a
# good: [ae01981d03e51173cbfd328b62b22988f1eeb751] [AArch64] Initialize HasMTE
git bisect good ae01981d03e51173cbfd328b62b22988f1eeb751
# first bad commit: [31cfb311c5cbd94963d21a76be8b7c3bec1419cc] Reinstate r359059, reverted in r359361, with a fix to properly prevent us emitting the operand of __builtin_constant_p if it has side-effects.

[nickdesaulniers]

Looks like

include/linux/of.h:238:37: error: multiple unsequenced modifications to 'cell' [-Werror,-Wunsequenced]

is recurring between arch's. We should figure out if these warnings are correct about ambiguous sequence points (the pre & post increment/decrement operators are famous for that) or if Clang is somehow messed up now due to that commit (not obvious how).

The warning alone doesn't show enough info. We'll have to look at the kernel sources. If the kernel sources aren't obviously wrong, than maybe we can provide @zygoloid a creduced test case.

[nickdesaulniers]

• include/linux/of.h#L238
• include/uapi/linux/swab.h#L18

so be32_to_cpu will evaluate its argument 4 times, so that postincrement to cell happens 4 times...

It was probably exposed by the clang commit that improved compat with __builtin_constant_p, which was likely generating additional runtime code.

-	while (size--)
-		r = (r << 32) | be32_to_cpu(*(cell++));
+	while (size--) {
+		r = (r << 32) | be32_to_cpu(*(cell));
+              ++cell;
+      }

[nickdesaulniers]

reminds me of @kees ' comment.

[nickdesaulniers]

cc @gwelymernans

[nickdesaulniers]

yeah, I'm pretty sure clang is not wrong here. ran the preprocessed source through clang-format:

typedef long u64;
typedef unsigned int __u32;
typedef unsigned int __be32;

extern u64 __fswab32(__u32);

u64 of_read_number(const __be32 *cell, int size) {
  u64 r = 0;
  while (size--)
    r = (r << 32) |
        (__builtin_constant_p((__u32)((__u32)(__be32)(*(cell++))))
             ? ((__u32)(
                   (((__u32)((__u32)(__be32)(*(cell++))) & (__u32)0x000000ffUL)
                    << 24) |
                   (((__u32)((__u32)(__be32)(*(cell++))) & (__u32)0x0000ff00UL)
                    << 8) |
                   (((__u32)((__u32)(__be32)(*(cell++))) &
                     (__u32)0x00ff0000UL) >>
                    8) |
                   (((__u32)((__u32)(__be32)(*(cell++))) &
                     (__u32)0xff000000UL) >>
                    24)))
             : __fswab32((__u32)(__be32)(*(cell++))));
  return r;
}

[nickdesaulniers]

yeah, I'm pretty sure clang is not wrong here. ran the preprocessed source through clang-format:

typedef long u64;
typedef unsigned int __u32;
typedef unsigned int __be32;

static inline __attribute__((__always_inline__))
__attribute__((__gnu_inline__)) __attribute__((__unused__))
__attribute__((__no_instrument_function__))
u64 of_read_number(const __be32 *cell, int size)
{                                                                               
  u64 r = 0;                                                                     
  while (size--)                                                                 
    r = (r << 32) | (__builtin_constant_p((__u32)(( __u32)(__be32)(*(cell++)))) ? (      (__u32)( (((__u32)(( __u32)(__be32)(*(cell++))) & (__u32)0x000000ffUL) << 24) | (      ((__u32)(( __u32)(__be32)(*(cell++))) & (__u32)0x0000ff00UL) << 8) | (((__u32)((       __u32)(__be32)(*(cell++))) & (__u32)0x00ff0000UL) >> 8) | (((__u32)(( __u32)(__be32)(*(cell++))) & (__u32)0xff000000UL) >> 24))) : __fswab32(( __u32)(__be32)(*(cell++))));
  return r;                                                                      
} 

[nathanchance]

I assume that GCC's preprocessed source looks the same. I'll see if I can double back to the other warnings tomorrow or later this week because they are quite numerous...

[nathanchance]

reminds me of @kees ' comment.

Just looked into this more. This appears to be the same thing here.

Relevant snippets:

include/linux/of.h:

/* Helper to read a big number; size is in cells (not bytes) */
static inline u64 of_read_number(const __be32 *cell, int size)
{
	u64 r = 0;
	while (size--)
		r = (r << 32) | be32_to_cpu(*(cell++));
	return r;
}

include/uapi/linux/byteorder/little_endian.h::

#define __be32_to_cpu(x) __swab32((__force __u32)(__be32)(x))

include/uapi/linux/swab.h:

static inline __attribute_const__ __u32 __fswab32(__u32 val)
{
#if defined(__arch_swab32)
	return __arch_swab32(val);
#else
	return ___constant_swab32(val);
#endif
}
...
/**
 * __swab32 - return a byteswapped 32-bit value
 * @x: value to byteswap
 */
#ifdef __HAVE_BUILTIN_BSWAP32__
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
#else
#define __swab32(x)				\
	(__builtin_constant_p((__u32)(x)) ?	\
	___constant_swab32(x) :			\
	__fswab32(x))
#endif

cell is not a compile time constant so we should be using __fswab32 and avoiding all of this (__arch_swab32 is defined for every arch we care about except for arm64...?).

Although, to take it further, we could be avoiding all of this if __HAVE_BUILTIN_BSWAP__ was defined for clang. Right now, it's only defined in include/linux/compiler-gcc.h but clang appears to have it in clang/include/clang/Basic/Builtins.def.

I'd love to creduce something but I wonder if the interestingness test should be the warning or something different.

[nathanchance]

@phongt sent a patch to eliminate one of the most prevalent warnings: https://lore.kernel.org/lkml/20190430090044.16345-1-tranmanphong@gmail.com/

[nickdesaulniers]

v2: https://lore.kernel.org/r/20190430145624.30470-1-tranmanphong@gmail.com/

[phongt]

https://gist.github.com/nathanchance/6c3f376ad59190a9e0aaa87481052571

more patches need to send.

patch for power supply core was sent https://lore.kernel.org/lkml/20190502012704.12505-1-tranmanphong@gmail.com/

[zygoloid]

Code guarded by if (__builtin_constant_p(x)) will only ever be reachable in practice if x has no side-effects (because that's the only case where x can be folded to a compile-time constant). As a result, these -Wunsequenced warnings are false-positives, because they are firing in dead code. I've made the constant evaluation feeding into this warning smarter in r359844, which should suppress the false-positive warnings.

[nathanchance]

@zygoloid I can confirm that I do not see these warnings anymore with r359844, thanks for following up on this!