- User Since
- Dec 15 2015, 10:55 AM (67 w, 12 h)
@mehdi_amini, we have other places where we call GlobalValue::getGUID with a name that is not an IR name, e.g. in BitcodeReader.cpp. I wouldn't be opposed to moving to a design where it's harder to pass the wrong kind of name, or even a design where the IR name and the RealLinkageName and the GlobalIdentifier aren't all different things, but that is going to be a larger change. Can we get this fix in in the meantime?
Thanks, pcc, for helping debug this, and rnk for the suggestion to use getRealLinkageName to solve it.
Mon, Mar 27
Adding the offset also affects non-LTO archive members, right? If so, I prefer to fix that in a follow up. For this one, I'd like to change ParentName + MB.getBufferIdentifier() to toString(this), which is more consistent with how we form the names in other places. I will also add a test.
Wed, Mar 22
Tue, Mar 21
added comment, as suggested by @davidxl
@rnk, thanks for taking a look. I have an alternative implementation that records which flags were added to enable LTO and provides functions to filter those out. That might lead to cleaner command lines. On the other hand, this is simpler and follows what we already do for SANITIZER_COMMON_FLAGS (in compiler-rt/CMakeLists.txt), so I decided to see what people think about this implementation, first.
Thanks for reviewing!
Mon, Mar 20
apply similar change to darwin_test_archs
The "error adding symbols" message means we're still not using the right linker (it comes from ld, in a build where lld should be used as the linker).
With this new diff, check-profile runs the same number of tests as it does in the base revision (102, in my case). Unfortunately, when building with thinlto, they all fail (error message: llvm-build-clang/lib/clang/5.0.0/lib/linux/libclang_rt.profile-i386.a: error adding symbols: File format not recognized).
Changed test_target_arch to pass CMAKE_EXE_LINKER_FLAGS and argstring, which matches what it did before.
I'm seeing 100 tests passed, 2 unsupported without this change, and 50 passed, 1 unsupported with the change. That doesn't seem right.
Thanks, davidxl. With your command line I do get the check-profile target.
I configured the build using
Fri, Mar 17
@davidxl: My ninja doesn't know about check-profile. Did you mean check-clang-profile? That gives me 32 tests with and without this change.
clang-format done right and removed NoSymbols local
Some more background on this change:
@beanz, @davidxl : It isn't clear to me from the comment that was in CMakeLists.txt what exact problem CMP0056 NEW caused with test_target_arch, so I'm not completely sure what I did avoids the problem. Could one of you check if my change seems ok or if there is a better way?
Thu, Mar 16
Yes, I'll check Chromium builds once we're happy with the implementation.
Wed, Mar 15
@ruiu, how is this?
@ruiu: Yes, that would be a lot simpler. Let me explain why I did it the way I did it, and then I'll be happy to simplify it if you still feel that's better.
Thanks for the clarifications! lgtm
Thanks for making it easy to write minimal PDBs! I added a couple of inline comments about the code.
Tue, Mar 7
Fixing the other issues in a follow-up seems fine. This lgtm.
Mon, Mar 6
Fri, Mar 3
Checking for linux when really you want to check for ELF doesn't seem right. In this case, I think there is an better way to do it; instead of relying on llvm-objdump, could you emit an LLVM assembly file and check that for presence of the string you want? I think if you compile with clang -g -S -emit-llvm, it will give you LLVM assembly with metadata for the records you need and you won't need to generate an object file.
Wed, Mar 1
Tue, Feb 28
refactored check for lld-link to be more explicit and less confusing
Mon, Feb 27
added error message for when unsupported combination of platform, LTO, and linker is used
This causes us to eliminate less duplication, right? Is folding of identical non-functions something we may want to re-enable at a later point? Should we make the behavior controllable by a flag - or perhaps only implement the MSVC-compatible behavior when using msvclto?
Feb 23 2017
changed error message
fail early with a friendlier message when using -flto without -fuse-ld=lld
added missing --
Implemented @hans's suggestion of moving the tests into cl-options.c.
check for absence of undefined foo, instead of presence of main
use llvm-nm instead of looking at the bitcode
Nit: I'd use llvm-nm to check the symbol tables of the object files so that we're checking the inputs that the linker actually reads, rather than a byproduct of LTO.
Feb 22 2017
@pcc: How is this instead?
Is clang-cl using lld as default? How is the switch done? Ideally we should have a nice error message from the driver if -flto is used without lld.
Feb 21 2017
Feb 17 2017
changes requested by @ruiu
With this change, LLVM, Clang, and lld compile using LTO and pass tests on Windows. Without the change, the tests fail to compile ("Associative COMDAT symbol ... does not exist", https://bugs.llvm.org//show_bug.cgi?id=31974).
Feb 9 2017
use a subdirectory
How about creating a temporary subdirectory (e.g. %T/tmp/) and remove that directory entirely?
Feb 8 2017
Feb 7 2017
Including minwindef.h makes the compiler very unhappy, complaining about a lot of undefined types and identifiers. I guess if we wanted to include something, we would have to include something much larger. I think there is a good argument to be made to just define the one macro we care about here. Ok to ship as-is?