Mini-patch for cccp.c

Mini-patch for cccp.c

[Thomas Koenig]

Here's a mini-patch to cccp.c.

Rationale:

- system_header_p as char saves no space, since the struct gets padded
  anyway

- It may slow down processing infinitesimally (converting to/from int)

- using chars as Booleans are a bad idea, anyway

- This patch shuts up a spurious warning by checkergcc (which is why I
  did it in the first place)

Hope I don't have to sign a copyright assignment to the FSF for this
one-liner :-)

Tue Sep 30 14:54:55 CEST 1997  Thomas Koenig  (ig25@rz.uni-karlsruhe.de)

	* cccp.c: change system_header_p in struct file_buf to int

+++ cccp.c	Tue Sep 30 14:44:02 1997
@@ -519,7 +519,7 @@
   /* Object to be freed at end of input at this level.  */
   U_CHAR *free_ptr;
   /* True if this is a header file included using <FILENAME>.  */
-  char system_header_p;
+  int system_header_p;
 } instack[INPUT_STACK_MAX];
 
 static int last_error_tick;	   /* Incremented each time we print it.  */
-- 
Thomas Koenig, Thomas.Koenig@ciw.uni-karlsruhe.de, ig25@dkauni2.bitnet.
The joy of engineering is to find a straight line on a double
logarithmic diagram.

Re: Mini-patch for cccp.c

[Jeffrey A Law]

  In message < 199709301300.OAA01773@mvmap66.ciw.uni-karlsruhe.de >you write:
  > - system_header_p as char saves no space, since the struct gets padded
  >   anyway
But if we want to add more fields later it may save space.

  > - It may slow down processing infinitesimally (converting to/from int)
Or it may speed it up.  It's target dependent.

  > - using chars as Booleans are a bad idea, anyway
Why?

  > - This patch shuts up a spurious warning by checkergcc (which is why I
  >   did it in the first place)
What is the warning?

jeff

Re: Mini-patch for cccp.c

[Thomas Koenig]

Jeffrey A Law wrote:

>  > - This patch shuts up a spurious warning by checkergcc (which is why I
>  >   did it in the first place)
>What is the warning?

The problem is that checkergcc warns about

typedef struct {
    int a; char b;
} foo_type;

foo_type bar()
{
    foo_type res;
    res.a = 19; res.b = 'a';
    return res;
}

int main()
{
    foo_type c;
    c = bar();
    printf("a = %d, b = %c\n",c.a,c.b);
    return 0;
}

with a 'read uninitialized bytes' warning in the 'return res' line of
bar().

The reason is clear: sizeof(res) is 8 because of padding, and 'return res'
copies 8 bytes onto the caller's stack in two 4-byte words.  checkergcc,
which only manipulates the assembler, has no way of knowing that the three
extra bytes don't mean anything, and warns about them.

Obviously, this is a Checker artifact.  Checker is immensely useful
otherwise (I found the other two bugs I reported recently with it), but
its output can be rather overwhelming.  Therefore, I think it may be
worth making this particular change which doesn't hurt anything else,
AFAIK, just to shut it up in this particular instance.

-- 
Thomas Koenig, Thomas.Koenig@ciw.uni-karlsruhe.de, ig25@dkauni2.bitnet.
The joy of engineering is to find a straight line on a double
logarithmic diagram.

Re: Mini-patch for cccp.c

[Paul Koning]

Since lots of structs have padding all over the place, what you
describe is clearly a Checker bug.  It doesn't make sense to me to
change one of the many structs in one of the many programs to work
around this bug.

	paul

Re: Mini-patch for cccp.c

[Thomas Koenig]

Paul Koning wrote:

>Since lots of structs have padding all over the place, what you
>describe is clearly a Checker bug.  It doesn't make sense to me to
>change one of the many structs in one of the many programs to work
>around this bug.

That's not the only reason for this change, although this prompted it.

If you look at finclude() in cccp.c, you'll find this code:

static void
finclude (f, inc, op, system_header_p, dirptr)
     int f;
     struct include_file *inc;
     FILE_BUF *op;
     int system_header_p;

[...]

  FILE_BUF *fp;

  fp->system_header_p = system_header_p;

Now, system_header_p has to be an int beause there might be a K&R
compiler which, without prototypes, promotes a char to an int.  OTOH,
fp->system_header_p is a char.  Does this add to the clarity of the
code?

In another place, system_header_p is used not as a boolean, but as a
small int, i.e. in do_line():

static int
do_line (buf, limit, op, keyword)
     U_CHAR *buf, *limit;
     FILE_BUF *op;

[...]

      if (*bp == '1')
        file_change = enter_file;
      else if (*bp == '2')
        file_change = leave_file;
      else if (*bp == '3')
        ip->system_header_p = 1;
      else if (*bp == '4')
        ip->system_header_p = 2;

To use a char in such circumstances appears to me very, very unclean.
-- 
Thomas Koenig, Thomas.Koenig@ciw.uni-karlsruhe.de, ig25@dkauni2.bitnet.
The joy of engineering is to find a straight line on a double
logarithmic diagram.

Re: Mini-patch for cccp.c

[Chip Salzenberg]

According to Thomas Koenig:
> To use a char in such circumstances appears to me very, very unclean.

Your intuition needs training, then.  In C, char is just tinyint.
Pretending otherwise is foolish.
-- 
Chip Salzenberg          - a.k.a. -           <chip@pobox.com>
        "He's Mr. Big of 'Big And Tall' fame."  // MST3K

Re: Mini-patch for cccp.c

[Lee Iverson]

In message < 199710011636.MAA12811@rio.atlantic.net > you write:
> According to Thomas Koenig:
> > To use a char in such circumstances appears to me very, very unclean.
> 
> Your intuition needs training, then.  In C, char is just tinyint.
> Pretending otherwise is foolish.

Hmmm...  Perhaps your otherwise razor-sharp intuition needs a little
honing Chip.  As far as bools go, you're right, there's not much to
choose between char and int except for the fact that on some machines
memory loads and stores of individual chars may be significantly
slower than ints.

But one must always remember that char may be either a signed or
unsigned quantity, depending on the implementation. In this case a
char is definitely *not* tinyint!

-------------------------------------------------------------------------------
Lee Iverson     		SRI International
leei@ai.sri.com			333 Ravenswood Ave., Menlo Park CA 94025
http://www.ai.sri.com/~leei/	(650) 859-3307

Re: Mini-patch for cccp.c

[Chip Salzenberg]

According to Lee Iverson:
> But one must always remember that char may be either a signed or
> unsigned quantity, depending on the implementation. In this case a
> char is definitely *not* tinyint!

I should have been more specific.  I know about the signed/unsigned
char issue (I've been around since before you could count on being
allowed to say "unsigned char").  I meant, "tiny integral type".
-- 
Chip Salzenberg          - a.k.a. -           <chip@pobox.com>
        "He's Mr. Big of 'Big And Tall' fame."  // MST3K

Re: Mini-patch for cccp.c

[Jeffrey A Law]

  In message < 199710011904.MAA12461@Canada.AI.SRI.COM >you write:
  > choose between char and int except for the fact that on some machines
  > memory loads and stores of individual chars may be significantly
  > slower than ints.
And on other machines they may be faster.  It's highly target dependent.
jeff

Re: Mini-patch for cccp.c

[John Carr]

>> choose between char and int except for the fact that on some machines
>> memory loads and stores of individual chars may be significantly
>> slower than ints.
> And on other machines they may be faster.  It's highly target dependent.

On what relatively modern (< 5 years old) gcc hosts are chars faster
than ints?  They used to be faster on a few machines without caches,
but now:

	SPARC	signed char slower on Ultra, otherwise equal
	x86	char sometimes slower, never faster
	MIPS	no effect
	alpha	char slower
	PPC	no effect

Re: Mini-patch for cccp.c

[Jeffrey A Law]

  In message < 199710070008.UAA00600@jfc. >you write:

  > On what relatively modern (< 5 years old) gcc hosts are chars faster
  > than ints?  They used to be faster on a few machines without caches,
  > but now:
mn10x00 -- not that these are likely to ever host gcc.

chars can be faster on the H8 too since the H8 has better addressing
modes and bitwise manipulation of char values in memory.  They're
also more likely to fit in the '8bit data areas'.
Jeff

Re: Mini-patch for cccp.c

[Jeffrey A Law]

  In message < 199710011532.QAA30355@mvmap66.ciw.uni-karlsruhe.de >you write:
  > If you look at finclude() in cccp.c, you'll find this code:
  > 
  > static void
  > finclude (f, inc, op, system_header_p, dirptr)
  >      int f;
  >      struct include_file *inc;
  >      FILE_BUF *op;
  >      int system_header_p;
  > 
  > [...]
  > 
  >   FILE_BUF *fp;
  > 
  >   fp->system_header_p = system_header_p;
  > 
  > Now, system_header_p has to be an int beause there might be a K&R
  > compiler which, without prototypes, promotes a char to an int.
  > 
  > fp->system_header_p is a char.  Does this add to the clarity of the
  > code?
Marginally, if at all.

  > In another place, system_header_p is used not as a boolean, but as a
  > small int, i.e. in do_line():
That's still OK.  Using a char as a small int is fine as long as the code
doesn't depend on signedness.

jeff

Re: Mini-patch for cccp.c

[Jeffrey A Law]

  In message < 199710010943.KAA04765@mvmap66.ciw.uni-karlsruhe.de >you write:
  > The reason is clear: sizeof(res) is 8 because of padding, and 'return res'
  > copies 8 bytes onto the caller's stack in two 4-byte words.  checkergcc,
  > which only manipulates the assembler, has no way of knowing that the three
  > extra bytes don't mean anything, and warns about them.
  > 
  > Obviously, this is a Checker artifact.  Checker is immensely useful
  > otherwise (I found the other two bugs I reported recently with it), but
  > its output can be rather overwhelming.  Therefore, I think it may be
  > worth making this particular change which doesn't hurt anything else,
  > AFAIK, just to shut it up in this particular instance.
I disagree that it's worth fixing -- basically we'll be fixing this stuff
until the end of time.  checkergcc should have some other means to figure
this kind of stuff out.

Jeff

Re: Mini-patch for cccp.c

[Thomas Koenig]

I wrote:

[...]

Playing around with checkergcc some more, I found that
expand_to_temp_buffer didn't initialize some of the members of the
FILE_BUF it returns.  Dunno wether this has any grave consequences,
but I think it's a bad idea just on general principles.

Below is a patch against the origninal 970924 cccp.c which fixes this
(it also includes the previous patch).

Tue Sep 30 16:50:55 CEST 1997  Thomas Koenig  (ig25@rz.uni-karlsruhe.de)

	* cccp.c: (struct file_buf) change system_header_p to int
          (expand_to_temp_buffer) initialize all members of obuf

 static int last_error_tick;	   /* Incremented each time we print it.  */
--- cccp.c.orig	Tue Sep 30 14:43:08 1997
+++ cccp.c	Tue Sep 30 16:05:35 1997
@@ -519,7 +519,7 @@
   /* Object to be freed at end of input at this level.  */
   U_CHAR *free_ptr;
   /* True if this is a header file included using <FILENAME>.  */
-  char system_header_p;
+  int system_header_p;
 } instack[INPUT_STACK_MAX];
 
 static int last_error_tick;	   /* Incremented each time we print it.  */
@@ -3549,9 +3549,14 @@
 
   obuf.length = length * 2 + 100; /* Usually enough.  Why be stingy?  */
   obuf.bufp = obuf.buf = (U_CHAR *) xmalloc (obuf.length);
+  obuf.nominal_fname = 0;
+  obuf.inc = 0;
+  obuf.dir = 0;
   obuf.fname = 0;
   obuf.macro = 0;
+  obuf.if_stack = 0;
   obuf.free_ptr = 0;
+  obuf.system_header_p = 0;
 
   CHECK_DEPTH ({return obuf;});
 
-- 
Thomas Koenig, Thomas.Koenig@ciw.uni-karlsruhe.de, ig25@dkauni2.bitnet.
The joy of engineering is to find a straight line on a double
logarithmic diagram.

Re: Mini-patch for cccp.c

[Jeffrey A Law]

  In message < 199709301508.QAA02218@mvmap66.ciw.uni-karlsruhe.de >you write:
  > Below is a patch against the origninal 970924 cccp.c which fixes this
  > (it also includes the previous patch).
  > 
  > Tue Sep 30 16:50:55 CEST 1997  Thomas Koenig  (ig25@rz.uni-karlsruhe.de)
  > 
  > 	* cccp.c: (struct file_buf) change system_header_p to int
  >           (expand_to_temp_buffer) initialize all members of obuf
