RFR: 8302073: Specifying OnError handler prevents WatcherThread to break a deadlock in report_and_die()

[Christian Hagedorn]

On Tue, 14 Mar 2023 18:13:11 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>>> Decoder, in particular, should not use malloc. Therefore I also opened https://bugs.openjdk.org/browse/JDK-8303862 to track that. I won't have time to work on that. I have the hope that maybe @chhagedorn can :-) ?
>> 
>> Does any of our current allocation strategies allow the usage of a custom scratch buffer (i.e. the error reporting scratch buffer) as memory location? If not, it could get more complicated. I could still try to tackle that but I'm not sure if I have the necessary knowledge in that area.
>
>> > > Decoder, in particular, should not use malloc. Therefore I also opened https://bugs.openjdk.org/browse/JDK-8303862 to track that. I won't have time to work on that. I have the hope that maybe @chhagedorn can :-) ?
>> > 
>> > 
>> > Does any of our current allocation strategies allow the usage of a custom scratch buffer (i.e. the error reporting scratch buffer) as memory location? If not, it could get more complicated. I could still try to tackle that but I'm not sure if I have the necessary knowledge in that area.
>> 
>> What we usually do for these kind of problems is to pass the scratch buffer via argument to the processing functions. And make that use either it or, if NULL was passes, allocate its own thing.
>> 
>> Another way to do this would be to add a scratch buffer pointer to Thread.
>> 
>> A third way to do this (I had been experimenting with it) would be to pre-allocate a scratch buffer, and once error handling began, to use that inside os::malloc. That is the most involved solution, though, and I'm not particularly fond of it. I know many reviewers would hate it, too :-)
> 
> Thanks for the summary. Could we also use "placement new" to use the scratch buffer to allocate and create objects to? For example, when creating a decoder, to use something like
> 
> decoder = new(scratch_buffer) ElfDecoder();
> 
> here:
> https://github.com/openjdk/jdk/blob/4e631fa43fd821846c12ae2177360c44cf770766/src/hotspot/share/utilities/decoder.cpp#L62-L70
> Then we can still utilize polymorphism. But then `AbstractDecoder` needs to be something else than `CHeapObj`. And of course, this approach also needs careful checking that a new object actually fits into the provided scratch buffer. On top of that, it gets more complicated when we want to keep multiple objects alive in the same scratch buffer. So, I'm not sure if this approach is feasible, though.

> > > > > Decoder, in particular, should not use malloc. Therefore I also opened https://bugs.openjdk.org/browse/JDK-8303862 to track that. I won't have time to work on that. I have the hope that maybe @chhagedorn can :-) ?
> > > > 
> > > > 
> > > > Does any of our current allocation strategies allow the usage of a custom scratch buffer (i.e. the error reporting scratch buffer) as memory location? If not, it could get more complicated. I could still try to tackle that but I'm not sure if I have the necessary knowledge in that area.
> > > 
> > > 
> > > What we usually do for these kind of problems is to pass the scratch buffer via argument to the processing functions. And make that use either it or, if NULL was passes, allocate its own thing.
> > > Another way to do this would be to add a scratch buffer pointer to Thread.
> > > A third way to do this (I had been experimenting with it) would be to pre-allocate a scratch buffer, and once error handling began, to use that inside os::malloc. That is the most involved solution, though, and I'm not particularly fond of it. I know many reviewers would hate it, too :-)
> > 
> > 
> > Thanks for the summary. Could we also use "placement new" to use the scratch buffer to allocate and create objects to? For example, when creating a decoder, to use something like
> > ```
> > decoder = new(scratch_buffer) ElfDecoder();
> > ```
> > 
> > 
> >     
> >       
> >     
> > 
> >       
> >     
> > 
> >     
> >   
> > here:
> > https://github.com/openjdk/jdk/blob/4e631fa43fd821846c12ae2177360c44cf770766/src/hotspot/share/utilities/decoder.cpp#L62-L70
> > 
> > Then we can still utilize polymorphism. But then `AbstractDecoder` needs to be something else than `CHeapObj`. And of course, this approach also needs careful checking that a new object actually fits into the provided scratch buffer. On top of that, it gets more complicated when we want to keep multiple objects alive in the same scratch buffer. So, I'm not sure if this approach is feasible, though.
> 
> Its possible to use placement new, and also to place several objects like this; but I realize now the scratch buffer might be too small.

Yes, that might be the case.

> 
> I need to think this through a bit. Maybe providing a pre-allocated buffer for os::malloc would be the right thing to do.

That might be better and it sounds easier to do instead of going with a separate allocation strategy with placement new using the scratch buffer.

-------------

PR: https://git.openjdk.org/jdk/pull/12925