A patch for integer promotions.

Hello.

The attached program promotion_bug.c shows a bug in the current code generating integer promotions: independently from the sizes of integer types, all small integers are promoted to type `int'.

The bug can only be observed when selecting a 16 bit target architecture, e.g., pic16 (but the bug is caused by target-independent clang code). The following is the ast dump + comments showing evidence for the bug:

# clang-cc -ast-dump -triple=pic16-unknown-unknown promotion_bug.c

int main() (CompoundStmt 0x2b896c0 <promotion_bug.c:1:16, line:8:1>
   (DeclStmt 0x2b8b6c0 <line:2:3, col:31>
     0x2b8b670 "int size_of_int[2]" <=== sizeof(int) = 2
   (DeclStmt 0x2b86f60 <line:3:3, col:37>
     0x2b86f10 "short size_of_short[2]" <=== sizeof(short) = 2
   (DeclStmt 0x2b8a860 <line:5:3, col:25>
     0x2b86f90 "unsigned short us =
       (ImplicitCastExpr 0x2b86fe0 <col:23> 'unsigned short'
         (IntegerLiteral 0x2b7b0c0 <col:23> 'int' 10))"
   (DeclStmt 0x2b8a920 <line:6:3, col:13>
     0x2b8a890 "int i =
       (IntegerLiteral 0x2b8a8e0 <col:11> 'int' 20)"
   (BinaryOperator 0x2b94800 <line:7:3, col:12> 'int' '='
     (DeclRefExpr 0x2b8a950 <col:3> 'int' Var='i' 0x2b8a890)
     (BinaryOperator 0x2b8aa90 <col:7, col:12> 'int' '+'
       (ImplicitCastExpr 0x2b8aa10 <col:7> 'int' <== buggy promotion
         (DeclRefExpr 0x2b8a990 <col:7> 'unsigned short' Var='us' 0x2b86f90))
       (ImplicitCastExpr 0x2b8aa50 <col:12> 'int' <== buggy promotion
         (DeclRefExpr 0x2b8a9d0 <col:12> 'unsigned short' Var='us' 0x2b86f90)))))

The attached patch adds new method
   QualType ASTContext::getPromotedIntegerType(QualType Promotable);
and uses it when generating implicit casts encoding promotions.

After applying the patch, we obtain correct promotions.

Cheers,
Enea Zaffanella.

promotion_bug.c (145 Bytes)

promotion_bug.patch (4.21 KB)

Enea Zaffanella wrote:

Hello.

The attached program promotion_bug.c shows a bug in the current code generating integer promotions: independently from the sizes of integer types, all small integers are promoted to type `int'.

While at it, I would also like to ask for the reason(s) why a method such as

   QualType Sema::UsualArithmeticConversionsType(QualType, QualType);

belongs to Sema, whereas methods such as

   QualType ASTContext::mergeTypes(QualType, QualType);
   QualType ASTContext::mergeFunctionTypes(QualType, QualType);

belong to ASTContext.

As far as I can see, the former method looks very similar to the other two: it requires access to mostly the same info from the context and it does not need to directly access/change nodes in the AST or issue diagnostics.

If there are no objections, I would like to move the former method into ASTContext. This change, besides making things more consistent, would make the method easily accessible to clients (that was indeed the original motivation for my investigation).

If you agree, I can merge this with the patch previously sent.

Cheers,
Enea Zaffanella.

Enea Zaffanella wrote:

Enea Zaffanella wrote:

Hello.

The attached program promotion_bug.c shows a bug in the current code generating integer promotions: independently from the sizes of integer types, all small integers are promoted to type `int'.

While at it, I would also like to ask for the reason(s) why a method such as

   QualType Sema::UsualArithmeticConversionsType(QualType, QualType);

belongs to Sema, whereas methods such as

   QualType ASTContext::mergeTypes(QualType, QualType);
   QualType ASTContext::mergeFunctionTypes(QualType, QualType);

belong to ASTContext.

As far as I can see, the former method looks very similar to the other two: it requires access to mostly the same info from the context and it does not need to directly access/change nodes in the AST or issue diagnostics.

If there are no objections, I would like to move the former method into ASTContext. This change, besides making things more consistent, would make the method easily accessible to clients (that was indeed the original motivation for my investigation).

If you agree, I can merge this with the patch previously sent.

Here is attached the cumulative patch.
Is it possible they get applied before code is frozen?

Cheers,
Enea Zaffanella

IntegerPromotions_and_ASTContext.patch (19 KB)

Patch looks great; applied in r79412. Sorry about the delay!

-Eli

Eli Friedman wrote:

Here is attached the cumulative patch.
Is it possible they get applied before code is frozen?

Patch looks great; applied in r79412. Sorry about the delay!

-Eli

Hi.

Thank you for applying the patch. We seem to have identified another promotion problem, this time related to the promotion of bit-fields.

Apparently, clang accepts further integral bit-field types besides those that are mandated by the C99 standard (_Bool, signed int, unsigned int). However, the code meant to check for bit-field promotions will only insert the promotions for the types mandated by the standard, so that, e.g., a bit-field having `unsigned short' type will not be subject to bit-field promotion rules; it will instead be subject to usual promotion rules.

