RFR: 8003310: Enable -Wunused when compiling with GCC

[Mikael Vidstedt]

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