[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]

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

Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #388000|review+                     |review-
              Flags|                            |

--- Comment #7 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 388000
  --> https://bugs.webkit.org/attachment.cgi?id=388000
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?

-- 
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/20200118/e3a6bc0c/attachment-0001.htm>