[all-commits] [llvm/llvm-project] d5684b: Warn when unique objects might be duplicated in sh...

[Devon Loehr via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: d5684b8402d2175e80a2792db58e9a8e960a8544
      https://github.com/llvm/llvm-project/commit/d5684b8402d2175e80a2792db58e9a8e960a8544
  Author: Devon Loehr <DKLoehr at users.noreply.github.com>
  Date:   2025-02-03 (Mon, 03 Feb 2025)

  Changed paths:
    M clang/docs/ReleaseNotes.rst
    M clang/include/clang/Basic/DiagnosticGroups.td
    M clang/include/clang/Basic/DiagnosticSemaKinds.td
    M clang/include/clang/Sema/Sema.h
    M clang/lib/Sema/SemaDecl.cpp
    A clang/test/SemaCXX/unique_object_duplication.cpp
    A clang/test/SemaCXX/unique_object_duplication.h

  Log Message:
  -----------
  Warn when unique objects might be duplicated in shared libraries (#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.



To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications