[PATCH] D65730: [Sanitizer] Linux Shadow mapping explicit thp support when in madvise mode

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp:71
 
-bool NoHugePagesInRegion(uptr addr, uptr size) {
+bool HugePagesModeInRegion(uptr addr, uptr size) {
 #ifdef MADV_NOHUGEPAGE  // May not be defined on old systems.
----------------
This patch moved no_huge_pages_for_shadow into function, now it's about shadow.
bool SetShadowRegionHugePageMode()

This patch does two things:
1. changes NoHugePagesInRegion -> SetShadowRegionHugePageMode
2. adds transparent_hugepages

May I ask you to split that into two patches?


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp:74
+  char *thp_file;
+  uptr thp_file_len;
+  uptr len;
----------------
move var declarations close to first use


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp:78
+  if (mode == -1) {
+      mode = 0;
+      if (ReadFileToBuffer("/sys/kernel/mm/transparent_hugepages/enabled",
----------------
Can you please move 73-87 in to some function

```
enum TransparentHugePagesMode {
}

TransparentHugePagesMode GetTransparentHugePagesMode {
 static TransparentHugePagesMode mode = []() {
    ...
 }
 return mode;
}
```


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp:81
+      &thp_file, &thp_file_len, &len)) {
+        if (internal_strstr(thp_file, "[madvise]"))
+          mode = 1;
----------------
This patch changes behavior for [madvise] 
Are there particular platform you care about which uses that?


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp:88
+  }
+  if (common_flags()->no_huge_pages_for_shadow)
+    return madvise((char *)addr, size, MADV_NOHUGEPAGE) == 0;
----------------
"if (common_flags()->no_huge_pages_for_shadow)" is cheep so it should go before transparent_hugepages/ check


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp:91
+  else if (mode == 1)
+    return madvise((char *)addr, size, MADV_HUGEPAGE) == 0;
+  return true;
----------------
Can we just remove transparent_hugepages/enabled check
and always do: "advise((char *)addr, size, MADV_HUGEPAGE) == 0;" ?




Repository:
  rCRT Compiler Runtime

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65730/new/

https://reviews.llvm.org/D65730