RFR: 8348286: [AIX] clang 17 introduces new warning Wtentative-Definitions which produces Build errors [v3]

[Kim Barrett]

On Thu, 23 Jan 2025 13:48:06 GMT, Joachim Kern <jkern at openjdk.org> wrote:

>> We (SAP) try to introduce the ‘IBM Open XL C/C++ for AIX 17.1.2’ (based on clang 17) as a feasible compiler for jdk25, because in combination with the 17.1.3 runtime this would enable the support for ubsan.
>> Unfortunately, the new compiler comes along with a new set of compiler warnings turned into errors by -Werror.
>> One of the warnings is -Wtentative-definitions. It is fired when a variable definition in a header is found:
>> /jdk/src/java.desktop/share/native/libawt/awt/image/imageInitIDs.h:36:20: error: possible missing 'extern' on global variable definition in header [-Werror,-Wtentative-definitions]
>>    36 | IMGEXTERN jfieldID g_BImgRasterID;
>>       | ^
>> From now on headers allow only extern declarations of variables. The definition must take place in a c/cpp file. This means e.g.
>> imageInitIDs.h:36:20
>>    36 extern jfieldID g_BImgRasterID;
>> and the corresponding c-File
>>    jfieldID g_BImgRasterID;
>> 
>> The other possible solution would be to compile everything with
>> -Wno-tentative-definitions
>> which could be set in flags-cflags.m4, if compiling with clang 17.
>
> Joachim Kern has updated the pull request incrementally with one additional commit since the last revision:
> 
>   remove old solution

Coming late to the party, since this PR has already been integrated.

First, tentative definitions are only applicable to C, not to C++.

I think `-Wtentative-definition` doesn't really buy all that much. It looks
like it turns a *potential* link-time failure into a definite compile-time
warning. But I don't think I'd want to globally disable it, as it seems like
it would usually be a valid complaint (unless the code is jumping through some
hoops as done by imageInitIDs.[ch]), and probably provides a better error
message too.

gcc provides `-fcommon`, which controls the placement of tentative
definitions. We're not using that, instead implicitly using the default
`-fno-common`. (There is one place where we unnecessarily use `-fno-common`
explicitly: https://bugs.openjdk.org/browse/JDK-8349565.)

With `-fcommon`, tentative definitions in different translation units are
merged, with potential access penalties. With `-fno-common`, instead of
merging one gets multiple definition errors at link time. Note that code that
relies on `-fcommon` has never been standards compliant.

The changes related to fp_pipewire.h seem fine.

I'm not keen on the large copy-paste-modify for imageInitIDs.[ch]. I kind of
prefer a different approach, of locally suppressing the warning, i.e.
something like (untested, may need version conditionalization):

In imageInitIDs.c

+ PRAGMA_DIAG_PUSH
+ #pragma clang diagnostic ignored "-Wtentative-definition"
  #define IMGEXTERN
  #include "imageInitIDs.h"
+ PRAGMA_DIAG_POP


I'll leave it up to the client team to decide whether they want to make such a
change, or leave things as-is.

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

PR Comment: https://git.openjdk.org/jdk/pull/23236#issuecomment-2640539884