Closed babsingh closed 5 years ago
fyi OMR committers: @charliegracie @0xdaryl @youngar, OpenJ9 committers: @DanHeidinga @pshipton.
if we're breaking the public API, is this also an appropriate time to change the name of the J9
structures?
is this also an appropriate time to change the name of the J9 structures?
J9SignalHandlerRecord
, J9CurrentSignal
and J9.*AsyncHandlerRecord
are internally used in the omrsig
functions. Changing J9
to OMR
for such internal structures shouldn't impact OMR consumers. So, the name change is doable for such structures, and reasonable as part of this task. The name change also applies to the comments and local variables (j9Info
) in the scope of the omrsig
functions.
omrsig
functions also use global macros such as J9ZOS390
, J9OS_I5
, J9OS_I5_V5R4
, J9THREAD_PROC
, J9THREAD_PRIORITY_MAX
, J9THREAD_CATEGORY_SYSTEM_THREAD
etc. Changing names for such global macros will be difficult since it will greatly impact other OMR components and the consumers of OMR; so, it shouldn't be performed as part of this task.
Can you explain where the need to use 64-bit data types comes from? I don't see any constants mentioned above that need more than 32 bits.
IIUC there are now 39 signals, and each needs it's own bit in the various flags.
Grep for // will be changed to uint64_t
in the main description; it will point to the fields which need to be converted from uint32_t
to uint64_t
. These fields store OMRPORT_SIG_FLAG_*
, and can only support 32 flags with uint32_t
. Currently, 25 signal flags are used. We need to support 14 new async signals, which makes the total flags required to be 39.
Why does each signal need a separate bit?
Why does each signal need a separate bit?
The current implementation relies upon bit masking for identifying supported signal flags, mapping signal handlers with multiple signals, and distinguishing between sync and async signals. So, each signal flag needs a separate bit. Also, functions such as omrsig_can_protect
, omrsig_protect
, omrsig_set_async_signal_handler
and others accept a bit-mask of port library signal flags. This allows an operation to be performed on a group of signals.
Approach 1: change uint32_t
to uint64_t
.
[Approach 2; suggested by @keithc-ca] Instead of changing the data-structure fields to uint64_t
and use bit-masking, we can employ alternate techniques such as
This won't require the data-structure fields to be changed to uint64_t
. We will be able to support 2^32
signals instead of 64 signals with uint64_t
.
But, more memory will be required to store the new data-structures; more processing time will be required at some places; and more effort will be needed to modify the implementation.
Also, omrsig_*
functions will not be able accept a bit-mask (representing a group of signal flags); omrsig
functions will need to be called separately for each signal or a list of signal flags will need to be supplied.
Port library signal flags also represent operations such as *_FLAG_MAY_RETURN
and *_FLAG_MAY_CONTINUE_EXECUTION
. Functions such as omrsig_protect
accept operation flags masked with signal flags. Without bit-masking, the type signature of functions such as omrsig_protect
would need to be changed to distinguish between signals and operations.
Approach 2 will also break the public API since we will need an input alternative for a bit-mask of signal flags.
Approach 2 is good if the number of signals on the operating systems will increase more than 64 in the future.
What are everyone's thoughts? Approach 1 or 2?
I prefer approach 1 with some careful rollout to enable adopters to adjust to the new signatures.
Possible rollout approach that allows reclaiming the existing "good" api names:
_legacy
or something similar._legacy
apis. (1-2 weeks?)uint64_t
and update the _legacy
versions to forward to the 64bit versions._legacy
apis.First, there are several unused bits in the current API, e.g.:
define OMRPORT_SIG_FLAG_SIGRESERVED8 0x100
which can be repurposed, so we are only actually using 21 bits (by my count), not 25.
Signals are logically segregated as synchronous or asynchronous, so sig_protect
for example, doesn't need to be concerned about asynchronous signals and sig_set_async_signal_handler
doesn't have to consider synchronous signals.
I think there are choices available for the values of OMRPORT_SIG_FLAG_*
macros (although the _FLAG_
portion of those names is misleading) that reflect that partitioning so 32-bits would still be sufficient.
Another point to consider: DDR was not designed to accommodate field type changes. One consequence is that the code generated at build time for J9CurrentSignalPointer
cannot do the right thing for portLibSignalType
which in old core files will be uint32_t
and in newer core files would be uint64_t
. The same will be true of every data structure affected by the enlargement of fields holding signal masks. https://github.com/ibmruntimes/openj9-openjdk-jdk8/pull/117 (and similar changes for other versions) will improve the situation, but not for IBM builds which continue to use a legacy tool to extract type information.
First, there are several unused bits in the current API
In the past, I tried re-purposing the *_SIGRESERVED*
bits but it caused failures. Not sure if there is indirect/hidden use of the *RESERVED*
bits. I will need to verify if those bits are (really) unreserved.
Signals are logically segregated as synchronous or asynchronous, so sig_protect for example, doesn't need to be concerned about asynchronous signals and sig_set_async_signal_handler doesn't have to consider synchronous signals.
There are functions which can accept/return both async and sync signals such as omrsig_can_protect
, omrsig_map_os_signal_to_portlib_signal
, omrsig_map_portlib_signal_to_os_signal
, omrsig_register_os_handler
, omrsig_is_signal_ignored
, etc. These functions will become non-functional if there is overlap between sync and async signal flags.
Also, omrsig_protect
and omrsig_set_async_signal_handler
use common functions (internally) such as registerMasterHandlers
to register signal handlers for both sync and async signals.
So, segregating sync and async signals, with overlapping signal flags to preserve the uint32_t
fields, will completely change the core functionality of the public API and internal functions.
Side note: Distinction between async and sync signals is implementation based. For example, Hotspot considers SIGBUS
as an async signal on some platforms whereas OMR considers SIGBUS
as a sync signal. The distinction between async and sync signals may change depending upon future requirements.
I think there are choices available for the values of OMRPORT_SIGFLAG* macros (although the FLAG portion of those names is misleading) that reflect that partitioning so 32-bits would still be sufficient.
Assuming that the choices available refers to these (non-signal
) flags:
[1] #define OMRPORT_SIG_FLAG_MAY_RETURN 1
[2] #define OMRPORT_SIG_FLAG_MAY_CONTINUE_EXECUTION 2
[3] #define OMRPORT_SIG_FLAG_DOES_NOT_MAP_TO_POSIX 0x200000
[1] and [2] are used in omrsig_protect
and omrsig_can_protect
, and these flags don't need to be stored in a data-structure since they aren't referenced later-on. If [1] and [2] are segregated from the signal flags, then the type signature of omrsig_protect
and omrsig_can_protect
would need to be modified to accept signal
flags and non-signal
flags. This is reasonable/doable.
[3] is only used in j9vm_le_condition_handler
(zOS specific), and I think we can get away without storing it. Also, segregating [3] from the signal flags shouldn't impact the type signature of any function.
Let's assume that the four *_SIGRESERVED*
bits can be reused and the three non-signal
flags ([1], [2], [3]) are segregated from the signal
flags. then, the number of total-used signal flags will be 18 (25 - 4 - 3), and the total-free signal flags will be 14 (32 - 18). This should allow us to support the 14 new signals without changing the fields to uint64_t
. This approach is only possible if the *_SIGRESERVED*
bits can be re-purposed (pending verification).
The above approach will still break the public API for omrsig_protect
and omrsig_can_protect
functions, and no flags will remain to support other signals in the future. Since we are going to break the public API, shouldn't we include the change from uint32_t
to uint64_t
? Otherwise, we will need to break the public API again if we need to support other signals in the future. For reference, AIX has 42 signals, which is the largest number of signals among all the operating systems supported by OMR/OpenJ9.
DDR was not designed to accommodate field type changes.
I do see an upcoming fix in OpenJ9 for dynamic creation of DDR pointer classes after the core file examination (https://github.com/eclipse/openj9/pull/3099), which should handle the uint32_t
to uint64_t
change.
In case, we are unable to support the 14 new signals with a uint32_t
data-type. Will the DDR issue for IBM builds block us from making the uint32_t
to uint64_t
change? Can an exception be made depending upon the importance of the impacted structures (J9CurrentSignal
, J9UnixAsyncHandlerRecord
, OMRUnixSignalInfo
)?
[Update] Created https://github.com/eclipse/omr/pull/3737:
_SIGRESERVED
bits to support new signals.omrsig_set_async_signal_handler
allow multiple signals to be specified in a uint32_t
bit-mask. Using the 31st
bit is equivalent to hitting the bound for uint32_t
(2^32); overflow will happen if other bits are set along with the 31st
bit in a uint32_t
bit-mask. So, I couldn't use the 31st
bit (0x80000000) in OMRPORT_SIG_FLAG_SIG*
flags. Crashes were seen.[Update] Currently, I am pursuing the changes which don't impact omrsig
's public API:
SIGIO
: https://github.com/eclipse/omr/pull/3762. uint32_t
bit-mask: there is an overflow issue which prevents the 31st bit to be used.j9
from omrsig
codebase (internal data structures, comments and functions).@charliegracie @youngar I need approval from the OMR committers on which approach to pursue. To summarize, there are two approaches:
omrsig_protect
and omrsig_can_protect
. We will be able to add support for the 14 signals listed in the issue's description. But, there will be no room left to support any other signals since all the 32 bits in the uint32_t
bit-mask will be utilized. OMR supports AIX, which has 42 signals; we may be asked to support these signals in the future; if this happens, we will have to transition from uint32_t
to uint64_t
or another approach to support more than 32 signals.uint64_t
bit-mask to store signal flags. This will support 64 signals, which is sufficient to support all the signals on AIX. This approach will impact the data structures and functions listed in the issue's description. This approach will break the public API once; Approach 1 may break the public API twice if more signals need to be supported in the future. With this approach, there are concerns about DDR since DDR wasn't designed to accommodate field type changes; a fix may be upcoming.Which approach should be pursued: Approach 1 or Approach 2?
If I'm reading this correctly, both approaches will result in breaking API changes.
Looking at [1], I see signals numbered to 39 though there are some gaps in the numbering scheme.
As I said above, I still prefer the change to uint64_t
plan with a careful rollout plan.
[1] https://www.ibm.com/support/knowledgecenter/SSLTBW_2.2.0/com.ibm.zos.v2r2.bpxb100/ysigh.htm
[1] above lists the signals on zOS. The 42 signals on AIX are listed below:
some gaps in the numbering scheme.
In [1], there are no gaps in the numbering scheme; the signal numbers are just presented in an out of order manner.
@charliegracie can you approve one of the two approaches mentioned here: https://github.com/eclipse/omr/issues/3713#issuecomment-484672198?
I am not sure which approach I prefer. I will think about it more but I do not see a great path forward.
This set of definitions partitions the signal 'flags' as I suggested, including SIGTTIN
, SIGTTOU
, and SIGSYS
that have not already been added. They would replace the current OMRPORT_SIG_FLAG_*
macro definitions in omrport.h
.
As noted in the comments, this scheme allows for the addition of up to 19 more synchronous signals and up to 5 more asynchronous signals.
The bottom two bits distinguish synchronous and asynchronous signals. No API function permits the mixing of synchronous and asynchronous signals so all current client code patterns continue to be legal and illegal uses can still be detected (but more simply).
Internal implementation data structures may require use of a 64-bit word (or an extra 32-bit word), but no client code should need to change at all.
/* signal classification bits */
#define OMRPORT_SIG_FLAG_IS_ASYNC ((uint32_t)0x01)
#define OMRPORT_SIG_FLAG_IS_SYNC ((uint32_t)0x02)
#define OMRPORT_SIG_FLAG_MAY_RETURN ((uint32_t)0x04)
#define OMRPORT_SIG_FLAG_MAY_CONTINUE_EXECUTION ((uint32_t)0x08)
/*
* all signal codes include exactly one of OMRPORT_SIG_FLAG_IS_SYNC or OMRPORT_SIG_FLAG_IS_ASYNC
* plus a multiple of this
*/
#define OMRPORT_SIG_SMALLEST_SIGNAL_FLAG ((uint32_t)0x10)
/* synchronous signals */
#define OMRPORT_SIG_FLAG_SIGSEGV ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 0) | OMRPORT_SIG_FLAG_IS_SYNC)
#define OMRPORT_SIG_FLAG_SIGBUS ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 1) | OMRPORT_SIG_FLAG_IS_SYNC)
#define OMRPORT_SIG_FLAG_SIGILL ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 2) | OMRPORT_SIG_FLAG_IS_SYNC)
#define OMRPORT_SIG_FLAG_SIGFPE ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 3) | OMRPORT_SIG_FLAG_IS_SYNC)
#define OMRPORT_SIG_FLAG_SIGTRAP ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 4) | OMRPORT_SIG_FLAG_IS_SYNC)
#define OMRPORT_SIG_FLAG_SIGABEND ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 5) | OMRPORT_SIG_FLAG_IS_SYNC)
#define OMRPORT_SIG_FLAG_SIGFPE_DIV_BY_ZERO ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 6) | OMRPORT_SIG_FLAG_SIGFPE)
#define OMRPORT_SIG_FLAG_SIGFPE_INT_DIV_BY_ZERO ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 7) | OMRPORT_SIG_FLAG_SIGFPE)
#define OMRPORT_SIG_FLAG_SIGFPE_INT_OVERFLOW ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 8) | OMRPORT_SIG_FLAG_SIGFPE)
/* asynchronous signals */
#define OMRPORT_SIG_FLAG_SIGALRM ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 0) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGQUIT ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 1) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGABRT ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 2) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGTERM ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 3) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGRECONFIG ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 4) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGINT ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 5) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGXFSZ ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 6) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGCHLD ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 7) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGTSTP ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 8) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGIO ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 9) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGHUP ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 10) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGCONT ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 11) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGWINCH ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 12) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGUSR1 ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 13) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGUSR2 ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 14) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGURG ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 15) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGXCPU ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 16) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGVTALRM ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 17) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGPROF ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 18) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGPIPE ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 19) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGTTIN ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 20) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGTTOU ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 21) | OMRPORT_SIG_FLAG_IS_ASYNC)
#define OMRPORT_SIG_FLAG_SIGSYS ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 22) | OMRPORT_SIG_FLAG_IS_ASYNC)
/*
* The API can accommodate up to 19 more synchronous signals and up to 5 more asynchronous signals:
*
* #define OMRPORT_SIG_FLAG_MAX_SYNC ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 27) | OMRPORT_SIG_FLAG_IS_SYNC)
* #define OMRPORT_SIG_FLAG_MAX_ASYNC ((OMRPORT_SIG_SMALLEST_SIGNAL_FLAG << 27) | OMRPORT_SIG_FLAG_IS_ASYNC)
*/
#if defined(J9ZOS390)
#define OMRPORT_SIG_FLAG_SIGALLSYNC (OMRPORT_SIG_FLAG_SIGSEGV | OMRPORT_SIG_FLAG_SIGBUS | OMRPORT_SIG_FLAG_SIGILL | OMRPORT_SIG_FLAG_SIGFPE | OMRPORT_SIG_FLAG_SIGTRAP | OMRPORT_SIG_FLAG_SIGABEND)
#else
#define OMRPORT_SIG_FLAG_SIGALLSYNC (OMRPORT_SIG_FLAG_SIGSEGV | OMRPORT_SIG_FLAG_SIGBUS | OMRPORT_SIG_FLAG_SIGILL | OMRPORT_SIG_FLAG_SIGFPE | OMRPORT_SIG_FLAG_SIGTRAP)
#endif /* defined(J9ZOS390) */
#define OMRPORT_SIG_FLAG_SIGALLASYNC \
( OMRPORT_SIG_FLAG_SIGALRM | OMRPORT_SIG_FLAG_SIGQUIT | OMRPORT_SIG_FLAG_SIGABRT | OMRPORT_SIG_FLAG_SIGTERM \
| OMRPORT_SIG_FLAG_SIGRECONFIG | OMRPORT_SIG_FLAG_SIGINT | OMRPORT_SIG_FLAG_SIGXFSZ | OMRPORT_SIG_FLAG_SIGCHLD \
| OMRPORT_SIG_FLAG_SIGTSTP | OMRPORT_SIG_FLAG_SIGIO | OMRPORT_SIG_FLAG_SIGHUP | OMRPORT_SIG_FLAG_SIGCONT \
| OMRPORT_SIG_FLAG_SIGWINCH | OMRPORT_SIG_FLAG_SIGUSR1 | OMRPORT_SIG_FLAG_SIGUSR2 | OMRPORT_SIG_FLAG_SIGURG \
| OMRPORT_SIG_FLAG_SIGXCPU | OMRPORT_SIG_FLAG_SIGVTALRM | OMRPORT_SIG_FLAG_SIGPROF | OMRPORT_SIG_FLAG_SIGPIPE \
| OMRPORT_SIG_FLAG_SIGSYS | OMRPORT_SIG_FLAG_SIGTTIN | OMRPORT_SIG_FLAG_SIGTTOU )
I feel like the proposal from @keithc-ca is the best solution. This means that we do not break API compatibility while still providing some room for growth. @babsingh I am approving the most recent approval from @keithc-ca as the direction I would like to see taken.
Thanks @keithc-ca; your approach looks good; I will pursue it.
Sub-task of https://github.com/eclipse/omr/issues/2332.
In the
omrsig
functions,uint32_t
data-type to store the port library signal flags. This restricts us to 32 signal flags. Currently, 25 signal flags are being used out of the 32 signal flags.omrsig
functions need to extend support to 14 new signals:SIGUSR1, SIGUSR2, SIGPIPE, SIGALRM, SIGCHLD, SIGTSTP, SIGTTIN, SIGTTOU, SIGURG, SIGXCPU, SIGVTALRM, SIGPROF, SIGIO and SIGSYS
.uint32_t
data-type.uint64_t
data-type is required to represent port library signal flags in theomrsig
functions in order to support the aforementioned POSIX asynchronous signals.omrsig
functions, which are part of the public API. But, there will be no impact on the core functionality of theomrsig
functions.The following functions (in the public API) will be impacted:
[S]
- function's type signature will be impacted.[I]
- function's internal implementation will be impacted.uint32_t
will be changed touint64_t
for the port library signal flags in the inputs and outputs of the above functions.The following data-structures will be impacted:
The source code is contained in the following files:
Note: Each platform and architecture may have its own implementation.
Current port library signal flags: