RFR: 8185746: Remove Mutex destructor assertion

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR: 8185746: Remove Mutex destructor assertion

Kim Barrett
Please review this small change to improve the debugging experience
when a mutex is destroyed in a bad state.

I've removed the assert in ~Mutex, which was making the same checks as
in ~Monitor, but providing much less information.

Also, the ~Monitor assert now (carefully) includes the name in the
error message, as that may also be very helpful information.

CR:
https://bugs.openjdk.java.net/browse/JDK-8185746

Webrev:
http://cr.openjdk.java.net/~kbarrett/8185746/hotspot.00/

Testing:
Destroyed a locked mutex and looked at the assert message.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8185746: Remove Mutex destructor assertion

David Holmes
Hi Kim,

On 3/08/2017 8:41 AM, Kim Barrett wrote:
> Please review this small change to improve the debugging experience
> when a mutex is destroyed in a bad state.
>
> I've removed the assert in ~Mutex, which was making the same checks as
> in ~Monitor, but providing much less information.

Okay. Please can you add a comment to the ~Mutex noting that eg:

// Defer any state checking to ~Monitor
Mutex::~Mutex() { }

> Also, the ~Monitor assert now (carefully) includes the name in the
> error message, as that may also be very helpful information.

Good strategy watching for a potentially corrupted name!

Thanks,
David
-----

> CR:
> https://bugs.openjdk.java.net/browse/JDK-8185746
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8185746/hotspot.00/
>
> Testing:
> Destroyed a locked mutex and looked at the assert message.
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8185746: Remove Mutex destructor assertion

Kim Barrett
> On Aug 2, 2017, at 7:23 PM, David Holmes <[hidden email]> wrote:
>
> Hi Kim,
>
> On 3/08/2017 8:41 AM, Kim Barrett wrote:
>> Please review this small change to improve the debugging experience
>> when a mutex is destroyed in a bad state.
>> I've removed the assert in ~Mutex, which was making the same checks as
>> in ~Monitor, but providing much less information.
>
> Okay. Please can you add a comment to the ~Mutex noting that eg:
>
> // Defer any state checking to ~Monitor
> Mutex::~Mutex() { }

Well, I was waffling over just removing it completely.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8185746: Remove Mutex destructor assertion

Kim Barrett
> On Aug 2, 2017, at 7:31 PM, Kim Barrett <[hidden email]> wrote:
>
>> On Aug 2, 2017, at 7:23 PM, David Holmes <[hidden email]> wrote:
>>
>> Hi Kim,
>>
>> On 3/08/2017 8:41 AM, Kim Barrett wrote:
>>> Please review this small change to improve the debugging experience
>>> when a mutex is destroyed in a bad state.
>>> I've removed the assert in ~Mutex, which was making the same checks as
>>> in ~Monitor, but providing much less information.
>>
>> Okay. Please can you add a comment to the ~Mutex noting that eg:
>>
>> // Defer any state checking to ~Monitor
>> Mutex::~Mutex() { }
>
> Well, I was waffling over just removing it completely.

Note also that the structure here makes it pretty easy to accidentally
invoke undefined behavior by destructor slicing. That's an entirely
different problem, but it bugs me...

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8185746: Remove Mutex destructor assertion

coleen.phillimore

Looks good.

On 8/2/17 7:36 PM, Kim Barrett wrote:

>> On Aug 2, 2017, at 7:31 PM, Kim Barrett <[hidden email]> wrote:
>>
>>> On Aug 2, 2017, at 7:23 PM, David Holmes <[hidden email]> wrote:
>>>
>>> Hi Kim,
>>>
>>> On 3/08/2017 8:41 AM, Kim Barrett wrote:
>>>> Please review this small change to improve the debugging experience
>>>> when a mutex is destroyed in a bad state.
>>>> I've removed the assert in ~Mutex, which was making the same checks as
>>>> in ~Monitor, but providing much less information.
>>> Okay. Please can you add a comment to the ~Mutex noting that eg:
>>>
>>> // Defer any state checking to ~Monitor
>>> Mutex::~Mutex() { }
>> Well, I was waffling over just removing it completely.
> Note also that the structure here makes it pretty easy to accidentally
> invoke undefined behavior by destructor slicing. That's an entirely
> different problem, but it bugs me...
Can you describe what you mean by "destructor slicing"?  Do you mean
calling ~Monitor on an instance of Mutex?

Thanks,
Coleen

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8185746: Remove Mutex destructor assertion

Kim Barrett
> On Aug 2, 2017, at 7:49 PM, [hidden email] wrote:
>
>
> Looks good.

Thanks.

