Type>>isObjCInterfaceType() blows up

Ahoy!

I’m trying to determine the SourceLocation of an Objective-C instance variable’s type, if there is one. This seemed like a reasonable way to start:

clang::ObjCIvarDecl* iVar = … //your ivar here…
clang::Type* t = iVar->getType().getTypePtr();
t->isObjCInterfaceType();

Unfortunately, I get an EXC_BAD_ACCESS on the final line. For the test case I’m working on, I would have expected it to simply return false, since my ivar is an int.

Thanks in advance,

Emerson

What does iVar->getType().dump() print?

Does ‘iVar->getType()->isObjCInterfaceType()’ work?

-Chris

Ahoy!

I’m trying to determine the SourceLocation of an Objective-C instance variable’s type, if there is one. This seemed like a reasonable way to start:

clang::ObjCIvarDecl* iVar = … //your ivar here…
clang::Type* t = iVar->getType().getTypePtr();
t->isObjCInterfaceType();

Unfortunately, I get an EXC_BAD_ACCESS on the final line. For the test case I’m working on, I would have expected it to simply return false, since my ivar is an int.

I don’t know if this is your problem, but…

There shouldn’t be a reason to use getTypePtr() explicitly…just do iVar->getType()->isObjCInterfaceType()…

Another point…since most types are uniqued, we don’t have a SourceLocation to identify them.

snaroff

Solution:
I was using ASTParser.cpp, which allocated an ASTContext on the stack:

ASTContext Context(PP.getSourceManager(), PP.getTargetInfo(),
PP.getIdentifierTable(), PP.getSelectorTable());

When I returned from the method, the ASTContext gets destroyed, after which I wasn’t getting valid types. Changed the line in ASTParser to allocate the ASTContext on the heap:

ASTContext* Context = new ASTContext(PP.getSourceManager(), PP.getTargetInfo(),
PP.getIdentifierTable(), PP.getSelectorTable());

And isObjCInterfaceType works as expected. Thanks to Ted, Chris, and Steve for helping me out with this.

-e

Interesting. I’m sure you’re not the last person to be bitten by this…

Chris, do you think the API should try and protect against this type of mistake?

Seems like we should have a clang::ASTContext::Create() function that removes the need for a public constructor.

snaroff

ASTContext is very useful as a stack-based data structure. I'm not sure what the right fix is, but I don't really want to lose this ability...

-Chris

Chris Lattner wrote:

ASTContext is very useful as a stack-based data structure. I'm not sure what the right fix is, but I don't really want to lose this ability...

What do you mean by useful? (That's an honest question.)

Emerson's proposed patch would allow TranslationUnit objects to safely delete their ASTContext objects. (Right now, the ASTContext objects are created on the stack in the serializing case and on the heap in deserializing case. I noticed this when tracking down memory leaks.)

Sam

If it makes the two cases more uniform, I would be happy to "give up" stack allocation of ASTContext, particularly if the new scheme is for TranslationUnit to always own it.

-Chris

One aside: if TranslationUnit always owns ASTContext, then it doesn't have to be explicitly allocated in TranslationUnit, just kept as a field. TranslationUnit can in turn be stack or heap allocated.

I agree with Chris that ASTContext (and numerous other data structures in clang) are useful as stack-allocated objects. Different clients can choose to stack or heap allocate ASTcontext, depending on their needs. As long as their is a clear ownership policy (which includes a stack frame "owning" an ASTContext object), then there shouldn't be any issues. Forcing ASTContext to be heap allocated just potentially turns one set of bugs (using released stack memory) into another (memory leaks from the heap) when we don't reason well about the lifetime of an ASTContext (or any other) object.

Ted Kremenek wrote:

One aside: if TranslationUnit always owns ASTContext, then it doesn't
have to be explicitly allocated in TranslationUnit, just kept as a
field. TranslationUnit can in turn be stack or heap allocated.

I see where I was confused now. I was under the impression that
deserialized non-POD types had to be heap allocated. And indeed, the
heap versus stack problem I mentioned earlier would go away if
ASTContext objects were always created (however that may be) as a
TranslationUnit member.

Sorry about the noise!

Sam

That's right; non-POD types don't have to be heap allocated to work with the deserializer.

For such objects we don't use EmitOwnedPtr/ReadOwnedPtr, but instead manually register their persistent references using EmitPtr (which only writes an integer to the bitcode file representing a serialized handle for the object, not the contents of the object itself). In fact, EmitOwnedPtr and ReadOwnedPtr are implemented on top of EmitPtr and ReadPtr.

This allows us to handle in the serializer cases where an object A has references to fields in an object B.

Chris Lattner wrote:

ASTContext is very useful as a stack-based data structure. I'm
not sure what the right fix is, but I don't really want to lose
this ability...

What do you mean by useful? (That's an honest question.)

Emerson's proposed patch would allow TranslationUnit objects to
safely delete their ASTContext objects. (Right now, the ASTContext
objects are created on the stack in the serializing case and on the
heap in deserializing case. I noticed this when tracking down
memory leaks.)

If it makes the two cases more uniform, I would be happy to "give up"
stack allocation of ASTContext, particularly if the new scheme is for
TranslationUnit to always own it.

One aside: if TranslationUnit always owns ASTContext, then it doesn't
have to be explicitly allocated in TranslationUnit, just kept as a
field. TranslationUnit can in turn be stack or heap allocated.

I agree with Chris that ASTContext (and numerous other data structures
in clang) are useful as stack-allocated objects. Different clients
can choose to stack or heap allocate ASTcontext, depending on their
needs. As long as their is a clear ownership policy (which includes a
stack frame "owning" an ASTContext object), then there shouldn't be
any issues. Forcing ASTContext to be heap allocated just potentially
turns one set of bugs (using released stack memory) into another
(memory leaks from the heap) when we don't reason well about the
lifetime of an ASTContext (or any other) object.

owns long-lived heap-allocated AST nodes seems kind of odd. Maybe it's my ObjC/Java background/bias showing through (i.e. both of these languages are reference based).

I guess you guys like the syntactic convenience of not having to explicitly delete the object...which I can understand.

That said, I think we need to keep our eye on making the API as straightforward as possible. Since many developers will be coming to us with Java/ObjC backgrounds, it is interesting to see what potholes people fall into. In this instance, I don't think we need to do anything now (I agree).

snaroff