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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In D154324#4522541 <https://reviews.llvm.org/D154324#4522541>, @alexfh wrote:

> In D154324#4520096 <https://reviews.llvm.org/D154324#4520096>, @alexfh wrote:
>
>> In D154324#4516964 <https://reviews.llvm.org/D154324#4516964>, @alexfh wrote:
>>
>>> In D154324#4516917 <https://reviews.llvm.org/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:
> F28380583: modules-repro.tar.gz <https://reviews.llvm.org/F28380583>
>
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154324/new/

https://reviews.llvm.org/D154324