Improper implicit pointer cast in AST

[Abramo Bagnara]

Il 26/09/2014 00:28, Richard Smith ha scritto:
> +      if ((LHSIsNull && !RHSIsNull) ||
> +          (LCanPointeeTy->isIncompleteOrObjectType() &&
> +           RCanPointeeTy->isVoidType()))
> 
> I don't think this is right: isIncompleteOrObjectType returns true for
> 'void', but the C standard says we do no conversions if both types are
> some flavour of void*. It also returns false for pointer-to-function

The condition in patch mimics exactly what I read in C99. I've just
noted that in C11, incomplete types have been excluded. We should have
different behavior in the two standards?

As C99 leads to unspecified cast for cases like (void*) ptr1 == (const
void*) ptr2, I'd prefer to use the C11 superior specification.

> types, which we support here as an extension. Also, you should handle

Do you mean that we want to interpret the condition "is a pointer to
object type" as "is not a pointer to void", right?

> the case where one operand is (void*)0 and the other operand is T* and
> non-null. Per the C standard, first we convert the null pointer to the
> other type, and *then* we convert towards the void* type (if it's still
> present), so in this case we convert to T* not to void*.

If I've understood what you mean you're saying that with current condition:

(T*) ptr == (void*)0 is modified to (void*) (T*) ptr == (void*) 0

while

(void*) 0 == (T*) ptr is modified to (void*) 0 == (void*) (T*) ptr

So, to gain some simmetry, we should interpret the two conditions in
standard in a short circuit way (i.e. if X then do A else if Y then do B).

Right?

> 
> Also, I think you still need an implicit conversion here if the address
> space is different, even if nothing else is (but I don't know which
> direction we should convert in that case).

It does not seems to me a good idea: it seems to me rather insane to
accept comparison of pointers with different address spaces...

> Please also provide some CodeGen tests showing that we do the right
> thing here, in particular if the address spaces differ.

The changes should never change generated code, I'm missing something?

I've attached a revised patch to explain how I've interpreted the issues
you've noted.

-- 
Abramo Bagnara

BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara at bugseng.com
-------------- next part --------------
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp	(revisione 218504)
+++ lib/Sema/SemaChecking.cpp	(copia locale)
@@ -5849,7 +5849,8 @@
 static void AnalyzeComparison(Sema &S, BinaryOperator *E) {
   // The type the comparison is being performed in.
   QualType T = E->getLHS()->getType();
-  assert(S.Context.hasSameUnqualifiedType(T, E->getRHS()->getType())
+  assert((S.Context.hasSameUnqualifiedType(T, E->getRHS()->getType()) ||
+          !S.getLangOpts().CPlusPlus)
          && "comparison with mismatched types");
   if (E->isValueDependent())
     return AnalyzeImpConvsInComparison(S, E);
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp	(revisione 218504)
+++ lib/Sema/SemaExpr.cpp	(copia locale)
@@ -8045,10 +8045,17 @@
       unsigned AddrSpaceR = RCanPointeeTy.getAddressSpace();
       CastKind Kind = AddrSpaceL != AddrSpaceR ? CK_AddressSpaceConversion
                                                : CK_BitCast;
+      // C11 6.5.9p5
       if (LHSIsNull && !RHSIsNull)
         LHS = ImpCastExprToType(LHS.get(), RHSType, Kind);
-      else
+      else if ((!LHSIsNull && RHSIsNull)
         RHS = ImpCastExprToType(RHS.get(), LHSType, Kind);
+      else if (!LCanPointeeTy->isVoidType() &&
+               RCanPointeeTy->isVoidType()))
+        LHS = ImpCastExprToType(LHS.get(), RHSType, Kind);
+      else if (LCanPointeeTy->isVoidType() &&
+               !RCanPointeeTy->isVoidType())
+        RHS = ImpCastExprToType(RHS.get(), LHSType, Kind);
     }
     return ResultTy;
   }