tsan: fix bug in shadow reset introduced in D128909

[thanm]

Summary

Correct a bug in the code that resets shadow memory introduced as part
of a previous change for the Go race detector (D128909). The bug was
that only the most recently added shadow segment was being reset, as
opposed to the entire extent of the segment created so far. This
fixes a bug identified in Google internal testing (b/240733951).

[dvyukov]

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

203–204

Do we need the ctx->mapped_shadow_begin variable at all?
It's used in MapShadow, but only to update itself. So it looks we can remove it now.

But also what if the mapped region (ctx->mapped_shadow_begin) is far from ShadowBeg? Won't we mmap/memset potentially huge region (terabytes)? Maybe we need to fix how we update ctx->mapped_shadow_begin instead?

E.g. instead of:

if (ctx->mapped_shadow_begin < shadow_begin)
  ctx->mapped_shadow_begin = shadow_begin;

do something like:

if (!ctx->mapped_shadow_begin || ctx->mapped_shadow_begin > shadow_begin)
   ctx->mapped_shadow_begin = shadow_begin;

?

[thanm]

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

203–204

You are the expert here, I am arriving late on the scene. Happy to try your second suggestion.

Won't we mmap/memset potentially huge region (terabytes)?

What confuses me here is that for regular C/C++ tsan (e.g. not SANITIZER_GO) it's going to default to clearing ShadowBegin() through ShadowEnd(), which presumably is a very large region. Wouldn't your concern apply there too?

[dvyukov]

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

203–204

You are the expert here, I am arriving late on the scene. Happy to try your second suggestion.

It just looks like mapped_shadow_begin is currently maintained incorrectly (does not really point to the beginning of the used shadow region). Otherwise I don't know how to explain that extreme slowdown in the test we observed.

Wouldn't your concern apply there too?

C++ tsan does not work on Windows. And Windows is the reason we added these mapped_shadow_begin/end. On unixes it's fine to mmap terabytes of virtual address space.

[thanm]

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

203–204

Right, of course.

OK, this strategy:

if (!ctx->mapped_shadow_begin || ctx->mapped_shadow_begin > shadow_begin)
   ctx->mapped_shadow_begin = shadow_begin;

looks good-- fixes the google test timeout and seems to be clean with race.bat on windows. I will update the patch.

[thanm]

Switch shadow clear back to [mapped_shadow_begin, mapped_shadow_end]
but revise recipe for updating mapped_shadow_begin.

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

for (auto& slot : ctx->slots) {
     slot.thr = nullptr;
     ctx->slot_queue.PushBack(&slot);
   }
 
   DPrintf("Resetting shadow...\n");
   auto shadow_begin = ShadowBeg();
   auto shadow_end = ShadowEnd();
 #if SANITIZER_GO
   CHECK_NE(0, ctx->mapped_shadow_begin);
   shadow_begin = ctx->mapped_shadow_begin;
   shadow_end = ctx->mapped_shadow_end;
   VPrintf(2, "shadow_begin-shadow_end: (0x%zx-0x%zx)\n",
           shadow_begin, shadow_end);
 #endif
 
 #if SANITIZER_WINDOWS
   auto resetFailed =
       !ZeroMmapFixedRegion(shadow_begin, shadow_end - shadow_begin);
if (!data_mapped) {
     if (!MmapFixedSuperNoReserve(shadow_begin, shadow_end - shadow_begin, "shadow"))
       Die();
   } else {
     VPrintf(2, "ctx->mapped_shadow_{begin,end} = (0x%zx-0x%zx)\n",
             ctx->mapped_shadow_begin, ctx->mapped_shadow_end);
     // Second and subsequent calls map heap.
     if (shadow_end <= ctx->mapped_shadow_end)
       return;
-    if (ctx->mapped_shadow_begin < shadow_begin)
+    if (!ctx->mapped_shadow_begin || ctx->mapped_shadow_begin > shadow_begin)
        ctx->mapped_shadow_begin = shadow_begin;
     if (shadow_begin < ctx->mapped_shadow_end)
       shadow_begin = ctx->mapped_shadow_end;
     VPrintf(2, "MapShadow begin/end = (0x%zx-0x%zx)\n",
             shadow_begin, shadow_end);
     if (!MmapFixedSuperNoReserve(shadow_begin, shadow_end - shadow_begin,
                                  "shadow"))
       Die();
     ctx->mapped_shadow_end = shadow_end;