[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
https://github.com/DKLoehr created
https://github.com/llvm/llvm-project/pull/117622
When a hidden object is built into multiple shared libraries, each instance of
the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global variable or static
data member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated), and
- Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem semantically if one of the following is true:
1. The object is mutable (the copies won't be in sync), or
2. Its initialization has side effects (it may now run more than once), or
3. The value of its address is used (different copies have different addresses).
To detect this, we add a new -Wunique-object-duplication warning. It warns on
cases (1) and (2) above. To be conservative, we only warn in case (2) if we are
certain the initializer has side effects, and we don't warn on `new` because
the only side effect is some extra memory usage.
We don't currently warn on case (3) because doing so is prone to false
positives: there are many reasons for taking the address which aren't
inherently problematic (e.g. passing to a function that expects a pointer). We
only run into problems if the code inspects the value of the address.
The check is currently disabled for windows, which uses its own analogue of
visibility (declimport/declexport). The check is also disabled inside
templates, since it can give false positives if a template is never
instantiated.
### Resolving the warning
The warning can be fixed in several ways:
- If the object in question doesn't need to be mutable, it should be made
const. Note that the variable must be completely immutable, e.g. we'll warn on
`const int* p` because the pointer itself is mutable. To silence the warning,
it should instead be `const int* const p`.
- If the object must be mutable, it (or the enclosing function, in the case of
static local variables) should be made visible using
`__attribute((visibility("default")))`
- If the object is supposed to be duplicated, it should be be given internal
linkage.
### Testing
I've tested the warning by running it on clang itself, as well as on chromium.
Compiling clang resulted in [10 warnings across 6
files](https://github.com/user-attachments/files/17908069/clang-warnings.txt),
while Chromium resulted in [160 warnings across 85
files](https://github.com/user-attachments/files/17908072/chromium-warnings.txt),
mostly in third-party code. Almost all warnings were due to mutable variables.
I evaluated the warnings by manual inspection. I believe all the resulting
warnings are true positives, i.e. they represent potentially-problematic code
where duplication might cause a problem. For the clang warnings, I also
validated them by either adding `const` or visibility annotations as
appropriate.
### Limitations
I am aware of four main limitations with the current warning:
1. We do not warn when the address of a duplicated object is taken, since doing
so is prone to false positives. I'm hopeful that we can create a refined
version in the future, however.
2. We only warn for side-effectful initialization if we are certain side
effects exist. Warning on potential side effects produced a huge number of
false positives; I don't expect there's much that can be done about this in
modern C++ code bases, since proving a lack of side effects is difficult.
3. Windows uses a different system (declexport/import) instead of visibility.
From manual testing, it seems to behave analogously to the visibility system
for the purposes of this warning, but to keep things simple the warning is
disabled on windows for now.
4. We don't warn on code inside templates. This is unfortuate, since it masks
many real issues, e.g. a templated variable which is implicitly instantiated
the same way in multiple TUs should be globally unique, but may accidentally be
duplicated. Unfortunately, we found some potential false positives during
testing that caused us to disable the warning for now.
>From 9acba88ec7e5d182d7ca32e143506eb3e7e50e8e Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 21 Nov 2024 19:29:00 +
Subject: [PATCH] Warn when unique objects might be duplicated in shared
libraries
When a hidden object is built into multiple shared libraries, each
instance of the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global
variable or static member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated), and
- Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem if
[clang] [libcxx] Warn when unique objects might be duplicated in shared libraries (PR #117622)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/117622
>From d944b2fde573a4fb352400ce3425121265b02685 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 21 Nov 2024 19:29:00 +
Subject: [PATCH 1/2] Warn when unique objects might be duplicated in shared
libraries
When a hidden object is built into multiple shared libraries, each
instance of the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global
variable or static member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated), and
- Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem if one of the following is true:
1. The object is mutable (the copies won't be in sync), or
2. Its initialization has side effects (it may now run more than once), or
3. The value of its address is used (which one?).
To detect this, we add a new -Wunique-object-duplication warning.
It warns on cases (1) and (2) above. To be conservative, we only
warn in case (2) if we are certain the initializer has side effects,
and we don't warn on `new` because the only side effect is some
extra memory usage.
We don't currently warn on case (3) because doing so is prone to
false positives: there are many reasons for taking the address which
aren't inherently problematic (e.g. passing to a function that expects
a pointer). We only run into problems if the code inspects the value
of the address.
The check is currently disabled for windows, which uses its own analogue
of visibility (declimport/declexport). The check is also disabled inside
templates, since it can give false positives if a template is never
instantiated.
---
clang/include/clang/Basic/DiagnosticGroups.td | 1 +
.../clang/Basic/DiagnosticSemaKinds.td| 9 +
clang/include/clang/Sema/Sema.h | 6 +
clang/lib/Sema/SemaDecl.cpp | 101 ++
.../SemaCXX/unique_object_duplication.cpp | 16 ++
.../test/SemaCXX/unique_object_duplication.h | 187 ++
6 files changed, 320 insertions(+)
create mode 100644 clang/test/SemaCXX/unique_object_duplication.cpp
create mode 100644 clang/test/SemaCXX/unique_object_duplication.h
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index df9bf94b5d0398..c8c446a7b8cfa4 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -690,6 +690,7 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
+def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
// Allow differentiation between GNU statement expressions in a macro versus
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 6ff24c2bc8faad..b8768c28a0a3e3 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6104,6 +6104,15 @@ def warn_static_local_in_extern_inline : Warning<
def note_convert_inline_to_static : Note<
"use 'static' to give inline function %0 internal linkage">;
+def warn_possible_object_duplication_mutable : Warning<
+ "%0 is mutable, has hidden visibility, and external linkage; it may be "
+ "duplicated when built into a shared library">,
+ InGroup;
+def warn_possible_object_duplication_init : Warning<
+ "%0 has hidden visibility, and external linkage; its initialization may run "
+ "more than once when built into a shared library">,
+ InGroup;
+
def ext_redefinition_of_typedef : ExtWarn<
"redefinition of typedef %0 is a C11 feature">,
InGroup >;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 24abd5d95dd844..124979973ad15e 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3643,6 +3643,12 @@ class Sema final : public SemaBase {
NonTrivialCUnionContext UseContext,
unsigned NonTrivialKind);
+ /// Certain globally-unique variables might be accidentally duplicated if
+ /// built into multiple shared libraries with hidden visibility. This can
+ /// cause problems if the variable is mutable, its initialization is
+ /// effectful, or its address is taken.
+ bool GloballyUniqueObjectMightBeAccidentallyDuplicated(const VarDecl *dcl);
+
/// AddInitializerToDecl - Adds the initializer Init to the
/// declaration dcl. If DirectInit is true,
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/117622
>From d944b2fde573a4fb352400ce3425121265b02685 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 21 Nov 2024 19:29:00 +
Subject: [PATCH] Warn when unique objects might be duplicated in shared
libraries
When a hidden object is built into multiple shared libraries, each
instance of the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global
variable or static member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated), and
- Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem if one of the following is true:
1. The object is mutable (the copies won't be in sync), or
2. Its initialization has side effects (it may now run more than once), or
3. The value of its address is used (which one?).
To detect this, we add a new -Wunique-object-duplication warning.
It warns on cases (1) and (2) above. To be conservative, we only
warn in case (2) if we are certain the initializer has side effects,
and we don't warn on `new` because the only side effect is some
extra memory usage.
We don't currently warn on case (3) because doing so is prone to
false positives: there are many reasons for taking the address which
aren't inherently problematic (e.g. passing to a function that expects
a pointer). We only run into problems if the code inspects the value
of the address.
The check is currently disabled for windows, which uses its own analogue
of visibility (declimport/declexport). The check is also disabled inside
templates, since it can give false positives if a template is never
instantiated.
---
clang/include/clang/Basic/DiagnosticGroups.td | 1 +
.../clang/Basic/DiagnosticSemaKinds.td| 9 +
clang/include/clang/Sema/Sema.h | 6 +
clang/lib/Sema/SemaDecl.cpp | 101 ++
.../SemaCXX/unique_object_duplication.cpp | 16 ++
.../test/SemaCXX/unique_object_duplication.h | 187 ++
6 files changed, 320 insertions(+)
create mode 100644 clang/test/SemaCXX/unique_object_duplication.cpp
create mode 100644 clang/test/SemaCXX/unique_object_duplication.h
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index df9bf94b5d0398..c8c446a7b8cfa4 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -690,6 +690,7 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
+def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
// Allow differentiation between GNU statement expressions in a macro versus
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 6ff24c2bc8faad..b8768c28a0a3e3 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6104,6 +6104,15 @@ def warn_static_local_in_extern_inline : Warning<
def note_convert_inline_to_static : Note<
"use 'static' to give inline function %0 internal linkage">;
+def warn_possible_object_duplication_mutable : Warning<
+ "%0 is mutable, has hidden visibility, and external linkage; it may be "
+ "duplicated when built into a shared library">,
+ InGroup;
+def warn_possible_object_duplication_init : Warning<
+ "%0 has hidden visibility, and external linkage; its initialization may run "
+ "more than once when built into a shared library">,
+ InGroup;
+
def ext_redefinition_of_typedef : ExtWarn<
"redefinition of typedef %0 is a C11 feature">,
InGroup >;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 24abd5d95dd844..124979973ad15e 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3643,6 +3643,12 @@ class Sema final : public SemaBase {
NonTrivialCUnionContext UseContext,
unsigned NonTrivialKind);
+ /// Certain globally-unique variables might be accidentally duplicated if
+ /// built into multiple shared libraries with hidden visibility. This can
+ /// cause problems if the variable is mutable, its initialization is
+ /// effectful, or its address is taken.
+ bool GloballyUniqueObjectMightBeAccidentallyDuplicated(const VarDecl *dcl);
+
/// AddInitializerToDecl - Adds the initializer Init to the
/// declaration dcl. If DirectInit is true, this
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/117622
>From ba531f3dce1b992dad191e5a9f724ebdc6750d6c Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 21 Nov 2024 19:29:00 +
Subject: [PATCH] Warn when unique objects might be duplicated in shared
libraries
When a hidden object is built into multiple shared libraries, each
instance of the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global
variable or static member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated), and
- Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem if one of the following is true:
1. The object is mutable (the copies won't be in sync), or
2. Its initialization has side effects (it may now run more than once), or
3. The value of its address is used (which one?).
To detect this, we add a new -Wunique-object-duplication warning.
It warns on cases (1) and (2) above. To be conservative, we only
warn in case (2) if we are certain the initializer has side effects,
and we don't warn on `new` because the only side effect is some
extra memory usage.
We don't currently warn on case (3) because doing so is prone to
false positives: there are many reasons for taking the address which
aren't inherently problematic (e.g. passing to a function that expects
a pointer). We only run into problems if the code inspects the value
of the address.
The check is currently disabled for windows, which uses its own analogue
of visibility (declimport/declexport). The check is also disabled inside
templates, since it can give false positives if a template is never
instantiated.
Because the warning fires in several places, included third-party
libraries, it is currently disabled-by-default until all these
instances have been fixed. It should be enabled-by-default afterwards.
---
clang/include/clang/Basic/DiagnosticGroups.td | 1 +
.../clang/Basic/DiagnosticSemaKinds.td| 9 +
clang/include/clang/Sema/Sema.h | 6 +
clang/lib/Sema/SemaDecl.cpp | 101 ++
.../SemaCXX/unique_object_duplication.cpp | 16 ++
.../test/SemaCXX/unique_object_duplication.h | 187 ++
6 files changed, 320 insertions(+)
create mode 100644 clang/test/SemaCXX/unique_object_duplication.cpp
create mode 100644 clang/test/SemaCXX/unique_object_duplication.h
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index 3ac490d30371b1..ac8163467c5c1d 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -695,6 +695,7 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
+def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
// Allow differentiation between GNU statement expressions in a macro versus
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 8020be6c57bcfb..68d954a70d91b6 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6108,6 +6108,15 @@ def warn_static_local_in_extern_inline : Warning<
def note_convert_inline_to_static : Note<
"use 'static' to give inline function %0 internal linkage">;
+def warn_possible_object_duplication_mutable : Warning<
+ "%0 is mutable, has hidden visibility, and external linkage; it may be "
+ "duplicated when built into a shared library">,
+ InGroup, DefaultIgnore;
+def warn_possible_object_duplication_init : Warning<
+ "%0 has hidden visibility, and external linkage; its initialization may run "
+ "more than once when built into a shared library">,
+ InGroup, DefaultIgnore;
+
def ext_redefinition_of_typedef : ExtWarn<
"redefinition of typedef %0 is a C11 feature">,
InGroup >;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index b8684d11460eda..55229df4dc924a 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3654,6 +3654,12 @@ class Sema final : public SemaBase {
NonTrivialCUnionContext UseContext,
unsigned NonTrivialKind);
+ /// Certain globally-unique variables might be accidentally duplicated if
+ /// built into multiple shared libraries with hidden visibility. This can
+ /// cause problems if the variable is mutable, its initialization is
+ ///
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
DKLoehr wrote: Because this warning keeps firing in many different places, including in third-party libraries, I've disabled it by default until I can go through and fix all the instances. I think this provides a cleaner separation of concerns, so that this PR isn't clogged up with various housekeeping changes. Once the warning is cleaned up, it should be enabled-by-default. https://github.com/llvm/llvm-project/pull/117622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
DKLoehr wrote: Ping https://github.com/llvm/llvm-project/pull/117622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
DKLoehr wrote: Ping https://github.com/llvm/llvm-project/pull/117622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
DKLoehr wrote: Thanks @zmodem for merging (and also reverting). Follow-up PR with the fix is #125526. https://github.com/llvm/llvm-project/pull/117622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #125526)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/125526
>From 486c3297f1a316a103c6583daf732af2d00d0b96 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 21 Nov 2024 19:29:00 +
Subject: [PATCH 1/6] Warn when unique objects might be duplicated in shared
libraries
When a hidden object is built into multiple shared libraries, each
instance of the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global
variable or static member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated), and
- Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem if one of the following is true:
1. The object is mutable (the copies won't be in sync), or
2. Its initialization has side effects (it may now run more than once), or
3. The value of its address is used (which one?).
To detect this, we add a new -Wunique-object-duplication warning.
It warns on cases (1) and (2) above. To be conservative, we only
warn in case (2) if we are certain the initializer has side effects,
and we don't warn on `new` because the only side effect is some
extra memory usage.
We don't currently warn on case (3) because doing so is prone to
false positives: there are many reasons for taking the address which
aren't inherently problematic (e.g. passing to a function that expects
a pointer). We only run into problems if the code inspects the value
of the address.
The check is currently disabled for windows, which uses its own analogue
of visibility (declimport/declexport). The check is also disabled inside
templates, since it can give false positives if a template is never
instantiated.
Because the warning fires in several places, included third-party
libraries, it is currently disabled-by-default until all these
instances have been fixed. It should be enabled-by-default afterwards.
---
clang/include/clang/Basic/DiagnosticGroups.td | 1 +
.../clang/Basic/DiagnosticSemaKinds.td| 9 +
clang/include/clang/Sema/Sema.h | 6 +
clang/lib/Sema/SemaDecl.cpp | 101 ++
.../SemaCXX/unique_object_duplication.cpp | 16 ++
.../test/SemaCXX/unique_object_duplication.h | 187 ++
6 files changed, 320 insertions(+)
create mode 100644 clang/test/SemaCXX/unique_object_duplication.cpp
create mode 100644 clang/test/SemaCXX/unique_object_duplication.h
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index abb575002e1182..d63827cf39b06c 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -694,6 +694,7 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
+def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
// Allow differentiation between GNU statement expressions in a macro versus
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 5876b2a6ae0eab..6b7fdfbb3cbb6c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6169,6 +6169,15 @@ def warn_static_local_in_extern_inline : Warning<
def note_convert_inline_to_static : Note<
"use 'static' to give inline function %0 internal linkage">;
+def warn_possible_object_duplication_mutable : Warning<
+ "%0 is mutable, has hidden visibility, and external linkage; it may be "
+ "duplicated when built into a shared library">,
+ InGroup, DefaultIgnore;
+def warn_possible_object_duplication_init : Warning<
+ "%0 has hidden visibility, and external linkage; its initialization may run "
+ "more than once when built into a shared library">,
+ InGroup, DefaultIgnore;
+
def ext_redefinition_of_typedef : ExtWarn<
"redefinition of typedef %0 is a C11 feature">,
InGroup >;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 472a0e25adc975..f04f5dccc39401 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3664,6 +3664,12 @@ class Sema final : public SemaBase {
NonTrivialCUnionContext UseContext,
unsigned NonTrivialKind);
+ /// Certain globally-unique variables might be accidentally duplicated if
+ /// built into multiple shared libraries with hidden visibility. This can
+ /// cause problems if the variable is mutable, its initialization is
+
[clang] Warn when unique objects might be duplicated in shared libraries (PR #125526)
https://github.com/DKLoehr created
https://github.com/llvm/llvm-project/pull/125526
This is attempt 2 to merge this, the first one is #117622. This properly
disables the tests when building for playstation, since the warning is disabled
there.
When a hidden object is built into multiple shared libraries, each instance of
the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global variable or static
data member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
Is defined in a header (so it might appear in multiple TUs), and
Has external linkage (otherwise it's supposed to be duplicated), and
Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem semantically if one of the following is true:
The object is mutable (the copies won't be in sync), or
Its initialization has side effects (it may now run more than once), or
The value of its address is used (different copies have different addresses).
To detect this, we add a new -Wunique-object-duplication warning. It warns on
cases (1) and (2) above. To be conservative, we only warn in case (2) if we are
certain the initializer has side effects, and we don't warn on new because the
only side effect is some extra memory usage.
We don't currently warn on case (3) because doing so is prone to false
positives: there are many reasons for taking the address which aren't
inherently problematic (e.g. passing to a function that expects a pointer). We
only run into problems if the code inspects the value of the address.
The check is currently disabled for windows, which uses its own analogue of
visibility (declimport/declexport). The check is also disabled inside
templates, since it can give false positives if a template is never
instantiated.
Resolving the warning
The warning can be fixed in several ways:
If the object in question doesn't need to be mutable, it should be made const.
Note that the variable must be completely immutable, e.g. we'll warn on const
int* p because the pointer itself is mutable. To silence the warning, it should
instead be const int* const p.
If the object must be mutable, it (or the enclosing function, in the case of
static local variables) should be made visible using
__attribute((visibility("default")))
If the object is supposed to be duplicated, it should be be given internal
linkage.
Testing
I've tested the warning by running it on clang itself, as well as on chromium.
Compiling clang resulted in [10 warnings across 6
files](https://github.com/user-attachments/files/17908069/clang-warnings.txt),
while Chromium resulted in [160 warnings across 85
files](https://github.com/user-attachments/files/17908072/chromium-warnings.txt),
mostly in third-party code. Almost all warnings were due to mutable variables.
I evaluated the warnings by manual inspection. I believe all the resulting
warnings are true positives, i.e. they represent potentially-problematic code
where duplication might cause a problem. For the clang warnings, I also
validated them by either adding const or visibility annotations as appropriate.
Limitations
I am aware of four main limitations with the current warning:
We do not warn when the address of a duplicated object is taken, since doing so
is prone to false positives. I'm hopeful that we can create a refined version
in the future, however.
We only warn for side-effectful initialization if we are certain side effects
exist. Warning on potential side effects produced a huge number of false
positives; I don't expect there's much that can be done about this in modern
C++ code bases, since proving a lack of side effects is difficult.
Windows uses a different system (declexport/import) instead of visibility. From
manual testing, it seems to behave analogously to the visibility system for the
purposes of this warning, but to keep things simple the warning is disabled on
windows for now.
We don't warn on code inside templates. This is unfortuate, since it masks many
real issues, e.g. a templated variable which is implicitly instantiated the
same way in multiple TUs should be globally unique, but may accidentally be
duplicated. Unfortunately, we found some potential false positives during
testing that caused us to disable the warning for now.
>From 798b3f21593499194487c9877b8f4a3d9e44bb6e Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 21 Nov 2024 19:29:00 +
Subject: [PATCH 1/6] Warn when unique objects might be duplicated in shared
libraries
When a hidden object is built into multiple shared libraries, each
instance of the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global
variable or static member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated)
[clang] Enable -Wunique-object-duplication inside templated code (PR #125902)
@@ -3669,6 +3669,7 @@ class Sema final : public SemaBase {
/// cause problems if the variable is mutable, its initialization is
/// effectful, or its address is taken.
bool GloballyUniqueObjectMightBeAccidentallyDuplicated(const VarDecl *Dcl);
+ void DiagnoseDangerousUniqueObjectDuplication(const VarDecl *Dcl);
DKLoehr wrote:
Not sure I agree, but not too important so changed it.
https://github.com/llvm/llvm-project/pull/125902
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Enable -Wunique-object-duplication inside templated code (PR #125902)
@@ -3669,6 +3669,7 @@ class Sema final : public SemaBase {
/// cause problems if the variable is mutable, its initialization is
/// effectful, or its address is taken.
bool GloballyUniqueObjectMightBeAccidentallyDuplicated(const VarDecl *Dcl);
+ void DiagnoseDangerousUniqueObjectDuplication(const VarDecl *Dcl);
DKLoehr wrote:
I think it's meaningful because it's possible for objects to be duplicated
"harmlessly", in which case we don't warn because the only problem is a little
extra memory usage. This is the case for constants whose initializers don't
have side effects.
https://github.com/llvm/llvm-project/pull/125902
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Enable -Wunique-object-duplication inside templated code (PR #125902)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/125902
>From d95344cf393bcf0a8580e81f4848f5f72c67a652 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Tue, 4 Feb 2025 16:47:01 +
Subject: [PATCH 1/5] Move into separate function, call in
CheckCompleteVariableDeclaration
---
clang/include/clang/Sema/Sema.h | 1 +
clang/lib/Sema/SemaDecl.cpp | 94 +
2 files changed, 50 insertions(+), 45 deletions(-)
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 1870d1271c556..4aa5d82a7b535 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3669,6 +3669,7 @@ class Sema final : public SemaBase {
/// cause problems if the variable is mutable, its initialization is
/// effectful, or its address is taken.
bool GloballyUniqueObjectMightBeAccidentallyDuplicated(const VarDecl *Dcl);
+ void DiagnoseDangerousUniqueObjectDuplication(const VarDecl *Dcl);
/// AddInitializerToDecl - Adds the initializer Init to the
/// declaration dcl. If DirectInit is true, this is C++ direct
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 74e0fcec2d911..eb8fbf424d264 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13436,6 +13436,53 @@ bool
Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
return true;
}
+void Sema::DiagnoseDangerousUniqueObjectDuplication(const VarDecl* VD) {
+ // If this object has external linkage and hidden visibility, it might be
+ // duplicated when built into a shared library, which causes problems if it's
+ // mutable (since the copies won't be in sync) or its initialization has side
+ // effects (since it will run once per copy instead of once globally)
+ // FIXME: Windows uses dllexport/dllimport instead of visibility, and we
don't
+ // handle that yet. Disable the warning on Windows for now.
+ // FIXME: Checking templates can cause false positives if the template in
+ // question is never instantiated (e.g. only specialized templates are used).
+ if (!Context.getTargetInfo().shouldDLLImportComdatSymbols() &&
+ !VD->isTemplated() &&
+ GloballyUniqueObjectMightBeAccidentallyDuplicated(VD)) {
+// Check mutability. For pointers, ensure that both the pointer and the
+// pointee are (recursively) const.
+QualType Type = VD->getType().getNonReferenceType();
+if (!Type.isConstant(VD->getASTContext())) {
+ Diag(VD->getLocation(), diag::warn_possible_object_duplication_mutable)
+ << VD;
+} else {
+ while (Type->isPointerType()) {
+Type = Type->getPointeeType();
+if (Type->isFunctionType())
+ break;
+if (!Type.isConstant(VD->getASTContext())) {
+ Diag(VD->getLocation(),
+ diag::warn_possible_object_duplication_mutable)
+ << VD;
+ break;
+}
+ }
+}
+
+// To keep false positives low, only warn if we're certain that the
+// initializer has side effects. Don't warn on operator new, since a
mutable
+// pointer will trigger the previous warning, and an immutable pointer
+// getting duplicated just results in a little extra memory usage.
+const Expr *Init = VD->getAnyInitializer();
+if (Init &&
+Init->HasSideEffects(VD->getASTContext(),
+ /*IncludePossibleEffects=*/false) &&
+!isa(Init->IgnoreParenImpCasts())) {
+ Diag(Init->getExprLoc(), diag::warn_possible_object_duplication_init)
+ << VD;
+}
+ }
+}
+
void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) {
// If there is no declaration, there was an error parsing it. Just ignore
// the initializer.
@@ -14655,6 +14702,8 @@ void Sema::CheckCompleteVariableDeclaration(VarDecl
*var) {
return;
}
+ DiagnoseDangerousUniqueObjectDuplication(var);
+
// Require the destructor.
if (!type->isDependentType())
if (const RecordType *recordType = baseType->getAs())
@@ -14842,51 +14891,6 @@ void Sema::FinalizeDeclaration(Decl *ThisDecl) {
if (DC->getRedeclContext()->isFileContext() && VD->isExternallyVisible())
AddPushedVisibilityAttribute(VD);
- // If this object has external linkage and hidden visibility, it might be
- // duplicated when built into a shared library, which causes problems if it's
- // mutable (since the copies won't be in sync) or its initialization has side
- // effects (since it will run once per copy instead of once globally)
- // FIXME: Windows uses dllexport/dllimport instead of visibility, and we
don't
- // handle that yet. Disable the warning on Windows for now.
- // FIXME: Checking templates can cause false positives if the template in
- // question is never instantiated (e.g. only specialized templates are used).
- if (!Context.getTargetInfo().shouldDLLImportComdatSymbols() &&
- !VD->isTemplated() &&
- GloballyUniqueObjectMig
[clang] Enable -Wunique-object-duplication inside templated code (PR #125902)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/125902
>From d95344cf393bcf0a8580e81f4848f5f72c67a652 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Tue, 4 Feb 2025 16:47:01 +
Subject: [PATCH 1/4] Move into separate function, call in
CheckCompleteVariableDeclaration
---
clang/include/clang/Sema/Sema.h | 1 +
clang/lib/Sema/SemaDecl.cpp | 94 +
2 files changed, 50 insertions(+), 45 deletions(-)
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 1870d1271c556cb..4aa5d82a7b535ec 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3669,6 +3669,7 @@ class Sema final : public SemaBase {
/// cause problems if the variable is mutable, its initialization is
/// effectful, or its address is taken.
bool GloballyUniqueObjectMightBeAccidentallyDuplicated(const VarDecl *Dcl);
+ void DiagnoseDangerousUniqueObjectDuplication(const VarDecl *Dcl);
/// AddInitializerToDecl - Adds the initializer Init to the
/// declaration dcl. If DirectInit is true, this is C++ direct
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 74e0fcec2d911bc..eb8fbf424d264f2 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13436,6 +13436,53 @@ bool
Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
return true;
}
+void Sema::DiagnoseDangerousUniqueObjectDuplication(const VarDecl* VD) {
+ // If this object has external linkage and hidden visibility, it might be
+ // duplicated when built into a shared library, which causes problems if it's
+ // mutable (since the copies won't be in sync) or its initialization has side
+ // effects (since it will run once per copy instead of once globally)
+ // FIXME: Windows uses dllexport/dllimport instead of visibility, and we
don't
+ // handle that yet. Disable the warning on Windows for now.
+ // FIXME: Checking templates can cause false positives if the template in
+ // question is never instantiated (e.g. only specialized templates are used).
+ if (!Context.getTargetInfo().shouldDLLImportComdatSymbols() &&
+ !VD->isTemplated() &&
+ GloballyUniqueObjectMightBeAccidentallyDuplicated(VD)) {
+// Check mutability. For pointers, ensure that both the pointer and the
+// pointee are (recursively) const.
+QualType Type = VD->getType().getNonReferenceType();
+if (!Type.isConstant(VD->getASTContext())) {
+ Diag(VD->getLocation(), diag::warn_possible_object_duplication_mutable)
+ << VD;
+} else {
+ while (Type->isPointerType()) {
+Type = Type->getPointeeType();
+if (Type->isFunctionType())
+ break;
+if (!Type.isConstant(VD->getASTContext())) {
+ Diag(VD->getLocation(),
+ diag::warn_possible_object_duplication_mutable)
+ << VD;
+ break;
+}
+ }
+}
+
+// To keep false positives low, only warn if we're certain that the
+// initializer has side effects. Don't warn on operator new, since a
mutable
+// pointer will trigger the previous warning, and an immutable pointer
+// getting duplicated just results in a little extra memory usage.
+const Expr *Init = VD->getAnyInitializer();
+if (Init &&
+Init->HasSideEffects(VD->getASTContext(),
+ /*IncludePossibleEffects=*/false) &&
+!isa(Init->IgnoreParenImpCasts())) {
+ Diag(Init->getExprLoc(), diag::warn_possible_object_duplication_init)
+ << VD;
+}
+ }
+}
+
void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) {
// If there is no declaration, there was an error parsing it. Just ignore
// the initializer.
@@ -14655,6 +14702,8 @@ void Sema::CheckCompleteVariableDeclaration(VarDecl
*var) {
return;
}
+ DiagnoseDangerousUniqueObjectDuplication(var);
+
// Require the destructor.
if (!type->isDependentType())
if (const RecordType *recordType = baseType->getAs())
@@ -14842,51 +14891,6 @@ void Sema::FinalizeDeclaration(Decl *ThisDecl) {
if (DC->getRedeclContext()->isFileContext() && VD->isExternallyVisible())
AddPushedVisibilityAttribute(VD);
- // If this object has external linkage and hidden visibility, it might be
- // duplicated when built into a shared library, which causes problems if it's
- // mutable (since the copies won't be in sync) or its initialization has side
- // effects (since it will run once per copy instead of once globally)
- // FIXME: Windows uses dllexport/dllimport instead of visibility, and we
don't
- // handle that yet. Disable the warning on Windows for now.
- // FIXME: Checking templates can cause false positives if the template in
- // question is never instantiated (e.g. only specialized templates are used).
- if (!Context.getTargetInfo().shouldDLLImportComdatSymbols() &&
- !VD->isTemplated() &&
- GloballyUniqueO
[clang] Enable -Wunique-object-duplication inside templated code (PR #125902)
DKLoehr wrote: Thanks for the review! https://github.com/llvm/llvm-project/pull/125902 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/117622
>From 798b3f21593499194487c9877b8f4a3d9e44bb6e Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 21 Nov 2024 19:29:00 +
Subject: [PATCH 1/5] Warn when unique objects might be duplicated in shared
libraries
When a hidden object is built into multiple shared libraries, each
instance of the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global
variable or static member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated), and
- Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem if one of the following is true:
1. The object is mutable (the copies won't be in sync), or
2. Its initialization has side effects (it may now run more than once), or
3. The value of its address is used (which one?).
To detect this, we add a new -Wunique-object-duplication warning.
It warns on cases (1) and (2) above. To be conservative, we only
warn in case (2) if we are certain the initializer has side effects,
and we don't warn on `new` because the only side effect is some
extra memory usage.
We don't currently warn on case (3) because doing so is prone to
false positives: there are many reasons for taking the address which
aren't inherently problematic (e.g. passing to a function that expects
a pointer). We only run into problems if the code inspects the value
of the address.
The check is currently disabled for windows, which uses its own analogue
of visibility (declimport/declexport). The check is also disabled inside
templates, since it can give false positives if a template is never
instantiated.
Because the warning fires in several places, included third-party
libraries, it is currently disabled-by-default until all these
instances have been fixed. It should be enabled-by-default afterwards.
---
clang/include/clang/Basic/DiagnosticGroups.td | 1 +
.../clang/Basic/DiagnosticSemaKinds.td| 9 +
clang/include/clang/Sema/Sema.h | 6 +
clang/lib/Sema/SemaDecl.cpp | 101 ++
.../SemaCXX/unique_object_duplication.cpp | 16 ++
.../test/SemaCXX/unique_object_duplication.h | 187 ++
6 files changed, 320 insertions(+)
create mode 100644 clang/test/SemaCXX/unique_object_duplication.cpp
create mode 100644 clang/test/SemaCXX/unique_object_duplication.h
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index 209792f851b6ae..7919680930d396 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -696,6 +696,7 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
+def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
// Allow differentiation between GNU statement expressions in a macro versus
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index db911ed121e951..f7bfda5c40be00 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6162,6 +6162,15 @@ def warn_static_local_in_extern_inline : Warning<
def note_convert_inline_to_static : Note<
"use 'static' to give inline function %0 internal linkage">;
+def warn_possible_object_duplication_mutable : Warning<
+ "%0 is mutable, has hidden visibility, and external linkage; it may be "
+ "duplicated when built into a shared library">,
+ InGroup, DefaultIgnore;
+def warn_possible_object_duplication_init : Warning<
+ "%0 has hidden visibility, and external linkage; its initialization may run "
+ "more than once when built into a shared library">,
+ InGroup, DefaultIgnore;
+
def ext_redefinition_of_typedef : ExtWarn<
"redefinition of typedef %0 is a C11 feature">,
InGroup >;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index d0fc735be5f763..2a92efb9f2fdd8 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3663,6 +3663,12 @@ class Sema final : public SemaBase {
NonTrivialCUnionContext UseContext,
unsigned NonTrivialKind);
+ /// Certain globally-unique variables might be accidentally duplicated if
+ /// built into multiple shared libraries with hidden visibility. This can
+ /// cause problems if the variable is mutable, its initialization is
+
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
DKLoehr wrote: Addressed feedback, responded where appropriate. I've also done some digging into how the warning should work on Windows: it seems like it'll be very straightforward (just replace "has hidden visibility" with "doesn't have a dll import/export attribute"), but I'll leave that for a future PR to keep each the PRs focused. https://github.com/llvm/llvm-project/pull/117622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/117622
>From 798b3f21593499194487c9877b8f4a3d9e44bb6e Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 21 Nov 2024 19:29:00 +
Subject: [PATCH 1/4] Warn when unique objects might be duplicated in shared
libraries
When a hidden object is built into multiple shared libraries, each
instance of the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global
variable or static member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated), and
- Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem if one of the following is true:
1. The object is mutable (the copies won't be in sync), or
2. Its initialization has side effects (it may now run more than once), or
3. The value of its address is used (which one?).
To detect this, we add a new -Wunique-object-duplication warning.
It warns on cases (1) and (2) above. To be conservative, we only
warn in case (2) if we are certain the initializer has side effects,
and we don't warn on `new` because the only side effect is some
extra memory usage.
We don't currently warn on case (3) because doing so is prone to
false positives: there are many reasons for taking the address which
aren't inherently problematic (e.g. passing to a function that expects
a pointer). We only run into problems if the code inspects the value
of the address.
The check is currently disabled for windows, which uses its own analogue
of visibility (declimport/declexport). The check is also disabled inside
templates, since it can give false positives if a template is never
instantiated.
Because the warning fires in several places, included third-party
libraries, it is currently disabled-by-default until all these
instances have been fixed. It should be enabled-by-default afterwards.
---
clang/include/clang/Basic/DiagnosticGroups.td | 1 +
.../clang/Basic/DiagnosticSemaKinds.td| 9 +
clang/include/clang/Sema/Sema.h | 6 +
clang/lib/Sema/SemaDecl.cpp | 101 ++
.../SemaCXX/unique_object_duplication.cpp | 16 ++
.../test/SemaCXX/unique_object_duplication.h | 187 ++
6 files changed, 320 insertions(+)
create mode 100644 clang/test/SemaCXX/unique_object_duplication.cpp
create mode 100644 clang/test/SemaCXX/unique_object_duplication.h
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index 209792f851b6ae..7919680930d396 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -696,6 +696,7 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
+def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
// Allow differentiation between GNU statement expressions in a macro versus
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index db911ed121e951..f7bfda5c40be00 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6162,6 +6162,15 @@ def warn_static_local_in_extern_inline : Warning<
def note_convert_inline_to_static : Note<
"use 'static' to give inline function %0 internal linkage">;
+def warn_possible_object_duplication_mutable : Warning<
+ "%0 is mutable, has hidden visibility, and external linkage; it may be "
+ "duplicated when built into a shared library">,
+ InGroup, DefaultIgnore;
+def warn_possible_object_duplication_init : Warning<
+ "%0 has hidden visibility, and external linkage; its initialization may run "
+ "more than once when built into a shared library">,
+ InGroup, DefaultIgnore;
+
def ext_redefinition_of_typedef : ExtWarn<
"redefinition of typedef %0 is a C11 feature">,
InGroup >;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index d0fc735be5f763..2a92efb9f2fdd8 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3663,6 +3663,12 @@ class Sema final : public SemaBase {
NonTrivialCUnionContext UseContext,
unsigned NonTrivialKind);
+ /// Certain globally-unique variables might be accidentally duplicated if
+ /// built into multiple shared libraries with hidden visibility. This can
+ /// cause problems if the variable is mutable, its initialization is
+
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
@@ -6153,6 +6153,15 @@ def warn_static_local_in_extern_inline : Warning< def note_convert_inline_to_static : Note< "use 'static' to give inline function %0 internal linkage">; +def warn_possible_object_duplication_mutable : Warning< + "%0 is mutable, has hidden visibility, and external linkage; it may be " DKLoehr wrote: Ah, sorry, I realized my comment was confusing. I meant to say that I like your version more, so I changed it. Apologies! https://github.com/llvm/llvm-project/pull/117622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/117622
>From 798b3f21593499194487c9877b8f4a3d9e44bb6e Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 21 Nov 2024 19:29:00 +
Subject: [PATCH 1/5] Warn when unique objects might be duplicated in shared
libraries
When a hidden object is built into multiple shared libraries, each
instance of the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global
variable or static member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated), and
- Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem if one of the following is true:
1. The object is mutable (the copies won't be in sync), or
2. Its initialization has side effects (it may now run more than once), or
3. The value of its address is used (which one?).
To detect this, we add a new -Wunique-object-duplication warning.
It warns on cases (1) and (2) above. To be conservative, we only
warn in case (2) if we are certain the initializer has side effects,
and we don't warn on `new` because the only side effect is some
extra memory usage.
We don't currently warn on case (3) because doing so is prone to
false positives: there are many reasons for taking the address which
aren't inherently problematic (e.g. passing to a function that expects
a pointer). We only run into problems if the code inspects the value
of the address.
The check is currently disabled for windows, which uses its own analogue
of visibility (declimport/declexport). The check is also disabled inside
templates, since it can give false positives if a template is never
instantiated.
Because the warning fires in several places, included third-party
libraries, it is currently disabled-by-default until all these
instances have been fixed. It should be enabled-by-default afterwards.
---
clang/include/clang/Basic/DiagnosticGroups.td | 1 +
.../clang/Basic/DiagnosticSemaKinds.td| 9 +
clang/include/clang/Sema/Sema.h | 6 +
clang/lib/Sema/SemaDecl.cpp | 101 ++
.../SemaCXX/unique_object_duplication.cpp | 16 ++
.../test/SemaCXX/unique_object_duplication.h | 187 ++
6 files changed, 320 insertions(+)
create mode 100644 clang/test/SemaCXX/unique_object_duplication.cpp
create mode 100644 clang/test/SemaCXX/unique_object_duplication.h
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index 209792f851b6ae..7919680930d396 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -696,6 +696,7 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
+def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
// Allow differentiation between GNU statement expressions in a macro versus
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index db911ed121e951..f7bfda5c40be00 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6162,6 +6162,15 @@ def warn_static_local_in_extern_inline : Warning<
def note_convert_inline_to_static : Note<
"use 'static' to give inline function %0 internal linkage">;
+def warn_possible_object_duplication_mutable : Warning<
+ "%0 is mutable, has hidden visibility, and external linkage; it may be "
+ "duplicated when built into a shared library">,
+ InGroup, DefaultIgnore;
+def warn_possible_object_duplication_init : Warning<
+ "%0 has hidden visibility, and external linkage; its initialization may run "
+ "more than once when built into a shared library">,
+ InGroup, DefaultIgnore;
+
def ext_redefinition_of_typedef : ExtWarn<
"redefinition of typedef %0 is a C11 feature">,
InGroup >;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index d0fc735be5f763..2a92efb9f2fdd8 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3663,6 +3663,12 @@ class Sema final : public SemaBase {
NonTrivialCUnionContext UseContext,
unsigned NonTrivialKind);
+ /// Certain globally-unique variables might be accidentally duplicated if
+ /// built into multiple shared libraries with hidden visibility. This can
+ /// cause problems if the variable is mutable, its initialization is
+
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
@@ -13374,6 +13374,62 @@ void Sema::checkNonTrivialCUnion(QualType QT,
SourceLocation Loc,
.visit(QT, nullptr, false);
}
+bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
+const VarDecl *Dcl) {
+ if (!Dcl || !getLangOpts().CPlusPlus)
DKLoehr wrote:
Ok, changed.
https://github.com/llvm/llvm-project/pull/117622
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
DKLoehr wrote: I've now added documentation for this warning explaining what it means and how to resolve it. I also added a release note mentioning its existence. https://github.com/llvm/llvm-project/pull/117622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/117622
>From 5ef8b8f3b84133ac7501331bf9b86b0b2f8b9ed9 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 21 Nov 2024 19:29:00 +
Subject: [PATCH 1/3] Warn when unique objects might be duplicated in shared
libraries
When a hidden object is built into multiple shared libraries, each
instance of the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global
variable or static member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated), and
- Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem if one of the following is true:
1. The object is mutable (the copies won't be in sync), or
2. Its initialization has side effects (it may now run more than once), or
3. The value of its address is used (which one?).
To detect this, we add a new -Wunique-object-duplication warning.
It warns on cases (1) and (2) above. To be conservative, we only
warn in case (2) if we are certain the initializer has side effects,
and we don't warn on `new` because the only side effect is some
extra memory usage.
We don't currently warn on case (3) because doing so is prone to
false positives: there are many reasons for taking the address which
aren't inherently problematic (e.g. passing to a function that expects
a pointer). We only run into problems if the code inspects the value
of the address.
The check is currently disabled for windows, which uses its own analogue
of visibility (declimport/declexport). The check is also disabled inside
templates, since it can give false positives if a template is never
instantiated.
Because the warning fires in several places, included third-party
libraries, it is currently disabled-by-default until all these
instances have been fixed. It should be enabled-by-default afterwards.
---
clang/include/clang/Basic/DiagnosticGroups.td | 1 +
.../clang/Basic/DiagnosticSemaKinds.td| 9 +
clang/include/clang/Sema/Sema.h | 6 +
clang/lib/Sema/SemaDecl.cpp | 101 ++
.../SemaCXX/unique_object_duplication.cpp | 16 ++
.../test/SemaCXX/unique_object_duplication.h | 187 ++
6 files changed, 320 insertions(+)
create mode 100644 clang/test/SemaCXX/unique_object_duplication.cpp
create mode 100644 clang/test/SemaCXX/unique_object_duplication.h
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index b0ad76026fdb35..26bac212075261 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -696,6 +696,7 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
+def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
// Allow differentiation between GNU statement expressions in a macro versus
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index db54312ad965e8..1f25961145d161 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6142,6 +6142,15 @@ def warn_static_local_in_extern_inline : Warning<
def note_convert_inline_to_static : Note<
"use 'static' to give inline function %0 internal linkage">;
+def warn_possible_object_duplication_mutable : Warning<
+ "%0 is mutable, has hidden visibility, and external linkage; it may be "
+ "duplicated when built into a shared library">,
+ InGroup, DefaultIgnore;
+def warn_possible_object_duplication_init : Warning<
+ "%0 has hidden visibility, and external linkage; its initialization may run "
+ "more than once when built into a shared library">,
+ InGroup, DefaultIgnore;
+
def ext_redefinition_of_typedef : ExtWarn<
"redefinition of typedef %0 is a C11 feature">,
InGroup >;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index a41f16f6dc8c9b..26edffc150c65f 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3661,6 +3661,12 @@ class Sema final : public SemaBase {
NonTrivialCUnionContext UseContext,
unsigned NonTrivialKind);
+ /// Certain globally-unique variables might be accidentally duplicated if
+ /// built into multiple shared libraries with hidden visibility. This can
+ /// cause problems if the variable is mutable, its initialization is
+
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/117622
>From 5ef8b8f3b84133ac7501331bf9b86b0b2f8b9ed9 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 21 Nov 2024 19:29:00 +
Subject: [PATCH 1/2] Warn when unique objects might be duplicated in shared
libraries
When a hidden object is built into multiple shared libraries, each
instance of the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global
variable or static member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated), and
- Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem if one of the following is true:
1. The object is mutable (the copies won't be in sync), or
2. Its initialization has side effects (it may now run more than once), or
3. The value of its address is used (which one?).
To detect this, we add a new -Wunique-object-duplication warning.
It warns on cases (1) and (2) above. To be conservative, we only
warn in case (2) if we are certain the initializer has side effects,
and we don't warn on `new` because the only side effect is some
extra memory usage.
We don't currently warn on case (3) because doing so is prone to
false positives: there are many reasons for taking the address which
aren't inherently problematic (e.g. passing to a function that expects
a pointer). We only run into problems if the code inspects the value
of the address.
The check is currently disabled for windows, which uses its own analogue
of visibility (declimport/declexport). The check is also disabled inside
templates, since it can give false positives if a template is never
instantiated.
Because the warning fires in several places, included third-party
libraries, it is currently disabled-by-default until all these
instances have been fixed. It should be enabled-by-default afterwards.
---
clang/include/clang/Basic/DiagnosticGroups.td | 1 +
.../clang/Basic/DiagnosticSemaKinds.td| 9 +
clang/include/clang/Sema/Sema.h | 6 +
clang/lib/Sema/SemaDecl.cpp | 101 ++
.../SemaCXX/unique_object_duplication.cpp | 16 ++
.../test/SemaCXX/unique_object_duplication.h | 187 ++
6 files changed, 320 insertions(+)
create mode 100644 clang/test/SemaCXX/unique_object_duplication.cpp
create mode 100644 clang/test/SemaCXX/unique_object_duplication.h
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index b0ad76026fdb35..26bac212075261 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -696,6 +696,7 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
+def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
// Allow differentiation between GNU statement expressions in a macro versus
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index db54312ad965e8..1f25961145d161 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6142,6 +6142,15 @@ def warn_static_local_in_extern_inline : Warning<
def note_convert_inline_to_static : Note<
"use 'static' to give inline function %0 internal linkage">;
+def warn_possible_object_duplication_mutable : Warning<
+ "%0 is mutable, has hidden visibility, and external linkage; it may be "
+ "duplicated when built into a shared library">,
+ InGroup, DefaultIgnore;
+def warn_possible_object_duplication_init : Warning<
+ "%0 has hidden visibility, and external linkage; its initialization may run "
+ "more than once when built into a shared library">,
+ InGroup, DefaultIgnore;
+
def ext_redefinition_of_typedef : ExtWarn<
"redefinition of typedef %0 is a C11 feature">,
InGroup >;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index a41f16f6dc8c9b..26edffc150c65f 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3661,6 +3661,12 @@ class Sema final : public SemaBase {
NonTrivialCUnionContext UseContext,
unsigned NonTrivialKind);
+ /// Certain globally-unique variables might be accidentally duplicated if
+ /// built into multiple shared libraries with hidden visibility. This can
+ /// cause problems if the variable is mutable, its initialization is
+
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
DKLoehr wrote: > Can you share a few of the examples where it triggers, to get a feel for what > this looks like on the LLVM code? There's a text file linked in the initial description ([here](https://github.com/user-attachments/files/18563888/clang-warnings.txt), for convenience) that summarizes the warnings I examined in clang itself. I also found a few more in other parts of the LLVM project before giving up on whack-a-mole and just turning the warning off: ``` | /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-78z5r-1/llvm-project/github-pull-requests/build-runtimes/libcxxabi/test-suite-install/include/c++/v1/__algorithm/shuffle.h:59:17: error: '__x' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library [-Werror,-Wunique-object-duplication] # |59 | static char __x; ``` ``` /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-5wwkv-1/llvm-project/github-pull-requests/third-party/benchmark/src/log.h:59:14: error: 'log_level' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library [-Werror,-Wunique-object-duplication] 59 | static int log_level = 0; | ^ /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-5wwkv-1/llvm-project/github-pull-requests/third-party/benchmark/src/log.h:64:18: error: 'null_log' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library [-Werror,-Wunique-object-duplication] 64 | static LogType null_log(static_cast(nullptr)); | ^ /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-5wwkv-1/llvm-project/github-pull-requests/third-party/benchmark/src/log.h:69:18: error: 'error_log' is mutable, has hidden visibility, and external linkage; it may be duplicated when built into a shared library [-Werror,-Wunique-object-duplication] 69 | static LogType error_log(&std::clog); | ^ ``` https://github.com/llvm/llvm-project/pull/117622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/117622
>From 798b3f21593499194487c9877b8f4a3d9e44bb6e Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 21 Nov 2024 19:29:00 +
Subject: [PATCH 1/4] Warn when unique objects might be duplicated in shared
libraries
When a hidden object is built into multiple shared libraries, each
instance of the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global
variable or static member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated), and
- Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem if one of the following is true:
1. The object is mutable (the copies won't be in sync), or
2. Its initialization has side effects (it may now run more than once), or
3. The value of its address is used (which one?).
To detect this, we add a new -Wunique-object-duplication warning.
It warns on cases (1) and (2) above. To be conservative, we only
warn in case (2) if we are certain the initializer has side effects,
and we don't warn on `new` because the only side effect is some
extra memory usage.
We don't currently warn on case (3) because doing so is prone to
false positives: there are many reasons for taking the address which
aren't inherently problematic (e.g. passing to a function that expects
a pointer). We only run into problems if the code inspects the value
of the address.
The check is currently disabled for windows, which uses its own analogue
of visibility (declimport/declexport). The check is also disabled inside
templates, since it can give false positives if a template is never
instantiated.
Because the warning fires in several places, included third-party
libraries, it is currently disabled-by-default until all these
instances have been fixed. It should be enabled-by-default afterwards.
---
clang/include/clang/Basic/DiagnosticGroups.td | 1 +
.../clang/Basic/DiagnosticSemaKinds.td| 9 +
clang/include/clang/Sema/Sema.h | 6 +
clang/lib/Sema/SemaDecl.cpp | 101 ++
.../SemaCXX/unique_object_duplication.cpp | 16 ++
.../test/SemaCXX/unique_object_duplication.h | 187 ++
6 files changed, 320 insertions(+)
create mode 100644 clang/test/SemaCXX/unique_object_duplication.cpp
create mode 100644 clang/test/SemaCXX/unique_object_duplication.h
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index 209792f851b6ae..7919680930d396 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -696,6 +696,7 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
+def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
// Allow differentiation between GNU statement expressions in a macro versus
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index db911ed121e951..f7bfda5c40be00 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6162,6 +6162,15 @@ def warn_static_local_in_extern_inline : Warning<
def note_convert_inline_to_static : Note<
"use 'static' to give inline function %0 internal linkage">;
+def warn_possible_object_duplication_mutable : Warning<
+ "%0 is mutable, has hidden visibility, and external linkage; it may be "
+ "duplicated when built into a shared library">,
+ InGroup, DefaultIgnore;
+def warn_possible_object_duplication_init : Warning<
+ "%0 has hidden visibility, and external linkage; its initialization may run "
+ "more than once when built into a shared library">,
+ InGroup, DefaultIgnore;
+
def ext_redefinition_of_typedef : ExtWarn<
"redefinition of typedef %0 is a C11 feature">,
InGroup >;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index d0fc735be5f763..2a92efb9f2fdd8 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3663,6 +3663,12 @@ class Sema final : public SemaBase {
NonTrivialCUnionContext UseContext,
unsigned NonTrivialKind);
+ /// Certain globally-unique variables might be accidentally duplicated if
+ /// built into multiple shared libraries with hidden visibility. This can
+ /// cause problems if the variable is mutable, its initialization is
+
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
@@ -0,0 +1,187 @@
+/**
+ * When building shared libraries, hidden objects which are defined in header
+ * files will be duplicated, with one copy in each shared library. If the
object
+ * was meant to be globally unique (one copy per program), this can cause very
+ * subtle bugs. This file contains tests for the -Wunique-object-duplication
+ * warning, which is meant to detect this.
+ *
+ * Roughly, an object might be incorrectly duplicated if:
+ * - Is defined in a header (so it might appear in multiple TUs), and
+ * - Has external linkage (otherwise it's supposed to be duplicated), and
+ * - Has hidden visibility (or else the dynamic linker will handle it)
+ *
+ * Duplication becomes an issue only if one of the following is true:
+ * - The object is mutable (the copies won't be in sync), or
+ * - Its initialization may has side effects (it may now run more than once),
or
+ * - The value of its address is used.
+ *
+ * Currently, we only detect the first two, and only warn on effectful
+ * initialization if we're certain there are side effects. Warning if the
+ * address is taken is prone to false positives, so we don't warn for now.
+ *
+ * The check is also disabled on Windows for now, since it uses
+ * dllimport/dllexport instead of visibility.
+ */
+
+#define HIDDEN __attribute__((visibility("hidden")))
+#define DEFAULT __attribute__((visibility("default")))
+
+// Helper functions
+constexpr int init_constexpr(int x) { return x; };
+extern double init_dynamic(int);
+
+/**
+ * Case one: Static local variables in an externally-visible function
+
**/
+namespace StaticLocalTest {
+
+inline void has_static_locals_external() {
+ // Mutable
+ static int disallowedStatic1 = 0; // hidden-warning {{'disallowedStatic1' is
mutable, has hidden visibility, and external linkage; it may be duplicated when
built into a shared library}}
+ // Initialization might run more than once
+ static const double disallowedStatic2 = disallowedStatic1++; //
hidden-warning {{'disallowedStatic2' has hidden visibility, and external
linkage; its initialization may run more than once when built into a shared
library}}
+
+ // OK, because immutable and compile-time-initialized
+ static constexpr int allowedStatic1 = 0;
+ static const float allowedStatic2 = 1;
+ static constexpr int allowedStatic3 = init_constexpr(2);
+ static const int allowedStatic4 = init_constexpr(3);
+}
+
+// Don't warn for non-inline functions, since they can't (legally) appear
+// in more than one TU in the first place.
+void has_static_locals_non_inline() {
+ // Mutable
+ static int allowedStatic1 = 0;
+ // Initialization might run more than once
+ static const double allowedStatic2 = allowedStatic1++;
+}
+
+// Everything in this function is OK because the function is TU-local
+static void has_static_locals_internal() {
+ static int allowedStatic1 = 0;
+ static double allowedStatic2 = init_dynamic(2);
+ static char allowedStatic3 = []() { return allowedStatic1++; }();
+
+ static constexpr int allowedStatic4 = 0;
+ static const float allowedStatic5 = 1;
+ static constexpr int allowedStatic6 = init_constexpr(2);
+ static const int allowedStatic7 = init_constexpr(3);
+}
+
+namespace {
+
+// Everything in this function is OK because the function is also TU-local
+void has_static_locals_anon() {
+ static int allowedStatic1 = 0;
+ static double allowedStatic2 = init_dynamic(2);
+ static char allowedStatic3 = []() { return allowedStatic1++; }();
+
+ static constexpr int allowedStatic4 = 0;
+ static const float allowedStatic5 = 1;
+ static constexpr int allowedStatic6 = init_constexpr(2);
+ static const int allowedStatic7 = init_constexpr(3);
+}
+
+} // Anonymous namespace
+
+HIDDEN inline void static_local_always_hidden() {
+static int disallowedStatic1 = 3; // hidden-warning {{'disallowedStatic1'
is mutable, has hidden visibility, and external linkage; it may be duplicated
when built into a shared library}}
+ // expected-warning@-1
{{'disallowedStatic1' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+{
+ static int disallowedStatic2 = 3; // hidden-warning
{{'disallowedStatic2' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+// expected-warning@-1
{{'disallowedStatic2' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+}
+
+auto lmb = []() {
+ static int disallowedStatic3 = 3; // hidden-warning
{{'disallowedStatic3' is mutable, has hidden visibility, and external linkage;
it may be duplicated when built into a shared library}}
+
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
@@ -13374,6 +13374,62 @@ void Sema::checkNonTrivialCUnion(QualType QT,
SourceLocation Loc,
.visit(QT, nullptr, false);
}
+bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
+const VarDecl *Dcl) {
+ if (!Dcl || !getLangOpts().CPlusPlus)
DKLoehr wrote:
It shouldn't be possible, but I figured it was better to check it explicitly to
be safe.
https://github.com/llvm/llvm-project/pull/117622
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
@@ -0,0 +1,187 @@ +/** + * When building shared libraries, hidden objects which are defined in header + * files will be duplicated, with one copy in each shared library. If the object + * was meant to be globally unique (one copy per program), this can cause very + * subtle bugs. This file contains tests for the -Wunique-object-duplication + * warning, which is meant to detect this. + * + * Roughly, an object might be incorrectly duplicated if: + * - Is defined in a header (so it might appear in multiple TUs), and + * - Has external linkage (otherwise it's supposed to be duplicated), and + * - Has hidden visibility (or else the dynamic linker will handle it) + * + * Duplication becomes an issue only if one of the following is true: + * - The object is mutable (the copies won't be in sync), or + * - Its initialization may has side effects (it may now run more than once), or + * - The value of its address is used. + * + * Currently, we only detect the first two, and only warn on effectful + * initialization if we're certain there are side effects. Warning if the + * address is taken is prone to false positives, so we don't warn for now. + * + * The check is also disabled on Windows for now, since it uses + * dllimport/dllexport instead of visibility. + */ DKLoehr wrote: Fair enough; I always want more explanation when I'm looking at things, but now that it's explicitly documented we can just point to that. https://github.com/llvm/llvm-project/pull/117622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
@@ -6153,6 +6153,15 @@ def warn_static_local_in_extern_inline : Warning< def note_convert_inline_to_static : Note< "use 'static' to give inline function %0 internal linkage">; +def warn_possible_object_duplication_mutable : Warning< + "%0 is mutable, has hidden visibility, and external linkage; it may be " DKLoehr wrote: I like this more. https://github.com/llvm/llvm-project/pull/117622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
@@ -0,0 +1,187 @@
+/**
+ * When building shared libraries, hidden objects which are defined in header
+ * files will be duplicated, with one copy in each shared library. If the
object
+ * was meant to be globally unique (one copy per program), this can cause very
+ * subtle bugs. This file contains tests for the -Wunique-object-duplication
+ * warning, which is meant to detect this.
+ *
+ * Roughly, an object might be incorrectly duplicated if:
+ * - Is defined in a header (so it might appear in multiple TUs), and
+ * - Has external linkage (otherwise it's supposed to be duplicated), and
+ * - Has hidden visibility (or else the dynamic linker will handle it)
+ *
+ * Duplication becomes an issue only if one of the following is true:
+ * - The object is mutable (the copies won't be in sync), or
+ * - Its initialization may has side effects (it may now run more than once),
or
+ * - The value of its address is used.
+ *
+ * Currently, we only detect the first two, and only warn on effectful
+ * initialization if we're certain there are side effects. Warning if the
+ * address is taken is prone to false positives, so we don't warn for now.
+ *
+ * The check is also disabled on Windows for now, since it uses
+ * dllimport/dllexport instead of visibility.
+ */
+
+#define HIDDEN __attribute__((visibility("hidden")))
+#define DEFAULT __attribute__((visibility("default")))
+
+// Helper functions
+constexpr int init_constexpr(int x) { return x; };
+extern double init_dynamic(int);
+
+/**
+ * Case one: Static local variables in an externally-visible function
+
**/
+namespace StaticLocalTest {
+
+inline void has_static_locals_external() {
+ // Mutable
+ static int disallowedStatic1 = 0; // hidden-warning {{'disallowedStatic1' is
mutable, has hidden visibility, and external linkage; it may be duplicated when
built into a shared library}}
+ // Initialization might run more than once
+ static const double disallowedStatic2 = disallowedStatic1++; //
hidden-warning {{'disallowedStatic2' has hidden visibility, and external
linkage; its initialization may run more than once when built into a shared
library}}
+
+ // OK, because immutable and compile-time-initialized
+ static constexpr int allowedStatic1 = 0;
+ static const float allowedStatic2 = 1;
+ static constexpr int allowedStatic3 = init_constexpr(2);
+ static const int allowedStatic4 = init_constexpr(3);
+}
+
+// Don't warn for non-inline functions, since they can't (legally) appear
+// in more than one TU in the first place.
+void has_static_locals_non_inline() {
+ // Mutable
+ static int allowedStatic1 = 0;
+ // Initialization might run more than once
+ static const double allowedStatic2 = allowedStatic1++;
+}
+
+// Everything in this function is OK because the function is TU-local
+static void has_static_locals_internal() {
+ static int allowedStatic1 = 0;
+ static double allowedStatic2 = init_dynamic(2);
+ static char allowedStatic3 = []() { return allowedStatic1++; }();
+
+ static constexpr int allowedStatic4 = 0;
+ static const float allowedStatic5 = 1;
+ static constexpr int allowedStatic6 = init_constexpr(2);
+ static const int allowedStatic7 = init_constexpr(3);
+}
+
+namespace {
+
+// Everything in this function is OK because the function is also TU-local
+void has_static_locals_anon() {
+ static int allowedStatic1 = 0;
+ static double allowedStatic2 = init_dynamic(2);
+ static char allowedStatic3 = []() { return allowedStatic1++; }();
+
+ static constexpr int allowedStatic4 = 0;
+ static const float allowedStatic5 = 1;
+ static constexpr int allowedStatic6 = init_constexpr(2);
+ static const int allowedStatic7 = init_constexpr(3);
DKLoehr wrote:
In this case the lines are slightly different. It's probably redundant but I
don't think it hurts to check the different variations (const/constexpr,
literal/constexpr initialization).
https://github.com/llvm/llvm-project/pull/117622
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
https://github.com/DKLoehr edited https://github.com/llvm/llvm-project/pull/117622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
@@ -0,0 +1,187 @@
+/**
+ * When building shared libraries, hidden objects which are defined in header
+ * files will be duplicated, with one copy in each shared library. If the
object
+ * was meant to be globally unique (one copy per program), this can cause very
+ * subtle bugs. This file contains tests for the -Wunique-object-duplication
+ * warning, which is meant to detect this.
+ *
+ * Roughly, an object might be incorrectly duplicated if:
+ * - Is defined in a header (so it might appear in multiple TUs), and
+ * - Has external linkage (otherwise it's supposed to be duplicated), and
+ * - Has hidden visibility (or else the dynamic linker will handle it)
+ *
+ * Duplication becomes an issue only if one of the following is true:
+ * - The object is mutable (the copies won't be in sync), or
+ * - Its initialization may has side effects (it may now run more than once),
or
+ * - The value of its address is used.
+ *
+ * Currently, we only detect the first two, and only warn on effectful
+ * initialization if we're certain there are side effects. Warning if the
+ * address is taken is prone to false positives, so we don't warn for now.
+ *
+ * The check is also disabled on Windows for now, since it uses
+ * dllimport/dllexport instead of visibility.
+ */
+
+#define HIDDEN __attribute__((visibility("hidden")))
+#define DEFAULT __attribute__((visibility("default")))
+
+// Helper functions
+constexpr int init_constexpr(int x) { return x; };
+extern double init_dynamic(int);
+
+/**
+ * Case one: Static local variables in an externally-visible function
+
**/
+namespace StaticLocalTest {
+
+inline void has_static_locals_external() {
+ // Mutable
+ static int disallowedStatic1 = 0; // hidden-warning {{'disallowedStatic1' is
mutable, has hidden visibility, and external linkage; it may be duplicated when
built into a shared library}}
+ // Initialization might run more than once
+ static const double disallowedStatic2 = disallowedStatic1++; //
hidden-warning {{'disallowedStatic2' has hidden visibility, and external
linkage; its initialization may run more than once when built into a shared
library}}
+
+ // OK, because immutable and compile-time-initialized
+ static constexpr int allowedStatic1 = 0;
+ static const float allowedStatic2 = 1;
+ static constexpr int allowedStatic3 = init_constexpr(2);
+ static const int allowedStatic4 = init_constexpr(3);
+}
+
+// Don't warn for non-inline functions, since they can't (legally) appear
+// in more than one TU in the first place.
+void has_static_locals_non_inline() {
+ // Mutable
+ static int allowedStatic1 = 0;
+ // Initialization might run more than once
+ static const double allowedStatic2 = allowedStatic1++;
+}
+
+// Everything in this function is OK because the function is TU-local
+static void has_static_locals_internal() {
+ static int allowedStatic1 = 0;
+ static double allowedStatic2 = init_dynamic(2);
+ static char allowedStatic3 = []() { return allowedStatic1++; }();
+
+ static constexpr int allowedStatic4 = 0;
+ static const float allowedStatic5 = 1;
+ static constexpr int allowedStatic6 = init_constexpr(2);
+ static const int allowedStatic7 = init_constexpr(3);
DKLoehr wrote:
Makes sense. I'll keep one around just to make sure it doesn't warn, but the
rest are probably redundant.
https://github.com/llvm/llvm-project/pull/117622
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
@@ -13386,6 +13386,62 @@ void Sema::checkNonTrivialCUnion(QualType QT,
SourceLocation Loc,
.visit(QT, nullptr, false);
}
+bool Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
+const VarDecl *dcl) {
+ if (!dcl || !getLangOpts().CPlusPlus)
DKLoehr wrote:
C has different rules about this kind of thing, and we already have a [very
similar
check](https://github.com/llvm/llvm-project/blob/edf3a55bcecc8b0441a7a5fe6bda2023f86667a3/clang/include/clang/Basic/DiagnosticSemaKinds.td#L6133)
for enforcing them.
https://github.com/llvm/llvm-project/pull/117622
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/117622
>From 5ef8b8f3b84133ac7501331bf9b86b0b2f8b9ed9 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 21 Nov 2024 19:29:00 +
Subject: [PATCH 1/6] Warn when unique objects might be duplicated in shared
libraries
When a hidden object is built into multiple shared libraries, each
instance of the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global
variable or static member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated), and
- Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem if one of the following is true:
1. The object is mutable (the copies won't be in sync), or
2. Its initialization has side effects (it may now run more than once), or
3. The value of its address is used (which one?).
To detect this, we add a new -Wunique-object-duplication warning.
It warns on cases (1) and (2) above. To be conservative, we only
warn in case (2) if we are certain the initializer has side effects,
and we don't warn on `new` because the only side effect is some
extra memory usage.
We don't currently warn on case (3) because doing so is prone to
false positives: there are many reasons for taking the address which
aren't inherently problematic (e.g. passing to a function that expects
a pointer). We only run into problems if the code inspects the value
of the address.
The check is currently disabled for windows, which uses its own analogue
of visibility (declimport/declexport). The check is also disabled inside
templates, since it can give false positives if a template is never
instantiated.
Because the warning fires in several places, included third-party
libraries, it is currently disabled-by-default until all these
instances have been fixed. It should be enabled-by-default afterwards.
---
clang/include/clang/Basic/DiagnosticGroups.td | 1 +
.../clang/Basic/DiagnosticSemaKinds.td| 9 +
clang/include/clang/Sema/Sema.h | 6 +
clang/lib/Sema/SemaDecl.cpp | 101 ++
.../SemaCXX/unique_object_duplication.cpp | 16 ++
.../test/SemaCXX/unique_object_duplication.h | 187 ++
6 files changed, 320 insertions(+)
create mode 100644 clang/test/SemaCXX/unique_object_duplication.cpp
create mode 100644 clang/test/SemaCXX/unique_object_duplication.h
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index b0ad76026fdb35..26bac212075261 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -696,6 +696,7 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
+def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
// Allow differentiation between GNU statement expressions in a macro versus
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index db54312ad965e8..1f25961145d161 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6142,6 +6142,15 @@ def warn_static_local_in_extern_inline : Warning<
def note_convert_inline_to_static : Note<
"use 'static' to give inline function %0 internal linkage">;
+def warn_possible_object_duplication_mutable : Warning<
+ "%0 is mutable, has hidden visibility, and external linkage; it may be "
+ "duplicated when built into a shared library">,
+ InGroup, DefaultIgnore;
+def warn_possible_object_duplication_init : Warning<
+ "%0 has hidden visibility, and external linkage; its initialization may run "
+ "more than once when built into a shared library">,
+ InGroup, DefaultIgnore;
+
def ext_redefinition_of_typedef : ExtWarn<
"redefinition of typedef %0 is a C11 feature">,
InGroup >;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index a41f16f6dc8c9b..26edffc150c65f 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3661,6 +3661,12 @@ class Sema final : public SemaBase {
NonTrivialCUnionContext UseContext,
unsigned NonTrivialKind);
+ /// Certain globally-unique variables might be accidentally duplicated if
+ /// built into multiple shared libraries with hidden visibility. This can
+ /// cause problems if the variable is mutable, its initialization is
+
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/117622
>From 5ef8b8f3b84133ac7501331bf9b86b0b2f8b9ed9 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 21 Nov 2024 19:29:00 +
Subject: [PATCH 1/2] Warn when unique objects might be duplicated in shared
libraries
When a hidden object is built into multiple shared libraries, each
instance of the library will get its own copy. If
the object was supposed to be globally unique (e.g. a global
variable or static member), this can cause very subtle bugs.
An object might be incorrectly duplicated if it:
- Is defined in a header (so it might appear in multiple TUs), and
- Has external linkage (otherwise it's supposed to be duplicated), and
- Has hidden visibility (or else the dynamic linker will handle it)
The duplication is only a problem if one of the following is true:
1. The object is mutable (the copies won't be in sync), or
2. Its initialization has side effects (it may now run more than once), or
3. The value of its address is used (which one?).
To detect this, we add a new -Wunique-object-duplication warning.
It warns on cases (1) and (2) above. To be conservative, we only
warn in case (2) if we are certain the initializer has side effects,
and we don't warn on `new` because the only side effect is some
extra memory usage.
We don't currently warn on case (3) because doing so is prone to
false positives: there are many reasons for taking the address which
aren't inherently problematic (e.g. passing to a function that expects
a pointer). We only run into problems if the code inspects the value
of the address.
The check is currently disabled for windows, which uses its own analogue
of visibility (declimport/declexport). The check is also disabled inside
templates, since it can give false positives if a template is never
instantiated.
Because the warning fires in several places, included third-party
libraries, it is currently disabled-by-default until all these
instances have been fixed. It should be enabled-by-default afterwards.
---
clang/include/clang/Basic/DiagnosticGroups.td | 1 +
.../clang/Basic/DiagnosticSemaKinds.td| 9 +
clang/include/clang/Sema/Sema.h | 6 +
clang/lib/Sema/SemaDecl.cpp | 101 ++
.../SemaCXX/unique_object_duplication.cpp | 16 ++
.../test/SemaCXX/unique_object_duplication.h | 187 ++
6 files changed, 320 insertions(+)
create mode 100644 clang/test/SemaCXX/unique_object_duplication.cpp
create mode 100644 clang/test/SemaCXX/unique_object_duplication.h
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index b0ad76026fdb35..26bac212075261 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -696,6 +696,7 @@ def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
+def UniqueObjectDuplication : DiagGroup<"unique-object-duplication">;
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
def StaticFloatInit : DiagGroup<"static-float-init", [GNUStaticFloatInit]>;
// Allow differentiation between GNU statement expressions in a macro versus
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index db54312ad965e8..1f25961145d161 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6142,6 +6142,15 @@ def warn_static_local_in_extern_inline : Warning<
def note_convert_inline_to_static : Note<
"use 'static' to give inline function %0 internal linkage">;
+def warn_possible_object_duplication_mutable : Warning<
+ "%0 is mutable, has hidden visibility, and external linkage; it may be "
+ "duplicated when built into a shared library">,
+ InGroup, DefaultIgnore;
+def warn_possible_object_duplication_init : Warning<
+ "%0 has hidden visibility, and external linkage; its initialization may run "
+ "more than once when built into a shared library">,
+ InGroup, DefaultIgnore;
+
def ext_redefinition_of_typedef : ExtWarn<
"redefinition of typedef %0 is a C11 feature">,
InGroup >;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index a41f16f6dc8c9b..26edffc150c65f 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3661,6 +3661,12 @@ class Sema final : public SemaBase {
NonTrivialCUnionContext UseContext,
unsigned NonTrivialKind);
+ /// Certain globally-unique variables might be accidentally duplicated if
+ /// built into multiple shared libraries with hidden visibility. This can
+ /// cause problems if the variable is mutable, its initialization is
+
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
DKLoehr wrote: @zmodem Thanks for the review! I've addressed the code comments. To your questions: > What about code that's not going into a shared library? My impression is that you wouldn't mark things as having hidden visibility if they're not going to a shared library at least some of the time. That said, if there's a way to check if something is being compiled into a shared library, it would make sense to add it as a condition for the warning. > The new warning is DefaultIgnore. I intend to make the warning on-by-default. However, it currently triggers at several places in the LLVM project, breaking the CI. Fixing them is nontrivial and cluttered up the PR, so I made it DefaultIgnore in order to keep things focused. Should the PR be accepted I plan to go through and clean things up, then default-enable the warning. > Would warning during instantiation work? That's the most likely solution! I need to look into it, but haven't had time yet. Since the warning is useful even without templates, I didn't think doing so should block making the PR. https://github.com/llvm/llvm-project/pull/117622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn when unique objects might be duplicated in shared libraries (PR #117622)
DKLoehr wrote: If a review was requested from you and this code isn't related, apologies: For some reason attempting to rebase onto the current state of the repo ended up including a bunch of random extra commits that aren't related, and that resulted in a bunch of people automatically being added as reviewers that I can't remove. Sorry for the inconvenience. https://github.com/llvm/llvm-project/pull/117622 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Remove xbegin and _xend (PR #126952)
https://github.com/DKLoehr created https://github.com/llvm/llvm-project/pull/126952 `intrin.h` contains declarations for both `xbegin` and `_xend`, but they should already be included transitively from `rtmintrin.h` via `immintrin.h` and/or `x86intrin.h`. Having them in both places causes problems if both headers are included. Furthermore, the `intrin.h` declaration of `xbegin` seems to be bugged anyway, since it's missing its leading underscore. Fixes #95133 >From 748451908cc46416dab8ec427e8df656e61d7d94 Mon Sep 17 00:00:00 2001 From: Devon Loehr Date: Wed, 12 Feb 2025 18:17:05 + Subject: [PATCH] Remove xbegin and _xend --- clang/lib/Headers/intrin.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/clang/lib/Headers/intrin.h b/clang/lib/Headers/intrin.h index 376046aeeaf5e..3dd1eb45817d4 100644 --- a/clang/lib/Headers/intrin.h +++ b/clang/lib/Headers/intrin.h @@ -162,8 +162,6 @@ void _Store_HLERelease(long volatile *, long); void _Store64_HLERelease(__int64 volatile *, __int64); void _StorePointer_HLERelease(void *volatile *, void *); void _WriteBarrier(void); -unsigned __int32 xbegin(void); -void _xend(void); /* These additional intrinsics are turned on in x64/amd64/x86_64 mode. */ #if defined(__x86_64__) && !defined(__arm64ec__) ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Remove xbegin and _xend (PR #126952)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/126952
>From 748451908cc46416dab8ec427e8df656e61d7d94 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Wed, 12 Feb 2025 18:17:05 +
Subject: [PATCH 1/2] Remove xbegin and _xend
---
clang/lib/Headers/intrin.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/clang/lib/Headers/intrin.h b/clang/lib/Headers/intrin.h
index 376046aeeaf5e..3dd1eb45817d4 100644
--- a/clang/lib/Headers/intrin.h
+++ b/clang/lib/Headers/intrin.h
@@ -162,8 +162,6 @@ void _Store_HLERelease(long volatile *, long);
void _Store64_HLERelease(__int64 volatile *, __int64);
void _StorePointer_HLERelease(void *volatile *, void *);
void _WriteBarrier(void);
-unsigned __int32 xbegin(void);
-void _xend(void);
/* These additional intrinsics are turned on in x64/amd64/x86_64 mode. */
#if defined(__x86_64__) && !defined(__arm64ec__)
>From 92723c431c8b09f92d416714244ca1104985e3e8 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 13 Feb 2025 15:35:02 +
Subject: [PATCH 2/2] Add regression test
---
clang/test/Headers/no-xend.cpp | 10 ++
1 file changed, 10 insertions(+)
create mode 100644 clang/test/Headers/no-xend.cpp
diff --git a/clang/test/Headers/no-xend.cpp b/clang/test/Headers/no-xend.cpp
new file mode 100644
index 0..31e6b25a27e81
--- /dev/null
+++ b/clang/test/Headers/no-xend.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang -Xclang -triple=x86_64-pc-win32 \
+// RUN: -fms-extensions -fms-compatibility
-fms-compatibility-version=17.00 \
+// RUN: -ffreestanding -fsyntax-only -Werror -Wsystem-headers \
+// RUN: -isystem %S/Inputs/include %s
+
+#include
+
+#pragma clang attribute push(__attribute__((target("avx"))), apply_to=function)
+#include
+#pragma clang attribute pop
\ No newline at end of file
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Remove xbegin and _xend (PR #126952)
DKLoehr wrote: Added a test based on the linked bug. I couldn't get it to reproduce in anything that seemed to fit in the existing file, so I made a new one. https://github.com/llvm/llvm-project/pull/126952 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Remove xbegin and _xend (PR #126952)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/126952
>From 01e40e4d8b3cc2068aa5a022c3acae246b6ac771 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Wed, 12 Feb 2025 18:17:05 +
Subject: [PATCH 1/2] Remove xbegin and _xend
---
clang/lib/Headers/intrin.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/clang/lib/Headers/intrin.h b/clang/lib/Headers/intrin.h
index 376046aeeaf5e..3dd1eb45817d4 100644
--- a/clang/lib/Headers/intrin.h
+++ b/clang/lib/Headers/intrin.h
@@ -162,8 +162,6 @@ void _Store_HLERelease(long volatile *, long);
void _Store64_HLERelease(__int64 volatile *, __int64);
void _StorePointer_HLERelease(void *volatile *, void *);
void _WriteBarrier(void);
-unsigned __int32 xbegin(void);
-void _xend(void);
/* These additional intrinsics are turned on in x64/amd64/x86_64 mode. */
#if defined(__x86_64__) && !defined(__arm64ec__)
>From 76df571382385e77a589971cb0740a07bfb856ed Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 13 Feb 2025 15:35:02 +
Subject: [PATCH 2/2] Add regression test
---
clang/test/Headers/no-xend.cpp | 10 ++
1 file changed, 10 insertions(+)
create mode 100644 clang/test/Headers/no-xend.cpp
diff --git a/clang/test/Headers/no-xend.cpp b/clang/test/Headers/no-xend.cpp
new file mode 100644
index 0..b2fb1de011557
--- /dev/null
+++ b/clang/test/Headers/no-xend.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple x86_64-pc-win32 \
+// RUN: -fms-extensions -fms-compatibility
-fms-compatibility-version=17.00 \
+// RUN: -ffreestanding -fsyntax-only -Werror -Wsystem-headers \
+// RUN: -isystem %S/Inputs/include %s
+
+#include
+
+#pragma clang attribute push(__attribute__((target("avx"))), apply_to=function)
+#include
+#pragma clang attribute pop
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Check for mutability better (PR #127843)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/127843
>From 7a919e29b221f1070c420e263760b7071dc01da8 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 20 Feb 2025 15:19:13 +
Subject: [PATCH 1/3] Implement mutable check in Sema
---
clang/lib/Sema/SemaDecl.cpp | 35 +++
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 362df485a025c..3f3fea7e23814 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13440,6 +13440,23 @@ bool
Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
return true;
}
+// Determine whether the object seems mutable for the purpose of diagnosing
+// possible unique object duplication, i.e. non-const-qualified, and
+// not an always-constant type like a function.
+// Not perfect: doesn't account for mutable members, for example, or
+// elements of container types.
+// For nested pointers, any individual level being non-const is sufficient.
+bool looksMutable(QualType T, const ASTContext &Ctx) {
+ T = T.getNonReferenceType();
+ if (T->isFunctionType())
+return false;
+ if (!T.isConstant(Ctx))
+return true;
+ if (T->isPointerType())
+return looksMutable(T->getPointeeType(), Ctx);
+ return false;
+}
+
void Sema::DiagnoseUniqueObjectDuplication(const VarDecl *VD) {
// If this object has external linkage and hidden visibility, it might be
// duplicated when built into a shared library, which causes problems if it's
@@ -13454,24 +13471,10 @@ void Sema::DiagnoseUniqueObjectDuplication(const
VarDecl *VD) {
!VD->isTemplated() &&
GloballyUniqueObjectMightBeAccidentallyDuplicated(VD)) {
-// Check mutability. For pointers, ensure that both the pointer and the
-// pointee are (recursively) const.
-QualType Type = VD->getType().getNonReferenceType();
-if (!Type.isConstant(VD->getASTContext())) {
+QualType Type = VD->getType();
+if (looksMutable(Type, VD->getASTContext())) {
Diag(VD->getLocation(), diag::warn_possible_object_duplication_mutable)
<< VD;
-} else {
- while (Type->isPointerType()) {
-Type = Type->getPointeeType();
-if (Type->isFunctionType())
- break;
-if (!Type.isConstant(VD->getASTContext())) {
- Diag(VD->getLocation(),
- diag::warn_possible_object_duplication_mutable)
- << VD;
- break;
-}
- }
}
// To keep false positives low, only warn if we're certain that the
>From 2e9bafe0191e9c9c694d5bb70e188607daae8bf5 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 20 Feb 2025 15:20:06 +
Subject: [PATCH 2/3] Add test
---
clang/test/SemaCXX/unique_object_duplication.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/clang/test/SemaCXX/unique_object_duplication.h
b/clang/test/SemaCXX/unique_object_duplication.h
index a59a8f91da8b8..861175766db70 100644
--- a/clang/test/SemaCXX/unique_object_duplication.h
+++ b/clang/test/SemaCXX/unique_object_duplication.h
@@ -99,6 +99,9 @@ inline void has_thread_local() {
thread_local int disallowedThreadLocal = 0; // hidden-warning
{{'disallowedThreadLocal' may be duplicated when built into a shared library:
it is mutable, has hidden visibility, and external linkage}}
}
+// Functions themselves are always immutable, so referencing them is okay
+inline auto& allowedFunctionReference = has_static_locals_external;
+
} // namespace StaticLocalTest
/**
>From be5482201c6d5004cc0a2cf48d1941f45905f5e7 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Fri, 21 Feb 2025 15:18:29 +
Subject: [PATCH 3/3] Make function static
---
clang/lib/Sema/SemaDecl.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 3f3fea7e23814..7633651337aa3 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13446,7 +13446,7 @@ bool
Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
// Not perfect: doesn't account for mutable members, for example, or
// elements of container types.
// For nested pointers, any individual level being non-const is sufficient.
-bool looksMutable(QualType T, const ASTContext &Ctx) {
+static bool looksMutable(QualType T, const ASTContext &Ctx) {
T = T.getNonReferenceType();
if (T->isFunctionType())
return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Remove xbegin and _xend (PR #126952)
DKLoehr wrote: Just checking in, this was approved a few days ago but hasn't been merged yet. https://github.com/llvm/llvm-project/pull/126952 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Check for mutability better (PR #127843)
DKLoehr wrote: Good call, changed. https://github.com/llvm/llvm-project/pull/127843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Disable unique-object-duplication warning in templates (PR #129120)
https://github.com/DKLoehr created
https://github.com/llvm/llvm-project/pull/129120
I've been trying to resolve instances of the unique-object-duplication warning
in chromium code. Unfortunately, I've found that practically speaking, it's
near-impossible to actually fix the problem when templates are involved.
My understanding is that the warning is correct -- the variables it's flagging
are indeed duplicated and potentially causing bugs as a result. The problem is
that hiddenness is contagious: if a templated class or variable depends on
something hidden, then it itself must also be hidden, even if the user
explicitly marked it visible. In order to make it actually visible, the user
must manually figure out everything that it depends on, mark them as visible,
and do so recursively until all of its ancestors are visible.
This process is extremely difficult and unergonomic, negating much of the
benefits of templates since now each new use requires additional work.
Furthermore, the process doesn't work if the user can't edit some of the files,
e.g. if they're in a third-party library.
Since a warning that can't practically be fixed isn't useful, this PR disables
the warning for _all_ templated code by inverting the check. The warning
remains active (and, in my experience, easily fixable) in non-templated code.
>From a52c1e72885f7a93ef77f707cb56db3053d5bd34 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 27 Feb 2025 21:01:48 +
Subject: [PATCH] Disable UOD warning in templates
---
clang/lib/Sema/SemaDecl.cpp | 14 ++--
.../test/SemaCXX/unique_object_duplication.h | 76 ++-
2 files changed, 15 insertions(+), 75 deletions(-)
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 285bd27a35a76..86e65e56accc8 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13427,9 +13427,13 @@ bool
Sema::GloballyUniqueObjectMightBeAccidentallyDuplicated(
FunDcl->getTemplateSpecializationKind() != TSK_Undeclared;
}
- // Non-inline functions/variables can only legally appear in one TU,
- // unless they were part of a template.
- if (!TargetIsInline && !TargetWasTemplated)
+ // Non-inline functions/variables can only legally appear in one TU
+ // unless they were part of a template. Unfortunately, making complex
+ // template instantiations visible is infeasible in practice, since
+ // everything the template depends on also has to be visible. To avoid
+ // giving impractical-to-fix warnings, don't warn if we're inside
+ // something that was templated, even on inline stuff.
+ if (!TargetIsInline || TargetWasTemplated)
return false;
// If the object isn't hidden, the dynamic linker will prevent duplication.
@@ -13469,8 +13473,8 @@ void Sema::DiagnoseUniqueObjectDuplication(const
VarDecl *VD) {
// FIXME: Windows uses dllexport/dllimport instead of visibility, and we
don't
// handle that yet. Disable the warning on Windows for now.
- // Don't diagnose if we're inside a template;
- // we'll diagnose during instantiation instead.
+ // Don't diagnose if we're inside a template, because it's not practical to
+ // fix the warning in most cases.
if (!Context.getTargetInfo().shouldDLLImportComdatSymbols() &&
!VD->isTemplated() &&
GloballyUniqueObjectMightBeAccidentallyDuplicated(VD)) {
diff --git a/clang/test/SemaCXX/unique_object_duplication.h
b/clang/test/SemaCXX/unique_object_duplication.h
index 861175766db70..e5c63efbf918c 100644
--- a/clang/test/SemaCXX/unique_object_duplication.h
+++ b/clang/test/SemaCXX/unique_object_duplication.h
@@ -165,81 +165,17 @@ namespace GlobalTest {
namespace TemplateTest {
-template
-int disallowedTemplate1 = 0; // hidden-warning {{'disallowedTemplate1'
may be duplicated when built into a shared library: it is mutable, has hidden
visibility, and external linkage}}
-
-template int disallowedTemplate1; // hidden-note {{in instantiation of}}
-
-
-// Should work for implicit instantiation as well
-template
-int disallowedTemplate2 = 0; // hidden-warning {{'disallowedTemplate2'
may be duplicated when built into a shared library: it is mutable, has hidden
visibility, and external linkage}}
-
-int implicit_instantiate() {
- return disallowedTemplate2; // hidden-note {{in instantiation of}}
-}
-
+// We never warn inside templates because it's frequently infeasible to
actually
+// fix the warning.
-// Ensure we only get warnings for templates that are actually instantiated
template
-int maybeAllowedTemplate = 0; // Not instantiated, so no warning here
-
-template
-int maybeAllowedTemplate = 1; // hidden-warning
{{'maybeAllowedTemplate' may be duplicated when built into a shared
library: it is mutable, has hidden visibility, and external linkage}}
-
-template <>
-int maybeAllowedTemplate = 2; // hidden-warning
{{'maybeAllowedTemplate' may be duplicated when built into a shared
library: it is mutable, has hidden
[clang] Warn about virtual methods in `final` classes (PR #131188)
https://github.com/DKLoehr created
https://github.com/llvm/llvm-project/pull/131188
There's never any point to adding a `virtual` specifier to methods in a `final`
class, since the class can't be subclassed. This adds a warning when we notice
this happening, as suggested in #131108.
We don't currently implement the second part of the suggestion, to warn on
`virtual` methods which are never overridden anywhere. Although it's feasible
to do this for things with internal linkage (so we can check at the end of the
TU), it's more complicated to implement and it's not clear it's worth the
effort.
I tested the warning by compiling chromium and clang itself. Chromium resulted
in [277 warnings across 109
files](https://github.com/user-attachments/files/19234889/warnings-chromium.txt),
while clang had [38 warnings across 29
files](https://github.com/user-attachments/files/19234888/warnings-clang.txt).
I inspected a subset of the warning sites manually, and they all seemed
legitimate.
This warning is very easy to fix (just remove the `virtual` specifier), so it's
suitable for on-by-default. However, I've currently made it off-by-default
because it fires at several places in the repo. I plan to submit a followup PR
fixing those places and enabling the warning by default.
>From fbd474fb5ae3adeaf1644a4d44e916e4d7c66395 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 13 Mar 2025 17:34:27 +
Subject: [PATCH 1/3] Initial warning commit
---
clang/include/clang/Basic/DiagnosticGroups.td | 2 ++
.../clang/Basic/DiagnosticSemaKinds.td| 3 +++
clang/lib/Sema/SemaDeclCXX.cpp| 5
.../SemaCXX/unnecessary-virtual-specifier.cpp | 27 +++
4 files changed, 37 insertions(+)
create mode 100644 clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index fac80fb4009aa..65593ddcb8608 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -372,6 +372,8 @@ def CXX11WarnInconsistentOverrideMethod :
def CXX11WarnSuggestOverrideDestructor :
DiagGroup<"suggest-destructor-override">;
def CXX11WarnSuggestOverride : DiagGroup<"suggest-override">;
+def WarnUnnecessaryVirtualSpecifier :
DiagGroup<"unnecessary-virtual-specifier">;
+
// Original name of this warning in Clang
def : DiagGroup<"c++0x-narrowing", [CXX11Narrowing]>;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 8e6e6e892cdd7..a87cf7e674b4c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2706,6 +2706,9 @@ def warn_final_dtor_non_final_class : Warning<
InGroup;
def note_final_dtor_non_final_class_silence : Note<
"mark %0 as '%select{final|sealed}1' to silence this warning">;
+def warn_unnecessary_virtual_specifier : Warning<
+ "virtual method %0 is inside a 'final' class and can never be overridden">,
+ InGroup;
// C++11 attributes
def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index dd779ee377309..1b2e494956d4b 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -7193,10 +7193,15 @@ void Sema::CheckCompletedCXXClass(Scope *S,
CXXRecordDecl *Record) {
// class without overriding any.
if (!M->isStatic())
DiagnoseHiddenVirtualMethods(M);
+
if (M->hasAttr())
HasMethodWithOverrideControl = true;
else if (M->size_overridden_methods() > 0)
HasOverridingMethodWithoutOverrideControl = true;
+
+// See if a method is marked as virtual inside of a final class.
+if (M->isVirtualAsWritten() && Record->isEffectivelyFinal())
+ Diag(M->getLocation(), diag::warn_unnecessary_virtual_specifier) <<
M;
}
if (!isa(M))
diff --git a/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
new file mode 100644
index 0..eb8397d9ade45
--- /dev/null
+++ b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier
-Wsuggest-override %s
+
+struct Foo final {
+ Foo() = default;
+ virtual ~Foo() = default; // expected-warning
{{virtual method}}
+ virtual Foo& operator=(Foo& other) = default; // expected-warning
{{virtual method}}
+ virtual Foo& operator=(Foo&& other) = default; // expected-warning
{{virtual method}}
+ void f();
+ virtual void f(int); // expected-warning {{virtual
method}}
+ int g(int x) { return x; };
+ virtual int g(bool); // expected-warning {{virtual
method}}
+ static int s();
+};
+
+struct BarBase {
[clang] Warn about virtual methods in `final` classes (PR #131188)
https://github.com/DKLoehr edited https://github.com/llvm/llvm-project/pull/131188 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn about virtual methods in `final` classes (PR #131188)
DKLoehr wrote: Changed to not warn on `virtual...override`. I'm inclined to agree that it doesn't seem worth the effort to extend this to things that aren't actually overridden in practice. https://github.com/llvm/llvm-project/pull/131188 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn about virtual methods in `final` classes (PR #131188)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/131188
>From fbd474fb5ae3adeaf1644a4d44e916e4d7c66395 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 13 Mar 2025 17:34:27 +
Subject: [PATCH 1/6] Initial warning commit
---
clang/include/clang/Basic/DiagnosticGroups.td | 2 ++
.../clang/Basic/DiagnosticSemaKinds.td| 3 +++
clang/lib/Sema/SemaDeclCXX.cpp| 5
.../SemaCXX/unnecessary-virtual-specifier.cpp | 27 +++
4 files changed, 37 insertions(+)
create mode 100644 clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index fac80fb4009aa..65593ddcb8608 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -372,6 +372,8 @@ def CXX11WarnInconsistentOverrideMethod :
def CXX11WarnSuggestOverrideDestructor :
DiagGroup<"suggest-destructor-override">;
def CXX11WarnSuggestOverride : DiagGroup<"suggest-override">;
+def WarnUnnecessaryVirtualSpecifier :
DiagGroup<"unnecessary-virtual-specifier">;
+
// Original name of this warning in Clang
def : DiagGroup<"c++0x-narrowing", [CXX11Narrowing]>;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 8e6e6e892cdd7..a87cf7e674b4c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2706,6 +2706,9 @@ def warn_final_dtor_non_final_class : Warning<
InGroup;
def note_final_dtor_non_final_class_silence : Note<
"mark %0 as '%select{final|sealed}1' to silence this warning">;
+def warn_unnecessary_virtual_specifier : Warning<
+ "virtual method %0 is inside a 'final' class and can never be overridden">,
+ InGroup;
// C++11 attributes
def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index dd779ee377309..1b2e494956d4b 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -7193,10 +7193,15 @@ void Sema::CheckCompletedCXXClass(Scope *S,
CXXRecordDecl *Record) {
// class without overriding any.
if (!M->isStatic())
DiagnoseHiddenVirtualMethods(M);
+
if (M->hasAttr())
HasMethodWithOverrideControl = true;
else if (M->size_overridden_methods() > 0)
HasOverridingMethodWithoutOverrideControl = true;
+
+// See if a method is marked as virtual inside of a final class.
+if (M->isVirtualAsWritten() && Record->isEffectivelyFinal())
+ Diag(M->getLocation(), diag::warn_unnecessary_virtual_specifier) <<
M;
}
if (!isa(M))
diff --git a/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
new file mode 100644
index 0..eb8397d9ade45
--- /dev/null
+++ b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier
-Wsuggest-override %s
+
+struct Foo final {
+ Foo() = default;
+ virtual ~Foo() = default; // expected-warning
{{virtual method}}
+ virtual Foo& operator=(Foo& other) = default; // expected-warning
{{virtual method}}
+ virtual Foo& operator=(Foo&& other) = default; // expected-warning
{{virtual method}}
+ void f();
+ virtual void f(int); // expected-warning {{virtual
method}}
+ int g(int x) { return x; };
+ virtual int g(bool); // expected-warning {{virtual
method}}
+ static int s();
+};
+
+struct BarBase {
+ virtual ~BarBase() = delete;
+ virtual void virt() {}
+ virtual int virt(int);
+ int nonvirt();
+};
+
+struct Bar final : BarBase {
+ ~Bar() override = delete;
+ void virt() override {};
+ virtual int virt(int) override;// expected-warning
{{virtual method}}
+ int nonvirt();
+};
>From 4a16ef81a4882785708a4973f78586119235e8f5 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 13 Mar 2025 17:59:39 +
Subject: [PATCH 2/6] Add documentation
---
clang/docs/ReleaseNotes.rst | 3 +++
clang/include/clang/Basic/DiagnosticGroups.td | 11 ++-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8989124611e66..15858631c17b7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -247,6 +247,9 @@ Improvements to Clang's diagnostics
- The ``-Wsign-compare`` warning now treats expressions with bitwise not(~)
and minus(-) as signed integers
except for the case where the operand is an unsigned integer
and throws warning if they are compared with unsigned integers (##18878).
+- The ``-Wunnecessary-virtual-specifier`` warning has
[clang] Warn about virtual methods in `final` classes (PR #131188)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/131188
>From fbd474fb5ae3adeaf1644a4d44e916e4d7c66395 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 13 Mar 2025 17:34:27 +
Subject: [PATCH 1/5] Initial warning commit
---
clang/include/clang/Basic/DiagnosticGroups.td | 2 ++
.../clang/Basic/DiagnosticSemaKinds.td| 3 +++
clang/lib/Sema/SemaDeclCXX.cpp| 5
.../SemaCXX/unnecessary-virtual-specifier.cpp | 27 +++
4 files changed, 37 insertions(+)
create mode 100644 clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index fac80fb4009aa..65593ddcb8608 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -372,6 +372,8 @@ def CXX11WarnInconsistentOverrideMethod :
def CXX11WarnSuggestOverrideDestructor :
DiagGroup<"suggest-destructor-override">;
def CXX11WarnSuggestOverride : DiagGroup<"suggest-override">;
+def WarnUnnecessaryVirtualSpecifier :
DiagGroup<"unnecessary-virtual-specifier">;
+
// Original name of this warning in Clang
def : DiagGroup<"c++0x-narrowing", [CXX11Narrowing]>;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 8e6e6e892cdd7..a87cf7e674b4c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2706,6 +2706,9 @@ def warn_final_dtor_non_final_class : Warning<
InGroup;
def note_final_dtor_non_final_class_silence : Note<
"mark %0 as '%select{final|sealed}1' to silence this warning">;
+def warn_unnecessary_virtual_specifier : Warning<
+ "virtual method %0 is inside a 'final' class and can never be overridden">,
+ InGroup;
// C++11 attributes
def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index dd779ee377309..1b2e494956d4b 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -7193,10 +7193,15 @@ void Sema::CheckCompletedCXXClass(Scope *S,
CXXRecordDecl *Record) {
// class without overriding any.
if (!M->isStatic())
DiagnoseHiddenVirtualMethods(M);
+
if (M->hasAttr())
HasMethodWithOverrideControl = true;
else if (M->size_overridden_methods() > 0)
HasOverridingMethodWithoutOverrideControl = true;
+
+// See if a method is marked as virtual inside of a final class.
+if (M->isVirtualAsWritten() && Record->isEffectivelyFinal())
+ Diag(M->getLocation(), diag::warn_unnecessary_virtual_specifier) <<
M;
}
if (!isa(M))
diff --git a/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
new file mode 100644
index 0..eb8397d9ade45
--- /dev/null
+++ b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier
-Wsuggest-override %s
+
+struct Foo final {
+ Foo() = default;
+ virtual ~Foo() = default; // expected-warning
{{virtual method}}
+ virtual Foo& operator=(Foo& other) = default; // expected-warning
{{virtual method}}
+ virtual Foo& operator=(Foo&& other) = default; // expected-warning
{{virtual method}}
+ void f();
+ virtual void f(int); // expected-warning {{virtual
method}}
+ int g(int x) { return x; };
+ virtual int g(bool); // expected-warning {{virtual
method}}
+ static int s();
+};
+
+struct BarBase {
+ virtual ~BarBase() = delete;
+ virtual void virt() {}
+ virtual int virt(int);
+ int nonvirt();
+};
+
+struct Bar final : BarBase {
+ ~Bar() override = delete;
+ void virt() override {};
+ virtual int virt(int) override;// expected-warning
{{virtual method}}
+ int nonvirt();
+};
>From 4a16ef81a4882785708a4973f78586119235e8f5 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 13 Mar 2025 17:59:39 +
Subject: [PATCH 2/5] Add documentation
---
clang/docs/ReleaseNotes.rst | 3 +++
clang/include/clang/Basic/DiagnosticGroups.td | 11 ++-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8989124611e66..15858631c17b7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -247,6 +247,9 @@ Improvements to Clang's diagnostics
- The ``-Wsign-compare`` warning now treats expressions with bitwise not(~)
and minus(-) as signed integers
except for the case where the operand is an unsigned integer
and throws warning if they are compared with unsigned integers (##18878).
+- The ``-Wunnecessary-virtual-specifier`` warning has
[clang] Warn about virtual methods in `final` classes (PR #131188)
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier %s
+
+struct Foo final {
+ Foo() = default;
+ virtual ~Foo() = default; // expected-warning {{virtual
method}}
+ virtual Foo& operator=(Foo& other) = default; // expected-warning {{virtual
method}}
+ virtual Foo& operator=(Foo&& other) = default; // expected-warning {{virtual
method}}
+ void f();
+ virtual void f(int); // expected-warning {{virtual
method}}
+ int g(int x) { return x; };
+ virtual int g(bool); // expected-warning {{virtual
method}}
+ static int s();
+};
+
+struct BarBase {
+ virtual ~BarBase() = delete;
+ virtual void virt() {}
+ virtual int virt(int);
+ int nonvirt();
+};
+
+struct Bar final : BarBase {
+ ~Bar() override = delete;
+ void virt() override {};
+ // `virtual ... override;` is a common pattern, so don't warn
+ virtual int virt(int) override;
DKLoehr wrote:
Yes, good catch.
https://github.com/llvm/llvm-project/pull/131188
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn about virtual methods in `final` classes (PR #131188)
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier %s
+
+struct Foo final {
+ Foo() = default;
+ virtual ~Foo() = default; // expected-warning
{{virtual method}}
+ virtual Foo& operator=(Foo& other) = default; // expected-warning
{{virtual method}}
+ virtual Foo& operator=(Foo&& other) = default; // expected-warning
{{virtual method}}
+ void f();
+ virtual void f(int); // expected-warning {{virtual
method}}
+ int g(int x) { return x; };
+ virtual int g(bool); // expected-warning {{virtual
method}}
+ static int s();
+};
+
+struct BarBase {
+ virtual ~BarBase() = delete;
+ virtual void virt() {}
+ virtual int virt(int);
+ int nonvirt();
+};
+
+struct Bar final : BarBase {
+ ~Bar() override = delete;
+ void virt() override {};
+ virtual int virt(int) override;// expected-warning
{{virtual method}}
DKLoehr wrote:
Alright, disabled for `virtual...override`.
https://github.com/llvm/llvm-project/pull/131188
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Enable unnecessary-virtual-specifier by default (PR #133265)
DKLoehr wrote: Ah, missed that, sorry. It seems there hasn't been a release since the warning was added, and the existing release note seems to still apply: https://github.com/llvm/llvm-project/blob/8bdcd0a96e65557c8c3bf506d186c49002db6463/clang/docs/ReleaseNotes.rst?plain=1#L291 https://github.com/llvm/llvm-project/pull/133265 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Enable unnecessary-virtual-specifier by default (PR #133265)
DKLoehr wrote: @zmodem Do you mind taking a look? https://github.com/llvm/llvm-project/pull/133265 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Enable unnecessary-virtual-specifier by default (PR #133265)
DKLoehr wrote: Ach, I was so focused on making the build work that I forgot to run tests... Fixed now, either by disabling the warning or adding it as expected, as seemed appropriate. https://github.com/llvm/llvm-project/pull/133265 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Enable unnecessary-virtual-specifier by default (PR #133265)
https://github.com/DKLoehr created
https://github.com/llvm/llvm-project/pull/133265
This turns on the unnecessary-virtual-specifier warning in genera, but disables
it when building LLVM. It also tweaks the warning description to be slightly
more accurate.
Background: I've been working on cleaning up this warning in two codebases:
LLVM and chromium (plus its dependencies). The chromium cleanup has been
straightforward. Git archaeology shows that there are two reasons for the
warnings: classes to which `final` was added after they were initially
committed, and classes with virtual destructors that nobody remarks on.
Presumably the latter case is because people are just very used to destructors
being virtual.
The LLVM cleanup was more surprising: I discovered that we have an [old
policy](https://llvm.org/docs/CodingStandards.html#provide-a-virtual-method-anchor-for-classes-in-headers)
about including out-of-line virtual functions in every class with a vtable,
even `final` ones. This means our codebase has many virtual "anchor" functions
which do nothing except control where the vtable is emitted, and which trigger
the warning. I looked into alternatives to satisfy the policy, such as using
destructors instead of introducing a new function, but it wasn't clear if they
had larger implications.
Overall, it seems like the warning is genuinely useful in most codebases
(evidenced by chromium and its dependencies), and LLVM is an unusual case.
Therefore we should enable the warning by default, and turn it off only for
LLVM builds.
>From 792e1f3d062415134b8dfc4e8ed52f769f3e01f8 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 27 Mar 2025 14:59:44 +
Subject: [PATCH 1/2] Enable by default
---
clang/include/clang/Basic/DiagnosticGroups.td| 7 +++
clang/include/clang/Basic/DiagnosticSemaKinds.td | 2 +-
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index b9f08d96151c9..e6e9ebbc2c304 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -377,13 +377,12 @@ def CXX11WarnSuggestOverride :
DiagGroup<"suggest-override">;
def WarnUnnecessaryVirtualSpecifier :
DiagGroup<"unnecessary-virtual-specifier"> {
code Documentation = [{
Warns when a ``final`` class contains a virtual method (including virtual
-destructors). Since ``final`` classes cannot be subclassed, their methods
-cannot be overridden, and hence the ``virtual`` specifier is useless.
+destructors) that does not override anything. Since ``final`` classes cannot be
+subclassed, their methods cannot be overridden, so there is no point to
+introducing new ``virtual`` methods.
The warning also detects virtual methods in classes whose destructor is
``final``, for the same reason.
-
-The warning does not fire on virtual methods which are also marked
``override``.
}];
}
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c77cde297dc32..05ded7d9ecef1 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2733,7 +2733,7 @@ def note_final_dtor_non_final_class_silence : Note<
"mark %0 as '%select{final|sealed}1' to silence this warning">;
def warn_unnecessary_virtual_specifier : Warning<
"virtual method %0 is inside a 'final' class and can never be overridden">,
- InGroup, DefaultIgnore;
+ InGroup;
// C++11 attributes
def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
>From 5a221ac1f11d3dfde2a241be65bd06af25c0c37f Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 27 Mar 2025 15:18:30 +
Subject: [PATCH 2/2] Disable for LLVM
---
llvm/cmake/modules/HandleLLVMOptions.cmake | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/cmake/modules/HandleLLVMOptions.cmake
b/llvm/cmake/modules/HandleLLVMOptions.cmake
index 185c9b63aada3..f2c5cf4241e3d 100644
--- a/llvm/cmake/modules/HandleLLVMOptions.cmake
+++ b/llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -689,7 +689,7 @@ if ( LLVM_COMPILER_IS_GCC_COMPATIBLE OR
CMAKE_CXX_COMPILER_ID MATCHES "XL" )
endif( LLVM_COMPILER_IS_GCC_COMPATIBLE OR CMAKE_CXX_COMPILER_ID MATCHES "XL" )
if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
- append("-Werror=unguarded-availability-new" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+ append("-Werror=unguarded-availability-new
-Wno-unnecessary-virtual-specifier" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
endif()
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND LLVM_ENABLE_LTO)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [libcxx] [clang] improved preservation of template keyword (PR #133610)
DKLoehr wrote:
I ended up with the following reproducer:
```
template struct S {
template
using Arg = T::template Arg;
void f(Arg);
void f(Arg);
};
```
Running this with `clang++ repro.cc -fsyntax-only -std=c++20` yields
```
repro.cc:5:8: error: class member cannot be redeclared
5 | void f(Arg);
|^
repro.cc:4:8: note: previous declaration is here
4 | void f(Arg);
|^
```
The same error occurs regardless of which types are inside the < >.
https://github.com/llvm/llvm-project/pull/133610
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Revert "Enable unnecessary-virtual-specifier by default" (PR #134105)
DKLoehr wrote: Thanks for the revert, can you link to a failed build or something so I know what exactly went wrong? Was the issue the presence of an unrecognized warning flag in the build files? https://github.com/llvm/llvm-project/pull/134105 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MS][clang] Error about ambiguous operator delete[] only when required (PR #135041)
DKLoehr wrote: Can't comment on MSVC's behavior, but this does seem to eliminate the errors we were seeing in chromium (at least for my one local build). https://github.com/llvm/llvm-project/pull/135041 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn about virtual methods in `final` classes (PR #131188)
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier %s
+
+struct Foo final {
+ Foo() = default;
+ virtual ~Foo() = default; // expected-warning
{{virtual method}}
+ virtual Foo& operator=(Foo& other) = default; // expected-warning
{{virtual method}}
+ virtual Foo& operator=(Foo&& other) = default; // expected-warning
{{virtual method}}
+ void f();
+ virtual void f(int); // expected-warning {{virtual
method}}
+ int g(int x) { return x; };
+ virtual int g(bool); // expected-warning {{virtual
method}}
+ static int s();
+};
+
+struct BarBase {
+ virtual ~BarBase() = delete;
+ virtual void virt() {}
+ virtual int virt(int);
+ int nonvirt();
+};
+
+struct Bar final : BarBase {
+ ~Bar() override = delete;
+ void virt() override {};
+ virtual int virt(int) override;// expected-warning
{{virtual method}}
DKLoehr wrote:
My intuition was that `virtual` and `override` are separate things and so it
makes sense to warn in this case, but I don't feel strongly about it, and
compatibility with older code seems like a good reason to keep quiet.
https://github.com/llvm/llvm-project/pull/131188
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn about virtual methods in `final` classes (PR #131188)
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier %s
+
+struct Foo final {
+ Foo() = default;
+ virtual ~Foo() = default; // expected-warning {{virtual
method}}
+ virtual Foo& operator=(Foo& other) = default; // expected-warning {{virtual
method}}
+ virtual Foo& operator=(Foo&& other) = default; // expected-warning {{virtual
method}}
+ void f();
+ virtual void f(int); // expected-warning {{virtual
method}}
+ int g(int x) { return x; };
+ virtual int g(bool); // expected-warning {{virtual
method}}
+ static int s();
+};
+
+struct BarBase {
+ virtual ~BarBase() = delete;
+ virtual void virt() {}
+ virtual int virt(int);
+ int nonvirt();
+};
+
+struct Bar final : BarBase {
+ ~Bar() override = delete;
+ void virt() override {};
+ // `virtual ... override;` is a common pattern, so don't warn
+ virtual int virt(int) override;
+ virtual int virt(bool);// expected-warning {{virtual
method}}
+ int nonvirt();
+};
DKLoehr wrote:
Yes, I didn't include this case because it's covered by `-Wsuggest-override`.
https://github.com/llvm/llvm-project/pull/131188
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn about virtual methods in `final` classes (PR #131188)
@@ -2706,6 +2706,9 @@ def warn_final_dtor_non_final_class : Warning<
InGroup;
def note_final_dtor_non_final_class_silence : Note<
"mark %0 as '%select{final|sealed}1' to silence this warning">;
+def warn_unnecessary_virtual_specifier : Warning<
+ "virtual method %0 is inside a 'final' class and can never be overridden">,
+ InGroup, DefaultIgnore;
DKLoehr wrote:
My intention is to go through and try to clean up all the instances where it
triggers in a followup PR, so we wouldn't have this one clogged with lots of
tiny changes in random files. I agree it should be on by default.
https://github.com/llvm/llvm-project/pull/131188
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn about virtual methods in `final` classes (PR #131188)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/131188
>From fbd474fb5ae3adeaf1644a4d44e916e4d7c66395 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 13 Mar 2025 17:34:27 +
Subject: [PATCH 1/4] Initial warning commit
---
clang/include/clang/Basic/DiagnosticGroups.td | 2 ++
.../clang/Basic/DiagnosticSemaKinds.td| 3 +++
clang/lib/Sema/SemaDeclCXX.cpp| 5
.../SemaCXX/unnecessary-virtual-specifier.cpp | 27 +++
4 files changed, 37 insertions(+)
create mode 100644 clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index fac80fb4009aa..65593ddcb8608 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -372,6 +372,8 @@ def CXX11WarnInconsistentOverrideMethod :
def CXX11WarnSuggestOverrideDestructor :
DiagGroup<"suggest-destructor-override">;
def CXX11WarnSuggestOverride : DiagGroup<"suggest-override">;
+def WarnUnnecessaryVirtualSpecifier :
DiagGroup<"unnecessary-virtual-specifier">;
+
// Original name of this warning in Clang
def : DiagGroup<"c++0x-narrowing", [CXX11Narrowing]>;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 8e6e6e892cdd7..a87cf7e674b4c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2706,6 +2706,9 @@ def warn_final_dtor_non_final_class : Warning<
InGroup;
def note_final_dtor_non_final_class_silence : Note<
"mark %0 as '%select{final|sealed}1' to silence this warning">;
+def warn_unnecessary_virtual_specifier : Warning<
+ "virtual method %0 is inside a 'final' class and can never be overridden">,
+ InGroup;
// C++11 attributes
def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index dd779ee377309..1b2e494956d4b 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -7193,10 +7193,15 @@ void Sema::CheckCompletedCXXClass(Scope *S,
CXXRecordDecl *Record) {
// class without overriding any.
if (!M->isStatic())
DiagnoseHiddenVirtualMethods(M);
+
if (M->hasAttr())
HasMethodWithOverrideControl = true;
else if (M->size_overridden_methods() > 0)
HasOverridingMethodWithoutOverrideControl = true;
+
+// See if a method is marked as virtual inside of a final class.
+if (M->isVirtualAsWritten() && Record->isEffectivelyFinal())
+ Diag(M->getLocation(), diag::warn_unnecessary_virtual_specifier) <<
M;
}
if (!isa(M))
diff --git a/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
new file mode 100644
index 0..eb8397d9ade45
--- /dev/null
+++ b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier
-Wsuggest-override %s
+
+struct Foo final {
+ Foo() = default;
+ virtual ~Foo() = default; // expected-warning
{{virtual method}}
+ virtual Foo& operator=(Foo& other) = default; // expected-warning
{{virtual method}}
+ virtual Foo& operator=(Foo&& other) = default; // expected-warning
{{virtual method}}
+ void f();
+ virtual void f(int); // expected-warning {{virtual
method}}
+ int g(int x) { return x; };
+ virtual int g(bool); // expected-warning {{virtual
method}}
+ static int s();
+};
+
+struct BarBase {
+ virtual ~BarBase() = delete;
+ virtual void virt() {}
+ virtual int virt(int);
+ int nonvirt();
+};
+
+struct Bar final : BarBase {
+ ~Bar() override = delete;
+ void virt() override {};
+ virtual int virt(int) override;// expected-warning
{{virtual method}}
+ int nonvirt();
+};
>From 4a16ef81a4882785708a4973f78586119235e8f5 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 13 Mar 2025 17:59:39 +
Subject: [PATCH 2/4] Add documentation
---
clang/docs/ReleaseNotes.rst | 3 +++
clang/include/clang/Basic/DiagnosticGroups.td | 11 ++-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8989124611e66..15858631c17b7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -247,6 +247,9 @@ Improvements to Clang's diagnostics
- The ``-Wsign-compare`` warning now treats expressions with bitwise not(~)
and minus(-) as signed integers
except for the case where the operand is an unsigned integer
and throws warning if they are compared with unsigned integers (##18878).
+- The ``-Wunnecessary-virtual-specifier`` warning has
[clang] Warn about virtual methods in `final` classes (PR #131188)
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier %s
+
+struct Foo final {
+ Foo() = default;
+ virtual ~Foo() = default; // expected-warning {{virtual
method}}
+ virtual Foo& operator=(Foo& other) = default; // expected-warning {{virtual
method}}
+ virtual Foo& operator=(Foo&& other) = default; // expected-warning {{virtual
method}}
+ void f();
+ virtual void f(int); // expected-warning {{virtual
method}}
+ int g(int x) { return x; };
+ virtual int g(bool); // expected-warning {{virtual
method}}
+ static int s();
+};
+
+struct BarBase {
+ virtual ~BarBase() = delete;
+ virtual void virt() {}
+ virtual int virt(int);
+ int nonvirt();
+};
+
+struct Bar final : BarBase {
+ ~Bar() override = delete;
+ void virt() override {};
+ // `virtual ... override;` is a common pattern, so don't warn
+ virtual int virt(int) override;
+ virtual int virt(bool);// expected-warning {{virtual
method}}
DKLoehr wrote:
In this case we're getting a warning because `virt(bool)` isn't declared in the
base class, so this isn't an override at all.
The case where the function is overriding without the `override` keyword is
handled by `-Wsuggest-override`, so I didn't include that here.
https://github.com/llvm/llvm-project/pull/131188
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn about virtual methods in `final` classes (PR #131188)
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier %s
+
+struct Foo final {
+ Foo() = default;
+ virtual ~Foo() = default; // expected-warning {{virtual
method}}
+ virtual Foo& operator=(Foo& other) = default; // expected-warning {{virtual
method}}
+ virtual Foo& operator=(Foo&& other) = default; // expected-warning {{virtual
method}}
+ void f();
+ virtual void f(int); // expected-warning {{virtual
method}}
+ int g(int x) { return x; };
+ virtual int g(bool); // expected-warning {{virtual
method}}
+ static int s();
+};
+
+struct BarBase {
+ virtual ~BarBase() = delete;
+ virtual void virt() {}
+ virtual int virt(int);
+ int nonvirt();
+};
+
+struct Bar final : BarBase {
+ ~Bar() override = delete;
+ void virt() override {};
+ // `virtual ... override;` is a common pattern, so don't warn
+ virtual int virt(int) override;
+ virtual int virt(bool);// expected-warning {{virtual
method}}
+ int nonvirt();
+};
DKLoehr wrote:
Got it, changed.
https://github.com/llvm/llvm-project/pull/131188
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Warn about virtual methods in `final` classes (PR #131188)
DKLoehr wrote: @zmodem Would you take a look? What do you think about implementing the suggestion about checking if virtual methods actually get overridden or not? https://github.com/llvm/llvm-project/pull/131188 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MS][clang] Make sure vector deleting dtor calls correct operator delete (PR #133950)
DKLoehr wrote: We're seeing several errors building chromium that bisect to this change: `error: member 'operator delete[]' found in multiple base classes of different types`. An example build is [here](https://ci.chromium.org/ui/p/chromium/builders/ci/ToTLinux/29767/overview); the output is ``` ../../third_party/pdfium/xfa/fxfa/cxfa_ffexclgroup.cpp:16:19: error: member 'operator delete[]' found in multiple base classes of different types 16 | CXFA_FFExclGroup::~CXFA_FFExclGroup() = default; | ^ ../../third_party/pdfium/xfa/fxfa/cxfa_ffexclgroup.cpp:16:41: note: in defaulted destructor for 'CXFA_FFExclGroup' first required here 16 | CXFA_FFExclGroup::~CXFA_FFExclGroup() = default; | ^ ../../v8/include/cppgc/garbage-collected.h:69:8: note: member found by ambiguous name lookup 69 | void operator delete[](void*) = delete; |^ ../../v8/include/cppgc/garbage-collected.h:103:8: note: member found by ambiguous name lookup 103 | void operator delete[](void*) = delete; |^ ``` I'm working on putting together a minimal repro. https://github.com/llvm/llvm-project/pull/133950 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MS][clang] Make sure vector deleting dtor calls correct operator delete (PR #133950)
DKLoehr wrote: Yes, I've reproduced it locally with current trunk clang. https://github.com/llvm/llvm-project/pull/133950 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MS][clang] Make sure vector deleting dtor calls correct operator delete (PR #133950)
DKLoehr wrote:
This is the smallest reproduction I could find:
```
struct BaseDelete1 {
void operator delete[](void *);
};
struct BaseDelete2 {
void operator delete[](void *);
};
struct BaseDestructor {
virtual ~BaseDestructor() = default;
};
struct Final : BaseDelete1, BaseDelete2, BaseDestructor {};
```
When run with `clang++ repro.cc -fsyntax-only -std=c++20`, it yields the
following error. Removing any one of the three base classes causes it to
compile without problems.
```
repro.cc:10:8: error: member 'operator delete[]' found in multiple base classes
of different types
10 | struct Final : BaseDelete1, BaseDelete2, BaseDestructor {};
|^
repro.cc:10:8: note: in implicit destructor for 'Final' first required here
repro.cc:2:8: note: member found by ambiguous name lookup
2 | void operator delete[](void *);
|^
repro.cc:5:8: note: member found by ambiguous name lookup
5 | void operator delete[](void *);
|^
```
https://github.com/llvm/llvm-project/pull/133950
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MS][clang] Make sure vector deleting dtor calls correct operator delete (PR #133950)
DKLoehr wrote:
Thanks! Unfortunately it looks like the fix introduced a regression of #134265
on Windows. Repro:
```
struct Base {
virtual ~Base();
void operator delete[](void *) = delete;
};
class __declspec(dllexport) Derived : Base {};
```
Running `clang-cl repro.cc /std:c++20` results in the following error after
```
repro.cc(5,29): error: attempt to use a deleted function
5 | class __declspec(dllexport) Derived : Base {};
| ^
repro.cc(5,29): note: in implicit destructor for 'Derived' first required here
repro.cc(5,18): note: due to 'Derived' being dllexported
5 | class __declspec(dllexport) Derived : Base {};
| ^
repro.cc(3,8): note: 'operator delete[]' has been explicitly marked deleted here
3 | void operator delete[](void *) = delete;
|^
```
https://github.com/llvm/llvm-project/pull/133950
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [MS][clang] Make sure vector deleting dtor calls correct operator delete (PR #133950)
DKLoehr wrote: I'm not familiar enough to say what we should be doing; perhaps @rnk or @zmodem have thoughts? It seems like we should at least emit a better error message, since just looking at the code doesn't make it clear to me why `operator delete[]` is relevant. Since this is causing clang to become more restrictive, the best thing to do might be to revert to whatever we used to do in this situation, and then make a followup change which specifically explains why clang is becoming more restrictive. https://github.com/llvm/llvm-project/pull/133950 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [libcxx] [clang] improved preservation of template keyword (PR #133610)
DKLoehr wrote: We're also seeing `class member cannot be redeclared` errors when trying to build chromium: ``` In file included from ../../components/system_cpu/cpu_probe.cc:16: In file included from ../../components/system_cpu/cpu_probe_linux.h:14: ../../base/threading/sequence_bound.h:281:8: error: class member cannot be redeclared 281 | void PostTaskWithThisObject( |^ ../../base/threading/sequence_bound.h:264:8: note: previous definition is here 264 | void PostTaskWithThisObject( |^ ``` The file in question can be viewed [here](https://source.chromium.org/chromium/chromium/src/+/main:base/threading/sequence_bound.h;l=263;drc=72ba73a81b090ee221a9f107333c3e5e964cf750). It seems like the compiler is getting confused between `const UnwrappedT&` and `UnwrappedT*`, where `UnwrappedT` is, ultimately, a templated type. I'm also looking for a reduced repro, though I expect alexfh will produce one that works for both of us. https://github.com/llvm/llvm-project/pull/133610 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Enable unnecessary-virtual-specifier by default (PR #133265)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/133265
>From 792e1f3d062415134b8dfc4e8ed52f769f3e01f8 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 27 Mar 2025 14:59:44 +
Subject: [PATCH 1/4] Enable by default
---
clang/include/clang/Basic/DiagnosticGroups.td| 7 +++
clang/include/clang/Basic/DiagnosticSemaKinds.td | 2 +-
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index b9f08d96151c9..e6e9ebbc2c304 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -377,13 +377,12 @@ def CXX11WarnSuggestOverride :
DiagGroup<"suggest-override">;
def WarnUnnecessaryVirtualSpecifier :
DiagGroup<"unnecessary-virtual-specifier"> {
code Documentation = [{
Warns when a ``final`` class contains a virtual method (including virtual
-destructors). Since ``final`` classes cannot be subclassed, their methods
-cannot be overridden, and hence the ``virtual`` specifier is useless.
+destructors) that does not override anything. Since ``final`` classes cannot be
+subclassed, their methods cannot be overridden, so there is no point to
+introducing new ``virtual`` methods.
The warning also detects virtual methods in classes whose destructor is
``final``, for the same reason.
-
-The warning does not fire on virtual methods which are also marked
``override``.
}];
}
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c77cde297dc32..05ded7d9ecef1 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2733,7 +2733,7 @@ def note_final_dtor_non_final_class_silence : Note<
"mark %0 as '%select{final|sealed}1' to silence this warning">;
def warn_unnecessary_virtual_specifier : Warning<
"virtual method %0 is inside a 'final' class and can never be overridden">,
- InGroup, DefaultIgnore;
+ InGroup;
// C++11 attributes
def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
>From 5a221ac1f11d3dfde2a241be65bd06af25c0c37f Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 27 Mar 2025 15:18:30 +
Subject: [PATCH 2/4] Disable for LLVM
---
llvm/cmake/modules/HandleLLVMOptions.cmake | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/cmake/modules/HandleLLVMOptions.cmake
b/llvm/cmake/modules/HandleLLVMOptions.cmake
index 185c9b63aada3..f2c5cf4241e3d 100644
--- a/llvm/cmake/modules/HandleLLVMOptions.cmake
+++ b/llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -689,7 +689,7 @@ if ( LLVM_COMPILER_IS_GCC_COMPATIBLE OR
CMAKE_CXX_COMPILER_ID MATCHES "XL" )
endif( LLVM_COMPILER_IS_GCC_COMPATIBLE OR CMAKE_CXX_COMPILER_ID MATCHES "XL" )
if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
- append("-Werror=unguarded-availability-new" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+ append("-Werror=unguarded-availability-new
-Wno-unnecessary-virtual-specifier" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
endif()
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND LLVM_ENABLE_LTO)
>From fff138d2047e2867a969d36a999eba5f9722761f Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 27 Mar 2025 18:45:37 +
Subject: [PATCH 3/4] Disable in tests
---
.../Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp | 2 +-
clang/test/CXX/class/p2-0x.cpp | 2 +-
clang/test/SemaCXX/MicrosoftExtensions.cpp | 1 +
clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp | 5 +++--
4 files changed, 6 insertions(+), 4 deletions(-)
diff --git
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp
index 4209db14eaa52..106091b240af6 100644
---
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp
+++
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor
-verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor
-verify %s -Wno-unnecessary-virtual-specifier
#include "mock-types.h"
diff --git a/clang/test/CXX/class/p2-0x.cpp b/clang/test/CXX/class/p2-0x.cpp
index 5b39e0ada7e2c..2043486457baf 100644
--- a/clang/test/CXX/class/p2-0x.cpp
+++ b/clang/test/CXX/class/p2-0x.cpp
@@ -28,7 +28,7 @@ struct C : A { }; // expected-error {{base 'A' is marked
'final'}}
namespace Test4 {
-struct A final { virtual void func() = 0; }; // expected-warning {{abstract
class is marked 'final'}} expected-note {{unimplemented pure virtual method
'func' in 'A'}}
+struct A final { virtual void func() = 0; }; // expected-warning {{abstract
class is marked 'final'}} expected-note {{unimplemented pure virtual method
'func' in 'A'}} expected-warning {{virtual method 'func' is in
[clang] [llvm] Enable unnecessary-virtual-specifier by default (PR #133265)
@@ -689,7 +689,7 @@ if ( LLVM_COMPILER_IS_GCC_COMPATIBLE OR
CMAKE_CXX_COMPILER_ID MATCHES "XL" )
endif( LLVM_COMPILER_IS_GCC_COMPATIBLE OR CMAKE_CXX_COMPILER_ID MATCHES "XL" )
if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
- append("-Werror=unguarded-availability-new" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+ append("-Werror=unguarded-availability-new
-Wno-unnecessary-virtual-specifier" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
DKLoehr wrote:
Done
https://github.com/llvm/llvm-project/pull/133265
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CLANG-CL] ignores Wpadded (PR #134426)
=?utf-8?q?Théo?= De Magalhaes , =?utf-8?q?Théo?= De Magalhaes , =?utf-8?q?Théo?= De Magalhaes , =?utf-8?q?Théo?= De Magalhaes , =?utf-8?q?Théo?= De Magalhaes , =?utf-8?q?Théo?= De Magalhaes , =?utf-8?q?Théo?= De Magalhaes , =?utf-8?q?Théo?= De Magalhaes ,Theo de Magalhaes , =?utf-8?q?Théo?= De Magalhaes , =?utf-8?q?Théo?= De Magalhaes , =?utf-8?q?Théo?= De Magalhaes Message-ID: In-Reply-To: DKLoehr wrote: After this change, it seems that `-Wpadded` is included in `-Wall` for `clang-cl` only, but not `clang` or `clang++`. I'm not sure why, but the inconsistency seems strange, is it intentional? (Reproduction: I compiled the sample program in #61702 using all three executables, with and without -Wall; only `clang-cl.exe -Wall` produced this warning) https://github.com/llvm/llvm-project/pull/134426 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [CLANG-CL] ignores Wpadded (PR #134426)
=?utf-8?q?Th=C3=A9o?= De Magalhaes , =?utf-8?q?Th=C3=A9o?= De Magalhaes , =?utf-8?q?Th=C3=A9o?= De Magalhaes , =?utf-8?q?Th=C3=A9o?= De Magalhaes , =?utf-8?q?Th=C3=A9o?= De Magalhaes , =?utf-8?q?Th=C3=A9o?= De Magalhaes , =?utf-8?q?Th=C3=A9o?= De Magalhaes , =?utf-8?q?Th=C3=A9o?= De Magalhaes ,Theo de Magalhaes , =?utf-8?q?Th=C3=A9o?= De Magalhaes , =?utf-8?q?Th=C3=A9o?= De Magalhaes , =?utf-8?q?Th=C3=A9o?= De Magalhaes Message-ID: In-Reply-To: DKLoehr wrote: Got it, thanks. https://github.com/llvm/llvm-project/pull/134426 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Generate empty .clang-format-ignore before running tests (PR #136154)
https://github.com/DKLoehr created
https://github.com/llvm/llvm-project/pull/136154
Followup to #136022, this ensures formatting tests are run with an empty
`.clang-format-ignore` in their root directory, to prevent failures if the file
also exists higher in the tree.
>From 804fcdd84e8551005bfa5dae58d24f9852608360 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 17 Apr 2025 16:10:55 +
Subject: [PATCH] Generate empty .clang-format-ignore before running tests
---
clang/test/Format/lit.local.cfg | 6 ++
1 file changed, 6 insertions(+)
diff --git a/clang/test/Format/lit.local.cfg b/clang/test/Format/lit.local.cfg
index b060c79226cbd..cf79e8df89a33 100644
--- a/clang/test/Format/lit.local.cfg
+++ b/clang/test/Format/lit.local.cfg
@@ -1,3 +1,4 @@
+import os
import platform
import lit.formats
@@ -27,3 +28,8 @@ config.suffixes = [
# python implementation does, so use that for cross platform compatibility
if platform.system() == "AIX":
config.test_format = lit.formats.ShTest()
+
+# Create an empty .clang-format-ignore file so that tests don't get messed
+# up if one exists higher in the tree
+with open(os.path.join("Format", ".clang-format-ignore"), 'w'):
+pass
\ No newline at end of file
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Generate empty .clang-format-ignore before running tests (PR #136154)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/136154
>From 804fcdd84e8551005bfa5dae58d24f9852608360 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 17 Apr 2025 16:10:55 +
Subject: [PATCH 1/2] Generate empty .clang-format-ignore before running tests
---
clang/test/Format/lit.local.cfg | 6 ++
1 file changed, 6 insertions(+)
diff --git a/clang/test/Format/lit.local.cfg b/clang/test/Format/lit.local.cfg
index b060c79226cbd..cf79e8df89a33 100644
--- a/clang/test/Format/lit.local.cfg
+++ b/clang/test/Format/lit.local.cfg
@@ -1,3 +1,4 @@
+import os
import platform
import lit.formats
@@ -27,3 +28,8 @@ config.suffixes = [
# python implementation does, so use that for cross platform compatibility
if platform.system() == "AIX":
config.test_format = lit.formats.ShTest()
+
+# Create an empty .clang-format-ignore file so that tests don't get messed
+# up if one exists higher in the tree
+with open(os.path.join("Format", ".clang-format-ignore"), 'w'):
+pass
\ No newline at end of file
>From 94d80cbb3b63c14f047fe253c5adf2ce26e4f554 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 17 Apr 2025 13:27:27 -0400
Subject: [PATCH 2/2] Just create file in cwd
Should be more robust to whether the Format folder already exists or not.
---
clang/test/Format/lit.local.cfg | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/test/Format/lit.local.cfg b/clang/test/Format/lit.local.cfg
index cf79e8df89a33..20e217664997b 100644
--- a/clang/test/Format/lit.local.cfg
+++ b/clang/test/Format/lit.local.cfg
@@ -31,5 +31,5 @@ if platform.system() == "AIX":
# Create an empty .clang-format-ignore file so that tests don't get messed
# up if one exists higher in the tree
-with open(os.path.join("Format", ".clang-format-ignore"), 'w'):
-pass
\ No newline at end of file
+with open(".clang-format-ignore", 'w'):
+pass
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Generate empty .clang-format-ignore before running tests (PR #136154)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/136154
>From 804fcdd84e8551005bfa5dae58d24f9852608360 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 17 Apr 2025 16:10:55 +
Subject: [PATCH 1/2] Generate empty .clang-format-ignore before running tests
---
clang/test/Format/lit.local.cfg | 6 ++
1 file changed, 6 insertions(+)
diff --git a/clang/test/Format/lit.local.cfg b/clang/test/Format/lit.local.cfg
index b060c79226cbd..cf79e8df89a33 100644
--- a/clang/test/Format/lit.local.cfg
+++ b/clang/test/Format/lit.local.cfg
@@ -1,3 +1,4 @@
+import os
import platform
import lit.formats
@@ -27,3 +28,8 @@ config.suffixes = [
# python implementation does, so use that for cross platform compatibility
if platform.system() == "AIX":
config.test_format = lit.formats.ShTest()
+
+# Create an empty .clang-format-ignore file so that tests don't get messed
+# up if one exists higher in the tree
+with open(os.path.join("Format", ".clang-format-ignore"), 'w'):
+pass
\ No newline at end of file
>From 94d80cbb3b63c14f047fe253c5adf2ce26e4f554 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 17 Apr 2025 13:27:27 -0400
Subject: [PATCH 2/2] Just create file in cwd
Should be more robust to whether the Format folder already exists or not.
---
clang/test/Format/lit.local.cfg | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/test/Format/lit.local.cfg b/clang/test/Format/lit.local.cfg
index cf79e8df89a33..20e217664997b 100644
--- a/clang/test/Format/lit.local.cfg
+++ b/clang/test/Format/lit.local.cfg
@@ -31,5 +31,5 @@ if platform.system() == "AIX":
# Create an empty .clang-format-ignore file so that tests don't get messed
# up if one exists higher in the tree
-with open(os.path.join("Format", ".clang-format-ignore"), 'w'):
-pass
\ No newline at end of file
+with open(".clang-format-ignore", 'w'):
+pass
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Generate empty .clang-format-ignore before running tests (PR #136154)
https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/136154
>From 804fcdd84e8551005bfa5dae58d24f9852608360 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 17 Apr 2025 16:10:55 +
Subject: [PATCH 1/2] Generate empty .clang-format-ignore before running tests
---
clang/test/Format/lit.local.cfg | 6 ++
1 file changed, 6 insertions(+)
diff --git a/clang/test/Format/lit.local.cfg b/clang/test/Format/lit.local.cfg
index b060c79226cbd..cf79e8df89a33 100644
--- a/clang/test/Format/lit.local.cfg
+++ b/clang/test/Format/lit.local.cfg
@@ -1,3 +1,4 @@
+import os
import platform
import lit.formats
@@ -27,3 +28,8 @@ config.suffixes = [
# python implementation does, so use that for cross platform compatibility
if platform.system() == "AIX":
config.test_format = lit.formats.ShTest()
+
+# Create an empty .clang-format-ignore file so that tests don't get messed
+# up if one exists higher in the tree
+with open(os.path.join("Format", ".clang-format-ignore"), 'w'):
+pass
\ No newline at end of file
>From 94d80cbb3b63c14f047fe253c5adf2ce26e4f554 Mon Sep 17 00:00:00 2001
From: Devon Loehr
Date: Thu, 17 Apr 2025 13:27:27 -0400
Subject: [PATCH 2/2] Just create file in cwd
Should be more robust to whether the Format folder already exists or not.
---
clang/test/Format/lit.local.cfg | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/test/Format/lit.local.cfg b/clang/test/Format/lit.local.cfg
index cf79e8df89a33..20e217664997b 100644
--- a/clang/test/Format/lit.local.cfg
+++ b/clang/test/Format/lit.local.cfg
@@ -31,5 +31,5 @@ if platform.system() == "AIX":
# Create an empty .clang-format-ignore file so that tests don't get messed
# up if one exists higher in the tree
-with open(os.path.join("Format", ".clang-format-ignore"), 'w'):
-pass
\ No newline at end of file
+with open(".clang-format-ignore", 'w'):
+pass
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Handle instantiated members to determine visibility (PR #136128)
DKLoehr wrote: This is breaking builds when compiling chromium as well, bisected to this change. Example build: https://ci.chromium.org/ui/p/chromium/builders/ci/ToTLinux%20(dbg)/31974/overview. Compiler output can be seen by clicking `[raw]` under the `compile` step, but I'm working on a minimized reproduction now. https://github.com/llvm/llvm-project/pull/136128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Handle instantiated members to determine visibility (PR #136128)
DKLoehr wrote: I'm working on reducing my reproduction, but it's taking a while. https://github.com/llvm/llvm-project/pull/136128 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Handle instantiated members to determine visibility (PR #136128)
DKLoehr wrote:
Here's what cvise gave me; unfortunately, I'm in a rush so I don't have time to
clean it up any further. It crashes when run with `clang++ repro.cc -std=c++20
-Wno-everyt hing -fsyntax-only -ferror-limit=0`
repro.cc:
```
template using conditional_t = _IfRes;
template void forward();
template class OnceCallback;
template class, typename> bool is_instantiation_v;
template class C, typename... Ts>
constexpr bool is_instantiation_v> = true;
template class C, typename T>
concept is_instantiation = is_instantiation_v;
template constexpr bool kIsWeakMethod = false;
template struct TypeList;
struct C;
template using DropTypeListItem = TypeList;
template struct MakeFunctionTypeImpl;
template
struct MakeFunctionTypeImpl> {
using Type = R(Args...);
};
template
using MakeFunctionType = MakeFunctionTypeImpl::Type;
template using ExtractArgs = Signature;
template using ExtractReturnType = Signature;
template struct FunctorTraits;
template struct DecayedFunctorTraits;
template
struct DecayedFunctorTraits {
using RunType = R;
template
static void Invoke(Method, ReceiverPtr);
};
template
struct FunctorTraits : DecayedFunctorTraits {};
template struct InvokeHelper;
template
struct InvokeHelper {
template
static void MakeItSo(Functor, BoundArgsTuple, RunArgs...) {
Traits::Invoke(0, forward...);
}
};
template >
using TransformToUnwrappedType = decltype(ForwardedType());
template struct Invoker2;
template
struct Invoker2 {
static void RunOnce() { RunImpl(0, 0); }
template
static void RunImpl(Functor, BoundArgsTuple) {
InvokeHelper, Traits, R>::MakeItSo(0, 0,
forward...);
}
};
template class CallbackT> struct BindHelper2 {
static constexpr bool kIsOnce =
is_instantiation>;
template static void BindImpl(Functor) {
using Traits = FunctorTraits>;
Invoker2<
Traits,
MakeFunctionType<
ExtractReturnType,
DropTypeListItem>>>::RunOnce;
}
};
void (C::*BindOnce2void___trans_tmp_1)();
void BindOnce2void() {
BindHelper2::BindImpl(BindOnce2void___trans_tmp_1);
}
```
https://github.com/llvm/llvm-project/pull/136128
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
