Issue 4479046: [google][RFA] add extra text to stack frame warnings
|
Created:
13 years ago by cgd Modified:
12 years, 11 months ago Reviewers:
Diego Novillo, pinskia CC:
gcc-patches_gcc.gnu.org Base URL:
svn+ssh://gcc.gnu.org/svn/gcc/branches/google/main/ Visibility:
Public. |
Patch Set 1 #
MessagesTotal messages: 7
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
[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
Sign in to reply to this message.
[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
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
