Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[Magnus Ihse Bursie]

On 2018-04-05 14:09, David Holmes wrote:

:) V12 looks fine. Sorry I didnt see this before previous email.


Finally! :-)

I'll wait for Thomas to test on AIX as well, then I believe this is 
finally ready to push.


/Magnus



David

On 5/04/2018 9:51 PM, Magnus Ihse Bursie wrote:

On 2018-04-05 13:07, Magnus Ihse Bursie wrote:

On 2018-04-05 12:30, David Holmes wrote:

On 5/04/2018 7:52 PM, Magnus Ihse Bursie wrote:

On 2018-04-05 10:30, David Holmes wrote:

On 5/04/2018 6:07 PM, Magnus Ihse Bursie wrote:



On 2018-04-04 09:59, David Holmes wrote:

Hi Magnus,

Sorry for the delay ...

On 28/03/2018 8:15 PM, Magnus Ihse Bursie wrote:


On 2018-03-27 18:24, Thomas Stüfe wrote:

Hi Magnus,

just a cursory look, will look in greater detail tomorrow.

This is good, thanks for doing this.

As I wrote offlist, please remove the painfully wrong AIX 
"workarounds". I do not know why we did this - the original 
code is quite old - my assumption is that dlsym(RTLD_NEXT) 
was not working as expected on older AIX versions. Well, it 
should work now according to IBMs manpages. Will test later.

Ok.


__thread is not Posix. I would prefer 
pthread_get/set_specific instead, which is more portable.


I have surrounded this code with #ifdef MACOSX now.

Here is an updated webrev. It includes the changes requested 
by you and David:


* No more AIX workarounds
* Solaris use pthreads
* The "reentry" code is #ifdef MACOSX.


That all seems good.

Good. :)



I also assumed that NSIG is available and working on Solaris. 
I'll let David decide if he is happy with that. The 
alternative is to go back to the Solaris-specific code that 
allocates sact on the heap.


Unfortunately NSIG is a problem on Solaris:

http://austingroupbugs.net/view.php?id=741

It's use also precludes the use of the real-time signals - 
which limits Linux as well. But I'm not completely clear on 
exactly how signals may be numbered in their entirety so I 
wouldn't necessarily suggest trying to use SIGRTMAX+1 as the 
number of available signals, other than on Solaris.
Ok. I hope I understand you correctly. I have replaced NSIG with 
MAX_SIGNALS, which is defined as NSIG on all platforms except 
Solaris, where it is defined as SIGRTMAX+1.


Updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.08 



(8th time's a charm..?)


Nope - you can't use SIGRTMAX+1 to allocate a static array as it 
is not a constant expression. That's why Solaris uses malloc.


Duh. Right.

Okay. Like this, then?

http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.09

I have restored the calls to allocate_sact() in the same locations 
as in the original solaris version. Hopefully, those are correct. :-)


Yes you have restored them in the same locations.

As for correctness ... there are some preexisting issues I spotted. 
Do you really want to know? ;-) Well only one correctness issue, 
plus one unnecessary code issue:

Will it ever end?! :-)



Correctness:

83 #ifdef SOLARIS
  84   if (sact == NULL) {
  85 sact = (struct sigaction *)malloc((MAX_SIGNALS) * 
(size_t)sizeof(struct sigaction));
  86 memset(sact, 0, (MAX_SIGNALS) * (size_t)sizeof(struct 
sigaction));

  87   }
  88
  89   if (sact == NULL) {
  90 printf("%s\n", "libjsig.so unable to allocate memory");
  91 exit(0);
  92   }

The NULL check at line 89 needs to move to line 86 before we do 
memset on a NULL pointer.


Redundant code:

 142 static void save_signal_handler(int sig, sa_handler_t disp, 
bool is_sigset) {

 143   sigset_t set;
 144
 145   allocate_sact();

There are two calls to save_signal_handler, both preceded by a call 
to allocate_sact(), so we don't need to do it again at line 145.


Ok, I'll fix it while I'm at it.

New webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.10


Also, here is a webrev with the same patch applied to jdk/hs. (I 
have also included the needed changes to how libjsig is compiled, 
which are pushed to jdk/jdk but not yet integrated into jdk/hs). The 
patch file from this webrev can be applied to jdk/hs, so Thomas 
hopefully can test the AIX changes.


Let's hope this is the final iteration...
*LOL* Of course not. I forgot to press save in the editor before 
creating the webrev and starting the tests. *arrrghh* I can't believe 
this.


http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.12

/Magnus



/Magnus



Thanks,
David


/Magnus


David



/Magnus



Thanks,
David

Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.05 



Once again, here is also a webrev that shows the difference 
between the original files and the new, unified file. Even if 
it's hard to read, it shows what the effects will be for each 
platform.


Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.04/ 



/Magnus



Thanks!

Thomas





On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie 

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[David Holmes]

:) V12 looks fine. Sorry I didnt see this before previous email.

David

On 5/04/2018 9:51 PM, Magnus Ihse Bursie wrote:

On 2018-04-05 13:07, Magnus Ihse Bursie wrote:

On 2018-04-05 12:30, David Holmes wrote:

On 5/04/2018 7:52 PM, Magnus Ihse Bursie wrote:

On 2018-04-05 10:30, David Holmes wrote:

On 5/04/2018 6:07 PM, Magnus Ihse Bursie wrote:



On 2018-04-04 09:59, David Holmes wrote:

Hi Magnus,

Sorry for the delay ...

On 28/03/2018 8:15 PM, Magnus Ihse Bursie wrote:


On 2018-03-27 18:24, Thomas Stüfe wrote:

Hi Magnus,

just a cursory look, will look in greater detail tomorrow.

This is good, thanks for doing this.

As I wrote offlist, please remove the painfully wrong AIX 
"workarounds". I do not know why we did this - the original 
code is quite old - my assumption is that dlsym(RTLD_NEXT) was 
not working as expected on older AIX versions. Well, it should 
work now according to IBMs manpages. Will test later.

Ok.


__thread is not Posix. I would prefer pthread_get/set_specific 
instead, which is more portable.


I have surrounded this code with #ifdef MACOSX now.

Here is an updated webrev. It includes the changes requested by 
you and David:


* No more AIX workarounds
* Solaris use pthreads
* The "reentry" code is #ifdef MACOSX.


That all seems good.

Good. :)



I also assumed that NSIG is available and working on Solaris. 
I'll let David decide if he is happy with that. The alternative 
is to go back to the Solaris-specific code that allocates sact 
on the heap.


Unfortunately NSIG is a problem on Solaris:

http://austingroupbugs.net/view.php?id=741

It's use also precludes the use of the real-time signals - which 
limits Linux as well. But I'm not completely clear on exactly how 
signals may be numbered in their entirety so I wouldn't 
necessarily suggest trying to use SIGRTMAX+1 as the number of 
available signals, other than on Solaris.
Ok. I hope I understand you correctly. I have replaced NSIG with 
MAX_SIGNALS, which is defined as NSIG on all platforms except 
Solaris, where it is defined as SIGRTMAX+1.


Updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.08

(8th time's a charm..?)


Nope - you can't use SIGRTMAX+1 to allocate a static array as it is 
not a constant expression. That's why Solaris uses malloc.


Duh. Right.

Okay. Like this, then?

http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.09

I have restored the calls to allocate_sact() in the same locations 
as in the original solaris version. Hopefully, those are correct. :-)


Yes you have restored them in the same locations.

As for correctness ... there are some preexisting issues I spotted. 
Do you really want to know? ;-) Well only one correctness issue, plus 
one unnecessary code issue:

Will it ever end?! :-)



Correctness:

83 #ifdef SOLARIS
  84   if (sact == NULL) {
  85 sact = (struct sigaction *)malloc((MAX_SIGNALS) * 
(size_t)sizeof(struct sigaction));
  86 memset(sact, 0, (MAX_SIGNALS) * (size_t)sizeof(struct 
sigaction));

  87   }
  88
  89   if (sact == NULL) {
  90 printf("%s\n", "libjsig.so unable to allocate memory");
  91 exit(0);
  92   }

The NULL check at line 89 needs to move to line 86 before we do 
memset on a NULL pointer.


Redundant code:

 142 static void save_signal_handler(int sig, sa_handler_t disp, bool 
is_sigset) {

 143   sigset_t set;
 144
 145   allocate_sact();

There are two calls to save_signal_handler, both preceded by a call 
to allocate_sact(), so we don't need to do it again at line 145.


Ok, I'll fix it while I'm at it.

New webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.10


Also, here is a webrev with the same patch applied to jdk/hs. (I have 
also included the needed changes to how libjsig is compiled, which are 
pushed to jdk/jdk but not yet integrated into jdk/hs). The patch file 
from this webrev can be applied to jdk/hs, so Thomas hopefully can 
test the AIX changes.


Let's hope this is the final iteration...
*LOL* Of course not. I forgot to press save in the editor before 
creating the webrev and starting the tests. *arrrghh* I can't believe this.


http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.12

/Magnus



/Magnus



Thanks,
David


/Magnus


David



/Magnus



Thanks,
David

Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.05 



Once again, here is also a webrev that shows the difference 
between the original files and the new, unified file. Even if 
it's hard to read, it shows what the effects will be for each 
platform.


Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.04/ 



/Magnus



Thanks!

Thomas





On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie 
> wrote:


    When I was about to update jsig.c, I noticed that the four 
copies
    for aix, linux, macosx and solaris were basically the same, 
with
    

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[David Holmes]

On 5/04/2018 9:07 PM, Magnus Ihse Bursie wrote:

On 2018-04-05 12:30, David Holmes wrote:

On 5/04/2018 7:52 PM, Magnus Ihse Bursie wrote:

On 2018-04-05 10:30, David Holmes wrote:

On 5/04/2018 6:07 PM, Magnus Ihse Bursie wrote:



On 2018-04-04 09:59, David Holmes wrote:

Hi Magnus,

Sorry for the delay ...

On 28/03/2018 8:15 PM, Magnus Ihse Bursie wrote:


On 2018-03-27 18:24, Thomas Stüfe wrote:

Hi Magnus,

just a cursory look, will look in greater detail tomorrow.

This is good, thanks for doing this.

As I wrote offlist, please remove the painfully wrong AIX 
"workarounds". I do not know why we did this - the original code 
is quite old - my assumption is that dlsym(RTLD_NEXT) was not 
working as expected on older AIX versions. Well, it should work 
now according to IBMs manpages. Will test later.

Ok.


__thread is not Posix. I would prefer pthread_get/set_specific 
instead, which is more portable.


I have surrounded this code with #ifdef MACOSX now.

Here is an updated webrev. It includes the changes requested by 
you and David:


* No more AIX workarounds
* Solaris use pthreads
* The "reentry" code is #ifdef MACOSX.


That all seems good.

Good. :)



I also assumed that NSIG is available and working on Solaris. 
I'll let David decide if he is happy with that. The alternative 
is to go back to the Solaris-specific code that allocates sact on 
the heap.


Unfortunately NSIG is a problem on Solaris:

http://austingroupbugs.net/view.php?id=741

