Skip to content
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

8293422: DWARF emitted by Clang cannot be parsed #10287

Closed
wants to merge 10 commits into from

Conversation

chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented Sep 15, 2022

The DWARF debugging symbols emitted by Clang is different from what GCC is emitting. While GCC produces a complete .debug_aranges section (which is required in the DWARF parser), Clang does not. As a result, the DWARF parser cannot find the necessary information to proceed and create the line number information:

The .debug_aranges section contains address range to compilation unit offset mappings. The parsing algorithm can just walk through all these entries to find the correct address range that contains the library offset of the current pc. This gives us the compilation unit offset into the .debug_info section from where we can proceed to parse the line number information.

Without a complete .debug_aranges section, we fail with an assertion that we could not find the correct entry. Since JDK-8293402, we will still get the complete stack trace at least. Nevertheless, we should still fix this assertion failure of course. But that would require a different parsing approach. We need to parse the entire .debug_info section instead to get to the correct compilation unit. This, however, would require a lot more work.

I therefore suggest to disable DWARF parsing for Clang for now and file an RFE to support Clang in the future with a different parsing approach. I'm using the __clang__ ifdef to bail out in get_source_info() and disable the gtests. I've noticed that we are currently running the gtests with NOT PRODUCT which I think is not necessary - the gtests should also work fine with product builds. I've corrected this as well but that could also be done separately.

Thanks,
Christian


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10287/head:pull/10287
$ git checkout pull/10287

Update a local copy of the PR:
$ git checkout pull/10287
$ git pull https://git.openjdk.org/jdk pull/10287/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10287

View PR using the GUI difftool:
$ git pr show -t 10287

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10287.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 15, 2022

👋 Welcome back chagedorn! 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 openjdk bot added the rfr Pull request is ready for review label Sep 15, 2022
@openjdk
Copy link

openjdk bot commented Sep 15, 2022

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

  • hotspot

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.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Sep 15, 2022
@mlbridge
Copy link

mlbridge bot commented Sep 15, 2022

Webrevs

@tschatzl
Copy link
Contributor

From https://discourse.llvm.org/t/clang-does-not-produce-full-debug-aranges-section-with-thinlto/64898/8, one needs to use -gdwarf-aranges with clang. However I am not sure whether we will hit that bug with "thin LTO" mentioned there (or others) if we use it.

… and add new fix with -gdwarf-aranges + -gdwarf-4 for Clang 5.0+
@chhagedorn
Copy link
Member Author

Thanks Thomas for that link. I was not aware of this -gdwarf-aranges flag. I've tried it out and it indeed seems to work. But I was not able to build with -flto=thin as it resulted in build failures. So, I'm not sure what would happen and if it's even possible to build with it in general. Nevertheless, I suggest to go with that -gdwarf-aranges flag solution for now and remove the previously suggested bailout fix for Clang.

However, -gdwarf-aranges (and -gdwarf-4 which I think we should also add to avoid getting the unsupported DWARF 5 format) was only added in Clang 5.0. But we must support down to 3.5 according to:

jdk/doc/building.md

Lines 353 to 354 in cb72f80

The minimum accepted version of clang is 3.5. Older versions will not be
accepted by `configure`.

I therefore changed the previous complete bailout fix to a bailout fix for Clang versions older than 5.0.

I've noticed that Clang is emitting a full relative path for the filename in the form of src/hotspot/share/compiler/compilerThread.cpp:58 with debug builds (it only emits the filename with release builds). I therefore added an additional method strip_path_prefix() to get rid of the path prefix.

@tschatzl
Copy link
Contributor

Thanks Thomas for that link. I was not aware of this -gdwarf-aranges flag. I've tried it out and it indeed seems to work. But I was not able to build with -flto=thin as it resulted in build failures.

I only thought that maybe we do compile with -flto=thin already, and so we could not use these flags.

So, I'm not sure what would happen and if it's even possible to build with it in general. Nevertheless, I suggest to go with that -gdwarf-aranges flag solution for now and remove the previously suggested bailout fix for Clang.

I agree.

However, -gdwarf-aranges (and -gdwarf-4 which I think we should also add to avoid getting the unsupported DWARF 5 format) was only added in Clang 5.0. But we must support down to 3.5 according to:

jdk/doc/building.md

Lines 353 to 354 in cb72f80

The minimum accepted version of clang is 3.5. Older versions will not be
accepted by `configure`.

I therefore changed the previous complete bailout fix to a bailout fix for Clang versions older than 5.0.

