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

Kim Barrett kim.barrett at oracle.com
Mon Apr 13 17:15:23 UTC 2015

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:


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

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

More information about the hotspot-gc-dev mailing list