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

[Albert ARIBAUD]

Hi Tom,

On Mon, 10 Feb 2014 17:28:19 -0500, Tom Rini <trini at ti.com> wrote:

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

Indeed, this is not *ARM-specific* code, but it is *ARM-run* code.

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

I'm not quite convinced that checkpatch would detect all cases --
granted, it could detect "packed" attributes, but not all "packed"
structs are Bad(tm), and besides, not all unaligned accesses are due to
packed structures (bad pointer arithmetic is also a good candidate, one
which results from code semantics, not source).

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

Which is precisely why the solution is to tell GCC not to optimize, by
telling it to use explicitly unaligned accesses for the data which we
want to bypas normal C alignment rules.

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

It is true that GCC might change behavior in the future and break
things on us; after all, that's what 4.7 did with ARMv7 and native
unaligned accesses. But then, we should expect GCC possibly doing it
again in the future for new targets, causing packed code to break on
us, unless we explicitly tell it not to by stating where unaligned
accesses should be performed.

(IMO, making unaligned accesses explicit -- either with a macro or by
breaking unaligned fields into smaller aligned fields -- goes hand in
hand with adding packed attributes: if you pack a record, chances are
you're *wanting* unalignd fields, and if you want them, you should
make them explicit.)

Back to the issue at hand, I think we should fix it with a patch like

http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/171646/focus=179000

Also, upon re-reading doc/ARM.unaligned-accesses, I notice that the
rationale for the hardware and compiler settings is not explicit; I'll
add it.

Amicalement,
-- 
Albert.