[webkit-reviews] review denied: [Bug 206389] It should be possible to build JavaScriptCore with LLVM Source-based Code Coverage, run the tests and see the coverage data : [Attachment 388000] proposed patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied	review:
Bug 206389: It should be possible to build JavaScriptCore with LLVM
Source-based Code Coverage, run the tests and see the coverage data
https://bugs.webkit.org/show_bug.cgi?id=206389

Attachment 388000: proposed patch

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




--- 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?