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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jan 21 05:24:39 PST 2020


https://bugs.webkit.org/show_bug.cgi?id=206389

--- Comment #10 from Tuomas Karkkainen <tuomas.webkit at apple.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200121/3985afbe/attachment.htm>


More information about the webkit-unassigned mailing list