Created attachment 33373 [details] Patch to delete existing .mod file prior to updating Under Win32 using GNU Fortran 4.9.0 or 4.9.1, module files (.mod) are not updated properly when necessary because the code to do so is simply calling "rename()" to move the temporary .mod file to the permanent name without first deleting any existing .mod files. The Windows C runtime states that this operation will fail if a file of the same target name exists already (see the "Remarks" section at http://msdn.microsoft.com/en-us/library/zw5t957f.aspx). Versions 4.8 and earlier of GNU Fortran first performed an "unlink()" on the existing file to delete it, so this bug is new with the 4.9 series. I've attached a patch for 4.9.1 that reinstates the unlink operation that was present in 4.8 and earlier.
(In reply to Jeffrey Armstrong from comment #0) > Created attachment 33373 [details] > Patch to delete existing .mod file prior to updating > > Under Win32 using GNU Fortran 4.9.0 or 4.9.1, module files (.mod) are not > updated properly when necessary because the code to do so is simply calling > "rename()" to move the temporary .mod file to the permanent name without > first deleting any existing .mod files. The Windows C runtime states that > this operation will fail if a file of the same target name exists already > (see the "Remarks" section at > http://msdn.microsoft.com/en-us/library/zw5t957f.aspx). > > Versions 4.8 and earlier of GNU Fortran first performed an "unlink()" on the > existing file to delete it, so this bug is new with the 4.9 series. > > I've attached a patch for 4.9.1 that reinstates the unlink operation that > was present in 4.8 and earlier. Looks like Janne removed that chunk of code in r198023. Janne, is there any reason that this cannot be restored? Jeff, Do you have a copyright assignment on file? Although the patch is small and reverts to older code (which means an assignment is most likely not necessary), if you plan to contribute in the future, then having the assignment on file would be nice.
I do not have a copyright assignment on file, but this patch is pretty minor. I'm willing to fill one out (after I read it, of course), but it shouldn't hold up this patch. The new lines in the patch are taken verbatim from 4.8.2.
On Thu, Aug 21, 2014 at 05:21:50PM +0000, jeffrey.armstrong at approximatrix dot com wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62215 > > --- Comment #2 from Jeffrey Armstrong <jeffrey.armstrong at approximatrix dot com> --- > I do not have a copyright assignment on file, but this patch is pretty minor. > I'm willing to fill one out (after I read it, of course), but it shouldn't hold > up this patch. The new lines in the patch are taken verbatim from 4.8.2. > Not having a copyright assignment won't interfere with this patch being applied. I'm waiting for confirmation from Janne that I can revert the changes in his previous patch. The reason I asked about an assignment is that I am aware of SimplyFortran (which looks like a nice product, but I'm not a Windows user so haven't tried it). Every once in a while, I'll scan the forums for gfortran issues. I'm hoping that you may consider helping to fix other issues with gfortran.
Comment on attachment 33373 [details] Patch to delete existing .mod file prior to updating > /* Module file have changed, replace the old one. */ >+ if (unlink (filename) && errno != ENOENT) >+ gfc_fatal_error ("Can't delete module file '%s': %s", filename, >+ xstrerror (errno)); >+ > if (rename (filename_tmp, filename)) > gfc_fatal_error ("Can't rename module file '%s' to '%s': %s", > filename_tmp, filename, xstrerror (errno)); POSIX states: "If the link named by the new argument exists and the file's link count becomes 0 when it is removed and no process has the file open, the space occupied by the file shall be freed and the file shall no longer be accessible. If one or more processes have the file open when the last link is removed, the link shall be removed before rename() returns, but the removal of the file contents shall be postponed until all references to the file are closed." Thus, I wonder whether one should use an #ifdef instead of simply reinstating the previous version. For instance, "#ifdef __MINGW32__". (I don't know what __CYGWIN__ does, nor whether any other platform has the same issues.)
While checking for MinGW (or MinGW-W64) (or non-POSIX Cygwin if it's a thing...) would allow this to be fixed for Windows, it would continue to assume that any other C runtime would have a POSIX-compliant "rename()" function. I would think that could be dangerous. That said, I can't think of another C runtime that doesn't provide a POSIX-compliant "rename()" function other than some versions of the OpenWatcom C runtime (I'm pretty sure you guys don't care about that) and any MS-DOS or OS/2 C runtimes. Is there a big cost or danger to "unlink()" the existing file first? POSIX "rename()" is going to do that anyway; my patch (and GNU Fortran < 4.9) simply does the deleting explicitly.
(In reply to Steve Kargl from comment #3) > Not having a copyright assignment won't interfere with this > patch being applied. I'm waiting for confirmation from Janne > that I can revert the changes in his previous patch. I don't remember, but I guess I read the description from the POSIX standard and assumed non-POSIX systems would behave the same way. But now that I look at C11 (n1570.pdf) it says that if *new exists the behavior is implementation-defined. So please go ahead and commit the patch to trunk and 4.9. As for the #ifdef stuff, instead of trying to enumerate all non-POSIX systems one could go the other way instead, e.g. something like #if not defined(__unix__) || not defined(_POSIX_VERSION) ... unlink stuff #endif (See http://sourceforge.net/p/predef/wiki/Home/ for lists of various pre-defined macros) That would save an extra syscall on POSIX systems, but I don't think it really matters, so do as you see fit.
(In reply to Janne Blomqvist from comment #6) > So please go ahead and commit the patch to trunk and 4.9. It's fine from my side as well. > As for the #ifdef stuff, [...] > That would save an extra syscall on POSIX systems, but I don't think it > really matters, so do as you see fit. As it is only in the compiler and not in the libgfortran library (as I somehow thought it would be), simply reinstating the unlink call is probably best.
Author: jb Date: Fri Aug 29 20:46:15 2014 New Revision: 214742 URL: https://gcc.gnu.org/viewcvs?rev=214742&root=gcc&view=rev Log: PR 62215 Reinstate unlinking old module file before renaming. 2014-08-29 Jeffrey Armstrong <jeffrey.armstrong@approximatrix.com> PR fortran/62215 * module.c (gfc_dump_module): Unlink old module file before renaming new one. Modified: trunk/gcc/fortran/ChangeLog trunk/gcc/fortran/module.c
Author: jb Date: Fri Aug 29 20:49:16 2014 New Revision: 214743 URL: https://gcc.gnu.org/viewcvs?rev=214743&root=gcc&view=rev Log: PR 62215 Unlink old module file before renaming. 2014-08-29 Jeffrey Armstrong <jeffrey.armstrong@approximatrix.com> Backport from trunk PR fortran/62215 * module.c (gfc_dump_module): Unlink old module file before renaming new one. Modified: branches/gcc-4_9-branch/gcc/fortran/ChangeLog branches/gcc-4_9-branch/gcc/fortran/module.c
Fixed on trunk and 4.9, closing. Thanks for the report and patch!