[C++20] [Modules] [ODRHash] Use CanonicalType for base classes

[ChuanqiXu]

Summary

This comes from https://reviews.llvm.org/D153003

By @rsmith, the test case is valid since:

Per [temp.type]/1.4 (http://eel.is/c++draft/temp.type#1.4),

Two template-ids are the same if [...] their corresponding template template-arguments refer to the same template.

so B<A> and B<NS::A> are the same type. The stricter "same sequence of tokens" rule doesn't apply here, because using-declarations are not definitions.

we should either (preferably) be including only the syntactic form of the base specifier (because local syntax is what the ODR covers), or the canonical type (which should be the same for both using-declarations).

Here we adopt the second suggested solutions.

[ChuanqiXu]

I'd like to land this one later since the change looks trivial and consistent with the suggestion from @rsmith

[cor3ntin]

Give some time for others to review but from the discussions, this look reasonable.

[v.g.vassilev]

I believe this change fixes the original test case but not the problem described in https://reviews.llvm.org/D153003

Here is a test case that illustrates it:

diff
diff --git a/clang/test/Modules/odr_hash.cpp b/clang/test/Modules/odr_hash.cpp
index fffac5e318f5..c798b36bf21e 100644
--- a/clang/test/Modules/odr_hash.cpp
+++ b/clang/test/Modules/odr_hash.cpp
@@ -2097,6 +2097,19 @@ struct S21 {
 S21 s21;
 #endif
 
+template<typename T> struct S22a;
+#if defined(FIRST)
+struct S22 {
+  using Type = S22a<S22>;
+};
+#elif defined(SECOND)
+struct S22 {
+  using Type = S22a<TemplateArgument::S22>;
+};
+#else
+S22 s22;
+#endif
+
 #define DECLS                   \
   OneClass<int> a;              \
   OneInt<1> b;                  \

[ChuanqiXu]

In D154324#4474886, @v.g.vassilev wrote:

I believe this change fixes the original test case but not the problem described in https://reviews.llvm.org/D153003

Here is a test case that illustrates it:

diff
diff --git a/clang/test/Modules/odr_hash.cpp b/clang/test/Modules/odr_hash.cpp
index fffac5e318f5..c798b36bf21e 100644
--- a/clang/test/Modules/odr_hash.cpp
+++ b/clang/test/Modules/odr_hash.cpp
@@ -2097,6 +2097,19 @@ struct S21 {
 S21 s21;
 #endif
 
+template<typename T> struct S22a;
+#if defined(FIRST)
+struct S22 {
+  using Type = S22a<S22>;
+};
+#elif defined(SECOND)
+struct S22 {
+  using Type = S22a<TemplateArgument::S22>;
+};
+#else
+S22 s22;
+#endif
+
 #define DECLS                   \
   OneClass<int> a;              \
   OneInt<1> b;                  \

Oh, it looks like this is a separate problem. Since what @rsmith said only covers the base class case:

we should either (preferably) be including only the syntactic form of the base specifier (because local syntax is what the ODR covers), or the canonical type (which should be the same for both using-declarations).

I'd like to send another patch to fix the issue you mentioned.

[Hahnfeld]

In D154324#4475736, @ChuanqiXu wrote:

Oh, it looks like this is a separate problem. Since what @rsmith said only covers the base class case:

we should either (preferably) be including only the syntactic form of the base specifier (because local syntax is what the ODR covers), or the canonical type (which should be the same for both using-declarations).

I'd like to send another patch to fix the issue you mentioned.

Note that it's the same underlying issue; https://reviews.llvm.org/D153003 should also address this case.

[ChuanqiXu]

In D154324#4475972, @Hahnfeld wrote:

In D154324#4475736, @ChuanqiXu wrote:

Oh, it looks like this is a separate problem. Since what @rsmith said only covers the base class case:

we should either (preferably) be including only the syntactic form of the base specifier (because local syntax is what the ODR covers), or the canonical type (which should be the same for both using-declarations).

I'd like to send another patch to fix the issue you mentioned.

Note that it's the same underlying issue; https://reviews.llvm.org/D153003 should also address this case.

Agreed. The good solution should address both reproducer. While that doesn't prevent this to be a good patch, it shows the test case may not be covered after we fix the root cause. Let's continue with this after we find a better test case.

[v.g.vassilev]

In D154324#4475997, @ChuanqiXu wrote:

In D154324#4475972, @Hahnfeld wrote:

In D154324#4475736, @ChuanqiXu wrote:

Oh, it looks like this is a separate problem. Since what @rsmith said only covers the base class case:

we should either (preferably) be including only the syntactic form of the base specifier (because local syntax is what the ODR covers), or the canonical type (which should be the same for both using-declarations).

I'd like to send another patch to fix the issue you mentioned.

Note that it's the same underlying issue; https://reviews.llvm.org/D153003 should also address this case.

Agreed. The good solution should address both reproducer. While that doesn't prevent this to be a good patch, it shows the test case may not be covered after we fix the root cause. Let's continue with this after we find a better test case.

In fact, I was not suggesting we should block this patch. Let's land this and work on the second part of it in a subsequent patch.

[ChuanqiXu]

In D154324#4476000, @v.g.vassilev wrote:

In D154324#4475997, @ChuanqiXu wrote:

In D154324#4475972, @Hahnfeld wrote:

In D154324#4475736, @ChuanqiXu wrote:

Oh, it looks like this is a separate problem. Since what @rsmith said only covers the base class case:

we should either (preferably) be including only the syntactic form of the base specifier (because local syntax is what the ODR covers), or the canonical type (which should be the same for both using-declarations).

I'd like to send another patch to fix the issue you mentioned.

Note that it's the same underlying issue; https://reviews.llvm.org/D153003 should also address this case.

Agreed. The good solution should address both reproducer. While that doesn't prevent this to be a good patch, it shows the test case may not be covered after we fix the root cause. Let's continue with this after we find a better test case.

In fact, I was not suggesting we should block this patch. Let's land this and work on the second part of it in a subsequent patch.

Got it. Thanks.

[ChuanqiXu]

I filed an issue for the new issue: https://github.com/llvm/llvm-project/issues/63947

[alexfh]

Hi, we've started seeing compilation errors with our modularized build after this commit. The errors say 'SomeType' has different definitions in different modules, but then point to the same definition that comes from the same textual header included into two modules.

The setup (which I couldn't completely isolate yet) is roughly similar to this (hopefully, I didn't miss any important parts):

Textual header p.h:

#include <type_traits>

#include "protobuf/generated_enum_util.h"
...

template <typename T,
          typename =
              typename std::enable_if<proto2::is_proto_enum<T>::value>::type>
class SomeType : E<S<T>> {
...
};

Module A, a.h:

#include <type_traits>

#include "protobuf/generated_enum_util.h"

namespace q {
template <typename T,
          typename std::enable_if<::proto2::is_proto_enum<T>::value>::type>
class X {};
}

#include "p.h"

Module B, b.h:

// ...
// something likely unrelated
// ...
#include "p.h"

Module C (uses module A, module B), c.h:

#include "a.h"
#include "b.h"

[ChuanqiXu]

In D154324#4516605, @alexfh wrote:

Hi, we've started seeing compilation errors with our modularized build after this commit. The errors say 'SomeType' has different definitions in different modules, but then point to the same definition that comes from the same textual header included into two modules.

The setup (which I couldn't completely isolate yet) is roughly similar to this (hopefully, I didn't miss any important parts):

Textual header p.h:

#include <type_traits>

#include "protobuf/generated_enum_util.h"
...

template <typename T,
          typename =
              typename std::enable_if<proto2::is_proto_enum<T>::value>::type>
class SomeType : E<S<T>> {
...
};

Module A, a.h:

#include <type_traits>

#include "protobuf/generated_enum_util.h"

namespace q {
template <typename T,
          typename std::enable_if<::proto2::is_proto_enum<T>::value>::type>
class X {};
}

#include "p.h"

Module B, b.h:

// ...
// something likely unrelated
// ...
#include "p.h"

Module C (uses module A, module B), c.h:

#include "a.h"
#include "b.h"

Maybe we got something wrong with this. I'd like to revert this patch in case it breaks something. But would you like to reduce your reproducer further to a state without external includes to STL or protobuf? Then we can add the reduced reproducer to the tests to avoid further regressions.

[alexfh]

In D154324#4516917, @ChuanqiXu wrote:

In D154324#4516605, @alexfh wrote:

Hi, we've started seeing compilation errors with our modularized build after this commit. The errors say 'SomeType' has different definitions in different modules, but then point to the same definition that comes from the same textual header included into two modules.

The setup (which I couldn't completely isolate yet) is roughly similar to this (hopefully, I didn't miss any important parts):

Textual header p.h:

#include <type_traits>

#include "protobuf/generated_enum_util.h"
...

template <typename T,
          typename =
              typename std::enable_if<proto2::is_proto_enum<T>::value>::type>
class SomeType : E<S<T>> {
...
};

Module A, a.h:

#include <type_traits>

#include "protobuf/generated_enum_util.h"

namespace q {
template <typename T,
          typename std::enable_if<::proto2::is_proto_enum<T>::value>::type>
class X {};
}

#include "p.h"

Module B, b.h:

// ...
// something likely unrelated
// ...
#include "p.h"

Module C (uses module A, module B), c.h:

#include "a.h"
#include "b.h"

Maybe we got something wrong with this. I'd like to revert this patch in case it breaks something. But would you like to reduce your reproducer further to a state without external includes to STL or protobuf? Then we can add the reduced reproducer to the tests to avoid further regressions.

That turned out to be quite time-consuming, but I can try nevertheless. I also asked @rsmith if he could figure out what the problem is. Hopefully, he can help with the test case, if gets to the bottom of the problem.

[alexfh]

In D154324#4516964, @alexfh wrote:

In D154324#4516917, @ChuanqiXu wrote:

Maybe we got something wrong with this. I'd like to revert this patch in case it breaks something. But would you like to reduce your reproducer further to a state without external includes to STL or protobuf? Then we can add the reduced reproducer to the tests to avoid further regressions.

That turned out to be quite time-consuming, but I can try nevertheless. I also asked @rsmith if he could figure out what the problem is. Hopefully, he can help with the test case, if gets to the bottom of the problem.

I have a reduced reproducer, but I still depends on the internal build setup. I need a bit more time to make the reproducer standalone.

[alexfh]

In D154324#4520096, @alexfh wrote:

In D154324#4516964, @alexfh wrote:

In D154324#4516917, @ChuanqiXu wrote:

Maybe we got something wrong with this. I'd like to revert this patch in case it breaks something. But would you like to reduce your reproducer further to a state without external includes to STL or protobuf? Then we can add the reduced reproducer to the tests to avoid further regressions.

That turned out to be quite time-consuming, but I can try nevertheless. I also asked @rsmith if he could figure out what the problem is. Hopefully, he can help with the test case, if gets to the bottom of the problem.

I have a reduced reproducer, but I still depends on the internal build setup. I need a bit more time to make the reproducer standalone.

Okay, here's the repro:

modules-repro.tar.gz1 KBDownload

And my observations with it:

$ cat a.cppmap
module "a" {
  export *
  module "a.h" {
    export *
    header "a.h"
  }
  use "c"
}
$ cat b.cppmap
module "b" {
  export *
  module "b.h" {
    export *
    header "b.h"
  }
  use "c"
}
$ cat c.cppmap
module "c" {
  export *
  module "c1.h" {
    export *
    textual header "c1.h"
  }
  module "c2.h" {
    export *
    textual header "c2.h"
  }
  module "c3.h" {
    export *
    textual header "c3.h"
  }
}
$ cat test.cppmap
module "test" {
  export *
  use "a"
  use "b"
}
$ cat a.h
#ifndef A_H_
#define A_H_

#include "c1.h"

namespace q {
template <typename T,
          typename std::enable_if<::p::P<T>::value>::type>
class X {};
}  // namespace q

#include "c3.h"

#endif  // A_H_
$ cat b.h
#ifndef B_H_
#define B_H_

#include "c2.h"

#endif  // B_H_
$ cat c1.h
#ifndef C1_H_
#define C1_H_

namespace std {
template <class _Tp, _Tp __v>
struct integral_constant {
  static constexpr const _Tp value = __v;
  typedef _Tp value_type;
  typedef integral_constant type;
  constexpr operator value_type() const noexcept { return value; }
  constexpr value_type operator()() const noexcept { return value; }
};

template <class _Tp, _Tp __v>
constexpr const _Tp integral_constant<_Tp, __v>::value;

typedef integral_constant<bool, true> true_type;
typedef integral_constant<bool, false> false_type;

template <bool, class _Tp = void>
struct enable_if {};
template <class _Tp>
struct enable_if<true, _Tp> {
  typedef _Tp type;
};
}  // namespace std

namespace p {
template <typename T>
struct P : ::std::false_type {};
}

#endif  // C1_H_
$ cat c2.h
#ifndef C2_H_
#define C2_H_

#include "c3.h"

enum E {};
namespace p {
template <>
struct P<E> : std::true_type {};
}  // namespace proto2

inline void f(::util::EnumErrorSpace<E>) {}

#endif  // C2_H_
$ cat c3.h
#ifndef C3_H_
#define C3_H_

#include "c1.h"

namespace util {

template <typename T>
class ErrorSpaceImpl;

class ErrorSpace {
 protected:
  template <bool* addr>
  struct OdrUse {
    constexpr OdrUse() : b(*addr) {}
    bool& b;
  };
  template <typename T>
  struct Registerer {
    static bool register_token;
    static constexpr OdrUse<&register_token> kRegisterTokenUse{};
  };

 private:
  template <typename T>
  static const ErrorSpace* GetBase() {
    return 0;
  }

  static bool Register(const ErrorSpace* (*space)()) { return true; }
};

template <typename T>
bool ErrorSpace::Registerer<T>::register_token =
    Register(&ErrorSpace::GetBase<T>);

template <typename T>
class ErrorSpaceImpl : public ErrorSpace {
 private:
  static constexpr Registerer<ErrorSpaceImpl> kRegisterer{};
};

template <typename T, typename = typename std::enable_if<p::P<T>::value>::type>
class EnumErrorSpace : public ErrorSpaceImpl<EnumErrorSpace<T>> {};

}  // namespace util
#endif  // C3_H_
$ cat test.cc
#include "a.h"
#include "b.h"

int main(int, char**) {}
$ clang -fmodules -fno-implicit-modules -fno-implicit-module-maps -fmodule-name=c -fmodule-map-file=c.cppmap -xc++ -c c.cppmap -Xclang=-emit-module -o c.pcm
$ clang -fmodules -fno-implicit-modules -fno-implicit-module-maps -fmodule-name=a -fmodule-map-file=a.cppmap -fmodule-map-file=c.cppmap -xc++ -c a.cppmap -Xclang=-emit-module -o a.pcm
$ clang -fmodules -fno-implicit-modules -fno-implicit-module-maps -fmodule-name=b -fmodule-map-file=b.cppmap -fmodule-map-file=c.cppmap -xc++ -c b.cppmap -Xclang=-emit-module -o b.pcm
$ clang -fmodules -fno-implicit-modules -fno-implicit-module-maps -fmodule-name=test -fmodule-map-file=test.cppmap -fmodule-map-file=a.cppmap -fmodule-map-file=b.cppmap -Xclang=-fmodule-file=a.pcm -Xclang=-fmodule-file=b.pcm -xc++ -c test.cc -o test.pcm
In module 'b':
./c3.h:44:7: error: 'util::EnumErrorSpace' has different definitions in different modules; definition in module 'b.b.h' is here
   44 | class EnumErrorSpace : public ErrorSpaceImpl<EnumErrorSpace<T>> {};
      |       ^
./c3.h:44:7: note: definition in module 'a.a.h' is here
   44 | class EnumErrorSpace : public ErrorSpaceImpl<EnumErrorSpace<T>> {};
      |       ^
1 error generated.

[alexfh]

In D154324#4522541, @alexfh wrote:

In D154324#4520096, @alexfh wrote:

In D154324#4516964, @alexfh wrote:

In D154324#4516917, @ChuanqiXu wrote:

Maybe we got something wrong with this. I'd like to revert this patch in case it breaks something. But would you like to reduce your reproducer further to a state without external includes to STL or protobuf? Then we can add the reduced reproducer to the tests to avoid further regressions.

That turned out to be quite time-consuming, but I can try nevertheless. I also asked @rsmith if he could figure out what the problem is. Hopefully, he can help with the test case, if gets to the bottom of the problem.

I have a reduced reproducer, but I still depends on the internal build setup. I need a bit more time to make the reproducer standalone.

Okay, here's the repro:

modules-repro.tar.gz1 KBDownload

And my observations with it:

$ cat a.cppmap
module "a" {
  export *
  module "a.h" {
    export *
    header "a.h"
  }
  use "c"
}
$ cat b.cppmap
module "b" {
  export *
  module "b.h" {
    export *
    header "b.h"
  }
  use "c"
}
$ cat c.cppmap
module "c" {
  export *
  module "c1.h" {
    export *
    textual header "c1.h"
  }
  module "c2.h" {
    export *
    textual header "c2.h"
  }
  module "c3.h" {
    export *
    textual header "c3.h"
  }
}
$ cat test.cppmap
module "test" {
  export *
  use "a"
  use "b"
}
$ cat a.h
#ifndef A_H_
#define A_H_

#include "c1.h"

namespace q {
template <typename T,
          typename std::enable_if<::p::P<T>::value>::type>
class X {};
}  // namespace q

#include "c3.h"

#endif  // A_H_
$ cat b.h
#ifndef B_H_
#define B_H_

#include "c2.h"

#endif  // B_H_
$ cat c1.h
#ifndef C1_H_
#define C1_H_

namespace std {
template <class _Tp, _Tp __v>
struct integral_constant {
  static constexpr const _Tp value = __v;
  typedef _Tp value_type;
  typedef integral_constant type;
  constexpr operator value_type() const noexcept { return value; }
  constexpr value_type operator()() const noexcept { return value; }
};

template <class _Tp, _Tp __v>
constexpr const _Tp integral_constant<_Tp, __v>::value;

typedef integral_constant<bool, true> true_type;
typedef integral_constant<bool, false> false_type;

template <bool, class _Tp = void>
struct enable_if {};
template <class _Tp>
struct enable_if<true, _Tp> {
  typedef _Tp type;
};
}  // namespace std

namespace p {
template <typename T>
struct P : ::std::false_type {};
}

#endif  // C1_H_
$ cat c2.h
#ifndef C2_H_
#define C2_H_

#include "c3.h"

enum E {};
namespace p {
template <>
struct P<E> : std::true_type {};
}  // namespace proto2

inline void f(::util::EnumErrorSpace<E>) {}

#endif  // C2_H_
$ cat c3.h
#ifndef C3_H_
#define C3_H_

#include "c1.h"

namespace util {

template <typename T>
class ErrorSpaceImpl;

class ErrorSpace {
 protected:
  template <bool* addr>
  struct OdrUse {
    constexpr OdrUse() : b(*addr) {}
    bool& b;
  };
  template <typename T>
  struct Registerer {
    static bool register_token;
    static constexpr OdrUse<&register_token> kRegisterTokenUse{};
  };

 private:
  template <typename T>
  static const ErrorSpace* GetBase() {
    return 0;
  }

  static bool Register(const ErrorSpace* (*space)()) { return true; }
};

template <typename T>
bool ErrorSpace::Registerer<T>::register_token =
    Register(&ErrorSpace::GetBase<T>);

template <typename T>
class ErrorSpaceImpl : public ErrorSpace {
 private:
  static constexpr Registerer<ErrorSpaceImpl> kRegisterer{};
};

template <typename T, typename = typename std::enable_if<p::P<T>::value>::type>
class EnumErrorSpace : public ErrorSpaceImpl<EnumErrorSpace<T>> {};

}  // namespace util
#endif  // C3_H_
$ cat test.cc
#include "a.h"
#include "b.h"

int main(int, char**) {}
$ clang -fmodules -fno-implicit-modules -fno-implicit-module-maps -fmodule-name=c -fmodule-map-file=c.cppmap -xc++ -c c.cppmap -Xclang=-emit-module -o c.pcm
$ clang -fmodules -fno-implicit-modules -fno-implicit-module-maps -fmodule-name=a -fmodule-map-file=a.cppmap -fmodule-map-file=c.cppmap -xc++ -c a.cppmap -Xclang=-emit-module -o a.pcm
$ clang -fmodules -fno-implicit-modules -fno-implicit-module-maps -fmodule-name=b -fmodule-map-file=b.cppmap -fmodule-map-file=c.cppmap -xc++ -c b.cppmap -Xclang=-emit-module -o b.pcm
$ clang -fmodules -fno-implicit-modules -fno-implicit-module-maps -fmodule-name=test -fmodule-map-file=test.cppmap -fmodule-map-file=a.cppmap -fmodule-map-file=b.cppmap -Xclang=-fmodule-file=a.pcm -Xclang=-fmodule-file=b.pcm -xc++ -c test.cc -o test.pcm
In module 'b':
./c3.h:44:7: error: 'util::EnumErrorSpace' has different definitions in different modules; definition in module 'b.b.h' is here
   44 | class EnumErrorSpace : public ErrorSpaceImpl<EnumErrorSpace<T>> {};
      |       ^
./c3.h:44:7: note: definition in module 'a.a.h' is here
   44 | class EnumErrorSpace : public ErrorSpaceImpl<EnumErrorSpace<T>> {};
      |       ^
1 error generated.

BTW, if in a.h I change

typename std::enable_if<::p::P<T>::value>::type>

to

typename std::enable_if<p::P<T>::value>::type>

Compilation succeeds.

[Hahnfeld]

In D154324#4522551, @alexfh wrote:

BTW, if in a.h I change

typename std::enable_if<::p::P<T>::value>::type>

to

typename std::enable_if<p::P<T>::value>::type>

Compilation succeeds.

For the fun of it, could you test https://reviews.llvm.org/D153003 on this reproducer and also the internal, real code?

[rsmith]

OK, I see. The problem is that the canonical version of the type can be spelled in different ways in different translation units, due to us treating some expressions as being equivalent despite them not being the same under the ODR. For example, we consider these function template declarations to be redeclarations:

namespace N {
  int x;
  template<typename T> void f(decltype(T(x)));
}
template<typename T> void f(decltype(T(::N::x))) {}

... but the ODR considers the expressions x and ::N::x to be distinct. That means that the canonical form of the type "decltype of N::x-cast-to-<type-template-parameter-0-0>" has two possible different spellings. So we can't use the approach of mapping to the canonical type before forming an ODR hash -- doing so is not correct.

Instead, let's use the other approach that I suggested, and add the spelling of the base class specifier (the type in its TypeSourceInfo) to the ODR hash instead of its canonical type.

clang/lib/AST/ODRHash.cpp

596

Let's hash the type-as-written instead of hashing the canonical type. (Base.getType() gets the unqualified version of the type, which can partially desugar it, and can lead to different representations in different TUs.)

[rsmith]

I've done a pass through this file looking for places where we incorrectly add to the ODR hash a type that was written within some other entity than the one that we're ODR hashing, that could validly be spelled differently in different declarations of that other entity. There are quite a lot of them; please see the comments here for places that need fixing.

Ideally, we should only be hashing a type when we have a corresponding TypeSourceInfo (that describes how that type was written in the source code) and hence a TypeLoc. Similarly, for template arguments, we'd like to have a TemplateArguentLoc instead of a TemplateArgument. So if you want to handle this properly, the best thing would be to change this code so that it can only hash TemplateArgumentLocs and TypeLocs, not TemplateArguments and QualTypes, but that would be a substantial amount of work; just changing it so we start with a type-as-written should be good enough to get it working properly.

clang/lib/AST/ODRHash.cpp

297–302

For a DeclaratorDecl we should be adding D->getTypeSourceInfo()->getType() (the type as written), not D->getType() (the resolved type); for a ValueDecl that is not a DeclaratorDecl, we shouldn't include the type at all, because it wasn't written in the source code.

354

397

862–910

We should not be stripping typedefs here!

915–939

We should only be hashing the type that was written in the source code here, not the adjusted type that's computed from it (and might partially desugar that original type).

976

We should defensively not include the equivalent type here, because it wasn't written in the entity that we're ODR hashing and might in principle depend on the spelling of a type elsewhere (depending on what the attribute does). The modified type is written in the source so it's fine to include it.

998

We should not hash this, because it can differ between identical types that are written the same way.

1187–1203

This code is also wrong, and looks like the root cause of the issue here. We shouldn't be including the underlying type of a typedef type in the ODR hash, because it can be written differently in different declarations of the typedef declaration.

If we want to include the underlying type of the typedef here, we'll need a new kind of hashing to capture only the value of the typedef and not how it was written. I don't think it's worth it; let's just not include the definition of a referenced typedef in the hash for now.

1210–1211

We should also not hash this, because it can differ between identical types.

[v.g.vassilev]

@rsmith, thanks for the suggestions! Could you go over ODRHash::AddTemplateName suggest how to fix it to address https://reviews.llvm.org/D153003 and https://reviews.llvm.org/D41416#4496451?

[alexfh]

In D154324#4522552, @Hahnfeld wrote:

In D154324#4522551, @alexfh wrote:

BTW, if in a.h I change

typename std::enable_if<::p::P<T>::value>::type>

to

typename std::enable_if<p::P<T>::value>::type>

Compilation succeeds.

For the fun of it, could you test https://reviews.llvm.org/D153003 on this reproducer and also the internal, real code?

Without this patch (D154324), D153003 alone doesn't cause problems with the code that this patch broke. But that's not much information: thousands of Clang and LLVM commits didn't break that code either :)

Applying D153003 on top of D154324 fixes both the reduced and the original case.

[rsmith]

In D154324#4524235, @v.g.vassilev wrote:

@rsmith, thanks for the suggestions! Could you go over ODRHash::AddTemplateName suggest how to fix it to address https://reviews.llvm.org/D153003 and https://reviews.llvm.org/D41416#4496451?

AddTemplateName looks fine as-is to me; I think the problem in D153003 is that we'd stepped outside of the entity we were odr-hashing and started hashing something else, which (legitimately) was different between translation units.

For D41416, ODR hashing may not be the best mechanism to hash the template arguments, unfortunately. ODR hashing is (or perhaps, should be) about determining whether two things are spelled the same way and have the same meaning (as required by the C++ ODR), whereas I think what you're looking for is whether they have the same meaning regardless of spelling. Maybe we can get away with reusing ODR hashing anyway, on the basis that any canonical, non-dependent template argument should have the same (invented) spelling in every translation unit, but I'm not certain that's true in all cases. There may still be cases where the canonical type includes some aspect of "whatever we saw first", in which case the ODR hash can differ across translation units for non-dependent, canonical template arguments that are spelled differently but have the same meaning, though I can't think of one off-hand.

[v.g.vassilev]

In D154324#4524368, @rsmith wrote:

In D154324#4524235, @v.g.vassilev wrote:

@rsmith, thanks for the suggestions! Could you go over ODRHash::AddTemplateName suggest how to fix it to address https://reviews.llvm.org/D153003 and https://reviews.llvm.org/D41416#4496451?

AddTemplateName looks fine as-is to me; I think the problem in D153003 is that we'd stepped outside of the entity we were odr-hashing and started hashing something else, which (legitimately) was different between translation units.

For D41416, ODR hashing may not be the best mechanism to hash the template arguments, unfortunately. ODR hashing is (or perhaps, should be) about determining whether two things are spelled the same way and have the same meaning (as required by the C++ ODR), whereas I think what you're looking for is whether they have the same meaning regardless of spelling. Maybe we can get away with reusing ODR hashing anyway, on the basis that any canonical, non-dependent template argument should have the same (invented) spelling in every translation unit, but I'm not certain that's true in all cases. There may still be cases where the canonical type includes some aspect of "whatever we saw first", in which case the ODR hash can differ across translation units for non-dependent, canonical template arguments that are spelled differently but have the same meaning, though I can't think of one off-hand.

Thanks for investigating. I am happy to try to get away with (mis)using ODR hashing and see if we (and for how long) could get away with it. @Hahnfeld and I discussed to use the llvm FoldingSet technique if ODR hash falls short. Is that or it was something else what you had in mind as an alternative to ODR hashing?

[ChuanqiXu]

@alexfh Thanks for your reproducer! I've reverted the commit. @rsmith thanks for your very detailed suggestion too! I'll try to address them in a separate review page.

[ChuanqiXu]

@rsmith I try to apply your suggestion in https://reviews.llvm.org/D156210 and I met some regression issues. I feel the only solution is to get a new kind of hashing to capture only the value of the typedef. How do you think about this?

clang/lib/AST/ODRHash.cpp

void Visit(const Decl *D) {
     Inherited::Visit(D);
   }
 
   void VisitNamedDecl(const NamedDecl *D) {
     Hash.AddDeclarationName(D->getDeclName());
     Inherited::VisitNamedDecl(D);
   }
 
   void VisitValueDecl(const ValueDecl *D) {
     if (!isa<FunctionDecl>(D)) {
       AddQualType(D->getType());
     }
     Inherited::VisitValueDecl(D);
   }
 
   void VisitVarDecl(const VarDecl *D) {
     Hash.AddBoolean(D->isStaticLocal());
     Hash.AddBoolean(D->isConstexpr());
     const bool HasInit = D->hasInit();
     Hash.AddBoolean(HasInit);
     if (HasInit) {
       AddStmt(D->getInit());
public:
   void VisitObjCIvarDecl(const ObjCIvarDecl *D) {
     ID.AddInteger(D->getCanonicalAccessControl());
     Inherited::VisitObjCIvarDecl(D);
   }
 
   void VisitObjCPropertyDecl(const ObjCPropertyDecl *D) {
     ID.AddInteger(D->getPropertyAttributes());
     ID.AddInteger(D->getPropertyImplementation());
     AddQualType(D->getType());
     AddDecl(D);
 
     Inherited::VisitObjCPropertyDecl(D);
   }
 
   void VisitFunctionDecl(const FunctionDecl *D) {
     // Handled by the ODRHash for FunctionDecl
     ID.AddInteger(D->getODRHash());
void VisitObjCMethodDecl(const ObjCMethodDecl *Method) {
 
     ImplicitParamDecl *Self = Method->getSelfDecl();
     Hash.AddBoolean(Self);
     if (Self)
       ID.AddInteger(Self->getParameterKind());
 
     AddDecl(Method);
 
     AddQualType(Method->getReturnType());
     ID.AddInteger(Method->param_size());
     for (auto Param : Method->parameters())
       Hash.AddSubDecl(Param);
 
     if (Method->hasBody()) {
       const bool IsDefinition = Method->isThisDeclarationADefinition();
       Hash.AddBoolean(IsDefinition);
       if (IsDefinition) {
void ODRHash::AddCXXRecordDecl(const CXXRecordDecl *Record) {
   AddBoolean(TD);
   if (TD) {
     AddTemplateParameterList(TD->getTemplateParameters());
   }
 
   ID.AddInteger(Record->getNumBases());
   auto Bases = Record->bases();
   for (const auto &Base : Bases) {
-    AddQualType(Base.getType());
+    AddQualType(Base.getType().getCanonicalType());
     ID.AddInteger(Base.isVirtual());
     ID.AddInteger(Base.getAccessSpecifierAsWritten());
   }
 }
 
 void ODRHash::AddRecordDecl(const RecordDecl *Record) {
   assert(!isa<CXXRecordDecl>(Record) &&
          "For CXXRecordDecl should call AddCXXRecordDecl.");
if (II) {
       Hash.AddIdentifierInfo(II);
     }
   }
 
   void VisitQualifiers(Qualifiers Quals) {
     ID.AddInteger(Quals.getAsOpaqueValue());
   }
 
   // Return the RecordType if the typedef only strips away a keyword.
   // Otherwise, return the original type.
   static const Type *RemoveTypedef(const Type *T) {
     const auto *TypedefT = dyn_cast<TypedefType>(T);
     if (!TypedefT) {
       return T;
     }
 
     const TypedefNameDecl *D = TypedefT->getDecl();
     QualType UnderlyingType = D->getUnderlyingType();
 
     if (UnderlyingType.hasLocalQualifiers()) {
       return T;
     }
 
     const auto *ElaboratedT = dyn_cast<ElaboratedType>(UnderlyingType);
     if (!ElaboratedT) {
       return T;
     }
 
     if (ElaboratedT->getQualifier() != nullptr) {
       return T;
     }
 
     QualType NamedType = ElaboratedT->getNamedType();
     if (NamedType.hasLocalQualifiers()) {
       return T;
     }
 
     const auto *RecordT = dyn_cast<RecordType>(NamedType);
     if (!RecordT) {
       return T;
     }
 
     const IdentifierInfo *TypedefII = TypedefT->getDecl()->getIdentifier();
     const IdentifierInfo *RecordII = RecordT->getDecl()->getIdentifier();
     if (!TypedefII || !RecordII ||
         TypedefII->getName() != RecordII->getName()) {
       return T;
     }
 
     return RecordT;
   }
 
   void Visit(const Type *T) {
     T = RemoveTypedef(T);
     ID.AddInteger(T->getTypeClass());
     Inherited::Visit(T);
   }
 
   void VisitType(const Type *T) {}
 
   void VisitAdjustedType(const AdjustedType *T) {
     QualType Original = T->getOriginalType();
     QualType Adjusted = T->getAdjustedType();
 
     // The original type and pointee type can be the same, as in the case of
     // function pointers decaying to themselves.  Set a bool and only process
     // the type once, to prevent doubling the work.
     SplitQualType split = Adjusted.split();
     if (auto Pointer = dyn_cast<PointerType>(split.Ty)) {
       if (Pointer->getPointeeType() == Original) {
         Hash.AddBoolean(true);
         ID.AddInteger(split.Quals.getAsOpaqueValue());
         AddQualType(Original);
         VisitType(T);
         return;
       }
     }
 
     // The original type and pointee type are different, such as in the case
     // of a array decaying to an element pointer.  Set a bool to false and
     // process both types.
     Hash.AddBoolean(false);
     AddQualType(Original);
     AddQualType(Adjusted);
 
     VisitType(T);
   }
 
   void VisitDecayedType(const DecayedType *T) {
     // getDecayedType and getPointeeType are derived from getAdjustedType
     // and don't need to be separately processed.
     VisitAdjustedType(T);
   }
 
public:
   void VisitVariableArrayType(const VariableArrayType *T) {
     AddStmt(T->getSizeExpr());
     VisitArrayType(T);
   }
 
   void VisitAttributedType(const AttributedType *T) {
     ID.AddInteger(T->getAttrKind());
     AddQualType(T->getModifiedType());
     AddQualType(T->getEquivalentType());
 
     VisitType(T);
   }
 
   void VisitBlockPointerType(const BlockPointerType *T) {
     AddQualType(T->getPointeeType());
     VisitType(T);
   }
 
   void VisitBuiltinType(const BuiltinType *T) {
     ID.AddInteger(T->getKind());
     VisitType(T);
   }
 
   void VisitComplexType(const ComplexType *T) {
     AddQualType(T->getElementType());
     VisitType(T);
   }
 
   void VisitDecltypeType(const DecltypeType *T) {
     AddStmt(T->getUnderlyingExpr());
     AddQualType(T->getUnderlyingType());
     VisitType(T);
   }
 
   void VisitDependentDecltypeType(const DependentDecltypeType *T) {
     VisitDecltypeType(T);
   }
 
   void VisitDeducedType(const DeducedType *T) {
void VisitTemplateTypeParmType(const TemplateTypeParmType *T) {
     ID.AddInteger(T->getDepth());
     ID.AddInteger(T->getIndex());
     Hash.AddBoolean(T->isParameterPack());
     AddDecl(T->getDecl());
   }
 
   void VisitTypedefType(const TypedefType *T) {
     AddDecl(T->getDecl());
     QualType UnderlyingType = T->getDecl()->getUnderlyingType();
     VisitQualifiers(UnderlyingType.getQualifiers());
     while (true) {
       if (const TypedefType *Underlying =
               dyn_cast<TypedefType>(UnderlyingType.getTypePtr())) {
         UnderlyingType = Underlying->getDecl()->getUnderlyingType();
         continue;
       }
       if (const ElaboratedType *Underlying =
               dyn_cast<ElaboratedType>(UnderlyingType.getTypePtr())) {
         UnderlyingType = Underlying->getNamedType();
         continue;
       }
 
       break;
     }
     AddType(UnderlyingType.getTypePtr());
     VisitType(T);
   }
 
   void VisitTypeOfExprType(const TypeOfExprType *T) {
     AddStmt(T->getUnderlyingExpr());
     Hash.AddBoolean(T->isSugared());
     if (T->isSugared())
       AddQualType(T->desugar());
 
     VisitType(T);
   }
   void VisitTypeOfType(const TypeOfType *T) {
     AddQualType(T->getUnmodifiedType());
     VisitType(T);
   }
 

clang/test/Modules/pr63595.cppm

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -I%t %t/module1.cppm -o %t/module1.pcm
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface -I%t %t/module2.cppm -o %t/module2.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/merge.cpp -verify -fsyntax-only
+
+//--- header.h
+namespace NS {
+template <int I>
+class A {
+};
+
+template <template <int I_> class T>
+class B {
+};
+}
+
+//--- module1.cppm
+// inside NS, using C = B<A>
+module;
+export module module1;
+#include "header.h"
+namespace NS {
+using C = B<A>;
+}
+export struct D : NS::C {};
+
+//--- module2.cppm
+// inside NS, using C = B<NS::A>
+module;
+export module module2;
+#include "header.h"
+namespace NS {
+using C = B<NS::A>;
+}
+export struct D : NS::C {};
+
+//--- merge.cpp
+// expected-no-diagnostics
+import module1;
+import module2;
+D d;