review request (S): 6981788 GC map generator sometimes picks up the wrong kind of instruction operand

[Tom Rodriguez]

On Oct 25, 2010, at 12:22 PM, John Rose wrote:

> On Oct 25, 2010, at 11:30 AM, Tom Rodriguez wrote:
> 
>> On Oct 23, 2010, at 12:04 AM, John Rose wrote:
>> 
>>> 6981788: GC map generator sometimes picks up the wrong kind of instruction operand
>>> Summary: Distinguish pool indexes from cache indexes in recently changed code.
>>> http://cr.openjdk.java.net/~jrose/6981788/webrev.00
>> 
>> I don't understand this line:
>> 
>> +        int idx = currentBC->has_index_u4() ? currentBC->get_index_u4() : currentBC->get_index_u2_cpcache();
>> 
>> idx is used to index into the constant pool immediately after, so why is it asking for cpcache?
> 
> CP::name_and_type_ref_index_at and related accessors use CP::remap_instruction_operand_from_cache to interpret their indexes.  The idea is to take a "raw" index from the rewritten bytecodes and hand it to the CP, and have the CP interpret it correctly, whether it is a normal index or a cache index.  The CP accessors themselves are (in their appearance) ambiguous as to which kind of index each one takes.  The "uncached" versions make it clear they take a plain index, and suggest that the regular versions take cache indexes.  There is a comment paragraph around line 500 of constantPoolOop.hpp which explains this.

Ah, yes.  I'd forgotten that.  That looks fine.

> (I just noticed that the comment mentions byte swapping, which is out of date.  I'll change that, since byte swapping is now managed by the bytecode stream.)
> 
> It has worked like this for a long time, but recently I made it more obvious by making the bytecode stream accessors call out whether they were intending to pick up a cache index or regular index.  A next step could be to distinguish the CP accessors more clearly also.

It's certainly a bit confusing when you look at it closely.  Most of the time the distinction is ignored/invisible by users of the constant pools.

> 
>> Also why is this right?
>> 
>> +  bool            ref = ldc->has_cache_index() || cp->is_pointer_entry(ldc->pool_index());
>> 
>> What does has_cache_index have to do with ref'ness?
> 
> It's an internal detail:  ldc constants which are rewritten to use the cache are all oops.  Primitive constants stay put in the main CP.  Shall I put in a comment there?

That's what I figured.  Please do.  Is there an appropriate place where that assumption could be asserted?  Looks good otherwise.

tom

> 
> Thanks,
> -- John
> 
>> tom
>> 
>>> 
>>> The bug reproduces with ScavengeALot on an invokedynamic instruction and on a fast_aldc instruction (ldc/MH).  
>>> 
>>> Verified the fix.  Tried it with and without TraceNewOopMapGeneration.
>>> 
>>> -- John
>> 
>