ztong0001 added inline comments.
================ Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:757 SanOpts.set(SanitizerKind::HWAddress, false); + if (mask & SanitizerKind::LocalBounds) + Fn->addFnAttr(llvm::Attribute::NoSanitizeBounds); ---------------- ztong0001 wrote: > melver wrote: > > These 2 checks can be reduced to 1 due to SanitizerKind::Bounds including > > both. However, as noted, this only affects local-bounds, so I'm not sure if > > we want to include both -- perhaps for completeness it makes sense, but in > > the array-bounds only case this attribute will be a nop (AFAIK). > > > > Also, I think we don't want to attach the attribute if bounds checking > > isn't enabled -- at least it seems unnecessary to do so. > > > > See the following suggested change: > > > > ``` > > diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp > > b/clang/lib/CodeGen/CodeGenFunction.cpp > > index 842ce0245d45..c1f3a3014a19 100644 > > --- a/clang/lib/CodeGen/CodeGenFunction.cpp > > +++ b/clang/lib/CodeGen/CodeGenFunction.cpp > > @@ -740,6 +740,7 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, > > QualType RetTy, > > } while (false); > > > > if (D) { > > + const bool SanitizeBounds = SanOpts.hasOneOf(SanitizerKind::Bounds); > > bool NoSanitizeCoverage = false; > > > > for (auto Attr : D->specific_attrs<NoSanitizeAttr>()) { > > @@ -754,16 +755,15 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, > > QualType RetTy, > > SanOpts.set(SanitizerKind::KernelHWAddress, false); > > if (mask & SanitizerKind::KernelHWAddress) > > SanOpts.set(SanitizerKind::HWAddress, false); > > - if (mask & SanitizerKind::LocalBounds) > > - Fn->addFnAttr(llvm::Attribute::NoSanitizeBounds); > > - if (mask & SanitizerKind::ArrayBounds) > > - Fn->addFnAttr(llvm::Attribute::NoSanitizeBounds); > > > > // SanitizeCoverage is not handled by SanOpts. > > if (Attr->hasCoverage()) > > NoSanitizeCoverage = true; > > } > > > > + if (SanitizeBounds && !SanOpts.hasOneOf(SanitizerKind::Bounds)) > > + Fn->addFnAttr(llvm::Attribute::NoSanitizeBounds); > > + > > if (NoSanitizeCoverage && CGM.getCodeGenOpts().hasSanitizeCoverage()) > > Fn->addFnAttr(llvm::Attribute::NoSanitizeCoverage); > > } > > > > ``` > Agreed. I will revise patch and commit description. BTW: Just to double check we are on the same page when no_sanitize(bounds) is provided, fsanitize=array-bounds is also disabled -- right ? I can confirm this attribute is also affecting array-bounds as this is handled in clang side and we are setting llvm::Attribute::NoSanitizeBounds so that clang's array-bounds can also see this. I will also add another test for -fsanitize=array-bounds Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119816/new/ https://reviews.llvm.org/D119816 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits