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 #6961

Closed
wants to merge 97 commits into from

Conversation

hseigel
Copy link
Member

@hseigel hseigel commented Jan 4, 2022

Please review this change for JDK-8214976. This change adds attribute warnings to header file compilerWarnings.hpp so that compilation warnings get issued when certain system functions are called directly, instead of hotspot's os:: versions of the functions. Many additional files were changed because of compilation warnings resulting from the compilerWarnings.hpp changes.

A sample warning is:

.../open/test/hotspot/gtest/logging/test_log.cpp:63:19: error: call to 'fopen' declared with attribute warning: use os::fopen [-Werror=attribute-warning]
   63 |   FILE* fp = fopen(TestLogFileName, "r");
      |              ~~~~~^~~~~~~~~~~~~~~~~~~~~~

Note that changing src/hotspot/os/linux/gc/z/zMountPoint_linux.cpp to call os:: functions requires adding "#include "runtime/os.hpp" and caused test gc/z/TestAllocateHeapAt.java to fail. So, for now, I just added PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION to zMountPoint_linux.cpp. There's a similar issue with gtest/logging/test_logDecorators.cpp.

Attribute warnings for additional functions, such as malloc(), were not included in this change because they require lots of source code changes.

This change was tested by running mach5 tiers 1-2 on Linux, Mac OS, and Windows, Mach5 tiers 3-5 on Linux x64. Also, builds were done on Linux-zero, Linux-s390, and Linux-ppc.

Thanks, Harold


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8214976: Warn about uses of functions replaced for portability

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6961

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

Using diff file

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

William Kemper and others added 6 commits Dec 28, 2021
…exception_handlers()) failed: no exception handler expected" after JDK-8275638

Reviewed-by: rbackman, vlivanov
Reviewed-by: erikj, clanger
…ct' value for scalar-replaced boxed vector but got: NULL

Reviewed-by: kvn, thartmann
@bridgekeeper
Copy link

bridgekeeper bot commented Jan 4, 2022

👋 Welcome back hseigel! 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 added the rfr label Jan 4, 2022
@openjdk
Copy link

openjdk bot commented Jan 4, 2022

@hseigel 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 Jan 4, 2022
@mlbridge
Copy link

mlbridge bot commented Jan 4, 2022

Webrevs

merykitty and others added 3 commits Jan 4, 2022
@@ -108,6 +108,8 @@
#include <sys/utsname.h>
#include <sys/vminfo.h>

PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(close); // prevents compiler warnings for all functions

Choose a reason for hiding this comment

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

Why suppress the warning for this file? I think there are only 3 calls to close, so it seems like they could just be fixed.

@@ -107,6 +107,8 @@
#define MAP_ANONYMOUS MAP_ANON
#endif

PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(close); // prevents compiler warnings for all functions

Choose a reason for hiding this comment

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

Again here, why suppress the warning rather than fix the small number of calls in this file.

src/hotspot/os/linux/gc/z/zMountPoint_linux.cpp Outdated Show resolved Hide resolved
@@ -150,6 +150,8 @@ enum CoredumpFilterBit {
DAX_SHARED_BIT = 1 << 8
};

PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(closedir); // prevents compiler warning for all functions

Choose a reason for hiding this comment

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

Why suppress this warning rather than fix the one call. Note the closedir in os::dir_is_empty is in the os "namespace" and hence is already calling the os wrapper. It could be explicitly qualified anyway, if that seems confusing.

@@ -90,6 +90,8 @@
#define assert_with_errno(cond, msg) check_with_errno(assert, cond, msg)
#define guarantee_with_errno(cond, msg) check_with_errno(guarantee, cond, msg)

PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(closedir); // prevents compiler warnings for all functions

Choose a reason for hiding this comment

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

This should be scoped over the one use, in the implementation of os::closedir, rather than applied to the whole file.

src/hotspot/share/jvmci/jvmciCodeInstaller.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/os.cpp Outdated Show resolved Hide resolved
test/hotspot/gtest/logging/test_logDecorators.cpp Outdated Show resolved Hide resolved
#define PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(name) \
PRAGMA_DISABLE_GCC_WARNING("-Wattribute-warning")

FORBID_C_FUNCTION(void abort(void), "use os::abort");

Choose a reason for hiding this comment

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

It would be better to put all of these after all the #endif, so that if we add macro implementations for other platforms (like windows), these will be covered by the additional platforms.

Copy link
Member Author

@hseigel hseigel Jan 5, 2022

Choose a reason for hiding this comment

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

Done.

Choose a reason for hiding this comment

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

"after all the #endif" wasn't quite what I meant. Still needs to be inside the include guard. And I forgot there are platform-specific compilerWarnings files; the gcc macro definitions should be in compilerWarnings_gcc.hpp rather than conditionally defined here. This file should contain something like

#ifndef FORBID_C_FUNCTION
#define FORBID_C_FUNCTION(signature, alternative)
#endif

#ifndef PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION
#define PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(name)
#endif

Choose a reason for hiding this comment

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

Some of these functions are posix while some are portable (C StdLib). Indeed,
some of the os:: functions exist to provide a shared API with Windows where
none exists natively.

So I think compilerWarnings.hpp should have forbid-decls for only portable
names, and there should be another group (in another file? not sure how to set
it up though. for now just conditional in compilerWarnings.hpp?) for the
posix functions that aren't present in Windows. The additional #includes
should be separated accordingly, as needed.

But need to be careful about <dirent.h>. There's a problem with that header
on XLC. See comment in globalDefinitions_xlc.hpp. Ugh!

FORBID_C_FUNCTION(char *strtok(char*, const char*), "use strtok_r");

#else

Choose a reason for hiding this comment

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

