KCOV: Use arch-dependend data type for the PCs

[alexander.lochmann_tu-dortmund.de]

Summary

On i386, the pc is only 32 bit of width. The buffer accessed via /dev/kcov and the typecasts
of the return value of __builtin_return_address strictly use uint64_t.
In the former case, every second pc which kcovtrace prints is zero.
The latter case leads to a compiler warning that aborts the build process.
In both cases unsigned long is now used. Since its size varies on i386 and x86_64, it solves
both issues. *EDIT*: Using uintptr_t.

The Linux kernel uses unsigned long as well: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kcov.c#n94

[mhorne]

Hi, can you update your diff with full context? Run either git diff -U999999 or svn diff -x -U999999 depending on which mirror you are using.

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

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().

[alexander.lochmann_tu-dortmund.de]

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

[alexander.lochmann_tu-dortmund.de]

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.

[andrew]

The size was set so a 32 bit tracing tool can trace a 64 bit kernel without loosing information.

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

[andrew]

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.

[alexander.lochmann_tu-dortmund.de]

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.

I get your point. Using uintptr_t within the coverage subsystem and uint64_t internally should work fine as you can see in my patch.
uintptr_t within the coverage subsystem is necessary. Otherwise the GCC will reject peaces like this: (uint64_t)__builtin_return_address(0).

With your updated version kcovtrace does not compile anymore, because a 32bit FreeBSD does not offer __atomic_store_8:

/tmp/kcovtrace-5349c4.o: In function `main':
kcovtrace.c:(.text+0x1f1): undefined reference to `__atomic_store_8'
kcovtrace.c:(.text+0x278): undefined reference to `__atomic_load_8'

Is the atomic operation really necessary?