RFR: 8003310: Enable -Wunused when compiling with GCC

Thu Apr 4 12:14:34 PDT 2013

David, Coleen, Vladimir, others - thanks for the reviews and the 
associated patience!

Cheers,
Mikael

On 4/3/2013 12:30 AM, David Holmes wrote:
> Sorry for the delay.
>
> Ship it! (As seems to be the new catch-cry ;-) )
>
> David
>
> On 29/03/2013 2:40 AM, Mikael Vidstedt wrote:
>>
>> I've been waiting for Joe's changes to bubble up so as to not add a
>> conflict unnecessarily, and since they're now in hotspot-rt I have
>> uploaded a webrev for what I hope is a final time *knocking on wood*. I
>> would appreciate a quick check to see that everything still looks fine:
>>
>> http://cr.openjdk.java.net/~mikael/webrevs/8003310/webrev.08/webrev/
>>
>> The only relevant changes from the last webrev should be:
>>
>> make/linux/makefiles/gcc.make:
>>
>> * Eliminate the ADDITIONAL_WARNINGS variable and just use WARNING_FLAGS
>> directly
>> * Clean up the -Wconversion logic
>>
>> src/share/vm/runtime/arguments.cpp:
>>
>> * Updated after the recent changes to SERIAL/INCLUDE_ALL_GCS (8005915)
>>
>> Thanks,
>> Mikael
>>
>> On 2/21/2013 7:16 PM, David Holmes wrote:
>>> One final comment - please liaise with Joe Provino who is adding
>>> -Wundef so we can get a consistent approach as to where these -Wxxx
>>> flags get added. :)
>>>
>>>
>>> (Don't know what I was looking at with the trace_locking stuff - it
>>> ends up as an empty method.)
>>>
>>> David
>>>
>>> On 22/02/2013 6:00 AM, Mikael Vidstedt wrote:
>>>>
>>>> On 2013-02-20 10:04, Coleen Phillimore wrote:
>>>>> On 2/20/2013 12:33 PM, Mikael Vidstedt wrote:
>>>>>>
>>>>>> On 2/19/2013 12:01 PM, Christian Thalinger wrote:
>>>>>>> On Feb 18, 2013, at 3:14 PM, Mikael Vidstedt
>>>>>>> <mikael.vidstedt at oracle.com> wrote:
>>>>>>>
>>>>>>>> A really good control question, thanks a lot for asking!
>>>>>>>>
>>>>>>>> As a matter of fact these changes are basically just doing what 
>>>>>>>> the
>>>>>>>> compiler already does for us, so it's more about cleaning up the
>>>>>>>> source code than reducing the size of the binary. So in theory
>>>>>>>> these changes should have no impact on the binary size at all, but
>>>>>>>> it actually turns out they do. Very specifically, the fact that I
>>>>>>>> moved the same_page() function from being duplicated in the three
>>>>>>>> os_<os>.cpp files to having it be in globalDefinitions.cpp makes
>>>>>>>> the binary grow a few bytes (54 bytes to be specific). The reason
>>>>>>>> is a bit subtle:
>>>>>>>>
>>>>>>>> The same_page() function is (was) static in the respective 
>>>>>>>> os_*.cpp
>>>>>>>> files. They are only ever used when the Verbose flag is true, and
>>>>>>>> furthermore the Verbose option is a develop only flag, which means
>>>>>>>> it is hardcoded to false in product. The compiler knows that's the
>>>>>>>> case and eliminates the same_page() function completely.
>>>>>>>>
>>>>>>>> Since I moved it to globalDefinitions.cpp there's no way easy for
>>>>>>>> the compiler to know that it is not being used, so it will 
>>>>>>>> actually
>>>>>>>> keep the function.
>>>>>>>>
>>>>>>>> Unless there are strong opinions I'm not going to do anything 
>>>>>>>> about
>>>>>>>> this.
>>>>>>>>
>>>>>>>> However, this made me question my earlier experiments with using
>>>>>>>> the const bool = false construct in constantPool.cpp, because 
>>>>>>>> after
>>>>>>>> all that is the exact same pattern. And it turns out that I must
>>>>>>>> have done something wrong when I was performing the experiments,
>>>>>>>> because when I do the same thing again now it turns out the
>>>>>>>> compiler actually *does* deadcode eliminate the debug-only
>>>>>>>> functions. So I take everything back and conclude that const 
>>>>>>>> bool =
>>>>>>>> false is indeed a great way to make sure the debug code does not
>>>>>>>> rot over time, and that the product binary will *not* contain the
>>>>>>>> dead functions. Sorry for any confusion this may have caused.
>>>>>>>>
>>>>>>>> With all that in mind, here's another version of the webrev:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8003310/webrev04/webrev/ 
>>>>>>>>
>>>>>>> src/share/vm/interpreter/interpreterRuntime.cpp:
>>>>>>>
>>>>>>> I suppose that #if 0 is to keep that code for...? Could we add a
>>>>>>> comment why we keep it?
>>>>>>
>>>>>> This was by "popular request" (from David Holmes) :)
>>>>>>
>>>>>> I personally don't know if and how this is being used. Note that
>>>>>> ObjectSynchronizer::trace_locking is PRODUCT_RETURN and the
>>>>>> non-product implementation in synchronizer.cpp is empty (modulo the
>>>>>> comment saying "Don't know what to do here"), so one can question 
>>>>>> the
>>>>>> value of keeping the any of the trace_locking functions, but they 
>>>>>> may
>>>>>> be someone's favorite debugging tool. Until proven differently I 
>>>>>> will
>>>>>> leave it in there for now. If somebody can help me provide a useful
>>>>>> comment about how the code is actually being used then I will
>>>>>> certainly add it, otherwise I'll keep this as it is.
>>>>>
>>>>> If I remember correctly, this trace_locking code was associated 
>>>>> with a
>>>>> flag that we removed a long time ago.   I don't think if we want to
>>>>> implement TraceLocking we'd use this function.  My vote is that it 
>>>>> and
>>>>> the ObjectMonitor:: version should be removed.
>>>>
>>>> Thanks Coleen, that makes sense. I've prepared a final version of the
>>>> webrev with the trace_locking methods removed too:
>>>>
>>>> http://cr.openjdk.java.net/~mikael/webrevs/8003310/webrev.06/webrev/
>>>>
>>>> Any final comments?
>>>>
>>>> Cheers,
>>>> Mikael
>>>>
>>>>>
>>>>> Coleen
>>>>>>
>>>>>>> src/share/vm/oops/constantPool.cpp:
>>>>>>>
>>>>>>> So we use:
>>>>>>>
>>>>>>> + const bool debug_cpool = false;
>>>>>>>
>>>>>>> but we still have the DBG macro.  Can't we have a static debug
>>>>>>> method that takes all printf arguments and prints them? The
>>>>>>> debug_cpool should make that method empty and the compiler can
>>>>>>> remove it.  I really start to hate all these macros...
>>>>>>
>>>>>> I share your macro hate and I actually played around with removing
>>>>>> the macros just as you say. There is one small problem with doing it
>>>>>> that way, and that's the fact that a few of the printf:s print
>>>>>> symbols names, and as part of that they need to create utf8 strings
>>>>>> using sym->as_utf8(). The compiler can unfortunately not know that
>>>>>> that function is for all intents and purposes a no-op, so it will
>>>>>> keep the call even in product meaning increased binary size and it
>>>>>> will also add to the runtime since it actually performs the call and
>>>>>> creates the utf8 string. So I'm going to keep the macros for now.
>>>>>>
>>>>>>
>>>>>> I also moved the clamp_address_in_page to the globalDefinitions.hpp
>>>>>> header file, meaning it will be inlined and dead code eliminated.
>>>>>>
>>>>>> New webrev here:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8003310/webrev.05/webrev/
>>>>>>
>>>>>> Final comments?
>>>>>>
>>>>>> Cheers,
>>>>>> Mikael
>>>>>>
>>>>>
>>>>
>>