[PATCH] D136554: Implement CWG2631

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

Sorry for the delay, I was out on vacation for a bit. I have a repro for this new issue now: F25778542: repro.tar.gz <https://reviews.llvm.org/F25778542>

  $ CLANG=~/dev/clang ./repro.sh
  ++ dirname /tmp/repro/repro.sh
  + DIR=/tmp/repro
  + : /tmp/D136554
  + rm -rf /tmp/D136554
  + mkdir -p /tmp/D136554
  + cd /tmp/D136554
  + cp /tmp/repro/lib.h /tmp/repro/outer.h /tmp/repro/x.h /tmp/repro/y.h /tmp/repro/z.h /tmp/repro/lib.cppmap /tmp/repro/outer.cppmap /tmp/repro/x.cppmap /tmp/repro/y.cppmap /tmp/repro/z.cppmap .
  + COMMON_ARGS=('-Wno-mismatched-tags' '-Xclang=-fmodules-embed-all-files' '-Xclang=-fmodules-local-submodule-visibility' '-fmodules' '-fno-implicit-modules' '-fno-implicit-module-maps' '-std=gnu++17' '-Xclang=-fmodule-map-file-home-is-cwd')
  + export COMMON_ARGS
  + /home/rupprecht/dev/clang -Wno-mismatched-tags -Xclang=-fmodules-embed-all-files -Xclang=-fmodules-local-submodule-visibility -fmodules -fno-implicit-modules -fno-implicit-module-maps -std=gnu++17 -Xclang=-fmodule-map-file-home-is-cwd -fmodule-name=x -fmodule-map-file=x.cppmap -fmodule-map-file=lib.cppmap -xc++ -Xclang=-emit-module -c x.cppmap -o x.pcm
  + /home/rupprecht/dev/clang -Wno-mismatched-tags -Xclang=-fmodules-embed-all-files -Xclang=-fmodules-local-submodule-visibility -fmodules -fno-implicit-modules -fno-implicit-module-maps -std=gnu++17 -Xclang=-fmodule-map-file-home-is-cwd -fmodule-name=y -fmodule-map-file=y.cppmap -fmodule-map-file=lib.cppmap -fmodule-map-file=z.cppmap -xc++ -Xclang=-emit-module -c y.cppmap -o y.pcm
  + /home/rupprecht/dev/clang -Wno-mismatched-tags -Xclang=-fmodules-embed-all-files -Xclang=-fmodules-local-submodule-visibility -fmodules -fno-implicit-modules -fno-implicit-module-maps -std=gnu++17 -Xclang=-fmodule-map-file-home-is-cwd -fmodule-name=outer -fmodule-map-file=outer.cppmap -Xclang=-fmodule-file=x.pcm -Xclang=-fmodule-file=y.pcm -fmodule-map-file=lib.cppmap -fmodule-map-file=x.cppmap -fmodule-map-file=y.cppmap -xc++-header -fsyntax-only -c outer.h -o outer.h.processed
  In module 'x':
  ./lib.h:5:25: error: 'Foo' has different definitions in different modules; first difference is definition in module 'x.x.h' found data member 'kConstant' with an initializer
    static constexpr char kConstant = '+';
    ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
  ./lib.h:5:25: note: but in 'y.y.h' found data member 'kConstant' with a different initializer
    static constexpr char kConstant = '+';
    ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
  1 error generated.



In D136554#4000654 <https://reviews.llvm.org/D136554#4000654>, @cor3ntin wrote:

> In D136554#4000363 <https://reviews.llvm.org/D136554#4000363>, @rupprecht wrote:
>
>> I applied this version of the patch and the crash is now gone 🎉
>>
>> However, now I get this inexplicable error -- I'm not entirely sure it's related, maybe I'm holding it wrong:
>>
>>   In module '<foo>':
>>   foo.h$line:$num: error: 'foo::FooClass' has different definitions in different modules; first difference is definition in module 'something.h' found data member 'kFooDelimiter' with an initializer
>>     static constexpr char kFooDelimiter = '+';
>>     ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
>>   foo.h:$line:$num note: but in 'other.h' found data member 'kFooDelimiter' with a different initializer
>>     static constexpr char kFooDelimiter = '+';
>>     ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
>>
>> The definition seems straightforward:
>>
>>   class FooClass final {
>>     ...
>>     static constexpr char kFooDelimiter = '+';
>>     ...
>>   };
>
> This is *very* surprising to me.
> I could explain it if  the member was not static though, as it would be the kind of things the patch affects. But static data members are handled very differently.
>
> Was that liking chrome? It didn't come up in my tests

No, it's some other internal target. There isn't any way for you to be able to test against these targets, so unfortunately the best I can offer is I'll patch in any updated versions of this patch, see what breaks, and try to reduce it when reporting.

There's barely any code in the repro I attached, it's just a bunch of header/module layering. My guess is that clang is usually able to see that the two definitions in different modules is the same and therefore dedupes them, but this change adds some unique bit of information that makes clang think it's different. Note that the headers need to be in a particular order to break.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136554