RFR - JDK-8133416: [TESTBUG] Remove @ignore for closed/runtime/4345157/Prog.java
Stas Smirnov
stanislav.smirnov at oracle.com
Thu Nov 19 02:30:41 UTC 2015
Hi,
please see the latest webrev
http://cr.openjdk.java.net/~stsmirno/8133416/webrev.02
I have moved conditional compilation from a ".c" file to the
JtregNative.gmk file. And also removed extra "wait" in Prog.java.
Tested on all platforms.
On 12/11/15 21:54, David Holmes wrote:
> 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