[Clang][Sema] Enabled implicit conversion warning for CompoundAssignment operator.

[fahadnayyar]

Summary

This change enables implicit conversion warnings (like Wshorten-64-to-32) for compound assignment operator with integral operands.

rdar://10466193

[fahadnayyar]

Fixing clang-format error.

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

14300

Moving this function to another spot in the file makes comparing the new and old code pretty hard -- would it make more sense to forward declare the needed static functions so this can stay where it was and it's easier to see what's changed?

[fahadnayyar]

Added forward declare for CheckImplicitConversion to make the diff look clean.

[fahadnayyar]

clang/lib/Sema/SemaChecking.cpp

14300

@aaron.ballman I've made the required changes. Please have a look, thanks!

[aaron.ballman]

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.

clang/lib/Sema/SemaChecking.cpp

13534–13535

This comment isn't quite accurate, right? It's checking for any kind of implicit conversion issue (such as changing signs even if the integer widths stay the same).

clang/test/Sema/conversion-64-32.c

22

Can you also add tests for the shift assign behavior, since that's being handled in a special way?

[fahadnayyar]

Fixed libcxx failure and added test case for expected warning for shift assigsn operator.

[fahadnayyar]

In D139114#4023456, @aaron.ballman wrote:

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.

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.

clang/lib/Sema/SemaChecking.cpp

13534–13535

Changed the comment!

[fahadnayyar]

Added summary in clang release notes.

[arichardson]

clang/lib/Sema/SemaChecking.cpp

13536

Why is this restricted to integers? It looks like CheckImplicitConversion also handles other types? Does calling it unconditionally cause any issues?

[fahadnayyar]

clang/lib/Sema/SemaChecking.cpp

13536

@arichardson since AnalyzeCompoundAssignment already does some custom checks for certain data types like floats, I added the change only for integers as I am trying to fix the behaviour only for integral operands in this patch. Otherwise we see more than one (required) warning for floats operands and also for shift assign operator.

This patch targets only integral changes like test 3-5 in conversion-64-32.c

[ldionne]

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.

[fahadnayyar]

Rebasing on main.

[MaskRay]

libunwind/src/AddressSpace.hpp

249 ↗

(On Diff #487955)

Thanks. LGTM for this file. You can just fix libunwind in a separate commit so that this patch focuses on its main work. You can then remove #libunwind from the reviewer list.

[MaskRay]

clang/docs/ReleaseNotes.rst

354

Use double backquotes for -Wsign-conversion, -Wshorten-64-to-32 (and missing a -), += and >=

[fahadnayyar]

Moving the libunwind change to a separate patch and some refactoring.

[fahadnayyar]

Moved the libunwind change to this separate patch: https://reviews.llvm.org/D141515

[aaron.ballman]

Assuming pre-commit CI is happy, this LGTM aside from a few minor things.

clang/lib/Sema/SemaChecking.cpp

13516–13517

We should probably update this comment as well.

13535–13536

Adding a comment so we don't lose the question.

[fahadnayyar]

Refactoring.

[vitalybuka]

https://lab.llvm.org/buildbot/#/builders/37/builds/19499

[chapuni]

Excuse me, I have reverted this.

Before landing this, could you please fix new warnings in the current codebase, please?

[MaskRay]

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.

[nathanchance]

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.

[aaron.ballman]

In D139114#4053022, @chapuni wrote:

Excuse me, I have reverted this.

Before landing this, could you please fix new warnings in the current codebase, please?

Thank you for the revert (and good to see you again!).

In D139114#4053166, @nathanchance wrote:

For what it’s worth, this triggers a LOT in the Linux kernel for the pattern that @MaskRay pointed out:

Out of curiosity, has it found any true positives that you can tell?

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.

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).

[aaron.ballman]

Marking as changes requested so it's clear this isn't ready to re-land

[nathanchance]

In D139114#4053166, @nathanchance wrote:

For what it’s worth, this triggers a LOT in the Linux kernel for the pattern that @MaskRay pointed out:

