[libc++] Disables -Wweak-vtables diagnostics.

[mordante]

This is a preparation to use Clang HEAD in the CI.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

This is a preparation to use Clang HEAD in the CI.

---

Full diff: https://github.com/llvm/llvm-project/pull/85577.diff

1 Files Affected:

• (modified) libcxx/include/__expected/bad_expected_access.h (+3)

diff --git a/libcxx/include/__expected/bad_expected_access.h b/libcxx/include/__expected/bad_expected_access.h
index 27f01d9350eea6..32547eba657633 100644
--- a/libcxx/include/__expected/bad_expected_access.h
+++ b/libcxx/include/__expected/bad_expected_access.h
@@ -27,6 +27,8 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 template <class _Err>
 class bad_expected_access;
 
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wweak-vtables")
 template <>
 class bad_expected_access<void> : public exception {
 protected:
@@ -58,6 +60,7 @@ class bad_expected_access : public bad_expected_access<void> {
 private:
   _Err __unex_;
 };
+_LIBCPP_DIAGNOSTIC_POP
 
 _LIBCPP_END_NAMESPACE_STD
 

[philnik777]

Please add a comment why we disabled -Wweak-vtables here. We shouldn't just disable it because it happens to be diagnosed. We generally try to avoid weak vtables. FWIW I don't think we should disable it here either. We should instead provide a strong definition in the dylib if it's available on the target.

[mordante]

We could. Do you think that would that be worth an ABI break?

[philnik777]

I don't think we have to make it an ABI break. The weak symbols have, as the name suggests, weak linkage. So we can simply provide a strong definition in the dylib and only enable the generation of weak symbols on platforms which don't have them in the dylib yet. Similar to how we conditionally provide additional instantiations of iostream types.

[mordante]

AFAIK We only use weak symbols for the replaceable functions and some debug functions. Not for other functions. I'm not sure whether we want do that. It would allow users to provide strong definitions of destructors they should not provide.

I can't find the example you mean, do you mean the templates guarded by _LIBCPP_AVAILABILITY_HAS_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1 ?

[philnik777]

AFAIK We only use weak symbols for the replaceable functions and some debug functions. Not for other functions. I'm not sure whether we want do that. It would allow users to provide strong definitions of destructors they should not provide.

inline symbols have weak linkage, we don't have to give that explicitly. You can see that in https://godbolt.org/z/a3P4P7qhE. The .weak test() gives test() weak linkage, and explicitly giving it weak linkage doesn't make any difference.

I can't find the example you mean, do you mean the templates guarded by _LIBCPP_AVAILABILITY_HAS_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1 ?

Yes.

[philnik777]

There is only a difference for special member functions, since there are multiple sometimes. I don't know why clang decides to call the complete object destructor instead of the base object destructor when adding [[gnu::weak]] (maybe because it's considered replaceable with [[gnu::weak]]), but I don't think that's relevant. The fact that they are weak symbols without [[gnu::weak]] doesn't change.

What exactly did you try to add a definition to the dylib?

[mordante]

In the header I removed _LIBCPP_HIDE_FROM_ABI_VIRTUAL and changed = default to {}, I also tried with _LIBCPP_OVERRIDABLE_FUNC_VIS and _LIBCPP_WEAK in the dylib I tried with and without _LIBCPP_WEAK. And all permutations of this; but clang dislikes the re-declaration.

[mordante]

@philnik777 friendly ping. I really like to move this patch forward since it blocks testing with Clang 19 in the CI.

[ldionne]

@mordante Is Clang trunk complaining about the vtable of bad_expected_access<void> only, or about the one for the base template as well? Cause for the base template we can't do anything, but we could indeed do what @philnik777 said for the <void> specialization. We were basically "lazy" when we introduced bad_expected_access and decided not to put it in the dylib, but we could have.

[mordante]

I checked and only disabling it for the void specialization works. However I tried to use _LIBCPP_WEAK with that specialization and that resulted in compilation errors. If you or @philnik777 know directly how to solve this feel free to commit patches to this review to solve it.

[philnik777]

As the comment says. (Why TF is GitHub preventing me from requesting changes without a comment?)

[ldionne]

I am fine with this change as-is. It unblocks a Clang migration and it is consistent with what we do for bad_function_call. We should investigate how we can use availability instead for both bad_function_call and bad_expected_access, but that can (and should) be done in a separate patch cause it's non-trivial from an ABI standpoint.

[philnik777]

I'd still like a comment why we disable it or a TODO that it shouldn't be the case.

[ldionne]

I'd still like a comment why we disable it or a TODO that it shouldn't be the case.

Is this enough? #87390

[philnik777]

I'd still like a comment why we disable it or a TODO that it shouldn't be the case.

Is this enough? #87390

Sure.