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

[Tom Rini]

On Mon, Feb 10, 2014 at 06:26:46PM +0100, Albert ARIBAUD wrote:
> Hi Tom,
> 
> On Mon, 10 Feb 2014 11:24:03 -0500, Tom Rini <trini at ti.com> wrote:
> 
> > On Mon, Feb 10, 2014 at 05:12:24PM +0100, Albert ARIBAUD wrote:
> > > Hi Måns,
> > > 
> > > On Mon, 10 Feb 2014 15:14:49 +0000, Måns Rullgård <mans at mansr.com>
> > > wrote:
> > > 
> > > > Albert ARIBAUD <albert.u.boot at aribaud.net> writes:
> > > > 
> > > > > Hi Tom,
> > > > >
> > > > > On Mon, 10 Feb 2014 08:21:39 -0500, Tom Rini <trini at ti.com> wrote:
> > > > >
> > > > >> On Mon, Feb 10, 2014 at 10:24:47AM +0100, Albert ARIBAUD wrote:
> > > > >> > Hi Tom,
> > > > >> > 
> > > > >> > On Tue,  4 Feb 2014 12:05:33 -0500, Tom Rini <trini at ti.com> wrote:
> > > > >> > 
> > > > >> > > When we tell the compiler to optimize for ARMv7 it assumes a default of
> > > > >> > > unaligned accesses being supported at the hardware level and can make
> > > > >> > > use of this to perform what it deems as an optimization in any case,
> > > > >> > > including allowing for data to become unaligned.  We explicitly disallow
> > > > >> > > this hardware feature so we must tell the compiler.
> > > > >> > > 
> > > > >> > > Cc: Albert ARIBAUD <albert.u.boot at aribaud.net>
> > > > >> > > Cc: Mans Rullgard <mans at mansr.com>
> > > > >> > > Signed-off-by: Tom Rini <trini at ti.com>
> > > > >> > 
> > > > >> > NAK -- the discrepancy between the compiler being told to allow native
> > > > >> > unaligned accesses while at the same time telling the hardware to trap
> > > > >> > them is conscious and voluntary. It was chosen to help detect unaligned
> > > > >> > accesses which are rarely necessary and can be explicitly performed by
> > > > >> > software on a case by case basis.
> > > > >> > 
> > > > >> > If and when a specific file requires unaligned access which cannot be
> > > > >> > made by some other mean than enabling -mno-unaligned-access, then this
> > > > >> > file should have it added, not the whole of U-Boot.
> > > > >> 
> > > > >> Right, I recall the discussion, and we chose wrong.
> > > > >
> > > > > I am quite prepared to discuss whether we chose wrong or right, and
> > > > > to change my mind when the conditions are right, but I'll need more than
> > > > > such a short and simple statement. :)
> > > > 
> > > > I already gave you a detailed explanation some months ago.  You refused
> > > > to read it.
> > > 
> > > I can hardly have "refused to read" a message which I *answered*, laid
> > > out how I thought the issue should be solved... and got no answer after
> > > this.
> > > 
> > > Now, are we going to discuss the technical issue or is this going to go
> > > debian-TC -- which I wouldn't see the point of.
> > 
> > Well, here's the point that I haven't seen an answer to.  If we tell the
> > compiler "you may choose to use unaligned accesses as an optimization,
> > we support this", the compiler says "OK, I shall do that", and then we
> > fail at run time because we don't actually allow the unaligned access,
> > how is this not a problem on our end for the first part of the equation,
> > keeping in mind that the real world is poorly designed and when we write
> > code to this reality the compiler does the correct thing in all cases
> > (or it's a compiler bug).
> 
> Both in the thread you are referring to, and in this current thread, I
> have already explained why we allow native unaligned accesses in the
> compiler and prevent them in the hardware, and how this may seem
> contradictory but is not ; but I'm ok with explaining it again, so here
> comes.
> 
> U-Boot runs on all sorts of hardware. Some hardware can perform native
> unaligned accesses without any penalty; some can, but with a penalty;
> some just can't.
> 
> Therefore, except for some parts of it which are specific to a given
> architecture, the U-Boot source code should not assume that native
> unaligned accesses are possible. In fact, it should assume they are
> impossible.
> 
> Now if we restrict ourselves to a subset of ARM architectures, then
> native unaligned accesses could be used. And if we restrict ourselves
> to ARMv7, they are almost costless.
> 
> So on ARMv7 we could write code which does not care about alignment
> because native unaligned accesses are just as cheap as aligned ones.
> 
> The problem is, since U-Boot runs on a lot of platforms, such
> alignment-agnostic code might end up built and run on hardware which
> does not allow native unaligned accesses. Granted, this hardware will
> detect it; but this might happen quite some time after the code was
> pulled in.

Right.

> Therefore, we should try and detect unaligned accesses as early as
> possible.

Then what I'm saying is that our proposal for catching them isn't valid,
which I'll get to.

> To perform such a detection, there is a bit in ARM architectures which,
> when set, causes native unaligned accesses to trap, allowing us to
> see them as early as possible, which is good: we want to catch such
> accesses.
>
> Now, there is also a compiler flag related to alignments, and that is
> -m[no-]unaligned-access. Its effect is not to allow or disallow
> unaligned accesses, but to allow or disallow *native* unaligned
> accesses. If -mno-unaligned-access is in effect, then native unaligned
> accesses will be replaced by software unaligned accesses, e.g. an
> even-address dword access will be actually performed as a combination
> of byte and word accesses, all aligned. This will prevent the hardware
> from detecting the dword unaligned access, which is bad: we don't want
> to miss such accesses.

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

> *That* is the reason for forbidding unaligned accesses at the hardware
> level while enabling them at the compiler level: it is the only
> combination which ensures that unsuspected unaligned accesses are
> detected at run time.

And it has the unintended side-effect of generating native unaligned
accesses in cases where we have annotated the sources correctly and when
native unaligned access would be disallowed, the compiler would work
correctly.

> Now back to your question, "how is this not a problem on our end for the
> first part of the equation":
> 
> - first, if "the first part of the equation" means "the compiler
>   setting" as opposed to the hardware setting, then the question fails
>   to realize that we don't (and should not) consider the compiler and
>   hardware settings separately; they work in *combination*.

That they work in combination is my point.  We're saying -march=armv7a
-munaligned-access (the latter implicitly as it is the default from the
compiler) and _NOT_ honoring the requirements.

> - second, assuming the question is "how is it not a problem on our end
>   that some code traps due to the combined hardware and compiler
>   settings", the answer is: because the setting was not designed to
>   catch *ARMv7* issues; it was designed to catch *U-Boot* issues. In
>   other words, such traps show that there is code which won't work
>   elsewhere than on ARMv7-like hardware which does not care about
>   alignment.

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

> The only drawback of this setup is that while some code does unneeded
> unaligned accesses (and is thus obviously wrong), some other code
> *requires* unaligned accesses (because of standards, or of hardware).
> Such code will trap... but *only* if it performs unaligned accesses
> *natively*, which it should not since it might run on a hardware not
> capable of native unaligned accesses.
> 
> That is why I consider that the fault is in the trapped software, not
> in the trap. The solution is to make the software use software, not
> native, unaligned accesses. The exact solution depends on whether the
> code has only a few such unaligned accesses (in which case we should
> use explicit unaligned access macros) or many (in which case, for the
> file considered, we can enable -mno-unaligned-access). You'll find
> instances of both in the U-Boot code.

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.

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

-- 
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/fdb916d6/attachment.pgp>