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

[Christian Thalinger]

On Apr 17, 2014, at 10:16 AM, David Chase <david.r.chase at oracle.com> 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.

+ #if defined(__clang_major__) || (__GNUC__ >= 4) && (__GNUC_MINOR__ >= 6)
Do all versions of Clang support push and pop?

> 
> (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.

Yes, it fixes the problem but it will be difficult to remember or know (for new people) that you should use print_raw instead.

> 
> (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))

Can we please not use a macro for this?  Use an inline method instead.

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