Fix GCC 9 Code coverage by adding its JSON format #11538

helaan · 2020-06-02T18:09:43Z

GCC 9 changed the intermediate format to a JSON based format and with it
changed the meaning of the -i flag. Because of these changes it was not
possible to generate code coverage with GCC 9. This patch addresses that
by adding its format next to the existing GCov parser.

Addresses: #9406

GCC 9 changed the intermediate format to a JSON based format and with it changed the meaning of the `-i` flag. Because of these changes it was not possible to generate code coverage with GCC 9. This patch addresses that by adding its format next to the existing GCov parser. Addresses: bazelbuild#9406

ulfjack · 2020-06-03T10:56:45Z

...CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator/GcovJsonParser.java

+
+// Classes for the Gson data mapper representing the structure of the GCov JSON format
+
+class GcovJsonFormat {


I think having multiple top-level classes in a java file is against the style guide

Also, at least provide a link to the documentation for the JSON file format.

Would it be acceptable to make them inner classes? I think that separating these 5 small classes that do not have associated methods and can't really be reused in other parts of the codebase into their own file would be close to littering.

Furthermore I'm not following the styleguide w.r.t. field naming as well because the elements in the JSON file use this naming, is that an issue as well?

The GCov manpage contains a thorough explanation of the fomat, however I'm having issues finding a versioned permalink other than their git source: https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/doc/gcov.texi;h=dcdd7831ff063483d43e5347af0b67083c85ecc4;hb=4212a6a3e44f870412d9025eeb323fd4f50a61da#l184 which is only in texinfo format

Inner classes are fine. I don't know how strict the style check is on the names. Please add a comment saying that these have to match the upstream definitions, so they don't get 'auto-corrected' on import.

ulfjack · 2020-06-03T10:58:16Z

tools/test/collect_cc_coverage.sh

-          cat *.gcov >> "$output_file"
-          # Delete the .gcov files.
-          rm *.gcov
+          gcov_version=$("${GCOV}" --version | sed -n -r -e 's/^.*\s([0-9]\.[0-9]\.[0-9])\s?.*$/\1/p')


Note that changes to this file go live immediately, whereas the coverage collector must be released.

I don't think there is a way to deliver this fix without making changes in both parts. Using a combination (updated script but old collector or vice versa) shouldn't be hurtful, you'll just get missing coverage as you did before the patch.

Is it possible to release an updated Coverage Collector when the bazel update containing this fix is released?

The coverage collector release is separate from the bazel release, but someone at Google will have to make the push.

ulfjack · 2020-06-03T10:59:27Z

tools/test/collect_cc_coverage.sh

+          gcov_version=$("${GCOV}" --version | sed -n -r -e 's/^.*\s([0-9]\.[0-9]\.[0-9])\s?.*$/\1/p')
+
+          # Check the gcov version so we can process the data correctly
+          if [ "$(printf '%s\n%s' "9.1.0" "$gcov_version" | sort -V | head -n 1)" != "$gcov_version" ]; then


I think the style guide says to prefer [[ ]] over [ ].

ulfjack · 2020-06-03T11:00:07Z

tools/test/collect_cc_coverage.sh

-          rm *.gcov
+          gcov_version=$("${GCOV}" --version | sed -n -r -e 's/^.*\s([0-9]\.[0-9]\.[0-9])\s?.*$/\1/p')
+
+          # Check the gcov version so we can process the data correctly


I think it would be good to explain here how this check works. It took me a minute to figure out.

ulfjack · 2020-06-03T11:03:01Z

tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator/Main.java

@@ -78,8 +79,13 @@ static int runWithArgs(String... args) throws ExecutionException, InterruptedExc
                getTracefiles(flags, filesInCoverageDir),
                LcovParser::parse,
                flags.parseParallelism()),
