This is an archive of the discontinued LLVM Phabricator instance.

Fix a bug in unavailable checking
ClosedPublic

Authored by manmanren on Dec 7 2015, 3:44 PM.

Details

Summary

The issue —————————————
Given the following test case, we don’t emit any message, and will silently ignore the variable "foo2", with a release compiler; we will get an assertion failure with an assert compiler.
typedef struct {

const char *name;
id field;

} Test9;

extern void doSomething(Test9 arg);
void test9() {

Test9 foo2 = {0, 0};
doSomething(foo2);

}

The root cause —————————————
Back in r140457 we gave InitListChecker a verification-only mode, and will use CanUseDecl instead of DiagnoseUseOfDecl for verification-only mode.
These two functions handle unavailable issues differently:
In Sema::CanUseDecl, we say the decl is invalid with the following condition

if (D->getAvailability() == AR_Unavailable &&
    cast<Decl>(CurContext)->getAvailability() != AR_Unavailable)

In Sema::DiagnoseUseOfDecl, we say the decl is usable by ignoring the return code of DiagnoseAvailabilityOfDecl

So with an assert build, we will hit an assertion in diagnoseListInit

assert(DiagnoseInitList.HadError() &&
       "Inconsistent init list check result.");

The question is how to make the two consistent, should we follow what is implemented in CanUseDecl or DiagnoseUseOfDecl?
If we follow what is implemented in CanUseDecl and treat Decls with unavailable issues as invalid, the variable decl of “foo2” will be marked as invalid.
Since unavailable checking is processed in delayed diagnostics (r197627), we will silently ignore the diagnostics when we find out that the variable decl is invalid.

In Sema::PopParsingDeclaration

switch (diag.Kind) {
case DelayedDiagnostic::Deprecation:
case DelayedDiagnostic::Unavailable:
  // Don't bother giving deprecation/unavailable diagnostics if
  // the decl is invalid.
  if (!decl->isInvalidDecl())
    handleDelayedAvailabilityCheck(*this, diag, decl);

The proposed fix ———————————————
It seems that for overload resolution, we want to say decls with unavailable issues are invalid; but for everything else, we should say they are valid but
emit diagnostics. For this, we can add another flag “UnavailableCheck” for the verification-only mode. Depending on the value of the flag, CanUseDecl
can return different values for unavailable issues.

Feedback is appreciated. Any other suggestion is welcome.

Cheers,
Manman

Diff Detail

Event Timeline

manmanren updated this revision to Diff 42117.Dec 7 2015, 3:44 PM
manmanren retitled this revision from to Fix a bug in unavailable checking.
manmanren updated this object.
manmanren added reviewers: rjmccall, rsmith.
manmanren added a subscriber: cfe-commits.

Ping

Appreciate comments on the general direction.

Cheers,
Manman

doug.gregor edited edge metadata.Feb 16 2016, 4:33 PM

The approach and patch look okay to me, but can we give "UnavailableCheck" a less ambiguous name? For example, "TreatUnavailableAsInvalid"?

manmanren updated this revision to Diff 48228.Feb 17 2016, 12:05 PM
manmanren edited edge metadata.

Doug,

Thanks for reviewing! I updated the patch to use TreatUnavailableAsInvalid.

Cheers,
Manman

doug.gregor accepted this revision.Mar 9 2016, 3:55 PM
doug.gregor edited edge metadata.

LGTM

This revision is now accepted and ready to land.Mar 9 2016, 3:55 PM
This revision was automatically updated to reflect the committed changes.