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

Arthur Eubanks aeubanks at google.com
Fri Jun 28 00:45:50 UTC 2019


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
>


More information about the tsan-dev mailing list