Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for building AArch64 with Clang #374

Merged
merged 6 commits into from
Jun 2, 2023

Conversation

arichardson
Copy link
Contributor

These changes allow me to build with
make CC="/usr/bin/clang -target aarch64-unknown-elf" LD=/usr/bin/ld.lld TOOLCHAIN_PREFIX=/usr/bin/ qemu-virt-arm64-test

@arichardson
Copy link
Contributor Author

Unlike #322 this does not add a LLVM= flag to change the default it just allows building if CC/LD are explicitly set to LLVM tools.

@arichardson arichardson changed the title Add support for building with Clang (AArch64 tested so far) Add support for building AArch64 with Clang Mar 10, 2023
@arichardson
Copy link
Contributor Author

ping?

@travisg
Copy link
Member

travisg commented Apr 8, 2023 via email

@arichardson
Copy link
Contributor Author

These look pretty good, hoping to get a look at it this weekend. Mostly need to make sure it works well with GCC and see about trying to make it more generic. There have been multiple attempts to compile with clang over the years but they're generally fairly limited.

Thanks! I did build with GCC as well, and ran the build all script so hopefully it all works. Once these changes are in I'll submit another PR with changes to allow building for RISC-V with clang.

make/macros.mk Outdated
@@ -58,3 +58,17 @@ define MAKECONFIGHEADER
echo $3 >> $1.tmp; \
$(call TESTANDREPLACEFILE,$1.tmp,$1)
endef

check_compiler_flag = $(shell $(CC) -c -xc /dev/null -o /dev/null $(1) 2>/dev/null && echo yes || echo no)
check_warning_supported = $(call check_compiler_flag,-Werror=unknown-warning-option $(1))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems problematic, since gcc doesn't seem to support -Werror=unknown-warning-option. It causes it always to choose that the switch is not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I thought I tested this with GCC but it does look like the docs don't mention it. Will try to update this to only add the flag for clang

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh thanks. Been trying to figure out a workaround, but seems that the correct solution is to either only test for clang or to have two paths, based on whether or not the -Werror=unknown-warning-option switch seems to work.

If it's not functional it seems that gcc has some strange behavior with regards to 'no-' switches. If you pass a -Wno- switch that it doesn't recognize it'll accept it unless it gets a compile failure, in which case it then tells you of the weird switch. This seems to be intentional, to allow for backwards compatibility with older versions of the compiler. documented at https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#:~:text=When%20an%20unrecognized%20warning%20option%20is%20requested

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started to go down the route of just fixing the code that the -Wno- switches were working around, but they're kinda annoying. The big warning failure is in some apis that have the NONNULL attribute set for particular arguments. Inside the function if you test for null anyway (which is a generally good idea, IMO) you get a warning. Kinda annoying, the main reason i had the NONNULL set was to catch obvious null cases at the caller, but not to cause it to elide all checks in the function itself. Really probably means the nonnull attribute is largely a waste of time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay here - I've pushed a new version that works for both GCC and Clang in my testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the attribute((nonnull)) that warning probably does make sense since the way that attribute works is that the compiler will elide all NULL checks. Unlike this attribute the Clang _Nonnull/_Nullable annotations only affect compiler warnings and not code generation.

This GCC extension is not supported by clang and in this case is not
necessary. Move the function to the top level to avoid the syntax error.
Clang sets the defines for GCC 4.2.1, so we have to check __clang__ for
these macros in addition to the GCC version.
Clang does not accept this .if condition since phys_offset is a register
alias and not an absolute expression. We can keep these two instructions
here if the argument is zero since the result will be the same.
Additionally, this macro is only called once and always passes a non-zero
argument. If more calls are added in the future and avoiding these two
instructions just before a loop is really important, we could use
`.ifnc \phys_offset,0` instead, but that looks rather obscure to me.
The LLD linker does not allow joined short arguments, so split -dT <script>
into -d -T <script>.
Currently, clang does not support the -Wno-nonnull-compare and
-Wmaybe-uninitialized warning flags so this adds lots of unknown warning
flag output for each compile job when not using GCC.
This commit adds a makefile macro to check for supported warning flags
and only adds them if the compiler actually supports them.
Clang incorrectly diagnoses msr operations as need a 64-bit operand even
if the underlying register is actually 32 bits. Silence this warning.
There are quite a few occurrences of this warning so I opted to add the
-Wno-flag instead of wrapping all callsites in
`#pragma clang diagnostic ignored -Wasm-operand-widths`.
@travisg
Copy link
Member

travisg commented Jun 2, 2023

LGTM, sorry for the delay!

@travisg travisg merged commit 923541d into littlekernel:master Jun 2, 2023
@arichardson arichardson deleted the aarch64-clang branch June 2, 2023 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants