JDK-8312395: Improve assertions in growableArray #14946

MBaesken · 2023-07-20T07:43:51Z

There are a number of assertions in growableArray , for example to check for a valid index to access/remove elements.
Those assertions can be improved, e.g. by showing the bad index value used in case of failure.

Example for an assertion in the 'at(index i)' access method
old assertion looks like
assert(0 <= i && i < _len) failed: illegal index

new assertion looks like
assert(0 <= i && i < _len) failed: illegal index -559030609, 1 accessible elements

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

JDK-8312395: Improve assertions in growableArray (Bug - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to d4e3ba68
Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to d4e3ba68

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14946

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-07-20T07:44:55Z

👋 Welcome back mbaesken! 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 · 2023-07-20T07:46:40Z

@MBaesken 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.

mlbridge · 2023-07-20T07:50:49Z

Webrevs

dholmes-ora

Approval in principle but I have a suggestion.

Thanks

dholmes-ora · 2023-07-20T07:56:55Z

src/hotspot/share/utilities/growableArray.hpp

@@ -142,17 +142,17 @@ class GrowableArrayView : public GrowableArrayBase {
  }

  E& at(int i) {
-    assert(0 <= i && i < _len, "illegal index");
+    assert(0 <= i && i < _len, "illegal index %d, %d accessible elements", i, _len);


How about:

"Illegal index %d for length %d"

?

That sounds like a good suggestions !

MBaesken · 2023-07-20T08:46:14Z

I adjusted the output following your suggestion and also added some output to a few more asserts.

Btw any idea why so much 'this->_len' syntax is used at some places and not just _len or other field names ?

tstuefe

Looks good!

openjdk · 2023-07-20T09:02:29Z

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

8312395: Improve assertions in growableArray

Reviewed-by: dholmes, 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 4 new commits pushed to the master branch:

8d29329: 8312320: Remove javax/rmi/ssl/SSLSocketParametersTest.sh from ProblemList
94eb44b: 8312394: [linux] SIGSEGV if kernel was built without hugepage support
3283328: 8311130: AArch64: Sync SVE related CPU features with VM options
a742767: 8312246: NPE when HSDB visits bad oop

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.

tstuefe · 2023-07-20T09:03:11Z

looks

I adjusted the output following your suggestion and also added some output to a few more asserts.

Btw any idea why so much 'this->_len' syntax is used at some places and not just _len or other field names ?

Probably some IDE-code-completion-related artifacts. E.g. typing "this->" in CDT gives me all members, and maybe someone just hit Enter then and took the full completion

dholmes-ora

One further suggestion but approving anyway.

I think the use of this->xxx is just personal style. I find it unnecessary especially given the other uses in this file.

Thanks.

dholmes-ora · 2023-07-20T10:27:57Z

src/hotspot/share/utilities/growableArray.hpp

-    assert(0 <= start, "illegal index");
-    assert(start < end && end <= _len, "erase called with invalid range");
+    assert(0 <= start, "illegal start index %d", start);
+    assert(start < end && end <= _len, "erase called with invalid range %d, %d", start, end);


I think you'd want to see _len here too - suggestion:

"erase called with invalid range (%d, %d) for length %d", start, end, _len

Hi David, I agree !

tstuefe · 2023-07-20T11:22:30Z

I think the use of this->xxx is just personal style. I find it unnecessary especially given the other uses in this file.

Interestingly enough I get compiler errors when leaving out "this->" (GCC 10.3). It may be because the member is part of a templatized base class. Odd, though.

Thanks.

MBaesken · 2023-07-20T15:05:58Z

Hi David and Thomas, thanks for the reviews !

/integrate

openjdk · 2023-07-20T15:07:03Z

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

9fa944e: 8312019: Simplify and modernize java.util.BitSet.equals
fe41910: 8312459: Problem list java/awt/GraphicsDevice/DisplayModes/CycleDMImage.java for macOS
8d29329: 8312320: Remove javax/rmi/ssl/SSLSocketParametersTest.sh from ProblemList
94eb44b: 8312394: [linux] SIGSEGV if kernel was built without hugepage support
3283328: 8311130: AArch64: Sync SVE related CPU features with VM options
a742767: 8312246: NPE when HSDB visits bad oop

Your commit was automatically rebased without conflicts.

openjdk · 2023-07-20T15:07:09Z

@MBaesken Pushed as commit b772e67.

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

kimbarrett · 2023-07-20T15:30:42Z

I think the use of this->xxx is just personal style. I find it unnecessary especially given the other uses in this file.

Interestingly enough I get compiler errors when leaving out "this->" (GCC 10.3). It may be because the member is part of a templatized base class. Odd, though.

Qualification with this-> is needed in a bunch of places here because of the
C++ name lookup rules. Unqualified name lookup does not search dependent base
classes. For example, in GrowableArrayWithAllocator an unqualified use of
_len won't find that member in the base class, but will instead (probably)
fail to find it in namespace scope during phase1 (pre-instantiation) template
checking lookup (of 2 phase lookup), and so fail to compile. It isn't possible
to resolve it in phase1 because the dependent base class definition isn't
known until instantiation.

dholmes-ora · 2023-07-20T21:41:35Z

Thanks @kimbarrett !

JDK-8312395

16d7fab

openjdk bot added the rfr Pull request is ready for review label Jul 20, 2023

openjdk bot added the hotspot hotspot-dev@openjdk.org label Jul 20, 2023

dholmes-ora suggested changes Jul 20, 2023

View reviewed changes

adjust some output and some asserts

d4e3ba6

tstuefe approved these changes Jul 20, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 20, 2023

dholmes-ora approved these changes Jul 20, 2023

View reviewed changes

improve assert in remove_range

b9e7e49

openjdk bot added the integrated Pull request has been integrated label Jul 20, 2023

openjdk bot closed this Jul 20, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8312395: Improve assertions in growableArray #14946

JDK-8312395: Improve assertions in growableArray #14946

MBaesken commented Jul 20, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 20, 2023

openjdk bot commented Jul 20, 2023

mlbridge bot commented Jul 20, 2023 •

edited

Loading

dholmes-ora left a comment

dholmes-ora Jul 20, 2023

MBaesken Jul 20, 2023

MBaesken commented Jul 20, 2023

tstuefe left a comment

openjdk bot commented Jul 20, 2023 •

edited

Loading

tstuefe commented Jul 20, 2023

dholmes-ora left a comment

dholmes-ora Jul 20, 2023

MBaesken Jul 20, 2023

tstuefe commented Jul 20, 2023

MBaesken commented Jul 20, 2023

openjdk bot commented Jul 20, 2023

openjdk bot commented Jul 20, 2023

kimbarrett commented Jul 20, 2023

dholmes-ora commented Jul 20, 2023

JDK-8312395: Improve assertions in growableArray #14946

JDK-8312395: Improve assertions in growableArray #14946

Conversation

MBaesken commented Jul 20, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 20, 2023

openjdk bot commented Jul 20, 2023

mlbridge bot commented Jul 20, 2023 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Jul 20, 2023

Choose a reason for hiding this comment

MBaesken Jul 20, 2023

Choose a reason for hiding this comment

MBaesken commented Jul 20, 2023

tstuefe left a comment

Choose a reason for hiding this comment

openjdk bot commented Jul 20, 2023 • edited Loading

tstuefe commented Jul 20, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Jul 20, 2023

Choose a reason for hiding this comment

MBaesken Jul 20, 2023

Choose a reason for hiding this comment

tstuefe commented Jul 20, 2023

MBaesken commented Jul 20, 2023

openjdk bot commented Jul 20, 2023

openjdk bot commented Jul 20, 2023

kimbarrett commented Jul 20, 2023

dholmes-ora commented Jul 20, 2023

MBaesken commented Jul 20, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 20, 2023 •

edited

Loading

openjdk bot commented Jul 20, 2023 •

edited

Loading