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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
