RFR: 8136978 Much nearly duplicated code for vmError support
Thomas Stüfe
thomas.stuefe at gmail.com
Wed Nov 18 09:52:50 UTC 2015
Hi Sebastian,
Your patch fails to apply on my repository. I tested against a freshly
pulled jdk9/dev. I think it may conflict with "8080775: Better argument
formatting for assert() and friends".
--------------
>From looking at it, this change is a nice cleanup!
Here are some few cosmetic issues:
- os::formatDebugMessage() and void os::startDebugging():
Standard naming scheme would be lowercase with underscores. Could you
rename them "format_debug_message" and "start_debugging", respectivly?
-
http://cr.openjdk.java.net/~sebastian/8136978/webrev.00/src/os/posix/vm/os_posix.hpp.udiff.html
Could you please group
static int unblock_signals(const sigset_t *set);
with the other signal set functions above and maybe also add a comment?
---
Also, I would like to see in the name of "unblock_signals" reflected that
this is thread-specific, e.g. "unblock_thread_signals".
Or even better, a more generic function which both blocks and unblocks
signals for the calling thread, e.g.
"os::Posix::set_thread_signal_mask(const sigset_t *set, bool do_block)".
---
Kind Regards, Thomas
On Fri, Oct 16, 2015 at 6:33 AM, Sebastian Sickelmann <
sebastian.sickelmann at gmx.de> wrote:
> Hi,
>
> i have looked at the enhancement JDK-8136978, please find my first
> suggestion
> at [0] http://cr.openjdk.java.net/~sebastian/8136978/webrev.00/.
>
> I first looked also at another solution but don't liked it. For some
> details
> on this and comments of Coleen and Kim to this, see the thread at
> hotspot-dev[1].
>
> Right now i only compile-"tested" it on my linux(x86_64) machine, so there
> might
> be some error in aix,bsd,solaris. I am not sure if i can setup my machine
> to compile
> those as well. For the windows-part i am actually also not able to compile
> or test it.
> I think there is a good change to optimize the use of #include's in the
> changed files,
> but I am not sure how i can effectively work out where i can reduce some
> imports.
>
> There are no additional tests for this. What is the best way to really
> test such a change
> on all platforms. Do you use your development-machine for this, or is
> there some
> infrastructure that can test such multi-platform changes for you?
>
> Here is a short description of the suggested change:
>
> Nearly identically implementations of VMError moved from the
> os/[linux|aix|bsd|solaris]
> to a os/posix. The parts that are different were refactored and are now in
> the os-specific
> implementations of the os class. The two os specific methods
> ucontext_get_pc and
> ucontext_set_pc are moved to the declaration of the os::Posix class. The
> implementations
> of those remain in the os_[linux|aix|bsd|solaris].cpp implementations but
> are renamed
> acordingly. All uses of these methods are replaces to use the "os::Poxis
> prefix".
>
> For the method VMError::show_message_box also the windows implementation
> is changed. Now
> there are two methods in the declarartion of the class os that are used to
> help the
> os-independent implementation of show_message. The two messages are named
> formatDebugMessage
> and startDebugging. The os-independet implemetation of show_message can be
> found in
> share/vm/utilities/vmError.cpp
>
>
> -- Sebastian
>
>
> [0] http://cr.openjdk.java.net/~sebastian/8136978/webrev.00/
> [1]
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-October/020249.html
> suggestion to this.
>
>
>
More information about the hotspot-runtime-dev
mailing list