RFA: Avoid use of ftruncate() in libgcov

RFA: Avoid use of ftruncate() in libgcov

[Richard Sandiford]

Most of libgcov uses standard C file functions, but one exception that
seems to have caused trouble is the use of ftruncate() in gcov_truncate().
In the case of windows, this led to a patch to gcov-io.h itself:

2004-03-22  Danny Smith  <dannysmith@users.sourceforge.net>

	PR target/14291
	* gcov-io.h (gcov_truncate): Define ftruncate as _chsize for
	__MINGW32__.

whereas in the case of sh-elf, Joern added a new ftruncate() system
call to newlib and the simulator:

   http://sources.redhat.com/ml/newlib/2003/msg00439.html

Most of Joern's patch is sh-specific, and similar newlib/sim changes
would be needed for other simulator targets too.

So... would it be OK to drop the use of ftruncate() and rely simply on
closing and reopening the file in w+ mode?  Patch below.  I've verified
that the truncation still works on i686-pc-linux-gnu after this patch.

I wasn't entirely sure whether functions like __gcov_open() are considered
internal to libgcov or whether they're part of some external (stable?) API.
Since I couldn't see any user documentation of the libgcov functions, and
since it doesn't look like we install any header files associated with it,
I assumed the function was internal and could be changed.

Bootstrapped & regression tested on i686-pc-linux-gnu.  OK for mainline?

Richard


	* gcov-io.h (gcov_open): Remove prototype for single-argument form.
	(gcov_truncate): Delete.
	* gcov-io.c (gcov_open): Always take a mode argument.
	* libgcov.c (gcov_exit): Pass mode argument to gcov_open().  Avoid
	using gcov_truncate if an existing file has the wrong time stamp.
	Close the file instead, and reopen it with a mode of -1.

Index: gcov-io.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/gcov-io.c,v
retrieving revision 1.15
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.15 gcov-io.c
*** gcov-io.c	23 Feb 2004 17:02:50 -0000	1.15
--- gcov-io.c	28 Jul 2004 12:10:00 -0000
*************** static inline gcov_unsigned_t from_file 
*** 54,68 ****
     opening an existing file and <0 on creating a new one.  */
  
  GCOV_LINKAGE int
- #if IN_LIBGCOV
- gcov_open (const char *name)
- #else
  gcov_open (const char *name, int mode)
- #endif
  {
- #if IN_LIBGCOV
-   const int mode = 0;
- #endif
  #if GCOV_LOCKED
    struct flock s_flock;
    int fd;
--- 54,61 ----
Index: gcov-io.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/gcov-io.h,v
retrieving revision 1.50
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.50 gcov-io.h
*** gcov-io.h	25 Apr 2004 16:38:07 -0000	1.50
--- gcov-io.h	28 Jul 2004 12:10:01 -0000
*************** GCOV_LINKAGE struct gcov_var
*** 499,508 ****
     you use the functions for reading, then gcov_rewrite then the
     functions for writing.  Your file may become corrupted if you break
     these invariants.  */
- #if IN_LIBGCOV
- GCOV_LINKAGE int gcov_open (const char */*name*/);
- #else
  GCOV_LINKAGE int gcov_open (const char */*name*/, int /*direction*/);
  GCOV_LINKAGE int gcov_magic (gcov_unsigned_t, gcov_unsigned_t);
  #endif
  GCOV_LINKAGE int gcov_close (void);
--- 499,506 ----
     you use the functions for reading, then gcov_rewrite then the
     functions for writing.  Your file may become corrupted if you break
     these invariants.  */
  GCOV_LINKAGE int gcov_open (const char */*name*/, int /*direction*/);
+ #if !IN_LIBGCOV
  GCOV_LINKAGE int gcov_magic (gcov_unsigned_t, gcov_unsigned_t);
  #endif
  GCOV_LINKAGE int gcov_close (void);
*************** GCOV_LINKAGE void gcov_write_counter (gc
*** 522,528 ****
  GCOV_LINKAGE void gcov_write_tag_length (gcov_unsigned_t, gcov_unsigned_t);
  GCOV_LINKAGE void gcov_write_summary (gcov_unsigned_t /*tag*/,
  				      const struct gcov_summary *);
- static void gcov_truncate (void);
  static void gcov_rewrite (void);
  GCOV_LINKAGE void gcov_seek (gcov_position_t /*position*/);
  #else
--- 520,525 ----
*************** gcov_rewrite (void)
*** 595,609 ****
    gcov_var.offset = 0;
    fseek (gcov_var.file, 0L, SEEK_SET);
  }
- 
- #ifdef __MINGW32__
- #define ftruncate _chsize
- #endif
- static inline void
- gcov_truncate (void)
- {
-   ftruncate (fileno (gcov_var.file), 0L);
- }
  #endif
  
  #endif /* IN_LIBGCOV >= 0 */
--- 592,597 ----
Index: libgcov.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/libgcov.c,v
retrieving revision 1.25
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.25 libgcov.c
*** libgcov.c	23 Apr 2004 22:50:16 -0000	1.25
--- libgcov.c	28 Jul 2004 12:10:02 -0000
*************** gcov_exit (void)
*** 204,210 ****
  	  fi_stride &= ~(__alignof__ (struct gcov_fn_info) - 1);
  	}
        
