diff mbox

[FFmpeg-devel] avutil/mem: Mark DECLARE_ASM_ALIGNED as visibility("hidden") for __GNUC__

Message ID CAFP8O3+esH6LNkVvvTxR9e8tpNZdwMiYVvMmGQMQRL7iwet+YQ@mail.gmail.com
State Superseded
Headers show

Commit Message

Fangrui Song Feb. 21, 2019, 1:37 a.m. UTC 
Sorry if this doesn't attach to the correct thread as I didn't
subscribe to this list and don't know the Message-ID of the thread.

> The word "also" indicates here that this should be an independent patch.

I added `#if defined(__GNUC__) && !(defined(_WIN32) ||
defined(__CYGWIN__))`, not `#if (defined(__GNUC__) ||
defined(__clang__)) && !(defined(_WIN32) || defined(__CYGWIN__))`. For
consistency I removed the defined(__clang__) below. If that change
should be an independent one, here is the amended version without the
removal of defined(__clang__)


Inline asm code assumes these DECLARE_ASM_ALIGNED declared global
constants are non-preemptive, e.g.

libavcodec/x86/cabac.h
        "lea    "MANGLE(ff_h264_cabac_tables)", %0      \n\t"

On ELF platforms, if -Wl,-Bsymbolic
-Wl,--version-script,libavcodec/libavcodec.ver are removed from the
linker command line, the symbol will be considered preemptive and fail
to link to a DSO:

    ld.lld: error: relocation R_X86_64_PC32 cannot be used against
symbol ff_h264_cabac_tables; recompile with -fPIC

It is better to express the intention explicitly and mark such global
constants hidden (non-preemptive). It also improves portability as no
linker magic is required.

DECLARE_ASM_CONST uses the "static" specifier to indicate internal
linkage. The visibility annotation is unnecessary.

Signed-off-by: Fangrui Song <maskray@google.com>
---
 libavutil/mem.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

     #define DECLARE_ASM_CONST(n,t,v)    static const t av_used
__attribute__ ((aligned (n))) v
 #elif defined(_MSC_VER)
     #define DECLARE_ALIGNED(n,t,v)      __declspec(align(n)) t v

Comments

Fangrui Song Feb. 21, 2019, 2:06 a.m. UTC | #1 
> Why is it a good idea to remove them from the linker command line?