It's use also precludes the use of the real-time signals - which 
limits Linux as well. But I'm not completely clear on exactly how 
signals may be numbered in their entirety so I wouldn't 
necessarily suggest trying to use SIGRTMAX+1 as the number of 
available signals, other than on Solaris.
Ok. I hope I understand you correctly. I have replaced NSIG with 
MAX_SIGNALS, which is defined as NSIG on all platforms except 
Solaris, where it is defined as SIGRTMAX+1.


Updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.08

(8th time's a charm..?)


Nope - you can't use SIGRTMAX+1 to allocate a static array as it is 
not a constant expression. That's why Solaris uses malloc.


Duh. Right.

Okay. Like this, then?

http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.09

I have restored the calls to allocate_sact() in the same locations as 
in the original solaris version. Hopefully, those are correct. :-)


Yes you have restored them in the same locations.

As for correctness ... there are some preexisting issues I spotted. Do 
you really want to know? ;-) Well only one correctness issue, plus one 
unnecessary code issue:

Will it ever end?! :-)



Correctness:

83 #ifdef SOLARIS
  84   if (sact == NULL) {
  85 sact = (struct sigaction *)malloc((MAX_SIGNALS) * 
(size_t)sizeof(struct sigaction));
  86 memset(sact, 0, (MAX_SIGNALS) * (size_t)sizeof(struct 
sigaction));

  87   }
  88
  89   if (sact == NULL) {
  90 printf("%s\n", "libjsig.so unable to allocate memory");
  91 exit(0);
  92   }

The NULL check at line 89 needs to move to line 86 before we do memset 
on a NULL pointer.


Redundant code:

 142 static void save_signal_handler(int sig, sa_handler_t disp, bool 
is_sigset) {

 143   sigset_t set;
 144
 145   allocate_sact();

There are two calls to save_signal_handler, both preceded by a call to 
allocate_sact(), so we don't need to do it again at line 145.


Ok, I'll fix it while I'm at it.


You don't seem to have fixed the unnecessary allocate_sact() at line 145.

David
-



New webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.10


Also, here is a webrev with the same patch applied to jdk/hs. (I have 
also included the needed changes to how libjsig is compiled, which are 
pushed to jdk/jdk but not yet integrated into jdk/hs). The patch file 
from this webrev can be applied to jdk/hs, so Thomas hopefully can test 
the AIX changes.


Let's hope this is the final iteration...

/Magnus



Thanks,
David


/Magnus


David



/Magnus



Thanks,
David

Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.05


Once again, here is also a webrev that shows the difference 
between the original files and the new, unified file. Even if 
it's hard to read, it shows what the effects will be for each 
platform.


Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.04/ 



/Magnus



Thanks!

Thomas





On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie 
> wrote:


    When I was about to update jsig.c, I noticed that the four 
copies
    for aix, linux, macosx and solaris were basically the same, 
with
    only small differences. Some of the differences were not 
even well

    motivated, but were likely the result of this code duplication
    causing the code to diverge.

    A better solution is to unify them all into a single unix 
version,
    with the 

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[Magnus Ihse Bursie]

On 2018-04-05 13:07, Magnus Ihse Bursie wrote:

On 2018-04-05 12:30, David Holmes wrote:

On 5/04/2018 7:52 PM, Magnus Ihse Bursie wrote:

On 2018-04-05 10:30, David Holmes wrote:

On 5/04/2018 6:07 PM, Magnus Ihse Bursie wrote:



On 2018-04-04 09:59, David Holmes wrote:

Hi Magnus,

Sorry for the delay ...

On 28/03/2018 8:15 PM, Magnus Ihse Bursie wrote:


On 2018-03-27 18:24, Thomas Stüfe wrote:

Hi Magnus,

just a cursory look, will look in greater detail tomorrow.

This is good, thanks for doing this.

As I wrote offlist, please remove the painfully wrong AIX 
"workarounds". I do not know why we did this - the original 
code is quite old - my assumption is that dlsym(RTLD_NEXT) was 
not working as expected on older AIX versions. Well, it should 
work now according to IBMs manpages. Will test later.

Ok.


__thread is not Posix. I would prefer pthread_get/set_specific 
instead, which is more portable.


I have surrounded this code with #ifdef MACOSX now.

Here is an updated webrev. It includes the changes requested by 
you and David:


* No more AIX workarounds
* Solaris use pthreads
* The "reentry" code is #ifdef MACOSX.


That all seems good.

Good. :)



I also assumed that NSIG is available and working on Solaris. 
I'll let David decide if he is happy with that. The alternative 
is to go back to the Solaris-specific code that allocates sact 
on the heap.


Unfortunately NSIG is a problem on Solaris:

http://austingroupbugs.net/view.php?id=741

It's use also precludes the use of the real-time signals - which 
limits Linux as well. But I'm not completely clear on exactly how 
signals may be numbered in their entirety so I wouldn't 
necessarily suggest trying to use SIGRTMAX+1 as the number of 
available signals, other than on Solaris.
Ok. I hope I understand you correctly. I have replaced NSIG with 
MAX_SIGNALS, which is defined as NSIG on all platforms except 
Solaris, where it is defined as SIGRTMAX+1.


Updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.08

(8th time's a charm..?)


Nope - you can't use SIGRTMAX+1 to allocate a static array as it is 
not a constant expression. That's why Solaris uses malloc.


Duh. Right.

Okay. Like this, then?

http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.09

I have restored the calls to allocate_sact() in the same locations 
as in the original solaris version. Hopefully, those are correct. :-)


Yes you have restored them in the same locations.

As for correctness ... there are some preexisting issues I spotted. 
Do you really want to know? ;-) Well only one correctness issue, plus 
one unnecessary code issue:

Will it ever end?! :-)



Correctness:

83 #ifdef SOLARIS
  84   if (sact == NULL) {
  85 sact = (struct sigaction *)malloc((MAX_SIGNALS) * 
(size_t)sizeof(struct sigaction));
  86 memset(sact, 0, (MAX_SIGNALS) * (size_t)sizeof(struct 
sigaction));

  87   }
  88
  89   if (sact == NULL) {
  90 printf("%s\n", "libjsig.so unable to allocate memory");
  91 exit(0);
  92   }

The NULL check at line 89 needs to move to line 86 before we do 
memset on a NULL pointer.


Redundant code:

 142 static void save_signal_handler(int sig, sa_handler_t disp, bool 
is_sigset) {

 143   sigset_t set;
 144
 145   allocate_sact();

There are two calls to save_signal_handler, both preceded by a call 
to allocate_sact(), so we don't need to do it again at line 145.


Ok, I'll fix it while I'm at it.

New webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.10


Also, here is a webrev with the same patch applied to jdk/hs. (I have 
also included the needed changes to how libjsig is compiled, which are 
pushed to jdk/jdk but not yet integrated into jdk/hs). The patch file 
from this webrev can be applied to jdk/hs, so Thomas hopefully can 
test the AIX changes.


Let's hope this is the final iteration...
*LOL* Of course not. I forgot to press save in the editor before 
creating the webrev and starting the tests. *arrrghh* I can't believe this.


http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.12

/Magnus



/Magnus



Thanks,
David


/Magnus


David



/Magnus



Thanks,
David

Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.05 



Once again, here is also a webrev that shows the difference 
between the original files and the new, unified file. Even if 
it's hard to read, it shows what the effects will be for each 
platform.


Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.04/ 



/Magnus



Thanks!

Thomas





On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie 
> wrote:


    When I was about to update jsig.c, I noticed that the four 
copies
    for aix, linux, macosx and solaris were basically the same, 
with
    only small differences. Some of the differences were not 
even well

    motivated, but were likely the result of this code 

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[Magnus Ihse Bursie]

On 2018-04-05 13:07, Magnus Ihse Bursie wrote:

On 2018-04-05 12:30, David Holmes wrote:

On 5/04/2018 7:52 PM, Magnus Ihse Bursie wrote:

On 2018-04-05 10:30, David Holmes wrote:

On 5/04/2018 6:07 PM, Magnus Ihse Bursie wrote:



On 2018-04-04 09:59, David Holmes wrote:

Hi Magnus,

Sorry for the delay ...

On 28/03/2018 8:15 PM, Magnus Ihse Bursie wrote:


On 2018-03-27 18:24, Thomas Stüfe wrote:

Hi Magnus,

just a cursory look, will look in greater detail tomorrow.

This is good, thanks for doing this.

As I wrote offlist, please remove the painfully wrong AIX 
"workarounds". I do not know why we did this - the original 
code is quite old - my assumption is that dlsym(RTLD_NEXT) was 
not working as expected on older AIX versions. Well, it should 
work now according to IBMs manpages. Will test later.

Ok.


__thread is not Posix. I would prefer pthread_get/set_specific 
instead, which is more portable.


I have surrounded this code with #ifdef MACOSX now.

Here is an updated webrev. It includes the changes requested by 
you and David:


* No more AIX workarounds
* Solaris use pthreads
* The "reentry" code is #ifdef MACOSX.


That all seems good.

Good. :)



I also assumed that NSIG is available and working on Solaris. 
I'll let David decide if he is happy with that. The alternative 
is to go back to the Solaris-specific code that allocates sact 
on the heap.


Unfortunately NSIG is a problem on Solaris:

http://austingroupbugs.net/view.php?id=741

It's use also precludes the use of the real-time signals - which 
limits Linux as well. But I'm not completely clear on exactly how 
signals may be numbered in their entirety so I wouldn't 
necessarily suggest trying to use SIGRTMAX+1 as the number of 
available signals, other than on Solaris.
Ok. I hope I understand you correctly. I have replaced NSIG with 
MAX_SIGNALS, which is defined as NSIG on all platforms except 
Solaris, where it is defined as SIGRTMAX+1.


Updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.08

(8th time's a charm..?)


Nope - you can't use SIGRTMAX+1 to allocate a static array as it is 
not a constant expression. That's why Solaris uses malloc.


Duh. Right.

Okay. Like this, then?

http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.09

I have restored the calls to allocate_sact() in the same locations 
as in the original solaris version. Hopefully, those are correct. :-)


Yes you have restored them in the same locations.

As for correctness ... there are some preexisting issues I spotted. 
Do you really want to know? ;-) Well only one correctness issue, plus 
one unnecessary code issue:

Will it ever end?! :-)



Correctness:

83 #ifdef SOLARIS
  84   if (sact == NULL) {
  85 sact = (struct sigaction *)malloc((MAX_SIGNALS) * 
(size_t)sizeof(struct sigaction));
  86 memset(sact, 0, (MAX_SIGNALS) * (size_t)sizeof(struct 
sigaction));

  87   }
  88
  89   if (sact == NULL) {
  90 printf("%s\n", "libjsig.so unable to allocate memory");
  91 exit(0);
  92   }

The NULL check at line 89 needs to move to line 86 before we do 
memset on a NULL pointer.


