Re: Silently checking whether diagnostics would occur

Re: Silently checking whether diagnostics would occur

[Kaveh R. Ghazi]

 > From: "Joseph S. Myers" <jsm28@cam.ac.uk>
 > > 
 > > Although I appreciate now the reasons against using a global var, I
 > > also see maintenance problems down the road ensuring that one always
 > > uses the above idiom when adding new format checks.
 > > 
 > > Any ideas on making it more automatic and less dependent on good
 > > behavior?  Since `warning' is a varargs function, for portability
 > > reasons I can't make the above snippet a macro.
 > 
 > I'd suggest making this into a format_warning function (taking the int *
 > and the normal warning() parameters).

Wouldn't I then have to use va_arg to process arguments and duplicate
the innards of the `warning' function?  Eh, not too bad I guess.


 >   Remember the calls from tfaff() and
 > maybe_read_dollar_number() and finish_dollar_format_checking() and adjust
 > the parameters of these functions to include the int *, and note that
 > format checking is not presently reentrant because of static variables
 > used in the $ format checking (I've added reentrancy to my TODO list).

Thanks for the heads-up about those ancillary functions, I forgot
about them!  Oh joy, another 10 warning spots on top of the 50 I
already have to fix. :-) BTW, I'll probably nuke tfaff, its a silly
one liner.




 > In addition, -pedantic should not affect code generation, so before
 > calling the format checking the value of `pedantic' should be saved and it
 > should be set to some known value (I suggest 1).

Good point, thanks for the feedback.

		--Kaveh
--
Kaveh R. Ghazi			Engagement Manager / Project Services
ghazi@caip.rutgers.edu		Qwest Internet Solutions

Re: Silently checking whether diagnostics would occur

[Kaveh R. Ghazi]

 > From: "Joseph S. Myers" <jsm28@cam.ac.uk>
 > 
 > On Thu, 14 Sep 2000, Kaveh R. Ghazi wrote:
 > 
 > > One nit with passing in a function is that attribute printf doesn't
 > > work on function pointers IIRC, so all the calls to the warning
 > > function ptr would lose format checks.  (Though I suppose that could
 > > be fixed.)  Sigh, that would have been cleaner since I could do
 > 
 > This should probably be considered a bug (which should be fixed) that
 > format attributes don't attach properly to the function type.

I agree, would you like to fix it? :-)

I looked at this a year ago, but lost interest.  If you want to take a
stab by all means please do.

The first problem is that in c-common.c:decl_attributes, the A_FORMAT
case specifically errors when it receives something that is not a
FUNCTION_DECL.  So you need to also handle POINTER_TYPEs (for function
pointers) or AGGREGATE_TYPEs (for arrays or structs containing
function pointers), as well descending subexpressions of these, until
you get to the eventual FUNCTION_DECL if you want to fix this.

That's just to get the attribute to be accepted.  You also have to
muck with c-typeck.c:build_function_call to actually do the
appropriate checks.

 > 
 > Marc Espie's __attribute__((__nonnull__)) patch allows (I think) for one
 > function to have multiple printf/scanf attributes (which could make sense
 > if all but as must one are vprintf/vscanf-like), but I don't know if it
 > fixes this problem.

I haven't seen his patch, so I can't say.

		--Kaveh
--
Kaveh R. Ghazi			Engagement Manager / Project Services
ghazi@caip.rutgers.edu		Qwest Internet Solutions

Re: Silently checking whether diagnostics would occur

[Joern Rennecke]

> One nit with passing in a function is that attribute printf doesn't
> work on function pointers IIRC, so all the calls to the warning
> function ptr would lose format checks.  (Though I suppose that could
> be fixed.)  Sigh, that would have been cleaner since I could do
> "#pragma poison warning" to make sure the function pointer parameter
> was always used in preference to `warning'.

You could have an inline function that is used to dereference the function
pointer.

INLINE void dispatch_warning PARAMS ((void (*fun)(), const char *fmt, ...)) ATTRIBUTE_PRINTF_2;

