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

8296115: Allow for compiling the JDK with strict standards conformance #10912

Closed
wants to merge 7 commits into from

Conversation

TheShermanTanker
Copy link
Contributor

@TheShermanTanker TheShermanTanker commented Oct 31, 2022

JDK-8241499 proposes to set the -permissive- flag for the Microsoft Visual C++ compiler, to enforce strict standards conformance during compilation, making native code behave more strictly. While adding it to default builds is likely not practical given how much testing is required, as an option it can prove helpful in finding areas of native code that are not conformant to the standard. Instead of applying this to just one compiler, we can also include this for every compiler that has support for such a strict mode, which this change does.


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

Issues

  • JDK-8296115: Allow for compiling the JDK with strict standards conformance
  • JDK-8241499: Enable new "permissive-" for standard C++ compliance on Visual Studio if possible

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10912

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

Using diff file

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

@TheShermanTanker
Copy link
Contributor Author

/issue 8241499

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 31, 2022

👋 Welcome back jwaters! 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 changed the title 8296115 8296115: Allow for compiling the JDK with strict standards conformance Oct 31, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 31, 2022
@openjdk
Copy link

openjdk bot commented Oct 31, 2022

@TheShermanTanker
Adding additional issue to issue list: 8241499: Enable new "permissive-" for standard C++ compliance on Visual Studio if possible.

@openjdk
Copy link

openjdk bot commented Oct 31, 2022

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

  • build

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 build build-dev@openjdk.org label Oct 31, 2022
@mlbridge
Copy link

mlbridge bot commented Oct 31, 2022

Webrevs

@openjdk
Copy link

openjdk bot commented Oct 31, 2022

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

8296115: Allow for compiling the JDK with strict standards conformance
8241499: Enable new "permissive-" for standard C++ compliance on Visual Studio if possible

Reviewed-by: erikj, ihse

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@erikj79, @magicus) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 31, 2022
@TheShermanTanker
Copy link
Contributor Author

Tagging @magicus as well, sorry for the noise

make/autoconf/spec.gmk.in Outdated Show resolved Hide resolved
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.

Looks good to me.

@magicus
Copy link
Member

magicus commented Nov 2, 2022

Goes the GHA test suite pass with this flag? If so, I think it would make sense to turn it on for our GHA builds. That will help raise the bar for future code coming in, and give this option some real testing.

@TheShermanTanker
Copy link
Contributor Author

TheShermanTanker commented Nov 2, 2022

Goes the GHA test suite pass with this flag? If so, I think it would make sense to turn it on for our GHA builds. That will help raise the bar for future code coming in, and give this option some real testing.

Given my experience with these errors I doubt it will work off the bat with some of the particularly strict compilers, such as gcc for instance. I'll leave the decision to enable this flag to someone else for now

@TheShermanTanker
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 2, 2022
@openjdk
Copy link

openjdk bot commented Nov 2, 2022

@TheShermanTanker
Your change (at version 1dd9765) is now ready to be sponsored by a Committer.

@erikj79
Copy link
Member

erikj79 commented Nov 2, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Nov 2, 2022

Going to push as commit a124d8e.

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

openjdk bot commented Nov 2, 2022

@erikj79 @TheShermanTanker Pushed as commit a124d8e.

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

@TheShermanTanker TheShermanTanker deleted the permissive branch November 2, 2022 17:04
@magicus
Copy link
Member

magicus commented Nov 7, 2022

I experimented with enabling this for GHA, and it turned out that there are huge efforts needed to even make the code base compile with this flag turned on. I'm starting to wonder if it was such a great idea to introduce this, after all. I realize I did a poor job when reviewing this to ask if this did compile. Well, I can tell now, it does not. For gcc, it seems practically impossible to ever get it to work, since all hotspot constructs such as

DEBUG_ONLY(if (checkResult) log_trace("Checked"));

which are very common, will result in a warning about a superfluous semicolon in non-debug builds. (And similar for AARCH64_ONLY(...); etc).

For clang, this warning of semi-colon could be turned off separately, which -- combined with some other disabled warnings -- made it possible to compile. However, the code it complained about was in most cases fine, it was just using non-standard gcc or clang extensions to the C++ standard. We really need to raise this to a discussion if this should be considered a problem.

Here is a commit showing most (but not all) of the changes needed to make clang compile on macOS with the new conformance option turned on: 793c57a

