[FFmpeg-devel] [PATCH] strict-aliasing-safe aes.c

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> On Tue, Jun 29, 2010 at 02:02:41AM +0100, M?ns Rullg?rd wrote:
>> Michael Niedermayer <michaelni at gmx.at> writes:
>> 
>> > On Mon, Jun 28, 2010 at 10:13:31PM +0200, Reimar D?ffinger wrote:
>> >> On Mon, Jun 28, 2010 at 10:02:15PM +0200, Michael Niedermayer wrote:
>> >> > On Mon, Jun 28, 2010 at 05:55:30PM +0200, Reimar D?ffinger wrote:
>> >> > > Hello,
>> >> > > this uses AV_WN32A, AV_WN64A etc. macros.
>> >> > > The generated code is the same on x86_64 (assuming I did not mess up that test).
>> >> > 
>> >> > >  aes.c |   20 +++++++++++++-------
>> >> > >  1 file changed, 13 insertions(+), 7 deletions(-)
>> >> > > 76705b63949d4abbe51b0d7d59537045ae91179a  aesalias.diff
>> >> > 
>> >> > this makes the code unreadable, iam thus against it.
>> >> 
>> >> Well, what is the alternative? The current code seems to not work with some compilers.
>> >
>> > Fix the compiler.
>> 
>> There is nothing wrong with the compiler.  It implements the C99
>> language exactly as specified.
>
> The C99 standard isnt everything, a mpeg2 encoder also could return
> nothing but a black picture independant of the input, yet it wouldnt be a
> reasonable mpeg encoder or a usefull one.

It would still have to output a bitstream compatible with the
specification.

> I know you are the one who prefered the mpeg demuxer to comply to the spec
> even at the cost of not demuxing actual existing files.

I fail to see the relevance.

>> > This language lawyering and pedantic gnu style compliance crussade is
>> 
>> This has nothing to do with GNU and everything to do with the C99 standard.
>
> yes, that comment was about something else

Then why bring it up here?

>> > really annoying. Anything that gnu considers to be worth warning
>> > about makes people run and change their code, and often to the
>> > worse.
>> 
>> In this particular case the code is in violation of the spec.  That
>> the compiler only recently got the ability to detect this does not
>> make it less of a violation.
>
> again the comment is not about aliassing violations but warnings in general

But here we have some very real errors that gcc has been warning about
for YEARS.

>> > what i think should be done is: people should post a feature
>> > request on the <put affected compiler here> bug tracker about it
>> > acting a bit more reasonable and handle such trivial aliassing
>> > cases. And everyone who considers this important should reply
>> > there and express his oppinon.
>> 
>> THERE IS NO BUG IN THE COMPILER!
>
> please send your strawman back as well, i never said theres a bug.
> I said the compiler behaves unreasonable and i clearly was talking about
> a feature request.

The compiler behaves according the C spec.  What is unreasonable about
that?

> we arent talking about the same thing it seems.

How about we start doing that then?  I'm talking about the patch for
aes.c Reimar sent.

> I guess we agree that the compiler should generate fast code.

Yes, but it _also_ must be correct code.  The generated code must be
correct for any possible intent of the programmer.  This means some
pointers must be assumed to alias.

> I guess we agree that fast code needs types sometimes accessed
> through different types.

Yes.

> I guess we agree that there must be some way for the programmer to
> specify how pointers can alias and some default rules about aliasing
> in absence.

Yes, and the defaults must be chosen to allow a decent level of
performance without detailed 

> What iam saying is that a compiler should detect 
> *(uint32_t*)p
> and accept it as correct code

This _is_ correct code provided whatever "p" points to is always
accessed as uint32_t.  If it is ever accessed as another type, this is
a strict aliasing violation.

> like it per spec has to accept ((magic_union_type*)p)->u32

The union trick exists so type punning can be done in a safe way
without sacrificing the optimisations possible with strict aliasing
rules.

> I also think that the aliassing model used by gcc is severly lacking.
> It for example lacks the possibility of the programmer specifying how
> 2 pointers can alias exactly. The issue just came up with the dct
> optimization and forced us to adapt our design of it.
> I dont like having to design our code around shortcommings of the compiler

You are not adequately distinguishing between the language spec and
the compiler.

>> > I dont mind us working around it here if theres some effort
>> > toward fixing this where the problem is (aka compilers and the C
>> > spec) even if that effort is small and unlikely to be successfull
>> > but if everything thats done is directed toward turning every
>> > file into a unmaintaunable mess with not one second spend on
>> > solving the actual problem then i dont think i will approve such
>> > workarounds. Id like to have at least some hope even if small
>> > that this red tape around every bit of optimized code one day
>> > would become unneeded
>> 
>> The main author of libswscale is in no position to speak about an
>> unmaintainable mess.
>
> I have less problem maintaining it

Then why don't you fix it?

> besides instead of continuing to reply to your language lawyering trolling
> ill refer you to linus torvalds:
> http://kerneltrap.org/mailarchive/linux-kernel/2007/10/26/359162
> http://lkml.org/lkml/2003/2/26/158
> Ive seen better quotes from him about it but i cant find them atm

This is the opposite of our situation.  Linus dislikes optimisations
that are allowed by the spec but cause trouble for real-world code.
Not performing these optimisations would never break anything.

Your sloppy aliasing would break perfectly valid code, and that is not
acceptable.

-- 
M?ns Rullg?rd
mans at mansr.com