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

Update Linux makefile to allow compilation with GCC 7.3 #1921

Merged
merged 1 commit into from
May 18, 2018

Conversation

babsingh
Copy link
Contributor

GCC versions greater than 5 default to GNU11 but OpenJ9 uses
GNU89 inline semantics. -fgnu89-inline option is appended to CFLAGS
to allow compilation with GCC versions greater than 5 and GNU89 inline
semantics. Reference - https://gcc.gnu.org/gcc-5/porting_to.html.

JDK11 Linux x86 DockerFile no longer needs to initialize CFLAGS with
-fgnu89-inline since the Linux makefile now accounts for -fgnu89-inline
option if the GCC version is greater than 5.

Signed-off-by: Babneet Singh [email protected]

GCC versions greater than 5 default to GNU11 but OpenJ9 uses
GNU89 inline semantics. -fgnu89-inline option is appended to CFLAGS
to allow compilation with GCC versions greater than 5 and GNU89 inline
semantics. Reference - https://gcc.gnu.org/gcc-5/porting_to.html.

JDK11 Linux x86 DockerFile no longer needs to initialize CFLAGS with
-fgnu89-inline since the Linux makefile now accounts for -fgnu89-inline
option if the GCC version is greater than 5.

Signed-off-by: Babneet Singh <[email protected]>
@babsingh
Copy link
Contributor Author

Compilation passes on xLinux, pLinux and zLinux with gcc-4.8. Compilation passes on xLinux with gcc-7.3. Compile failures related to OpenJDK source code seen when compiling on pLinux with gcc-7.3. No zLinux machine available to compile with gcc-7.3.

@babsingh
Copy link
Contributor Author

This pull request solves the error: inline function $INLINED_FUNCTION_NAME declared but never defined when using GCC 7.3. Reference: #1684. This error was first seen in multiple places within trj9/runtime/MethodMetaData.hMethodMetaData.h (JIT source). GNU89 inline semantics are used within OpenJ9 but GCC 5+ defaults to GNU11 semantics. To allow compilation with GCC 5+ and GNU89 inline semantics, -fgnu89-inline is appended to CFLAGS.

For using this approach, I would like to request approval from JIT members: @mpirvu, @fjeremic, @andrewcraik since JIT source code is impacted by the compile error.

@babsingh
Copy link
Contributor Author

fyi - @pshipton

to allow compilation with GCC versions greater than 5 and GNU89 inline
semantics. Reference - https://gcc.gnu.org/gcc-5/porting_to.html.
-->
GCCVERSIONGTEQ5 := $(shell expr `gcc -dumpversion | cut -f1 -d.` \>= 5)
Copy link
Member

Choose a reason for hiding this comment

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

What happens when gcc 10 ships? (Not a blocker for merging this now but something to think about while we're looking at this or we'll have to rediscover it later)

Copy link
Contributor Author

@babsingh babsingh May 16, 2018

Choose a reason for hiding this comment

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

  1. gcc -dumpversion | cut -f1 -d. will evaluate to 10
  2. expr 10 \>= 5 will evaluate to 1
  3. GCCVERSIONGTEQ5 = 1
  4. if GCCVERSIONGTEQ5 == 1 then CFLAGS+=-fgnu89-inline

We want to use -fgnu89-inline for GCC versions >= 5 since they will default to GNU11 semantics.

Copy link
Member

Choose a reason for hiding this comment

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

I misread the cut command.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should not be invoking gcc, but instead checking if we're using the gcc toolchain and only then invoking $(CC) -dumpversion. A system has at most one tool called 'gcc', but the user may have configured their build to use a different version; the test should be based on the selected compiler.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll put together a PR to address my previous comment.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. Thanks @keithc-ca

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @keithc-ca

Copy link
Contributor

Choose a reason for hiding this comment

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

#2205 proposes a fix for this.

Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

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

LGTM

@DanHeidinga
Copy link
Member

Jenkins test sanity xlinux

Copy link
Contributor

@andrewcraik andrewcraik left a comment

Choose a reason for hiding this comment

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

LGTM

@DanHeidinga DanHeidinga merged commit d60a605 into eclipse-openj9:master May 18, 2018
@babsingh babsingh deleted the jdk11_gcc73_v1 branch August 17, 2020 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants