| Message ID | ff8d99a0-b0ab-3104-9d0b-5e2b4ae4ae8a@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 11/16/2016 09:49 AM, Martin Sebor wrote: > I'm looking for an approval of the attached patch. > > I've adjusted the documentation based on Sandra's input (i.e., > documented the negative of the option rather than the positive; > thank you for the review, btw.) > > On 11/08/2016 08:13 PM, Martin Sebor wrote: >> The -fprintf-return-value optimization has been disabled since >> the last time it caused a bootstrap failure on powerpc64le. With >> the underlying problems fixed GCC has bootstrapped fine on all of >> powerpc64, powerpc64le and x86_64 and tested with no regressions. >> I'd like to re-enable the option. The attached patch does that. > > [snip] > > Index: gcc/doc/invoke.texi > =================================================================== > --- gcc/doc/invoke.texi (revision 242500) > +++ gcc/doc/invoke.texi (working copy) > @@ -384,7 +384,7 @@ Objective-C and Objective-C++ Dialects}. > -fno-toplevel-reorder -fno-trapping-math -fno-zero-initialized-in-bss @gol > -fomit-frame-pointer -foptimize-sibling-calls @gol > -fpartial-inlining -fpeel-loops -fpredictive-commoning @gol > --fprefetch-loop-arrays -fprintf-return-value @gol > +-fprefetch-loop-arrays -fno-printf-return-value @gol > -fprofile-correction @gol > -fprofile-use -fprofile-use=@var{path} -fprofile-values @gol > -fprofile-reorder-functions @gol Please keep this list alphabetized -- the other "-fno-*" options are sorted as such. The documentation parts of the patch are OK with that fixed, but I can't approve changing the default for the option. -Sandra
On 11/17/2016 10:34 PM, Sandra Loosemore wrote: > On 11/16/2016 09:49 AM, Martin Sebor wrote: >> I'm looking for an approval of the attached patch. >> >> I've adjusted the documentation based on Sandra's input (i.e., >> documented the negative of the option rather than the positive; >> thank you for the review, btw.) >> >> On 11/08/2016 08:13 PM, Martin Sebor wrote: >>> The -fprintf-return-value optimization has been disabled since >>> the last time it caused a bootstrap failure on powerpc64le. With >>> the underlying problems fixed GCC has bootstrapped fine on all of >>> powerpc64, powerpc64le and x86_64 and tested with no regressions. >>> I'd like to re-enable the option. The attached patch does that. >> >> [snip] >> >> Index: gcc/doc/invoke.texi >> =================================================================== >> --- gcc/doc/invoke.texi (revision 242500) >> +++ gcc/doc/invoke.texi (working copy) >> @@ -384,7 +384,7 @@ Objective-C and Objective-C++ Dialects}. >> -fno-toplevel-reorder -fno-trapping-math -fno-zero-initialized-in-bss >> @gol >> -fomit-frame-pointer -foptimize-sibling-calls @gol >> -fpartial-inlining -fpeel-loops -fpredictive-commoning @gol >> --fprefetch-loop-arrays -fprintf-return-value @gol >> +-fprefetch-loop-arrays -fno-printf-return-value @gol >> -fprofile-correction @gol >> -fprofile-use -fprofile-use=@var{path} -fprofile-values @gol >> -fprofile-reorder-functions @gol > > Please keep this list alphabetized -- the other "-fno-*" options are > sorted as such. The documentation parts of the patch are OK with that > fixed, but I can't approve changing the default for the option. I kept the option in the same place on the assumption that it was the "printf" radix of the name, not the "no-" prefix, that these were alphabetized by. But now that you point it out and I've looked more carefully at some of the other options, I see that in some sections they are listed strictly alphabetically (-fno-foo after -fmoo) while in others it's the way you suggest. AFAICS, the former style looks prevalent in the C++ Language Options and the in Warning Options, for example. The latter seems to be more popular in the Optimization Options section (though there are counterexamples). I'm happy to follow either of these conventions as long as its consistent. Otherwise, without both kinds of examples to choose from, I don't think I can trust my brain to remember yet another rule. Martin
On 11/18/2016 09:01 AM, Martin Sebor wrote: > On 11/17/2016 10:34 PM, Sandra Loosemore wrote: >> On 11/16/2016 09:49 AM, Martin Sebor wrote: >>> I'm looking for an approval of the attached patch. >>> >>> I've adjusted the documentation based on Sandra's input (i.e., >>> documented the negative of the option rather than the positive; >>> thank you for the review, btw.) >>> >>> On 11/08/2016 08:13 PM, Martin Sebor wrote: >>>> The -fprintf-return-value optimization has been disabled since >>>> the last time it caused a bootstrap failure on powerpc64le. With >>>> the underlying problems fixed GCC has bootstrapped fine on all of >>>> powerpc64, powerpc64le and x86_64 and tested with no regressions. >>>> I'd like to re-enable the option. The attached patch does that. >>> >>> [snip] >>> >>> Index: gcc/doc/invoke.texi >>> =================================================================== >>> --- gcc/doc/invoke.texi (revision 242500) >>> +++ gcc/doc/invoke.texi (working copy) >>> @@ -384,7 +384,7 @@ Objective-C and Objective-C++ Dialects}. >>> -fno-toplevel-reorder -fno-trapping-math -fno-zero-initialized-in-bss >>> @gol >>> -fomit-frame-pointer -foptimize-sibling-calls @gol >>> -fpartial-inlining -fpeel-loops -fpredictive-commoning @gol >>> --fprefetch-loop-arrays -fprintf-return-value @gol >>> +-fprefetch-loop-arrays -fno-printf-return-value @gol >>> -fprofile-correction @gol >>> -fprofile-use -fprofile-use=@var{path} -fprofile-values @gol >>> -fprofile-reorder-functions @gol >> >> Please keep this list alphabetized -- the other "-fno-*" options are >> sorted as such. The documentation parts of the patch are OK with that >> fixed, but I can't approve changing the default for the option. > > I kept the option in the same place on the assumption that it was > the "printf" radix of the name, not the "no-" prefix, that these > were alphabetized by. > > But now that you point it out and I've looked more carefully at > some of the other options, I see that in some sections they are > listed strictly alphabetically (-fno-foo after -fmoo) while in > others it's the way you suggest. AFAICS, the former style looks > prevalent in the C++ Language Options and the in Warning Options, > for example. The latter seems to be more popular in the > Optimization Options section (though there are counterexamples). > > I'm happy to follow either of these conventions as long as its > consistent. Otherwise, without both kinds of examples to choose > from, I don't think I can trust my brain to remember yet another > rule. Well, how about the rule is that you look at the convention of the specific list you are adding something to? At least that way we retain local consistency and don't mess up those parts of the documentation that we have already tried to organize in some way. There's so much material in the command-line options chapter that it would be hard to figure out how to present it even if the content were completely static, much less when people are adding/renaming/modifying options all the time. -Sandra
On 11/18/2016 10:38 AM, Sandra Loosemore wrote: > On 11/18/2016 09:01 AM, Martin Sebor wrote: >> On 11/17/2016 10:34 PM, Sandra Loosemore wrote: >>> On 11/16/2016 09:49 AM, Martin Sebor wrote: >>>> I'm looking for an approval of the attached patch. >>>> >>>> I've adjusted the documentation based on Sandra's input (i.e., >>>> documented the negative of the option rather than the positive; >>>> thank you for the review, btw.) >>>> >>>> On 11/08/2016 08:13 PM, Martin Sebor wrote: >>>>> The -fprintf-return-value optimization has been disabled since >>>>> the last time it caused a bootstrap failure on powerpc64le. With >>>>> the underlying problems fixed GCC has bootstrapped fine on all of >>>>> powerpc64, powerpc64le and x86_64 and tested with no regressions. >>>>> I'd like to re-enable the option. The attached patch does that. >>>> >>>> [snip] >>>> >>>> Index: gcc/doc/invoke.texi >>>> =================================================================== >>>> --- gcc/doc/invoke.texi (revision 242500) >>>> +++ gcc/doc/invoke.texi (working copy) >>>> @@ -384,7 +384,7 @@ Objective-C and Objective-C++ Dialects}. >>>> -fno-toplevel-reorder -fno-trapping-math -fno-zero-initialized-in-bss >>>> @gol >>>> -fomit-frame-pointer -foptimize-sibling-calls @gol >>>> -fpartial-inlining -fpeel-loops -fpredictive-commoning @gol >>>> --fprefetch-loop-arrays -fprintf-return-value @gol >>>> +-fprefetch-loop-arrays -fno-printf-return-value @gol >>>> -fprofile-correction @gol >>>> -fprofile-use -fprofile-use=@var{path} -fprofile-values @gol >>>> -fprofile-reorder-functions @gol >>> >>> Please keep this list alphabetized -- the other "-fno-*" options are >>> sorted as such. The documentation parts of the patch are OK with that >>> fixed, but I can't approve changing the default for the option. >> >> I kept the option in the same place on the assumption that it was >> the "printf" radix of the name, not the "no-" prefix, that these >> were alphabetized by. >> >> But now that you point it out and I've looked more carefully at >> some of the other options, I see that in some sections they are >> listed strictly alphabetically (-fno-foo after -fmoo) while in >> others it's the way you suggest. AFAICS, the former style looks >> prevalent in the C++ Language Options and the in Warning Options, >> for example. The latter seems to be more popular in the >> Optimization Options section (though there are counterexamples). >> >> I'm happy to follow either of these conventions as long as its >> consistent. Otherwise, without both kinds of examples to choose >> from, I don't think I can trust my brain to remember yet another >> rule. > > Well, how about the rule is that you look at the convention of the > specific list you are adding something to? At least that way we retain > local consistency and don't mess up those parts of the documentation > that we have already tried to organize in some way. There's so much > material in the command-line options chapter that it would be hard to > figure out how to present it even if the content were completely static, > much less when people are adding/renaming/modifying options all the time. I think it would be be ideal if all the options were sorted the same way in all sections. Is there some command to have texinfo sort them for us? If not, can we write a script to sort them, either each time just before generating the docs or manually? (I'm happy to help.) Otherwise, consistency will continue to be an elusive goal. There are at least two reasons why I don't think following the style used by each section is likely to yield good results (and clearly hasn't to date). First, the big sections already have examples of both approaches (or simply options out of order). In some of them it can also be hard to tell if the radix of the options you're looking to for guidance starts with an 'n'. Second, when adding more than one option to different sections (such as the -Wformat-length and -fprintf-format-length options) having to remember to apply a different sort order for each (warnings are sorted by radix but optimization options, for the most parts, strictly alphabetically), seems also likely to trip people up. Martin PS I don't mind moving the -fno-printf-return-value option up or down and I will do it before committing the patch. I would just prefer to be able not to have to remember and worry about all these subtle rules. There are too many of them and they tend to take time and energy away from things that matter more (like fixing bugs).
On 11/18/2016 11:52 AM, Martin Sebor wrote: > > I think it would be be ideal if all the options were sorted the same > way in all sections. Is there some command to have texinfo sort them > for us? If not, can we write a script to sort them, either each time > just before generating the docs or manually? (I'm happy to help.) > Otherwise, consistency will continue to be an elusive goal. I'm not aware of texinfo way to do this automatically. > > There are at least two reasons why I don't think following the style > used by each section is likely to yield good results (and clearly > hasn't to date). First, the big sections already have examples of > both approaches (or simply options out of order). In some of them > it can also be hard to tell if the radix of the options you're > looking to for guidance starts with an 'n'. Second, when adding > more than one option to different sections (such as the > -Wformat-length and -fprintf-format-length options) having to > remember to apply a different sort order for each (warnings are > sorted by radix but optimization options, for the most parts, > strictly alphabetically), seems also likely to trip people up. Let's split this issue off by moving the option into the location Sandra has asked so that we're at least kindof, sorta, locally consistent. That allows your patch to go forward. Then separately we can see if we can bring more sense to the larger issue. Sandra has tried to work towards bring sanity to our documentation (which has grown like field bindweed over time) and we can include a discussion about this issue in that larger effort. > PS I don't mind moving the -fno-printf-return-value option up or > down and I will do it before committing the patch. I would just > prefer to be able not to have to remember and worry about all > these subtle rules. There are too many of them and they tend > to take time and energy away from things that matter more (like > fixing bugs). Understood. But that's also part of the reason why we delegate things -- there's a million little things to remember and nobody can remember them all. So it's a balance between saying "we should clean this up and bring consistency here now" and "the maintainer has asked for a change, let's do that and address the consistency issues separately". There's obviously pros and cons to each decision which I don't enumerate ;-) Jeff
On 11/18/2016 11:52 AM, Martin Sebor wrote: > > [snip] > I think it would be be ideal if all the options were sorted the same > way in all sections. Is there some command to have texinfo sort them > for us? If not, can we write a script to sort them, either each time > just before generating the docs or manually? (I'm happy to help.) > Otherwise, consistency will continue to be an elusive goal. > > There are at least two reasons why I don't think following the style > used by each section is likely to yield good results (and clearly > hasn't to date). First, the big sections already have examples of > both approaches (or simply options out of order). In some of them > it can also be hard to tell if the radix of the options you're > looking to for guidance starts with an 'n'. Second, when adding > more than one option to different sections (such as the > -Wformat-length and -fprintf-format-length options) having to > remember to apply a different sort order for each (warnings are > sorted by radix but optimization options, for the most parts, > strictly alphabetically), seems also likely to trip people up. This is wandering off into a general documentation maintenance discussion that has little to do with the original patch review request. GCC has way too many command-line options and most of them are interesting to only some tiny fraction of users. The documentation needs to be structured to focus on the options users are most likely to need or find useful, and not bury the information about the most important options in the middle of a huge pile of barely-useful documentation about barely-useful options. For this reason, I think a strict alphabetical sorting across the board is not helpful. E.g., for the list of optimization options, the -O options are clearly more important than the -f options controlling individual optimizations, so -O should be the first thing users see when they look at that section. Similarly for debug options, the vast majority of users won't care about anything other than -g. For target-specific options, it may make sense to list -march/-mcpu/-mtune together regardless of their alphabetization with respect to other -m options for that target. My last round of cleanup on invoke.texi was focused on refining the categorization of options and moving some option documentation that was clearly mis-categorized to more appropriate places or newly-created categories. Having fewer items listed in each category makes the ordering of things within the group less critical. I think people looking for documentation for a specific option should be looking in the index first. We should have @cindex entries for both the -ffoo and -fno-foo variants so the alphabetization works either way. Anyway.... long story short.... there's probably no perfect organization for this chapter, and the sheer number of options being documented makes it a pile of work even to implement small changes in convention across the board. That shouldn't stop us from making incremental improvements in organization, one section at a time, or taking care not to make the current situation worse when adding documentation for new options. -Sandra
gcc/c-family/ChangeLog: * c.opt (-fprintf-return-value): Enable by default. gcc/ChangeLog: * doc/invoke.texi (-fprintf-return-value): Document that option is enabled by default. Index: gcc/c-family/c.opt =================================================================== --- gcc/c-family/c.opt (revision 242500) +++ gcc/c-family/c.opt (working copy) @@ -1550,7 +1550,7 @@ C++ ObjC++ Var(flag_pretty_templates) Init(1) -fno-pretty-templates Do not pretty-print template specializations as the template signature followed by the arguments. fprintf-return-value -C ObjC C++ ObjC++ LTO Optimization Var(flag_printf_return_value) Init(0) +C ObjC C++ ObjC++ LTO Optimization Var(flag_printf_return_value) Init(1) Treat known sprintf return values as constants. freplace-objc-classes Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 242500) +++ gcc/doc/invoke.texi (working copy) @@ -384,7 +384,7 @@ Objective-C and Objective-C++ Dialects}. -fno-toplevel-reorder -fno-trapping-math -fno-zero-initialized-in-bss @gol -fomit-frame-pointer -foptimize-sibling-calls @gol -fpartial-inlining -fpeel-loops -fpredictive-commoning @gol --fprefetch-loop-arrays -fprintf-return-value @gol +-fprefetch-loop-arrays -fno-printf-return-value @gol -fprofile-correction @gol -fprofile-use -fprofile-use=@var{path} -fprofile-values @gol -fprofile-reorder-functions @gol @@ -8286,18 +8286,19 @@ dependent on the structure of loops within the sou Disabled at level @option{-Os}. -@item -fprintf-return-value -@opindex fprintf-return-value -Substitute constants for known return value of formatted output functions -such as @code{sprintf}, @code{snprintf}, @code{vsprintf}, and @code{vsnprintf} -(but not @code{printf} of @code{fprintf}). This transformation allows GCC -to optimize or even eliminate branches based on the known return value of -these functions called with arguments that are either constant, or whose -values are known to be in a range that makes determining the exact return -value possible. For example, both the branch and the body of the @code{if} -statement (but not the call to @code{snprint}) can be optimized away when -@code{i} is a 32-bit or smaller integer because the return value is guaranteed -to be at most 8. +@item -fno-printf-return-value +@opindex fno-printf-return-value +Do not substitute constants for known return value of formatted output +functions such as @code{sprintf}, @code{snprintf}, @code{vsprintf}, and +@code{vsnprintf} (but not @code{printf} or @code{fprintf}). This +transformation allows GCC to optimize or even eliminate branches based +on the known return value of these functions called with arguments that +are either constant, or whose values are known to be in a range that +makes determining the exact return value possible. For example, when +@option{-fprintf-return-value} is in effect, both the branch and the +body of the @code{if} statement (but not the call to @code{snprintf}) +can be optimized away when @code{i} is a 32-bit or smaller integer +because the return value is guaranteed to be at most 8. @smallexample char buf[9]; @@ -8308,7 +8309,7 @@ if (snprintf (buf, "%08x", i) >= sizeof buf) The @option{-fprintf-return-value} option relies on other optimizations and yields best results with @option{-O2}. It works in tandem with the @option{-Wformat-length} option. The @option{-fprintf-return-value} -option is disabled by default. +option is enabled by default. @item -fno-peephole @itemx -fno-peephole2