Redundant code:

 142 static void save_signal_handler(int sig, sa_handler_t disp, bool 
is_sigset) {

 143   sigset_t set;
 144
 145   allocate_sact();

There are two calls to save_signal_handler, both preceded by a call 
to allocate_sact(), so we don't need to do it again at line 145.


Ok, I'll fix it while I'm at it.

New webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.10


Also, here is a webrev with the same patch applied to jdk/hs. (I have 
also included the needed changes to how libjsig is compiled, which are 
pushed to jdk/jdk but not yet integrated into jdk/hs). The patch file 
from this webrev can be applied to jdk/hs, so Thomas hopefully can 
test the AIX changes.

... and here is the link:

http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.11

/Magnus



Let's hope this is the final iteration...

/Magnus



Thanks,
David


/Magnus


David



/Magnus



Thanks,
David

Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.05 



Once again, here is also a webrev that shows the difference 
between the original files and the new, unified file. Even if 
it's hard to read, it shows what the effects will be for each 
platform.


Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.04/ 



/Magnus



Thanks!

Thomas





On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie 
> wrote:


    When I was about to update jsig.c, I noticed that the four 
copies
    for aix, linux, macosx and solaris were basically the same, 
with
    only small differences. Some of the differences were not 
even well

    motivated, but were likely the result of this code duplication
    causing the code to diverge.

    A better solution is to unify them all into a single unix 
version,

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[Magnus Ihse Bursie]

On 2018-04-05 12:30, David Holmes wrote:

On 5/04/2018 7:52 PM, Magnus Ihse Bursie wrote:

On 2018-04-05 10:30, David Holmes wrote:

On 5/04/2018 6:07 PM, Magnus Ihse Bursie wrote:



On 2018-04-04 09:59, David Holmes wrote:

Hi Magnus,

Sorry for the delay ...

On 28/03/2018 8:15 PM, Magnus Ihse Bursie wrote:


On 2018-03-27 18:24, Thomas Stüfe wrote:

Hi Magnus,

just a cursory look, will look in greater detail tomorrow.

This is good, thanks for doing this.

As I wrote offlist, please remove the painfully wrong AIX 
"workarounds". I do not know why we did this - the original code 
is quite old - my assumption is that dlsym(RTLD_NEXT) was not 
working as expected on older AIX versions. Well, it should work 
now according to IBMs manpages. Will test later.

Ok.


__thread is not Posix. I would prefer pthread_get/set_specific 
instead, which is more portable.


I have surrounded this code with #ifdef MACOSX now.

Here is an updated webrev. It includes the changes requested by 
you and David:


* No more AIX workarounds
* Solaris use pthreads
* The "reentry" code is #ifdef MACOSX.


That all seems good.

Good. :)



I also assumed that NSIG is available and working on Solaris. 
I'll let David decide if he is happy with that. The alternative 
is to go back to the Solaris-specific code that allocates sact on 
the heap.


Unfortunately NSIG is a problem on Solaris:

http://austingroupbugs.net/view.php?id=741

It's use also precludes the use of the real-time signals - which 
limits Linux as well. But I'm not completely clear on exactly how 
signals may be numbered in their entirety so I wouldn't 
necessarily suggest trying to use SIGRTMAX+1 as the number of 
available signals, other than on Solaris.
Ok. I hope I understand you correctly. I have replaced NSIG with 
MAX_SIGNALS, which is defined as NSIG on all platforms except 
Solaris, where it is defined as SIGRTMAX+1.


Updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.08

(8th time's a charm..?)


Nope - you can't use SIGRTMAX+1 to allocate a static array as it is 
not a constant expression. That's why Solaris uses malloc.


Duh. Right.

Okay. Like this, then?

http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.09

I have restored the calls to allocate_sact() in the same locations as 
in the original solaris version. Hopefully, those are correct. :-)


Yes you have restored them in the same locations.

As for correctness ... there are some preexisting issues I spotted. Do 
you really want to know? ;-) Well only one correctness issue, plus one 
unnecessary code issue:

Will it ever end?! :-)



Correctness:

83 #ifdef SOLARIS
  84   if (sact == NULL) {
  85 sact = (struct sigaction *)malloc((MAX_SIGNALS) * 
(size_t)sizeof(struct sigaction));
  86 memset(sact, 0, (MAX_SIGNALS) * (size_t)sizeof(struct 
sigaction));

  87   }
  88
  89   if (sact == NULL) {
  90 printf("%s\n", "libjsig.so unable to allocate memory");
  91 exit(0);
  92   }

The NULL check at line 89 needs to move to line 86 before we do memset 
on a NULL pointer.


Redundant code:

 142 static void save_signal_handler(int sig, sa_handler_t disp, bool 
is_sigset) {

 143   sigset_t set;
 144
 145   allocate_sact();

There are two calls to save_signal_handler, both preceded by a call to 
allocate_sact(), so we don't need to do it again at line 145.


Ok, I'll fix it while I'm at it.

New webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.10


Also, here is a webrev with the same patch applied to jdk/hs. (I have 
also included the needed changes to how libjsig is compiled, which are 
pushed to jdk/jdk but not yet integrated into jdk/hs). The patch file 
from this webrev can be applied to jdk/hs, so Thomas hopefully can test 
the AIX changes.


Let's hope this is the final iteration...

/Magnus



Thanks,
David


/Magnus


David



/Magnus



Thanks,
David

Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.05


Once again, here is also a webrev that shows the difference 
between the original files and the new, unified file. Even if 
it's hard to read, it shows what the effects will be for each 
platform.


Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.04/ 



/Magnus



Thanks!

Thomas





On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie 
> wrote:


    When I was about to update jsig.c, I noticed that the four 
copies
    for aix, linux, macosx and solaris were basically the same, 
with
    only small differences. Some of the differences were not 
even well

    motivated, but were likely the result of this code duplication
    causing the code to diverge.

    A better solution is to unify them all into a single unix 
version,
    with the few platform-specific changes handled on a 
per-platform

    basis.

    I've made the following notable changes:

    * I have removed the 

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[David Holmes]

On 5/04/2018 7:52 PM, Magnus Ihse Bursie wrote:

On 2018-04-05 10:30, David Holmes wrote:

On 5/04/2018 6:07 PM, Magnus Ihse Bursie wrote:



On 2018-04-04 09:59, David Holmes wrote:

Hi Magnus,

Sorry for the delay ...

On 28/03/2018 8:15 PM, Magnus Ihse Bursie wrote:


On 2018-03-27 18:24, Thomas Stüfe wrote:

Hi Magnus,

just a cursory look, will look in greater detail tomorrow.

This is good, thanks for doing this.

As I wrote offlist, please remove the painfully wrong AIX 
"workarounds". I do not know why we did this - the original code 
is quite old - my assumption is that dlsym(RTLD_NEXT) was not 
working as expected on older AIX versions. Well, it should work 
now according to IBMs manpages. Will test later.

Ok.


__thread is not Posix. I would prefer pthread_get/set_specific 
instead, which is more portable.


I have surrounded this code with #ifdef MACOSX now.

Here is an updated webrev. It includes the changes requested by you 
and David:


* No more AIX workarounds
* Solaris use pthreads
* The "reentry" code is #ifdef MACOSX.


That all seems good.

Good. :)



I also assumed that NSIG is available and working on Solaris. I'll 
let David decide if he is happy with that. The alternative is to go 
back to the Solaris-specific code that allocates sact on the heap.


Unfortunately NSIG is a problem on Solaris:

http://austingroupbugs.net/view.php?id=741

It's use also precludes the use of the real-time signals - which 
limits Linux as well. But I'm not completely clear on exactly how 
signals may be numbered in their entirety so I wouldn't necessarily 
suggest trying to use SIGRTMAX+1 as the number of available signals, 
other than on Solaris.
Ok. I hope I understand you correctly. I have replaced NSIG with 
MAX_SIGNALS, which is defined as NSIG on all platforms except 
Solaris, where it is defined as SIGRTMAX+1.


Updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.08

(8th time's a charm..?)


Nope - you can't use SIGRTMAX+1 to allocate a static array as it is 
not a constant expression. That's why Solaris uses malloc.


Duh. Right.

Okay. Like this, then?

http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.09

I have restored the calls to allocate_sact() in the same locations as in 
the original solaris version. Hopefully, those are correct. :-)


Yes you have restored them in the same locations.

As for correctness ... there are some preexisting issues I spotted. Do 
you really want to know? ;-) Well only one correctness issue, plus one 
unnecessary code issue:


Correctness:

83 #ifdef SOLARIS
  84   if (sact == NULL) {
  85 sact = (struct sigaction *)malloc((MAX_SIGNALS) * 
(size_t)sizeof(struct sigaction));

  86 memset(sact, 0, (MAX_SIGNALS) * (size_t)sizeof(struct sigaction));
  87   }
  88
  89   if (sact == NULL) {
  90 printf("%s\n", "libjsig.so unable to allocate memory");
  91 exit(0);
  92   }

The NULL check at line 89 needs to move to line 86 before we do memset 
on a NULL pointer.


Redundant code:

 142 static void save_signal_handler(int sig, sa_handler_t disp, bool 
is_sigset) {

 143   sigset_t set;
 144
 145   allocate_sact();

There are two calls to save_signal_handler, both preceded by a call to 
allocate_sact(), so we don't need to do it again at line 145.


Thanks,
David


/Magnus


David



/Magnus



Thanks,
David

Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.05


Once again, here is also a webrev that shows the difference between 
the original files and the new, unified file. Even if it's hard to 
read, it shows what the effects will be for each platform.


Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.04/


/Magnus



Thanks!

Thomas





On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie 
> wrote:


    When I was about to update jsig.c, I noticed that the four copies
    for aix, linux, macosx and solaris were basically the same, with
    only small differences. Some of the differences were not even 
well

    motivated, but were likely the result of this code duplication
    causing the code to diverge.

    A better solution is to unify them all into a single unix 
version,

    with the few platform-specific changes handled on a per-platform
    basis.

    I've made the following notable changes:

    * I have removed the version check for Solaris. All other
    platforms seem to do fine without it, and in general, we don't
    mistrust other JDK libraries. An alternative is to add this
    version check to all other platforms instead. If you think 
this is

    the correct course of action, let me know and I'll fix it.

    * Solaris used to have a dynamically allocated sact, instead of a
    statically allocated array as all other platforms have. It's not
    likely to be large, and the size is known at compile time, so
    there seems to be no good reason for 

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[Magnus Ihse Bursie]

On 2018-04-05 10:30, David Holmes wrote:

On 5/04/2018 6:07 PM, Magnus Ihse Bursie wrote:



On 2018-04-04 09:59, David Holmes wrote:

Hi Magnus,

Sorry for the delay ...

On 28/03/2018 8:15 PM, Magnus Ihse Bursie wrote:


On 2018-03-27 18:24, Thomas Stüfe wrote:

Hi Magnus,

just a cursory look, will look in greater detail tomorrow.

This is good, thanks for doing this.

As I wrote offlist, please remove the painfully wrong AIX 
"workarounds". I do not know why we did this - the original code 
is quite old - my assumption is that dlsym(RTLD_NEXT) was not 
working as expected on older AIX versions. Well, it should work 
now according to IBMs manpages. Will test later.

Ok.


__thread is not Posix. I would prefer pthread_get/set_specific 
instead, which is more portable.


I have surrounded this code with #ifdef MACOSX now.

Here is an updated webrev. It includes the changes requested by you 
and David:


* No more AIX workarounds
* Solaris use pthreads
* The "reentry" code is #ifdef MACOSX.


That all seems good.

Good. :)



