- User Since
- Apr 18 2013, 12:16 AM (339 w, 4 d)
Fri, Oct 18
Ah, that's true. How about something like "Used if DWARF debug info is used (which is not common)"?
Thu, Oct 17
I applied this patch and built clang with ThinLTO. Here is the generated file:
Code-wise, it's quite a bit of code for dwarf debug info which isn't really used all that much, but meh. I kind of feel like it might possibly make sense to move all the CoffMingw bits into a separate lld/ directory instead of having it in lld/COFF since it's lots of special purpose code. But that's not my call :)
I think some parts of the verbose dwarf code here could be refactored into lld/Common and share it with the ELF linker, but I'd prefer doing that as a separate step after landing this if that's tolerable (as some refactorings easily end up dragging on longer than initially expected). For other mingw related bits, we do have COFF/MinGW.cpp for logic that is easily decoupled from the rest of the COFF logic.
This seems useful. Can I see an example output?
Wed, Oct 16
Nico, are you happy with this change?
Thank you for doing this!
Tue, Oct 15
This patch looks good to me, but I'd like to get a second opinion from other devs because this could be a big change in UX. Nico, you recently worked on error messages. What do you think of this change?
Do you want to land this or abandon? I think I'm fine either relying on a compiler magic or landing this patch (and remove the compiler hack), but I'm not very happy about landing this and leaving the compiler hack, so I guess you need to make your mind.
It seems that the patch description is too terse. Please explain the bug that this patch is fixing in the commit message.
Thu, Oct 10
As jhenderson suggested, could you add a description about this behavior? Looks like llvm-ar doesn't have a manual page, so the second best thing to add a description is probably the help message for --help.
- address review comments
- merged with a existing test file
Using error instead of fatal is not a high priority matter for the MinGW driver because I believe users are using this driver as a command. But still it is better to use error because it is more like a contract as a library. So, can you make a follow-up change?
Wed, Oct 9
I'm not sure if there's a standard for the wasm object files and how the linker works, but if there's any, could you add a pointer to the document to the comment?
LGTM with this change.
Tue, Oct 8
OK, I'll just apply your change and submit. Thanks!
Is it OK to drop -debug option from the lld-link command line? I think we don't need that option there.
Build reproducibility (a property that if you feed the exact same inputs to compilers or linkers you can get the exact same output) is important. Unfrotunately, by default, COFF has a timestamp field that changes virtually every time, but it looks like the difference between you and me is a difference how things are laid out in the file. That is not supposed to vary.
It looks like the same test fails on Windows with the same output, so I'll fix it to match the actual output and submit. Does this sound OK?
The test failed on my Linux box with the following message. Is that expected?
- report error instead of ignoring -export-dynamic
Mon, Oct 7
So, you are creating an executable that has an import table that has only ordinals. Is my understanding correct? Indeed, that condition is rare and I've thought of that case before when I implemented this part.
I think you can de-template some functions now, but that can be done in a follow-up patch.
Fri, Oct 4
Generally looking good.
LGTM but I'd like to know what rnk thinks about this change.
I think I prefer not to remove the leading underscore for i386 because as you wrote it makes the output ambiguous.
If you are OK with the other patches, please stamp.
Thu, Oct 3
How about always using elf::write32 instead of endian::write32<E>?
Yeah, I'm fine with quoting the entire file with the namespaces, but I'm not entirely happy about adding endian:: to read32. Looks like the problem is that we use the same name for two functions. Can you just rename one to avoid the name collision? Does that make the code look simpler?