| Message ID | CAO9iq9GRf9z3538RbDxv6_E5gcC7JC08kBynpYSf3mbfauYfrQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 18 Sep 2014, Janne Blomqvist wrote: > On Wed, Sep 17, 2014 at 11:36 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote: > > On the other hand, the tree *is* broken for some ports; I'd > > prefer regressions to that. So, unless you're onto this, how do > > you feel about me committing the posted patch and opening a PR > > for the regressions? > > I committed > > Index: inquire.c > =================================================================== > --- inquire.c (revision 215337) > +++ inquire.c (working copy) > @@ -92,9 +92,9 @@ inquire_via_unit (st_parameter_inquire * > else if (u->unit_number == options.stderr_unit) > fstrcpy (iqp->name, iqp->name_len, "CONERR$", sizeof("CONERR$")); > else > - fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len); > + cf_strcpy (iqp->name, iqp->name_len, u->filename); > #else > - fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len); > + cf_strcpy (iqp->name, iqp->name_len, u->filename); > #endif > } > > as obvious (r215338). (Bah, without the indentation fixed...) 'k so we'll track the regressions in a PR. brgds, H-P
On Thu, Sep 18, 2014 at 12:57 AM, Hans-Peter Nilsson <hp@bitrange.com> wrote: > On Thu, 18 Sep 2014, Janne Blomqvist wrote: >> On Wed, Sep 17, 2014 at 11:36 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote: >> > On the other hand, the tree *is* broken for some ports; I'd >> > prefer regressions to that. So, unless you're onto this, how do >> > you feel about me committing the posted patch and opening a PR >> > for the regressions? >> >> I committed >> >> Index: inquire.c >> =================================================================== >> --- inquire.c (revision 215337) >> +++ inquire.c (working copy) >> @@ -92,9 +92,9 @@ inquire_via_unit (st_parameter_inquire * >> else if (u->unit_number == options.stderr_unit) >> fstrcpy (iqp->name, iqp->name_len, "CONERR$", sizeof("CONERR$")); >> else >> - fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len); >> + cf_strcpy (iqp->name, iqp->name_len, u->filename); >> #else >> - fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len); >> + cf_strcpy (iqp->name, iqp->name_len, u->filename); >> #endif >> } >> >> as obvious (r215338). > > (Bah, without the indentation fixed...) Fixed in r215340. :) > 'k so we'll track the regressions in a PR. Do you prefer to tack on to 62768 or a new PR? Note that the r215338 fix only applies when using INQUIRE(...NAME=...), so I don't think manually disabling HAVE_TTYNAME{_R} helps in finding the cause of the regressions, while I haven't gone through every testcase you mentioned none of the few ones I did check inquired for the name. So there must be something else. Can you check where it's actually failing? Is it failing the testcase (call abort() ) or is there a segfault etc.? I suggest trying e.g. gfortran.dg/inquire.f90 which is quite a simple testcase.
On Thu, 18 Sep 2014, Janne Blomqvist wrote: > On Thu, Sep 18, 2014 at 12:57 AM, Hans-Peter Nilsson <hp@bitrange.com> wrote: > > 'k so we'll track the regressions in a PR. > > Do you prefer to tack on to 62768 or a new PR? Hijacking 62768 for the purposes of reporting a regression for its fix would not be proper. > Note that the r215338 fix only applies when using > INQUIRE(...NAME=...), so I don't think manually disabling > HAVE_TTYNAME{_R} helps in finding the cause of the regressions, while > I haven't gone through every testcase you mentioned none of the few > ones I did check inquired for the name. So there must be something > else. Can you check where it's actually failing? Is it failing the > testcase (call abort() ) or is there a segfault etc.? Will tell in a new PR, unless I see a really obvious fix. > I suggest trying > e.g. gfortran.dg/inquire.f90 which is quite a simple testcase. Thanks. brgds, H-P
On Wed, 17 Sep 2014, Hans-Peter Nilsson wrote: > On Thu, 18 Sep 2014, Janne Blomqvist wrote: > > On Thu, Sep 18, 2014 at 12:57 AM, Hans-Peter Nilsson <hp@bitrange.com> wrote: > > > 'k so we'll track the regressions in a PR. > > > > Do you prefer to tack on to 62768 or a new PR? > > Hijacking 62768 for the purposes of reporting a regression for > its fix would not be proper. > Will tell in a new PR, unless I see a really obvious fix. False alarm. If you look back at the patch I posted, there's a typo. :-} Duly warned about, but I'd rather expect the build to fail. Apparently libgfortran is not compiled with -Werror, at least not for crosses. Maybe -Werror is there for native but I'm not sure as I see some "warning: array subscript has type 'char' [-Wchar-subscripts]" which seems generic and also some others. Though no more than can be fixed or excepted, IMHO. brgds, H-P
On Thu, Sep 18, 2014 at 11:14 AM, Hans-Peter Nilsson <hp@bitrange.com> wrote: > On Wed, 17 Sep 2014, Hans-Peter Nilsson wrote: >> On Thu, 18 Sep 2014, Janne Blomqvist wrote: >> > On Thu, Sep 18, 2014 at 12:57 AM, Hans-Peter Nilsson <hp@bitrange.com> wrote: >> > > 'k so we'll track the regressions in a PR. >> > >> > Do you prefer to tack on to 62768 or a new PR? >> >> Hijacking 62768 for the purposes of reporting a regression for >> its fix would not be proper. > >> Will tell in a new PR, unless I see a really obvious fix. > > False alarm. Ok, good; I was a bit perplexed what could be wrong. > If you look back at the patch I posted, there's a > typo. :-} Duly warned about, but I'd rather expect the build to > fail. Yes, strange that it didn't fail. There's no prototype for cf_fstrcpy, and since we use std=gnu11 prototypes should be mandatory. Also, since there's no symbol called cf_fstrcpy so at least the linking should fail. Unless the link picked up some old inquire.o file? > Apparently libgfortran is not compiled with -Werror, at least > not for crosses. Maybe -Werror is there for native but I'm not > sure as I see some "warning: array subscript has type 'char' > [-Wchar-subscripts]" which seems generic and also some others. > Though no more than can be fixed or excepted, IMHO. No, Werror isn't used. It was tried, but apparently caused issues. From the changelog: 2008-06-13 Tobias Burnus <burnus@net-b.de> * configure.ac (AM_CFLAGS): Remove -Werror again. * configure: Regenerate. 2008-06-13 Tobias Burnus <burnus@net-b.de> PR libgfortran/36518 * configure.ac (AM_CFLAGS): Add -Werror. * configure: Regenerate. * m4/ifunction_logical.m4: Cast "n" to "(int)". * generated/any_l16.c: Regenerate. * generated/any_l2.c: Regenerate. * generated/all_l1.c: Regenerate. * generated/all_l2.c: Regenerate. * generated/all_l16.c: Regenerate. * generated/any_l4.c: Regenerate. * generated/count_4_l.c: Regenerate. * generated/count_8_l.c: Regenerate. * generated/all_l4.c: Regenerate. * generated/count_1_l.c: Regenerate. * generated/count_16_l.c: Regenerate. * generated/any_l8.c: Regenerate. * generated/count_2_l.c: Regenerate. * generated/any_l1.c: Regenerate. * generated/all_l8.c: Regenerate. I have a vague recollection that there were issues with system headers on non-glibc targets. It would be nice if Werror was used by default, I think we've had a few cases where bugs have slipped past due to it.
On Sep 18 2014, Janne Blomqvist wrote: > >> Apparently libgfortran is not compiled with -Werror, at least >> not for crosses. Maybe -Werror is there for native but I'm not >> sure as I see some "warning: array subscript has type 'char' >> [-Wchar-subscripts]" which seems generic and also some others. >> Though no more than can be fixed or excepted, IMHO. > >No, Werror isn't used. It was tried, but apparently caused issues. >From the changelog: > >2008-06-13 Tobias Burnus <burnus@net-b.de> > > * configure.ac (AM_CFLAGS): Remove -Werror again. > >I have a vague recollection that there were issues with system headers >on non-glibc targets. It would be nice if Werror was used by default, >I think we've had a few cases where bugs have slipped past due to it. I wasn't involved, but that sounds more than just likely! I have had that experience with several options, including Werror, pedantic and specific standards ones. My experience is that most vendors clean up at least the standard C headers with time, and usually the more basic POSIX ones, but any others often remain beyond redemption. And what is not going to help is the ongoing incompatibilities in de jure and de facto standards. I have certainly seen standard headers that would compile only with specific language selection options. Oh, yes, their COMPILER supported other ones - you just couldn't use some important system headers with them :-( If I get time, I will look at the libfortran header use and see if I can make any useful specific comments. Regards, Nick Maclaren.
On Thu, 18 Sep 2014, Janne Blomqvist wrote: > > If you look back at the patch I posted, there's a > > typo. :-} Duly warned about, but I'd rather expect the build to > > fail. > > Yes, strange that it didn't fail. There's no prototype for cf_fstrcpy, > and since we use std=gnu11 prototypes should be mandatory. Also, since > there's no symbol called cf_fstrcpy so at least the linking should > fail. Unless the link picked up some old inquire.o file? For closure: no linking certainly *did* fail and no executable was created for those tests; failing linking correctly counts as a fail too. > > Apparently libgfortran is not compiled with -Werror, at least > > not for crosses. Maybe -Werror is there for native but I'm not > > sure as I see some "warning: array subscript has type 'char' > > [-Wchar-subscripts]" which seems generic and also some others. > > Though no more than can be fixed or excepted, IMHO. > > No, Werror isn't used. It was tried, but apparently caused issues. 'k. Maybe -Werror=implicit-function-declaration is a middle way. brgds, H-P
On Thu, Sep 18, 2014 at 11:33 PM, Hans-Peter Nilsson <hp@bitrange.com> wrote: > On Thu, 18 Sep 2014, Janne Blomqvist wrote: >> > If you look back at the patch I posted, there's a >> > typo. :-} Duly warned about, but I'd rather expect the build to >> > fail. >> >> Yes, strange that it didn't fail. There's no prototype for cf_fstrcpy, >> and since we use std=gnu11 prototypes should be mandatory. Also, since >> there's no symbol called cf_fstrcpy so at least the linking should >> fail. Unless the link picked up some old inquire.o file? > > For closure: no linking certainly *did* fail and no executable > was created for those tests; failing linking correctly counts as > a fail too. > >> > Apparently libgfortran is not compiled with -Werror, at least >> > not for crosses. Maybe -Werror is there for native but I'm not >> > sure as I see some "warning: array subscript has type 'char' >> > [-Wchar-subscripts]" which seems generic and also some others. >> > Though no more than can be fixed or excepted, IMHO. >> >> No, Werror isn't used. It was tried, but apparently caused issues. > > 'k. Maybe -Werror=implicit-function-declaration is a middle > way. Good idea. I committed r215741 as obvious which adds this to the compile flags. I'm sure there are other warnings that can be enabled with -Werror=... in a similar fashion, but this is a start at least. Another approach would be to enable -Werror if some conditions are met. Such as - native build - --enable-maintainer-mode - glibc target I'm not in the mood to torture myself with autofoo to do this ATM, but food for thought..
Index: inquire.c =================================================================== --- inquire.c (revision 215337) +++ inquire.c (working copy) @@ -92,9 +92,9 @@ inquire_via_unit (st_parameter_inquire * else if (u->unit_number == options.stderr_unit) fstrcpy (iqp->name, iqp->name_len, "CONERR$", sizeof("CONERR$")); else - fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len); + cf_strcpy (iqp->name, iqp->name_len, u->filename); #else - fstrcpy (iqp->name, iqp->name_len, u->file, u->file_len); + cf_strcpy (iqp->name, iqp->name_len, u->filename); #endif }