null-check in G1KeepAliveClosure::do_oop() - needed?

[Thomas Schatzl]

Hi,

On Mon, 2014-09-08 at 10:19 -0400, Tony Printezis wrote:
> Hi all,
> 
> I was a bit perplexed by the following code:
> 
> class G1KeepAliveClosure: public OopClosure {
>    ...
>    void do_oop(oop* p) {
>      oop obj = *p;
> 
>      G1CollectedHeap::in_cset_state_t cset_state = _g1->in_cset_state(obj);
>      if (obj == NULL || cset_state == G1CollectedHeap::InNeither) {
>        return;
>      }
> 
> Is the NULL check needed (can obj actually be NULL)?

No, does not seem so. The only use of G1KeepAliveClosure seems to be the
use in JNIHandleBlock::weak_oops_do. That one already filters out NULL
values.

> If it is, why is it 
> done after the look-up in in_cset_state()? And is in_cset_state() 
> actually robust wrt obj being NULL?

No. This is a bug. If you have time, please change the NULL check to an
assert and remove it in the regular code.

Thanks,
  Thomas