[OpenJDK 2D-Dev] [REGRESSION?] Build warnings at jdhuff.c with GCC 6

[Philip Race]

On 8/24/16, 5:31 AM, Yasumasa Suenaga wrote:
> Hi Erik, Phil,
>
> Thank you for replying.
> I understand background of JDK-8074827.
>
>> In this particular case is shift-negative-value a new warning in GCC 6?
>
> Yes, this feature is implemented GCC 6:
> https://gnu.wildebeest.org/blog/mjw/2016/02/15/looking-forward-to-gcc6-many-new-warnings/

I see. I tracked down the commit so yes, this seems to be the case.
>
> BTW, why is libjavajpeg only enabled these warnings?
> For example, liblcms is disabled format-nonliteral, type-limits, and
> misleading-indentation.

We only disable the warnings that are observed .. not all warnings else
we might as
well turn off warnings completely.
>
> I agree compiler warnings is very useful to fix. However, I think a
> part of source of libjavajpeg is third-party (developed by Independent
> JPEG Group).
> According to [1], warnings in this library should be suppressed.

Yes. FWIW this is the most stable 3rd party library we have - by a long
way -
so it *could* be argued that tweaking it isn't likely to get lost any
time soon
but I'd still prefer to keep the source as it came.
>
> If all binaries which are included in JDK/JRE should be enabled all
> compiler warnings, I think LCMS and any other libraries should be fixed.

Well you would *have* to get upstream to resolve it.
But it is whack-a-mole. As you have discovered new compilers
generate new warnings .. and it is not monotonic either.

