kernel: Avoid duplicating symbols in the kernel and downstream users #31807
Conversation
In some cases, we'll want to be more aggressive or care about different things when building the kernel. In this case, a warning is added for symbols which may be duplicated between the kernel and downstream users. A few caveats for now: - This warning is introduced in clang 21, which is not yet released - The warning doesn't actually show unless the CXX_VISIBILITY_PRESET hack is disabled in CMake, which won't happen for a while. So this warning is not helpful for CI at the moment, but it's possible to use it manually (as I have for this PR) by disabling the hack locally until then.
Fixes warning and potential bug whereby init_flag may exist in both libbitcoinkernel as well as a downstream user, as opposed to being shared as intended. src/support/lockedpool.h:224:31: warning: 'init_flag' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage [-Wunique-object-duplication]
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31807. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Thinking about this some more, this would mostly only be a problem if we attempted to define a native c++ api for the kernel, as opposed to the C api which does not expose our internal headers. Because currently, we mark all symbols as externally visible. But still, with a c++17-style |
I don't think these concerns are relevant to shared kernel library users, as llvm/llvm-project#117622 clearly limits its scope to "multiple shared libraries". |
I do believe that this case is covered by the ODR:
(highlighted by me). |
The ODR defines what happens "in a program". Symbol resolution between shared libs and binaries is linker/loader specific. See ld's options for duplicate/weak symbol handling for example, or macOS's complicated mess of namespacing. Windows forces you to specify "I'm importing this from a dll", which is kinda nice actually. You're right that the warning is aimed at "multiple shared libraries", but our Linux binaries are basically just shared libs with an entry point. You can even link against bitcoind or dlopen from it as though it's a .so. That's why I'm concerned about linker behavior. Going to convert this to draft for now. I doubt it'll even matter much as we'll probably never selectively export our c++ symbols. But I still don't have 100% confidence in the Edit: Hah, here's an s/o answer that more elegantly says what I attempted above. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
The possibility of this happening came up in the discussion here: #31158 (comment), and it turned that we already had a case of it in our code.
tl;dr: Certain symbols when defined in headers may end up existing in both the shared lib and the application using it, leading to subtle bugs. More info can be found here.
The solution is generally to avoid defining static/inline vars in headers if they aren't meant to be shared with downstream kernel users.
Clang has recently added a warning for this (see link above), so this PR enables that too. It adds a new place to specify warnings which only apply to the kernel. In this case, it's warnings that only apply to shared libs. But I can also imagine wanting to enable more aggressive warnings for the kernel than the rest of the code.
The warning is only enabled by clang 21 (which is not yet released), and only useful when our kernel symbol visibility hack is disabled, which we can't do yet. So it's mostly a placeholder for the future. In the meantime it can be used manually by building clang from git and disabling our hack, which I've done, and can confirm that after this change there are no more warnings.