- User Since
- Aug 19 2016, 10:21 AM (65 w, 2 d)
Fri, Nov 17
Ah all right, that makes sense then.
What are the advantages to the section merging, apart from matching link's ouput? I personally prefer LLD keeping the separate sections, since it's easier to examine them that way.
Tue, Nov 14
Mon, Nov 13
It wasn't automatically closed because the commit (https://reviews.llvm.org/rL317960) referenced the wrong differential in its Differential Revision field. Not sure how that happened.
Sun, Nov 12
Thu, Nov 9
Wed, Nov 8
Thanks for productionizing this! It'll be awesome to have it in-tree.
Tue, Nov 7
This seems fine as far as avoiding the warnings themselves go, but I'm not familiar enough with install_name and rpath on macOS to know if the new policy behavior is doing the right thing.
Fri, Nov 3
Thu, Nov 2
Rename from CREATE to INSTALL
I'm wondering if LLVM_INSTALL_BINUTILS_SYMLINKS would be a better name. I went with "create" instead of "install" initially because this option will create the symlinks in the build tree in addition to the install tree, and "install" doesn't convey that fully, but I think most people would care a lot more about the installation part.
Add option and documentation
GNU binutils nm also flags all of the .idata$X chunks as 'i' (while this patch only makes it set on .idata$2 and .idata$6) and also flags impfunction as 'I'.
I'm not super familiar with the option parser by any means, but I think changing specific flags to be ZeroOrMore rather than Optional is much better than doing a global change for the behavior of Optional.
Not sure of a better implementation strategy, but I am a fan of adding support for the dashed variants, since I've hit the issue as well in the past.
lld-link will include debug info sections in the linked binary if you pass -debug (and you probably also want to pass -nopdb to prevent PDB emission if you're using DWARF). Perhaps you should make the MinGW driver pass those two options by default and then not pass them if -s is passed to it?
@jakehehrlich I see you got that Herald rule set up :)
Wed, Nov 1
Also title says -fused-ld instead of -fuse-ld
Tue, Oct 31
This solves the issue in D39149, at least.
I confirmed that these warnings go away with D39462 applied, and they reappear if I manually specify -Wmaybe-tautological-constant-compare. Thank you!
Mon, Oct 30
I'm thinking you could account for all possible type sizes in the existing (enabled by default) warning, and have a different warning for possibly tautological comparisons. E.g. if a long is being compared against INT_MAX, you know that's only tautological on some platforms, so it should go under -Wpossible-tautological-constant-compare (which would only be enabled by -Weverything and not -Wall or -Wextra); -Wtautological-constant-compare should be reserved for definitely tautological cases.
Sun, Oct 29
I assume a corresponding change will also be made for COFF afterward?
Fri, Oct 27
Like I said, not a huge fan of this change, but also not a huge fan of running into clang diagnostics (especially since we build with -Werror downstream).
When you say "latest Windows SDK headers", do you mean SDK version 10.0.16299.15?
Thank you for being flexible on these, @ruiu! It makes our life a lot easier downstream :)
Thu, Oct 26
And lld currently silently mislinks inputs with .ctors sections
Wed, Oct 25
At the developer meeting @peter.smith had said the thunks implementation could be moved out into its own file provided one relocation function was available to it. If it's easy enough to do, perhaps we want to do that before landing, to alleviate the concerns around the relocations code becoming even more complicated?
Tue, Oct 24
Mon, Oct 23
Rebase atop D39216
https://reviews.llvm.org/rL316365 does this already.
BTW LLVM requires a minimum of Windows 7 according to https://releases.llvm.org/3.8.0/docs/ReleaseNotes.html, so you can rely on SRW locks always being present. If LLVM still has a fallback path for when SRW locks are absent, that could be cleaned up.
The grouping by import library is definitely important for both Microsoft-style and binutils-style import libraries. I'm pretty sure the second issue (how to get the correct ILT and IAT start addresses inside d000000.o) is solved by them special casing imports handling, as I mentioned before.
Oct 20 2017
Sorry, this has been on my queue for a long time, but I haven't gotten the chance to get to it yet. I'll try to take a look (at this and your other patches) in the next few days.
Oct 16 2017
Oct 15 2017
You can use /Gy (or -ffunction-sections if you're not using the cl-style driver) to force each function into its own COMDAT section.
Oct 13 2017
Oct 12 2017
I looked into this some more. MinGW import libraries don't appear to contain the null directory table entry that's supposed to terminate the import directory table. This doesn't cause usually cause problems for link.exe, because you'll probably be linking against a non-MinGW import library, which provides the null entry (as .idata$3). Even if you're only linking against a MinGW import library, it looks like the loader doesn't require the entire terminating entry to be null; if either the Name RVA or the IAT RVA is null, the entry is considered to be a terminator. You can craft examples which cause problems though. All of the below discussion assumes 32-bit VS tools and MinGW, for reasons that will become apparent later.
I can commit it for you, but I remembered that this should probably include a test case, assuming that there are tests set up for this functionality (and I hope there are).
I'd wait for @compnerd to take a look before acting on my comments, since I don't have much context on this code.
Looks like a pretty simple and obvious fix.
(as in, create a new differential. Adding llvm-commits to this one won't trigger an email with the patch contents, I believe.)
Looks like a pretty small and obvious fix to me. However, you should reupload this patch and add llvm-commits as a subscriber, so that an email gets sent out to the mailing list.
Also adding more Windows people in case they have any ideas.
Thanks for the detailed explanation! I'm pretty familiar with how short import libraries work, but I hadn't looked into MinGW-style import libraries too much before.
Oct 11 2017
Could you elaborate on "the first object file in the static import library to be included is the function itself"? Is the function in question the linker-provided import thunk, or something else?
Oct 9 2017
I spoke to @EricWF on IRC last week and got his approval to commit this without review if no one had gotten to it in a couple of days, so I'm going ahead with that.
Oct 6 2017
I'd been meaning to complain about lld-link outputting "warning" instead of "error" for undefined symbols. Thanks for fixing :)
Oct 5 2017
Does dlopen cause issues even with RTLD_GLOBAL?
Context is missing again.
Oct 4 2017
@rjmccall, this adds a libc++ build dependency on LLVM's cmake modules. There were some issues when @zturner had added a similar dependency for his work on lit. To confirm, Apple still cares about the use case of building libc++ without having any LLVM cmake modules available (either from source or from an LLVM installation), right?