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

[Bengt Rutisson]

Hi Kim,

On 2015-04-13 19:15, Kim Barrett wrote:
> 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.

Sounds good. Thanks.

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

:) Great. Thanks!

I'm surprised that the Hotspot style guidelines have the relaxed 
comment. Thanks for pointing that out. Personally I really prefer to 
always have braces around substatements since I've seen too many cases 
where it's gone bad when someone adds one more line to something they 
think is scoped.

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

Yes, I think you are right, that it should be safe to read the mark word 
here. Removing the comment is still the right thing to do. I guess the 
question is if we should also change the ignore mark word parameter to 
false. As you point out there is not much extra checking happening even 
if you do that. So, I would be inclined to leave it as is right now. 
Your call.

Thanks,
Bengt

>