[Static Analysis][Clang-Plugin] Analysis to produce an error on when having an attribution expression instead of logical expression in MOZ_ASSERT

RESOLVED FIXED in Firefox 50

Status

defect
P2
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: andi, Assigned: andi)

Tracking

Trunk
mozilla50
x86
Linux

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

From time to time, we have this kind of code:
>>  MOZ_ASSERT(numElems = 16, "unexpected partial load");

We should implement in our clang-plugin a checker like that. Initially we should implement it for MOZ_ASSERT and later on we should extend it for NS_ASSERT
Assignee

Updated

3 years ago
Summary: Analysis to produce an error on when having an attribution expression instead of logical expression in MOZ_ASSERT → [Static Analysis][Clang-Plugin] Analysis to produce an error on when having an attribution expression instead of logical expression in MOZ_ASSERT
Assignee

Updated

3 years ago
Assignee: nobody → bpostelnicu
Priority: -- → P2
For what it's worth, recent clang has a new warning for side-effectful expressions that are used in an unevaluated context.  |numElems = 16| is a side-effectful expression.  And it happens that in MOZ_ASSERT's innards, we pass that expression through a decltype to verify that the expression isn't a function or a few other vacuously-true things.

So *if* people use new-enough clang, they will already get a warning for this.  And on that note, having just dealt with all such instances in bug 1282795, I can verify that building just SpiderMonkey doesn't encounter any instances of this.
Good, maybe we can call the code from clang.
Assignee

Comment 6

3 years ago
Comment on attachment 8767904 [details]
Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62272/diff/1-2/
Attachment #8767904 - Flags: review?(michael) → review?(nfroyd)
Assignee

Comment 7

3 years ago
Comment on attachment 8767904 [details]
Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62272/diff/2-3/
Attachment #8767904 - Attachment description: Bug 1283395 - add markup functions for static analysis builds. → Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression.
Attachment #8767904 - Flags: review?(nfroyd) → review?(michael)
Comment on attachment 8767904 [details]
Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression.

https://reviewboard.mozilla.org/r/62272/#review59112

::: build/clang-plugin/clang-plugin.cpp:776
(Diff revision 3)
> +  if (method && method->getName() == assertName)
> +    return true;
> +  return false;

return method && method->getName() == assertName;

