[cfe-commits] r153778 - /cfe/trunk/lib/CodeGen/CGObjCMac.cpp

[jahanian]

On Mar 30, 2012, at 3:25 PM, John McCall wrote:

> On Mar 30, 2012, at 2:45 PM, jahanian wrote:
>> On Mar 30, 2012, at 2:29 PM, John McCall wrote:
>>> Author: rjmccall
>>> Date: Fri Mar 30 16:29:05 2012
>>> New Revision: 153778
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=153778&view=rev
>>> Log:
>>> Fix a pair of invalidation bugs when emitting protocol definitions
>>> in the fragile and non-fragile Mac ObjC runtimes.  No useful test
>>> case.  Fixes rdar://problem/11072576.
>>> 
>>> Modified:
>>>  cfe/trunk/lib/CodeGen/CGObjCMac.cpp
>>> 
>>> Modified: cfe/trunk/lib/CodeGen/CGObjCMac.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGObjCMac.cpp?rev=153778&r1=153777&r2=153778&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/CodeGen/CGObjCMac.cpp (original)
>>> +++ cfe/trunk/lib/CodeGen/CGObjCMac.cpp Fri Mar 30 16:29:05 2012
>>> @@ -1914,7 +1914,7 @@
>>> See EmitProtocolExtension().
>>> */
>>> llvm::Constant *CGObjCMac::GetOrEmitProtocol(const ObjCProtocolDecl *PD) {
>>> -  llvm::GlobalVariable *&Entry = Protocols[PD->getIdentifier()];
>>> +  llvm::GlobalVariable *Entry = Protocols[PD->getIdentifier()];
>>> 
>>> // Early exit if a defining object has already been generated.
>>> if (Entry && Entry->hasInitializer())
>>> @@ -1997,6 +1997,8 @@
>>>   Entry->setSection("__OBJC,__protocol,regular,no_dead_strip");
>>>   // FIXME: Is this necessary? Why only for protocol?
>>>   Entry->setAlignment(4);
>>> +
>>> +    Protocols[PD->getIdentifier()] = Entry;
>> 
>> A test case would have been nice. It uclear to me what is being fixed any why above assignment is necessary.
>> A brief description of the problem and the fix is the next best thing.
> 
> Sorry, I thought "invalidation bug" covered it.  The code was keeping
> a reference into the Protocols table across the emission of the protocol
> body.  Unfortunately, the emission of the protocol may require the
> emission of references to other protocols, which in turn may require
> modifying the Protocols table and therefore invalidating the original
> reference.

Got it. Thanks much for the explanation. 

- Fariborz

> 
> The test case is basically to make a ton of protocols that imply other
> protocols and hope they trigger the right circumstances to crash the
> compiler.  I generally don't think that's a very useful kind of test to
> keep around, since it's a memory bug and can be easily masked by
> changes in the data structure implementation.
> 
> John.