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

[Tom Rini]

On Tue, Feb 11, 2014 at 09:44:50AM +0100, Albert ARIBAUD wrote:
> 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.

Right, and irrelevant.

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

Right.  But it's like the warnings about volatile and typedef.  There's
exceptions (see above), but when we see it we need to question it.  And
people doing stupid things to pointers isn't a good excuse and probably
not even something that would be fixedup with -mno-unaligned-access.

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

No.  The solution is to tell the compiler which optimizations it can,
and cannot use.  If we stop telling it that native unaligned accesses
work it won't decide to make use of those optimizations.

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

You're saying that in the future gcc might start generating broken code
for a platform, but that it would not be a gcc bug?  Lets step back and
remember what exactly gcc did.

They decided that (and added support, and optimizations around) given
that ARMv6 and later can do native unaligned accesses, they would assume
that when generating for those targets, that feature would be enabled.
The Linux Kernel decided "OK, lets set that bit in hardware".  We
decided "lets not set that bit in hardware, and do other things too".

I've decided that we made the wrong choice here and we should go with
"no native unaligned access support on ARM".  This puts us back to how
things were prior to GCC adding support for optimizing around native
unaligned access support.  This does not change (nor does our current
behavor!) how to deal with broken hardware or broken software.

-- 
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/20140212/7532ad05/attachment.pgp>