[FFmpeg-devel] [PATCH] all: silence clang -Wabsolute-value for unsigned subtractions

[Ganesh Ajjanagadde]

On Sat, Aug 22, 2015 at 10:26 AM, Nicolas George <george at nsup.org> wrote:
> Le quintidi 5 fructidor, an CCXXIII, Ganesh Ajjanagadde a écrit :
>> +    /* cast for -Wabsolute-value in clang 3.5+ */
>> +    return abs((int) (t1 - t2)) + abs((int) ((c1 & 0x000000ff) - (c2 & 0x000000ff))) +
>> +        abs((int) (((c1 & 0x0000ff00) >> 8) - ((c2 & 0x0000ff00) >> 8))) +
>> +        abs((int) (((c1 & 0x00ff0000) >> 16) - ((c2 & 0x00ff0000) >> 16)));
>
> I believe this is not correct, with or without the cast (the cast is
> implicit due to the prototype of abs(), of course): if the result is morally
> negative, converting the morally-negative unsigned to signed is an undefined
> behaviour.

The cast is indeed implicit; I just added it to preserve exact old
behavior with suppression of this warning,
which it does.
However, you raise an excellent point that the old code was actually
not standards compliant,
and we need to fix this.

>
> The correct fix would probably be "abs((int)x - (int)y)". Hopefully, the
> compiler will generate the exact same code. A macro absdiff() would probably
> make things more readable.

The cast idea is incorrect as stated, though your idea is sound:
int is not guaranteed to be of 32 bits on all platforms.
Casting a uint32_t to an int will be bad.
The correct (and arguably more sustainable solution) is to do what the
clang mail does:

unsigned f(unsigned a, unsigned b) {
  if (a > b)
    return a - b;
  else
    return b - a;
}
but as a macro for generality.
(perhaps with ternary operator for compactness, but that is a detail).
I assume this is what you had in mind with your macro idea.
This is guaranteed to be always right: there are no issues with overflow,
the types of operands and result can be guaranteed to be same.
However, note that this won't extend to signed operands,
simply because MAX - (-MAX) > MAX, requiring larger width for the result.
This will be a problem for another day.
I hence would name it accordingly to clarify it is only for unsigned operands.

I will add such a macro to avutil/common due to general use unless
people object.

Note that if we want generality, tgmath might offer a solution.
I don't see it being used, and suspect we don't have it on all platforms.

As an aside, I find this whole is rather silly from a mathematical perspective,
I mean since we are masking, the difference will be always at max 8/9
bits in length,
though individually the operands are large.

>
> Not that you did not introduce the undefined behaviour. You just noticed it
> with that new warning.
>
> Regards,
>
> --
>   Nicolas George
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>