[modules][ODRHash] Compare ODR hashes to detect mismatches in duplicate ObjCInterfaceDecl.

[vsapsai]

Summary

The purpose of the change is to start using ODR hash for comparison
instead of ASTStructuralEquivalence and ODR hash calculation is,
unfortunately, incomplete. The decision to use ODR hash is made because
we can use the same mechanism while checking duplicates encountered
during deserialization. For that case ASTStructuralEquivalence is not a
good fit because it can lead to excessive deserialization and
deserialized decls would be used only for ASTStructuralEquivalence.

rdar://82908223

[vsapsai]

As a proof of concept how ODR hash comparison can be used during deserialization, please see D124289. My point is to highlight the amount of reuse we gain from using ODR hash (I particularly enjoy the test case reuse). I agree the error messages aren't particularly useful yet and it will take some work to make code in ASTReader::diagnoseOdrViolations usable from parser. But I believe that should be doable.

[jansvoboda11]

This sounds reasonable to me. What use-cases does ASTStructuralEquivalence fit better than ODR hashes?

clang/include/clang/Sema/Sema.h

3300

I'm not sure I'm a fan of using the exact same function name for checking ODR hashes and structural equivalence.

[vsapsai]

In D124287#3474369, @jansvoboda11 wrote:

This sounds reasonable to me. What use-cases does ASTStructuralEquivalence fit better than ODR hashes?

I think the biggest advantage of ASTStructuralEquivalence is that it doesn't affect memory consumption by Decls. But it makes repeated comparisons more expensive.

Another ASTStructuralEquivalence advantage is that it's easier to add new checks. For example, comparing ivars is pretty trivial with ASTStructuralEquivalence but with ODR hash we need to handle that ivar types can be structs/unions, including anonymous and nested structs.

Cannot say that for sure but I think ASTStructuralEquivalence is easier to extend and to adopt for different purposes, while ODR hash seems to be more invasive. But this argument is more hand-wavy.

Most likely ASTStructuralEquivalence has other positive qualities but that's what I was able to come up off the top of my head. And you can see that in my decision-making ASTStructuralEquivalence doesn't look like a compelling option.

clang/include/clang/Sema/Sema.h

3300

That's my failure of imagination, will come with something different. And after you've pointed it out, I've realized it's not a good use of overloading too.

[jansvoboda11]

clang/lib/AST/ODRHash.cpp

528

Is this important to implement now, or are you fine leaving this be for the time being?

[jansvoboda11]

In D124287#3476040, @vsapsai wrote:

In D124287#3474369, @jansvoboda11 wrote:

This sounds reasonable to me. What use-cases does ASTStructuralEquivalence fit better than ODR hashes?

I think the biggest advantage of ASTStructuralEquivalence is that it doesn't affect memory consumption by Decls. But it makes repeated comparisons more expensive.

Another ASTStructuralEquivalence advantage is that it's easier to add new checks. For example, comparing ivars is pretty trivial with ASTStructuralEquivalence but with ODR hash we need to handle that ivar types can be structs/unions, including anonymous and nested structs.

Cannot say that for sure but I think ASTStructuralEquivalence is easier to extend and to adopt for different purposes, while ODR hash seems to be more invasive. But this argument is more hand-wavy.

Most likely ASTStructuralEquivalence has other positive qualities but that's what I was able to come up off the top of my head. And you can see that in my decision-making ASTStructuralEquivalence doesn't look like a compelling option.

Makes sense. If most Decl classes had the member variables that enable ODR hashing, would it make sense to keep structural equivalence logic around? That sounds a bit redundant to me.

[iains]

In D124287#3479484, @jansvoboda11 wrote:

In D124287#3476040, @vsapsai wrote:

In D124287#3474369, @jansvoboda11 wrote:

This sounds reasonable to me. What use-cases does ASTStructuralEquivalence fit better than ODR hashes?

I think the biggest advantage of ASTStructuralEquivalence is that it doesn't affect memory consumption by Decls. But it makes repeated comparisons more expensive.

Another ASTStructuralEquivalence advantage is that it's easier to add new checks. For example, comparing ivars is pretty trivial with ASTStructuralEquivalence but with ODR hash we need to handle that ivar types can be structs/unions, including anonymous and nested structs.

Cannot say that for sure but I think ASTStructuralEquivalence is easier to extend and to adopt for different purposes, while ODR hash seems to be more invasive. But this argument is more hand-wavy.

Most likely ASTStructuralEquivalence has other positive qualities but that's what I was able to come up off the top of my head. And you can see that in my decision-making ASTStructuralEquivalence doesn't look like a compelling option.

