[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D74846#1891486 <https://reviews.llvm.org/D74846#1891486>, @llunak wrote:

> In D74846#1889826 <https://reviews.llvm.org/D74846#1889826>, @dblaikie wrote:
>
> > I know it's a bit of an awkward situation to test - but please include one to demonstrate why the original code was inappropriate. At least useful for reviewing/validating the patch, but also might help someone else not to make the same change again in the future.
>
>
> I don't know how to do that, except by doing the does-not-crash test that you refused above. The only way I can trigger the change in ASTDeclWriter::VisitVarDecl() to make any difference is by using the _declspec(selectany), but that crashes without the fix and does nothing with it.


Could you explain more what you mean by "does nothing"? I would've assumed it would go down a path to generate IR that was otherwise/previously untested/unreachable (due to the crash) - so testing that the resulting IR is as expected (that the IR contains a selectany (or however that's lowered to IR) declaration and that declaration is used to initialize the 'desc' variable in main?) is what I would think would be appropriate here.

> I've tried static variables, static const variables, templated statics, inline variables, statics in functions and I don't know what the code actually affects other than _declspec(selectany) crashing.
> 
> Also, since I apparently don't know what the code in fact causes or why _declspec(selectany) crashes, I actually also don't know why the original code was inappropriate, other than that it crashes for an unknown reason. Since I simply reused the modules code for PCH, maybe _declspec(selectany) would crash with modules too? I don't know.

Could you verify this? My attempt to do so doesn't seem to have reproduced the failure with modular codegen:

pch.h

  struct CD3DX12_DEFAULT {};
  extern const __declspec(selectany) CD3DX12_DEFAULT D3D12_DEFAULT;
  
  struct CD3DX12_CPU_DESCRIPTOR_HANDLE {
    CD3DX12_CPU_DESCRIPTOR_HANDLE(CD3DX12_DEFAULT) { ptr = 0; }
    unsigned int ptr;
  };

pch.modulemap

  module pch { header "pch.h" }

use.cpp

  #include "pch.h"
  
  int main() {
    CD3DX12_CPU_DESCRIPTOR_HANDLE desc = CD3DX12_CPU_DESCRIPTOR_HANDLE(D3D12_DEFAULT);
    return desc.ptr;
  }

Commands

  Crashes:
  clang-cl /Ycpch.h pch.cpp /c /Fox.obj /Fpx.pch 
  clang-cl /Yupch.h use.cpp /I. /c /Foy.obj /Fpx.pch
  
  Passes:
  clang -cc1 -fdeclspec -triple x86_64-pc-windows-msvc19.11.0 -fmodules-codegen -fmodules -emit-module -fmodule-name=pch pch.modulemap -x c++ -o pch.pcm
  clang -cc1 -triple x86_64-pc-windows-msvc19.11.0 -emit-llvm -o pch.ll pch.pcm
  clang -cc1 -triple x86_64-pc-windows-msvc19.11.0 -fmodule-name=pch.pcm -fdeclspec use.cpp -emit-llvm

(tried it with a specifically windows-y triple, just in case that was relevant - still possible that there's some other variations between the clang-cl driver and the clang cc1 invocations I'm using for the modules testing here that would turn out to be the critical difference, rather than pch V modules)

> But before we get to this, can we please first fix my patch for 10.0? I didn't check properly for -fmodules-codegen in D69778 <https://reviews.llvm.org/D69778>, that's for certain. That can be fixed before finding out why exactly it causes the trouble it causes.

Generally I'm not in favor of committing fixes/changes that are not understood (that leaves traps in the codebase & potentially other bugs, if we're committing code we don't understand). I'd rather revert the original patch until it's better understood.

> In D74846#1891208 <https://reviews.llvm.org/D74846#1891208>, @aganea wrote:
> 
>> @llunak Do you have time to address this patch this week? If not, I can finish it and land it. Please let me know. We'd like to see it merged into 10.0.
> 
> 
> Feel free to do whatever you think would help you.




Repository:
  rC Clang

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

https://reviews.llvm.org/D74846