RFR - JDK-8133416: [TESTBUG] Remove @ignore for closed/runtime/4345157/Prog.java
Stas Smirnov
stanislav.smirnov at oracle.com
Wed Nov 11 16:22:46 UTC 2015
Hi,
please see the updated webrev,
webrev: http://cr.openjdk.java.net/~stsmirno/8133416/webrev.01
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