Skip to content

Handle incomplete types in BaseRange #1117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jul 13, 2021
3 changes: 3 additions & 0 deletions clang/include/clang/AST/ExprUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,9 @@ class ExprUtil {
static bool EqualValue(ASTContext &Ctx, Expr *E1, Expr *E2,
EquivExprSets *EquivExprs);

// EqualTypes returns true if T1 and T2 are lexicographically equivalent.
static bool EqualTypes(ASTContext &Ctx, QualType T1, QualType T2);

// CheckIsNonModifying suppresses diagnostics while checking
// whether e is a non-modifying expression.
static bool CheckIsNonModifying(Sema &S, Expr *E);
Expand Down
5 changes: 5 additions & 0 deletions clang/lib/AST/ExprUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,11 @@ bool ExprUtil::EqualValue(ASTContext &Ctx, Expr *E1, Expr *E2,
return R == Lexicographic::Result::Equal;
}

bool ExprUtil::EqualTypes(ASTContext &Ctx, QualType T1, QualType T2) {
Lexicographic::Result R = Lexicographic(Ctx, nullptr).CompareType(T1, T2);
return R == Lexicographic::Result::Equal;
}

bool ExprUtil::CheckIsNonModifying(Sema &S, Expr *E) {
return S.CheckIsNonModifying(E, Sema::NonModifyingContext::NMC_Unknown,
Sema::NonModifyingMessage::NMM_None);
Expand Down
82 changes: 72 additions & 10 deletions clang/lib/Sema/SemaBounds.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -931,9 +931,10 @@ namespace {
// The proof system is incomplete, so there are will be statements that
// cannot be proved true or false. That's why "maybe" is a result.
enum class ProofResult {
True, // Definitely provable.
False, // Definitely false (an error)
Maybe // We're not sure yet.
True, // Definitely provable.
False, // Definitely false (an error)
Maybe, // We're not sure yet.
IncompleteTypes // Unable to prove due to incomplete referent types.
};

// The kind of statement that we are trying to prove true or false.
Expand Down Expand Up @@ -1036,6 +1037,7 @@ namespace {
enum Kind {
ConstantSized,
VariableSized,
IncompleteConstantSized,
Invalid
};

Expand All @@ -1046,24 +1048,29 @@ namespace {
llvm::APSInt UpperOffsetConstant;
Expr *LowerOffsetVariable;
Expr *UpperOffsetVariable;
bool LowerConstantIncomplete;
bool UpperConstantIncomplete;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may suffice to have just one boolean field IsConstantIncomplete.

In CreateBaseRange, we get LowerKind (returned by SplitIntoBaseAndOffset on Lower) and UpperKind (returned by SplitIntoBaseAndOffset on Upper) Then, IsConstantIncomplete is set to true if either LowerKind or UpperKind is IncompleteConstantSized.

If we do this, the function CheckIncompleteConstantOffsets will not be required (this is causing the function EqualTypes to be called twice unnecessarily). Then we can directly call CheckIncompleteOffset.


public:
BaseRange(Sema &S) : S(S), Base(nullptr), LowerOffsetConstant(1, true),
UpperOffsetConstant(1, true), LowerOffsetVariable(nullptr), UpperOffsetVariable(nullptr) {
UpperOffsetConstant(1, true), LowerOffsetVariable(nullptr), UpperOffsetVariable(nullptr),
LowerConstantIncomplete(false), UpperConstantIncomplete(false) {
}

BaseRange(Sema &S, Expr *Base,
llvm::APSInt &LowerOffsetConstant,
llvm::APSInt &UpperOffsetConstant) :
S(S), Base(Base), LowerOffsetConstant(LowerOffsetConstant), UpperOffsetConstant(UpperOffsetConstant),
LowerOffsetVariable(nullptr), UpperOffsetVariable(nullptr) {
LowerOffsetVariable(nullptr), UpperOffsetVariable(nullptr),
LowerConstantIncomplete(false), UpperConstantIncomplete(false) {
}

BaseRange(Sema &S, Expr *Base,
Expr *LowerOffsetVariable,
Expr *UpperOffsetVariable) :
S(S), Base(Base), LowerOffsetConstant(1, true), UpperOffsetConstant(1, true),
LowerOffsetVariable(LowerOffsetVariable), UpperOffsetVariable(UpperOffsetVariable) {
LowerOffsetVariable(LowerOffsetVariable), UpperOffsetVariable(UpperOffsetVariable),
LowerConstantIncomplete(false), UpperConstantIncomplete(false) {
}

private:
Expand Down Expand Up @@ -1249,6 +1256,11 @@ namespace {
return ProofResult::Maybe;
}

// Check whether we are allowed to continue with the proof for constant
// offsets based on expressions with incomplete referent types.
if (!CheckIncompleteConstantOffsets(R))
return ProofResult::IncompleteTypes;

if (ExprUtil::EqualValue(S.Context, Base, R.Base, EquivExprs)) {
ProofResult LowerBoundsResult = CompareLowerOffsetsImpl(R, Cause, EquivExprs, Facts);
ProofResult UpperBoundsResult = CompareUpperOffsetsImpl(R, Cause, EquivExprs, Facts);
Expand Down Expand Up @@ -1298,6 +1310,11 @@ namespace {
return ProofResult::Maybe;
}

// Check whether we are allowed to continue with the proof for constant
// offsets based on expressions with incomplete referent types.
if (!CheckIncompleteConstantOffsets(R))
return ProofResult::IncompleteTypes;

FreeVariablePosition BasePos = CombineFreeVariablePosition(
FreeVariablePosition::Lower, FreeVariablePosition::Upper);
FreeVariablePosition DeclaredBasePos = CombineFreeVariablePosition(
Expand Down Expand Up @@ -1326,6 +1343,39 @@ namespace {
return ProofResult::Maybe;
}

// This function checks whether it is possible to compare the lower and
// upper offsets of this and R, based on whether the offsets were derived
// from an expression involving a base with an incomplete referent type.
bool CheckIncompleteConstantOffsets(BaseRange &R) {
return CheckIncompleteOffset(R, LowerConstantIncomplete,
R.LowerConstantIncomplete) &&
CheckIncompleteOffset(R, UpperConstantIncomplete,
R.UpperConstantIncomplete);
}

// This function checks whether it is possible to compare the lower or
// upper offset of this and R, based on whether the offset was derived
// from an expression involving a base with an incomplete referent type.
//
// If both offsets are based on incomplete referent types T1 and T2,
// then the offsets should have been multiplied by sizeof(T1) and
// sizeof(T2), where the sizes of T1 and T2 could not be determined.
// In this case, we require that T1 and T2 are equivalent types.
// Otherwise, we cannot continue to compare the offsets.
//
// If only one of the offsets is based on an incomplete referent type,
// then we cannot continue to compare the offsets.
bool CheckIncompleteOffset(BaseRange &R, bool IsIncomplete, bool RIsIncomplete) {
if (IsIncomplete) {
if (!RIsIncomplete)
return false;
if (!ExprUtil::EqualTypes(S.Context, Base->getType(), R.Base->getType()))
return false;
} else if (RIsIncomplete)
return false;
return true;
}

// This function proves whether this.LowerOffset <= R.LowerOffset.
// Depending on whether these lower offsets are ConstantSized or VariableSized, various cases should be checked:
// - If `this` and `R` both have constant lower offsets (i.e., if the following condition holds:
Expand Down Expand Up @@ -1735,6 +1785,14 @@ namespace {
UpperOffsetVariable = Upper;
}

void SetLowerConstantIncomplete(bool Incomplete) {
LowerConstantIncomplete = Incomplete;
}

void SetUpperConstantIncomplete(bool Incomplete) {
UpperConstantIncomplete = Incomplete;
}

void Dump(raw_ostream &OS) {
OS << "Range:\n";
OS << "Base: ";
Expand Down Expand Up @@ -1824,7 +1882,7 @@ namespace {
}
llvm::APSInt ElemSize;
if (!ExprUtil::getReferentSizeInChars(S.Context, Base->getType(), ElemSize))
goto exit;
return BaseRange::Kind::IncompleteConstantSized;
OffsetConstant = OffsetConstant.smul_ov(ElemSize, Overflow);
if (Overflow)
goto exit;
Expand Down Expand Up @@ -2030,11 +2088,11 @@ namespace {
Expr *Upper = RB->getUpperExpr();
Expr *LowerBase, *UpperBase;
llvm::APSInt LowerOffsetConstant(1, true);
llvm::APSInt UpperOffsetConstant(1, true);
llvm::APSInt UpperOffsetConstant(1, true);
Expr *LowerOffsetVariable = nullptr;
Expr *UpperOffsetVariable = nullptr;
SplitIntoBaseAndOffset(Lower, LowerBase, LowerOffsetConstant, LowerOffsetVariable);
SplitIntoBaseAndOffset(Upper, UpperBase, UpperOffsetConstant, UpperOffsetVariable);
BaseRange::Kind LowerKind = SplitIntoBaseAndOffset(Lower, LowerBase, LowerOffsetConstant, LowerOffsetVariable);
BaseRange::Kind UpperKind = SplitIntoBaseAndOffset(Upper, UpperBase, UpperOffsetConstant, UpperOffsetVariable);

// If both of the offsets are constants, the range is considered constant-sized.
// Otherwise, it is a variable-sized range.
Expand All @@ -2044,6 +2102,8 @@ namespace {
R->SetLowerVariable(LowerOffsetVariable);
R->SetUpperConstant(UpperOffsetConstant);
R->SetUpperVariable(UpperOffsetVariable);
R->SetLowerConstantIncomplete(LowerKind == BaseRange::Kind::IncompleteConstantSized);
R->SetUpperConstantIncomplete(UpperKind == BaseRange::Kind::IncompleteConstantSized);
return true;
}
}
Expand Down Expand Up @@ -2106,6 +2166,8 @@ namespace {
DeclaredRange, Cause, EquivExprs, Facts, FreeVariables);
if (R == ProofResult::True)
return R;
if (R == ProofResult::IncompleteTypes)
return ProofResult::Maybe;
if (R == ProofResult::False || R == ProofResult::Maybe) {
if (R == ProofResult::False && SrcRange.IsEmpty())
Cause = CombineFailures(Cause, ProofFailure::SrcEmpty);
Expand Down
34 changes: 33 additions & 1 deletion clang/test/CheckedC/static-checking/bounds-decl-checking.c
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,9 @@ void test_addition_commutativity(void) {
// Test uses of incomplete types

struct S;
struct R;
extern void test_f30(_Array_ptr<const void> p_ptr : byte_count(1));
extern void test_f31(_Array_ptr<struct S> p : count(0));

int f30(_Ptr<struct S> p) {
// TODO: Github Checked C repo issue #422: Extend constant-sized ranges to cover Ptr to an incomplete type
Expand All @@ -286,7 +288,37 @@ int f30(_Ptr<struct S> p) {
return 0;
}

int f31(_Ptr<void> p) {
int f31(_Array_ptr<struct S> p : count(1), _Array_ptr<struct R> q : count(1)) { // expected-note {{(expanded) declared bounds are 'bounds(p, p + 1)'}}
// We cannot compare unequal incomplete referent types 'struct S' and 'struct R' of p and q.
p = (_Array_ptr<struct S>)q; // expected-warning {{cannot prove declared bounds for 'p' are valid after assignment}} \
// expected-note {{(expanded) inferred bounds are 'bounds(q, q + 1)'}}
}

int f32(_Array_ptr<struct S> p : count(0), _Array_ptr<struct S> q : count(1)) {
// p and q have the same incomplete referent type 'struct S'.
p = q;
}

int f33(_Ptr<struct S> p) {
test_f31(p);
return 0;
}

int f34(_Array_ptr<struct S> p : bounds(p, p), _Array_ptr<struct S> q : count(0)) { // expected-note {{(expanded) declared bounds are 'bounds(p, p)'}}
// The upper bound q + 0 is an incomplete constant sized upper offset,
// but the upper bound p is not an incomplete constant sized upper offset.
p = q; // expected-warning {{cannot prove declared bounds for 'p' are valid after assignment}} \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the expected warning on line 310 in this test case is not really correct. The lower/upper bounds of p are not marked as Lower/UpperConstantIncomplete because of the way these fields are set. Currently, these fields are set only when the call to GetReferentSizeInChars fails. In the case of the bounds expression (p, p), I guess the control is going to the exit label in SplitIntoBaseAndOffset where the lower and upper bounds are split as p + 0 without calling GetReferentSizeInChars, and therefore the incompleteness of the type of p is not recorded.

I suggest the following:
Try checking if the type of Base is incomplete in the code under the exit label in SplitIntoBaseAndOffset. If the type is incomplete, return Kind::IncompleteConstantSized. If this gets further complicated or causes more issues/failures, file an issue for this test case and adding a TODO here with a reference to the filed issue.

Ideally, a BaseRange object must be marked as having an incomplete type if the type of the base (in this case p) is incomplete.

// expected-note {{(expanded) inferred bounds are 'bounds(q, q + 0)'}}
return 0;
}

int f35(_Array_ptr<struct S> p : count(i), _Array_ptr<struct S> q : count(i + 1), int i) { // expected-note {{(expanded) declared bounds are 'bounds(p, p + i)'}}
p = q; // expected-warning {{cannot prove declared bounds for 'p' are valid after assignment}} \
// expected-note {{(expanded) inferred bounds are 'bounds(q, q + i + 1)'}}
return 0;
}

int f36(_Ptr<void> p) {
test_f30(p);
return 0;
}
Expand Down