RFR: 8003310: Enable -Wunused when compiling with GCC

Mon Feb 18 15:45:11 PST 2013

Use PRODUCT_RETURN[0]:

+ address clamp_address_in_page(address addr, address page_address) 
PRODUCT_RETURN0;

and

#ifndef PRODUCT
+ static address clamp_address_in_page(address addr, address 
page_address, intptr_t page_size) {

Vladimir

On 2/18/13 3:14 PM, Mikael Vidstedt 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/
>
> The only change since the earlier webrev is in constantPool.cpp, where I
> updated the code to use the const bool = false idiom.
>
> I would appreciate a review or two of that piece specifically.
>
> Thanks,
> Mikael
>
> On 2/14/2013 8:04 PM, Azeem Jiva wrote:
>> Did you measure how much the libjvm.so shrank with your changes?
>>
>> On 2/14/2013 7:27 PM, Coleen Phillmore wrote:
>>>
>>> Looks good Mikael!  I like these sorts of changes.
>>> Coleen
>>>
>>> On 2/1/2013 11:35 AM, Mikael Vidstedt wrote:
>>>>
>>>> All,
>>>>
>>>> I finally found some time to update the webrev:
>>>>
>>>> http://cr.openjdk.java.net/~mikael/8003310/webrev.03/webrev/
>>>>
>>>> Not much has changes since the first webrev. I played around with
>>>> turning the #ifdef in constantPool.cpp into a const bool = false,
>>>> but one of the downsides I noticed with that is that the code will
>>>> still be compiled into product and especially for this type of
>>>> function with a relatively large number of constant strings in it it
>>>> unnecessarily adds to the footprint. This does not invalidate the
>>>> const bool = false idiom, but there may be cases where it is
>>>> beneficial to use alternatives to it.
>>>>
>>>> Cheers,
>>>> Mikael
>>>>
>>>> On 11/15/2012 5:00 PM, Mikael Vidstedt wrote:
>>>>> On 2012-11-13 06:20, Coleen Phillimore wrote:
>>>>>> On 11/13/2012 8:41 AM, Coleen Phillimore wrote:
>>>>>>>
>>>>>>> Hi Mikael,   This is good!  I have the same comments as David.
>>>>>>>
>>>>>>> For constantPool.cpp can you add #undef DBG after that code.
>>>>>>>
>>>>>>> Maybe there should be a new convention for DEBUG_<filename in
>>>>>>> caps> for unused code that people want to leave around for
>>>>>>> debugging.   I don't recommend we add a lot of code like this,
>>>>>>> but having such an ifdef would differentiate code that was just
>>>>>>> left in unintentionally from code that we want to be left in
>>>>>>> (occasionally).
>>>>>>
>>>>>> Actually, I want to revise this.  We had this discussion about
>>>>>> this recently rwt metaspace debugging and favored a const static
>>>>>> variable = false; under #ifdef DEBUG or ASSERT and the debugging
>>>>>> code under DEBUG/ASSERT so that it doesn't bit rot.   This is for
>>>>>> the code left in intentionally. Which makes me wonder why
>>>>>> print_cpool_bytes() is there under DEBUG_CPOOL claiming to be
>>>>>> "temporary" when there is another printing function in that file
>>>>>> already.
>>>>>
>>>>> The reason why I moved it under DBG was because it is only ever
>>>>> called wrapped with a DBG(). I can play around with making it a
>>>>> const static = false as you outlined.
>>>>>
>>>>> The other print functions in constantPool.cpp are implementations
>>>>> of the virtual functions inherited from Metadata, so they are not
>>>>> quite as temporary as print_cpool_bytes, if that makes sense.
>>>>>
>>>>> Cheers,
>>>>> Mikael
>>>>>
>>>>>>
>>>>>> Coleen
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>>
>>>>>>> On 11/13/2012 12:47 AM, 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.
>>>>>>>>
>>>>>>>> - why was the "static" removed from a number functions. They now
>>>>>>>> have global visibility rather than being restricted to their files?
>>>>>>>>
>>>>>>>>
>>>>>>>> In globaleDefinitions.cpp:
>>>>>>>>
>>>>>>>> + void GlobalDefinitions::test_globals() {
>>>>>>>> +   intptr_t page_size = 4096;
>>>>>>>>
>>>>>>>> Page size may not be 4K - will the test still be valid?
>>>>>>>>
>>>>>>>>
>>>>>>>> The comments describing clamp_address_in_page don't need to be
>>>>>>>> on both the declaration and definition.
>>>>>>>>
>>>>>>>> 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
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>