Page MenuHomePhabricator

Fix handling of constinit thread_locals with a forward-declared type.
Needs ReviewPublic

Authored by jyknight on Jun 5 2020, 3:32 PM.

Details

Reviewers
rsmith
Summary

ItaniumCXXABI::usesThreadWrapperFunction calls
VarDecl::needsDestruction, which calls QualType::isDestructedType,
which checks CXXRecordDecl::hasTrivialDestructor -- but only if the
type has a definition.

Most (maybe all other?) callers of isDestructedType call it on a
complete type, but in this particular codepath, it can be called on an
incomplete type, and thus return false for a type which, when the
definition is available, does turn out to have a non-trivial
destructor.

FIXME: I'm not sure if this fix is the correct fix -- whether this
function ought to be changed like this, or if the fix belongs
somewhere else.

Diff Detail

Unit TestsFailed

TimeTest
1,040 msClang.CodeGenCXX::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/lib/clang/11.0.0/include -nostdsysteminc -triple x86_64-apple-darwin10 /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/clang/test/CodeGenCXX/delete.cpp -emit-llvm -o - | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/clang/test/CodeGenCXX/delete.cpp
130 msClang.CodeGenCXX::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; c:\ws\prod\llvm-project\build\bin\clang.exe -cc1 -internal-isystem c:\ws\prod\llvm-project\build\lib\clang\11.0.0\include -nostdsysteminc -triple x86_64-apple-darwin10 C:\ws\prod\llvm-project\clang\test\CodeGenCXX\delete.cpp -emit-llvm -o - | c:\ws\prod\llvm-project\build\bin\filecheck.exe C:\ws\prod\llvm-project\clang\test\CodeGenCXX\delete.cpp
2,780 msProfile-x86_64.Profile-x86_64::Unknown Unit Message ("")
Script: -- : 'RUN: at line 3'; mkdir -p /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/projects/compiler-rt/test/profile/Profile-x86_64/Output/instrprof-gcov-multithread_fork.test.tmp.d

Event Timeline

jyknight created this revision.Jun 5 2020, 3:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2020, 3:32 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rsmith added a comment.Jun 5 2020, 4:22 PM

Your FIXME is a concern. I think it would be preferable for this function to require as a precondition that the type is complete. If the only call where that isn't true is the call from ItaniumCXXABI::usesThreadWrapperFunction, it seems reasonable to perform a complete-type check there before checking for trivial destructibility. When called from Sema, returning a conservatively-correct value here is likely to paper over bugs where a RequireCompleteType call is missing and a definition could be instantiated.