> On 8/2/17 7:36 PM, Kim Barrett wrote:
>>> On Aug 2, 2017, at 7:31 PM, Kim Barrett <[hidden email]> wrote:
>>>
>>>> On Aug 2, 2017, at 7:23 PM, David Holmes <[hidden email]> wrote:
>>>>
>>>> Hi Kim,
>>>>
>>>> On 3/08/2017 8:41 AM, Kim Barrett wrote:
>>>>> Please review this small change to improve the debugging experience
>>>>> when a mutex is destroyed in a bad state.
>>>>> I've removed the assert in ~Mutex, which was making the same checks as
>>>>> in ~Monitor, but providing much less information.
>>>> Okay. Please can you add a comment to the ~Mutex noting that eg:
>>>>
>>>> // Defer any state checking to ~Monitor
>>>> Mutex::~Mutex() { }
>>> Well, I was waffling over just removing it completely.
>> Note also that the structure here makes it pretty easy to accidentally
>> invoke undefined behavior by destructor slicing. That's an entirely
>> different problem, but it bugs me...
> Can you describe what you mean by "destructor slicing"?  Do you mean calling ~Monitor on an instance of Mutex?

Yes, exactly that.  Typically by something like

Monitor* m = <mutex>;

delete m;

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8185746: Remove Mutex destructor assertion

David Holmes
In reply to this post by Kim Barrett
On 3/08/2017 9:36 AM, Kim Barrett wrote:

>> On Aug 2, 2017, at 7:31 PM, Kim Barrett <[hidden email]> wrote:
>>
>>> On Aug 2, 2017, at 7:23 PM, David Holmes <[hidden email]> wrote:
>>>
>>> Hi Kim,
>>>
>>> On 3/08/2017 8:41 AM, Kim Barrett wrote:
>>>> Please review this small change to improve the debugging experience
>>>> when a mutex is destroyed in a bad state.
>>>> I've removed the assert in ~Mutex, which was making the same checks as
>>>> in ~Monitor, but providing much less information.
>>>
>>> Okay. Please can you add a comment to the ~Mutex noting that eg:
>>>
>>> // Defer any state checking to ~Monitor
>>> Mutex::~Mutex() { }
>>
>> Well, I was waffling over just removing it completely.

That works too.

> Note also that the structure here makes it pretty easy to accidentally
> invoke undefined behavior by destructor slicing. That's an entirely
> different problem, but it bugs me...

Please explain.

Thanks,
David

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8185746: Remove Mutex destructor assertion

David Holmes
In reply to this post by Kim Barrett
On 3/08/2017 9:55 AM, Kim Barrett wrote:

>> On Aug 2, 2017, at 7:49 PM, [hidden email] wrote:
>>
>>
>> Looks good.
>
> Thanks.
>
>> On 8/2/17 7:36 PM, Kim Barrett wrote:
>>>> On Aug 2, 2017, at 7:31 PM, Kim Barrett <[hidden email]> wrote:
>>>>
>>>>> On Aug 2, 2017, at 7:23 PM, David Holmes <[hidden email]> wrote:
>>>>>
>>>>> Hi Kim,
>>>>>
>>>>> On 3/08/2017 8:41 AM, Kim Barrett wrote:
>>>>>> Please review this small change to improve the debugging experience
>>>>>> when a mutex is destroyed in a bad state.
>>>>>> I've removed the assert in ~Mutex, which was making the same checks as
>>>>>> in ~Monitor, but providing much less information.
>>>>> Okay. Please can you add a comment to the ~Mutex noting that eg:
>>>>>
>>>>> // Defer any state checking to ~Monitor
>>>>> Mutex::~Mutex() { }
>>>> Well, I was waffling over just removing it completely.
>>> Note also that the structure here makes it pretty easy to accidentally
>>> invoke undefined behavior by destructor slicing. That's an entirely
>>> different problem, but it bugs me...
>> Can you describe what you mean by "destructor slicing"?  Do you mean calling ~Monitor on an instance of Mutex?
>
> Yes, exactly that.  Typically by something like
>
> Monitor* m = <mutex>;
> …
> delete m;

virtual destructor needed?

David

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8185746: Remove Mutex destructor assertion

coleen.phillimore


On 8/2/17 8:09 PM, David Holmes wrote:

