Closed Bug 1454667 Opened 5 years ago Closed 4 years ago

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


(Developer Infrastructure :: Source Code Analysis, enhancement)

3 Branch
Not set


(firefox62 fixed)

Tracking Status
firefox62 --- fixed


(Reporter: andi, Assigned: andi)




(1 file)

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.

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:

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:
Comment on attachment 8968536 [details]
Bug 1454667 - clear out CPPFLAGS before compiling test files for the clang plugin.

Thank you!  r=me with the change below.

::: build/autoconf/clang-plugin.m4:106
(Diff revision 2)
>              _SAVE_CXXFLAGS="$CXXFLAGS"
>              _SAVE_CXX="$CXX"
>              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
clear out CPPFLAGS before compiling test files for the clang plugin. r=froydnj
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Version: Version 3 → 3 Branch
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.