Unrelated errors introduce additional runtime diagnostics by skipping CFG construction

[rnk]

Consider: https://gcc.godbolt.org/z/dq6q8xxhY

#include <string.h>
struct Foo {
    virtual ~Foo();
    int x;
};
#error
constexpr bool alwaysFalse = false;
void maywarn(Foo *p) {
    if constexpr (alwaysFalse) {
        memset(p, 0, sizeof(*p));
    }
    if (alwaysFalse) {
        memset(p, 0, sizeof(*p)); // warns
    }
}

This produces this -Wdynamic-class-memaccess warning:

<source>:15:16: warning: destination for this 'memset' call is a pointer to dynamic class 'Foo'; vtable pointer will be overwritten [-Wdynamic-class-memaccess]
        memset(p, 0, sizeof(*p));
        ~~~~~~ ^
<source>:15:16: note: explicitly cast the pointer to silence this warning
        memset(p, 0, sizeof(*p));
               ^
               (void*)

This goes away if you remove the #error.

This causes issues because this latent warning can be present in header files in a warning-clean codebase, and it only appears when developers later introduce an error. The extra warning can obscure the actual problem because, with -Werror, developers end up seeing two errors.

These two conditionals prevent Clang from doing CFG-based warnings in the presence of errors:

• https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/Sema.cpp#L2224
• https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/AnalysisBasedWarnings.cpp#L2227

To fix the bug, we need to modify those conditionals to either:

• Always construct a CFG for analysis even in the presence of errors
• Don't issue any diagnostics about possibly unreachable code in the presence of other errors

I've prototyped both changes, and they break a lot of Clang tests, and I'm filing this issue to track followup.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

[aaronpuchert]

Why does -Wdynamic-class-memaccess even do a reachability analysis? This seems like a regular type-based warning, for which we typically don't check reachability. But there are lots of users of DiagRuntimeBehavior/DiagIfReachable, and potentially all of them are affected by this.

Always construct a CFG for analysis even in the presence of errors

Might be difficult. Especially if the error is in the same function, or affects the function in some way.

Don't issue any diagnostics about possibly unreachable code in the presence of other errors

That seems like the right approach to me. We do the same for -Wthread-safety-analysis, and probably other CFG-based warning.

[chaitanyav]

@rnk @aaronpuchert is this still open to work?

[xgupta]

@rnk @aaronpuchert is this still open to work?

Yes, it seems.

[youngsun4786]

@rnk @aaronpuchert Hello, is this issue open to work? It's my first time contributing to llvm!

[aaronpuchert]

I'm not aware of anyone working on this.