Page MenuHomePhabricator

[Sema] Don't treat a non-null template argument as if it were null.
ClosedPublic

Authored by efriedma on Sep 29 2022, 5:36 PM.

Details

Summary

The way this code checks whether a pointer is null is wrong for other reasons; it doesn't actually check whether a null pointer constant is a "constant" in the C++ standard sense. But this fix at least makes sure we don't treat a non-null pointer as if it were null.

Fixes https://github.com/llvm/llvm-project/issues/57883

Diff Detail

Event Timeline

efriedma created this revision.Sep 29 2022, 5:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2022, 5:36 PM
efriedma requested review of this revision.Sep 29 2022, 5:36 PM
shafik added a subscriber: shafik.Sep 29 2022, 6:28 PM
shafik added inline comments.
clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1-11.cpp
31

It looks like in C++17 mode we catch this case: https://godbolt.org/z/s43oE5qWE

shafik added inline comments.Sep 29 2022, 6:35 PM
clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1-11.cpp
31

Another case to check for:

IP<(int*)(1-1)> ip9;

In C++11 the wording use to allow integer constant expressions

It looks like in Sema::CheckTemplateArgument in C++17 mode we end up calling CheckConvertedConstantExpression which sees a IntegralToPointer cast which is basically an reintepret_cast and rejects it as ill-formed.

In the C++11 case we end up in CheckTemplateArgumentAddressOfObjectOrFunction.

Can you also add a release note for the fix?

efriedma updated this revision to Diff 464363.Sep 30 2022, 12:26 PM

Add release note. Add test coverage for C++17, since it uses a different codepath. (The C++17 codepath appears to work correctly without any changes.)

erichkeane added inline comments.Sep 30 2022, 12:36 PM
clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1-11.cpp
31

The new diagnostic here is unfortunate. That 'does not refer to any declaration' doesn't really let me know that it is illegal because that isn't a NPE.

The 'treat it as a null ptr' here is obviously awful, but I find myself wondering if we can do better on this diagnostic trivially enough?

rsmith added a subscriber: rsmith.Sep 30 2022, 12:59 PM
rsmith added inline comments.
clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1-11.cpp
31

It looks like in C++17 mode we catch this case: https://godbolt.org/z/s43oE5qWE

That's diagnosing the cast, not the template argument value. You can get around the cast diagnostic by forcing constant folding with a __builtin_constant_p conditional (example). Then we diagnose as

<source>:7:4: error: non-type template argument refers to subobject '(int *)1'

... which seems like a worse diagnostic than the C++11 one, because it's not even true.

efriedma added inline comments.Sep 30 2022, 1:01 PM
clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1-11.cpp
31

It's easy enough to use a different message; we just need to detect the particular case we're considering, print an error, and return NPV_Error. But I'm not sure what a better diagnostic looks like. "standard C++ does not allow using '(int*)1' as a non-type template argument"? (The fact that null is allowed isn't really relevant here.)

shafik added inline comments.Oct 3 2022, 11:30 AM
clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1-11.cpp
31

That's diagnosing the cast, not the template argument value. You can get around the cast diagnostic by forcing constant folding with a __builtin_constant_p conditional (example). Then we diagnose as

<source>:7:4: error: non-type template argument refers to subobject '(int *)1'

... which seems like a worse diagnostic than the C++11 one, because it's not even true.

Great point, I missed that earlier.

My point was that if we are catching this in C++17 mode maybe we can reuse the machinery to catch this other modes as well and fix the diagnostic quality as well. Although I realize that may not be easy but worth at least understanding.

Any suggestions for what the error message should look like, if you aren't satisfied with the current messages?

aaron.ballman added inline comments.Oct 13 2022, 12:15 PM
clang/lib/Sema/SemaTemplate.cpp
6501–6502

It's worth noting that the rules we follow changed slightly: http://eel.is/c++draft/temp.arg.nontype#2 so if we're making repairs in this area, are there broader changes we need to consider? (That's fine as a FIXME comment if the changes end up being unrelated to this specific fix.)

clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1-11.cpp
31

For comparison, ICC diagnoses this with:
error: invalid nontype template argument of type "int *"
and GCC diagnoses it as:
error: 'reinterpret_cast' from integer to pointer and
error: '1' is not a valid template argument for 'int*' because it is not the address of a variable

Either one of those diagnostics seems like an improvement over our status quo.

efriedma added inline comments.Oct 13 2022, 1:25 PM
clang/lib/Sema/SemaTemplate.cpp
6501–6502

C++17 significantly revamped the rules for template arguments; we don't use this code at all for C++17 or later. (It might be worth considering re-unifying the codepaths to some extent; first evaluate under the C++17 rules, then verify if the argument is valid under the earlier standard's rules. But I really just wanted to write a quick bugfix for the obvious miscompile, not completely rewrite the code.)

efriedma updated this revision to Diff 467601.Oct 13 2022, 2:11 PM

Add dedicated error message.

aaron.ballman accepted this revision.Oct 14 2022, 6:12 AM

LGTM, thank you for the fix!

This revision is now accepted and ready to land.Oct 14 2022, 6:12 AM
erichkeane accepted this revision.Oct 14 2022, 6:20 AM

I would love it if the diagnostic had a little more info why it isn't, but this is probably good enough for now.

This revision was landed with ongoing or failed builds.Oct 19 2022, 2:42 PM
This revision was automatically updated to reflect the committed changes.