https://github.com/flash1729 created https://github.com/llvm/llvm-project/pull/214218
> [!NOTE] > Stacked on #212768 (the first commit in this PR); will rebase to a single > commit once that lands. 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`); there was no 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. These are the same trigger conditions as the tidy check, so prefix compares and non-constant sizes stay silent. Dynamic classes keep their existing, more specific warning. Casting an argument to `(void *)` silences the new one through the shared fixit note. Incomplete, dependent, function, and sizeless pointee types are skipped, and templates are diagnosed per instantiation. The known `_Atomic` false positive in `hasUniqueObjectRepresentations` is fixed by #212768. The predicate's remaining unhandled types (vectors, `_Complex`, some ObjC and OpenCL types) conservatively report non-unique. That's the same behavior the tidy check has shipped since 2021, and none of these types show up in memcmp'd structs anywhere in the monorepo. Whole-struct memcmp itself appears at roughly ten sites in the tree, none of which trigger the warning, and check-clang passes clean. >From 8cebfe1aa29d466308528cccd539c38080a5029f Mon Sep 17 00:00:00 2001 From: flash1729 <[email protected]> Date: Wed, 29 Jul 2026 17:20:06 +0530 Subject: [PATCH 1/2] [clang] Handle _Atomic types in hasUniqueObjectRepresentations Atomic types aren't scalar, so the predicate rejected them before layout was checked and fell through to false. _Atomic(T) shares T's representation when sizes match; padding from size rounding makes it non-unique. Fixes the FIXME in bugprone-suspicious-memory-comparison (D89651). --- .../checkers/bugprone/suspicious-memory-comparison.c | 4 +--- clang/docs/ReleaseNotes.md | 6 ++++++ clang/lib/AST/ASTContext.cpp | 9 ++++++++- clang/test/SemaCXX/type-traits.cpp | 10 ++++++++++ 4 files changed, 25 insertions(+), 4 deletions(-) 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 80e770a40838a..20b37f466b84f 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -327,6 +327,12 @@ features cannot lower the translation-unit ABI level; - Fixed a crash when classifying a call to a builtin with dependent arguments, such as when the call is used as an `auto` non-type template argument. +- `__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/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp index 2228811546c0f..6aea6cfcf25ca 100644 --- a/clang/lib/AST/ASTContext.cpp +++ b/clang/lib/AST/ASTContext.cpp @@ -3062,6 +3062,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; @@ -3102,7 +3110,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/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); >From b1897b8c6eaac71947dde77b91ab1c4fe361b384 Mon Sep 17 00:00:00 2001 From: flash1729 <[email protected]> Date: Wed, 5 Aug 2026 14:29:38 +0530 Subject: [PATCH 2/2] [clang][Sema] Add -Wsuspicious-memcmp for memcmp on types without unique object representations Warn when memcmp/bcmp is used as a whole-object equality test on a type whose equal values may differ in object representation (padding, float encodings). Same trigger conditions as bugprone-suspicious-memory-comparison; on by default under -Wsuspicious-memaccess like its siblings. --- clang/docs/ReleaseNotes.md | 8 +++ clang/include/clang/Basic/DiagnosticGroups.td | 4 +- .../clang/Basic/DiagnosticSemaKinds.td | 5 ++ clang/lib/Sema/SemaChecking.cpp | 24 ++++++- clang/test/Sema/warn-suspicious-memcmp.c | 66 +++++++++++++++++++ clang/test/SemaCXX/warn-suspicious-memcmp.cpp | 62 +++++++++++++++++ 6 files changed, 166 insertions(+), 3 deletions(-) create mode 100644 clang/test/Sema/warn-suspicious-memcmp.c create mode 100644 clang/test/SemaCXX/warn-suspicious-memcmp.cpp diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index 20b37f466b84f..3ce668ad4a709 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -150,6 +150,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`, diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index b7072634cccf3..f12b78dc0b321 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 89e2f956971b3..2de921d3eafd3 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/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index ef07e8c6133ed..5f574efbee1c0 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/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); +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
