clang 17.0.2: No "unreachable code" warning if source file included in another source file

[pkl97]

When compiling unreachable_warning.cpp (with the content below) using option -Wunreachable-code-break, a warning is triggered as expected:

void f(int i)
{
    switch (i)
    {
        case 3: return; break;
    }
}

The warning emitted:

[user@host]$ clang++ -c -Wunreachable-code-break unreachable_warning.cpp
unreachable_warning.cpp:5:25: warning: 'break' will never be executed [-Wunreachable-code-break]
        case 3: return; break;
                        ^~~~~
1 warning generated.

However if unreachable_warning.cpp is wrapped in another source file (as is frequently done in "unity builds" or "jumbo builds") then the warning is no longer emitted:

[user@host]$ cat unity_build.cpp
#include "unreachable_warning.cpp"

[user@host]$ clang++ -c -Wunreachable-code-break unity_build.cpp
[user@host]$

[shafik]

This looks purposeful:

llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp

Lines 125 to 134 in fee2953

	static void CheckUnreachable(Sema &S, AnalysisDeclContext &AC) {
	// As a heuristic prune all diagnostics not in the main file. Currently
	// the majority of warnings in headers are false positives. These
	// are largely caused by configuration state, e.g. preprocessor
	// defined code, etc.
	//
	// Note that this is also a performance optimization. Analyzing
	// headers many times can be expensive.
	if (!S.getSourceManager().isInMainFile(AC.getDecl()->getBeginLoc()))
	return;

CC @AaronBallman

[AaronBallman]

Correct, this is behaving by design and I don't think we intend to change the behavior here due to risk of significant increase to compile times.

[pkl97]

Thanks @shafik for the thorough investigation!

@AaronBallman:
Is there any chance to relax this feature a bit? Compile time increase should only be an issue if dealing with actual header files (files with extensions like .h, .hh, .hpp, .hxx) that are included from many different source files.
If the included file is a source file however, the inclusion only occurs once across the whole project, in a single translation unit. So there would be no overhead, except applying the warning to a source file for which it was intended in the first place.
The code could check for an extension like .c, .cc, .cpp, .cxx and only apply the warning in those cases. This should be a rather safe approach.

[AaronBallman]

Thanks @shafik for the thorough investigation!

@AaronBallman: Is there any chance to relax this feature a bit? Compile time increase should only be an issue if dealing with actual header files (files with extensions like .h, .hh, .hpp, .hxx) that are included from many different source files. If the included file is a source file however, the inclusion only occurs once across the whole project, in a single translation unit. So there would be no overhead, except applying the warning to a source file for which it was intended in the first place. The code could check for an extension like .c, .cc, .cpp, .cxx and only apply the warning in those cases. This should be a rather safe approach.

There's always a chance! :-) I generally agree with your logic, but I think this leads to a bit of a game of whack-a-mole with compiler or language features that don't work so well in unity builds. I'm wondering if a better approach would be to design a flag to enable "unity build mode" that configures the compiler to behave in a nicer manner for this use case. For example, this could also allow us to diagnose linkage issues (same identifier in anonymous namespaces that suddenly link together), macro leakage, and other unity-build-specific issues. The downside is: it's also a much bigger feature request than what you're after.

[dwblaikie]

(& at the point we're talking about unity build support - I tend to go further and wonder what problems we have in other infrastructure (LTO, ThinLTO) that could be improved to address the use case for unity builds in a more scalable manner (& one more consistent with other users/use cases we have))

[vszakats]

With unity, this particular issue is the first downside we hit so far.

IMO LTO and unity/jumbo target different problems. unity is useful to make the build (much) faster, while LTO is making the result run faster with usually slower (but certainly not much faster) build times. (since link time, it also involves dealing with dependencies and potential issues with them.) Disk footprint also has different properties.

It's also not just unity mode. There can be other legitimate reasons to include a .c source into another one. libssh2 uses it to include the selected crypto backend code without resorting to build/make-specific tricks.

The suggested method to make disctinction between included headers vs. source via suffix sounds nice and would solve both unity and other cases without the complications of a new compiler option.

[dwblaikie]

With unity, this particular issue is the first downside we hit so far.

IMO LTO and unity/umbo target different problems. unity is useful to make the build (much) faster, while LTO is making the result run faster with usually slower (but certainly not much faster) build times. (since link time, it also involves dealing with dependencies and potential issues with them.) Disk footprint also has different properties.

Ah, fair enough - my mistake. Header modules might be more relevant, then.

It's also not just unity mode. There can be other legitimate reasons to include a .c source into another one. libssh2 uses it to include the selected crypto backend code without resorting to build/make-specific tricks.

The suggested method to make disctinction between included headers vs. source via suffix sounds nice and would solve both unity and other cases without the complications of a new compiler option.

Yeah - I suspect there's an underlying issue that this warning just isn't very good/robust/fully developed. So I'd mostly just discourage people from using it for now - not sure how I feel about making an extension based filter... I guess I wouldn't outright say no, and it's probably more @AaronBallman's call, but I'm not super keen, let's say.