1454667 - For SprintFliteralChecker subsitute hasIgnoringParenImpCasts with hasDecent that fails on android builds.

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Andi [:andi]]

A wired behaviour is on Android builds and specially for this checker it seems that in this context hasIgnoringParenImpCasts fails. As a workaround in this particular context we can use hasDecent since no other AST nodes are going to be created that would mix with our analysis.

Comment 1

[Andi [:andi]]

Attachment: Bug 1454667 - clear out CPPFLAGS before compiling test files for the clang plugin.

Review commit: https://reviewboard.mozilla.org/r/237224/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/237224/

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8968536 [details]
Bug 1454667 - clear out CPPFLAGS before compiling test files for the clang plugin.

https://reviewboard.mozilla.org/r/237224/#review243070

Please try the comment below with the patch stack from bug 1428158; if you don't feel comfortable touching `clang-plugin.m4`, let me know and I can produce a patch for you to use.

::: commit-message-7ff49:1
(Diff revision 1)
> +Bug 1454667 - fix sprintfliteralchecker by using hasDesendant against ignoringParenImpCasts. r?froydnj

Nit 1: "SprintfLiteralChecker", with proper capitalization, please.

Nit 2: "hasDescendant".

::: build/clang-plugin/SprintfLiteralChecker.cpp:14
(Diff revision 1)
>    AstMatcher->addMatcher(
>        callExpr(
>            isSnprintfLikeFunc(),
>            allOf(hasArgument(
>                      0, ignoringParenImpCasts(declRefExpr().bind("buffer"))),
> -                anyOf(hasArgument(1, sizeOfExpr(hasIgnoringParenImpCasts(
> +                anyOf(hasArgument(1, sizeOfExpr(hasDescendant(

I'm not an expert here, but I don't think this is the correct matcher.  You also don't provide an explanation for *why* this is the correct fix.

After looking at this, I think the problem comes from a similar issue as attachment 8958893 [details] [diff] [review] in bug 1428158: that patch fixed compilation for `HAVE_NEW_ASTMATCHER_NAMES`, but it didn't fix the similar code just below that's trying to set `HAS_ACCEPTS_IGNORINGPARENIMPCASTS`.  I'd bet if this code:

https://searchfox.org/mozilla-central/source/build/autoconf/clang-plugin.m4#125-149

was changed to clear out `CPPFLAGS` and reset it, that would fix the issues that this matcher is seeing.

Comment 3

[Andi [:andi]]

Ok i see we're your point, let me detail a little bit what this matcher does. It look for direct descendents of the given node and matches them according to declRefExpr. In our case I consider this to be acceptable since I don't imagine a case were we will generate a false-positive by doing so.

Comment 4

[Andi [:andi]]

Will update asap `clang-plugin.m4` and hopefully it will work.

Comment 5

[Andi [:andi]]

Comment on attachment 8968536 [details]
Bug 1454667 - clear out CPPFLAGS before compiling test files for the clang plugin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237224/diff/1-2/

Comment 6

[Andi [:andi]]

It fixes the issue in away more elegant way! I've also started a try build to make sure we don't regress by any chance:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f9db5c986080be724136a073783e22fc3a92322

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8968536 [details]
Bug 1454667 - clear out CPPFLAGS before compiling test files for the clang plugin.

https://reviewboard.mozilla.org/r/237224/#review249226

Thank you!  r=me with the change below.

::: build/autoconf/clang-plugin.m4:106
(Diff revision 2)
>              _SAVE_CXXFLAGS="$CXXFLAGS"
>              _SAVE_CXX="$CXX"
>              _SAVE_MACOSX_DEPLOYMENT_TARGET="$MACOSX_DEPLOYMENT_TARGET"
>              unset MACOSX_DEPLOYMENT_TARGET
>              CXXFLAGS="${LLVM_CXXFLAGS}"
>              CXX="${HOST_CXX}"
>              AC_TRY_COMPILE([#include "clang/ASTMatchers/ASTMatchers.h"],
>                             [clang::ast_matchers::cxxConstructExpr();],
>                             ac_cv_have_new_ASTMatcher_names="yes",
>                             ac_cv_have_new_ASTMatcher_names="no")
>              CXX="$_SAVE_CXX"
>              CXXFLAGS="$_SAVE_CXXFLAGS"

Please go ahead and fold the part 2 patch from bug 1428158 into this patch.

Comment 8

[Andi [:andi]]

Comment on attachment 8968536 [details]
Bug 1454667 - clear out CPPFLAGS before compiling test files for the clang plugin.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237224/diff/2-3/

Comment 9

[Andi [:andi]]

Again making sure we don't regress: https://treeherder.mozilla.org/#/jobs?repo=try&revision=858b22c42aa31eea1c4d9ed171cfc751c24c06f8

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c09f5d341886
clear out CPPFLAGS before compiling test files for the clang plugin. r=froydnj

Comment 11

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/c09f5d341886