search

google / syzkaller

syzkaller is an unsupervised coverage-guided kernel fuzzer
Apache License 2.0
5.31k stars 1.21k forks source link

sys/syz-extract: combine .const files #1983

Closed dvyukov closed 4 years ago

dvyukov commented 4 years ago

Currently we have .const file per arch. This is somewhat inconvenient for PR reviews: 8 files with mostly auto-generated content. Originally the motivation was that it may be hard to generate const files for some arches/descriptions. Say, we get some .const files for an arch contributed by somebody who can generate these const files, and then we keep them intact until that person sends an update. We update .const files for other arches, but keep that other arch as is. This capability is not used for any of the descriptions. And we would not like to degrade to that scenario now. We have some "special" descriptions, but that is per description, not per arch.

The proposal: combine all per-arch .const files into a single file with some simple syntax to have per-arch values, if they are different. E.g.:

SAME_FOR_EACH_ARCH = 42
PER_ARCH = 386:4 amd64:8 arm:4 arm64:8 ppc64le:8 mips64le:8 riscv64:8 s390x:8

Or, to make it clear what arches has the same value:

PER_ARCH = amd64,arm64,ppc64le,mips64le,riscv64,s390x:8 386,arm:4

Not sure how far we want to go, but we could the default value (chosen to cover majority of arches):

PER_ARCH = 8 4:386,arm

"is not defined" may be represented as some special symbol, e.g.:

PER_ARCH = 8 ???:386,arm
ARM_SPECIFIC = ??? 42:arm

As a bonus we could highlight with an info comment lines that contain "???".

dvyukov commented 4 years ago

@xairy @ramosian-glider @melver @necipfazil you reviewed some PRs with descriptions now: will it be an improvement?

melver commented 4 years ago

Yes, it might help manual inspection and review. For constants that are the same for each arch, it certainly removes some overhead on the reviewer side.

However, if we assume that all .const files are auto-generated, and we trust syz-extract, not all that much is gained.

necipfazil commented 4 years ago

I have been relying on syz-extract for auto-generated consts and hasn't been explicitly reviewing them unless there are unset ones. For user-defined consts (define const val), I was using the descriptions for review.

Maybe there are some scenarios that require explicit review of those consts. I either didn't encounter them, or wasn't aware and overlooked. If that is not the case, though, I cannot see value in such change for the reviewer. On the other hand, this seems more tidy since it is more terse and will result in having much less number of files.

xairy commented 4 years ago

LGTM overall.

SAME_FOR_EACH_ARCH = 42
PER_ARCH = 386:4 amd64:8 arm:4 arm64:8 ppc64le:8 mips64le:8 riscv64:8 s390x:8

This one looks good and easy to parse.

dvyukov commented 4 years ago

However, if we assume that all .const files are auto-generated, and we trust syz-extract for auto-generated consts and hasn't been explicitly reviewing them

The point is that it's not convenient to not review them now. The last PR I looked was 2 .txt files + 16 .const files, it was hard to scroll to the parts I need to review ignoring all the ones that I don't need to review, and I wasn't sure that I found all files I need to look at. (yes, I know about file filters and collapsing, but it's still an additional step, and also I do want to see the "is not set" cases)