> On 3/08/2017 9:55 AM, Kim Barrett wrote:
>>> On Aug 2, 2017, at 7:49 PM, [hidden email] wrote:
>>>
>>>
>>> Looks good.
>>
>> Thanks.
>>
>>> On 8/2/17 7:36 PM, Kim Barrett wrote:
>>>>> On Aug 2, 2017, at 7:31 PM, Kim Barrett <[hidden email]>
>>>>> wrote:
>>>>>
>>>>>> On Aug 2, 2017, at 7:23 PM, David Holmes
>>>>>> <[hidden email]> wrote:
>>>>>>
>>>>>> Hi Kim,
>>>>>>
>>>>>> On 3/08/2017 8:41 AM, Kim Barrett wrote:
>>>>>>> Please review this small change to improve the debugging experience
>>>>>>> when a mutex is destroyed in a bad state.
>>>>>>> I've removed the assert in ~Mutex, which was making the same
>>>>>>> checks as
>>>>>>> in ~Monitor, but providing much less information.
>>>>>> Okay. Please can you add a comment to the ~Mutex noting that eg:
>>>>>>
>>>>>> // Defer any state checking to ~Monitor
>>>>>> Mutex::~Mutex() { }
>>>>> Well, I was waffling over just removing it completely.
>>>> Note also that the structure here makes it pretty easy to accidentally
>>>> invoke undefined behavior by destructor slicing. That's an entirely
>>>> different problem, but it bugs me...
>>> Can you describe what you mean by "destructor slicing"?  Do you mean
>>> calling ~Monitor on an instance of Mutex?
>>
>> Yes, exactly that.  Typically by something like
>>
>> Monitor* m = <mutex>;
>> …
>> delete m;
>
> virtual destructor needed?

Yes, that was my question.  I guess with Monitor, we don't want to pay
the word for the vtable?  But why not, it already has an embedded 64
character string.

Coleen

>
> David
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8185746: Remove Mutex destructor assertion

Thomas Schatzl
In reply to this post by Kim Barrett
Hi,

On Wed, 2017-08-02 at 19:31 -0400, Kim Barrett wrote:

> >
> > On Aug 2, 2017, at 7:23 PM, David Holmes <[hidden email]>
> > wrote:
> >
> > Hi Kim,
> >
> > On 3/08/2017 8:41 AM, Kim Barrett wrote:
> > >
> > > Please review this small change to improve the debugging
> > > experience

Looks good.

> > > when a mutex is destroyed in a bad state.
> > > I've removed the assert in ~Mutex, which was making the same
> > > checks as
> > > in ~Monitor, but providing much less information.
> > Okay. Please can you add a comment to the ~Mutex noting that eg:
> >
> > // Defer any state checking to ~Monitor
> > Mutex::~Mutex() { }
> Well, I was waffling over just removing it completely.
>

  do it :) I can't see, apart from it being a bug (to be fixed in
another CR?), why the constructor of Monitor is not virtual.

Thomas
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8185746: Remove Mutex destructor assertion

Kim Barrett
In reply to this post by coleen.phillimore
> On Aug 2, 2017, at 9:25 PM, [hidden email] wrote:
>
>
>
> On 8/2/17 8:09 PM, David Holmes wrote:
>> On 3/08/2017 9:55 AM, Kim Barrett wrote:
>>>> On Aug 2, 2017, at 7:49 PM, [hidden email] wrote:
>>>>
>>>>
>>>> Looks good.
>>>
>>> Thanks.
>>>
>>>> On 8/2/17 7:36 PM, Kim Barrett wrote:
>>>>>> On Aug 2, 2017, at 7:31 PM, Kim Barrett <[hidden email]> wrote:
>>>>>>
>>>>>>> On Aug 2, 2017, at 7:23 PM, David Holmes <[hidden email]> wrote:
>>>>>>>
>>>>>>> Hi Kim,
>>>>>>>
>>>>>>> On 3/08/2017 8:41 AM, Kim Barrett wrote:
>>>>>>>> Please review this small change to improve the debugging experience
>>>>>>>> when a mutex is destroyed in a bad state.
>>>>>>>> I've removed the assert in ~Mutex, which was making the same checks as
>>>>>>>> in ~Monitor, but providing much less information.
>>>>>>> Okay. Please can you add a comment to the ~Mutex noting that eg:
>>>>>>>
>>>>>>> // Defer any state checking to ~Monitor
>>>>>>> Mutex::~Mutex() { }
>>>>>> Well, I was waffling over just removing it completely.
>>>>> Note also that the structure here makes it pretty easy to accidentally
>>>>> invoke undefined behavior by destructor slicing. That's an entirely
>>>>> different problem, but it bugs me...
>>>> Can you describe what you mean by "destructor slicing"?  Do you mean calling ~Monitor on an instance of Mutex?
>>>
>>> Yes, exactly that.  Typically by something like
>>>
>>> Monitor* m = <mutex>;
>>> …
>>> delete m;
>>
>> virtual destructor needed?
>
> Yes, that was my question.  I guess with Monitor, we don't want to pay the word for the vtable?  But why not, it already has an embedded 64 character string.

Plus the cache line padding for most instances.  There’s quite a (known) mess in that hierarchy.
See comments for MONITOR_NAME_LEN and for the Mutex class.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8185746: Remove Mutex destructor assertion