-phil.
>
> Which policy is correct?
>
>
> Thanks,
>
> Yasumasa
>
>
> [1] http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-June/004495.html
>
>
> On 2016/08/24 18:48, Erik Joelsson wrote:
>> Hello,
>>
>>
>> On 2016-08-23 18:12, Phil Race wrote:
>>> On 08/23/2016 08:47 AM, Erik Joelsson wrote:
>>>> Hello,
>>>>
>>>> I do agree that maintaining the list of disabled warnings will be
>>>> impossible unless we have a structured way of tracking for which
>>>> compiler versions we disable what. Ideally we should be able to easily
>>>> add conditions for when certain warnings should be disabled. We are
>>>> unfortunately lacking that today and at least I don't have the
>>>> bandwidth to fix that anytime soon.
>>>>
>>>> The official compilers are only really official for Oracle. The
>>>> OpenJDK can (and should) be buildable with a much wider range of
>>>> compiler versions.
>>> I agree there. This is fortunately not an "unbuildable" situation.
>>> The only other option I can think of which may or may not be palatable
>>> is to explicitly
>>> check the compiler version and add that disabled warning only for that
>>> exact compiler version.
>>> There'd still be some maintenance as that compiler version became
>>> either
>>> official .. or obsolete ..
>>>
>>> Is there precedent (or any kind of support) for that ?
>> What I had in mind was a structured way of adding conditionals for
>> some kind of ranges of compiler versions, or at least something like
>> 6.*, or "greater than 4.9.3". It's pretty simple today to check for
>> exact compiler versions but then we end up with a lot of changes when
>> minor versions are bumped. I don't think that would be worth it.
>>
>> In this particular case is shift-negative-value a new warning in GCC
>> 6? If that's the case it doesn't actually hurt adding it since GCC is
>> nice enough to not complain about unknown warning tags. If we do,
>> just make sure to specify in a comment that it's specific to GCC
>> version 6+.
>>
>> /Erik
>>> -phil.
>>>
>>>> Luckily we have the workaround of setting --disable-warnings-as-errors
>>>> when this situation occurs.
>>>>
>>>> For reference, the compilers Oracle uses are listed in
>>>> README-builds.md in the top level directory in the forest.
>>>>
>>>> So for now at least, I think Phil is right.
>>>>
>>>> /Erik
>>>>
>>>> On 2016-08-23 17:11, Philip Race wrote:
>>>>> Erik .. please chime in if you disagree with the following
>>>>> The goal here is to have no warnings with the *official* compilers.
>>>>> If you are using something else and get warnings then either fix
>>>>> the *source* or else you need to use --disable-warning-as-errors.
>>>>>
>>>>> Otherwise we'll be suppressing the warnings for a whole range
>>>>> of compilers and no one will know what the set is and these
>>>>> bugs will become 'unfixable' and a continual chore of churn.
>>>>> It will be something we should address as we move *official*
>>>>> compilers.
>>>>>
>>>>> In fact the warning you want to suppress is one I just got rid of
>>>>> (the
>>>>> suppression)
>>>>> http://hg.openjdk.java.net/jdk9/client/jdk/rev/d4f7412a51d2
>>>>> I don't think we want to add it back unless the *official* compilers
>>>>> object
>>>>> .. I am sure that official list is documented somewhere.
>>>>>
>>>>> -phil.
>>>>>
>>>>>
>>>>> On 8/23/16, 6:10 AM, Yasumasa Suenaga wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> I've fixed several warnings when we build OpenJDK with GCC 6 in
>>>>>> JDK-8160294.
>>>>>> However, I encountered shift-negative-value warnings at jdhuff.c
>>>>>> on my
>>>>>> Fedora 24 (GCC 6.1.1):
>>>>>> --------------
>>>>>> /home/ysuenaga/OpenJDK/hs/jdk/src/java.desktop/share/native/libjavajpeg/jdhuff.c:458:13:
>>>>>>
>>>>>>
>>>>>> warning: left shift of negative value [-Wshift-negative-value]
>>>>>> { 0, ((-1)<<1) + 1, ((-1)<<2) + 1, ((-1)<<3) + 1, ((-1)<<4) + 1,
>>>>>> ^~
>>>>>> /home/ysuenaga/OpenJDK/hs/jdk/src/java.desktop/share/native/libjavajpeg/jdhuff.c:458:28:
>>>>>>
>>>>>>
>>>>>> warning: left shift of negative value [-Wshift-negative-value]
>>>>>> { 0, ((-1)<<1) + 1, ((-1)<<2) + 1, ((-1)<<3) + 1, ((-1)<<4) + 1,
>>>>>> ^~
>>>>>> /home/ysuenaga/OpenJDK/hs/jdk/src/java.desktop/share/native/libjavajpeg/jdhuff.c:458:43:
>>>>>>
>>>>>>
>>>>>> warning: left shift of negative value [-Wshift-negative-value]
>>>>>> { 0, ((-1)<<1) + 1, ((-1)<<2) + 1, ((-1)<<3) + 1, ((-1)<<4) + 1,
>>>>>> :
>>>>>> --------------
>>>>>>
>>>>>> I think these warnings are available from JDK-8074827.
>>>>>> This issue enables warnings to fix in the source code.
>>>>>> However, in review of JDK-8160294, I heared that warnings in IJG
>>>>>> JPEG
>>>>>> library should just be suppressed:
>>>>>>
>>>>>> http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-June/004497.html
>>>>>>
>>>>>>
>>>>>>
>>>>>> So I think they should be suppressed and we should fix as below:
>>>>>> --------------
>>>>>> diff -r b548b8217d8c make/lib/Awt2dLibraries.gmk
>>>>>> --- a/make/lib/Awt2dLibraries.gmk Mon Aug 22 19:28:36 2016 -0700
>>>>>> +++ b/make/lib/Awt2dLibraries.gmk Tue Aug 23 22:08:44 2016 +0900
>>>>>> @@ -490,7 +490,7 @@
>>>>>> CFLAGS := $(CFLAGS_JDKLIB) $(BUILD_LIBJAVAJPEG_HEADERS) \
>>>>>> $(LIBJAVA_HEADER_FLAGS) \
>>>>>> -I$(SUPPORT_OUTPUTDIR)/headers/java.desktop, \
>>>>>> - DISABLED_WARNINGS_gcc := clobbered, \
>>>>>> + DISABLED_WARNINGS_gcc := clobbered shift-negative-value, \
>>>>>> MAPFILE := $(JDK_TOPDIR)/make/mapfiles/libjpeg/mapfile-vers, \
>>>>>> LDFLAGS := $(LDFLAGS_JDKLIB) \
>>>>>> $(call SET_SHARED_LIBRARY_ORIGIN), \
>>>>>> --------------
>>>>>>
>>>>>> If it is correct, I file it to JBS and upload webrev.
>>>>>> Do you think about it?
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>