[google][RFA] add extra text to stack frame warnings (issue4479046)

[google][RFA] add extra text to stack frame warnings (issue4479046)

[Chris Demetriou]

Diego,

I know this is a truly horrible and broken way to do this, but alternatives
(e.g., NLS) don't really work for us.

bootstrapped without new configuration (x86-64 ubuntu lucid, didn't bother
to run tests), bootstrapped with option (x86-64 ubuntu lucid, with full
tests, no regressions).  Manually tested that the resulting warning
looks right, too.


OK for google/main?


chris
---
[gcc/ChangeLog.google-main]
2011-05-05  Chris Demetriou  <cgd@google.com>

        * doc/install.texi (--with-warn-frame-larger-than-extra-text): New.
	* configure.ac (--with-warn-frame-larger-than-extra-text): New.
        (WARN_FRAME_LARGER_THAN_EXTRA_TEXT): Define.
        * final.c (final_start_function): Use
        WARN_FRAME_LARGER_THAN_EXTRA_TEXT.
        * configure: Regenerate.
        * config.in: Regenerate.

Index: gcc/doc/install.texi
===================================================================
--- gcc/doc/install.texi	(revision 173353)
+++ gcc/doc/install.texi	(working copy)
@@ -1707,6 +1707,10 @@
 See @option{-canonical-prefixes} or @option{-no-canonical-prefixes} for
 more details, including how to override this configuration option when
 compiling.
+
+@item --with-warn-frame-larger-than-extra-text=@var{text}
+Append @samp{@var{text}} to frame size warnings generated by
+the @option{-Wframe-larger-than} warning flag.
 @end table
 
 @subheading Cross-Compiler-Specific Options
Index: gcc/final.c
===================================================================
--- gcc/final.c	(revision 173353)
+++ gcc/final.c	(working copy)
@@ -1576,9 +1576,13 @@
   if (warn_frame_larger_than
     && get_frame_size () > frame_larger_than_size)
   {
-      /* Issue a warning */
+      /* Issue a warning.  (WARN_FRAME_LARGER_THAN_EXTRA_TEXT is
+         provided by configuration.  The way extra text is added
+         here may prevent localization from working properly.
+         It's totally broken.)  */
       warning (OPT_Wframe_larger_than_,
-               "the frame size of %wd bytes is larger than %wd bytes",
+               "the frame size of %wd bytes is larger than %wd bytes"
+               WARN_FRAME_LARGER_THAN_EXTRA_TEXT,
                get_frame_size (), frame_larger_than_size);
   }
 
Index: gcc/configure.ac
===================================================================
--- gcc/configure.ac	(revision 173353)
+++ gcc/configure.ac	(working copy)
@@ -4951,6 +4951,20 @@
 fi
 
 
+warn_frame_larger_than_extra_text=
+AC_ARG_WITH(warn-frame-larger-than-extra-text,
+[  --with-warn-frame-larger-than-extra-text=TEXT
+                          specifies extra text for frame size warnings],
+[case "${withval}" in
+yes)	AC_MSG_ERROR(bad value ${withval} given for frame size warning text) ;;
+no)	;;
+*)	warn_frame_larger_than_extra_text="$withval" ;;
+esac])
+AC_DEFINE_UNQUOTED(WARN_FRAME_LARGER_THAN_EXTRA_TEXT,
+                   "$warn_frame_larger_than_extra_text",
+                   [Define to be extra text for frame size warnings.])
+
+
 # Configure the subdirectories
 # AC_CONFIG_SUBDIRS($subdirs)
 

--
This patch is available for review at http://codereview.appspot.com/4479046

Re: [google][RFA] add extra text to stack frame warnings (issue4479046)

[Diego Novillo]

On Thu, May 5, 2011 at 14:27, Chris Demetriou <cgd@google.com> wrote:
> Diego,
>
> I know this is a truly horrible and broken way to do this, but alternatives
> (e.g., NLS) don't really work for us.
>
> bootstrapped without new configuration (x86-64 ubuntu lucid, didn't bother
> to run tests), bootstrapped with option (x86-64 ubuntu lucid, with full
> tests, no regressions).  Manually tested that the resulting warning
> looks right, too.
>
>
> OK for google/main?
>
>
> chris
> ---
> [gcc/ChangeLog.google-main]
> 2011-05-05  Chris Demetriou  <cgd@google.com>
>
>        * doc/install.texi (--with-warn-frame-larger-than-extra-text): New.
>        * configure.ac (--with-warn-frame-larger-than-extra-text): New.
>        (WARN_FRAME_LARGER_THAN_EXTRA_TEXT): Define.
>        * final.c (final_start_function): Use
>        WARN_FRAME_LARGER_THAN_EXTRA_TEXT.
>        * configure: Regenerate.
>        * config.in: Regenerate.


