RFR: 8256379: Replace SIZE_FORMAT with 'z' length modifier

[Andrew John Hughes]

On Wed, 2 Aug 2023 01:31:43 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

> Replace SIZE_FORMAT with %zu and SIZE_FORMAT_X with 0x%zx using a 4 line sed script:
> 
>   sed -e 's/" SIZE_FORMAT "/%zu/g' -e 's/" SIZE_FORMAT,/%zu",/' \
>       -e 's/print(SIZE_FORMAT "/print("%zu/g' \
>       -e 's/" SIZE_FORMAT_X "/0x%zx/g' -e 's/" SIZE_FORMAT_X,/0x%zx",/g' \
>       -e 's/" SIZE_FORMAT$/%zu"/' \
>        $f >$f.new
> 
> Minor fixups afterwards.  I didn't change SIZE_FORMAT_W(number) because it would have required a better sed script.
> 
> The definitions in globalDefinitions.hpp can be removed with a later PR, once all instances are cleaned out.  This is partial so that people start using %zu instead.
> 
> Tested with tier1 on Oracle supported platforms.

I actually agree with using `%z` for this, but this seems an unusual way to approach it. The `SIZE_FORMAT` is an abstraction which allows the actual format specifier to be anything defined in `globalDefinitions.hpp`. Removing it to hardcode a format thus seems like a bit of a backward step to me.

I've actually had a patch hanging around for the best part of a decade that uses `%z` as the format specifier, but only on s390 (31-bit). As you may know, `size_t` and `int` aren't interchangeable on s390, which used to throw up all sorts of errors with the casts in HotSpot. On that platform, it really is necessary to use `%z` for the size format.

The patch pretty much got dropped on the floor as we didn't build on s390 on newer JDKs and obviously we'd need to start there to get it upstream and then backport. The actual patch was pretty simple though:

~~~
diff --git openjdk.orig/hotspot/src/share/vm/utilities/globalDefinitions.hpp openjdk/hotspot/src/share/vm/utilities/globalDefinitions.hpp
--- openjdk.orig/hotspot/src/share/vm/utilities/globalDefinitions.hpp
+++ openjdk/hotspot/src/share/vm/utilities/globalDefinitions.hpp
@@ -1389,12 +1389,21 @@
 
 #define INTPTR_FORMAT_W(width)   "%" #width PRIxPTR
 
+#if defined(S390) && !defined(_LP64)
+#define SSIZE_FORMAT          "%z"   PRIdPTR
+#define SIZE_FORMAT           "%z"   PRIuPTR
+#define SIZE_FORMAT_HEX       "0x%z" PRIxPTR
+#define SSIZE_FORMAT_W(width) "%"   #width "z" PRIdPTR
+#define SIZE_FORMAT_W(width)  "%"   #width "z" PRIuPTR
+#define SIZE_FORMAT_HEX_W(width) "0x%" #width "z" PRIxPTR
+#else   // !S390
 #define SSIZE_FORMAT          "%"   PRIdPTR
 #define SIZE_FORMAT           "%"   PRIuPTR
 #define SIZE_FORMAT_HEX       "0x%" PRIxPTR
 #define SSIZE_FORMAT_W(width) "%"   #width PRIdPTR
 #define SIZE_FORMAT_W(width)  "%"   #width PRIuPTR
 #define SIZE_FORMAT_HEX_W(width) "0x%" #width PRIxPTR
+#endif  // S390
 
 #define INTX_FORMAT           "%" PRIdPTR
 #define UINTX_FORMAT          "%" PRIuPTR
~~~

plus a bunch of cases where the wrong format was being used in the `printf` statements (likely resolved now anyway; this was against 8u)

I think you could do something similar and include backwards compatibility by detecting whether `%z` is usable during `configure` and then adding a similar `#ifdef` but based on something defined by `configure`.

I'd be happy to propose something if you don't already have an alternate solution in the works.

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

PR Comment: https://git.openjdk.org/jdk/pull/15115#issuecomment-1664765034