llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Aditya Medhane (flash1729) <details> <summary>Changes</summary> memcmp is an unreliable equality test for any type without unique object representations: padding bytes are indeterminate ([EXP42-C](https://wiki.sei.cmu.edu/confluence/display/c/EXP42-C.+Do+not+compare+padding+data)) and floating-point encodings break the value-to-bytes mapping in both directions ([FLP37-C](https://wiki.sei.cmu.edu/confluence/display/c/FLP37-C.+Do+not+use+object+representations+to+compare+floating-point+values)). `CheckMemaccessArguments` already warns about memset and memcpy on problematic record types, but those checks are gated on the function kind, so memcmp falls through silently. clang-tidy has caught this since [D89651](https://reviews.llvm.org/D89651) (`bugprone-suspicious-memory-comparison`), but there's never been a compiler warning for it. Add `-Wsuspicious-memcmp`, on by default under `-Wsuspicious-memaccess` like its siblings. It warns when `memcmp` or `bcmp` (any builtin spelling) compares objects of a type without unique object representations and the constant size argument covers the whole object. The predicate (`ASTContext::hasUniqueObjectRepresentations`) and the trigger conditions are the same ones the tidy check uses, so prefix compares and non-constant sizes stay silent. Dynamic classes keep their existing, more specific warning. Casting an argument to `(void *)` silences it, and the fixit suggests exactly that. The check skips incomplete, dependent, function, and sizeless pointee types, and diagnoses templates per instantiation. --- Patch is 21.88 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/214218.diff 16 Files Affected: - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-memory-comparison.c (+1-3) - (modified) clang/docs/ReleaseNotes.md (+14) - (modified) clang/include/clang/Basic/DiagnosticGroups.td (+3-1) - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+5) - (modified) clang/lib/AST/ASTContext.cpp (+8-1) - (modified) clang/lib/Sema/SemaChecking.cpp (+22-2) - (added) clang/test/Sema/warn-suspicious-memcmp.c (+66) - (modified) clang/test/SemaCXX/type-traits.cpp (+10) - (added) clang/test/SemaCXX/warn-suspicious-memcmp.cpp (+62) - (modified) libcxx/include/__atomic/atomic_ref.h (+5-1) - (modified) libcxx/include/__atomic/atomic_sync.h (+5-1) - (modified) libcxx/include/__cxx03/__atomic/atomic_sync.h (+5-1) - (modified) libcxx/test/libcxx/atomics/atomics.types.generic/padding.pass.cpp (+7) - (modified) libcxx/test/libcxx/atomics/clear_padding.pass.cpp (+7) - (modified) libcxx/test/std/atomics/atomics.types.generic/padding.pass.cpp (+7) - (modified) libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp (+7) ``````````diff diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-memory-comparison.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-memory-comparison.c index d3ecffec9a781..1f8bd7bd9f1ad 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-memory-comparison.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/suspicious-memory-comparison.c @@ -286,9 +286,7 @@ struct AtomicMember { }; void Test_AtomicMember(void) { - // FIXME: this is a false positive as the list of objects with unique object - // representations is incomplete. + // _Atomic(int) has the same object representation as int: no warning. struct AtomicMember a, b; memcmp(&a, &b, sizeof(struct AtomicMember)); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of type 'struct AtomicMember' which does not have a unique object representation; consider comparing the members of the object manually } diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index 66346bf40193f..4c2a84dcb298b 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -189,6 +189,14 @@ features cannot lower the translation-unit ABI level; - Fixed concept template parameters not being recognized in `-Wdocumentation` when mentioned in tparam comments. (#GH64087) +- Added `-Wsuspicious-memcmp` (on by default, grouped under + `-Wsuspicious-memaccess`), which warns when `memcmp` or `bcmp` is used as a + whole-object equality test on a type that does not have a unique object + representation, such as a struct with padding bytes or floating-point + members, where two equal values may compare unequal. Partial (prefix) + comparisons and non-constant sizes are not diagnosed; casting a pointer + argument to `void *` silences the warning. + - `-Wunused-but-set-variable` now diagnoses file-scope variables with internal linkage (`static` storage class) that are assigned but never used. This new coverage is added under the subgroup `-Wunused-but-set-global`, @@ -375,6 +383,12 @@ features cannot lower the translation-unit ABI level; - Fixed a crash in ``__builtin_dump_struct`` when ``-Werror`` promotes format warnings to errors. (#GH211943) +- `__has_unique_object_representations` now returns `true` for `_Atomic` types + whose object representation is identical to that of their value type, such + as `_Atomic(int)`. Atomic types whose size is rounded up to a power of two + (adding padding bits) continue to report `false`. This also fixes a false + positive in the `bugprone-suspicious-memory-comparison` clang-tidy check. + #### Bug Fixes to Attribute Support - The `counted_by`/`counted_by_or_null` diagnostic that rejects a pointer whose diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 698f15c57aa9e..ceb8dde7f3c4b 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -1014,9 +1014,11 @@ def NonTrivialMemcall : DiagGroup<"nontrivial-memcall">; def NonTrivialMemaccess : DiagGroup<"nontrivial-memaccess", [NonTrivialMemcall]>; def NonportableSystemIncludePath : DiagGroup<"nonportable-system-include-path">; def SuspiciousBzero : DiagGroup<"suspicious-bzero">; +def SuspiciousMemcmp : DiagGroup<"suspicious-memcmp">; def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess", [SizeofPointerMemaccess, DynamicClassMemaccess, - NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>; + NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero, + SuspiciousMemcmp]>; def StaticInInline : DiagGroup<"static-in-inline">; def StaticLocalInInline : DiagGroup<"static-local-in-inline">; def UniqueObjectDuplication : DiagGroup<"unique-object-duplication"> { diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 6bd9871e91b45..4639e5b6f3a36 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -909,6 +909,11 @@ def warn_dyn_class_memaccess : Warning< InGroup<DynamicClassMemaccess>; def note_bad_memaccess_silence : Note< "explicitly cast the pointer to silence this warning">; +def warn_suspicious_memcmp_nonunique : Warning< + "%select{first|second}0 operand of this %1 call is a pointer to type %2 " + "which does not have a unique object representation; consider comparing " + "%select{the values|the members of the object}3 manually">, + InGroup<SuspiciousMemcmp>; def warn_sizeof_pointer_expr_memaccess : Warning< "'%0' call operates on objects of type %1 while the size is based on a " "different type %2">, diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp index 5f1e5b30ee50c..c1a481b62e24c 100644 --- a/clang/lib/AST/ASTContext.cpp +++ b/clang/lib/AST/ASTContext.cpp @@ -3059,6 +3059,14 @@ bool ASTContext::hasUniqueObjectRepresentations( "hasUniqueObjectRepresentations should not be called with an " "incomplete type"); + // _Atomic(T) shares T's object representation unless its size was rounded + // up to a power of two, in which case the extra bytes are padding. Atomic + // types are never trivially copyable, so (9.1) is judged on the value type. + if (const auto *AT = Ty->getAs<AtomicType>()) + return getTypeSize(AT) == getTypeSize(AT->getValueType()) && + hasUniqueObjectRepresentations(AT->getValueType(), + CheckIfTriviallyCopyable); + // (9.1) - T is trivially copyable... if (CheckIfTriviallyCopyable && !Ty.isTriviallyCopyableType(*this)) return false; @@ -3099,7 +3107,6 @@ bool ASTContext::hasUniqueObjectRepresentations( // FIXME: More cases to handle here (list by rsmith): // vectors (careful about, eg, vector of 3 foo) // _Complex int and friends - // _Atomic T // Obj-C block pointers // Obj-C object pointers // and perhaps OpenCL's various builtin types (pipe, sampler_t, event_t, diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index 0db040ed90e3f..9462a6eb91829 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -11141,13 +11141,14 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call, if (PointeeTy == QualType()) continue; + const bool IsCmp = BId == Builtin::BImemcmp || BId == Builtin::BIbcmp; + // Always complain about dynamic classes. bool IsContained; if (const CXXRecordDecl *ContainedRD = getContainedDynamicClass(PointeeTy, IsContained)) { unsigned OperationType = 0; - const bool IsCmp = BId == Builtin::BImemcmp || BId == Builtin::BIbcmp; // "overwritten" if we're warning about the destination for any call // but memcmp; otherwise a verb appropriate to the call. if (ArgIdx != 0 || IsCmp) { @@ -11171,7 +11172,26 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call, PDiag(diag::warn_arc_object_memaccess) << ArgIdx << FnName << PointeeTy << Call->getCallee()->getSourceRange()); - else if (const auto *RD = PointeeTy->getAsRecordDecl()) { + else if (IsCmp && !PointeeTy->isDependentType() && + !PointeeTy->isIncompleteType() && !PointeeTy->isFunctionType() && + !PointeeTy->isSizelessType()) { + // Comparing objects whose equal values may differ in object + // representation (padding bytes, multiple floating-point encodings) + // is not a reliable equality test. Only diagnose when the constant + // length covers the entire object; a partial (prefix) compare of + // leading members is deliberate use. + Expr::EvalResult SizeResult; + if (LenExpr->isValueDependent() || + !LenExpr->EvaluateAsInt(SizeResult, Context) || + SizeResult.Val.getInt().ult(static_cast<uint64_t>( + Context.getTypeSizeInChars(PointeeTy).getQuantity())) || + Context.hasUniqueObjectRepresentations(PointeeTy)) + continue; + DiagRuntimeBehavior(Dest->getExprLoc(), Dest, + PDiag(diag::warn_suspicious_memcmp_nonunique) + << ArgIdx << FnName << PointeeTy + << !PointeeTy->isScalarType()); + } else if (const auto *RD = PointeeTy->getAsRecordDecl()) { // FIXME: Do not consider incomplete types even though they may be // completed later. GCC does not diagnose such code, but we may want to diff --git a/clang/test/Sema/warn-suspicious-memcmp.c b/clang/test/Sema/warn-suspicious-memcmp.c new file mode 100644 index 0000000000000..d994f9680878e --- /dev/null +++ b/clang/test/Sema/warn-suspicious-memcmp.c @@ -0,0 +1,66 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -verify %s +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -Wno-suspicious-memcmp -verify=quiet %s +// quiet-no-diagnostics + +typedef __SIZE_TYPE__ size_t; +int memcmp(const void *s1, const void *s2, size_t n); +int bcmp(const void *s1, const void *s2, size_t n); + +struct Padded { char tag; int x; }; // 3 padding bytes after 'tag' +struct Dense { int a; int b; }; // no padding +struct WithFloat { float x; float y; }; // no padding, but float encodings +struct WithAtomic { _Atomic(int) x; }; // layout-identical to int +union Slack { char c; int i; }; // 'c' leaves 3 bytes of slack +struct NeverDefined; + +void test_padded(struct Padded *a, struct Padded *b, size_t n) { + memcmp(a, b, sizeof(struct Padded)); // expected-warning{{first operand of this 'memcmp' call is a pointer to type 'struct Padded' which does not have a unique object representation; consider comparing the members of the object manually}} \ + // expected-note{{explicitly cast the pointer to silence this warning}} + memcmp(a, b, 8); // expected-warning{{first operand of this 'memcmp' call is a pointer to type 'struct Padded'}} \ + // expected-note{{explicitly cast the pointer to silence this warning}} + memcmp(a, b, sizeof(int)); // prefix compare of leading members: no warning + memcmp(a, b, n); // non-constant size: no warning + memcmp((const void *)a, (const void *)b, sizeof(struct Padded)); // silenced +} + +void test_spellings(struct Padded *a, struct Padded *b) { + bcmp(a, b, sizeof(struct Padded)); // expected-warning{{first operand of this 'bcmp' call is a pointer to type 'struct Padded'}} \ + // expected-note{{explicitly cast the pointer to silence this warning}} + __builtin_memcmp(a, b, sizeof(struct Padded)); // expected-warning{{first operand of this '__builtin_memcmp' call is a pointer to type 'struct Padded'}} \ + // expected-note{{explicitly cast the pointer to silence this warning}} +} + +void test_scalars(float *x, float *y) { + memcmp(x, y, sizeof(float)); // expected-warning{{first operand of this 'memcmp' call is a pointer to type 'float' which does not have a unique object representation; consider comparing the values manually}} \ + // expected-note{{explicitly cast the pointer to silence this warning}} +} + +void test_float_struct(struct WithFloat *a, struct WithFloat *b) { + memcmp(a, b, sizeof(struct WithFloat)); // expected-warning{{first operand of this 'memcmp' call is a pointer to type 'struct WithFloat' which does not have a unique object representation; consider comparing the members of the object manually}} \ + // expected-note{{explicitly cast the pointer to silence this warning}} +} + +void test_union(union Slack *a, union Slack *b) { + memcmp(a, b, sizeof(union Slack)); // expected-warning{{first operand of this 'memcmp' call is a pointer to type 'union Slack'}} \ + // expected-note{{explicitly cast the pointer to silence this warning}} +} + +void test_arrays(void) { + struct Padded a[3], b[3]; + memcmp(a, b, sizeof(a)); // expected-warning{{first operand of this 'memcmp' call is a pointer to type 'struct Padded}} \ + // expected-note{{explicitly cast the pointer to silence this warning}} +} + +// No warnings below this point. + +void test_dense(struct Dense *a, struct Dense *b) { + memcmp(a, b, sizeof(struct Dense)); +} + +void test_atomic(struct WithAtomic *a, struct WithAtomic *b) { + memcmp(a, b, sizeof(struct WithAtomic)); +} + +void test_incomplete(struct NeverDefined *a, struct NeverDefined *b) { + memcmp(a, b, 16); +} diff --git a/clang/test/SemaCXX/type-traits.cpp b/clang/test/SemaCXX/type-traits.cpp index ff74461308fb1..246da619fe046 100644 --- a/clang/test/SemaCXX/type-traits.cpp +++ b/clang/test/SemaCXX/type-traits.cpp @@ -3504,6 +3504,16 @@ static_assert(__has_unique_object_representations(const int *), "as are pointers static_assert(__has_unique_object_representations(volatile int *), "as are pointers"); static_assert(__has_unique_object_representations(const volatile int *), "as are pointers"); +static_assert(__has_unique_object_representations(_Atomic(int)), "layout-identical atomics are"); +static_assert(!__has_unique_object_representations(_Atomic(float)), "value type is not unique"); +struct AtomicReprThreeChars { char a, b, c; }; +static_assert(!__has_unique_object_representations(_Atomic(AtomicReprThreeChars)), + "atomic size rounded up to a power of two adds padding"); +struct AtomicReprMember { _Atomic(int) x; }; +static_assert(__has_unique_object_representations(AtomicReprMember), "atomic member, no padding"); +struct AtomicReprPadded { char c; _Atomic(int) x; }; +static_assert(!__has_unique_object_representations(AtomicReprPadded), "padding before atomic member"); + class C {}; using FP = int (*)(int); using PMF = int (C::*)(int); diff --git a/clang/test/SemaCXX/warn-suspicious-memcmp.cpp b/clang/test/SemaCXX/warn-suspicious-memcmp.cpp new file mode 100644 index 0000000000000..4b8a93ad68b7e --- /dev/null +++ b/clang/test/SemaCXX/warn-suspicious-memcmp.cpp @@ -0,0 +1,62 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -verify %s + +extern "C" int memcmp(const void *s1, const void *s2, decltype(sizeof(0)) n); + +struct Padded { char tag; int x; }; +struct Dense { int a, b; }; + +class Poly { +public: + virtual ~Poly(); + int x; +}; + +class MixedAccess { +public: + int a; +private: + int b; + +public: + int sum() const { return a + b; } +}; + +void test_basic(Padded *a, Padded *b) { + memcmp(a, b, sizeof(Padded)); // expected-warning{{first operand of this 'memcmp' call is a pointer to type 'Padded' which does not have a unique object representation; consider comparing the members of the object manually}} \ + // expected-note{{explicitly cast the pointer to silence this warning}} +} + +void test_dense(Dense *a, Dense *b) { + memcmp(a, b, sizeof(Dense)); // no warning +} + +// Dynamic classes are owned by -Wdynamic-class-memaccess; the new warning +// must not fire on top of it. +void test_poly(Poly *a, Poly *b) { + memcmp(a, b, sizeof(Poly)); // expected-warning{{first operand of this 'memcmp' call is a pointer to dynamic class 'Poly'; vtable pointer will be compared}} \ + // expected-note{{explicitly cast the pointer to silence this warning}} +} + +// Deliberate scope cut: non-standard-layout without padding stays silent here +// (clang-tidy's bugprone-suspicious-memory-comparison still diagnoses it). +void test_mixed(MixedAccess *a, MixedAccess *b) { + memcmp(a, b, sizeof(MixedAccess)); // no warning +} + +template <typename T> +bool eq(T &a, T &b) { + return memcmp(&a, &b, sizeof(T)) == 0; // expected-warning{{first operand of this 'memcmp' call is a pointer to type 'Padded' which does not have a unique object representation; consider comparing the members of the object manually}} \ + // expected-note{{explicitly cast the pointer to silence this warning}} +} + +// Dependent length: must not crash, and must respect the size rule once +// instantiated. +template <int N> +bool eqn(Padded &a, Padded &b) { + return memcmp(&a, &b, N) == 0; // no warning for N < sizeof(Padded) +} + +bool test_templates(Padded p1, Padded p2, Dense d1, Dense d2) { + return eq(p1, p2) && // expected-note{{in instantiation of function template specialization 'eq<Padded>' requested here}} + eq(d1, d2) && eqn<4>(p1, p2); +} diff --git a/libcxx/include/__atomic/atomic_ref.h b/libcxx/include/__atomic/atomic_ref.h index 78f1f24f4c7a4..7b2483174fe5b 100644 --- a/libcxx/include/__atomic/atomic_ref.h +++ b/libcxx/include/__atomic/atomic_ref.h @@ -96,7 +96,11 @@ struct __atomic_ref_base { return true; } _Tp __curr = __copy; - if (std::memcmp(__clear_padding(__prev), __clear_padding(__curr), sizeof(_Tp)) != 0) { + // Comparing the object representations is the point here; the casts opt out + // of -Wsuspicious-memcmp. + if (std::memcmp(static_cast<const void*>(__clear_padding(__prev)), + static_cast<const void*>(__clear_padding(__curr)), + sizeof(_Tp)) != 0) { // Value representation without padding bits do not compare equal -> // write the current content of *ptr into *expected std::memcpy(__expected, std::addressof(__copy), sizeof(_Tp)); diff --git a/libcxx/include/__atomic/atomic_sync.h b/libcxx/include/__atomic/atomic_sync.h index c96cd63a82e3e..5d8d2d45a4d89 100644 --- a/libcxx/include/__atomic/atomic_sync.h +++ b/libcxx/include/__atomic/atomic_sync.h @@ -248,7 +248,11 @@ _LIBCPP_HIDE_FROM_ABI void __atomic_notify_all(const _AtomicWaitable&) {} template <typename _Tp> _LIBCPP_HIDE_FROM_ABI bool __cxx_nonatomic_compare_equal(_Tp const& __lhs, _Tp const& __rhs) { - return std::memcmp(std::addressof(__lhs), std::addressof(__rhs), sizeof(_Tp)) == 0; + // The object representations are compared on purpose ([atomics.wait]); the casts + // opt out of -Wsuspicious-memcmp. + return std::memcmp(static_cast<const void*>(std::addressof(__lhs)), + static_cast<const void*>(std::addressof(__rhs)), + sizeof(_Tp)) == 0; } template <class _AtomicWaitable, class _Tp> diff --git a/libcxx/include/__cxx03/__atomic/atomic_sync.h b/libcxx/include/__cxx03/__atomic/atomic_sync.h index ca029f0384058..df81b23f83acb 100644 --- a/libcxx/include/__cxx03/__atomic/atomic_sync.h +++ b/libcxx/include/__cxx03/__atomic/atomic_sync.h @@ -181,7 +181,11 @@ _LIBCPP_HIDE_FROM_ABI void __atomic_notify_all(const _AtomicWaitable&) {} template <typename _Tp> _LIBCPP_HIDE_FROM_ABI bool __cxx_nonatomic_compare_equal(_Tp const& __lhs, _Tp const& __rhs) { - return std::memcmp(std::addressof(__lhs), std::addressof(__rhs), sizeof(_Tp)) == 0; + // The object representations are compared on purpose ([atomics.wait]); the casts + // opt out of -Wsuspicious-memcmp. + return std::memcmp(static_cast<const void*>(std::addressof(__lhs)), + static_cast<const void*>(std::addressof(__rhs)), + sizeof(_Tp)) == 0; } template <class _Tp> diff --git a/libcxx/test/libcxx/atomics/atomics.types.generic/padding.pass.cpp b/libcxx/test/libcxx/atomics/atomics.types.generic/padding.pass.cpp index ce6e44915188e..5de181c32e07e 100644 --- a/libcxx/test/libcxx/atomics/atomics.types.generic/padding.pass.cpp +++ b/libcxx/test/libcxx/atomics/atomics.types.generic/padding.pass.cpp @@ -11,6 +11,13 @@ // Older Clang doesn't implement __builtin_clear_padding // XFAIL: clang-21, apple-clang-21, clang-22 +// This test deliberately inspects object representations, padding included. +#ifdef __clang__ +# if __has_warning("-Wsuspicious-memcmp") +# pragma clang diagnostic ignored "-Wsuspicious-memcmp" +# endif +#endif + // atomic_init is deprecated // ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS diff --git a/libcxx/test/libcxx/atomics/clear_padding.pass.cpp b/libcxx/test/libcxx/atomics/clear_padding.pass.cpp index 69d4a7f9b694a..09d17b32aaddf 100644 --- a/libcxx/test/libcxx/atomics/clear_padding.pass.cpp +++ b/libcxx/test/libcxx/atomics/clear_padding.pass.cpp @@ -11,6 +11,13 @@ //... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/214218 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
