RFR(XS): 8046611: Build errors with gcc on sparc/fastdebug

[Christian Tornqvist]

Looks good!

Thanks,
Christian

-----Original Message-----
From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of Mikael Vidstedt
Sent: Thursday, June 19, 2014 9:45 PM
To: David Holmes
Cc: Christian Thalinger; hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR(XS): 8046611: Build errors with gcc on sparc/fastdebug


Since nobody has raised any concerns, here is a webrev which removes the functions. Please review!

http://cr.openjdk.java.net/~mikael/webrevs/8046611/webrev.02/webrev/

Cheers,
Mikael

> On Jun 18, 2014, at 22:53, Mikael Vidstedt <mikael.vidstedt at oracle.com> wrote:
> 
> 
> I think it's reasonable to say that if nobody speaks up for these methods they should be removed; handy as they may be, if nobody has used them in recent time they are realistically just baggage.
> 
> Unless somebody says differently I will prepare a webrev to have them removed.
> 
> Cheers,
> Mikael
> 
>>> On Jun 18, 2014, at 20:01, David Holmes <david.holmes at oracle.com> wrote:
>>> 
>>> On 19/06/2014 11:21 AM, Christian Thalinger wrote:
>>> Old, long-time unused debug methods are worthless.  I’d suggested to just remove these methods.
>> 
>> Sometimes when developing code, or debugging it, it is very useful to add additional introspection functions to aid with testing and debugging. Past developers have left these in place in various bits of code throughout the VM. As time progresses people who don't use them see them simply as dead code to be removed.
>> 
>> While I can see the utility in such methods in general (not commenting on these specific ones) I also see the reality that over time new developers don't even realize the code exists and so tend to roll their own introspection tools as needed.
>> 
>> So I would not fight hard for this code to stay. But it isn't my code and I don't know that it is unused by other developers.
>> 
>> Cheers,
>> David
>> 
>>>> On Jun 17, 2014, at 9:08 PM, Mikael Vidstedt <mikael.vidstedt at oracle.com> wrote:
>>>> 
>>>> 
>>>> I can file an RFE on that if we feel that it's worth following up on.
>>>> 
>>>> Does anybody have comments on the rest of the new webrev?
>>>> 
>>>> Cheers,
>>>> Mikael
>>>> 
>>>>> On Jun 16, 2014, at 5:48, Marcus Lagergren <marcus.lagergren at oracle.com> wrote:
>>>>> 
>>>>> Maybe we could introduce an unused define, if there’s no platform independent way of doing it in C.
>>>>> 
>>>>> GCC has something line  __attribute__ ((deprecated)), and there’s 
>>>>> #pragma unused on some platforms…
>>>>> 
>>>>> /M
>>>>> 
>>>>> 
>>>>>> On 13 Jun 2014, at 08:08, Mikael Vidstedt <mikael.vidstedt at oracle.com> wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 2014-06-11 18:36, David Holmes wrote:
>>>>>>> Hi Mikael,
>>>>>>> 
>>>>>>>> On 12/06/2014 5:47 AM, Mikael Vidstedt wrote:
>>>>>>>> 
>>>>>>>> Please review the below small change which fixes a couple of 
>>>>>>>> build errors when building the sparc files with gcc.
>>>>>>>> 
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8046611
>>>>>>>> Webrev: 
>>>>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8046611/webrev.00/we
>>>>>>>> brev
>>>>>>>> 
>>>>>>>> Without the fix the following two errors are seen:
>>>>>>>> 
>>>>>>>> hotspot/src/cpu/sparc/vm/frame_sparc.cpp:442: error: ‘frame 
>>>>>>>> nth_sender(int)’ defined but not used
>>>>>>>> hotspot/src/share/vm/runtime/safepoint.cpp:771: error: ‘void 
>>>>>>>> print_me(intptr_t*, intptr_t*, bool*)’ defined but not used
>>>>>>>> 
>>>>>>>> The functions in question are debug functoins declared static. 
>>>>>>>> Assuming we'll want to keep them around the fix is to simply 
>>>>>>>> remove the static keyword from them.
>>>>>>> 
>>>>>>> This seems so wrong. Changing them to non-static simply stops the compiler from being able to deduce whether they are used or not. But can they actually be used if declared non-static?
>>>>>>> 
>>>>>>> Can we not tag them some way to tell the compiler to ignore them?
>>>>>>> 
>>>>>>> Debugging functions can be useful even if rarely used. But no one will know they exist if they aren't in the code so I'm wary of simply deleting everything of that nature.
>>>>>> 
>>>>>> I don't know of any way to reliably tag functions as used apart from removing the static keyword.
>>>>>> 
>>>>>> I totally redid the safepoint.cpp debug function(s) part of the change. First of all they no longer need to handle the 32-bit SPARC case, so that part of the code goes away. That also enables removing the print_longs function (it will be the same as print_ptrs), and I did some additional clean up on top of that. Verified that it works by manually enabling the const bool and looked at the output.
>>>>>> 
>>>>>> I'm not sure what to do about the nth_sender function though, mostly because I don't know how people expect to use it. We could for example move it to utilities/debug.cpp and export it as the other debug function there, which is what I've done in the below webrev just so that we can see what it would look like. Gladly taking feedback on it though, especially from people actually having used the function in recent times. If it hasn't been used recently it really should go away...
>>>>>> 
>>>>>> Updated webrev:
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8046611/webrev.01/webr
>>>>>> ev/
>>>>>> 
>>>>>> Cheers,
>>>>>> Mikael
>>>>>> 
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> 
>>>>>>>> Cheers,
>>>>>>>> Mikael
>>>