I also assumed that NSIG is available and working on Solaris. I'll 
let David decide if he is happy with that. The alternative is to go 
back to the Solaris-specific code that allocates sact on the heap.


Unfortunately NSIG is a problem on Solaris:

http://austingroupbugs.net/view.php?id=741

It's use also precludes the use of the real-time signals - which 
limits Linux as well. But I'm not completely clear on exactly how 
signals may be numbered in their entirety so I wouldn't necessarily 
suggest trying to use SIGRTMAX+1 as the number of available signals, 
other than on Solaris.
Ok. I hope I understand you correctly. I have replaced NSIG with 
MAX_SIGNALS, which is defined as NSIG on all platforms except 
Solaris, where it is defined as SIGRTMAX+1.


Updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.08

(8th time's a charm..?)


Nope - you can't use SIGRTMAX+1 to allocate a static array as it is 
not a constant expression. That's why Solaris uses malloc.


Duh. Right.

Okay. Like this, then?

http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.09

I have restored the calls to allocate_sact() in the same locations as in 
the original solaris version. Hopefully, those are correct. :-)


/Magnus


David



/Magnus



Thanks,
David

Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.05


Once again, here is also a webrev that shows the difference between 
the original files and the new, unified file. Even if it's hard to 
read, it shows what the effects will be for each platform.


Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.04/


/Magnus



Thanks!

Thomas





On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie 
> wrote:


    When I was about to update jsig.c, I noticed that the four copies
    for aix, linux, macosx and solaris were basically the same, with
    only small differences. Some of the differences were not even 
well

    motivated, but were likely the result of this code duplication
    causing the code to diverge.

    A better solution is to unify them all into a single unix 
version,

    with the few platform-specific changes handled on a per-platform
    basis.

    I've made the following notable changes:

    * I have removed the version check for Solaris. All other
    platforms seem to do fine without it, and in general, we don't
    mistrust other JDK libraries. An alternative is to add this
    version check to all other platforms instead. If you think 
this is

    the correct course of action, let me know and I'll fix it.

    * Solaris used to have a dynamically allocated sact, instead of a
    statically allocated array as all other platforms have. It's not
    likely to be large, and the size is known at compile time, so
    there seems to be no good reason for this.

    * Linux and macosx used a uint32_t/uint64_t instead of sigset_t
    for jvmsigs, as aix and solaris do. This is a less robust
    solution, and the added checks show that it has failed in the
    past. Now all platforms use sigset_t/sigismember().

    Also worth noting:

    * Solaris is not using pthreads, but it's own thread library,
    which accounts for most of the #ifdef SOLARIS.

    * In general, if an implementation was needed on one platform, 
but

    has no effect or is harmless on others, I've kept it on all
    platforms instead of sprinkling the code with #ifdefs.

    To facilitate code review, here is a specially crafted webrev 
that
    shows the differences compared to each of the individual, 
original

    per-OS versions of jsig.c:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.01
 



    Bug: https://bugs.openjdk.java.net/browse/JDK-8200298

    

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[Magnus Ihse Bursie]

On 2018-04-04 09:59, David Holmes wrote:

Hi Magnus,

Sorry for the delay ...

On 28/03/2018 8:15 PM, Magnus Ihse Bursie wrote:


On 2018-03-27 18:24, Thomas Stüfe wrote:

Hi Magnus,

just a cursory look, will look in greater detail tomorrow.

This is good, thanks for doing this.

As I wrote offlist, please remove the painfully wrong AIX 
"workarounds". I do not know why we did this - the original code is 
quite old - my assumption is that dlsym(RTLD_NEXT) was not working 
as expected on older AIX versions. Well, it should work now 
according to IBMs manpages. Will test later.

Ok.


__thread is not Posix. I would prefer pthread_get/set_specific 
instead, which is more portable.


I have surrounded this code with #ifdef MACOSX now.

Here is an updated webrev. It includes the changes requested by you 
and David:


* No more AIX workarounds
* Solaris use pthreads
* The "reentry" code is #ifdef MACOSX.


That all seems good.

Good. :)



I also assumed that NSIG is available and working on Solaris. I'll 
let David decide if he is happy with that. The alternative is to go 
back to the Solaris-specific code that allocates sact on the heap.


Unfortunately NSIG is a problem on Solaris:

http://austingroupbugs.net/view.php?id=741

It's use also precludes the use of the real-time signals - which 
limits Linux as well. But I'm not completely clear on exactly how 
signals may be numbered in their entirety so I wouldn't necessarily 
suggest trying to use SIGRTMAX+1 as the number of available signals, 
other than on Solaris.
Ok. I hope I understand you correctly. I have replaced NSIG with 
MAX_SIGNALS, which is defined as NSIG on all platforms except Solaris, 
where it is defined as SIGRTMAX+1.


Updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.08

(8th time's a charm..?)

/Magnus



Thanks,
David

Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.05


Once again, here is also a webrev that shows the difference between 
the original files and the new, unified file. Even if it's hard to 
read, it shows what the effects will be for each platform.


Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.04/


/Magnus



Thanks!

Thomas





On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie 
> wrote:


    When I was about to update jsig.c, I noticed that the four copies
    for aix, linux, macosx and solaris were basically the same, with
    only small differences. Some of the differences were not even well
    motivated, but were likely the result of this code duplication
    causing the code to diverge.

    A better solution is to unify them all into a single unix version,
    with the few platform-specific changes handled on a per-platform
    basis.

    I've made the following notable changes:

    * I have removed the version check for Solaris. All other
    platforms seem to do fine without it, and in general, we don't
    mistrust other JDK libraries. An alternative is to add this
    version check to all other platforms instead. If you think this is
    the correct course of action, let me know and I'll fix it.

    * Solaris used to have a dynamically allocated sact, instead of a
    statically allocated array as all other platforms have. It's not
    likely to be large, and the size is known at compile time, so
    there seems to be no good reason for this.

    * Linux and macosx used a uint32_t/uint64_t instead of sigset_t
    for jvmsigs, as aix and solaris do. This is a less robust
    solution, and the added checks show that it has failed in the
    past. Now all platforms use sigset_t/sigismember().

    Also worth noting:

    * Solaris is not using pthreads, but it's own thread library,
    which accounts for most of the #ifdef SOLARIS.

    * In general, if an implementation was needed on one platform, but
    has no effect or is harmless on others, I've kept it on all
    platforms instead of sprinkling the code with #ifdefs.

    To facilitate code review, here is a specially crafted webrev that
    shows the differences compared to each of the individual, original
    per-OS versions of jsig.c:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.01


    Bug: https://bugs.openjdk.java.net/browse/JDK-8200298
    
    WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.03


    /Magnus

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[David Holmes]

On 5/04/2018 6:07 PM, Magnus Ihse Bursie wrote:



On 2018-04-04 09:59, David Holmes wrote:

Hi Magnus,

Sorry for the delay ...

On 28/03/2018 8:15 PM, Magnus Ihse Bursie wrote:


On 2018-03-27 18:24, Thomas Stüfe wrote:

Hi Magnus,

just a cursory look, will look in greater detail tomorrow.

This is good, thanks for doing this.

As I wrote offlist, please remove the painfully wrong AIX 
"workarounds". I do not know why we did this - the original code is 
quite old - my assumption is that dlsym(RTLD_NEXT) was not working 
as expected on older AIX versions. Well, it should work now 
according to IBMs manpages. Will test later.

Ok.


__thread is not Posix. I would prefer pthread_get/set_specific 
instead, which is more portable.


I have surrounded this code with #ifdef MACOSX now.

Here is an updated webrev. It includes the changes requested by you 
and David:


* No more AIX workarounds
* Solaris use pthreads
* The "reentry" code is #ifdef MACOSX.


That all seems good.

Good. :)



I also assumed that NSIG is available and working on Solaris. I'll 
let David decide if he is happy with that. The alternative is to go 
back to the Solaris-specific code that allocates sact on the heap.


Unfortunately NSIG is a problem on Solaris:

http://austingroupbugs.net/view.php?id=741

It's use also precludes the use of the real-time signals - which 
limits Linux as well. But I'm not completely clear on exactly how 
signals may be numbered in their entirety so I wouldn't necessarily 
suggest trying to use SIGRTMAX+1 as the number of available signals, 
other than on Solaris.
Ok. I hope I understand you correctly. I have replaced NSIG with 
MAX_SIGNALS, which is defined as NSIG on all platforms except Solaris, 
where it is defined as SIGRTMAX+1.


Updated webrev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.08

