Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 3, 2024
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions libcxx/include/__expected/bad_expected_access.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ _LIBCPP_BEGIN_NAMESPACE_STD
template <class _Err>
class bad_expected_access;

_LIBCPP_DIAGNOSTIC_PUSH
_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wweak-vtables")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

template <>
class bad_expected_access<void> : public exception {
protected:
Expand All @@ -44,6 +46,7 @@ class bad_expected_access<void> : public exception {
// it adds deployment target restrictions.
_LIBCPP_HIDE_FROM_ABI_VIRTUAL const char* what() const noexcept override { return "bad access to std::expected"; }
};
_LIBCPP_DIAGNOSTIC_POP

template <class _Err>
class bad_expected_access : public bad_expected_access<void> {
Expand Down
Loading