8254824: SignalHandlerMark have no purpose #677

robehn · 2020-10-15T08:46:01Z

SignalHandlerMark increase and decrease _num_nested_signal, but it is never read.
We have no caller to is_inside_signal_handler().

Testing T1.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Testing

	Linux x64	Windows x64	macOS x64
Build	✔️ (5/5 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)

Issue

JDK-8254824: SignalHandlerMark have no purpose

Reviewers

Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to b1151e8
Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to b1151e8
David Holmes (@dholmes-ora - Reviewer)
Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to b1151e8

Download

$ git fetch https://git.openjdk.java.net/jdk pull/677/head:pull/677
$ git checkout pull/677

bridgekeeper · 2020-10-15T08:47:17Z

👋 Welcome back rehn! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2020-10-15T08:49:47Z

@robehn The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2020-10-15T08:54:14Z

Webrevs

01: Full - Incremental (54a1104)
00: Full (b1151e8)

tstuefe

Looks good. Seems like a leftover from Solaris.

openjdk · 2020-10-15T09:06:45Z

@robehn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8254824: SignalHandlerMark have no purpose

Reviewed-by: stuefe, shade, dholmes, coleenp

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 17 new commits pushed to the master branch:

402d01a: 8254795: Remove obsolete template files
07ec35e: 8254623: gc/g1/TestHumongousConcurrentStartUndo.java still fails sometimes
0570cc1: 8254855: Clean up and remove unused code in vmIntrinsics
1742c44: 8254695: G1: Next mark bitmap clear not cancelled after marking abort
34583eb: 8254161: Prevent instantiation of EnumSet subclasses through deserialization
3d23bd8: 8254814: [Vector API] Fix an AVX512 crash after JDK-8223347
7c0d417: 8251535: Partial peeling at unsigned test adds incorrect loop exit check
5145bed: 8254125: Assertion in cppVtables.cpp during builds on 32bit Windows
bdda205: 8254369: Node::disconnect_inputs may skip precedences
96bb6e7: 8251325: Miss 'L' for long value in if statement
... and 7 more: https://git.openjdk.java.net/jdk/compare/9359ff03ae6b9e09e7defef148864f40e949b669...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

shipilev

Looks fine to me.

robehn · 2020-10-15T09:15:03Z

Thanks @tstuefe and @shipilev!

coleenp · 2020-10-15T11:25:23Z

As much as I like seeing old code removed, wouldn't this be something that would be useful? Should Mutex::lock with safepoint check for instance check for this?

mlbridge · 2020-10-15T11:37:10Z

Mailing list message from David Holmes on hotspot-runtime-dev:

On 15/10/2020 7:08 pm, Thomas Stuefe wrote:

On Thu, 15 Oct 2020 08:46:01 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

SignalHandlerMark increase and decrease _num_nested_signal, but it is never read.
We have no caller to is_inside_signal_handler().

Testing T1.

Looks good. Seems like a leftover from Solaris.

Yes (for os::fork_and_exec), and also the old Thread::current() code:

inline Thread* Thread::current() {
#ifdef ASSERT
// This function is very high traffic. Define PARANOID to enable expensive
// asserts.
#ifdef PARANOID
// Signal handler should call ThreadLocalStorage::get_thread_slow()
Thread* t = ThreadLocalStorage::get_thread_slow();
assert(t != NULL && !t->is_inside_signal_handler(),
"Don't use Thread::current() inside signal handler");
#endif
#endif
Thread* thread = ThreadLocalStorage::thread();
assert(thread != NULL, "just checking");
return thread;
}

Arguably we should be checking that a lot of code is never executed in a
signal handling context, but crash reporting will likely invalidate any
attempt to do (unless we have an escape clause for when doing error
reporting).

David
-----

src/hotspot/os_cpu/bsd_x86/os_bsd_x86.cpp

src/hotspot/share/runtime/thread.hpp

tstuefe · 2020-10-15T17:32:34Z

As much as I like seeing old code removed, wouldn't this be something that would be useful? Should Mutex::lock with safepoint check for instance check for this?

Maybe. But as it is, beside being unused it is pretty inconsistent: only the main signal hndler uses this, not the SR_handler nor the secondary error handler. So, Thread::_num_nested_signal would only ever be >1 if a second signal happened right inside the signal handling but before fatal error handling kicks in, which as its first step installs the secondary error handler.

BTW one thing I wondered about is that all the JVM_handle_xxx_signal() functions return an int; but we return anything out of {true, false, 0, 1.} And the return value is then ignored at the caller. For example, the "abort_if_unrecognized" option does not seem to do anything - can probably be removed.

coleenp · 2020-10-15T18:59:50Z

So the code I was worried about is the code between JVM_handle_x_signal and calling report_and_die. There's a lot of duplicated code in JVM_handle_x_signal. I moved some of it but if we move more of it far away, it would be nice to know we aren't calling Thread::current() or some Mutex inside of the signal handler. That said, I'm ok with its deletion.

tstuefe · 2020-10-16T07:08:20Z

So the code I was worried about is the code between JVM_handle_x_signal and calling report_and_die. There's a lot of duplicated code in JVM_handle_x_signal. I moved some of it but if we move more of it far away, it would be nice to know we aren't calling Thread::current() or some Mutex inside of the signal handler. That said, I'm ok with its deletion.

