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
8257228: G1: SIGFPE in G1ConcurrentRefine::create(int*) due to buffers_to_cards overflow #1489
Conversation
…s_to_cards overflow
/issue add JDK-8257228 |
👋 Welcome back jiefu! A progress list of the required criteria for merging this PR into |
@DamonFool This issue is referenced in the PR title - it will now be updated. |
@DamonFool |
@DamonFool The |
Webrevs
|
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.
This is only a problem when using values for some command line options that
are far out of the "normal" range. That's just a general problem; we have
far too many options, and some of them interact in interesting ways, so that
it's pretty much impossible to fully test or check for problem cases. And we
don't want to set artificially low limit values for individual options
because it's hard to know what some application might find useful. In this
case, it does look like we can reasonably do more checking though.
if (value == 0) return 0; | ||
size_t res = value * G1UpdateBufferSize; | ||
|
||
if (res < value || res < G1UpdateBufferSize) { // Check overflow |
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.
That's not a correct multiplication overflow check.
if (res < value || res < G1UpdateBufferSize) { // Check overflow | ||
vm_exit_during_initialization("buffers_to_cards overflow!"); | ||
} | ||
return res; |
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.
buffers_to_cards is only used with command line options as arguments. So
it's not inappropriate to use vm_exit_during_initialization, even though
this function isn't obviously about initialization. Maybe it should be
changed to
// Convert configuration values in units of buffers to number of cards.
static size_t configuration_buffers_to_cards(size_t value, const char* value_name) {
... use value_name in the error message
}
An alternative would be to move the checking to constraint functions for
the command line options. That puts the checking far from the code where
it matters though, but maybe a comment in the conversion function referring
to the option constraints would be sufficient. This might provide better
reporting?
/test |
@DamonFool you need to get approval to run the tests in tier1 for commits up until 76dec20 |
Hi @kimbarrett , Thanks for your review and comments. The mul-overflow checking had been fixed. For the sake of maintenance, I didn't use constraint function. There are quite a few flags involved in it such as:
Maybe, you can add the constraint function for each of them this time. What do you think? Thanks. |
/test |
@DamonFool you need to get approval to run the tests in tier1 for commits up until e3ea5de |
|
||
public class TestBuffersToCardsOverflow { | ||
public static void main(String... args) throws Exception { | ||
ProcessTools.executeTestJava("-XX:G1ConcRefinementThresholdStep=16G", |
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.
It seems like this will fail to start on a 32bit platform, because the threshold step exceeds the possible range.
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.
Good catch!
Fixed. Thanks.
These updates look okay.
The constraint function could be on G1UpdateBufferSize, checking each of |
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.
Lgtm
@DamonFool 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:
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 1 new commit pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
(Just trying out what happens if I try to issue a test request) /test |
Thanks @tschatzl for your review. @kimbarrett , are you OK with this change? |
@@ -243,15 +252,17 @@ static size_t calc_min_yellow_zone_size() { | |||
|
|||
static size_t calc_init_green_zone() { | |||
size_t green = G1ConcRefinementGreenZone; | |||
char* name = (char*) "G1ConcRefinementGreenZone"; |
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.
Change the type of name to const char*
and eliminate the cast here and on line 258.
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.
Amazing!
It gets compiled without the casts by just adding 'const'.
Updated. Thanks.
/test |
@DamonFool you need to get approval to run the tests in tier1 for commits up until efa0946 |
/integrate |
@DamonFool Since your change was applied there has been 1 commit pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit f2a0988. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi all,
SIGFPE was observed by running:
The reason is that buffers_to_cards [1] returns 0 for 'step' due to overflow.
It would be better to add overflow check logic is it.
Testing:
Thanks.
Best regards,
Jie
[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp#L235
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/1489/head:pull/1489
$ git checkout pull/1489