kernel: Avoid duplicating symbols in the kernel and downstream users

theuni commented at 7:32 pm on February 5, 2025: member

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.

build: add kernel-specific warnings

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.

5ca103aac6

kernel: avoid potential duplicate object in shared library/binary

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]

763c9ddfab

DrahtBot commented at 7:32 pm on February 5, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31807.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

No conflicts as of last run.

DrahtBot added the label Validation on Feb 5, 2025

hebasto added the label Build system on Feb 5, 2025

theuni commented at 8:02 pm on February 5, 2025: member

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 inline variable in a header, it’s not clear to me that all linkers are guaranteed to do the right thing in all cases (I admit I might be totally wrong on that), and I would at least like some way to be warned about these so that we can investigate.

hebasto commented at 11:46 am on February 6, 2025: member

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.

I don’t think these concerns are relevant to shared kernel library users, as https://github.com/llvm/llvm-project/pull/117622 clearly limits its scope to “multiple shared libraries”.

hebasto commented at 1:01 pm on February 6, 2025: member

But still, with a c++17-style inline variable in a header, it’s not clear to me that all linkers are guaranteed to do the right thing in all cases (I admit I might be totally wrong on that), and I would at least like some way to be warned about these so that we can investigate.

I do believe that this case is covered by the ODR:

There can be more than one definition in a program of each of the following: … inline variable … If all these requirements are satisfied, the program behaves as if there is only one definition in the entire program. Otherwise, the program is ill-formed, no diagnostic required.

(highlighted by me).

theuni commented at 3:05 pm on February 6, 2025: member

I do believe that this case is covered by the ODR:

There can be more than one definition in a program of each of the following: … inline variable … If all these requirements are satisfied, the program behaves as if there is only one definition in the entire program. Otherwise, the program is ill-formed, no diagnostic required.

(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 cs_main issue from #31158.

Edit: Hah, here’s an s/o answer that more elegantly says what I attempted above.

theuni marked this as a draft on Feb 6, 2025

kernel: Avoid duplicating symbols in the kernel and downstream users #31807

Code Coverage & Benchmarks

Reviews

Conflicts