This is an archive of the discontinued LLVM Phabricator instance.

[JITLink][COFF] Handle out-of-order COMDAT second symbol.
ClosedPublic

Authored by sunho on Jul 13 2022, 7:42 PM.

Details

Summary

Handle out-of-order COMDAT second symbols. In llvm codegen, the second symbol of COMDAT sequence always follows the first symbol in the global symbol list. But, when the object file came from MSVC compiler, these can come in out of order.

Diff Detail

Event Timeline

sunho created this revision.Jul 13 2022, 7:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 7:42 PM
sunho requested review of this revision.Jul 13 2022, 7:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 7:42 PM
sunho updated this revision to Diff 445187.Jul 15 2022, 10:51 PM

Oh, good catch! Thanks

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

Shouldn't this assign to PendingComdatExports[Symbol.getSectionNumber()] now?

sunho added inline comments.Jul 17 2022, 10:39 AM
llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp
524

good catch! it miraclely built succesfully as variable with the same name was introduced.

sunho updated this revision to Diff 445673.Jul 18 2022, 7:12 PM
sunho marked an inline comment as done.
sunho added inline comments.Jul 19 2022, 6:35 AM
llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp
524

For clearness, I changed PendinComdatExport to reference so that original node will be just set None.

lhames accepted this revision.Jul 19 2022, 6:18 PM

LGTM.

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

What's the maximum number of sections in a COFF object? Could this get large for pathological objects? (In which case a DenseMap<SectionIndex, ComdatExportRequest> might be better?)

This revision is now accepted and ready to land.Jul 19 2022, 6:18 PM
This revision was landed with ongoing or failed builds.Jul 25 2022, 7:05 AM
This revision was automatically updated to reflect the committed changes.