{"action":"created","issue":{"url":"https://api.github.com/repos/eclipse/omr/issues/2524","repository_url":"https://api.github.com/repos/eclipse/omr","labels_url":"https://api.github.com/repos/eclipse/omr/issues/2524/labels{/name}","comments_url":"https://api.github.com/repos/eclipse/omr/issues/2524/comments","events_url":"https://api.github.com/repos/eclipse/omr/issues/2524/events","html_url":"https://github.com/eclipse/omr/pull/2524","id":320059162,"number":2524,"title":"Extend OMR Signal API [Part 2]","user":{"login":"babsingh","id":8634038,"avatar_url":"https://avatars1.githubusercontent.com/u/8634038?v=4","gravatar_id":"","url":"https://api.github.com/users/babsingh","html_url":"https://github.com/babsingh","followers_url":"https://api.github.com/users/babsingh/followers","following_url":"https://api.github.com/users/babsingh/following{/other_user}","gists_url":"https://api.github.com/users/babsingh/gists{/gist_id}","starred_url":"https://api.github.com/users/babsingh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/babsingh/subscriptions","organizations_url":"https://api.github.com/users/babsingh/orgs","repos_url":"https://api.github.com/users/babsingh/repos","events_url":"https://api.github.com/users/babsingh/events{/privacy}","received_events_url":"https://api.github.com/users/babsingh/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2018-05-03T19:59:03Z","updated_at":"2018-05-04T16:56:18Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/eclipse/omr/pulls/2524","html_url":"https://github.com/eclipse/omr/pull/2524","diff_url":"https://github.com/eclipse/omr/pull/2524.diff","patch_url":"https://github.com/eclipse/omr/pull/2524.patch"},"body":"9 new commits have been added to support OMR Signal API extension/improvement.\r\n\r\n1) **Add new OMR Signal API: omrsig_register_os_handler**\r\n\r\n    omrsig_register_os_handler will register a handler with the OS. Stubs\r\n    have been added for omrsig_register_os_handler.\r\n\r\n    On Unix variants, we cache the original OS handler in oldActions while\r\n    registering the first handler with the OS. The original OS handler is\r\n    restored during shutdown.\r\n\r\n    If OMRSIG_SIGACTION is called outside OMR, then the original OS handler\r\n    will be overwritten without being cached in oldActions. Subsequently,\r\n    the oldActions may cache a user handler instead of the original OS\r\n    handler. During shutdown, we may not restore the original OS handler\r\n    incorrectly.\r\n\r\n    The new API, omrsig_register_os_handler, will let a user register a\r\n    handler with the OS while properly caching the original OS handler. This\r\n    will allow us to properly restore the original OS handler during\r\n    shutdown.\r\n\r\n2) **Re-format comments' layout**\r\n\r\n3) **Rename signalsWithMasterHandlers and masterHandlerMonitor**\r\n\r\n    signalsWithMasterHandlers is used to determine if we have registered a\r\n    handler with a signal. With the introduction of\r\n    omrsig_register_os_handler, a user can register handlers other than the\r\n    master handlers. signalsWithMasterHandlers has been renamed to\r\n    signalsWithHandlers to better represent its purpose.\r\n\r\n    masterHandlerMonitor is acquired whenever a handler is registered with\r\n    the OS. Since a user can register any arbitrary handler with the OS\r\n    using omrsig_register_os_handler, masterHandlerMonitor is renamed to\r\n    registerHandlerMonitor to better represent its purpose.\r\n\r\n    After restoring the original OS handler in sig_full_shutdown,\r\n    oldActions[index].restore should be reset to 0.\r\n\r\n4) **Add implementation of omrsig_register_os_handler [Unix]**\r\n\r\n    Implementation of omrsig_register_os_handler has been added in\r\n    unix/omrsignal.c.\r\n\r\n    Supporting entry and exit trace points have been added.\r\n\r\n    registerSignalHandlerWithOS returns OMRPORT_SIG_ERROR for OS signals\r\n    that are included in signalMap.\r\n\r\n5) **Fix omrsig_set_async_signal_handler signature**\r\n\r\n    Currently, omrsig_set_async_signal_handler returns an uint32_t.\r\n\r\n    But, unix and ztpf implementations of omrsig_set_async_signal_handler\r\n    set return value to OMRPORT_SIG_ERROR (-1).\r\n\r\n    In order to allow negative return values,\r\n    omrsig_set_async_signal_handler's signature is changed to return an\r\n    int32_t.\r\n\r\n6) **Use error macros instead of undocumented constants for errors**\r\n\r\n    In omrsig_set_async_signal_handler and\r\n    omrsig_set_single_async_signal_handler, constants such as 1 and -1 are\r\n    directly used as error codes. Using constants directly leads to\r\n    inconsistency in implementation across platforms.\r\n\r\n    Now onwards, omrsig_set_async_signal_handler and\r\n    omrsig_set_single_async_signal_handler will consistently return\r\n    OMRPORT_SIG_ERROR (-1) for error.\r\n\r\n7) **Cache info if a master handler is registered with a signal**\r\n\r\n    omrsig_protect and omrsig_set_*_async_signal_handler may be invoked\r\n    multiple times. If a master handler is already registered with a signal,\r\n    then we don't want to re-register the master handler with that signal.\r\n\r\n    If a master handler is registered with a signal, then this information\r\n    is cached in signalsWithMasterHandlers. The port library signal bit is\r\n    set in signalsWithMasterHandlers.\r\n\r\n    When a user overrides the master handler by invoking\r\n    omrsig_register_os_handler, then the port library signal bit is un-set\r\n    in signalsWithMasterHandlers.\r\n\r\n    omrsig_set_single_async_signal_handler returns the oldOSHandler. The\r\n    oldOSHandler can be the master handler if the user re-invokes\r\n    omrsig_set_single_async_signal_handler for a signal. So, a user can\r\n    invoke omrsig_register_os_handler using a master handler. In this case,\r\n    we must set signalsWithMasterHandlers bits accordingly in\r\n    omrsig_register_os_handler.\r\n\r\n    This allows us to prevent OS calls to re-register master signal\r\n    handlers.\r\n\r\n8) **Remove unnecessary write barrier**\r\n\r\n    At the end of registerSignalHandlerWithOS, a write barrier is issued\r\n    before updating signalsWithHandlers. According to the comment, the write\r\n    barrier is required since registerHandlerMonitor isn't acquired.\r\n\r\n    registerSignalHandlerWithOS is invoked from:\r\n    i) registerMasterHandlers\r\n    ii) omrsig_register_os_handler [registerHandlerMonitor acquired]\r\n\r\n    registerMasterHandlers is invoked from:\r\n    i) omrsig_protect [registerHandlerMonitor acquired]\r\n    ii) omrsig_set_async_signal_handler [registerHandlerMonitor acquired]\r\n    iii) omrsig_set_single_async_signal_handler [registerHandlerMonitor\r\n    acquired]\r\n\r\n    I have confirmed that registerHandlerMonitor is acquired for every call\r\n    to registerSignalHandlerWithOS. So, the write barrier is no longer\r\n    needed.\r\n\r\n9) **Properly handle NULL values for oldOSHandler**\r\n\r\n    omrsig_set_single_async_signal_handler and omrsig_register_os_handler\r\n    have been updated to properly handle NULL values for oldOSHandler.\r\n    Function description has been updated for both of them. A trace point\r\n    was accessing oldOSHandler without a NULL check. A NULL check has been\r\n    added before the trace point accesses oldOSHandler.\r\n\r\nSigned-off-by: Babneet Singh <sbabneet@ca.ibm.com>"},"comment":{"url":"https://api.github.com/repos/eclipse/omr/issues/comments/386662865","html_url":"https://github.com/eclipse/omr/pull/2524#issuecomment-386662865","issue_url":"https://api.github.com/repos/eclipse/omr/issues/2524","id":386662865,"user":{"login":"babsingh","id":8634038,"avatar_url":"https://avatars1.githubusercontent.com/u/8634038?v=4","gravatar_id":"","url":"https://api.github.com/users/babsingh","html_url":"https://github.com/babsingh","followers_url":"https://api.github.com/users/babsingh/followers","following_url":"https://api.github.com/users/babsingh/following{/other_user}","gists_url":"https://api.github.com/users/babsingh/gists{/gist_id}","starred_url":"https://api.github.com/users/babsingh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/babsingh/subscriptions","organizations_url":"https://api.github.com/users/babsingh/orgs","repos_url":"https://api.github.com/users/babsingh/repos","events_url":"https://api.github.com/users/babsingh/events{/privacy}","received_events_url":"https://api.github.com/users/babsingh/received_events","type":"User","site_admin":false},"created_at":"2018-05-04T16:56:18Z","updated_at":"2018-05-04T16:56:18Z","author_association":"CONTRIBUTOR","body":"@charliegracie @pshipton these changes can be reviewed."}}
{"actor":{"display_login":"babsingh"}}