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

Tue Feb 25 08:07:47 PST 2020

llunak added a comment.

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. 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. 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.

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