This is an archive of the discontinued LLVM Phabricator instance.

[JITLink][COFF] Handle duplicate external symbols.
ClosedPublic

Authored by sunho on Jul 16 2022, 11:48 AM.

Details

Summary

Handles duplicate external symbols. This happens in few static libraries generaed from msvc toolchain.

Diff Detail

Event Timeline

sunho created this revision.Jul 16 2022, 11:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2022, 11:48 AM
sunho requested review of this revision.Jul 16 2022, 11:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2022, 11:48 AM

Is this happening on a regular basis? Any idea why it happens?

llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp
229

Tracking all symbols names in new collection appears expensive. Could we check LinkGraph::external_symbols() instead?

sunho added a comment.Jul 17 2022, 4:59 PM

@sgraenitz I'm not exactly sure what's going on but this happens a lot when I load up microsoft stl library (msvcp.lib, libcpmt.lib) which is needed to load static orc runtime. I can dig more into why this is happening.

For collection side, maybe we should change jitlink graph to store external symbols with DenseMap instead? We must iterate over the entire external_symobls() in order to check duplicate right now. (which sounded expensive for me) I always thought we should change it to densemap but didn't proceed on as it has been invalid for object file to list symbol twice.

sunho added a comment.EditedJul 17 2022, 5:51 PM

The object file in the testcase is accepted without error by link.exe, so it seems to be allowed to list duplicate external symbols in MSVC.

sunho added a comment.Jul 17 2022, 6:02 PM

Also, for densemap idea, we could clear up the old todo of changing stringref to symbolstringref in order to make it more efficient. (densemap with 64 bit integer key should be dead efficient)

sunho added a comment.Jul 19 2022, 6:45 AM

I tried symbolstringptr idea but it needs larger changes, I have it in local I can submit it as another patch. Maybe make sense to keep as it is here for this patch?

lhames accepted this revision.Jul 25 2022, 2:45 PM

LGTM.

llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp
229

LinkGraph::external_symbols() is a vector at the moment -- it won't scale well if there are lots of duplicates. I wonder if LinkGraph should have a SmallDenseMap structure -- vector if/when it has <N elements, otherwise a DenseMap?

That's a problem for another patch though. I think it's fine for COFFLinkGraphBuilder to use a separate map while we're bringing COFF support up, and we can tune it later.

This revision is now accepted and ready to land.Jul 25 2022, 2:45 PM

For collection side, maybe we should change jitlink graph to store external symbols with DenseMap instead? We must iterate over the entire external_symobls() in order to check duplicate right now. (which sounded expensive for me) I always thought we should change it to densemap but didn't proceed on as it has been invalid for object file to list symbol twice.

I totally missed that you had already identified this issue @sunho. Sorry! :)