Example: suppose the following code is parsed on a target where short and int have the same size (e.g., pic16).

struct S {
   unsigned short us : 12;
} s;

int foo(void) {
   return s.us + s.us;
}

Here clang inserts the usual (i.e., non-bit-field) promotion, obtaining code equivalent to

int foo(void) {
   return ((int) (((unsigned int) (s.us)) + ((unsigned int) (s.us))));
}

In contrast, if the bit-field promotions would be applied, we would obtain code equivalent to:

int foo(void) {
   return (int) (s.us) + (int) (s.us);
}

Now, even though we are talking about an implementation-defined behavior, I guess that clang is striving for maximum compatibility with GCC and, according to our observation, GCC applies bit-field promotions to *all* bit-fields having a declared bit-size smaller than that of an int, no matter what their declared type is.

This behavior sounds reasonable, so please find here attached the corresponding patch.

Besides implementing gcc bit-field extensions behavior, the patch also moves the corresponding function into ASTContext, so as to make it easily available to clients:

   QualType ASTContext::isPromotableBitField(Expr *E);

Let us know if there is anything unclear.

Cheers,
Enea Zaffanella.

BitField_Promotion.patch (6.65 KB)

+/// \brief Whether this is a promotable bitfield reference according
+/// to C99 6.3.1.1p2, bullet 2 (and GCC extensions).
+///
+/// \returns the type this bit-field will promote to, or NULL if no
+/// promotion occurs.
+QualType ASTContext::isPromotableBitField(Expr *E) {
+ FieldDecl *Field = E->getBitField();
+ if (!Field)
+ return QualType();
+
+ const BuiltinType *BT = Field->getType()->getAsBuiltinType();
+ if (!BT)
+ return QualType();

What about enum types?

+ assert(BitWidth >= IntSize);
+ switch (BT->getKind()) {
+ case BuiltinType::Bool:
+ case BuiltinType::Char_S:
+ case BuiltinType::Char_U:
+ case BuiltinType::SChar:
+ case BuiltinType::UChar:
+ case BuiltinType::Short:
+ case BuiltinType::UShort:
+ case BuiltinType::Int:
+ case BuiltinType::UInt:
+ assert(BitWidth == IntSize);
+ // Here promotion occurs and it only depends on signedness.
+ return Field->getType()->isSignedIntegerType() ? IntTy : UnsignedIntTy;
+ default:
+ // Types having rank greater than int are not subject to promotions.
+ return QualType();
+ }

This should use getIntegerRank instead of a switch.

Would you please include a test of some sort? You can use the test I
committed for the previous patch as a model.

-Eli

Eli Friedman wrote:

+/// \brief Whether this is a promotable bitfield reference according
+/// to C99 6.3.1.1p2, bullet 2 (and GCC extensions).
+///
+/// \returns the type this bit-field will promote to, or NULL if no
+/// promotion occurs.
+QualType ASTContext::isPromotableBitField(Expr *E) {
+ FieldDecl *Field = E->getBitField();
+ if (!Field)
+ return QualType();
+
+ const BuiltinType *BT = Field->getType()->getAsBuiltinType();
+ if (!BT)
+ return QualType();

What about enum types?

Right, we were forgetting this case.

+ assert(BitWidth >= IntSize);
+ switch (BT->getKind()) {
+ case BuiltinType::Bool:
+ case BuiltinType::Char_S:
+ case BuiltinType::Char_U:
+ case BuiltinType::SChar:
+ case BuiltinType::UChar:
+ case BuiltinType::Short:
+ case BuiltinType::UShort:
+ case BuiltinType::Int:
+ case BuiltinType::UInt:
+ assert(BitWidth == IntSize);
+ // Here promotion occurs and it only depends on signedness.
+ return Field->getType()->isSignedIntegerType() ? IntTy : UnsignedIntTy;
+ default:
+ // Types having rank greater than int are not subject to promotions.
+ return QualType();
+ }

This should use getIntegerRank instead of a switch.

Now using getIntegerTypeOrder (because it accepts QualType arguments).

Would you please include a test of some sort? You can use the test I
committed for the previous patch as a model.

-Eli

Please find attached the revised patch (adding some tests to bitfield-promote.c) as well as new test file bitfield-promote-int-16bit.c.

Cheers,
Enea Zaffanella

BitField_Promotion_rev2.patch (7.42 KB)

bitfield-promote-int-16bit.c (564 Bytes)

Applied in r79510 with some minor fixes.

In the future, please use "svn add" to integrate the new test into the patch.

-Eli