-            parseFiles(
-                getGcovInfoFiles(filesInCoverageDir), GcovParser::parse, flags.parseParallelism()));
+	    Coverage.merge(


I'd only use a single call to Coverage.merge here and pass all parsed files to that call. If merge only takes two arguments, then change that to take more than two. I think it's strictly better to pass everything in at once, so Coverage.merge has the full list - there may be optimizations it can do in that case.

This allows merging more than 2 coverage sets together.

- Changed the regex to only select the major version - As a result, remove the whole printf/sort/head pipeline - Change the regex to accept versions that contain multiple digits

ulfjack · 2020-06-04T10:55:37Z

@meisterT

meisterT · 2020-06-04T13:42:53Z

tools/test/collect_cc_coverage.sh

+          # Check the gcov version so we can process the data correctly, GCC 9
+          # contains the required changes
+          if [[ $gcov_major_version -ge 9 ]]; then
+              # GCOV 9.1.0 or higher generate a JSON based coverage format


there is no GCOV 9.0.0?

GCC's versioning is a bit weird: 9.0.0 are development releases and the first actual release is 9.1.0. So there are only GCC 9.0.0 development snapshots, which no one (still) uses and even if they are in use, they'll probably already contain this JSON code.

I probably should have updated the comment to say GCOV 9 instead

Source: https://gcc.gnu.org/develop.html#num_scheme

I've clarified the comment a few days ago, is that better?

Sorry for the delay. It's stuck in internal review for a while now. Let me see if I can do something about it.

xingxinghuo1000 · 2020-07-27T14:56:25Z

...CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator/GcovJsonParser.java

+
+  class GcovJsonLine {
+    GcovJsonBranch[] branches;
+    int count;


The type of count could be long?
Take a look at : #9406 (comment)

Generate a coverage report in CI with Bazel. This requires using Linux and version of GCC that works with the Bazel GCov parser[1]. In this case, CI uses Ubuntu 18.04 with GCC 7. [1]: bazelbuild/bazel#11538

helaan requested a review from lberki as a code owner June 2, 2020 18:09

googlebot added the cla: yes label Jun 2, 2020

helaan force-pushed the fix/gcc9-coverage branch from a6781c8 to fc37c66 Compare June 2, 2020 18:14

ulfjack requested changes Jun 3, 2020

View reviewed changes

helaan added 3 commits June 4, 2020 12:22

Move top-level classes to inner classes for styleguide reasons

2f8aadd

Change Coverage.merge to accept varargs

fa80ee3

This allows merging more than 2 coverage sets together.

Simplify version selection logic

b26ecb8

- Changed the regex to only select the major version - As a result, remove the whole printf/sort/head pipeline - Change the regex to accept versions that contain multiple digits

ulfjack approved these changes Jun 4, 2020

View reviewed changes

meisterT reviewed Jun 4, 2020

View reviewed changes

Clarify first gcov release and spell gcov consistently

72f61a3

bazel-io closed this in 3b75bc7 Jun 16, 2020

xingxinghuo1000 reviewed Jul 27, 2020

View reviewed changes

oquenchil mentioned this pull request Nov 25, 2020

Fix broken Bazel tests on Ubuntu 20.04 #12164

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GCC 9 Code coverage by adding its JSON format #11538

Fix GCC 9 Code coverage by adding its JSON format #11538

helaan commented Jun 2, 2020

ulfjack Jun 3, 2020

ulfjack Jun 3, 2020

helaan Jun 3, 2020

ulfjack Jun 3, 2020

ulfjack Jun 3, 2020

helaan Jun 3, 2020

ulfjack Jun 3, 2020

ulfjack Jun 3, 2020

ulfjack Jun 3, 2020

ulfjack Jun 3, 2020

ulfjack commented Jun 4, 2020

meisterT Jun 4, 2020

helaan Jun 4, 2020

helaan Jun 16, 2020

meisterT Jun 16, 2020

xingxinghuo1000 Jul 27, 2020 •

edited

Loading


		// Classes for the Gson data mapper representing the structure of the GCov JSON format

		class GcovJsonFormat {

Fix GCC 9 Code coverage by adding its JSON format #11538

Fix GCC 9 Code coverage by adding its JSON format #11538

Conversation

helaan commented Jun 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulfjack commented Jun 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xingxinghuo1000 Jul 27, 2020 • edited Loading

Choose a reason for hiding this comment

xingxinghuo1000 Jul 27, 2020 •

edited

Loading