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

[John Rose]

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.

(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.

> 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?

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
>