RFR: 8003310: Enable -Wunused when compiling with GCC

[David Holmes]

On 16/11/2012 10:20 AM, Mikael Vidstedt wrote:
> On 2012-11-12 21:47, David Holmes wrote:
>> Hi Mikael,
>>
>> A couple of general observations as really the "owners" of each file
>> needs to assess the changes:
>>
>> - sometimes functions exist for debugging/tracing and calls will be
>> added to the code by engineers as they debug. For example MBFence in
>> synchronizer.cpp allows you to add fences into expressions.
>
> For MBFence specifically, given that it is a static in synchronizer.cpp
> I'm going to assume it's a leftover and any future debugging/tracing
> should be able to do a call to OrderAccess::fence() directly instead,

Not really, the helper defines a function so you can insert the barrier 
at arbitrary locations, including within expressions. You can't do that 
with OrderAccess::fence() unless you add gratuitous parentheses and use 
of the comma operator. So anyone wanting this will end up redefining 
what is presently there. That said this case is so trivial that 
recreating it is not an issue.

> but in general you're absolutely right and I'm counting on the owners to
> point out if there are functions that are frequently used during
> development.
>
> Put differently - is anybody counting on the following functions, if not
> *they will be removed*:
>
<snip>
> src/share/vm/interpreter/interpreterRuntime.cpp:void
> trace_locking(Handle& h_locking_obj, bool is_locking)

I would say leave this in place but use ifndef PRODUCT around it.

>> - why was the "static" removed from a number functions. They now have
>> global visibility rather than being restricted to their files?
>
> GCC knows that static functions declared in a file but not used in that
> same file are dead, and it emits a warning for each of them. My
> assumption was that the three functions for which I removed the static
> keyword are being used for debugging purposes, called from, say, gdb. It
> would be very helpful to get feedback on if that assumption is correct.

The same could be true for other methods here - they may exist to be 
invoked from the debugger. Unfortunately we've lost a lot of historical 
knowledge here.

> Or again, put differently - is anybody using any the following functions
> from a debugger. If I don't get confirmation on that they'll also be
> removed.
>
> src/share/vm/gc_implementation/g1/g1CollectorPolicy.cpp:char*
> region_num_to_mbs(int length)
> src/share/vm/opto/block.cpp:void edge_dump(GrowableArray<CFGEdge *> *edges)
> src/share/vm/opto/block.cpp:void trace_dump(Trace *traces[], int count)
>
>> In globaleDefinitions.cpp:
>>
>> + void GlobalDefinitions::test_globals() {
>> + intptr_t page_size = 4096;
>>
>> Page size may not be 4K - will the test still be valid?
>
> I'll add tests with a couple of other page sizes as well!

Not quite what I meant. If os::vm_page_size() != 4k will this test be valid?

Cheers,
David
------

>> The comments describing clamp_address_in_page don't need to be on both
>> the declaration and definition.
>
> I'll keep it on the declaration only.
>
>>
>> Cheers,
>> David
>> ------
>>
>> On 13/11/2012 1:59 PM, Mikael Vidstedt wrote:
>>>
>>> All,
>>>
>>> Please review the below change. The change adds the -Wunused flag when
>>> compiling with GCC and removes a number of unused functions/dead code.
>>>
>>> In the process I found one function (same_page) which was duplicated in
>>> four different places. I merged it to a single function, renamed it to
>>> clamp_address_in_page, added some comments and refactored it to be
>>> slightly easier to understand. I also added unit tests for it. Feedback
>>> appreciated (especially on the name).
>>>
>>> http://cr.openjdk.java.net/~mikael/8003310/webrev.00/
>>>
>>> Passes JPRT and the built-in unit tests (-XX:+ExecuteInternalVMTests).
>>>
>>> Thanks,
>>> Mikael
>>>
>