(8th time's a charm..?)


Nope - you can't use SIGRTMAX+1 to allocate a static array as it is not 
a constant expression. That's why Solaris uses malloc.


David



/Magnus



Thanks,
David

Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.05


Once again, here is also a webrev that shows the difference between 
the original files and the new, unified file. Even if it's hard to 
read, it shows what the effects will be for each platform.


Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.04/


/Magnus



Thanks!

Thomas





On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie 
> wrote:


    When I was about to update jsig.c, I noticed that the four copies
    for aix, linux, macosx and solaris were basically the same, with
    only small differences. Some of the differences were not even well
    motivated, but were likely the result of this code duplication
    causing the code to diverge.

    A better solution is to unify them all into a single unix version,
    with the few platform-specific changes handled on a per-platform
    basis.

    I've made the following notable changes:

    * I have removed the version check for Solaris. All other
    platforms seem to do fine without it, and in general, we don't
    mistrust other JDK libraries. An alternative is to add this
    version check to all other platforms instead. If you think this is
    the correct course of action, let me know and I'll fix it.

    * Solaris used to have a dynamically allocated sact, instead of a
    statically allocated array as all other platforms have. It's not
    likely to be large, and the size is known at compile time, so
    there seems to be no good reason for this.

    * Linux and macosx used a uint32_t/uint64_t instead of sigset_t
    for jvmsigs, as aix and solaris do. This is a less robust
    solution, and the added checks show that it has failed in the
    past. Now all platforms use sigset_t/sigismember().

    Also worth noting:

    * Solaris is not using pthreads, but it's own thread library,
    which accounts for most of the #ifdef SOLARIS.

    * In general, if an implementation was needed on one platform, but
    has no effect or is harmless on others, I've kept it on all
    platforms instead of sprinkling the code with #ifdefs.

    To facilitate code review, here is a specially crafted webrev that
    shows the differences compared to each of the individual, original
    per-OS versions of jsig.c:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.01
 



    Bug: https://bugs.openjdk.java.net/browse/JDK-8200298
    
    WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.03
 



    /Magnus

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[David Holmes]

Hi Magnus,

Sorry for the delay ...

On 28/03/2018 8:15 PM, Magnus Ihse Bursie wrote:


On 2018-03-27 18:24, Thomas Stüfe wrote:

Hi Magnus,

just a cursory look, will look in greater detail tomorrow.

This is good, thanks for doing this.

As I wrote offlist, please remove the painfully wrong AIX 
"workarounds". I do not know why we did this - the original code is 
quite old - my assumption is that dlsym(RTLD_NEXT) was not working as 
expected on older AIX versions. Well, it should work now according to 
IBMs manpages. Will test later.

Ok.


__thread is not Posix. I would prefer pthread_get/set_specific 
instead, which is more portable.


I have surrounded this code with #ifdef MACOSX now.

Here is an updated webrev. It includes the changes requested by you and 
David:


* No more AIX workarounds
* Solaris use pthreads
* The "reentry" code is #ifdef MACOSX.


That all seems good.

I also assumed that NSIG is available and working on Solaris. I'll let 
David decide if he is happy with that. The alternative is to go back to 
the Solaris-specific code that allocates sact on the heap.


Unfortunately NSIG is a problem on Solaris:

http://austingroupbugs.net/view.php?id=741

It's use also precludes the use of the real-time signals - which limits 
Linux as well. But I'm not completely clear on exactly how signals may 
be numbered in their entirety so I wouldn't necessarily suggest trying 
to use SIGRTMAX+1 as the number of available signals, other than on Solaris.


Thanks,
David

Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.05


Once again, here is also a webrev that shows the difference between the 
original files and the new, unified file. Even if it's hard to read, it 
shows what the effects will be for each platform.


Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.04/


/Magnus



Thanks!

Thomas





On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie 
> 
wrote:


    When I was about to update jsig.c, I noticed that the four copies
    for aix, linux, macosx and solaris were basically the same, with
    only small differences. Some of the differences were not even well
    motivated, but were likely the result of this code duplication
    causing the code to diverge.

    A better solution is to unify them all into a single unix version,
    with the few platform-specific changes handled on a per-platform
    basis.

    I've made the following notable changes:

    * I have removed the version check for Solaris. All other
    platforms seem to do fine without it, and in general, we don't
    mistrust other JDK libraries. An alternative is to add this
    version check to all other platforms instead. If you think this is
    the correct course of action, let me know and I'll fix it.

    * Solaris used to have a dynamically allocated sact, instead of a
    statically allocated array as all other platforms have. It's not
    likely to be large, and the size is known at compile time, so
    there seems to be no good reason for this.

    * Linux and macosx used a uint32_t/uint64_t instead of sigset_t
    for jvmsigs, as aix and solaris do. This is a less robust
    solution, and the added checks show that it has failed in the
    past. Now all platforms use sigset_t/sigismember().

    Also worth noting:

    * Solaris is not using pthreads, but it's own thread library,
    which accounts for most of the #ifdef SOLARIS.

    * In general, if an implementation was needed on one platform, but
    has no effect or is harmless on others, I've kept it on all
    platforms instead of sprinkling the code with #ifdefs.

    To facilitate code review, here is a specially crafted webrev that
    shows the differences compared to each of the individual, original
    per-OS versions of jsig.c:
    http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.01




    Bug: https://bugs.openjdk.java.net/browse/JDK-8200298
    
    WebRev:
    http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.03




    /Magnus

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[Magnus Ihse Bursie]

On 2018-03-29 14:51, Thomas Stüfe wrote:

Hi Magnus,

On Thu, Mar 29, 2018 at 8:47 AM, Thomas Stüfe > wrote:




On Thu, Mar 29, 2018 at 12:37 AM, Magnus Ihse Bursie
> wrote:

On 2018-03-28 21:21, Thomas Stüfe wrote:

Hi Magnus,

I had a closer look at the changes, especially at jsig.c. It
all comes slowly back. The AIX version has been almost
comically wrong.

About NSIG, I remember that we had coding in our port which
needed to know the max number of signals, and this was
surprisingly hard since on some platforms. Sometimes NSIG
does not contain the real time signals. Or it did not even
exist. I think we ended up hardcoding an own max signal limit .

So wherever you access sact with a signal number as index,
I'd like to see a bounds check. Or at least an assert - since
this is plain C, a C-assert would do for me (see
http://pubs.opengroup.org/onlinepubs/009695399/functions/assert.html
).
This also would guard against user parameter error.

That's probably a good improvement. Do you think it could be
handled as a follow-up bug?


A simple bounds check would suffice for me. At the start of the
external sigaction() and sigset(), just do a:

if (sig < 0 || sig >= NSIG) {
  return -1;
}

Okay. I've added something like this (a cast was also needed, and we 
should set errno if returning an error, to mimick system behaviour).




In the external signal(), do a:

if (sig < 0 || sig >= NSIG) {
return SIGERR;
}



I also wonder whether this coding could not be simplified
quite a bit by not calling the OS versions of signal() at all
but instead doing all signal work via OS sigaction: after
all, signal() can be implemented in terms of sigaction()
(with the flag SA_SIGINFO cleared), and so can sigset(). This
would remove the necessity of reentry-handling for macos, and
also remove the need for save_signal_handler().

That also sounds like an excellent suggestion. Do you think it
could be handled as a follow-up bug?


Sure!



I will test this version on AIX tomorrow. After that, I'll
have some vacation, but maybe someone else from my team will
take over.

I like this work, this is a good simplification.

Thanks. :)

However, it's a bit outside what I normally do. :) Not that I
dislike writing some good old C for a change, but I'm really
trying to focus on cleaning up the flag handling in the build
system. This was just a side-track when I was going to make a
change in the jsig.c files (for adding JNIEXPORT) and realized
it was four almost identical copies. I thought it would be
trivial to unify them, and if it's trivial, it's better to do
it yourself right away, than to file a bug on someone else, or
so my motto goes. :)


However, it turned out to be more work than I expected, and
I'm losing interest in pushing this any further. Still, it
would be a shame if the work I've done so far go to waste, but
I'd really prefer it if someone else could pick up this patch
and finish it.


You are almost there. Lets finish this.


Oookay, I'll give it a few more cycles :)



The source looks good to me, if you add above mentioned bounds
checks. I'll build it on AIX later today (I just found out your
8200357 - which I need to build on AIX - does not apply to hs, and
I need to switch to jdk/jdk. Sigh. The unification of hs/jdk
cannot come too early.)


I'm about to give up.

I cannot build jdk/jdk on AIX since it misses a number of fixes for 
all include changes which happened in hotspot lately.

On jdk/hs your libjsig change won't apply.

So I am stuck here and a bit out of time. If you want to press it, go 
on push it and we fix any follow up errors on AIX after easter. I am 
fine with the look of the change (modulo the sig bounds check 
mentioned earlier).


I'm in no hurry to push this.

Here's an updated webrev with the bounds check in place. Let me know 
if/when you can try it on AIX, and/or if you need help to get the patch 
to apply. You might also have noticed that the signal tests are being 
open sourced, right now, which will definitively help you. (Just a lucky 
coincidence!)


Webrev: http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.07

/Magnus





Best Regards,

Thomas


..Thomas

/Magnus




Thanks, Thomas




On Wed, Mar 28, 2018 at 12:15 PM, Magnus Ihse Bursie

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[Thomas Stüfe]

Hi Magnus,

On Thu, Mar 29, 2018 at 8:47 AM, Thomas Stüfe 
wrote:

>
>
> On Thu, Mar 29, 2018 at 12:37 AM, Magnus Ihse Bursie <
> magnus.ihse.bur...@oracle.com> wrote:
>
>> On 2018-03-28 21:21, Thomas Stüfe wrote:
>>
>> Hi Magnus,
>>
>> I had a closer look at the changes, especially at jsig.c. It all comes
>> slowly back. The AIX version has been almost comically wrong.
>>
>> About NSIG, I remember that we had coding in our port which needed to
>> know the max number of signals, and this was surprisingly hard since on
>> some platforms. Sometimes NSIG does not contain the real time signals. Or
>> it did not even exist. I think we ended up hardcoding an own max signal
>> limit .
>>
>> So wherever you access sact with a signal number as index, I'd like to
>> see a bounds check. Or at least an assert - since this is plain C, a
>> C-assert would do for me (see http://pubs.opengroup.org/onli
>> nepubs/009695399/functions/assert.html). This also would guard against
>> user parameter error.
>>
>> That's probably a good improvement. Do you think it could be handled as a
>> follow-up bug?
>>
>>
> A simple bounds check would suffice for me. At the start of the external
> sigaction() and sigset(), just do a:
>
> if (sig < 0 || sig >= NSIG) {
>   return -1;
> }
>
> In the external signal(), do a:
>
> if (sig < 0 || sig >= NSIG) {
>   return SIGERR;
> }
>
>
>> I also wonder whether this coding could not be simplified quite a bit by
>> not calling the OS versions of signal() at all but instead doing all signal
>> work via OS sigaction: after all, signal() can be implemented in terms of
>> sigaction() (with the flag SA_SIGINFO cleared), and so can sigset().
>> This would remove the necessity of reentry-handling for macos, and also
>> remove the need for save_signal_handler().
>>
>> That also sounds like an excellent suggestion. Do you think it could be
>> handled as a follow-up bug?
>>
>
> Sure!
>
>>
>> I will test this version on AIX tomorrow. After that, I'll have some
>> vacation, but maybe someone else from my team will take over.
>>
>> I like this work, this is a good simplification.
>>
>> Thanks. :)
>>
>> However, it's a bit outside what I normally do. :) Not that I dislike
>> writing some good old C for a change, but I'm really trying to focus on
>> cleaning up the flag handling in the build system. This was just a
>> side-track when I was going to make a change in the jsig.c files (for
>> adding JNIEXPORT) and realized it was four almost identical copies. I
>> thought it would be trivial to unify them, and if it's trivial, it's better
>> to do it yourself right away, than to file a bug on someone else, or so my
>> motto goes. :)
>>
>>
> However, it turned out to be more work than I expected, and I'm losing
>> interest in pushing this any further. Still, it would be a shame if the
>> work I've done so far go to waste, but I'd really prefer it if someone else
>> could pick up this patch and finish it.
>>
>>
> You are almost there. Lets finish this.
>
> The source looks good to me, if you add above mentioned bounds checks.
> I'll build it on AIX later today (I just found out your 8200357 - which I
> need to build on AIX - does not apply to hs, and I need to switch to
> jdk/jdk. Sigh. The unification of hs/jdk cannot come too early.)
>
>
I'm about to give up.

I cannot build jdk/jdk on AIX since it misses a number of fixes for all
include changes which happened in hotspot lately.
On jdk/hs your libjsig change won't apply.

So I am stuck here and a bit out of time. If you want to press it, go on
push it and we fix any follow up errors on AIX after easter. I am fine with
the look of the change (modulo the sig bounds check mentioned earlier).

Best Regards,

Thomas




