Martin Storsjö: > On Wed, 12 Mar 2025, Andreas Rheinhardt 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 <p...@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? >> >>> 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. >> >>> 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). >> >>> 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/ >> >>> + * 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: > > FWIW, in e4ac156b7c47725327dff78bb83f5eecbaee3add we added > attribute_visibility_hidden to a bunch of symbols that are accessed from > aarch64 assembly, in a similar fashion. While the effects are different, > I wonder if we should abstract these two down to a more generic > attribute for any symbol accessed directly from any assembly. >
As I said above: We would only need two macros: One for any object accessed from inline assembly and one for objects accessed by external assembly (and not inline assembly). The latter would be hidden visibility+small code model, the former would also have av_used. Of course, both would allow to specify alignment. But naming is hard. How about DECLARE_ASM_VAR and DECLARE_INLINE_ASM_VAR? - Andreas _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
