Page MenuHomePhabricator

[clang] Add type check for explicit instantiation of static data members
Needs ReviewPublic

Authored by nomanous on Oct 30 2020, 2:36 AM.

Details

Summary

Add type check for explicit instantiation of template classes' static data members. This is a bug fix for #38205.

Diff Detail

Unit TestsFailed

TimeTest
410 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

nomanous created this revision.Oct 30 2020, 2:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2020, 2:36 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nomanous requested review of this revision.Oct 30 2020, 2:36 AM
nomanous updated this revision to Diff 301872.Oct 30 2020, 6:22 AM

Change some format.

Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
This comment was removed by nomanous.
nomanous edited the summary of this revision. (Show Details)Oct 30 2020, 11:11 PM
nomanous edited reviewers, added: aaron.ballman, dblaikie; removed: lvoufo.Nov 5 2020, 5:37 AM

How's this compare to the similar checks for variable templates? Is there some code/checking we could share here?

How's this compare to the similar checks for variable templates? Is there some code/checking we could share here?

The code of checks for variable templates is just before the additional code. But the conditions of if statement and the format of diagnostics are different, so it may be better to do the check for static member instantiation separately. And I have reused the type comparing function (with an argument different).

dblaikie added inline comments.Nov 6 2020, 4:37 PM
clang/include/clang/AST/ASTContext.h
2342–2359

I'm guessing this function is pretty widely used - so this change might be a bit invasive/may not be what all callers want?

@rsmith - this is getting a bit out of my wheelhouse and I'd rather like you to take a look when you've got a chance

clang/lib/Sema/SemaTemplate.cpp
10149–10151

The ObjC comment seems a bit out of place here - since the code here doesn't seem to mention them. Maybe skip that part here, and leave it to be described up in the hasSameType function?

clang/test/SemaCXX/template-explicit-instant-type-mismatch.cpp
15–17 ↗(On Diff #301872)

I assume this was already tested somewhere else, perhaps? (since your change was about static member variables of class templates, and didn't touch the existing code for variable templates) So probably doesn't need re-testing here.

nomanous updated this revision to Diff 303709.Nov 8 2020, 5:12 AM

In this update I delete some extra test code, move several comments and split the new type comparing code into a new function to make sure that the commonly used old type comparing function not affected by it.

nomanous updated this revision to Diff 303712.Nov 8 2020, 5:15 AM

Some format fixes.

nomanous updated this revision to Diff 303749.Nov 8 2020, 8:49 PM

Some more format fixes.

Tests missing

I mistakenly missed that file in the last commit. I'll add it soon.

condy added a subscriber: condy.Jan 17 2021, 6:52 PM

The tests failed though

The tests failed though

Which? Do you mean the unit test? It seems to be irrelevant with this revision. This revision doesn't change the code covered by that test. The failure should be caused by other commit on the trunk.

nomanous updated this revision to Diff 317299.Jan 18 2021, 2:19 AM
nomanous set the repository for this revision to rG LLVM Github Monorepo.Jan 18 2021, 2:54 AM
rsmith added inline comments.Jan 18 2021, 7:31 PM
clang/lib/Sema/SemaTemplate.cpp
10146–10159

Can we combine this with the previous if? This is really checking the same thing that the hasSameType check above checked.

10151

The check of TSK seems unnecessary (and incorrect) here -- we should check the type is correct regardless of whether this is an explicit instantiation declaration or an explicit instantiation definition.

10152

Please can you point us at an example that needs this "ignore lifetime" nuance? We should check with Apple folks what they want to happen here, and we should presumably have the same rule for all explicit instantiations of templated variables -- both in the static data member case here and the variable template case a few lines above.

My expectation is that we want one of these two rules:

  1. the declared lifetime should match exactly between the declaration and the explicit instantiation, or
  2. there cannot be a declared lifetime on the explicit instantiation, and a lifetime on the template declaration is ignored by the check

(or a combination of these rules where we accept either option). I don't think that matches what you're doing here -- in particular, I think a wrong declared lifetime on the explicit instantiation should result in an error.

nomanous marked an inline comment as done.Jan 26 2021, 1:49 AM
nomanous added inline comments.
clang/lib/Sema/SemaTemplate.cpp
10146–10159

It seems difficult to combine this with the previous one.

  1. The two if blocks are small and both only contain two statements, but the two statements contained by the posterior one are all different from statements contained by the previous one (they use different diagnostics);
  2. The conditions are different. The previous one uses PrevTemplate to make sure that it's in a variable template declaration and the posterior one uses IsStaticDataMemberInstantiation to make sure that it's in a static data member instantiation of a template class.

So, if we want to combine the two blocks, we must write two sub-if-blocks in the combined block, which, I think, has no advantage over the current code (if not more confusing).

10152

I'll do some test and give you a code snippet to trigger the lifetime mismatch when lifetime specifiers are not ignored.

nomanous added inline comments.Jan 28 2021, 11:38 PM
clang/lib/Sema/SemaTemplate.cpp
10152

With lifetime specifiers not ignored, use command

clang -cc1  -fobjc-arc -fblocks -fsyntax-only snippet.mm

to compile the obj-c++ file snippet.mm whose contents are as follows:

typedef void (^fptr)();
template<typename T> struct StaticMembers {
  static fptr f;
};

template<typename T>
fptr StaticMembers<T>::f = [] {};

template fptr StaticMembers<float>::f;

The compiler will give a type mismatch error, saying that f's definition in the template class has a type '__strong fptr' (aka 'void (^__strong)()') while its explicit instantiation has a type 'fptr' (aka 'void (^)()'), though they should essentially have the same type.

This problem only happens in explicit instantiation of a static member of block type and doesn't affect static members of other types.

@rsmith

haowei removed a reviewer: haowei.Wed, Feb 24, 5:49 PM