[RFC] Fix compilation failure of remote-fileio.c

[RFC] Fix compilation failure of remote-fileio.c

[Eli Zaretskii]

remote-fileio.c won't compile if `struct stat' doesn't have the
`st_blocks' member.  The patch below fixes that, but I'm not sure it
(and the associated patch to gdb/configure.in, which is not shown
below) is the right fix.  Is it perhaps better not to use st_blocks at
all, and instead compute the number of blocks as shown by the patch,
for all platforms?

2003-12-28  Eli Zaretskii  <eliz@elta.co.il>

	* remote-fileio.c (remote_fileio_to_fio_stat)
	(remote_fileio_func_fstat) [!HAVE_STRUCT_STAT_ST_BLOCKS]:
	Support systems that don't have st_blocks in their `struct stat'.


--- gdb/remote-fileio.c~0	2003-06-11 13:50:10.000000000 +0000
+++ gdb/remote-fileio.c	2003-12-28 10:21:06.000000000 +0000
@@ -411,7 +411,13 @@ remote_fileio_to_fio_stat (struct stat *
   remote_fileio_to_fio_uint ((long) st->st_rdev, fst->fst_rdev);
   remote_fileio_to_fio_ulong ((LONGEST) st->st_size, fst->fst_size);
   remote_fileio_to_fio_ulong ((LONGEST) st->st_blksize, fst->fst_blksize);
+#if HAVE_STRUCT_STAT_ST_BLOCKS
   remote_fileio_to_fio_ulong ((LONGEST) st->st_blocks, fst->fst_blocks);
+#else
+  remote_fileio_to_fio_ulong ((LONGEST) st->st_size / (LONGEST) st->st_blksize
+			      + ((LONGEST) st->st_size % (LONGEST) st->st_blksize) != 0,
+			      fst->fst_blocks);
+#endif
   remote_fileio_to_fio_time (st->st_atime, fst->fst_atime);
   remote_fileio_to_fio_time (st->st_mtime, fst->fst_mtime);
   remote_fileio_to_fio_time (st->st_ctime, fst->fst_ctime);
@@ -1131,7 +1137,9 @@ remote_fileio_func_fstat (char *buf)
       st.st_rdev = 0;
       st.st_size = 0;
       st.st_blksize = 512;
+#if HAVE_STRUCT_STAT_ST_BLOCKS
       st.st_blocks = 0;
+#endif
       if (!gettimeofday (&tv, NULL))
 	st.st_atime = st.st_mtime = st.st_ctime = tv.tv_sec;
       else

Re: [RFC] Fix compilation failure of remote-fileio.c

[Daniel Jacobowitz]

On Sun, Dec 28, 2003 at 03:52:13PM +0200, Eli Zaretskii wrote:
> remote-fileio.c won't compile if `struct stat' doesn't have the
> `st_blocks' member.  The patch below fixes that, but I'm not sure it
> (and the associated patch to gdb/configure.in, which is not shown
> below) is the right fix.  Is it perhaps better not to use st_blocks at
> all, and instead compute the number of blocks as shown by the patch,
> for all platforms?

Probably not.  If it's correct for DJGPP then it's a bit of an
accident.

The reason it isn't correct in general:
                  blksize_t     st_blksize;  /* blocksize for filesystem I/O */
                  blkcnt_t      st_blocks;   /* number of blocks allocated */

It's not clear from those comments, but st_blksize is the "optimal IO
size":

       The value st_blocks gives the size of the file in 512-byte
       blocks.  (This may be smaller than st_size/512 e.g. when the
       file has holes.) The value st_blksize gives the "preferred"
       blocksize for efficient file system I/O.  (Writing to a file in
       smaller chunks may cause an inefficient read-modify-rewrite.)

So you probably want st->st_size / 512 instead.

> +#else
> +  remote_fileio_to_fio_ulong ((LONGEST) st->st_size / (LONGEST) st->st_blksize
> +			      + ((LONGEST) st->st_size % (LONGEST) st->st_blksize) != 0,
> +			      fst->fst_blocks);
> +#endif

A marginally more efficient way to write this is:
 ((LONGEST) st->st_size + st->st_blksize - 1) / st->st_blksize

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFC] Fix compilation failure of remote-fileio.c

[Eli Zaretskii]

> Date: Sun, 28 Dec 2003 18:38:00 -0500
> From: Daniel Jacobowitz <drow@mvista.com>
> 
>        The value st_blocks gives the size of the file in 512-byte
>        blocks.  (This may be smaller than st_size/512 e.g. when the
>        file has holes.) The value st_blksize gives the "preferred"
>        blocksize for efficient file system I/O.  (Writing to a file in
>        smaller chunks may cause an inefficient read-modify-rewrite.)

Ah, right, I forgot about that.

(Is this optimal size for I/O still relevant for modern systems?)

> So you probably want st->st_size / 512 instead.

So, if I keep the conditional and use the divide-by-512 way for
systems that don't have st_blocks, is the patch approved?  Or do I
need to wait for someone else?

(Thanks for the other suggestions, I will use them.)

Re: [RFC] Fix compilation failure of remote-fileio.c

[Daniel Jacobowitz]

On Mon, Dec 29, 2003 at 08:58:43AM +0200, Eli Zaretskii wrote:
> > Date: Sun, 28 Dec 2003 18:38:00 -0500
> > From: Daniel Jacobowitz <drow@mvista.com>
> > 
> >        The value st_blocks gives the size of the file in 512-byte
> >        blocks.  (This may be smaller than st_size/512 e.g. when the
> >        file has holes.) The value st_blksize gives the "preferred"
> >        blocksize for efficient file system I/O.  (Writing to a file in
> >        smaller chunks may cause an inefficient read-modify-rewrite.)
> 
> Ah, right, I forgot about that.
> 
> (Is this optimal size for I/O still relevant for modern systems?)

Not as much as it used to be, but I believe so.

> > So you probably want st->st_size / 512 instead.
> 
> So, if I keep the conditional and use the divide-by-512 way for
> systems that don't have st_blocks, is the patch approved?  Or do I
> need to wait for someone else?
> 
> (Thanks for the other suggestions, I will use them.)

Hmm... since it doesn't compile as-is, I think this is OK.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFC] Fix compilation failure of remote-fileio.c

[Eli Zaretskii]

> Date: Mon, 29 Dec 2003 11:01:42 -0500
> From: Daniel Jacobowitz <drow@mvista.com>
> 
> Hmm... since it doesn't compile as-is, I think this is OK.

I need some help in tweaking the configury stuff for this.  I see that
src/gdb/configure was produced by Autoconf 2.13; is that an unmodified
Autoconf 2.13?  Because running Autoconf 2.13 installed on
fencepost.gnu.org produces quite a few errors about undefined macros.
What am I missing?

Also, if it _is_ Autoconf 2.13, then it seems like it doesn't know
about AC_CHECK_MEMBER, so what should I put in configure.in for
testing the existence of st_blocks?

TIA

Re: [RFC] Fix compilation failure of remote-fileio.c

[Michael Elizabeth Chastain]

eli> Okay, but how do you run autoconf to regenerate gdb/configure?  I'm
eli> used to have some Makefile rule to DTRT, but I cannot seem to find it
eli> in the current CVS, or did I miss something?

It's easy.  You just run autoconf without any parameters:

  mv configure SAVE-configure
  /berman/migchain/install/host/autoconf-000227/bin/autoconf
  diff SAVE-configure configure

You could mess with your $PATH, but I find it easier to just type
in the long path name (using a lot of TAB autocompletion the first
time, and "!/" after that).

eli> Thanks, I suspected that AC_TRY_COMPILE is the only way to go, but I
eli> wasn't sure, as my autoconf experience is virtually non-existent.

My autoconf skills are only a little bit higher than yours, so if
Nathanael or Daniel comes in and tells you to do it different,
that's fine.

Michael C

Re: [RFC] Fix compilation failure of remote-fileio.c

[Eli Zaretskii]

> Date: Tue, 30 Dec 2003 03:26:25 -0500 (EST)
> From: mec.gnu@mindspring.com (Michael Elizabeth Chastain)
> 
> I would recommend:
> 
>   -- download autoconf 000227
>   -- build your own private copy of it
>   -- check that it generates identical "configure"
> 
> Do that before banging on configure.in

Done, and the results committed.

Thanks!

Re: [RFC] Fix compilation failure of remote-fileio.c

[Eli Zaretskii]

> Date: Tue, 30 Dec 2003 03:26:25 -0500 (EST)
> From: mec.gnu@mindspring.com (Michael Elizabeth Chastain)
> 
>   ftp://sources.redhat.com/pub/binutils/autoconf-000227.tar.bz2
> 
> This version of autoconf identifies itself as "autoconf version 2.13",
> but produces different "configure" files than autoconf 2.13.
> Fun fun fun!

Indeed!

Thanks for the pointer, I will fetch that.

> I would recommend:
> 
>   -- download autoconf 000227
>   -- build your own private copy of it
>   -- check that it generates identical "configure"

Okay, but how do you run autoconf to regenerate gdb/configure?  I'm
used to have some Makefile rule to DTRT, but I cannot seem to find it
in the current CVS, or did I miss something?  (The reason I'm asking
something so stupid is because perhaps the errors I saw indicate that
I invoked autoconf incorrectly.)

> Do that before banging on configure.in

Sure.  For the moment, I've removed the change from configure.in, so
as not to break the CVS version.  This means that st_blocks in
remote-fileio.c is not the optimal value on _all_ platforms (since
HAVE_STRUCT_STAT_ST_BLOCKS is not defined anywhere), but at least GDB
will compile and run in a reasonable way.

> eli> Also, if it _is_ Autoconf 2.13, then it seems like it doesn't know
> eli> about AC_CHECK_MEMBER, so what should I put in configure.in for
> eli> testing the existence of st_blocks?
> 
> Maybe cut-and-paste from the l_addr check?

Thanks, I suspected that AC_TRY_COMPILE is the only way to go, but I
wasn't sure, as my autoconf experience is virtually non-existent.

Re: [RFC] Fix compilation failure of remote-fileio.c

[Michael Elizabeth Chastain]

Hi Eli,

src/gdb/configure is produced with this version of autoconf:

  ftp://sources.redhat.com/pub/binutils/autoconf-000227.tar.bz2

This version of autoconf identifies itself as "autoconf version 2.13",
but produces different "configure" files than autoconf 2.13.
Fun fun fun!

The difference is that autoconf-000227 has some "--site-file"
stuff in it that I don't even understand.  But that's the blessed
version for gdb.

eli> Because running Autoconf 2.13 installed on fencepost.gnu.org
eli> produces quite a few errors about undefined macros.  What am I missing?

Oddly, when I run the real autoconf 2.13 on current cvs HEAD,
it does run.  It just produces a different "configure" than the
version in cvs.  autoconf 000227 produces the identical "configure".
So if you are getting "undefined macros" then there is something
even more wrong.

I would recommend:

  -- download autoconf 000227
  -- build your own private copy of it
  -- check that it generates identical "configure"

Do that before banging on configure.in

eli> Also, if it _is_ Autoconf 2.13, then it seems like it doesn't know
eli> about AC_CHECK_MEMBER, so what should I put in configure.in for
eli> testing the existence of st_blocks?

Maybe cut-and-paste from the l_addr check?

Michael C