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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits