User Details
- User Since
- Aug 19 2016, 10:21 AM (343 w, 6 d)
Yesterday
Fair enough; I'm convinced :) LGTM
Wed, Mar 22
I'd need to learn more about the use case (I see you're awaiting a response on the issue), but I'm not a huge fan personally; it seems really weird to not be going through the standard CMake mechanisms for cross-compilation (setting a CMAKE_SYSTEM_NAME), and if you do that then this should work as expected. We should understand why LLVM_USE_HOST_TOOLS isn't being set automatically.
Tue, Mar 21
Fri, Mar 17
D146227 is a fix diff that's up and waiting for CI.
Thu, Mar 16
Fri, Mar 10
LGTM
Note that we aren't using toString(InputFile*) here because it
includes the install name for dylibs in its output, and ld64's map file
does not contain those.
LGTM
Thu, Mar 9
Heh, "target" being an extremely overloaded term strikes again :)
Wed, Mar 8
Okay, -print-before-all did the trick, and the local variables it's complaining about are from https://github.com/llvm/llvm-project/blob/08b65c5c9e58a4f9e79a52a0fccd70e4c4ebd43b/libcxx/include/__memory/uninitialized_algorithms.h#L520. I guess we have some ODR issue with the specific types that template is being instantiated with (reverse_iterator<reverse_iterator<internal_type*>>) that's being exposed now?
I'm still seeing LTO errors after this patch:
fragment covers entire variable call void @llvm.dbg.value(metadata i64 poison, metadata !798696, metadata !DIExpression(DW_OP_LLVM_fragment, 0, 64)), !dbg !798698 !798696 = !DILocalVariable(name: "__first", arg: 2, scope: !798693, file: !16879, line: 520, type: !159997) fragment is larger than or outside of variable call void @llvm.dbg.value(metadata ptr %5, metadata !798696, metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !798698 !798696 = !DILocalVariable(name: "__first", arg: 2, scope: !798693, file: !16879, line: 520, type: !159997) fragment covers entire variable call void @llvm.dbg.value(metadata i64 poison, metadata !798697, metadata !DIExpression(DW_OP_LLVM_fragment, 0, 64)), !dbg !798698 !798697 = !DILocalVariable(name: "__last", arg: 3, scope: !798693, file: !16879, line: 520, type: !159997) fragment is larger than or outside of variable call void @llvm.dbg.value(metadata ptr %7, metadata !798697, metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !798698 !798697 = !DILocalVariable(name: "__last", arg: 3, scope: !798693, file: !16879, line: 520, type: !159997) fragment is larger than or outside of variable call void @llvm.dbg.value(metadata ptr %9, metadata !798696, metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !798698 !798696 = !DILocalVariable(name: "__first", arg: 2, scope: !798693, file: !16879, line: 520, type: !159997) fragment is larger than or outside of variable call void @llvm.dbg.value(metadata ptr %12, metadata !798696, metadata !DIExpression(DW_OP_LLVM_fragment, 64, 64)), !dbg !798698 !798696 = !DILocalVariable(name: "__first", arg: 2, scope: !798693, file: !16879, line: 520, type: !159997) LLVM ERROR: Broken module found, compilation aborted!
One other alternative for Rust to consider would be to support building as part of an LLVM build using LLVM_EXTERNAL_PROJECTS and LLVM_EXTERNAL_<PROJECT>_SOURCE_DIR, so that Rust becomes part of the LLVM build graph and can easily access its targets. See https://github.com/apple/swift/blob/c95e11c011e5e6ee19f9c21332ff04df29dd49db/cmake/modules/SwiftSharedCMakeConfig.cmake#L269 for how Swift does it.
Adding the usual CMake reviewers.
Tue, Mar 7
LGTM
Fri, Mar 3
LGTM
Thu, Mar 2
Wed, Mar 1
Hmm, what cache key does ccache use for compilers? Is it the --version output string, the path, or some combination? If the path is involved then I don't see any value in using ccache for configuring anything past stage1, since the compiler will (presumably) be installed somewhere else afterwards and not used directly from the build tree. If it's just --version, what are the cache pollution concerns? The compiler binaries output by all the stages should produce the same outputs given the same inputs (and presumably have the same --version), right?
Tue, Feb 28
Rename per review comment
Fri, Feb 24
Ping @ldionne, would you be able to take a look at this soon (or are you okay waiving the libc++ blocking requirement for it)? This is really useful for Android armv7, where the triple is traditionally spelled armv7-none-linux-androideabi but normalized to arvm7-none-linux-android, and this patch would resolve that discrepancy.
Wed, Feb 22
Ping @MaskRay
Feb 16 2023
it's just a matter of replacing -I with -isystem to get the correct behaviour
Feb 13 2023
Jan 10 2023
@tamas' suggestion would be a good change to make IMO, but I think it's outside the scope of this patch, and the patch as-is improves the status quo, so LGTM.
@MaskRay, were you still planning to review this/do you have any unaddressed concerns?
Jan 9 2023
Ah, I didn't consider the case where you were using LLVM_RUNTIME_TARGETS but not LLVM_BUILTIN_TARGETS. Thanks.
Jan 7 2023
LGTM, thank you!
Jan 6 2023
I'm investigating a size increase we observed after this change for Meta's Android apps. This increases the size of compiler-rt by 1.6 KB, which is small by itself, but then compiler-rt is statically linked into each SO, and some of our apps have hundreds of SOs, so the increase adds up to a sizeable total. (We would ideally have far fewer SOs, but that's a pretty involved change.)
Jan 5 2023
Would there be any objections to adding a flag to restore the previous behavior? This is a fairly hefty size increase for us, and we don't think the additional checking is worth the size increase in our case. We also intend to look into the size increases more to determine how much is from guarding calls to e.g. __cxa_throw, where you will reuse the stack frame, vs. calls to e.g. abort or _Unwind_Resume, where the stack frame should never be reused and checking the stack cookie doesn't seem worthwhile (if my logic is sound, which I'm not 100% sure of).
Jan 4 2023
Address comment
Dec 13 2022
Rebase to see if CI is happier
Dec 12 2022
Dec 11 2022
Pinging the libc++abi reviewer group.
Dec 9 2022
Dec 7 2022
The size regressions from this are acceptably small for us, so we have no problems with that. Thanks for flagging it.
I'm not able to apply the latest version of the patch, and it looks like CI isn't either. Which revision is this patch based on top of?
Dec 6 2022
I'll be going on an extended vacation soon. If anyone needs this sooner, they should feel free to pick it up, otherwise I'll get to it when I'm back in February. (Similarly, there's no rush on our end for you to figure out the platform issues you observed.)
Dec 5 2022
LGTM
Dec 2 2022
Ah, I was hoping I'd be able to rebase the current version onto the 15.x release branch cleanly, but I'm running into conflicts that I'm not sure how to resolve. I'll wait for the rebased version and then try that.
Dec 1 2022
Thanks for looking into the APK size impact. Could you give me a couple of days (I'll try my best to get to this tomorrow) to evaluate this on Meta's Android apps as well? I maintain the libc++ used by those, and we have some apps that are incredibly sensitive to APK size (increases of even a few KB are flagged), so I'd appreciate the chance to evaluate this before it lands. (If I haven't been able to get to this by Wednesday of next week, feel free to go ahead regardless.)
Nov 30 2022
I think that'd be pretty nice. Right now there's a bunch of different and inconsistent ways to control which parts of compiler-rt you build (e.g. the sanitizers have COMPILER_RT_SANITIZERS_TO_BUILD, but then things like XRay and libfuzzer have dedicated COMPILER_RT_BUILD_* variables), and it'd be great to have a consistent interface instead. I know @compnerd and some others have been interested in reorganizing compiler-rt, and this could be part of that.
CI is happy now. Could someone from the libc++abi group please take a look?
Nov 29 2022
Pausing this while D138461 is sorted out.
Hopefully make CI happy this time
LGTM
Might be worth a mention in either https://llvm.org/docs/AdvancedBuilds.html#multi-stage-pgo, to make it a bit more discoverable.
Nov 28 2022
Rebase and synchronize threads to avoid duplicates
This seems like a pretty simple and non-intrusive method to be able to tune the outliner. LGTM if you add a test case.
Thanks for the revert.
Nov 23 2022
It's not. I'm out for a few days; could you revert this for me please?
Nov 22 2022
Rebase