RFR - JDK-8133416: [TESTBUG] Remove @ignore for closed/runtime/4345157/Prog.java
David Holmes
david.holmes at oracle.com
Fri Nov 13 02:54:01 UTC 2015
Hi Stas,
On 12/11/2015 11:04 PM, Stas Smirnov wrote:
> 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.
Also talk to Jerry who has a similar issue with a native test that is
only for one platform.
> 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
This is all historical and obsolete. If you want solaris then just check
for SOLARIS. We're only compiling on Solaris 10+
> 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,
No - sigwait only returns EINTR if interrupted by an unblocked signal
that is not one you expect. For the signals you expect it returns 0.
> 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.
The overall purpose of what the test is trying to test is still not
clear from the actual test code. We use one Java program to exec a
custom JVM launcher, which in turn executes a Java method that will
throw an exception. Meanwhile the main Java program waits a little while
and then destroys the process it created by sending SIGTERM. The SIGTERM
should be caught via the sigwait and we exit(0) which signifies success.
But what is it actually testing? The test name is ThreadSignalMask - so
what is it about threads and signal masks that is being tested ?
> 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?
The problem here is that you are effectively creating a new test by
open-sourcing what was an internal test. Hence the bar for acceptance is
somewhat higher.
Thanks,
David
>>
>> 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