[PATCH] D156054: [Clang][Sema] DR722 (nullptr and varargs) and missing -Wvarargs diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: jcranmer-intel, zahiraam.
aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/FormatString.cpp:484-487
+      if (const auto *PT = argTy->getAs<PointerType>()) {
+        if (PT->getPointeeType()->isCharType())
+          return Match;
+      }
----------------
We can simplify a bit further these days.


================
Comment at: clang/lib/AST/FormatString.cpp:522-534
+      if (const auto *PT = argTy->getAs<PointerType>()) {
+        QualType pointeeTy = PT->getPointeeType();
+        if (pointeeTy->isVoidType() || (!Ptr && pointeeTy->isCharType()))
+          return Match;
+        return NoMatchPedantic;
+      } else if (argTy->isNullPtrType()) {
+        return MatchPromotion;
----------------
Fixes to match the usual coding style rules.

Wow, Phab's diff engine struggles with that edit. Basically: no `else` after a `return`, remove braces on single-line `if`, renamed to `PointeeTy`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1062-1072
+  // C2x 6.5.2.2p6:
+  //   The integer promotions are performed on each trailing argument, and
+  //   trailing arguments that have type float are promoted to double. These are
+  //   called the default argument promotions. No other conversions are
+  //   performed implicitly.
+
+  // C++ [expr.call]p7, per DR722:
----------------
I think this code should live in `Sema::DefaultArgumentPromotion()` because it's related specifically to default argument promotions: https://eel.is/c++draft/expr#call-11.sentence-5


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17317-17319
+    if (TInfo->getType()->isSpecificBuiltinType(BuiltinType::Float) ||
+        TInfo->getType()->isSpecificBuiltinType(BuiltinType::Half))
       PromoteType = Context.DoubleTy;
----------------
Hmmm... the existing code seems wrong to me because it's not paying any attention to `FLT_EVAL_METHOD`, but I think it probably should? CC @jcranmer-intel @zahiraam for opinions.

Actually, I wonder if the correct approach here is to split `Sema::DefaultArgumentPromotion()` up so that we can calculate what the default argument promoted type is of the expression independent of performing the actual promotion, and call the promotion type calculation logic here?


================
Comment at: clang/test/Sema/format-pointer.c:39
+  printf("%p", np);
+  scanf("%p", &np);  // expected-warning {{format specifies type 'void **' but the argument has type 'nullptr_t *'}}
+  scanf("%p", &npp);  // pedantic-warning {{format specifies type 'void **' but the argument has type 'nullptr_t **'}}
----------------
MitalAshok wrote:
> Should this be a pedantic warning?
> 
> If this reads a non-null pointer, the bit pattern of `np` will be messed up but `np` will still read as a nullptr.
> If it reads a null pointer, it should be fine.
> 
That's a fun scenario, good test case!

C23 7.26.6.2p10:  Except in the case of a % specifier, the input item (or, in the case of a %n directive, the count of input characters) is converted to a type appropriate to the conversion specifier. If the input item is not a matching sequence, the execution of the directive fails: this condition is a matching failure. Unless assignment suppression was indicated by a *, the result of the conversion is placed in the object pointed to by the first argument following the format argument that has not already received a conversion result. If this object does not have an appropriate type, or if the result of the conversion cannot be represented in the object, the behavior is undefined.

p12, the p specifier: Matches an implementation-defined set of sequences, which should be the same as the set of sequences that may be produced by the %p conversion of the fprintf function. The corresponding argument shall be a pointer to a pointer of void. The input item is converted to a pointer value in an implementation-defined manner. If the input item is a value converted earlier during the same program execution, the pointer that results shall compare equal to that value; otherwise the behavior of the %p conversion is undefined.

The corresponding argument is not a pointer to a pointer of void, so it's UB, not just pedantically so.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156054/new/

https://reviews.llvm.org/D156054