On Wed, Apr 05, 2023 at 07:48:54PM -0400, Tom Rini wrote:

When building for ARM64, we need to pass -ffixed-x18 and otherwise pass -ffixed-r9. Rather than having this logic in two places, we can do this once in arch/arm/config.mk. Further, while gcc will ignore being passed both -ffixed-r9 and -ffixed-x18 and simply use -ffixed-x18, clang will note that -ffixed-r9 is not used. Remove this duplication to also remove the warning.

Signed-off-by: Tom Rini trini@konsulko.com Reviewed-by: Simon Glass sjg@chromium.org

Applied to u-boot/master, thanks!

arch/arm/cpu/armv8/config.mk | 1 - 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/arm/config.mk b/arch/arm/config.mk index bf781f102620..5530d02b66c4 100644 --- a/arch/arm/config.mk +++ b/arch/arm/config.mk @@ -3,7 +3,13 @@ # (C) Copyright 2000-2002 # Wolfgang Denk, DENX Software Engineering, wd@denx.de.

-CFLAGS_NON_EFI := -fno-pic -ffixed-r9 -ffunction-sections -fdata-sections \ +ifeq ($(CONFIG_ARM64),y) +FIXED_REG := -ffixed-x18 +else +FIXED_REG := -ffixed-r9 +endif

+CFLAGS_NON_EFI := -fno-pic $(FIXED_REG) -ffunction-sections -fdata-sections \ -fstack-protector-strong CFLAGS_EFI := -fpic -fshort-wchar

@@ -15,7 +21,7 @@ ifneq ($(LTO_ENABLE),y) PLATFORM_RELFLAGS += -ffunction-sections -fdata-sections endif

-PLATFORM_RELFLAGS += -fno-common -ffixed-r9 +PLATFORM_RELFLAGS += -fno-common $(FIXED_REG) PLATFORM_RELFLAGS += $(call cc-option, -msoft-float) \ $(call cc-option,-mgeneral-regs-only) \ $(call cc-option,-mshort-load-bytes,$(call cc-option,-malignment-traps,)) diff --git a/arch/arm/cpu/armv8/config.mk b/arch/arm/cpu/armv8/config.mk index ca06ed3d4f92..4d74b2a533e0 100644 --- a/arch/arm/cpu/armv8/config.mk +++ b/arch/arm/cpu/armv8/config.mk @@ -2,7 +2,6 @@ # # (C) Copyright 2002 # Gary Jennejohn, DENX Software Engineering, garyj@denx.de -PLATFORM_RELFLAGS += -fno-common -ffixed-x18 PLATFORM_RELFLAGS += $(call cc-option,-mbranch-protection=none)

PF_NO_UNALIGNED := $(call cc-option, -mstrict-align)

Hi

On Thu, 6 Apr 2023 at 09:16, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 4/6/23 01:48, Tom Rini wrote:

...

When building with clang we see this warning: field guid within 'struct efi_hii_keyboard_layout' is less aligned than 'efi_guid_t' and is usually due to 'struct efi_hii_keyboard_layout' being packed, which can lead to unaligned accesses [-Wunaligned-access]

Which is correct and true as EFI payloads are by specification allowed to do unaligned access. And so to silence the warning here and only here, we make use of #pragma to push/pop turning off the warning.

Signed-off-by: Tom Rini trini@konsulko.com

Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Ilias Apalodimas ilias.apalodimas@linaro.org

---

include/efi_api.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/include/efi_api.h b/include/efi_api.h index dc6e5ce236c9..f3bcbf593bea 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -1168,9 +1168,21 @@ struct efi_key_descriptor { u16 affected_attribute; } __packed;

