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

[John McCall]

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.

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.