Kim Barrett
> On Aug 3, 2017, at 1:35 PM, Kim Barrett <[hidden email]> wrote:
> There’s quite a (known) mess in that hierarchy.

Note that some of the bad aspects of the inheritance of Mutex from Monitor could
be fixed by C++11 deleted functions, e.g. in Mutex, instead of

private:
   bool notify ()    { ShouldNotReachHere(); return false; }

with C++11 one could have

public:
  bool notify() = delete;

Still not perfect, since one can still cast a Mutex reference to a Monitor reference
and then use notify.  (That’s true now too.)  Better would be to fix the inheritance
structure.  (And just flipping the order isn’t, IMO, the best way to do that, but that’s
a potentially much longer discussion.)

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8185746: Remove Mutex destructor assertion

Kim Barrett
In reply to this post by Thomas Schatzl
> On Aug 3, 2017, at 8:11 AM, Thomas Schatzl <[hidden email]> wrote:
>
> Hi,
>
> On Wed, 2017-08-02 at 19:31 -0400, Kim Barrett wrote:
>>>
>>> On Aug 2, 2017, at 7:23 PM, David Holmes <[hidden email]>
>>> wrote:
>>>
>>> Hi Kim,
>>>
>>> On 3/08/2017 8:41 AM, Kim Barrett wrote:
>>>>
>>>> Please review this small change to improve the debugging
>>>> experience
>
> Looks good.

Thanks.

>>>> when a mutex is destroyed in a bad state.
>>>> I've removed the assert in ~Mutex, which was making the same
>>>> checks as
>>>> in ~Monitor, but providing much less information.
>>> Okay. Please can you add a comment to the ~Mutex noting that eg:
>>>
>>> // Defer any state checking to ~Monitor
>>> Mutex::~Mutex() { }
>> Well, I was waffling over just removing it completely.
>>
>
>   do it :) I can't see, apart from it being a bug (to be fixed in
> another CR?), why the constructor of Monitor is not virtual.

It’s gone now.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8185746: Remove Mutex destructor assertion

Thomas Schatzl
Hi,

On Thu, 2017-08-03 at 13:44 -0400, Kim Barrett wrote:

> >
> > On Aug 3, 2017, at 8:11 AM, Thomas Schatzl <[hidden email]
> > om> wrote:
> >
> > On Wed, 2017-08-02 at 19:31 -0400, Kim Barrett wrote:
> > >
> > > >
> > > > On Aug 2, 2017, at 7:23 PM, David Holmes <[hidden email]
> > > > om>
> > > > wrote:
> > > >
> > > > Hi Kim,
> > > >
> > > > On 3/08/2017 8:41 AM, Kim Barrett wrote:
> > > > >
> > > > >
> > > > > Please review this small change to improve the debugging
> > > > > experience
> > Looks good.
> Thanks.
>
> > > > [...]
> > > > // Defer any state checking to ~Monitor
> > > > Mutex::~Mutex() { }
> > > Well, I was waffling over just removing it completely.
> > >
> >   do it :) I can't see, apart from it being a bug (to be fixed in
> > another CR?), why the constructor of Monitor is not virtual.
> It’s gone now.
>

  still looks good.

Thomas

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8185746: Remove Mutex destructor assertion

Kim Barrett
> On Aug 7, 2017, at 5:55 AM, Thomas Schatzl <[hidden email]> wrote:
>
> Hi,
>
> On Thu, 2017-08-03 at 13:44 -0400, Kim Barrett wrote:
>>>
>>> On Aug 3, 2017, at 8:11 AM, Thomas Schatzl <[hidden email]
>>> om> wrote:
>>>
>>> On Wed, 2017-08-02 at 19:31 -0400, Kim Barrett wrote:
>>>>
>>>>>
>>>>> On Aug 2, 2017, at 7:23 PM, David Holmes <[hidden email]
>>>>> om>
>>>>> wrote:
>>>>>
>>>>> Hi Kim,
>>>>>
>>>>> On 3/08/2017 8:41 AM, Kim Barrett wrote:
>>>>>>
>>>>>>
>>>>>> Please review this small change to improve the debugging
>>>>>> experience
>>> Looks good.
>> Thanks.
>>
>>>>> [...]
>>>>> // Defer any state checking to ~Monitor
>>>>> Mutex::~Mutex() { }
>>>> Well, I was waffling over just removing it completely.
>>>>
>>>   do it :) I can't see, apart from it being a bug (to be fixed in
>>> another CR?), why the constructor of Monitor is not virtual.
>> It’s gone now.
>>
>
>   still looks good.
>
> Thomas

Thanks Thomas, Coleen, and David.

Loading...