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

Yocto build compatibility #5627

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

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.

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.
@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

@@ -900,7 +909,7 @@ func (target *Target) lazyInit() {
// Also fail if SOURCEDIR_GOOS is set b/c in that case user probably assumes it will work.
if (target.OS != runtime.GOOS || !runningOnCI) && getSourceDir(target) == "" {
for _, comp := range []string{target.CCompiler, target.CxxCompiler} {
if _, err := exec.LookPath(comp); err != nil {
if _, err := exec.LookPath(strings.Fields(comp)[0]); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is strings.Fields necessary? Can the comp contain whitespaces?

// https://github.com/google/syzkaller/commit/06db3cec94c54e1cf720cdd5db72761514569d56
target.Triple = ""
if OS == Linux {
if cc := os.Getenv("SYZ_CC_" + OS + "_" + arch); cc != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we can also move it out of if OS == Linux -- OS is part of the env variable name anyway.

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.

3 participants