Page MenuHomePhabricator

IR: Consider two DISubprograms to be odr-equal if they have the same template parameters.

Authored by pcc on Jan 27 2017, 8:09 PM.



In ValueMapper we create new operands for MDNodes and
rely on MDNode::replaceWithUniqued to create a new MDNode
with the specified operands. However this doesn't always
actually happen correctly for DISubprograms because when we
uniquify the new node, we only odr-compare it with existing nodes
(MDNodeSubsetEqualImpl<DISubprogram>::isDeclarationOfODRMember). Although
the TemplateParameters field can refer to a distinct DICompileUnit via
DITemplateTypeParameter::type -> DICompositeType::scope -> DISubprogram::unit,
it is not currently included in the odr comparison. As a result, we can end
up getting our original DISubprogram back, which means we will have a cloned
module referring to the DICompileUnit in the original module, which causes
a verification error.

The fix I implemented was to consider TemplateParameters to be one of the
odr-equal properties. But I'm a little uncomfortable with this. In general it
seems unsound to rely on distinct MDNodes never being reachable from nodes
which we only check odr-equality of. My only long term suggestion would be
to separate odr-uniquing from full uniquing.

Test case to come.

Diff Detail


Event Timeline

pcc created this revision.Jan 27 2017, 8:09 PM
pcc added a comment.Jan 27 2017, 9:35 PM

It occurred to me that this may have something to do with the enableDebugTypeODRUniquing() thing, so figured we could just turn it off if we end up splitting the module. But I took a closer look at the code and discovered that they are completely separate (clang has this turned off anyway, as I'd expect). Taking a look at history it looks like both mechanisms were added around the same time. Why do we have both? Can't we have enableDebugTypeODRUniquing() also turn on the isDeclarationOfODRMember uniquing?

krasin added a subscriber: krasin.Jan 30 2017, 7:55 AM

I have manually verified that this change gets rid of an assert during linking Chromium. Can you please also include a test case similar to the one mentioned in

pcc updated this revision to Diff 86372.Jan 30 2017, 5:26 PM
  • Add test case
pcc updated this revision to Diff 86383.Jan 30 2017, 7:35 PM
  • Add source
pcc added a comment.Feb 3 2017, 2:44 PM

Ping. Since the long term fix probably won't happen any time soon, does anyone have any objection to this?

pcc updated this revision to Diff 87285.Feb 6 2017, 12:48 PM
  • Smaller test case, and add a FIXME
aprantl added inline comments.Feb 6 2017, 12:52 PM
22 ↗(On Diff #87285)

Can you add a short comment here that explained what is being tested?
Otherwise lgtm.

This revision was automatically updated to reflect the committed changes.