| Message ID | 20220711091850.4158449-1-martin@martin.st |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/2] libavutil: Add av_visibility_hidden for setting hidden symbol visibility | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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
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
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
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?
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".
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
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?
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
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
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
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?
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, \
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(-)