| Message ID | AANLkTim4=MUheNEr5QBtrVm6Rm63-LF80S+9b1VwXefH@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Feb 9, 2011 at 8:16 PM, Kai Tietz <ktietz70@googlemail.com> wrote: > Hello, > > As for windows native targets unlink isn't possible on still opened files, it is > important to prevent dangling file-descriptors, which are closed not > until application's > exit. > > 2011-02-09 Kai Tietz > > PR lto/47241 > * lto.c (lto_read_section_data): Free > fd_name in failure case. > For mingw targets don't hash file-descriptor. > (read_cgraph_and_symbols): Close current_lto_file > in failure case. > > > Tested for x86_64-pc-linux-gnu, and x86_64-w64-mingw32. Ok for apply? But we don't unlink the file from lto1 but from another process, no? Or does that also cause problems? Richard. > Regards, > Kai > > > -- > | (\_/) This is Bunny. Copy and paste > | (='.'=) Bunny into your signature to help > | (")_(") him gain world domination >
2011/2/9 Richard Guenther <richard.guenther@gmail.com>: > On Wed, Feb 9, 2011 at 8:16 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >> Hello, >> >> As for windows native targets unlink isn't possible on still opened files, it is >> important to prevent dangling file-descriptors, which are closed not >> until application's >> exit. >> >> 2011-02-09 Kai Tietz >> >> PR lto/47241 >> * lto.c (lto_read_section_data): Free >> fd_name in failure case. >> For mingw targets don't hash file-descriptor. >> (read_cgraph_and_symbols): Close current_lto_file >> in failure case. >> >> >> Tested for x86_64-pc-linux-gnu, and x86_64-w64-mingw32. Ok for apply? > > But we don't unlink the file from lto1 but from another process, no? Or > does that also cause problems? As long as one process holds a handle (file-descriptor) to a file and the file wasn't opened with shared delete mode (which is via C-runtime functions on windows not possible AFAIK), even a different process can't unlink a file. As lto1 normally gets called isolated and doesn't spawn dependent sub-process AFAICS, it shouldn't be an issue. The only scenario I see here could be an abnormal process-termination, as here handles can be released delayed or simply can remain as zombie. Nevertheless in such a situation, the closing won't help here much either. So the part about closing file-handles directly after read shouldn't be in general not necessary. Kai
On Wed, Feb 9, 2011 at 9:58 PM, Kai Tietz <ktietz70@googlemail.com> wrote: > 2011/2/9 Richard Guenther <richard.guenther@gmail.com>: >> On Wed, Feb 9, 2011 at 8:16 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >>> Hello, >>> >>> As for windows native targets unlink isn't possible on still opened files, it is >>> important to prevent dangling file-descriptors, which are closed not >>> until application's >>> exit. >>> >>> 2011-02-09 Kai Tietz >>> >>> PR lto/47241 >>> * lto.c (lto_read_section_data): Free >>> fd_name in failure case. >>> For mingw targets don't hash file-descriptor. >>> (read_cgraph_and_symbols): Close current_lto_file >>> in failure case. >>> >>> >>> Tested for x86_64-pc-linux-gnu, and x86_64-w64-mingw32. Ok for apply? >> >> But we don't unlink the file from lto1 but from another process, no? Or >> does that also cause problems? > > As long as one process holds a handle (file-descriptor) to a file and > the file wasn't opened with shared delete mode (which is via C-runtime > functions on windows not possible AFAIK), even a different process > can't unlink a file. Ugh. NFS has at least invented silly-renaming for that ;) > As lto1 normally gets called isolated and doesn't spawn dependent > sub-process AFAICS, it shouldn't be an issue. The only scenario I see > here could be an abnormal process-termination, as here handles can be > released delayed or simply can remain as zombie. Nevertheless in such > a situation, the closing won't help here much either. So the part > about closing file-handles directly after read shouldn't be in general > not necessary. Ok. fd_name = xstrdup (file_data->file_name); fd = open (file_data->file_name, O_RDONLY|O_BINARY); if (fd == -1) - return NULL; + { + free (fd_name); + fd_name = NULL; + return NULL; + } instead do fd = open (...) if (fd == -1) return NULL fd_name = xstrdup (...) the rest of the patch is ok if it helps. Richard. > Kai >
2011/2/9 Richard Guenther <richard.guenther@gmail.com>: > On Wed, Feb 9, 2011 at 9:58 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >> 2011/2/9 Richard Guenther <richard.guenther@gmail.com>: >>> On Wed, Feb 9, 2011 at 8:16 PM, Kai Tietz <ktietz70@googlemail.com> wrote: >>>> Hello, >>>> >>>> As for windows native targets unlink isn't possible on still opened files, it is >>>> important to prevent dangling file-descriptors, which are closed not >>>> until application's >>>> exit. >>>> >>>> 2011-02-09 Kai Tietz >>>> >>>> PR lto/47241 >>>> * lto.c (lto_read_section_data): Free >>>> fd_name in failure case. >>>> For mingw targets don't hash file-descriptor. >>>> (read_cgraph_and_symbols): Close current_lto_file >>>> in failure case. >>>> >>>> >>>> Tested for x86_64-pc-linux-gnu, and x86_64-w64-mingw32. Ok for apply? >>> >>> But we don't unlink the file from lto1 but from another process, no? Or >>> does that also cause problems? >> >> As long as one process holds a handle (file-descriptor) to a file and >> the file wasn't opened with shared delete mode (which is via C-runtime >> functions on windows not possible AFAIK), even a different process >> can't unlink a file. > > Ugh. NFS has at least invented silly-renaming for that ;) > >> As lto1 normally gets called isolated and doesn't spawn dependent >> sub-process AFAICS, it shouldn't be an issue. The only scenario I see >> here could be an abnormal process-termination, as here handles can be >> released delayed or simply can remain as zombie. Nevertheless in such >> a situation, the closing won't help here much either. So the part >> about closing file-handles directly after read shouldn't be in general >> not necessary. > > Ok. > > fd_name = xstrdup (file_data->file_name); > fd = open (file_data->file_name, O_RDONLY|O_BINARY); > if (fd == -1) > - return NULL; > + { > + free (fd_name); > + fd_name = NULL; > + return NULL; > + } > > instead do > > fd = open (...) > if (fd == -1) > return NULL > fd_name = xstrdup (...) > > the rest of the patch is ok if it helps. > > Richard. > > >> Kai >> > Committed at revision 169999 with your suggested adjustment. So I the PR/47241 is from gcc side fixed AFAICS. The remaining part is in binutils' ld. Regards, Kai
Index: gcc/gcc/lto/lto.c =================================================================== --- gcc.orig/gcc/lto/lto.c 2011-01-11 20:36:21.000000000 +0100 +++ gcc/gcc/lto/lto.c 2011-02-09 20:02:40.291549900 +0100 @@ -593,7 +593,11 @@ lto_read_section_data (struct lto_file_d fd_name = xstrdup (file_data->file_name); fd = open (file_data->file_name, O_RDONLY|O_BINARY); if (fd == -1) - return NULL; + { + free (fd_name); + fd_name = NULL; + return NULL; + } } #if LTO_MMAP_IO @@ -619,9 +623,17 @@ lto_read_section_data (struct lto_file_d || read (fd, result, len) != (ssize_t) len) { free (result); - return NULL; + result = NULL; } - +#ifdef __MINGW32__ + /* Native windows doesn't supports delayed unlink on opened file. So + We close file here again. This produces higher I/O load, but at least + it prevents to have dangling file handles preventing unlink. */ + free (fd_name); + fd_name = NULL; + close (fd); + fd = -1; +#endif return result; #endif } @@ -2147,7 +2159,11 @@ read_cgraph_and_symbols (unsigned nfiles file_data = lto_file_read (current_lto_file, resolution, &count); if (!file_data) - break; + { + lto_obj_file_close (current_lto_file); + current_lto_file = NULL; + break; + } decl_data[last_file_ix++] = file_data;