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

Submitted by Fāng-ruì Sòng on Feb. 20, 2019, 9:13 a.m.

Details

Message ID CAFP8O3LXFVbGwk+=P-MhG9cs5bJHZhhzhfNpJi52uErFQ5d8aQ@mail.gmail.com
State New
Headers show

Commit Message

Fāng-ruì Sòng Feb. 20, 2019, 9:13 a.m.
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.

Also remove __clang__ as clang pretends to be gcc 4.2 and defines __GNUC__

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

+    #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

Comments

Carl Eugen Hoyos Feb. 21, 2019, 12:18 a.m.
2019-02-20 10:13 GMT+01:00, Fāng-ruì Sòng <maskray-at-google.com@ffmpeg.org>:
> 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

Why is it a good idea to remove them from the linker command line?

> 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.
>
> Also remove __clang__ as clang pretends to be gcc 4.2 and defines __GNUC__

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

Carl Eugen
Henrik Gramner March 14, 2019, 1:58 a.m.
On Wed, Feb 20, 2019 at 8:03 PM Fāng-ruì Sòng
<maskray-at-google.com@ffmpeg.org> wrote:
> --- a/libavutil/mem.h
> +++ b/libavutil/mem.h
>
> +#if defined(__GNUC__) && !(defined(_WIN32) || defined(__CYGWIN__))
> +    #define DECLARE_HIDDEN __attribute__ ((visibility ("hidden")))
> +#else
> +    #define DECLARE_HIDDEN
> +#endif

libavutil/mem.h is a public header so any defines added should have
appropriate prefixes (yes, the existing defines violate this which is
something that should be addressed, but that's a different issue).

Alternatively maybe those macros should be moved to some internal
header because I don't really see any value of having inline asm
support macros public.
Fāng-ruì Sòng March 14, 2019, 2:36 a.m.
On Thu, Mar 14, 2019 at 9:59 AM Henrik Gramner <henrik@gramner.com> wrote:
>
> On Wed, Feb 20, 2019 at 8:03 PM Fāng-ruì Sòng
> <maskray-at-google.com@ffmpeg.org> wrote:
> > --- a/libavutil/mem.h
> > +++ b/libavutil/mem.h
> >
> > +#if defined(__GNUC__) && !(defined(_WIN32) || defined(__CYGWIN__))
> > +    #define DECLARE_HIDDEN __attribute__ ((visibility ("hidden")))
> > +#else
> > +    #define DECLARE_HIDDEN
> > +#endif
>
> libavutil/mem.h is a public header so any defines added should have
> appropriate prefixes (yes, the existing defines violate this which is
> something that should be addressed, but that's a different issue).

Do you have suggestions on the naming? av_declare_hidden?

> Alternatively maybe those macros should be moved to some internal
> header because I don't really see any value of having inline asm
> support macros public.

That would change too many files. I'd rather not do that in this patch.

Patch hide | download patch | download mbox

diff --git a/libavutil/mem.h b/libavutil/mem.h
index 5fb1a02dd9..47abe2c8e9 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
@@ -108,9 +114,9 @@ 
     #define DECLARE_ALIGNED(n,t,v)      t __attribute__ ((aligned
(FFMIN(n, 16)))) v
     #define DECLARE_ASM_ALIGNED(n,t,v)  t av_used __attribute__
((aligned (FFMIN(n, 16)))) v
     #define DECLARE_ASM_CONST(n,t,v)    static const t av_used
__attribute__ ((aligned (FFMIN(n, 16)))) v
-#elif defined(__GNUC__) || defined(__clang__)
+#elif defined(__GNUC__)
     #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