Do we want in google/integration maybe?   Or do you see this going to trunk?

OK for either, in any case.  Just decide where you think it's best to have.


Diego.

Re: [google][RFA] add extra text to stack frame warnings (issue4479046)

[Andrew Pinski]

On Thu, May 5, 2011 at 11:47 AM, Diego Novillo <dnovillo@google.com> wrote:
>> OK for google/main?

Is there a reason why this cannot be an option that someone passes on
the command line of GCC instead of a configure option?  Also can you
show an example of why this message would be changed?

Thanks,
Andrew Pinski

Re: [google][RFA] add extra text to stack frame warnings (issue4479046)

[Chris Demetriou]

On Thu, May 5, 2011 at 12:19, Andrew Pinski <pinskia@gmail.com> wrote:
> Is there a reason why this cannot be an option that someone passes on
> the command line of GCC instead of a configure option?

I don't think we ever considered that approach.
That's actually a great idea, I think better for our purposes than a
configuration option.
(Previously, it didn't much matter, since in our tree this was a small
local patch directly to final.c.)

Thank you, I'm going to do over taking the approach you suggested.


> Also can you
> show an example of why this message would be changed?

We use the stack frame size warning on some of our internal code.
(Obvious, I guess -- otherwise, why would I be messing with it.  8-)

In summary, -Wframe-larger-than does not always produce obvious results.  8-)

There are common questions, e.g.:
* why we care about this warning at all (i.e., "why does stack frame
size matter?!").
* how to identify the cause of the warning (since it's not necessarily
obvious what's causing stack growth, and because the warning is
somewhat ... finicky thanks to inlining and thanks to
sometimes-less-than-great reuse of stack space from dead variables in
optimized and especially unoptimized code).
* how to work around, or if absolutely necessary disable the warning.

So, to help, when we output the frame-size warning, we also provide a
link to an internal documentation page to help with the stuff
mentioned above.

Of necessity, the doc link we provide explains our internal
circumstances and workarounds.  (Generic documentation wouldn't help
with a number of the questions.)


In theory, a more general warning-text-addition mechanism could be useful.
e.g. a flag that said "when outputting a warning about flag 'foo',
output this additional text" could be useful.
However, we haven't felt the need to do this for other warnings.

IMO, a general solution along these lines would be solving a problem
that ~nobody has.  8-)

If one wanted to dive into warning message changes, there are other,
more substantial changes IMO that would be generally useful and would
enable this type of functionality via external tools.
E.g., structured warnings with fixed identifiers (numbers, words,
whatever), blah blah blah.
If there were support for *that*, then people could write wrapper
tools that automatically annotate warnings with additional information
as necessary.
(it would also make parsing errors/warnings a lot easier.  8-)



Anyway, thanks for the suggestion.  8-)


chris

Re: [google][RFA] add extra text to stack frame warnings (issue4479046)

[Andrew Pinski]

On Fri, May 6, 2011 at 1:52 AM, Chris Demetriou <cgd@google.com> wrote:
> In theory, a more general warning-text-addition mechanism could be useful.
> e.g. a flag that said "when outputting a warning about flag 'foo',
> output this additional text" could be useful.
> However, we haven't felt the need to do this for other warnings.
>
> IMO, a general solution along these lines would be solving a problem
> that ~nobody has.  8-)

We already output the option which enables the warning that seems like
a general solution.

Thanks,
Andrew Pinski

Re: [google][RFA] add extra text to stack frame warnings (issue4479046)

[Chris Demetriou]

[resending plain text.  Sorry for the noise.]

[sorry, i got stuck doing a bunch of other things.]

On Fri, May 6, 2011 at 10:05, Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Fri, May 6, 2011 at 1:52 AM, Chris Demetriou <cgd@google.com> wrote:
> > In theory, a more general warning-text-addition mechanism could be useful.
> > e.g. a flag that said "when outputting a warning about flag 'foo',
> > output this additional text" could be useful.
> > However, we haven't felt the need to do this for other warnings.
> >
> > IMO, a general solution along these lines would be solving a problem
> > that ~nobody has.  8-)
>
> We already output the option which enables the warning that seems like
> a general solution.

Yeah.  I was going to look into using something similar to implement a
generic text-addition mechanism for warnings... but got stuck in
previously-mentioned bunch of other things.  8-(
I ran out of time, ended up checking this in as-is (after updating the
changelog date and retesting).
I may get back to doing it the better way... but it won't be for a while.


chris