Crash on finalizer in tsanOopMap

[Jean Christophe Beyler]

Yeah, I was wondering if it was just a bit too relaxed but yes better than
nothing :)

LGTM,
Jc

On Fri, Jul 12, 2019 at 10:40 AM Arthur Eubanks <aeubanks at google.com> wrote:

> LGTM
>
> On Fri, Jul 12, 2019 at 9:19 AM Man Cao <manc at google.com> wrote:
>
>> Yes, any valid oop pointer should be in the reserved space. Here the code
>> is not allocating anything on the heap, and pointer_adjuster->do_oop()
>> typically just loads from the header word and updates the holder (
>> target_oop).
>>
>> So this assert just checks if target_oop looks like a valid pointer
>> after adjusting. It is a quite relaxed check, but better than nothing.
>>
>> -Man
>>
>> On Thu, Jul 11, 2019, 19:22 Jean Christophe Beyler <jcbeyler at google.com>
>> wrote:
>>
>>> Just by curiosity, isn't any memory allocated in the heap in the
>>> reserved space? So essentially, this assert is no longer really checking
>>> for anything?
>>>
>>> Or does this actually do what we want? :-)
>>> Jc
>>>
>>> On Thu, Jul 11, 2019 at 6:09 PM Man Cao <manc at google.com> wrote:
>>>
>>>> Can I have reviews for this bug fix?
>>>> https://cr.openjdk.java.net/~manc/tsan20190711/webrev.00/
>>>> The assertion check is too strong for target_oop after calling
>>>> pointer_adjuster->do_oop().
>>>>
>>>>
>>>> -Man
>>>>
>>>>
>>>> On Thu, Jul 11, 2019 at 10:52 AM Arthur Eubanks <aeubanks at google.com>
>>>> wrote:
>>>>
>>>> > Here it is:
>>>> >
>>>> http://cr.openjdk.java.net/~aeubanks/tsanfinalizercrash/NonRacyFinalizerLoopTest.java
>>>> >
>>>> > On Wed, Jul 10, 2019 at 4:59 PM Man Cao <manc at google.com> wrote:
>>>> >
>>>> >> I don't see the attached reproducer. I don't think you could add
>>>> >> attachment on the mailing list though.
>>>> >> Could you inline it in the email or put it on webrev?
>>>> >>
>>>> >> -Man
>>>> >>
>>>> >>
>>>> >> On Mon, Jul 8, 2019 at 1:23 PM Arthur Eubanks <aeubanks at google.com>
>>>> >> wrote:
>>>> >>
>>>> >>> I have a simpler reproducer attached.
>>>> >>>
>>>> >>> Some logging:
>>>> >>>            // The object survived GC, add its updated oop to the
>>>> new oops
>>>> >>> map.
>>>> >>>            oop target_oop = cast_to_oop((intptr_t)source_obj);
>>>> >>>            pointer_adjuster->do_oop(&target_oop);
>>>> >>> +          oopDesc *target_obj = target_oop;
>>>> >>> +          if (!heap->is_in(target_oop)) {
>>>> >>> +            fprintf(stderr, "%p\n", source_obj);
>>>> >>> +            fprintf(stderr, "%p\n", target_obj);
>>>> >>> +          }
>>>> >>>            assert(heap->is_in(target_oop), "Adjustment failed");
>>>> >>> -          oopDesc *target_obj = target_oop;
>>>> >>>            new_map->put(target_obj, obj_size);
>>>> >>>
>>>> >>> 0xffea5570
>>>> >>> 0xf8238138
>>>> >>>
>>>> >>> On Wed, Jul 3, 2019 at 10:44 AM Arthur Eubanks <aeubanks at google.com
>>>> >
>>>> >>> wrote:
>>>> >>>
>>>> >>> > Trying to implement finalizer support, I created a test
>>>> (attached), and
>>>> >>> > tsanOopMap.cpp seems buggy.
>>>> >>> >
>>>> >>> > # after -XX: or in .hotspotrc:
>>>> SuppressErrorAt=/tsanOopMap.cpp:294
>>>> >>> > #
>>>> >>> > # A fatal error has been detected by the Java Runtime Environment:
>>>> >>> > #
>>>> >>> > #  Internal Error
>>>> >>> >
>>>> >>>
>>>> (/usr/local/google/home/aeubanks/jdk/tsan/src/hotspot/share/tsan/tsanOopMap.cpp:294),
>>>> >>> > pid=16182, tid=16188
>>>> >>> > #  assert(heap->is_in(target_oop)) failed: Adjustment failed
>>>> >>> > #
>>>> >>> > # JRE version: OpenJDK Runtime Environment (13.0) (fastdebug build
>>>> >>> > 13-internal+0-adhoc.aeubanks.tsan)
>>>> >>> > # Java VM: OpenJDK 64-Bit Server VM (fastdebug
>>>> >>> > 13-internal+0-adhoc.aeubanks.tsan, interpreted mode, tiered,
>>>> compressed
>>>> >>> > oops, g1 gc, linux-amd64)
>>>> >>> > # Problematic frame:
>>>> >>> > # V  [libjvm.so+0x164e63b]
>>>> >>> >  TsanOopMapImpl::TsanOopSizeMap::collect_oops(BoolObjectClosure*,
>>>> >>> > OopClosure*, GrowableArray<TsanOopMapImpl::PendingMove>*, int*,
>>>> char**,
>>>> >>> > char**)+0x24b
>>>> >>> > #
>>>> >>> > # No core dump will be written. Core dumps have been disabled. To
>>>> >>> enable
>>>> >>> > core dumping, try "ulimit -c unlimited" before starting Java again
>>>> >>> > #
>>>> >>> > # An error report file with more information is saved as:
>>>> >>> > #
>>>> >>> >
>>>> >>>
>>>> /usr/local/google/home/aeubanks/jdk/tsan/build/test-support/jtreg_test_hotspot_jtreg_tsan_NonRacyFinalizerLoopTest_java/scratch/0/hs_err_pid16182.log
>>>> >>> > #
>>>> >>> > # If you would like to submit a bug report, please visit:
>>>> >>> > #   http://bugreport.java.com/bugreport/crash.jsp
>>>> >>> > #
>>>> >>> >
>>>> >>>
>>>> >>
>>>>
>>>
>>>
>>> --
>>>
>>> Thanks,
>>> Jc
>>>
>>

-- 

Thanks,
Jc