> ..Thomas
>
>
>
>> /Magnus
>>
>>
>>
>> Thanks, Thomas
>>
>>
>>
>>
>> On Wed, Mar 28, 2018 at 12:15 PM, Magnus Ihse Bursie <
>> magnus.ihse.bur...@oracle.com> wrote:
>>
>>>
>>> On 2018-03-27 18:24, Thomas Stüfe wrote:
>>>
>>> Hi Magnus,
>>>
>>> just a cursory look, will look in greater detail tomorrow.
>>>
>>> This is good, thanks for doing this.
>>>
>>> As I wrote offlist, please remove the painfully wrong AIX "workarounds".
>>> I do not know why we did this - the original code is quite old - my
>>> assumption is that dlsym(RTLD_NEXT) was not working as expected on older
>>> AIX versions. Well, it should work now according to IBMs manpages. Will
>>> test later.
>>>
>>> Ok.
>>>
>>>
>>> __thread is not Posix. I would prefer pthread_get/set_specific instead,
>>> which is more portable.
>>>
>>>
>>> I have surrounded this code with #ifdef MACOSX now.
>>>
>>> Here is an updated webrev. It includes the changes requested by you and
>>> David:
>>>
>>> * No more AIX workarounds
>>> * Solaris use pthreads
>>> * The "reentry" code is #ifdef MACOSX.
>>>
>>> I also assumed that NSIG is available and working on Solaris. I'll let
>>> David decide if he is happy with that. The alternative is to go back to the
>>> Solaris-specific code that 

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[Thomas Stüfe]

On Thu, Mar 29, 2018 at 12:37 AM, Magnus Ihse Bursie <
magnus.ihse.bur...@oracle.com> wrote:

> On 2018-03-28 21:21, Thomas Stüfe wrote:
>
> Hi Magnus,
>
> I had a closer look at the changes, especially at jsig.c. It all comes
> slowly back. The AIX version has been almost comically wrong.
>
> About NSIG, I remember that we had coding in our port which needed to know
> the max number of signals, and this was surprisingly hard since on some
> platforms. Sometimes NSIG does not contain the real time signals. Or it did
> not even exist. I think we ended up hardcoding an own max signal limit .
>
> So wherever you access sact with a signal number as index, I'd like to see
> a bounds check. Or at least an assert - since this is plain C, a C-assert
> would do for me (see http://pubs.opengroup.org/onlinepubs/009695399/
> functions/assert.html). This also would guard against user parameter
> error.
>
> That's probably a good improvement. Do you think it could be handled as a
> follow-up bug?
>
>
A simple bounds check would suffice for me. At the start of the external
sigaction() and sigset(), just do a:

if (sig < 0 || sig >= NSIG) {
  return -1;
}

In the external signal(), do a:

if (sig < 0 || sig >= NSIG) {
  return SIGERR;
}


> I also wonder whether this coding could not be simplified quite a bit by
> not calling the OS versions of signal() at all but instead doing all signal
> work via OS sigaction: after all, signal() can be implemented in terms of
> sigaction() (with the flag SA_SIGINFO cleared), and so can sigset(). This
> would remove the necessity of reentry-handling for macos, and also remove
> the need for save_signal_handler().
>
> That also sounds like an excellent suggestion. Do you think it could be
> handled as a follow-up bug?
>

Sure!

>
> I will test this version on AIX tomorrow. After that, I'll have some
> vacation, but maybe someone else from my team will take over.
>
> I like this work, this is a good simplification.
>
> Thanks. :)
>
> However, it's a bit outside what I normally do. :) Not that I dislike
> writing some good old C for a change, but I'm really trying to focus on
> cleaning up the flag handling in the build system. This was just a
> side-track when I was going to make a change in the jsig.c files (for
> adding JNIEXPORT) and realized it was four almost identical copies. I
> thought it would be trivial to unify them, and if it's trivial, it's better
> to do it yourself right away, than to file a bug on someone else, or so my
> motto goes. :)
>
>
However, it turned out to be more work than I expected, and I'm losing
> interest in pushing this any further. Still, it would be a shame if the
> work I've done so far go to waste, but I'd really prefer it if someone else
> could pick up this patch and finish it.
>
>
You are almost there. Lets finish this.

The source looks good to me, if you add above mentioned bounds checks. I'll
build it on AIX later today (I just found out your 8200357 - which I need
to build on AIX - does not apply to hs, and I need to switch to jdk/jdk.
Sigh. The unification of hs/jdk cannot come too early.)

..Thomas


> /Magnus
>
>
>
> Thanks, Thomas
>
>
>
>
> On Wed, Mar 28, 2018 at 12:15 PM, Magnus Ihse Bursie <
> magnus.ihse.bur...@oracle.com> wrote:
>
>>
>> On 2018-03-27 18:24, Thomas Stüfe wrote:
>>
>> Hi Magnus,
>>
>> just a cursory look, will look in greater detail tomorrow.
>>
>> This is good, thanks for doing this.
>>
>> As I wrote offlist, please remove the painfully wrong AIX "workarounds".
>> I do not know why we did this - the original code is quite old - my
>> assumption is that dlsym(RTLD_NEXT) was not working as expected on older
>> AIX versions. Well, it should work now according to IBMs manpages. Will
>> test later.
>>
>> Ok.
>>
>>
>> __thread is not Posix. I would prefer pthread_get/set_specific instead,
>> which is more portable.
>>
>>
>> I have surrounded this code with #ifdef MACOSX now.
>>
>> Here is an updated webrev. It includes the changes requested by you and
>> David:
>>
>> * No more AIX workarounds
>> * Solaris use pthreads
>> * The "reentry" code is #ifdef MACOSX.
>>
>> I also assumed that NSIG is available and working on Solaris. I'll let
>> David decide if he is happy with that. The alternative is to go back to the
>> Solaris-specific code that allocates sact on the heap.
>>
>> Webrev: http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/
>> webrev.05
>>
>> Once again, here is also a webrev that shows the difference between the
>> original files and the new, unified file. Even if it's hard to read, it
>> shows what the effects will be for each platform.
>>
>> Webrev: http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/
>> webrev.04/
>>
>> /Magnus
>>
>>
>> Thanks!
>>
>> Thomas
>>
>>
>>
>>
>>
>> On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie <
>> magnus.ihse.bur...@oracle.com> wrote:
>>
>>> When I was about to update jsig.c, I noticed that the four copies for
>>> aix, linux, macosx and solaris were basically 

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[Magnus Ihse Bursie]

On 2018-03-28 21:21, Thomas Stüfe wrote:

Hi Magnus,

I had a closer look at the changes, especially at jsig.c. It all comes 
slowly back. The AIX version has been almost comically wrong.


About NSIG, I remember that we had coding in our port which needed to 
know the max number of signals, and this was surprisingly hard since 
on some platforms. Sometimes NSIG does not contain the real time 
signals. Or it did not even exist. I think we ended up hardcoding an 
own max signal limit .


So wherever you access sact with a signal number as index, I'd like to 
see a bounds check. Or at least an assert - since this is plain C, a 
C-assert would do for me (see 
http://pubs.opengroup.org/onlinepubs/009695399/functions/assert.html). 
This also would guard against user parameter error.
That's probably a good improvement. Do you think it could be handled as 
a follow-up bug?




I also wonder whether this coding could not be simplified quite a bit 
by not calling the OS versions of signal() at all but instead doing 
all signal work via OS sigaction: after all, signal() can be 
implemented in terms of sigaction() (with the flag SA_SIGINFO 
cleared), and so can sigset(). This would remove the necessity of 
reentry-handling for macos, and also remove the need for 
save_signal_handler().
That also sounds like an excellent suggestion. Do you think it could be 
handled as a follow-up bug?


I will test this version on AIX tomorrow. After that, I'll have some 
vacation, but maybe someone else from my team will take over.


I like this work, this is a good simplification.

Thanks. :)

However, it's a bit outside what I normally do. :) Not that I dislike 
writing some good old C for a change, but I'm really trying to focus on 
cleaning up the flag handling in the build system. This was just a 
side-track when I was going to make a change in the jsig.c files (for 
adding JNIEXPORT) and realized it was four almost identical copies. I 
thought it would be trivial to unify them, and if it's trivial, it's 
better to do it yourself right away, than to file a bug on someone else, 
or so my motto goes. :)


However, it turned out to be more work than I expected, and I'm losing 
interest in pushing this any further. Still, it would be a shame if the 
work I've done so far go to waste, but I'd really prefer it if someone 
else could pick up this patch and finish it.


/Magnus



Thanks, Thomas




On Wed, Mar 28, 2018 at 12:15 PM, Magnus Ihse Bursie 
> 
wrote:



On 2018-03-27 18:24, Thomas Stüfe wrote:

Hi Magnus,

just a cursory look, will look in greater detail tomorrow.

This is good, thanks for doing this.

As I wrote offlist, please remove the painfully wrong AIX
"workarounds". I do not know why we did this - the original code
is quite old - my assumption is that dlsym(RTLD_NEXT) was not
working as expected on older AIX versions. Well, it should work
now according to IBMs manpages. Will test later.

Ok.


__thread is not Posix. I would prefer pthread_get/set_specific
instead, which is more portable.


I have surrounded this code with #ifdef MACOSX now.

Here is an updated webrev. It includes the changes requested by
you and David:

* No more AIX workarounds
* Solaris use pthreads
* The "reentry" code is #ifdef MACOSX.

I also assumed that NSIG is available and working on Solaris. I'll
let David decide if he is happy with that. The alternative is to
go back to the Solaris-specific code that allocates sact on the heap.

Webrev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.05


Once again, here is also a webrev that shows the difference
between the original files and the new, unified file. Even if it's
hard to read, it shows what the effects will be for each platform.

Webrev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.04/


/Magnus



Thanks!

Thomas





On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie
> wrote:

When I was about to update jsig.c, I noticed that the four
copies for aix, linux, macosx and solaris were basically the
same, with only small differences. Some of the differences
were not even well motivated, but were likely the result of
this code duplication causing the code to diverge.

A better solution is to unify them all into a single unix
version, with the few platform-specific changes handled on a
per-platform basis.

I've made the following notable changes:

* I have removed the version check for Solaris. All other
platforms seem to do fine without it, and 

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[Thomas Stüfe]

Hi Magnus,

I had a closer look at the changes, especially at jsig.c. It all comes
slowly back. The AIX version has been almost comically wrong.

About NSIG, I remember that we had coding in our port which needed to know
the max number of signals, and this was surprisingly hard since on some
platforms. Sometimes NSIG does not contain the real time signals. Or it did
not even exist. I think we ended up hardcoding an own max signal limit .

