RFR: 8003310: Enable -Wunused when compiling with GCC
Mikael Vidstedt
mikael.vidstedt at oracle.com
Mon Feb 18 16:05:33 PST 2013
If we really believe this is worth addressing (and I'm not sure it is)
then I'd much rather have the function be defined in the
globalDefinitions.hpp file, potentially as a static method in the
GlobalDefinitions class which is currently only for unit testing. That
way it would be included in the respective .cpp compile units and
completely deadcode eliminated in the same way it is today without
having to change its contract.
Cheers,
Mikael
On 2/18/2013 3:45 PM, Vladimir Kozlov wrote:
> 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
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
More information about the hotspot-dev
mailing list