I think some of these warnings are definitely legit, like import-preprocessor-directive-pedantic (where the code uses #import instead of #include) or c++17-extensions (we should not include code that is beyond C++14, at least not in this point in time). But most are complaints about benign use of gcc/clang extensions.

And the pedantic modes of gcc is really, really cranky. And it can't be piecewise turned off like clang, so as I said, I doubt this can ever get to work.

(Taggning @kimbarrett since I know you are interested in such questions)

@magicus
Copy link
Member

magicus commented Nov 7, 2022

Furthermore, I'm starting to wonder if it was such a great idea to combine this with the visual studio /permissive-. While the gcc/clang switches are about strict standards compliance, the visual studio switch is more of a "get the VS compiler more in line with other modern compilers", and Microsoft has explicitly warned that this will be enabled, then default, then the only choice, going forward. So this is really something we would want to see enabled all the time.

So, I am more or less proposing that we back this out, and that instead:

  1. we try to enable /permissive- by default for Windows (this will require some code changes first)

  2. we consider if we should raise the bar of "pedantictness" for clang, but with warnings we do not care about being turned off. (An alternative is that we explicitly turn on some selected additional warnings that we think are useful)

  3. we entirely drops the idea that we could ever achieve anything like this for gcc, unless they up their game in having more finely grained warnings

  4. and if you want to experiment with more strict compilation, to reach your goal of compiler/os-independence, you just use the standard method of --with-extra-c-flags to pass -Wpedantict etc.

@TheShermanTanker
Copy link
Contributor Author

I experimented with enabling this for GHA, and it turned out that there are huge efforts needed to even make the code base compile with this flag turned on. I'm starting to wonder if it was such a great idea to introduce this, after all. I realize I did a poor job when reviewing this to ask if this did compile. Well, I can tell now, it does not. For gcc, it seems practically impossible to ever get it to work, since all hotspot constructs such as

DEBUG_ONLY(if (checkResult) log_trace("Checked"));

which are very common, will result in a warning about a superfluous semicolon in non-debug builds. (And similar for AARCH64_ONLY(...); etc).

For clang, this warning of semi-colon could be turned off separately, which -- combined with some other disabled warnings -- made it possible to compile. However, the code it complained about was in most cases fine, it was just using non-standard gcc or clang extensions to the C++ standard. We really need to raise this to a discussion if this should be considered a problem.

Here is a commit showing most (but not all) of the changes needed to make clang compile on macOS with the new conformance option turned on: 793c57a

I think some of these warnings are definitely legit, like import-preprocessor-directive-pedantic (where the code uses #import instead of #include) or c++17-extensions (we should not include code that is beyond C++14, at least not in this point in time). But most are complaints about benign use of gcc/clang extensions.

And the pedantic modes of gcc is really, really cranky. And it can't be piecewise turned off like clang, so as I said, I doubt this can ever get to work.

(Taggning @kimbarrett since I know you are interested in such questions)

Oh dear, I was expecting several issues with gcc but it seems like I grossly underestimated just how strict it would be. I'll back this out and reopen 8241499 if there aren't any objections to that, since this is my mess to clean up. From what I've tested -permissive- didn't really break anything, so it should be trivial to change that to just be on by default instead of through a flag (Testing not considered, at least)

c++17-extensions (we should not include code that is beyond C++14, at least not in this point in time).

There are plans for having code compatibility past C++14? Unlike the permissive switch, from what I've tested a large chunk of java.desktop code is really not happy with anything C++17 or above, so that would be a really big problem if so

@magicus
Copy link
Member

magicus commented Nov 9, 2022

@TheShermanTanker Yes, I think that would be the best way forward. Thank you for taking care of it!

@djelinski
Copy link
Member

FWIW, -permissive- is getting less permissive with every compiler iteration; with MSVC2022 ( C Compiler: Version 19.33.31629) even hotspot doesn't build.

@magicus
Copy link
Member

magicus commented Nov 9, 2022

@djelinski That's a bit alarming. :-/ Do you have any idea if it complains "correctly" on Hotspot by detecting fishy code that should be fixed, or if it is overly cranky and is complaining about reasonable but non-standard extensions?

@TheShermanTanker
Copy link
Contributor Author

/issue remove 8241499

@openjdk
Copy link

openjdk bot commented Nov 9, 2022

@TheShermanTanker The command issue can only be used in open pull requests.

@djelinski
Copy link
Member

@magicus that depends on how you define "cranky"; in default mode, long and int are fully interchangeable. In permissive-, they are distinct types. As a result, compilation of code that assumes that jint is equal to int breaks (on Windows, jint is typedef'd to long).

Example failure:

C:\...\src\hotspot\share\opto\mulnode.cpp(248): error C2668: 'uabs': ambiguous call to overloaded function
C:\...\src\hotspot\share\utilities/globalDefinitions.hpp(1146): note: could be 'unsigned int uabs(int)'
C:\...\src\hotspot\share\utilities/globalDefinitions.hpp(1145): note: or       'julong uabs(jlong)'
C:\...\src\hotspot\share\utilities/globalDefinitions.hpp(1136): note: or       'julong uabs(julong)'
C:\...\src\hotspot\share\utilities/globalDefinitions.hpp(1127): note: or       'unsigned int uabs(unsigned int)'
C:\...\src\hotspot\share\opto\mulnode.cpp(248): note: while trying to match the argument list '(const jint)'

FWIW, clang-cl also fails on this.

@TheShermanTanker
Copy link
Contributor Author

@magicus that depends on how you define "cranky"; in default mode, long and int are fully interchangeable. In permissive-, they are distinct types. As a result, compilation of code that assumes that jint is equal to int breaks (on Windows, jint is typedef'd to long).

Example failure:

C:\...\src\hotspot\share\opto\mulnode.cpp(248): error C2668: 'uabs': ambiguous call to overloaded function
C:\...\src\hotspot\share\utilities/globalDefinitions.hpp(1146): note: could be 'unsigned int uabs(int)'
C:\...\src\hotspot\share\utilities/globalDefinitions.hpp(1145): note: or       'julong uabs(jlong)'
C:\...\src\hotspot\share\utilities/globalDefinitions.hpp(1136): note: or       'julong uabs(julong)'
C:\...\src\hotspot\share\utilities/globalDefinitions.hpp(1127): note: or       'unsigned int uabs(unsigned int)'
C:\...\src\hotspot\share\opto\mulnode.cpp(248): note: while trying to match the argument list '(const jint)'

FWIW, clang-cl also fails on this.

I recall having had to deal with this while trying to wrestle the Windows JDK into being accepted by gcc, the solution was to simply define jint to int instead as it is with the Unix JDK (there were also sizeof checks to typedef jlong to either long or long long, but that's another story). From what I recall back when this was an issue for me this actually worked very well, the only time it choked up was when a jint was compared with a long, which is surprisingly only in rather few sections of native code. The inverse however, trying to make the JDK go with jint being typedef'd as long, is significantly harder

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