gcov kernel profiling works by emulating parts of GCC's libgcov in the
kernel, including a definition of struct gcov_info used by profiling
code to handle coverage data. The original definition of this data type
is not available outside of GCC's source tree, and when it changes with
new GCC versions the result may be hard-to-debug kernel failures [1].
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. This check should help
reduce the number of run-time failures when using gcov kernel support
with new GCC versions that include updates to struct gcov_info.
Tested with various GCC versions between 4.9 and 10, and also Clang 11.
[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1891288
Signed-off-by: Peter Oberparleiter <[email protected]>
---
kernel/gcov/Makefile | 9 +++++++++
kernel/gcov/gcc_4_7.c | 3 +++
kernel/gcov/geninfosize.sh | 19 +++++++++++++++++++
3 files changed, 31 insertions(+)
create mode 100755 kernel/gcov/geninfosize.sh
diff --git a/kernel/gcov/Makefile b/kernel/gcov/Makefile
index 16f8ecc7d882..1cbeb0de944e 100644
--- a/kernel/gcov/Makefile
+++ b/kernel/gcov/Makefile
@@ -1,6 +1,15 @@
# SPDX-License-Identifier: GPL-2.0
ccflags-y := -DSRCTREE='"$(srctree)"' -DOBJTREE='"$(objtree)"'
+ifeq ($(CONFIG_CC_IS_GCC),y)
+ GCOV_INFO_SIZE := $(shell $(srctree)/$(src)/geninfosize.sh)
+ ifneq ($(GCOV_INFO_SIZE),)
+ ccflags-y += -DGCC_GCOV_INFO_SIZE=$(GCOV_INFO_SIZE)
+ else
+ $(error Could not determine size of GCC's struct gcov_info)
+ endif
+endif
+
obj-y := base.o fs.o
obj-$(CONFIG_CC_IS_GCC) += gcc_base.o gcc_4_7.o
obj-$(CONFIG_CC_IS_CLANG) += clang.o
diff --git a/kernel/gcov/gcc_4_7.c b/kernel/gcov/gcc_4_7.c
index c53408a00d0b..fce003cfc790 100644
--- a/kernel/gcov/gcc_4_7.c
+++ b/kernel/gcov/gcc_4_7.c
@@ -94,6 +94,9 @@ struct gcov_info {
struct gcov_fn_info **functions;
};
+_Static_assert(sizeof(struct gcov_info) == GCC_GCOV_INFO_SIZE,
+ "struct gcov_info must be updated");
+
/**
* gcov_info_filename - return info filename
* @info: profiling data set
diff --git a/kernel/gcov/geninfosize.sh b/kernel/gcov/geninfosize.sh
new file mode 100755
index 000000000000..c05f1374fcb3
--- /dev/null
+++ b/kernel/gcov/geninfosize.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# Determine size of GCC's internal struct gcov_info. CC must be set correctly.
+#
+# Note: GCC adds an instance of its built-in version of struct gcov_info under
+# the label .LPBX0 to assembler output when compiling with --coverage
+#
+
+echo "void fn(void) {}" | $CC -S --coverage -x c - -o - | \
+while read a b c ; do
+ # .size .LPBX0, 112
+ if [ "$a" = ".size" -a "$b" = ".LPBX0," ] ; then
+ echo "$c"
+ break
+ fi
+done
+
+exit 0
--
2.25.1
On Thu, Mar 11, 2021 at 5:07 AM Peter Oberparleiter
<[email protected]> 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?
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).
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.
IOW, it feels like some ELF munging (possibly even with just scripting
with "objdump") should be able to add verification for a bit more than
just the size.
I guess the size is kind of critical, because of how GCOV_COUNTERS has
changed, but if any other layout issue changes, the size might not be
all that relevant.
For example, looking at the current "struct gcov_info" gcc uses, it's
very badly packed, with 32-bit fields literally interspersed with
64-bit fields. So I could easily imagine that somebody goes "heyt,
guys, we need to add another GCOV counter, but we don't need to change
the size of the gcov_info, because we can just out the "version" and
"stamp" integers next to each other and getting rid of the padding
makes up for the extra counter".
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..
Linus
Hi Peter,
I love your patch! Yet something to improve:
[auto build test ERROR on linux/master]
[also build test ERROR on linus/master hnaz-linux-mm/master v5.12-rc2 next-20210311]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Peter-Oberparleiter/gcov-fail-build-on-gcov_info-size-mismatch/20210311-211208
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git a74e6a014c9d4d4161061f770c9b4f98372ac778
config: nds32-randconfig-s032-20210311 (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.3-262-g5e674421-dirty
# https://github.com/0day-ci/linux/commit/01c5ab69a25e3cdfeff9cccf0f1fae26b583cc39
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Peter-Oberparleiter/gcov-fail-build-on-gcov_info-size-mismatch/20210311-211208
git checkout 01c5ab69a25e3cdfeff9cccf0f1fae26b583cc39
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=nds32
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Thu, Mar 11, 2021 at 11:34 AM kernel test robot <[email protected]> wrote:
>
> >> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
Wth? I'm not seeing how this can fail on nds32 - doesn't look like a
bashism, everything is properly quoted, etc etc. Plus it's a
cross-compile anyway, so the shell in question should be the same as
on all the other architectures.
Presumably the nds32 assembly contains something odd and unexpected,
but with the quoting, I can't see how even that could matter.
Yeah, the test itself could probably be simplified to testing both
conditions at the same time:
[ "$a $b" = ".size .LPBX0," ]
but that's a separate issue.
Funky. What am I missing? Presumably something really stupid.
Linus
On 3/12/21 4:02 AM, Linus Torvalds wrote:
> On Thu, Mar 11, 2021 at 11:34 AM kernel test robot <[email protected]> wrote:
>>>> kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
> Wth? I'm not seeing how this can fail on nds32 - doesn't look like a
> bashism, everything is properly quoted, etc etc. Plus it's a
> cross-compile anyway, so the shell in question should be the same as
> on all the other architectures.
>
> Presumably the nds32 assembly contains something odd and unexpected,
> but with the quoting, I can't see how even that could matter.
>
> Yeah, the test itself could probably be simplified to testing both
> conditions at the same time:
>
> [ "$a $b" = ".size .LPBX0," ]
>
> but that's a separate issue.
>
> Funky. What am I missing? Presumably something really stupid.
>
> Linus
Hi Linus,
The issue is from a=!, and [ "$a $b" = ".size .LPBX0," ] can avoid the
error.
+ [ ! = .size -a ABI = .LPBX0, ]
./kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
Best Regards,
Rong Chen
On 11.03.2021 19:38, Linus Torvalds wrote:
> On Thu, Mar 11, 2021 at 5:07 AM Peter Oberparleiter
> <[email protected]> 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
On Thu, Mar 11, 2021 at 7:50 PM Rong Chen <[email protected]> wrote:
>
>
> The issue is from a=!, and [ "$a $b" = ".size .LPBX0," ] can avoid the
> error.
>
> + [ ! = .size -a ABI = .LPBX0, ]
> ./kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
But that's not what the patch did.
The patch used quotes around $a, so "$a" should still be fine.
See:
[torvalds@ryzen ~]$ a="!" [ "$a" = ".size" ]
is fine, but
[torvalds@ryzen ~]$ a="!" [ $a = ".size" ]
-bash: [: =: unary operator expected
and the patch I saw, and that the test robot replied to, had that
correct quoting, afaik.
So I still don't see what the test robot is complaining about. Was
there an earlier version of the patch without the quotes that I didn't
see?
Or is the shell on the test robot doing something really really odd,
and it's somehow nds32-specific?
Linus
On 3/13/21 1:52 AM, Linus Torvalds wrote:
> On Thu, Mar 11, 2021 at 7:50 PM Rong Chen <[email protected]> wrote:
>>
>> The issue is from a=!, and [ "$a $b" = ".size .LPBX0," ] can avoid the
>> error.
>>
>> + [ ! = .size -a ABI = .LPBX0, ]
>> ./kernel/gcov/geninfosize.sh: 13: [: =: unexpected operator
> But that's not what the patch did.
>
> The patch used quotes around $a, so "$a" should still be fine.
>
> See:
>
> [torvalds@ryzen ~]$ a="!" [ "$a" = ".size" ]
>
> is fine, but
>
> [torvalds@ryzen ~]$ a="!" [ $a = ".size" ]
> -bash: [: =: unary operator expected
>
> and the patch I saw, and that the test robot replied to, had that
> correct quoting, afaik.
>
> So I still don't see what the test robot is complaining about. Was
> there an earlier version of the patch without the quotes that I didn't
> see?
>
> Or is the shell on the test robot doing something really really odd,
> and it's somehow nds32-specific?
>
> Linus
Hi Linus,
It can be reproduced with '-a' option in dash:
$ a="!"
$ [ "$a" = ".size" ]
$ [ "$a" = ".size" -a "$b" = ".LPBX0," ]
sh: 2: [: =: unexpected operator
and there is a advice for the option at
https://wiki.ubuntu.com/DashAsBinSh, I'm not sure it's the best practice
or not.
While dash supports most uses of the -a and -o options, they have
very confusing semantics even in bash and are best avoided. Commands
like the following:
[ \( "$foo" = "$bar" -a -f /bin/baz \) -o ! -x /bin/quux ]
should be replaced with:
(([ "$foo" = "$bar" ] && [ -f /bin/baz ]) || [ ! -x /bin/quux ])
Best Regards,
Rong Chen
On Sun, Mar 14, 2021 at 7:32 PM Rong Chen <[email protected]> wrote:
>
> It can be reproduced with '-a' option in dash:
Oh, ok. That kind of explains it.
'dash' is trash. Please somebody make a bug report.
> $ a="!"
> $ [ "$a" = ".size" ]
> $ [ "$a" = ".size" -a "$b" = ".LPBX0," ]
> sh: 2: [: =: unexpected operator
This is 100% a dash bug. There is no question what-so-ever about it.
This is not some kind of "POSIX is ambiguous", or "the handling of
'-a' is complicated".
It's simply just that dash is buggy.
> While dash supports most uses of the -a and -o options, they have
> very confusing semantics even in bash and are best avoided.
No, they have perfectly sane semantics in bash, and in POSIX.
See
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html
and there is absolutely zero question that bash does this correctly,
and dash does not.
But yes, it seems to be easy to work around, but still - could some
Ubuntu person please open a bug report on dash?
Linus
Linus Torvalds wrote:
> On Sun, Mar 14, 2021 at 7:32 PM Rong Chen <[email protected]> wrote:
> >
> > It can be reproduced with '-a' option in dash:
>
> Oh, ok. That kind of explains it.
>
> 'dash' is trash. Please somebody make a bug report.
>
> > $ a="!"
> > $ [ "$a" = ".size" ]
> > $ [ "$a" = ".size" -a "$b" = ".LPBX0," ]
> > sh: 2: [: =: unexpected operator
>
> This is 100% a dash bug. There is no question what-so-ever about it.
> This is not some kind of "POSIX is ambiguous", or "the handling of
> '-a' is complicated".
>
> It's simply just that dash is buggy.
>
> > While dash supports most uses of the -a and -o options, they have
> > very confusing semantics even in bash and are best avoided.
>
> No, they have perfectly sane semantics in bash, and in POSIX.
>
> See
>
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html
>
> and there is absolutely zero question that bash does this correctly,
> and dash does not.
>
> But yes, it seems to be easy to work around, but still - could some
> Ubuntu person please open a bug report on dash?
fwiw, https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=850202
Linus Torvalds <[email protected]> wrote:
>
> See:
>
> [torvalds@ryzen ~]$ a="!" [ "$a" = ".size" ]
>
> is fine, but
>
> [torvalds@ryzen ~]$ a="!" [ $a = ".size" ]
> -bash: [: =: unary operator expected
This isn't doing what you think it's doing. The first assignment
to a is not in effect when the shell is expanding $a so what you're
actually running is
a="!" [ = .size ]
Which is why it bombs out.
To get the desired result you need a semicolon:
$ a="!"; [ $a = ".size" ]
$
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt