Bug 206389 – It should be possible to build JavaScriptCore with LLVM Source-based Code Coverage, run the tests and see the coverage data

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tuomas Karkkainen]

It should be possible to build JavaScriptCore with LLVM Source-based Code Coverage, run the tests and see the coverage data

[Tuomas Karkkainen]

Created attachment 387995 [details]
proposed patch

initial proposal for the patch.

start with e.g.:

> Tools/Scripts/set-webkit-configuration --force-optimization-level=O3 --debug --no-asan --coverage

then:

> Tools/Scripts/run-javascriptcore-tests --debug --no-build --coverage

Known limitations:

* CMake build is not supported.
* ASAN and coverage cannot be enabled at the same time. This isn't an inherent limitation, just a limitation of the current build scripts.

Reference for LLVM Source-based Code Coverage:
https://clang.llvm.org/docs/SourceBasedCodeCoverage.html

[Tuomas Karkkainen]

and build with 

> Tools/Scripts/build-jsc

between the two steps above.

[Tuomas Karkkainen]

Created attachment 388000 [details]
proposed patch

add ASAN_OTHER_LDFLAGS back into Tools/coverage/coverage.xcconfig

oops.

[Robin Morisset]

Comment on attachment 388000 [details]
proposed patch

I tested it and it worked just fine.
I don't know perl so I can't tell if the code is good, but it looks reasonable.

[Saam Barati]

Comment on attachment 388000 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388000&action=review

r=me

Nice!

> Tools/Scripts/run-javascriptcore-tests:505
> +  $coverageDir = tempdir();
> +  $htmlDir = File::Spec->catfile($coverageDir, "html_report");
> +  $profdataPath = File::Spec->catfile($coverageDir, "jsc_tests.profdata");
> +  $envVars .=  " LLVM_PROFILE_FILE=" . File::Spec->catfile($coverageDir, "jsc_test_%9m.profraw");

nit: 4 spaces (there might be more of these I miss)

> Tools/Scripts/run-javascriptcore-tests:669
> +    unshift @command, "xcrun", "llvm-cov", "show", builtDylibPathForName("JavaScriptCore"), "--format=html", "--instr-profile=".$profdataPath, "--output-dir=".$htmlDir;

nit: space on the left/right of your "." concats

> Tools/Scripts/run-javascriptcore-tests:675
> +    print("Coverage report is in ".$htmlDir."\n");

ditto

> Tools/Scripts/run-javascriptcore-tests:680
> +  convertProfrawToProfdata();
> +  generateHtmlFromProfdata();

nit: 4 spaces

> Tools/Scripts/run-javascriptcore-tests:697
> +  processCoverageData();

nit: this should be four spaces

> Tools/Scripts/webkitdirs.pm:909
> +    die "cannot enabled both ASAN and Coverage at this time\n" if $coverageIsEnabled && $asanIsEnabled;

"enabled" => "enable"

[Saam Barati]

Comment on attachment 388000 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388000&action=review

> Tools/Scripts/run-javascriptcore-tests:673
> +    unshift @openCommand, "open", $htmlDir."/index.html";

as you suggested, maybe also print out the name of this file to the output of this script

[Alexey Proskuryakov]

Comment on attachment 388000 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388000&action=review

r- because coverage.xcconfig issues indicate that this hasn't been tested very thoroughly. Otherwise, looks pretty clean and comprehensive.

> Tools/Scripts/run-javascriptcore-tests:244
> +  --[no-]coverage               Enable (or disable) LLVM Source-based Code Coverage instrumentation for the test run (default: $coverageDefault)

Was this tested against embedded targets, or only macOS?

