Bug 265181
| Summary: | Clang doesn't properly report the C++ error when float is used as an argument in bit-wise operators (only on armv6 some error is reported instead of reporting it on all architectures) | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Yuri Victorovich <yuri> |
| Component: | misc | Assignee: | freebsd-toolchain (Nobody) <toolchain> |
| Status: | Closed Not A Bug | ||
| Severity: | Affects Only Me | CC: | dim, marklmi26-fbsd |
| Priority: | --- | ||
| Version: | 13.0-RELEASE | ||
| Hardware: | Any | ||
| OS: | Any | ||
|
Description
Yuri Victorovich
2022-07-13 02:06:51 UTC
(In reply to Yuri Victorovich from comment #0) The port does a patch that involves: @@ -983,7 +991,7 @@ char *Alsa_pcmi::play_floatre (const float *src, char while (nfrm--) { d = *src; - *((float *) dst) = __bswap_32 (d); + *((float *) dst) = bswap_32 (d); dst += _play_step; src += step; } @@ -1105,7 +1113,7 @@ const char *Alsa_pcmi::capt_floatre (const char *src, while (nfrm--) { d = *((float *) src); - *dst = __bswap_32 (d); + *dst = bswap_32 (d); dst += step; src += _capt_step; } I'll use Alsa_pcmi::play_floatre as the example to carry my points. The complaint is about (after expansion but with some text replaced by "MORE_NOT_SHOWN"): *((float *) dst) = ((((d) & 0xff000000U) >> 24) | MORE_NOT_SHOWN via: zita-alsa-pcmi.cc:994:28: error: invalid operands to binary expression ('float' and 'unsigned int') *((float *) dst) = bswap_32 (d); ^~~~~~~~~~~~ /usr/include/infiniband/byteswap.h:39:25: note: expanded from macro 'bswap_32' #define bswap_32 bswap32 ^ /usr/include/sys/endian.h:62:20: note: expanded from macro 'bswap32' #define bswap32(x) __bswap32(x) ^~~~~~~~~~~~ /usr/include/machine/endian.h:134:6: note: expanded from macro '__bswap32' __bswap32_constant(x) : \ ^~~~~~~~~~~~~~~~~~~~~ /usr/include/machine/endian.h:118:12: note: expanded from macro '__bswap32_constant' ((((x) & 0xff000000U) >> 24) | \ ~~~ ^ ~~~~~~~~~~~ But the original unpatched code is (note the type of d): char *Alsa_pcmi::play_floatre (const float *src, char *dst, int nfrm, int step) { float d; while (nfrm--) { d = *src; *((float *) dst) = __bswap_32 (d); dst += _play_step; src += step; } return dst; } So the end result after patching and expanding looks like: char *Alsa_pcmi::play_floatre (const float *src, char *dst, int nfrm, int step) { float d; while (nfrm--) { d = *src; *((float *) dst) = ((((d) & 0xff000000U) >> 24) | MORE_NOT_SHOWN; dst += _play_step; src += step; } return dst; } So d is if type float. So (d) is of type float. So (d) & 0xff000000U has & taking a mix of float on the left and unsigned int on the right. This is what the complaint is about. This is not an example of integer converting to float. For & the language rules are: QUOTE The operands shall be of integral or unscoped enumeration type. The usual arithmetic conversions (7.4) are performed. END QUOTE The rule in 7.4 relative to float is: QUOTE (after scoped enumeration, long double, and double for "either operand"): Otherwise, if either operand is float, the other shall be converted to float. END QUOTE Thus (d) & 0xff000000U turns into: (d) & static_cast<float>(0xff000000U) And then neither operand is an "integral or unscoped enumeration type". Thus the reported error. Looks like a correct message to me. (In reply to Yuri Victorovich from comment #0) I'll also note that the one line summary does not report the error message that the compiler actually produced: QUOTE error: invalid operands to binary expression ('float' and 'unsigned int') END QUOTE The existing one line summary is highly misleading compared to the actual error message listed in the description. (In reply to Mark Millard from comment #1) Why doesn't it fail on other architectures? I think this should either succeed everywhere or fail everywhere. Clang treats it differently on different architectures for some reason. The code is obviously incorrect. (In reply to Mark Millard from comment #1) Since I referenced "7.4" material explicitly, I should have reported that I used: INCITS/ISO/IEC 14882:2020 (2021) as my language reference. I do not expect that any language properties that I used are specific to anything that new for the C++ vintage. For example, ISO/IEC 14882:2011 also deals with "if either operand is float" for "the usual arithmetic conversions" before involving integral promotions and the rules that apply to the results of the integral promotions. Thus still float & float results. The & still requires integral or unscoped enumeration operands. (In reply to Yuri Victorovich from comment #4) clang is not what supplied bswap_32 : /usr/include/infiniband/byteswap.h provides: #define bswap_32 bswap32 (I switch to /usr/src/ style paths starting here.) /usr/13_0R-src/sys/sys/endian.h provides: #define bswap32(x) __bswap32(x) You are welcome to go explore the range architecture specific definitions in: (The space then tab sequence in the []'s below likely will not make it through the copy/paste sequence.) # grep -r "define[ ]*\<__bswap32\>" /usr/13_0R-src/sys/ | more /usr/13_0R-src/sys/arm64/include/endian.h:#define __bswap32(x) \ /usr/13_0R-src/sys/arm64/include/endian.h:#define __bswap32(x) __bswap32_var(x) /usr/13_0R-src/sys/arm/include/endian.h:#define __bswap32(x) \ /usr/13_0R-src/sys/arm/include/endian.h:#define __bswap32(x) __bswap32_var(x) /usr/13_0R-src/sys/x86/include/endian.h:#define __bswap32(x) __builtin_bswap32(x) /usr/13_0R-src/sys/riscv/include/endian.h:#define __bswap32(x) \ /usr/13_0R-src/sys/riscv/include/endian.h:#define __bswap32(x) __bswap32_var(x) /usr/13_0R-src/sys/mips/include/endian.h:#define __bswap32(x) ((__uint32_t)(__is_constant((x)) ? \ /usr/13_0R-src/sys/powerpc/include/endian.h:#define __bswap32(x) (__is_constant(x) ? __bswap32_const(x) : \ But even with just that much of a grep, note the x86 using: __builtin_bswap32(x) which is not going to have any binary operators involved at all. Such is not clang's problem for why that is different. I'll note that FreeBSD's arm/include/endian.h definitions are far from uniform for type handling: static __inline __uint32_t __bswap32_var(__uint32_t v) { __uint32_t t1; __asm __volatile("eor %1, %0, %0, ror #16\n" "bic %1, %1, #0x00ff0000\n" "mov %0, %0, ror #8\n" "eor %0, %0, %1, lsr #8\n" : "+r" (v), "=r" (t1)); return (v); } . . . #ifdef __OPTIMIZE__ #define __bswap32_constant(x) \ ((((x) & 0xff000000U) >> 24) | \ (((x) & 0x00ff0000U) >> 8) | \ (((x) & 0x0000ff00U) << 8) | \ (((x) & 0x000000ffU) << 24)) . . . #define __bswap32(x) \ ((__uint32_t)(__builtin_constant_p(x) ? \ __bswap32_constant(x) : \ __bswap32_var(x))) #else #define __bswap16(x) __bswap16_var(x) #define __bswap32(x) __bswap32_var(x) #endif /* __OPTIMIZE__ */ __bswap32_var will not produce such binary operator related messages. But, for __OPTIMIZE__, __bswap32_constant usage will. So the __bswap32(x) usage will vary based on constant expressions vs. non-constant expressions. Again: Not clang++'s problem since it is not clang's set of definitions. clang++ is doing what it is told to by the language standard and code it was supplied. Mark, Thanks for your analysis. Yuri Ah, the bswap32, bswap_32, etc fiasco. There's three or four different standards. And I'm trying to iron it all out. Everywhere, though, __bswap32, etc, should be defined as __builtin_bswap32, etc. Anything else will be suboptimal. I started to sort all this out in https://reviews.freebsd.org/D31964 (where I start to use __ versions everywhere) https://reviews.freebsd.org/D31962 (this is almost ready to go in, but there's a couple of stragglers in ports that I've not had the time to fix) The problem is that endian.h, sys/endian.h and byteswap.h are different APIs that vary a bit, that also has a number of different autoconf and autoconf-like scripts that have various different bugs that get it wrong, or detect things to include in one way, and detect which macros to use in a different way and this mismatch causes problems. |