Skip to content

Commit 9696355

Browse files
klauslerklausler
authored
[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 #122060.
1 parent 89bbaf3 commit 9696355

File tree

8 files changed

+85
-53
lines changed

8 files changed

+85
-53
lines changed

‎flang/include/flang/Evaluate/shape.h‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@ std::optional<Constant<ExtentType>> AsConstantShape(
4343
FoldingContext &, const Shape &);
4444
Constant<ExtentType> AsConstantShape(const ConstantSubscripts &);
4545

46+
// AsConstantExtents returns a constant shape. It may contain
47+
// invalid negative extents; use HasNegativeExtent() to check.
4648
ConstantSubscripts AsConstantExtents(const Constant<ExtentType> &);
4749
std::optional<ConstantSubscripts> AsConstantExtents(
4850
FoldingContext &, const Shape &);
@@ -53,6 +55,7 @@ inline std::optional<ConstantSubscripts> AsConstantExtents(
5355
}
5456
return std::nullopt;
5557
}
58+
5659
Shape AsShape(const ConstantSubscripts &);
5760
std::optional<Shape> AsShape(const std::optional<ConstantSubscripts> &);
5861

@@ -287,14 +290,18 @@ std::optional<Constant<ExtentType>> GetConstantShape(
287290
}
288291
}
289292

293+
// Combines GetShape and AsConstantExtents; only returns valid shapes.
290294
template <typename A>
291295
std::optional<ConstantSubscripts> GetConstantExtents(
292296
FoldingContext &context, const A &x) {
293297
if (auto shape{GetShape(context, x, /*invariantOnly=*/true)}) {
294-
return AsConstantExtents(context, *shape);
295-
} else {
296-
return std::nullopt;
298+
if (auto extents{AsConstantExtents(context, *shape)}) {
299+
if (!HasNegativeExtent(*extents)) {
300+
return extents;
301+
}
302+
}
297303
}
304+
return std::nullopt;
298305
}
299306

300307
// Get shape that does not depends on callee scope symbols if the expression

