[PATCH] D104076: [clang-cl][sanitizer] Add -fsanitize-address-use-after-return to clang.

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka accepted this revision.
vitalybuka added a comment.
This revision is now accepted and ready to land.

LGTM with some nits and if you extract FunctionStackPoisoner::initializeCallbacks into a separate patch



================
Comment at: clang/test/CodeGen/asan-use-after-return.cpp:3
+// RUN:     | FileCheck %s --check-prefixes=CHECK-RUNTIME \
+// RUN:         --implicit-check-not="call{{.*}}__asan_stack_malloc_always_"
+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-linux %s \
----------------
I guess call{{.*}} can be removed from implicit-check-not?


================
Comment at: clang/test/Driver/fsanitize-use-after-return.c:17-20
+// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
+// RUN:   -fsanitize-address-use-after-return=always %s -### 2>&1 | \
+// RUN:   FileCheck -check-prefix=CHECK-ALWAYS-ARG %s
+// CHECK-ALWAYS-ARG: "-fsanitize-address-use-after-return=always"
----------------
we also want test like this:

```
// RUN: %clang -target x86_64-apple-macosx10.15-gnu -fsanitize=address \
// RUN:   -fsanitize-address-use-after-return=never -fsanitize-address-use-after-return=always %s -### 2>&1 | \
// RUN:   FileCheck -check-prefix=CHECK-ALWAYS %s
```


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizer.h:105
+      bool UseAfterScope = false,
+      llvm::AsanDetectStackUseAfterReturnMode UseAfterReturn =
+          llvm::AsanDetectStackUseAfterReturnMode::Never);
----------------
it's already in llvm:: namespace


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:637
+                   bool UseAfterScope = false,
+                   llvm::AsanDetectStackUseAfterReturnMode UseAfterReturn =
+                       llvm::AsanDetectStackUseAfterReturnMode::Never)
----------------
-llvm::


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:652-654
+    if (ClUseAfterReturn != ClUseAfterReturn.getDefault().getValue()) {
+      this->UseAfterReturn = ClUseAfterReturn;
+    }
----------------
CL should override argument even if it's default:

maybe at line 643: 
UseAfterReturn(ClUseAfterReturn.getNumOccurrences() ? ClUseAfterReturn : UseAfterReturn)



================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:771
+      bool UseAfterScope = false,
+      llvm::AsanDetectStackUseAfterReturnMode UseAfterReturn =
+          llvm::AsanDetectStackUseAfterReturnMode::Never)
----------------
drop llvm::


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:2973
   IRBuilder<> IRB(*C);