Out of curiosity, has it found any true positives that you can tell?

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;

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.

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).

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.

[nathanchance]

I’ll have to filter the warnings to see if there are any other instances with other operators that appear problematic.

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 |
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[fahadnayyar]

@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.

[aaron.ballman]

In D139114#4059603, @fahadnayyar wrote:

@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.

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.

clang/docs/ReleaseNotes.rst

 - Fix an issue that makes Clang crash on lambda template parameters. This fixes
   `Issue 57960 <https://github.com/llvm/llvm-project/issues/57960>`_
 - Fix issue that the standard C++ modules importer will call global
   constructor/destructor for the global varaibles in the importing modules.
   This fixes `Issue 59765 <https://github.com/llvm/llvm-project/issues/59765>`_
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+- Clang now supports implicit conversion warnings (``-Wsign-conversion``,
+  ``Wshorten-64-to-32``, etc) for compound assignment operators (like +=, -=, <<=, >>= etc) 
+  with integral operands.
 - Clang will now check compile-time determinable string literals as format strings.
   Fixes `Issue 55805: <https://github.com/llvm/llvm-project/issues/55805>`_.
 - ``-Wformat`` now recognizes ``%b`` for the ``printf``/``scanf`` family of
   functions and ``%B`` for the ``printf`` family of functions. Fixes
   `Issue 56885: <https://github.com/llvm/llvm-project/issues/56885>`_.
 - Introduced ``-Wsingle-bit-bitfield-constant-conversion``, grouped under
   ``-Wbitfield-constant-conversion``, which diagnoses implicit truncation when
   ``1`` is assigned to a 1-bit signed integer bitfield. This fixes

clang/lib/Sema/SemaChecking.cpp

S.DiagRuntimeBehavior(E->getExprLoc(), E,
                               << E->getSourceRange() << SourceRange(CContext));
   } else {
     S.Diag(E->getExprLoc(), DiagID)
         << E->getType() << T.getUnqualifiedType() << PrettySourceValue
         << PrettyTargetValue << E->getSourceRange() << SourceRange(CContext);
   }
 }
 
+static void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
+                                    SourceLocation CC,
+                                    bool *ICContext = nullptr,
+                                    bool IsListInit = false);
+
 /// Analyze the given compound assignment for the possible losing of
-/// floating-point precision.
+/// floating-point precision and for implicit conversions for integral operands.
 static void AnalyzeCompoundAssignment(Sema &S, BinaryOperator *E) {
   assert(isa<CompoundAssignOperator>(E) &&
          "Must be compound assignment operation");
   // Recurse on the LHS and RHS in here
   AnalyzeImplicitConversions(S, E->getLHS(), E->getOperatorLoc());
   AnalyzeImplicitConversions(S, E->getRHS(), E->getOperatorLoc());
 
   if (E->getLHS()->getType()->isAtomicType())
     S.Diag(E->getOperatorLoc(), diag::warn_atomic_implicit_seq_cst);
 
   // Now check the outermost expression
   const auto *ResultBT = E->getLHS()->getType()->getAs<BuiltinType>();
   const auto *RBT = cast<CompoundAssignOperator>(E)
                         ->getComputationResultType()
                         ->getAs<BuiltinType>();
 
+  // Check for implicit conversions for compound assignment statements with
+  // intergral operands.
+  // FIXME: should this handle floating-point as well?
+  if (E->getLHS()->getType()->isIntegerType() &&
+      E->getRHS()->getType()->isIntegerType() && !E->isShiftAssignOp())
+    CheckImplicitConversion(S, E->getRHS(), E->getType(),
+                            E->getRHS()->getExprLoc());
+
   // The below checks assume source is floating point.
-  if (!ResultBT || !RBT || !RBT->isFloatingPoint()) return;
+  if (!ResultBT || !RBT || !RBT->isFloatingPoint())
+    return;
 
   // If source is floating point but target is an integer.
   if (ResultBT->isInteger())
     return DiagnoseImpCast(S, E, E->getRHS()->getType(), E->getLHS()->getType(),
                            E->getExprLoc(), diag::warn_impcast_float_integer);
 
   if (!ResultBT->isFloatingPoint())
     return;
if ((LHS->getValue() == 0 || LHS->getValue() == 1) &&
       // Do not diagnose common idioms.
       return;
     if (LHS->getValue() != 0 && RHS->getValue() != 0)
       S.Diag(ExprLoc, diag::warn_integer_constants_in_conditional_always_true);
   }
 }
 
 static void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
