Page MenuHomePhabricator

[COFF] Add libcall symbols to the link when LTO is being used.
Needs RevisionPublic

Authored by akhuang on Aug 16 2019, 10:33 AM.

Details

Reviewers
rnk
Summary

If a library function is defined in a bitcode file in an archive
member, add the archive members to the link before LTO, since LTO may
create references to the library functions.

Similar to the change for ELF (https://reviews.llvm.org/D50017).

This is a fix for https://bugs.llvm.org/show_bug.cgi?id=42626#c3.

Event Timeline

akhuang created this revision.Aug 16 2019, 10:33 AM
pcc added a subscriber: pcc.Aug 16 2019, 10:42 AM
pcc added inline comments.
lld/COFF/SymbolTable.cpp
588

The logic here should depend on the name. If it begins with \01 use find on name.substr(1), otherwise use findUnderscore.

595

In D50475 we limited this to symbols defined in bitcode. We should do the same for COFF.

lld/test/COFF/libcall-archive.ll
11

I think this isn't actually testing for what you expect here because you're looking at the LTO output file, which won't contain libcall-archive.s since that isn't a bitcode file. Maybe you could test this by running FileCheck on the map file (/lldmap: output).

rnk added inline comments.Aug 16 2019, 11:33 AM
lld/COFF/SymbolTable.cpp
588

So far as I can tell, none of the symbols from RuntimeLibcalls use the \01 escape prefix. Is that necessary? Can this be simplified to just one call to findUnderscore?

595

Yep, agreed.

lld/test/COFF/libcall-archive.ll
11

Right, you would have to look at %t.exe directly, and that usually doesn't have symbols usable with llvm-nm. The map file seems like a good solution.

pcc added inline comments.Aug 16 2019, 12:08 PM
lld/COFF/SymbolTable.cpp
588

I vaguely recalled that we had some, but yes, apparently we don't. Simplifying to findUnderscore sounds good then.

akhuang updated this revision to Diff 215662.Aug 16 2019, 1:15 PM
akhuang marked 3 inline comments as done.
  • Check that file is bitcode
  • Change test to look at lld map
rnk added inline comments.Aug 16 2019, 2:03 PM
lld/COFF/Driver.cpp
1780

I think this should be inside the do / while loop above in case one of the bitcode objects that provides a libcall introduces a new undefined symbol reference.

lld/COFF/SymbolTable.cpp
588

I think you should be able to simplify this down to just findUnderscore, no need to check for startswith("_"). I think the reason you have it is that in your test case, the .s file would normally use a "mangled" symbol for i686-windows-msvc, so it would provide the symbol __sync_val_compare_and_swap_8.

akhuang updated this revision to Diff 215679.Aug 16 2019, 2:36 PM
akhuang marked an inline comment as done.
  • Move libcall check inside while loop
  • Remove find()
akhuang marked an inline comment as done.Aug 16 2019, 2:37 PM
rnk accepted this revision.Aug 19 2019, 2:28 PM

lgtm with one change

lld/COFF/SymbolTable.cpp
594

I'd use isa<BitcodeFile>(l->getFile()) instead, rather than checking for the magic bytes again.

This revision is now accepted and ready to land.Aug 19 2019, 2:28 PM
akhuang marked an inline comment as done.Aug 19 2019, 3:02 PM
akhuang added inline comments.
lld/COFF/SymbolTable.cpp
594

It seems like isa<BitcodeFile>(l->getFile()) is always false and the file is an ArchiveFile?

rnk requested changes to this revision.Aug 20 2019, 11:35 AM

lgtm

lld/COFF/SymbolTable.cpp
594

I see, you're right. I see this is also what is done for ELF. The whole idea of a lazy symbol is that we may not have loaded the object file yet.

This revision now requires changes to proceed.Aug 20 2019, 11:35 AM
hans added a subscriber: hans.Fri, Aug 23, 3:37 AM
In D66355#1637842, @rnk wrote:

lgtm

You wrote lgtm, but it says changes required. And it looks like it landed in r369694.

Someone asked about merging this to the relaese_90 branch. What do you think?

rnk added a comment.Fri, Aug 23, 12:53 PM
In D66355#1637842, @rnk wrote:

lgtm

You wrote lgtm, but it says changes required. And it looks like it landed in r369694.

Oops.

Someone asked about merging this to the relaese_90 branch. What do you think?

I think we should do it. Ultimately, this is porting some well-tested logic from ELF back to COFF.

hans added a comment.Mon, Aug 26, 1:35 AM

Someone asked about merging this to the relaese_90 branch. What do you think?

I think we should do it. Ultimately, this is porting some well-tested logic from ELF back to COFF.

Sounds good. Merged in r369883.