mirror of
https://github.com/llvm/llvm-project.git
synced 2025-04-29 12:46:06 +00:00
dfc28736d4
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.
See llvm/docs/README.txt