RFR - JDK-8133416: [TESTBUG] Remove @ignore for closed/runtime/4345157/Prog.java
David Holmes
david.holmes at oracle.com
Fri Nov 20 02:20:19 UTC 2015
Hi Stas,
I have no further comments.
Thanks,
David
On 19/11/2015 12:30 PM, Stas Smirnov wrote:
> 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