Sorry about the delay.

Craig, does this look ok now?

In D55029#1312167, @ruiu wrote:

I wouldn't rush to submit this now, given that this issue is not new at all. Maybe we can just wait for Peter's response?

Thanks Zhizhou! I'm curious about Peter's answer to the 64KiB default as well, but I think we should move forward with getting this patch submitted.

I just wanted to follow up here.

Yabin did some of the work to bring TSan to Android. Perhaps he knows whether this is has significant use. Considering that it is meant as a debugging aide, I would actually be comfortable with moving it. You are also correct that we never officially marked it as a feature of the NDK (or platform). That is something we want to do though.

LGTM, but please wait for dberris approval.

Please remove the reference to b/X in the commit message. LLVM doesn't allow internal bug numbers, and you described the issue well enough without it.

This LGTM, but we should wait to hear from Kristof before submitting.

Really cool! Thanks for making everything easier to use out-of-the-box.

+enh and danalbert - in case they have any other concerns about this (Ryan's already on the review). From my perspective, everything here makes sense for resolving this issue.

Thanks @dblaikie for the quick fixup. I must have accidentally dropped the '!', because I did run check-all to test the change.

In D52191#1238628, @dblaikie wrote:

Sure, looks good. Though my other/vague concern is why does this case error about fomit-frame-pointer having no effect, but other things (like using -fomit-frame-pointer on a target that requires frame pointers) that ignore -fomit-frame-pointer don't? Weird. But it probably makes sense somehow.

+Ryan in case there is anything here that could affect Bionic loading from these pages.

Thanks again! @alexshap feel free to submit this or I can do it in the morning tomorrow (can't stay up any later to watch for unlikely breakage, etc.).

Thanks for noticing this missing feature, as well as for the patch! You really should add a simple test to test/tools/llvm-objcopy/strip-all-gnu.test so that this doesn't ever regress. The test strip-all-and-keep-symbol.test is an example that has multiple RUN lines. Here is what you need to change the strip-all-gnu.test to (I believe) in order to get this working:

In D51713#1232414, @manojgupta wrote:

What is the call generated with -pg for AMR32, gnu_mcount_nc or _mount? gnu_mcount_nc with "-pg" is known to be broken ( https://bugs.llvm.org/show_bug.cgi?id=33845)

Union punning is one of my pet peeves, so I am glad to see this. bit_cast seems generally useful too, so thanks for adding it.

Thanks for the update. I'll let the project maintainers give a final LGTM, but I'm happy with it.

http://b/32510864 was the internal bug request, so I am noting it here for future reference, but I think that the patch itself is pretty self-explanatory (rather than filing a distinct upstream bug about this issue).

This was discovered in the Android build system (which passes -fomit-frame-pointer by default for ARM configurations. This leads to the inability to specify -pg, since there is no way to override the mere presence of -fomit-frame-pointer.

Thanks for the detailed explanations. I only had one minor nitpick.

Please upload patches with context, as it makes it easier to review. https://llvm.org/docs/Phabricator.html has instructions on how to do this (search for "context").

Thanks for cleaning this up and adding better checks for Android. :)

Sorry, this got buried in my inbox back in April right before EuroLLVM.

In D49534#1177242, @alexshap wrote:

@srhines - is there anything blocking this diff ?

Definitely deal with the submodule stuff. We don't want a new file checked in for tools/clang.

I restored the original change, since I had confused things with arcanist before. PTAL

Revert back to not layering the positional argument change.

Gah, you are correct. Our Android build system had grown to depend on this behavior from llvm-strip previously. Doing an update now is how I am finding all of these issues. I'll fix it in our build system and abandon this CL. Thanks for the review.

Switch to using .back() because I was lazy in the first upload. :)

• Fix positional output argument for llvm-strip.

Ran git-clang-format and added rationale to the test.

Thanks for the quick review.

https://reviews.llvm.org/D47100 is the commit where this got dropped.

Removed this in favor of the suggestions here. Setting the CMAKE_FIND_ROOT_PATH_MODE* variables does make this properly hermetic.

Removed this in favor of the suggestions in https://reviews.llvm.org/D48459. Setting the CMAKE_FIND_ROOT_PATH_MODE* variables does make this properly hermetic.

In D48538#1151578, @marco-c wrote:

On Tue, Jul 3, 2018 at 4:29 PM Marco Castelluccio via Phabricator <reviews@reviews.llvm.org> wrote:

marco-c updated this revision to Diff 154015.
marco-c added a comment.

I've taken another approach which looks simpler to me. Instead of having two lists, I only have a shared list and I use a static variable's address as an identifier for a dynamic object.

I've also added another test (which would have failed with the previous iteration of the patch) which dlopens three libraries.

I will file follow-up bugs for things that don't look right in the llvm-cov output which are not regressions from this patch:

- instrprof-shared-main-gcov-flush_no-writeout.c.gcov: lines 23, 26 and 30 should not be covered; lines 33 and 35 should not be covered (they are considered as uncoverable instead).
- instrprof-dlopen-func{2,3}.c.gcov: the only line in the function should be covered once and not thrice

Regarding removing writeout and only always using __gcov_flush, do you see any disadvantage if we do that?

writeout function only writes the profile for the current dynamic module which is different from the behavior of __gcov_flush. The form is expected when shared lib is unloaded -- i.e., do not dump other load module's profile unless being explicitly told so (to use gcov_flush). The write out is similar to gcc's gcov_exit. In short, we should keep it as it is.

Sorry, what I meant is that we could remove llvm_gcov_writeout and only keep llvm_gcov_flush (they are defined in lib/Transforms/Instrumentation/GCOVProfiling.cpp and passed to llvm_gcov_init). We currently have writeout functions and flush functions, where the difference is just that the flush function is also resetting the counters to 0.
At exit, instead of calling llvm_gcov_writeout, we could call llvm_gcov_flush. Since we are at exit, the fact that we reset the counters to 0 for the current module doesn't matter (as the module is not usable anymore after that).
At exit we would still call llvm_gcov_flush only for the module that is exiting, at gcov_flush we would call __llvm_gcov_flush for all modules.

Regarding llvm_gcov_flush that was recently added, should I remove it since we can now unhide __gcov_flush?

I suggest we keep it -- there is no harm keeping it. llvm_gcov_flush can be documented to be always non hidden. There might also be a use case that user want to discover the dumper using dlsym, but only want to dump profile for that shared library. In that case, a symbol with protected visibility is needed which is a wrapper to the write-out method.

Should we make llvm_gcov_flush hidden and gcov_flush unhidden then? gcov_flush unhidden would be for compatibility with GCC.

This is really cool. I just had one minor suggestion for testing, and will otherwise defer to the ARM64-related folks to accept the patch. The rest looks quite reasonable to me, though.

LGTM. Thanks for making the checking more clear. This should help bridge the gap for now, so that we can resume getting per-library profile data in Android.

Thanks for picking this up again and updating the change to add llvm_gcov_flush().

In D48459#1143012, @smeenai wrote:

Actually, I would imagine that if you're cross-compiling or using a custom sysroot, you should probably also specify CMAKE_FIND_ROOT_PATH and set CMAKE_FIND_ROOT_PATH_MODE_PACKAGE to ONLY, to limit all these searches to the desired directories?

(I'm actually playing around with a bunch of this stuff myself right now. I'm curious about your CMake setup for a custom sysroot or cross-compilation and what issues you've come across.)

In D48580#1144224, @efriedma wrote:

x1-x7 are argument registers in the calling convention; what's supposed to happen if there's a call in the code?

I can see two possibilities:

1. We emit an error.
2. We change the calling convention so it doesn't use the reserved register.
   
   Your patch implements neither of these choices; instead, it will just miscompile.

In 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.

Agree. This is the first time anyone is linking against static sanitizers on Android, so this is just something that we missed updating in the past.

Rui, I added you to this review (and the other corresponding one), as you reviewed ecbeckmann's original work here. Can you take a quick look at these? Thanks.