User Details
- User Since
- Aug 23 2012, 7:46 PM (553 w, 36 m)
Wed, Mar 22
Tue, Mar 21
Thank you for taking the time to properly remove this.
You probably need to remove the tests that use these triples (or switch them to a non-Android variant).
Mon, Mar 20
LGTM, but @phosek should also be able to confirm this is ok.
Thu, Mar 16
Feb 14 2023
Thanks for cleaning this up, @kongyi.
Feb 6 2023
Sep 30 2022
Thank you, Kees and reviewers, for helping to make progress here.
Sep 14 2022
Aug 24 2022
Thanks for cleaning this up!
Aug 11 2022
+Dan as FYI, but https://github.com/android/ndk/wiki/Changelog-r24 shows that the NDK has moved forward to at least a minimum target API of 19, so this code is indeed obsolete. We might want to go and update Clang to also forbid setting a target API less than 19 too.
Jun 24 2022
Jun 23 2022
Jun 22 2022
Jun 14 2022
We had some internal Google folks hit this exact issue recently. I don't think that the same "default" CPU should be used for debugging tools like llvm-objdump, and that is the crux of the matter here. Perhaps updating the test to specify "generic" as the CPU when passed to llvm-objdump for those tests is the right pattern? Then the default for disassembly should be to have all attributes/features enabled unless the developer specifies on the command line that they want a different target.
May 31 2022
Mar 15 2022
Looks great. Thanks for noticing and adding this.
Feb 9 2022
Thanks for getting this fixed quickly!
Feb 3 2022
Jan 26 2022
Verified with the docs from musl.
Dec 14 2021
Dec 8 2021
Ryan's comment above is correct that this won't affect the Android 12 platform image. The more important task will be backporting this patch for any necessary NDK, since apps compiled with outlined atomics would start causing problems otherwise. I will take care of that part once this is merged.
Nov 29 2021
Nov 18 2021
Per https://developer.android.com/guide/topics/renderscript/migrate, we are starting to turn things down as part of the deprecation. I reached out internally to the folks helping with that to make sure that we don't cause problems by deleting upstream code. I suspect that we'll be able to make more progress with adapting upstream soon. In the meantime, this CL is fine.
Nov 16 2021
Thanks for the expanded (and more specific) tests.
Aug 5 2021
LGTM from the Android side. Thanks for finding and fixing the original issue, as well as adding this stronger test. :)
Jun 11 2021
Jun 8 2021
Apr 28 2021
Apr 7 2021
Adding Dan as an FYI. While this doesn't impact Android platform or regular NDK users, I suppose that someone could have some esoteric build rules that are relying on this, but they should probably be more explicit about what they're running in those cases anyways.
Mar 19 2021
Mar 6 2021
Mar 4 2021
Feb 26 2021
+2 for relanding this.
Feb 9 2021
Jan 21 2021
Dec 1 2020
Nov 3 2020
Would it be better/easier if we just change the default in the Clang driver at this point to use LLD for Android triples? I believe Dan has patches for doing that which we hadn't submitted yet (because we only recently made it the NDK default).
Oct 22 2020
Oct 21 2020
Oct 20 2020
Oct 13 2020
I think this is ok, but I'd like @eugenis to confirm that he is fine with it too. Thanks for explaining the complexity across the different API levels.
Oct 12 2020
Sep 15 2020
Sep 11 2020
Thanks for creating the new test, and for making this more flexible. Everything else looks good here.
Sep 10 2020
Sep 9 2020
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).
Sep 3 2020
Aug 26 2020
Aug 24 2020
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
Thanks, Ryan!
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.