[U-Boot] [PATCH v2] arm: Switch to -mno-unaligned-access when supported by the compiler

[Tom Rini]

On Mon, Feb 10, 2014 at 11:17:23PM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20140210212630.GB7049 at bill-the-cat> you wrote:
> > 
> > Then gcc has a bug and you need to convince them to fix it.  What gcc
> > does, as Mans has explained, and this invalidates the "lets catch
> > unaligned access problems" notion, is for ARMv6 and higher say "we
> > assume by default the hardware can perform native unaligned access, so
> > make use of this in our optimization choices".
> 
> GCC for ARM has often perplexed me - I remember cases where it
> generated 4 single-byte accesses to a u32 data type with perfectly
> valid 32 bit alignment (like a device register).  Unfortunaltely I
> never was able to have this considered a bug.  Everybody else thought
> it was perfectly normal and it had always been like that (on ARM).
> 
> > But this part isn't true.  Or rather, it is (or may, at the whim of the
> > compiler) catching every case where we say __attribute__((packed)) and
> > then don't follow up ensuring that every access is then fixed up by
> > hand, rather than letting the compiler do it.
> >
> > We've essentially picked "lets blow things up at times" over "lets keep
> > an eye out for __packed being used in code, and be careful there".
> 
> I think many people use __packed without thinking.  Some code is just
> horrible.  The fact that ARM code is full of examples where device I/O
> is performed without compiler checking for data types is just an
> indication.

Some quick grepping around and it's not really ARM code that's full of
the references, it's
1) Code shared with Linux (NAND, UBI or filesystems, JFFS2/YAFFS2).
2) USB (which is a special case of the above).

> I know this is bad, but do you see a way to make the compiler issue
> clear warnings or errors for such "accidential" unaligned accesses?

No, but we could make checkpatch complain about it pretty easily I bet.

> > The problem is that __packed means we can see this problem whenever its
> > used.  This is the design practice we need to be wary of, and make sure
> > we're coding to an unfortunate reality rather than misoptimizing things.
> 
> __packed should be strictly forbidden everywhere except where mandated
> by definitions for example of protocol implementations etc.  And even
> there I tend to consider it wrong to use 32 or 16 bit types for data
> fields that are misaligned (assum=ing the whole data structure is
> properly aligned).

I agree we shouldn't use __packed without a good reason.  And I don't
think we've got many no-reason uses right now but I'm all in favor of
dropping them and keeping an eye on new users.  The problem is that
unless we do something, any of the valid and we aren't going to / can't
change them __packed structs will be the next place things blow up when
gcc optimizes things in a new (and valid based on what we've told it!)
way.

> > > Regarding the EFI code, there was a patch submitted (see the thread you
> > > have pointed me to) which had the proper unaligned access macros and
> > > thus did not require -mno-unaligned-access for this file, and certainly
> > > does not require it for the whole of ARM.
> >
> > Right, the EFI patch takes valid code and makes it differently valid.
> 
> Takes valid code?  But would this original code run on an architecture
> where any unaligned access causes a machine check?  My understanding
> is it doesn't?

It would run because being __packed gcc would know to do the right thing
on that architecture.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140210/e39abb80/attachment.pgp>