- User Since
- Aug 19 2016, 10:21 AM (217 w, 3 d)
Do we have any logic to emit an n_strx for an empty string? If so, that would need to be adjusted to emit 1 instead of 0. If not, LGTM.
Ping. Assuming everyone is okay with the direction, any comments on the change itself?
Fri, Oct 16
Both CMake and the driver (assuming it's invoked as clang++/g++) should take care of adding the C++ standard library, so this LGTM.
Thu, Oct 15
Wed, Oct 14
This is needed to avoid potential race conditions where some of the C++ headers have been copied but not all of them, and some other runtime that doesn't depend on the C++ headers sees this incomplete state and gets a spurious build failure, so LGTM.
Address review comments
Tue, Oct 13
Reading up on the Ninja multi-config generator a bit, you can define your own build types, so you should be able to create e.g. one build type for LTO and another for PIC. You'd still need some external logic to define that you use the LTO mode for executables and the PIC mode for libraries, but the multiple distribution support should work nicely in conjunction with that.
Mon, Oct 12
Fri, Oct 9
Tue, Oct 6
LGTM, but I'll give the sanitizer folks a bit of time to review as well.
Mon, Oct 5
I think the reason for this restriction is that on Windows, __declspec(dllimport) changes the compiler's code generation to assume that the symbol will be imported (i.e. if the compiler sees a call to a function X which is marked __declspec(dllimport), it'll generate an indirect call to __imp_X instead). If you're linking against libc++ statically, the imported version of the symbol will of course not exist, so you want to disable the __declspec(dllimport) annotations by default if you're building just the static library for Windows.
Fri, Oct 2
It is not entirely clear whether I use the "ARM64" (Apple) or "AArch64" (non-Apple) naming convention. Guidance is appreciated.
LGTM barring the comment about more tests.
Thu, Oct 1
Wed, Sep 30
Mon, Sep 28
I was abusing this functionality to be able to get the C++ headers installed without having to build all the runtimes build prerequisites; we have a configuration where we only need the C++ headers installed (and not the library), and using the runtime-libcxx-headers was an easy way to get them without needing to first build Clang and all the other tools the runtimes build requires. I should be able to come up with something else for that use case though.
Wed, Sep 23
The LLVMExternalProjectUtils change is needed either way, but I believe the WinMsvc case to be a CMake bug, and I've put up https://gitlab.kitware.com/cmake/cmake/-/merge_requests/5264 to fix it. (We should still go ahead with it as a workaround for now though.)
https://gitlab.kitware.com/cmake/cmake/-/commit/55196a1440e26917d40e6a7a3eb8d9fb323fa657 is the relevant CMake change, and I believe we need to add logic to find llvm-lib. I'll play with that.
It seems strange for CMake to prefer "ar" to "lib" when targeting Windows. Do you happen to know the CMake change that's responsible?
You have a small typo in the commit message: s/ldd/lld/
Tue, Sep 22
LGTM. It's interesting that this is a thing.
The test cleanups are really nice, but I'm not a fan of coding this sort of test-specific behavior (especially the path) into LLD itself :/ (I know we have some precedent with LLD_IN_TEST, but that's only used in canExitEarly and it's a pretty minor behavior change.)
Mon, Sep 21
Sep 17 2020
Sep 16 2020
What was the conclusion for the comments about the priority level (100 vs. 101)?
Sep 9 2020
Sep 8 2020
Aug 28 2020
Aug 27 2020
LGTM; good catch.
Aug 26 2020
LGTM; this is a nice cleanup.
LGTM. Thanks for limiting re-exports to sub-documents in the same TBD!
You're missing a test case.
Aug 25 2020
Aug 24 2020
I think llvm-libtool-darwin would also need some adjustments to handle bitcode objects, archives, and universal binaries. That's definitely out of scope for this diff, but I'm just flagging it as another tool with missing bitcode support.
Aug 21 2020
@hans, would you consider this for 11.0? 11.0 LLD will no longer perform ARM-Thumb interworking for symbols that aren't marked STT_FUNC, so this fixes cases where interworking would incorrectly not occur because of a simple alias.
Aug 20 2020
Check LLVM_RUNTIME_TARGETS as well
Okay, I would argue that this version of the change is much better than the current situation. In particular, it disallows the loophole of specifying a triple for a particular platform instead of "darwin", and it forces users of "darwin" to acknowledge they understand how the Darwin compiler-rt build works by setting a particular variable (and it still disallows multiple Darwin triples). Lemme know what you think.
Add explicit error and flag
Aug 16 2020
rG402b063c8067 should fix it. Sorry for the breakage!
That didn't work, but I'm able to repro locally using the CMake cache file used by the builder, so I'm digging into it.
Aug 15 2020
I pushed rG12b4df991950 as a speculative fix. I'll monitor the bot.