I see your point now, I agree with you that it could be useful information. But its very easy to re-introduce if we want. We don't even need a RAII object, we could just increase the signal nesting level in javaSignalHandler() directly.

robehn · 2020-10-16T08:02:35Z

So the code I was worried about is the code between JVM_handle_x_signal and calling report_and_die. There's a lot of duplicated code in JVM_handle_x_signal. I moved some of it but if we move more of it far away, it would be nice to know we aren't calling Thread::current() or some Mutex inside of the signal handler. That said, I'm ok with its deletion.

I see your point now, I agree with you that it could be useful information. But its very easy to re-introduce if we want. We don't even need a RAII object, we could just increase the signal nesting level in javaSignalHandler() directly.

If you execute any code which takes a Mutex you risk deadlocking, since they are not recursive and pthread_mutex is not signal safe. (if they were you risk that the data they are protecting are inconsistent when reading it from signal handler any ways)
Any code path not carefully review should be considered unsafe to potentially re-enter from signal handler.
We also on purpose call code which is not signal safe, e.g. vfprintf is not signal safe, thus any code that can log is not safe AFAICT.
So just one check in one specific case is not that useful and since we do call unsafe code intentionally the rules are very fuzzy.

dholmes-ora

Looks good.
Thanks.

robehn · 2020-10-19T06:27:12Z

/integrate

openjdk · 2020-10-19T06:29:21Z

@robehn Since your change was applied there have been 26 commits pushed to the master branch:

736e077: 8254178: Remove .hgignore
51a865d: 8254085: javax/swing/text/Caret/TestCaretPositionJTextPane.java failed with "RuntimeException: Wrong caret position"
dd032b7: 8254798: Deprecate for removal an empty finalize() methods in java.desktop module
272bb5d: 8253455: Record Classes javax.lang.model changes
c17d585: 8246774: implement Record Classes as a standard feature in Java
0b3e6c5: 8194126: Regression automated Test '/open/test/jdk/javax/swing/JColorChooser/Test7194184.java' fails
ce1aac1: 8028707: javax/swing/JComboBox/6236162/bug6236162.java fails on azure
83ea863: 8253559: The INDEX page should link to Serialized Form and Constant Values pages
e66c6bb: 8254349: The TestNoScreenMenuBar test should be updated
402d01a: 8254795: Remove obsolete template files
... and 16 more: https://git.openjdk.java.net/jdk/compare/9359ff03ae6b9e09e7defef148864f40e949b669...master

Your commit was automatically rebased without conflicts.

Pushed as commit 011dd0d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

SignalHandlerMark removed

b1151e8

robehn marked this pull request as ready for review October 15, 2020 08:46

openjdk bot added the rfr Pull request is ready for review label Oct 15, 2020

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Oct 15, 2020

tstuefe approved these changes Oct 15, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 15, 2020

shipilev approved these changes Oct 15, 2020

View reviewed changes

dholmes-ora suggested changes Oct 15, 2020

View reviewed changes

src/hotspot/os_cpu/bsd_x86/os_bsd_x86.cpp Show resolved Hide resolved

src/hotspot/share/runtime/thread.hpp Show resolved Hide resolved

coleenp approved these changes Oct 15, 2020

View reviewed changes

Added comments

54a1104

dholmes-ora approved these changes Oct 16, 2020

View reviewed changes

openjdk bot closed this Oct 19, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 19, 2020

robehn deleted the 8254824-SignalHandlerMark-have-no-purpose branch October 19, 2020 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8254824: SignalHandlerMark have no purpose #677

8254824: SignalHandlerMark have no purpose #677

robehn commented Oct 15, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 15, 2020

openjdk bot commented Oct 15, 2020

mlbridge bot commented Oct 15, 2020 •

edited

Loading

tstuefe left a comment

openjdk bot commented Oct 15, 2020 •

edited

Loading

shipilev left a comment

robehn commented Oct 15, 2020

coleenp commented Oct 15, 2020

mlbridge bot commented Oct 15, 2020

tstuefe commented Oct 15, 2020 •

edited

Loading

coleenp commented Oct 15, 2020

tstuefe commented Oct 16, 2020

robehn commented Oct 16, 2020

dholmes-ora left a comment

robehn commented Oct 19, 2020

openjdk bot commented Oct 19, 2020

8254824: SignalHandlerMark have no purpose #677

8254824: SignalHandlerMark have no purpose #677

Conversation

robehn commented Oct 15, 2020 • edited by openjdk bot Loading

Progress

Testing

Issue

Reviewers

Download

bridgekeeper bot commented Oct 15, 2020

openjdk bot commented Oct 15, 2020

mlbridge bot commented Oct 15, 2020 • edited Loading

Webrevs

tstuefe left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 15, 2020 • edited Loading

shipilev left a comment

Choose a reason for hiding this comment

robehn commented Oct 15, 2020

coleenp commented Oct 15, 2020

mlbridge bot commented Oct 15, 2020

tstuefe commented Oct 15, 2020 • edited Loading

coleenp commented Oct 15, 2020

tstuefe commented Oct 16, 2020

robehn commented Oct 16, 2020

dholmes-ora left a comment

Choose a reason for hiding this comment

robehn commented Oct 19, 2020

openjdk bot commented Oct 19, 2020

robehn commented Oct 15, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 15, 2020 •

edited

Loading

openjdk bot commented Oct 15, 2020 •

edited

Loading

tstuefe commented Oct 15, 2020 •

edited

Loading