[9] RFR(L): 8037816 : Fix for 8036122 breaks build with Xcode5/clang

[Coleen Phillimore]

These PRAGMA_FORMAT_NONLITERAL_IGNORED macros are gross.  I can't 
believe they complain if the format string is const char*.   Can you 
replace some like the ones in metaspace.cpp at least with macros?

I didn't see any others that could be replaced with macros.

Overall, I think (don't take this wrong) this doesn't look as bad as I 
was afraid it would.

Thanks,
Coleen

On 4/17/14, 4:16 PM, David Chase wrote:
> BUG: recent changes to enabled warnings cause clang builds on Mavericks to fail.
> Format checking now complains when a non-constant string is used as a format.
>
> WEBREV: http://cr.openjdk.java.net/~drchase/8037816/webrev.00/
>
> FIX: This patch is adapted from Gerard Ziemski’s patch for XCode5 on Mavericks.
> It seemed to me that the "right" way to do this was not to disable the checking with
> pragmas that we had just enabled on the command line, but rather to correct the
> problems as much as possible.  I attempted to even enhance the checking by
> decorating some internal printf-like methods with the format-printf attribute,
> but was not able to deal with the spew of errors from newer versions of gcc.
> I did, however, manage to get it clean on Mavericks with those attributes enabled,
> so the problem has been reduced to dealing with one compiler's warnings.
>
> The patch includes four sorts of changes:
>
> (1) printf-like functions and methods that are marked as printf-like will no longer trigger complaints about their internal uses of format strings.  A macro conditional on platform was introduced for this.
> ATTRIBUTE_PRINTF(fmt,vargs)
>
> (2) The warning can be disabled on a case-by-case basis; a family of macros conditional on platform were introduced for this.
> PRAGMA_FORMAT_NONLITERAL_IGNORED
> PRAGMA_DIAG_PUSH
> PRAGMA_DIAG_POP
> PRAGMA_FORMAT_NONLITERAL_IGNORED_EXTERNAL
> PRAGMA_FORMAT_NONLITERAL_IGNORED_INTERNAL
> The last two are for use with older versions of gcc that do not support PUSH and POP, in combination with large methods containing many formatting calls -- newer gcc compilers will get sharper checking.
>
> (3) In many cases, the best fix is to replace a call to a method expecting a format with one to a method expecting only a string.  That is, replace “print(s)” with “print_raw(s)”.  Note that when there is a single parameter this cannot possibly be a mistake.
>
> (4) Use new P2I macro to clean up some of the Linux format warnings; this was feasible for some of the printf-like internal methods that were not called too often with technically-mismatched types and format specifiers.
> #define P2I(p) ((intptr_t)(p))
> #define SIZE_FORMAT_XW(width) "%" #width PRIxPTR
>
> The fix is conditional for compilation on other platforms where warnings and pragmas differ.
>
> There is a potentially conflicting fix from Coleen Phillimore also in flight through the repositories; those two calls are left unchanged in this fix to avoid conflicts.
>
> Copyright changes deferred till later, because those will just inflate the size of the diffs; I have a script for fixing those quickly.
>
>
> NON-FIX: because recent versions of gcc also complain about incorrect integer size/signedness and pointer conversions in format strings, I had to retract the printf attribute from two frequently-called methods; that would have required an even larger and more invasive patch.
>
> I considered making this conditional on compilation with clang, but
>
> (1) without Coleen’s patch, this will fail on Mavericks+XCode 5.
> I can wait, and introduce a more-conditional ATTRIBUTE_PRINTF macro.
>
> and
>
> (2) until it can be checked on more than just that platform or we have a Mavericks+clang machine in the JPRT stable, the build risks being semi-broken for up-to-date Mac users with future changes.
>
> A next RFE would be to make progress towards that goal; the attributes to enable are present in two comments in header files (debug.hpp and ostream.hpp), and formatting can be cleaned up to deal with the recent-version gcc complaints.
>
> The remaining gcc warnings fall into several categories:
>
> 1) printing pointer with an integer format (fix this with P2I and macros for pointer format).
>
> 2) printing size_t with a “wrong” format (on some platforms).
>
> 3) printing intptr_t with a “wrong” format (on some platforms).
>
> One complication is use of archaic tools that don’t support c99; this forces the use of more cumbersome methods for specifying format strings.
>
> TESTING:
>
> Built on:
> MountainLion + XCode 4.6.3
> Mavericks + XCode 5.1.1
> Ubuntu (64-bit) + gcc 4.7
> Ubuntu (64-bit) + gcc 4.8
> Ubuntu (64-bit) + gcc 4.4
> Solaris11-sparc+SS12u1
> (mixture of open and closed builds above; Mavericks was built both open and closed).
>
> JPRT built clean for all the usual suspects
>
> Run (fastdebug) on:
> jtreg local
>
> Note that most of the changes are not tested because many of them involve code that is only executed with particular flags or in error cases.  I hand-verified that after preprocessing, the one formatting macro inserted into ostream.cpp expanded into something sensible.  On a 64-bit OS, this:
>
>       indent().print(SIZE_FORMAT_XW(07)":", i);
>
> expands to
>
>        indent().print("%" "07" "lx"":", i);
>
> vs the old
>
>        indent().print("%07x:", i);
>
> (i is size_t, so lx is the appropriate format specifier)
>
> I also hand-checked the changes using two different ways of viewing the diffs (BBEdit diff, and webrev-new).
>
> David
>