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

[v.g.vassilev]

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.

[rtrieu]

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?

[rtrieu]

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]

Remove the TypeMap

[v.g.vassilev]

@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]

LGTM

Thank you for reducing this test case for ODR hashing.

lib/AST/ODRHash.cpp

void VisitTemplateTypeParmType(const TemplateTypeParmType *T) {
     Hash.AddBoolean(T->isParameterPack());
     AddDecl(T->getDecl());
   }
 };
 } // namespace
 
 void ODRHash::AddType(const Type *T) {
   assert(T && "Expecting non-null pointer.");
-  auto Result = TypeMap.insert(std::make_pair(T, TypeMap.size()));
+  const Type *TypeInTypeMap = T;
+  if (const ElaboratedType *ElTy = dyn_cast<ElaboratedType>(T))
+    TypeInTypeMap = ElTy->getNamedType().getTypePtr();
+  auto Result = TypeMap.insert(std::make_pair(TypeInTypeMap, TypeMap.size()));
   ID.AddInteger(Result.first->second);
   // On first encounter of a Type pointer, process it.  Every time afterwards,
   // only the index value is needed.
   if (!Result.second) {
     return;
   }
 
   ODRTypeVisitor(ID, *this).Visit(T);

test/Modules/Inputs/odr_hash-elaborated-types/first.h

+#ifndef FIRST
+#define FIRST
+
+#include "textual_time.h"
+
+#endif

test/Modules/Inputs/odr_hash-elaborated-types/module.modulemap

+module M {
+  module first { header "first.h" export *}
+  module second { header "second.h" export *}
+  export *
+}

test/Modules/Inputs/odr_hash-elaborated-types/second.h

+#ifndef SECOND
+#define SECOND
+
+#include "textual_stat.h"
+
+#endif

test/Modules/Inputs/odr_hash-elaborated-types/textual_stat.h

+#ifndef _SYS_STAT_H
+#define _SYS_STAT_H
+
+#include "textual_time.h"
+
+struct stat {
+  struct timespec st_atim;
+  struct timespec st_mtim;
+};
+
+#endif

test/Modules/Inputs/odr_hash-elaborated-types/textual_time.h

+#ifndef _TIME_H
+#define _TIME_H
+
+struct timespec { };
+
+#endif

test/Modules/odr_hash-elaborated-types.cpp

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++1z -I%S/Inputs/odr_hash-elaborated-types -verify %s
+// RUN: %clang_cc1 -std=c++1z -fmodules -fmodules-local-submodule-visibility -fmodule-map-file=%S/Inputs/odr_hash-elaborated-types/module.modulemap -fmodules-cache-path=%t -x c++ -I%S/Inputs/odr_hash-elaborated-types -verify %s
+
+#include "textual_stat.h"
+
+#include "first.h"
+#include "second.h"
+
+void use() { struct stat value; }
+
+// expected-no-diagnostics