Re: [FFmpeg-devel] [PATCH v5] Mark C globals with small code model

[Pranav Kant via ffmpeg-devel <ffmpeg-devel@ffmpeg.org>]

Ping.

On Tue, Apr 15, 2025 at 4:22 PM Pranav Kant <prka@google.com> wrote:

> Hello again. Is there anything else I can do here?
>
> On Fri, Apr 4, 2025 at 11:40 AM Pranav Kant <prka@google.com> wrote:
>
>> Any thoughts on this?
>>
>> On Thu, Mar 20, 2025 at 5:30 PM Pranav Kant <prka@google.com> wrote:
>>
>>> Patch version v5:
>>> - Uses two new macros DECLARE_ASM_VAR (used for both external and inline
>>> asm) and DECLARE_EXTERNAL_ASM_VAR (used only for external asm)
>>> - I intend to remove explicit existing use of
>>> attribute_visibility_hidden in follow-up patch and instead use
>>> DECLARE_EXTERNAL_ASM_VAR for those variables
>>> - Other variables will be marked with these two new macros in a
>>> follow-up patch. I want to settle down on the infrastructure first with a
>>> handful of variables.
>>>
>>> Let me know if you have any questions.
>>>
>>> On Thu, Mar 20, 2025 at 5:28 PM Pranav Kant <prka@google.com> wrote:
>>>
>>>> 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, external assembly and inline assembly
>>>> continue to access these globals using older PC-relative addressing
>>>> which may not work because globals may be placed too far away.
>>>>
>>>> Introduce new macros for such variables that mark them with small code
>>>> model attribute. This ensures that these variables are never allocated
>>>> in large data sections, and continue to be validly accessed from
>>>> 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.h             |  4 +++-
>>>>  libavcodec/cabac.h              |  4 +++-
>>>>  libavcodec/x86/constants.h      | 12 +++++++-----
>>>>  libavutil/attributes.h          |  6 ++++++
>>>>  libavutil/attributes_internal.h | 16 ++++++++++++++++
>>>>  libavutil/mem_internal.h        | 13 +++++++++++++
>>>>  6 files changed, 48 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/libavcodec/ac3dsp.h b/libavcodec/ac3dsp.h
>>>> index b1b2bced8f..a3c55a833b 100644
>>>> --- a/libavcodec/ac3dsp.h
>>>> +++ b/libavcodec/ac3dsp.h
>>>> @@ -25,11 +25,13 @@
>>>>  #include <stddef.h>
>>>>  #include <stdint.h>
>>>>
>>>> +#include "libavutil/mem_internal.h"
>>>> +
>>>>  /**
>>>>   * Number of mantissa bits written for each bap value.
>>>>   * bap values with fractional bits are set to 0 and are calculated
>>>> separately.
>>>>   */
>>>> -extern const uint16_t ff_ac3_bap_bits[16];
>>>> +extern DECLARE_EXTERNAL_ASM_VAR(16, const uint16_t,
>>>> ff_ac3_bap_bits)[16];
>>>>
>>>>  typedef struct AC3DSPContext {
>>>>      /**
>>>> diff --git a/libavcodec/cabac.h b/libavcodec/cabac.h
>>>> index 38d06b2842..df352258c6 100644
>>>> --- a/libavcodec/cabac.h
>>>> +++ b/libavcodec/cabac.h
>>>> @@ -29,7 +29,9 @@
>>>>
>>>>  #include <stdint.h>
>>>>
>>>> -extern const uint8_t ff_h264_cabac_tables[512 + 4*2*64 + 4*64 + 63];
>>>> +#include "libavutil/mem_internal.h"
>>>> +
>>>> +extern DECLARE_ASM_VAR(1, const uint8_t, ff_h264_cabac_tables)[512 +
>>>> 4*2*64 + 4*64 + 63];
>>>>  #define H264_NORM_SHIFT_OFFSET 0
>>>>  #define H264_LPS_RANGE_OFFSET 512
>>>>  #define H264_MLPS_STATE_OFFSET 1024
>>>> diff --git a/libavcodec/x86/constants.h b/libavcodec/x86/constants.h
>>>> index 0c6bf41fa0..2561302604 100644
>>>> --- a/libavcodec/x86/constants.h
>>>> +++ b/libavcodec/x86/constants.h
>>>> @@ -23,13 +23,14 @@
>>>>
>>>>  #include <stdint.h>
>>>>
>>>> +#include "libavutil/mem_internal.h"
>>>>  #include "libavutil/x86/asm.h"
>>>>
>>>> -extern const ymm_reg  ff_pw_1;
>>>> +extern DECLARE_EXTERNAL_ASM_VAR(32, const ymm_reg, ff_pw_1);
>>>>  extern const ymm_reg  ff_pw_2;
>>>>  extern const xmm_reg  ff_pw_3;
>>>> -extern const ymm_reg  ff_pw_4;
>>>> -extern const xmm_reg  ff_pw_5;
>>>> +extern DECLARE_ASM_VAR(32, const ymm_reg, ff_pw_4);
>>>> +extern DECLARE_ASM_VAR(16, const xmm_reg, ff_pw_5);
>>>>  extern const xmm_reg  ff_pw_8;
>>>>  extern const xmm_reg  ff_pw_9;
>>>>  extern const uint64_t ff_pw_15;
>>>> @@ -43,7 +44,7 @@ extern const uint64_t ff_pw_128;
>>>>  extern const ymm_reg  ff_pw_255;
>>>>  extern const ymm_reg  ff_pw_256;
>>>>  extern const ymm_reg  ff_pw_512;
>>>> -extern const ymm_reg  ff_pw_1023;
>>>> +extern DECLARE_EXTERNAL_ASM_VAR(32, const ymm_reg, ff_pw_1023);
>>>>  extern const ymm_reg  ff_pw_1024;
>>>>  extern const ymm_reg  ff_pw_2048;
>>>>  extern const ymm_reg  ff_pw_4095;
>>>> @@ -52,9 +53,10 @@ extern const ymm_reg  ff_pw_8192;
>>>>  extern const ymm_reg  ff_pw_m1;
>>>>
>>>>  extern const ymm_reg  ff_pb_0;
>>>> -extern const ymm_reg  ff_pb_1;
>>>> +extern DECLARE_EXTERNAL_ASM_VAR(32, const ymm_reg, ff_pb_1);
>>>>  extern const ymm_reg  ff_pb_2;
>>>>  extern const ymm_reg  ff_pb_3;
>>>> +extern DECLARE_ASM_VAR(32, const xmm_reg, ff_pb_15);
>>>>  extern const ymm_reg  ff_pb_80;
>>>>  extern const ymm_reg  ff_pb_FE;
>>>>  extern const uint64_t ff_pb_FC;
>>>> 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
>>>> + * 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)
>>>> +#    define attribute_mcmodel_small __attribute__((model("small")))
>>>> +#else
>>>> +#    define attribute_mcmodel_small
>>>> +#endif
>>>> +
>>>>  #endif /* AVUTIL_ATTRIBUTES_INTERNAL_H */
>>>> diff --git a/libavutil/mem_internal.h b/libavutil/mem_internal.h
>>>> index c027fa51c3..efb4c89c39 100644
>>>> --- a/libavutil/mem_internal.h
>>>> +++ b/libavutil/mem_internal.h
>>>> @@ -29,6 +29,7 @@
>>>>  #endif
>>>>
>>>>  #include "attributes.h"
>>>> +#include "attributes_internal.h"
>>>>  #include "macros.h"
>>>>
>>>>  /**
>>>> @@ -113,6 +114,18 @@
>>>>  #define DECLARE_ALIGNED_32(t,v) DECLARE_ALIGNED_T(ALIGN_32, t, v)
>>>>  #define DECLARE_ALIGNED_64(t,v) DECLARE_ALIGNED_T(ALIGN_64, t, v)
>>>>
>>>> +// DECLARE_ASM_VAR used for variables accessed by inline asm
>>>> +// and external assembly
>>>> +#define DECLARE_ASM_VAR(n,t,v)             \
>>>> +        attribute_mcmodel_small            \
>>>> +        alignas(n) t v
>>>> +// DECLARE_EXTERNAL_ASM_VAR used for variables exclusively
>>>> +// accessed by external assembly
>>>> +#define DECLARE_EXTERNAL_ASM_VAR(n,t,v)    \
>>>> +        attribute_visibility_hidden        \
>>>> +        attribute_mcmodel_small            \
>>>> +        alignas(n) t v
>>>> +
>>>>  // Some broken preprocessors need a second expansion
>>>>  // to be forced to tokenize __VA_ARGS__
>>>>  #define E1(x) x
>>>> --
>>>> 2.49.0.395.g12beb8f557-goog
>>>>
>>>>
_______________________________________________
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".