[flang] Better messages and error recovery for a bad RESHAPE #122604
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-semantics Author: Peter Klausler (klausler) ChangesAdd 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 #122060. Full diff: https://github.com/llvm/llvm-project/pull/122604.diff 8 Files Affected:
diff --git a/flang/include/flang/Evaluate/shape.h b/flang/include/flang/Evaluate/shape.h
index e679a001235490..dd9119e1461f25 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> &);
@@ -283,14 +286,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 c82995c38f79f7..310425cefb8d87 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 b3b96985affc7a..d1a72070a3fb9d 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,14 +50,14 @@ 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)
- !ERROR: Size of 'shape=' argument must not be greater than 15
+ !ERROR: Size of 'shape=' argument (16) must not be greater than 15
CALL ext_sub(RESHAPE([(n, n=1,20)], &
[1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]))
!ERROR: Reference to the procedure 'ext_sub' has an implicit interface that is distinct from another reference: incompatible dummy argument #1: incompatible dummy data object shapes
- !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)
|
Huh, this is the first time I see that a fuzzer was used to improve the user experience! :-) |
| } | ||
| } | ||
| if (!ok) { | ||
| // convert into an invalid intrinsic procedure call below |
There was a problem hiding this comment.
I don't understand this comment. Also, ok seems to be set, but not used below.
There was a problem hiding this comment.
An erroneous reference to an intrinsic procedure that produces error messages is replaced with an artificial call to an intrinsic whose name is a special invalid identifier by MakeInvalidIntrinsic(). If that replacement call gets run through folding or validation code again later, it won't elicit further error messages.
I set ok to false at every point where I've produced an error message. For consistency, this included one point where the variable is actually dead.
eugeneepshteyn
approved these changes
Jan 13, 2025
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 llvm#122060.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 #122060.