Static variable inside inline function emitted in a different section group than the function

[smeenai]

We ran into an interesting crash when upgrading from C++17 to C++20. We had an inline function being compiled into some object files built with C++17 and some built with C++20. This inline function has a static const std::string variable, which becomes constexpr in C++20. A simplified version is as follows (the [[clang::nodestroy]] generates shorter code but the issue also exists without it):

inline const std::string &get() {
    [[clang::no_destroy]] static const std::string empty;
    return empty;
}

The generated code for C++17 and C++20 can be seen in https://godbolt.org/z/evo95T1dj. In C++17, the string object is zero-initialized at runtime, so the object is stored in writable memory. (It's stored in .bss, so I don't know why the runtime zero-initialization is necessary, but I digress.) In C++20, the string object is stored as a bunch of zero bytes in the read-only section .rodata and not initialized at runtime.

The crash arose because the linker ended up picking get from a C++17 object but get::empty from a C++20 object. This meant that get's runtime initialization was trying to write to read-only memory, which of course segfaulted. I haven't dug into how this particular linker selection ended up occurring (it was in an LTO build, which might have complicated things), but as far as I understand, since Clang is emitting get and get::empty in different COMDAT section groups, it's valid for the linker to mix and match them like this. On the other hand, if Clang had emitted get::empty in the same section group as get, the linker would have had to pick or discard both of them as a unit, so the bad mixing couldn't have occurred. Is this understanding correct, and if so, what's the reason for Clang using two different section groups here?

CC @rnk and @MaskRay

[llvmbot]

@llvm/issue-subscribers-clang-codegen

Author: Shoaib Meenai (smeenai)

Details

We ran into an interesting crash when upgrading from C++17 to C++20. We had an inline function being compiled into some object files built with C++17 and some built with C++20. This inline function has a `static const std::string` variable, which becomes constexpr in C++20. A simplified version is as follows (the `[[clang::nodestroy]]` generates shorter code but the issue also exists without it):

inline const std::string &get() {
    [[clang::no_destroy]] static const std::string empty;
    return empty;
}

The generated code for C++17 and C++20 can be seen in https://godbolt.org/z/evo95T1dj. In C++17, the string object is zero-initialized at runtime, so the object is stored in writable memory. (It's stored in .bss, so I don't know why the runtime zero-initialization is necessary, but I digress.) In C++20, the string object is stored as a bunch of zero bytes in the read-only section .rodata and not initialized at runtime.

The crash arose because the linker ended up picking get from a C++17 object but get::empty from a C++20 object. This meant that get's runtime initialization was trying to write to read-only memory, which of course segfaulted. I haven't dug into how this particular linker selection ended up occurring (it was in an LTO build, which might have complicated things), but as far as I understand, since Clang is emitting get and get::empty in different COMDAT section groups, it's valid for the linker to mix and match them like this. On the other hand, if Clang had emitted get::empty in the same section group as get, the linker would have had to pick or discard both of them as a unit, so the bad mixing couldn't have occurred. Is this understanding correct, and if so, what's the reason for Clang using two different section groups here?

CC @rnk and @MaskRay

[rnk]

I think the comdat behavior is correct and intentional, if we put static locals in the same comdat group as the function, that would break inlining. Inlining effectively clones the function body and generates new references to the static local from outside the comdat group.

Static locals in inline functions are extremely ABI-fragile, and typically it's not a win to inline these functions. I would generally recommend sinking inline functions like this out of headers.

[smeenai]

You're right. My specific problem, after looking more into it, turned out to be related to inlining as well. I don't think this technically counts as an ODR violation (since the source tokens are the same in all TUs), so it was kinda surprising to have it blow up like this, but I guess it's fragile regardless.

[dwblaikie]

Yeah, I think th ecomdat behavior is correct - but it still seems like there's a bug here? Like I think we want to/would expect to support having an inline function with a static local compiled with C++17 and C++20 and get correct behavior...

