[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

Thank you for the reviews, @efriedma! I have committed in 
c92f505346b80fd053ef191bbc66810c9d564b0c 



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

https://reviews.llvm.org/D103611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.

LGTM


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

https://reviews.llvm.org/D103611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:15789-15790
+: Context.getCorrespondingUnsignedType(UnderlyingType);
+if (Context.typesAreCompatible(PromoteType, UnderlyingType,
+   /*CompareUnqualified*/ true))
+  PromoteType = QualType();

I believe I could switch to using `hasSameType()` here though, if you preferred.



Comment at: clang/lib/Sema/SemaExpr.cpp:15775
+  if (Context.typesAreCompatible(PromoteType, UnderlyingType,
+ /*CompareUnqualified*/ true))
 PromoteType = QualType();

efriedma wrote:
> If we're not going to take advantage of the C notion of compatibility, might 
> as well just check hasSameType().
We do still make use of that in C. For example, `mergeTypes()` (called by 
`typesAreCompatible()` in C mode) also does work for functions without a 
prototype (a distinctly C concept), special logic for merging enums and 
integers, qualifier handling, etc.


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

https://reviews.llvm.org/D103611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 350575.
aaron.ballman added a comment.

Updated the comment and removed a check for C++ mode.


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

https://reviews.llvm.org/D103611

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/varargs.cpp


Index: clang/test/SemaCXX/varargs.cpp
===
--- clang/test/SemaCXX/varargs.cpp
+++ clang/test/SemaCXX/varargs.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -std=c++03 -verify %s
-// RUN: %clang_cc1 -std=c++11 -verify %s
+// RUN: %clang_cc1 -std=c++03 -Wno-c++11-extensions -triple i386-pc-unknown 
-verify %s
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin9 -verify %s
 
 __builtin_va_list ap;
 
@@ -28,6 +28,33 @@
   };
 }
 
+// Ensure the correct behavior for promotable type UB checking.
+void promotable(int a, ...) {
+  enum Unscoped1 { One = 0x7FFF };
+  (void)__builtin_va_arg(ap, Unscoped1); // ok
+
+  enum Unscoped2 { Two = 0x };
+  (void)__builtin_va_arg(ap, Unscoped2); // ok
+
+  enum class Scoped { Three };
+  (void)__builtin_va_arg(ap, Scoped); // ok
+
+  enum Fixed : int { Four };
+  (void)__builtin_va_arg(ap, Fixed); // ok
+
+  enum FixedSmall : char { Five };
+  (void)__builtin_va_arg(ap, FixedSmall); // expected-warning {{second 
argument to 'va_arg' is of promotable type 'FixedSmall'; this va_arg has 
undefined behavior because arguments will be promoted to 'int'}}
+
+  enum FixedLarge : long long { Six };
+  (void)__builtin_va_arg(ap, FixedLarge); // ok
+
+  // Ensure that qualifiers are ignored.
+  (void)__builtin_va_arg(ap, const volatile int);  // ok
+
+  // Ensure that signed vs unsigned doesn't matter either.
+  (void)__builtin_va_arg(ap, unsigned int);
+}
+
 #if __cplusplus >= 201103L
 // We used to have bugs identifying the correct enclosing function scope in a
 // lambda.
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15750,8 +15750,46 @@
 QualType PromoteType;
 if (TInfo->getType()->isPromotableIntegerType()) {
   PromoteType = Context.getPromotedIntegerType(TInfo->getType());
-  if (Context.typesAreCompatible(PromoteType, TInfo->getType()))
+  // [cstdarg.syn]p1 defers the C++ behavior to what the C standard says,
+  // and C2x 7.16.1.1p2 says, in part:
+  //   If type is not compatible with the type of the actual next argument
+  //   (as promoted according to the default argument promotions), the
+  //   behavior is undefined, except for the following cases:
+  // - both types are pointers to qualified or unqualified versions of
+  //   compatible types;
+  // - one type is a signed integer type, the other type is the
+  //   corresponding unsigned integer type, and the value is
+  //   representable in both types;
+  // - one type is pointer to qualified or unqualified void and the
+  //   other is a pointer to a qualified or unqualified character type.
+  // Given that type compatibility is the primary requirement (ignoring
+  // qualifications), you would think we could call typesAreCompatible()
+  // directly to test this. However, in C++, that checks for *same type*,
+  // which causes false positives when passing an enumeration type to
+  // va_arg. Instead, get the underlying type of the enumeration and pass
+  // that.
+  QualType UnderlyingType = TInfo->getType();
+  if (const auto *ET = UnderlyingType->getAs())
+UnderlyingType = ET->getDecl()->getIntegerType();
+  if (Context.typesAreCompatible(PromoteType, UnderlyingType,
+ /*CompareUnqualified*/ true))
 PromoteType = QualType();
+
+  // If the types are still not compatible, we need to test whether the
+  // promoted type and the underlying type are the same except for
+  // signedness. Ask the AST for the correctly corresponding type and see
+  // if that's compatible.
+  if (!PromoteType.isNull() &&
+  PromoteType->isUnsignedIntegerType() !=
+  UnderlyingType->isUnsignedIntegerType()) {
+UnderlyingType =
+UnderlyingType->isUnsignedIntegerType()
+? Context.getCorrespondingSignedType(UnderlyingType)
+: Context.getCorrespondingUnsignedType(UnderlyingType);
+if (Context.typesAreCompatible(PromoteType, UnderlyingType,
+   /*CompareUnqualified*/ true))
+  PromoteType = QualType();
+  }
 }
 if (TInfo->getType()->isSpecificBuiltinType(BuiltinType::Float))
   PromoteType = Context.DoubleTy;


