sys/targets: avoid building TestOS on OpenBSD

[blackgnezdo]

OpenBSD's missing syscall function yet TestOS requires it.

[blackgnezdo]

I did try to set BuildOS: Linux in the relevant places. This one took effect and I was no longer getting the missing ubsan libraries.

I was unable to similarly disable TestArch64Fuzz and TestArch64Fork. They were still coming with BuildOS: OpenBSD into this test. There's some magic I'm missing.

[blackgnezdo]

With this change we are down to a single failing test due to golang/go#65083

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.0%. Comparing base (47d9c0e) to head (f22c74d).

Additional details and impacted files

see 1 file with indirect coverage changes

[a-nogikh]

Okay, so we want to disable TestGenerate for TestOS targets on OpenBSD.

Since we try to keep all hacks in targets.go, what do you think about this?

Change it

syzkaller/sys/targets/targets.go

Line 696 in 454571b

target.BuildOS = goos

To os.Linux by default and goos if it's not OpenBSD.

[blackgnezdo]

Okay, so we want to disable TestGenerate for TestOS targets on OpenBSD.

Since we try to keep all hacks in targets.go, what do you think about this?

Change it

syzkaller/sys/targets/targets.go

Line 696 in 454571b

target.BuildOS = goos

To os.Linux by default and goos if it's not OpenBSD.

I did this. Looks like test os is not a problem any more. I see a different problem now (probably added since the last time I tried).

[blackgnezdo]

The new problem is:

--- FAIL: TestCommonExt (0.26s)
    common_ext_test.go:26: failed to build program:
...
        c++: error: unsupported option '-fsanitize=address' for target 'amd64-unknown-openbsd7.5'
        
        compiler invocation: c++ [-o /tmp/syz-executor332668517 -DGOOS_test=1 -DGOARCH_64=1 -DHOSTGOOS_openbsd=1 ../../executor/executor.cc -m64 -lutil -O2 -pthread -Wall -Werror -Wparentheses -Wunused-const-variable -Wframe-larger-than=16384 -Wno-stringop-overflow -Wno-array-bounds -Wno-format-overflow -Wno-unused-but-set-variable -Wno-unused-command-line-argument -fsanitize=address -no-pie -fno-exceptions]
FAIL
coverage: 28.1% of statements
FAIL	github.com/google/syzkaller/pkg/ipc	22.629s

Which is not surprising considering OpenBSD doesn't have ASAN, but this seems like a new failure.

[blackgnezdo]

The new problem is:

--- FAIL: TestCommonExt (0.26s)
    common_ext_test.go:26: failed to build program:
...
        c++: error: unsupported option '-fsanitize=address' for target 'amd64-unknown-openbsd7.5'
        
        compiler invocation: c++ [-o /tmp/syz-executor332668517 -DGOOS_test=1 -DGOARCH_64=1 -DHOSTGOOS_openbsd=1 ../../executor/executor.cc -m64 -lutil -O2 -pthread -Wall -Werror -Wparentheses -Wunused-const-variable -Wframe-larger-than=16384 -Wno-stringop-overflow -Wno-array-bounds -Wno-format-overflow -Wno-unused-but-set-variable -Wno-unused-command-line-argument -fsanitize=address -no-pie -fno-exceptions]
FAIL
coverage: 28.1% of statements
FAIL	github.com/google/syzkaller/pkg/ipc	22.629s

Which is not surprising considering OpenBSD doesn't have ASAN, but this seems like a new failure.

This is a regression due to the change in my commit. The change makes the flag detection/filtering logic in target.lazyInit no longer kick in because runtime.GOOS != target.BuildOS at https://github.com/blackgnezdo/syzkaller/blob/81e2d4afdc682abb021aae9b5ca3306066776578/sys/targets/targets.go#L864

[a-nogikh]

Is this the only failing test?

I think we should be checking for runtime.GOOS != sysTarget.BuildOS in TestCommonExt.

[blackgnezdo]

Is this the only failing test?

Sadly, no, it hit at least two more tests with similar issues:

--- FAIL: TestFuzz (0.10s)
    fuzzer_test.go:327: failed to build program:
...
	error: unknown warning option '-Wno-stringop-overflow'; did you mean '-Wno-shift-overflow'? [-Werror,-Wunknown-warning\
-option]
	error: unknown warning option '-Wno-format-overflow'; did you mean '-Wno-shift-overflow'? [-Werror,-Wunknown-warning-o\
ption]

	compiler invocation: c++ [-o /tmp/syz-executor989183268 -DGOOS_test=1 -DGOARCH_64_fuzz=1 -DHOSTGOOS_openbsd=1 ../../ex\
ecutor/executor.cc -m64 -lutil -O2 -pthread -Wall -Werror -Wparentheses -Wunused-const-variable -Wframe-larger-than=16384 -Wno\
-stringop-overflow -Wno-array-bounds -Wno-format-overflow -Wno-unused-but-set-variable -Wno-unused-command-line-argument -no-p\
ie -fsanitize-coverage=trace-pc -g]
FAIL
coverage: 27.6% of statements
FAIL    github.com/google/syzkaller/pkg/fuzzer  0.255s
...
--- FAIL: TestZlib (0.21s)
    ipc_test.go:30: failed to build program:
