[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

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #388427|review?                     |review-
              Flags|                            |

--- Comment #16 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 388427
  --> https://bugs.webkit.org/attachment.cgi?id=388427
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.

-- 
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/20200122/c5c8a049/attachment-0001.htm>