From: Pranav Kant via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> Cc: Pranav Kant <prka@google.com> Subject: Re: [FFmpeg-devel] [PATCH v4] Mark C globals with small code model Date: Thu, 13 Mar 2025 17:16:17 -0700 Message-ID: <CAPvNhsL-V9bKMDk1pw3DuZb-X_nZw0=-mnjBbNQYWQhADk65bQ@mail.gmail.com> (raw) In-Reply-To: <GV1P250MB07370CAFC2098EA75B92DFCF8FD12@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM> Thank you for taking a look. On Tue, Mar 11, 2025 at 4:45 PM Andreas Rheinhardt < andreas.rheinhardt@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@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
next prev parent reply other threads:[~2025-03-14 0:16 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2025-02-25 21:37 [FFmpeg-devel] [PATCH] " Pranav Kant via ffmpeg-devel 2025-02-25 23:03 ` Andreas Rheinhardt 2025-02-26 19:44 ` Pranav Kant via ffmpeg-devel 2025-02-26 19:45 ` Pranav Kant via ffmpeg-devel 2025-02-27 3:36 ` James Zern via ffmpeg-devel 2025-02-28 1:14 ` Michael Niedermayer 2025-03-07 2:13 ` Pranav Kant via ffmpeg-devel 2025-03-03 20:56 ` [FFmpeg-devel] [PATCH v3] " Pranav Kant via ffmpeg-devel 2025-03-11 19:17 ` [FFmpeg-devel] [PATCH v4] " Pranav Kant via ffmpeg-devel 2025-03-11 19:18 ` Pranav Kant via ffmpeg-devel 2025-03-11 23:45 ` Andreas Rheinhardt 2025-03-12 7:05 ` Martin Storsjö 2025-03-12 7:17 ` Andreas Rheinhardt 2025-03-12 7:47 ` Martin Storsjö 2025-03-14 0:16 ` Pranav Kant via ffmpeg-devel [this message] 2025-02-28 2:31 ` [FFmpeg-devel] [PATCH] " Lynne 2025-03-07 2:14 ` Pranav Kant via ffmpeg-devel
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAPvNhsL-V9bKMDk1pw3DuZb-X_nZw0=-mnjBbNQYWQhADk65bQ@mail.gmail.com' \ --to=ffmpeg-devel@ffmpeg.org \ --cc=prka@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel This inbox may be cloned and mirrored by anyone: git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \ ffmpegdev@gitmailbox.com public-inbox-index ffmpegdev Example config snippet for mirrors. AGPL code for this site: git clone https://public-inbox.org/public-inbox.git