diff mbox

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

Message ID CAFP8O3Ly_b=bOOuwnLNMwNSH+Pjd_bVz1Wd+nWnEkPMFTS4egQ@mail.gmail.com
State New
Headers show

Commit Message

Fangrui Song March 14, 2019, 3:35 a.m. UTC 
On Thu, Mar 14, 2019 at 10:36 AM Fāng-ruì Sòng <maskray@google.com> wrote:
>
> 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.

I still don't have access to my neomutt so I send the patch as an
attached file via the webmail
diff mbox

Patch

From 64fc0ce5fa80d6d2b6bc93f539cca48a844bc672 Mon Sep 17 00:00:00 2001
From: Fangrui Song <maskray@google.com>
Date: Wed, 20 Feb 2019 16:25:46 +0800
Subject: [PATCH] avutil/mem: Mark DECLARE_ASM_ALIGNED as visibility("hidden")
 for __GNUC__

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"

These constants are currently defined in C files with default visibility.
Such object files cannot link to a shared object without
-Wl,-Bsymbolic or -Wl,--version-script,libavcodec/libavcodec.ver:

    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. -Bsymbolic is dangerous as it breaks C++ semantics
about address uniqueness of inline functions, type_info, ...
--version-script can have the same problem unless used very carefully.

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..98a14b826b 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 av_visibility_hidden __attribute__ ((visibility ("hidden")))
+#else
+    #define av_visibility_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))) av_visibility_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
-- 
2.20.1