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

Tue Feb 25 12:40:38 PST 2020

llunak added a comment.

In D74846#1891580 <https://reviews.llvm.org/D74846#1891580>, @dblaikie wrote:

> In D74846#1891486 <https://reviews.llvm.org/D74846#1891486>, @llunak wrote:
>
> > 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.

I think it'll help to split this into smaller parts:

1. In D69778 <https://reviews.llvm.org/D69778> I made it possible to use -fmodules-codegen also for PCHs by simply enabling the ModulesCodegen code also when -fmodules-codegen is used together with -building-pch-with-obj. Looking at ASTDeclWriter::VisitVarDecl() in current git it can be clearly seen that I messed up, as the code around line 1020 depends on -building-pch-with-obj but not -fmodules-codegen, so the ModulesCodegen code gets activated also when clang-cl /Yc is used, without -fmodules-codegen. That's clearly wrong and that's why aganea runs into a crash caused by ModulesCodegen code despite not asking for it. I find that part very clear and understood, both versions of my patch fixed that in some way, and that's what should be fixed for 10.0.
2. In  D69778 <https://reviews.llvm.org/D69778> I included all the ModulesCodegen code, even the one checking for Module::ModuleInterfaceUnit instead of -fmodules-codegen (and that's how I made the mistake). I assumed Module::ModuleInterfaceUnit was some kind of equivalent to the PCH's own object file. That might have been a mistake, I don't know. Given that I'm unable to find a variable for which that piece of code would make a difference, I can't even check. Regardless of what it is, it's unimportant for 10.0.
3. There's the crash about the _declspec(selectany) variable from the bugreport. It was triggered by my mistake, but I don't know what the actual cause of it is. And as said before, it may not be related to my patch at all other than that it got triggered by it. Again, it's presumably not important enough for 10.0.

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

"Does nothing" means that my current patch disables the ModulesCodegen code for it, so it "does nothing" other than taking the normal non-ModulesCodegen paths, so there's nothing to test compared to the normal case. At least for the 1) case I don't see what to test and how to do it. And for case 2) I also don't know what to test, because I can't find what difference it makes in the PCH case, except for triggering the crash. And I don't know how to test the crash either (except by crashing/not-crashing), because it crashes before it generates any code.

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

Your testcase is not equivalent. In your case ASTDeclWriter::VisitVarDecl() has ModulesCodegen set to false for 'D3D12_DEFAULT', because 'Writer.WritingModule->Kind == Module::ModuleInterfaceUnit' is false. Due to my mistake, ModulesCodegen was true in the PCH case.

Repository:
  rC Clang

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

https://reviews.llvm.org/D74846