| Message ID | CAJnFk2e3opjzWtfRt8NNUGpst8KBs3FnTF5vjaWBFEGG=R8FwA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] Wrong warning message fix for gcc 9 | expand |
Ping A week ago I decided it is so minor that I can report here with a patch without creating bugzilla PR. Technically it is a "9 regression" in new functionality added back in summer. Patch was succesfully bootstrapped and regtested on x86_64. Ok for trunk? чт, 21 мар. 2019 г. в 18:53, Roman Zhuykov <zhroma@ispras.ru>: > > Hello, I have found a minor diagnostic issue. > > Since r262910 we got a little odd error messages in cases like this: > $ gcc -flto-compression-level=2-O2 -c empty.c > gcc: error: argument to '-flto-compression-level=' is not between 0 and 9 > $ g++ -fabi-version=2-O2 -c empty.cpp > cc1plus: error: '-fabi-version=1' is no longer supported > > Old messages were more correct: > $ gcc-8 -flto-compression-level=2-O2 -c empty.c > gcc: error: argument to '-flto-compression-level=' should be a > non-negative integer > $ g++-8 -fabi-version=2-O2 -c empty.cpp > g++: error: argument to '-fabi-version=' should be a non-negative integer > > When UInteger option value string is not a number, but it's first char > is a digit, integral_argument function returns -1 without setting > *err. > > Proposed untested patch attached. > > -- > Best Regards, > Roman Zhuykov > > gcc/ChangeLog: > > 2019-03-21 Roman Zhuykov <zhroma@ispras.ru> > > * opts-common.c (integral_argument): Set errno properly in one case. > > diff --git a/gcc/opts-common.c b/gcc/opts-common.c > --- a/gcc/opts-common.c > +++ b/gcc/opts-common.c > @@ -205,8 +205,10 @@ integral_argument (const char *arg, int *err, > bool byte_size_suffix) > value = strtoull (arg, &end, 0); > if (*end) > { > - /* errno is most likely EINVAL here. */ > - *err = errno; > + if (errno) > + *err = errno; > + else > + *err = EINVAL; > return -1; > }
On 3/28/19 11:49 AM, Roman Zhuykov wrote: > Ping > > A week ago I decided it is so minor that I can report here with a > patch without creating bugzilla PR. > Technically it is a "9 regression" in new functionality added back in summer. > > Patch was succesfully bootstrapped and regtested on x86_64. > > Ok for trunk? Thanks for the patch! The change makes sense to me. Can you please also add a test case to the test suite? I can't approve patches, even obvious ones, so please wait for an approval from a maintainer before committing it. Martin > > чт, 21 мар. 2019 г. в 18:53, Roman Zhuykov <zhroma@ispras.ru>: >> >> Hello, I have found a minor diagnostic issue. >> >> Since r262910 we got a little odd error messages in cases like this: >> $ gcc -flto-compression-level=2-O2 -c empty.c >> gcc: error: argument to '-flto-compression-level=' is not between 0 and 9 >> $ g++ -fabi-version=2-O2 -c empty.cpp >> cc1plus: error: '-fabi-version=1' is no longer supported >> >> Old messages were more correct: >> $ gcc-8 -flto-compression-level=2-O2 -c empty.c >> gcc: error: argument to '-flto-compression-level=' should be a >> non-negative integer >> $ g++-8 -fabi-version=2-O2 -c empty.cpp >> g++: error: argument to '-fabi-version=' should be a non-negative integer >> >> When UInteger option value string is not a number, but it's first char >> is a digit, integral_argument function returns -1 without setting >> *err. >> >> Proposed untested patch attached. >> >> -- >> Best Regards, >> Roman Zhuykov >> >> gcc/ChangeLog: >> >> 2019-03-21 Roman Zhuykov <zhroma@ispras.ru> >> >> * opts-common.c (integral_argument): Set errno properly in one case. >> >> diff --git a/gcc/opts-common.c b/gcc/opts-common.c >> --- a/gcc/opts-common.c >> +++ b/gcc/opts-common.c >> @@ -205,8 +205,10 @@ integral_argument (const char *arg, int *err, >> bool byte_size_suffix) >> value = strtoull (arg, &end, 0); >> if (*end) >> { >> - /* errno is most likely EINVAL here. */ >> - *err = errno; >> + if (errno) >> + *err = errno; >> + else >> + *err = EINVAL; >> return -1; >> }
Martin Sebor wrote 2019-03-28 22:44: > On 3/28/19 11:49 AM, Roman Zhuykov wrote: >> Ping >> >> A week ago I decided it is so minor that I can report here with a >> patch without creating bugzilla PR. >> Technically it is a "9 regression" in new functionality added back in >> summer. >> >> Patch was succesfully bootstrapped and regtested on x86_64. >> >> Ok for trunk? > Have found an option, which passes buggy function and all subsequent checks: "-fdiagnostics-minimum-margin-width=-1" gives an error, but "-fdiagnostics-minimum-margin-width=42xyz" silently sets it to -1 :) > Thanks for the patch! The change makes sense to me. Can you > please also add a test case to the test suite? Added the test, is such filename (and contents) ok ? > I can't approve patches, even obvious ones, so please wait for > an approval from a maintainer before committing it. CC'ed to "option handling" maintainer here. -- Roman gcc/testsuite/ChangeLog: 2019-03-29 Roman Zhuykov <zhroma@ispras.ru> * gcc.dg/diag-sanity.c: New test. gcc/ChangeLog: 2019-03-29 Roman Zhuykov <zhroma@ispras.ru> * opts-common.c (integral_argument): Set errno properly in one case. diff --git a/gcc/opts-common.c b/gcc/opts-common.c --- a/gcc/opts-common.c +++ b/gcc/opts-common.c @@ -205,8 +205,10 @@ integral_argument (const char *arg, int *err, bool byte_size_suffix) value = strtoull (arg, &end, 0); if (*end) { - /* errno is most likely EINVAL here. */ - *err = errno; + if (errno) + *err = errno; + else + *err = EINVAL; return -1; } diff --git a/gcc/testsuite/gcc.dg/diag-sanity.c b/gcc/testsuite/gcc.dg/diag-sanity.c new file mode 100644 --- /dev/null +++ b/gcc/testsuite/gcc.dg/diag-sanity.c @@ -0,0 +1,7 @@ +/* Verify that an invalid argument is diagnosed correcly. + { dg-do compile } + { dg-options "-fdiagnostics-minimum-margin-width=42xyz -flto-compression-level=2-O2" } */ + + +/* { dg-error "argument to '-fdiagnostics-minimum-margin-width=' should be a non-negative integer" "" { target *-*-* } 0 } + { dg-error "argument to '-flto-compression-level=' should be a non-negative integer" "" { target *-*-* } 0 } */ > Martin > >> >> чт, 21 мар. 2019 г. в 18:53, Roman Zhuykov <zhroma@ispras.ru>: >>> >>> Hello, I have found a minor diagnostic issue. >>> >>> Since r262910 we got a little odd error messages in cases like this: >>> $ gcc -flto-compression-level=2-O2 -c empty.c >>> gcc: error: argument to '-flto-compression-level=' is not between 0 >>> and 9 >>> $ g++ -fabi-version=2-O2 -c empty.cpp >>> cc1plus: error: '-fabi-version=1' is no longer supported >>> >>> Old messages were more correct: >>> $ gcc-8 -flto-compression-level=2-O2 -c empty.c >>> gcc: error: argument to '-flto-compression-level=' should be a >>> non-negative integer >>> $ g++-8 -fabi-version=2-O2 -c empty.cpp >>> g++: error: argument to '-fabi-version=' should be a non-negative >>> integer >>> >>> When UInteger option value string is not a number, but it's first >>> char >>> is a digit, integral_argument function returns -1 without setting >>> *err. >>> >>> Proposed untested patch attached. >>> >>> -- >>> Best Regards, >>> Roman Zhuykov >>> >>> gcc/ChangeLog: >>> >>> 2019-03-21 Roman Zhuykov <zhroma@ispras.ru> >>> >>> * opts-common.c (integral_argument): Set errno properly in one case. >>> >>> diff --git a/gcc/opts-common.c b/gcc/opts-common.c >>> --- a/gcc/opts-common.c >>> +++ b/gcc/opts-common.c >>> @@ -205,8 +205,10 @@ integral_argument (const char *arg, int *err, >>> bool byte_size_suffix) >>> value = strtoull (arg, &end, 0); >>> if (*end) >>> { >>> - /* errno is most likely EINVAL here. */ >>> - *err = errno; >>> + if (errno) >>> + *err = errno; >>> + else >>> + *err = EINVAL; >>> return -1; >>> }
On 3/29/19 6:31 AM, zhroma wrote: > Martin Sebor wrote 2019-03-28 22:44: >> On 3/28/19 11:49 AM, Roman Zhuykov wrote: >>> Ping >>> >>> A week ago I decided it is so minor that I can report here with a >>> patch without creating bugzilla PR. >>> Technically it is a "9 regression" in new functionality added back in >>> summer. >>> >>> Patch was succesfully bootstrapped and regtested on x86_64. >>> >>> Ok for trunk? >> > > Have found an option, which passes buggy function and all subsequent > checks: > "-fdiagnostics-minimum-margin-width=-1" gives an error, but > "-fdiagnostics-minimum-margin-width=42xyz" silently sets it to -1 :) > >> Thanks for the patch! The change makes sense to me. Can you >> please also add a test case to the test suite? > > Added the test, is such filename (and contents) ok ? > >> I can't approve patches, even obvious ones, so please wait for >> an approval from a maintainer before committing it. > > CC'ed to "option handling" maintainer here. > > -- > Roman > > > gcc/testsuite/ChangeLog: > > 2019-03-29 Roman Zhuykov <zhroma@ispras.ru> > > * gcc.dg/diag-sanity.c: New test. > > > gcc/ChangeLog: > > 2019-03-29 Roman Zhuykov <zhroma@ispras.ru> > > * opts-common.c (integral_argument): Set errno properly in one case. Thanks. I ran this through my tester (no regressions, as expected) and installed it on the trunk. Jeff
diff --git a/gcc/opts-common.c b/gcc/opts-common.c --- a/gcc/opts-common.c +++ b/gcc/opts-common.c @@ -205,8 +205,10 @@ integral_argument (const char *arg, int *err, bool byte_size_suffix) value = strtoull (arg, &end, 0); if (*end) { - /* errno is most likely EINVAL here. */ - *err = errno; + if (errno) + *err = errno; + else + *err = EINVAL; return -1; }