[PATCH] D14411: Use __attribute__((internal_linkage)) when available.

2015-11-05 Thread Evgeniy Stepanov via cfe-commits 
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.

2020-03-09 Thread Louis Dionne via Phabricator via cfe-commits 
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.

2015-12-09 Thread Evgeniy Stepanov via cfe-commits 
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.

2015-12-09 Thread Evgeniy Stepanov via cfe-commits 
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.

2015-12-09 Thread Richard Smith via cfe-commits 
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.

2015-12-09 Thread Eric Fiselier via cfe-commits 
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.

2015-12-09 Thread Eric Fiselier via cfe-commits 
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.

2015-12-09 Thread Evgeniy Stepanov via cfe-commits 
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.

2015-12-10 Thread Evgeniy Stepanov via cfe-commits 
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.

2015-12-10 Thread Evgeniy Stepanov via cfe-commits 
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.

2016-04-21 Thread Eric Fiselier via cfe-commits 
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.

2016-04-21 Thread Evgeniy Stepanov via cfe-commits 
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