[FFmpeg-devel] [PATCH 2/2] Use gcc/clang builtins for av_sat_(add|sub)_64_c if available.

[Carl Eugen Hoyos]

Am Fr., 1. Mai 2020 um 21:58 Uhr schrieb James Almer <jamrial at gmail.com>:
>
> On 5/1/2020 4:46 PM, Carl Eugen Hoyos wrote:
> > Am Fr., 1. Mai 2020 um 21:42 Uhr schrieb James Almer <jamrial at gmail.com>:
> >>
> >> On 5/1/2020 4:37 PM, Carl Eugen Hoyos wrote:
> >>> Am Fr., 1. Mai 2020 um 21:34 Uhr schrieb James Almer <jamrial at gmail.com>:
> >>>>
> >>>> On 5/1/2020 4:27 PM, Carl Eugen Hoyos wrote:
> >>>>> Am Fr., 1. Mai 2020 um 20:22 Uhr schrieb James Almer <jamrial at gmail.com>:
> >>>>>>
> >>>>>> On 5/1/2020 3:07 PM, Carl Eugen Hoyos wrote:
> >>>>>>> Am Fr., 1. Mai 2020 um 19:24 Uhr schrieb Dale Curtis <dalecurtis at chromium.org>:
> >>>>>>>>
> >>>>>>>> Signed-off-by: Dale Curtis <dalecurtis at chromium.org>
> >>>>>>>> ---
> >>>>>>>>  libavutil/common.h | 10 ++++++++++
> >>>>>>>>  1 file changed, 10 insertions(+)
> >>>>>>>
> >>>>>>> Imo, this is guaranteed to break x86 compilation with some unusual
> >>>>>>> toolchain.
> >>>>>>> I looked carefully at all occurrences of AV_GCC_VERSION_AT_LEAST()
> >>>>>>> and I believe they are in fact different (not for x86 or combined with other
> >>>>>>> checks).
> >>>>>>
> >>>>>> Can you elaborate on this? AV_GCC_VERSION_AT_LEAST() is a public macro
> >>>>>> used in public headers to choose one or another path depending on if the
> >>>>>> compiler is a recent enough GCC, or something else.
> >>>>>
> >>>>>> What toolchain could this break
> >>>>>
> >>>>> It definitely breaks icc compilation on Linux.
> >>>>
> >>>> So ICC on Linux defines __GNUC__ >= 5 yet doesn't support these builtins?
> >>>
> >>> No, but yes, this is the effect.
> >>
> >> So that means ICC is broken, defining support for features it doesn't
> >> really has.
> >> We can workaround that with a !defined(__INTEL_COMPILER) check, i guess,
> >> but perhaps we may want to stop supporting broken compilers.
> >
> > Or we add the necessary two lines of configure code necessary to avoid
> > the issue.
>
> No, a pre-processor check is more than enough. If we really care about
> ICC, then Dale Curtis can add !defined(__INTEL_COMPILER) to the check,
> like it's done in plenty other places for this exact same purpose.

No, it is not enough.

> >>>>>> and why specifically x86?
> >>>>>
> >>>>> I mentioned x86 because afaics, all existing relevant uses of
> >>>>> AV_GCC_VERSION_AT_LEAST() will not be triggered on x86.
> >>>>
> >>>> AV_GCC_VERSION_AT_LEAST is used in a lot of arch agnostic libavutil
> >>>> headers, and in x86/intmath.h
> >>>
> >>> This is exactly what I meant above with "carefully".
> >>
> >> It would be nice if you could stop being so terse and explain what's on
> >> your mind for once. You're not being clear at all, and i asked you to
> >> elaborate, something you still haven't done.
> >
> > To make this crystal-clear:
> > It would be nice if you could stop writing offending and annoying mails as
> > you did today.
>
> How am i writing offending emails? I asked you to elaborate on something
> and barely got any information in return, then asked you to be less
> terse and actually elaborate on what you were trying to argue.

This seems untrue to me.

> > The intmath.h check is surrounded by an additional check that looks much
> > stricter to me than the AV_GCC_VERSION_AT_LEAST() check there.
>
> The AV_GCC_VERSION_AT_LEAST check in x86/intmath.h is wrapped in a
> __GNUC__ check and a __BMI2__ check. The former has no effect as far as
> the macro is concerned since it's already a part of it, and the latter
> is yet another GCC define, hence being checked after __GNUC__.
> And being in the x86 folder, it's of course triggered on x86 targets.
>
> Meanwhile, all the AV_GCC_VERSION_AT_LEAST checks in attributes.h are
> triggered for every single target, with considerations for clang and ICC
> where required.

Which case in attributes.h can cause a compilation failure?
I am curious because iirc I wrote some of them, I definitely tested them.

Carl Eugen