On 21 March 2017 at 20:11, Breno Guimarães <bren...@gmail.com> wrote:
> Hi, > > Thanks for the suggestions. I actually had tested with a fake header file > (so I could build more interesting testcases) but it didn't occur to me to > actually make it part of the codebase test. > > I was afraid of being too kind to exempt PredefinedExpr symbols in any > context, and that's why I only do it if the symbol appears on a system > header. Had I exempted the symbol in any context, this would be allowed: > > void some_user_function(const char*); > some_user_func(__PRETTY_FUNCTION__); > > That's a judgment call, really. I went for the safe side, but wouldn't > mind to change it at all. As you pointed we already allow the decay of > string literals, so it may be the better way. > I think from a user's perspective, __PRETTY_FUNCTION__ is much more like a macro that expands to a string literal than it is like a magic const char[], so exempting this seems reasonable to me. On the other hand, I went a little beyond the original scope. I added the > precondition of not being a "system symbol decay in system header". > By "system symbol" I mean a PredefinedExpr or any symbol declared in a > system header. > > Let's do it like this: I'll create a header, include it passing the > -isystem flag, and add several more tests and scenarios. I'll add the > result the code after my changes produce, and we can judge on the concrete > case what makes more sense. > > Is that reasonable? > Sure, sounds good. > Best Regards, > Breno G. > > > > > On Tue, Mar 21, 2017 at 11:37 PM, Richard Smith <rich...@metafoo.co.uk> > wrote: > >> 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(MatchFin >>>>> der >>>>> >> *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(st >>>>> ringLiteral()))) >>>>> >> + unless(hasSourceExpression(st >>>>> ringLiteral())), >>>>> >> + 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 >>> >>> >> > > > -- > 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