...
        c++: error: unsupported option '-fsanitize=address' for target 'amd64-unknown-openbsd7.5'

        compiler invocation: c++ [-o /tmp/syz-executor1489303267 -DGOOS_test=1 -DGOARCH_64=1 -DHOSTGOOS_openbsd=1 ../../execut\
or/executor.cc -m64 -lutil -O2 -pthread -Wall -Werror -Wparentheses -Wunused-const-variable -Wframe-larger-than=16384 -Wno-str\
ingop-overflow -Wno-array-bounds -Wno-format-overflow -Wno-unused-but-set-variable -Wno-unused-command-line-argument -fsanitiz\
e=address -no-pie -fno-exceptions]
FAIL
coverage: 51.9% of statements
FAIL    github.com/google/syzkaller/pkg/ipc     15.067s

I think we should be checking for runtime.GOOS != sysTarget.BuildOS in TestCommonExt.

I can do this here and in the two places above, but it feels like the commit that set us on this path is directionaly dubious.

[dvyukov]

IIRC target.BrokenCompiler was supposed to mark targets for which can't build now.
I see pkg/scource tests check it, and they are not failing on OpenBSD, right?
Is target.BrokenCompiler set in these failing tests? If yes, I think we should just skip tests if target.BrokenCompiler is set.
Additionally we could try to prevent such cases in future, or at least make easier to debug. I can think of either checking target.BrokenCompiler in csource.BuildFile and returning an error that hints that if it's a test then it should skip this target; or adding csource.TestBuildFile that will accept testing.T and do t.Skip itself.

[blackgnezdo]

IIRC target.BrokenCompiler was supposed to mark targets for which can't build now. I see pkg/scource tests check it, and they are not failing on OpenBSD, right?

It is broken in google:master and is what set us on this path. Yet, it is broken by syscall function not existing anymore:

                <stdin>:357:3: error: call to undeclared function 'syscall'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
                                syscall(SYS_mutate_buffer, /*p=*/0x20000000ul);

Hence this commit 81e2d4a, which makes csource pass.

Is target.BrokenCompiler set in these failing tests?

I think it is not set, at least adding this commit doesn't get the test skipped:

modified   executor/common_ext_test.go
@@ -14,6 +14,7 @@ import (
 	"github.com/google/syzkaller/pkg/ipc/ipcconfig"
 	"github.com/google/syzkaller/pkg/osutil"
 	"github.com/google/syzkaller/prog"
+	"github.com/google/syzkaller/sys/targets"
 )
 
 func TestCommonExt(t *testing.T) {
@@ -21,6 +22,10 @@ func TestCommonExt(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	sysTarget := targets.Get(target.OS, target.Arch)
+	if sysTarget.BrokenCompiler != "" {
+		t.Skipf("skipping, broken cross-compiler: %v", sysTarget.BrokenCompiler)
+	}
 	bin, err := csource.BuildFile(target, "executor.cc", "-DSYZ_TEST_COMMON_EXT_EXAMPLE=1")
 	if err != nil {
 		t.Fatal(err)

If yes, I think we should just skip tests if target.BrokenCompiler is set. Additionally we could try to prevent such cases in future, or at least make easier to debug. I can think of either checking target.BrokenCompiler in csource.BuildFile and returning an error that hints that if it's a test then it should skip this target; or adding csource.TestBuildFile that will accept testing.T and do t.Skip itself.

We can revisit this once the BrokenCompiler is set, right?

[dvyukov]

Ah, I see.
When we test for broken compiler, we compile something simple like "int main() {}", so it does not fail due to missing syscall.

I think it's more explicit to do the following for the TestOS:

if runtime.GOOS == OpenBSD {
	target.BrokenCompiler = "can't build TestOS on OpenBSD due to missing syscall function."
}

Then we can add check for target.BrokenCompiler to all tests that compile executor.

[blackgnezdo]

Ah, I see. When we test for broken compiler, we compile something simple like "int main() {}", so it does not fail due to missing syscall.

I think it's more explicit to do the following for the TestOS:

if runtime.GOOS == OpenBSD {
	target.BrokenCompiler = "can't build TestOS on OpenBSD due to missing syscall function."
}

Then we can add check for target.BrokenCompiler to all tests that compile executor.

I believe I found the relevant tests and added the requisite skipping. The tests now pass on OpenBSD. PTAL?