In D20586#445268, @dim wrote:

In D20586#445254, @alexander.lochmann_tu-dortmund.de wrote:

So for kern.mk, is this option needed at all? Can we simply remove it?

This is a very old addition, from rS104455, where it claimed "This reduces the size of GENERIC's text space by 73999 bytes (about 2%). The bloat is from approximately 3437 strings longer than 31 characters being padded to a 32-byte boundary".

I have no objection against removing it, but some people are very fond of obtaining a kernel image that is as small as possible. So if we can keep the option in there for base gcc 4.2.1, that would be the least controversial.

In D20587#445258, @andrew wrote:

In D20587#445257, @andrew wrote:

I've created https://github.com/google/syzkaller/pull/1229 to fix the bug in syzkaller.

This has now been merged. Can you try with an updated kcovtrace.

In D20586#444960, @dim wrote:

In fact, the -mno-align-long-strings option is a custom addition to our base gcc (from rS169705, quite a long time ago). Upstream gcc never supported this option.

In any case, this change is incomplete in two ways:

• It should be dependent on the gcc version. For gcc 4.2.1, keep the old spelling, assuming that it will use base gcc. For newer gcc versions, check where the new spelling was introduced, and make it dependent on that version.
• The option is also used in stand/i386/boot2/Makefile, supposedly to keep the binary size small. This is a very sensitive area, as boot2 can blow up with different versions of compilers for no discernible reason. Any opion change there needs to be carefully tested for size regressions. And obviously, it also needs to be gcc version dependent.

There is actually a version check in stand/i386/boot2/Makefile: .if ${COMPILER_TYPE} == "gcc" && ${COMPILER_VERSION} <= 40201
So for kern.mk, is this option needed at all? Can we simply remove it?

In D20587#445231, @alexander.lochmann_tu-dortmund.de wrote:

For trace_cmp(), are you sure that this has to be changed as well?
The Linux Kernel uses uint64_t here as well: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kcov.c#n116

The different versions of sanitizer_cov_trace_cmp() handle the different data type sizes. Each version of sanitizer_cov_trace_cmp() redirects to trace_cmp().
Its arguments must therefore offer space for the largest variant of sanitizer_cov_trace_cmp() which ist sanitizer_cov_trace_cmp8().
In the other cases, there is no problem.

In D20587#444955, @mhorne wrote:

As for the changes: although unsigned long is the correct way of expressing native word-size, in this case uintptr_t is a better choice for a type as it is guaranteed to be large enough to hold a pointer value.

Mhmkay. I'll switch to uintptr_t.

I believe this is what was used in early versions of the patch, but it was changed to uint64_t for compatibility with Syzkaller. @andrew can better speak to the reasons for doing so and whether or not this change would cause any issues there.

That sounds intereseting, because kcovtrace provided by syzkaller itself uses unsigned long: https://github.com/google/syzkaller/blob/master/tools/kcovtrace/kcovtrace.c#L37

Also I've only given it a brief look, but you are definitely missing one or two cases in kern_kcov.c, namely the KCOV_ELEMENT_SIZE macro and arguments to trace_cmp().

I'll fix that, too.
For trace_cmp(), are you sure that this has to be changed as well?
The Linux Kernel uses uint64_t here as well: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kcov.c#n116