Re: Silently checking whether diagnostics would occur

[Joseph S. Myers]

On Thu, 14 Sep 2000, Kaveh R. Ghazi wrote:

> One nit with passing in a function is that attribute printf doesn't
> work on function pointers IIRC, so all the calls to the warning
> function ptr would lose format checks.  (Though I suppose that could
> be fixed.)  Sigh, that would have been cleaner since I could do

This should probably be considered a bug (which should be fixed) that
format attributes don't attach properly to the function type.

Marc Espie's __attribute__((__nonnull__)) patch allows (I think) for one
function to have multiple printf/scanf attributes (which could make sense
if all but as must one are vprintf/vscanf-like), but I don't know if it
fixes this problem.

> I'll look into passing in an int*, then I guess all calls to warning()
> would instead be:
> 
>  > if (foo)
>  >   {*foo = 1; return;}
>  > else
>  >   warning ("blah blah", arg1, arg2);
> 
> Although I appreciate now the reasons against using a global var, I
> also see maintenance problems down the road ensuring that one always
> uses the above idiom when adding new format checks.
> 
> Any ideas on making it more automatic and less dependent on good
> behavior?  Since `warning' is a varargs function, for portability
> reasons I can't make the above snippet a macro.

I'd suggest making this into a format_warning function (taking the int *
and the normal warning() parameters).  Remember the calls from tfaff() and
maybe_read_dollar_number() and finish_dollar_format_checking() and adjust
the parameters of these functions to include the int *, and note that
format checking is not presently reentrant because of static variables
used in the $ format checking (I've added reentrancy to my TODO list).

In addition, -pedantic should not affect code generation, so before
calling the format checking the value of `pedantic' should be saved and it
should be set to some known value (I suggest 1).

-- 
Joseph S. Myers
jsm28@cam.ac.uk

Re: Silently checking whether diagnostics would occur

[Michael Meissner]

On Thu, Sep 14, 2000 at 11:30:49AM -0400, Kaveh R. Ghazi wrote:
>  > From: Michael Meissner <meissner@cygnus.com>
>  > 
>  > Rather than returning an error value, you could have the parameter be
>  > an int pointer, and if the pointer is NULL, otherwise set the pointer
>  > to non-zero and return.  Another way to write the function is to pass
>  > a function that writes the error messages, and for the case you don't
>  > want the error messages, have it record in a static about being called
>  > and return, and change the other callers to pass error or what have
>  > you for the function.
> 
> One nit with passing in a function is that attribute printf doesn't
> work on function pointers IIRC, so all the calls to the warning
> function ptr would lose format checks.  (Though I suppose that could
> be fixed.)  Sigh, that would have been cleaner since I could do
> "#pragma poison warning" to make sure the function pointer parameter
> was always used in preference to `warning'.

Use a special purpose warning function then that has fixed arguments, instead
of a general stdarg function.

-- 
Michael Meissner, Red Hat, Inc.
PMB 198, 174 Littleton Road #3, Westford, Massachusetts 01886, USA
Work:	  meissner@redhat.com		phone: +1 978-486-9304
Non-work: meissner@spectacle-pond.org	fax:   +1 978-692-4482

Re: Silently checking whether diagnostics would occur

[Kaveh R. Ghazi]

 > From: Michael Meissner <meissner@cygnus.com>
 > 
 > Rather than returning an error value, you could have the parameter be
 > an int pointer, and if the pointer is NULL, otherwise set the pointer
 > to non-zero and return.  Another way to write the function is to pass
 > a function that writes the error messages, and for the case you don't
 > want the error messages, have it record in a static about being called
 > and return, and change the other callers to pass error or what have
 > you for the function.

One nit with passing in a function is that attribute printf doesn't
work on function pointers IIRC, so all the calls to the warning
function ptr would lose format checks.  (Though I suppose that could
be fixed.)  Sigh, that would have been cleaner since I could do
"#pragma poison warning" to make sure the function pointer parameter
was always used in preference to `warning'.


I'll look into passing in an int*, then I guess all calls to warning()
would instead be:

 > if (foo)
 >   {*foo = 1; return;}
 > else
 >   warning ("blah blah", arg1, arg2);

Although I appreciate now the reasons against using a global var, I
also see maintenance problems down the road ensuring that one always
uses the above idiom when adding new format checks.

Any ideas on making it more automatic and less dependent on good
behavior?  Since `warning' is a varargs function, for portability
reasons I can't make the above snippet a macro.

		Thanks,
		--Kaveh
--
Kaveh R. Ghazi			Engagement Manager / Project Services
ghazi@caip.rutgers.edu		Qwest Internet Solutions

Re: Silently checking whether diagnostics would occur

[Kaveh R. Ghazi]

 > From: Joern Rennecke <amylaar@cygnus.co.uk>
 > 
 > > So rather than do a complete restructuring of the format checking
 > > function, I though it would be much easier and cleaner to be able to
 > > call the checks conceptually like this:
 > > 
 > >  > check_diagnostics_silently = 1;  /* new global vars */
 > >  > diagnostic_occurred = 0;
 > 
 > Simpler to implement the change, but cleaner?
 > 
 > I can see it now.  Somewhere down the line, someone uses this
 > format checking in another utility function.  That function gets
 > called by another utility function under certain circumstances.
 > That function is then used by some piece of format-checking code.
 > 
 > After a few month of debugging, someone figures out why certain programs
 > just won't show any format warnings beyond some point even if there are
 > more problems left, and invents a stack for these global variables in
 > a static array.
 > 
 > The array overflows for another set of programs.  It is replaced with
 > a linked list of malloced structures.
 > 
 > Some time later, other programs are seen to cause the compiler to gobble
 > up all available memory the memory while parsing, making the host grind
 > to a halt, because the mallocing of structures causes a memory leak
 > when doing error recovery.
 > So we sprinkle a few strategically garbage collector calls into the
 > parser.
 > 
 > However, now some other data structures that should be live throughout the
 > parsing stage will be freed prematurely...

Okay, you've convinced me. :-)

I'll do it another way.

--
Kaveh R. Ghazi			Engagement Manager / Project Services
ghazi@caip.rutgers.edu		Qwest Internet Solutions

Re: Silently checking whether diagnostics would occur

[Michael Meissner]

On Wed, Sep 13, 2000 at 05:33:36PM -0400, Kaveh R. Ghazi wrote:
> If you mean rewriting check_format_info, that requires a lot of
> obtrusive surgery on the code.  As I mentioned above, that function is
> not structured to return a status either as a return-value, or via one
> of its parameters were I to add one.  There are about 50 places in
> that function which warn about format specifier problems.  I would
> have to add bits at each location to mark that a warning occured and
> return that bit at all of the 15 spots from which the function can
> return.  Anyone writing new format warnings within it would have to
> remember to do the same.  Its really hairy.  My alternative is about
> five lines of code.
>
> What is the specific problem you wish to avoid that global vars cause?
> Is there some other way to design this to meet your goals?  What if
> the setting were a static variable in diagnostic.c with functions
> exported to set/unset it?

I just don't like global variables changing the behavior of functions,
particularly error reporting functions.  It is gross.  The problem is once you
have this global, somebody else is going to use it, and sooner or later these
two passes (or a backend and a MI pass) will use the variable in such a
non-obvious way.

Rather than returning an error value, you could have the parameter be an int
pointer, and if the pointer is NULL, otherwise set the pointer to non-zero and
return.  Another way to write the function is to pass a function that writes
the error messages, and for the case you don't want the error messages, have it
record in a static about being called and return, and change the other callers
to pass error or what have you for the function.

-- 
Michael Meissner, Red Hat, Inc.
PMB 198, 174 Littleton Road #3, Westford, Massachusetts 01886, USA
Work:	  meissner@redhat.com		phone: +1 978-486-9304
Non-work: meissner@spectacle-pond.org	fax:   +1 978-692-4482

Re: Silently checking whether diagnostics would occur

[Mike Stump]

> From: Joern Rennecke <amylaar@cygnus.co.uk>
> To: "Kaveh R. Ghazi" <ghazi@caip.rutgers.edu>
> Date: Thu, 14 Sep 2000 00:38:09 +0100 (BST)

> I can see it now.  Somewhere down the line, someone uses this
> format checking in another utility function.  That function gets
> called by another utility function under certain circumstances.
> That function is then used by some piece of format-checking code.

> [ ... ]

You show an amazing predictive abilities...

:-)

For those not familiar with the compiler internals, this is a good
description of the variables named current_function_*, and the ones
that aren't, that that had been in the compiler and how the code
changed over time.  That code is slightly cleaner today, but still
suffers in various ways.

Re: Silently checking whether diagnostics would occur

[Joern Rennecke]

> So rather than do a complete restructuring of the format checking
> function, I though it would be much easier and cleaner to be able to
> call the checks conceptually like this:
> 
>  > check_diagnostics_silently = 1;  /* new global vars */
>  > diagnostic_occurred = 0;

Simpler to implement the change, but cleaner?

I can see it now.  Somewhere down the line, someone uses this
format checking in another utility function.  That function gets
called by another utility function under certain circumstances.
That function is then used by some piece of format-checking code.

After a few month of debugging, someone figures out why certain programs
just won't show any format warnings beyond some point even if there are
more problems left, and invents a stack for these global variables in
a static array.

The array overflows for another set of programs.  It is replaced with
a linked list of malloced structures.

Some time later, other programs are seen to cause the compiler to gobble
up all available memory the memory while parsing, making the host grind
to a halt, because the mallocing of structures causes a memory leak
when doing error recovery.
So we sprinkle a few strategically garbage collector calls into the
parser.

However, now some other data structures that should be live throughout the
parsing stage will be freed prematurely...

Re: Silently checking whether diagnostics would occur

[Kaveh R. Ghazi]

 > From: Michael Meissner <meissner@cygnus.com>
 > 
 > On Wed, Sep 13, 2000 at 11:50:02AM -0400, Kaveh R. Ghazi wrote:
 > > Hi Gabriel,
 > > 
 > > While writing a __builtin_printf expander, I found that it would be
 > > useful to be able to call the printf format checking routines silently
 > > from builtins.c to see if the printf call being expanded passed format
 > > checks before I try to optimize it.  (This needs to happen regardless
 > > of, and independent of, whether the user specifies -Wformat.)  The
 > > current format checking routines are setup to emit warnings at random
 > > points, not to return a handy status code.  (See check_format_info in
 > > c-common.c.)
 > > 
 > > So rather than do a complete restructuring of the format checking
 > > function, I though it would be much easier and cleaner to be able to
 > > call the checks conceptually like this:
 > > 
 > >  > check_diagnostics_silently = 1;  /* new global vars */
 > >  > diagnostic_occurred = 0;
 > >  > 
 > >  > <do something which might warn>
 > >  > 
 > >  > if (diagnostic_occurred) <react>
 > >  > check_diagnostics_silently = 0;
 > > 
 > > and modify diagnostic.c:count_error to do this before anything else:
 > > 
 > >  >   if (check_diagnostics_silently)
 > >  >     {
 > >  >       diagnostic_occurred = 1;
 > >  >       return 0;
 > >  >     }
 > 
 > I really, really do not like adding global variables for this
 > purpose (yes, I'm probably guily of just such behavior myself).  I
 > would rather the checking be abstracted into a function with an
 > argument that says whether to issue the warning or not, and a
 > return value that indicates whether an error would have occurred.
 > Michael Meissner, Red Hat, Inc.

If you mean rewriting check_format_info, that requires a lot of
obtrusive surgery on the code.  As I mentioned above, that function is
not structured to return a status either as a return-value, or via one
of its parameters were I to add one.  There are about 50 places in
that function which warn about format specifier problems.  I would
have to add bits at each location to mark that a warning occured and
return that bit at all of the 15 spots from which the function can
return.  Anyone writing new format warnings within it would have to
remember to do the same.  Its really hairy.  My alternative is about
five lines of code.

What is the specific problem you wish to avoid that global vars cause?
Is there some other way to design this to meet your goals?  What if
the setting were a static variable in diagnostic.c with functions
exported to set/unset it?

		--Kaveh
--
Kaveh R. Ghazi			Engagement Manager / Project Services
ghazi@caip.rutgers.edu		Qwest Internet Solutions

Re: Silently checking whether diagnostics would occur

[Michael Meissner]

On Wed, Sep 13, 2000 at 11:50:02AM -0400, Kaveh R. Ghazi wrote:
> Hi Gabriel,
> 
> While writing a __builtin_printf expander, I found that it would be
> useful to be able to call the printf format checking routines silently
> from builtins.c to see if the printf call being expanded passed format
> checks before I try to optimize it.  (This needs to happen regardless
> of, and independent of, whether the user specifies -Wformat.)  The
> current format checking routines are setup to emit warnings at random
> points, not to return a handy status code.  (See check_format_info in
> c-common.c.)
> 
> So rather than do a complete restructuring of the format checking
> function, I though it would be much easier and cleaner to be able to
> call the checks conceptually like this:
> 
>  > check_diagnostics_silently = 1;  /* new global vars */
>  > diagnostic_occurred = 0;
>  > 
>  > <do something which might warn>
>  > 
>  > if (diagnostic_occurred) <react>
>  > check_diagnostics_silently = 0;
> 
> and modify diagnostic.c:count_error to do this before anything else:
> 
>  >   if (check_diagnostics_silently)
>  >     {
>  >       diagnostic_occurred = 1;
>  >       return 0;
>  >     }

I really, really do not like adding global variables for this purpose (yes, I'm
probably guily of just such behavior myself).  I would rather the checking be
abstracted into a function with an argument that says whether to issue the
warning or not, and a return value that indicates whether an error would have
occurred.

-- 
Michael Meissner, Red Hat, Inc.
PMB 198, 174 Littleton Road #3, Westford, Massachusetts 01886, USA
Work:	  meissner@redhat.com		phone: +1 978-486-9304
Non-work: meissner@spectacle-pond.org	fax:   +1 978-692-4482

Silently checking whether diagnostics would occur

[Kaveh R. Ghazi]

Hi Gabriel,

While writing a __builtin_printf expander, I found that it would be
useful to be able to call the printf format checking routines silently
from builtins.c to see if the printf call being expanded passed format
checks before I try to optimize it.  (This needs to happen regardless
of, and independent of, whether the user specifies -Wformat.)  The
current format checking routines are setup to emit warnings at random
points, not to return a handy status code.  (See check_format_info in
c-common.c.)

So rather than do a complete restructuring of the format checking
function, I though it would be much easier and cleaner to be able to
call the checks conceptually like this:

 > check_diagnostics_silently = 1;  /* new global vars */
 > diagnostic_occurred = 0;
 > 
 > <do something which might warn>
 > 
 > if (diagnostic_occurred) <react>
 > check_diagnostics_silently = 0;

and modify diagnostic.c:count_error to do this before anything else:

 >   if (check_diagnostics_silently)
 >     {
 >       diagnostic_occurred = 1;
 >       return 0;
 >     }

This would fool gcc into keeping quiet about the warnings while still
letting me know whether some kind of diagnostic would have issued.
Note the current controls like inhibit_warnings don't seem to do what
quite I want.  It would silence the warnings without me being able to
tell that a warning would have occured.  I also don't want to worry
about side effects modifying gcc's internal state, e.g. errorcount, or
whether the user specified -Werror.  My approach seems to solve all of
these issues.

First of all, I wanted to know if there was already something in there
that I missed which allows me to do this.  Second, if nothing exists
is this approach something you feel would be acceptable into
diagnostic.c?  I'll provide a patch if you react positively.

		Thanks,
		--Kaveh
--
Kaveh R. Ghazi			Engagement Manager / Project Services
ghazi@caip.rutgers.edu		Qwest Internet Solutions