- User Since
- Aug 23 2012, 7:46 PM (333 w, 6 d)
Tue, Jan 8
Sorry about the delay.
Dec 11 2018
Dec 7 2018
Dec 3 2018
Craig, does this look ok now?
Nov 30 2018
Nov 28 2018
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.
Nov 16 2018
I just wanted to follow up here.
Nov 1 2018
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.
Oct 26 2018
Oct 25 2018
LGTM, but please wait for dberris approval.
Oct 24 2018
Oct 22 2018
Oct 19 2018
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.
Oct 10 2018
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.
Oct 8 2018
Oct 2 2018
Sep 20 2018
Sep 19 2018
+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.
Sep 18 2018
Thanks @dblaikie for the quick fixup. I must have accidentally dropped the '!', because I did run check-all to test the change.
Sep 17 2018
+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:
Sep 13 2018
Sep 12 2018
Sep 7 2018
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.
Sep 5 2018
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.
Aug 31 2018
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. :)
Aug 30 2018
Aug 28 2018
Sorry, this got buried in my inbox back in April right before EuroLLVM.
Aug 21 2018
Jul 26 2018
Jul 24 2018
Definitely deal with the submodule stuff. We don't want a new file checked in for tools/clang.
Jul 20 2018
I restored the original change, since I had confused things with arcanist before. PTAL
Revert back to not layering the positional argument change.
Jul 19 2018
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.
Jul 12 2018
Jul 10 2018
Thanks for the quick review.
https://reviews.llvm.org/D47100 is the commit where this got dropped.
Jul 6 2018
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.
Jul 3 2018
Jun 29 2018
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().
Jun 26 2018
Jun 25 2018
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.