llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Anand.k.vinu (anandkvinu) <details> <summary>Changes</summary> format using git clang-format While looking into issue 223064 I confirmed the repro and traced the problem to the reconstruction of SubobjectDesignator from an APValue. When a constant lvalue is stored, for example when a local variable keeps its value between statements in a constexpr function, Clang later rebuilds the SubobjectDesignator from the flattened APValue path using findMostDerivedSubobject. This function walks through each path entry. For array, complex and field entries it updates MostDerivedPathLength, MostDerivedArraySize and MostDerivedIsArrayElement together because these entries represent a new most derived object. Base class entries are different because a base class cast does not change the most derived object. The existing code correctly left MostDerivedPathLength unchanged for base class entries, but it still reset MostDerivedArraySize and MostDerivedIsArrayElement to zero and false. As a result, if a base class entry appeared after the actual most derived entry in the path, the array information was lost. This can be seen in the original repro where the path for p contains an array element followed by a base class entry. During reconstruction the most derived path length remains correct, but the array size is reset. When the later downcast performs bounds checked pointer arithmetic, Clang therefore thinks that the pointer is not part of an array and incorrectly reports an out of bounds access. The fix is to remove the else branch that resets ArraySize and IsArray for base class entries. Base class entries now leave these values unchanged, just like MostDerivedPathLength. This keeps the array information from the last path entry that actually changed the most derived object. I also added regression tests to constant expression cxx1z.cpp covering the original case, a non primary base class with a non zero offset, multiple consecutive base class entries, pointer arithmetic performed directly on the downcast pointer and a negative out of bounds case. The negative test also confirms that the existing bounds checking is still working correctly. With the fix Clang gives the more specific diagnostic saying that element 5 is being accessed in an array of 2 elements instead of falling back to the generic past the end of object diagnostic. I checked each test against a pre fix build to make sure they actually failed before the change. I then confirmed that all the tests pass with the fix. I also ran the full SemaCXX test suite. There were 1429 passing tests out of 1430. The remaining failure is an existing unrelated XFAIL. Fixes 223064 --- Full diff: https://github.com/llvm/llvm-project/pull/223184.diff 2 Files Affected: - (modified) clang/lib/AST/ExprConstant.cpp (-4) - (modified) clang/test/SemaCXX/constant-expression-cxx1z.cpp (+83) ``````````diff diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp index 1286c77ad1c69..00568d1d029e3 100644 --- a/clang/lib/AST/ExprConstant.cpp +++ b/clang/lib/AST/ExprConstant.cpp @@ -235,10 +235,6 @@ namespace { ArraySize = 0; MostDerivedLength = I + 1; IsArray = false; - } else { - // Path[I] describes a base class. - ArraySize = 0; - IsArray = false; } } return MostDerivedLength; diff --git a/clang/test/SemaCXX/constant-expression-cxx1z.cpp b/clang/test/SemaCXX/constant-expression-cxx1z.cpp index c0766f70cf881..f985065009a19 100644 --- a/clang/test/SemaCXX/constant-expression-cxx1z.cpp +++ b/clang/test/SemaCXX/constant-expression-cxx1z.cpp @@ -178,6 +178,89 @@ namespace LambdaCallOp { } } +// Regression tests for https://github.com/llvm/llvm-project/issues/223064 +// +// The constant evaluator loses track of an lvalue's most-derived array +// information when an APValue containing a base-class path entry is +// reconstructed into an LValue/SubobjectDesignator. A base-class path +// entry does not advance the most-derived path length, so it must not +// clobber the most-derived array size/flag either; those fields belong +// to the entry at MostDerivedPathLength, not to whatever entry was +// processed last. +namespace GH223064 { + +// Original reproducer: + +namespace BasicBaseSubobject { + struct A { int n; }; + struct B : A {} b[2]; + + constexpr int *f() { + A *p = b; + return &static_cast<B*>(p)[1].n; + } + static_assert(f() == &b[1].n, ""); +} + +// Same shape, but through the second of two non-virtual bases, so the +// base subobject sits at a non-zero offset within the derived object. +namespace NonPrimaryBase { + struct A { int n; }; + struct A2 { int m; }; + struct B : A, A2 {} b[2]; + + constexpr int *f() { + A2 *p = &b[0]; + return &static_cast<B*>(p)[1].m; + } + static_assert(f() == &b[1].m, ""); +} + +// Two trailing base-class path entries in a row (C -> B -> A), rather than +// just one +namespace MultipleTrailingBaseEntries { + struct A { int n; }; + struct B : A {}; + struct C : B {} c[2]; + + constexpr int *f() { + A *p = &c[0]; + return &static_cast<C*>(p)[1].n; + } + static_assert(f() == &c[1].n, ""); +} + +// Pointer arithmetic performed directly on the downcast pointer, rather +// than array subscripting, must also see the correct bounds. +namespace ArithmeticOnDowncastPointer { + struct A { int n; }; + struct B : A {} b[3]; + + constexpr int *f() { + A *p = b; + B *q = static_cast<B*>(p) + 2; + return &q->n; + } + static_assert(f() == &b[2].n, ""); +} + +// indexing +// past the end of the array through the same base-subobject pattern +// should still be rejected. +namespace OutOfBoundsStillRejected { + struct A { int n; }; + struct B : A {} b[2]; + + constexpr int *f(int i) { + A *p = b; + return &static_cast<B*>(p)[i].n; // expected-note {{cannot refer to element 5 of array of 2 elements in a constant expression}} + } + static_assert(f(5) == nullptr, ""); // expected-error {{not an integral constant expression}} \ + // expected-note {{in call to 'f(5)'}} +} + +} // namespace GH223064 + // This used to crash due to an assertion failure, // see gh#67690 namespace { `````````` </details> https://github.com/llvm/llvm-project/pull/223184 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