In short, it improves portability. I'm not suggesting removing
-Bsymbolic or --version-script from the ffmpeg build system. I mean
users will no longer have to specify the two options to link ffmpeg
object files into their own shared objects (AFAIK this patch address
all C side issues. There are some other problem in *.asm code, though;
I also saw BROKEN_RELOCATIONS in the code, thinking that it was
probably added when developers noticed it could bring problems. I
didn't dig up the history to learn more)

When using a different build system when the relevant SHFLAGS options
(-Bsymbolic and --version-script) aren't specified, there wouldn't be
mysterious linker errors "relocation R_X86_64_PC32 cannot be used
against ...". If the linker options aren't mandatory, ffmpeg can be
more easily integrated to other projects or build systems.

Or when linking libavcodec/*.o with other object files from the main
program to form another shared object (not
libavcodec/libavcodec.so.58). The current limitation (these global
constants having default visibility) means every shared object linking
in libavcodec/cabac.o (with the default visibility
ff_h264_cabac_tables) and libavcodec/x86/constants.o have to use
either -Bsymbolic, or to construct their own version script.

* -Bsymbolic this option applies to all exported symbols on the linker
command line, not just ffmpeg object files. This makes symbols
non-preemptive, and in particular, breaks C++ [dcl.inline], which says
"A static local variable in an inline function with external linkage
always refers to the same object." If this option is used, two
function-local static object may have different addresses.
* --version-script  libavcodec/libavcodec.ver specifies `global: av_*;
local: *;` Again, the problem is that it applies to all exported
symbols (may affect program code). If the program code doesn't want
all its symbols to be marked local, it'll have to define its own
version script. This is a hindrance that can be avoided.


On Thu, Feb 21, 2019 at 9:37 AM Fāng-ruì Sòng <maskray@google.com> wrote:
>
> Sorry if this doesn't attach to the correct thread as I didn't
> subscribe to this list and don't know the Message-ID of the thread.
>
> > The word "also" indicates here that this should be an independent patch.
>
> I added `#if defined(__GNUC__) && !(defined(_WIN32) ||
> defined(__CYGWIN__))`, not `#if (defined(__GNUC__) ||
> defined(__clang__)) && !(defined(_WIN32) || defined(__CYGWIN__))`. For
> consistency I removed the defined(__clang__) below. If that change
> should be an independent one, here is the amended version without the
> removal of defined(__clang__)
>
>
> Inline asm code assumes these DECLARE_ASM_ALIGNED declared global
> constants are non-preemptive, e.g.
>
> libavcodec/x86/cabac.h
>         "lea    "MANGLE(ff_h264_cabac_tables)", %0      \n\t"
>
> On ELF platforms, if -Wl,-Bsymbolic
> -Wl,--version-script,libavcodec/libavcodec.ver are removed from the
> linker command line, the symbol will be considered preemptive and fail
> to link to a DSO:
>
>     ld.lld: error: relocation R_X86_64_PC32 cannot be used against
> symbol ff_h264_cabac_tables; recompile with -fPIC
>
> It is better to express the intention explicitly and mark such global
> constants hidden (non-preemptive). It also improves portability as no
> linker magic is required.
>
> DECLARE_ASM_CONST uses the "static" specifier to indicate internal
> linkage. The visibility annotation is unnecessary.
>
> Signed-off-by: Fangrui Song <maskray@google.com>
> ---
>  libavutil/mem.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/libavutil/mem.h b/libavutil/mem.h
> index 5fb1a02dd9..9afeed0b43 100644
> --- a/libavutil/mem.h
> +++ b/libavutil/mem.h
> @@ -100,6 +100,12 @@
>   * @param v Name of the variable
>   */
>
> +#if defined(__GNUC__) && !(defined(_WIN32) || defined(__CYGWIN__))
> +    #define DECLARE_HIDDEN __attribute__ ((visibility ("hidden")))
> +#else
> +    #define DECLARE_HIDDEN
> +#endif
> +
>  #if defined(__INTEL_COMPILER) && __INTEL_COMPILER < 1110 || defined(__SUNPRO_C)
>      #define DECLARE_ALIGNED(n,t,v)      t __attribute__ ((aligned (n))) v
>      #define DECLARE_ASM_ALIGNED(n,t,v)  t __attribute__ ((aligned (n))) v
> @@ -110,7 +116,7 @@
>      #define DECLARE_ASM_CONST(n,t,v)    static const t av_used
> __attribute__ ((aligned (FFMIN(n, 16)))) v
>  #elif defined(__GNUC__) || defined(__clang__)
>      #define DECLARE_ALIGNED(n,t,v)      t __attribute__ ((aligned (n))) v
> -    #define DECLARE_ASM_ALIGNED(n,t,v)  t av_used __attribute__
> ((aligned (n))) v
> +    #define DECLARE_ASM_ALIGNED(n,t,v)  t av_used __attribute__
> ((aligned (n))) DECLARE_HIDDEN v
>      #define DECLARE_ASM_CONST(n,t,v)    static const t av_used
> __attribute__ ((aligned (n))) v
>  #elif defined(_MSC_VER)
>      #define DECLARE_ALIGNED(n,t,v)      __declspec(align(n)) t v
Fangrui Song March 5, 2019, 4:55 a.m. UTC | #2 
On Thu, Feb 21, 2019 at 10:06 AM Fāng-ruì Sòng <maskray@google.com> wrote:
>
> > Why is it a good idea to remove them from the linker command line?
>
> In short, it improves portability. I'm not suggesting removing
> -Bsymbolic or --version-script from the ffmpeg build system. I mean
> users will no longer have to specify the two options to link ffmpeg
> object files into their own shared objects (AFAIK this patch address
> all C side issues. There are some other problem in *.asm code, though;
> I also saw BROKEN_RELOCATIONS in the code, thinking that it was
> probably added when developers noticed it could bring problems. I
> didn't dig up the history to learn more)
>
> When using a different build system when the relevant SHFLAGS options
> (-Bsymbolic and --version-script) aren't specified, there wouldn't be
> mysterious linker errors "relocation R_X86_64_PC32 cannot be used
> against ...". If the linker options aren't mandatory, ffmpeg can be
> more easily integrated to other projects or build systems.
>
> Or when linking libavcodec/*.o with other object files from the main
> program to form another shared object (not
> libavcodec/libavcodec.so.58). The current limitation (these global
> constants having default visibility) means every shared object linking
> in libavcodec/cabac.o (with the default visibility
> ff_h264_cabac_tables) and libavcodec/x86/constants.o have to use
> either -Bsymbolic, or to construct their own version script.
>
> * -Bsymbolic this option applies to all exported symbols on the linker
> command line, not just ffmpeg object files. This makes symbols
> non-preemptive, and in particular, breaks C++ [dcl.inline], which says
> "A static local variable in an inline function with external linkage
> always refers to the same object." If this option is used, two
> function-local static object may have different addresses.
> * --version-script  libavcodec/libavcodec.ver specifies `global: av_*;
> local: *;` Again, the problem is that it applies to all exported
> symbols (may affect program code). If the program code doesn't want
> all its symbols to be marked local, it'll have to define its own
> version script. This is a hindrance that can be avoided.
>
>
> On Thu, Feb 21, 2019 at 9:37 AM Fāng-ruì Sòng <maskray@google.com> wrote:
> >
> > Sorry if this doesn't attach to the correct thread as I didn't
> > subscribe to this list and don't know the Message-ID of the thread.
> >
> > > The word "also" indicates here that this should be an independent patch.
> >
> > I added `#if defined(__GNUC__) && !(defined(_WIN32) ||
> > defined(__CYGWIN__))`, not `#if (defined(__GNUC__) ||
> > defined(__clang__)) && !(defined(_WIN32) || defined(__CYGWIN__))`. For
> > consistency I removed the defined(__clang__) below. If that change
> > should be an independent one, here is the amended version without the
> > removal of defined(__clang__)
> >
> >
> > Inline asm code assumes these DECLARE_ASM_ALIGNED declared global
> > constants are non-preemptive, e.g.
> >
> > libavcodec/x86/cabac.h
> >         "lea    "MANGLE(ff_h264_cabac_tables)", %0      \n\t"
> >
> > On ELF platforms, if -Wl,-Bsymbolic
> > -Wl,--version-script,libavcodec/libavcodec.ver are removed from the
> > linker command line, the symbol will be considered preemptive and fail
> > to link to a DSO:
> >
> >     ld.lld: error: relocation R_X86_64_PC32 cannot be used against
> > symbol ff_h264_cabac_tables; recompile with -fPIC
> >
> > It is better to express the intention explicitly and mark such global
> > constants hidden (non-preemptive). It also improves portability as no
> > linker magic is required.
> >
> > DECLARE_ASM_CONST uses the "static" specifier to indicate internal
> > linkage. The visibility annotation is unnecessary.
> >
> > Signed-off-by: Fangrui Song <maskray@google.com>
> > ---
> >  libavutil/mem.h | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavutil/mem.h b/libavutil/mem.h
> > index 5fb1a02dd9..9afeed0b43 100644
> > --- a/libavutil/mem.h
> > +++ b/libavutil/mem.h
> > @@ -100,6 +100,12 @@
> >   * @param v Name of the variable
> >   */
> >
> > +#if defined(__GNUC__) && !(defined(_WIN32) || defined(__CYGWIN__))
> > +    #define DECLARE_HIDDEN __attribute__ ((visibility ("hidden")))
> > +#else
> > +    #define DECLARE_HIDDEN
> > +#endif
> > +
> >  #if defined(__INTEL_COMPILER) && __INTEL_COMPILER < 1110 || defined(__SUNPRO_C)
> >      #define DECLARE_ALIGNED(n,t,v)      t __attribute__ ((aligned (n))) v
> >      #define DECLARE_ASM_ALIGNED(n,t,v)  t __attribute__ ((aligned (n))) v
> > @@ -110,7 +116,7 @@
> >      #define DECLARE_ASM_CONST(n,t,v)    static const t av_used
> > __attribute__ ((aligned (FFMIN(n, 16)))) v
> >  #elif defined(__GNUC__) || defined(__clang__)
> >      #define DECLARE_ALIGNED(n,t,v)      t __attribute__ ((aligned (n))) v
> > -    #define DECLARE_ASM_ALIGNED(n,t,v)  t av_used __attribute__
> > ((aligned (n))) v
> > +    #define DECLARE_ASM_ALIGNED(n,t,v)  t av_used __attribute__
> > ((aligned (n))) DECLARE_HIDDEN v
> >      #define DECLARE_ASM_CONST(n,t,v)    static const t av_used
> > __attribute__ ((aligned (n))) v
> >  #elif defined(_MSC_VER)
> >      #define DECLARE_ALIGNED(n,t,v)      __declspec(align(n)) t v
>

Friendly ping :)
Carl Eugen Hoyos March 6, 2019, 8:14 a.m. UTC | #3 
2019-02-21 2:37 GMT+01:00, Fāng-ruì Sòng <maskray-at-google.com@ffmpeg.org>:
> Sorry if this doesn't attach to the correct thread as I didn't
> subscribe to this list and don't know the Message-ID of the thread.

The thread works fine here with gmail but your patch was corrupted,
you have to attach it.

Carl Eugen
diff mbox

Patch

diff --git a/libavutil/mem.h b/libavutil/mem.h
index 5fb1a02dd9..9afeed0b43 100644
--- a/libavutil/mem.h
+++ b/libavutil/mem.h
@@ -100,6 +100,12 @@ 
  * @param v Name of the variable
  */

+#if defined(__GNUC__) && !(defined(_WIN32) || defined(__CYGWIN__))
+    #define DECLARE_HIDDEN __attribute__ ((visibility ("hidden")))
+#else
+    #define DECLARE_HIDDEN
+#endif
+
 #if defined(__INTEL_COMPILER) && __INTEL_COMPILER < 1110 || defined(__SUNPRO_C)
     #define DECLARE_ALIGNED(n,t,v)      t __attribute__ ((aligned (n))) v
     #define DECLARE_ASM_ALIGNED(n,t,v)  t __attribute__ ((aligned (n))) v
@@ -110,7 +116,7 @@ 
     #define DECLARE_ASM_CONST(n,t,v)    static const t av_used
__attribute__ ((aligned (FFMIN(n, 16)))) v
 #elif defined(__GNUC__) || defined(__clang__)
     #define DECLARE_ALIGNED(n,t,v)      t __attribute__ ((aligned (n))) v
-    #define DECLARE_ASM_ALIGNED(n,t,v)  t av_used __attribute__
((aligned (n))) v
+    #define DECLARE_ASM_ALIGNED(n,t,v)  t av_used __attribute__
((aligned (n))) DECLARE_HIDDEN v