RFR - JDK-8133416: [TESTBUG] Remove @ignore for closed/runtime/4345157/Prog.java

[David Holmes]

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.

>>> 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. ??

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.

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.

>> 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 :)

>>
>>     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.

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
>>
>