-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8368176: ASAN should not inhibit hs-err file generation #27404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
8368176: ASAN should not inhibit hs-err file generation #27404
Conversation
👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
ASAN_CFLAGS="$ASAN_CFLAGS -fsanitize-recover=address" | ||
elif test "x$TOOLCHAIN_TYPE" = "xmicrosoft"; then | ||
# -Oy- is equivalent to -fno-omit-frame-pointer in GCC/Clang. | ||
ASAN_CFLAGS="-fsanitize=address -Oy- -DADDRESS_SANITIZER" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the -fsanitize-recover=address
compiler options supported when TOOLCHAIN_TYPE == microsoft
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at Visual Studio documentation and such, it looks like -fsanitize-recover
isn't supported (yet).
https://developercommunity.visualstudio.com/t/add-fsanitize-recoveraddress-support-to-asan/1459414
Hi, jdk support address sanitizer and leak sanitizer for now. It seems that this PR only concern address sanitizer, does the leak sanitizer also need this enhancement |
gcc15.2 docs list the sanitizers that support recovery, and "leak" is not in But maybe we should consider this PR premature, since `-fsanitize-recovery=address" is still experimental. |
Shouldn't there be a configure check for the availability of the |
ASAN, when catching an error, will abort the process.
Two things control this:
-fsanitize-recover=address
(resp.-fno-sanitize-recover=address
. This controls whether, once ASAN returns from its error report, the compiler-generated ASAN stubs will abort the process. This is by default set to-fno-sanitize-recover=address
, so we won't recover.halt_on_error
controls whether ASAN itself returns from its error handler or whether it aborts the process. This, by default, is set to1
, so by default ASAN aborts.We "double abort" in the sense that two options are overlaid and both prevent the process from continuing.
I propose that we set, during build time for ASAN builds, the option
-fsanitize-recover=address
. Now, we can control whether to abort or not using the runtime settinghalt_on_error=0
. By default, we still will abort, sincehalt_on_error=1
. So, the default behavior won't change. However, we can now at least decide to do it differently.What would that give us?
By aborting right away, ASAN denies the JVM the option to catch the error and write an hs-err file. Of course, not every error that ASAN catches will result in a segfault or in an assertion. The JVM could lurch on for a bit before it stumbles. However, the chance for the JVM to stop on its own very soon after a memory corruption happens is pretty good. Then we get a hs-err file and a crash dump in close correlation to the error ASAN caught.
And even if there is no close relationship between the original ASAN error and the eventual segfault/assertion (think ASAN sees a double free, JVM continues, and after a while asserts somewhere else as a remote consequence of the error - the stacks in the hs-err file won't be related to the original error) - the hs-err file is shock-full of helpful information about running threads (see also JDK-8368124), memory mappings, JVM flags, etc. All of that would make it easier to understand the ASAN report.
And even if the JVM survives, one can still attach to the still living process and grab thread dumps, VM.info reports, heap dumps etc.
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27404/head:pull/27404
$ git checkout pull/27404
Update a local copy of the PR:
$ git checkout pull/27404
$ git pull https://git.openjdk.org/jdk.git pull/27404/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27404
View PR using the GUI difftool:
$ git pr show -t 27404
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27404.diff
Using Webrev
Link to Webrev Comment