[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.

cfe/trunk/include/clang/AST/ODRHash.h

 // construct an ODRHash object, then call Add* methods over the nodes that
 // need to be hashed.  Then call CalculateHash to get the hash value.
 // Typically, only one Add* call is needed.  clear can be called to reuse the
 // object.
 class ODRHash {
   // Use DenseMaps to convert from DeclarationName and Type pointers
   // to an index value.
   llvm::DenseMap<DeclarationName, unsigned> DeclNameMap;
-  llvm::DenseMap<const Type*, unsigned> TypeMap;
 
   // Save space by processing bools at the end.
   llvm::SmallVector<bool, 128> Bools;
 
   llvm::FoldingSetNodeID ID;
 
 public:
   ODRHash() {}

cfe/trunk/lib/AST/ODRHash.cpp

void ODRHash::AddTemplateParameterList(const TemplateParameterList *TPL) {
   ID.AddInteger(TPL->size());
   for (auto *ND : TPL->asArray()) {
     AddSubDecl(ND);
   }
 }
 
 void ODRHash::clear() {
   DeclNameMap.clear();
-  TypeMap.clear();
   Bools.clear();
   ID.clear();
 }
 
 unsigned ODRHash::CalculateHash() {
   // Append the bools to the end of the data segment backwards.  This allows
   // for the bools data to be compressed 32 times smaller compared to using
   // ID.AddBoolean
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()));
-  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);
 }
 
 void ODRHash::AddQualType(QualType T) {
   AddBoolean(T.isNull());
   if (T.isNull())
     return;
   SplitQualType split = T.split();
   ID.AddInteger(split.Quals.getAsOpaqueValue());
   AddType(split.Ty);
 }
 
 void ODRHash::AddBoolean(bool Value) {
   Bools.push_back(Value);
 }

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

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

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

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

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

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

cfe/trunk/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

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

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

cfe/trunk/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