[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