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

Tue Jun 29 14:11:25 CEST 2010

On Tue, Jun 29, 2010 at 12:28:50PM +0100, M?ns Rullg?rd wrote:
> 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.

yes, of course :)

> 
> > 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?

i dont know

> 
> >> > 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?

nothing
The unreasonable part if that suggestions that dont violate the C spec are
thrown out by arguing the C spec doesnt require it.
Its like, the c spec says a compiler can do A,B or C as it prefers
and then if someone comes along and says "B would be much easier and efficent
and lead to more readable code". One awnsers "no, we wont because the c spec
does not require us to do B".
Iam sure you see the problem in my silly example above.

The C spec does not require a compiler to generate broken code for
*(uint32_t*)p, it allows a compiler to do so.

> 
> > 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.

reimar said, he had a better idea, iam be waiting for that

[...]
> 
> > 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.

gcc in addition requires strict aliassing to be locally disabled
Iam saying "locally disabled" as an access through av_alias can access
anything not just union members, this is much broader than it should be

*(uint32_t*)p
if it where supported would be much clearer and narrower. It would just mean
this access accesses p. If ps type was uint16_t that would mean it could
alias with any other access to a uint16_t array (if the compiler failed to
completely solve the aliassing), Not this access can alias
all uint32_t/uint16/uint8/... arrays

> 
> > 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 ulimately dont care what the language spec says when all compilers we want
to support handle things the way we need them to do.
If all compilers we cared about, supported *(uint32_t*)p then there would be
no need to do messy tricks.

> 
> >> > 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?

fix what?

> 
> > 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.

Its not the opposite, its the same.
the compilers "optimisations" break aes.c
and the spec gives the compiler the right to do this, no half sane
human would expect that for code like *(uint32_t*)p the compiler would
be unaware that a access through p happened.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100629/5a22d48b/attachment.pgp>