From d4923e2b69bc46240f56461d76b61baa6c7cdf4b Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Tue, 28 Sep 2021 14:04:11 -0400 Subject: [PATCH 1/8] mkString fixes that were previously part of the multi-decl overhaul. Back when the multi-decl overhaul (https://github.com/correctcomputation/checkedc-clang/pull/657) switched all unchanged multi-decl members from Decl::print to mkString, these fixes were needed to prevent 3C's output from getting worse on some unchanged multi-decl members in the regression tests. The fixes are no longer needed for that reason, but Mike still asked me to submit them. They do benefit some more complex cases that occur in the regression tests, so the updates to the expected output of those tests provide test coverage for the fixes. --- clang/lib/3C/ConstraintVariables.cpp | 29 +++++++++++++++++++++++++--- clang/test/3C/compound_literal.c | 2 +- clang/test/3C/fptr_array.c | 10 +++++----- clang/test/3C/ptr_array.c | 4 ++-- 4 files changed, 34 insertions(+), 11 deletions(-) diff --git a/clang/lib/3C/ConstraintVariables.cpp b/clang/lib/3C/ConstraintVariables.cpp index 4da97be8b4b0..0b1b5ae0626e 100644 --- a/clang/lib/3C/ConstraintVariables.cpp +++ b/clang/lib/3C/ConstraintVariables.cpp @@ -833,10 +833,31 @@ PointerVariableConstraint::mkString(Constraints &CS, if (!ForItype && InferredGenericIndex == -1 && isVoidPtr()) K = Atom::A_Wild; + // In a case like `_Ptr g[1]`, we need to push the ` g` onto EndStrs + // before we can push the `>` and start drilling into T. + // + // Exception: In a case like `int *g[1]`, we don't want an extra space after + // the `*` but we don't yet know whether we have a `*`, so we skip emitting + // the name. We have to also skip the addArrayAnnotations because it would + // cause the array annotations to be emitted before the name. + // + // Exception to the exception: In a case like `void (*fs[2])()`, we still + // want to avoid emitting the name (which would add an extra space after the + // `*`), but we _do_ need to call addArrayAnnotations so the array + // annotations end up with the variable name rather than after the function + // parameter list. The function pointer code knows to emit the name before + // EndStrs. + // + // This passes the current regression tests but feels very ad-hoc. + // REVIEW: Help me redesign this code! if (PrevArr && ArrSizes.at(TypeIdx).first != O_SizedArray && !EmittedName) { - EmittedName = true; - addArrayAnnotations(ConstArrs, EndStrs); - EndStrs.push_front(" " + UseName); + if (K != Atom::A_Wild || FV != nullptr) { + addArrayAnnotations(ConstArrs, EndStrs); + } + if (K != Atom::A_Wild) { + EmittedName = true; + EndStrs.push_front(" " + UseName); + } } PrevArr = ArrSizes.at(TypeIdx).first == O_SizedArray; @@ -926,6 +947,8 @@ PointerVariableConstraint::mkString(Constraints &CS, // the the stack array type. if (PrevArr && !EmittedName && AllArrays) { EmittedName = true; + // Note: If the whole type is an array, we can't have a "*", so we don't + // need to worry about an extra space. EndStrs.push_front(" " + UseName); } diff --git a/clang/test/3C/compound_literal.c b/clang/test/3C/compound_literal.c index b749aeb3ad7d..d656369b3abe 100644 --- a/clang/test/3C/compound_literal.c +++ b/clang/test/3C/compound_literal.c @@ -67,7 +67,7 @@ void lists() { int *d[2] = (int *[2]){&x, (int *)1}; //CHECK_NOALL: int *d[2] = (int *[2]){&x, (int *)1}; - //CHECK_ALL: int * d _Checked[2] = (int * _Checked[2]){&x, (int *)1}; + //CHECK_ALL: int *d _Checked[2] = (int * _Checked[2]){&x, (int *)1}; int *d0 = d[0]; //CHECK: int *d0 = d[0]; diff --git a/clang/test/3C/fptr_array.c b/clang/test/3C/fptr_array.c index 2e6c22907303..a82e1db710e9 100644 --- a/clang/test/3C/fptr_array.c +++ b/clang/test/3C/fptr_array.c @@ -9,7 +9,7 @@ void (*fs[2])(int *); void (*f)(int *); -//CHECK_NOALL: void (* fs[2])(_Ptr) = {((void *)0)}; +//CHECK_NOALL: void (*fs[2])(_Ptr) = {((void *)0)}; //CHECK_NOALL: void (*f)(_Ptr) = ((void *)0); //CHECK_ALL: _Ptr)> fs _Checked[2] = {((void *)0)}; //CHECK_ALL: _Ptr)> f = ((void *)0); @@ -17,7 +17,7 @@ void (*f)(int *); void (*gs[2])(int *); void g_impl(int *x) { x = 1; } void (*g)(int *) = g_impl; -//CHECK_NOALL: void (* gs[2])(int * : itype(_Ptr)) = {((void *)0)}; +//CHECK_NOALL: void (*gs[2])(int * : itype(_Ptr)) = {((void *)0)}; //CHECK_NOALL: void g_impl(int *x : itype(_Ptr)) { x = 1; } //CHECK_NOALL: void (*g)(int * : itype(_Ptr)) = g_impl; @@ -30,21 +30,21 @@ void (*h)(void *); int *(*is[2])(void); int *(*i)(void); -//CHECK_NOALL: _Ptr (* is[2])(void) = {((void *)0)}; +//CHECK_NOALL: _Ptr (*is[2])(void) = {((void *)0)}; //CHECK_NOALL: _Ptr (*i)(void) = ((void *)0); //CHECK_ALL: _Ptr<_Ptr (void)> is _Checked[2] = {((void *)0)}; //CHECK_ALL: _Ptr<_Ptr (void)> i = ((void *)0); int *(**js[2])(void); int *(**j)(void); -//CHECK_NOALL: _Ptr (** js[2])(void) = {((void *)0)}; +//CHECK_NOALL: _Ptr (**js[2])(void) = {((void *)0)}; //CHECK_NOALL: _Ptr (**j)(void) = ((void *)0); //CHECK_ALL: _Ptr<_Ptr<_Ptr (void)>> js _Checked[2] = {((void *)0)}; //CHECK_ALL: _Ptr<_Ptr<_Ptr (void)>> j = ((void *)0); int *(*ks[2][2])(void); int *(*k)(void); -//CHECK_NOALL: _Ptr (* ks[2][2])(void) = {((void *)0)}; +//CHECK_NOALL: _Ptr (*ks[2][2])(void) = {((void *)0)}; //CHECK_NOALL: _Ptr (*k)(void) = ((void *)0); //CHECK_ALL: _Ptr<_Ptr (void)> ks _Checked[2] _Checked[2] = {((void *)0)}; //CHECK_ALL: _Ptr<_Ptr (void)> k = ((void *)0); diff --git a/clang/test/3C/ptr_array.c b/clang/test/3C/ptr_array.c index ab4fa535bb41..72ea53909cb7 100644 --- a/clang/test/3C/ptr_array.c +++ b/clang/test/3C/ptr_array.c @@ -28,7 +28,7 @@ void test1(int *a) { int *b[1] = {a}; //CHECK_NOALL: int *b[1] = {a}; - //CHECK_ALL: int * b _Checked[1] = {a}; + //CHECK_ALL: int *b _Checked[1] = {a}; } /* Example from from the issue */ @@ -40,7 +40,7 @@ int *foo() { int z = 3; int *ptrs[4] = {&x, &y, &z, (int *)5}; //CHECK_NOALL: int *ptrs[4] = {&x, &y, &z, (int *)5}; - //CHECK_ALL: int * ptrs _Checked[4] = {&x, &y, &z, (int *)5}; + //CHECK_ALL: int *ptrs _Checked[4] = {&x, &y, &z, (int *)5}; int *ret; //CHECK: int *ret; for (int i = 0; i < 4; i++) { From 6d3c495d113d59ad116999cc1df2eb2980f060df Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Tue, 28 Sep 2021 14:33:15 -0400 Subject: [PATCH 2/8] Bonus fix: Avoid an extra space after a return type ending in `*`. I'm including this because the code fix was similar to a fix that was originally in the multi-decl overhaul before the same fix was needed in https://github.com/correctcomputation/checkedc-clang/pull/702 and I was aware of the issue from item 4 of https://github.com/correctcomputation/checkedc-clang/issues/703. --- clang/lib/3C/ConstraintVariables.cpp | 2 +- clang/test/3C/itype_typedef.c | 2 +- clang/test/3C/itypes_for_extern.c | 9 +++++++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/clang/lib/3C/ConstraintVariables.cpp b/clang/lib/3C/ConstraintVariables.cpp index 0b1b5ae0626e..4d7f4f828f17 100644 --- a/clang/lib/3C/ConstraintVariables.cpp +++ b/clang/lib/3C/ConstraintVariables.cpp @@ -1001,7 +1001,7 @@ PointerVariableConstraint::mkString(Constraints &CS, Ss << Str; } - if (IsReturn && !ForItype) + if (IsReturn && !ForItype && !StringRef(Ss.str()).endswith("*")) Ss << " "; return Ss.str(); diff --git a/clang/test/3C/itype_typedef.c b/clang/test/3C/itype_typedef.c index f86d7d6b914e..6de35ae7c6bc 100644 --- a/clang/test/3C/itype_typedef.c +++ b/clang/test/3C/itype_typedef.c @@ -34,7 +34,7 @@ void test2(td2 a) { typedef int *td3; td3 test3() { //CHECK: typedef _Ptr td3; -//CHECK: int * test3(void) : itype(td3) { +//CHECK: int *test3(void) : itype(td3) { return (int*) 1; } diff --git a/clang/test/3C/itypes_for_extern.c b/clang/test/3C/itypes_for_extern.c index 46edee5f0649..de6b156afbb6 100644 --- a/clang/test/3C/itypes_for_extern.c +++ b/clang/test/3C/itypes_for_extern.c @@ -30,14 +30,19 @@ int *baz(int *a, int len, int *b) { return a; } +// REVIEW: The problem described in the following comment has been fixed roughly +// in the "first way": the canonical form of the type from mkString no longer +// has the space, so the test will pass as long as the original code didn't have +// the space either. If we still want to pursue the second fix, do we want to +// note that somehow, e.g., by filing an issue? // FIXME: The space between after the first star is needed for the idempotence // test to pass. If there isn't a space there, 3c will add one when // re-run on its original output. This should be fixed ideally in two // ways. First, the space shouldn't be added when not present in the // original source, and second, the second conversion should not rewrite // the declaration. -void buz(int * (*f)(int *, int *)) {} -//CHECK: void buz(int * ((*f)(int *, int *)) : itype(_Ptr<_Ptr (_Ptr, _Ptr)>)) _Checked {} +void buz(int *(*f)(int *, int *)) {} +//CHECK: void buz(int *((*f)(int *, int *)) : itype(_Ptr<_Ptr (_Ptr, _Ptr)>)) _Checked {} typedef int * int_star; void typedef_test(int_star p) {} From 1ce3450229b0994de095db473a3352ba444bdbce Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Wed, 29 Sep 2021 16:47:31 -0400 Subject: [PATCH 3/8] Remove a comment now that John has filed an issue for follow-up. --- clang/test/3C/itypes_for_extern.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/clang/test/3C/itypes_for_extern.c b/clang/test/3C/itypes_for_extern.c index de6b156afbb6..6566cc1e25bb 100644 --- a/clang/test/3C/itypes_for_extern.c +++ b/clang/test/3C/itypes_for_extern.c @@ -30,17 +30,6 @@ int *baz(int *a, int len, int *b) { return a; } -// REVIEW: The problem described in the following comment has been fixed roughly -// in the "first way": the canonical form of the type from mkString no longer -// has the space, so the test will pass as long as the original code didn't have -// the space either. If we still want to pursue the second fix, do we want to -// note that somehow, e.g., by filing an issue? -// FIXME: The space between after the first star is needed for the idempotence -// test to pass. If there isn't a space there, 3c will add one when -// re-run on its original output. This should be fixed ideally in two -// ways. First, the space shouldn't be added when not present in the -// original source, and second, the second conversion should not rewrite -// the declaration. void buz(int *(*f)(int *, int *)) {} //CHECK: void buz(int *((*f)(int *, int *)) : itype(_Ptr<_Ptr (_Ptr, _Ptr)>)) _Checked {} From 59da4ff5b9c6494f3771f3af053112529d253f9d Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Wed, 29 Sep 2021 16:51:28 -0400 Subject: [PATCH 4/8] Update the comment in mkString to reflect that we're planning to accept the changes even though they seem about as ad-hoc as the existing code. https://correct-computation.slack.com/archives/G01GKGKHMFD/p1626879436256100?thread_ts=1626818406.255000&cid=G01GKGKHMFD --- clang/lib/3C/ConstraintVariables.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/clang/lib/3C/ConstraintVariables.cpp b/clang/lib/3C/ConstraintVariables.cpp index 4d7f4f828f17..3cfb147fdb9f 100644 --- a/clang/lib/3C/ConstraintVariables.cpp +++ b/clang/lib/3C/ConstraintVariables.cpp @@ -848,8 +848,11 @@ PointerVariableConstraint::mkString(Constraints &CS, // parameter list. The function pointer code knows to emit the name before // EndStrs. // - // This passes the current regression tests but feels very ad-hoc. - // REVIEW: Help me redesign this code! + // This feels very ad-hoc but currently helps a number of the more complex + // regression tests and makes mkString correct in some additional cases in + // which 3C currently doesn't use it but might use it in the future. We hope + // to eventually overhaul this code + // (https://github.com/correctcomputation/checkedc-clang/issues/703). if (PrevArr && ArrSizes.at(TypeIdx).first != O_SizedArray && !EmittedName) { if (K != Atom::A_Wild || FV != nullptr) { addArrayAnnotations(ConstArrs, EndStrs); From 2c5e8ace3ff55786013a1998174a374d59a1e5e0 Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Fri, 1 Oct 2021 17:13:47 -0400 Subject: [PATCH 5/8] mkString simplifications. As I dug into the reasons why all the conditionals in the PR so far were needed under the current design of the rest of mkString, I realized that the design could be straightened out a bit and correctly handle one more case involving checked pointers to fixed-size arrays. This doesn't address the more difficult problems with wild pointers to fixed-size arrays or with wild pointer levels being in reverse order in general (see https://github.com/correctcomputation/checkedc-clang/issues/703). --- clang/lib/3C/ConstraintVariables.cpp | 119 +++++++++------------------ clang/test/3C/definedType.c | 2 + 2 files changed, 41 insertions(+), 80 deletions(-) diff --git a/clang/lib/3C/ConstraintVariables.cpp b/clang/lib/3C/ConstraintVariables.cpp index 3cfb147fdb9f..482b85fbc479 100644 --- a/clang/lib/3C/ConstraintVariables.cpp +++ b/clang/lib/3C/ConstraintVariables.cpp @@ -750,12 +750,19 @@ PointerVariableConstraint::mkString(Constraints &CS, UNPACK_OPTS(EmitName, ForItype, EmitPointee, UnmaskTypedef, UseName, ForItypeBase); + // This function has become pretty ad-hoc and has a number of known bugs: see + // https://github.com/correctcomputation/checkedc-clang/issues/703. We hope to + // overhaul it in the future. + // The name field encodes if this variable is the return type for a function. // TODO: store this information in a separate field. bool IsReturn = getName() == RETVAR; - if (UseName.empty()) + if (UseName.empty()) { UseName = getName(); + if (UseName.empty()) + EmitName = false; + } if (IsTypedef && !UnmaskTypedef) { std::string QualTypedef = gatherQualStrings() + TypedefString; @@ -782,13 +789,9 @@ PointerVariableConstraint::mkString(Constraints &CS, bool EmittedBase = false; // Have we emitted the name of the variable yet? bool EmittedName = false; - // Was the last variable an Array? - bool PrevArr = false; - // Is the entire type so far an array? - bool AllArrays = true; if (!EmitName || IsReturn) EmittedName = true; - uint32_t TypeIdx = 0; + int TypeIdx = 0; // If we've set a GenericIndex for void, it means we're converting it into // a generic function so give it the default generic type name. @@ -802,7 +805,6 @@ PointerVariableConstraint::mkString(Constraints &CS, } auto It = Vars.begin(); - auto I = 0; // Skip over first pointer level if only emitting pointee string. // This is needed when inserting type arguments. if (EmitPointee) @@ -810,8 +812,8 @@ PointerVariableConstraint::mkString(Constraints &CS, // Iterate through the vars(), but if we have an internal typedef, then stop // once you reach the typedef's level. for (; It != Vars.end() && IMPLIES(TypedefLevelInfo.HasTypedef, - I < TypedefLevelInfo.TypedefLevel); - ++It, I++) { + TypeIdx < TypedefLevelInfo.TypedefLevel); + ++It, TypeIdx++) { const auto &V = *It; ConstAtom *C = nullptr; if (ForItypeBase) { @@ -833,36 +835,18 @@ PointerVariableConstraint::mkString(Constraints &CS, if (!ForItype && InferredGenericIndex == -1 && isVoidPtr()) K = Atom::A_Wild; - // In a case like `_Ptr g[1]`, we need to push the ` g` onto EndStrs - // before we can push the `>` and start drilling into T. - // - // Exception: In a case like `int *g[1]`, we don't want an extra space after - // the `*` but we don't yet know whether we have a `*`, so we skip emitting - // the name. We have to also skip the addArrayAnnotations because it would - // cause the array annotations to be emitted before the name. - // - // Exception to the exception: In a case like `void (*fs[2])()`, we still - // want to avoid emitting the name (which would add an extra space after the - // `*`), but we _do_ need to call addArrayAnnotations so the array - // annotations end up with the variable name rather than after the function - // parameter list. The function pointer code knows to emit the name before - // EndStrs. - // - // This feels very ad-hoc but currently helps a number of the more complex - // regression tests and makes mkString correct in some additional cases in - // which 3C currently doesn't use it but might use it in the future. We hope - // to eventually overhaul this code - // (https://github.com/correctcomputation/checkedc-clang/issues/703). - if (PrevArr && ArrSizes.at(TypeIdx).first != O_SizedArray && !EmittedName) { - if (K != Atom::A_Wild || FV != nullptr) { - addArrayAnnotations(ConstArrs, EndStrs); - } - if (K != Atom::A_Wild) { + // In a case like `_Ptr p[2]`, the ` p[2]` needs to end up _after_ the + // `>`, so we need to push the ` p[2]` onto EndStrs _before_ the code below + // pushes the `>`. In general, before we visit a checked pointer level (not + // a checked array level), we need to transfer any pending array levels and + // emit the name (if applicable). + if (K != Atom::A_Wild && ArrSizes.at(TypeIdx).first != O_SizedArray) { + addArrayAnnotations(ConstArrs, EndStrs); + if (!EmittedName) { EmittedName = true; EndStrs.push_front(" " + UseName); } } - PrevArr = ArrSizes.at(TypeIdx).first == O_SizedArray; switch (K) { case Atom::A_Ptr: @@ -871,7 +855,6 @@ PointerVariableConstraint::mkString(Constraints &CS, // We need to check and see if this level of variable // is constrained by a bounds safe interface. If it is, // then we shouldn't re-write it. - AllArrays = false; EmittedBase = false; Ss << "_Ptr<"; EndStrs.push_front(">"); @@ -885,7 +868,6 @@ PointerVariableConstraint::mkString(Constraints &CS, // we should substitute [K]. if (emitArraySize(ConstArrs, TypeIdx, K)) break; - AllArrays = false; // We need to check and see if this level of variable // is constrained by a bounds safe interface. If it is, // then we shouldn't re-write it. @@ -896,27 +878,24 @@ PointerVariableConstraint::mkString(Constraints &CS, case Atom::A_NTArr: if (emitArraySize(ConstArrs, TypeIdx, K)) break; - AllArrays = false; - // This additional check is to prevent fall-through from the array. - if (K == Atom::A_NTArr) { - // If this is an NTArray. - getQualString(TypeIdx, Ss); + // If this is an NTArray. + getQualString(TypeIdx, Ss); - // We need to check and see if this level of variable - // is constrained by a bounds safe interface. If it is, - // then we shouldn't re-write it. - EmittedBase = false; - Ss << "_Nt_array_ptr<"; - EndStrs.push_front(">"); - break; - } - LLVM_FALLTHROUGH; + // We need to check and see if this level of variable + // is constrained by a bounds safe interface. If it is, + // then we shouldn't re-write it. + EmittedBase = false; + Ss << "_Nt_array_ptr<"; + EndStrs.push_front(">"); + break; // If there is no array in the original program, then we fall through to // the case where we write a pointer value. case Atom::A_Wild: if (emitArraySize(ConstArrs, TypeIdx, K)) break; - AllArrays = false; + // FIXME: This code emits wild pointer levels with the outermost on the + // left. The outermost should be on the right (item 6 of + // https://github.com/correctcomputation/checkedc-clang/issues/703). if (FV != nullptr) { FptrInner << "*"; getQualString(TypeIdx, FptrInner); @@ -936,24 +915,12 @@ PointerVariableConstraint::mkString(Constraints &CS, llvm_unreachable("impossible"); break; } - TypeIdx++; } - // If the previous variable was an array or - // if we are leaving an array run, we need to emit the - // annotation for a stack-array - if (PrevArr && !ConstArrs.empty()) - addArrayAnnotations(ConstArrs, EndStrs); - - // If the whole type is an array so far, and we haven't emitted - // a name yet, then emit the name so that it appears before - // the the stack array type. - if (PrevArr && !EmittedName && AllArrays) { - EmittedName = true; - // Note: If the whole type is an array, we can't have a "*", so we don't - // need to worry about an extra space. - EndStrs.push_front(" " + UseName); - } + // If we have a function pointer, we need to transfer any pending array + // levels to EndStrs before the FV code below reads EndStrs. It doesn't hurt + // to transfer pending array levels here unconditionally. + addArrayAnnotations(ConstArrs, EndStrs); if (!EmittedBase) { // If we have a FV pointer, then our "base" type is a function pointer type. @@ -984,24 +951,16 @@ PointerVariableConstraint::mkString(Constraints &CS, } } - // Add closing elements to type - for (std::string Str : EndStrs) { - Ss << Str; - } - // No space after itype. - if (!EmittedName && !UseName.empty()) { + if (!EmittedName) { if (!StringRef(Ss.str()).endswith("*")) Ss << " "; Ss << UseName; } - // Final array dropping. - if (!ConstArrs.empty()) { - std::deque ArrStrs; - addArrayAnnotations(ConstArrs, ArrStrs); - for (std::string Str : ArrStrs) - Ss << Str; + // Add closing elements to type + for (std::string Str : EndStrs) { + Ss << Str; } if (IsReturn && !ForItype && !StringRef(Ss.str()).endswith("*")) diff --git a/clang/test/3C/definedType.c b/clang/test/3C/definedType.c index 0593f0849e47..344657841754 100644 --- a/clang/test/3C/definedType.c +++ b/clang/test/3C/definedType.c @@ -57,6 +57,8 @@ baz **f[1]; // CHECK_ALL: _Ptr<_Ptr> f _Checked[1] = {((void *)0)}; baz (*g)[1]; // CHECK_ALL: _Ptr g = ((void *)0); +baz *(*g2)[1]; +// CHECK_ALL: _Ptr<_Ptr _Checked[1]> g2 = ((void *)0); baz h[1][1]; // CHECK_ALL: baz h _Checked[1] _Checked[1]; From 34b4a549219e1fd40b758b2640c1161c04c9c58f Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Sat, 2 Oct 2021 09:50:26 -0400 Subject: [PATCH 6/8] Fix checked pointers to arrays of wild function pointers. Any array levels still pending when we reach a function pointer should become part of the function pointer declarator even if some text for an outer checked pointer level has already been added to Ss. However, any array levels that are outside the checked pointer level and have already been committed to EndStrs should remain at the end of the entire type and not get sucked into the function pointer declarator. To maintain this distinction, we need to use a separate `FptrEndStrs` list for the array levels in the function pointer declarator. --- clang/lib/3C/ConstraintVariables.cpp | 21 ++++++++------------- clang/test/3C/fptr_array.c | 10 ++++++++++ 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/clang/lib/3C/ConstraintVariables.cpp b/clang/lib/3C/ConstraintVariables.cpp index 482b85fbc479..39fcd3e746c7 100644 --- a/clang/lib/3C/ConstraintVariables.cpp +++ b/clang/lib/3C/ConstraintVariables.cpp @@ -917,23 +917,17 @@ PointerVariableConstraint::mkString(Constraints &CS, } } - // If we have a function pointer, we need to transfer any pending array - // levels to EndStrs before the FV code below reads EndStrs. It doesn't hurt - // to transfer pending array levels here unconditionally. - addArrayAnnotations(ConstArrs, EndStrs); - if (!EmittedBase) { // If we have a FV pointer, then our "base" type is a function pointer type. if (FV) { - if (Ss.str().empty()) { - if (!EmittedName) { - FptrInner << UseName; - EmittedName = true; - } - for (std::string Str : EndStrs) - FptrInner << Str; - EndStrs.clear(); + if (!EmittedName) { + FptrInner << UseName; + EmittedName = true; } + std::deque FptrEndStrs; + addArrayAnnotations(ConstArrs, FptrEndStrs); + for (std::string Str : FptrEndStrs) + FptrInner << Str; bool EmitFVName = !FptrInner.str().empty(); if (EmitFVName) Ss << FV->mkString(CS, MKSTRING_OPTS(UseName = FptrInner.str(), @@ -959,6 +953,7 @@ PointerVariableConstraint::mkString(Constraints &CS, } // Add closing elements to type + addArrayAnnotations(ConstArrs, EndStrs); for (std::string Str : EndStrs) { Ss << Str; } diff --git a/clang/test/3C/fptr_array.c b/clang/test/3C/fptr_array.c index a82e1db710e9..8d12acc69153 100644 --- a/clang/test/3C/fptr_array.c +++ b/clang/test/3C/fptr_array.c @@ -53,6 +53,9 @@ void (* const l)(int *); //CHECK_NOALL: void (*const l)(_Ptr) = ((void *)0); //CHECK_ALL: const _Ptr)> l = ((void *)0); +#define UNWRITABLE_MS_DECL void (*ms[2])(int *) +UNWRITABLE_MS_DECL; + void test(void){ fs[1] = f; gs[1] = g; @@ -63,4 +66,11 @@ void test(void){ void (* const ls[1])(int *) = {l}; //CHECK_NOALL: void (*const ls[1])(_Ptr) = {l}; //CHECK_ALL: const _Ptr)> ls _Checked[1] = {l}; + + void (*(*pms)[2])(int *) = &ms; + //CHECK: _Ptr pms = &ms; + + void (*(*apms[1])[2])(int *) = {&ms}; + //CHECK_NOALL: void (*(*apms[1])[2])(int *) = {&ms}; + //CHECK_ALL: _Ptr apms _Checked[1] = {&ms}; } From 4a04607544b5999498a41224df8096f403444e53 Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Sat, 2 Oct 2021 10:23:16 -0400 Subject: [PATCH 7/8] Address John's review comments. Also update a link to go to an existing issue John pointed out. --- clang/lib/3C/ConstraintVariables.cpp | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/clang/lib/3C/ConstraintVariables.cpp b/clang/lib/3C/ConstraintVariables.cpp index 39fcd3e746c7..011a438ead5b 100644 --- a/clang/lib/3C/ConstraintVariables.cpp +++ b/clang/lib/3C/ConstraintVariables.cpp @@ -852,9 +852,6 @@ PointerVariableConstraint::mkString(Constraints &CS, case Atom::A_Ptr: getQualString(TypeIdx, Ss); - // We need to check and see if this level of variable - // is constrained by a bounds safe interface. If it is, - // then we shouldn't re-write it. EmittedBase = false; Ss << "_Ptr<"; EndStrs.push_front(">"); @@ -868,22 +865,16 @@ PointerVariableConstraint::mkString(Constraints &CS, // we should substitute [K]. if (emitArraySize(ConstArrs, TypeIdx, K)) break; - // We need to check and see if this level of variable - // is constrained by a bounds safe interface. If it is, - // then we shouldn't re-write it. EmittedBase = false; Ss << "_Array_ptr<"; EndStrs.push_front(">"); break; case Atom::A_NTArr: - if (emitArraySize(ConstArrs, TypeIdx, K)) - break; // If this is an NTArray. getQualString(TypeIdx, Ss); + if (emitArraySize(ConstArrs, TypeIdx, K)) + break; - // We need to check and see if this level of variable - // is constrained by a bounds safe interface. If it is, - // then we shouldn't re-write it. EmittedBase = false; Ss << "_Nt_array_ptr<"; EndStrs.push_front(">"); @@ -894,8 +885,8 @@ PointerVariableConstraint::mkString(Constraints &CS, if (emitArraySize(ConstArrs, TypeIdx, K)) break; // FIXME: This code emits wild pointer levels with the outermost on the - // left. The outermost should be on the right (item 6 of - // https://github.com/correctcomputation/checkedc-clang/issues/703). + // left. The outermost should be on the right + // (https://github.com/correctcomputation/checkedc-clang/issues/161). if (FV != nullptr) { FptrInner << "*"; getQualString(TypeIdx, FptrInner); From b8c3729f6b8be2a1d1c5c976f348d01e9853eefa Mon Sep 17 00:00:00 2001 From: "Matt McCutchen (Correct Computation)" Date: Tue, 5 Oct 2021 16:41:26 -0400 Subject: [PATCH 8/8] Improve the ConstraintVariable::mkString comment a bit. --- clang/include/clang/3C/ConstraintVariables.h | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/clang/include/clang/3C/ConstraintVariables.h b/clang/include/clang/3C/ConstraintVariables.h index 470363066b11..65b0974decb7 100644 --- a/clang/include/clang/3C/ConstraintVariables.h +++ b/clang/include/clang/3C/ConstraintVariables.h @@ -42,8 +42,15 @@ typedef std::set CVars; // Holds Atoms, one for each of the pointer (*) declared in the program. typedef std::vector CAtoms; +// Options for ConstraintVariable::mkString, using the code pattern described in +// clang/include/clang/3C/OptionalParams.h. Use the MKSTRING_OPTS macro to +// generate a MkStringOpts instance at a call site. struct MkStringOpts { + // True when the generated string should include + // the name of the variable, false for just the type. bool EmitName = true; + // True when the generated string is expected + // to be used inside an itype. bool ForItype = false; bool EmitPointee = false; bool UnmaskTypedef = false; @@ -113,11 +120,12 @@ class ConstraintVariable { qtyToStr(QT, N == RETVAR ? "" : N)) {} public: - // Create a "for-rewriting" representation of this ConstraintVariable. - // The 'emitName' parameter is true when the generated string should include - // the name of the variable, false for just the type. - // The 'forIType' parameter is true when the generated string is expected - // to be used inside an itype. + // Generate source code for the type and (in certain cases) the name of the + // variable or function represented by this ConstraintVariable that reflects + // any changes made by 3C and is suitable to insert during rewriting. + // + // This method is used in several contexts with special requirements, which + // are addressed by the options in MkStringOpts; see the comments there. virtual std::string mkString(Constraints &CS, const MkStringOpts &Opts = {}) const = 0;