Makes sense. If most Decl classes had the member variables that enable ODR hashing, would it make sense to keep structural equivalence logic around? That sounds a bit redundant to me.

Presumably ASTStructuralEquivalence can also be used as a tie-breaker in the case of a hash collision?

[jansvoboda11]

In D124287#3479493, @iains wrote:

Presumably ASTStructuralEquivalence can also be used as a tie-breaker in the case of a hash collision?

Ah, that's a good point.

[vsapsai]

Recollected that ASTStructuralEquivalence by default tends to be order-dependent while ODR hash order-independent. Different approaches can be achieved with different mechanisms but some behavior is easier to achieve.

Another peculiarity of ASTStructuralEquivalence is that it allows more customizability. Initializing StructuralEquivalenceContext is more involved than getODRHash() but adding knobs for tweaking comparison behavior is easier.

clang/lib/AST/ODRHash.cpp

528

When I was posting this patch, I thought it wasn't critical to cover everything immediately. Now I'm thinking about the ways to minimize the uncovered elements. Specifically, I'm thinking about adding support for protocols first and then for interfaces. Protocols aren't as useful as interfaces but they are simpler and can be easier to add.

And AddSubDecl should cover some of the nested decls already, need to add more cases to the test.

[vsapsai]

clang/lib/AST/ODRHash.cpp

528

I've made good progress at handling anonymous RecordDecl. So think I should add handling ivars instead of the lame FIXME.

[vsapsai]

Abandoning this change as now it is split between D124286 and D140073.

clang/include/clang/AST/DeclObjC.h

enum InheritedDesignatedInitializersState {
 
       /// The class does not inherit designated initializers.
       IDI_NotInherited = 2
     };
 
     /// One of the \c InheritedDesignatedInitializersState enumeratos.
     mutable unsigned InheritedDesignatedInitializers : 2;
 
+    /// Tracks whether a ODR hash has been computed for this interface.
+    unsigned HasODRHash : 1;
+
+    /// A hash of parts of the class to help in ODR checking.
+    unsigned ODRHash = 0;
+
     /// The location of the last location in this declaration, before
     /// the properties/methods. For example, this will be the '>', '}', or
     /// identifier,
     SourceLocation EndLoc;
 
     DefinitionData()
         : ExternallyCompleted(false), IvarListMissingImplementation(true),
           HasDesignatedInitializers(false),
-          InheritedDesignatedInitializers(IDI_Unknown) {}
+          InheritedDesignatedInitializers(IDI_Unknown), HasODRHash(false) {}
   };
 
   /// The type parameters associated with this class, if any.
   ObjCTypeParamList *TypeParamList = nullptr;
 
   /// Contains a pointer to the data associated with this class,
   /// which will be NULL if this class has not yet been defined.
   ///
public:
   /// Retrieves the canonical declaration of this Objective-C class.
   ObjCInterfaceDecl *getCanonicalDecl() override { return getFirstDecl(); }
   const ObjCInterfaceDecl *getCanonicalDecl() const { return getFirstDecl(); }
 
   // Low-level accessor
   const Type *getTypeForDecl() const { return TypeForDecl; }
   void setTypeForDecl(const Type *TD) const { TypeForDecl = TD; }
 
+  /// Get precomputed ODRHash or add a new one.
+  unsigned getODRHash();
+
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K == ObjCInterface; }
 
 private:
   const ObjCInterfaceDecl *findInterfaceWithDesignatedInitializers() const;
   bool inheritsDesignatedInitializers() const;
+
+  /// True if a valid hash is stored in ODRHash.
+  bool hasODRHash() const;
+  void setHasODRHash(bool HasHash);
 };
 
 /// ObjCIvarDecl - Represents an ObjC instance variable. In general, ObjC
 /// instance variables are identical to C. The only exception is Objective-C
 /// supports C++ style access control. For example:
 ///
 ///   \@interface IvarExample : NSObject
 ///   {

clang/include/clang/AST/ODRHash.h

 
 public:
   ODRHash() {}
 
   // Use this for ODR checking classes between modules.  This method compares
   // more information than the AddDecl class.
   void AddCXXRecordDecl(const CXXRecordDecl *Record);
 
+  // Use this for ODR checking ObjC interfaces. This
+  // method compares more information than the AddDecl class.
+  void AddObjCInterfaceDecl(const ObjCInterfaceDecl *Interface);
+
   // Use this for ODR checking functions between modules.  This method compares
   // more information than the AddDecl class.  SkipBody will process the
   // hash as if the function has no body.
   void AddFunctionDecl(const FunctionDecl *Function, bool SkipBody = false);
 
   // Use this for ODR checking enums between modules.  This method compares
   // more information than the AddDecl class.
   void AddEnumDecl(const EnumDecl *Enum);