Index: clang/test/SemaCXX/varargs.cpp
===
--- clang/test/SemaCXX/varargs.cpp
+++ clang/test/SemaCXX/varargs.cpp
@@ -1,5 +1,5 @@
-// 

[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:15775
+  if (Context.typesAreCompatible(PromoteType, UnderlyingType,
+ /*CompareUnqualified*/ true))
 PromoteType = QualType();

If we're not going to take advantage of the C notion of compatibility, might as 
well just check hasSameType().



Comment at: clang/lib/Sema/SemaExpr.cpp:15783
+  // test for typesAreCompatible() will already properly consider those to
+  // be compatible types.
+  if (Context.getLangOpts().CPlusPlus && !PromoteType.isNull() &&

aaron.ballman wrote:
> efriedma wrote:
> > This explanation doesn't seem right.  Signed and unsigned types are never 
> > considered "compatible".
> > 
> > If I'm understanding correctly, the case this code addresses is promotion 
> > according to `[conv.prom]`p3: "A prvalue of an unscoped enumeration type 
> > whose underlying type is not fixed [...]".  Somehow, the enum ends up with 
> > an unsigned underlying type, but we promote to int?  And this doesn't 
> > happen in C somehow?
> > This explanation doesn't seem right. Signed and unsigned types are never 
> > considered "compatible".
> 
> Good point, I think that comment is wrong.
> 
> > If I'm understanding correctly, the case this code addresses is promotion 
> > according to [conv.prom]p3: "A prvalue of an unscoped enumeration type 
> > whose underlying type is not fixed [...]". Somehow, the enum ends up with 
> > an unsigned underlying type, but we promote to int? And this doesn't happen 
> > in C somehow?
> 
> That's correct. What I am seeing is:
> ```
> enum Unscoped { One = 0x7FFF };
> ```
> C++:
> `PromoteType` = Builtin (Int)
> `UnderlyingType` = Builtin (UInt)
> 
> C:
> `PromoteType` = Builtin (UInt)
> `UnderlyingType` = Builtin (UInt)
> 
> 
> `enum Unscoped { One = 0x };`
> 
> C++:
> `PromoteType` = Builtin (UInt)
> `UnderlyingType` = Builtin (UInt)
> 
> C:
> `PromoteType` = Builtin (UInt)
> `UnderlyingType` = Builtin (UInt)
> 
> At least on i386-pc-unknown.
> 
> So I think this code is almost correct for that test, but is over-constrained 
> by only doing this in C++. WDYT?
That makes more sense.

Not sure this particular issue can show up in C; there's a check for C++ in 
Sema::ActOnEnumBody.  But no harm at least.


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

https://reviews.llvm.org/D103611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:15783
+  // test for typesAreCompatible() will already properly consider those to
+  // be compatible types.
+  if (Context.getLangOpts().CPlusPlus && !PromoteType.isNull() &&

efriedma wrote:
> This explanation doesn't seem right.  Signed and unsigned types are never 
> considered "compatible".
> 
> If I'm understanding correctly, the case this code addresses is promotion 
> according to `[conv.prom]`p3: "A prvalue of an unscoped enumeration type 
> whose underlying type is not fixed [...]".  Somehow, the enum ends up with an 
> unsigned underlying type, but we promote to int?  And this doesn't happen in 
> C somehow?
> This explanation doesn't seem right. Signed and unsigned types are never 
> considered "compatible".

Good point, I think that comment is wrong.

> If I'm understanding correctly, the case this code addresses is promotion 
> according to [conv.prom]p3: "A prvalue of an unscoped enumeration type whose 
> underlying type is not fixed [...]". Somehow, the enum ends up with an 
> unsigned underlying type, but we promote to int? And this doesn't happen in C 
> somehow?

That's correct. What I am seeing is:
```
enum Unscoped { One = 0x7FFF };
```
C++:
`PromoteType` = Builtin (Int)
`UnderlyingType` = Builtin (UInt)

C:
`PromoteType` = Builtin (UInt)
`UnderlyingType` = Builtin (UInt)


`enum Unscoped { One = 0x };`

C++:
`PromoteType` = Builtin (UInt)
`UnderlyingType` = Builtin (UInt)

C:
`PromoteType` = Builtin (UInt)
`UnderlyingType` = Builtin (UInt)

At least on i386-pc-unknown.

So I think this code is almost correct for that test, but is over-constrained 
by only doing this in C++. WDYT?


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

https://reviews.llvm.org/D103611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:15783
+  // test for typesAreCompatible() will already properly consider those to
+  // be compatible types.
+  if (Context.getLangOpts().CPlusPlus && !PromoteType.isNull() &&

This explanation doesn't seem right.  Signed and unsigned types are never 
considered "compatible".

If I'm understanding correctly, the case this code addresses is promotion 
according to `[conv.prom]`p3: "A prvalue of an unscoped enumeration type whose 
underlying type is not fixed [...]".  Somehow, the enum ends up with an 
unsigned underlying type, but we promote to int?  And this doesn't happen in C 
somehow?


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

https://reviews.llvm.org/D103611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 350060.
aaron.ballman added a comment.

Added some more logic and new tests.

The CI pipeline pointed out a valid issue which has now been corrected -- C++ 
was not properly handling the case where one type was `int` and the other was 
`unsigned int`, which are compatible types in C but not the same type in C++. 
The test adds some triples and a new test case to ensure that we are able to 
consistently test this property in both directions.

@efriedma, would you mind taking a second look just to be sure you're still 
happy with the fix?


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

https://reviews.llvm.org/D103611

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/varargs.cpp


Index: clang/test/SemaCXX/varargs.cpp
===
--- clang/test/SemaCXX/varargs.cpp
+++ clang/test/SemaCXX/varargs.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -std=c++03 -verify %s
-// RUN: %clang_cc1 -std=c++11 -verify %s
+// RUN: %clang_cc1 -std=c++03 -Wno-c++11-extensions -triple i386-pc-unknown 
-verify %s
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin9 -verify %s
 
 __builtin_va_list ap;
 
@@ -28,6 +28,33 @@
   };
 }
 
+// Ensure the correct behavior for promotable type UB checking.
+void promotable(int a, ...) {
+  enum Unscoped1 { One = 0x7FFF };
+  (void)__builtin_va_arg(ap, Unscoped1); // ok
+
+  enum Unscoped2 { Two = 0x };
+  (void)__builtin_va_arg(ap, Unscoped2); // ok
+
+  enum class Scoped { Three };
+  (void)__builtin_va_arg(ap, Scoped); // ok
+
+  enum Fixed : int { Four };
+  (void)__builtin_va_arg(ap, Fixed); // ok
+
+  enum FixedSmall : char { Five };
+  (void)__builtin_va_arg(ap, FixedSmall); // expected-warning {{second 
argument to 'va_arg' is of promotable type 'FixedSmall'; this va_arg has 
undefined behavior because arguments will be promoted to 'int'}}
+
+  enum FixedLarge : long long { Six };
+  (void)__builtin_va_arg(ap, FixedLarge); // ok
+
+  // Ensure that qualifiers are ignored.
+  (void)__builtin_va_arg(ap, const volatile int);  // ok
+
+  // Ensure that signed vs unsigned doesn't matter either.
+  (void)__builtin_va_arg(ap, unsigned int);
+}
+
 #if __cplusplus >= 201103L
 // We used to have bugs identifying the correct enclosing function scope in a
 // lambda.
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15750,8 +15750,48 @@
 QualType PromoteType;
 if (TInfo->getType()->isPromotableIntegerType()) {
   PromoteType = Context.getPromotedIntegerType(TInfo->getType());
-  if (Context.typesAreCompatible(PromoteType, TInfo->getType()))
+  // [cstdarg.syn]p1 defers the C++ behavior to what the C standard says,
+  // and C2x 7.16.1.1p2 says, in part:
+  //   If type is not compatible with the type of the actual next argument
+  //   (as promoted according to the default argument promotions), the
+  //   behavior is undefined, except for the following cases:
+  // - both types are pointers to qualified or unqualified versions of
+  //   compatible types;
+  // - one type is a signed integer type, the other type is the
+  //   corresponding unsigned integer type, and the value is
+  //   representable in both types;
+  // - one type is pointer to qualified or unqualified void and the
+  //   other is a pointer to a qualified or unqualified character type.
+  // Given that type compatibility is the primary requirement (ignoring
+  // qualifications), you would think we could call typesAreCompatible()
+  // directly to test this. However, in C++, that checks for *same type*,
+  // which causes false positives when passing an enumeration type to
+  // va_arg. Instead, get the underlying type of the enumeration and pass
+  // that.
+  QualType UnderlyingType = TInfo->getType();
+  if (const auto *ET = UnderlyingType->getAs())
+UnderlyingType = ET->getDecl()->getIntegerType();
+  if (Context.typesAreCompatible(PromoteType, UnderlyingType,
+ /*CompareUnqualified*/ true))
 PromoteType = QualType();
+
+  // If the types are still not compatible, in C++ we need to test whether
+  // the promoted type and the underlying type are the same except for
+  // signedness. Ask the AST for the correctly corresponding type and see
+  // if that's compatible. We don't need to do this in C because the above
+  // test for typesAreCompatible() will already properly consider those to
+  // be compatible types.
+  if (Context.getLangOpts().CPlusPlus && !PromoteType.isNull() &&
+  PromoteType->isUnsignedIntegerType() !=
+  UnderlyingType->isUnsignedIntegerType()) {
+UnderlyingType =
+

[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 349945.
aaron.ballman added a comment.

Changed the test case to speculatively see if the CI pipeline is happy with 
this (I'm trying to avoid adding a triple to the test).


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

https://reviews.llvm.org/D103611

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/varargs.cpp


Index: clang/test/SemaCXX/varargs.cpp
===
--- clang/test/SemaCXX/varargs.cpp
+++ clang/test/SemaCXX/varargs.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++03 -verify %s
+// RUN: %clang_cc1 -std=c++03 -Wno-c++11-extensions -verify %s
 // RUN: %clang_cc1 -std=c++11 -verify %s
 
 __builtin_va_list ap;
@@ -28,6 +28,30 @@
   };
 }
 
+// Ensure the correct behavior for promotable type UB checking.
+void promotable(int a, ...) {
+  enum Unscoped { One = 0x7FFF };
+  (void)__builtin_va_arg(ap, Unscoped); // ok
+
+  enum class Scoped { Two };
+  (void)__builtin_va_arg(ap, Scoped); // ok
+
+  enum Fixed : int { Three };
+  (void)__builtin_va_arg(ap, Fixed); // ok
+
+  enum FixedSmall : char { Four };
+  (void)__builtin_va_arg(ap, FixedSmall); // expected-warning {{second 
argument to 'va_arg' is of promotable type 'FixedSmall'; this va_arg has 
undefined behavior because arguments will be promoted to 'int'}}
+
+  enum FixedLarge : long long { Five };
+  (void)__builtin_va_arg(ap, FixedLarge); // ok
+
+  // Ensure that qualifiers are ignored.
+  (void)__builtin_va_arg(ap, const volatile int);  // ok
+
+  // Ensure that signed vs unsigned doesn't matter either.
+  (void)__builtin_va_arg(ap, unsigned int);
+}
+
 #if __cplusplus >= 201103L
 // We used to have bugs identifying the correct enclosing function scope in a
 // lambda.
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15750,7 +15750,29 @@
 QualType PromoteType;
 if (TInfo->getType()->isPromotableIntegerType()) {
   PromoteType = Context.getPromotedIntegerType(TInfo->getType());
-  if (Context.typesAreCompatible(PromoteType, TInfo->getType()))
+  // [cstdarg.syn]p1 defers the C++ behavior to what the C standard says,
+  // and C2x 7.16.1.1p2 says, in part:
+  //   If type is not compatible with the type of the actual next argument
+  //   (as promoted according to the default argument promotions), the
+  //   behavior is undefined, except for the following cases:
+  // - both types are pointers to qualified or unqualified versions of
+  //   compatible types;
+  // - one type is a signed integer type, the other type is the
+  //   corresponding unsigned integer type, and the value is
+  //   representable in both types;
+  // - one type is pointer to qualified or unqualified void and the
+  //   other is a pointer to a qualified or unqualified character type.
+  // Given that type compatibility is the primary requirement (ignoring
+  // qualifications), you would think we could call typesAreCompatible()
+  // directly to test this. However, in C++, that checks for *same type*,
+  // which causes false positives when passing an enumeration type to
+  // va_arg. Instead, get the underlying type of the enumeration and pass
+  // that.
+  QualType UnderlyingType = TInfo->getType();
+  if (const auto *ET = UnderlyingType->getAs())
+UnderlyingType = ET->getDecl()->getIntegerType();
+  if (Context.typesAreCompatible(PromoteType, UnderlyingType,
+ /*CompareUnqualified*/ true))
 PromoteType = QualType();
 }
 if (TInfo->getType()->isSpecificBuiltinType(BuiltinType::Float))


