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.

Sign up for GitHub

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

Jump to bottom

Fix -Wnonnull warnings with GCC 11 #7403

Merged
merged 1 commit into from
Sep 17, 2024
Merged

Fix -Wnonnull warnings with GCC 11 #7403

merged 1 commit into from
Sep 17, 2024

Conversation

cjjdespres
Copy link
Contributor

Version 11 and later of GCC consider the implicit this parameter of nonstatic member functions to be annotated with nonnull for the purposes of the -Wnonnull warning. This change generates new warnings that are resolved here with the use of TR_ASSERT_FATAL on the objects that the compiler thinks could be NULL. The macro TR_ASSERT is insufficient, as the TR::assertion method isn't (and cannot be) annotated with OMR_NORETURN.

Fixes: #7402

@cjjdespres
Copy link
Contributor Author

cjjdespres commented Jul 9, 2024 •
edited
Loading

OpenJ9 itself appears to build fully with -DDEBUG with these changes. The full JDK21 build still fails with -DDEBUG, sadly, because of an assert in openj9 that's encountered when building the rest of the JDK image. I'll put up an issue over there with the details.

@cjjdespres
Copy link
Contributor Author

A partial alternate solution would be to remove TR_IgnoreAssert. That would allow the asserts changed in this PR to remain non-fatal because we could then annotate TR::assertion() (and maybe Compilation::failCompilation() while we're at it) with OMR_NORETURN. How much is that option used?

I think that would improve warnings under -DDEBUG in the long run.

@cjjdespres
Copy link
Contributor Author

I suppose TR_IgnoreAssert could be guarded by a new compilation flag that's off by default, so a debug build with non-fatal asserts turned off could still be built.

@cjjdespres
Copy link
Contributor Author

@mpirvu These were the few lines in omr/ I mentioned that needed to change to build with -DDEBUG with GCC 11.

0xdaryl
0xdaryl reviewed Sep 12, 2024
View reviewed changes
compiler/optimizer/CopyPropagation.cpp Outdated
@@ -1230,7 +1230,7 @@ void TR_CopyPropagation::replaceCopySymbolReferenceByOriginalIn(TR::SymbolRefere
if (origNode->getNumChildren() == 2)
{
twoChildrenCase = true;
TR_ASSERT(0, "Only in WCode we can add extra children\n");
TR_ASSERT_FATAL(0, "Only in WCode we can add extra children\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

The fatal assert message should be re-written to be "Cannot add extra children". The WCode message is no longer relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Maybe the code handling the twoChildrenCase should be removed in future?

compiler/optimizer/CopyPropagation.cpp Outdated
@@ -1230,7 +1230,7 @@ void TR_CopyPropagation::replaceCopySymbolReferenceByOriginalIn(TR::SymbolRefere
if (origNode->getNumChildren() == 2)
{
twoChildrenCase = true;
TR_ASSERT(0, "Only in WCode we can add extra children\n");
TR_ASSERT_FATAL(0, "Only in WCode we can add extra children\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I follow how making this assert fatal resolves the problem since it does not operate on the nullness of a particular pointer (only "0").

For the record, I think this should be a fatal assert as you have it, but I'm not clear how this resolves the warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I should have explained. I'd have to recompile to be sure, but from what I recall, the actual compilation error was a little down from here, in one of the if (twoChildrenCase) blocks. (I think the dumpOptDetails call might have been the source). This is the only place where twoChildrenCase can be set to true, so the fatal assert makes those blocks unreachable.

@cjjdespres
Fix -Wnonnull warnings with GCC 11
c40a511 
Version 11 and later of GCC consider the implicit `this` parameter of
nonstatic member functions to be annotated with `nonnull` for the
purposes of the `-Wnonnull` warning. This change generates new warnings
that are resolved here with the use of `TR_ASSERT_FATAL` on the objects
that the compiler thinks could be `NULL`. The macro `TR_ASSERT` is
insufficient, as the `TR::assertion` method isn't (and cannot be)
annotated with `OMR_NORETURN`.

Signed-off-by: Christian Despres <despresc@ibm.com>
@0xdaryl
Copy link
Contributor

0xdaryl commented Sep 16, 2024

Jenkins build all

@0xdaryl 0xdaryl self-assigned this Sep 17, 2024
@0xdaryl 0xdaryl merged commit e136a7a into eclipse:master Sep 17, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xdaryl 0xdaryl 0xdaryl approved these changes

@vijaysun-omr vijaysun-omr Awaiting requested review from vijaysun-omr vijaysun-omr is a code owner

@mstoodle mstoodle Awaiting requested review from mstoodle mstoodle is a code owner

Assignees

@0xdaryl 0xdaryl

Labels
comp:compiler
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Debug builds are broken with -Werror in GCC 11 due to -Wnonnull changes
2 participants
@cjjdespres @0xdaryl