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

[efriedma]

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

[shafik]

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]

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

[shafik]

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.

[aaron.ballman]

Can you also add a release note for the fix?

[efriedma]

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]

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]

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]

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]

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.

[efriedma]

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

[aaron.ballman]

clang/lib/Sema/SemaTemplate.cpp

6509–6510

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]

clang/lib/Sema/SemaTemplate.cpp

6509–6510

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]

Add dedicated error message.

[aaron.ballman]

LGTM, thank you for the fix!

[erichkeane]

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

clang/docs/ReleaseNotes.rst

- Fixed a crash in C++20 mode in Clang and Clangd when compile source
   `Issue 53628 <https://github.com/llvm/llvm-project/issues/53628>`_
 - The template arguments of a variable template being accessed as a
   member will now be represented in the AST.
 - Fix incorrect handling of inline builtins with asm labels.
 - Finished implementing C++ DR2565, which results in a requirement becoming
   not satisfied in the event of an instantiation failures in a requires expression's
   parameter list. We previously handled this correctly in a constraint evaluation
   context, but not in a requires clause evaluated as a boolean.
+- Reject non-type template arguments formed by casting a non-zero integer
+  to a pointer in pre-C++17 modes, instead of treating them as null
+  pointers.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now check compile-time determinable string literals as format strings.
   Fixes `Issue 55805: <https://github.com/llvm/llvm-project/issues/55805>`_.
 - ``-Wformat`` now recognizes ``%b`` for the ``printf``/``scanf`` family of
   functions and ``%B`` for the ``printf`` family of functions. Fixes
   `Issue 56885: <https://github.com/llvm/llvm-project/issues/56885>`_.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def note_template_arg_refers_here : Note<
   "non-type template argument refers here">;
 def err_template_arg_not_object_or_func : Error<
   "non-type template argument does not refer to an object or function">;
 def err_template_arg_not_pointer_to_member_form : Error<
   "non-type template argument is not a pointer to member constant">;
 def err_template_arg_member_ptr_base_derived_not_supported : Error<
   "sorry, non-type template argument of pointer-to-member type %1 that refers "
   "to member %q0 of a different class is not supported yet">;
+def err_template_arg_invalid : Error<
+  "non-type template argument '%0' is invalid">;
 def ext_template_arg_extra_parens : ExtWarn<
   "address non-type template argument cannot be surrounded by parentheses">;
 def warn_cxx98_compat_template_arg_extra_parens : Warning<
   "redundant parentheses surrounding address non-type template argument are "
   "incompatible with C++98">, InGroup<CXX98Compat>, DefaultIgnore;
 def err_pointer_to_member_type : Error<
   "invalid use of pointer to member type after %select{.*|->*}0">;
 def err_pointer_to_member_call_drops_quals : Error<

clang/lib/Sema/SemaTemplate.cpp

S.Diag(DiagLoc, diag::err_template_arg_not_address_constant)
       << Arg->getType() << Arg->getSourceRange();
     for (unsigned I = 0, N = Notes.size(); I != N; ++I)
       S.Diag(Notes[I].first, Notes[I].second);
 
     S.Diag(Param->getLocation(), diag::note_template_param_here);
     return NPV_Error;
   }
 
   // C++11 [temp.arg.nontype]p1:
   //   - an address constant expression of type std::nullptr_t
   if (Arg->getType()->isNullPtrType())
     return NPV_NullPointer;
 
   //   - a constant expression that evaluates to a null pointer value (4.10); or
   //   - a constant expression that evaluates to a null member pointer value
   //     (4.11); or
