Skip to content

Conversation

@sephalon
Copy link
Contributor

@sephalon sephalon commented Dec 19, 2024

See this line in the Yocto recipe on why we need Makefile: rename CFLAGS to CXXFLAGS and add LDFLAGS. sys/targets: allow users to override hardcoded cross-compilers and sys/targets: remove hardcoded CFlags for ARM are based upon a Yocto recipe patch. Note that there might be reasons why we need to force emitting ARMv6 code, but I could not find any hint in the history; so please let me know if we need to approach this issue in a different way.

@sephalon sephalon force-pushed the yocto_build_compatibility branch from 48d2854 to 9c029f8 Compare December 19, 2024 17:18
@a-nogikh a-nogikh self-requested a review December 20, 2024 21:22
@a-nogikh
Copy link
Collaborator

Regarding the sys/targets: remove hardcoded CFlags for ARM commit. We intentionally do not worry about whether the compiler flags in the Target are always supported because we check for it later. See this part of targets.go:

newCFlags := []string{}
for _, flag := range target.CFlags {
for {
if res := flags[flag]; res == nil || *res {
// The flag is either verified to be supported or must be supported.
newCFlags = append(newCFlags, flag)
} else if fallback := fallbackCFlags[flag]; fallback != "" {
// The flag is not supported, but probably we can replace it by another one.
flag = fallback
continue
}
break
}
}

It wonder why this logic did not work properly in your case. Does it only fire when a specific subset of flags is specified?

Copy link
Collaborator

@a-nogikh a-nogikh left a comment

Choose a reason for hiding this comment

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

Looks good to me

@sephalon
Copy link
Contributor Author

sephalon commented Jan 8, 2025

Regarding the sys/targets: remove hardcoded CFlags for ARM commit. We intentionally do not worry about whether the compiler flags in the Target are always supported because we check for it later. See this part of targets.go:

newCFlags := []string{}
for _, flag := range target.CFlags {
for {
if res := flags[flag]; res == nil || *res {
// The flag is either verified to be supported or must be supported.
newCFlags = append(newCFlags, flag)
} else if fallback := fallbackCFlags[flag]; fallback != "" {
// The flag is not supported, but probably we can replace it by another one.
flag = fallback
continue
}
break
}
}

It wonder why this logic did not work properly in your case. Does it only fire when a specific subset of flags is specified?

@a-nogikh If I understand this logic correctly, only flags listed in optionalCFlags are added to uncommonCFlags and undergo runtime checking via checkFlagSupported(). Therefore, if I add -march=armv6 to optionalCFlags, it is removed by checkFlagSupported() and compilation runs through fine. However, this mechanism obviously will not be able to remove -D__LINUX_ARM_ARCH__=6 as well, which might have unintended side effects. Therefore, is it possible to just remove both -D__LINUX_ARM_ARCH__=6 and -march=armv6 from the hardcoded CFlags?

@sephalon sephalon force-pushed the yocto_build_compatibility branch from 9c029f8 to 7c09bf9 Compare January 8, 2025 15:47
@a-nogikh
Copy link
Collaborator

a-nogikh commented Jan 9, 2025

However, this mechanism obviously will not be able to remove -D__LINUX_ARM_ARCH__=6 as well, which might have unintended side effects. Therefore, is it possible to just remove both -D__LINUX_ARM_ARCH__=6 and -march=armv6 from the hardcoded CFlags?

Actually, I wonder if -D__LINUX_ARM_ARCH__=6 could have had any effect at all. (@dvyukov please correct me if I'm wrong) CFlags only affect the compilation of syz-executor and C reproducers, whereas D__LINUX_ARM_ARCH__ is only used in the non-uapi part of the Linux kernel.

It might affect syz_extract though (since we do compile the kernel using CFlags there), but that's unlikely to affect Yocto (?) unless you want to write custom syzkaller descriptions for your kernel tree.


@dvyukov @ramosian-glider Do you know if we have any specific reasons to keep -D__LINUX_ARM_ARCH__=6 and -march=armv6 for building arm32 binaries for arm64 machines?

@ramosian-glider
Copy link
Member

I think it should be safe to remove these -D and -march flags already. They date back to 2017, and today armv7 should be more or less the standard.

Following standard conventions simplifies the Yocto recipe.
Currently, cross compiler names are hardcoded for each OS/arch combo.
However, toolchain tuples differ, especially when using vendor provided
toolchains or building with Yocto. Allow users to specify the cross
compiler for an OS/arch combo using SYZ_CC_<os>_<arch> environment
variables.
Depending on the cross compiler build configuration, it might not be
able to emit ARMv6 Thumb-1 instructions leading to "sorry,
unimplemented: Thumb-1 hard-float VFP ABI" error.
@a-nogikh a-nogikh added this pull request to the merge queue Jan 14, 2025
Merged via the queue into google:master with commit 0dce240 Jan 14, 2025
17 checks passed
@sephalon sephalon deleted the yocto_build_compatibility branch January 15, 2025 08:39
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.

5 participants