Ping. On Tue, Apr 15, 2025 at 4:22 PM Pranav Kant <p...@google.com> wrote:
> Hello again. Is there anything else I can do here? > > On Fri, Apr 4, 2025 at 11:40 AM Pranav Kant <p...@google.com> wrote: > >> Any thoughts on this? >> >> On Thu, Mar 20, 2025 at 5:30 PM Pranav Kant <p...@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 <p...@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 <p...@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-requ...@ffmpeg.org with subject "unsubscribe".