So wherever you access sact with a signal number as index, I'd like to see
a bounds check. Or at least an assert - since this is plain C, a C-assert
would do for me (see
http://pubs.opengroup.org/onlinepubs/009695399/functions/assert.html). This
also would guard against user parameter error.

I also wonder whether this coding could not be simplified quite a bit by
not calling the OS versions of signal() at all but instead doing all signal
work via OS sigaction: after all, signal() can be implemented in terms of
sigaction() (with the flag SA_SIGINFO cleared), and so can sigset(). This
would remove the necessity of reentry-handling for macos, and also remove
the need for save_signal_handler().

I will test this version on AIX tomorrow. After that, I'll have some
vacation, but maybe someone else from my team will take over.

I like this work, this is a good simplification.

Thanks, Thomas




On Wed, Mar 28, 2018 at 12:15 PM, Magnus Ihse Bursie <
magnus.ihse.bur...@oracle.com> wrote:

>
> On 2018-03-27 18:24, Thomas Stüfe wrote:
>
> Hi Magnus,
>
> just a cursory look, will look in greater detail tomorrow.
>
> This is good, thanks for doing this.
>
> As I wrote offlist, please remove the painfully wrong AIX "workarounds". I
> do not know why we did this - the original code is quite old - my
> assumption is that dlsym(RTLD_NEXT) was not working as expected on older
> AIX versions. Well, it should work now according to IBMs manpages. Will
> test later.
>
> Ok.
>
>
> __thread is not Posix. I would prefer pthread_get/set_specific instead,
> which is more portable.
>
>
> I have surrounded this code with #ifdef MACOSX now.
>
> Here is an updated webrev. It includes the changes requested by you and
> David:
>
> * No more AIX workarounds
> * Solaris use pthreads
> * The "reentry" code is #ifdef MACOSX.
>
> I also assumed that NSIG is available and working on Solaris. I'll let
> David decide if he is happy with that. The alternative is to go back to the
> Solaris-specific code that allocates sact on the heap.
>
> Webrev: http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-
> libjsig/webrev.05
>
> Once again, here is also a webrev that shows the difference between the
> original files and the new, unified file. Even if it's hard to read, it
> shows what the effects will be for each platform.
>
> Webrev: http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-
> libjsig/webrev.04/
>
> /Magnus
>
>
> Thanks!
>
> Thomas
>
>
>
>
>
> On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie <
> magnus.ihse.bur...@oracle.com> wrote:
>
>> When I was about to update jsig.c, I noticed that the four copies for
>> aix, linux, macosx and solaris were basically the same, with only small
>> differences. Some of the differences were not even well motivated, but were
>> likely the result of this code duplication causing the code to diverge.
>>
>> A better solution is to unify them all into a single unix version, with
>> the few platform-specific changes handled on a per-platform basis.
>>
>> I've made the following notable changes:
>>
>> * I have removed the version check for Solaris. All other platforms seem
>> to do fine without it, and in general, we don't mistrust other JDK
>> libraries. An alternative is to add this version check to all other
>> platforms instead. If you think this is the correct course of action, let
>> me know and I'll fix it.
>>
>> * Solaris used to have a dynamically allocated sact, instead of a
>> statically allocated array as all other platforms have. It's not likely to
>> be large, and the size is known at compile time, so there seems to be no
>> good reason for this.
>>
>> * Linux and macosx used a uint32_t/uint64_t instead of sigset_t for
>> jvmsigs, as aix and solaris do. This is a less robust solution, and the
>> added checks show that it has failed in the past. Now all platforms use
>> sigset_t/sigismember().
>>
>> Also worth noting:
>>
>> * Solaris is not using pthreads, but it's own thread library, which
>> accounts for most of the #ifdef SOLARIS.
>>
>> * In general, if an implementation was needed on one platform, but has no
>> effect or is harmless on others, I've kept it on all platforms instead of
>> sprinkling the code with #ifdefs.
>>
>> To facilitate code review, here is a specially crafted webrev that shows
>> the differences compared to each of the individual, original per-OS
>> versions of jsig.c:
>> http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.01
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8200298
>> WebRev: 

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[Magnus Ihse Bursie]

On 2018-03-27 18:24, Thomas Stüfe wrote:

Hi Magnus,

just a cursory look, will look in greater detail tomorrow.

This is good, thanks for doing this.

As I wrote offlist, please remove the painfully wrong AIX 
"workarounds". I do not know why we did this - the original code is 
quite old - my assumption is that dlsym(RTLD_NEXT) was not working as 
expected on older AIX versions. Well, it should work now according to 
IBMs manpages. Will test later.

Ok.


__thread is not Posix. I would prefer pthread_get/set_specific 
instead, which is more portable.


I have surrounded this code with #ifdef MACOSX now.

Here is an updated webrev. It includes the changes requested by you and 
David:


* No more AIX workarounds
* Solaris use pthreads
* The "reentry" code is #ifdef MACOSX.

I also assumed that NSIG is available and working on Solaris. I'll let 
David decide if he is happy with that. The alternative is to go back to 
the Solaris-specific code that allocates sact on the heap.


Webrev: http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.05

Once again, here is also a webrev that shows the difference between the 
original files and the new, unified file. Even if it's hard to read, it 
shows what the effects will be for each platform.


Webrev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.04/


/Magnus



Thanks!

Thomas





On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie 
> 
wrote:


When I was about to update jsig.c, I noticed that the four copies
for aix, linux, macosx and solaris were basically the same, with
only small differences. Some of the differences were not even well
motivated, but were likely the result of this code duplication
causing the code to diverge.

A better solution is to unify them all into a single unix version,
with the few platform-specific changes handled on a per-platform
basis.

I've made the following notable changes:

* I have removed the version check for Solaris. All other
platforms seem to do fine without it, and in general, we don't
mistrust other JDK libraries. An alternative is to add this
version check to all other platforms instead. If you think this is
the correct course of action, let me know and I'll fix it.

* Solaris used to have a dynamically allocated sact, instead of a
statically allocated array as all other platforms have. It's not
likely to be large, and the size is known at compile time, so
there seems to be no good reason for this.

* Linux and macosx used a uint32_t/uint64_t instead of sigset_t
for jvmsigs, as aix and solaris do. This is a less robust
solution, and the added checks show that it has failed in the
past. Now all platforms use sigset_t/sigismember().

Also worth noting:

* Solaris is not using pthreads, but it's own thread library,
which accounts for most of the #ifdef SOLARIS.

* In general, if an implementation was needed on one platform, but
has no effect or is harmless on others, I've kept it on all
platforms instead of sprinkling the code with #ifdefs.

To facilitate code review, here is a specially crafted webrev that
shows the differences compared to each of the individual, original
per-OS versions of jsig.c:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.01


Bug: https://bugs.openjdk.java.net/browse/JDK-8200298

WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.03


/Magnus

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[Magnus Ihse Bursie]

On 2018-03-28 01:03, David Holmes wrote:

Hi Magnus,

I appreciate what you are trying to do here but there are reasons this 
kind of cleanup keeps getting deferred :) - it's not trivial and deals 
with really old code that isn't always clear. You may have bitten off 
more than you want to chew here. (And I'm strapped for time to work 
through this with you - sorry.)


*sighs* Yeah, I'm starting to realize that. I thought it was more or 
less trivial, and I didn't want to add new duplicated code. Let's see if 
I can give reasonable answers to your questions here, but if that's not 
enough, I'll drop this.


On the positive side you can use the pthread functions on Solaris so 
no need to special case for that.

Ok!


However there are still a number of issues:

  52 #ifndef NSIG
  53   #define NSIG SIGRTMAX
  54 #endif

SIGRTMAX is Solaris only, but even on Solaris NSIG might be defined 
(it's conditional in sys/signal.h but I don't know when the conditions 
are enabled).


 56 static struct sigaction sact[NSIG];

SIGRTMAX is a macro not a constant so you can't declare a static array 
using it. You would need to malloc the array as done on Solaris.

Ah, I see. That explains the special code in Solaris.


 58 static __thread bool reentry = false; /* prevent reentry deadlock 
(per-thread) */


I didn't realize this was in the OSX version as we don't 
unconditionally use compiler-based thread locals. I would make this 
field and its use OSX specific

Ok.


148 #ifdef SOLARIS
 149 sact[sig].sa_flags = SA_NODEFER;
 150 if (sig != SIGILL && sig != SIGTRAP && sig != SIGPWR) {
 151   sact[sig].sa_flags |= SA_RESETHAND;
 152 }
 153 #else

I'd have to do some research to see why this is needed so best to keep 
it.


178 #ifdef SOLARIS
 179 if (is_sigset && sigblocked) {
 180   /* We won't honor the SIG_HOLD request to change the signal 
mask */

 181   oldhandler = SIG_HOLD;
 182 }
 183 #endif

Ditto.

More below ...

On 27/03/2018 7:42 PM, Magnus Ihse Bursie wrote:
When I was about to update jsig.c, I noticed that the four copies for 
aix, linux, macosx and solaris were basically the same, with only 
small differences. Some of the differences were not even well 
motivated, but were likely the result of this code duplication 
causing the code to diverge.


A better solution is to unify them all into a single unix version, 
with the few platform-specific changes handled on a per-platform basis.


I've made the following notable changes:

* I have removed the version check for Solaris. All other platforms 
seem to do fine without it, and in general, we don't mistrust other 
JDK libraries. An alternative is to add this version check to all 
other platforms instead. If you think this is the correct course of 
action, let me know and I'll fix it.


That's fine. I actually thought this had gone. Or it may be we have a 
bug to clean this up. Which reminds me - there may be a number of 
existing bugs that get handled by the changes you're making. But there 
may still be open bugs that need fixing.
I searched JBS for "jsig" and "libjsig". Then I screened all hits, and 
filtered out all bugs where this was just mentioned in passing. In the 
end, I found only this which has bearing to the current changes:
https://bugs.openjdk.java.net/browse/JDK-8170639 "[Linux] jsig is 
limited to a maximum of 64 signals"


I also found a couple of bugs related to build (JDK-8164790, 
JDK-8146563) and a discussion about using __thread (JDK-8137018).


JDK-8170639 will be fixed by the propsed patch.



* Solaris used to have a dynamically allocated sact, instead of a 
statically allocated array as all other platforms have. It's not 
likely to be large, and the size is known at compile time, so there 
seems to be no good reason for this.


See above. If you didn't get a compile-time error here then it's 
likely NSIG was already defined.


> cat sig.c
#include 

static int sigs[SIGRTMAX];

int main(int argc, char* argv[]) {
  sigs[0] = 1;
}

 > CC -c sig.c
"sig.c", line 3: Error: An integer constant expression is required 
within the array subscript operator.

1 Error(s) detected.
Yes, you are right, NSIG is defined when I compile on Solaris. I looked 
into the conditionals, but I could not either easily determine when they 
are true. But maybe that's not really necessary to do? Most of the 
system provided functions are guarded by feature tests, but we don't 
normally verify that we fulfill the proper conditions, but just use 
them, and if it works, we're happy.


(That said, we should probably be more prudent on how we use 
feature/standard defines...)




* Linux and macosx used a uint32_t/uint64_t instead of sigset_t for 
jvmsigs, as aix and solaris do. This is a less robust solution, and 
the added checks show that it has failed in the past. Now all 
platforms use sigset_t/sigismember().


Probably historical (macosx copied linux) and a good change to make. 
That said I have to wonder why we didn't make it 

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[David Holmes]

Hi Magnus,

I appreciate what you are trying to do here but there are reasons this 
kind of cleanup keeps getting deferred :) - it's not trivial and deals 
with really old code that isn't always clear. You may have bitten off 
more than you want to chew here. (And I'm strapped for time to work 
through this with you - sorry.)


