| Message ID | 20200428115306.1038059-1-stefansf@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PR94769] fortran/io.c: Fix use of uninitialized variable num | expand |
On Tue, Apr 28, 2020 at 01:53:07PM +0200, Stefan Schulze Frielinghaus via Gcc-patches wrote: > gcc/fortran/ChangeLog: > > 2020-04-28 Stefan Schulze Frielinghaus <stefansf@linux.ibm.com> > > PR fortran/94769 > * io.c (check_io_constraints): Initialize local variable num. > --- > gcc/fortran/io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c > index e066666e01d..4526f729d1d 100644 > --- a/gcc/fortran/io.c > +++ b/gcc/fortran/io.c > @@ -3840,7 +3840,7 @@ if (condition) \ > > if (dt->asynchronous) > { > - int num; > + int num = 2; > static const char * asynchronous[] = { "YES", "NO", NULL }; Just nitpicking, wouldn't -1 be more usual value? And, I think there should be an assertion that it didn't remain -1 after the call, above /* For "YES", mark related symbols as asynchronous. */ do gcc_checking (num != -1); or so. Note, the reason why this triggers only on s390x is the vastly different inlining parameters the target uses, that causes a lot of headaches everywhere. Jakub
On Tue, Apr 28, 2020 at 2:44 PM Stefan Schulze Frielinghaus via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > While bootstrapping GCC on S/390 the following warning occurs: > > gcc/fortran/io.c: In function 'bool gfc_resolve_dt(gfc_code*, gfc_dt*, locus*)': > gcc/fortran/io.c:3857:7: error: 'num' may be used uninitialized in this function [-Werror=maybe-uninitialized] > 3857 | if (num == 0) > | ^~ > gcc/fortran/io.c:3843:11: note: 'num' was declared here > 3843 | int num; > > Since gfc_resolve_dt is a non-static function we cannot assume anything about > argument DT. Argument DT gets passed to function check_io_constraints which > passes values depending on DT, namely dt->asynchronous->value.character.string > to function compare_to_allowed_values as well as argument warn which is true as > soon as DT->dterr is true. Thus both arguments depend on DT. > > If function compare_to_allowed_values is called with > dt->asynchronous->value.character.string not being an allowed value, and > ALLOWED_F2003 as well as ALLOWED_GNU being NULL (which is the case at the > particular call side), and WARN equals true, then the function returns with a > non-zero value and leaves num uninitialized which renders the warning true. > > Initializing num to any value but zero mimics the behavior of an uninitialized > variable except UB because zero is the only value it is tested for at the > moment. Since num is used as an index into array asynchronous initializing it > to 2 seems plausible. > > Bootstrapped and regtested on S/390. Ok for master? You need to CC fortran patches to fortran@gcc.gnu.org (done hereby) Richard. > gcc/fortran/ChangeLog: > > 2020-04-28 Stefan Schulze Frielinghaus <stefansf@linux.ibm.com> > > PR fortran/94769 > * io.c (check_io_constraints): Initialize local variable num. > --- > gcc/fortran/io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c > index e066666e01d..4526f729d1d 100644 > --- a/gcc/fortran/io.c > +++ b/gcc/fortran/io.c > @@ -3840,7 +3840,7 @@ if (condition) \ > > if (dt->asynchronous) > { > - int num; > + int num = 2; > static const char * asynchronous[] = { "YES", "NO", NULL }; > > /* Note: gfc_reduce_init_expr reports an error if not init-expr. */ > -- > 2.25.3 >
On Tue, Apr 28, 2020 at 3:23 PM Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On Tue, Apr 28, 2020 at 01:53:07PM +0200, Stefan Schulze Frielinghaus via Gcc-patches wrote: > > gcc/fortran/ChangeLog: > > > > 2020-04-28 Stefan Schulze Frielinghaus <stefansf@linux.ibm.com> > > > > PR fortran/94769 > > * io.c (check_io_constraints): Initialize local variable num. > > --- > > gcc/fortran/io.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c > > index e066666e01d..4526f729d1d 100644 > > --- a/gcc/fortran/io.c > > +++ b/gcc/fortran/io.c > > @@ -3840,7 +3840,7 @@ if (condition) \ > > > > if (dt->asynchronous) > > { > > - int num; > > + int num = 2; > > static const char * asynchronous[] = { "YES", "NO", NULL }; > > Just nitpicking, wouldn't -1 be more usual value? > And, I think there should be an assertion that it didn't remain -1 after the > call, above > /* For "YES", mark related symbols as asynchronous. */ > do > gcc_checking (num != -1); > or so. > > Note, the reason why this triggers only on s390x is the vastly different > inlining parameters the target uses, that causes a lot of headaches > everywhere. ... which also do not get adjusted when the global settings do. s390 param_max_inline_insns_auto is now (for >= z13) more than five times the default one! param_inline_min_speedup is also waaaay off (2 vs. 30) But it's always the target maintainers call ... (note that -finline-functions is now enabled at -O2 so this will _vastly_ increase binary size there). Honza, I guess you didn't consider targets playing with those defaults? A better approach than setting a different absolute value per target is to scale the default with a factor. Richard. > Jakub >
Hi Stefan, I prefer Jakub's suggestion – his change LGTM. Cheers, Tobias On 4/28/20 2:30 PM, Jakub Jelinek via Gcc-patches wrote: > On Tue, Apr 28, 2020 at 01:53:07PM +0200, Stefan Schulze Frielinghaus via Gcc-patches wrote: >> gcc/fortran/ChangeLog: >> >> 2020-04-28 Stefan Schulze Frielinghaus <stefansf@linux.ibm.com> >> >> PR fortran/94769 >> * io.c (check_io_constraints): Initialize local variable num. >> --- >> gcc/fortran/io.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c >> index e066666e01d..4526f729d1d 100644 >> --- a/gcc/fortran/io.c >> +++ b/gcc/fortran/io.c >> @@ -3840,7 +3840,7 @@ if (condition) \ >> >> if (dt->asynchronous) >> { >> - int num; >> + int num = 2; >> static const char * asynchronous[] = { "YES", "NO", NULL }; > Just nitpicking, wouldn't -1 be more usual value? > And, I think there should be an assertion that it didn't remain -1 after the > call, above > /* For "YES", mark related symbols as asynchronous. */ > do > gcc_checking (num != -1); > or so. > > Note, the reason why this triggers only on s390x is the vastly different > inlining parameters the target uses, that causes a lot of headaches > everywhere. > > Jakub > ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Hi Tobias, On Tue, Apr 28, 2020 at 05:06:15PM +0200, Tobias Burnus wrote: > Hi Stefan, > > I prefer Jakub's suggestion – his change LGTM. Please find attached an updated patch. Bootstrapped and regtested on S/390. Ok for master? Cheers, Stefan > On 4/28/20 2:30 PM, Jakub Jelinek via Gcc-patches wrote: > > On Tue, Apr 28, 2020 at 01:53:07PM +0200, Stefan Schulze Frielinghaus via Gcc-patches wrote: > > > gcc/fortran/ChangeLog: > > > > > > 2020-04-28 Stefan Schulze Frielinghaus <stefansf@linux.ibm.com> > > > > > > PR fortran/94769 > > > * io.c (check_io_constraints): Initialize local variable num. > > > --- > > > gcc/fortran/io.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c > > > index e066666e01d..4526f729d1d 100644 > > > --- a/gcc/fortran/io.c > > > +++ b/gcc/fortran/io.c > > > @@ -3840,7 +3840,7 @@ if (condition) \ > > > > > > if (dt->asynchronous) > > > { > > > - int num; > > > + int num = 2; > > > static const char * asynchronous[] = { "YES", "NO", NULL }; > > Just nitpicking, wouldn't -1 be more usual value? > > And, I think there should be an assertion that it didn't remain -1 after the > > call, above > > /* For "YES", mark related symbols as asynchronous. */ > > do > > gcc_checking (num != -1); > > or so. > > > > Note, the reason why this triggers only on s390x is the vastly different > > inlining parameters the target uses, that causes a lot of headaches > > everywhere. > > > > Jakub > > > ----------------- > Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany > Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter From 6118c522e96b978a2a8ba5df5fd90f7b38176e16 Mon Sep 17 00:00:00 2001 From: Stefan Schulze Frielinghaus <stefansf@linux.ibm.com> Date: Tue, 28 Apr 2020 13:14:28 +0200 Subject: [PATCH] fortran/io.c: Fix use of uninitialized variable num [PR94769] While bootstrapping GCC on S/390 the following warning occurs: gcc/fortran/io.c: In function 'bool gfc_resolve_dt(gfc_code*, gfc_dt*, locus*)': gcc/fortran/io.c:3857:7: error: 'num' may be used uninitialized in this function [-Werror=maybe-uninitialized] 3857 | if (num == 0) | ^~ gcc/fortran/io.c:3843:11: note: 'num' was declared here 3843 | int num; Since gfc_resolve_dt is a non-static function we cannot assume anything about argument DT. Argument DT gets passed to function check_io_constraints which passes values depending on DT, namely dt->asynchronous->value.character.string to function compare_to_allowed_values as well as argument warn which is true as soon as DT->dterr is true. Thus both arguments depend on DT. If function compare_to_allowed_values is called with dt->asynchronous->value.character.string not being an allowed value, and ALLOWED_F2003 as well as ALLOWED_GNU being NULL (which is the case at the particular call side), and WARN equals true, then the function returns with a non-zero value and leaves num uninitialized which renders the warning true. Initialized num to -1 and added an assert statement. gcc/fortran/ChangeLog: 2020-04-28 Stefan Schulze Frielinghaus <stefansf@linux.ibm.com> PR fortran/94769 * io.c (check_io_constraints): Initialize local variable num to -1 and assert that it receives a meaningful value by function compare_to_allowed_values. --- gcc/fortran/io.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c index e066666e01d..981cf9e88dd 100644 --- a/gcc/fortran/io.c +++ b/gcc/fortran/io.c @@ -3840,7 +3840,7 @@ if (condition) \ if (dt->asynchronous) { - int num; + int num = -1; static const char * asynchronous[] = { "YES", "NO", NULL }; /* Note: gfc_reduce_init_expr reports an error if not init-expr. */ @@ -3853,6 +3853,8 @@ if (condition) \ io_kind_name (k), warn, &dt->asynchronous->where, &num)) return false; + gcc_checking_assert (num != -1); + /* For "YES", mark related symbols as asynchronous. */ if (num == 0) {
On Wed, Apr 29, 2020 at 01:53:01PM +0200, Stefan Schulze Frielinghaus wrote: > Hi Tobias, > > On Tue, Apr 28, 2020 at 05:06:15PM +0200, Tobias Burnus wrote: > > Hi Stefan, > > > > I prefer Jakub's suggestion – his change LGTM. > > Please find attached an updated patch. Bootstrapped and regtested on > S/390. Ok for master? Ok, thanks. > gcc/fortran/ChangeLog: > > 2020-04-28 Stefan Schulze Frielinghaus <stefansf@linux.ibm.com> > > PR fortran/94769 > * io.c (check_io_constraints): Initialize local variable num to > -1 and assert that it receives a meaningful value by function > compare_to_allowed_values. > --- > gcc/fortran/io.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c > index e066666e01d..981cf9e88dd 100644 > --- a/gcc/fortran/io.c > +++ b/gcc/fortran/io.c > @@ -3840,7 +3840,7 @@ if (condition) \ > > if (dt->asynchronous) > { > - int num; > + int num = -1; > static const char * asynchronous[] = { "YES", "NO", NULL }; > > /* Note: gfc_reduce_init_expr reports an error if not init-expr. */ > @@ -3853,6 +3853,8 @@ if (condition) \ > io_kind_name (k), warn, &dt->asynchronous->where, &num)) > return false; > > + gcc_checking_assert (num != -1); > + > /* For "YES", mark related symbols as asynchronous. */ > if (num == 0) > { > -- > 2.25.3 > Jakub
diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c index e066666e01d..4526f729d1d 100644 --- a/gcc/fortran/io.c +++ b/gcc/fortran/io.c @@ -3840,7 +3840,7 @@ if (condition) \ if (dt->asynchronous) { - int num; + int num = 2; static const char * asynchronous[] = { "YES", "NO", NULL }; /* Note: gfc_reduce_init_expr reports an error if not init-expr. */
While bootstrapping GCC on S/390 the following warning occurs: gcc/fortran/io.c: In function 'bool gfc_resolve_dt(gfc_code*, gfc_dt*, locus*)': gcc/fortran/io.c:3857:7: error: 'num' may be used uninitialized in this function [-Werror=maybe-uninitialized] 3857 | if (num == 0) | ^~ gcc/fortran/io.c:3843:11: note: 'num' was declared here 3843 | int num; Since gfc_resolve_dt is a non-static function we cannot assume anything about argument DT. Argument DT gets passed to function check_io_constraints which passes values depending on DT, namely dt->asynchronous->value.character.string to function compare_to_allowed_values as well as argument warn which is true as soon as DT->dterr is true. Thus both arguments depend on DT. If function compare_to_allowed_values is called with dt->asynchronous->value.character.string not being an allowed value, and ALLOWED_F2003 as well as ALLOWED_GNU being NULL (which is the case at the particular call side), and WARN equals true, then the function returns with a non-zero value and leaves num uninitialized which renders the warning true. Initializing num to any value but zero mimics the behavior of an uninitialized variable except UB because zero is the only value it is tested for at the moment. Since num is used as an index into array asynchronous initializing it to 2 seems plausible. Bootstrapped and regtested on S/390. Ok for master? gcc/fortran/ChangeLog: 2020-04-28 Stefan Schulze Frielinghaus <stefansf@linux.ibm.com> PR fortran/94769 * io.c (check_io_constraints): Initialize local variable num. --- gcc/fortran/io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)