This change enables implicit conversion warnings (like Wshorten-64-to-32) for compound assignment operator with integral operands.
rdar://10466193
Differential D139114
[Clang][Sema] Enabled implicit conversion warning for CompoundAssignment operator. fahadnayyar on Dec 1 2022, 7:02 AM. Authored by
Details This change enables implicit conversion warnings (like Wshorten-64-to-32) for compound assignment operator with integral operands. rdar://10466193
Diff Detail
Event Timeline
Comment Actions It looks like this change breaks libc++ (see the precommit CI failures) by making more changes than expected. I'm now seeing implicit conversion changes signedness diagnostics where we didn't previously get them. Is that expected and intentional? (I think it may be a fix: https://godbolt.org/z/hTaaf8c5P so I'm adding the libc++ folks just in case they disagree.) Also, these changes should come with an entry in the release notes.
Comment Actions Fixed libcxx failure and added test case for expected warning for shift assigsn operator. Comment Actions I've made some change to suppress the warning in libcxx, please have a look and let me know if the change can break the semantics of the function in any way. Since gcc also give warning on this example, I guess we should include this behaviour in clang as well.
Comment Actions The code in libunwind does look fishy to me (before the change), so I think flagging this in the compiler is good. I don't know that code at all, though, so I'm pinging a few libunwind regulars so they can let us know what they think. FWIW it looks reasonable to me. @rprichard @MaskRay You probably also want to rebase your patch on top of main -- the failures you are seeing right now seem to be spurious issues. @aaron.ballman When libunwind is changed, the CI now only triggers the usual libc++ pipeline even though Clang was changed too. I'll look into fixing that. @fahadnayyar that's unrelated to your patch, just for information.
Comment Actions Assuming pre-commit CI is happy, this LGTM aside from a few minor things.
Comment Actions Excuse me, I have reverted this. Before landing this, could you please fix new warnings in the current codebase, please? Comment Actions This will cause warnings for the common usage related to bitmask macros/enumerators. #include <elf.h> int main(void) { unsigned char other = 0x81; other &= ~STO_AARCH64_VARIANT_PCS; return other; } a.c:5:12: warning: implicit conversion from 'int' to 'unsigned char' changes value from -129 to 127 [-Wconstant-conversion] other &= ~STO_AARCH64_VARIANT_PCS; (enumerator) llvm/tools/llvm-readobj/ELFDumper.cpp:3869:18: warning: implicit conversion from 'int' to 'uint8_t' (aka 'unsigned char') changes value from -129 to 127 [-Wconstant-conversion] This diagnostic may be noisy for some projects. Comment Actions For what it’s worth, this triggers a LOT in the Linux kernel for the pattern that @MaskRay pointed out: https://gist.github.com/nathanchance/a76c2ba0a22dda3251c4ec5ee9e3f285 $ rg -c -- -Wconstant-conversion $TMP_FOLDER/4c37671a7c946ac246b52fa44a6a649b89d6310b-build.log 28364 The breakdown shows that one site in a common header is responsible for 19,352 of those instances, but there are a lot of one off instances, which will take a long time to fix: https://gist.github.com/nathanchance/85338a1e0693590cc93dba1d3e0bb050 It would be nice if this was split into a separate flag that could be disabled, as -Wconstant-conversion has not been very noisy up until this point. Comment Actions Thank you for the revert (and good to see you again!). Out of curiosity, has it found any true positives that you can tell?
We can definitely split it into a separate flag. Alternatively, we could investigate disabling the warning for that code pattern (or moving that bit under its own flag). Comment Actions
I have not been able to look too closely since I am on mobile until Wednesday but of the instances I examined, they all appear to be false positives (or maybe true positives that we really just don’t care about) . The Linux kernel has a macro BIT(x), which is just 1UL << x, so any negation is going to be a really large number but expected to truncate when assigning to a narrower width variable because the bit being cleared might only be in the first few positions. That is at least the case with the really noisy instance of the warning that I mentioned above #define FWNODE_FLAG_LINKS_ADDED BIT(0) #define FWNODE_FLAG_NOT_DEVICE BIT(1) #define FWNODE_FLAG_INITIALIZED BIT(2) #define FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD BIT(3) #define FWNODE_FLAG_BEST_EFFORT BIT(4) struct fwnode_handle { struct fwnode_handle *secondary; const struct fwnode_operations *ops; struct device *dev; struct list_head suppliers; struct list_head consumers; u8 flags; }; … if (initialized) fwnode->flags |= FWNODE_FLAG_INITIALIZED; else fwnode->flags &= ~FWNODE_FLAG_INITIALIZED;
Right, I would suspect that emitting this for &= will rarely show anything interesting. I’ll have to filter the warnings to see if there are any other instances with other operators that appear problematic. I’ll see how easy that is or if there is a simple way to omit those instances that can be added to the patch itself for testing. Comment Actions
I count a single warning that triggers for an arithmetic operator (which might be a bug): ../drivers/net/wireless/ralink/rt2x00/rt2800lib.c:10067:14: warning: implicit conversion from 'int' to 's8' (aka 'signed char') changes value from 128 to -128 [-Wconstant-conversion] cal_val -= 128; ^~~ and a few that trigger for |= (but I think they are all false positives?): ../drivers/usb/gadget/udc/bdc/bdc_core.c:62:17: warning: implicit conversion from 'unsigned long' to 'u32' (aka 'unsigned int') changes value from 18446744071830503424 to 2415919104 [-Wconstant-conversion] temp |= BDC_COS|BDC_COP_STP; ~~~~~~~^~~~~~~~~~~~ ../drivers/net/wireless/realtek/rtlwifi/rtl8192ee/led.c:62:13: warning: implicit conversion from 'unsigned long' to 'u32' (aka 'unsigned int') changes value from 18446744073707454463 to 4292870143 [-Wconstant-conversion] ledcfg |= ~BIT(21); ^~~~~~~~ ../drivers/net/ethernet/intel/igb/igb_main.c:1834:42: warning: implicit conversion from 'unsigned long' to 'u32' (aka 'unsigned int') changes value from 18446744073709486592 to 4294902272 [-Wconstant-conversion] tqavctrl |= E1000_TQAVCTRL_DATATRANTIM | ~~~~~~~~~~~~~~~~~~~~~~~~~~~^ ../drivers/iio/adc/imx7d_adc.c:243:10: warning: implicit conversion from 'unsigned long' to 'u32' (aka 'unsigned int') changes value from 18446744073172680704 to 3758096384 [-Wconstant-conversion] cfg1 |= (IMX7D_REG_ADC_CH_CFG1_CHANNEL_EN | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Comment Actions @aaron.ballman do you think that we should call `CheckImplicitConversion``` only for arithemetic compound assignment operators like +=, -=, /=, *= and %= ? For bitwiseAssign operators (|=, &=, ^=) and shiftAssign operators (<<= and >>=) we may have to check the semantics to understand what implicit conversions are allowed and what are not allowed and which of these we should include with `-Wconversion``` flag. Comment Actions Based on what @nathanchance was seeing in practice, I think it might make sense to only enable this for arithmetic compound assignments for the moment. Also, I think it makes sense to land the modified version of this after the Clang 16 branch point so that we have more time for this functionality to bake before we unleash it on the world at large. |
Use double backquotes for -Wsign-conversion, -Wshorten-64-to-32 (and missing a -), += and >=