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

8214976: Warn about uses of functions replaced for portability #8982

Closed
wants to merge 5 commits into from

Conversation

kimbarrett
Copy link

@kimbarrett kimbarrett commented Jun 1, 2022

Please review this new mechanism for "poisoning" uses of certain functions,
along with marking specific uses as still being permitted. One use for this
is to discourage use of external library functions for which HotSpot provides
extended versions or portability shims. Another is to prevent uses of certain
functions considered "insecure", promoting alternatives or requiring strong
scrutiny.

The new mechanism is defined in compilerWarnings.hpp, with compiler-specific
implementations. Comments in the code describe the details. For some
compilers we didn't find any features that would permit such poisoning. As a
result, only shared code has complete coverage.

For gcc and clang we use an attribute that warns about references to an
annotated function, and scoped suppression of that warning. This
functionality is provided since gcc9 and clang14. (I haven't tested with
clang; version 14 is more recent than I have access for. But the docs
describe an explicit match to the gcc functionality.)

For Visual Studio I was unable to find a workable poisoning mechanism. Using
deprecation annotations and scoped suppression of the consequent warnings
seemed plausible, but ran into problems. (See comments in code for details.)
But we provide scoped suppression of deprecation warnings anyway. Many of the
functions we'd poison are already conditionally deprecated under the control
of macros like _CRT_SECURE_NO_WARNINGS and _CRT_NONSTDC_NO_DEPRECATE. Once
we've removed or annotated all calls associated with one of those macros we
can remove the macro from our builds, preventing new uses from creeping into
unshared code.

The aix-ppc port doesn't provide poisoning support. The version of clang used
by xlclang is older than the warning attribute and suppression.

Along with the new mechanisms, a few functions are poisoned by this change.
More functions can be added as folks have time to address their uses.

The poisoning declarations are in globalDefinitions.hpp, since the signatures
may use types that have platform-specific definitions or locations that are
provided within that header. That header is included (directly or indirectly)
nearly everywhere, so provides good coverage.

The rest of the changes are to address existing uses of the newly forbidden
functions, either using something else or annotating them as permitted uses.


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-8214976: Warn about uses of functions replaced for portability

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8982/head:pull/8982
$ git checkout pull/8982

Update a local copy of the PR:
$ git checkout pull/8982
$ git pull https://git.openjdk.java.net/jdk pull/8982/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8982

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8982.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 1, 2022

👋 Welcome back kbarrett! 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
Copy link

openjdk bot commented Jun 1, 2022

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

@openjdk openjdk bot added the hotspot label Jun 1, 2022
@kimbarrett kimbarrett marked this pull request as ready for review Jun 2, 2022
@openjdk openjdk bot added the rfr label Jun 2, 2022
@mlbridge
Copy link

mlbridge bot commented Jun 2, 2022

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Kim,
This seems fine to me.
Thanks.

@openjdk
Copy link

openjdk bot commented Jun 2, 2022

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

8214976: Warn about uses of functions replaced for portability

Reviewed-by: dholmes, tschatzl, coleenp

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jun 2, 2022
coleenp
coleenp approved these changes Jun 8, 2022
Copy link
Contributor

@coleenp coleenp left a comment

Looks good! thank you for this change.

@@ -94,7 +97,7 @@
} \
} \
fprintf(stderr, "OKIDOKI"); \
exit(0); \
gtest_exit_from_child_vm(0); \
Copy link
Contributor

@coleenp coleenp Jun 8, 2022

Choose a reason for hiding this comment

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

Are the backslashes lined up here? Can you line them up, if not? Thanks for the comment why this function.

Copy link
Author

@kimbarrett kimbarrett Jun 8, 2022

Choose a reason for hiding this comment

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

Fixed.

@kimbarrett
Copy link
Author

kimbarrett commented Jun 8, 2022

Thanks for reviews @dholmes-ora , @tschatzl , and @coleenp .

@kimbarrett
Copy link
Author

kimbarrett commented Jun 8, 2022

/integrate

@openjdk-notifier
Copy link

openjdk-notifier bot commented Jun 8, 2022

@kimbarrett Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

@openjdk
Copy link

openjdk bot commented Jun 8, 2022

Going to push as commit 04f02ac.

@openjdk openjdk bot added the integrated label Jun 8, 2022
@openjdk openjdk bot closed this Jun 8, 2022
@openjdk openjdk bot removed ready rfr labels Jun 8, 2022
@openjdk
Copy link

openjdk bot commented Jun 8, 2022

@kimbarrett Pushed as commit 04f02ac.

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

@kimbarrett kimbarrett deleted the disallow-functions branch Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot integrated
4 participants