RFR: 8003310: Enable -Wunused when compiling with GCC

[Coleen Phillimore]

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.

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
>