This is an archive of the discontinued LLVM Phabricator instance.

[ODRHash] Do not rely on Type* when computing the hash.
ClosedPublic

Authored by v.g.vassilev on Jun 23 2018, 2:26 PM.

Details

Summary

ODRHash aims to provide Cross-TU stable hashing. Making clang::Type pointer part of the hash connects (remotely) the ODRHash with the TU-specific ::Profile hasher.

r332281 exposed the issue by changing the way the ASTContext different elaborated types if there is an owning tag. In that case, ODRHash stores two different types in its TypeMap which yields false ODR violation in modules.

The current state of implementation shouldn't need the TypeMap concept anymore. Rip it out.

Diff Detail

Repository
rL LLVM

Event Timeline

v.g.vassilev created this revision.Jun 23 2018, 2:26 PM

Running this, I see that it causes a regression in one of the other ODR Hash tests. Specifically, the diagnostics on line 1150 & 1151 of test/Modules/odr_hash.cpp are not seen. Was that an expected side-effect of this patch?

Hey Vassil,

I have been giving this a little more thought and I have a solution I'd like you to try out. If the type pointers aren't unique enough, then the indexing scheme will not work for hashing. Can you try this idea in your build environment and see if works for you? That is, use this version AddType:

void ODRHash::AddType(const Type *T) {
  assert(T && "Expecting non-null pointer.");

  ODRTypeVisitor(ID, *this).Visit(T);
}

If the recursion issue that originally made the indexing scheme necessary is no longer present, then this would be path forward for ODR Hashing.

v.g.vassilev retitled this revision from [ODRHash] Do not put elaborated types in the TypeMap to [ODRHash] Rip out the registration of Type* in TypeMap.

Remove the TypeMap

@rtrieu, your comment was along the lines of what I was thinking of. I have updated the patch. Without testing it on large scale, I the current change seems to work for us.

rtrieu accepted this revision.Jun 27 2018, 2:02 PM

LGTM

Thank you for reducing this test case for ODR hashing.

This revision is now accepted and ready to land.Jun 27 2018, 2:02 PM
v.g.vassilev retitled this revision from [ODRHash] Rip out the registration of Type* in TypeMap to [ODRHash] Do not rely on Type* when computing the hash..Jun 28 2018, 12:51 AM
v.g.vassilev edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.