- User Since
- Aug 23 2012, 7:46 PM (351 w, 2 d)
Fri, May 17
The revert is fine with me, although I still would like Ryan to follow up on verifying that the suggested fix for Bionic will indeed work for all cases. I think it is fine for you to merge now to unbreak things for everyone else though. Thanks for working through the problem.
This LGTM if Ryan is happy with it. Thanks for taking care of getting a workaround implemented until this can be fixed.
Thu, May 16
Tue, Apr 30
Fri, Apr 26
I had just a few small suggestions to make things more consistent. Feel free to ignore my fallthrough comment, but I feel like -Wimplicit-fallthrough has been quite valuable in Android.
Sat, Apr 20
I added Peter and Oliver since they might have some suggestions here too.
Apr 17 2019
Thanks for taking a look at this.
Apr 16 2019
Thanks for finding this. Note that this is the exact code in comparesf2.c.
Apr 11 2019
Apr 4 2019
Thank you for fixing this quickly!
Apr 3 2019
Apr 1 2019
In addition to -fno-everything, don't forget about -fnothing and -fno-nothing, which seem like they would also be nice to have.
Mar 29 2019
Mar 25 2019
@jmorse can I get your approval on this CL before we submit it? Thanks.
https://reviews.llvm.org/D59807 has the fix for the nondeterminism. Thanks for the hints about the reverse iteration order. That helps make it more likely to fail deterministically, but it still runs a small chance of "passing" if the fix isn't there.
Switching it to a MapVector makes it deterministic. I was just struggling late last night to try to reduce the .ll file down to something worth checking in! If you want to take over to minimize the test case, that would be much appreciated. Let me know so I can continue doing that if needed. Thanks.
Mar 23 2019
This patch appears to generate non-reproducible builds in some cases. I can craft a more minimal test case, but the following link (https://godbolt.org/z/sWucUZ) is what I have been using. If I run Clang multiple times, the output eventually swaps the order of some undef DEBUG_VALUE's. I am just passing "-O1 -g" with that .ii file. It isn't obvious to me yet what is causing this to be unordered/non-deterministic.
Mar 20 2019
Mar 13 2019
Mar 7 2019
Feb 20 2019
Dan, this seems pretty important for the NDK. If you submit this, would you want it cherry-picked ASAP?
Feb 15 2019
I asked Chris to comment here about libunwindstack. Hopefully we can get the problem fixed, but I think this workaround might be necessary for code running on older devices in any case (i.e. NDK apps).
Feb 14 2019
Very cool, thank you for coming up with a great way to improve this case.
Feb 7 2019
Would it be reasonable to have a test for this with perhaps an invalid GCC installation? There is some mock GCC/sysroot testing in https://github.com/llvm/llvm-project/blob/master/clang/test/Driver/android-gcc-toolchain.c and https://github.com/llvm/llvm-project/blob/master/clang/test/Driver/android-ndk-standalone.cpp. I am not sure that it will be easy to trip this same bug that way, but I think it is possible.
LGTM for removing the broken Android builders.
Feb 6 2019
Feb 5 2019
Jan 30 2019
Jan 22 2019
Jan 8 2019
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. :)