On 21 March 2017 at 18:57, Breno Guimarães via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> Hey guys, > > I'm sorry the test did not make the decay explicit. It's indeed sort of > tricky: https://bugs.llvm.org/show_bug.cgi?id=32239 > > Basically, assert is expanded into: > > (__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __FILE__, __LINE__, #e) > > Maybe in your libc, and even in most libc's, but not in general, and we can't and shouldn't rely on that in this test. (And there may not even *be* a usable <assert.h> in the include path...) > Well, __func__ and __FILE__ are const char[], while __assert_rtn expects > const char*. > > I would think __func__ (and its friends __FUNCTION__, __PRETTY_FUNCTION__, etc) should be exempted from this warning just like decay on a string literal is. __FILE__ is string literal, so should be exempted already. Perhaps checking for a system header is not the right change at all? > I've also seen this in my own environment (Linux) with slightly different > function names, but basically the same problem. > > Any ideas of how to rewrite the test (or the code?) to have the change pushed? > > For the current approach: add a header file (to Inputs/...) that contains the macro you want to use and add a -isystem path to the test pointing at your Inputs/... directory. To whitelist conversions of __func__ etc you should also check for PredefinedExpr wherever we're currently checking for StringLiteral. > Thanks! > > Breno G. > > > > > On Tue, Mar 21, 2017 at 10:36 PM, Richard Smith via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Thank you! >> >> On 21 March 2017 at 18:22, Aaron Ballman <aa...@aaronballman.com> wrote: >> >>> On Tue, Mar 21, 2017 at 9:15 PM, Richard Smith <rich...@metafoo.co.uk> >>> wrote: >>> > On 21 March 2017 at 12:01, Aaron Ballman via cfe-commits >>> > <cfe-commits@lists.llvm.org> wrote: >>> >> >>> >> Author: aaronballman >>> >> Date: Tue Mar 21 14:01:17 2017 >>> >> New Revision: 298421 >>> >> >>> >> URL: http://llvm.org/viewvc/llvm-project?rev=298421&view=rev >>> >> Log: >>> >> Prevent cppcoreguidelines-pro-bounds-array-to-pointer-decay from >>> >> diagnosing array to pointer decay stemming from system macros. >>> >> >>> >> Patch by Breno Rodrigues Guimaraes. >>> >> >>> >> Modified: >>> >> >>> >> clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProBoun >>> dsArrayToPointerDecayCheck.cpp >>> >> >>> >> clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pr >>> o-bounds-array-to-pointer-decay.cpp >>> >> >>> >> Modified: >>> >> clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProBoun >>> dsArrayToPointerDecayCheck.cpp >>> >> URL: >>> >> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/ >>> clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayChe >>> ck.cpp?rev=298421&r1=298420&r2=298421&view=diff >>> >> >>> >> ============================================================ >>> ================== >>> >> --- >>> >> clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProBoun >>> dsArrayToPointerDecayCheck.cpp >>> >> (original) >>> >> +++ >>> >> clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProBoun >>> dsArrayToPointerDecayCheck.cpp >>> >> Tue Mar 21 14:01:17 2017 >>> >> @@ -47,6 +47,25 @@ AST_MATCHER_P(Expr, hasParentIgnoringImp >>> >> return InnerMatcher.matches(*E, Finder, Builder); >>> >> } >>> >> >>> >> +AST_MATCHER(ImplicitCastExpr, isArrayToPointerDecay) { >>> >> + return Node.getCastKind() == CK_ArrayToPointerDecay; >>> >> +} >>> >> + >>> >> +AST_MATCHER(ImplicitCastExpr, sysSymbolDecayInSysHeader) { >>> >> + auto &SM = Finder->getASTContext().getSourceManager(); >>> >> + if (SM.isInSystemMacro(Node.getLocStart())) { >>> >> + if (isa<PredefinedExpr>(Node.getSubExpr())) >>> >> + return true; >>> >> + >>> >> + if (const auto *SymbolDeclRef = >>> >> dyn_cast<DeclRefExpr>(Node.getSubExpr())) { >>> >> + const ValueDecl *SymbolDecl = SymbolDeclRef->getDecl(); >>> >> + if (SymbolDecl && SM.isInSystemHeader(SymbolDecl >>> ->getLocation())) >>> >> + return true; >>> >> + } >>> >> + } >>> >> + return false; >>> >> +} >>> >> + >>> >> void ProBoundsArrayToPointerDecayCheck::registerMatchers(MatchFinder >>> >> *Finder) { >>> >> if (!getLangOpts().CPlusPlus) >>> >> return; >>> >> @@ -56,10 +75,12 @@ void ProBoundsArrayToPointerDecayCheck:: >>> >> // 2) inside a range-for over an array >>> >> // 3) if it converts a string literal to a pointer >>> >> Finder->addMatcher( >>> >> - implicitCastExpr(unless(hasParent(arraySubscriptExpr())), >>> >> + implicitCastExpr(isArrayToPointerDecay(), >>> >> + unless(hasParent(arraySubscriptExpr())), >>> >> >>> >> unless(hasParentIgnoringImpCasts(explicitCastExpr())), >>> >> unless(isInsideOfRangeBeginEndStmt()), >>> >> - unless(hasSourceExpression(stringLiteral()))) >>> >> + unless(hasSourceExpression(stringLiteral())), >>> >> + unless(sysSymbolDecayInSysHeader())) >>> >> .bind("cast"), >>> >> this); >>> >> } >>> >> @@ -67,8 +88,6 @@ void ProBoundsArrayToPointerDecayCheck:: >>> >> void ProBoundsArrayToPointerDecayCheck::check( >>> >> const MatchFinder::MatchResult &Result) { >>> >> const auto *MatchedCast = >>> >> Result.Nodes.getNodeAs<ImplicitCastExpr>("cast"); >>> >> - if (MatchedCast->getCastKind() != CK_ArrayToPointerDecay) >>> >> - return; >>> >> >>> >> diag(MatchedCast->getExprLoc(), "do not implicitly decay an array >>> into >>> >> a " >>> >> "pointer; consider using >>> >> gsl::array_view or " >>> >> >>> >> Modified: >>> >> clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pr >>> o-bounds-array-to-pointer-decay.cpp >>> >> URL: >>> >> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/ >>> test/clang-tidy/cppcoreguidelines-pro-bounds-array-to-pointe >>> r-decay.cpp?rev=298421&r1=298420&r2=298421&view=diff >>> >> >>> >> ============================================================ >>> ================== >>> >> --- >>> >> clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pr >>> o-bounds-array-to-pointer-decay.cpp >>> >> (original) >>> >> +++ >>> >> clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pr >>> o-bounds-array-to-pointer-decay.cpp >>> >> Tue Mar 21 14:01:17 2017 >>> >> @@ -1,4 +1,5 @@ >>> >> // RUN: %check_clang_tidy %s >>> >> cppcoreguidelines-pro-bounds-array-to-pointer-decay %t >>> >> +#include <assert.h> >>> > >>> > >>> > This test is using a header that we do not supply (unlike stddef.h >>> which >>> > Clang provides itself). That does not seem especially reasonable to >>> me; this >>> > test is failing for us as a result. Can you supply a fake <assert.h> >>> system >>> > header as an input to this test? >>> > >>> >> >>> >> #include <stddef.h> >>> >> >>> >> namespace gsl { >>> >> @@ -34,6 +35,11 @@ void f() { >>> >> >>> >> for (auto &e : a) // OK, iteration internally decays array to >>> pointer >>> >> e = 1; >>> >> + >>> >> + assert(false); // OK, array decay inside system header macro >>> > >>> > >>> > Huh? What decay is this referring to? >>> >>> I am now wondering the same question... >>> >>> I've rolled this back r298470 and will follow up with the author. >>> >>> Thanks! >>> >>> ~Aaron >>> >>> > >>> >> + >>> >> + assert(a); >>> >> + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not implicitly >>> decay an >>> >> array into a pointer; consider using gsl::array_view or an explicit >>> cast >>> >> instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay] >>> >> } >>> >> >>> >> const char *g() { >>> >> >>> >> >>> >> _______________________________________________ >>> >> cfe-commits mailing list >>> >> cfe-commits@lists.llvm.org >>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>> > >>> > >>> >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >> > > > -- > Breno Rodrigues Guimarães > Universidade Federal de Minas Gerais - UFMG, Brasil > (Federal University of Minas Gerais, Brazil) > > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
