[clang-tools-extra] r298421 - Prevent cppcoreguidelines-pro-bounds-array-to-pointer-decay from diagnosing array to pointer decay stemming from system macros.
Breno Guimarães via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 21 20:11:48 PDT 2017
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.
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?
Best Regards,
Breno G.
On Tue, Mar 21, 2017 at 11:37 PM, Richard Smith <richard at metafoo.co.uk>
wrote:
> 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(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 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
>>
>>
>
--
Breno Rodrigues Guimarães
Universidade Federal de Minas Gerais - UFMG, Brasil
(Federal University of Minas Gerais, Brazil)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170322/5a2fe47f/attachment-0001.html>
More information about the cfe-commits
mailing list