-Wunreachable-code triggers undesirable warnings when log-streaming into a sometimes-disabled logging macro

[pbos]

In Chromium / abseil we have some logging macros that are sometimes compiled as no-ops. This sort of boils to a more complicated version of this (which I wanted to simplify our implementation to):

#include <iostream>

#define LAZY_STREAM(stream, condition) \
  switch (0)                           \
  case 0:                              \
  default:                             \
    if (!(condition))                  \
      ;                                \
    else                               \
      (stream)

#define ENABLE_LOGGING 0
#define MAYBE_LOG() LAZY_STREAM(std::cout, ENABLE_LOGGING)

int main() {
  MAYBE_LOG();
  MAYBE_LOG() << "hello";  // error: code will never be executed [-Werror,-Wunreachable-code]
}

Here when ENABLE_LOGGING is false MAYBE_LOG() gets correctly compiled into nothing. MAYBE_LOG() << "hello"; however triggers dead-code warnings for the << "hello"; bit when logging is disabled at compile time.

https://godbolt.org/z/f9EPMordr

No idea what heuristic upgrades would make sense here. Special casing << makes sense to me, as may exonerating binary operators with a macro LHS (the dead-codiness comes from inside the macro). Unsure about whether MAYBE_LOG(), foo; should warn on dead foo or not.

chromium and absl currently spells this differently and gets away with streaming logs to dead code:

#define LAZY_STREAM(stream, condition)                                  \
  !(condition) ? (void) 0 : ::logging::LogMessageVoidify() & (stream)

This seems to be sufficient to confuse clang enough to not issue dead-code warnings to (stream) even though it's part of the compile-time-known dead branch of the ternary.

Changing our implementation of LAZY_STREAM to the former results in dead-code warnings ~everywhere in chromium. Ideally we shouldn't make it harder for the compiler to deduce that this is indeed dead code, but that it should be OK since it originates from a macro (or some other heuristic that lets us do this). crrev.com/c/5905021 has the try run that explodes everywhere.

[firewave]

This also happens when using streaming operators with qDebug() from Qt in "release" builds: https://doc.qt.io/qt-6/qtlogging.html#qDebug. I guess it could be the same issue.

[erichkeane]

IMO, I don't see a good way for us to heuristically determine that this is is intentionally dead code. Someone else might be doing the same lexical code with different intent, and actually get value from the diagnostic.

I think we could perhaps have a 'poison the unreachable-code-detector-for-this-statement' attribute, but I don't know enough about the unreachable code analysis to suggest better than that.

[shafik]

I also agree, I don't think this is a false positive either. It seems like this would end up with a lot of false negatives.

[pbos]

Can you think of a good example where exonerating streaming parameters to dead-code statements originating inside macros would be harmful?

Per "This also happens when using streaming operators with qDebug()" above it looks like this warning is at least undesirable in two real instances. To me qDebug() << foo(); and qDebug("%d", foo()); shouldn't necessarily issue different dead-code warnings if neither evaluate foo() and both generate functionally-equivalent dead code.

I don't know that MAYBE_LOG(foo()); that expands to if (true) else std::cout << foo(); should not issue dead-code warnings (current behavior), but MAYBE_LOG() << foo(); that expands to the same should. Is it any more misleading that << foo(); is unreachable when streaming than as a macro argument? I assume we could exonerate something very narrow here and avoid "a lot of false negatives" in the delta between MAYBE_LOG(foo()); and MAYBE_LOG() << foo();.