Index: clang/test/SemaCXX/varargs.cpp
===
--- clang/test/SemaCXX/varargs.cpp
+++ clang/test/SemaCXX/varargs.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++03 -verify %s
+// RUN: %clang_cc1 -std=c++03 -Wno-c++11-extensions -verify %s
 // RUN: %clang_cc1 -std=c++11 -verify %s
 
 __builtin_va_list ap;
@@ -28,6 +28,30 @@
   };
 }
 
+// Ensure the correct behavior for promotable type UB checking.
+void promotable(int a, ...) {
+  enum Unscoped { One = 0x7FFF };
+  (void)__builtin_va_arg(ap, Unscoped); // ok
+
+  enum class Scoped { Two };
+  (void)__builtin_va_arg(ap, Scoped); // ok
+
+  enum Fixed : int { Three };
+  (void)__builtin_va_arg(ap, Fixed); // ok
+
+  enum FixedSmall : char { Four };
+  (void)__builtin_va_arg(ap, FixedSmall); // expected-warning {{second argument to 'va_arg' is of promotable type 'FixedSmall'; this va_arg has undefined behavior because arguments will be promoted to 'int'}}
+
+  enum FixedLarge : long long { Five };
+  (void)__builtin_va_arg(ap, FixedLarge); // ok
+
+  // Ensure that qualifiers are ignored.
+  (void)__builtin_va_arg(ap, const volatile 

[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


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

https://reviews.llvm.org/D103611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D103611#2797044 , @efriedma wrote:

> I'm a little nervous about using C type merging in C++; it's not designed to 
> support C++ types, so it might end up crashing or something like that.  I 
> think I'd prefer to explicitly convert enum types to their underlying type.

I got lulled into thinking `mergeTypes()` worked for C++ because it handles 
references and cites the C++ standard first thing. But I see later that it also 
claims some C++ types are unreachable. So I switched to the underlying type for 
the enumeration, good suggestion!


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

https://reviews.llvm.org/D103611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 349833.
aaron.ballman added a comment.

Update based on review feedback.


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

https://reviews.llvm.org/D103611

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/varargs.cpp


Index: clang/test/SemaCXX/varargs.cpp
===
--- clang/test/SemaCXX/varargs.cpp
+++ clang/test/SemaCXX/varargs.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++03 -verify %s
+// RUN: %clang_cc1 -std=c++03 -Wno-c++11-extensions -verify %s
 // RUN: %clang_cc1 -std=c++11 -verify %s
 
 __builtin_va_list ap;
@@ -28,6 +28,30 @@
   };
 }
 
+// Ensure the correct behavior for promotable type UB checking.
+void promotable(int a, ...) {
+  enum Unscoped { One };
+  (void)__builtin_va_arg(ap, Unscoped); // ok
+
+  enum class Scoped { Two };
+  (void)__builtin_va_arg(ap, Scoped); // ok
+
+  enum Fixed : int { Three };
+  (void)__builtin_va_arg(ap, Fixed); // ok
+
+  enum FixedSmall : char { Four };
+  (void)__builtin_va_arg(ap, FixedSmall); // expected-warning {{second 
argument to 'va_arg' is of promotable type 'FixedSmall'; this va_arg has 
undefined behavior because arguments will be promoted to 'int'}}
+
+  enum FixedLarge : long long { Five };
+  (void)__builtin_va_arg(ap, FixedLarge); // ok
+
+  // Ensure that qualifiers are ignored.
+  (void)__builtin_va_arg(ap, const volatile int);  // ok
+
+  // Ensure that signed vs unsigned doesn't matter either.
+  (void)__builtin_va_arg(ap, unsigned int);
+}
+
 #if __cplusplus >= 201103L
 // We used to have bugs identifying the correct enclosing function scope in a
 // lambda.
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15750,7 +15750,29 @@
 QualType PromoteType;
 if (TInfo->getType()->isPromotableIntegerType()) {
   PromoteType = Context.getPromotedIntegerType(TInfo->getType());
-  if (Context.typesAreCompatible(PromoteType, TInfo->getType()))
+  // [cstdarg.syn]p1 defers the C++ behavior to what the C standard says,
+  // and C2x 7.16.1.1p2 says, in part:
+  //   If type is not compatible with the type of the actual next argument
+  //   (as promoted according to the default argument promotions), the
+  //   behavior is undefined, except for the following cases:
+  // - both types are pointers to qualified or unqualified versions of
+  //   compatible types;
+  // - one type is a signed integer type, the other type is the
+  //   corresponding unsigned integer type, and the value is
+  //   representable in both types;
+  // - one type is pointer to qualified or unqualified void and the
+  //   other is a pointer to a qualified or unqualified character type.
+  // Given that type compatibility is the primary requirement (ignoring
+  // qualifications), you would think we could call typesAreCompatible()
+  // directly to test this. However, in C++, that checks for *same type*,
+  // which causes false positives when passing an enumeration type to
+  // va_arg. Instead, get the underlying type of the enumeration and pass
+  // that.
+  QualType UnderlyingType = TInfo->getType();
+  if (const auto *ET = UnderlyingType->getAs())
+UnderlyingType = ET->getDecl()->getIntegerType();
+  if (Context.typesAreCompatible(PromoteType, UnderlyingType,
+ /*CompareUnqualified*/ true))
 PromoteType = QualType();
 }
 if (TInfo->getType()->isSpecificBuiltinType(BuiltinType::Float))


