Re: [PATCH] D31130: B32239 clang-tidy should not warn about array to pointer decay on system macros
Nice! I'm glad to see the patch being revived. Thanks for that :) Em seg, 5 de out de 2020 10:48, fiesh via Phabricator < revi...@reviews.llvm.org> escreveu: > fiesh added a comment. > > Continued in https://reviews.llvm.org/D88833 > > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D31130/new/ > > https://reviews.llvm.org/D31130 > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [clang-tools-extra] r298421 - Prevent cppcoreguidelines-pro-bounds-array-to-pointer-decay from diagnosing array to pointer decay stemming from system macros.
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)
Well, __func__ and __FILE__ are const char[], while __assert_rtn
expects const char*.
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?
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 wrote:
>
>> On Tue, Mar 21, 2017 at 9:15 PM, Richard Smith
>> wrote:
>> > On 21 March 2017 at 12:01, Aaron Ballman via cfe-commits
>> > 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(Node.getSubExpr()))
>> >> + return true;
>> >> +
>> >> +if (const auto *SymbolDeclRef =
>> >> dyn_cast(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("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-
>> pointer-decay.cpp?rev=298421&r1=298420&r2=298421&view=diff
>> >>
>> >>
>> ==
>> >> ---
>> >> clang-tools-extra/trunk/test/clang-tidy/cppcoreg
Re: [clang-tools-extra] r298421 - Prevent cppcoreguidelines-pro-bounds-array-to-pointer-decay from diagnosing array to pointer decay stemming from system macros.
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 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 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 wrote: >>> >>>> On Tue, Mar 21, 2017 at 9:15 PM, Richard Smith >>>> wrote: >>>> > On 21 March 2017 at 12:01, Aaron Ballman via cfe-commits >>>> > 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&vi
