Message-ID: <1c7a49e7-0e27-561b-a2f9-d42a83dc4c29@linux.ibm.com>
Date:   Fri, 12 Mar 2021 18:46:06 +0100
From:   Peter Oberparleiter <oberpar@...ux.ibm.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] gcov: fail build on gcov_info size mismatch

On 11.03.2021 19:38, Linus Torvalds wrote:
> On Thu, Mar 11, 2021 at 5:07 AM Peter Oberparleiter
> <oberpar@...ux.ibm.com> wrote:
>>
>> This patch adds a compile-time check to ensure that the kernel's version
>> of struct gcov_info has the same length as the one used by GCC as
>> determined by looking at GCC's assembler output.
> 
> So I don't think this is a bad idea, but if you end up test-compiling
> something, could we not verify things a bit more?

I thought about this as well, based on the idea to not only look at the
size, but potentially also at the assembler-level definition of GCC's
gcov_info version, and to compare that to how the kernel's version looks
like. But then I thought that this still doesn't catch the cases where
the semantics of a struct member changed.

> If you actually build the object file, you should be able to then
> check much more. You'll find the pointer to the struct gcov_info in
> "__gcov_.fn", which is admittedly hard to then link against a test
> program (because of that dot in the name that means that you can't
> even use "attribute((alias..))" to generate some other name for it).

This is an idea I hadn't considered - a user space test program that
accesses GCC's gcov_info data using the kernel's definition would be
able to fail a lot more gracefully than the kernel could.

Getting a pointer to the gcov_info struct in a program isn't actually
that difficult: just provide a custom function named __gcov_init and
don't link with libgcov. At program start constructor code will then
call this function with the gcov_info pointer as parameter. This is
exactly how the kernel collects all these gcov_info instances.

> But then you could test not only the size, but you could verify that
> the "filename" field matches, that the n_functions field should be 1
> etc.

Taking that idea a step further, a test program could exercise the exact
same code that the kernel uses to generate a .gcda file from in-memory
gcov_info data (or die/hang when there's a mismatch in the struct
definition) and write that out. The resulting .gcda file could then be
verified for correctness by passing it to GCC's gcov tool.

This shouldn't be too difficult to achieve since there is already a
kernel function that converts a gcov_info into .gcda file format
in-memory with no actual dependency on kernel infrastructure. Of course
this requires the gcov kernel code to be taught to live in user space
with a healthy dose of ifdef-ing.

Is there a convention on what is the lesser evil: adding multiple
ifdef-endifs all over the place or moving code around for no apparent
reason other than to gather all required parts in one place for a single
ifdef?

[...]

> I dunno. The gcov code has obviously never actually done anything like
> this before, so maybe I'm just taking the "we could verify
> _something_" and my reaction is that there could be even more
> verification if we really want to go down that rabbit hole..

I'll try to come up with a new patch that introduces a test like
described above. If that approach fails we can fall back to the original
approach (plus the fix for the kernel test robot findings).


Regards,
  Peter

-- 
Peter Oberparleiter
Linux on Z Development - IBM Germany

Powered by blists - more mailing lists