[clang-tools-extra] r298421 - Prevent cppcoreguidelines-pro-bounds-array-to-pointer-decay from diagnosing array to pointer decay stemming from system macros.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 21 19:37:32 PDT 2017
On 21 March 2017 at 18:57, Breno Guimarães via cfe-commits <
cfe-commits at 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 at lists.llvm.org> wrote:
>
>> Thank you!
>>
>> On 21 March 2017 at 18:22, Aaron Ballman <aaron at aaronballman.com> wrote:
>>
>>> On Tue, Mar 21, 2017 at 9:15 PM, Richard Smith <richard at metafoo.co.uk>
>>> wrote:
>>> > On 21 March 2017 at 12:01, Aaron Ballman via cfe-commits
>>> > <cfe-commits at 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 at lists.llvm.org
>>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>> >
>>> >
>>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at 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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170321/39711e2b/attachment-0001.html>
More information about the cfe-commits
mailing list