- User Since
- Aug 20 2014, 4:35 PM (347 w, 43 m)
Sorry for not seeing this. Ignoring macho_embedded is the right call. That is really just used for macho baremetal and not many people outside Apple would bother with it.
Wed, Mar 31
Tue, Mar 30
Mon, Mar 29
Sat, Mar 27
I think this bug is the other way around. LLVMTableGenGlobalISel should not be using DISABLE_LLVM_LINK_LLVM_DYLIB.
Thu, Mar 25
Yes, clang-tablegen could link libLLVM. That said, in the common case of building LLVM & Clang together (which is generally the advised way to develop), depending on libLLVM adds a bunch of extra unneeded dependencies on clang-tablegen, and constrains the build tool’s ability to parallelize builds.
Woo! Thanks for cleaning up one of my old messes :)
You can’t make tablegen link the LLVM dylib since parts of the dylib depend on tablegen.
Wed, Mar 17
The *_LTO and *_LLD variables are usable independently of each other even on non-Apple platforms, so it makes sense to need to set them both. LLVM’s LTO is supported in Gold, and I believe our build system supports MSVC’s LTCG (which is their version of LTO).
Tue, Mar 16
If you add include(CMakeDetermineSystem) to the top of the cache file, it should initialize CMAKE_HOST_SYSTEM_PROCESSOR, which also probably gives you a reasonable value to key off.
I'm super sorry to be popping in so late on this. Is this really the direction we should take?
Mar 15 2021
LGTM. I should have updated that in my patch.
Mar 10 2021
This should not impact the Apple Clang releases since those are always clean builds.
Mar 9 2021
So... this patch is fine. (marked as approved). That said, we should really be working to remove the LLVM_BUILD_EXTERNAL_COMPILER_RT option entirely in favor of just using LLVM_ENABLE_RUNTIMES=compiler-rt. That build process does support invoking the sub-project build on build invocation to make sure builds get re-run, and it supports better understanding of the dependencies between runtimes.
Jan 12 2021
All of my major concerns about adding additional build systems are addressed by the support policy and classifying additional build systems as “peripheral”.
Oct 15 2020
Oct 7 2020
Using lld on Darwin isn't ideal, the system linker is desirable there.
Aug 20 2020
I think this change makes configuration confusingly inaccurate. Since the compiler-rt Darwin build does not support picking and choosing which targets to configure making the configuration fail if a user tries to is the reasonable approach.
Aug 17 2020
Aug 7 2020
Jul 27 2020
The CMake code looks good to me. If @aaron.ballman is on board with it from the Windows perspective it sounds like this is the right fix.
Jul 24 2020
Updated to also pass the linker -object_path_lto
@JDevlieghere you are right, I'm missing the change to how clang determines it needs to pass the linker the object file path. Update coming momentarily.
Fixing bad test case.
The clang fix -> https://reviews.llvm.org/D84565
I was talking to @bogner about this. I think this patch is reasonable, but it is important to note that this patch (and the previous version) are both workarounds for a bug in clang. Anytime the clang driver is provided a flag to generate debug info and it is linking temporary object files, clang should insert a dsymutil step. Right now it only inserts the step if there is a compiler or assembler input to the compiler. I filed a bug to track it (https://bugs.llvm.org/show_bug.cgi?id=46841), and I will upload a patch to the clang driver to fix this issue.
Jul 21 2020
Sadly I think @rnk isn't around. This is something I'd really like him to weigh in on. I assume there was a reason for doing this per-object file rather than for everything, and I don't have the context.
Jul 9 2020
Logically your patch here looks fine. I'd rather see it use a std::lock_guard as the original code did, with a nested scope added, but that is pretty nitpicky.
There are a lot of downsides to global constructors. One is that they are difficult to debug because the code runs before main, so if anything goes wrong callstacks are odd, debuggers sometimes struggle to attach, and general headaches ensue.
Jul 8 2020
Jun 15 2020
It is kinda bad form to merge a change that has unaddressed feedback.
I have several concerns about this.
May 27 2020
May 20 2020
Sorry for being late to the party on this.
Apr 20 2020
I'm not sure this is actually the right approach. Adding the directory's INCLUDE_DIRECTORIES option also adds any path added with include_directories, which includes system paths and other locations that shouldn't ever have tablegen files.
Apr 13 2020
I think this has some unintended consequences. If your tool wants to use libLLVM and libClang you really need libClang to be linked against libLLVM, otherwise you're actually just hiding the problem.
Mar 26 2020
Yea... this is not how plugin linking should work.
Jan 22 2020
Jan 14 2020
Looks totally reasonable to me, and like a good simplification of the logic.
Jan 11 2020
Dec 17 2019
I think this is good, and it has been sitting a while.
Dec 5 2019
Dec 4 2019
I conferred off-list with @compnerd. He and I talked through my concerns, and I believe this patch is fine as-is, so LGTM.
@labath sorry for not replying on-list. I've actually been discussing offline with @compnerd. If llvm-config is printing an absolute path, I'm concerned about how that will interact with binary package distributions for the llvm-dev packages. Not sure if @tstellar has any thoughts on that.
Dec 3 2019
Dec 2 2019
I like the idea behind this. I provided more detailed feedback to Don off-list, but I really don't like the const void* parameter to the callback functions. I think we can make that match the parser data type, and have compile-time errors for type mismatches. That would be vastly preferable to casting void pointers.
Nov 19 2019
Given that, this change LGTM. Using an extra argument would be nice, but this is a macro, not a function, and IIRC CMake argument parsing in macros doesn't really work, so that would be a big change.
When cross compiling any LLVM-based project we do not rely on installed copies of table gen, instead we configure and build host tablegen tools. That functionality in the LLVM build system is flexible enough to support any sub-project that provides its own tablegen tool.
Nov 14 2019
Yea. Unfortunately the migration stalled out because there was no way to make the registration method work with the new pass manager.
Nov 13 2019
This looks like a great cleanup to me.
Nov 11 2019
I very much dislike using EXCLUDE_FROM_ALL. I really believe all should be all, not most things I think are important. In general iteration cycles the check-* targets won't build or link libLLVM unless you specify LLVM_LINK_LLVM_DYLIB, and if you actually want to build everything as a means of testing your changes the all target should do that.
Nov 8 2019
This should only be broken for CMake 3.14 and later. We should include that in the comments around this change.
Reviewing the CMake change more closely, that is a bug. They shouldn't have changed the default behavior in that way. At the very least that change should have been a policy change.
I suspect this behavior change was caused by: https://gitlab.kitware.com/cmake/cmake/commit/b0b820ea39fe4edaf0672ba899a7042ef93a20d2
It is a bit gross that Python does an @rpath install name, that is generally against convention. I can see adding an option to add rpaths to liblldb making sense. I certainly agree LLDB shouldn't be in the business of packaging transitive dependencies.
Nov 6 2019
Nov 5 2019
Yea, that makes sense. Sorry again for the run around.
RPCUtils.h and RPCSerialization.h can both also be excluded.
Adding exclude header "ExecutionEngine/Orc/OrcError.h" to the module specification for LLVM_ExecutionEngine seems like a more appropriate fix.
Sorry, I had missed the atomic use directly in the tool. Sorry for my incorrect statements. This seems like a reasonable fix.
This confuses me. LLVMOrcError only directly depends on LLVMSupport. It doesn't include or need anything from ExecutionEngine.h.
Rebasing and updating to separate arm32 and arm64.
Nov 1 2019
Oct 31 2019
A few comments inline.
Oct 30 2019
Oct 29 2019
LLVM_NO_DEAD_STRIP isn't actually useful only for when plugins are enabled. It is also useful in a lot of contexts around building JIT host processes. The advantage of the old name was it specified exactly what it did (i.e. not dead strip code).