If we can't support that, seems important to document and maybe warn/break/something in this situation - otherwise that's pretty subtle/easy to trip over.

[rnk]

I think you are right, that is the bug: There is an ABI incompatibilty between C++17 and C++20 due to the constexpr-ification of std::string. Maybe route to libc++? It seems like an unfixable problem, however.

[dwblaikie]

I think you are right, that is the bug: There is an ABI incompatibilty between C++17 and C++20 due to the constexpr-ification of std::string. Maybe route to libc++? It seems like an unfixable problem, however.

Tagging libc++.

As for fixes, /maybe/ there's room for an ABI fix - if the object is constinit, then the global flag that tracks its initialization should be already set as "initialized" - and that flag should be in the same comdat group as the global itself and so should get picked together?

So I guess what I'm suggesting is a clang change - that we always emit a guard variable (const value 1) for comdat constinit variables.

Is that feasible? Would it need to be opt-in with an attribute instead (that'd be unfortunate - it'd mean other people's code would still silently break, even if we fixed libc++ std::string with the attribute)

(hmm, looks like currently we don't put the guard and the variable in the same comdat... https://godbolt.org/z/Kq6v1M7h6 )

[ldionne]

🤯 This is one surprising ABI break. They're always surprising and interesting, but this one's a good one.

I'm thinking about ways that we could fix this on the library side, but I really can't think of much. Our usual tool to solve these problems is to use ABI tags to ensure we don't de-duplicate things in undesirable ways across TUs, but it doesn't apply here (and we definitely need the function-local static to be deduped).

The only other "fix" I could think of would be to somehow make it so that std::string is constant initialized in all Standard modes, but this would only solve the problem for default construction. Unless we can basically apply Making std::string constexpr in all Standard modes as a DR, but I'm 99% certain that's impossible due to missing core language features in constexpr.

This would also not solve the issue in case it arose in user code, which is possible. I'm sure it's not too hard to write a simple class that exhibits this exact behavior without using the Standard library.

So TLDR I think we could potentially investigate ways to make this very narrow case work from the libc++ side, but other than that I don't see a lot of solutions library-side. Open to any suggestions though, of course.

[dwblaikie]

The only other "fix" I could think of would be to somehow make it so that std::string is constant initialized in all Standard modes, but this would only solve the problem for default construction. Unless we can basically apply Making std::string constexpr in all Standard modes as a DR, but I'm 99% certain that's impossible due to missing core language features in constexpr.

Which wouldn't address the shipped situation, yeah? (like I have some code already compiled with c++17 and I'm linking it into my new C++20 code) but the ABI fixes I'm suggesting wouldn't help there either, I guess... or maybe they would? Hmm, nope - if we created a new comdat group for the static+guard, the old code would keep its static+guard and new code would get a new one, so inlined copies of the inline function could diverge/end up with two versions of the variable which would be bad.

Yeah, maybe we're solidly in "no way to make this backwards compatible" territory? Probably... but then still potentially worth trying to find a "I compile my code fresh, some in C++17 and some in 20, and it should still work together" solution.

[dwblaikie]

Yeah, any case of making a ctor constinit-able across versions. (hmm, what do we do if we /optimize/ code to the point where it's constinit? I guess we still put it in a writable section to ensure it can be redundantly initialized if it gets picked up by a less optimized version of the ctor? Or do we avoid it in some other way?)

(Hmm, if we optimize out the initialization, we still emit the whole guard acquire/release/etc - and the only thing it guards is llvm.invariant.start.p0 call... at least it doesn't modify the global object, so there's no actual race, etc... wonder if we could optimize that a bit more somehow)

If we did put the variable, guard, and function in the same comdat, at least they'd be discarded by the linker if using --gc-sections maybe? Or is there some other way to say "if you keep 1A, you need to keep 1B as well" but you can discard 1A even if you do keep 1B or something.. I 'm not sure.