I installed the second hunk of this patch.

A couple ChangeLog notes.

The colon goes immediately after the first close paren.  ie

	* file (blah): Some change.

End sentences with a period.

The second line of a change should start with a single tab.

I've fixed these problems for the patches I've already installed.

Thanks!
jeff

Re: Mini-patch for cccp.c

[Thomas Koenig]

I wrote:

[...]

Playing around with checkergcc some more, I found that
expand_to_temp_buffer didn't initialize some of the members of the
FILE_BUF it returns.  Dunno wether this has any grave consequences,
but I think it's a bad idea just on general principles.

Below is a patch against the origninal 970924 cccp.c which fixes this
(it also includes the previous patch).

Tue Sep 30 16:50:55 CEST 1997  Thomas Koenig  (ig25@rz.uni-karlsruhe.de)

	* cccp.c: (struct file_buf) change system_header_p to int
          (expand_to_temp_buffer) initialize all members of obuf

 static int last_error_tick;	   /* Incremented each time we print it.  */
--- cccp.c.orig	Tue Sep 30 14:43:08 1997
+++ cccp.c	Tue Sep 30 16:05:35 1997
@@ -519,7 +519,7 @@
   /* Object to be freed at end of input at this level.  */
   U_CHAR *free_ptr;
   /* True if this is a header file included using <FILENAME>.  */
-  char system_header_p;
+  int system_header_p;
 } instack[INPUT_STACK_MAX];

 static int last_error_tick;	   /* Incremented each time we print it.  */
@@ -3549,9 +3549,14 @@

   obuf.length = length * 2 + 100; /* Usually enough.  Why be stingy?  */
   obuf.bufp = obuf.buf = (U_CHAR *) xmalloc (obuf.length);
+  obuf.nominal_fname = 0;
+  obuf.inc = 0;
+  obuf.dir = 0;
   obuf.fname = 0;
   obuf.macro = 0;
+  obuf.if_stack = 0;
   obuf.free_ptr = 0;
+  obuf.system_header_p = 0;

   CHECK_DEPTH ({return obuf;});

--
Thomas Koenig, Thomas.Koenig@ciw.uni-karlsruhe.de, ig25@dkauni2.bitnet.
The joy of engineering is to find a straight line on a double
logarithmic diagram.

Re: Mini-patch for cccp.c

[Peter Seebach]

Chip Salzenberg          - a.k.a. -           <chip@pobox.com> writes:
>According to Thomas Koenig:
>> To use a char in such circumstances appears to me very, very unclean.

>Your intuition needs training, then.  In C, char is just tinyint.
>Pretending otherwise is foolish.

char is actually a very awkward integer type; you can't even be sure whether
it's signed or unsigned, and it has a lot of additional aliasing baggage
and properties.  We do not recommend that it be used for anything other than
aliasing and characters.  *sigh*.

The best way to bool in C9X will probably be 'bool'.  :)

-s

Re: Mini-patch for cccp.c

[Joe Buck]

> char is actually a very awkward integer type; you can't even be sure whether
> it's signed or unsigned, and it has a lot of additional aliasing baggage
> and properties.

However, "unsigned char" is a perfectly safe type to use to represent a
small integer.

Re: Mini-patch for cccp.c

[Chip Salzenberg]

According to Peter Seebach:
> Chip Salzenberg          - a.k.a. -           <chip@pobox.com> writes:
> >According to Thomas Koenig:
> >> To use a char in such circumstances appears to me very, very unclean.
> 
> >Your intuition needs training, then.  In C, char is just tinyint.
> >Pretending otherwise is foolish.
> 
> char is actually a very awkward integer type; you can't even be sure whether
> it's signed or unsigned, and it has a lot of additional aliasing baggage
> and properties.

OK, it's just an awkward inadequately specified tinyint.  But I see no
problem with storing values 0-127 in one.
-- 
Chip Salzenberg          - a.k.a. -           <chip@pobox.com>
        "He's Mr. Big of 'Big And Tall' fame."  // MST3K

Re: Mini-patch for cccp.c

[Kaveh R. Ghazi]

 > Tue Sep 30 16:50:55 CEST 1997  Thomas Koenig  (ig25@rz.uni-karlsruhe.de)
 > 
 > 	* cccp.c: 
 >           (expand_to_temp_buffer) initialize all members of obuf

	Having to remember to init structure members when new ones are
