[PATCH] D108323: [asan] Added -inline-small-callbacks LLVM flag, which would force inline code for 8 and 16 byte data types when otherwise a callback would have been used.

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.



Comment at: clang/test/CodeGen/asan-use-callbacks.cpp:15
 
-int deref(int *p) {
+long deref(long *p) {
   return *p;

kcc wrote:
> As we introduce a difference in behavior for small and large accesses, 
> I would extend this test to have 1, 2, 4, and 16-byte accesses. 
changes in llvm/lib should be tested in corresponding llvm/test, not clang/test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108323

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108323: [asan] Added -inline-small-callbacks LLVM flag, which would force inline code for 8 and 16 byte data types when otherwise a callback would have been used.

[Kostya Serebryany via Phabricator via cfe-commits]

kcc added a comment.

What's the code size implications?




Comment at: clang/test/CodeGen/asan-use-callbacks.cpp:15
 
-int deref(int *p) {
+long deref(long *p) {
   return *p;

As we introduce a difference in behavior for small and large accesses, 
I would extend this test to have 1, 2, 4, and 16-byte accesses. 



Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:310
+static cl::opt ClInlineSmallCallbacks(
+"asan-inline-small-callbacks",
+cl::desc("Inline callbacks for 8 and 16 byte types."), cl::Hidden,

The flag semantics are weird. We first say "use callbacks", then we say "but 
not for small callbacks". 
Perhaps, instead, introduce a flag 
asan-short-instrumentation-with-call-threshold
and when present use this flag for 8/16 instead of the 
asan-instrumentation-with-call-threshold?



Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1749
 
+  if (ClInlineSmallCallbacks && AccessSizeIndex > 2) {
+UseCalls = false;

perhaps move this logic to where UseCalls is computed initially?
(see my other comment too)



Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1751
+UseCalls = false;
+  }
+

for single-statement if bodies this code base does not use {}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108323

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108323: [asan] Added -inline-small-callbacks LLVM flag, which would force inline code for 8 and 16 byte data types when otherwise a callback would have been used.

[Kirill Stoimenov via Phabricator via cfe-commits]

kstoimenov created this revision.
kstoimenov added a reviewer: vitalybuka.
Herald added a subscriber: hiraditya.
kstoimenov requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

The reason to inline the 8 and 16 byte access checks is that the code for those 
is smaller than the 1, 2 and 4 byte checks. This allows to have a balanced, 
middle ground setting between size and speed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108323

Files:
  clang/test/CodeGen/asan-use-callbacks.cpp
  llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp


Index: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
===
--- llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -306,6 +306,11 @@
 cl::desc("Realign stack to the value of this flag (power of two)"),
 cl::Hidden, cl::init(32));
 
+static cl::opt ClInlineSmallCallbacks(
+"asan-inline-small-callbacks",
+cl::desc("Inline callbacks for 8 and 16 byte types."), cl::Hidden,
+cl::init(false));
+
 static cl::opt ClInstrumentationWithCallsThreshold(
 "asan-instrumentation-with-call-threshold",
 cl::desc(
@@ -1741,6 +1746,10 @@
   Value *AddrLong = IRB.CreatePointerCast(Addr, IntptrTy);
   size_t AccessSizeIndex = TypeSizeToSizeIndex(TypeSize);
 
+  if (ClInlineSmallCallbacks && AccessSizeIndex > 2) {
+UseCalls = false;
+  }
+
   if (UseCalls) {
 if (Exp == 0)
   IRB.CreateCall(AsanMemoryAccessCallback[IsWrite][0][AccessSizeIndex],
Index: clang/test/CodeGen/asan-use-callbacks.cpp
===
--- clang/test/CodeGen/asan-use-callbacks.cpp
+++ clang/test/CodeGen/asan-use-callbacks.cpp
@@ -4,10 +4,14 @@
 // RUN: %clang -target x86_64-linux-gnu -S -emit-llvm -o - \
 // RUN: -fsanitize=address %s -fsanitize-address-outline-instrumentation \
 // RUN: | FileCheck %s --check-prefixes=CHECK-OUTLINE
+// RUN: %clang -target x86_64-linux-gnu -S -emit-llvm -o - \
+// RUN: -fsanitize=address %s -fsanitize-address-outline-instrumentation \
+// RUN: -mllvm -asan-inline-small-callbacks \
+// RUN: | FileCheck %s --check-prefixes=CHECK-NO-OUTLINE
 
-// CHECK-NO-OUTLINE-NOT: call{{.*}}@__asan_load4
-// CHECK-OUTLINE: call{{.*}}@__asan_load4
+// CHECK-NO-OUTLINE-NOT: call{{.*}}@__asan_load8
+// CHECK-OUTLINE: call{{.*}}@__asan_load8
 
-int deref(int *p) {
+long deref(long *p) {
   return *p;
 }


Index: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
===
--- llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -306,6 +306,11 @@
 cl::desc("Realign stack to the value of this flag (power of two)"),
 cl::Hidden, cl::init(32));
 
+static cl::opt ClInlineSmallCallbacks(
+"asan-inline-small-callbacks",
+cl::desc("Inline callbacks for 8 and 16 byte types."), cl::Hidden,
+cl::init(false));
+
 static cl::opt ClInstrumentationWithCallsThreshold(
 "asan-instrumentation-with-call-threshold",
 cl::desc(
@@ -1741,6 +1746,10 @@
   Value *AddrLong = IRB.CreatePointerCast(Addr, IntptrTy);
   size_t AccessSizeIndex = TypeSizeToSizeIndex(TypeSize);
 
+  if (ClInlineSmallCallbacks && AccessSizeIndex > 2) {
+UseCalls = false;
+  }
+
   if (UseCalls) {
 if (Exp == 0)
   IRB.CreateCall(AsanMemoryAccessCallback[IsWrite][0][AccessSizeIndex],
Index: clang/test/CodeGen/asan-use-callbacks.cpp
===
--- clang/test/CodeGen/asan-use-callbacks.cpp
+++ clang/test/CodeGen/asan-use-callbacks.cpp
@@ -4,10 +4,14 @@
 // RUN: %clang -target x86_64-linux-gnu -S -emit-llvm -o - \
 // RUN: -fsanitize=address %s -fsanitize-address-outline-instrumentation \
 // RUN: | FileCheck %s --check-prefixes=CHECK-OUTLINE
+// RUN: %clang -target x86_64-linux-gnu -S -emit-llvm -o - \
+// RUN: -fsanitize=address %s -fsanitize-address-outline-instrumentation \
+// RUN: -mllvm -asan-inline-small-callbacks \
+// RUN: | FileCheck %s --check-prefixes=CHECK-NO-OUTLINE
 
-// CHECK-NO-OUTLINE-NOT: call{{.*}}@__asan_load4
-// CHECK-OUTLINE: call{{.*}}@__asan_load4
+// CHECK-NO-OUTLINE-NOT: call{{.*}}@__asan_load8
+// CHECK-OUTLINE: call{{.*}}@__asan_load8
 
-int deref(int *p) {
+long deref(long *p) {
   return *p;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits