RFR: 8075466: Address not aligned in Klass::decode_klass_not_null

[Kim Barrett]

On Apr 13, 2015, at 7:12 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
> 
> On 13/04/15 04:47, Kim Barrett wrote:
>> Please review this change to the filtering of SATB queue contents.
>> 
>> The problem is that a SATB queue may contain a stale reference to an
>> eagerly reclaimed humongous object.  Assert-conditional verification
>> during full queue processing that all entries are oop's can now fail.
>> 
>> We deal with this by eliminating the pre-filtering verification
>> (removed call to ObjPtrQueue::verify_oops_in_buffer and removed that
>> now unused function).  Instead, ObjPtrQueue::filter has been revised
>> to be more careful about oop-ness assumptions, and to assert the
>> oop-ness of entries that are retained.
>> 
>> This is related to
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8073717
>> 
>> which is another failure due to unexpected stuff in SATB queues as a
>> result of eager reclaim of a humongous object.
>> 
>> As an aside, ObjPtrQueue is now poorly named.  A followup CR will be
>> filed to rename it to something like SATBQueue.
>> 
>> CR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8075466
>> 
>> 
>> Webrev:
>> 
>> http://cr.openjdk.java.net/~kbarrett/8075466/webrev.00/
> 
> Nice refactoring and fix! Some minor comments:

Bengt,

Thanks for looking at this.  Responses inline.

> satbQueue.cpp
> 
> What's the advantage of casting to HeapWord** and HeapWord* in ObjPtrQueue::filter() rather than just working with void** and void* when popping from and pushing to the "buf”?

The old code was casting to oop*, which is semantically wrong now. I
think there was a point during development of this change where
HeapWord* was needed, but you are right, the current code doesn't need
it and can just use void* and void**.  I will make that change.

> 83   if (entry >= region->next_top_at_mark_start()) return false;
> 
> Please add some brackets to this if statement.
> 
> if (entry >= region->next_top_at_mark_start()) {
>   return false;
> }

HotSpotStyleGuide suggests it might be ok as is:

  "Use braces around substatements. (Relaxable for extremely simple
  substatements on the same line.)"

OTOH, the return out there at the end of a relatively long line could
be easily overlooked, especially without syntax highlighting.  And
we're not always reading code in a context where syntax highlighting
is provided.

All of which is a long-winded way of saying, sure, I'll make that
change. 

> 85   // Can obj really have it's mark word set?  It's not in young gen...
> 86   assert(((oop)entry)->is_oop(true /* ignore mark word */),
> 87          err_msg("Invalid oop in SATB buffer: " PTR_FORMAT, p2i(entry)));
> 
> What does the comment mean? This is concurrent marking so anyone can be manipulating the mark work. Locking etc. I think that is why the assert avoids checking the mark word.

I haven't previously needed to go delving into any of the details of
the mark word and its manipulation.  That was interesting.  I'll be
removing the comment.

On the other hand, I still don't see why we *need* to ignore the mark
word here.  Ignoring it avoids reading it at all, but none of the
further checking there appears to be troublesome, though perhaps not
very interesting.