This reverts commit c59e5ef59d.
Reason for revert: This is fixed now.
Apologies for the breakage, but I guess that is always the risk when
adding additional checks.
I found all the places that called clang_code_coverage_wrapper.py and
modified the command lines to guarantee that the build commands would
be run - changing clang_code_coverage_wrapper.py does not do this. This
let me iteratively discover that I needed to add .mm, .m, and .S to the
list of valid extensions. Then I removed the command-line modifications,
which were just extra spaces added.
I could modify more command lines to move source files closer to the
front but I don't want to expand the scope of this change when I'm
primarily just trying to get it to work.
Original change's description:
> Revert "Put source and object file names at the front"
>
> This reverts commit 769565cd01.
>
> Reason for revert: This change appears to cause a breakage on iOS for .mm files (https://ci.chromium.org/p/chromium/builders/try/ios-simulator?cursor=id%3E8843484712786022016)
>
> Original change's description:
> > Put source and object file names at the front
> >
> > When monitoring compilation to look for anomalously slow or memory
> > -intensive or whatever compiles the most important piece of information
> > is the name of the file being compiled. Therefore this should be _first_
> > instead of almost last.
> >
> > Due to the way that clang_code_coverage_wrapper.py works the source file
> > name has to be immediately after the /c option. This undocumented rule
> > caused confusing failures in early versions of this change so this
> > change also documents and enforces this rule.
> >
> > This change is essentially the compiler equivalent of the linker
> > command-line change made in crrev.com/c/2705598
> >
> > Change-Id: I96dd34d6157a8f9c3875180fb53e555cbd9a3975
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2981118
> > Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
> > Reviewed-by: Dirk Pranke <dpranke@google.com>
> > Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#895898}
>
> Change-Id: Id9fc985376fd488e190f996574f9316a9eaf4127
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2986433
> Commit-Queue: Zain Afzal <zafzal@google.com>
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Reviewed-by: Zain Afzal <zafzal@google.com>
> Owners-Override: Zain Afzal <zafzal@google.com>
> Cr-Commit-Position: refs/heads/master@{#895937}
Change-Id: Ib69143f8c800ac0525dcea80078b1781e2572187
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2988286
Reviewed-by: Dirk Pranke <dpranke@google.com>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#896264}
This reverts commit 769565cd01.
Reason for revert: This change appears to cause a breakage on iOS for .mm files (https://ci.chromium.org/p/chromium/builders/try/ios-simulator?cursor=id%3E8843484712786022016)
Original change's description:
> Put source and object file names at the front
>
> When monitoring compilation to look for anomalously slow or memory
> -intensive or whatever compiles the most important piece of information
> is the name of the file being compiled. Therefore this should be _first_
> instead of almost last.
>
> Due to the way that clang_code_coverage_wrapper.py works the source file
> name has to be immediately after the /c option. This undocumented rule
> caused confusing failures in early versions of this change so this
> change also documents and enforces this rule.
>
> This change is essentially the compiler equivalent of the linker
> command-line change made in crrev.com/c/2705598
>
> Change-Id: I96dd34d6157a8f9c3875180fb53e555cbd9a3975
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2981118
> Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
> Reviewed-by: Dirk Pranke <dpranke@google.com>
> Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#895898}
Change-Id: Id9fc985376fd488e190f996574f9316a9eaf4127
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2986433
Commit-Queue: Zain Afzal <zafzal@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Zain Afzal <zafzal@google.com>
Owners-Override: Zain Afzal <zafzal@google.com>
Cr-Commit-Position: refs/heads/master@{#895937}
When monitoring compilation to look for anomalously slow or memory
-intensive or whatever compiles the most important piece of information
is the name of the file being compiled. Therefore this should be _first_
instead of almost last.
Due to the way that clang_code_coverage_wrapper.py works the source file
name has to be immediately after the /c option. This undocumented rule
caused confusing failures in early versions of this change so this
change also documents and enforces this rule.
This change is essentially the compiler equivalent of the linker
command-line change made in crrev.com/c/2705598
Change-Id: I96dd34d6157a8f9c3875180fb53e555cbd9a3975
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2981118
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@google.com>
Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#895898}
Some flakiness has been observed on mac-rel after coverage was enabled
for that builder. The problems correlate with builds that have no
changed source files in Chrome, so we think that forcing coverage for
clang_profiling.cc should solve the problem.
Also remove an outdated comment.
Bug: 1141727
Change-Id: I7ebaeda2ae3a724965e7afa65f20164340c544d9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2559320
Auto-Submit: Sajjad Mirza <sajjadm@chromium.org>
Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
Commit-Queue: Yuke Liao <liaoyuke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#831035}
The wrapper currently removes only one set of the flags for code
coverage partial instrumentation. But there can be situation where
multiple sets of same flags are in compile command and all of these need
to be removed.
Bug: 1137569
Change-Id: I32c48710fd9c0c66d7c218f329efcd222704607e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2466976
Commit-Queue: Zhaoyang Li <zhaoyangli@chromium.org>
Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816694}
This reverts commit 212853047a.
Reason for revert: <INSERT REASONING HERE>
Original change's description:
> Revert "Make the coverage config reusable for generic profiling."
>
> This reverts commit c7695f6312.
>
> Reason for revert: Possible reason for win10_chromium_x64_rel_ng failing builds with error:
>
> lld-link: error: undefined symbol: __llvm_profile_dump
> >>> referenced by .\..\..\base\test\clang_profiling.cc:23
> >>> obj/base\base/clang_profiling.obj:(void __cdecl base::WriteClangProfilingProfile(void))
>
> Original change's description:
> > Make the coverage config reusable for generic profiling.
> >
> > Most of the coverage configuration can be re-used for general
> > Clang-based profiling (e.g. the profiling step of a PGO build).
> > This changes most of the |use_clang_coverage| checks to
> > |use_clang_profiling|.
> >
> > The toolchain opt-out option is still limited to the coverage
> > builds (grep for "toolchain_use_clang_coverage" in CodeSearch).
> > I.e. targets built simply with |use_clang_profiling=true| won't
> > be opted out by the clang_code_coverage_wrapper.py script.
> >
> > This CL is a pure refactoring with no functional change.
> > Getting profiling information from the sandboxed renderers work
> > this way:
> > - After creating a new child process the browser will open a
> > profiling file for this process and pass the handle to the
> > child via the SetProfilingProfile Mojo call (see change in
> > content/browser/renderer_host/render_process_host_impl.cc).
> > - This process will used by the LLVM instrumentation to record
> > profiling information.
> > - When terminating, child processes will call
> > WriteClangProfilingProfile to write the data to the profiling
> > data file.
> > - Once all the raw profile data files have been collected,
> > they'll be merged (via the "llvm-profdata" tool) and used.
> >
> > Note that this isn't meant to be enabled into any build that
> > will be shipped to users (a profiling build is usually 10x
> > bigger than a normal build anyway, not really shippable).
> >
> > The goal of this refactoring is to make it possible to reuse
> > this mechanism for the instrumentation step of PGO builds.
> >
> > Bug: 1058979
> > Change-Id: If8321234f6b6f86befd81f6e9e068a342728d605
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2090493
> > Reviewed-by: Will Harris <wfh@chromium.org>
> > Reviewed-by: Nico Weber <thakis@chromium.org>
> > Reviewed-by: Scott Violet <sky@chromium.org>
> > Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
> > Reviewed-by: Sajjad Mirza <sajjadm@chromium.org>
> > Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#749015}
>
> TBR=sky@chromium.org,thakis@chromium.org,wfh@chromium.org,sebmarchand@chromium.org,liaoyuke@chromium.org,sajjadm@chromium.org
>
> Change-Id: I6aaf638f6385afe35c488575aac84e3e952b76a3
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1058979
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2096332
> Reviewed-by: Jeevan Shikaram <jshikaram@chromium.org>
> Commit-Queue: Jeevan Shikaram <jshikaram@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#749040}
TBR=sky@chromium.org,thakis@chromium.org,wfh@chromium.org,sebmarchand@chromium.org,liaoyuke@chromium.org,sajjadm@chromium.org,jshikaram@chromium.org
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1058979
Change-Id: I5492c81b707e5741bd7c494893e3bc7953221263
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2097305
Reviewed-by: Sébastien Marchand <sebmarchand@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749185}
There is nothing wrong with having coverage in those files but for some
reason my crrev.com/2046404 change that includes them breaks
angle_unittests by making code coverage tooling add some output to
stderr of one of their binaries.
Until the problem of the stderr output is resolved I need a way to pass
CQ.
Here I'm adding the files I think are causing the problem to see if my
cl will pass CQ after.
Bug: 1051561
Change-Id: I14c4b195bda151bbddc57fdd573b2c93c2a59b63
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2054527
Commit-Queue: Oliver Li <olivierli@chromium.org>
Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#741282}
On Windows it is possible for the coverage tryjob to compile a target
that includes clang_coverage.cc but does not instrument any files.
This causes a link-time failure, as clang_coverage.cc refers to
__llvm_profile_dump, but with no instrumented files the compiler will
not include the profile runtime library into the target. The symbol
remains unresolved.
To work around this, we can force coverage instrumentation for the
clang_coverage.cc file itself, thus ensuring that any target that
includes it will always have the profiling runtime pulled in.
Bug: 990480
Change-Id: Ia6e1f003baaac4a050eec235942aa5a5e345ec52
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1978036
Commit-Queue: Sajjad Mirza <sajjadm@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726934}
Paths in `--files-to-instrument` are expected to be normalized OS-native
paths, so the input source file has to be converted to match.
This is most relevant on Windows, where GN creates build commands that
use `/` for filesystem paths, but the canonical form should use `\`.
Bug: 990480
Change-Id: If7b5da729f0a98ec7945092b0f228cebd50be3a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1973360
Commit-Queue: Sajjad Mirza <sajjadm@chromium.org>
Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
Reviewed-by: Roberto Carrillo <robertocn@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726120}
This is a reland of cdc92f96b8
Original change's description:
> Reland "Adding an exclusion list to the coverage wrapper script."
>
> This is a reland of 6b7213a453
>
> Original change's description:
> > Adding an exclusion list to the coverage wrapper script.
> >
> > The script now assumes GN adds coverage flags to the cflags for each
> > source file. The script removes the flag if the file is in an exclusion
> > list, or if `--files-to-instrument` is set it will also remove flags if
> > the file is not listed in that argument.
> >
> > The script supports multiple exclusion lists that can be selected with
> > the `--target-os` flag.
> >
> > It's expected that GN will set these flags correctly and invoke the
> > script in all coverage builds. http://crrev.com/c/1496002 does that.
> >
> >
> > Bug: 918215
> > Change-Id: I4454f67067a083581f31a22cfef85368825789f9
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1495994
> > Commit-Queue: Sajjad Mirza <sajjadm@google.com>
> > Reviewed-by: Max Moroz <mmoroz@chromium.org>
> > Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
> > Reviewed-by: Roberto Carrillo <robertocn@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#637892}
>
> Bug: 918215
> Change-Id: I1254d288ff263ff94c46b46e6be99738e31b9574
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1506974
> Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
> Reviewed-by: Max Moroz <mmoroz@chromium.org>
> Commit-Queue: Sajjad Mirza <sajjadm@google.com>
> Cr-Commit-Position: refs/heads/master@{#638362}
Bug: 918215
Change-Id: Ibd1e68a066bddd69e46c9cb32fcd9df209d97a1a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1509459
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Commit-Queue: Sajjad Mirza <sajjadm@google.com>
Cr-Commit-Position: refs/heads/master@{#638758}
This is a reland of 6b7213a453
Original change's description:
> Adding an exclusion list to the coverage wrapper script.
>
> The script now assumes GN adds coverage flags to the cflags for each
> source file. The script removes the flag if the file is in an exclusion
> list, or if `--files-to-instrument` is set it will also remove flags if
> the file is not listed in that argument.
>
> The script supports multiple exclusion lists that can be selected with
> the `--target-os` flag.
>
> It's expected that GN will set these flags correctly and invoke the
> script in all coverage builds. http://crrev.com/c/1496002 does that.
>
>
> Bug: 918215
> Change-Id: I4454f67067a083581f31a22cfef85368825789f9
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1495994
> Commit-Queue: Sajjad Mirza <sajjadm@google.com>
> Reviewed-by: Max Moroz <mmoroz@chromium.org>
> Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
> Reviewed-by: Roberto Carrillo <robertocn@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#637892}
Bug: 918215
Change-Id: I1254d288ff263ff94c46b46e6be99738e31b9574
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1506974
Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Commit-Queue: Sajjad Mirza <sajjadm@google.com>
Cr-Commit-Position: refs/heads/master@{#638362}
This reverts commit 6b7213a453.
Reason for revert: This CL will break per-CL coverage gathering without CL 1496002, and that other CL is not ready yet.
Original change's description:
> Adding an exclusion list to the coverage wrapper script.
>
> The script now assumes GN adds coverage flags to the cflags for each
> source file. The script removes the flag if the file is in an exclusion
> list, or if `--files-to-instrument` is set it will also remove flags if
> the file is not listed in that argument.
>
> The script supports multiple exclusion lists that can be selected with
> the `--target-os` flag.
>
> It's expected that GN will set these flags correctly and invoke the
> script in all coverage builds. http://crrev.com/c/1496002 does that.
>
>
> Bug: 918215
> Change-Id: I4454f67067a083581f31a22cfef85368825789f9
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1495994
> Commit-Queue: Sajjad Mirza <sajjadm@google.com>
> Reviewed-by: Max Moroz <mmoroz@chromium.org>
> Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
> Reviewed-by: Roberto Carrillo <robertocn@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#637892}
TBR=stgao@chromium.org,robertocn@chromium.org,mmoroz@chromium.org,liaoyuke@chromium.org,sajjadm@google.com
Change-Id: I0cadb9c3d3ed0600648e273ffe58e7dc3f6ef7f8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 918215
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1504960
Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638167}
The script now assumes GN adds coverage flags to the cflags for each
source file. The script removes the flag if the file is in an exclusion
list, or if `--files-to-instrument` is set it will also remove flags if
the file is not listed in that argument.
The script supports multiple exclusion lists that can be selected with
the `--target-os` flag.
It's expected that GN will set these flags correctly and invoke the
script in all coverage builds. http://crrev.com/c/1496002 does that.
Bug: 918215
Change-Id: I4454f67067a083581f31a22cfef85368825789f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1495994
Commit-Queue: Sajjad Mirza <sajjadm@google.com>
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Reviewed-by: Yuke Liao <liaoyuke@chromium.org>
Reviewed-by: Roberto Carrillo <robertocn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#637892}
This CL adds "-mllvm" and "-limited-coverage-experimental=true" cflags
to the code coverage compiler wrapper because they're critical for
reducing the size of the generated binaries.
Bug: 933512
Change-Id: I0d61a36b71605fc160fea164f280206084afe302
Reviewed-on: https://chromium-review.googlesource.com/c/1493383
Auto-Submit: Yuke Liao <liaoyuke@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Commit-Queue: Max Moroz <mmoroz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636304}
This CL implements the Clang code coverage compiler wrapper, which can
be used to instrument a subset of source files, and the main use case
is to enable generating code coverage reports at per-cl level during
try jobs.
This CL only focuses on the wrapper itself, how to hook this wrapper
into the build system will be addressed in a separate CL.
Bug: 898695
Change-Id: Idb21640b5566ce78089059b3d0116390b488a383
Reviewed-on: https://chromium-review.googlesource.com/c/1301969
Commit-Queue: Yuke Liao <liaoyuke@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Takuto Ikuta <tikuta@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604381}
