| Message ID | bd0c73ab-8bec-03a2-a77e-7ec0948b5622@suse.cz |
|---|---|
| State | New |
| Headers | show |
On 07/10/2017 02:35 AM, Martin Liška wrote: > On 07/07/2017 09:20 PM, Martin Sebor wrote: >> A conflict in my patch for bug 81345 made me notice that r249734 >> recently added a new option property, IntegerRange. The change >> below adds brief documentation of the property to the manual. >> >> Martin, can you please check to make sure I didn't miss anything? >> >> Btw., while experimenting with the property I noticed that there >> is no error when option that specifies IntegerRange is set in >> the .opt file to a value outside that range. Would it be hard >> to add some checks the the awk scripts to validate that the >> argument values are in the range? It might help avoid bugs >> similar to 81345). > > Sure, please take a look at attached patch. Can you please test it? The detection works fine for the Init problem (thanks!) but it doesn't catch the out-of-range initializer in LangEnabledBy(C, Wall, 2, 0) or in Alias(Wfoobar=, 1, 0). I don't know enough about the option scripts yet to gauge how difficult handling these might be. Do you have any idea? If you think it's doable but outside the scope of this tweak let me know and I'll open a bug for it to help us remember to handle it at some point too. >> >> By the way of an example, the following invalid specification >> is accepted but then causes errors when GCC runs. >> >> Wfoobar >> C ObjC C++ ObjC++ Warning Alias(Wfoobar=, 1, 0) >> >> Wfoobar= >> C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_foobar) Warning LangEnabledBy(C ObjC C++ ObjC++, Wall, 2, 0) Init (7) IntegerRange(3, 5) > > Here one needs to have 'Init (7)' without space! Ugh. I only recently realized this but keep forgetting. It seems like another unnecessary trap that would be nice to fix at some point. Thanks Martin > >> >> Martin >> >> diff --git a/gcc/doc/options.texi b/gcc/doc/options.texi >> index 3b68aab..af56e9f 100644 >> --- a/gcc/doc/options.texi >> +++ b/gcc/doc/options.texi >> @@ -264,6 +264,12 @@ option handler. @code{UInteger} should also be used on options like >> @code{-falign-loops}=@var{n} are supported to make sure the saved >> options are given a full integer. >> >> +@item IntegerRange(@var{min}, @var{max}) >> +The option's integer argument is expected to be in the range specified >> +by @var{min} and @var{max}, inclusive. The option parser will check >> +and reject option arguments that are outside the range before passing >> +it to the relevant option handler. > > LGTM, thanks for the documentation entry. > > Martin > >> + >> @item ToLower >> The option's argument should be converted to lowercase as part of >> putting it in canonical form, and before comparing with the strings >
On 07/10/2017 05:08 PM, Martin Sebor wrote: > On 07/10/2017 02:35 AM, Martin Liška wrote: >> On 07/07/2017 09:20 PM, Martin Sebor wrote: >>> A conflict in my patch for bug 81345 made me notice that r249734 >>> recently added a new option property, IntegerRange. The change >>> below adds brief documentation of the property to the manual. >>> >>> Martin, can you please check to make sure I didn't miss anything? >>> >>> Btw., while experimenting with the property I noticed that there >>> is no error when option that specifies IntegerRange is set in >>> the .opt file to a value outside that range. Would it be hard >>> to add some checks the the awk scripts to validate that the >>> argument values are in the range? It might help avoid bugs >>> similar to 81345). >> >> Sure, please take a look at attached patch. Can you please test it? > > The detection works fine for the Init problem (thanks!) but it > doesn't catch the out-of-range initializer in LangEnabledBy(C, > Wall, 2, 0) or in Alias(Wfoobar=, 1, 0). I don't know enough > about the option scripts yet to gauge how difficult handling > these might be. Do you have any idea? If you think it's > doable but outside the scope of this tweak let me know and I'll > open a bug for it to help us remember to handle it at some point > too. Well, it's definitely doable, but doing that in the current awk script is quite cumbersome. I would prefer to have the option generation rewritten e.g. in Python where current awk script is not nice to reuse an already parsed information. More class-oriented approach would be desired here. Please create a PR, I maybe rewrite it in future if we can benefit from that. Are you interested in the current patch that handles 'Init' directive to go in? Martin > >>> >>> By the way of an example, the following invalid specification >>> is accepted but then causes errors when GCC runs. >>> >>> Wfoobar >>> C ObjC C++ ObjC++ Warning Alias(Wfoobar=, 1, 0) >>> >>> Wfoobar= >>> C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_foobar) Warning LangEnabledBy(C ObjC C++ ObjC++, Wall, 2, 0) Init (7) IntegerRange(3, 5) >> >> Here one needs to have 'Init (7)' without space! > > Ugh. I only recently realized this but keep forgetting. It > seems like another unnecessary trap that would be nice to fix > at some point. > > Thanks > Martin > >> >>> >>> Martin >>> >>> diff --git a/gcc/doc/options.texi b/gcc/doc/options.texi >>> index 3b68aab..af56e9f 100644 >>> --- a/gcc/doc/options.texi >>> +++ b/gcc/doc/options.texi >>> @@ -264,6 +264,12 @@ option handler. @code{UInteger} should also be used on options like >>> @code{-falign-loops}=@var{n} are supported to make sure the saved >>> options are given a full integer. >>> >>> +@item IntegerRange(@var{min}, @var{max}) >>> +The option's integer argument is expected to be in the range specified >>> +by @var{min} and @var{max}, inclusive. The option parser will check >>> +and reject option arguments that are outside the range before passing >>> +it to the relevant option handler. >> >> LGTM, thanks for the documentation entry. >> >> Martin >> >>> + >>> @item ToLower >>> The option's argument should be converted to lowercase as part of >>> putting it in canonical form, and before comparing with the strings >> >
On 07/11/2017 07:32 AM, Martin Liška wrote: > On 07/10/2017 05:08 PM, Martin Sebor wrote: >> On 07/10/2017 02:35 AM, Martin Liška wrote: >>> On 07/07/2017 09:20 PM, Martin Sebor wrote: >>>> A conflict in my patch for bug 81345 made me notice that r249734 >>>> recently added a new option property, IntegerRange. The change >>>> below adds brief documentation of the property to the manual. >>>> >>>> Martin, can you please check to make sure I didn't miss anything? >>>> >>>> Btw., while experimenting with the property I noticed that there >>>> is no error when option that specifies IntegerRange is set in >>>> the .opt file to a value outside that range. Would it be hard >>>> to add some checks the the awk scripts to validate that the >>>> argument values are in the range? It might help avoid bugs >>>> similar to 81345). >>> >>> Sure, please take a look at attached patch. Can you please test it? >> >> The detection works fine for the Init problem (thanks!) but it >> doesn't catch the out-of-range initializer in LangEnabledBy(C, >> Wall, 2, 0) or in Alias(Wfoobar=, 1, 0). I don't know enough >> about the option scripts yet to gauge how difficult handling >> these might be. Do you have any idea? If you think it's >> doable but outside the scope of this tweak let me know and I'll >> open a bug for it to help us remember to handle it at some point >> too. > > Well, it's definitely doable, but doing that in the current awk script > is quite cumbersome. > I would prefer to have the option generation rewritten e.g. in Python > where current awk > script is not nice to reuse an already parsed information. More > class-oriented approach would > be desired here. > > Please create a PR, I maybe rewrite it in future if we can benefit from > that. I created bug 81397 for all these problems. > Are you interested in the current patch that handles 'Init' directive to > go in? Yes please, thank you! Martin > > Martin > >> >>>> >>>> By the way of an example, the following invalid specification >>>> is accepted but then causes errors when GCC runs. >>>> >>>> Wfoobar >>>> C ObjC C++ ObjC++ Warning Alias(Wfoobar=, 1, 0) >>>> >>>> Wfoobar= >>>> C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_foobar) >>>> Warning LangEnabledBy(C ObjC C++ ObjC++, Wall, 2, 0) Init (7) >>>> IntegerRange(3, 5) >>> >>> Here one needs to have 'Init (7)' without space! >> >> Ugh. I only recently realized this but keep forgetting. It >> seems like another unnecessary trap that would be nice to fix >> at some point. >> >> Thanks >> Martin >> >>> >>>> >>>> Martin >>>> >>>> diff --git a/gcc/doc/options.texi b/gcc/doc/options.texi >>>> index 3b68aab..af56e9f 100644 >>>> --- a/gcc/doc/options.texi >>>> +++ b/gcc/doc/options.texi >>>> @@ -264,6 +264,12 @@ option handler. @code{UInteger} should also be >>>> used on options like >>>> @code{-falign-loops}=@var{n} are supported to make sure the saved >>>> options are given a full integer. >>>> >>>> +@item IntegerRange(@var{min}, @var{max}) >>>> +The option's integer argument is expected to be in the range specified >>>> +by @var{min} and @var{max}, inclusive. The option parser will check >>>> +and reject option arguments that are outside the range before passing >>>> +it to the relevant option handler. >>> >>> LGTM, thanks for the documentation entry. >>> >>> Martin >>> >>>> + >>>> @item ToLower >>>> The option's argument should be converted to lowercase as part of >>>> putting it in canonical form, and before comparing with the strings >>> >> >
From 443280fdc8a1215c8045752033f8290dd4e493c9 Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Mon, 10 Jul 2017 10:33:49 +0200 Subject: [PATCH] To test version. --- gcc/common.opt | 7 +++++++ gcc/opt-functions.awk | 4 +++- gcc/optc-gen.awk | 3 ++- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/gcc/common.opt b/gcc/common.opt index e81165c488b..2dd2f79f6ab 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -3109,4 +3109,11 @@ fipa-ra Common Report Var(flag_ipa_ra) Optimization Use caller save register across calls if possible. + +Wfoobar +C ObjC C++ ObjC++ Warning Alias(Wfoobar=, 1, 0) + +Wfoobar= +C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_foobar) Warning LangEnabledBy(C ObjC C++ ObjC++, Wall, 2, 0) Init(7) IntegerRange(3, 5) + ; This comment is to ensure we retain the blank line above. diff --git a/gcc/opt-functions.awk b/gcc/opt-functions.awk index ad0b52c0903..5ee93f12feb 100644 --- a/gcc/opt-functions.awk +++ b/gcc/opt-functions.awk @@ -314,11 +314,13 @@ function search_var_name(name, opt_numbers, opts, flags, n_opts) return "" } -function integer_range_info(range_option) +function integer_range_info(range_option, init, option) { if (range_option != "") { start = nth_arg(0, range_option); end = nth_arg(1, range_option); + if (init != "" && init != "-1" && (init < start || init > end)) + print "#error initial value " init " of '" option "' must be in range [" start "," end "]" return start ", " end } else diff --git a/gcc/optc-gen.awk b/gcc/optc-gen.awk index 45b1b95e7ec..3cb0005ba40 100644 --- a/gcc/optc-gen.awk +++ b/gcc/optc-gen.awk @@ -400,7 +400,8 @@ for (i = 0; i < n_opts; i++) { " 0, %s,\n", cl_flags, cl_bit_fields) printf(" %s, %s, %s }%s\n", var_ref(opts[i], flags[i]), - var_set(flags[i]), integer_range_info(opt_args("IntegerRange", flags[i])), comma) + var_set(flags[i]), integer_range_info(opt_args("IntegerRange", flags[i]), + opt_args("Init", flags[i]), opts[i]), comma) } print "};" -- 2.13.2