-                                    SourceLocation CC,
-                                    bool *ICContext = nullptr,
-                                    bool IsListInit = false) {
+                                    SourceLocation CC, bool *ICContext,
+                                    bool IsListInit) {
   if (E->isTypeDependent() || E->isValueDependent()) return;
 
   const Type *Source = S.Context.getCanonicalType(E->getType()).getTypePtr();
   const Type *Target = S.Context.getCanonicalType(T).getTypePtr();
   if (Source == Target) return;
   if (Target->isDependentType()) return;
 
   // If the conversion context location is invalid don't complain. We also
if (const EnumType *TargetEnum = Target->getAs<EnumType>())
         if (S.SourceMgr.isInSystemMacro(CC))
           return;
 
         return DiagnoseImpCast(S, E, SourceType, T, CC,
                                diag::warn_impcast_different_enum_types);
       }
 }
 
 static void CheckConditionalOperator(Sema &S, AbstractConditionalOperator *E,
                                      SourceLocation CC, QualType T);
 
 static void CheckConditionalOperand(Sema &S, Expr *E, QualType T,
                                     SourceLocation CC, bool &ICContext) {
   E = E->IgnoreParenImpCasts();
 
   if (auto *CO = dyn_cast<AbstractConditionalOperator>(E))
     return CheckConditionalOperator(S, CO, CC, T);

clang/test/Sema/conversion-64-32.c

 int4 test1(long2 a) {
   int4  v127 = a;  // no warning.
   return v127; 
 }
 
 int test2(long v) {
   return v / 2; // expected-warning {{implicit conversion loses integer precision: 'long' to 'int'}}
 }
+
+// rdar://10466193
+void test3(int i, long long ll) {
+  i += ll; // expected-warning {{implicit conversion loses integer precision}}
+  i -= ll; // expected-warning {{implicit conversion loses integer precision}}
+  i *= ll; // expected-warning {{implicit conversion loses integer precision}}
+  i /= ll; // expected-warning {{implicit conversion loses integer precision}}
+}
+
+void test4(int i, long long ll) {
+  i += i-ll; // expected-warning {{implicit conversion loses integer precision}}
+  i += i+ll; // expected-warning {{implicit conversion loses integer precision}}
+  i -= i-ll; // expected-warning {{implicit conversion loses integer precision}}
+  i -= i+ll; // expected-warning {{implicit conversion loses integer precision}}
+}
+
+void test5(int i, int j, long long ll) {
+  i += (i-j)*ll; // expected-warning {{implicit conversion loses integer precision}}
+  i += (i+j)*ll; // expected-warning {{implicit conversion loses integer precision}}
+  i -= ll/(i-j); // expected-warning {{implicit conversion loses integer precision}}
+  i -= ll/(i-j); // expected-warning {{implicit conversion loses integer precision}}
+}
+
+void test6(char c) {
+  c <<= 999999; // expected-warning {{shift count >= width of type}} \
+                  // cxx17-warning {{shift count >= width of type}} \
+                  // ref-warning {{shift count >= width of type}} \
+                  // ref-cxx17-warning {{shift count >= width of type}}
+  c >>= 999999; // expected-warning {{shift count >= width of type}} \
+                  // cxx17-warning {{shift count >= width of type}} \
+                  // ref-warning {{shift count >= width of type}} \
+                  // ref-cxx17-warning {{shift count >= width of type}}
+}