RFR - JDK-8133416: [TESTBUG] Remove @ignore for closed/runtime/4345157/Prog.java
Stas Smirnov
stanislav.smirnov at oracle.com
Thu Nov 12 13:04:27 UTC 2015
Hi David,
On 12/11/15 09:53, David Holmes wrote:
> Hi Stas,
>
> On 12/11/2015 2:22 AM, Stas Smirnov wrote:
>> Hi,
>>
>> please see the updated webrev,
>> webrev: http://cr.openjdk.java.net/~stsmirno/8133416/webrev.01
>
> test/runtime/ThreadSignalMask/exeThreadSignalMask.c
>
> I would still prefer to see the compilation selection being done via
> the makefile rather than invoking the compiler to do nothing. You can
> test the current OS and ARCH easily enough. This test:
>
> #if defined(__sun) && defined(__SVR4)
>
> is not an appropriate test for solaris anyway.
I will take a look on the make file approach, probably missed your comment.
Why do you think it is inappropriate test? From what I know here is the
full logic to detect Solaric/SunOS
#if defined(sun) || defined(__sun)
# if defined(__SVR4) || defined(__svr4__)
/* Solaris */
# else
/* SunOS */
# endif
#endif
so in my case I just use a condition to detect Solaris
>
> The sigwait do/while loop has not been changed and is still wrong as
> per my earlier comments. If an unexpected signal arrives the err will
> be EINTR so this:
>
> 234 if (err != 0 && err != EINTR) {
> 235 // print error message if unexpected signal occurred
> 236 fprintf(stderr, "main: sigwait() error: %s\n",
> strerror(err));
>
> should simply be:
>
> 234 if (err == EINTR) {
> 235 // print error message if unexpected signal occurred
> 236 fprintf(stderr, "main: sigwait() error: %s\n", strerror(err));
>
> though you may also want a case for some other error occurring.
Well, let me try to explain here, from how I understand this,
process executes and if everything is ok we wait for an expected signals
to exit 0, otherwise we exit with an error and do not wait.
p.destroy(); sends a SIGTERM signal to the process and we expects it, so
sigwait exits with err == EINTR, or if it has already exited we will
just get an error code,
that's why if exit code is not 0 and not EINTR it is some unexpected
interruption and we print an error message and continue waiting
otherwise we exit normally with a success result.
>
> ---
>
> test/runtime/ThreadSignalMask/Prog.java
>
> My comments regarding the very odd code in this class were not
> addressed. What is it supposed to be doing?
missed it, sorry, yep, probably you are right
>
> ---
>
> test/runtime/ThreadSignalMask/ThreadSignalMask.java
>
> I still think the basic approach of this test is very poor. launching
> a VM and hoping it will get to the right place before a signal is sent
> is error prone. And I'm still unclear what exactly constitutes success
> here - does destroying the executable expect to get a zero return code?
a comment above describes the logic.
Talking about the current approach, well, it works and works as expected
in both positive and negative cases, a better solution did not come to
my mind immediately when I was porting it from shell to Java.
I assume there are lots of old tests that probably can be rewritten, but
it is a matter of another task, right?
>
> Thanks,
> David
>
>> On 10/11/15 15:36, Stas Smirnov wrote:
>>> Hi David,
>>>
>>> please see inline
>>>
>>> On 10/11/15 07:53, David Holmes wrote:
>>>> Hi Stas,
>>>>
>>>> Piggy-backing on Dan's comments and your responses ...
>>>>
>>>> And apologies if all the problems stem from the original code that
>>>> has been rewritten, I can only comment on what I'm reviewing here.
>>>>
>>>> On 9/11/2015 11:52 PM, Stas Smirnov wrote:
>>>>> Hi Dan,
>>>>>
>>>>> thanks for your feedback, please see my comments below
>>>>>
>>>>> On 06/11/15 22:07, Daniel D. Daugherty wrote:
>>>>>> On 11/4/15 3:33 AM, Stas Smirnov wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> please review this fix for JDK-8133416.
>>>>>>>
>>>>>>> The test has been fixed and rewritten in Java to start using native
>>>>>>> support mechanism in Jtreg.
>>>>>>> Also, its native part has been modified to avoid compilation on
>>>>>>> inappropriate platforms and also includes improvements.
>>>>
>>>> I would think the simplest way to avoid compilation would be to
>>>> conditionally add this test's directory to the set of source
>>>> directories in JtregNative.gmk based on the OS value. I also see no
>>>> reason to restrict this test to sparcv9 (though it does make the
>>>> native code a little harder to write to find the libjvm.so location).
>>>> Going further there's no reason this couldn't run on Linux, or any
>>>> other POSIX compatible systems.
>>>>
>>>> Ideally you should not need to define:
>>>>
>>>> #define _POSIX_PTHREAD_SEMANTICS
>>>>
>>>> in the source - as that is used by programs using Solaris threads,
>>>> instead of POSIX threads, but which want POSIX semantics for certain
>>>> common APIs. But I can see that the test compilation is using the
>>>> same settings as the main hotspot build, which means we get Solaris
>>>> threads and -lthread. So please add a comment explaining this.
>>> I will add comment with a text like "to enable POSIX semantics for
>>> certain common APIs"
>>>>
>>>>>>> Unfortunately the JBS issue is not visible to the community as well
>>>>>>> as the original version of the test sources.
>>>>>>>
>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8133416
>>>>>>> webrev: http://cr.openjdk.java.net/~stsmirno/8133416/webrev.00
>>>>>>
>>>>>> make/test/JtregNative.gmk
>>>>>> No comments.
>>>>
>>>> See prior comment about conditionally building this test.
>>>>
>>>>>>
>>>>>> test/runtime/ThreadSignalMask/Prog.java
>>>>>> No comments.
>>>>
>>>> What are you trying to do here:
>>>>
>>>> 28 try {
>>>> 29 Prog.class.wait();
>>>> 30 } catch (Exception e) {
>>>>
>>>> this will throw IllegalMonitorStateException as you can't call wait()
>>>> on an Object unless you own the monitor of the object via use of
>>>> "synchronized".
>>>>
>>>>>>
>>>>>> test/runtime/ThreadSignalMask/ThreadSignalMask.java
>>>>>> No comments.
>>>>
>>>> "No comments" is my problem with this class :) I can't work out what
>>>> it is trying to do as there are no comments. I'm not even sure what
>>>> this class exec's via processBuilder - I thought it would be the
>>>> native C program, which in turn invokes Prog, but I can't see where
>>>> that happens. ??
>>> Comments, sure.
>>> exeThreadSignalMask.c is being compiled into an executable
>>> ThreadSignalMask which is being executed via ProcessBuilder and the
>>> testjdk path is an input argument for it.
>>> Then native code invokes Prog using
>>> cls = (*env)->FindClass(env, "Prog");
>>> and later
>>> (*env)->CallStaticVoidMethod(env, cls, mid, args);
>>>>
>>>> Here:
>>>>
>>>> 89 for (long interval : intervalsArray) {
>>>> 90 Process p = pb.start();
>>>> 91
>>>> 92 // sleep for a specified period of time to let
>>>> native run
>>>> 93 sleep(interval);
>>>> 94 p.destroy();
>>>> 95 // sleep for a specified period of time to let
>>>> native exit
>>>> 96 sleep(interval);
>>>>
>>>> This is poor logic for hoping that we're waiting long enough for the
>>>> exec'd process to run - simply try with a longer timeout. If I
>>>> understood what the test was doing better I may have a better
>>>> suggestion - though often a sentinel file is used to coordinate
>>>> processes. The second sleep is unnecessary as you can use
>>>> Process.waitFor and then get the return value.
>>>>
>>> Second sleep, agree
>>>> The indent style is also wrong for Java code:
>>>>
>>>> 55 Path classFilePath = Paths.get(
>>>> 56 testClasses,
>>>> 57 Prog.class.getSimpleName() + ".class");
>>>>
>>>> should be
>>>>
>>>> Path classFilePath = Paths.get(testClasses,
>>>> Prog.class.getSimpleName() +
>>>> ".class");
>>>>
>>>> and
>>>> 73 Files.copy(
>>>> 74 executableFilePath,
>>>> 75 executableFileLocalPath,
>>>> 76 StandardCopyOption.REPLACE_EXISTING);
>>>>
>>>> should be:
>>>>
>>>> Files.copy(executableFilePath,
>>>> executableFileLocalPath,
>>>> StandardCopyOption.REPLACE_EXISTING);
>>>>
>>>> etc.
>>> thanks, will fix it
>>>>
>>>>>> test/runtime/ThreadSignalMask/exeThreadSignalMask.c
>>>>>> General: I think the indent standard for 'C' was also two
>>>>>> spaces,
>>>>>> but I could be wrong...
>>>>> I do not know about any strict requirement about spaces, just for
>>>>> me 4
>>>>> spaces are more readable, is it that critical?
>>>>
>>>> Hotspot C-style is 2-space indent. Java code is (universally?) 4
>>>> spaces. Might as well get used to it :)
>>> 2-spaces, got it
>>>>
>>>>>>
>>>>>> L39 char path[PATH_MAX];
>>>>>> Recommend '+1' for NULL termination space.
>>>>>>
>>>>>> L45: extern void exit(int);
>>>>>> Hard to believe this isn't in a header file...
>>>>> warnings tell me that it is not
>>>>
>>>> It is in stdlib.h so that should be included.
>>> I will do so
>>>>
>>>> Looking at a few others things:
>>>>
>>>> - more comments explaining what the test is trying to achieve please!
>>>> - there's no error checking for any of the pthread_* functions
>>>> - names like doStuff, somethr and atchJVM are not very information
>>>> nor do they need to be abbreviated like that
>>>> - atchJVM is not used
>>>>
>>>>
>>>> I don't quite follow this loop and subsequent code:
>>>>
>>>> 206 do {
>>>> 207 int err;
>>>> 208
>>>> 209 sig = 0;
>>>> 210 err = sigwait(&set, &sig);
>>>> 211 if (err != 0 && err != EINTR) {
>>>> 212 fprintf(stderr, "main: sigwait() error: %s\n",
>>>> strerror(err));
>>>> 213 } else {
>>>> 214 fprintf(stderr, "main: sigwait() got:
>>>> %d\nSucceed!\n", sig);
>>>> 215 exit(0);
>>>> 216 }
>>>> 217 } while (sig != SIGTERM && sig != SIGINT);
>>>> 218
>>>> 219 pthread_join(thr1, NULL);
>>>> 220
>>>> 221 closeHandle();
>>>> 222 fputs("Main thread exiting.\n", stderr);
>>>> 223 return 0;
>>>>
>>>> If we get EINTR because an unblocked signal was received and
>>>> interrupted the sigwait then we will go to the else at #213 and exit
>>>> at #214. If we get any of the expected signals we follow the exit
>>>> path. So the loop condition will only be checked if sigwait returned
>>>> an error (other than EINTR), and AFAICS the loop will never terminate
>>>> other than by the call to exit(0).
>>>>
>>>> Also unclear why all output is to stderr
>>>>
>>>> Thanks,
>>>> David
>>>> ------
>>>>
>>>>>>
>>>>>> L50: fputs("Error occurred while closing handle", stderr);
>>>>>> Will this output include a newline?
>>>>> thanks for catching this
>>>>>>
>>>>>> L62: char lib[PATH_MAX];
>>>>>> Recommend '+1' for NULL termination space.
>>>>>>
>>>>>> L63: snprintf(lib, sizeof (lib),
>>>>>> "%s/lib/sparcv9/server/libjvm.so", path);
>>>>>> I don't think snprintf() will NULL terminate if the
>>>>>> buffer is
>>>>>> full.
>>>>>> Add: lib[PATH_MAX] = '\0';
>>>>>>
>>>>>> L180: strncpy(path, argv[1], PATH_MAX);
>>>>>> strncpy() won't NULL terminate if buffer is full.
>>>>>> Add: path[PATH_MAX] = '\0';
>>>>> actually from the manpage snprintf does include a NULL termination
>>>>> character and strncpy does not,
>>>>> so I have wrote
>>>>> strncpy(path, argv[1], PATH_MAX);
>>>>> path[PATH_MAX - 1] = '\0';
>>>>>
>>>>> and a question to you, since snprintf does handle NULL termination,
>>>>> will
>>>>> it be more reasonable to use it here as well,
>>>>> snprintf(path, sizeof (path), "%s", argv[1] );
>>>>> or, since we do not need any formatted string here just use two
>>>>> lines I
>>>>> wrote above?
>>>>>
>>>>> If you are ok with it, I will not send a new webrev, just add this
>>>>> fixes
>>>>> in the result commit.
>>>>>>
>>>>>>
>>>>>> I think that's it...
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>> Tested: the fix has been tested on all platforms with the
>>>>>>> hotspot/test/runtime testset
>>>>>>
>>>>>
>>>
>>
More information about the hotspot-runtime-dev
mailing list