Hi Christian,
Sorry for the delay in coming back to this, I wanted to see what other
feedback arose.
On 25/01/2022 7:43 pm, Christian Hagedorn wrote:
Hi David
This will be really useful - thank you. :)
I'm glad to hear that! :-) Thanks for your overall comments!
All build file changes need to be seen by the build team.
Right, thanks for adding it again.
That said I have two general concerns both related to executing
non-async-signal-safe code in the signal handler via the error reporting logic.
Now I know we already do a ton of stuff in error reporting not guaranteed (in
any way) to be safe to do in a signal handler, but whenever we add something
new I have to ask the question: how likely is this additional code to lead to
secondary failures (hangs or crashes)?
That's a valid concern. I've also asked myself this question when I had
initially started using some assertions. We should not crash again during error
reporting. I've therefore tried to be as conservative as possible and added
bailouts instead, also in loops when reading data. But of course, this is just
a best effort and by no means a guarantee to be safe (especially in terms of
crashes). What could be alternatives to make this better?
If the parsing code turns out to be very problematic in a signal
handling context, then we could disable it in that context. So we really
want to try and do a lot of testing by throwing random signals at the VM
and see what breaks.
Secondly, on the same issue the use of unified logging within this code seems
even more likely to be problematic - I'm not aware of us currently using UL
during error reporting. It may work in basic usecases but if it triggers
logfile rotation or other more complex actions what then?
I haven't thought about this before. To be honest, I think UL printing of the
`dwarf` tag is only useful during development when adding something new to the
parser or when debugging. I don't see much value of these messages otherwise -
even less for a Java user. As a first step, I could change the logs from
`log_X()` to `log_develop_X()` but that just shifts the problem to non-product
builds. Another option (or additional thing) could be to guard the log messages
with a new develop flag that's disabled by default. By setting it for
development, we accept that it might be unsafe which should be fine.
I think changing the logging to develop only is a reasonable step. I
don't see logging of crash handling / error reporting as generally
useful for the end user.
Thanks,
David
Thanks,
Christian
-------------
PR: https://git.openjdk.java.net/jdk/pull/7126
