Page MenuHomePhabricator

[flang] avoid GCC < 8 compiler failure after D80794
ClosedPublic

Authored by clementval on Jun 4 2020, 11:11 AM.

Details

Summary

Patch D80794 remove the custom flags for release build for flang.
This leads to build failure with GCC < 8. This patch add upperbound check
in order to avoid the -Werror=array-bounds to trigger a build failure.

/home/4vn/versioning/llvm-project/flang/lib/Decimal/big-radix-floating-point.h:183:29: error: array subscript is above array bounds [-Werror=array-bounds]
           digit_[j] = digit_[j + remove];
                       ~~~~~~^
/home/4vn/versioning/llvm-project/flang/lib/Decimal/big-radix-floating-point.h:183:29: error: array subscript is above array bounds [-Werror=array-bounds]
           digit_[j] = digit_[j + remove];
                       ~~~~~~^
/home/4vn/versioning/llvm-project/flang/lib/Decimal/big-radix-floating-point.h:183:29: error: array subscript is above array bounds [-Werror=array-bounds]
           digit_[j] = digit_[j + remove];
                       ~~~~~~^
/home/4vn/versioning/llvm-project/flang/lib/Decimal/big-radix-floating-point.h:183:29: error: array subscript is above array bounds [-Werror=array-bounds]
           digit_[j] = digit_[j + remove];
                       ~~~~~~^
/home/4vn/versioning/llvm-project/flang/lib/Decimal/big-radix-floating-point.h:183:29: error: array subscript is above array bounds [-Werror=array-bounds]
           digit_[j] = digit_[j + remove];
/home/4vn/versioning/llvm-project/flang/include/flang/Evaluate/integer.h:809:28: error: array subscript is above array bounds [-Werror=array-bounds]
               xy += product[to];
                     ~~~~~~~^
/home/4vn/versioning/llvm-project/flang/include/flang/Evaluate/integer.h:810:22: error: array subscript is above array bounds [-Werror=array-bounds]
               product[to] = xy & partMask;
               ~~~~~~~^
/home/4vn/versioning/llvm-project/flang/include/flang/Evaluate/integer.h:809:28: error: array subscript is above array bounds [-Werror=array-bounds]
               xy += product[to];
                     ~~~~~~~^

Diff Detail

Event Timeline

clementval created this revision.Jun 4 2020, 11:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2020, 11:11 AM
clementval edited the summary of this revision. (Show Details)Jun 4 2020, 11:12 AM
clementval added a project: Restricted Project.
DavidTruby accepted this revision.Jun 4 2020, 11:27 AM

This looks good to me.

As an aside, I think this perfectly demonstrates why we should not force on Werror or have it on by default. We don't have public build bots for even the compilers we claim to support and seemingly innocent changes can cause spurious build failures for other users simply because we are forcing Werror on. And worse, at the moment we are forcing Werror on with no way to turn it off meaning these spurious errors can't even be avoided. This comment isn't intended in any way to block this patch; I just wanted to note it here.

This revision is now accepted and ready to land.Jun 4 2020, 11:27 AM

This looks good to me.

As an aside, I think this perfectly demonstrates why we should not force on Werror or have it on by default. We don't have public build bots for even the compilers we claim to support and seemingly innocent changes can cause spurious build failures for other users simply because we are forcing Werror on. And worse, at the moment we are forcing Werror on with no way to turn it off meaning these spurious errors can't even be avoided. This comment isn't intended in any way to block this patch; I just wanted to note it here.

Fully agree with your point on Werror. Thanks for the review.

This revision was automatically updated to reflect the committed changes.

If this is a version-specific work around for a g++ bug, please don't leave it enabled for later fixed versions of g++. These loops can be executed as part of formatted floating-point I/O and the performance cost of your extra per-iteration overhead may affect application runtime performance.

If this is a version-specific work around for a g++ bug, please don't leave it enabled for later fixed versions of g++. These loops can be executed as part of formatted floating-point I/O and the performance cost of your extra per-iteration overhead may affect application runtime performance.

Sure I'll send a patch to have the workaround only for GCC < 8.

If there are bugs in compiler warnings - probably disable the warning in the build system (I think we have a few such warning disablings), if the workaround is particularly awkward. (if there are nice rephrasing of the same code hat don't need to be commented but just look good/correct/fine/normal way to write the code, then I'd be willing to overlook some warning bugs like that)