Patchwork [U-Boot] arm: prevent using movt/movw address loads

login
register
mail settings
Submitter Jeroen Hofstee
Date Aug. 24, 2013, 11:55 a.m.
Message ID <1377345338-9695-1-git-send-email-jeroen@myspectrum.nl>
Download mbox | patch
Permalink /patch/269634/
State Accepted
Delegated to: Albert ARIBAUD
Headers show

Comments

Jeroen Hofstee - Aug. 24, 2013, 11:55 a.m.
The movt/movw instruction can be used to hardcode an
memory location in the instruction itself. The linker
starts complaining about this if the compiler decides
to do so: "relocation R_ARM_MOVW_ABS_NC against `a local
symbol' can not be used" and it is not support by U-boot
as well. Prevent their use by requiring word relocations.
This allows u-boot to be build at other optimalization
levels then -Os.

Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>
Cc: TigerLiu@viatech.com.cn
Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
---
 arch/arm/config.mk | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
TigerLiu@viatech.com.cn - Sept. 17, 2013, 10:44 a.m.
Hi, experts:
>-# check that only R_ARM_RELATIVE relocations are generated
> ifneq ($(CONFIG_SPL_BUILD),y)
>-ALL-y	+= checkarmreloc
>+# Check that only R_ARM_RELATIVE relocations are generated.
>+ALL-y += checkarmreloc
>+# The movt / movw can hardcode 16 bit parts of the addresses in the
>+# instruction. Relocation is not supported for that case, so disable
>+# such usage by requiring word relocations.
>+PLATFORM_CPPFLAGS += $(call cc-option, -mword-relocations)
> endif
Jeroen's patch is very simple.
So, is there any side-effect?
If not, why not add it into 2013.10 release version? :)

Best wishes,
Jeroen Hofstee - Sept. 17, 2013, 6:34 p.m.
On 09/17/2013 12:44 PM, TigerLiu@viatech.com.cn wrote:
>
> Jeroen's patch is very simple.
> So, is there any side-effect?

Not that I am aware of.
> If not, why not add it into 2013.10 release version? :)

That is up to Albert and Tom.

Regards,
Jeroen
Tom Rini - Sept. 19, 2013, 9:16 p.m.
On Sat, Aug 24, 2013 at 01:55:38PM +0200, Jeroen Hofstee wrote:

> The movt/movw instruction can be used to hardcode an
> memory location in the instruction itself. The linker
> starts complaining about this if the compiler decides
> to do so: "relocation R_ARM_MOVW_ABS_NC against `a local
> symbol' can not be used" and it is not support by U-boot
> as well. Prevent their use by requiring word relocations.
> This allows u-boot to be build at other optimalization
> levels then -Os.
> 
> Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>
> Cc: TigerLiu@viatech.com.cn
> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
>  arch/arm/config.mk | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Is this also something we need for llvm?  I am hesitant here because as
Wolfgang points out, -O0 is usually the wrong way to debug a problem and
I'll add we're well into the age where debuggers work just fine with
optimized code.  If there's some -O2 enabled gcc flag we want because of
a measurable performance win, we should add it specifically to -Os.
Jeroen Hofstee - Sept. 20, 2013, 5:15 p.m.
Hello Tom,

On 09/19/2013 11:16 PM, Tom Rini wrote:
> On Sat, Aug 24, 2013 at 01:55:38PM +0200, Jeroen Hofstee wrote:
>
>> The movt/movw instruction can be used to hardcode an
>> memory location in the instruction itself. The linker
>> starts complaining about this if the compiler decides
>> to do so: "relocation R_ARM_MOVW_ABS_NC against `a local
>> symbol' can not be used" and it is not support by U-boot
>> as well. Prevent their use by requiring word relocations.
>> This allows u-boot to be build at other optimalization
>> levels then -Os.
>>
>> Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>
>> Cc: TigerLiu@viatech.com.cn
>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
>> ---
>>   arch/arm/config.mk | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
> Is this also something we need for llvm?

You guessed that right, for clang actually (llvm has
already been taught to not emit movw/movt pairs,
when requested not to do so). So with the
-mword-relocations || present I can teach clang to tell llvm
not to do it.

I am not aware of any reason why gcc could not
decide to do the same in future releases. A pointer
comparison e.g. is of exactly the same size (afaik).
In this case U-boot will no longer compile without
mentioned flag.

> I am hesitant here because as
> Wolfgang points out, -O0 is usually the wrong way to debug a problem and
> I'll add we're well into the age where debuggers work just fine with
> optimized code.

mmm, I don't share your concern here. Not that I
disagree with what Wolfgang said, but since it is
unrelated to the patch itself. What I read was that
Wolfgang tried to explain to a ML poster without a
proper name that it might be even harder at times to
find a bug at -O0, since it is a different binary and
that it is not considered a bug. I assume the fast
majority of U-boot developers know these to debug
things..

If you really have that little trust in U-boot developers
a more proper way would be to actually create a make
rule checking cflags and point them to a nice debugging
document. And I really hope you don't do that ;)

One thing I can think of in favour of -O0 is for educational
purposes. You can run u-boot in qemu then without the,
at times weird optimized jumps, to get an idea about basic
code flow.

> If there's some -O2 enabled gcc flag we want because of
> a measurable performance win, we should add it specifically to -Os.
>
First of all the default -Os is unchanged and I have no
intention to change it. -O2 won't build without the patch
last time I checked ;)

Anyway, I like the flag since it helps to not special case
clang and it guarantees builds with gcc at all optimisation
levels, now and in the future. I don't care if it goes in this
release or the next one.