‎flang/include/flang/Evaluate/tools.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1154,7 +1154,7 @@ bool IsExpandableScalar(const Expr<T> &expr, FoldingContext &context,
11541154
const Shape &shape, bool admitPureCall = false) {
11551155
if (UnexpandabilityFindingVisitor{admitPureCall}(expr)) {
11561156
auto extents{AsConstantExtents(context, shape)};
1157-
return extents && GetSize(*extents) == 1;
1157+
return extents && !HasNegativeExtent(*extents) && GetSize(*extents) == 1;
11581158
} else {
11591159
return true;
11601160
}

‎flang/lib/Evaluate/check-expression.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -432,7 +432,8 @@ std::optional<Expr<SomeType>> NonPointerInitializationExpr(const Symbol &symbol,
432432
"Implied-shape parameter '%s' has rank %d but its initializer has rank %d"_err_en_US,
433433
symbol.name(), symRank, folded.Rank());
434434
}
435-
} else if (auto extents{AsConstantExtents(context, symTS->shape())}) {
435+
} else if (auto extents{AsConstantExtents(context, symTS->shape())};
436+
extents && !HasNegativeExtent(*extents)) {
436437
if (folded.Rank() == 0 && symRank == 0) {
437438
// symbol and constant are both scalars
438439
return {std::move(folded)};

‎flang/lib/Evaluate/fold-designator.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,8 @@ static std::optional<ArrayRef> OffsetToArrayRef(FoldingContext &context,
220220
Shape lbs{GetRawLowerBounds(context, entity)};
221221
auto lower{AsConstantExtents(context, lbs)};
222222
auto elementBytes{ToInt64(elementType.MeasureSizeInBytes(context, true))};
223-
if (!extents || !lower || !elementBytes || *elementBytes <= 0) {
223+
if (!extents || HasNegativeExtent(*extents) || !lower || !elementBytes ||
224+
*elementBytes <= 0) {
224225
return std::nullopt;
225226
}
226227
int rank{GetRank(shape)};

‎flang/lib/Evaluate/fold-implementation.h‎

Lines changed: 57 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -899,51 +899,66 @@ template <typename T> Expr<T> Folder<T>::RESHAPE(FunctionRef<T> &&funcRef) {
899899
std::optional<std::vector<ConstantSubscript>> shape{
900900
GetIntegerVector<ConstantSubscript>(args[1])};
901901
std::optional<std::vector<int>> order{GetIntegerVector<int>(args[3])};
902-
if (!source || !shape || (args[2] && !pad) || (args[3] && !order)) {
903-
return Expr<T>{std::move(funcRef)}; // Non-constant arguments
904-
} else if (shape.value().size() > common::maxRank) {
905-
context_.messages().Say(
906-
"Size of 'shape=' argument must not be greater than %d"_err_en_US,
907-
common::maxRank);
908-
} else if (HasNegativeExtent(shape.value())) {
909-
context_.messages().Say(
910-
"'shape=' argument must not have a negative extent"_err_en_US);
911-
} else {
912-
std::optional<uint64_t> optResultElement{TotalElementCount(shape.value())};
913-
if (!optResultElement) {
902+
std::optional<uint64_t> optResultElement;
903+
std::optional<std::vector<int>> dimOrder;
904+
bool ok{true};
905+
if (shape) {
906+
if (shape->size() > common::maxRank) {
907+
context_.messages().Say(
908+
"Size of 'shape=' argument (%zd) must not be greater than %d"_err_en_US,
909+
shape->size(), common::maxRank);
910+
ok = false;
911+
} else if (HasNegativeExtent(*shape)) {
914912
context_.messages().Say(
915-
"'shape=' argument has too many elements"_err_en_US);
913+
"'shape=' argument (%s) must not have a negative extent"_err_en_US,
914+
DEREF(args[1]->UnwrapExpr()).AsFortran());
915+
ok = false;
916916
} else {
917-
int rank{GetRank(shape.value())};
918-
uint64_t resultElements{*optResultElement};
919-
std::optional<std::vector<int>> dimOrder;
920-
if (order) {
921-
dimOrder = ValidateDimensionOrder(rank, *order);
922-
}
923-
std::vector<int> *dimOrderPtr{dimOrder ? &dimOrder.value() : nullptr};
924-
if (order && !dimOrder) {
917+
optResultElement = TotalElementCount(*shape);
918+
if (!optResultElement) {
925919
context_.messages().Say(
926-
"Invalid 'order=' argument in RESHAPE"_err_en_US);
927-
} else if (resultElements > source->size() && (!pad || pad->empty())) {
920+
"'shape=' argument (%s) specifies an array with too many elements"_err_en_US,
921+
DEREF(args[1]->UnwrapExpr()).AsFortran());
922+
ok = false;
923+
}
924+
}
925+
if (order) {
926+
dimOrder = ValidateDimensionOrder(GetRank(*shape), *order);
927+
if (!dimOrder) {
928928
context_.messages().Say(
929-
"Too few elements in 'source=' argument and 'pad=' "
930-
"argument is not present or has null size"_err_en_US);
931-
} else {
932-
Constant<T> result{!source->empty() || !pad
933-
? source->Reshape(std::move(shape.value()))
934-
: pad->Reshape(std::move(shape.value()))};
935-
ConstantSubscripts subscripts{result.lbounds()};
936-
auto copied{result.CopyFrom(*source,
937-
std::min(static_cast<uint64_t>(source->size()), resultElements),
938-
subscripts, dimOrderPtr)};
939-
if (copied < resultElements) {
940-
CHECK(pad);
941-
copied += result.CopyFrom(
942-
*pad, resultElements - copied, subscripts, dimOrderPtr);
943-
}
944-
CHECK(copied == resultElements);
945-
return Expr<T>{std::move(result)};
929+
"Invalid 'order=' argument (%s) in RESHAPE"_err_en_US,
930+
DEREF(args[3]->UnwrapExpr()).AsFortran());
931+
ok = false;
932+
}
933+
}
934+
}
935+
if (!ok) {
936+
// convert into an invalid intrinsic procedure call below
937+
} else if (!source || !shape || (args[2] && !pad) || (args[3] && !order)) {
938+
return Expr<T>{std::move(funcRef)}; // Non-constant arguments
939+
} else {
940+
uint64_t resultElements{*optResultElement};
941+
std::vector<int> *dimOrderPtr{dimOrder ? &dimOrder.value() : nullptr};
942+
if (resultElements > source->size() && (!pad || pad->empty())) {
943+
context_.messages().Say(
944+
"Too few elements in 'source=' argument and 'pad=' "
945+
"argument is not present or has null size"_err_en_US);
946+
ok = false;
947+
} else {
948+
Constant<T> result{!source->empty() || !pad
949+
? source->Reshape(std::move(shape.value()))
950+
: pad->Reshape(std::move(shape.value()))};
951+
ConstantSubscripts subscripts{result.lbounds()};
952+
auto copied{result.CopyFrom(*source,
953+
std::min(static_cast<uint64_t>(source->size()), resultElements),
954+
subscripts, dimOrderPtr)};
955+
if (copied < resultElements) {
956+
CHECK(pad);
957+
copied += result.CopyFrom(
958+
*pad, resultElements - copied, subscripts, dimOrderPtr);
946959
}
960+
CHECK(copied == resultElements);
961+
return Expr<T>{std::move(result)};
947962
}
948963
}
949964
// Invalid, prevent re-folding
@@ -1398,7 +1413,8 @@ AsFlatArrayConstructor(const Expr<SomeKind<CAT>> &expr) {
13981413
template <typename T>
13991414
std::optional<Expr<T>> FromArrayConstructor(
14001415
FoldingContext &context, ArrayConstructor<T> &&values, const Shape &shape) {
1401-
if (auto constShape{AsConstantExtents(context, shape)}) {
1416+
if (auto constShape{AsConstantExtents(context, shape)};
1417+
constShape && !HasNegativeExtent(*constShape)) {
14021418
Expr<T> result{Fold(context, Expr<T>{std::move(values)})};
14031419
if (auto *constant{UnwrapConstantValue<T>(result)}) {
14041420
// Elements and shape are both constant.

‎flang/lib/Semantics/tools.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1584,7 +1584,8 @@ const std::optional<parser::Name> &MaybeGetNodeName(
15841584

15851585
std::optional<ArraySpec> ToArraySpec(
15861586
evaluate::FoldingContext &context, const evaluate::Shape &shape) {
1587-
if (auto extents{evaluate::AsConstantExtents(context, shape)}) {
1587+
if (auto extents{evaluate::AsConstantExtents(context, shape)};
1588+
extents && !evaluate::HasNegativeExtent(*extents)) {
15881589
ArraySpec result;
15891590
for (const auto &extent : *extents) {
15901591
result.emplace_back(ShapeSpec::MakeExplicit(Bound{extent}));

‎flang/test/Semantics/bug122060.f90‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
! RUN: %python %S/test_errors.py %s %flang_fc1
2+
integer, parameter :: a = -10
3+
! ERROR: Assignment to constant 'a' is not allowed
4+
! ERROR: 'shape=' argument ([INTEGER(4)::-10_4]) must not have a negative extent
5+
a = b() - reshape([c], [a])
6+
END

‎flang/test/Semantics/reshape.f90‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,13 @@ program reshaper
2020
integer :: array8(2,3) = RESHAPE([(n, n=1,4)], [2,3], [99_8])
2121
!ERROR: Actual argument for 'pad=' has bad type or kind 'REAL(4)'
2222
real :: array9(2,3) = RESHAPE([(n, n=1,4)], [2,3], [99.9])
23-
!ERROR: Invalid 'order=' argument in RESHAPE
23+
!ERROR: Invalid 'order=' argument ([INTEGER(4)::2_4,3_4]) in RESHAPE
2424
real :: array10(2,3) = RESHAPE([(n,n=1,4)],[2,3],[99],[2,3])
2525
!ERROR: Actual argument for 'order=' has bad type 'REAL(4)'
2626
real :: array11(2,3) = RESHAPE([(n, n=1,4)], [2,3], [99], [2.2,3.3])
27-
!ERROR: Invalid 'order=' argument in RESHAPE
27+
!ERROR: Invalid 'order=' argument ([INTEGER(4)::1_4]) in RESHAPE
2828
real :: array12(2,3) = RESHAPE([(n, n=1,4)], [2,3], [99], [1])
29-
!ERROR: Invalid 'order=' argument in RESHAPE
29+
!ERROR: Invalid 'order=' argument ([INTEGER(4)::1_4,1_4]) in RESHAPE
3030
real :: array13(2,3) = RESHAPE([(n, n = 1, 4)], [2, 3], [99], [1, 1])
3131

3232
! Examples that have caused problems
@@ -50,13 +50,13 @@ program reshaper
5050
integer, parameter :: array22(2) = RESHAPE(array21, [2])
5151

5252
integer(8), parameter :: huge_shape(2) = [I64_MAX, I64_MAX]
53-
!ERROR: 'shape=' argument has too many elements
53+
!ERROR: 'shape=' argument ([INTEGER(8)::9223372036854775807_8,9223372036854775807_8]) specifies an array with too many elements
5454
integer :: array23(I64_MAX, I64_MAX) = RESHAPE([1, 2, 3], huge_shape)
5555

5656
CALL ext_sub(RESHAPE([(n, n=1,20)], &
5757
!ERROR: 'shape=' argument must be a vector of at most 15 elements (has 16)
5858
[1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]))
59-
!ERROR: 'shape=' argument must not have a negative extent
59+
!ERROR: 'shape=' argument ([INTEGER(4)::1_4,-5_4,3_4]) must not have a negative extent
6060
CALL ext_sub(RESHAPE([(n, n=1,20)], [1, -5, 3]))
6161
!ERROR: 'order=' argument has unacceptable rank 2
6262
array20 = RESHAPE([(n, n = 1, 4)], [2, 3], order = bad_order)

0 commit comments

Comments
 (0)