bugGNUstep - Bugs: bug #41111, C++ destructors not called...

Group
 
 

bug #41111: C++ destructors not called correctly in finalize breaking objc_setAssociatedObject

Submitter:  Doug Warren <doug_w>
Submitted:  Sun 05 Jan 2014 06:30:36 PM UTC
   
 
Category:  Base/Foundation Severity:  3 - Normal
Item Group:  Bug Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  In Test
* Mandatory Fields

Add a New Comment Rich Markup
   

Tue 07 Jan 2014 10:46:54 AM UTC, comment #3: 

Hmm … after spending a long time looking at this (and into the runtime code a little), it seems the report is rather misleading.

Firstly, the test case is broken to always fail because it depends on the associated object being deallocated, and that object is in an autorelease pool which won't be deallocated until after the test takes place.
i fixed that.

Secondly, the two code snippets do behave identically on my system at least; the suggested code fix doesn't work.

It seems the actual problem is in the logic of the check for small objects earlier … which was causing the deallocation to start with the wrong class.  I reversed the logic of the test in svn trunk, and I think that fixes the issue.

Richard Frith-Macdonald <CaS>
Group Member
Mon 06 Jan 2014 11:44:26 AM UTC, comment #2: 

Thanks for the testcase … I added it.
But, please could you clarify if I've misunderstood or if this is a libobjc bug rather than a base bug?

Richard Frith-Macdonald <CaS>
Group Member
Sun 05 Jan 2014 07:32:37 PM UTC, comment #1: 

I'm confused.
As far as I can see the two code snippets should have identical effects.
Both step up the class hierarchy, and both call each destructor implementation found.
The only difference seems to be that one stops traversing the hierarchy after the last destructor is called, and the other continues traversing the hierarchy up to the root class.
However, the destructorClass variable is not used … after this, so whether its the first superclass without a destructor or the root class doesn't matter.

Unless … perhaps this is illustrating some bug in class_respondsToSelector() ?
Are you saying that this is returning NO for some cases when it should return YES?
My understanding is that this function should return YES if instances of a class respond to a selector (ie if the method is implemented by the class or by any of its superclasses).

Richard Frith-Macdonald <CaS>
Group Member
Sun 05 Jan 2014 06:30:36 PM UTC, original submission:  

NSObject -finalize method does not call C++ destructors correctly in every case.  This breaks libobjc2's objc_setAssociatedObject retain policy.  I've attached a GNUStep style test that fails to demonstrate this.

The problem looks like it was this optimization:
  /* C++ destructors must be called in the opposite order to their
   * creators, so start at the leaf class and then go up the tree until we
   * get to the root class.  As a small optimisation, we don't bother
   * visiting any classes that don't have an implementation of this method
   * (including one inherited from a superclass).
   *
   * Care must be taken not to call inherited .cxx_destruct methods.
   */
  while (class_respondsToSelector(destructorClass, cxx_destruct))
    {
      IMP newDestructor;

      newDestructor
= class_getMethodImplementation(destructorClass, cxx_destruct);
      destructorClass = class_getSuperclass(destructorClass);

      if (newDestructor != destructor)
  {
  newDestructor(self, cxx_destruct);
  destructor = newDestructor;
}
    }

In my local tree I've replaced it with:
while (destructorClass)
{
if (class_respondsToSelector(destructorClass, cxx_destruct))
{
IMP newDestructor = class_getMethodImplementation(destructorClass, cxx_destruct);
destructorClass = class_getSuperclass(destructorClass);

if (newDestructor != destructor)
{
newDestructor(self, cxx_destruct);
destructor = newDestructor;
}
}
else
{
destructorClass = class_getSuperclass(destructorClass);
}
}

And I've been using it for about a month now with no ill effect.

Doug Warren <doug_w>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #30195:  associated.m added by doug_w (1KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by CaS (Posted a comment)
  • -email is unavailable- added by doug_w (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-01-07 CaS StatusNeed Info Fixed
        Open/ClosedOpen In Test
    2014-01-06 CaS StatusNone Need Info
    2014-01-05 doug_w Attached File- Added associated.m, #30195

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code