This is an archive of the discontinued LLVM Phabricator instance.

[Sema, NFC] Reset HasFallthroughStmt when clearing FunctionScopeInfo
ClosedPublic

Authored by erik.pilkington on Jul 25 2016, 11:24 AM.

Details

Summary

The FunctionScopeInfo stack in Sema uses an optimization where the memory for the top-level functions is reused. The function FunctionScopeInfo::Clear() is used to reset this memory to the original, marking all the flags as false. The flag HasFallthroughStmt was omitted from this, meaning that once it set, it remained so for the rest of the TU. This meant we ran DiagnoseSwitchLabelsFallthrough() for every top level function in a TU where one function used [[fallthrough]].

Thanks!

Diff Detail

Repository
rL LLVM

Event Timeline

erik.pilkington retitled this revision from to [Sema, NFC] Reset HasFallthroughStmt when clearing FunctionScopeInfo.
erik.pilkington updated this object.
erik.pilkington added a reviewer: rsmith.
erik.pilkington added a subscriber: cfe-commits.
arphaman accepted this revision.Nov 7 2016, 4:42 AM
arphaman added a reviewer: arphaman.
arphaman added a subscriber: arphaman.

LGTM, Thanks.

This revision is now accepted and ready to land.Nov 7 2016, 4:42 AM

I don't suppose there's a testcase we could generate for this fix?

I looked at the way HasFallthroughStmt is used, and it didn't seem so, no. It's used in the following manner in AnalysisBasedWarnings.cpp:

bool FallThroughDiagFull =
    !Diags.isIgnored(diag::warn_unannotated_fallthrough, D->getLocStart());
bool FallThroughDiagPerFunction = !Diags.isIgnored(
    diag::warn_unannotated_fallthrough_per_function, D->getLocStart());
if (FallThroughDiagFull || FallThroughDiagPerFunction ||
    fscope->HasFallthroughStmt) {
  DiagnoseSwitchLabelsFallthrough(S, AC, !FallThroughDiagFull);
}

So it seems to me that even if HasFallthroughStmt isn't cleared, the analysis won't show up anything different for a function that re-used the scope info, because when diagnostics are ignored and the flag actually leads to incorrect call to DiagnoseSwitchLabelsFallthrough , the analysis won't show any warnings since the diagnostics are ignored. And likewise if one of the diagnostics is enabled then it doesn't matter if HasFallthroughStmt is true, so we won't be able to observe a difference there.

This revision was automatically updated to reflect the committed changes.