[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
Wed Mar 22 11:31:37 PDT 2017
On 21 March 2017 at 20:11, Breno Guimarães <brenorg at 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 <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/a5282e14/attachment-0001.html>
More information about the cfe-commits
mailing list