| Message ID | 20161115204658.14288-1-joerg.krause@embedded.rocks |
|---|---|
| State | Accepted |
| Headers | show |
On 15-11-16 21:46, Jörg Krause wrote: > Some ARM architectures like ARNv7-M only supports Thumb instructions, > but the tremor build configuration enables ARM assembly code > unconditionally for all arm triplets by defining _ARM_ASSEM_. > > We are overriding this by undefining this macro for ARM architectures > not supporting ARM instructions. > > Fixes: > http://autobuild.buildroot.net/results/054/054f1c77b0e5d46b2dc53769469c0ed03e6b79ef/ > http://autobuild.buildroot.net/results/ba1/ba1760b1428584f70e44dbffb8218ff3ee55e702/ > http://autobuild.buildroot.net/results/2a6/2a687853cf0bc832fef29f88de0d85bd495fe87d/ > http://autobuild.buildroot.net/results/cb6/cb6c560bf31834aadbe3d13a118b31ea8190159b/ > > Signed-off-by: Jörg Krause <joerg.krause@embedded.rocks> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> However, just to be sure, could you do a test with a toolchain for a cpu that does have ARM instructions, but is configured with BR2_ARM_INSTRUCTIONS_THUMB? Regards, Arnout > --- > package/tremor/tremor.mk | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/package/tremor/tremor.mk b/package/tremor/tremor.mk > index 97b0ce3..2a791b6 100644 > --- a/package/tremor/tremor.mk > +++ b/package/tremor/tremor.mk > @@ -16,9 +16,17 @@ TREMOR_DEPENDENCIES = libogg > > # tremor has ARM assembly code that cannot be compiled in Thumb2 mode, > # so we must force the traditional ARM mode. > +# However, some ARM architectures like ARNv7-M only supports Thumb > +# instructions, but the tremor build configuration enables ARM assembly > +# code unconditionally for all arm triplets by defining _ARM_ASSEM_. > +# We are overriding this by undefining this macro for the ARM > +# architectures not supporting ARM instructions. > ifeq ($(BR2_arm),y) > -TREMOR_CONF_ENV = \ > - CFLAGS="$(TARGET_CFLAGS) -marm" > +ifeq ($(BR2_ARM_CPU_HAS_ARM),y) > +TREMOR_CONF_ENV = CFLAGS="$(TARGET_CFLAGS) -marm" > +else > +TREMOR_CONF_ENV = CFLAGS="$(TARGET_CFLAGS) -U_ARM_ASSEM_" > +endif > endif > > $(eval $(autotools-package)) >
On Tue, 2016-11-15 at 22:18 +0100, Arnout Vandecappelle wrote: > > On 15-11-16 21:46, Jörg Krause wrote: > > Some ARM architectures like ARNv7-M only supports Thumb > > instructions, > > but the tremor build configuration enables ARM assembly code > > unconditionally for all arm triplets by defining _ARM_ASSEM_. > > > > We are overriding this by undefining this macro for ARM > > architectures > > not supporting ARM instructions. > > > > Fixes: > > http://autobuild.buildroot.net/results/054/054f1c77b0e5d46b2dc53769 > > 469c0ed03e6b79ef/ > > http://autobuild.buildroot.net/results/ba1/ba1760b1428584f70e44dbff > > b8218ff3ee55e702/ > > http://autobuild.buildroot.net/results/2a6/2a687853cf0bc832fef29f88 > > de0d85bd495fe87d/ > > http://autobuild.buildroot.net/results/cb6/cb6c560bf31834aadbe3d13a > > 118b31ea8190159b/ > > > > Signed-off-by: Jörg Krause <joerg.krause@embedded.rocks> > > Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> Many thanks for the review! > However, just to be sure, could you do a test with a toolchain for a > cpu that > does have ARM instructions, but is configured with > BR2_ARM_INSTRUCTIONS_THUMB? Tested successfully with BR2_cortex_a9 and BR2_ARM_CPU_HAS_THUMB2. Best regards, Jörg Krause
Hello, On Tue, 15 Nov 2016 21:46:58 +0100, Jörg Krause wrote: > Some ARM architectures like ARNv7-M only supports Thumb instructions, ARNv7 -> ARMv7 > +ifeq ($(BR2_ARM_CPU_HAS_ARM),y) > +TREMOR_CONF_ENV = CFLAGS="$(TARGET_CFLAGS) -marm" > +else > +TREMOR_CONF_ENV = CFLAGS="$(TARGET_CFLAGS) -U_ARM_ASSEM_" > +endif I would have preferred a better solution than this, with a patch to configure.in, to add the appropriate autoconf checks to decide whether or not the optimized ARM code should be used or not. However, consider the fact that the upstream Tremor project seems more or less dead, it means there is very little chance to get our patches merged upstream. So your simple solution is good enough. Therefore: applied to master. Thanks a lot! Thomas
diff --git a/package/tremor/tremor.mk b/package/tremor/tremor.mk index 97b0ce3..2a791b6 100644 --- a/package/tremor/tremor.mk +++ b/package/tremor/tremor.mk @@ -16,9 +16,17 @@ TREMOR_DEPENDENCIES = libogg # tremor has ARM assembly code that cannot be compiled in Thumb2 mode, # so we must force the traditional ARM mode. +# However, some ARM architectures like ARNv7-M only supports Thumb +# instructions, but the tremor build configuration enables ARM assembly +# code unconditionally for all arm triplets by defining _ARM_ASSEM_. +# We are overriding this by undefining this macro for the ARM +# architectures not supporting ARM instructions. ifeq ($(BR2_arm),y) -TREMOR_CONF_ENV = \ - CFLAGS="$(TARGET_CFLAGS) -marm" +ifeq ($(BR2_ARM_CPU_HAS_ARM),y) +TREMOR_CONF_ENV = CFLAGS="$(TARGET_CFLAGS) -marm" +else +TREMOR_CONF_ENV = CFLAGS="$(TARGET_CFLAGS) -U_ARM_ASSEM_" +endif endif $(eval $(autotools-package))
Some ARM architectures like ARNv7-M only supports Thumb instructions, but the tremor build configuration enables ARM assembly code unconditionally for all arm triplets by defining _ARM_ASSEM_. We are overriding this by undefining this macro for ARM architectures not supporting ARM instructions. Fixes: http://autobuild.buildroot.net/results/054/054f1c77b0e5d46b2dc53769469c0ed03e6b79ef/ http://autobuild.buildroot.net/results/ba1/ba1760b1428584f70e44dbffb8218ff3ee55e702/ http://autobuild.buildroot.net/results/2a6/2a687853cf0bc832fef29f88de0d85bd495fe87d/ http://autobuild.buildroot.net/results/cb6/cb6c560bf31834aadbe3d13a118b31ea8190159b/ Signed-off-by: Jörg Krause <joerg.krause@embedded.rocks> --- package/tremor/tremor.mk | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)