[PATCH] D38124: Hide some symbols to avoid a crash on shutdown when using code coverage
marco-c added a comment. The change was already positively reviewed with the request for a test, which has been written since then. Repository: rCRT Compiler Runtime https://reviews.llvm.org/D38124 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54195: Fix linker option for -fprofile-arcs -ftest-coverage
marco-c accepted this revision.
marco-c added a comment.
This revision is now accepted and ready to land.
Thanks!
Comment at: test/Driver/clang_f_opts.c:89
+// RUN: %clang -### -fprofile-arcs -ftest-coverage %s 2>&1 | FileCheck
-check-prefix=CHECK-u %s
+// CHECK-u-NOT: "-u{{.*}}"
+
Could you add a test for --coverage too?
Repository:
rC Clang
https://reviews.llvm.org/D54195
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52034: [Clang] Add options -Xclang -coverage-filter and -Xclang -coverage-exclude to filter the files to instrument with gcov
marco-c added inline comments. Comment at: include/clang/Driver/CC1Options.td:236 +def coverage_exclude_EQ : Joined<["-"], "coverage-exclude=">, + Alias; def coverage_exit_block_before_body : Flag<["-"], "coverage-exit-block-before-body">, calixte wrote: > vsk wrote: > > Have you checked whether gcc supports similar options? If so, it would be > > great if we could match their name & behavior. > The only one I found -finstrument-functions-exclude-file-list > (https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html). > But no regex and no way to include one file only. > I took the names from gcovr: > https://manpages.debian.org/jessie/gcovr/gcovr.1.en.html We could file a bug in GCC's Bugzilla and agree with them about the options. Repository: rC Clang https://reviews.llvm.org/D52034 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38221: Add CoreOption flag to "-coverage" option to make it available for clang-cl
marco-c created this revision. The -coverage option is not a CoreOption, so it is not available to clang-cl. This patch adds the CoreOption flag to "-coverage" to allow it to be used with clang-cl. https://reviews.llvm.org/D38221 Files: include/clang/Driver/Options.td test/Driver/coverage.c Index: test/Driver/coverage.c === --- /dev/null +++ test/Driver/coverage.c @@ -0,0 +1,7 @@ +// Test coverage flag. +// +// RUN: %clang_cl -### -coverage %s -o foo/bar.o 2>&1 | FileCheck -check-prefix=CLANG-CL-COVERAGE %s +// CLANG-CL-COVERAGE-NOT: error: +// CLANG-CL-COVERAGE-NOT: warning: +// CLANG-CL-COVERAGE-NOT: argument unused +// CLANG-CL-COVERAGE-NOT: unknown argument Index: include/clang/Driver/Options.td === --- include/clang/Driver/Options.td +++ include/clang/Driver/Options.td @@ -515,7 +515,7 @@ def client__name : JoinedOrSeparate<["-"], "client_name">; def combine : Flag<["-", "--"], "combine">, Flags<[DriverOption, Unsupported]>; def compatibility__version : JoinedOrSeparate<["-"], "compatibility_version">; -def coverage : Flag<["-", "--"], "coverage">; +def coverage : Flag<["-", "--"], "coverage">, Flags<[CoreOption]>; def cpp_precomp : Flag<["-"], "cpp-precomp">, Group; def current__version : JoinedOrSeparate<["-"], "current_version">; def cxx_isystem : JoinedOrSeparate<["-"], "cxx-isystem">, Group, Index: test/Driver/coverage.c === --- /dev/null +++ test/Driver/coverage.c @@ -0,0 +1,7 @@ +// Test coverage flag. +// +// RUN: %clang_cl -### -coverage %s -o foo/bar.o 2>&1 | FileCheck -check-prefix=CLANG-CL-COVERAGE %s +// CLANG-CL-COVERAGE-NOT: error: +// CLANG-CL-COVERAGE-NOT: warning: +// CLANG-CL-COVERAGE-NOT: argument unused +// CLANG-CL-COVERAGE-NOT: unknown argument Index: include/clang/Driver/Options.td === --- include/clang/Driver/Options.td +++ include/clang/Driver/Options.td @@ -515,7 +515,7 @@ def client__name : JoinedOrSeparate<["-"], "client_name">; def combine : Flag<["-", "--"], "combine">, Flags<[DriverOption, Unsupported]>; def compatibility__version : JoinedOrSeparate<["-"], "compatibility_version">; -def coverage : Flag<["-", "--"], "coverage">; +def coverage : Flag<["-", "--"], "coverage">, Flags<[CoreOption]>; def cpp_precomp : Flag<["-"], "cpp-precomp">, Group; def current__version : JoinedOrSeparate<["-"], "current_version">; def cxx_isystem : JoinedOrSeparate<["-"], "cxx-isystem">, Group, ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45454: Make __gcov_flush visible outside a shared library
marco-c added a comment. In https://reviews.llvm.org/D45454#1070884, @belleyb wrote: > @chh I had a chance to try out your proposed changes. It's not causing us any > trouble. In fact, `__gcov_flush()` is not even used at all (at least in LLVM > 5.0.1).. I can recompile llvm, compiler_rt and clang and re-run all the tests > with `__gcov_flush` commented out! No problem. > > I would suggest adding a bit more documentation to `__gcov_flush()`, thus > describing what those "special cases" are... __gcov_flush is only used if you actually call it (it's needed for example if you want to profile only part of your program). In GCC, __gcov_flush is not hidden, so perhaps we should do the same to keep the same behavior? I've also submitted https://reviews.llvm.org/D48538, which is making __gcov_flush flush counters for all shared libraries (like GCC does, with the same caveat: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83879). https://reviews.llvm.org/D45454 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45454: Make __gcov_flush visible outside a shared library
marco-c added a comment. Yes, the behavior changed very recently, I would be surprised if somebody came to depend on it. It's more likely that some clients are depending on the old behavior. - Marco. Il 26/06/2018 22:43, Stephen Hines via Phabricator ha scritto: > srhines added a comment. > > In https://reviews.llvm.org/D45454#1144099, @davidxl wrote: > >> GCC's behavior is not documented and it also has changed over the years. >> >> Unless there is a bug, changing LLVM's gcov_flush visibility can potentially >> break clients that depend on this behavior. > > I think that's the issue though. LLVM changed visibility of this function > recently too. We had Android code depending on this function being visible, > and that broke. > > https://reviews.llvm.org/D45454 https://reviews.llvm.org/D45454 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45454: Add llvm_gcov_flush to be called outside a shared library
marco-c added a comment. Why keeping a __gcov_flush which is incompatible with GCC and previous versions of LLVM and adding a __llvm_gcov_flush which is compatible? I feel the other way around (or even just having an unhidden "__gcov_flush" and not introducing "__llvm_gcov_flush") would be more sensible. https://reviews.llvm.org/D45454 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45454: Add llvm_gcov_flush to be called outside a shared library
marco-c added a comment. OK! Sounds good to me to keep it hidden until https://reviews.llvm.org/D48538 is done (I'm going to try to finish it soon). Il 29/06/2018 19:34, David Li via Phabricator ha scritto: > davidxl added a comment. > > With the current gcov_flush implementation in LLVM, making gcov_flush's > visibility to be default will simply lead to wrong behavior. GCC libgcov's > implementation is more elaborate -- it allows gcov_flush to dump gcda data > for all dynamic objects while making sure gcov_exit only dumps the gcda files > from the same dynamic module (otherwise, there will be wrong profile > merging). This is done via two levels of linked list. > > The patch https://reviews.llvm.org/D48538 is in the right direction, but not > complete yet. > > https://reviews.llvm.org/D45454 https://reviews.llvm.org/D45454 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D74953: [profile] Don't dump counters when forking and don't reset when calling exec** functions
marco-c added a comment.
Also, as we discussed, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93623#c5
regarding exec.
Comment at: compiler-rt/lib/profile/GCDAProfiling.c:665
+ gcov_lock();
+ // Avoid a concurrent modification of the lists during the fork
+ pid = fork();
Could you expand the comment explaining a situation where this could fail if we
didn't lock?
Comment at: compiler-rt/lib/profile/GCDAProfiling.c:671
+pid_t child_pid = getpid();
+if (child_pid != parent_pid) {
+ // The pid changed so we've a fork
Nit: do we need this check or can we just use the earlier one on pid == 0?
Comment at: compiler-rt/lib/profile/GCDAProfiling.c:675
+ // No need to lock here since we just forked and cannot have any other
+ // threads.
+ struct fn_node *curr = reset_fn_list.head;
What if we have a thread in another process making modifications to the list?
Comment at: llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp:635
void GCOVProfiler::AddFlushBeforeForkAndExec() {
- SmallVector ForkAndExecs;
+ SmallVector, 2> ForkAndExecs;
for (auto &F : M->functions()) {
Since we are now mostly doing different things on forks and execs, we could
remove this vector and just do the operations directly instead of adding to the
vec.
Comment at: llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp:671
+// We split just after the fork to have a counter for the lines after
+// Anyway there's a bug:
+// void foo() { fork(); }
Isn't this bug fixed by splitting the block?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74953/new/
https://reviews.llvm.org/D74953
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D74953: [profile] Don't dump counters when forking and don't reset when calling exec** functions
marco-c accepted this revision.
marco-c added a comment.
This revision is now accepted and ready to land.
Could you test the last iteration of the patch on Mozilla's CI (with the
workaround for the mismatch in LLVM version used by Rust)?
Comment at: llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp:635
void GCOVProfiler::AddFlushBeforeForkAndExec() {
- SmallVector ForkAndExecs;
+ SmallVector, 2> ForkAndExecs;
for (auto &F : M->functions()) {
calixte wrote:
> marco-c wrote:
> > Since we are now mostly doing different things on forks and execs, we could
> > remove this vector and just do the operations directly instead of adding to
> > the vec.
> If we make the insertions of new code in the second for loop, we'll
> invalidate the iterator used in this loop.
M->getOrInsertFunction is what would invalidate the iterator?
You could clean things up a bit by having two vectors then, one for forks and
one for execs.
Comment at: llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp:686
+ // Since the process is replaced by a new one we need to write out gcdas
+ // No need to reset the counters since there'll be lost after the exec**
+ FunctionType *FTy = FunctionType::get(Builder.getVoidTy(), {}, false);
Replace with `// No need to reset the counters since they'll be lost after the
exec**`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74953/new/
https://reviews.llvm.org/D74953
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