added is prone to error.  Isn't it better to do bzero(&foo, sizeof(foo))
and then set any non zero members?
--
Kaveh R. Ghazi				Project Manager
ghazi@caip.rutgers.edu			ICon CMT Corp.

Re: Mini-patch for cccp.c

[Andreas Schwab]

Kaveh R Ghazi <ghazi@caip.rutgers.edu> writes:

|>> Tue Sep 30 16:50:55 CEST 1997  Thomas Koenig  (ig25@rz.uni-karlsruhe.de)
|>> 
|>> 	* cccp.c: 
|>>           (expand_to_temp_buffer) initialize all members of obuf

|> 	Having to remember to init structure members when new ones are
|> added is prone to error.  Isn't it better to do bzero(&foo, sizeof(foo))
|> and then set any non zero members?

Pedantically this won't work for pointers in the structure, ie. they still
have to be initialized explicitly.  Or initialize it with a prototype
static variable that is implicitly initialized.

-- 
Andreas Schwab                                      "And now for something
schwab@issan.informatik.uni-dortmund.de              completely different"

Re: Mini-patch for cccp.c

[Kamil Iskra]

On 7 Oct 1997, Andreas Schwab wrote:

> |> 	Having to remember to init structure members when new ones are
> |> added is prone to error.  Isn't it better to do bzero(&foo, sizeof(foo))
> |> and then set any non zero members?
> Pedantically this won't work for pointers in the structure, ie. they still
> have to be initialized explicitly.

Do you mean machines where the machine representation of NULL-addresses
has some bits set? I wonder if GCC supports such machines at all?

Just curious...

/ Kamil Iskra - AMIGA 1200, 68030 50MHz, HDD 1.6 GB, 18 MB RAM \
| iskra@student.uci.agh.edu.pl  kiskra@ernie.icslab.agh.edu.pl |
| http://student.uci.agh.edu.pl/~iskra                         |
\ PGP public key available via Finger or WWW                   /

Re: Mini-patch for cccp.c

[Joern Rennecke]

> > Pedantically this won't work for pointers in the structure, ie. they still
> > have to be initialized explicitly.
> 
> Do you mean machines where the machine representation of NULL-addresses
> has some bits set? I wonder if GCC supports such machines at all?

There are currently a number of places where pointer arrays are initialized
incorrectly - using bzero.  However, that is no reason to spread this bug.

Re: Mini-patch for cccp.c

[Jeffrey A Law]

  In message < 199710070006.UAA23461@caip.rutgers.edu >you write:
  >  > Tue Sep 30 16:50:55 CEST 1997  Thomas Koenig  (ig25@rz.uni-karlsruhe.de)
  >  > 
  >  > 	* cccp.c: 
  >  >           (expand_to_temp_buffer) initialize all members of obuf
  > 
  > 	Having to remember to init structure members when new ones are
  > added is prone to error.  Isn't it better to do bzero(&foo, sizeof(foo))
  > and then set any non zero members?
This has been a hotly debated topic elsewhere (gdb developers).

There's pros and cons for each.  I personally don't prefer one method
over the other.

jeff

Re: Mini-patch for cccp.c

[meissner]

| 
| On 7 Oct 1997, Andreas Schwab wrote:
| 
| > |> 	Having to remember to init structure members when new ones are
| > |> added is prone to error.  Isn't it better to do bzero(&foo, sizeof(foo))
| > |> and then set any non zero members?
| > Pedantically this won't work for pointers in the structure, ie. they still
| > have to be initialized explicitly.
| 
| Do you mean machines where the machine representation of NULL-addresses
| has some bits set? I wonder if GCC supports such machines at all?

Yes there have been machines out there that at least comptemplated making NULL
have non-zero bits (I know I at least flirted with it on the Data General
MV/Eclipse computers but never did, the PR1ME, Livermore S1, and Symbolics
computers come to mind as machines that had a non-zero NULL pointer).  No, GCC
would never run on those machines.

GCC makes a number of simplifying assumptions about the machines it generates
code for, such as all pointers are the same size and format (though I recently
did a port for a machine that had two different formats, and I had to not
implement certain things like trampolines and labels as values because of it --
it wasn't pretty).