!       if (!gcov_open (gi_ptr->filename))
  	{
  	  fprintf (stderr, "profiling:%s:Cannot open\n", gi_ptr->filename);
  	  continue;
--- 204,210 ----
  	  fi_stride &= ~(__alignof__ (struct gcov_fn_info) - 1);
  	}
        
!       if (!gcov_open (gi_ptr->filename, 0))
  	{
  	  fprintf (stderr, "profiling:%s:Cannot open\n", gi_ptr->filename);
  	  continue;
*************** gcov_exit (void)
*** 231,237 ****
  	    {
  	      /* Read from a different compilation. Overwrite the
  		 file.  */
! 	      gcov_truncate ();
  	      goto rewrite;
  	    }
  	  
--- 231,243 ----
  	    {
  	      /* Read from a different compilation. Overwrite the
  		 file.  */
! 	      gcov_close ();
! 	      if (!gcov_open (gi_ptr->filename, -1))
! 		{
! 		  fprintf (stderr, "profiling:%s:Cannot open\n",
! 			   gi_ptr->filename);
! 		  continue;
! 		}
  	      goto rewrite;
  	    }
  	  

Re: RFA: Avoid use of ftruncate() in libgcov

[Nathan Sidwell]

Richard Sandiford wrote:
> So... would it be OK to drop the use of ftruncate() and rely simply on
> closing and reopening the file in w+ mode?  Patch below.  I've verified
> that the truncation still works on i686-pc-linux-gnu after this patch.
closing and opening will break concurrent updates. (which I think manifests
on an enable-coverage bootstrap with -j<somenumber>).  An alternative to
ftruncate would be some kind of explicit STOP record in the file.  But I think
that's tricky given that there are some seek-to-end operations going on.

> I wasn't entirely sure whether functions like __gcov_open() are considered
> internal to libgcov or whether they're part of some external (stable?) API.
> Since I couldn't see any user documentation of the libgcov functions, and
> since it doesn't look like we install any header files associated with it,
> I assumed the function was internal and could be changed.
Those are internal to libgcov.  The entire coverage API and file structure
is internal to a gcc release, but the file structure is designed to be
forward compatible.

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::     CodeSourcery LLC
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

Re: RFA: Avoid use of ftruncate() in libgcov

[Richard Sandiford]

Nathan Sidwell <nathan@codesourcery.com> writes:
> Richard Sandiford wrote:
>> So... would it be OK to drop the use of ftruncate() and rely simply on
>> closing and reopening the file in w+ mode?  Patch below.  I've verified
>> that the truncation still works on i686-pc-linux-gnu after this patch.
> closing and opening will break concurrent updates. (which I think manifests
> on an enable-coverage bootstrap with -j<somenumber>).

This is probably a silly question, but break in what way?  ftruncate()
only gets called if the file is about to be rewritten from scratch.
If instance A of the program closes the file, then instance B opens
it before A has a chance to truncate it, I'd have expected A's second
open to fail.  How is that different from failing to open the file
in the first place?

Richard

Re: RFA: Avoid use of ftruncate() in libgcov

[Richard Sandiford]

Nathan Sidwell <nathan@codesourcery.com> writes:
> ftruncate is used when it's detected that the coverage file was produced
> by a different version of the object file (it's been recompiled perhaps).
> We need to throw away the data and start afresh, and we want to do that
> exactly once.  By using close/open to achieve that there will be the
> opportunity for the following to happen,
>
> instanceA		instanceB
> open
> discover mismatch
> close
> 			open
> 			discover mismatch
> 			close
> open
> write data
> close
> 			open
> 			write data
> 			close
>
> thus losing instanceA's data.

But why is that any worse than the other scenarios in which data
from one instance is lost?  E.g.:

instanceA		instanceB
open
discover mismatch
truncate
			try to open --> fail, skip to next file
write data
close

which loses instanceB's data.

Richard

Re: RFA: Avoid use of ftruncate() in libgcov

[Nathan Sidwell]

Richard Sandiford wrote:
> Nathan Sidwell <nathan@codesourcery.com> writes:
> 
>>Richard Sandiford wrote:
>>
>>>So... would it be OK to drop the use of ftruncate() and rely simply on
>>>closing and reopening the file in w+ mode?  Patch below.  I've verified
>>>that the truncation still works on i686-pc-linux-gnu after this patch.
>>
>>closing and opening will break concurrent updates. (which I think manifests
>>on an enable-coverage bootstrap with -j<somenumber>).
> 
> 
> This is probably a silly question, but break in what way?  ftruncate()
> only gets called if the file is about to be rewritten from scratch.
> If instance A of the program closes the file, then instance B opens
> it before A has a chance to truncate it, I'd have expected A's second
> open to fail.  How is that different from failing to open the file
> in the first place?

ftruncate is used when it's detected that the coverage file was produced
by a different version of the object file (it's been recompiled perhaps).
We need to throw away the data and start afresh, and we want to do that
exactly once.  By using close/open to achieve that there will be the
opportunity for the following to happen,

instanceA		instanceB
open
discover mismatch
close
			open
			discover mismatch
			close
open
write data
close
			open
			write data
			close

thus losing instanceA's data.

Anyway, I'm testing a patch that implements an EOF tag, so truncation
will not be necessary.

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::     CodeSourcery LLC
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

Re: RFA: Avoid use of ftruncate() in libgcov

[Richard Sandiford]

Richard Sandiford <rsandifo@redhat.com> writes:
> But why is that any worse than the other scenarios in which data
> from one instance is lost?

Sorry, I obviously need to go back to class.  I'd forgotten it was
possible for two processes to have the same file open for writing
at the same time.  Duh.

Richard

Re: RFA: Avoid use of ftruncate() in libgcov

[Nathan Sidwell]

Richard Sandiford wrote:

> But why is that any worse than the other scenarios in which data
> from one instance is lost?  E.g.:
> 
> instanceA		instanceB
> open
> discover mismatch
> truncate
> 			try to open --> fail, skip to next file
> write data
> close
> 
> which loses instanceB's data.
I thought the second open blocks.  It should do. Does it not?

nathan


-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::     CodeSourcery LLC
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

Re: RFA: Avoid use of ftruncate() in libgcov

[Richard Sandiford]

Nathan Sidwell <nathan@codesourcery.com> writes:
>> But why is that any worse than the other scenarios in which data
>> from one instance is lost?  E.g.:
>> instanceA		instanceB
>> open
>> discover mismatch
>> truncate
>> 			try to open --> fail, skip to next file
>> write data
>> close
>> which loses instanceB's data.
> I thought the second open blocks.  It should do.

Yes, if you have F_SETLKW.  We don't for newlib though, so I was getting
caught up on the fopen() version.  I guess you're not trying to guarantee
freedom for race conditions for that though.  Sigh.

...jedi hand wave...

Richard