- User Since
- Aug 23 2012, 7:46 PM (421 w, 2 d)
Tue, Sep 15
Fri, Sep 11
Thanks for creating the new test, and for making this more flexible. Everything else looks good here.
Thu, Sep 10
Wed, Sep 9
Sorry, Jiyong, I had forgotten to subscribe llvm-commits when I first uploaded this (because it's part of compiler-rt and doesn't seem to have a default rule to add them). In any case, this patch LGTM, and I am going to go ahead and approve it since this only affects Android. Let's give it 24 hours for anyone else to comment before committing though (especially since I messed up with the original upload/review).
Thu, Sep 3
Wed, Aug 26
Mon, Aug 24
Yes, let's just submit this now. If it breaks further, then we can consider other options. I'm not sure why there is a separate libc++abi reviewer listed here.
Aug 14 2020
@asmith, are you still working on this patch? The gap-fill feature is definitely needed for parity with existing objcopy implementations.
Jul 13 2020
Jun 15 2020
Jun 10 2020
Thanks, Ryan, for diagnosing and addressing this bug.
Jun 1 2020
Thanks, Nick, for cleaning this up and always striving to make things more compatible.
May 29 2020
Apr 30 2020
I'm also satisfied with this from the Android side. Thank you for discussing this with Ryan.
Apr 24 2020
LGTM on the Android side. Thanks Dan for handling this.
Apr 22 2020
Apr 16 2020
pragma clang attribute is interesting, but how do you apply that in a selective fashion to local variables (especially in a way that can be automated)? At first, I didn't think the goal for this should be to create a frequently used option for most end users, but I do think that it could be quite useful for more folks when debugging, especially if it is easy to automate (which optimization-fuel approaches are, while pragmas are not).
Apr 8 2020
Thank you for making this clear.
I agree with Ryan. Please update the Clang driver for this to pass this same flag for 32-bit ARM as we already pass for AArch64.
Apr 3 2020
I should have looked more closely at the AOSP sources, because we already hard-code the expected 4KB max-page-size. We do have some crufty flags that can be cleaned up, so I'll do that instead (https://android-review.googlesource.com/c/platform/build/soong/+/1278815). :) I'll also go ahead and create/test a CL to revert the Clang driver change forcing 4KB max-page-size next week.
So it turns out that the ld shipped in the NDK actually did have the 64KB max-page-size patch, so not taking this patch would change current user behavior. Thanks to @MaskRay for sharing the exact binutils patch we were looking for to verify that it has it. For that reason, this patch should be fine for our 32-bit ARM NDK users. For the platform, we'll see if we should just set the max-page-size back to 4KB explicitly in the build rules, but that shouldn't affect this CL.
We (Android Google folks) have an internal thread going on about what implications this might have, but I can say that I am concerned about making this kind of change for ARM Android targets. Even though we manually set a 4KB max page size in our platform builds for 32-bit, we also have to consider the needs of NDK developers (i.e. those folks making native applications/games/etc.). Some of their build configurations might behave very differently with this change, so my current thinking is that 64KB should not be the default for ARM Android targets.
Apr 1 2020
Mar 31 2020
Mar 30 2020
Mar 26 2020
Mar 20 2020
Thanks, Dan, for setting this up.
Feb 27 2020
Feb 14 2020
Dec 10 2019
Nov 6 2019
Oct 25 2019
Oct 17 2019
Oct 4 2019
Sep 9 2019
Sep 5 2019
Looks really nice. I am sure the NDK developers will be happy to see support for static OpenMP. Do you want to add the public NDK github issue link in the commit message?
Sep 4 2019
LGTM, but please wait for someone more familiar with LangRef policies to review that part.
Aug 29 2019
Aug 26 2019
Thanks for improving this for LLDB and Android.
Aug 6 2019
Jul 12 2019
You have patch.patch in the diff above. You should drop that.
s/integrates/integrated in the title, but otherwise this looks good.
Jul 3 2019
Jul 2 2019
Jun 25 2019
Craig, can you confirm that this is acceptable? I don't think there are any chips with SSE4.2 but without cx16, so this just seemed like an oversight. It might be a good idea to really audit the list of possible CPU features for other missing inclusions. Another idea would be to set a baseline minimum CPU for Android, which would cut down on having to specify so many features separately.
Jun 3 2019
FYI, https://reviews.llvm.org/D56571 is the review where this was accepted.
May 31 2019
May 30 2019
May 29 2019
Everything looks great. Thanks for adding these improvements. While it's probably safe to commit this, perhaps you should give it 24 hours in case some of the other clang-tidy folks have different style or testing concerns.
May 23 2019
May 21 2019
Thanks for picking this up and finishing it.
May 17 2019
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.
May 16 2019
Apr 30 2019
Apr 26 2019
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.
Apr 20 2019
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.