Page MenuHomePhabricator

Add linkage check to ASTNodeImporter::hasSameVisibilityContext and rename to hasSameVisibilityContextAndLinkage
ClosedPublic

Authored by shafik on Feb 14 2020, 1:06 PM.

Details

Summary

This fixed is based on the assert in LinkageComputer::getLVForDecl(…) which assumes that all the decls in a redecl chain have the same linkage.

This change was trigged by a bug that came up when debugging llc and running the following expression in while in `SelectionDAG::getNode(…)

p VT.getVectorElementType() == Operand.getValueType()

Evaluating this expression leads to import of an operator== for I believe a std::set iterator. One with external linkage and one with unique external linkage but they end in the same redecl chain which triggers the assert in LinkageComputer::getLVForDecl(…).

This case has proven difficult to reduce to a minimal test case.

Diff Detail

Event Timeline

shafik created this revision.Feb 14 2020, 1:06 PM

I would love to have a minimal test case for this but every approach I tried has failed, so I would appreciate if there are ideas here.

balazske added inline comments.Feb 17 2020, 2:36 AM
clang/lib/AST/ASTImporter.cpp
989

This would be equivalent but more clear code (formatting is probably not correct here):

if (Found->getLinkageInternal() != From->getLinkageInternal())
  return false;

if (From->hasExternalFormalLinkage())
  return true;

return Importer.GetFromTU(Found) == From->getTranslationUnitDecl() &&
       From->isInAnonymousNamespace() == Found->isInAnonymousNamespace();
martong accepted this revision.Feb 17 2020, 6:35 AM

LGTM!

clang/lib/AST/ASTImporter.cpp
989

Yeah, this function is not easy to follow, so this could benefit from a refactoring, I agree. But that is somewhat orthogonal to this change in my opinion.

This revision is now accepted and ready to land.Feb 17 2020, 6:35 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2020, 12:50 PM