Improper implicit pointer cast in AST

[Richard Smith]

On Fri, Sep 26, 2014 at 1:12 AM, Abramo Bagnara <abramo.bagnara at bugseng.com>
wrote:

> 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?
>

Oh wow, you can't leave those C folks alone for a minute. Look at

  http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1439.pdf

They made 'void' an object type. *sigh*

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


Me too, if only the C11 spec said what we thought it did. I think we should
carry on as if the C spec said the sane thing (no conversion if both
operands are of void pointer types).

> 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?


Yes, that sounds good to me.

> 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?


Yes.

> 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...


I think you are introducing a behavioral change here: we used to always
cast the two pointers to be pointers into the same address space. We no
longer do so. This needs test coverage, and you may need to add a
diagnostic for the address-space-mismatch case. (I'm also not sure this
change is correct, but equally I'm not sure it's wrong...)

> 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?
>

See above.

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


Looks good apart from the address space cast issue.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141016/d6daba73/attachment.html>