Warnings not raised from a macro: -Wundefined-bool-conversion/-Wtautological-undefined-compare

Clang Frontend
#1

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.

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

$ cat undefined+macro.cpp

#2

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

#3

Hi,

I think Martin is right. Preprocessor is only textual replacement, compiler should not know about this step and compiles code resulting from preprocess step.

I used Martin’s source code I have got different number of warnings if preprocess step was made as extra step before compilation, in my opinion it is a bug.

Common sense tells me that this “equation” should be true every time: clang++ -c = clang++ -E + clang++ -c.

Mira

d:\Tmp>clang++ -v

clang version 3.6.0 (220682)

Target: i686-pc-windows-gnu

Thread model: posix

d:\Tmp>clang++ -c foo.cpp

foo.cpp:9:13: 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

~ ^~~~

foo.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

^ ~

foo.cpp:30:8: 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]

RET(&r != 0); // does NOT warn

^ ~

foo.cpp:2:13: note: expanded from macro ‘RET’

return (!(expr))

^

3 warnings generated.

d:\Tmp>clang++ -E foo.cpp > foo2.cpp

d:\Tmp>clang++ -c foo2.cpp

foo.cpp:9:13: 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));

~ ^~~~

foo.cpp:14:13: 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));

~ ^~~~

foo.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);

^ ~

foo.cpp:30:14: 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));

^ ~

4 warnings generated.

#4

A lot of people disagree. If you want to force it to give all warnings,
use -save-temps.

Joerg

#5

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

#6

Hi,

Thanks for the -save-temps hint, it helps with this particular issue,
unfortunatelly it does not solve the cause of the problem.

I guess that Clang-based tools inherit the same macro treatment from clang
parser and give an inaccurate results.

For example this code fragment is handled incorrectly by command: clang-tidy
-check=-*,readability-braces-around-statements -fix

#define MACRO_1 i++
#define MACRO_2

void test(int i)
{
  if( i % 3) i--; // OK
  else if( i % 2) MACRO_1; // KO, no braces
  else MACRO_2; // OK
}

Maybe I am stubborn, I still think that preprocess step should be made
completely before the next steps.

Best regards

Mira

#7

+curdeius@gmail.com

I found this thread while searching for something else in the list archive.

This report would definitely be more visible on llvm.org/bugs, I’ve filed it as llvm.org/PR22785.

The issue has nothing to do with the Clang’s treatment of macros in -Wundefined-bool-conversion/-Wtautological-undefined-compare diagnostics. It’s just an over-eager attempt to avoid breaking the code by messing with macros.

Marek (cc-ed here) is the author of the corresponding check. Maybe he’ll be interested in fixing the issue.

– Alex

#8

Hi,

As Alex has stated already, the problem lies in the fact that the code doesn’t handle macros.
That wasn’t an attempt to avoid breaking code, but rather a question of simplicity and lack of knowledge about clang-tidy’s way of handling macros.
I’ll take a look at it in my spare time this weekend and give you more feedback about what can be done.
I’ve just seen that twice in the code, the check returns immediately if it encounters a macro.

Best regards,
Marek

#9

Hi,
As Alex has stated already, the problem lies in the fact that the code
doesn't handle macros.
That wasn't an attempt to avoid breaking code, but rather a question of
simplicity and lack of knowledge about clang-tidy's way of handling macros.
I'll take a look at it in my spare time this weekend and give you more
feedback about what can be done.
I've just seen that twice in the code, the check returns immediately if it
encounters a macro.

It's the easiest way to avoid messing up code in macros (it was probably me
who suggested inserting this check), but there may be a more accurate way
to do this that won't lead to completely failing to insert braces when a
statement is a single macro expansion.

As a possible way to solve this, I'd suggest trying to check if both
locations where the braces are going to be inserted are on the same level
of macro expansion in the same file. Lexer::makeFileCharRange is a rather
advanced method that could help here.

In any case, it makes sense to continue the discussion (if any) on
llvm.org/PR22785.

-- Alex