+/* The warniing:

• • field guid within 'struct efi_hii_keyboard_layout' is less aligned than 'efi_guid_t' and is usually due to 'struct efi_hii_keyboard_layout' being packed, which can lead to unaligned accesses [-Wunaligned-access]
• • is intentional due to EFI requiring unaligned access to be supported.
• */ struct efi_hii_keyboard_layout { u16 layout_length;

+#ifdef CONFIG_CC_IS_CLANG +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wunaligned-access" +#endif efi_guid_t guid; +#ifdef CONFIG_CC_IS_CLANG +#pragma clang diagnostic pop +#endif

I don't think that the clang warning should be suppressed. We should replace efi_guid_t by u8[16] instead. This will force us to take the missing alignment into account when accessing the component in future.

•   efi_guid_t guid;
  

•   u8 guid[16];
  

I think what Heinrich suggests is fine, the EFI spec itself says that efi_guid must be 8-byte aligned unless otherwise specified. But we have another issue here. We aren't supposed to include structs that contain flexible length arrays into another array or struct. Quoting the C spec [0] Such a structure (and any union containing, possibly recursively, a member that is such a structure) shall not be a member of a structure or an element of an array. IOW efi_hii_keyboard_layout should not include struct efi_key_descriptor descriptors[]; since we include it at efi_hii_keyboard_package

Regards /Ilias

[0] https://www.dii.uchile.cl/~daespino/files/Iso_C_1999_definition.pdf chapter 6.7.2.1

Best regards

Heinrich

...

• u32 layout_descriptor_string_offset;
  u8 descriptor_count;
  struct efi_key_descriptor descriptors[];
  

On Tue, Nov 21, 2023 at 02:26:14PM +0100, Heinrich Schuchardt wrote:

On 4/6/23 01:48, Tom Rini wrote:

...

At this point, clang can be used on both 32bit and 64bit targets without issue.

Signed-off-by: Tom Rini trini@konsulko.com Reviewed-by: Simon Glass sjg@chromium.org

---

doc/build/clang.rst | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/doc/build/clang.rst b/doc/build/clang.rst index 1d35616eb5ef..222487032ce0 100644 --- a/doc/build/clang.rst +++ b/doc/build/clang.rst @@ -11,14 +11,6 @@ The ARM backend can be instructed not to use the r9 and x18 registers using supported inline assembly is needed to get and set the r9 or x18 value. This leads to larger code then strictly necessary, but at least works.

-**NOTE:** target compilation only work for _some_ ARM boards at the moment. -Also AArch64 is not supported currently due to a lack of private libgcc -support. Boards which reassign gd in c will also fail to compile, but there is -in no strict reason to do so in the ARM world, since crt0.S takes care of this. -These assignments can be avoided by changing the init calls but this is not in -mainline yet.

• Debian based

@@ -28,14 +20,14 @@ Required packages can be installed via apt, e.g.

  sudo apt-get install clang

-Note that we still use binutils for some tools so we must continue to set -CROSS_COMPILE. To compile U-Boot with Clang on Linux without IAS use e.g. +Note that we make use of the CROSS_COMPILE variable to determine what to tell +clang to use as the build target. +To compile U-Boot with Clang on Linux without IAS use e.g.

I had a hard time understanding this text block.

Note that the logic in question (from the top-level Makefile) is: ifeq ($(cc-name),clang) ifneq ($(CROSS_COMPILE),) CLANG_TARGET := --target=$(notdir $(CROSS_COMPILE:%-=%)) ...

So yes, "make CROSS_COMPILE=arm-linux-gnueabi- CC=clang" should in turn invoke "clang --target=arm-linux-gnueabi".

Do you mean:

"We use of the CROSS_COMPILE variable to derive the build target which is passed as -target parameter to clang.

The CROSS_COMPILE variable further determines the paths to other build tools. As assembler we use the binary pointed to by '$(CROSS_COMPILE)as' instead of the LLVM integrated assembler (IAS).

Here is an example demonstrating building U-Boot for the Raspberry Pi 2 using clang:"

That is clearer, yes, thanks. And then I see my example needs fixing again too. I'll v2 this patch at some point soon, thanks.