This is an archive of the discontinued LLVM Phabricator instance.

[libc++][filesystem] Avoid using anonymous namespaces in support headers
ClosedPublic

Authored by ldionne on Jun 7 2023, 9:17 AM.

Details

Summary

This avoids using anonymous namespaces in headers and ensures that
the various helper functions get deduplicated across the TUs
implementing <filesystem>. Otherwise, we'd get a definition of
these helper functions in each TU where they are used, which is
entirely unnecessary.

Diff Detail

Event Timeline

ldionne created this revision.Jun 7 2023, 9:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2023, 9:17 AM
ldionne requested review of this revision.Jun 7 2023, 9:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2023, 9:17 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik requested changes to this revision.Jun 7 2023, 9:25 AM
philnik added a subscriber: philnik.

I don't think there is any need to mark the functions as _LIBCPP_HIDE_FROM_ABI. These are all implementation details of the dylib, which is built with -fvisibility=hidden, so these symbols can never escape the dylib, making this pure noise.

This revision now requires changes to proceed.Jun 7 2023, 9:25 AM

I don't think there is any need to mark the functions as _LIBCPP_HIDE_FROM_ABI. These are all implementation details of the dylib, which is built with -fvisibility=hidden, so these symbols can never escape the dylib, making this pure noise.

These functions live in headers and they are included in multiple .cpp files that are linked together to create the dylib. Without HIDE_FROM_ABI and with the anonymous namespace, we get duplicate definitions in each .o file created using these headers. Also, the compiler complains about internal-linkage functions being unused in some of the .cpp files.

I don't think there is any need to mark the functions as _LIBCPP_HIDE_FROM_ABI. These are all implementation details of the dylib, which is built with -fvisibility=hidden, so these symbols can never escape the dylib, making this pure noise.

These functions live in headers and they are included in multiple .cpp files that are linked together to create the dylib. Without HIDE_FROM_ABI and with the anonymous namespace, we get duplicate definitions in each .o file created using these headers. Also, the compiler complains about internal-linkage functions being unused in some of the .cpp files.

I understand that, but why not simply remove the anonymous namespace? What's the point of additionally adding _LIBCPP_HIDE_FROM_ABI?

I understand that, but why not simply remove the anonymous namespace? What's the point of additionally adding _LIBCPP_HIDE_FROM_ABI?

I guess we build the dylib with -fvisibility=hidden, so we don't need it technically. That was the original purpose. We could also argue that it's more consistent with the rest of the code, but I agree it's technically not needed.

So I can either keep it for consistency, or just remove the namespace { } bit. Thoughts?

I understand that, but why not simply remove the anonymous namespace? What's the point of additionally adding _LIBCPP_HIDE_FROM_ABI?

I guess we build the dylib with -fvisibility=hidden, so we don't need it technically. That was the original purpose. We could also argue that it's more consistent with the rest of the code, but I agree it's technically not needed.

So I can either keep it for consistency, or just remove the namespace { } bit. Thoughts?

Most of the stuff in src/ is written very similar to the headers, but IMO we should strive to make that stuff more maintainable. i.e. avoid anything that isn't required, like _Uglification, adding _LIBCPP_HIDE_FROM_ABI everywhere etc. I'm pretty sure everybody would love to remove that shit in the headers if it weren't necessary.

Actually I just thought about this and if users are linking against static libc++.a, then they could potentially end up with symbols with the same name from different versions of libc++, which would be an ODR violation. In other words, we do need the same precautions in our own built library as we have in our headers due to static archives. So I believe _LIBCPP_HIDE_FROM_ABI is necessary if we drop the anonymous namespace. Do you agree?

ldionne updated this revision to Diff 529411.Jun 7 2023, 12:48 PM

Rebase to poke CI

Actually I just thought about this and if users are linking against static libc++.a, then they could potentially end up with symbols with the same name from different versions of libc++, which would be an ODR violation. In other words, we do need the same precautions in our own built library as we have in our headers due to static archives. So I believe _LIBCPP_HIDE_FROM_ABI is necessary if we drop the anonymous namespace. Do you agree?

How would users end up with multiple versions of a function? All the functions are exclusively in libc++.a, and linking against multiple versions or that will definitely blow up.

Mordante accepted this revision as: Mordante.Jun 8 2023, 10:38 AM
Mordante added a subscriber: Mordante.

