[PATCH] D14411: Use __attribute__((internal_linkage)) when available.
eugenis created this revision. eugenis added reviewers: EricWF, mclow.lists. eugenis added a subscriber: cfe-commits. eugenis set the repository for this revision to rL LLVM. Use __attribute__((internal_linkage)) instead of always_inline and visibility("hidden") when it is available. Repository: rL LLVM http://reviews.llvm.org/D14411 Files: include/__config Index: include/__config === --- include/__config +++ include/__config @@ -233,15 +233,23 @@ #endif #ifndef _LIBCPP_INLINE_VISIBILITY +#if __has_attribute(internal_linkage) +#define _LIBCPP_INLINE_VISIBILITY __attribute__ ((internal_linkage)) +#else #define _LIBCPP_INLINE_VISIBILITY __attribute__ ((__visibility__("hidden"), __always_inline__)) #endif +#endif #ifndef _LIBCPP_EXCEPTION_ABI #define _LIBCPP_EXCEPTION_ABI __attribute__ ((__visibility__("default"))) #endif #ifndef _LIBCPP_ALWAYS_INLINE -#define _LIBCPP_ALWAYS_INLINE __attribute__ ((__visibility__("hidden"), __always_inline__)) +#if __has_attribute(internal_linkage) +#define _LIBCPP_ALWAYS_INLINE __attribute__ ((internal_linkage)) +#else +#define _LIBCPP_ALWAYS_INLINE __attribute__ ((__visibility__("hidden"), __always_inline__)) +#endif #endif #if defined(__clang__) Index: include/__config === --- include/__config +++ include/__config @@ -233,15 +233,23 @@ #endif #ifndef _LIBCPP_INLINE_VISIBILITY +#if __has_attribute(internal_linkage) +#define _LIBCPP_INLINE_VISIBILITY __attribute__ ((internal_linkage)) +#else #define _LIBCPP_INLINE_VISIBILITY __attribute__ ((__visibility__("hidden"), __always_inline__)) #endif +#endif #ifndef _LIBCPP_EXCEPTION_ABI #define _LIBCPP_EXCEPTION_ABI __attribute__ ((__visibility__("default"))) #endif #ifndef _LIBCPP_ALWAYS_INLINE -#define _LIBCPP_ALWAYS_INLINE __attribute__ ((__visibility__("hidden"), __always_inline__)) +#if __has_attribute(internal_linkage) +#define _LIBCPP_ALWAYS_INLINE __attribute__ ((internal_linkage)) +#else +#define _LIBCPP_ALWAYS_INLINE __attribute__ ((__visibility__("hidden"), __always_inline__)) +#endif #endif #if defined(__clang__) ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D14411: Use __attribute__((internal_linkage)) when available.
ldionne added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. I think this should be closed since the work we did on `internal_linkage` in 2018. @eugenis Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D14411/new/ https://reviews.llvm.org/D14411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D14411: Use __attribute__((internal_linkage)) when available.
eugenis added a dependency: D12502: [libcxx] Better constain tuples constructors -- Fix PR23256 and PR22806. eugenis added a comment. Note, this breaks tuple_cat.pass.cpp test. With -O0, replacing always_inline with internal_linkage results in less optimization being done (namely, no inlining happens). This ends up exposing https://llvm.org/bugs/show_bug.cgi?id=23256 which is fixed by http://reviews.llvm.org/D12502 The same failure can be reproduced in the current ToT libc++ by running this test with -O2. This change depends on http://reviews.llvm.org/D12502. Repository: rL LLVM http://reviews.llvm.org/D14411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D14411: Use __attribute__((internal_linkage)) when available.
eugenis added a dependency: D15404: Cleanup: move visibility/linkage attributes to the first declaration (part 2).. eugenis added a comment. This change depends on http://reviews.llvm.org/D15404. Repository: rL LLVM http://reviews.llvm.org/D14411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D14411: Use __attribute__((internal_linkage)) when available.
rsmith added a subscriber: rsmith. Comment at: include/__config:237 @@ +236,3 @@ +#if __has_attribute(internal_linkage) +#define _LIBCPP_INLINE_VISIBILITY __attribute__ ((internal_linkage)) +#else Use `__internal_linkage__` here. Users are allowed to #define `internal_linkage` before including this header. (x4) Repository: rL LLVM http://reviews.llvm.org/D14411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D14411: Use __attribute__((internal_linkage)) when available.
EricWF added a comment. Why does this depend on http://reviews.llvm.org/D15404? Repository: rL LLVM http://reviews.llvm.org/D14411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D14411: Use __attribute__((internal_linkage)) when available.
EricWF added a comment. In http://reviews.llvm.org/D14411#306716, @EricWF wrote: > Why does this depend on http://reviews.llvm.org/D15404? Woops, I meant the tuple patch but I see the other comment now. I'm curious as to how inlininging ends up affecting which overload's SFINAE are evaluated. Drive by comment: Is the change from `__attribute__((__visibility__("hidden"), __always_inline__))` to `__attribute__((__internal_linkage__))` ABI compatible? Repository: rL LLVM http://reviews.llvm.org/D14411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D14411: Use __attribute__((internal_linkage)) when available.
eugenis added a comment. In http://reviews.llvm.org/D14411#306722, @EricWF wrote: > In http://reviews.llvm.org/D14411#306716, @EricWF wrote: > > > Why does this depend on http://reviews.llvm.org/D15404? > > > Woops, I meant the tuple patch but I see the other comment now. I'm curious > as to how inlininging ends up affecting which overload's SFINAE are evaluated. As I understand, in that test we pick a default(?) constructor instead of a move(?) constructor, and end up reading uninitialized memory. Then any code change can affect the test result. Like adding -O2 does, for example. > Drive by comment: Is the change from > `__attribute__((__visibility__("hidden"), __always_inline__))` to > `__attribute__((__internal_linkage__))` ABI compatible? I think so. I'll verify tomorrow. Repository: rL LLVM http://reviews.llvm.org/D14411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D14411: Use __attribute__((internal_linkage)) when available.
eugenis added dependencies: D15433: [libcxx] Remove inline/visibility attributes from exported template methods in valarray., D15432: [libcxx] Move member function definition before it's explicit template instantiation declaration in to satisfy GCC.. eugenis added a comment. Depends on http://reviews.llvm.org/D15432. Depends on http://reviews.llvm.org/D15433. With all that change, the switch to internal_linkage attribute removes 3 symbols from the libc++ export table, all in basic_string: insert(..., InputIterator insert(..., ForwardIterator replace(..., InputIterator These are template methods of a template class. They are instantiated only in functions/methods that are marked with LIBCPP_INLINE_VISIBILITY; normally they are exported as linkonce_odr; after the internal_linkage switch they are not instantiated at all because their callers are never evaluated. Do you think this is an ABI break? Repository: rL LLVM http://reviews.llvm.org/D14411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D14411: Use __attribute__((internal_linkage)) when available.
eugenis added a comment. With http://reviews.llvm.org/D15434, there is no difference in libc++ export list with the switch to internal_linkage. Repository: rL LLVM http://reviews.llvm.org/D14411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D14411: Use __attribute__((internal_linkage)) when available.
EricWF added a comment. Sorry about the long delay in reviewing this. @eugenis Are you still able/willing to proceed with this? Repository: rL LLVM http://reviews.llvm.org/D14411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D14411: Use __attribute__((internal_linkage)) when available.
eugenis added a comment. Yes, I'd like to try. I think this is blocked on the changes that move visibility attributes to the first declaration, right? Also, re: cfi-commits thread for r255177, it appears that on Mac we can neither hide nor expose existing methods (i.e. if something was hidden, it can not be exported and vice versa; basically, the set of exports must stay exactly the same). Repository: rL LLVM http://reviews.llvm.org/D14411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits