Bug #:

Submitted by:
Reviewed by:

The following code illustrates a bug in the semantic analysis for assignments:

int func() {
  int *P;
  char *x;
  P = x; // type of this assignment expression should be "int *", NOT "char *".
}

While the type checking/diagnostics are correct, the type of the assignment
expression is incorrect (which shows up during code gen).  With the fix,
the llvm code looks correct...

[dylan:~/llvm/tools/clang] admin% ../../Debug/bin/clang cast.c -emit-llvm
cast.c:4:5: warning: incompatible pointer types assigning 'char *' to 'int *'
  P = x; // type of assignment expression is "int *", NOT "char *".
  ~ ^ ~
; ModuleID = 'foo'

define i32 @func() {
entry:
        %P = alloca i32*                ; <i32**> [#uses=1]
        %x = alloca i8*         ; <i8**> [#uses=1]
        %allocapt = bitcast i32 undef to i32            ; <i32> [#uses=0]
        %tmp = load i8** %x             ; <i8*> [#uses=1]
        %conv = bitcast i8* %tmp to i32*                ; <i32*> [#uses=1]
        store i32* %conv, i32** %P
        ret i32 undef
}

Even though the fix was simple, I decided to rename/refactor the surrounding code
to make a clearer distinction between constraint checking and conversion.

- Renamed AssignmentConversionResult -> AssignmentCheckResult.
- Renamed UsualAssignmentConversions -> CheckAssignmentConstraints.
- Changed the return type of CheckAssignmentConstraints and CheckPointerTypesForAssignment
from QualType -> AssignmentCheckResult. These routines no longer take a reference to the result (obviously).
- Changed CheckAssignmentOperands to return the correct type (with spec annotations).

llvm-svn: 39601

clang/Sema/Sema.h

@@ -273,7 +273,7 @@ private:
   // or array, this routine simply returns the input type (unmodified).
   QualType DefaultFunctionArrayConversion(QualType t);
   
-  enum AssignmentConversionResult {
+  enum AssignmentCheckResult {
     Compatible,
     Incompatible,
     PointerFromInt, 
@@ -281,14 +281,13 @@ private:
     IncompatiblePointer,
     CompatiblePointerDiscardsQualifiers
   };
-  // UsualAssignmentConversions - conversions for assignment, argument passing, 
+  // CheckAssignmentConstraints - conversions for assignment, argument passing, 
   // variable initialization, and function return values. Currently used by 
-  // CheckAssignmentOperands, ParseCallExpr, and ParseReturnStmt. 
+  // CheckAssignmentOperands, ParseCallExpr, and ParseReturnStmt.  C99 6.5.16.
-  QualType UsualAssignmentConversions(QualType lhs, QualType rhs, // C99 6.5.16
+  AssignmentCheckResult CheckAssignmentConstraints(QualType lhs, QualType rhs);
-                                      AssignmentConversionResult &r);
+  // Helper function for CheckAssignmentConstraints (C99 6.5.16.1p1)
-  // Helper function for UsualAssignmentConversions (C99 6.5.16.1p1)
+  AssignmentCheckResult CheckPointerTypesForAssignment(QualType lhsType, 
-  QualType CheckPointerTypesForAssignment(QualType lhsType, QualType rhsType,
+                                                       QualType rhsType);
-                                          AssignmentConversionResult &r);
   
   /// the following "Check" methods will return a valid/converted QualType
   /// or a null QualType (indicating an error diagnostic was issued).

clang/Sema/SemaExpr.cpp

@@ -415,9 +415,9 @@ ParseCallExpr(ExprTy *Fn, SourceLocation LParenLoc,
       
       if (lhsType == rhsType) // common case, fast path...
         continue;
-      AssignmentConversionResult result;
+        
-      UsualAssignmentConversions(lhsType, rhsType, result);
+      AssignmentCheckResult result = CheckAssignmentConstraints(lhsType,
-
+                                                                rhsType);
       SourceLocation l = argExpr->getLocStart();
 
       // decode the result (notice that AST's are still created for extensions).
@@ -634,9 +634,8 @@ QualType Sema::UsualArithmeticConversions(QualType t1, QualType t2) {
 // routine is it effectively iqnores the qualifiers on the top level pointee.
 // This circumvents the usual type rules specified in 6.2.7p1 & 6.7.5.[1-3].
 // FIXME: add a couple examples in this comment.
-QualType Sema::CheckPointerTypesForAssignment(QualType lhsType, 
+Sema::AssignmentCheckResult 
-                                              QualType rhsType,
+Sema::CheckPointerTypesForAssignment(QualType lhsType, QualType rhsType) {
-                                              AssignmentConversionResult &r) {
   QualType lhptee, rhptee;
   
   // get the "pointed to" type (ignoring qualifiers at the top level)
@@ -647,6 +646,8 @@ QualType Sema::CheckPointerTypesForAssignment(QualType lhsType,
   lhptee = lhptee.getCanonicalType();
   rhptee = rhptee.getCanonicalType();
 
+  AssignmentCheckResult r = Compatible;
+  
   // C99 6.5.16.1p1: This following citation is common to constraints 
   // 3 & 4 (below). ...and the type *pointed to* by the left has all the 
   // qualifiers of the type *pointed to* by the right; 
@@ -668,10 +669,10 @@ QualType Sema::CheckPointerTypesForAssignment(QualType lhsType,
   else if (!Type::typesAreCompatible(lhptee.getUnqualifiedType(), 
                                      rhptee.getUnqualifiedType()))
     r = IncompatiblePointer; // this "trumps" PointerAssignDiscardsQualifiers
-  return rhsType;
+  return r;
 }
 
-/// UsualAssignmentConversions (C99 6.5.16) - This routine currently 
+/// CheckAssignmentConstraints (C99 6.5.16) - This routine currently 
 /// has code to accommodate several GCC extensions when type checking 
 /// pointers. Here are some objectionable examples that GCC considers warnings:
 ///
@@ -688,44 +689,37 @@ QualType Sema::CheckPointerTypesForAssignment(QualType lhsType,
 /// C99 spec dictates. 
 /// Note: the warning above turn into errors when -pedantic-errors is enabled. 
 ///
-QualType Sema::UsualAssignmentConversions(QualType lhsType, QualType rhsType,
+Sema::AssignmentCheckResult
-                                          AssignmentConversionResult &r) {
+Sema::CheckAssignmentConstraints(QualType lhsType, QualType rhsType) {
   // This check seems unnatural, however it is necessary to insure the proper
   // conversion of functions/arrays. If the conversion were done for all
   // DeclExpr's (created by ParseIdentifierExpr), it would mess up the unary
   // expressions that surpress this implicit conversion (&, sizeof).
   rhsType = DefaultFunctionArrayConversion(rhsType);
     
-  r = Compatible;
   if (lhsType->isArithmeticType() && rhsType->isArithmeticType())
-    return lhsType;
+    return Compatible;
   else if (lhsType->isPointerType()) {
-    if (rhsType->isIntegerType()) {
+    if (rhsType->isIntegerType())
-      r = PointerFromInt;
+      return PointerFromInt;
-      return rhsType;
+      
-    }
     if (rhsType->isPointerType())
-      return CheckPointerTypesForAssignment(lhsType, rhsType, r);
+      return CheckPointerTypesForAssignment(lhsType, rhsType);
   } else if (rhsType->isPointerType()) {
-    if (lhsType->isIntegerType()) {
+    // C99 6.5.16.1p1: the left operand is _Bool and the right is a pointer.
-      // C99 6.5.16.1p1: the left operand is _Bool and the right is a pointer.
+    if ((lhsType->isIntegerType()) && (lhsType != Context.BoolTy))
-      if (lhsType != Context.BoolTy)
+      return IntFromPointer;
-        r = IntFromPointer;
+
-      return rhsType;
-    }
     if (lhsType->isPointerType()) 
-      return CheckPointerTypesForAssignment(lhsType, rhsType, r);
+      return CheckPointerTypesForAssignment(lhsType, rhsType);
   } else if (isa<TagType>(lhsType) && isa<TagType>(rhsType)) {
     if (Type::tagTypesAreCompatible(lhsType, rhsType))
-      return rhsType;
+      return Compatible;
   } else if (lhsType->isReferenceType() || rhsType->isReferenceType()) {
     if (Type::referenceTypesAreCompatible(lhsType, rhsType))
-      // C++ 5.17p1: ...the type of the assignment exression is that of its left
+      return Compatible;
-      // operand.
-      return lhsType;
   }
-  r = Incompatible;
+  return Incompatible;
-  return QualType();
 }
 
 inline void Sema::InvalidOperands(SourceLocation loc, Expr *lex, Expr *rex) {
@@ -898,8 +892,7 @@ inline QualType Sema::CheckAssignmentOperands( // C99 6.5.16.1
   if (lhsType == rhsType) // common case, fast path...
     return lhsType;
   
-  AssignmentConversionResult result;
+  AssignmentCheckResult result = CheckAssignmentConstraints(lhsType, rhsType);
-  QualType resType = UsualAssignmentConversions(lhsType, rhsType, result);
 
   // decode the result (notice that extensions still return a type).
   switch (result) {
@@ -935,7 +928,13 @@ inline QualType Sema::CheckAssignmentOperands( // C99 6.5.16.1
          lex->getSourceRange(), rex->getSourceRange());
     break;
   }
-  return hadError ? QualType() : resType;
+  // C99 6.5.16p3: The type of an assignment expression is the type of the
+  // left operand unless the left operand has qualified type, in which case
+  // it is the unqualified version of the type of the left operand. 
+  // C99 6.5.16.1p2: In simple assignment, the value of the right operand
+  // is converted to the type of the assignment expression (above).
+  // C++ 5.17p1: the type of the assignment expression is that of its left oprdu.
+  return hadError ? QualType() : lhsType.getUnqualifiedType();
 }
 
 inline QualType Sema::CheckCommaOperands( // C99 6.5.17

clang/Sema/SemaStmt.cpp

@@ -241,8 +241,7 @@ Sema::ParseReturnStmt(SourceLocation ReturnLoc, ExprTy *RetValExp) {
   // C99 6.8.6.4p3(136): The return statement is not an assignment. The 
   // overlap restriction of subclause 6.5.16.1 does not apply to the case of 
   // function return.  
-  AssignmentConversionResult result;
+  AssignmentCheckResult result = CheckAssignmentConstraints(lhsType, rhsType);
-  QualType resType = UsualAssignmentConversions(lhsType, rhsType, result);
   bool hadError = false;
   
   // decode the result (notice that extensions still return a type).

clang/clang.xcodeproj/project.pbxproj

@@ -10,6 +10,8 @@
 		1A30A9E90B93A4C800201A91 /* ExprCXX.h in CopyFiles */ = {isa = PBXBuildFile; fileRef = 1A30A9E80B93A4C800201A91 /* ExprCXX.h */; };
 		1A869A700BA2164C008DA07A /* LiteralSupport.h in CopyFiles */ = {isa = PBXBuildFile; fileRef = 1A869A6E0BA2164C008DA07A /* LiteralSupport.h */; };
 		1A869AA80BA21ABA008DA07A /* LiteralSupport.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 1A869AA70BA21ABA008DA07A /* LiteralSupport.cpp */; };
+		84916BE50C161E580080778F /* Attr.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 84916BE40C161E580080778F /* Attr.cpp */; };
+		84916BE70C161E800080778F /* Attr.h in CopyFiles */ = {isa = PBXBuildFile; fileRef = 84916BE60C161E800080778F /* Attr.h */; };
 		DE01DA490B12ADA300AC22CE /* PPCallbacks.h in CopyFiles */ = {isa = PBXBuildFile; fileRef = DE01DA480B12ADA300AC22CE /* PPCallbacks.h */; };
 		DE06756C0C051CFE00EBBFD8 /* ParseExprCXX.cpp in Sources */ = {isa = PBXBuildFile; fileRef = DE06756B0C051CFE00EBBFD8 /* ParseExprCXX.cpp */; };
 		DE06B73E0A8307640050E87E /* LangOptions.h in CopyFiles */ = {isa = PBXBuildFile; fileRef = DE06B73D0A8307640050E87E /* LangOptions.h */; };
@@ -106,6 +108,23 @@
 		DED7D9E50A5257F6003AD0FB /* ScratchBuffer.cpp in Sources */ = {isa = PBXBuildFile; fileRef = DED7D9E40A5257F6003AD0FB /* ScratchBuffer.cpp */; };
 /* End PBXBuildFile section */
 
+/* Begin PBXBuildStyle section */
+		84916C3B0C1736920080778F /* Development */ = {
+			isa = PBXBuildStyle;
+			buildSettings = {
+				COPY_PHASE_STRIP = NO;
+			};
+			name = Development;
+		};
+		84916C3C0C1736920080778F /* Deployment */ = {
+			isa = PBXBuildStyle;
+			buildSettings = {
+				COPY_PHASE_STRIP = YES;
+			};
+			name = Deployment;
+		};
+/* End PBXBuildStyle section */
+
 /* Begin PBXCopyFilesBuildPhase section */
 		8DD76F690486A84900D96B5E /* CopyFiles */ = {
 			isa = PBXCopyFilesBuildPhase;
@@ -160,6 +179,7 @@
 				DE928B200C0565B000231DA4 /* ModuleBuilder.h in CopyFiles */,
 				DE928B7D0C0A615100231DA4 /* CodeGenModule.h in CopyFiles */,
 				DE928B810C0A615B00231DA4 /* CodeGenFunction.h in CopyFiles */,
+				84916BE70C161E800080778F /* Attr.h in CopyFiles */,
 			);
 			runOnlyForDeploymentPostprocessing = 1;
 		};
@@ -169,6 +189,8 @@
 		1A30A9E80B93A4C800201A91 /* ExprCXX.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; name = ExprCXX.h; path = clang/AST/ExprCXX.h; sourceTree = "<group>"; };
 		1A869A6E0BA2164C008DA07A /* LiteralSupport.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = LiteralSupport.h; sourceTree = "<group>"; };
 		1A869AA70BA21ABA008DA07A /* LiteralSupport.cpp */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.cpp; path = LiteralSupport.cpp; sourceTree = "<group>"; };
+		84916BE40C161E580080778F /* Attr.cpp */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.cpp; name = Attr.cpp; path = AST/Attr.cpp; sourceTree = "<group>"; };
+		84916BE60C161E800080778F /* Attr.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; name = Attr.h; path = include/clang/AST/Attr.h; sourceTree = "<group>"; };
 		8DD76F6C0486A84900D96B5E /* clang */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.executable"; includeInIndex = 0; path = clang; sourceTree = BUILT_PRODUCTS_DIR; };
 		DE01DA480B12ADA300AC22CE /* PPCallbacks.h */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.c.h; path = PPCallbacks.h; sourceTree = "<group>"; };
 		DE06756B0C051CFE00EBBFD8 /* ParseExprCXX.cpp */ = {isa = PBXFileReference; fileEncoding = 30; lastKnownFileType = sourcecode.cpp.cpp; name = ParseExprCXX.cpp; path = Parse/ParseExprCXX.cpp; sourceTree = "<group>"; };
@@ -432,6 +454,8 @@
 				DED677C80B6C854100AAD4A3 /* Builtins.cpp */,
 				DED62ABA0AE2EDF1001E80A4 /* Decl.cpp */,
 				DE0FCB330A9C21F100248FD5 /* Expr.cpp */,
+				84916BE60C161E800080778F /* Attr.h */,
+				84916BE40C161E580080778F /* Attr.cpp */,
 				DE3452400AEF1A2D00DBC861 /* Stmt.cpp */,
 				DE75EDF00B06880E0020CF81 /* Type.cpp */,
 				DE34621C0AFEB19B00DBC861 /* StmtPrinter.cpp */,
@@ -547,6 +571,12 @@
 		08FB7793FE84155DC02AAC07 /* Project object */ = {
 			isa = PBXProject;
 			buildConfigurationList = 1DEB923508733DC60010E9CD /* Build configuration list for PBXProject "clang" */;
+			buildSettings = {
+			};
+			buildStyles = (
+				84916C3B0C1736920080778F /* Development */,
+				84916C3C0C1736920080778F /* Deployment */,
+			);
 			hasScannedForEncodings = 1;
 			mainGroup = 08FB7794FE84155DC02AAC07 /* clang */;
 			projectDirPath = "";
@@ -611,6 +641,7 @@
 				DE4772FA0C10EAE5002239E8 /* CGStmt.cpp in Sources */,
 				DE4772FC0C10EAEC002239E8 /* CGExpr.cpp in Sources */,
 				DE4264FC0C113592005A861D /* CGDecl.cpp in Sources */,
+				84916BE50C161E580080778F /* Attr.cpp in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};