RFR (L): Add TSAN instrumentation for allocation and garbage collection

[Man Cao]

I've addressed all cleanup/refactoring comments except this one:

> Should this instead be an assert and we do the check for ThreadSanitizer
in the caller?
>  456   if (!ThreadSanitizer) return;
It would be ugly to make WeakProcessor conditionally run
TsanOopMap::weak_oops_do(), due to its use of function pointers.
JvmtiExport::weak_oops_do and Jfr::weak_oops_do have similar checks that
guards their entire execution.

I also updated comment at the top of tsanOopMap.hpp, as it tracks all Java
objects instead of only lock objects, and fixed a few compiler warnings.
Also removed indention for "private:" / "public:" keywords, per latest
OpenJDK style.

https://cr.openjdk.java.net/~manc/tsanOopMap/webrev.00-01.diff/
https://cr.openjdk.java.net/~manc/tsanOopMap/webrev.01/

-Man


On Fri, Jun 28, 2019 at 12:33 PM Man Cao <manc at google.com> wrote:

> I don't think it's worth it to change the algorithm for
> TsanOopSizeMap::rebuild_oops_map at this point. If want to improve
> performance, we should first think for better ways for tracking
> allocations, or parallelize TsanOopSizeMap::rebuild_oops_map. I'm happy to
> do other cleanup and refactoring, though.
>
> -Man
>
>
> On Fri, Jun 28, 2019 at 11:34 AM Arthur Eubanks <aeubanks at google.com>
> wrote:
>
>> I wonder if for the part in TsanOopSizeMap::rebuild_oops_map where we
>> tell TSAN about moved objects, instead of repeatedly doing passes until
>> everything is moved (which as mentioned in the comments has a worst case of
>> O(N^2)), we create a new fake heap area, for each oop we pretend to move it
>> to the new fake heap area in a contiguous chunk of memory, then pretend to
>> move each oop from the new fake area to the final location after the GC
>> move. That would be O(N) and would (I think) simplify the code.
>> TSAN <https://clang.llvm.org/docs/ThreadSanitizer.html> already has
>> 5-10x memory overhead.
>>
>> On Thu, Jun 27, 2019 at 5:45 PM Arthur Eubanks <aeubanks at google.com>
>> wrote:
>>
>>> Could you change the comment in tsanOopMap.hpp
>>>   39 // 1. add() is only passed a live oop.
>>>   40 // 2. add() must be thread-safe wrt itself.
>>> "add()" -> "add_*()"
>>>
>>>
>>> Another comment in tsanExternalDecls.hpp which seems inaccurate
>>> +  // Called lazily when an oop should start to be tracked by Tsan.
>>> It seems like we're calling __tsan_java_alloc() eagerly, not lazily.
>>>
>>>
>>> tsanOopMap.cpp:
>>>
>>> We fixed this, comment is wrong:
>>>  448 // TODO: We never call __tsan_java_fini; we should.
>>> And right above, maybe we could deallocate where we call
>>> __tsan_java_fini?
>>>  447 // TODO: We never deallocate the oopMap; we should.
>>>
>>> Should this instead be an assert and we do the check for ThreadSanitizer
>>> in the caller?
>>>  456   if (!ThreadSanitizer) return;
>>>
>>>
>>> I also need more time to review rebuild_oops_map() :).
>>>
>>> On Thu, Jun 27, 2019 at 2:49 PM Jean Christophe Beyler <
>>> jcbeyler at google.com> wrote:
>>>
>>>> Hi Man,
>>>>
>>>>
>>>> https://cr.openjdk.java.net/~manc/tsanOopMap/webrev.00/src/hotspot/share/runtime/sharedRuntime.cpp.udiff.html
>>>> // Note Well.
>>>> ->
>>>> // NOTE:
>>>>
>>>>
>>>> https://cr.openjdk.java.net/~manc/tsanOopMap/webrev.00/test/hotspot/jtreg/tsan/NonRacyGarbageCollectionLoopTest.java.html
>>>> -> I would put a note somewhere that if you change the 40 for HEAP_SIZE
>>>> you
>>>> should change the Xms/Xmx... (I imagined we could create the Xms/Xmx
>>>> with
>>>> string concatenation but that seems overkill)
>>>>
>>>>
>>>> https://cr.openjdk.java.net/~manc/tsanOopMap/webrev.00/src/hotspot/share/tsan/tsanOopMap.cpp.html
>>>>    - I find TsanOopBucket is not the right name, it might be TsanOop
>>>>
>>>>   -  120       do {
>>>>  121         h = hash(h);
>>>>  122         bucket = &buckets[h % _size];
>>>>  123       } while (bucket->has_oop() && bucket->get_oop() != o);
>>>>
>>>> This could be put in a helper method because done twice, no?
>>>>
>>>> - print_map is not used, is it?
>>>> - oopMap should be oop_map no?
>>>>
>>>> I admit I have not finished looking at rebuild_oops_map, it is a bit big
>>>> ;-) could we break it up?
>>>>
>>>> Thanks,
>>>> Jc
>>>>
>>>