-  switch (ClUseAfterReturn) {
-  case AsanDetectStackUseAfterReturnMode::Always:
-    for (int i = 0; i <= kMaxAsanStackMallocSizeClass; i++) {
-      std::string Suffix = itostr(i);
-      AsanStackMallocFunc[i] = M.getOrInsertFunction(
-          kAsanStackMallocAlwaysNameTemplate + Suffix, IntptrTy, IntptrTy);
-      AsanStackFreeFunc[i] =
-          M.getOrInsertFunction(kAsanStackFreeNameTemplate + Suffix,
-                                IRB.getVoidTy(), IntptrTy, IntptrTy);
-    }
-    break;
-  case AsanDetectStackUseAfterReturnMode::Runtime:
-    for (int i = 0; i <= kMaxAsanStackMallocSizeClass; i++) {
-      std::string Suffix = itostr(i);
-      AsanStackMallocFunc[i] = M.getOrInsertFunction(
-          kAsanStackMallocNameTemplate + Suffix, IntptrTy, IntptrTy);
-      AsanStackFreeFunc[i] =
-          M.getOrInsertFunction(kAsanStackFreeNameTemplate + Suffix,
-                                IRB.getVoidTy(), IntptrTy, IntptrTy);
-    }
-    break;
-  case AsanDetectStackUseAfterReturnMode::Never:
-    // Do Nothing
-    break;
-  case AsanDetectStackUseAfterReturnMode::Invalid:
-    // Do Nothing
-    break;
+  const char *kMallocNameTemplate = kAsanStackMallocNameTemplate;
+  if (ASan.UseAfterReturn == AsanDetectStackUseAfterReturnMode::Always) {
----------------
it looks like unrelated patch


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:2973
   IRBuilder<> IRB(*C);
-  switch (ClUseAfterReturn) {
-  case AsanDetectStackUseAfterReturnMode::Always:
-    for (int i = 0; i <= kMaxAsanStackMallocSizeClass; i++) {
-      std::string Suffix = itostr(i);
-      AsanStackMallocFunc[i] = M.getOrInsertFunction(
-          kAsanStackMallocAlwaysNameTemplate + Suffix, IntptrTy, IntptrTy);
-      AsanStackFreeFunc[i] =
-          M.getOrInsertFunction(kAsanStackFreeNameTemplate + Suffix,
-                                IRB.getVoidTy(), IntptrTy, IntptrTy);
-    }
-    break;
-  case AsanDetectStackUseAfterReturnMode::Runtime:
-    for (int i = 0; i <= kMaxAsanStackMallocSizeClass; i++) {
-      std::string Suffix = itostr(i);
-      AsanStackMallocFunc[i] = M.getOrInsertFunction(
-          kAsanStackMallocNameTemplate + Suffix, IntptrTy, IntptrTy);
-      AsanStackFreeFunc[i] =
-          M.getOrInsertFunction(kAsanStackFreeNameTemplate + Suffix,
-                                IRB.getVoidTy(), IntptrTy, IntptrTy);
-    }
-    break;
-  case AsanDetectStackUseAfterReturnMode::Never:
-    // Do Nothing
-    break;
-  case AsanDetectStackUseAfterReturnMode::Invalid:
-    // Do Nothing
-    break;
+  const char *kMallocNameTemplate = kAsanStackMallocNameTemplate;
+  if (ASan.UseAfterReturn == AsanDetectStackUseAfterReturnMode::Always) {
----------------
vitalybuka wrote:
> it looks like unrelated patch
could you please fix clang-tidy: warnings


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:2973
   IRBuilder<> IRB(*C);
-  switch (ClUseAfterReturn) {
-  case AsanDetectStackUseAfterReturnMode::Always:
-    for (int i = 0; i <= kMaxAsanStackMallocSizeClass; i++) {
-      std::string Suffix = itostr(i);
-      AsanStackMallocFunc[i] = M.getOrInsertFunction(
-          kAsanStackMallocAlwaysNameTemplate + Suffix, IntptrTy, IntptrTy);
-      AsanStackFreeFunc[i] =
-          M.getOrInsertFunction(kAsanStackFreeNameTemplate + Suffix,
-                                IRB.getVoidTy(), IntptrTy, IntptrTy);
-    }
-    break;
-  case AsanDetectStackUseAfterReturnMode::Runtime:
-    for (int i = 0; i <= kMaxAsanStackMallocSizeClass; i++) {
-      std::string Suffix = itostr(i);
-      AsanStackMallocFunc[i] = M.getOrInsertFunction(
-          kAsanStackMallocNameTemplate + Suffix, IntptrTy, IntptrTy);
-      AsanStackFreeFunc[i] =
-          M.getOrInsertFunction(kAsanStackFreeNameTemplate + Suffix,
-                                IRB.getVoidTy(), IntptrTy, IntptrTy);
-    }
-    break;
-  case AsanDetectStackUseAfterReturnMode::Never:
-    // Do Nothing
-    break;
-  case AsanDetectStackUseAfterReturnMode::Invalid:
-    // Do Nothing
-    break;
+  const char *kMallocNameTemplate = kAsanStackMallocNameTemplate;
+  if (ASan.UseAfterReturn == AsanDetectStackUseAfterReturnMode::Always) {
----------------
vitalybuka wrote:
> vitalybuka wrote:
> > it looks like unrelated patch
> could you please fix clang-tidy: warnings
now we insert functions also for Never and Invalid?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104076