::: build/clang-plugin/clang-plugin.cpp:1570
(Diff revision 3)
>  
> +void DiagnosticsMatcher::AssertAttributionChecker::run(
> +    const MatchFinder::MatchResult &Result) {
> +  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
> +  unsigned assignInsteadOfComp = Diag.getDiagnosticIDs()->getCustomDiagID(
> +      DiagnosticIDs::Error, "Forbidden attribution in assert expression, "

I would probably call it assignment, but froydnj is the person who is reviewing the interface within the code.

::: build/clang-plugin/clang-plugin.cpp:1588
(Diff revision 3)
> +  // Only evaluate the first argument from the CallExpr argument list.
> +  // The syntax of the call argument will look similar with:
> +  // !(Expr)
> +  // First there is an UnaryOperator followed by ParenExpr then check for
> +  // a binary operator of type BO_Assign and if so trigger an error message.
> +  const UnaryOperator *unOp = dyn_cast_or_null<UnaryOperator>(exprArg);
> +
> +  if (!unOp) {
> +    return;
> +  }
> +
> +  Expr *unOpResExpr = unOp->getSubExpr();
> +
> +  if (!unOpResExpr) {
> +    return;
> +  }
> +
> +  // Strip off any ParenExpr or ImplicitCastExprs
> +  unOpResExpr = unOpResExpr->IgnoreParenImpCasts();

I think most of this should be unnecessary. See my comment in TestAssertWithAttribution.cpp

::: build/clang-plugin/tests/TestAssertWithAttribution.cpp:1
(Diff revision 3)
> +static inline void CP_AssertFuncTest(bool exprResult) {

This should be:

`static inline bool CP_AssertFuncTest(bool x) __attribute__((always_inline)) { return x; }`

no?

::: build/clang-plugin/tests/TestAssertWithAttribution.cpp:4
(Diff revision 3)
> +#define MOZ_SA_CHECK_ATTRIB(expr) CP_AssertFuncTest(!(expr))
> +#define MOZ_ASSERT(expr) \
> + 	MOZ_SA_CHECK_ATTRIB(expr)

Why not avoid the complication of having to look through the unary operator, and just do this:

```
#define MOZ_ASSERT(cond) \
  if (MOZ_UNLIKELY(!CP_AssertFuncTest(cond))) { \
    MOZ_CRASH(); \
  }
```

That way you can just directly `IgnoreImplicit()` and then check for the `BinaryOperator` which is an assignment?

::: build/clang-plugin/tests/TestAssertWithAttribution.cpp:18
(Diff revision 3)
> +  MOZ_ASSERT(((p = 1)));  // expected-error {{Forbidden attribution in assert expression, causing the result to always be true}}
> +}
> +
> +void FunctionTest3(int p) {
> +	MOZ_ASSERT(p != 3);
> +}

Please also add a test for assignment of a class with an overloaded operator=. Those assignment nodes look different in the AST. This should be tested for in this test whether or not we decide to error on them.
Attachment #8767904 - Flags: review?(michael) → review-
https://reviewboard.mozilla.org/r/62300/#review59120

Why do we need a separate function marker when clang can figure this warning out without a separate function marker?  I'm not saying that we can't use separate function markers for this sort of thing (they're very useful!), but there's no justification here on why we need one.

::: mfbt/SAFunctions.h:24
(Diff revision 1)
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/*
> + * CP_AssertFuncTest - used in MOZ_ASSERT in order to test the possible

What is the `CP` prefix here?  We would typically use `MOZ` for this kind of function.

::: mfbt/SAFunctions.h:30
(Diff revision 1)
> + * presence of attributions instead of logical comparisons.
> + *
> + * Example:
> + * MOZ_ASSERT(retVal = true);
> + */
> +static MOZ_ALWAYS_INLINE void CP_AssertFuncTest(bool exprResult) {

How does this not get an unused argument error?
Comment on attachment 8767918 [details]
Bug 1283395 - add markup functions for static analysis builds.

https://reviewboard.mozilla.org/r/62300/#review59122
Attachment #8767918 - Flags: review?(nfroyd)
(In reply to Michael Layzell [:mystor] from comment #9)
 > ::: build/clang-plugin/tests/TestAssertWithAttribution.cpp:4
> (Diff revision 3)
> > +#define MOZ_SA_CHECK_ATTRIB(expr) CP_AssertFuncTest(!(expr))
> > +#define MOZ_ASSERT(expr) \
> > + 	MOZ_SA_CHECK_ATTRIB(expr)
> 
> Why not avoid the complication of having to look through the unary operator,
> and just do this:
> 
> ```
> #define MOZ_ASSERT(cond) \
>   if (MOZ_UNLIKELY(!CP_AssertFuncTest(cond))) { \
>     MOZ_CRASH(); \
>   }
> ```
> 
> That way you can just directly `IgnoreImplicit()` and then check for the
> `BinaryOperator` which is an assignment?
>
I understand what you are saying but what if we are dealing with a class that has overloaded operator bool and is specified as explicit, like:

  explicit operator bool() const { return true; }

This will not compile due to the explicit specifier that's why in the definition of MOZ_SA_CHECK_ATTRIB i've added ! operator.
Speaking now of the return type of CP_AssertFuncTest i prefer to have it void and not to include in in:

>   if (MOZ_UNLIKELY(!CP_AssertFuncTest(cond))) { \
>     MOZ_CRASH(); \
>   } 
  
> ::: build/clang-plugin/tests/TestAssertWithAttribution.cpp:18
> (Diff revision 3)
> > +  MOZ_ASSERT(((p = 1)));  // expected-error {{Forbidden attribution in assert expression, causing the result to always be true}}
> > +}
> > +
> > +void FunctionTest3(int p) {
> > +	MOZ_ASSERT(p != 3);
> > +}
> 
> Please also add a test for assignment of a class with an overloaded
> operator=. Those assignment nodes look different in the AST. This should be
> tested for in this test whether or not we decide to error on them.

I will implement this in my next patch.
(In reply to Nathan Froyd [:froydnj] from comment #10)
> https://reviewboard.mozilla.org/r/62300/#review59120
> 
> Why do we need a separate function marker when clang can figure this warning
> out without a separate function marker?  I'm not saying that we can't use
> separate function markers for this sort of thing (they're very useful!), but
> there's no justification here on why we need one.
>
This will greatly help us when we will integrate clang-plugin in the mozreview pipeline, by signing to the developer as errors these type of situations.
 
> ::: mfbt/SAFunctions.h:24
> (Diff revision 1)
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +/*
> > + * CP_AssertFuncTest - used in MOZ_ASSERT in order to test the possible
> 
> What is the `CP` prefix here?  We would typically use `MOZ` for this kind of
> function.
>
I will modify it to: AssertAssignmentTest 
> ::: mfbt/SAFunctions.h:30
> (Diff revision 1)
> > + * presence of attributions instead of logical comparisons.
> > + *
> > + * Example:
> > + * MOZ_ASSERT(retVal = true);
> > + */
> > +static MOZ_ALWAYS_INLINE void CP_AssertFuncTest(bool exprResult) {
> 
> How does this not get an unused argument error?
(In reply to Andi-Bogdan Postelnicu from comment #13)
> (In reply to Nathan Froyd [:froydnj] from comment #10)
> > https://reviewboard.mozilla.org/r/62300/#review59120
> > 
> > Why do we need a separate function marker when clang can figure this warning
> > out without a separate function marker?  I'm not saying that we can't use
> > separate function markers for this sort of thing (they're very useful!), but
> > there's no justification here on why we need one.
> >
> This will greatly help us when we will integrate clang-plugin in the
> mozreview pipeline, by signing to the developer as errors these type of
> situations.

This statement doesn't actually answer the question.

> > ::: mfbt/SAFunctions.h:24
> > (Diff revision 1)
> > > +#ifdef __cplusplus
> > > +extern "C" {
> > > +#endif
> > > +
> > > +/*
> > > + * CP_AssertFuncTest - used in MOZ_ASSERT in order to test the possible
> > 
> > What is the `CP` prefix here?  We would typically use `MOZ` for this kind of
> > function.
> >
> I will modify it to: AssertAssignmentTest

With the MOZ_ prefix, of course.
Comment on attachment 8767904 [details]
Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62272/diff/3-4/
Attachment #8767904 - Flags: review- → review?(michael)
Attachment #8767918 - Flags: review?(nfroyd)
Comment on attachment 8767918 [details]
Bug 1283395 - add markup functions for static analysis builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62300/diff/1-2/
Attachment #8767904 - Flags: review?(michael) → review-
Comment on attachment 8767904 [details]
Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression.

https://reviewboard.mozilla.org/r/62272/#review59406

::: build/clang-plugin/clang-plugin.cpp:1589
(Diff revisions 3 - 4)
>    exprArg = exprArg->IgnoreImplicit();
>  
>    // Only evaluate the first argument from the CallExpr argument list.
>    // The syntax of the call argument will look similar with:
>    // !(Expr)
> -  // First there is an UnaryOperator followed by ParenExpr then check for
> +  // First there is an UnaryOperator followed by ParenExpr then

then what?

::: build/clang-plugin/tests/TestAssertWithAssignment.cpp:1
(Diff revision 4)
> +static __attribute__((always_inline)) void MOZ_AssertAssignmentTest(bool expr) {
> +  (void)expr;
> +}

Perhaps try this instead?

```
template<typename T>
static inline __attribute__((always_inline)) T&&
MOZ_AssertAssignmentTest(T&& e)
{
  return e;
}
```

And then use it inline within the MOZ_UNLIKELY as:

```
if (MOZ_UNLIKELY(!MOZ_AssertAssignmentTest(expr))) {
```

That being said, if clang has an analysis which detects side-effectful expressions in non-side-effectful situations, then we could potentially do something like:

```
decltype(expr)
```

inside of the MOZ_ASSERT, and trigger the warning that way. If we can use the existing clang analysis I would prefer that over rolling our own.

::: build/clang-plugin/tests/TestAssertWithAssignment.cpp:10
(Diff revision 4)
> +  	MOZ_CHECK_ASSERT_ASSIGNMENT(expr); \
> + 	if (MOZ_UNLIKELY(!(expr))) { \

Making the MOZ_CHECK_ASSERT_ASSIGNMENT seperate means that the assignment will be checked twice instead of once. This seems like a bad thing.
Comment on attachment 8767918 [details]
Bug 1283395 - add markup functions for static analysis builds.

Clearing r? flag since it's cleared on mozreview as well.
Attachment #8767918 - Flags: review?(nfroyd)
(In reply to Michael Layzell [:mystor] from comment #17)
> Comment on attachment 8767904 [details]
> Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT
> assignment instead of logical expression.
> 
> https://reviewboard.mozilla.org/r/62272/#review59406
> 
> ::: build/clang-plugin/clang-plugin.cpp:1589
> (Diff revisions 3 - 4)
> >    exprArg = exprArg->IgnoreImplicit();
> >  
> >    // Only evaluate the first argument from the CallExpr argument list.
> >    // The syntax of the call argument will look similar with:
> >    // !(Expr)
> > -  // First there is an UnaryOperator followed by ParenExpr then check for
> > +  // First there is an UnaryOperator followed by ParenExpr then
> 
> then what?
> 
> ::: build/clang-plugin/tests/TestAssertWithAssignment.cpp:1
> (Diff revision 4)
> > +static __attribute__((always_inline)) void MOZ_AssertAssignmentTest(bool expr) {
> > +  (void)expr;
> > +}
> 
> Perhaps try this instead?
> 
> ```
> template<typename T>
> static inline __attribute__((always_inline)) T&&
> MOZ_AssertAssignmentTest(T&& e)
> {
>   return e;
> }
> ```
> 
> And then use it inline within the MOZ_UNLIKELY as:
> 
> ```
> if (MOZ_UNLIKELY(!MOZ_AssertAssignmentTest(expr))) {
> ```
> 
> That being said, if clang has an analysis which detects side-effectful
> expressions in non-side-effectful situations, then we could potentially do
> something like:
> 
> ```
> decltype(expr)
> ```
> 
just as we've talked on irc, MOZ_ASSERT is used in C files so this C++ syntax will not be supported, that's why in the first place i've used a simple C function
for our marker.
> inside of the MOZ_ASSERT, and trigger the warning that way. If we can use
> the existing clang analysis I would prefer that over rolling our own.
> 
> ::: build/clang-plugin/tests/TestAssertWithAssignment.cpp:10
> (Diff revision 4)
> > +  	MOZ_CHECK_ASSERT_ASSIGNMENT(expr); \
> > + 	if (MOZ_UNLIKELY(!(expr))) { \
> 
> Making the MOZ_CHECK_ASSERT_ASSIGNMENT seperate means that the assignment
> will be checked twice instead of once. This seems like a bad thing.
Yes you are right the expression will be evaluated two times, but i thought this can be mitigated since we don't un it in a live environment. Nether the less i will change it and include it in the If statement inside MOZ_UNLIKELY
Comment on attachment 8767904 [details]
Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62272/diff/4-5/
Attachment #8767904 - Flags: review- → review?(michael)
Attachment #8767918 - Flags: review?(nfroyd)
Comment on attachment 8767918 [details]
Bug 1283395 - add markup functions for static analysis builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62300/diff/2-3/
Comment on attachment 8767904 [details]
Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62272/diff/5-6/
Assignee

Updated

3 years ago
Attachment #8767918 - Attachment is obsolete: true
Attachment #8767918 - Flags: review?(nfroyd)
Comment on attachment 8767904 [details]
Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression.

https://reviewboard.mozilla.org/r/62272/#review61310

Looking better, but could be simplified a bit :)

::: build/clang-plugin/clang-plugin.cpp:47
(Diff revision 6)
> +// Trimmed down version of HasSideEffects from Expr class. We didn't use
> +// that function since it was buggy and it didn't allow us to specify what
> +// type of side effects are we willing to check.
> +// Function can be used having an Expr* and it will return if it nests an
> +// assignment operator, no matter if he is overloaded or not.
> +bool HasSideEffectAssignment(const Expr *expr) {

This seems simpler as we are considering much fewer cases in this version of the function than the original one:

```
bool ContainsAssignment(const Expr *E) {
  if (auto CE = dyn_cast<CXXOperatorCallExpr>(E)) {
    auto OP = CE->getOperator();
    if (OP == OO_Equal || (OP >= OO_PlusEqual && OP <= OO_PipeEqual)) {
      return true;
    }
  } else if (auto BO = dyn_cast<BinaryOperator>(E)) {
    if (BO->isAssignmentOp()) {
      return true;
    }
  }

  for (const Stmt *SubStmt : expr->children()) {
    auto E = dyn_cast_or_null<Expr>(SubStmt);
    if (E && ContainsAssignment(E)) {
      return true;
    }
  }

  return false;
}
```

And I might change the comment to be something more like:

```
// Check if the given expression contains an assignment expression. This can either take the form of a Binary Operator or a Overloaded Operator Call.
```

The function is dissimilar enough from HasSideEffects that referring people to the original seems unnecessary.

::: build/clang-plugin/clang-plugin.cpp:817
(Diff revision 6)
> +AST_MATCHER(CallExpr, isAssertAssignmentTestFunc) {
> +  static const std::string assertName = "MOZ_AssertAssignmentTest";
> +  const FunctionDecl *method = Node.getDirectCallee();
> +
> +  return method && method->getDeclName().isIdentifier() &&
> +      method->getName() == assertName;

Does StringRef not have an `operator==(const StringRef&, const char *)`?

::: build/clang-plugin/clang-plugin.cpp:1618
(Diff revision 6)
> +  if (!funcCall) {
> +    return;
> +  }
> +
> +  // Evaluate first parameter from the call list
> +  Expr *exprArg = const_cast<Expr*>(funcCall->getArg(0));

Why the const_cast? You're passing it to a function expecting a `const Expr *`

::: build/clang-plugin/clang-plugin.cpp:1620
(Diff revision 6)
> +  if (!exprArg) {
> +    return;
> +  }
> +
> +  if (HasSideEffectAssignment(exprArg)) {
> +    Diag.Report(funcCall->getLocStart(), assignInsteadOfComp);
> +  }

Why not just write:

```
const CallExpr *funcCall = Result.Nodes.getNodeAs<CallExpr>("funcCall");
if (funcCall && HasSideEffectAssignment(funcCall)) {
  Diag.Report(funcCall->getLocStart(), assignInsteadOfComp);
}
```

because your new HasSideEffectAssignment will check the first argument to the function without you having to descend into it explicitly.

You could also add a new AST_MATCHER(Expr, containsAssignmentExpr) which does the HasSideEffectAssignment call, and just unconditionally report the error here.

::: build/clang-plugin/tests/TestAssertWithAssignment.cpp:10
(Diff revision 6)
> + 	  if (MOZ_UNLIKELY(!MOZ_CHECK_ASSERT_ASSIGNMENT(expr))) { \
> + 	    MOZ_CRASH();\
> + 	  } \

You have tabs in here. I assume that this was unintentional.

::: build/clang-plugin/tests/TestAssertWithAssignment.cpp:24
(Diff revision 6)
> +void FunctionTest2(int p) {
> +  MOZ_ASSERT(((p = 1)));  // expected-error {{Forbidden assignment in assert expression}}
> +}
> +
> +void FunctionTest3(int p) {
> +	MOZ_ASSERT(p != 3);

Tab

::: build/clang-plugin/tests/TestAssertWithAssignment.cpp:41
(Diff revision 6)
> +void TestOverloadingFunc() {
> +  TestOverloading p(2);
> +
> +  MOZ_ASSERT(p);
> +
> +  MOZ_ASSERT(p = 3); // expected-error {{Forbidden assignment in assert expression}}

Your new code should also check MOZ_ASSERT(p = 10, true) and MOZ_ASSERT(Call(), p = 10) etc. (and other random more complex expressions)

Why not test that too?
Attachment #8767904 - Flags: review?(michael) → review-
(In reply to Michael Layzell [:mystor] from comment #23)


> 
> ::: build/clang-plugin/clang-plugin.cpp:817
> (Diff revision 6)
> > +AST_MATCHER(CallExpr, isAssertAssignmentTestFunc) {
> > +  static const std::string assertName = "MOZ_AssertAssignmentTest";
> > +  const FunctionDecl *method = Node.getDirectCallee();
> > +
> > +  return method && method->getDeclName().isIdentifier() &&
> > +      method->getName() == assertName;
> 
> Does StringRef not have an `operator==(const StringRef&, const char *)`?
> 
I don't think this is the best approach, since comparing two strings is more efficient than compering a string with a const char*, the first condition that has to be met is the size when comparing strings and this can be given in O(1) hence we avoid iterations. More i don't think that there is an overloading equality operator with that particular signature i think operator==(const StringRef&, const StringRef&) is enough 
  
> ::: build/clang-plugin/clang-plugin.cpp:1618
> (Diff revision 6)
> > +  if (!funcCall) {
> > +    return;
> > +  }
> > +
> > +  // Evaluate first parameter from the call list
> > +  Expr *exprArg = const_cast<Expr*>(funcCall->getArg(0));
> 
> Why the const_cast? You're passing it to a function expecting a `const Expr
> *`

Yes that is totally useless.
> 
> ::: build/clang-plugin/clang-plugin.cpp:1620
> (Diff revision 6)
> > +  if (!exprArg) {
> > +    return;
> > +  }
> > +
> > +  if (HasSideEffectAssignment(exprArg)) {
> > +    Diag.Report(funcCall->getLocStart(), assignInsteadOfComp);
> > +  }
> 
> Why not just write:
> 
> ```
> const CallExpr *funcCall = Result.Nodes.getNodeAs<CallExpr>("funcCall");
> if (funcCall && HasSideEffectAssignment(funcCall)) {
>   Diag.Report(funcCall->getLocStart(), assignInsteadOfComp);
> }
> ```
> 
> because your new HasSideEffectAssignment will check the first argument to
> the function without you having to descend into it explicitly.
> 

> You could also add a new AST_MATCHER(Expr, containsAssignmentExpr) which
> does the HasSideEffectAssignment call, and just unconditionally report the
> error here.
I think this can be added in a later patch when we want to check if we have assignments in argument passing to functions, like:
f(a=2);
> 
> ::: build/clang-plugin/tests/TestAssertWithAssignment.cpp:10
> (Diff revision 6)
> > + 	  if (MOZ_UNLIKELY(!MOZ_CHECK_ASSERT_ASSIGNMENT(expr))) { \
> > + 	    MOZ_CRASH();\
> > + 	  } \
> 
> You have tabs in here. I assume that this was unintentional.
> 
> ::: build/clang-plugin/tests/TestAssertWithAssignment.cpp:24
> (Diff revision 6)
> > +void FunctionTest2(int p) {
> > +  MOZ_ASSERT(((p = 1)));  // expected-error {{Forbidden assignment in assert expression}}
> > +}
> > +
> > +void FunctionTest3(int p) {
> > +	MOZ_ASSERT(p != 3);
> 
> Tab
> 
> ::: build/clang-plugin/tests/TestAssertWithAssignment.cpp:41
> (Diff revision 6)
> > +void TestOverloadingFunc() {
> > +  TestOverloading p(2);
> > +
> > +  MOZ_ASSERT(p);
> > +
> > +  MOZ_ASSERT(p = 3); // expected-error {{Forbidden assignment in assert expression}}
> 
> Your new code should also check MOZ_ASSERT(p = 10, true) and
> MOZ_ASSERT(Call(), p = 10) etc. (and other random more complex expressions)
> 
> Why not test that too?
Will add those examples as well but i didn't add MOZ_ASSERT with two arguments since the 2nd argument is considered a string literal.
Comment on attachment 8767904 [details]
Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62272/diff/6-7/
Attachment #8767904 - Flags: review- → review?(michael)
Attachment #8767904 - Flags: review?(michael) → review+
Comment on attachment 8767904 [details]
Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression.

https://reviewboard.mozilla.org/r/62272/#review61910

::: build/clang-plugin/clang-plugin.cpp:47
(Diff revisions 6 - 7)
> -// type of side effects are we willing to check.
> +// Overloaded Operator Call.
> -// Function can be used having an Expr* and it will return if it nests an
> -// assignment operator, no matter if he is overloaded or not.
>  bool HasSideEffectAssignment(const Expr *expr) {
> -
> -  switch (expr->getStmtClass()) {
> +  if (auto opCallExpr = dyn_cast<CXXOperatorCallExpr>(expr)) {
> +      auto binOp = opCallExpr->getOperator();

Indentation

::: build/clang-plugin/clang-plugin.cpp:51
(Diff revisions 6 - 7)
> -          ->getOperator();
> -      if (binOp == OO_Equal ||
> -          (binOp >= OO_PlusEqual && binOp <= OO_PipeEqual)) {
> -        return true;
> +      return true;
> -      }
> +    }
> -      break;
> +  } else if (auto binOpExpr = dyn_cast_or_null<BinaryOperator>(expr)) {

This shouldn't be a `dyn_cast_or_null` if you have already performed a `dyn_cast<CXXOperatorCallExpr>(expr)`. Either make them both `dyn_cast_or_null` and allow null pointers to be passed in, or make both of them `dyn_cast`
Comment on attachment 8767904 [details]
Bug 1283395 - clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62272/diff/7-8/
Assignee

Updated

3 years ago
Keywords: leave-open

Comment 28

3 years ago
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1988e4c59d1
clang-plugin - add an error if we encounter in MOZ_ASSERT assignment instead of logical expression. r=mystor
Posted patch Bug 1283395_2.diff (obsolete) — Splinter Review
Attachment #8772311 - Flags: review?(nfroyd)
Comment on attachment 8772311 [details] [diff] [review]
Bug 1283395_2.diff

Review of attachment 8772311 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below fixed.

::: mfbt/SAFunctions.h
@@ +7,5 @@
> +#ifndef mozilla_SAFunctions_h
> +#define mozilla_SAFunctions_h
> +
> +#include <stdbool.h>
> + 

Nit: trailing whitespace.

@@ +8,5 @@
> +#define mozilla_SAFunctions_h
> +
> +#include <stdbool.h>
> + 
> +/* Functions that are used as markers in Gecko code for static analysis. Their

Nit: local style is to format like so:

/*
 * Functions that are used...

@@ +38,5 @@
> +#define MOZ_CHECK_ASSERT_ASSIGNMENT(expr) MOZ_AssertAssignmentTest(!!(expr))
> +
> +#else
> +
> +#define MOZ_CHECK_ASSERT_ASSIGNMENT(expr) (expr)

We should make this (!!(expr)) for consistency with the non-MOZ_CLANG_PLUGIN case.

::: mfbt/moz.build
@@ +76,4 @@
>      'RefPtr.h',
>      'ReverseIterator.h',
>      'RollingMean.h',
> +    'SAFunctions.h',

Please call this StaticAnalysisFunctions.h; there's no need to use short names here.
Attachment #8772311 - Flags: review?(nfroyd) → review+
Posted patch Bug 1283395.diff (obsolete) — Splinter Review
Attachment #8772311 - Attachment is obsolete: true
Attachment #8772820 - Flags: review?(nfroyd)
Attachment #8772820 - Flags: review?(nfroyd) → review+

Comment 33

3 years ago
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8e5a0703821
add markup functions for static analysis builds. r=froydnj
Assignee

Updated

3 years ago
Keywords: leave-open
caused bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=32315399&repo=mozilla-inbound and so had to be backed out
Flags: needinfo?(bpostelnicu)

Comment 35

3 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/138937cd04cb
Backed out changeset c8e5a0703821 for bustage on a CLOSED TREE
(In reply to Carsten Book [:Tomcat] from comment #34)
> caused bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=32315399&repo=mozilla-
> inbound and so had to be backed out

yes the problem is with bool type that is being redeclared in stdbool.h on visual studio 2015 compiler.
Flags: needinfo?(bpostelnicu)
Due to the way how jemalloc.c implements bool, true and false, these were redefined in stdbool.h on windows platform:

#ifdef MOZ_MEMORY_WINDOWS

>>/* Some defines from the CRT internal headers that we need here. */
>>#define _CRT_SPINCOUNT 5000
>>#define __crtInitCritSecAndSpinCount InitializeCriticalSectionAndSpinCount
>>#include <io.h>
>>#include <windows.h>
>>#include <intrin.h>

>>#pragma warning( disable: 4267 4996 4146 )

>>#define	bool BOOL
>>#define	false FALSE
>>#define	true TRUE

I will resubmit the patch for review where we include stdbool.h in StaticAnalysisFunctions.h only if the compiler is set for c language and bool was not defined:

>>#ifndef __cplusplus
>>#ifndef bool
>>#include <stdbool.h>
>>#endif
>>#endif
Attachment #8772820 - Attachment is obsolete: true
Attachment #8774351 - Flags: review?(nfroyd)
Comment on attachment 8774351 [details] [diff] [review]
Bug 1283395.diff

Review of attachment 8774351 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/StaticAnalysisFunctions.h
@@ +11,5 @@
> +#ifndef bool
> +#include <stdbool.h>
> +#endif
> +#endif
> +/*

Nit: newline before this comment.
Attachment #8774351 - Flags: review?(nfroyd) → review+

Comment 40

3 years ago
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c839b1d1397
add markup functions for static analysis builds. r=froydnj
I'll take a look and see what's happening, this is a bit strange.
Flags: needinfo?(bpostelnicu)

Comment 43

3 years ago
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a80d40bf2276
add markup functions for static analysis builds. r=froydnj

Comment 44

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a80d40bf2276
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.