On Wed, Oct 23, 2013 at 2:58 PM, Nuno Lopes <[email protected]> wrote:
> Let me disagree with this change: > - I do not agree that '-fsanitize=local-bounds' is not a good compromise. > It can detect overflows on globals, heap, and stack, including arbitrary > pointer arithmetic, and therefore it subsumes '-fsanitize=array-bounds'. > The advantage of the latter is that it produces nice diagnostics, while the > former simply crashes the program when it detects an overflow. Therefore, I > think there's value in keeping *both* instrumentations in > -fsanitize=undefined. Yes, there is value in keeping both. But that doesn't argue for keeping this check in -fsanitize=undefined specifically -- all the other checks in -fsanitize=undefined are pure frontend checks that check source language rules and give nice diagnostics. It doesn't fit there. And if you run with "-fsanitize=address,undefined", this particular check provides no value. (-fsanitize=array-bounds does provide in such a configuration value, because it checks the language model for pointer arithmetic, not the LLVM IR rules, and so can detect some issues that ASan misses; -fsanitize=local-bounds is a pure subset of ASan.) Another issue is that local-bounds works by inserting a pass, which means that it does not work with LTO or in any other flow where the optimization passes are separated from IR emission. > BTW, '-fsanitize=local-bounds' properly inserts debug information in the > trap, and therefore you can use a debugger to pinpoint the offending > program statement. > - '-fsanitize=local-bounds' does not have false positives by design. > PR17653 is a bug which I'll fix soon. This pass was designed to have zero > false positives. > - Also, I don't recall any previous discussion on this change.. Nuno > > ----- Original Message ----- From: "Richard Smith" < > [email protected]> > Sent: Tuesday, October 22, 2013 11:51 PM > Subject: r193205 - Split -fsanitize=bounds to -fsanitize=array-bounds (for > thefrontend-inserted > > > > Author: rsmith >> Date: Tue Oct 22 17:51:04 2013 >> New Revision: 193205 >> >> URL: >> http://llvm.org/viewvc/llvm-**project?rev=193205&view=rev<http://llvm.org/viewvc/llvm-project?rev=193205&view=rev> >> Log: >> Split -fsanitize=bounds to -fsanitize=array-bounds (for the >> frontend-inserted >> check using the ubsan runtime) and -fsanitize=local-bounds (for the >> middle-end >> check which inserts traps). >> >> Remove -fsanitize=local-bounds from -fsanitize=undefined. It does not >> produce >> useful diagnostics and has false positives (PR17635), and is not a good >> compromise position between UBSan's checks and ASan's checks. >> >> Map -fbounds-checking to -fsanitize=local-bounds to restore Clang's >> historical >> behavior for that flag. >> >> Modified: >> cfe/trunk/include/clang/Basic/**Sanitizers.def >> cfe/trunk/lib/CodeGen/**BackendUtil.cpp >> cfe/trunk/lib/CodeGen/CGExpr.**cpp >> cfe/trunk/lib/CodeGen/**CGExprScalar.cpp >> cfe/trunk/lib/Driver/**SanitizerArgs.cpp >> cfe/trunk/test/CodeGen/bounds-**checking.c >> cfe/trunk/test/CodeGenCXX/**catch-undef-behavior.cpp >> cfe/trunk/test/Driver/bounds-**checking.c >> cfe/trunk/test/Driver/**fsanitize.c >> >> Modified: cfe/trunk/include/clang/Basic/**Sanitizers.def >> URL: http://llvm.org/viewvc/llvm-**project/cfe/trunk/include/** >> clang/Basic/Sanitizers.def?**rev=193205&r1=193204&r2=**193205&view=diff<http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Sanitizers.def?rev=193205&r1=193204&r2=193205&view=diff> >> ==============================**==============================** >> ================== >> --- cfe/trunk/include/clang/Basic/**Sanitizers.def (original) >> +++ cfe/trunk/include/clang/Basic/**Sanitizers.def Tue Oct 22 17:51:04 >> 2013 >> @@ -59,8 +59,8 @@ SANITIZER("leak", Leak) >> >> // UndefinedBehaviorSanitizer >> SANITIZER("alignment", Alignment) >> +SANITIZER("array-bounds", ArrayBounds) >> SANITIZER("bool", Bool) >> -SANITIZER("bounds", Bounds) >> SANITIZER("enum", Enum) >> SANITIZER("float-cast-**overflow", FloatCastOverflow) >> SANITIZER("float-divide-by-**zero", FloatDivideByZero) >> @@ -84,7 +84,7 @@ SANITIZER("dataflow", DataFlow) >> // -fsanitize=undefined includes all the sanitizers which have low >> overhead, no >> // ABI or address space layout implications, and only catch undefined >> behavior. >> SANITIZER_GROUP("undefined", Undefined, >> - Alignment | Bool | Bounds | Enum | FloatCastOverflow | >> + Alignment | Bool | ArrayBounds | Enum | >> FloatCastOverflow | >> FloatDivideByZero | Function | IntegerDivideByZero | Null >> | >> ObjectSize | Return | Shift | SignedIntegerOverflow | >> Unreachable | VLABound | Vptr) >> @@ -94,7 +94,7 @@ SANITIZER_GROUP("undefined", Undefined, >> // runtime support. This group is generally used in conjunction with the >> // -fsanitize-undefined-trap-on-**error flag. >> SANITIZER_GROUP("undefined-**trap", UndefinedTrap, >> - Alignment | Bool | Bounds | Enum | FloatCastOverflow | >> + Alignment | Bool | ArrayBounds | Enum | >> FloatCastOverflow | >> FloatDivideByZero | IntegerDivideByZero | Null | >> ObjectSize | >> Return | Shift | SignedIntegerOverflow | Unreachable | >> VLABound) >> @@ -103,5 +103,9 @@ SANITIZER_GROUP("integer", Integer, >> SignedIntegerOverflow | UnsignedIntegerOverflow | Shift | >> IntegerDivideByZero) >> >> +// -fbounds-checking >> +SANITIZER("local-bounds", LocalBounds) >> +SANITIZER_GROUP("bounds", Bounds, ArrayBounds | LocalBounds) >> + >> #undef SANITIZER >> #undef SANITIZER_GROUP >> >> Modified: cfe/trunk/lib/CodeGen/**BackendUtil.cpp >> URL: http://llvm.org/viewvc/llvm-**project/cfe/trunk/lib/CodeGen/** >> BackendUtil.cpp?rev=193205&r1=**193204&r2=193205&view=diff<http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=193205&r1=193204&r2=193205&view=diff> >> ==============================**==============================** >> ================== >> --- cfe/trunk/lib/CodeGen/**BackendUtil.cpp (original) >> +++ cfe/trunk/lib/CodeGen/**BackendUtil.cpp Tue Oct 22 17:51:04 2013 >> @@ -245,7 +245,7 @@ void EmitAssemblyHelper::**CreatePasses(Ta >> addObjCARCOptPass); >> } >> >> - if (LangOpts.Sanitize.Bounds) { >> + if (LangOpts.Sanitize.**LocalBounds) { >> PMBuilder.addExtension(**PassManagerBuilder::EP_** >> ScalarOptimizerLate, >> addBoundsCheckingPass); >> PMBuilder.addExtension(**PassManagerBuilder::EP_**EnabledOnOptLevel0, >> >> Modified: cfe/trunk/lib/CodeGen/CGExpr.**cpp >> URL: http://llvm.org/viewvc/llvm-**project/cfe/trunk/lib/CodeGen/** >> CGExpr.cpp?rev=193205&r1=**193204&r2=193205&view=diff<http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=193205&r1=193204&r2=193205&view=diff> >> ==============================**==============================** >> ================== >> --- cfe/trunk/lib/CodeGen/CGExpr.**cpp (original) >> +++ cfe/trunk/lib/CodeGen/CGExpr.**cpp Tue Oct 22 17:51:04 2013 >> @@ -641,7 +641,8 @@ static llvm::Value *getArrayIndexingBoun >> void CodeGenFunction::**EmitBoundsCheck(const Expr *E, const Expr *Base, >> llvm::Value *Index, QualType >> IndexType, >> bool Accessed) { >> - assert(SanOpts->Bounds && "should not be called unless adding bounds >> checks"); >> + assert(SanOpts->ArrayBounds && >> + "should not be called unless adding bounds checks"); >> >> QualType IndexedType; >> llvm::Value *Bound = getArrayIndexingBound(*this, Base, IndexedType); >> @@ -742,7 +743,7 @@ LValue CodeGenFunction::**EmitUnsupportedL >> >> LValue CodeGenFunction::**EmitCheckedLValue(const Expr *E, TypeCheckKind >> TCK) { >> LValue LV; >> - if (SanOpts->Bounds && isa<ArraySubscriptExpr>(E)) >> + if (SanOpts->ArrayBounds && isa<ArraySubscriptExpr>(E)) >> LV = EmitArraySubscriptExpr(cast<**ArraySubscriptExpr>(E), >> /*Accessed*/true); >> else >> LV = EmitLValue(E); >> @@ -2233,7 +2234,7 @@ LValue CodeGenFunction::**EmitArraySubscri >> QualType IdxTy = E->getIdx()->getType(); >> bool IdxSigned = IdxTy->**isSignedIntegerOrEnumerationTy**pe(); >> >> - if (SanOpts->Bounds) >> + if (SanOpts->ArrayBounds) >> EmitBoundsCheck(E, E->getBase(), Idx, IdxTy, Accessed); >> >> // If the base is a vector type, then we are forming a vector element >> lvalue >> >> Modified: cfe/trunk/lib/CodeGen/**CGExprScalar.cpp >> URL: http://llvm.org/viewvc/llvm-**project/cfe/trunk/lib/CodeGen/** >> CGExprScalar.cpp?rev=193205&**r1=193204&r2=193205&view=diff<http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprScalar.cpp?rev=193205&r1=193204&r2=193205&view=diff> >> ==============================**==============================** >> ================== >> --- cfe/trunk/lib/CodeGen/**CGExprScalar.cpp (original) >> +++ cfe/trunk/lib/CodeGen/**CGExprScalar.cpp Tue Oct 22 17:51:04 2013 >> @@ -1073,7 +1073,7 @@ Value *ScalarExprEmitter::**VisitArraySubs >> Value *Idx = Visit(E->getIdx()); >> QualType IdxTy = E->getIdx()->getType(); >> >> - if (CGF.SanOpts->Bounds) >> + if (CGF.SanOpts->ArrayBounds) >> CGF.EmitBoundsCheck(E, E->getBase(), Idx, IdxTy, /*Accessed*/true); >> >> bool IdxSigned = IdxTy->**isSignedIntegerOrEnumerationTy**pe(); >> @@ -2314,7 +2314,7 @@ static Value *emitPointerArithmetic(Code >> if (isSubtraction) >> index = CGF.Builder.CreateNeg(index, "idx.neg"); >> >> - if (CGF.SanOpts->Bounds) >> + if (CGF.SanOpts->ArrayBounds) >> CGF.EmitBoundsCheck(op.E, pointerOperand, index, >> indexOperand->getType(), >> /*Accessed*/ false); >> >> >> Modified: cfe/trunk/lib/Driver/**SanitizerArgs.cpp >> URL: http://llvm.org/viewvc/llvm-**project/cfe/trunk/lib/Driver/** >> SanitizerArgs.cpp?rev=193205&**r1=193204&r2=193205&view=diff<http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/SanitizerArgs.cpp?rev=193205&r1=193204&r2=193205&view=diff> >> ==============================**==============================** >> ================== >> --- cfe/trunk/lib/Driver/**SanitizerArgs.cpp (original) >> +++ cfe/trunk/lib/Driver/**SanitizerArgs.cpp Tue Oct 22 17:51:04 2013 >> @@ -256,8 +256,8 @@ bool SanitizerArgs::parse(const Driver & >> "-fsanitize=undefined-trap -fsanitize-undefined-trap-on-**error"; >> } else if (A->getOption().matches(**options::OPT_fbounds_checking) || >> A->getOption().matches(**options::OPT_fbounds_checking_**EQ)) >> { >> - Add = Bounds; >> - DeprecatedReplacement = "-fsanitize=bounds"; >> + Add = LocalBounds; >> + DeprecatedReplacement = "-fsanitize=local-bounds"; >> } else if (A->getOption().matches(**options::OPT_fsanitize_EQ)) { >> Add = parse(D, A, DiagnoseErrors); >> } else if (A->getOption().matches(**options::OPT_fno_sanitize_EQ)) { >> >> Modified: cfe/trunk/test/CodeGen/bounds-**checking.c >> URL: http://llvm.org/viewvc/llvm-**project/cfe/trunk/test/** >> CodeGen/bounds-checking.c?rev=**193205&r1=193204&r2=193205&**view=diff<http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/bounds-checking.c?rev=193205&r1=193204&r2=193205&view=diff> >> ==============================**==============================** >> ================== >> --- cfe/trunk/test/CodeGen/bounds-**checking.c (original) >> +++ cfe/trunk/test/CodeGen/bounds-**checking.c Tue Oct 22 17:51:04 2013 >> @@ -1,26 +1,29 @@ >> -// RUN: %clang_cc1 -fsanitize=bounds -emit-llvm -triple >> x86_64-apple-darwin10 < %s | FileCheck %s >> +// RUN: %clang_cc1 -fsanitize=local-bounds -emit-llvm -triple >> x86_64-apple-darwin10 %s -o - | FileCheck %s >> +// RUN: %clang_cc1 -fsanitize=array-bounds -O >> -fsanitize-undefined-trap-on-**error -emit-llvm -triple >> x86_64-apple-darwin10 -DNO_DYNAMIC %s -o - | FileCheck %s >> >> -// CHECK: @f >> +// CHECK-LABEL: @f >> double f(int b, int i) { >> double a[b]; >> - // CHECK: trap >> + // CHECK: call {{.*}} @llvm.trap >> return a[i]; >> } >> >> -// CHECK: @f2 >> +// CHECK-LABEL: @f2 >> void f2() { >> // everything is constant; no trap possible >> - // CHECK-NOT: trap >> + // CHECK-NOT: call {{.*}} @llvm.trap >> int a[2]; >> a[1] = 42; >> - >> + >> +#ifndef NO_DYNAMIC >> short *b = malloc(64); >> b[5] = *a + a[1] + 2; >> +#endif >> } >> >> -// CHECK: @f3 >> +// CHECK-LABEL: @f3 >> void f3() { >> int a[1]; >> - // CHECK: trap >> + // CHECK: call {{.*}} @llvm.trap >> a[2] = 1; >> } >> >> Modified: cfe/trunk/test/CodeGenCXX/**catch-undef-behavior.cpp >> URL: http://llvm.org/viewvc/llvm-**project/cfe/trunk/test/** >> CodeGenCXX/catch-undef-**behavior.cpp?rev=193205&r1=** >> 193204&r2=193205&view=diff<http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/catch-undef-behavior.cpp?rev=193205&r1=193204&r2=193205&view=diff> >> ==============================**==============================** >> ================== >> --- cfe/trunk/test/CodeGenCXX/**catch-undef-behavior.cpp (original) >> +++ cfe/trunk/test/CodeGenCXX/**catch-undef-behavior.cpp Tue Oct 22 >> 17:51:04 2013 >> @@ -1,4 +1,4 @@ >> -// RUN: %clang_cc1 -std=c++11 -fsanitize=signed-integer-** >> overflow,integer-divide-by-**zero,float-divide-by-zero,** >> shift,unreachable,return,vla-**bound,alignment,null,vptr,** >> object-size,float-cast-**overflow,bool,enum,bounds,**function >> -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s >> +// RUN: %clang_cc1 -std=c++11 -fsanitize=signed-integer-** >> overflow,integer-divide-by-**zero,float-divide-by-zero,** >> shift,unreachable,return,vla-**bound,alignment,null,vptr,** >> object-size,float-cast-**overflow,bool,enum,array-**bounds,function >> -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s >> >> struct S { >> double d; >> >> Modified: cfe/trunk/test/Driver/bounds-**checking.c >> URL: http://llvm.org/viewvc/llvm-**project/cfe/trunk/test/Driver/** >> bounds-checking.c?rev=193205&**r1=193204&r2=193205&view=diff<http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/bounds-checking.c?rev=193205&r1=193204&r2=193205&view=diff> >> ==============================**==============================** >> ================== >> --- cfe/trunk/test/Driver/bounds-**checking.c (original) >> +++ cfe/trunk/test/Driver/bounds-**checking.c Tue Oct 22 17:51:04 2013 >> @@ -1,11 +1,11 @@ >> // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2> %t >> // RUN: FileCheck -check-prefix=CHECK < %t %s >> -// CHECK: "-fsanitize=bounds" >> +// CHECK: "-fsanitize=array-bounds,**local-bounds" >> >> // RUN: %clang -fbounds-checking -### -fsyntax-only %s 2> %t >> // RUN: FileCheck -check-prefix=CHECK-OLD < %t %s >> -// CHECK-OLD: "-fsanitize=bounds" >> +// CHECK-OLD: "-fsanitize=local-bounds" >> >> // RUN: %clang -fbounds-checking=3 -### -fsyntax-only %s 2> %t >> // RUN: FileCheck -check-prefix=CHECK-OLD2 < %t %s >> -// CHECK-OLD2: "-fsanitize=bounds" >> +// CHECK-OLD2: "-fsanitize=local-bounds" >> >> Modified: cfe/trunk/test/Driver/**fsanitize.c >> URL: http://llvm.org/viewvc/llvm-**project/cfe/trunk/test/Driver/** >> fsanitize.c?rev=193205&r1=**193204&r2=193205&view=diff<http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/fsanitize.c?rev=193205&r1=193204&r2=193205&view=diff> >> ==============================**==============================** >> ================== >> --- cfe/trunk/test/Driver/**fsanitize.c (original) >> +++ cfe/trunk/test/Driver/**fsanitize.c Tue Oct 22 17:51:04 2013 >> @@ -1,17 +1,17 @@ >> // RUN: %clang -target x86_64-linux-gnu -fcatch-undefined-behavior %s >> -### 2>&1 | FileCheck %s --check-prefix=CHECK-**UNDEFINED-TRAP >> // RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined-trap >> -fsanitize-undefined-trap-on-**error %s -### 2>&1 | FileCheck %s >> --check-prefix=CHECK-**UNDEFINED-TRAP >> // RUN: %clang -target x86_64-linux-gnu -fsanitize-undefined-trap-on-**error >> -fsanitize=undefined-trap %s -### 2>&1 | FileCheck %s --check-prefix=CHECK- >> **UNDEFINED-TRAP >> -// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-** >> integer-overflow|integer-**divide-by-zero|float-divide-** >> by-zero|shift|unreachable|**return|vla-bound|alignment|** >> null|object-size|float-cast-**overflow|bounds|enum|bool),?){**14}"}} >> +// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-** >> integer-overflow|integer-**divide-by-zero|float-divide-** >> by-zero|shift|unreachable|**return|vla-bound|alignment|** >> null|object-size|float-cast-**overflow|array-bounds|enum|** >> bool),?){14}"}} >> // CHECK-UNDEFINED-TRAP: "-fsanitize-undefined-trap-on-**error" >> >> // RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined %s -### 2>&1 >> | FileCheck %s --check-prefix=CHECK-UNDEFINED >> -// CHECK-UNDEFINED: "-fsanitize={{((signed-**integer-overflow|integer-** >> divide-by-zero|float-divide-**by-zero|function|shift|** >> unreachable|return|vla-bound|**alignment|null|vptr|object-** >> size|float-cast-overflow|**bounds|enum|bool),?){16}"}} >> +// CHECK-UNDEFINED: "-fsanitize={{((signed-**integer-overflow|integer-** >> divide-by-zero|float-divide-**by-zero|function|shift|** >> unreachable|return|vla-bound|**alignment|null|vptr|object-** >> size|float-cast-overflow|**array-bounds|enum|bool),?){16}**"}} >> >> // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | >> FileCheck %s --check-prefix=CHECK-INTEGER >> // CHECK-INTEGER: "-fsanitize={{((signed-**integer-overflow|unsigned-** >> integer-overflow|integer-**divide-by-zero|shift),?){4}"}} >> >> // RUN: %clang -target x86_64-linux-gnu -fsanitize=thread,undefined >> -fno-thread-sanitizer -fno-sanitize=float-cast-**overflow,vptr,bool,enum >> %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-PARTIAL-**UNDEFINED >> -// CHECK-PARTIAL-UNDEFINED: "-fsanitize={{((signed-** >> integer-overflow|integer-**divide-by-zero|float-divide-** >> by-zero|function|shift|**unreachable|return|vla-bound|** >> alignment|null|object-size|**bounds),?){12}"}} >> +// CHECK-PARTIAL-UNDEFINED: "-fsanitize={{((signed-** >> integer-overflow|integer-**divide-by-zero|float-divide-** >> by-zero|function|shift|**unreachable|return|vla-bound|** >> alignment|null|object-size|**array-bounds),?){12}"}} >> >> // RUN: %clang -target x86_64-linux-gnu -fsanitize=address-full %s -### >> 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-FULL >> // CHECK-ASAN-FULL: "-fsanitize={{((address|init-** >> order|use-after-return|use-**after-scope),?){4}"}} >> @@ -101,7 +101,7 @@ >> // CHECK-DEPRECATED: argument '-fno-thread-sanitizer' is deprecated, use >> '-fno-sanitize=thread' instead >> // CHECK-DEPRECATED: argument '-faddress-sanitizer' is deprecated, use >> '-fsanitize=address' instead >> // CHECK-DEPRECATED: argument '-fno-address-sanitizer' is deprecated, use >> '-fno-sanitize=address' instead >> -// CHECK-DEPRECATED: argument '-fbounds-checking' is deprecated, use >> '-fsanitize=bounds' instead >> +// CHECK-DEPRECATED: argument '-fbounds-checking' is deprecated, use >> '-fsanitize=local-bounds' instead >> >> // RUN: %clang -target x86_64-linux-gnu -fsanitize=thread %s -### 2>&1 | >> FileCheck %s --check-prefix=CHECK-TSAN-NO-**PIE >> // CHECK-TSAN-NO-PIE: "-mrelocation-model" "pic" "-pic-level" "2" >> "-pie-level" "2" >> >> >> ______________________________**_________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/**mailman/listinfo/cfe-commits<http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits> >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
