Fix for PR4701

Hi Daniel,

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.

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

David

Hi Daniel,

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

Once again, can you provide a test case which demonstrates this? I have tests which attempt to access fields on an id-typed variable with no definition of id provided, and they do not crash and do use this code path. If you can demonstrate a condition where this manifests as a bug, then I would be more than happy to fix it. As, at the moment, it is working as I expect it to work, so I can't fix it because I am not seeing any breakage.

The comparison is only there to avoid a redundant implicit cast when there is one already (which can only happen in cases where a redefinition is supplied).

David

Ah, the problem appears to be related to the use of PCH:

David,

Have you had a chance to look at this crash yet? This is preventing us
from building various projects so I'd like to get it resolved soon.

- Daniel

Hi Daniel,

I've not yet - I don't know my way around the PCH code at all. I believe the problem is that Sema::MergeTypeDefDecl() will be called when building the PCH, but not when loading it. This sets the redefinition type, so without the call it is not properly loaded. The fix would make sure that Context.ObjCIdRedefinitionType and Context.ObjCClassRedefinitionType were saved in the PCH and loaded again afterwards, but I don't know enough about how PCH handling works to fix this.

I think the fix is to add lines to PCHWriter::WriteMetadata() to write these and corresponding lines in PCHReader:: to restore them, but maybe someone who knows how the PCH stuff works could comment?

David

This is fixed in r79583.

   - Doug