Request for review: 7034585 Adjust fillinStackTrace filtering to assist 6998871

[Tom Rodriguez]

On Apr 8, 2011, at 5:01 PM, David Holmes wrote:

> Tom Rodriguez said the following on 04/09/11 09:22:
>> Actually why are you checking for fillInStackTrace* instead of just fillInStackTrace?
>> if (method->name() == vmSymbols::fillIinStackTrace_name() &&
>>    throwable->is_a(method->method_holder())) {
> 
> Added flexibility. There is still some debate as to the final naming scheme in the JDK changes. At present an overload of fillInStackTrace is used with a dummy int parameter. The normal naming scheme would have the native method be called fillInStackTrace0. That would have required a VM change, hence the overload. But we ended up needing a VM change anyway, hence it is now possible to change the method name. I was trying to accommodate this either way. But perhaps it is simpler to just add the symbol for fillInStackTrace0 and check for either?

I think that would be more straightforward.

tom

> 
> David
> 
>> tom
>> On Apr 8, 2011, at 4:12 PM, Tom Rodriguez wrote:
>>> On Apr 8, 2011, at 3:57 PM, David Holmes wrote:
>>> 
>>>> Hi Tom,
>>>> 
>>>> Tom Rodriguez said the following on 04/09/11 06:53:
>>>>> Performance of fillInStackTrace is somewhat important so I think you should avoid using as_C_string since that creates a copy of both fillInStackTrace and the method being checked.  Maybe something like this:
>>>>> const char* fillInStackTrace = "fillInStackTrace";
>>>>> int len = 16;
>>>>> assert(strlen(fillInStackTrace) == len, "must agree");
>>>>> if (method->name()->starts_with(fillInStackTrace, 16) &&
>>>>>  throwable->is_a(method->method_holder())) {
>>>> I hear you, but we shouldn't be hard-coding names like that - which is why we use the vmSymbols entry in the first place.
>>> The reason we use the vmSymbols is so that we can do pointer compares.  An assert like this:
>>> 
>>> assert(strcmp(vmSymbols::fillInstackTrace()->as_C_string(), fillInStackTrace) == 0, "must be the same");
>>> 
>>> would mitigate your concern, though I think it's unlikely we will be changing the name fillInStackTrace any time soon.
>>> 
>>>> It's a pity symbols don't have a set of operators for comparing against other symbols etc. I'll poke a little deeper to see what impact as_C_string will have and whether there is a way to mitigate it - perhaps use the variant where I supply the buffer eg:
>>>> 
>>>> char[] buf = ...
>>>> if (method->name()->starts_with(vmSymbols::fillInstackTrace()->as_C_string(buf), 16) { ...
>>> It still seems like pointless work.  The name of the symbol is the value of the symbol so dynamically converting it to char* is just a waste.
>>> 
>>> tom
>>> 
>>>> Thanks,
>>>> David
>>>> 
>>>>> tom
>>>>> On Apr 8, 2011, at 4:33 AM, David Holmes wrote:
>>>>>> webrev:
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~dholmes/7034585/webrev/
>>>>>> 
>>>>>> When an exception is created, fillInStacktrace is called to populate the backtrace information. This is done in java_lang_Throwable::fill_in_stack_trace in the VM. Because the interesting part of the stacktrace is from the location where the exception was created, and upwards, filtering is applied in fill_in_stack_trace to remove the entry for fillInStackTrace() itself, and the exception constructors.
>>>>>> 
>>>>>> The current filtering code only expects to find a single frame for the fillInStackTrace method, so if an exception class overrides fillInStackTrace (and invokes super.fillInStackTrace) this actually disables the filtering of the constructors. Eg we see:
>>>>>> 
>>>>>> Exception in thread "main" MyException
>>>>>>     at MyException.fillInStackTrace(MyException.java:3)
>>>>>>     at java.lang.Throwable.<init>(Throwable.java:260)
>>>>>>     at java.lang.Exception.<init>(Exception.java:54)
>>>>>>     at java.lang.RuntimeException.<init>(RuntimeException.java:51)
>>>>>>     at MyException.<init>(MyException.java:1)
>>>>>>     at MyException.main(MyException.java:7)
>>>>>> 
>>>>>> instead of:
>>>>>> 
>>>>>> Exception in thread "main" MyException
>>>>>>     at MyException.main(MyException.java:7)
>>>>>> 
>>>>>> The changes to Throwable.java for 6998871 effectively introduce an additional fillInStackTrace() frame and so this too disables the desired filtering of the backtrace.
>>>>>> 
>>>>>> The proposal is quite simple: to modify fill_in_stack_trace so that it expects one or more fillInStackTrace frames, followed by zero or more constructor frames. This addresses the needs of 6998871 as well as fixing any user-defined overriding of fillInStackTrace.
>>>>>> 
>>>>>> Thanks,
>>>>>> David