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

RESOLVED FIXED in Firefox 62

Status

enhancement
RESOLVED FIXED
Last year
7 months ago

People

(Reporter: andi, Assigned: andi)

Tracking

3 Branch
mozilla62
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

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 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.
Attachment #8968536 - Flags: review?(nfroyd) → review-
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.
Will update asap `clang-plugin.m4` and hopefully it will work.
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 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.
Attachment #8968536 - Flags: review?(nfroyd) → review+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c09f5d341886
clear out CPPFLAGS before compiling test files for the clang plugin. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/c09f5d341886
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.