Hi Jonas and Nico,

Hi Jonas,

I've reverted for now - I took a quick look at the sanitizer test and couldn't grok whats going wrong, will try again when I'm awake :)

It seems that this commit has caused several test failures.

Hi Petr Hosek,

The previous patch doesn’t support the sequential commands, for example “mkdir existing_folder || true”. This caused sanitizer-windows test failed. The patch has been updated to support the sequential commands.

Reverted rL318911 since it broke the sanitizer-windows.

The test of MC/AsmParser/preserve-comments-crlf.s failed since it uses the unsupported diff option --strip-trailing-cr. I updated the internal diff shell command to support this option.

Optimised the patch by using python ‘getopt’ module.

Updated the builtin diff shell to support ‘-u’, ‘-b’ and ‘-w’ options. Now, all llvm, and clang tests passed with both Python 2 and Python 3.

1. I don't see any tests for the case where the commands *succeed*. Only for the case where they fail. Shouldn't we have some tests that verify the commands actually work?

The patch updated the test (valid-shell.txt) where lit internal shell commands succeeded.

Update: I actually do see where you're testing that they work, but I'm not convinced that the tests are sufficient. For several reasons.

1. it doesn't test that the builtin command is actually run versus the OS command. It only tests that whatever command is run doesn't fail.

2. It doesn't test that the result of the command is as expected, it only tests that it doesn't return an error. For a somewhat unrealistic illustration of this, imagine that all the commands just returned success without actually doing anything. The test as it is written now would pass.

2. Now that we have upwards of 6 builtin commands, I'm starting to think they should be separated into their own file. For example, lit/test/ShellBuiltins.py We could test them via unittests (e.g. see how BooleanExpression.py implements its unit tests).

What do you think?

In D25086#572183, @vsk wrote:

@MaggieYi ping, do you have the bandwidth to update this patch to use the new CoverageMapping::load API? If not, I can take a shot at it.

In D24687#546833, @vsk wrote:

I'd prefer to just tackle this issue in this patch. IMO it would be better to drop (or at least defer) the other changes.

Ok, I have updated the patch following your advice.

I agree with you and abandon this patch.

In D24367#539047, @vsk wrote:

On a low level, I'm concerned about the unnecessary quadratic behavior in this patch. If "N = |SourceFiles|", then renderPageNavigation() does O(N) work and it's called O(N) times. Given that llvm+clang has over 2,400 source files, this could actually be noticeable. We already get user reports about llvm-cov being slow...

Thanks for your pseudocode . Two issues have been fixed.

Thanks for your explanation. I have updated the patch following your advice.

In D23277#526594, @vsk wrote:

Thanks for splitting things up :).

I'm not sure that the javascript bit is needed. It looks like a way to remove/disable a link, but I don't see any issue with leaving the link active.

Reused the existing native_separator.c test to check fully covered source file case.

Thanks, the test has been added.

Thanks Vedant, the patch has been updated following your comments.

Updating the patch to support the "Goto first zero count" feature.

I have fixed the issues, Thanks.

Hi Vedant,

Many thanks for your comments. As I will be busy on other work in next several days, I will update the patch following your comments once I get those on track. Thanks for the patience.

I have put the change of adding the project summary into Phabricator (https://reviews.llvm.org/D23345).

Hello Vedant,

In D23281#510048, @vsk wrote:

I think it would be fine to commit the two patches separately but simultaneously. The bots may go red temporarily, but that won't be alarming if you mention that a follow-up commit in compiler-rt is needed in your commit message.

Do you have a linux machine to test the changes to test/profile/Linux/coverage_*? If not, I can help with that.

Fix a linux test.

Thanks Vedant.

In D23277#508994, @vsk wrote:

On a high-level, the changes you've made look good. However, I think that this
patch needs to be split up into 3 smaller patches for each functional change
you listed in your summary. That would make it easier for me to review it, and
would make the commit history easier to read through.

Following Vedant's comments, all issues have been fixed.

Thanks Vedant, I have updated the patch following your comments.

In D22692#495195, @vsk wrote:

Could you share more details about how you ran the test? I'm having a hard time understanding the failure because LLVM_SRC_ROOT is set in test/lit.cfg. That environment variable should be set on Windows builders if the test is run with llvm-lit test/tools/llvm-cov/report.cpp.

Thanks for adding the test.

The test in test/tools/llvm-cov/prevent_false_instantiations.h has been updated.

Thanks Vedant. I have updated the patch following your comments.

In D18278#474741, @vsk wrote:

Thanks for testing it out!

It seems that all the pending issues we are discussing actually relate to CoveragePrinter::getOutputPath, which is not a part of this patch. We can fix them before or after this patch is committed, and test the changes using "-format text -output-dir ...".

Given that this is a large change, I'd like to make sure it addresses Justin's concerns and that it has an explicit lgtm before moving forward.

There are two issues with the current patch on Windows, I am happy to patch these later if you want.

Thanks Harlan, LGTM.

Thanks Sean, the patch looks good to me.

Thanks Paul for your help.

Could someone commit it for me please (as I do not have commit access)? Thanks

Thanks, I have updated the test following your comments.

My patch changes 6 compiler flags, which are –coverage, -fprofile-arcs, -fprofile-generate, -fprofile-generate=, -fprofile-instr-generate, -fprofile-instr-generate=. I would like to keep line 7-10 in order to verify the change to using “hasFlag” instead of “hasArg”. For the other switches, I could simplify the tests to only check “–fx” and “–fno-x” in order to cut down on test proliferation.

Following Vedant's comments, two test issues have been fixed.

Thanks for reviewing the patch. I try to answer your questions, could you please let me know if this makes sense to you?

Ping

Thanks for helping me review and submit the patch.

Ping

Hi Anna,