[FFmpeg-devel] [PATCH v4] Mark C globals with small code model
Pranav Kant
prka at google.com
Fri Mar 14 02:16:17 EET 2025
Thank you for taking a look.
On Tue, Mar 11, 2025 at 4:45 PM Andreas Rheinhardt <
andreas.rheinhardt at outlook.com> wrote:
> Pranav Kant via ffmpeg-devel:
> > By default, all globals in C/C++ compiled by clang are allocated
> > in non-large data sections. See [1] for background on code models.
> > For PIC (Position independent code), this is fine as long as binary is
> > small but as binary size increases, users maybe want to use medium/large
> > code models (-mcmodel=medium) which moves data in to large sections.
> > As data in these large sections cannot be accessed using PIC code
> > anymore (as it may be too far away), compiler ends up using a different
> > instruction sequence when building C/C++ code -- using GOT to access
> > these globals (which can be relaxed by linker at link time if binary
> > ends up being smaller). However, assembly files continue to access these
> > globals defined in C/C++ files using older (and invalid instruction
> > sequence). So, we mark all such globals with an attribute that forces
> > them to be allocated in small sections allowing them to validly be
> > accessed from the assembly code.
> >
> > This patch should not have any affect on builds that use small code
> > model, which is the default mode.
> >
> > [1]
> https://eli.thegreenplace.net/2012/01/03/understanding-the-x64-code-models
> >
> > Signed-off-by: Pranav Kant <prka at google.com>
> > ---
> > libavcodec/ac3dsp.c | 2 ++
> > libavcodec/cabac.c | 2 ++
> > libavcodec/x86/constants.c | 8 ++++++++
> > libavutil/attributes.h | 6 ++++++
> > libavutil/attributes_internal.h | 16 ++++++++++++++++
> > 5 files changed, 34 insertions(+)
> >
> > diff --git a/libavcodec/ac3dsp.c b/libavcodec/ac3dsp.c
> > index 730fa70fff..d16b6c24c3 100644
> > --- a/libavcodec/ac3dsp.c
> > +++ b/libavcodec/ac3dsp.c
> > @@ -25,6 +25,7 @@
> >
> > #include "config.h"
> > #include "libavutil/attributes.h"
> > +#include "libavutil/attributes_internal.h"
> > #include "libavutil/common.h"
> > #include "libavutil/intmath.h"
> > #include "libavutil/mem_internal.h"
> > @@ -104,6 +105,7 @@ static void ac3_update_bap_counts_c(uint16_t
> mant_cnt[16], uint8_t *bap,
> > mant_cnt[bap[len]]++;
> > }
> >
> > +attribute_mcmodel_small
> > DECLARE_ALIGNED(16, const uint16_t, ff_ac3_bap_bits)[16] = {
>
> Shouldn't stuff like this be applied to the declaration so that C code
> can also take advantage of the knowledge that this object will be placed
> in the small code section?
>
That's right. I will have it corrected in the newer version.
>
> > 0, 0, 0, 3, 0, 4, 5, 6, 7, 8, 9, 10, 11, 12, 14, 16
> > };
> > diff --git a/libavcodec/cabac.c b/libavcodec/cabac.c
> > index 7d41cd2ae6..b8c6db29a2 100644
> > --- a/libavcodec/cabac.c
> > +++ b/libavcodec/cabac.c
> > @@ -24,11 +24,13 @@
> > * Context Adaptive Binary Arithmetic Coder.
> > */
> >
> > +#include "libavutil/attributes_internal.h"
> > #include "libavutil/error.h"
> > #include "libavutil/mem_internal.h"
> >
> > #include "cabac.h"
> >
> > +attribute_mcmodel_small
> > DECLARE_ASM_ALIGNED(1, const uint8_t, ff_h264_cabac_tables)[512 +
> 4*2*64 + 4*64 + 63] = {
>
> Your commit message ("However, assembly files continue to access")
> speaks only of assembly files, i.e. external asm. Yet this here is only
> used by inline ASM. Looking through the code the reason for this is that
> I thought that specifying the memory model is only necessary for stuff
> used by external asm, yet ff_h264_cabac_tables does not seem to be used
> by external ASM at all, only inline ASM. If I see this correctly, the
> reason for this is that LOCAL_MANGLE (and therefore MANGLE) uses rip
> addressing on x64 when configure sets the RIP define. But this means
> that the set of files needing attribute_mcmodel_small is a superset of
> the files currently using DECLARE_ASM_ALIGNED. This means that one would
> only need two macros for the variables accessed by ASM: One for only
> external ASM and one for inline ASM (and potentially external ASM)
> instead of adding attribute_mcmodel_small at various places in the
> codebase.
>
By "However, assembly files continue to access", I meant all assembly
references,
and yes, as you noted, LOCAL_MANGLE uses rip relative addressing. I will
have
the description fixed in the next version.
Making this attribute part of these macros makes sense. However, I have
noted
that not all such globals are marked with such macros. Eg: See ff_pw_1 in
libavcodec/x86/constants.c. This is accessed by "external" asm but not
marked
with any macro that would indicate such. This is even more true for globals
defined
in header files (eg: constants.c referenced above). I am guessing this is
not intentional.
I can go ahead with using DECLARE_ASM_VAR and DECLARE_INLINE_ASM_VAR
as you suggested for all such variables.
>
> > 9,8,7,7,6,6,6,6,5,5,5,5,5,5,5,5,
> > 4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,
> > diff --git a/libavcodec/x86/constants.c b/libavcodec/x86/constants.c
> > index bc7f2b17b8..347b7dd1d3 100644
> > --- a/libavcodec/x86/constants.c
> > +++ b/libavcodec/x86/constants.c
> > @@ -18,17 +18,21 @@
> > * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA
> > */
> >
> > +#include "libavutil/attributes_internal.h"
> > #include "libavutil/mem_internal.h"
> > #include "libavutil/x86/asm.h" // for xmm_reg
> > #include "constants.h"
> >
> > +attribute_mcmodel_small
> > DECLARE_ALIGNED(32, const ymm_reg, ff_pw_1) = {
> 0x0001000100010001ULL, 0x0001000100010001ULL,>
> 0x0001000100010001ULL, 0x0001000100010001ULL };
> > DECLARE_ALIGNED(32, const ymm_reg, ff_pw_2) = {
> 0x0002000200020002ULL, 0x0002000200020002ULL,
> >
> 0x0002000200020002ULL, 0x0002000200020002ULL };
> > DECLARE_ASM_ALIGNED(16, const xmm_reg, ff_pw_3) = {
> 0x0003000300030003ULL, 0x0003000300030003ULL };
> > +attribute_mcmodel_small
> > DECLARE_ASM_ALIGNED(32, const ymm_reg, ff_pw_4) = {
> 0x0004000400040004ULL, 0x0004000400040004ULL,
> >
> 0x0004000400040004ULL, 0x0004000400040004ULL };
> > +attribute_mcmodel_small
> > DECLARE_ASM_ALIGNED(16, const xmm_reg, ff_pw_5) = {
> 0x0005000500050005ULL, 0x0005000500050005ULL };
> > DECLARE_ALIGNED(16, const xmm_reg, ff_pw_8) = {
> 0x0008000800080008ULL, 0x0008000800080008ULL };
> > DECLARE_ASM_ALIGNED(16, const xmm_reg, ff_pw_9) = {
> 0x0009000900090009ULL, 0x0009000900090009ULL };
> > @@ -49,6 +53,7 @@ DECLARE_ALIGNED(32, const ymm_reg, ff_pw_256) = {
> 0x0100010001000100ULL, 0x010
> > DECLARE_ALIGNED(32, const ymm_reg, ff_pw_512) = {
> 0x0200020002000200ULL, 0x0200020002000200ULL,
> >
> 0x0200020002000200ULL, 0x0200020002000200ULL };
> > DECLARE_ALIGNED(16, const xmm_reg, ff_pw_1019) = {
> 0x03FB03FB03FB03FBULL, 0x03FB03FB03FB03FBULL };
> > +attribute_mcmodel_small
> > DECLARE_ALIGNED(32, const ymm_reg, ff_pw_1023) = {
> 0x03ff03ff03ff03ffULL, 0x03ff03ff03ff03ffULL,
> >
> 0x03ff03ff03ff03ffULL, 0x03ff03ff03ff03ffULL};
> > DECLARE_ALIGNED(32, const ymm_reg, ff_pw_1024) = {
> 0x0400040004000400ULL, 0x0400040004000400ULL,
> > @@ -66,13 +71,16 @@ DECLARE_ALIGNED(32, const ymm_reg, ff_pw_m1) = {
> 0xFFFFFFFFFFFFFFFFULL, 0xFFF
> >
> > DECLARE_ALIGNED(32, const ymm_reg, ff_pb_0) = {
> 0x0000000000000000ULL, 0x0000000000000000ULL,
> >
> 0x0000000000000000ULL, 0x0000000000000000ULL };
> > +attribute_mcmodel_small
> > DECLARE_ALIGNED(32, const ymm_reg, ff_pb_1) = {
> 0x0101010101010101ULL, 0x0101010101010101ULL,
> >
> 0x0101010101010101ULL, 0x0101010101010101ULL };
> > DECLARE_ALIGNED(32, const ymm_reg, ff_pb_2) = {
> 0x0202020202020202ULL, 0x0202020202020202ULL,
> >
> 0x0202020202020202ULL, 0x0202020202020202ULL };
> > +attribute_mcmodel_small
> > DECLARE_ALIGNED(32, const ymm_reg, ff_pb_3) = {
> 0x0303030303030303ULL, 0x0303030303030303ULL,
> >
> 0x0303030303030303ULL, 0x0303030303030303ULL };
> > DECLARE_ALIGNED(32, const xmm_reg, ff_pb_15) = {
> 0x0F0F0F0F0F0F0F0FULL, 0x0F0F0F0F0F0F0F0FULL };
> > +attribute_mcmodel_small
> > DECLARE_ALIGNED(32, const ymm_reg, ff_pb_80) = {
> 0x8080808080808080ULL, 0x8080808080808080ULL,
> >
> 0x8080808080808080ULL, 0x8080808080808080ULL };
> > DECLARE_ALIGNED(32, const ymm_reg, ff_pb_FE) = {
> 0xFEFEFEFEFEFEFEFEULL, 0xFEFEFEFEFEFEFEFEULL,
>
> Why do you add this attribute to only eight of these constants? How did
> you come up with this list? In addition to the above, git grep cextern
> shows that there are several more variables than just these one (e.g.
> ff_h263_loop_filter_strength, ff_sbr_noise_table).
>
I agree that this is not the full list. I will do the mechanical part of
adding it to everything
that is accessed by ASM once we settle down on the other stuff.
>
> > diff --git a/libavutil/attributes.h b/libavutil/attributes.h
> > index 04c615c952..dfc35fa31e 100644
> > --- a/libavutil/attributes.h
> > +++ b/libavutil/attributes.h
> > @@ -40,6 +40,12 @@
> > # define AV_HAS_BUILTIN(x) 0
> > #endif
> >
> > +#ifdef __has_attribute
> > +# define AV_HAS_ATTRIBUTE(x) __has_attribute(x)
> > +#else
> > +# define AV_HAS_ATTRIBUTE(x) 0
> > +#endif
> > +
> > #ifndef av_always_inline
> > #if AV_GCC_VERSION_AT_LEAST(3,1)
> > # define av_always_inline __attribute__((always_inline)) inline
> > diff --git a/libavutil/attributes_internal.h
> b/libavutil/attributes_internal.h
> > index bc85ce77ff..c557fa0af0 100644
> > --- a/libavutil/attributes_internal.h
> > +++ b/libavutil/attributes_internal.h
> > @@ -19,6 +19,7 @@
> > #ifndef AVUTIL_ATTRIBUTES_INTERNAL_H
> > #define AVUTIL_ATTRIBUTES_INTERNAL_H
> >
> > +#include "config.h"
> > #include "attributes.h"
> >
> > #if (AV_GCC_VERSION_AT_LEAST(4,0) || defined(__clang__)) &&
> (defined(__ELF__) || defined(__MACH__))
> > @@ -33,4 +34,19 @@
> >
> > #define EXTERN extern attribute_visibility_hidden
> >
> > +/**
> > + * Some globals defined in C files are used from hardcoded asm that
> assumes small
> > + * code model (that is, accessing these globals without GOT). This is a
> problem
> > + * when FFMpeg is built with medium code model (-mcmodel=medium) which
> allocates
>
> s/FFMpeg/FFmpeg/
>
Ack.
>
> > + * all globals in a data section that's unreachable with PC relative
> instructions
> > + * (small code model instruction sequence). We mark all such globals
> with this
> > + * attribute_mcmodel_small to ensure assembly accessible globals
> continue to be
> > + * allocated in sections reachable from PC relative instructions.
> > + */
> > +#if ARCH_X86_64 && defined(__ELF__) && AV_HAS_ATTRIBUTE(model)
>
> You make this ARCH_X86_64 only, yet the concept of code models also
> exists for other arches, e.g. AArch64. I presume that assembly for other
> arches presumes that we are not using the large code model for accesses,
> so that the same issue can happen with them. Then we have two options:
>
> a) Continue to use the same macro you are adding here. This will mean
> that stuff only used in assembly by different arches will nevertheless
> be declared as using the small code model, potentially clogging the
> small symbol address space unnecessarily.
> b) Use one macro per arch.
>
> I presume we want to go with a), because the objects we are talking
> about are so small that they should be in the small data section anyway.
>
Support for the medium code model for AArch64 (in clang and GCC) is not
available
as far as I can see. x86 is more of a problem from a binary size
perspective than
AArch64 as the range to access data is considerably smaller in x86 (with a
small
code model).
>
> > +# define attribute_mcmodel_small __attribute__((model("small")))
> > +#else
> > +# define attribute_mcmodel_small
> > +#endif
> > +
> > #endif /* AVUTIL_ATTRIBUTES_INTERNAL_H */
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
More information about the ffmpeg-devel
mailing list