Re: [FFmpeg-devel] [PATCH] avutil/mem: Mark DECLARE_ASM_ALIGNED as visibility("hidden") for __GNUC__
On Thu, Mar 14, 2019 at 10:36 AM Fāng-ruì Sòng wrote:
>
> On Thu, Mar 14, 2019 at 9:59 AM Henrik Gramner wrote:
> >
> > On Wed, Feb 20, 2019 at 8:03 PM Fāng-ruì Sòng
> > 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
--
宋方睿
From 64fc0ce5fa80d6d2b6bc93f539cca48a844bc672 Mon Sep 17 00:00:00 2001
From: Fangrui Song
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
---
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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/mem: Mark DECLARE_ASM_ALIGNED as visibility("hidden") for __GNUC__
On Thu, Mar 14, 2019 at 9:59 AM Henrik Gramner wrote:
>
> On Wed, Feb 20, 2019 at 8:03 PM Fāng-ruì Sòng
> 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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/mem: Mark DECLARE_ASM_ALIGNED as visibility("hidden") for __GNUC__
On Wed, Feb 20, 2019 at 8:03 PM Fāng-ruì Sòng
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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/mem: Mark DECLARE_ASM_ALIGNED as visibility("hidden") for __GNUC__
Ping :) In short, when people build .so from ffmpeg .o/.a files, they no longer need -Bsymbolic -Bsymbolic is bad because it breaks C++ semantics, e.g. address uniqueness of inline functions and their static local variables, uniqueness of template specializations, address uniqueness of type_info objects (used for C++ exceptions), etc On Thu, Mar 7, 2019 at 11:53 AM Fāng-ruì Sòng wrote: > Sorry, wrong signed-off-by line.. > > (isn't using my usual mail client mutt and i get very sick of the > webmail...) > > On Thu, Mar 7, 2019 at 11:36 AM Fāng-ruì Sòng wrote: > >> On Wed, Mar 6, 2019 at 4:14 PM Carl Eugen Hoyos >> wrote: >> >>> 2019-02-21 2:37 GMT+01:00, Fāng-ruì Sòng < >>> maskray-at-google@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 >>> >>> >> Sorry. Here is the attached patch (git format-patch -1 --stdout). >> > > > -- > 宋方睿 > -- 宋方睿 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/mem: Mark DECLARE_ASM_ALIGNED as visibility("hidden") for __GNUC__
Sorry, wrong signed-off-by line..
(isn't using my usual mail client mutt and i get very sick of the
webmail...)
On Thu, Mar 7, 2019 at 11:36 AM Fāng-ruì Sòng wrote:
> On Wed, Mar 6, 2019 at 4:14 PM Carl Eugen Hoyos
> wrote:
>
>> 2019-02-21 2:37 GMT+01:00, Fāng-ruì Sòng <
>> maskray-at-google@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
>>
>>
> Sorry. Here is the attached patch (git format-patch -1 --stdout).
>
--
宋方睿
From 63d1c46c9796ca4a804cc89573c8f4613716b50d Mon Sep 17 00:00:00 2001
From: Fangrui Song
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"
On ELF platforms, when -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. 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
---
libavutil/mem.h | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/libavutil/mem.h b/libavutil/mem.h
index 5fb1a02..9afeed0 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
--
2.20.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/mem: Mark DECLARE_ASM_ALIGNED as visibility("hidden") for __GNUC__
On Wed, Mar 6, 2019 at 4:14 PM Carl Eugen Hoyos wrote:
> 2019-02-21 2:37 GMT+01:00, Fāng-ruì Sòng >:
> > 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
>
>
Sorry. Here is the attached patch (git format-patch -1 --stdout).
From 63d1c46c9796ca4a804cc89573c8f4613716b50d Mon Sep 17 00:00:00 2001
From: Fangrui Song
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"
On ELF platforms, when -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. 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
---
libavutil/mem.h | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/libavutil/mem.h b/libavutil/mem.h
index 5fb1a02..9afeed0 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
--
2.20.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/mem: Mark DECLARE_ASM_ALIGNED as visibility("hidden") for __GNUC__
2019-02-21 2:37 GMT+01:00, Fāng-ruì Sòng : > 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/mem: Mark DECLARE_ASM_ALIGNED as visibility("hidden") for __GNUC__
On Thu, Feb 21, 2019 at 10:06 AM Fāng-ruì Sòng 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 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
> > ---
> > 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
> >
Re: [FFmpeg-devel] [PATCH] avutil/mem: Mark DECLARE_ASM_ALIGNED as visibility("hidden") for __GNUC__
> 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 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
> ---
> 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
--
宋方睿
___
ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/mem: Mark DECLARE_ASM_ALIGNED as visibility("hidden") for __GNUC__
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
---
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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avutil/mem: Mark DECLARE_ASM_ALIGNED as visibility("hidden") for __GNUC__
2019-02-20 10:13 GMT+01:00, Fāng-ruì Sòng : > 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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [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"
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
---
libavutil/mem.h | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
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
+#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
--
2.20.1
--
宋方睿
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
