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

[Fāng-ruì Sòng]

> 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 at 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 at 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



-- 
宋方睿