On the positive side you can use the pthread functions on Solaris so no 
need to special case for that.


However there are still a number of issues:

  52 #ifndef NSIG
  53   #define NSIG SIGRTMAX
  54 #endif

SIGRTMAX is Solaris only, but even on Solaris NSIG might be defined 
(it's conditional in sys/signal.h but I don't know when the conditions 
are enabled).


 56 static struct sigaction sact[NSIG];

SIGRTMAX is a macro not a constant so you can't declare a static array 
using it. You would need to malloc the array as done on Solaris.


 58 static __thread bool reentry = false; /* prevent reentry deadlock 
(per-thread) */


I didn't realize this was in the OSX version as we don't unconditionally 
use compiler-based thread locals. I would make this field and its use 
OSX specific


148 #ifdef SOLARIS
 149 sact[sig].sa_flags = SA_NODEFER;
 150 if (sig != SIGILL && sig != SIGTRAP && sig != SIGPWR) {
 151   sact[sig].sa_flags |= SA_RESETHAND;
 152 }
 153 #else

I'd have to do some research to see why this is needed so best to keep it.

178 #ifdef SOLARIS
 179 if (is_sigset && sigblocked) {
 180   /* We won't honor the SIG_HOLD request to change the signal 
mask */

 181   oldhandler = SIG_HOLD;
 182 }
 183 #endif

Ditto.

More below ...

On 27/03/2018 7:42 PM, Magnus Ihse Bursie wrote:
When I was about to update jsig.c, I noticed that the four copies for 
aix, linux, macosx and solaris were basically the same, with only small 
differences. Some of the differences were not even well motivated, but 
were likely the result of this code duplication causing the code to 
diverge.


A better solution is to unify them all into a single unix version, with 
the few platform-specific changes handled on a per-platform basis.


I've made the following notable changes:

* I have removed the version check for Solaris. All other platforms seem 
to do fine without it, and in general, we don't mistrust other JDK 
libraries. An alternative is to add this version check to all other 
platforms instead. If you think this is the correct course of action, 
let me know and I'll fix it.


That's fine. I actually thought this had gone. Or it may be we have a 
bug to clean this up. Which reminds me - there may be a number of 
existing bugs that get handled by the changes you're making. But there 
may still be open bugs that need fixing.


* Solaris used to have a dynamically allocated sact, instead of a 
statically allocated array as all other platforms have. It's not likely 
to be large, and the size is known at compile time, so there seems to be 
no good reason for this.


See above. If you didn't get a compile-time error here then it's likely 
NSIG was already defined.


> cat sig.c
#include 

static int sigs[SIGRTMAX];

int main(int argc, char* argv[]) {
  sigs[0] = 1;
}

 > CC -c sig.c
"sig.c", line 3: Error: An integer constant expression is required 
within the array subscript operator.

1 Error(s) detected.


* Linux and macosx used a uint32_t/uint64_t instead of sigset_t for 
jvmsigs, as aix and solaris do. This is a less robust solution, and the 
added checks show that it has failed in the past. Now all platforms use 
sigset_t/sigismember().


Probably historical (macosx copied linux) and a good change to make. 
That said I have to wonder why we didn't make it when we put in the 
guards on the size.



Also worth noting:

* Solaris is not using pthreads, but it's own thread library, which 
accounts for most of the #ifdef SOLARIS.


Not needed as above.

* In general, if an implementation was needed on one platform, but has 
no effect or is harmless on others, I've kept it on all platforms 
instead of sprinkling the code with #ifdefs.


That depends on your own knowledge/assumptions about the behaviour. I'd 
want to check each such case to verify that is the case. As above the 
use of __thread with "reentry" is not something to use unconditionally 
without closer examination.


To facilitate code review, here is a specially crafted webrev that shows 
the differences compared to each of the individual, original per-OS 
versions of jsig.c:

http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.01


Appreciated, but still very hard to see when a feature from one OS is 
now being used by all.


I'd be less concerned if we had extensive testing here but I don't think 
we do, so any issues only turn up when something that's been working 
since Java 1.4 suddenly stops working. What testing was done?


Thanks,
David
-


Bug: https://bugs.openjdk.java.net/browse/JDK-8200298
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.03


/Magnus

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[Thomas Stüfe]

Hi Magnus,

just a cursory look, will look in greater detail tomorrow.

This is good, thanks for doing this.

As I wrote offlist, please remove the painfully wrong AIX "workarounds". I
do not know why we did this - the original code is quite old - my
assumption is that dlsym(RTLD_NEXT) was not working as expected on older
AIX versions. Well, it should work now according to IBMs manpages. Will
test later.

__thread is not Posix. I would prefer pthread_get/set_specific instead,
which is more portable.

Thanks!

Thomas





On Tue, Mar 27, 2018 at 11:42 AM, Magnus Ihse Bursie <
magnus.ihse.bur...@oracle.com> wrote:

> When I was about to update jsig.c, I noticed that the four copies for aix,
> linux, macosx and solaris were basically the same, with only small
> differences. Some of the differences were not even well motivated, but were
> likely the result of this code duplication causing the code to diverge.
>
> A better solution is to unify them all into a single unix version, with
> the few platform-specific changes handled on a per-platform basis.
>
> I've made the following notable changes:
>
> * I have removed the version check for Solaris. All other platforms seem
> to do fine without it, and in general, we don't mistrust other JDK
> libraries. An alternative is to add this version check to all other
> platforms instead. If you think this is the correct course of action, let
> me know and I'll fix it.
>
> * Solaris used to have a dynamically allocated sact, instead of a
> statically allocated array as all other platforms have. It's not likely to
> be large, and the size is known at compile time, so there seems to be no
> good reason for this.
>
> * Linux and macosx used a uint32_t/uint64_t instead of sigset_t for
> jvmsigs, as aix and solaris do. This is a less robust solution, and the
> added checks show that it has failed in the past. Now all platforms use
> sigset_t/sigismember().
>
> Also worth noting:
>
> * Solaris is not using pthreads, but it's own thread library, which
> accounts for most of the #ifdef SOLARIS.
>
> * In general, if an implementation was needed on one platform, but has no
> effect or is harmless on others, I've kept it on all platforms instead of
> sprinkling the code with #ifdefs.
>
> To facilitate code review, here is a specially crafted webrev that shows
> the differences compared to each of the individual, original per-OS
> versions of jsig.c:
> http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.01
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8200298
> WebRev: http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/
> webrev.03
>
> /Magnus
>

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[Erik Joelsson]

Build change looks good. I will let someone more informed look at the 
code changes.


/Erik


On 2018-03-27 02:42, Magnus Ihse Bursie wrote:
When I was about to update jsig.c, I noticed that the four copies for 
aix, linux, macosx and solaris were basically the same, with only 
small differences. Some of the differences were not even well 
motivated, but were likely the result of this code duplication causing 
the code to diverge.


A better solution is to unify them all into a single unix version, 
with the few platform-specific changes handled on a per-platform basis.


I've made the following notable changes:

* I have removed the version check for Solaris. All other platforms 
seem to do fine without it, and in general, we don't mistrust other 
JDK libraries. An alternative is to add this version check to all 
other platforms instead. If you think this is the correct course of 
action, let me know and I'll fix it.


* Solaris used to have a dynamically allocated sact, instead of a 
statically allocated array as all other platforms have. It's not 
likely to be large, and the size is known at compile time, so there 
seems to be no good reason for this.


* Linux and macosx used a uint32_t/uint64_t instead of sigset_t for 
jvmsigs, as aix and solaris do. This is a less robust solution, and 
the added checks show that it has failed in the past. Now all 
platforms use sigset_t/sigismember().


Also worth noting:

* Solaris is not using pthreads, but it's own thread library, which 
accounts for most of the #ifdef SOLARIS.


* In general, if an implementation was needed on one platform, but has 
no effect or is harmless on others, I've kept it on all platforms 
instead of sprinkling the code with #ifdefs.


To facilitate code review, here is a specially crafted webrev that 
shows the differences compared to each of the individual, original 
per-OS versions of jsig.c:

http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.01

Bug: https://bugs.openjdk.java.net/browse/JDK-8200298
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.03


/Magnus

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[David Holmes]

Hi Magnus,

I will take a look at this tomorrow. There are some open bugs on jsig 
regarding the number of signals it can handle on different platforms 
IIRC. Also the Solaris differences may not be needed as the pthread 
functions can be used without any concern in most cases.


Thanks,
David

On 27/03/2018 7:42 PM, Magnus Ihse Bursie wrote:
When I was about to update jsig.c, I noticed that the four copies for 
aix, linux, macosx and solaris were basically the same, with only small 
differences. Some of the differences were not even well motivated, but 
were likely the result of this code duplication causing the code to 
diverge.


A better solution is to unify them all into a single unix version, with 
the few platform-specific changes handled on a per-platform basis.


I've made the following notable changes:

* I have removed the version check for Solaris. All other platforms seem 
to do fine without it, and in general, we don't mistrust other JDK 
libraries. An alternative is to add this version check to all other 
platforms instead. If you think this is the correct course of action, 
let me know and I'll fix it.


* Solaris used to have a dynamically allocated sact, instead of a 
statically allocated array as all other platforms have. It's not likely 
to be large, and the size is known at compile time, so there seems to be 
no good reason for this.


* Linux and macosx used a uint32_t/uint64_t instead of sigset_t for 
jvmsigs, as aix and solaris do. This is a less robust solution, and the 
added checks show that it has failed in the past. Now all platforms use 
sigset_t/sigismember().


Also worth noting:

* Solaris is not using pthreads, but it's own thread library, which 
accounts for most of the #ifdef SOLARIS.


* In general, if an implementation was needed on one platform, but has 
no effect or is harmless on others, I've kept it on all platforms 
instead of sprinkling the code with #ifdefs.


To facilitate code review, here is a specially crafted webrev that shows 
the differences compared to each of the individual, original per-OS 
versions of jsig.c:

http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.01

Bug: https://bugs.openjdk.java.net/browse/JDK-8200298
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8200298-unify-libjsig/webrev.03


/Magnus

Re: RFR (M): JDK-8200298 Unify all unix versions of libjsig/jsig.c

[John Paul Adrian Glaubitz]

On 03/27/2018 06:42 PM, Magnus Ihse Bursie wrote:
> * Linux and macosx used a uint32_t/uint64_t instead of sigset_t for jvmsigs, 
> as aix and solaris do. This is a less robust solution, and the added checks 
> show
> that it has failed in the past. Now all platforms use sigset_t/sigismember().

Ah, this is great. I guess Debian can then drop this patch we have for the MIPS
targets [1, 2]. This patch is necessary because MIPS has 128 signals and 
uint64_t
is not enough for that.

I will test whether your change makes this particular Debian patch obsolete.

Adrian

> [1] 
> https://sources.debian.org/src/openjdk-11/11%7E5-1/debian/patches/mips-sigset.diff/
> [2] 
> https://sources.debian.org/src/openjdk-9/9.0.4+12-3/debian/patches/mips-sigset.diff/

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913