[PATCH] D18003: [tsan] Disable randomized address space on linux when necessary.

[Dmitry Vyukov via llvm-commits]

dvyukov added inline comments.

================
Comment at: lib/tsan/rtl/tsan_platform_linux.cc:267
@@ -265,1 +266,3 @@
 
+static uptr GetHeapEnd() {
+  MemoryMappingLayout proc_maps(true);
----------------
Please call it somehow differently from "Heap". Heap commonly means "malloc heap" throughout the runtime, as is HeapMemEnd below.
I think we are mainly looking for mmaped modules here, so "ModulesEnd" would be fine. "MmapEnd" would be fine as well.


================
Comment at: lib/tsan/rtl/tsan_platform_linux.cc:272
@@ +271,3 @@
+  while (proc_maps.Next(&start, &end, 0, 0, 0, &prot)) {
+    // Omit stack
+    if (start <= (uptr)&heap_end && (uptr)&heap_end < end)
----------------
Please explain why we do this.

================
Comment at: lib/tsan/rtl/tsan_platform_linux.cc:311
@@ +310,3 @@
+    if (old_personality != -1 && (old_personality & ADDR_NO_RANDOMIZE) == 0) {
+      if (GetHeapEnd() < HeapMemEnd()) {
+        Report("WARNING: Program is run with randomized virtual address space,"
----------------
I am trying to convince myself that it won't break any of linux setups. We can have pie/non-pie binary. COMPAT mapping (setarch -L) is not supported now on x86 (modules mapped at 0x2a). Also disabled randomization is _not_ supported on x86 (modules mapped at 0x55).
So you have a good explanation as to why it all will continue to work?
I guess that we always have at least 1 dynamic library (static libc linking is not supported), so on x86 glibc should be mapped at 0x7f in supported configuration (randomization + no compat mapping). So GetHeapEnd should return 0x7f, and that's larger than HeapMemEnd.

But I think it will make failure mode for at least x86+COMPAT mapping much worse (infinite exec recursion instead of a readable message). Potentially there are other cases on other platforms (e.g. power/mips) when that will happen as well. Or maybe they legally have modules below heap?

I think it makes sense to tread more gradually and enable it just for aarch64 with an explanation as to why we are doing this. Other platforms will be enable to enable this by just altering ifdef condition later if necessary.

Or am I missing something?




http://reviews.llvm.org/D18003