Re: [RFC] PR 53063 encode group options in .opt files
On Fri, May 18, 2012 at 4:56 AM, Chung-Lin Tang wrote: > On 2012/5/18 03:26 PM, Gabriel Dos Reis wrote: >> On Fri, May 18, 2012 at 12:48 AM, Chung-Lin Tang >> wrote: >> >>> The point here is that, a group of changes that broke C bootstrap went >>> in undetected for several days, because of the partially C++ default. To >>> prevent that in the future, we should enforce similar checking in both C >>> and C++. >> >> As opposed to enforcing checking that makes sense in a C++ setting? > > I think that, with the current status quo, it would be best to maintain > things valid in both languages. But that does not make much sense. > > But really, can you give more specific examples of "functions > used in certain contexts [required to] have external linkage"? struct Apply > Because I > don't think that in general, allowing global functions without > declarations in headers is recommended C++ style either :) That would only suggest a large practice of C and a limited practice of C++. Even our own implementation of does not follow that practice. It is not even idiomatic modern C++ to require that. It is an example of something that makes sense in C (because > > Maybe what should be fixed is for -Wmissing-declarations to determine > between your mentioned cases of external linkage, and the "usual" case > (if it's not already capable of that). This is a rabbit hole. Your have determine that something is useful in C, and now want at all cost get into C++ without looking at idiomatic modern C++ and see whether it makes much sense there. > > Chung-Lin
Re: [RFC] PR 53063 encode group options in .opt files
On 2012/5/18 03:26 PM, Gabriel Dos Reis wrote: > On Fri, May 18, 2012 at 12:48 AM, Chung-Lin Tang > wrote: > >> The point here is that, a group of changes that broke C bootstrap went >> in undetected for several days, because of the partially C++ default. To >> prevent that in the future, we should enforce similar checking in both C >> and C++. > > As opposed to enforcing checking that makes sense in a C++ setting? I think that, with the current status quo, it would be best to maintain things valid in both languages. But really, can you give more specific examples of "functions used in certain contexts [required to] have external linkage"? Because I don't think that in general, allowing global functions without declarations in headers is recommended C++ style either :) Maybe what should be fixed is for -Wmissing-declarations to determine between your mentioned cases of external linkage, and the "usual" case (if it's not already capable of that). Chung-Lin
Re: [RFC] PR 53063 encode group options in .opt files
On Fri, May 18, 2012 at 12:48 AM, Chung-Lin Tang wrote: > The point here is that, a group of changes that broke C bootstrap went > in undetected for several days, because of the partially C++ default. To > prevent that in the future, we should enforce similar checking in both C > and C++. As opposed to enforcing checking that makes sense in a C++ setting?
Re: [RFC] PR 53063 encode group options in .opt files
On 2012/5/18 03:20 AM, Joseph S. Myers wrote: > On Fri, 18 May 2012, Chung-Lin Tang wrote: > >> Joseph, how does this look? It makes the default post-stage1 C++ >> bootstrap fail similarly without the other options.c Makefile change, so >> I guess it works as intended. > > For build system patches you ought to be asking the build system > maintainers for their views, rather than me. > Ok, CCing build system maintainers. Thanks, Chung-Lin
Re: [RFC] PR 53063 encode group options in .opt files
On 2012/5/18 01:51 AM, Gabriel Dos Reis wrote: > On Thu, May 17, 2012 at 12:41 PM, Gabriel Dos Reis > wrote: >> On Thu, May 17, 2012 at 12:28 PM, Manuel López-Ibáñez >> wrote: >>> On 17 May 2012 19:25, Gabriel Dos Reis >>> wrote: On Thu, May 17, 2012 at 12:19 PM, Chung-Lin Tang wrote: > On 2012/5/17 01:55 AM, Manuel López-Ibáñez wrote: >>> I'm guessing these changes are the cause of a full C bootstrap (--disable-build-poststage1-with-cxx) failure I'm seeing on trunk. The *_handle_option_auto function prototypes are not seen in options.c, and -Werror -Wmissing-prototypes are in effect (oddly, such strict checking is not enforced in the default post-stage1 C++ bootstrap) >> Yep, We should add -Wmissing-declarations to the post-stage1 flags, >> which also exists in C. Could you also add that to your patch? >> > > I'm a little unsure of how -Wmissing-declarations vs > -Wmissing-prototypes behave for C? Anyways here's a patch to add > -Wmissing-declarations for C++, keeping C as is. What is the purpose of -Wmissing-declarations for C++? >>> >>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50134 >> >> The point is: it is mostly useless for C++. The rationale for its >> existence in C are largely irrelevant in the context of C++. > > Let me add that the notion of "global function defined in header" is > largely meaningless in C++. I fear this is one of those cases where > we are trying too hard to push a C style to C++. We should not. > > The notion of "global function in C++" can only be interpreted as > meaning a function with external linkage -- because C++ has namespace > and C does not, and a there is no reason to treat the global scope > differently. > Furthermore, C++98 and C++03 requires that functions > used in certain contexts have external linkage. For GCC's own source code, > we would refrain from anonymous namespaces because they might interfer with > bootstrap compare and reproducibility. That means we are left with > having to declare > those functions right before their definitions, not but in headers; > that is just plain silly. The point here is that, a group of changes that broke C bootstrap went in undetected for several days, because of the partially C++ default. To prevent that in the future, we should enforce similar checking in both C and C++. Chung-Lin
Re: [RFC] PR 53063 encode group options in .opt files
On Fri, 18 May 2012, Chung-Lin Tang wrote: > Joseph, how does this look? It makes the default post-stage1 C++ > bootstrap fail similarly without the other options.c Makefile change, so > I guess it works as intended. For build system patches you ought to be asking the build system maintainers for their views, rather than me. -- Joseph S. Myers jos...@codesourcery.com
Re: [RFC] PR 53063 encode group options in .opt files
On Thu, May 17, 2012 at 12:41 PM, Gabriel Dos Reis wrote: > On Thu, May 17, 2012 at 12:28 PM, Manuel López-Ibáñez > wrote: >> On 17 May 2012 19:25, Gabriel Dos Reis wrote: >>> On Thu, May 17, 2012 at 12:19 PM, Chung-Lin Tang >>> wrote: On 2012/5/17 01:55 AM, Manuel López-Ibáñez wrote: >> I'm guessing these changes are the cause of a full C bootstrap >> > (--disable-build-poststage1-with-cxx) failure I'm seeing on trunk. The >> > *_handle_option_auto function prototypes are not seen in options.c, and >> > -Werror -Wmissing-prototypes are in effect (oddly, such strict checking >> > is not enforced in the default post-stage1 C++ bootstrap) > Yep, We should add -Wmissing-declarations to the post-stage1 flags, > which also exists in C. Could you also add that to your patch? > I'm a little unsure of how -Wmissing-declarations vs -Wmissing-prototypes behave for C? Anyways here's a patch to add -Wmissing-declarations for C++, keeping C as is. >>> >>> What is the purpose of -Wmissing-declarations for C++? >> >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50134 > > The point is: it is mostly useless for C++. The rationale for its > existence in C are largely irrelevant in the context of C++. Let me add that the notion of "global function defined in header" is largely meaningless in C++. I fear this is one of those cases where we are trying too hard to push a C style to C++. We should not. The notion of "global function in C++" can only be interpreted as meaning a function with external linkage -- because C++ has namespace and C does not, and a there is no reason to treat the global scope differently. Furthermore, C++98 and C++03 requires that functions used in certain contexts have external linkage. For GCC's own source code, we would refrain from anonymous namespaces because they might interfer with bootstrap compare and reproducibility. That means we are left with having to declare those functions right before their definitions, not but in headers; that is just plain silly. -- Gaby
Re: [RFC] PR 53063 encode group options in .opt files
On Thu, May 17, 2012 at 12:28 PM, Manuel López-Ibáñez wrote: > On 17 May 2012 19:25, Gabriel Dos Reis wrote: >> On Thu, May 17, 2012 at 12:19 PM, Chung-Lin Tang >> wrote: >>> On 2012/5/17 01:55 AM, Manuel López-Ibáñez wrote: > I'm guessing these changes are the cause of a full C bootstrap > > (--disable-build-poststage1-with-cxx) failure I'm seeing on trunk. The > > *_handle_option_auto function prototypes are not seen in options.c, and > > -Werror -Wmissing-prototypes are in effect (oddly, such strict checking > > is not enforced in the default post-stage1 C++ bootstrap) Yep, We should add -Wmissing-declarations to the post-stage1 flags, which also exists in C. Could you also add that to your patch? >>> >>> I'm a little unsure of how -Wmissing-declarations vs >>> -Wmissing-prototypes behave for C? Anyways here's a patch to add >>> -Wmissing-declarations for C++, keeping C as is. >> >> What is the purpose of -Wmissing-declarations for C++? > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50134 The point is: it is mostly useless for C++. The rationale for its existence in C are largely irrelevant in the context of C++. -- Gaby
Re: [RFC] PR 53063 encode group options in .opt files
On 17 May 2012 19:25, Gabriel Dos Reis wrote: > On Thu, May 17, 2012 at 12:19 PM, Chung-Lin Tang > wrote: >> On 2012/5/17 01:55 AM, Manuel López-Ibáñez wrote: I'm guessing these changes are the cause of a full C bootstrap > (--disable-build-poststage1-with-cxx) failure I'm seeing on trunk. The > *_handle_option_auto function prototypes are not seen in options.c, and > -Werror -Wmissing-prototypes are in effect (oddly, such strict checking > is not enforced in the default post-stage1 C++ bootstrap) >>> Yep, We should add -Wmissing-declarations to the post-stage1 flags, >>> which also exists in C. Could you also add that to your patch? >>> >> >> I'm a little unsure of how -Wmissing-declarations vs >> -Wmissing-prototypes behave for C? Anyways here's a patch to add >> -Wmissing-declarations for C++, keeping C as is. > > What is the purpose of -Wmissing-declarations for C++? http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50134 Cheers, Manuel.
Re: [RFC] PR 53063 encode group options in .opt files
On Thu, May 17, 2012 at 12:19 PM, Chung-Lin Tang wrote: > On 2012/5/17 01:55 AM, Manuel López-Ibáñez wrote: >>> I'm guessing these changes are the cause of a full C bootstrap >>> > (--disable-build-poststage1-with-cxx) failure I'm seeing on trunk. The >>> > *_handle_option_auto function prototypes are not seen in options.c, and >>> > -Werror -Wmissing-prototypes are in effect (oddly, such strict checking >>> > is not enforced in the default post-stage1 C++ bootstrap) >> Yep, We should add -Wmissing-declarations to the post-stage1 flags, >> which also exists in C. Could you also add that to your patch? >> > > I'm a little unsure of how -Wmissing-declarations vs > -Wmissing-prototypes behave for C? Anyways here's a patch to add > -Wmissing-declarations for C++, keeping C as is. What is the purpose of -Wmissing-declarations for C++? > > Joseph, how does this look? It makes the default post-stage1 C++ > bootstrap fail similarly without the other options.c Makefile change, so > I guess it works as intended. > > I'll commit both changes together once approved. > > Thanks, > Chung-Lin > > 2012-05-18 Chung-Lin Tang > > * configure.ac: Check for -Wmissing-declarations for C++. > * configure: Regenerate. > * Makefile.in (CXX_LOOSE_WARN): Set to @cxx_loose_warn@. > (GCC_WARN_CXXFLAGS): Add $(CXX_LOOSE_WARN).
Re: [RFC] PR 53063 encode group options in .opt files
On 2012/5/17 01:55 AM, Manuel López-Ibáñez wrote:
>> I'm guessing these changes are the cause of a full C bootstrap
>> > (--disable-build-poststage1-with-cxx) failure I'm seeing on trunk. The
>> > *_handle_option_auto function prototypes are not seen in options.c, and
>> > -Werror -Wmissing-prototypes are in effect (oddly, such strict checking
>> > is not enforced in the default post-stage1 C++ bootstrap)
> Yep, We should add -Wmissing-declarations to the post-stage1 flags,
> which also exists in C. Could you also add that to your patch?
>
I'm a little unsure of how -Wmissing-declarations vs
-Wmissing-prototypes behave for C? Anyways here's a patch to add
-Wmissing-declarations for C++, keeping C as is.
Joseph, how does this look? It makes the default post-stage1 C++
bootstrap fail similarly without the other options.c Makefile change, so
I guess it works as intended.
I'll commit both changes together once approved.
Thanks,
Chung-Lin
2012-05-18 Chung-Lin Tang
* configure.ac: Check for -Wmissing-declarations for C++.
* configure: Regenerate.
* Makefile.in (CXX_LOOSE_WARN): Set to @cxx_loose_warn@.
(GCC_WARN_CXXFLAGS): Add $(CXX_LOOSE_WARN).
Index: configure.ac
===
--- configure.ac(revision 187624)
+++ configure.ac(working copy)
@@ -347,6 +347,8 @@ ACX_PROG_CC_WARNING_OPTS(
m4_quote(m4_do([-Wstrict-prototypes -Wmissing-prototypes])),
[c_loose_warn])
ACX_PROG_CC_WARNING_OPTS(
+ m4_quote(m4_do([-Wmissing-declarations])), [cxx_loose_warn])
+ACX_PROG_CC_WARNING_OPTS(
m4_quote(m4_do([-Wmissing-format-attribute])), [strict_warn])
ACX_PROG_CC_WARNING_OPTS(
m4_quote(m4_do([-Wold-style-definition -Wc++-compat])), [c_strict_warn])
Index: Makefile.in
===
--- Makefile.in (revision 187624)
+++ Makefile.in (working copy)
@@ -157,6 +157,7 @@ coverageexts = .{gcda,gcno}
# C_STRICT_WARN is similar, with C-only warnings.
LOOSE_WARN = @loose_warn@
C_LOOSE_WARN = @c_loose_warn@
+CXX_LOOSE_WARN = @cxx_loose_warn@
STRICT_WARN = @strict_warn@
C_STRICT_WARN = @c_strict_warn@
@@ -188,7 +189,7 @@ VALGRIND_DRIVER_DEFINES = @valgrind_path_defines@
.-warn = $(STRICT_WARN)
build-warn = $(STRICT_WARN)
GCC_WARN_CFLAGS = $(LOOSE_WARN) $(C_LOOSE_WARN) $($(@D)-warn) $(if
$(filter-out $(STRICT_WARN),$($(@D)-warn)),,$(C_STRICT_WARN)) $(NOCOMMON_FLAG)
$($@-warn)
-GCC_WARN_CXXFLAGS = $(LOOSE_WARN) $($(@D)-warn) $(NOCOMMON_FLAG) $($@-warn)
+GCC_WARN_CXXFLAGS = $(LOOSE_WARN) $(CXX_LOOSE_WARN) $($(@D)-warn)
$(NOCOMMON_FLAG) $($@-warn)
# These files are to have specific diagnostics suppressed, or are not to
# be subject to -Werror:
Re: [RFC] PR 53063 encode group options in .opt files
On Thu, 17 May 2012, Chung-Lin Tang wrote: > 2012-05-17 Chung-Lin Tang > > * Makefile.in (options.c): Add options.h to included header > files, before tm.h. OK. -- Joseph S. Myers jos...@codesourcery.com
Re: [RFC] PR 53063 encode group options in .opt files
On 16 May 2012 19:47, Chung-Lin Tang wrote: > On 2012/5/10 04:53 AM, Manuel López-Ibáñez wrote: >> 2012-05-09 Manuel López-Ibáñez >> >> PR 53063 >> gcc/ >> * doc/options.texi (EnabledBy): Document >> * opts.c: Include opts.h and options.h before tm.h. >> (finish_options): Do not handle some sub-options here... >> (common_handle_option): ... instead call common_handle_option_auto >> here. >> * optc-gen.awk: Handle EnabledBy. >> * opth-gen.awk: Declare common_handle_option_auto. >> * common.opt (Wuninitialized): Use EnabledBy. Delete Init. >> (Wmaybe-uninitialized): Likewise. >> (Wunused-but-set-variable): Likewise. >> (Wunused-function): Likewise. >> (Wunused-label): Likewise. >> (Wunused-value): Likewise. >> (Wunused-variable): Likewise. >> * opt-read.awk: Create opt_numbers array. >> ada/ >> * gcc-interface/misc.c (gnat_parse_file): Move before ... >> (gnat_handle_option): ... this. Use handle_generated_option. >> c-family/ >> * c-opts.c (c_common_handle_option): Use handle_generated_option >> to enable sub-options. >> fortran/ >> * options.c: Include diagnostics.h instead of >> diagnostics-core.h. >> (set_Wall): Do not see warn_unused here. >> (gfc_handle_option): Set it here using handle_generated_option. > > I'm guessing these changes are the cause of a full C bootstrap > (--disable-build-poststage1-with-cxx) failure I'm seeing on trunk. The > *_handle_option_auto function prototypes are not seen in options.c, and > -Werror -Wmissing-prototypes are in effect (oddly, such strict checking > is not enforced in the default post-stage1 C++ bootstrap) Yep, We should add -Wmissing-declarations to the post-stage1 flags, which also exists in C. Could you also add that to your patch? > Here's a simple one-liner Makefile.in change that seems to make things > work. Okay to commit? I cannot approve, but I guess it is either this or also declare the functions in options.c, which seems overkill. Cheers, Manuel.
Re: [RFC] PR 53063 encode group options in .opt files
On 2012/5/10 04:53 AM, Manuel López-Ibáñez wrote: > 2012-05-09 Manuel López-Ibáñez > > PR 53063 > gcc/ > * doc/options.texi (EnabledBy): Document > * opts.c: Include opts.h and options.h before tm.h. > (finish_options): Do not handle some sub-options here... > (common_handle_option): ... instead call common_handle_option_auto here. > * optc-gen.awk: Handle EnabledBy. > * opth-gen.awk: Declare common_handle_option_auto. > * common.opt (Wuninitialized): Use EnabledBy. Delete Init. > (Wmaybe-uninitialized): Likewise. > (Wunused-but-set-variable): Likewise. > (Wunused-function): Likewise. > (Wunused-label): Likewise. > (Wunused-value): Likewise. > (Wunused-variable): Likewise. > * opt-read.awk: Create opt_numbers array. > ada/ > * gcc-interface/misc.c (gnat_parse_file): Move before ... > (gnat_handle_option): ... this. Use handle_generated_option. > c-family/ > * c-opts.c (c_common_handle_option): Use handle_generated_option > to enable sub-options. > fortran/ > * options.c: Include diagnostics.h instead of > diagnostics-core.h. > (set_Wall): Do not see warn_unused here. > (gfc_handle_option): Set it here using handle_generated_option. I'm guessing these changes are the cause of a full C bootstrap (--disable-build-poststage1-with-cxx) failure I'm seeing on trunk. The *_handle_option_auto function prototypes are not seen in options.c, and -Werror -Wmissing-prototypes are in effect (oddly, such strict checking is not enforced in the default post-stage1 C++ bootstrap) Here's a simple one-liner Makefile.in change that seems to make things work. Okay to commit? Thanks, Chung-Lin 2012-05-17 Chung-Lin Tang * Makefile.in (options.c): Add options.h to included header files, before tm.h. Index: Makefile.in === --- Makefile.in (revision 187594) +++ Makefile.in (working copy) @@ -2121,7 +2121,7 @@ options.c: optionlist $(srcdir)/opt-functions.awk $(srcdir)/optc-gen.awk $(AWK) -f $(srcdir)/opt-functions.awk -f $(srcdir)/opt-read.awk \ -f $(srcdir)/optc-gen.awk \ - -v header_name="config.h system.h coretypes.h tm.h" < $< > $@ + -v header_name="config.h system.h coretypes.h options.h tm.h" < $< > $@ options-save.c: optionlist $(srcdir)/opt-functions.awk $(srcdir)/opt-read.awk \ $(srcdir)/optc-save-gen.awk
Re: [RFC] PR 53063 encode group options in .opt files
On Sat, 12 May 2012, Manuel López-Ibáñez wrote: > Well, I was trying to avoid "merging" flags. Are there any examples of > other such flags that are merged? The thing is that the "merge" done > right now is really just concatenating existing flags. It doesn't work > to set Init(1) and Init(0) in two different .opt files, so it wouldn't > work to set LangEnabledBy in different files for the same option. I don't know examples of merging beyond concatenation. > One option is to parse LangEnabledBy before merging (which actually > happens in optc-gen.awk), which seems ugly, but probably is the most > straightforward solution. Would that be ok? It seems reasonable to me. -- Joseph S. Myers jos...@codesourcery.com
Re: [RFC] PR 53063 encode group options in .opt files
On 12 May 2012 18:12, Joseph S. Myers wrote: > On Sat, 12 May 2012, Manuel López-Ibáñez wrote: > >> Let's assume C-specific option -Wx enables common option -Wy. How can >> I record this information in c.opt? Using Wx LangEnables(C, Wy) does > > Wy > LangEnabledBy(C C++, Wx) > > There is no restriction on c.opt to contain only options marked as > specific to C-family languages - though it's clearly a bad idea to have > entries there with nothing at all C-family-specific about them. All the > entries for an option in all .opt files are merged by opt-gather.awk > before the other awk code deals with them. (You'd need to handle having > multiple LangEnabledBy fields on the merged option produced by > opt-gather.awk, but the principle is clear.) Well, I was trying to avoid "merging" flags. Are there any examples of other such flags that are merged? The thing is that the "merge" done right now is really just concatenating existing flags. It doesn't work to set Init(1) and Init(0) in two different .opt files, so it wouldn't work to set LangEnabledBy in different files for the same option. One option is to parse LangEnabledBy before merging (which actually happens in optc-gen.awk), which seems ugly, but probably is the most straightforward solution. Would that be ok? Do you see a better alternative? Cheers, Manuel. > -- > Joseph S. Myers > jos...@codesourcery.com
Re: [RFC] PR 53063 encode group options in .opt files
On Sat, 12 May 2012, Manuel López-Ibáñez wrote: > Let's assume C-specific option -Wx enables common option -Wy. How can > I record this information in c.opt? Using Wx LangEnables(C, Wy) does Wy LangEnabledBy(C C++, Wx) There is no restriction on c.opt to contain only options marked as specific to C-family languages - though it's clearly a bad idea to have entries there with nothing at all C-family-specific about them. All the entries for an option in all .opt files are merged by opt-gather.awk before the other awk code deals with them. (You'd need to handle having multiple LangEnabledBy fields on the merged option produced by opt-gather.awk, but the principle is clear.) -- Joseph S. Myers jos...@codesourcery.com
Re: [RFC] PR 53063 encode group options in .opt files
On 12 May 2012 17:18, Gabriel Dos Reis wrote: > On Sat, May 12, 2012 at 10:12 AM, Joseph S. Myers > wrote: >> I don't think common.opt should have LangEnabledBy listing different >> languages, though; that information should be in the front ends' .opt >> files. > > Agreed. Ideally, yes. But how? Let's set aside for a moment whether Wunused or Wall should be common or language-specific. Let's assume C-specific option -Wx enables common option -Wy. How can I record this information in c.opt? Using Wx LangEnables(C, Wy) does not work for those cases that require 2 options to enable a third, like Wextra && Wunused enabled Wunused-parameter. But lets set aside that problem also: Wx . LangEnables(C C++, Wy) but then, if Wx is common, and Wy is C/C++ specific, we have exactly the same problem as in the first case for: Wy LangEnabledBy(C C++, Wx) I don't see how we can avoid to record language-specific info in common.opt, unless we restrict which options can enable other options. With LangEnabledBy, lang-specific options won't be able to selectively enable common options. With LangEnables, common options won't be able to selectively enable lang-specific options. Both limitations seem too strict to me. Perhaps you had something in mind already? Cheers, Manuel.
Re: [RFC] PR 53063 encode group options in .opt files
On Sat, May 12, 2012 at 10:12 AM, Joseph S. Myers wrote: > I don't think common.opt should have LangEnabledBy listing different > languages, though; that information should be in the front ends' .opt > files. Agreed. > And, before we add such language-specific enabling of a > language-independent option, I'd like a good reason not to make -Wall a > common option (also having language-specific aspects) that enables > -Wunused for all languages - or else -Wall should (as a separate patch) be > made to enable -Wunused for all languages, to make the semantics simpler > and more consistent. The trouble is there is no agreement on common understanding of "-Wall" across front-ends, as the recent thread about -Wall showed. -- Gaby
Re: [RFC] PR 53063 encode group options in .opt files
On Sat, 12 May 2012, Manuel L?pez-Ib??ez wrote: > On 11 May 2012 21:23, Joseph S. Myers wrote: > > > > There's nothing wrong with having separate autogenerated functions for > > each language if you want to split things out that way, but it would seem > > simpler just to have one function called somewhere central, whatever > > option it is (common or not) that is enabling other options. > > The problem with this approach is that it is not clear how to decide > in this single function that -Wextra needs to enable a C-only option > or not. The function would need to know that it is called from the > C-FE somehow. Having a separate function for each language easily > overcomes this issue. I don't see why the function would need to know which front end it is called from. If it's called from another front end, enabling a C-only option should be harmless (do nothing because nothing will actually check the gcc_options field set by the C-only option / because no handler actually gets called for the option) > This is my current draft patch. It bootstraps and passes regression > tests. It seems flexible enough to implement almost all of Wall and > Wextra. Missing features are options that enabled by the combination > of two options (like Wunused-parameter) and options that are > conditional on something else (Wmain). > > Is this approach ok with you? Should I clean it up and provide a changelog? The approach is OK (even if I don't think separate functions are necessary, they are more modular and so logically better in that sense). I don't think common.opt should have LangEnabledBy listing different languages, though; that information should be in the front ends' .opt files. And, before we add such language-specific enabling of a language-independent option, I'd like a good reason not to make -Wall a common option (also having language-specific aspects) that enables -Wunused for all languages - or else -Wall should (as a separate patch) be made to enable -Wunused for all languages, to make the semantics simpler and more consistent. -- Joseph S. Myers jos...@codesourcery.com
Re: [RFC] PR 53063 encode group options in .opt files
On 11 May 2012 21:23, Joseph S. Myers wrote: > > There's nothing wrong with having separate autogenerated functions for > each language if you want to split things out that way, but it would seem > simpler just to have one function called somewhere central, whatever > option it is (common or not) that is enabling other options. The problem with this approach is that it is not clear how to decide in this single function that -Wextra needs to enable a C-only option or not. The function would need to know that it is called from the C-FE somehow. Having a separate function for each language easily overcomes this issue. This is my current draft patch. It bootstraps and passes regression tests. It seems flexible enough to implement almost all of Wall and Wextra. Missing features are options that enabled by the combination of two options (like Wunused-parameter) and options that are conditional on something else (Wmain). Is this approach ok with you? Should I clean it up and provide a changelog? Cheers, Manuel. > > -- > Joseph S. Myers > jos...@codesourcery.com group-options-3.diff Description: Binary data
Re: [RFC] PR 53063 encode group options in .opt files
On Fri, 11 May 2012, Manuel López-Ibáñez wrote: > >> What cases do we have where a language-independent option enables another > >> language-independent option only for some front ends? That's the only > >> case that should need a language-dependent generated function here. > > > > Wall enables Wunused in Ada Fortran and C family. Is there a reason for it not to do so for all languages? You only really need a front-end-specific generated function if there's something about the semantics that needs to be different in different front ends. > But the case of lang-indep enables lang-dependent is far more common: > Wextra enables some C-only options. That shouldn't be a problem. You can mark those options as enabled by -Wextra; at most you might need to ensure that enabling a wrong-language option this way doesn't generate the usual warning you get for using a wrong-language option on the command line. There's nothing wrong with having separate autogenerated functions for each language if you want to split things out that way, but it would seem simpler just to have one function called somewhere central, whatever option it is (common or not) that is enabling other options. -- Joseph S. Myers jos...@codesourcery.com
Re: [RFC] PR 53063 encode group options in .opt files
On 11 May 2012 19:09, Manuel López-Ibáñez wrote: > On 11 May 2012 19:04, Joseph S. Myers wrote: >> On Fri, 11 May 2012, Manuel López-Ibáñez wrote: >> >>> Great! Now we have EnabledBy for common options. Now, what should we >>> do with language specific settings? One idea could be to have >>> something like: >>> >>> LangEnabledBy(Fortran Ada, Wall) >>> >>> and then auto-generate something like: >>> >>> ada_handle_option_auto(); >>> fortran_handle_option_auto() >>> >>> which would be called by the *_handle_option() of each front-end. >>> >>> Does this sound reasonable? >> >> What cases do we have where a language-independent option enables another >> language-independent option only for some front ends? That's the only >> case that should need a language-dependent generated function here. > > Wall enables Wunused in Ada Fortran and C family. But the case of lang-indep enables lang-dependent is far more common: Wextra enables some C-only options. Cheers, Manuel.
Re: [RFC] PR 53063 encode group options in .opt files
On 11 May 2012 19:04, Joseph S. Myers wrote: > On Fri, 11 May 2012, Manuel López-Ibáñez wrote: > >> Great! Now we have EnabledBy for common options. Now, what should we >> do with language specific settings? One idea could be to have >> something like: >> >> LangEnabledBy(Fortran Ada, Wall) >> >> and then auto-generate something like: >> >> ada_handle_option_auto(); >> fortran_handle_option_auto() >> >> which would be called by the *_handle_option() of each front-end. >> >> Does this sound reasonable? > > What cases do we have where a language-independent option enables another > language-independent option only for some front ends? That's the only > case that should need a language-dependent generated function here. Wall enables Wunused in Ada Fortran and C family. Cheers, Manuel.
Re: [RFC] PR 53063 encode group options in .opt files
On Fri, 11 May 2012, Manuel López-Ibáñez wrote: > Great! Now we have EnabledBy for common options. Now, what should we > do with language specific settings? One idea could be to have > something like: > > LangEnabledBy(Fortran Ada, Wall) > > and then auto-generate something like: > > ada_handle_option_auto(); > fortran_handle_option_auto() > > which would be called by the *_handle_option() of each front-end. > > Does this sound reasonable? What cases do we have where a language-independent option enables another language-independent option only for some front ends? That's the only case that should need a language-dependent generated function here. -- Joseph S. Myers jos...@codesourcery.com
Re: [RFC] PR 53063 encode group options in .opt files
On 10 May 2012 16:05, Joseph S. Myers wrote: > On Wed, 9 May 2012, Manuel López-Ibáñez wrote: > >> 2012-05-09 Manuel López-Ibáñez >> >> PR 53063 >> gcc/ >> * doc/options.texi (EnabledBy): Document >> * opts.c: Include opts.h and options.h before tm.h. >> (finish_options): Do not handle some sub-options here... >> (common_handle_option): ... instead call common_handle_option_auto >> here. >> * optc-gen.awk: Handle EnabledBy. >> * opth-gen.awk: Declare common_handle_option_auto. >> * common.opt (Wuninitialized): Use EnabledBy. Delete Init. >> (Wmaybe-uninitialized): Likewise. >> (Wunused-but-set-variable): Likewise. >> (Wunused-function): Likewise. >> (Wunused-label): Likewise. >> (Wunused-value): Likewise. >> (Wunused-variable): Likewise. >> * opt-read.awk: Create opt_numbers array. >> ada/ >> * gcc-interface/misc.c (gnat_parse_file): Move before ... >> (gnat_handle_option): ... this. Use handle_generated_option. >> c-family/ >> * c-opts.c (c_common_handle_option): Use handle_generated_option >> to enable sub-options. >> fortran/ >> * options.c: Include diagnostics.h instead of >> diagnostics-core.h. >> (set_Wall): Do not see warn_unused here. >> (gfc_handle_option): Set it here using handle_generated_option. > > OK with the > >> \ No newline at end of file > > fixed (i.e. a newline added to the end of optc-gen.awk) and 2012 added to > copyright dates of modified files if not already there. Great! Now we have EnabledBy for common options. Now, what should we do with language specific settings? One idea could be to have something like: LangEnabledBy(Fortran Ada, Wall) and then auto-generate something like: ada_handle_option_auto(); fortran_handle_option_auto() which would be called by the *_handle_option() of each front-end. Does this sound reasonable? Cheers, Manuel.
Re: [RFC] PR 53063 encode group options in .opt files
On Wed, 9 May 2012, Manuel L?pez-Ib??ez wrote: > 2012-05-09 Manuel L?pez-Ib??ez > > PR 53063 > gcc/ > * doc/options.texi (EnabledBy): Document > * opts.c: Include opts.h and options.h before tm.h. > (finish_options): Do not handle some sub-options here... > (common_handle_option): ... instead call common_handle_option_auto here. > * optc-gen.awk: Handle EnabledBy. > * opth-gen.awk: Declare common_handle_option_auto. > * common.opt (Wuninitialized): Use EnabledBy. Delete Init. > (Wmaybe-uninitialized): Likewise. > (Wunused-but-set-variable): Likewise. > (Wunused-function): Likewise. > (Wunused-label): Likewise. > (Wunused-value): Likewise. > (Wunused-variable): Likewise. > * opt-read.awk: Create opt_numbers array. > ada/ > * gcc-interface/misc.c (gnat_parse_file): Move before ... > (gnat_handle_option): ... this. Use handle_generated_option. > c-family/ > * c-opts.c (c_common_handle_option): Use handle_generated_option > to enable sub-options. > fortran/ > * options.c: Include diagnostics.h instead of > diagnostics-core.h. > (set_Wall): Do not see warn_unused here. > (gfc_handle_option): Set it here using handle_generated_option. OK with the > \ No newline at end of file fixed (i.e. a newline added to the end of optc-gen.awk) and 2012 added to copyright dates of modified files if not already there. -- Joseph S. Myers jos...@codesourcery.com
Re: [RFC] PR 53063 encode group options in .opt files
On 9 May 2012 00:38, Joseph S. Myers wrote: > On Wed, 9 May 2012, Manuel López-Ibáñez wrote: > >> which looks correct to me. However, the build fails because now >> options.h requires input.h which requires line-map.h, which is not >> included when building for example libgcc. options.h is included by >> tm.h, so it basically appears everywhere. >> >> Any suggestions how to fix this? > > options.h already has some #if !defined(IN_LIBGCC2) && > !defined(IN_TARGET_LIBS) && !defined(IN_RTS) conditionals, so you could > arrange for some more such conditionals to be generated. This works great for libgcc, however, ada's runtime library is compiled with IN_GCC and includes options.h through tm.h, so it breaks: ../../xgcc -B../../ -c -DIN_GCC -g -O2 -W -Wall \ -iquote /home/manuel/test2/src/gcc \ -iquote . -iquote .. -iquote ../.. -iquote /home/manuel/test2/src/gcc/ada -iquote /home/manuel/test2/src/gcc -I/home/manuel/test2/src/gcc/../include \ ../rts/targext.c -o targext.o In file included from ../../options.h:3716:0, from ../../tm.h:19, from ../rts/targext.c:50: /home/manuel/test2/src/gcc/input.h:25:22: fatal error: line-map.h: No such file or directory #include "line-map.h" ^ That tm.h includes the whole options.h is bad, but I don't want to mess with it. So I am instead only including input.h if options.h was not included from tm.h. This requires surprisingly few changes. The other major issue is that if -Wunused is enabled by -Wall, it has to be enabled by handle_generated_option, so I need to modify every front-end that does that . In the future, we should also generate per-FE _auto function, but I didn't want to make this patch even larger. Bootstrapped and regression tested. OK? 2012-05-09 Manuel López-Ibáñez PR 53063 gcc/ * doc/options.texi (EnabledBy): Document * opts.c: Include opts.h and options.h before tm.h. (finish_options): Do not handle some sub-options here... (common_handle_option): ... instead call common_handle_option_auto here. * optc-gen.awk: Handle EnabledBy. * opth-gen.awk: Declare common_handle_option_auto. * common.opt (Wuninitialized): Use EnabledBy. Delete Init. (Wmaybe-uninitialized): Likewise. (Wunused-but-set-variable): Likewise. (Wunused-function): Likewise. (Wunused-label): Likewise. (Wunused-value): Likewise. (Wunused-variable): Likewise. * opt-read.awk: Create opt_numbers array. ada/ * gcc-interface/misc.c (gnat_parse_file): Move before ... (gnat_handle_option): ... this. Use handle_generated_option. c-family/ * c-opts.c (c_common_handle_option): Use handle_generated_option to enable sub-options. fortran/ * options.c: Include diagnostics.h instead of diagnostics-core.h. (set_Wall): Do not see warn_unused here. (gfc_handle_option): Set it here using handle_generated_option. group-options-2.diff Description: Binary data
Re: [RFC] PR 53063 encode group options in .opt files
On Wed, 9 May 2012, Manuel L?pez-Ib??ez wrote: > which looks correct to me. However, the build fails because now > options.h requires input.h which requires line-map.h, which is not > included when building for example libgcc. options.h is included by > tm.h, so it basically appears everywhere. > > Any suggestions how to fix this? options.h already has some #if !defined(IN_LIBGCC2) && !defined(IN_TARGET_LIBS) && !defined(IN_RTS) conditionals, so you could arrange for some more such conditionals to be generated. -- Joseph S. Myers jos...@codesourcery.com
Re: [RFC] PR 53063 encode group options in .opt files
On 6 May 2012 20:45, Joseph S. Myers wrote:
>>
>> One idea could be to have an additional auto_handle_option() that is
>> generated from the awk scripts and called after all other
>> handle_option functions. This function will populate a switch with
>> group options and the respective calls to handle_option_generated for
>> sub-options.
>>
>> Is this a good idea? Where would be the best place to call this function?
>
> That certainly seems one reasonable way to handle implications.
OK, so I implemented this in the patch below, and it generates code like:
bool
common_handle_option_auto (struct gcc_options *opts,
struct gcc_options *opts_set,
const struct cl_decoded_option *decoded,
unsigned int lang_mask, int kind,
location_t loc,
const struct cl_option_handlers *handlers,
diagnostic_context *dc)
{
size_t scode = decoded->opt_index;
int value = decoded->value;
enum opt_code code = (enum opt_code) scode;
gcc_assert (decoded->canonical_option_num_elements <= 2);
switch (code)
{
case OPT_Wuninitialized:
if (!opts_set->x_warn_maybe_uninitialized)
handle_generated_option (opts, opts_set,
OPT_Wmaybe_uninitialized, NULL, value,
lang_mask, kind, loc,
handlers, dc);
break;
case OPT_Wextra:
if (!opts_set->x_warn_uninitialized)
handle_generated_option (opts, opts_set,
OPT_Wuninitialized, NULL, value,
lang_mask, kind, loc,
handlers, dc);
break;
case OPT_Wunused:
if (!opts_set->x_warn_unused_but_set_variable)
handle_generated_option (opts, opts_set,
OPT_Wunused_but_set_variable, NULL, value,
lang_mask, kind, loc,
handlers, dc);
if (!opts_set->x_warn_unused_function)
handle_generated_option (opts, opts_set,
OPT_Wunused_function, NULL, value,
lang_mask, kind, loc,
handlers, dc);
if (!opts_set->x_warn_unused_label)
handle_generated_option (opts, opts_set,
OPT_Wunused_label, NULL, value,
lang_mask, kind, loc,
handlers, dc);
if (!opts_set->x_warn_unused_value)
handle_generated_option (opts, opts_set,
OPT_Wunused_value, NULL, value,
lang_mask, kind, loc,
handlers, dc);
if (!opts_set->x_warn_unused_variable)
handle_generated_option (opts, opts_set,
OPT_Wunused_variable, NULL, value,
lang_mask, kind, loc,
handlers, dc);
break;
default:
break;
}
return true;
}
which looks correct to me. However, the build fails because now
options.h requires input.h which requires line-map.h, which is not
included when building for example libgcc. options.h is included by
tm.h, so it basically appears everywhere.
Any suggestions how to fix this?
Cheers,
Manuel.
>
> --
> Joseph S. Myers
> jos...@codesourcery.com
group-options-2.diff
Description: Binary data
Re: [RFC] PR 53063 encode group options in .opt files
On Sun, 6 May 2012, Manuel López-Ibáñez wrote: > Wuninitialized is enabled by both Wall and Wextra. Wextra enables it > in the common part, however, Wall does it in the FE specific part > (c-family, fortran, ada). When enabled via Wall, opts_set does not get > updated. What is the best way to enable a sub-option? > > Using handle_option_generated does not set opt_set either, so the test > in finish_options_generated does not work as intended. (And the > setting of -Wall gets overridden by the setting of -Wextra). That's where the notion of distance comes in - if there's an explicit -Wuninitialized or -Wno-uninitialized option, the last one of those takes precedence, but otherwise the last -Wall / -Wno-all / -Wextra / -Wno-extra determines the setting of -Wuninitialized, but otherwise the default value applies. (I'd guess that -Werror=extra should count as a -Wextra variant - at the same distance from any options implied by -Wextra as -Wextra itself - though I'm not entirely sure.) (I don't think you actually need to record distance explicitly for these particular options. You do need to process them as they are seen, so that you can distinguish -Wall -Wno-extra and -Wno-extra -Wall.) > I could move the setting of Wall to something like what we do for > Wextra. However, this seems to me a step backwards. I think your > original idea was to drive everything through the *_handle_option > functions. Ideally, Wuninitialized should be handled like Wimplicit, > using handle_option_generated to enable suboptions. But I am not sure > what is the best way to implement this. Or in other words, what kind > of code we want to autogenerate to handle this transparently. > > One idea could be to have an additional auto_handle_option() that is > generated from the awk scripts and called after all other > handle_option functions. This function will populate a switch with > group options and the respective calls to handle_option_generated for > sub-options. > > Is this a good idea? Where would be the best place to call this function? That certainly seems one reasonable way to handle implications. -- Joseph S. Myers jos...@codesourcery.com
Re: [RFC] PR 53063 encode group options in .opt files
On 6 May 2012 13:56, Joseph S. Myers wrote: > On Sat, 5 May 2012, Manuel López-Ibáñez wrote: > >> Thanks for the hints. This is what I am currently >> bootstrapping+regtesting. It builds and works on a few manual tests. >> >> OK if it passes? >> >> 2012-05-05 Manuel López-Ibáñez >> >> PR c/53063 >> gcc/ >> * doc/options.texi (EnabledBy): Document. >> * opts.c (finish_options): Call finish_options_generated instead >> of handling some options explicitly. >> * optc-gen.awk: Handle EnabledBy. >> * opth-gen.awk: Declare finish_options_generated. >> * common.opt (Wuninitialized): Use EnabledBy. Delete Init. >> (Wunused-but-set-variable): Likewise. >> (Wunused-function): Likewise. >> (Wunused-label): Likewise. >> (Wunused-value): Likewise. >> (Wunused-variable): Likewise. >> * opt-read.awk: Create opt_numbers array. > > OK. Unfortunately, there are some issues with moving Wuninitialized to the new system. Wuninitialized is enabled by both Wall and Wextra. Wextra enables it in the common part, however, Wall does it in the FE specific part (c-family, fortran, ada). When enabled via Wall, opts_set does not get updated. What is the best way to enable a sub-option? Using handle_option_generated does not set opt_set either, so the test in finish_options_generated does not work as intended. (And the setting of -Wall gets overridden by the setting of -Wextra). I could move the setting of Wall to something like what we do for Wextra. However, this seems to me a step backwards. I think your original idea was to drive everything through the *_handle_option functions. Ideally, Wuninitialized should be handled like Wimplicit, using handle_option_generated to enable suboptions. But I am not sure what is the best way to implement this. Or in other words, what kind of code we want to autogenerate to handle this transparently. One idea could be to have an additional auto_handle_option() that is generated from the awk scripts and called after all other handle_option functions. This function will populate a switch with group options and the respective calls to handle_option_generated for sub-options. Is this a good idea? Where would be the best place to call this function? Cheers, Manuel.
Re: [RFC] PR 53063 encode group options in .opt files
On Sat, 5 May 2012, Manuel L?pez-Ib??ez wrote: > Thanks for the hints. This is what I am currently > bootstrapping+regtesting. It builds and works on a few manual tests. > > OK if it passes? > > 2012-05-05 Manuel L?pez-Ib??ez > > PR c/53063 > gcc/ > * doc/options.texi (EnabledBy): Document. > * opts.c (finish_options): Call finish_options_generated instead > of handling some options explicitly. > * optc-gen.awk: Handle EnabledBy. > * opth-gen.awk: Declare finish_options_generated. > * common.opt (Wuninitialized): Use EnabledBy. Delete Init. > (Wunused-but-set-variable): Likewise. > (Wunused-function): Likewise. > (Wunused-label): Likewise. > (Wunused-value): Likewise. > (Wunused-variable): Likewise. > * opt-read.awk: Create opt_numbers array. OK. -- Joseph S. Myers jos...@codesourcery.com
Re: [RFC] PR 53063 encode group options in .opt files
Thanks for the hints. This is what I am currently bootstrapping+regtesting. It builds and works on a few manual tests. OK if it passes? 2012-05-05 Manuel López-Ibáñez PR c/53063 gcc/ * doc/options.texi (EnabledBy): Document. * opts.c (finish_options): Call finish_options_generated instead of handling some options explicitly. * optc-gen.awk: Handle EnabledBy. * opth-gen.awk: Declare finish_options_generated. * common.opt (Wuninitialized): Use EnabledBy. Delete Init. (Wunused-but-set-variable): Likewise. (Wunused-function): Likewise. (Wunused-label): Likewise. (Wunused-value): Likewise. (Wunused-variable): Likewise. * opt-read.awk: Create opt_numbers array. On 5 May 2012 16:22, Joseph S. Myers wrote: > On Sat, 5 May 2012, Manuel López-Ibáñez wrote: > >> Comments? My knowledge of awk is basically zero, so suggestions on >> how to improve the code are very welcome. >> >> Should I cleanup the patch and submit it with a Changelog? > > Yes please. Some observations: > > * finish_options_generated should take an opts_set pointer and use that, > not initializers of -1, to determine whether particular options were > previously set. Thus you should be able to remove the Init(-1) for the > options you move to the new scheme (obviously you need to make sure > nothing else is checking for the -1 value or setting the relevant fields > without also updating the _set information; I did a spot check for > warn_unused_function and that looks OK, for example, as nothing sets it > except the -Wunused-function option via the Var flag, and the code you are > moving to the new system). > > * Rather than having the linear search in find_flags_by_name, build up a > mapping from option names to numbers in opt-read.awk. (All awk arrays are > actually associative arrays, you just need to put opt_numbers[$1] = n_opts > at an appropriate point when each option is read in.) > > -- > Joseph S. Myers > jos...@codesourcery.com group-options.diff Description: Binary data
Re: [RFC] PR 53063 encode group options in .opt files
On Sat, 5 May 2012, Manuel L?pez-Ib??ez wrote: > Comments? My knowledge of awk is basically zero, so suggestions on > how to improve the code are very welcome. > > Should I cleanup the patch and submit it with a Changelog? Yes please. Some observations: * finish_options_generated should take an opts_set pointer and use that, not initializers of -1, to determine whether particular options were previously set. Thus you should be able to remove the Init(-1) for the options you move to the new scheme (obviously you need to make sure nothing else is checking for the -1 value or setting the relevant fields without also updating the _set information; I did a spot check for warn_unused_function and that looks OK, for example, as nothing sets it except the -Wunused-function option via the Var flag, and the code you are moving to the new system). * Rather than having the linear search in find_flags_by_name, build up a mapping from option names to numbers in opt-read.awk. (All awk arrays are actually associative arrays, you just need to put opt_numbers[$1] = n_opts at an appropriate point when each option is read in.) -- Joseph S. Myers jos...@codesourcery.com
[RFC] PR 53063 encode group options in .opt files
Hi, This patch is a first step towards encoding the fact that some flags enable other flags in the .opt files. As a proof-of-concept, I have only implemented the check for a group option not overriding the value of the suboption if the suboption was set. In the future, we should handle more stuff automatically. However, I think this patch already leads to the removal of a big chunk of code. Bootstrapped and tested. I don't provide changelog and the code has some stylistic issues that I will fix if the strategy is acceptable. About the implementation, in PR53063 I proposed Wall C ObjC C++ ObjC++ Warning Enable most warning messages Enables(Wwhatever, Wfoo=2) However this would make difficult to implement options that are enabled by the combination of two options, like -Wunused -Wextra enables -Wunused-parameter. So I decided for, Wunused-value -Common Var(warn_unused_value) Init(-1) Warning +Common Var(warn_unused_value) Init(-1) Warning EnabledBy(Wunused) This can be easily extended to EnabledBy(Wunused & Wextra) or an arbitrary combination of options and logical operators. Comments? My knowledge of awk is basically zero, so suggestions on how to improve the code are very welcome. Should I cleanup the patch and submit it with a Changelog? Cheers, Manuel. group-options.diff Description: Binary data
