[cfe-dev] Fix for PR4701

[Daniel Dunbar]

On Tue, Aug 18, 2009 at 6:19 AM, David Chisnall<csdavec at swansea.ac.uk> wrote:
> Hi Daniel,
>
> On 18 Aug 2009, at 07:16, Daniel Dunbar wrote:
>
>> Hi David,
>>
>> I'd like to sort out what we are doing with IsaExpr. I still don't see
>> a reason to complicate codegen and other clients with this expression.
>> This could just be because I don't want to implement support for it
>> (*grin*), but I think that if we really care about being future
>> looking we would move people to runtime functions; a custom expression
>> node for syntax which is identical to other general language syntax is
>> pretty gross...
>
> I left the isa stuff for Steve to decide about.  The idea behind it
> (explained in Steve's very long email before the big change last month) was
> that isa should be a language feature, rather than an explicit ivar.  This
> simplifies base classes, where it's relatively easy to forget to declare the
> isa ivar and then have the compiler generate broken code, but given how rare
> writing base classes actually is, I'm not fully convinced that it's needed.

Ok, I'll bug Steve about this.

>> Also, I noticed a regression with this patch, the problem stems from this:
>> --
>> +    if (BaseType != Context.ObjCIdRedefinitionType) {
>> +      BaseType = Context.ObjCIdRedefinitionType;
>> --
>> is this supposed to be ==? Otherwise it doesn't make sense, since
>> Context.ObjCIdRedefitionType gets initialized to the null QualType.
>
> If a definition is supplied for the id type, then
> Context.ObjCIdRedefinitionType will have been filled in by Sema.  If not,
> then BaseType will already be an opaque type and so casting it should make
> no difference (although I can add an explicit test if you prefer).  Can you
> give me an example in which this causes regressions?  I think this now means
> that the ObjCIsaExpr construction section is no longer reached, which I can
> fix if Steve decides we want to keep ObjCIsaExpr or remove completely if he
> doesn't, but ->isa should still work in cases where the type is defined (it
> does for me, anyway...).

I think you missed the point here.

If ObjCIdRedefinitionType has not been assigned, then it will be the
null QualType. The comparison will succeed (since BaseType is not
null), then BaseType will be assigned to the null type, then the code
will crash a few lines later.

 - Daniel