I think, but have not tested it, that this facility can be implemented for Visual Studio using __declspec(deprecated) and suppressing warning C4996. Of course, doing that may trigger a bunch of warnings in Windows-specific files, so it might be best to do that as a followup change.

Copy link
Member Author

@hseigel hseigel Jan 5, 2022

Choose a reason for hiding this comment

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

Addressing this as a follow up change sounds good.

shipilev and others added 3 commits Jan 5, 2022
…8276660

Co-authored-by: Valerie Peng <valeriep@openjdk.org>
Reviewed-by: alanb, valeriep
Reviewed-by: aph, erikj
@dholmes-ora
Copy link
Member

dholmes-ora commented Jan 5, 2022

Hi Harold,

As I wrote in the bug report:

Just be mindful that this really only applies when shared code calls these functions. In os specific code it may be necessary/desirable to call the platform specific functions rather than the generic OS:: version.

We do not need to go through the os:: portability layer when already in os-specific code. The only time we need to use the os:: layer in that case is when the os:: layer adds additional semantics (like NMT) that we want.

@mlbridge
Copy link

mlbridge bot commented Jan 5, 2022

Mailing list message from Kim Barrett on hotspot-dev:

On Jan 5, 2022, at 7:34 AM, David Holmes <dholmes at openjdk.java.net> wrote:
As I wrote in the bug report:

Just be mindful that this really only applies when shared code calls these functions. In os specific code it may be necessary/desirable to call the platform specific functions rather than the generic OS:: version.

We do not need to go through the os:: portability layer when already in os-specific code. The only time we need to use the os:: layer in that case is when the os:: layer adds additional semantics (like NMT) that we want.

Calls that bypass the os:: portability layer require suppressing the warning for bypassing the portability layer.
I think in very nearly all such cases it?s better to stick with the portability layer, the obvious contrary case
being the implementation of the portability function in terms of the underlying ?native? function.

@hseigel
Copy link
Member Author

hseigel commented Jan 5, 2022

Hi Kim, David,
Should I remove FORBID_C_FUNCTION(char strdup(const char), from this change and revert the strdup changes? A future bug fix would handle FORBID_C_FUNCTION for strdup(), malloc() and free() together in one change?
Thanks, Harold

@hseigel
Copy link
Member Author

hseigel commented Jan 5, 2022

Hi Kim,

I don't think that FORBIDDEN_C_FUNCTION should be enforced for files os_aix.cpp, os_bsd.cpp, os_posix.cpp, and os_windows.cpp. These are platform specific files that intentionally call multiple native functions. For example, os_linux.cpp calls close_dir() fopen(), close(), readdir(), write(), lseek(), etc.

Since PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION ignores its argument and disables the warning for all native calls, I think it's reasonable to just specify PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION once at the beginning of the above files and not clutter them with many PRAGMA_DIAG_PUSH/POP's.

Thanks, Harold

@mlbridge
Copy link

mlbridge bot commented Jan 5, 2022

Mailing list message from Kim Barrett on hotspot-dev:

On Jan 5, 2022, at 10:32 AM, Harold Seigel <hseigel at openjdk.java.net> wrote:
Should I remove FORBID_C_FUNCTION(char *strdup(const char*), from this change and revert the strdup changes? A future bug fix would handle FORBID_C_FUNCTION for strdup(), malloc() and free() together in one change?

That seems like a good idea to me.

@mlbridge
Copy link

mlbridge bot commented Jan 5, 2022

Mailing list message from Kim Barrett on hotspot-dev:

On Jan 5, 2022, at 11:16 AM, Harold Seigel <hseigel at openjdk.java.net> wrote:
I don't think that FORBIDDEN_C_FUNCTION should be enforced for files os_aix.cpp, os_bsd.cpp, os_posix.cpp, and os_windows.cpp. These are platform specific files that intentionally call multiple native functions. For example, os_linux.cpp calls close_dir() fopen(), close(), readdir(), write(), lseek(), etc.

I disagree. I think there are or will be functions we want to just forbid outright, and others we want
to forbid outside the implementation of the os:: wrapper because we consistently want some added
feature of the portability wrapper (such as asserts or common argument manipulation or whatever).

Since PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION ignores its argument and disables the warning for all native calls, I think it's reasonable to just specify PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION once at the beginning of the above files and not clutter them with many PRAGMA_DIAG_PUSH/POP's.

I disagree.

That behavior of the gcc version of the pragma is an artifact of its current implementation, and
not the desired behavior. I couldn?t find a way to limit the disabling to the identifier in the argument
with gcc. That doesn?t mean that won?t be possible for other platforms, or for a future version of
gcc, or for someone more clever than me.

Even with the limited gcc implementation the argument documents intent.

The purpose of the pragma is to permit the os:: portability wrapper for a function to be implemented
in terms of the corresponding native function. If it weren?t for that requirement I wouldn?t want the
pragma to exist at all.

Anton Tarasov and others added 2 commits Jan 5, 2022
shipilev and others added 27 commits Jan 11, 2022
…lt to empty string

Reviewed-by: dholmes, sspitsyn, alanb
Reviewed-by: jbhateja, sviswanathan, kvn
… to vpopcntdq is not supported

Reviewed-by: kvn
Reviewed-by: tschatzl, kbarrett, sjohanss
…le.getParameterTypes() in trusted code

Reviewed-by: redestad
@hseigel
Copy link
Member Author

hseigel commented Jan 12, 2022

This pull request is being withdrawn do to repo corruption.

@hseigel hseigel closed this Jan 12, 2022
@openjdk
Copy link

openjdk bot commented Jan 12, 2022

⚠️ @hseigel This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot rfr