[FFmpeg-devel] [PATCH 1/2] libavutil: Add av_visibility_hidden for setting hidden symbol visibility

[FFmpeg-devel] [PATCH 1/2] libavutil: Add av_visibility_hidden for setting hidden symbol visibility

[Martin Storsjö]

Signed-off-by: Martin Storsjö <martin@martin.st>
---
 doc/APIchanges         | 3 +++
 libavutil/attributes.h | 6 ++++++
 libavutil/version.h    | 2 +-
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/doc/APIchanges b/doc/APIchanges
index 20b944933a..5d84bc27d7 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -14,6 +14,9 @@ libavutil:     2021-04-27
 
 API changes, most recent first:
 
+2022-xx-xx - xxxxxxxxxx - lavu 57.28.100 - attributes.h
+  Add av_visibility_hidden, for setting hidden visibilty on symbols.
+
 2022-06-12 - xxxxxxxxxx - lavf 59.25.100 - avio.h
   Add avio_vprintf(), similar to avio_printf() but allow to use it
   from within a function taking a variable argument list as input.
diff --git a/libavutil/attributes.h b/libavutil/attributes.h
index 04c615c952..dc4c88932c 100644
--- a/libavutil/attributes.h
+++ b/libavutil/attributes.h
@@ -170,4 +170,10 @@
 #    define av_noreturn
 #endif
 
+#if (AV_GCC_VERSION_AT_LEAST(4,0) || defined(__clang__)) && (defined(__ELF__) || defined(__MACH__))
+#    define av_visibility_hidden __attribute__((visibility("hidden")))
+#else
+#    define av_visibility_hidden
+#endif
+
 #endif /* AVUTIL_ATTRIBUTES_H */
diff --git a/libavutil/version.h b/libavutil/version.h
index 2e9e02dda8..87178e9e9a 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -79,7 +79,7 @@
  */
 
 #define LIBAVUTIL_VERSION_MAJOR  57
-#define LIBAVUTIL_VERSION_MINOR  27
+#define LIBAVUTIL_VERSION_MINOR  28
 #define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
-- 
2.25.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/2] libavcodec: Set hidden visibility on global symbols accessed from AArch64 assembly

[Martin Storsjö]

The AArch64 assembly accesses those symbols directly, without
indirection via e.g. the GOT on ELF. In order for this not to
require text relocations, those symbols need to be resolved fully
at link time, i.e. those symbols can't be interposable.

Normally, so far, this is achieved when linking shared libraries
in two ways; we have a version script (libavcodec/libavcodec.v) which
marks all symbols that don't start with av* as local. Additionally,
we try to add -Wl,-Bsymbolic to the linker options if supported,
making sure that such symbol references are resolved fully at link
time, instead of making them interposable.

When the libavcodec static library is linked into another shared
library, there's no guarantee that it uses similar options (even though
that would be favourable), which would end up requiring text relocations
in the AArch64 assembly.

Explicitly mark the symbols that are accessed from AArch64 assembly
as hidden, so that they are resolved fully at link time even without
the version script and -Wl,-Bsymbolic.

Signed-off-by: Martin Storsjö <martin@martin.st>
---
 libavcodec/aacsbrdata.h | 2 +-
 libavcodec/fft.h        | 2 +-
 libavcodec/vp9dsp.c     | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/libavcodec/aacsbrdata.h b/libavcodec/aacsbrdata.h
index 7a11594c9b..aa382c4b52 100644
--- a/libavcodec/aacsbrdata.h
+++ b/libavcodec/aacsbrdata.h
@@ -268,7 +268,7 @@ static const int8_t sbr_offset[6][16] = {
 };
 
 /* First eight entries repeated at end to simplify SIMD implementations. */
-const DECLARE_ALIGNED(16, INTFLOAT, AAC_RENAME(ff_sbr_noise_table))[][2] = {
+const av_visibility_hidden DECLARE_ALIGNED(16, INTFLOAT, AAC_RENAME(ff_sbr_noise_table))[][2] = {
 {Q31(-0.99948153278296f), Q31(-0.59483417516607f)}, {Q31( 0.97113454393991f), Q31(-0.67528515225647f)},
 {Q31( 0.14130051758487f), Q31(-0.95090983575689f)}, {Q31(-0.47005496701697f), Q31(-0.37340549728647f)},
 {Q31( 0.80705063769351f), Q31( 0.29653668284408f)}, {Q31(-0.38981478896926f), Q31( 0.89572605717087f)},
diff --git a/libavcodec/fft.h b/libavcodec/fft.h
index 706c9d07f5..c2241fbc7c 100644
--- a/libavcodec/fft.h
+++ b/libavcodec/fft.h
@@ -114,7 +114,7 @@ void ff_init_ff_cos_tabs(int index);
 #endif
 
 #define COSTABLE(size) \
-    COSTABLE_CONST DECLARE_ALIGNED(32, FFTSample, FFT_NAME(ff_cos_##size))[size/2]
+    COSTABLE_CONST av_visibility_hidden DECLARE_ALIGNED(32, FFTSample, FFT_NAME(ff_cos_##size))[size/2]
 
 extern COSTABLE(16);
 extern COSTABLE(32);
diff --git a/libavcodec/vp9dsp.c b/libavcodec/vp9dsp.c
index d8ddf74d4f..1be942d78b 100644
--- a/libavcodec/vp9dsp.c
+++ b/libavcodec/vp9dsp.c
@@ -29,7 +29,7 @@
 
 #include "vp9dsp.h"
 
-const DECLARE_ALIGNED(16, int16_t, ff_vp9_subpel_filters)[3][16][8] = {
+const av_visibility_hidden DECLARE_ALIGNED(16, int16_t, ff_vp9_subpel_filters)[3][16][8] = {
     [FILTER_8TAP_REGULAR] = {
         {  0,  0,   0, 128,   0,   0,  0,  0 },
         {  0,  1,  -5, 126,   8,  -3,  1,  0 },
-- 
2.25.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] libavcodec: Set hidden visibility on global symbols accessed from AArch64 assembly

[Andreas Rheinhardt]

Martin Storsjö:
> The AArch64 assembly accesses those symbols directly, without
> indirection via e.g. the GOT on ELF. In order for this not to
> require text relocations, those symbols need to be resolved fully
> at link time, i.e. those symbols can't be interposable.
> 
> Normally, so far, this is achieved when linking shared libraries
> in two ways; we have a version script (libavcodec/libavcodec.v) which
> marks all symbols that don't start with av* as local. Additionally,
> we try to add -Wl,-Bsymbolic to the linker options if supported,
> making sure that such symbol references are resolved fully at link
> time, instead of making them interposable.
> 
> When the libavcodec static library is linked into another shared
> library, there's no guarantee that it uses similar options (even though
> that would be favourable), which would end up requiring text relocations
> in the AArch64 assembly.
> 
> Explicitly mark the symbols that are accessed from AArch64 assembly
> as hidden, so that they are resolved fully at link time even without
> the version script and -Wl,-Bsymbolic.
> 
> Signed-off-by: Martin Storsjö <martin@martin.st>
> ---
>  libavcodec/aacsbrdata.h | 2 +-
>  libavcodec/fft.h        | 2 +-
>  libavcodec/vp9dsp.c     | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/aacsbrdata.h b/libavcodec/aacsbrdata.h
> index 7a11594c9b..aa382c4b52 100644
> --- a/libavcodec/aacsbrdata.h
> +++ b/libavcodec/aacsbrdata.h
> @@ -268,7 +268,7 @@ static const int8_t sbr_offset[6][16] = {
>  };
>  
>  /* First eight entries repeated at end to simplify SIMD implementations. */
> -const DECLARE_ALIGNED(16, INTFLOAT, AAC_RENAME(ff_sbr_noise_table))[][2] = {
> +const av_visibility_hidden DECLARE_ALIGNED(16, INTFLOAT, AAC_RENAME(ff_sbr_noise_table))[][2] = {
>  {Q31(-0.99948153278296f), Q31(-0.59483417516607f)}, {Q31( 0.97113454393991f), Q31(-0.67528515225647f)},
>  {Q31( 0.14130051758487f), Q31(-0.95090983575689f)}, {Q31(-0.47005496701697f), Q31(-0.37340549728647f)},
>  {Q31( 0.80705063769351f), Q31( 0.29653668284408f)}, {Q31(-0.38981478896926f), Q31( 0.89572605717087f)},
> diff --git a/libavcodec/fft.h b/libavcodec/fft.h
> index 706c9d07f5..c2241fbc7c 100644
> --- a/libavcodec/fft.h
> +++ b/libavcodec/fft.h
> @@ -114,7 +114,7 @@ void ff_init_ff_cos_tabs(int index);
>  #endif
>  
>  #define COSTABLE(size) \
> -    COSTABLE_CONST DECLARE_ALIGNED(32, FFTSample, FFT_NAME(ff_cos_##size))[size/2]
> +    COSTABLE_CONST av_visibility_hidden DECLARE_ALIGNED(32, FFTSample, FFT_NAME(ff_cos_##size))[size/2]
>  
>  extern COSTABLE(16);
>  extern COSTABLE(32);
> diff --git a/libavcodec/vp9dsp.c b/libavcodec/vp9dsp.c
> index d8ddf74d4f..1be942d78b 100644
> --- a/libavcodec/vp9dsp.c
> +++ b/libavcodec/vp9dsp.c
> @@ -29,7 +29,7 @@
>  
>  #include "vp9dsp.h"
>  
> -const DECLARE_ALIGNED(16, int16_t, ff_vp9_subpel_filters)[3][16][8] = {
> +const av_visibility_hidden DECLARE_ALIGNED(16, int16_t, ff_vp9_subpel_filters)[3][16][8] = {

Shouldn't you set this in vp9dsp.h, so that all users of it can benefit
from knowing that this symbol is always in the same DSO?

>      [FILTER_8TAP_REGULAR] = {
>          {  0,  0,   0, 128,   0,   0,  0,  0 },
>          {  0,  1,  -5, 126,   8,  -3,  1,  0 },

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] libavutil: Add av_visibility_hidden for setting hidden symbol visibility

[Andreas Rheinhardt]

Martin Storsjö:
> Signed-off-by: Martin Storsjö <martin@martin.st>
> ---
>  doc/APIchanges         | 3 +++
>  libavutil/attributes.h | 6 ++++++
>  libavutil/version.h    | 2 +-
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 20b944933a..5d84bc27d7 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -14,6 +14,9 @@ libavutil:     2021-04-27
>  
>  API changes, most recent first:
>  
> +2022-xx-xx - xxxxxxxxxx - lavu 57.28.100 - attributes.h
> +  Add av_visibility_hidden, for setting hidden visibilty on symbols.
> +
>  2022-06-12 - xxxxxxxxxx - lavf 59.25.100 - avio.h
>    Add avio_vprintf(), similar to avio_printf() but allow to use it
>    from within a function taking a variable argument list as input.
> diff --git a/libavutil/attributes.h b/libavutil/attributes.h
> index 04c615c952..dc4c88932c 100644
> --- a/libavutil/attributes.h
> +++ b/libavutil/attributes.h
> @@ -170,4 +170,10 @@
>  #    define av_noreturn
>  #endif
>  
> +#if (AV_GCC_VERSION_AT_LEAST(4,0) || defined(__clang__)) && (defined(__ELF__) || defined(__MACH__))
> +#    define av_visibility_hidden __attribute__((visibility("hidden")))
> +#else
> +#    define av_visibility_hidden
> +#endif
> +
>  #endif /* AVUTIL_ATTRIBUTES_H */
> diff --git a/libavutil/version.h b/libavutil/version.h
> index 2e9e02dda8..87178e9e9a 100644
> --- a/libavutil/version.h
> +++ b/libavutil/version.h
> @@ -79,7 +79,7 @@
>   */
>  
>  #define LIBAVUTIL_VERSION_MAJOR  57
> -#define LIBAVUTIL_VERSION_MINOR  27
> +#define LIBAVUTIL_VERSION_MINOR  28
>  #define LIBAVUTIL_VERSION_MICRO 100
>  
>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \

Hidden stuff is by definition not part of installed headers, so that
there is no point in adding a public define for this.
(Anyway: visibilty is not the correct spelling.)

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] libavutil: Add av_visibility_hidden for setting hidden symbol visibility

[Martin Storsjö]

On Mon, 11 Jul 2022, Andreas Rheinhardt wrote:

> Martin Storsjö:
>> Signed-off-by: Martin Storsjö <martin@martin.st>
>> ---
>>  doc/APIchanges         | 3 +++
>>  libavutil/attributes.h | 6 ++++++
>>  libavutil/version.h    | 2 +-
>>  3 files changed, 10 insertions(+), 1 deletion(-)
>> 
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index 20b944933a..5d84bc27d7 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -14,6 +14,9 @@ libavutil:     2021-04-27
>>
>>  API changes, most recent first:
>> 
>> +2022-xx-xx - xxxxxxxxxx - lavu 57.28.100 - attributes.h
>> +  Add av_visibility_hidden, for setting hidden visibilty on symbols.
>> +
>>  2022-06-12 - xxxxxxxxxx - lavf 59.25.100 - avio.h
>>    Add avio_vprintf(), similar to avio_printf() but allow to use it
>>    from within a function taking a variable argument list as input.
>> diff --git a/libavutil/attributes.h b/libavutil/attributes.h
>> index 04c615c952..dc4c88932c 100644
>> --- a/libavutil/attributes.h
>> +++ b/libavutil/attributes.h
>> @@ -170,4 +170,10 @@
>>  #    define av_noreturn
>>  #endif
>> 
>> +#if (AV_GCC_VERSION_AT_LEAST(4,0) || defined(__clang__)) && (defined(__ELF__) || defined(__MACH__))
>> +#    define av_visibility_hidden __attribute__((visibility("hidden")))
>> +#else
>> +#    define av_visibility_hidden
>> +#endif
>> +
>>  #endif /* AVUTIL_ATTRIBUTES_H */
>> diff --git a/libavutil/version.h b/libavutil/version.h
>> index 2e9e02dda8..87178e9e9a 100644
>> --- a/libavutil/version.h
>> +++ b/libavutil/version.h
>> @@ -79,7 +79,7 @@
>>   */
>>
>>  #define LIBAVUTIL_VERSION_MAJOR  57
>> -#define LIBAVUTIL_VERSION_MINOR  27
>> +#define LIBAVUTIL_VERSION_MINOR  28
>>  #define LIBAVUTIL_VERSION_MICRO 100
>>
>>  #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>
> Hidden stuff is by definition not part of installed headers, so that
> there is no point in adding a public define for this.

Good point - attribute.h would otherwise have been the natural spot, but I 
agree that it'd be better to not make it public at all. In what header 
would you prefer to have it?

// Martin
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] libavutil: Add av_visibility_hidden for setting hidden symbol visibility

[Andreas Rheinhardt]

Martin Storsjö:
> On Mon, 11 Jul 2022, Andreas Rheinhardt wrote:
> 
>> Martin Storsjö:
>>> Signed-off-by: Martin Storsjö <martin@martin.st>
>>> ---
>>>  doc/APIchanges         | 3 +++
>>>  libavutil/attributes.h | 6 ++++++
>>>  libavutil/version.h    | 2 +-
>>>  3 files changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/doc/APIchanges b/doc/APIchanges
>>> index 20b944933a..5d84bc27d7 100644
>>> --- a/doc/APIchanges
>>> +++ b/doc/APIchanges
>>> @@ -14,6 +14,9 @@ libavutil:     2021-04-27
>>>
>>>  API changes, most recent first:
>>>
>>> +2022-xx-xx - xxxxxxxxxx - lavu 57.28.100 - attributes.h
>>> +  Add av_visibility_hidden, for setting hidden visibilty on symbols.
>>> +
>>>  2022-06-12 - xxxxxxxxxx - lavf 59.25.100 - avio.h
>>>    Add avio_vprintf(), similar to avio_printf() but allow to use it
>>>    from within a function taking a variable argument list as input.
>>> diff --git a/libavutil/attributes.h b/libavutil/attributes.h
>>> index 04c615c952..dc4c88932c 100644
>>> --- a/libavutil/attributes.h
>>> +++ b/libavutil/attributes.h
>>> @@ -170,4 +170,10 @@
>>>  #    define av_noreturn
>>>  #endif
>>>
>>> +#if (AV_GCC_VERSION_AT_LEAST(4,0) || defined(__clang__)) &&
>>> (defined(__ELF__) || defined(__MACH__))
>>> +#    define av_visibility_hidden __attribute__((visibility("hidden")))
>>> +#else
>>> +#    define av_visibility_hidden
>>> +#endif
>>> +
>>>  #endif /* AVUTIL_ATTRIBUTES_H */
>>> diff --git a/libavutil/version.h b/libavutil/version.h
>>> index 2e9e02dda8..87178e9e9a 100644
>>> --- a/libavutil/version.h
>>> +++ b/libavutil/version.h
>>> @@ -79,7 +79,7 @@
>>>   */
>>>
>>>  #define LIBAVUTIL_VERSION_MAJOR  57
>>> -#define LIBAVUTIL_VERSION_MINOR  27
>>> +#define LIBAVUTIL_VERSION_MINOR  28
>>>  #define LIBAVUTIL_VERSION_MICRO 100
>>>
>>>  #define LIBAVUTIL_VERSION_INT  
>>> AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
>>
>> Hidden stuff is by definition not part of installed headers, so that
>> there is no point in adding a public define for this.
> 
> Good point - attribute.h would otherwise have been the natural spot, but
> I agree that it'd be better to not make it public at all. In what header
> would you prefer to have it?
> 

The typical place we put such things is libavutil/internal.h.

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] libavutil: Add av_visibility_hidden for setting hidden symbol visibility

[Anton Khirnov]

Quoting Andreas Rheinhardt (2022-07-11 12:57:32)
> > 
> > Good point - attribute.h would otherwise have been the natural spot, but
> > I agree that it'd be better to not make it public at all. In what header
> > would you prefer to have it?
> > 
> 
> The typical place we put such things is libavutil/internal.h.

misc headers are evil

how about a non-installed attribute_internal.h?

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] libavutil: Add av_visibility_hidden for setting hidden symbol visibility

[Henrik Gramner]

On Mon, Jul 11, 2022 at 11:19 AM Martin Storsjö <martin@martin.st> wrote:
> +#if (AV_GCC_VERSION_AT_LEAST(4,0) || defined(__clang__)) && (defined(__ELF__) || defined(__MACH__))
> +#    define av_visibility_hidden __attribute__((visibility("hidden")))
> +#else
> +#    define av_visibility_hidden
> +#endif

The usual approach is to compile with -fvisibility=hidden and
explicitly flag exported API symbols.

Is there a reason for doing this the other way around?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] libavutil: Add av_visibility_hidden for setting hidden symbol visibility

[Triang3l]

Yes, making everything except for av_ hidden by default would be more 
consistent with the current build process, which includes libavcodec.v. 
Though, this is a special case that results not only in increasing the 
shared object file size if libavcodec.v is not used, which is 
undesirable, yet harmless, but also in making the library not linkable 
with PIC at all unless those symbols are hidden or forced to be resolved 
at link time some other way.

Thanks for implementing the fix very quickly, by the way!

I'd also suggest writing a comment in the code describing specifically 
the original issue that the current instances of the usage of 
visibility("hidden") resolves, so the reason why it's used there is not 
forgotten, and there's a clear pattern of relation between movrel X() 
and av_visibility_hidden to follow when adding new assembly code. Though 
if the convention is to rely on `git blame` for this purpose, that 
shouldn't be necessary.

— Triang3l

On 11/07/2022 15:12, Henrik Gramner wrote:
> On Mon, Jul 11, 2022 at 11:19 AM Martin Storsjö <martin@martin.st> wrote:
>> +#if (AV_GCC_VERSION_AT_LEAST(4,0) || defined(__clang__)) && (defined(__ELF__) || defined(__MACH__))
>> +#    define av_visibility_hidden __attribute__((visibility("hidden")))
>> +#else
>> +#    define av_visibility_hidden
>> +#endif
> The usual approach is to compile with -fvisibility=hidden and
> explicitly flag exported API symbols.
>
> Is there a reason for doing this the other way around?
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] libavutil: Add av_visibility_hidden for setting hidden symbol visibility

[Andreas Rheinhardt]

Henrik Gramner:
> On Mon, Jul 11, 2022 at 11:19 AM Martin Storsjö <martin@martin.st> wrote:
>> +#if (AV_GCC_VERSION_AT_LEAST(4,0) || defined(__clang__)) && (defined(__ELF__) || defined(__MACH__))
>> +#    define av_visibility_hidden __attribute__((visibility("hidden")))
>> +#else
>> +#    define av_visibility_hidden
>> +#endif
> 
> The usual approach is to compile with -fvisibility=hidden and
> explicitly flag exported API symbols.
> 
> Is there a reason for doing this the other way around?

-fvisibility=hidden only affects the visibility of symbols defined in
the currently compiled translation unit. It does not allow the compiler
to make assumptions about external declarations that are used in this
translation unit (in other words, it has to presume the worst: That it
comes from a different DSO). E.g. this is ff_rdft_end on 32bit x86 if
ff_fft_end is declared with an explicit hidden attribute:

000000bb <ff_rdft_end>:

  bb:	83 44 24 04 18       	addl   $0x18,0x4(%esp)

  c0:	e9 fc ff ff ff       	jmp    c1 <ff_rdft_end+0x6>

			c1: R_386_PC32	ff_fft_end


And this is the same function if one uses -fvisibility=hidden instead of
the attribute:

000000bb <ff_rdft_end>:

  bb:	53                   	push   %ebx

  bc:	e8 fc ff ff ff       	call   bd <ff_rdft_end+0x2>

			bd: R_386_PC32	__x86.get_pc_thunk.bx

  c1:	81 c3 02 00 00 00    	add    $0x2,%ebx

			c3: R_386_GOTPC	_GLOBAL_OFFSET_TABLE_

  c7:	83 ec 14             	sub    $0x14,%esp

  ca:	8b 44 24 1c          	mov    0x1c(%esp),%eax

  ce:	83 c0 18             	add    $0x18,%eax

  d1:	50                   	push   %eax

  d2:	e8 fc ff ff ff       	call   d3 <ff_rdft_end+0x18>

			d3: R_386_PLT32	ff_fft_end

  d7:	83 c4 18             	add    $0x18,%esp

  da:	5b                   	pop    %ebx

  db:	c3                   	ret


The code is the same as if one had not used -fvisibility=hidden at all.

Of course, adding the attribute to every function/object is way too much
effort; that's why the pragma exists.

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] libavutil: Add av_visibility_hidden for setting hidden symbol visibility

[Timo Rothenpieler]

On 11.07.2022 16:26, Andreas Rheinhardt wrote:
> Henrik Gramner:
>> On Mon, Jul 11, 2022 at 11:19 AM Martin Storsjö <martin@martin.st> wrote:
>>> +#if (AV_GCC_VERSION_AT_LEAST(4,0) || defined(__clang__)) && (defined(__ELF__) || defined(__MACH__))
>>> +#    define av_visibility_hidden __attribute__((visibility("hidden")))
>>> +#else
>>> +#    define av_visibility_hidden
>>> +#endif
>>
>> The usual approach is to compile with -fvisibility=hidden and
>> explicitly flag exported API symbols.
>>
>> Is there a reason for doing this the other way around?
> 
> -fvisibility=hidden only affects the visibility of symbols defined in
> the currently compiled translation unit. It does not allow the compiler
> to make assumptions about external declarations that are used in this
> translation unit (in other words, it has to presume the worst: That it
> comes from a different DSO). E.g. this is ff_rdft_end on 32bit x86 if
> ff_fft_end is declared with an explicit hidden attribute:
> 
> 000000bb <ff_rdft_end>:
> 
>    bb:	83 44 24 04 18       	addl   $0x18,0x4(%esp)
> 
>    c0:	e9 fc ff ff ff       	jmp    c1 <ff_rdft_end+0x6>
> 
> 			c1: R_386_PC32	ff_fft_end
> 
> 
> And this is the same function if one uses -fvisibility=hidden instead of
> the attribute:
> 
> 000000bb <ff_rdft_end>:
> 
>    bb:	53                   	push   %ebx
> 
>    bc:	e8 fc ff ff ff       	call   bd <ff_rdft_end+0x2>
> 
> 			bd: R_386_PC32	__x86.get_pc_thunk.bx
> 
>    c1:	81 c3 02 00 00 00    	add    $0x2,%ebx
> 
> 			c3: R_386_GOTPC	_GLOBAL_OFFSET_TABLE_
> 
>    c7:	83 ec 14             	sub    $0x14,%esp
> 
>    ca:	8b 44 24 1c          	mov    0x1c(%esp),%eax
> 
>    ce:	83 c0 18             	add    $0x18,%eax
> 
>    d1:	50                   	push   %eax
> 
>    d2:	e8 fc ff ff ff       	call   d3 <ff_rdft_end+0x18>
> 
> 			d3: R_386_PLT32	ff_fft_end
> 
>    d7:	83 c4 18             	add    $0x18,%esp
> 
>    da:	5b                   	pop    %ebx
> 
>    db:	c3                   	ret
> 
> 
> The code is the same as if one had not used -fvisibility=hidden at all.
> 
> Of course, adding the attribute to every function/object is way too much
> effort; that's why the pragma exists.

Is this still true if you also add -fno-semantic-interposition?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] libavutil: Add av_visibility_hidden for setting hidden symbol visibility

[Andreas Rheinhardt]

Timo Rothenpieler:
> On 11.07.2022 16:26, Andreas Rheinhardt wrote:
>> Henrik Gramner:
>>> On Mon, Jul 11, 2022 at 11:19 AM Martin Storsjö <martin@martin.st>
>>> wrote:
>>>> +#if (AV_GCC_VERSION_AT_LEAST(4,0) || defined(__clang__)) &&
>>>> (defined(__ELF__) || defined(__MACH__))
>>>> +#    define av_visibility_hidden __attribute__((visibility("hidden")))
>>>> +#else
>>>> +#    define av_visibility_hidden
>>>> +#endif
>>>
>>> The usual approach is to compile with -fvisibility=hidden and
>>> explicitly flag exported API symbols.
>>>
>>> Is there a reason for doing this the other way around?
>>
>> -fvisibility=hidden only affects the visibility of symbols defined in
>> the currently compiled translation unit. It does not allow the compiler
>> to make assumptions about external declarations that are used in this
>> translation unit (in other words, it has to presume the worst: That it
>> comes from a different DSO). E.g. this is ff_rdft_end on 32bit x86 if
>> ff_fft_end is declared with an explicit hidden attribute:
>>
>> 000000bb <ff_rdft_end>:
>>
>>    bb:    83 44 24 04 18           addl   $0x18,0x4(%esp)
>>
>>    c0:    e9 fc ff ff ff           jmp    c1 <ff_rdft_end+0x6>
>>
>>             c1: R_386_PC32    ff_fft_end
>>
>>
>> And this is the same function if one uses -fvisibility=hidden instead of
>> the attribute:
>>
>> 000000bb <ff_rdft_end>:
>>
>>    bb:    53                       push   %ebx
>>
>>    bc:    e8 fc ff ff ff           call   bd <ff_rdft_end+0x2>
>>
>>             bd: R_386_PC32    __x86.get_pc_thunk.bx
>>
>>    c1:    81 c3 02 00 00 00        add    $0x2,%ebx
>>
>>             c3: R_386_GOTPC    _GLOBAL_OFFSET_TABLE_
>>
>>    c7:    83 ec 14                 sub    $0x14,%esp
>>
>>    ca:    8b 44 24 1c              mov    0x1c(%esp),%eax
>>
>>    ce:    83 c0 18                 add    $0x18,%eax
>>
>>    d1:    50                       push   %eax
>>
>>    d2:    e8 fc ff ff ff           call   d3 <ff_rdft_end+0x18>
>>
>>             d3: R_386_PLT32    ff_fft_end
>>
>>    d7:    83 c4 18                 add    $0x18,%esp
>>
>>    da:    5b                       pop    %ebx
>>
>>    db:    c3                       ret
>>
>>
>> The code is the same as if one had not used -fvisibility=hidden at all.
>>
>> Of course, adding the attribute to every function/object is way too much
>> effort; that's why the pragma exists.
> 
> Is this still true if you also add -fno-semantic-interposition?

Why should that change anything here? It just means that the compiler
may inline functions even though they could potentially be interposed.
But of course the compiler can't inline functions whose definition it
can't see.
Anyway, I tested it and there is no change.

- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] libavutil: Add av_visibility_hidden for setting hidden symbol visibility

[Martin Storsjö]

On Mon, 11 Jul 2022, Henrik Gramner wrote:

> On Mon, Jul 11, 2022 at 11:19 AM Martin Storsjö <martin@martin.st> wrote:
>> +#if (AV_GCC_VERSION_AT_LEAST(4,0) || defined(__clang__)) && (defined(__ELF__) || defined(__MACH__))
>> +#    define av_visibility_hidden __attribute__((visibility("hidden")))
>> +#else
>> +#    define av_visibility_hidden
>> +#endif
>
> The usual approach is to compile with -fvisibility=hidden and
> explicitly flag exported API symbols.
>
> Is there a reason for doing this the other way around?

Personally - primarily because that's way much more effort than I can put 
up right now, while this fixes the aarch64 text relocation issue (only) 
with fairly little effort.

// Martin
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] libavutil: Add av_visibility_hidden for setting hidden symbol visibility

[Martin Storsjö]

On Tue, 12 Jul 2022, Martin Storsjö wrote:

> On Mon, 11 Jul 2022, Henrik Gramner wrote:
>
>> On Mon, Jul 11, 2022 at 11:19 AM Martin Storsjö <martin@martin.st> wrote:
>>> +#if (AV_GCC_VERSION_AT_LEAST(4,0) || defined(__clang__)) && 
>>> (defined(__ELF__) || defined(__MACH__))
>>> +#    define av_visibility_hidden __attribute__((visibility("hidden")))
>>> +#else
>>> +#    define av_visibility_hidden
>>> +#endif
>> 
>> The usual approach is to compile with -fvisibility=hidden and
>> explicitly flag exported API symbols.
>> 
>> Is there a reason for doing this the other way around?
>
> Personally - primarily because that's way much more effort than I can put up 
> right now, while this fixes the aarch64 text relocation issue (only) with 
> fairly little effort.

... but I do kinda agree that that would be the ideal end form of the 
code. But Andreas point about that not getting the advantage for code 
referencing other hidden variables still stands though.

// Martin
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".