LGTM

LGTM

Ah, that's true. How about something like "Used if DWARF debug info is used (which is not common)"?

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?

LGTM

Nico, are you happy with this change?

LGTM

LGTM

LGTM

LGTM

Thank you for doing this!

LGTM

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?

LGTM

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.

LGTM

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.

LGTM

• 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?

LGTM

LGTM

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.

LGTM

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?

LGTM

• report error instead of ignoring -export-dynamic

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.

LGTM

I think you can de-template some functions now, but that can be done in a follow-up patch.

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.

Yes, please take a look at https://reviews.llvm.org/D68381 and https://reviews.llvm.org/D68378.

In D68323#1694097, @MaskRay wrote:

In D68323#1694096, @ruiu wrote:

In D68323#1694079, @MaskRay wrote:

In D68323#1694050, @ruiu wrote:

How about always using elf::write32 instead of endian::write32<E>?

lld::elf::* are more commonly used in lld. This will end up with a lot of qualified write32 read16 calls. I think we should reduce the number of using namespace llvm::support::endian;.

I mean we can replace all occurrences of endian::write32<E>() with write32(). It looks like lld still compiles after I run the following command to replace them.

sed -i 's/write32<.>/write32/g' lld/ELF/Arch/Mips.cpp

This will make the generated code slightly worse because the implementation tests config->endianness. llvm::support::endian::write16(p, v, config->endianness);

Personally I prefer keeping the templated version, though I don't mind changing it if you are strong about this or @atanasyan is fine with lld::elf::* functions.

In D68323#1694079, @MaskRay wrote:

In D68323#1694050, @ruiu wrote:

How about always using elf::write32 instead of endian::write32<E>?

lld::elf::* are more commonly used in lld. This will end up with a lot of qualified write32 read16 calls. I think we should reduce the number of using namespace llvm::support::endian;.

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?