Request for review (#4) 7174978: NPG: Fix bactrace builder for class redefinition

[Coleen Phillimore]

On 1/16/2013 2:19 AM, David Holmes wrote:
> Hi Coleen,
>
> A couple of follow ups ...

See inline comments.   New webrev:

http://cr.openjdk.java.net/~coleenp/7174978_6/

>
> On 16/01/2013 7:12 AM, Coleen Phillimore wrote:
>>> Copyrights to 2013 :)
>>>
>>
>> It's unclear whether we have to do this. I think there is going to be
>> some RE process that fixes these files.
>
> I'll believe it when I see it :)

I haven't seen any recent pronouncements about this.  I hate to see 
webrevs with copyright changes though because I have to scroll down for 
the "real" changes.    I have a new "commit" script  now though, others 
are welcome to copy it:

#/bin/csh -f
hg status | grep "^M" | sed -e "s/M//" >file.list
foreach f (`cat file.list`)
   echo $f
   sed -e "s/Copyright (c) \([0-9][0-9][0-9][0-9]\), 
[0-9][0-9][0-9][0-9], Oracle/Copyright (c) \1, 2013, Oracle/"  \
       -e "s/Copyright (c) \([0-9][0-9][0-9][0-9]\), Oracle/Copyright 
(c) \1, 2013, Oracle/" <$f >$f.new
   diff $f $f.new
   cp $f.new $f
end
hg commit


>
>>> src/share/vm/oops/constantPool.hpp
>>>
>>> ! void add_version(int version) {
>>> ! if (version == -1) {
>>> ! _saved._version = version; // keep overflow
>>> ! } else {
>>> ! int new_version = version++;
>>> ! _saved._version = version == INT_MAX ? -1 : version;
>>> ! }
>>> ! }
>>>
>>> new_version is unused here but I think you meant to do
>>>
>>> _saved._version = new_version == INT_MAX ? -1 : version;
>>>
>>
>> This was a pre-code-review cleanup... I didn't mean to assign version++
>> into new_version. The second line stays the same. I retested this fix.
>
> I'm unclear what the final form of this code now looks like.

   void add_version(int version)           {
     if (version == -1) {
       _saved._version = version;  // keep overflow
     } else {
       version++;
       _saved._version = (version == INT_MAX) ? -1 : version;
     }
   }

>
>> This code is only called for exceptions thrown at initialization time.
>> It's hard to test this code since most exceptions during initialization
>> in universe_post_init() cause
>>
>> # Internal Error
>> (/java/east/u1/cphillim/hg/rt.bt2-clean/src/share/vm/interpreter/linkResolver.cpp:902), 
>>
>> pid=10369, tid=47170653824768
>> # assert(resolved_method->method_holder()->is_linked()) failed: must be
>> linked
>> #
>>
>> Should be another CR, which I thought we'd already fixed!
>
> We did :) But we need to fix that error message so it tells us which 
> class has the problem (assert really needs to take a format string as 
> an arg :( ). We fixed the issue with java.lang.Class not being linked 
> when trying to construct a Throwable. Maybe it is Throwable or one of 
> the other exception types that is the problem.

Filed a bug.

Thanks,
Coleen
>
> Cheers,
> David