[cfe-dev] Warnings not raised from a macro: -Wundefined-bool-conversion/-Wtautological-undefined-compare

[Richtarsky, Martin]

> How is the macro supposed to know whether the pointer you gave
> it is the magically non-nullable kind or the regular flavour?
> Are you going to ask people to use two different macros depending on
> whether the argument is nullable or not?

I was expecting the preprocessing step to expand the code and the
warning generation being independent from that.

> One alternative is to build with -fsanitize=null,nonnull-attribute,returns-nonnull-attribute and run their tests.

That's possible but such things should be caught at compile time in a static language.

> You could gin up a patched clang that comments out the "Don't warn inside macros" logic in Sema::DiagnoseAlwaysNonNullPointer

Thanks for the pointer, that's helpful. Would a patch be welcome to control this from the command line (globally for all warnings which are disabled in macros, e.g. -warn-inside-macros)?

Joerg's suggestion is also helpful, it prints all warnings and I can keep fixing code while being sure I catch everything. But ideally I want to avoid the additional I/O of writing .ii and .s.

Thanks and Best regards,
Martin

From: Nick Lewycky [mailto:nlewycky at google.com] 
Sent: Mittwoch, 5. November 2014 08:05
To: Richtarsky, Martin
Cc: cfe-dev at cs.uiuc.edu
Subject: Re: [cfe-dev] Warnings not raised from a macro: -Wundefined-bool-conversion/-Wtautological-undefined-compare

On 4 November 2014 02:15, Richtarsky, Martin <martin.richtarsky at sap.com> wrote:
Hi,

while cleaning some code of "-Wundefined-bool-conversion"/"-Wtautological-undefined-compare" warnings I found that these are never raised when coming from a macro. I know this is intentional in clang, but I wonder whether the benefits of this really outweigh the risks.

clang 3.5 optimizes these checks away, so it is crucial to find all places that rely on them in the codebase. But when the code comes from a macro there is suddenly no way to do so at compile time. See functions f2() below.

I think this is correct behaviour. How is the macro supposed to know whether the pointer you gave it is the magically non-nullable kind or the regular flavour? Are you going to ask people to use two different macros depending on whether the argument is nullable or not?

How are others dealing with this problem e.g. at Google and Apple with large codebases likely affected by this?

With a similar macro, CHECK, I've been seeing callers doing "CHECK(this != nullptr);" and that gets flagged by the warning. I haven't seen any failures due to a pattern like you describe, and only something like a dozen or so code changes at Google.

One alternative is to build with -fsanitize=null,nonnull-attribute,returns-nonnull-attribute and run their tests.

But maybe you don't have tests. You could gin up a patched clang that comments out the "Don't warn inside macros" logic in Sema::DiagnoseAlwaysNonNullPointer. But maybe you can't easily vend a deliberately-non-production compiler. The last trick up my sleeve is clang-query.

clang-query> match implicitCastExpr(hasImplicitDestinationType(asString("_Bool")), hasSourceExpression(thisExpr()))

Match #1:

/usr/local/google/home/nlewycky/sap.cc:3:12: note: "root" binds here
    return this;
           ^~~~
1 match.


Hrm. I tried writing the obvious thing for address of reference, but this doesn't work:

clang-query> match implicitCastExpr(hasImplicitDestinationType(asString("_Bool")), hasSourceExpression(unaryOperator(hasOperatorName("&"), hasUnaryOperand(hasType(referenceType())))))
0 matches.

because apparently nothing has referenceType():

clang-query> match expr(hasType(referenceType()))
0 matches.
clang-query> match expr(hasType(qualType(referenceType())))
0 matches.

The testcase is:

bool test2(int &i) {
  return &i;
}

Not sure what I'm doing wrong with clang-query.

Nick

$ cat undefined+macro.cpp
----
#define RET(expr) \
  return (!(expr))

class A
{

int f1()
{
  return (!this);  // warns
}

int f2()
{
  RET(this);  // does NOT warn
}

};

int f1()
{
  int a = 1;
  int &r = a;
  return (&r != 0);  // warns
}

int f2()
{
  int a = 1;
  int &r = a;
  RET(&r != 0);  // does NOT warn
}
----

$ clang++ -c  undefined+macro.cpp


undefined+macro.cpp:9:12: warning: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Wundefined-bool-conversion]
  return (!this);  // warns
          ~^~~~
undefined+macro.cpp:23:12: warning: reference cannot be bound to dereferenced null pointer in well-defined C++ code; comparison may be assumed to always evaluate to true [-Wtautological-undefined-compare]
  return (&r != 0);  // warns
           ^    ~
2 warnings generated.


Best regards,
Martin

_______________________________________________
cfe-dev mailing list
cfe-dev at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev