Re: Checking patch

Re: Checking patch

[Jeffrey A Law]

  In message <199805022201.AAA00799@mira.isdn.cs.tu-berlin.de>you write:
  > Hi Jeff,
  > 
  > This is the final(?) version of the checking patch. It helps detecting
  > bugs in gcc when --enable-checking is given to configure, and has no
  > effect when it is not given.
  > 
  > When enabled, gcc builds and passes most of the testsuite. g++ dies
  > when compiling libgcc2. There is one invalid check in it:
  > CST_OR_CONSTRUCTOR_CHECK should really be CST_CHECK only, and the
  > processing of CONSTRUCTORs should be changed.
  > 
  > I expect another revision of this code once we are starting to fix the
  > bugs in g++.
  > 
  > If you reject this patch, please let me know why.
Cool!

First note -- I don't see a copyright assignment for gcc on record
with the FSF.   I see you've submitted an assignment for libg++, any
objection to also submitting one for gcc (I think this change is
probably large enough to warrant an assignment).

Second note -- this should have been sent to the list :-)  That
way everyone gets to look at it.  Some folks can even start playing
with it while we work out the final details.  I don't remember if
an earlier version went to the list or not.

+
+ #tree-check.h does not depend on gencheck, so that gencheck will
+ #is not built until tree.def changes
+ $(srcdir)/tree-check.h:       tree.def gencheck.c
+       $(MAKE) gencheck
+       gencheck >$(srcdir)/tree-check.h
+
+ gencheck: gencheck.c

Seems to me we should handle this in a manner similar to the
other gen* programs.  Is there some particular reason you can't
use something like this?

tree-check.h: s-check ; @true
s-check : tree.def gencheck $(srcdir)/move-if-change
        ./gencheck tree.def > tmp-check.h
        $(srcdir)/move-if-change tmp-check.h tree-check.h
        touch s-check

gencheck : gencheck.o tree.h tree.def
        $(HOST_CC) $(HOST_CFLAGS) $(HOST_LDFLAGS) -o $@ \
          gencheck.o $(HOST_LIBS)

We'll probably need to make minor tweaks to gencheck.c to deal
with system.h, toplev.h and similar stuff.  I haven't sat down
and looked at the latest round of changes, but given the simplicity
of gencheck is should be pretty trivial to bring it up to standards
re: system.h.

Most of the other problems are formatting issues.

+ tree_check(node, code, file, line, nofatal)

Always put a space before an open paren, except when defining
a macro with arguments or when it's immediately preceeded by
another open paren.  They're missing in many places, particularly
in function definitions.  They're also missing in macro bodies.

Always put a space after a comma.  Here's an example:

+ #define DO_CHECK(FUNC,t,param)   FUNC(t,param,__FILE__,__LINE__,0)

Should instead be:

+ #define DO_CHECK(FUNC, t, param)   FUNC (t, param, __FILE__, __LINE__, 0)


Most of your macros have this problem.

I see some things that aren't checked (EXPR_WFL_* stuff for example);
is there some reason why?

Why do we have the *_CHECK1 variants?

gencheck.c uses an ANSI concatenation character (#).  GCC must be
compilable by both ANSI and non-ANSI compilers, so that has to
be written to support '#' concat and the old /**/ concat method.
We may already have some checks/macros to do this.  It might be
worth a quick browse through the sources to check.

You should put whitespace around most binary operators.  ie

i=0;

should be

i = 0;

or 

i=j+2

should be
i = j + 2

I didn't see many instances of this problem.  The only one I
remember specifically is the for loop in gencheck.c.


Overall I think it's an excellent patch.  Most of the stuff I
mentioned should be relatively simple to fix.  The biggest hurdle
I see is getting a gcc assignment on file.

jeff

Re: Checking patch

[Jim Wilson]

	Second note -- this should have been sent to the list :-)

It was after I made the same request.  Apparently the first attempt bounced
because the message was too large, and I think that is why he sent it directly
to you.  The second attempt included the patch uuencoded, and succeeded.
I also think this looks like a useful feature, but hadn't had a chance to
look at the patch yet.  We are certainly getting some useful bug reports
out of it.

	I see some things that aren't checked (EXPR_WFL_* stuff for example);
	is there some reason why?

Perhaps parallel development.  The EXPR_WFL_* stuff is relatively recent.

Jim

Re: Checking patch

[Martin von Loewis]

> First note -- I don't see a copyright assignment for gcc on record
> with the FSF.   I see you've submitted an assignment for libg++, any
> objection to also submitting one for gcc (I think this change is
> probably large enough to warrant an assignment).

The one you have should be really for g++, as I was not sure whether
different assignments are needed for g++ and gcc. Anyway, I'll sent
one for gcc tomorrow, which will need the usual delay for
intercontinental snail mail.

> Second note -- this should have been sent to the list :-)

I'll sent an updated copy RSN.

[gencheck/tree-check rules]

> Seems to me we should handle this in a manner similar to the
> other gen* programs.  Is there some particular reason you can't
> use something like this?

The difference is that tree-check.h is not host-dependent. So there is
no need to regenerate it on the target. Instead, I'd like to see it
distributed. With your scheme, it seems that gencheck is always built
on the host, as s-check is not there.

If always building tree-check.h (which is included by tree.h) is not a
problem, a time-stamping scheme would certainly work.

> Most of the other problems are formatting issues.

I hope I fixed them all.

> I see some things that aren't checked (EXPR_WFL_* stuff for example);
> is there some reason why?

Checks are only in macros that perform cast-style operations.
For example,
#define EXPR_WFL_NODE(NODE) TREE_OPERAND((NODE), 0)

There is no cast in this macro (and neither an access to a union
branch). There is such an access in TREE_OPERAND, which already
contains EXPR_CHECK.

Also, I believe no checks go in accesses to .common. For example,

#define EXPR_WFL_FILENAME(NODE) (IDENTIFIER_POINTER ((NODE)->common.chain))

will work on any node. Of course, common.chain better contains an
IDENTIFIER, which is checked inside IDENTIFIER_POINTER.

There may be cases were accesses to common fields still assume
specific nodes. This might be arguable, as for TREE_CHAIN, so I left
them out. There might also checks for specific branches, e.g.
DECL_INITIAL is defined for FUNCTION_DECL, VAR_DECL, and PARM_DECL.
However, cc1plus overloads it in various ways for other _DECL nodes.

I'd like to sort these non-obvious violations in a second round.

> Why do we have the *_CHECK1 variants?

These return NULL_TREE on failure instead of calling fatal. This
is useful for combining checks.

> gencheck.c uses an ANSI concatenation character (#).

It doesn't really need concatenation, but stringification, to built
"VAR_DECL" when given VAR_DECL. Is this also an ANSI-only feature,
and if so, what is the work-around? Would it be acceptable in gcc
if tree-check.h is never rebuilt by users of non-ANSI compilers?

Martin

Re: Checking patch

[Andreas Schwab]

Martin von Loewis <martin@mira.isdn.cs.tu-berlin.de> writes:

|>> gencheck.c uses an ANSI concatenation character (#).

|> It doesn't really need concatenation, but stringification, to built
|> "VAR_DECL" when given VAR_DECL. Is this also an ANSI-only feature,
|> and if so, what is the work-around?

#ifndef __STDC__
#define STR(x) "x"
#endif

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

Re: Checking patch

[Jeffrey A Law]

  In message < 199805071939.VAA02308@mira.isdn.cs.tu-berlin.de >you write:
  > > First note -- I don't see a copyright assignment for gcc on record
  > > with the FSF.   I see you've submitted an assignment for libg++, any
  > > objection to also submitting one for gcc (I think this change is
  > > probably large enough to warrant an assignment).
  > 
  > The one you have should be really for g++, as I was not sure whether
  > different assignments are needed for g++ and gcc. Anyway, I'll sent
  > one for gcc tomorrow, which will need the usual delay for
  > intercontinental snail mail.
Well, it's recorded as "libg++", which is different from g++ or gcc.

  > > Second note -- this should have been sent to the list :-)
  > 
  > I'll sent an updated copy RSN.
Based on Jim's message and the fact that I found a couple more
copies in my pendingpatches folder, it sounds like you have sent
versions to the list.  Sorry for implying that you hadn't.  The
joys of having a thousand+ messages in my inbox when I returned
from vacation. :-)


  > [gencheck/tree-check rules]
  > 
  > > Seems to me we should handle this in a manner similar to the
  > > other gen* programs.  Is there some particular reason you can't
  > > use something like this?
  > 
  > The difference is that tree-check.h is not host-dependent. So there is
  > no need to regenerate it on the target. Instead, I'd like to see it
  > distributed. With your scheme, it seems that gencheck is always built
  > on the host, as s-check is not there.
We want to generally avoid distributing generated files, except for
a few special cases.  It's easier and less error prone to go ahead
and build it.

  > Checks are only in macros that perform cast-style operations.
  > For example,
  > #define EXPR_WFL_NODE(NODE) TREE_OPERAND((NODE), 0)
Ah.  OK.

  > I'd like to sort these non-obvious violations in a second round.
Sounds reasoanble.  Iteration to the "perfect solution" is a good
thing :-)


  > > Why do we have the *_CHECK1 variants?
  > 
  > These return NULL_TREE on failure instead of calling fatal. This
  > is useful for combining checks.
I'd almost prefer to just call abort on the first inconsistency
found.  Is there a reason not to do that?


  > > gencheck.c uses an ANSI concatenation character (#).
  > 
  > It doesn't really need concatenation, but stringification, to built
  > "VAR_DECL" when given VAR_DECL. Is this also an ANSI-only feature,
  > and if so, what is the work-around? Would it be acceptable in gcc
  > if tree-check.h is never rebuilt by users of non-ANSI compilers?
Sorry.  Second time in 3 months I've see '#' and said concatentation.

I believe we have some autoconf code to detect a working stringify
operator.  So you just have to conditionalize your code based on
the existence of working stringify.  See gengenrtl.c



jeff

Re: Checking patch

[Kaveh R. Ghazi]

 > From: Jeffrey A Law <law@cygnus.com>
 > 
 >   > > gencheck.c uses an ANSI concatenation character (#).
 >   > 
 >   > It doesn't really need concatenation, but stringification, to built
 >   > "VAR_DECL" when given VAR_DECL. Is this also an ANSI-only feature,
 >   > and if so, what is the work-around? Would it be acceptable in gcc
 >   > if tree-check.h is never rebuilt by users of non-ANSI compilers?
 > Sorry.  Second time in 3 months I've see '#' and said concatentation.
 > 
 > I believe we have some autoconf code to detect a working stringify
 > operator.  So you just have to conditionalize your code based on
 > the existence of working stringify.  See gengenrtl.c
 > jeff


	Actually, I'd like to take the opportunity to consolidate
stringify support into system.h, then we can just tell people to use
the STRINGIFY() macro.  (I've found two places which use stringify and
this would make a third.)  So, is this patch okay to install?

		--Kaveh




Fri May  8 11:00:35 1998  Kaveh R. Ghazi  <ghazi@caip.rutgers.edu>

	* system.h: Define the STRINGIFY() macro here.
	* protoize.c: Not here.
	* gengenrtl.c (DEF_RTL_EXPR): Use the STRINGIFY() macro.


diff -rup orig/egcs-CVS19980508/gcc/gengenrtl.c egcs-CVS19980508/gcc/gengenrtl.c
--- orig/egcs-CVS19980508/gcc/gengenrtl.c	Fri May  8 09:58:43 1998
+++ egcs-CVS19980508/gcc/gengenrtl.c	Fri May  8 10:20:04 1998
@@ -35,11 +35,7 @@ struct rtx_definition 
   const char *enumname, *name, *format;
 };
 
-#if defined(HAVE_CPP_STRINGIFY) || (defined(__GNUC__) && defined(__STDC__))
-#define DEF_RTL_EXPR(ENUM, NAME, FORMAT, CLASS) { # ENUM, NAME, FORMAT },
-#else
-#define DEF_RTL_EXPR(ENUM, NAME, FORMAT, CLASS) { "ENUM", NAME, FORMAT },
-#endif
+#define DEF_RTL_EXPR(ENUM, NAME, FORMAT, CLASS) { STRINGIFY(ENUM), NAME, FORMAT },
 
 struct rtx_definition defs[] = 
 {  
diff -rup orig/egcs-CVS19980508/gcc/protoize.c egcs-CVS19980508/gcc/protoize.c
--- orig/egcs-CVS19980508/gcc/protoize.c	Fri May  8 09:58:56 1998
+++ egcs-CVS19980508/gcc/protoize.c	Fri May  8 10:18:39 1998
@@ -201,14 +201,6 @@ extern size_t   strlen ()
 
 #define NONCONST
 
-/* Define a STRINGIFY macro that's right for ANSI or traditional C.  */
-
-#if defined(HAVE_CPP_STRINGIFY) || (defined(__GNUC__) && defined(__STDC__))
-#define STRINGIFY(STRING) #STRING
-#else
-#define STRINGIFY(STRING) "STRING"
-#endif
-
 /* Define a default place to find the SYSCALLS.X file.  */
 
 #ifndef STD_PROTO_DIR
diff -rup orig/egcs-CVS19980508/gcc/system.h egcs-CVS19980508/gcc/system.h
--- orig/egcs-CVS19980508/gcc/system.h	Fri May  8 09:59:06 1998
+++ egcs-CVS19980508/gcc/system.h	Fri May  8 10:17:48 1998
@@ -243,4 +243,16 @@
 #endif /* USE_SYSTEM_ABORT */
 #endif /* !abort */
 
+
+/* Define a STRINGIFY macro that's right for ANSI or traditional C.
+   HAVE_CPP_STRINGIFY only refers to the stage1 compiler.  Assume that
+   (non -traditional) gcc used in stage2 or later has the # operator. */
+#ifndef STRINGIFY
+# if defined(HAVE_CPP_STRINGIFY) || (defined(__GNUC__) && defined(__STDC__))
+#  define STRINGIFY(STRING) #STRING
+# else /* Traditional C does macro replacement inside quotes. */
+#  define STRINGIFY(STRING) "STRING"
+# endif
+#endif /* ! STRINGIFY */
+
 #endif /* __GCC_SYSTEM_H__ */

--
Kaveh R. Ghazi			Project Manager / Custom Development
ghazi@caip.rutgers.edu		Icon CMT Corp.

Re: Checking patch

[Jeffrey A Law]

  In message < 199805081547.LAA27492@caip.rutgers.edu >you write:
  > 	Actually, I'd like to take the opportunity to consolidate
  > stringify support into system.h, then we can just tell people to use
  > the STRINGIFY() macro.  (I've found two places which use stringify and
  > this would make a third.)  So, is this patch okay to install?
  > 
  > 		--Kaveh
  > 
  > 
  > 
  > 
  > Fri May  8 11:00:35 1998  Kaveh R. Ghazi  <ghazi@caip.rutgers.edu>
  > 
  > 	* system.h: Define the STRINGIFY() macro here.
  > 	* protoize.c: Not here.
  > 	* gengenrtl.c (DEF_RTL_EXPR): Use the STRINGIFY() macro.
Looks reasonable to me.  Please install it.

Thanks!
jeff

Re: Checking patch

[Richard Henderson]

On Fri, May 08, 1998 at 11:47:56AM -0400, Kaveh R. Ghazi wrote:
> +#ifndef STRINGIFY
> +# if defined(HAVE_CPP_STRINGIFY) || (defined(__GNUC__) && defined(__STDC__))
> +#  define STRINGIFY(STRING) #STRING

If you are going to do something generic like this, make it

#define STRINGIFY1(STRING)  #STRING
#define STRINGIFY(STRING)   STRINGIFY1(STRING)

as otherwise, by ISO rules,

#define foo bar
STRINGIFY(foo)

will yield "foo" not "bar" as might be expected.


r~

Re: Checking patch

[Kaveh R. Ghazi]

 > From: Richard Henderson <rth@dot.cygnus.com>
 > 
 > On Fri, May 08, 1998 at 11:47:56AM -0400, Kaveh R. Ghazi wrote:
 > > +#ifndef STRINGIFY
 > > +# if defined(HAVE_CPP_STRINGIFY) || (defined(__GNUC__) && defined(__STDC__))
 > > +#  define STRINGIFY(STRING) #STRING
 > 
 > If you are going to do something generic like this, make it
 > 
 > #define STRINGIFY1(STRING)  #STRING
 > #define STRINGIFY(STRING)   STRINGIFY1(STRING)
 > 
 > as otherwise, by ISO rules,
 > 
 > #define foo bar
 > STRINGIFY(foo)
 > 
 > will yield "foo" not "bar" as might be expected.
 > r~




	That's a good point, however I cannot get the !__STDC__ case to
work the same way.  Eg, if I have the following code:



 > #ifdef __STDC__
 > #define STRINGIFY1(STRING) # STRING
 > #else
 > #define STRINGIFY1(STRING) "STRING"
 > #endif
 > #define STRINGIFY(STRING) STRINGIFY1(STRING)
 > 
 > #define FOO hello world
 > 
 > int main()
 > {
 >   char * f = STRINGIFY(FOO);
 > }

preprocessing this file with "gcc -E foo.c" yields:

 > int main()
 > {
 >   char * f = "hello world"  ;
 > }

whereas using "gcc -traditional -E foo.c" gives:

 > int main()
 > {
 >   char * f = "FOO";
 > }

	So I'm not sure we want to support this behavior unless we can
find a way to make KNRish compilers handle the same construct, else
stage1 may break on hosts like sunos4, hpux9, etc.

	Do you have any ideas on how to solve this?

		Thanks,
		--Kaveh
--
Kaveh R. Ghazi			Project Manager / Custom Development
ghazi@caip.rutgers.edu		Icon CMT Corp.

Re: Checking patch

[Richard Henderson]

On Sun, May 10, 1998 at 09:17:33AM -0400, Kaveh R. Ghazi wrote:
> 	That's a good point, however I cannot get the !__STDC__ case to
> work the same way.

You are right, there's no way to do that with !__STDC__.  I forgot
about that.  So it would not be desirable to do the STRINGIFY1 thing
on the __STDC__ case.


r~

Re: Checking patch

[Kaveh R. Ghazi]

 > From: Richard Henderson <rth@dot.cygnus.com>
 > 
 > On Sun, May 10, 1998 at 09:17:33AM -0400, Kaveh R. Ghazi wrote:
 > > 	That's a good point, however I cannot get the !__STDC__ case to
 > > work the same way.
 > 
 > You are right, there's no way to do that with !__STDC__.  I forgot
 > about that.  So it would not be desirable to do the STRINGIFY1 thing
 > on the __STDC__ case.
 > r~

	Okay.  I installed the patch and added a comment in system.h
saying that macro expansion of aruments passed to STRINGIFY is not
supported. 

		--Kaveh
--
Kaveh R. Ghazi			Project Manager / Custom Development
ghazi@caip.rutgers.edu		Icon CMT Corp.