I understand that, but why not simply remove the anonymous namespace? What's the point of additionally adding _LIBCPP_HIDE_FROM_ABI?

I guess we build the dylib with -fvisibility=hidden, so we don't need it technically. That was the original purpose. We could also argue that it's more consistent with the rest of the code, but I agree it's technically not needed.

So I can either keep it for consistency, or just remove the namespace { } bit. Thoughts?

Most of the stuff in src/ is written very similar to the headers, but IMO we should strive to make that stuff more maintainable. i.e. avoid anything that isn't required, like _Uglification, adding _LIBCPP_HIDE_FROM_ABI everywhere etc. I'm pretty sure everybody would love to remove that shit in the headers if it weren't necessary.

I actually prefer consistency in our code base so using _Uglified everywhere. It also makes it easier to move code from the dylib to the header. (Obviously that has its own issues.)

LGTM!

libcxx/src/filesystem/filesystem_common.h
119 ↗(On Diff #529411)

It would be nice to do this in a followup. The dylib requires C++20.

121 ↗(On Diff #529411)

IIRC inline is not needed for templates, or am I mistaken?

libcxx/src/filesystem/posix_compat.h
248

This wchar_t is not guarded by _LIBCPP_HAS_NO_WIDE_CHARACTERS, maybe something for a followup patch.

ldionne marked 3 inline comments as done.Jun 12 2023, 8:47 AM

How would users end up with multiple versions of a function? All the functions are exclusively in libc++.a, and linking against multiple versions or that will definitely blow up.

Why would linking against multiple versions of libc++.a blow up? I know it's not a good idea but I'm curious if you know of specific concerns, except e.g. global stream initialization.

I actually prefer consistency in our code base so using _Uglified everywhere. It also makes it easier to move code from the dylib to the header. (Obviously that has its own issues.)

That's my preference as well.

libcxx/src/filesystem/filesystem_common.h
119 ↗(On Diff #529411)

Made a TODO locally.

121 ↗(On Diff #529411)

This is an explicit specialization, I think those are not linkonce-odr by default. Godbolt confirms this: https://godbolt.org/z/Wzb4Esb35

libcxx/src/filesystem/posix_compat.h
248

Added a todo locally.

How would users end up with multiple versions of a function? All the functions are exclusively in libc++.a, and linking against multiple versions or that will definitely blow up.

Why would linking against multiple versions of libc++.a blow up? I know it's not a good idea but I'm curious if you know of specific concerns, except e.g. global stream initialization.

You have multiple strong definitions of lots of symbols. How would that not blow up?

Mordante added inline comments.Jun 12 2023, 10:22 AM
libcxx/src/filesystem/filesystem_common.h
121 ↗(On Diff #529411)

Thanks for the info!

ldionne marked 3 inline comments as done.Jun 21 2023, 8:26 AM

How would users end up with multiple versions of a function? All the functions are exclusively in libc++.a, and linking against multiple versions or that will definitely blow up.

Why would linking against multiple versions of libc++.a blow up? I know it's not a good idea but I'm curious if you know of specific concerns, except e.g. global stream initialization.

You have multiple strong definitions of lots of symbols. How would that not blow up?

Right, obviously, not sure what I was thinking. This means we could just drop the anonymous namespace instead and it would work as well. I'd prefer using HIDE_FROM_ABI because it makes this clearer and more consistent with the rest of the code base, but let's start with just dropping anonymous namespaces.

ldionne updated this revision to Diff 533282.Jun 21 2023, 8:44 AM
ldionne retitled this revision from [libc++][filesystem] Use _LIBCPP_HIDE_FROM_ABI in common headers to [libc++][filesystem] Avoid using anonymous namespaces in support headers.
ldionne edited the summary of this revision. (Show Details)

Switch from HIDE_FROM_ABI to just inline.

philnik accepted this revision.Jun 21 2023, 9:34 AM
This revision is now accepted and ready to land.Jun 21 2023, 9:34 AM
ldionne updated this revision to Diff 533414.Jun 21 2023, 3:00 PM

Rebase to fix format issues.

ldionne updated this revision to Diff 533532.Jun 22 2023, 3:44 AM

Format mis-formatted header.

ldionne updated this revision to Diff 533701.Jun 22 2023, 11:06 AM

Add missing inline on function.