I've noticed that Clang is emitting a full relative path for the filename in the form of src/hotspot/share/compiler/compilerThread.cpp:58 with debug builds (it only emits the filename with release builds). I therefore added an additional method strip_path_prefix() to get rid of the path prefix.

Okay.

Copy link
Contributor

@tschatzl tschatzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestions

// Remove everything before the last slash including the slash itself to get the actual filename. This is required, for
// example, for Clang debug builds which emit a relative path while GCC only emits the filename.
void DwarfFile::LineNumberProgram::strip_path_prefix(char* filename, const size_t filename_len) {
char* last_slash = strrchr(filename, '/');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use os::file_separator() instead of / here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also corrected a second usage of / in the DWARF parser code.

int bytes_written = jio_snprintf(filename, filename_len - index_after_slash, "%s", filename + index_after_slash);
assert(bytes_written > 0, "could not strip path prefix");
// Add null terminator.
jio_snprintf(filename + bytes_written, 1, "%s", '\0');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the correct format string is %c here. I also looked into other code that appends the \0, that one just pokes it in via direct assignment, but snprintf does not seem wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, directly setting it is more straight forward.

// GCC only emits the filename.
// This gives us either "jni.cp" or "src/ho". In the latter case, we strip the path prefix to get to the actual
// filename which, however, is useless in this case - we get "ho".
ASSERT_TRUE(strcmp(buf, "jni.cp") == 0 || strcmp(buf, "ho") == 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not try it out, but since we already stripped the prefix before printing the file name, why can we get "ho" here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the comment to make it more clear. Due to the small buffer, we only read "src/ho" instead of "src/hotspot/share...". And since we are also calling strip_path_prefix, we strip "src/" and we get "ho" as filename which is not actually a filename.

Copy link
Contributor

@tschatzl tschatzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave it up to you what to do about the path stripping problem mentioned in the comment for DwarfFile::LineNumberProgram::get_filename_from_header.

@@ -1610,6 +1610,7 @@ bool DwarfFile::LineNumberProgram::get_filename_from_header(const uint32_t file_

if (current_index == file_index) {
// Found correct file.
strip_path_prefix(filename, filename_len);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some digging I believe this is the wrong place to strip the path prefix, and causes the strange workaround in the decoder_get_source_info_valid_truncated gtest.

The call to _reader.read_string() above should do the stripping as it is read; if like in the gtest, the given filename_len is too small to contain the original string, the strip_path_prefix tries to strip the too small buffer, but what has actually been intended was probably stripping the entire path and then limiting the return value.

I.e. a more useful implementation of this would be reading the string into a temporary buffer, stripping the path prefix and then copying the result to the output buffer.

I can see the reasoning for why the current implementation is as is, it is nowhere specified what the contents of the filename string in debug_aranges should be, and what should be actually printed.

Looking at callers of this method, it might actually a problem when using clang: VMError::print_native_stack uses a 128 byte buffer, NativeCallStack::print_on uses a 1024 byte buffer.

I can see that at least 128 bytes would be just a bit small, but then we (Oracle) do not use clang. It's up to you to fix this imo.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay to come back to this. I think you are right. We should always first strip the prefix path and only then cut the filename to fit it into the filename output buffer. I've pushed an update with a temporary buffer of size 1024 and reverted the gtest changes. Now it works with Clang and GCC without modifying the test.

@openjdk
Copy link

openjdk bot commented Sep 22, 2022

@chhagedorn 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:

8293422: DWARF emitted by Clang cannot be parsed

Reviewed-by: tschatzl, ihse, stuefe

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 6 new commits pushed to the master branch:

  • 59d8f67: 8297265: G1: Remove unnecessary null-check in RebuildCodeRootClosure::do_code_blob
  • 2fc340a: 8148041: Test java/awt/Mouse/TitleBarDoubleClick/TitleBarDoubleClick fails on Ubuntu with mouseReleased event after double click on title bar
  • e420661: 8297238: RISC-V: C2: Use Matcher::vector_element_basic_type when checking for vector element type in predicate
  • 891c706: 8295276: AArch64: Add backend support for half float conversion intrinsics
  • 3c09498: 8297241: Update sun/java2d/DirectX/OnScreenRenderingResizeTest/OnScreenRenderingResizeTest.java
  • 45d1807: 6312651: Compiler should only use verified interface types for optimization

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 22, 2022
Copy link
Contributor

@tschatzl tschatzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm. Thanks.

@chhagedorn
Copy link
Member Author

Thanks Thomas for your review!

@vnkozlov
Copy link
Contributor

/cc build

@openjdk openjdk bot added the build build-dev@openjdk.org label Oct 11, 2022
@openjdk
Copy link

openjdk bot commented Oct 11, 2022

@vnkozlov
The build label was successfully added.

@magicus
Copy link
Member

magicus commented Oct 12, 2022

We currently require clang 3.5 (when building on linux I presume; Xcode has their very own peculiar way of versioning clang). Maybe this is too old? Should we instead raise the bar to require clang 5.0, so we can skip the test?

@magicus
Copy link
Member

magicus commented Oct 12, 2022

Yeah, clang 5.0 was released in 2017. I'd recommend you just hard-code in the dwarf flags and instead apply this:

diff --git a/make/autoconf/toolchain.m4 b/make/autoconf/toolchain.m4
index adb4e182dcc..e1c097916d8 100644
--- a/make/autoconf/toolchain.m4
+++ b/make/autoconf/toolchain.m4
@@ -50,7 +50,7 @@ TOOLCHAIN_DESCRIPTION_microsoft="Microsoft Visual Studio"
 TOOLCHAIN_DESCRIPTION_xlc="IBM XL C/C++"
 
 # Minimum supported versions, empty means unspecified
-TOOLCHAIN_MINIMUM_VERSION_clang="3.5"
+TOOLCHAIN_MINIMUM_VERSION_clang="5.0"
 TOOLCHAIN_MINIMUM_VERSION_gcc="6.0"
 TOOLCHAIN_MINIMUM_VERSION_microsoft="19.28.0.0" # VS2019 16.8, aka MSVC 14.28
 TOOLCHAIN_MINIMUM_VERSION_xlc=""

@chhagedorn
Copy link
Member Author

That would be an option to bump that minimum version to get rid of the ifdefs for clang 5.0. But since this has quite an impact, are we allowed to just do that in this change or is it required to have a separate task for that with approvals first? We would also need to change the docs to reflect that change etc.

@magicus
Copy link
Member

magicus commented Oct 12, 2022

If is is a big change or not depends on how it affects builds on macos. I assume that clang functionality that was published in 2017 is already incorporated in the minimum supported version of Xcode on mac. (This needs to be verified, though)

For clang on linux; Oracle do not regularly build linux with clang, nor do our GHA build scripts. I don't know if anyone regularly tests this.

My guess is that the 3.5 limit was put in place when the clang for linux support was added, and it has not been modified since.

But you are probably right that it should be a separate PR, if not for anything else so to get proper attention to it. Do you want me to publish such a PR first, or do you want to continue with the current conditionals, and clean them up afterwards if/when we go to clang 5.0+?

@chhagedorn
Copy link
Member Author

If is is a big change or not depends on how it affects builds on macos. I assume that clang functionality that was published in 2017 is already incorporated in the minimum supported version of Xcode on mac. (This needs to be verified, though)

For clang on linux; Oracle do not regularly build linux with clang, nor do our GHA build scripts. I don't know if anyone regularly tests this.

My guess is that the 3.5 limit was put in place when the clang for linux support was added, and it has not been modified since.

Okay, I see, thanks for the summary!

But you are probably right that it should be a separate PR, if not for anything else so to get proper attention to it.

Yes, it's probably better to get more attention for this update.

Do you want me to publish such a PR first, or do you want to continue with the current conditionals, and clean them up afterwards if/when we go to clang 5.0+?

I think both is fine. But since I already have the patch ready, I suggest to move forward with it and then come back later to clean it up.

Copy link
Member

@magicus magicus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build changes are approved.

@chhagedorn
Copy link
Member Author

Thanks Magnus for your review of the build changes!

May a get a second review of the DWARF parser code changes?

Thanks,
Christian

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 15, 2022

@chhagedorn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@magicus
Copy link
Member

magicus commented Nov 16, 2022

@chhagedorn Are you waiting for an additional Hotspot review?

@chhagedorn
Copy link
Member Author

Yes, I think it would be good to get a second review of the DWARF parser changes. Maybe @tstuefe?

@tstuefe
Copy link
Member

tstuefe commented Nov 16, 2022

Yes, I think it would be good to get a second review of the DWARF parser changes. Maybe @tstuefe?

I'm a bit swamped, but try to take a look later today.

@chhagedorn
Copy link
Member Author

Thank you Thomas! Take your time, there is no hurry.

@@ -1601,14 +1601,18 @@ bool DwarfFile::LineNumberProgram::get_filename_from_header(const uint32_t file_
// We do not need to restore the position afterwards as this is the last step of parsing from the file for this compilation unit.
_reader.set_position(_header._file_names_offset);
uint32_t current_index = 1; // file_names start at index 1
const size_t dwarf_filename_len = 1024;
char dwarf_filename[dwarf_filename_len]; // Store the filename read from DWARF which is then copied to 'filename'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting such a large array on the stack is a bit borderline. Especially in error reporting, where you may build up stack repeatedly via secondary crash handling. I realize though that no good alternatives exist. C-heap may be corrupted, ResourceArea is also out of the question. Could we get away with using filename directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. Maybe I can change the algorithm to read single characters instead and reset the buffer once I'm encountering a file separator. I'll try that out.

@@ -861,6 +861,8 @@ class DwarfFile : public ElfFile {
bool does_offset_match_entry(uintptr_t previous_address, uint32_t previous_file, uint32_t previous_line);
void print_and_store_prev_entry(uint32_t previous_file, uint32_t previous_line);
bool get_filename_from_header(uint32_t file_index, char* filename, size_t filename_len);
static void strip_path_prefix(char* filename, const size_t filename_len);
static void copy_dwarf_filename_to_filename(char* src, size_t src_len, char* dst, size_t dst_len);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stupid question, do these have to be exposed? Or could they be just static helpers in elfFile.cpp?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, they don't need to be exposed in the sense of being declared in the header. But in terms of readability, I've decided to put them in the private block of class LineNumberProgram which is the only user of these methods. What would be the advantages of moving them completely to elfFile.cpp as static helper functions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I generally just prefer to keep things locally if possible. Slim interfaces, less polluted global namespace, possibly (though not here) less include deps. Don't worry, if you prefer it this way, keep it in.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. I don't have a strong preference here but to be consistent with the existing DWARF parser code, I'd rather keep it like that.

if (last_slash != nullptr) {
uint16_t index_after_slash = (uint16_t)(last_slash + 1 - filename);
// Copy filename to beginning of buffer.
int bytes_written = jio_snprintf(filename, filename_len - index_after_slash, "%s", filename + index_after_slash);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is guaranteed to work since the memory areas you move may interleave. You should copy char-wise, or use memmove(3).

void DwarfFile::LineNumberProgram::strip_path_prefix(char* filename, const size_t filename_len) {
char* last_slash = strrchr(filename, *os::file_separator());
if (last_slash != nullptr) {
uint16_t index_after_slash = (uint16_t)(last_slash + 1 - filename);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why uint16_t?

We have pointer_delta() for that btw if you want to be super correct. See globalDefinitions.hpp

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why uint16_t?

There is no specific reason. I'll change it to uint32_t.

We have pointer_delta() for that btw if you want to be super correct. See globalDefinitions.hpp

Ah that's great! Will use that one instead.

const size_t count = MIN(src_len, dst_len);
int bytes_written = jio_snprintf(dst, count, "%s", src);
// Add null terminator.
dst[count - 1] = '\0';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to return a truncated file name up to the caller of DwarfFile::LineNumberProgram::get_filename_from_header()? Will this not just confuse him? I think it makes more sense to cleanly handle truncation, and e.g. skip file parsing for dwarf files with too long names.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right that it's rather unexpected to get an incomplete filename back. But just silently skipping the filename might be confusing as well. We could either print an error (I guess that's useful either way but should only be printed with TraceDwarfLevel) or just return a generic "buffer overflow" string as filename instead if it fits into the provided filename buffer. And only if that's not possible we could silently skip the filename. Would that be an option?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional tracing and returning a generic string sounds fine.

…he fly and only caring about the actual filename (ignore prefix path when reading)
@chhagedorn
Copy link
Member Author

I've pushed an update and changed the algorithm in the following way to address @tstuefe review comments:

  • Just read and ignore the filenames from DWARF which do not correspond to the one we are looking for (i.e. when current_index != file_index).
  • Reading the filename of interest (i.e. when current_index == file_index):
    • Read single chars and stop once the null terminator is found.
    • Reset buffer when file separator is found to skip the prefix path.
    • On filename buffer overflow: Keep reading, we could still be reading a path prefix and reset the buffer again when finding a file separator.
    • If filename does not fit into the provided buffer, use a generic overflow message <OVERFLOW>. If that does not fit either, use the minimal filename L. This allows to at least have the source information L:line_no which almost always is already enough to get to the actual source code location. Doing it in this way lets the parser succeed instead of failing.

I've added some additional tests for the overflow scenarios and enforced get_source_info() to only accept buffers with a length of at least 2 to always allow the minimal filename L.

Submitted some testing again in our CI (results look good so far) and additionally by running gtests with Clang slowdebug, fastdebug and release builds.

I've done some additional manual testing, both with GCC and Clang builds. I've also played around by changing the buffer size in vmError.cpp. It works as expected:

  • buffer size 15, emitting <OVERFLOW> for filenames being too long:
V  [libjvm.so+0x8905fc]  CompileWrapper::~CompileWrapper()+0x56  (compile.cpp:492)
V  [libjvm.so+0x8921e6]  Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x1652  (compile.cpp:864)
V  [libjvm.so+0x77d171]  C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x179  (c2compiler.cpp:113)
V  [libjvm.so+0x8b0fc4]  CompileBroker::invoke_compiler_on_method(CompileTask*)+0x8e8  (<OVERFLOW>:2237)
V  [libjvm.so+0x8afc3d]  CompileBroker::compiler_thread_loop()+0x3ed  (<OVERFLOW>:1916)
V  [libjvm.so+0x8d047c]  CompilerThread::thread_entry(JavaThread*, JavaThread*)+0x72  (<OVERFLOW>:58)
V  [libjvm.so+0xc63342]  JavaThread::thread_main_inner()+0x144  (javaThread.cpp:699)
V  [libjvm.so+0xc631fa]  JavaThread::run()+0x182  (javaThread.cpp:684)
V  [libjvm.so+0x1337633]  Thread::call_run()+0x195  (thread.cpp:224)
V  [libjvm.so+0x10e38d7]  thread_native_entry(Thread*)+0x19b  (os_linux.cpp:710)
  • buffer size 2, emitting L for all filenames (being too long):
V  [libjvm.so+0x8905fc]  CompileWrapper::~CompileWrapper()+0x56  (compile.cpp:492)
V  [libjvm.so+0x8921e6]  Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x1652  (L:864)
V  [libjvm.so+0x77d171]  C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x179  (L:113)
V  [libjvm.so+0x8b0fc4]  CompileBroker::invoke_compiler_on_method(CompileTask*)+0x8e8  (L:2237)
V  [libjvm.so+0x8afc3d]  CompileBroker::compiler_thread_loop()+0x3ed  (L:1916)
V  [libjvm.so+0x8d047c]  CompilerThread::thread_entry(JavaThread*, JavaThread*)+0x72  (L:58)
V  [libjvm.so+0xc6339c]  JavaThread::thread_main_inner()+0x144  (L:699)
V  [libjvm.so+0xc63254]  JavaThread::run()+0x182  (L:684)
V  [libjvm.so+0x1337693]  Thread::call_run()+0x195  (L:224)
V  [libjvm.so+0x10e3937]  thread_native_entry(Thread*)+0x19b  (L:710)

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. If tests are green, fine for me. Thanks!

@chhagedorn
Copy link
Member Author

Thanks Thomas for your review and your feedback! Testing looked good.

@tschatzl do you also agree with the new approach?

@openjdk
Copy link

openjdk bot commented Nov 21, 2022

@chhagedorn this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8293422
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Nov 21, 2022
@openjdk openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Nov 21, 2022
@chhagedorn
Copy link
Member Author

Thanks @tschatzl for reviewing it again!

/integrate

@openjdk
Copy link

openjdk bot commented Nov 21, 2022

Going to push as commit 8b8d848.
Since your change was applied there have been 6 commits pushed to the master branch:

  • 59d8f67: 8297265: G1: Remove unnecessary null-check in RebuildCodeRootClosure::do_code_blob
  • 2fc340a: 8148041: Test java/awt/Mouse/TitleBarDoubleClick/TitleBarDoubleClick fails on Ubuntu with mouseReleased event after double click on title bar
  • e420661: 8297238: RISC-V: C2: Use Matcher::vector_element_basic_type when checking for vector element type in predicate
  • 891c706: 8295276: AArch64: Add backend support for half float conversion intrinsics
  • 3c09498: 8297241: Update sun/java2d/DirectX/OnScreenRenderingResizeTest/OnScreenRenderingResizeTest.java
  • 45d1807: 6312651: Compiler should only use verified interface types for optimization

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 21, 2022
@openjdk openjdk bot closed this Nov 21, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 21, 2022
@openjdk
Copy link

openjdk bot commented Nov 21, 2022

@chhagedorn Pushed as commit 8b8d848.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
5 participants