clang/include/clang/Sema/Sema.h

public:
   /// struct, or union).
   void ActOnTagStartDefinition(Scope *S, Decl *TagDecl);
 
   /// Perform ODR-like check for C/ObjC when merging tag types from modules.
   /// Differently from C++, actually parse the body and reject / error out
   /// in case of a structural mismatch.
   bool ActOnDuplicateDefinition(Decl *Prev, SkipBodyInfo &SkipBody);
 
+  /// Check ODR hashes for C/ObjC when merging types from modules.
+  /// Differently from C++, actually parse the body and reject in case
+  /// of a mismatch.
+  template <typename T,
+            typename = std::enable_if_t<std::is_base_of<NamedDecl, T>::value>>
+  bool ActOnDuplicateDefinition(T *Duplicate, T *Previous) {
+    if (Duplicate->getODRHash() != Previous->getODRHash())
+      return false;
+
+    // Make the previous decl visible.
+    makeMergedDefinitionVisible(Previous);
+    return true;
+  }
+
   typedef void *SkippedDefinitionContext;
 
   /// Invoked when we enter a tag definition that we're skipping.
   SkippedDefinitionContext ActOnTagStartSkippedDefinition(Scope *S, Decl *TD);
 
   void ActOnObjCContainerStartDefinition(ObjCContainerDecl *IDecl);
 
   /// ActOnStartCXXMemberDeclarations - Invoked when we have parsed a

clang/lib/AST/DeclObjC.cpp

 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTMutationListener.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
+#include "clang/AST/ODRHash.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/None.h"
if (!Method)
       Method = lookupPrivateMethod(Sel, true);
   }
 
   if (!Method && getSuperClass())
     return getSuperClass()->lookupPrivateMethod(Sel, Instance);
   return Method;
 }
 
+unsigned ObjCInterfaceDecl::getODRHash() {
+  assert(hasDefinition() && "ODRHash only for interfaces with definitions");
+
+  // Previously calculated hash is stored in DefinitionData.
+  if (hasODRHash())
+    return data().ODRHash;
+
+  // Only calculate hash on the first call of getODRHash per interface.
+  ODRHash Hasher;
+  Hasher.AddObjCInterfaceDecl(getDefinition());
+  data().ODRHash = Hasher.CalculateHash();
+  setHasODRHash(true);
+
+  return data().ODRHash;
+}
+
+bool ObjCInterfaceDecl::hasODRHash() const {
+  if (!hasDefinition())
+    return false;
+  return data().HasODRHash;
+}
+
+void ObjCInterfaceDecl::setHasODRHash(bool HasHash) {
+  assert(hasDefinition() && "Cannot set ODRHash without definition");
+  data().HasODRHash = HasHash;
+}
+
 //===----------------------------------------------------------------------===//
 // ObjCMethodDecl
 //===----------------------------------------------------------------------===//
 
 ObjCMethodDecl::ObjCMethodDecl(
     SourceLocation beginLoc, SourceLocation endLoc, Selector SelInfo,
     QualType T, TypeSourceInfo *ReturnTInfo, DeclContext *contextDecl,
     bool isInstance, bool isVariadic, bool isPropertyAccessor,

clang/lib/AST/ODRHash.cpp

void ODRHash::AddCXXRecordDecl(const CXXRecordDecl *Record) {
   auto Bases = Record->bases();
   for (auto Base : Bases) {
     AddQualType(Base.getType());
     ID.AddInteger(Base.isVirtual());
     ID.AddInteger(Base.getAccessSpecifierAsWritten());
   }
 }
 
+void ODRHash::AddObjCInterfaceDecl(const ObjCInterfaceDecl *IF) {
+  AddDecl(IF);
+
+  ObjCInterfaceDecl *SuperClass = IF->getSuperClass();
+  AddBoolean(SuperClass);
+  if (SuperClass)
+    ID.AddInteger(SuperClass->getODRHash());
+
+  //FIXME: Hash other interface-specific elements like protocols, etc.
+
+  // Filter out sub-Decls which will not be processed in order to get an
+  // accurate count of Decl's.
+  llvm::SmallVector<const Decl *, 16> Decls;
+  for (Decl *SubDecl : IF->decls())
+    if (isDeclToBeProcessed(SubDecl, IF))
+      Decls.push_back(SubDecl);
+
+  ID.AddInteger(Decls.size());
+  for (auto *SubDecl : Decls)
+    AddSubDecl(SubDecl);
+}
+
 void ODRHash::AddFunctionDecl(const FunctionDecl *Function,
                               bool SkipBody) {
   assert(Function && "Expecting non-null pointer.");
 
   // Skip functions that are specializations or in specialization context.
   const DeclContext *DC = Function;
   while (DC) {
     if (isa<ClassTemplateSpecializationDecl>(DC)) return;

clang/lib/Parse/ParseObjc.cpp

ObjCInterfaceDecl *ClsType = Actions.ActOnStartClassInterface(
       protocols.size(), protocolLocs.data(), EndProtoLoc, attrs, &SkipBody);
 
   if (Tok.is(tok::l_brace))
     ParseObjCClassInstanceVariables(ClsType, tok::objc_protected, AtLoc);
 
   ParseObjCInterfaceDeclList(tok::objc_interface, ClsType);
 
   if (SkipBody.CheckSameAsPrevious) {
-    if (Actions.ActOnDuplicateDefinition(SkipBody.Previous, SkipBody)) {
-      ClsType->mergeDuplicateDefinitionWithCommon(
-          cast<ObjCInterfaceDecl>(SkipBody.Previous)->getDefinition());
+    auto *PreviousDef = cast<ObjCInterfaceDecl>(SkipBody.Previous);
+    if (Actions.ActOnDuplicateDefinition(ClsType, PreviousDef)) {
+      ClsType->mergeDuplicateDefinitionWithCommon(PreviousDef->getDefinition());
     } else {
       Diag(AtLoc, diag::err_duplicate_class_def) << ClsType->getDeclName();
       Diag(SkipBody.Previous->getLocation(), diag::note_previous_definition);
       ClsType->setInvalidDecl();
     }
   }
 
   return ClsType;

clang/test/Modules/compare-objc-interface.m

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/include/first.h
+// RUN: cat %t/test.m        >> %t/include/first.h
+// RUN: echo "#undef FIRST"  >> %t/include/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/include/second-textual.h
+// RUN: cat %t/test.m         >> %t/include/second-textual.h
+// RUN: echo "#undef SECOND"  >> %t/include/second-textual.h
+
+// Test that each header can compile
+// RUN: %clang_cc1 -fsyntax-only -x objective-c %t/include/first.h -fblocks -fobjc-arc
+// RUN: %clang_cc1 -fsyntax-only -x objective-c %t/include/second-textual.h -fblocks -fobjc-arc
+
+// Run test
+// RUN: %clang_cc1 -I%t/include -verify %t/test.m -fblocks -fobjc-arc \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// In non-modular case we don't allow interface redefinitions. But with modules
+// previous definition can come from a hidden [sub]module. And in this case we
+// allow a new definition if it is equivalent to the hidden one.
+//
+// This test case is to verify equivalence checks.
+
+//--- include/common.h
+#ifndef COMMON_H
+#define COMMON_H
+__attribute__((objc_root_class))
+@interface NSObject
+@end
+
+@interface CommonClass: NSObject
+@end
+
+@protocol ProtoP @end
+@protocol ProtoQ @end
+#endif
+
+//--- include/first-empty.h
+//--- include/module.modulemap
+module Common {
+  header "common.h"
+  export *
+}
+module First {
+  module Empty {
+    header "first-empty.h"
+  }
+  module InitiallyHidden {
+    header "first.h"
+    export *
+  }
+}
+
+//--- test.m
+#if defined(FIRST) || defined(SECOND)
+#include "common.h"
+#endif
+#if !defined(FIRST) && !defined(SECOND)
+#include "first-empty.h"
+#include "second-textual.h"
+#endif
+
+#if defined(FIRST)
+@class CompareForwardDeclaration1;
+@interface CompareForwardDeclaration2: NSObject @end
+#elif defined(SECOND)
+@interface CompareForwardDeclaration1: NSObject @end
+@class CompareForwardDeclaration2;
+#else
+CompareForwardDeclaration1 *compareForwardDeclaration1;
+CompareForwardDeclaration2 *compareForwardDeclaration2;
+#endif
+
+#if defined(FIRST)
+@interface CompareSuperClass : NSObject @end
+#elif defined(SECOND)
+@interface CompareSuperClass : CommonClass @end
+#else
+CompareSuperClass *compareSuperClass;
+// expected-error@second-textual.h:* {{duplicate interface definition for class 'CompareSuperClass'}}
+// expected-note@first.h:* {{previous definition is here}}
+#endif