-  if ((EvalResult.Val.isLValue() && !EvalResult.Val.getLValueBase()) ||
+  if ((EvalResult.Val.isLValue() && EvalResult.Val.isNullPointer()) ||
       (EvalResult.Val.isMemberPointer() &&
        !EvalResult.Val.getMemberPointerDecl())) {
     // If our expression has an appropriate type, we've succeeded.
     bool ObjCLifetimeConversion;
     if (S.Context.hasSameUnqualifiedType(Arg->getType(), ParamType) ||
         S.IsQualificationConversion(Arg->getType(), ParamType, false,
                                      ObjCLifetimeConversion))
       return NPV_NullPointer;
 
     // The types didn't match, but we know we got a null pointer; complain,
     // then recover as if the types were correct.
     S.Diag(Arg->getExprLoc(), diag::err_template_arg_wrongtype_null_constant)
       << Arg->getType() << ParamType << Arg->getSourceRange();
     S.Diag(Param->getLocation(), diag::note_template_param_here);
     return NPV_NullPointer;
   }
 
+  if (EvalResult.Val.isLValue() && !EvalResult.Val.getLValueBase()) {
+    // We found a pointer that isn't null, but doesn't refer to an object.
+    // We could just return NPV_NotNullPointer, but we can print a better
+    // message with the information we have here.
+    S.Diag(Arg->getExprLoc(), diag::err_template_arg_invalid)
+      << EvalResult.Val.getAsString(S.Context, ParamType);
+    S.Diag(Param->getLocation(), diag::note_template_param_here);
+    return NPV_Error;
+  }
+
   // If we don't have a null pointer value, but we do have a NULL pointer
   // constant, suggest a cast to the appropriate type.
   if (Arg->isNullPointerConstant(S.Context, Expr::NPC_NeverValueDependent)) {
     std::string Code = "static_cast<" + ParamType.getAsString() + ">(";
     S.Diag(Arg->getExprLoc(), diag::err_template_arg_untyped_null_constant)
         << ParamType << FixItHint::CreateInsertion(Arg->getBeginLoc(), Code)
         << FixItHint::CreateInsertion(S.getLocForEndOfToken(Arg->getEndLoc()),
                                       ")");

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

 // RUN: %clang_cc1 -std=c++11 %s -verify -triple x86_64-linux-gnu
 
 namespace std {
   typedef decltype(nullptr) nullptr_t;
 }
 
-template<int *ip> struct IP {  // expected-note 5 {{template parameter is declared here}}
+template<int *ip> struct IP {  // expected-note 6 {{template parameter is declared here}}
   IP<ip> *ip2;
 };
 
 template<int &ip> struct IR {};
 
 constexpr std::nullptr_t get_nullptr() { return nullptr; }
 
 constexpr std::nullptr_t np = nullptr;
 
 std::nullptr_t nonconst_np; // expected-note{{declared here}}
 
 thread_local int tl; // expected-note {{refers here}}
 
 IP<0> ip0; // expected-error{{null non-type template argument must be cast to template parameter type 'int *'}}
 IP<(0)> ip1; // expected-error{{null non-type template argument must be cast to template parameter type 'int *'}}
 IP<nullptr> ip2;
 IP<get_nullptr()> ip3;
 IP<(int*)0> ip4;
 IP<np> ip5;
 IP<nonconst_np> ip5; // expected-error{{non-type template argument of type 'std::nullptr_t' is not a constant expression}} \
 // expected-note{{read of non-constexpr variable 'nonconst_np' is not allowed in a constant expression}}
 IP<(float*)0> ip6; // expected-error{{null non-type template argument of type 'float *' does not match template parameter of type 'int *'}}
 IP<&tl> ip7; // expected-error{{non-type template argument of type 'int *' is not a constant expression}}
+IP<(int*)1> ip8; // expected-error {{non-type template argument '(int *)1' is invalid}}
 
 IR<tl> ir1; // expected-error{{non-type template argument refers to thread-local object}}
 
 struct X { };
 template<int X::*pm> struct PM { // expected-note 2 {{template parameter is declared here}}
   PM<pm> *pm2;
 };
 

clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1.cpp

 }
 
 //   -- a constant expression that evaluates to a null pointer value (4.10); or
 //   -- a constant expression that evaluates to a null member pointer value
 //      (4.11); or
 //   -- a pointer to member expressed as described in 5.3.1.
 
 namespace bad_args {
-  template <int* N> struct X0 { }; // precxx17-note 2{{template parameter is declared here}}
+  template <int* N> struct X0 { }; // precxx17-note 4{{template parameter is declared here}}
   int i = 42;
   X0<&i + 2> x0a; // precxx17-error{{non-type template argument does not refer to any declaration}} \
                      cxx17-error {{non-type template argument is not a constant expression}} \
                      cxx17-note {{cannot refer to element 2 of non-array object in a constant expression}}
   int* iptr = &i;
 #if __cplusplus >= 201103L
   // expected-note@-2 {{declared here}}
 #endif
 
   X0<iptr> x0b;
 #if __cplusplus <= 199711L
   // expected-error@-2 {{non-type template argument for template parameter of pointer type 'int *' must have its address taken}}
 #else
   // precxx17-error@-4 {{non-type template argument of type 'int *' is not a constant expression}}
   // cxx17-error@-5 {{non-type template argument is not a constant expression}}
   // expected-note@-6 {{read of non-constexpr variable 'iptr' is not allowed in a constant expression}}
 #endif
+  X0<(int*)1> x0c;
+  // precxx17-error@-1 {{non-type template argument '(int *)1' is invalid}}
+  // cxx17-error@-2 {{non-type template argument is not a constant expression}}
+  // cxx17-note@-3 {{reinterpret_cast}}
+  X0<__builtin_constant_p(0) ? (int*)1 : (int*)1> x0d;
+  // precxx17-error@-1 {{non-type template argument '(int *)1' is invalid}}
+  // cxx17-error@-2 {{non-type template argument refers to subobject '(int *)1'}}
 }
 #endif // CPP11ONLY
 
 namespace default_args {
 #ifdef CPP11ONLY
 namespace lambdas {
 template<int I = ([] { return 5; }())> //expected-error {{constant expression}}
 int f();
 }
 #endif // CPP11ONLY
 
 }