RFR: 8003310: Enable -Wunused when compiling with GCC

[Mikael Vidstedt]

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, 
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*:

src/cpu/x86/vm/x86_64.ad:Address build_address(int b, int i, int s, int d)
src/cpu/x86/vm/methodHandles_x86.cpp:RegisterOrConstant constant(int value)
src/cpu/x86/vm/assembler_x86.cpp:int encode(XMMRegister r)
src/share/vm/c1/c1_LIRGenerator.cpp:Value maxvalue(IfOp* ifop)
src/share/vm/compiler/compileLog.cpp:const char* split_attrs(const char* 
&kind, char* buffer)
src/share/vm/compiler/compilerOracle.cpp:const char * 
command_name(OracleCommand command)
src/share/vm/gc_implementation/g1/ptrQueue.cpp:int 
byte_index_to_index(int ind)
src/share/vm/gc_implementation/g1/ptrQueue.cpp:int 
index_to_byte_index(int byte_ind)
src/share/vm/interpreter/interpreterRuntime.cpp:void 
trace_locking(Handle& h_locking_obj, bool is_locking)
src/share/vm/memory/heap.cpp:size_t align_to_allocation_size(size_t size)
src/share/vm/opto/connode.cpp:bool can_cause_alias(Node *n, 
PhaseTransform *phase)
src/share/vm/opto/subnode.cpp:Node *clone_cmp( Node *cmp, Node *cmp1, 
Node *cmp2, PhaseGVN *gvn, BoolTest::mask test )
src/share/vm/prims/jni.cpp:methodHandle 
jni_resolve_interface_call(Handle recv, methodHandle method, TRAPS)
src/share/vm/prims/jni.cpp:methodHandle jni_resolve_virtual_call(Handle 
recv, methodHandle method, TRAPS)


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

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!

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