RFR: 8003310: Enable -Wunused when compiling with GCC

[Christian Thalinger]

On Feb 20, 2013, at 9:33 AM, Mikael Vidstedt <mikael.vidstedt at oracle.com> 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.

It's a mystery...

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

Right.  Thanks for looking into it anyway.

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

Looks good.

-- Chris

> 
> Cheers,
> Mikael
>