This is an archive of the discontinued LLVM Phabricator instance.

[DWARFLinker] mark odr candidates inside the same object file.
ClosedPublic

Authored by avl on May 12 2022, 7:56 AM.

Details

Summary

This patch is extracted from D86539.

Current implementation of lookForDIEsToKeep() function skips types
duplications basing on the getCanonicalDIEOffset() data:

if (AttrSpec.Form != dwarf::DW_FORM_ref_addr && (UseOdr || IsModuleRef) &&
    Info.Ctxt &&
    Info.Ctxt != ReferencedCU->getInfo(Info.ParentIdx).Ctxt &&
    Info.Ctxt->getCanonicalDIEOffset() && isODRAttribute(AttrSpec.Attr))  <<<<<
  continue;

But that field is set after all compile units inside object file are processed:

for (auto &CurrentUnit : OptContext.CompileUnits)
  lookForDIEsToKeep(.., &CurrentUnit, ..);  // check CanonicalDIEOffset

DIECloner.cloneAllCompileUnits(); // set CanonicalDIEOffset

Thus, if the object file contains several compilation units - types would
not be deduplicated. The above solution works well for the case when the object file
contains only one compilation unit. But if the object file contains several compilation
units then types would not be deduplicated between these compilation units.

This patch changes the algorithm so that types were deduplicated between
compilation units from the same object file.

It produces binary incompatible output for the cases when several compilation units
are located inside the same object file.

Diff Detail

Event Timeline

avl created this revision.May 12 2022, 7:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2022, 7:56 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
avl requested review of this revision.May 12 2022, 7:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2022, 7:56 AM
avl edited the summary of this revision. (Show Details)May 12 2022, 8:01 AM
avl added a comment.Jun 13 2022, 12:12 AM

@aprantl @JDevlieghere Could you take a look at this review, please? Do you want me to do any specific testing for that change?

aprantl accepted this revision.Jun 13 2022, 5:04 PM

I think this looks reasonable. @JDevlieghere ?

This revision is now accepted and ready to land.Jun 13 2022, 5:04 PM
avl added a comment.Jun 15 2022, 1:07 PM

Thank you for the review!

This revision was landed with ongoing or failed builds.Jun 28 2022, 9:49 AM
This revision was automatically updated to reflect the committed changes.