[flang-commits] [flang] 9696355 - [flang] Better messages and error recovery for a bad RESHAPE (#122604)
via flang-commits
flang-commits at lists.llvm.org
Tue Jan 14 12:57:52 PST 2025
Author: Peter Klausler
Date: 2025-01-14T12:57:49-08:00
New Revision: 9696355484152eda5684e0ec6249f4c423f08e42
URL: https://github.com/llvm/llvm-project/commit/9696355484152eda5684e0ec6249f4c423f08e42
DIFF: https://github.com/llvm/llvm-project/commit/9696355484152eda5684e0ec6249f4c423f08e42.diff
LOG: [flang] Better messages and error recovery for a bad RESHAPE (#122604)
Add tests for negative array extents where necessary, motivated by a
compiler crash exposed by yet another fuzzer test, and improve overall
error message quality for RESHAPE().
Fixes https://github.com/llvm/llvm-project/issues/122060.
Added:
flang/test/Semantics/bug122060.f90
Modified:
flang/include/flang/Evaluate/shape.h
flang/include/flang/Evaluate/tools.h
flang/lib/Evaluate/check-expression.cpp
flang/lib/Evaluate/fold-designator.cpp
flang/lib/Evaluate/fold-implementation.h
flang/lib/Semantics/tools.cpp
flang/test/Semantics/reshape.f90
Removed:
################################################################################
diff --git a/flang/include/flang/Evaluate/shape.h b/flang/include/flang/Evaluate/shape.h
index a498444b0dd2b1..3e42ec691158bc 100644
--- a/flang/include/flang/Evaluate/shape.h
+++ b/flang/include/flang/Evaluate/shape.h
@@ -43,6 +43,8 @@ std::optional<Constant<ExtentType>> AsConstantShape(
FoldingContext &, const Shape &);
Constant<ExtentType> AsConstantShape(const ConstantSubscripts &);
+// AsConstantExtents returns a constant shape. It may contain
+// invalid negative extents; use HasNegativeExtent() to check.
ConstantSubscripts AsConstantExtents(const Constant<ExtentType> &);
std::optional<ConstantSubscripts> AsConstantExtents(
FoldingContext &, const Shape &);
@@ -53,6 +55,7 @@ inline std::optional<ConstantSubscripts> AsConstantExtents(
}
return std::nullopt;
}
+
Shape AsShape(const ConstantSubscripts &);
std::optional<Shape> AsShape(const std::optional<ConstantSubscripts> &);
@@ -287,14 +290,18 @@ std::optional<Constant<ExtentType>> GetConstantShape(
}
}
+// Combines GetShape and AsConstantExtents; only returns valid shapes.
template <typename A>
std::optional<ConstantSubscripts> GetConstantExtents(
FoldingContext &context, const A &x) {
if (auto shape{GetShape(context, x, /*invariantOnly=*/true)}) {
- return AsConstantExtents(context, *shape);
- } else {
- return std::nullopt;
+ if (auto extents{AsConstantExtents(context, *shape)}) {
+ if (!HasNegativeExtent(*extents)) {
+ return extents;
+ }
+ }
}
+ return std::nullopt;
}
// Get shape that does not depends on callee scope symbols if the expression
diff --git a/flang/include/flang/Evaluate/tools.h b/flang/include/flang/Evaluate/tools.h
index ec5fc7ab014856..669efb41b03442 100644
--- a/flang/include/flang/Evaluate/tools.h
+++ b/flang/include/flang/Evaluate/tools.h
@@ -1154,7 +1154,7 @@ bool IsExpandableScalar(const Expr<T> &expr, FoldingContext &context,
const Shape &shape, bool admitPureCall = false) {
if (UnexpandabilityFindingVisitor{admitPureCall}(expr)) {
auto extents{AsConstantExtents(context, shape)};
- return extents && GetSize(*extents) == 1;
+ return extents && !HasNegativeExtent(*extents) && GetSize(*extents) == 1;
} else {
return true;
}
diff --git a/flang/lib/Evaluate/check-expression.cpp b/flang/lib/Evaluate/check-expression.cpp
index 998378c44cdf41..726a0ab35ede4b 100644
--- a/flang/lib/Evaluate/check-expression.cpp
+++ b/flang/lib/Evaluate/check-expression.cpp
@@ -432,7 +432,8 @@ std::optional<Expr<SomeType>> NonPointerInitializationExpr(const Symbol &symbol,
"Implied-shape parameter '%s' has rank %d but its initializer has rank %d"_err_en_US,
symbol.name(), symRank, folded.Rank());
}
- } else if (auto extents{AsConstantExtents(context, symTS->shape())}) {
+ } else if (auto extents{AsConstantExtents(context, symTS->shape())};
+ extents && !HasNegativeExtent(*extents)) {
if (folded.Rank() == 0 && symRank == 0) {
// symbol and constant are both scalars
return {std::move(folded)};
diff --git a/flang/lib/Evaluate/fold-designator.cpp b/flang/lib/Evaluate/fold-designator.cpp
index 0d8c22fb297708..d7751ec3899173 100644
--- a/flang/lib/Evaluate/fold-designator.cpp
+++ b/flang/lib/Evaluate/fold-designator.cpp
@@ -220,7 +220,8 @@ static std::optional<ArrayRef> OffsetToArrayRef(FoldingContext &context,
Shape lbs{GetRawLowerBounds(context, entity)};
auto lower{AsConstantExtents(context, lbs)};
auto elementBytes{ToInt64(elementType.MeasureSizeInBytes(context, true))};
- if (!extents || !lower || !elementBytes || *elementBytes <= 0) {
+ if (!extents || HasNegativeExtent(*extents) || !lower || !elementBytes ||
+ *elementBytes <= 0) {
return std::nullopt;
}
int rank{GetRank(shape)};
diff --git a/flang/lib/Evaluate/fold-implementation.h b/flang/lib/Evaluate/fold-implementation.h
index 208188a4ffcc77..31d043f490fd85 100644
--- a/flang/lib/Evaluate/fold-implementation.h
+++ b/flang/lib/Evaluate/fold-implementation.h
@@ -899,51 +899,66 @@ template <typename T> Expr<T> Folder<T>::RESHAPE(FunctionRef<T> &&funcRef) {
std::optional<std::vector<ConstantSubscript>> shape{
GetIntegerVector<ConstantSubscript>(args[1])};
std::optional<std::vector<int>> order{GetIntegerVector<int>(args[3])};
- if (!source || !shape || (args[2] && !pad) || (args[3] && !order)) {
- return Expr<T>{std::move(funcRef)}; // Non-constant arguments
- } else if (shape.value().size() > common::maxRank) {
- context_.messages().Say(
- "Size of 'shape=' argument must not be greater than %d"_err_en_US,
- common::maxRank);
- } else if (HasNegativeExtent(shape.value())) {
- context_.messages().Say(
- "'shape=' argument must not have a negative extent"_err_en_US);
- } else {
- std::optional<uint64_t> optResultElement{TotalElementCount(shape.value())};
- if (!optResultElement) {
+ std::optional<uint64_t> optResultElement;
+ std::optional<std::vector<int>> dimOrder;
+ bool ok{true};
+ if (shape) {
+ if (shape->size() > common::maxRank) {
+ context_.messages().Say(
+ "Size of 'shape=' argument (%zd) must not be greater than %d"_err_en_US,
+ shape->size(), common::maxRank);
+ ok = false;
+ } else if (HasNegativeExtent(*shape)) {
context_.messages().Say(
- "'shape=' argument has too many elements"_err_en_US);
+ "'shape=' argument (%s) must not have a negative extent"_err_en_US,
+ DEREF(args[1]->UnwrapExpr()).AsFortran());
+ ok = false;
} else {
- int rank{GetRank(shape.value())};
- uint64_t resultElements{*optResultElement};
- std::optional<std::vector<int>> dimOrder;
- if (order) {
- dimOrder = ValidateDimensionOrder(rank, *order);
- }
- std::vector<int> *dimOrderPtr{dimOrder ? &dimOrder.value() : nullptr};
- if (order && !dimOrder) {
+ optResultElement = TotalElementCount(*shape);
+ if (!optResultElement) {
context_.messages().Say(
- "Invalid 'order=' argument in RESHAPE"_err_en_US);
- } else if (resultElements > source->size() && (!pad || pad->empty())) {
+ "'shape=' argument (%s) specifies an array with too many elements"_err_en_US,
+ DEREF(args[1]->UnwrapExpr()).AsFortran());
+ ok = false;
+ }
+ }
+ if (order) {
+ dimOrder = ValidateDimensionOrder(GetRank(*shape), *order);
+ if (!dimOrder) {
context_.messages().Say(
- "Too few elements in 'source=' argument and 'pad=' "
- "argument is not present or has null size"_err_en_US);
- } else {
- Constant<T> result{!source->empty() || !pad
- ? source->Reshape(std::move(shape.value()))
- : pad->Reshape(std::move(shape.value()))};
- ConstantSubscripts subscripts{result.lbounds()};
- auto copied{result.CopyFrom(*source,
- std::min(static_cast<uint64_t>(source->size()), resultElements),
- subscripts, dimOrderPtr)};
- if (copied < resultElements) {
- CHECK(pad);
- copied += result.CopyFrom(
- *pad, resultElements - copied, subscripts, dimOrderPtr);
- }
- CHECK(copied == resultElements);
- return Expr<T>{std::move(result)};
+ "Invalid 'order=' argument (%s) in RESHAPE"_err_en_US,
+ DEREF(args[3]->UnwrapExpr()).AsFortran());
+ ok = false;
+ }
+ }
+ }
+ if (!ok) {
+ // convert into an invalid intrinsic procedure call below
+ } else if (!source || !shape || (args[2] && !pad) || (args[3] && !order)) {
+ return Expr<T>{std::move(funcRef)}; // Non-constant arguments
+ } else {
+ uint64_t resultElements{*optResultElement};
+ std::vector<int> *dimOrderPtr{dimOrder ? &dimOrder.value() : nullptr};
+ if (resultElements > source->size() && (!pad || pad->empty())) {
+ context_.messages().Say(
+ "Too few elements in 'source=' argument and 'pad=' "
+ "argument is not present or has null size"_err_en_US);
+ ok = false;
+ } else {
+ Constant<T> result{!source->empty() || !pad
+ ? source->Reshape(std::move(shape.value()))
+ : pad->Reshape(std::move(shape.value()))};
+ ConstantSubscripts subscripts{result.lbounds()};
+ auto copied{result.CopyFrom(*source,
+ std::min(static_cast<uint64_t>(source->size()), resultElements),
+ subscripts, dimOrderPtr)};
+ if (copied < resultElements) {
+ CHECK(pad);
+ copied += result.CopyFrom(
+ *pad, resultElements - copied, subscripts, dimOrderPtr);
}
+ CHECK(copied == resultElements);
+ return Expr<T>{std::move(result)};
}
}
// Invalid, prevent re-folding
@@ -1398,7 +1413,8 @@ AsFlatArrayConstructor(const Expr<SomeKind<CAT>> &expr) {
template <typename T>
std::optional<Expr<T>> FromArrayConstructor(
FoldingContext &context, ArrayConstructor<T> &&values, const Shape &shape) {
- if (auto constShape{AsConstantExtents(context, shape)}) {
+ if (auto constShape{AsConstantExtents(context, shape)};
+ constShape && !HasNegativeExtent(*constShape)) {
Expr<T> result{Fold(context, Expr<T>{std::move(values)})};
if (auto *constant{UnwrapConstantValue<T>(result)}) {
// Elements and shape are both constant.
diff --git a/flang/lib/Semantics/tools.cpp b/flang/lib/Semantics/tools.cpp
index 9e180605c1b3bd..9091c6b49c20e1 100644
--- a/flang/lib/Semantics/tools.cpp
+++ b/flang/lib/Semantics/tools.cpp
@@ -1584,7 +1584,8 @@ const std::optional<parser::Name> &MaybeGetNodeName(
std::optional<ArraySpec> ToArraySpec(
evaluate::FoldingContext &context, const evaluate::Shape &shape) {
- if (auto extents{evaluate::AsConstantExtents(context, shape)}) {
+ if (auto extents{evaluate::AsConstantExtents(context, shape)};
+ extents && !evaluate::HasNegativeExtent(*extents)) {
ArraySpec result;
for (const auto &extent : *extents) {
result.emplace_back(ShapeSpec::MakeExplicit(Bound{extent}));
diff --git a/flang/test/Semantics/bug122060.f90 b/flang/test/Semantics/bug122060.f90
new file mode 100644
index 00000000000000..ef0fbcf3acb1e9
--- /dev/null
+++ b/flang/test/Semantics/bug122060.f90
@@ -0,0 +1,6 @@
+! RUN: %python %S/test_errors.py %s %flang_fc1
+integer, parameter :: a = -10
+! ERROR: Assignment to constant 'a' is not allowed
+! ERROR: 'shape=' argument ([INTEGER(4)::-10_4]) must not have a negative extent
+a = b() - reshape([c], [a])
+END
diff --git a/flang/test/Semantics/reshape.f90 b/flang/test/Semantics/reshape.f90
index cccba210885a69..3f3b28a43569f9 100644
--- a/flang/test/Semantics/reshape.f90
+++ b/flang/test/Semantics/reshape.f90
@@ -20,13 +20,13 @@ program reshaper
integer :: array8(2,3) = RESHAPE([(n, n=1,4)], [2,3], [99_8])
!ERROR: Actual argument for 'pad=' has bad type or kind 'REAL(4)'
real :: array9(2,3) = RESHAPE([(n, n=1,4)], [2,3], [99.9])
- !ERROR: Invalid 'order=' argument in RESHAPE
+ !ERROR: Invalid 'order=' argument ([INTEGER(4)::2_4,3_4]) in RESHAPE
real :: array10(2,3) = RESHAPE([(n,n=1,4)],[2,3],[99],[2,3])
!ERROR: Actual argument for 'order=' has bad type 'REAL(4)'
real :: array11(2,3) = RESHAPE([(n, n=1,4)], [2,3], [99], [2.2,3.3])
- !ERROR: Invalid 'order=' argument in RESHAPE
+ !ERROR: Invalid 'order=' argument ([INTEGER(4)::1_4]) in RESHAPE
real :: array12(2,3) = RESHAPE([(n, n=1,4)], [2,3], [99], [1])
- !ERROR: Invalid 'order=' argument in RESHAPE
+ !ERROR: Invalid 'order=' argument ([INTEGER(4)::1_4,1_4]) in RESHAPE
real :: array13(2,3) = RESHAPE([(n, n = 1, 4)], [2, 3], [99], [1, 1])
! Examples that have caused problems
@@ -50,13 +50,13 @@ program reshaper
integer, parameter :: array22(2) = RESHAPE(array21, [2])
integer(8), parameter :: huge_shape(2) = [I64_MAX, I64_MAX]
- !ERROR: 'shape=' argument has too many elements
+ !ERROR: 'shape=' argument ([INTEGER(8)::9223372036854775807_8,9223372036854775807_8]) specifies an array with too many elements
integer :: array23(I64_MAX, I64_MAX) = RESHAPE([1, 2, 3], huge_shape)
CALL ext_sub(RESHAPE([(n, n=1,20)], &
!ERROR: 'shape=' argument must be a vector of at most 15 elements (has 16)
[1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]))
- !ERROR: 'shape=' argument must not have a negative extent
+ !ERROR: 'shape=' argument ([INTEGER(4)::1_4,-5_4,3_4]) must not have a negative extent
CALL ext_sub(RESHAPE([(n, n=1,20)], [1, -5, 3]))
!ERROR: 'order=' argument has unacceptable rank 2
array20 = RESHAPE([(n, n = 1, 4)], [2, 3], order = bad_order)
More information about the flang-commits
mailing list