Regards,
Jeroen
Tom Rini - Sept. 20, 2013, 6:03 p.m.
On Fri, Sep 20, 2013 at 07:15:29PM +0200, Jeroen Hofstee wrote:
> Hello Tom,
> 
> On 09/19/2013 11:16 PM, Tom Rini wrote:
> >On Sat, Aug 24, 2013 at 01:55:38PM +0200, Jeroen Hofstee wrote:
> >
> >>The movt/movw instruction can be used to hardcode an
> >>memory location in the instruction itself. The linker
> >>starts complaining about this if the compiler decides
> >>to do so: "relocation R_ARM_MOVW_ABS_NC against `a local
> >>symbol' can not be used" and it is not support by U-boot
> >>as well. Prevent their use by requiring word relocations.
> >>This allows u-boot to be build at other optimalization
> >>levels then -Os.
> >>
> >>Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>
> >>Cc: TigerLiu@viatech.com.cn
> >>Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> >>---
> >>  arch/arm/config.mk | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >Is this also something we need for llvm?
> 
> You guessed that right, for clang actually (llvm has
> already been taught to not emit movw/movt pairs,
> when requested not to do so). So with the
> -mword-relocations || present I can teach clang to tell llvm
> not to do it.
> 
> I am not aware of any reason why gcc could not
> decide to do the same in future releases. A pointer
> comparison e.g. is of exactly the same size (afaik).
> In this case U-boot will no longer compile without
> mentioned flag.

OK.

[snip]
> >If there's some -O2 enabled gcc flag we want because of
> >a measurable performance win, we should add it specifically to -Os.
> >
> First of all the default -Os is unchanged and I have no
> intention to change it. -O2 won't build without the patch
> last time I checked ;)
> 
> Anyway, I like the flag since it helps to not special case
> clang and it guarantees builds with gcc at all optimisation
> levels, now and in the future. I don't care if it goes in this
> release or the next one.

Right, I'm OK picking this patch up then, on the grounds of making
clang/llvm work now, and potentially keeping a future gcc happy.
Simon Glass - Sept. 21, 2013, 2:43 a.m.
On Sat, Aug 24, 2013 at 5:55 AM, Jeroen Hofstee <jeroen@myspectrum.nl>wrote:

> The movt/movw instruction can be used to hardcode an
> memory location in the instruction itself. The linker
> starts complaining about this if the compiler decides
> to do so: "relocation R_ARM_MOVW_ABS_NC against `a local
> symbol' can not be used" and it is not support by U-boot
> as well. Prevent their use by requiring word relocations.
> This allows u-boot to be build at other optimalization
> levels then -Os.
>
> Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>
> Cc: TigerLiu@viatech.com.cn
> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
>

This is useful I think. I'm not sure that -O0 works very well anymore (at
least I need to make a few tweaks to use it), but -O1 is useful in some
cases to provide better debugging.

Acked-by: Simon Glass <sjg@chromium.org>
Albert ARIBAUD - Sept. 23, 2013, 1:48 p.m.
Hi Jeroen,

On Sat, 24 Aug 2013 13:55:38 +0200, Jeroen Hofstee
<jeroen@myspectrum.nl> wrote:

> The movt/movw instruction can be used to hardcode an
> memory location in the instruction itself. The linker
> starts complaining about this if the compiler decides
> to do so: "relocation R_ARM_MOVW_ABS_NC against `a local
> symbol' can not be used" and it is not support by U-boot
> as well. Prevent their use by requiring word relocations.
> This allows u-boot to be build at other optimalization
> levels then -Os.
> 
> Signed-off-by: Jeroen Hofstee <jeroen@myspectrum.nl>
> Cc: TigerLiu@viatech.com.cn
> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
>  arch/arm/config.mk | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/config.mk b/arch/arm/config.mk
> index 540a119..2277c82 100644
> --- a/arch/arm/config.mk
> +++ b/arch/arm/config.mk
> @@ -94,7 +94,11 @@ PLATFORM_RELFLAGS += -fno-optimize-sibling-calls
>  endif
>  endif
>  
> -# check that only R_ARM_RELATIVE relocations are generated
>  ifneq ($(CONFIG_SPL_BUILD),y)
> -ALL-y	+= checkarmreloc
> +# Check that only R_ARM_RELATIVE relocations are generated.
> +ALL-y += checkarmreloc
> +# The movt / movw can hardcode 16 bit parts of the addresses in the
> +# instruction. Relocation is not supported for that case, so disable
> +# such usage by requiring word relocations.
> +PLATFORM_CPPFLAGS += $(call cc-option, -mword-relocations)
>  endif

Applied as a bugfix to u-boot-arm/master, thanks!

Amicalement,

Patch

diff --git a/arch/arm/config.mk b/arch/arm/config.mk
index 540a119..2277c82 100644
--- a/arch/arm/config.mk
+++ b/arch/arm/config.mk
@@ -94,7 +94,11 @@  PLATFORM_RELFLAGS += -fno-optimize-sibling-calls
 endif
 endif
 
-# check that only R_ARM_RELATIVE relocations are generated
 ifneq ($(CONFIG_SPL_BUILD),y)
-ALL-y	+= checkarmreloc
+# Check that only R_ARM_RELATIVE relocations are generated.
+ALL-y += checkarmreloc
+# The movt / movw can hardcode 16 bit parts of the addresses in the
+# instruction. Relocation is not supported for that case, so disable
+# such usage by requiring word relocations.
+PLATFORM_CPPFLAGS += $(call cc-option, -mword-relocations)
 endif