RFR: 8295146: Clean up native code with newer C/C++ language features

[David Holmes]

On Thu, 10 Nov 2022 06:20:41 GMT, Julian Waters <jwaters at openjdk.org> wrote:

> After [JDK-8292008](https://bugs.openjdk.org/browse/JDK-8292008) and [JDK-8247283](https://bugs.openjdk.org/browse/JDK-8247283), some C and C++ code across the JDK can be replaced and simplified with cleaner language features that were previously not available due to required compatibility with the now unsupported Visual C++ 2017 compiler. These cleanups were highlighted by the very briefly integrated 8296115
> 
> No changes to the behaviour of the JDK has resulted in any way from this commit

This looks good in general. It is a pity there is so much simple moving of where "attributes" are listed, as it makes it look like the changes are more extensive than they really are - that said I prefer to see the attributes appear before a function/method signature rather than after, or somewhere in-between.

A few other comments below.

Thanks.

make/autoconf/flags-cflags.m4 line 632:

> 630:   if test "x$TOOLCHAIN_TYPE" = xgcc || test "x$TOOLCHAIN_TYPE" = xclang; then
> 631:     STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections -fdata-sections \
> 632:       -DJNIEXPORT='[[gnu::visibility(\"hidden\")]]'"

So IIUC we now use attributes via the C++11 syntax rather than compiler-specific syntax - even where the C++11 syntax is referring to a compiler specific attribute. Is that right?

src/hotspot/os/linux/os_perf_linux.cpp line 233:

> 231:  *  Ensure that 'fmt' does _NOT_ contain the first two "%d %s"
> 232:  */
> 233: SCANF_ARGS(2, 0) static int vread_statdata(const char* procfile, _SCANFMT_ const char* fmt, va_list args) {

If `SCANF_ARGS` can/must come first then I suggest adding a newline after it so the method signature is easier to spot. Applied everywhere of course.

src/hotspot/os/windows/os_windows.hpp line 35:

> 33: class Thread;
> 34: 
> 35: static unsigned __stdcall thread_native_entry(Thread*);

Why was this removed? This is needed to correctly specify the call sequence for the thread entry routine when used with `_beginThreadex`:
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/beginthread-beginthreadex?view=msvc-170

src/hotspot/share/cds/filemap.hpp line 482:

> 480: 
> 481:   // Errors.
> 482:   ATTRIBUTE_PRINTF(1, 2) static void fail_stop(const char *msg, ...);

Again I suggest a newline after `ATTRIBUTE_PRINTF`

src/hotspot/share/utilities/compilerWarnings.hpp line 47:

> 45: #endif
> 46: 
> 47: #ifndef PRAGMA_DISABLE_VISCPP_WARNING

Why rename this from `MSVC` to `VISCPP`? IIRC the full name is Microsft Visual Studio C++, so you new name is not obviously better and the change just adds noise to the PR. Further `MSVC` matches what MS themselves use and even the attribute namespace in C++11 is `MSVC`.
Update: I see the inconsistency with `compilerWarnings_visCPP.hpp`

src/hotspot/share/utilities/compilerWarnings_gcc.hpp line 37:

> 35: #endif
> 36: 
> 37: #if defined(__clang_major__) && \

Not clear why this was moved ??

src/hotspot/share/utilities/debug.hpp line 172:

> 170: void report_fatal(VMErrorType error_type, const char* file, int line, const char* detail_fmt, ...) ATTRIBUTE_PRINTF(4, 5);
> 171: void report_vm_out_of_memory(const char* file, int line, size_t size, VMErrorType vm_err_type,
> 172:                              const char* detail_fmt, ...) ATTRIBUTE_PRINTF(5, 6);

Why were the ATTRIBUTE_PRINTFs removed?

-------------

PR: https://git.openjdk.org/jdk/pull/11081