> Tools/Scripts/run-javascriptcore-tests:656
> +sub convertProfrawToProfdata {

As you can see elsewhere in this file, we use WebKit C++ coding style for Perl too. Please put braces for subs on separate lines.

sub convertProfrawToProfdata
{
    ...
}

> Tools/Scripts/run-javascriptcore-tests:662
> +    unshift @command, "xcrun", "llvm-profdata", "merge", @profrawFiles, "-o", $profdataPath;

Is it OK to always use the default toolchain? I suspect that we'll run into an issue sooner or later without --sdk or --toolchain. I'm concerned about embedded targets and about internal builds.

Same comment about every xcrun invocation.

> Tools/Scripts/run-javascriptcore-tests:666
> +sub generateHtmlFromProfdata {

WebKit coding style: generateHTMLFromProfdata.

> Tools/Scripts/run-javascriptcore-tests:674
> +    system(@openCommand);

Opening the result in a browser is not great in automated scenarios, and definitely not part of a function named "generateHtmlFromProfdata".

> Tools/Scripts/webkitdirs.pm:907
> +    appendToEnvironmentVariableList("WEBKIT_COVERAGE_BUILD", "1") if $coverageIsEnabled;

Why do we need this in the environment?

> Tools/coverage/coverage.xcconfig:1
> +OTHER_C_FLAGS=$(inherited) -fprofile-instr-generate -fcoverage-mapping

OTHER_C_FLAGS is not a thing, the build setting is OTHER_CFLAGS.

But also, I'm unsure that we can override build settings like this, even with $(inherited). Have you checked with -showBuildSettings that this doesn't change anything other than what's expected? If this actually works (with both modern and legacy build systems), then we should simplify asan.xcconfig in this way.

> Tools/coverage/coverage.xcconfig:3
> +ASAN_OTHER_LDFLAGS=$(inherited) -fprofile-instr-generate

ASAN?

[Alexey Proskuryakov]

Comment on attachment 388000 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388000&action=review

> Tools/coverage/coverage.xcconfig:2
> +OTHER_CPLUSPLUSFLAGS=$(inherited) -fprofile-instr-generate -fcoverage-mapping

Also, we always put semicolons at the end of xcconfig lines, and spaces around '='.

[Tuomas Karkkainen]

Created attachment 388290 [details]
proposed patch

- fixes Tools/Scripts/set-webkit-configuration --reset
- addresses most of the comments for the first patch

[Tuomas Karkkainen]

(In reply to Alexey Proskuryakov from comment #7)
> Comment on attachment 388000 [details]
> proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388000&action=review
> 
> r- because coverage.xcconfig issues indicate that this hasn't been tested
> very thoroughly. Otherwise, looks pretty clean and comprehensive.
> 
> > Tools/Scripts/run-javascriptcore-tests:244
> > +  --[no-]coverage               Enable (or disable) LLVM Source-based Code Coverage instrumentation for the test run (default: $coverageDefault)
> 
> Was this tested against embedded targets, or only macOS?

This was tested only on macOS. This currently only supports macOS, so ideally we'd want to disable any other targets.

> > Tools/Scripts/run-javascriptcore-tests:656
> > +sub convertProfrawToProfdata {
> 
> As you can see elsewhere in this file, we use WebKit C++ coding style for
> Perl too. Please put braces for subs on separate lines.

Thanks, fixed!

> sub convertProfrawToProfdata
> {
>     ...
> }
> 
> > Tools/Scripts/run-javascriptcore-tests:662
> > +    unshift @command, "xcrun", "llvm-profdata", "merge", @profrawFiles, "-o", $profdataPath;
> 
> Is it OK to always use the default toolchain? I suspect that we'll run into
> an issue sooner or later without --sdk or --toolchain. I'm concerned about
> embedded targets and about internal builds.
> 
> Same comment about every xcrun invocation.
> 
> > Tools/Scripts/run-javascriptcore-tests:666
> > +sub generateHtmlFromProfdata {
> 
> WebKit coding style: generateHTMLFromProfdata.

Fixed.

> > Tools/Scripts/run-javascriptcore-tests:674
> > +    system(@openCommand);
> 
> Opening the result in a browser is not great in automated scenarios, and
> definitely not part of a function named "generateHtmlFromProfdata".

I fully agree that this is a surprise given the function name.

I switched it to only printing out the report URL.

> > Tools/Scripts/webkitdirs.pm:907
> > +    appendToEnvironmentVariableList("WEBKIT_COVERAGE_BUILD", "1") if $coverageIsEnabled;
> 
> Why do we need this in the environment?

When building with coverage instrumentation, Tools/Scripts/check-for-weak-vtables-and-externals complains about certain symbols. Setting this environment variable makes the errors non-fatal. I added a comment to document this.

> > Tools/coverage/coverage.xcconfig:1
> > +OTHER_C_FLAGS=$(inherited) -fprofile-instr-generate -fcoverage-mapping
> 
> OTHER_C_FLAGS is not a thing, the build setting is OTHER_CFLAGS.
> 

Great catch, fixed.

> But also, I'm unsure that we can override build settings like this, even
> with $(inherited). Have you checked with -showBuildSettings that this
> doesn't change anything other than what's expected? If this actually works
> (with both modern and legacy build systems), then we should simplify
> asan.xcconfig in this way.

I'm unsure how to do that.

> > Tools/coverage/coverage.xcconfig:3
> > +ASAN_OTHER_LDFLAGS=$(inherited) -fprofile-instr-generate
> 
> ASAN?

I switched it to OTHER_LDFLAGS.

The reasoning for using the ASAN_ one was because all the .xcconfig files perform "OTHER_LDFLAGS = $(ASAN_OTHER_LDFLAGS);" 

The configurations from coverage.xcconfig seem to be applied after these so OTHER_LDFLAGS works just as well.

[Tuomas Karkkainen]

(In reply to Alexey Proskuryakov from comment #7)
> Comment on attachment 388000 [details]
> proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388000&action=review
> 
> 
> > Tools/Scripts/run-javascriptcore-tests:662
> > +    unshift @command, "xcrun", "llvm-profdata", "merge", @profrawFiles, "-o", $profdataPath;
> 
> Is it OK to always use the default toolchain? I suspect that we'll run into
> an issue sooner or later without --sdk or --toolchain. I'm concerned about
> embedded targets and about internal builds.
> 
> Same comment about every xcrun invocation.

Should xcrun be replaced with xcrun --sdk macosx ?

[Alexey Proskuryakov]

> Should xcrun be replaced with xcrun --sdk macosx ?

It should really be using xcodeSDK(). Even if this only supports macOS, its SDK can be macosx.internal. Having mismatches between macosx and macosx.internal was causing substantial pain for ASan in the past.

> I'm unsure how to do that.

You can add -showBuildSettings  to xcodebuild invocation (as a local change to webkitdirs.pm) then build with and without the overrides in coverage.xcconfig. I expect that you'll find the coverage options added, but many others disappear.

> The reasoning for using the ASAN_ one was because all the .xcconfig files perform "OTHER_LDFLAGS = $(ASAN_OTHER_LDFLAGS);" 

Right, ASAN adds options in this way to clearly guarantee that it is not overriding anything. But it's not OK to reuse the name, and also not OK to only do this for LDFLAGS, and not for C/C++ flags.

Not all targets have it quite the way you quoted, there are multiple that have their own options even for LDFLAGS, and many more for C/C++.

Source/ThirdParty/libwebrtc/Configurations/Base.xcconfig:OTHER_LDFLAGS = $(ASAN_OTHER_LDFLAGS) -fvisibility=default;

Source/WebKit/Configurations/GPUService.xcconfig:OTHER_LDFLAGS = $(inherited) $(OTHER_LDFLAGS_VERSIONED_FRAMEWORK_PATH) $(WK_RELOCATABLE_FRAMEWORKS_LDFLAGS);

Source/WebKit/Configurations/BaseXPCService.xcconfig:OTHER_LDFLAGS = $(inherited) $(OTHER_LDFLAGS_VERSIONED_FRAMEWORK_PATH) $(WK_RELOCATABLE_FRAMEWORKS_LDFLAGS);

etc.

I think that the way it works is that options are inherited for xcconfigs that are marked as inherited in the project, but those added on command line just override the variables. But I may be wrong.

[Tuomas Karkkainen]

Created attachment 388405 [details]
proposed patch

use `-sdk <sdk>` with xcrun if sdk has been specified.

[Tuomas Karkkainen]

(In reply to Alexey Proskuryakov from comment #12)
> > Should xcrun be replaced with xcrun --sdk macosx ?
> 
> It should really be using xcodeSDK(). Even if this only supports macOS, its
> SDK can be macosx.internal. Having mismatches between macosx and
> macosx.internal was causing substantial pain for ASan in the past.
> 

I made it use xcodeSDK() now. Because run-javascriptcore-tests is slightly different than the other scripts, it actually swallows --sdk <xyz> if --debug is also specified.

This solved one of the problems I was having on a specific machine with mismatched SDKs so thank you for pointing me in the right direction!

> > I'm unsure how to do that.
> 
> You can add -showBuildSettings  to xcodebuild invocation (as a local change
> to webkitdirs.pm) then build with and without the overrides in
> coverage.xcconfig. I expect that you'll find the coverage options added, but
> many others disappear.
> 
> > The reasoning for using the ASAN_ one was because all the .xcconfig files perform "OTHER_LDFLAGS = $(ASAN_OTHER_LDFLAGS);" 
> 
> Right, ASAN adds options in this way to clearly guarantee that it is not
> overriding anything. But it's not OK to reuse the name, and also not OK to
> only do this for LDFLAGS, and not for C/C++ flags.
> 
> Not all targets have it quite the way you quoted, there are multiple that
> have their own options even for LDFLAGS, and many more for C/C++.
> 
> Source/ThirdParty/libwebrtc/Configurations/Base.xcconfig:OTHER_LDFLAGS =
> $(ASAN_OTHER_LDFLAGS) -fvisibility=default;
> 
> Source/WebKit/Configurations/GPUService.xcconfig:OTHER_LDFLAGS =
> $(inherited) $(OTHER_LDFLAGS_VERSIONED_FRAMEWORK_PATH)
> $(WK_RELOCATABLE_FRAMEWORKS_LDFLAGS);
> 
> Source/WebKit/Configurations/BaseXPCService.xcconfig:OTHER_LDFLAGS =
> $(inherited) $(OTHER_LDFLAGS_VERSIONED_FRAMEWORK_PATH)
> $(WK_RELOCATABLE_FRAMEWORKS_LDFLAGS);
> 
> etc.
> 
> I think that the way it works is that options are inherited for xcconfigs
> that are marked as inherited in the project, but those added on command line
> just override the variables. But I may be wrong.

I tried this and it seems to work just as it should, e.g.

> OTHER_CFLAGS =  -fvisibility=default

becomes

> OTHER_CFLAGS =  -fvisibility=default -fprofile-instr-generate -fcoverage-mapping


I also tried using LocalOverrides.xcconfig together with coverage and without coverage.

LocalOverrides.xcconfig does not work as I would want it to, that is, it overrides the variables, swallowing the original value completely. E.g. -fvisibility=default disappears from the above.

[Tuomas Karkkainen]

Created attachment 388427 [details]
proposed patch

adds ChangeLog

[David Kilzer (:ddkilzer)]

Comment on attachment 388427 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388427&action=review

r- to fix if/else formatting.

> Tools/ChangeLog:10
> +        * Scripts/run-javascriptcore-tests:
> +        * Scripts/set-webkit-configuration:
> +        * Scripts/webkitdirs.pm:

Can we make `Tools/Scripts/set-webkit-configuration --coverage` take an optional string argument with the path to the coverage directory and write the string path to the Coverage text file instead of "YES"?

If we did that, then run-javascriptcore-tests wouldn't require any argument to run with coverage (it could just read the directory from the Coverage config file).

This would also simplify the command-line switches for all the scripts to just a single "--coverage[=s]" switch that takes an optional path argument.

> Tools/Scripts/run-javascriptcore-tests:221
> +my $coverageDefault = $coverage ? "coverage enabled" : "coverage disabled";

Should run-javascfriptcore-tests set its default value based on reading the Coverage config file (determineCoverageIsEnabled() from webkitdirs.pm) if there is no command-line switch?  Seems like we'd want this to "just work".

> Tools/Scripts/run-javascriptcore-tests:513
> +if ($coverage)
> +{
> +    if ($coverageDir)
> +    {
> +        print "Using output path specified on the command line for coverage data: $coverageDir\n";
> +    }
> +    else
> +    {
> +        $coverageDir = tempdir();
> +        print "Generating coverage data into $coverageDir\n";
> +    }

Please correct formatting of braces in if/else blocks (I'm not going to call out every instance of this, so here's an example):

if ($coverage) {
    if ($coverageDir) {
        print "Using output path specified on the command line for coverage data: $coverageDir\n";
    } else {
        $coverageDir = tempdir();
        print "Generating coverage data into $coverageDir\n";
    }

> Tools/coverage/coverage.xcconfig:3
> +OTHER_CFLAGS = $(inherited) -fprofile-instr-generate -fcoverage-mapping;
> +OTHER_CPLUSPLUSFLAGS = $(inherited) -fprofile-instr-generate -fcoverage-mapping;
> +OTHER_LDFLAGS = $(inherited) -fprofile-instr-generate;

To answer Alexey's question from another patch review:  Yes, $(inherited) should do the correct thing with asan.xcconfig files.

[Tuomas Karkkainen]

Created attachment 388447 [details]
proposed patch

fixes if/else brace formatting

[Alexey Proskuryakov]

Comment on attachment 388447 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388447&action=review

I guess we should simplify things to stop using ASAN_ variables then!

> Tools/Scripts/webkitdirs.pm:908
> +    # treats errors as non-fatal when it encounters missing symbols related to coverage

Please add a period at the end of the sentence.

[Tuomas Karkkainen]

Created attachment 388513 [details]
proposed patch

fix grammar.

[WebKit Commit Bot]

Comment on attachment 388513 [details]
proposed patch

Clearing flags on attachment: 388513

Committed r254969: <https://trac.webkit.org/changeset/254969>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58828375>