Index: clang/test/SemaCXX/varargs.cpp
===
--- clang/test/SemaCXX/varargs.cpp
+++ clang/test/SemaCXX/varargs.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++03 -verify %s
+// RUN: %clang_cc1 -std=c++03 -Wno-c++11-extensions -verify %s
 // RUN: %clang_cc1 -std=c++11 -verify %s
 
 __builtin_va_list ap;
@@ -28,6 +28,30 @@
   };
 }
 
+// Ensure the correct behavior for promotable type UB checking.
+void promotable(int a, ...) {
+  enum Unscoped { One };
+  (void)__builtin_va_arg(ap, Unscoped); // ok
+
+  enum class Scoped { Two };
+  (void)__builtin_va_arg(ap, Scoped); // ok
+
+  enum Fixed : int { Three };
+  (void)__builtin_va_arg(ap, Fixed); // ok
+
+  enum FixedSmall : char { Four };
+  (void)__builtin_va_arg(ap, FixedSmall); // expected-warning {{second argument to 'va_arg' is of promotable type 'FixedSmall'; this va_arg has undefined behavior because arguments will be promoted to 'int'}}
+
+  enum FixedLarge : long long { Five };
+  (void)__builtin_va_arg(ap, FixedLarge); // ok
+
+  // Ensure that qualifiers are ignored.
+  (void)__builtin_va_arg(ap, const volatile int);  // ok
+
+  // Ensure that signed vs unsigned doesn't matter either.
+  (void)__builtin_va_arg(ap, unsigned int);
+}
+
 

[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

I'm a little nervous about using C type merging in C++; it's not designed to 
support C++ types, so it might end up crashing or something like that.  I think 
I'd prefer to explicitly convert enum types to their underlying type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103611

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added reviewers: rsmith, eli.friedman, rjmccall.
aaron.ballman requested review of this revision.
Herald added a project: clang.

Clang checks whether the type given to `va_arg` will automatically cause 
undefined behavior, but this check was issuing false positives for enumerations 
in C++. The issue turned out to be because `typesAreCompatible()` in C++ checks 
whether the types are *the same*, so this falls back on the type merging logic 
to see whether the types are mergable or not in both C and C++.

This issue was found by a user on code like:

  typedef enum {
CURLINFO_NONE,
CURLINFO_EFFECTIVE_URL,
CURLINFO_LASTONE = 60
  } CURLINFO;
  
  ...
  
  __builtin_va_arg(list, CURLINFO); // warn about CURLINFO promoting to 'int' 
being UB in C++ but not C

Given that C++ defers to C for the rules around `va_arg`, the behavior should 
be the same in both C and C++ and not diagnose because `int` and `CURLINFO` are 
compatible types.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103611

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/varargs.cpp


Index: clang/test/SemaCXX/varargs.cpp
===
--- clang/test/SemaCXX/varargs.cpp
+++ clang/test/SemaCXX/varargs.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++03 -verify %s
+// RUN: %clang_cc1 -std=c++03 -Wno-c++11-extensions -verify %s
 // RUN: %clang_cc1 -std=c++11 -verify %s
 
 __builtin_va_list ap;
@@ -28,6 +28,30 @@
   };
 }
 
+// Ensure the correct behavior for promotable type UB checking.
+void promotable(int a, ...) {
+  enum Unscoped { One };
+  (void)__builtin_va_arg(ap, Unscoped); // ok
+
+  enum class Scoped { Two };
+  (void)__builtin_va_arg(ap, Scoped); // ok
+
+  enum Fixed : int { Three };
+  (void)__builtin_va_arg(ap, Fixed); // ok
+
+  enum FixedSmall : char { Four };
+  (void)__builtin_va_arg(ap, FixedSmall); // expected-warning {{second 
argument to 'va_arg' is of promotable type 'FixedSmall'; this va_arg has 
undefined behavior because arguments will be promoted to 'int'}}
+
+  enum FixedLarge : long long { Five };
+  (void)__builtin_va_arg(ap, FixedLarge); // ok
+
+  // Ensure that qualifiers are ignored.
+  (void)__builtin_va_arg(ap, const volatile int);  // ok
+
+  // Ensure that signed vs unsigned doesn't matter either.
+  (void)__builtin_va_arg(ap, unsigned int);
+}
+
 #if __cplusplus >= 201103L
 // We used to have bugs identifying the correct enclosing function scope in a
 // lambda.
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15750,7 +15750,27 @@
 QualType PromoteType;
 if (TInfo->getType()->isPromotableIntegerType()) {
   PromoteType = Context.getPromotedIntegerType(TInfo->getType());
-  if (Context.typesAreCompatible(PromoteType, TInfo->getType()))
+  // [cstdarg.syn]p1 defers the C++ behavior to what the C standard says,
+  // and C2x 7.16.1.1p2 says, in part:
+  //   If type is not compatible with the type of the actual next argument
+  //   (as promoted according to the default argument promotions), the
+  //   behavior is undefined, except for the following cases:
+  // - both types are pointers to qualified or unqualified versions of
+  //   compatible types;
+  // - one type is a signed integer type, the other type is the
+  //   corresponding unsigned integer type, and the value is
+  //   representable in both types;
+  // - one type is pointer to qualified or unqualified void and the
+  //   other is a pointer to a qualified or unqualified character type.
+  // Given that type compatibility is the primary requirement (ignoring
+  // qualifications), you would think we could call typesAreCompatible() to
+  // test this. However, in C++, that checks for *same type*, which causes
+  // false positives when passing an enumeration type to va_arg. Instead,
+  // attempt to merge the types and see how well that works.
+  QualType Merged =
+  Context.mergeTypes(PromoteType, TInfo->getType(),
+ /*OfBlockPointer*/ false, /*Unqualified*/ true);
+  if (!Merged.isNull())
 PromoteType = QualType();
 }
 if (TInfo->getType()->isSpecificBuiltinType(BuiltinType::Float))


Index: clang/test/SemaCXX/varargs.cpp
===
--- clang/test/SemaCXX/varargs.cpp
+++ clang/test/SemaCXX/varargs.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++03 -verify %s
+// RUN: %clang_cc1 -std=c++03 -Wno-c++11-extensions -verify %s
 // RUN: %clang_cc1 -std=c++11 -verify %s
 
 __builtin_va_list ap;
@@ -28,6 +28,30 @@
   };
 }
 
+// Ensure the correct behavior for promotable type UB checking.
+void promotable(int a, ...) {
+  enum Unscoped { One };
+