Re: Building GCC with -Wmissing-declarations and addressing its warnings
On 20 February 2014 18:16, Patrick Palka wrote: > On Thu, Feb 20, 2014 at 1:14 PM, Jonathan Wakely > wrote: >> On 20 February 2014 15:31, Patrick Palka wrote: >>> (I counted nearly 100 (non-debug) >>> functions that could be made static in gcc, and 4 in libstdc++, by the >>> way.) >> >> Which were the four in libstdc++? >> >> I only see __gslice_on_index and __concat_size_t. > > Those two along with _Rb_tree_rotate_left and _Rb_tree_rotate_right. See the comments above those functions. (That mistake shouldn't happen again, because we have the check-abi test to catch it.)
Re: Building GCC with -Wmissing-declarations and addressing its warnings
On Thu, Feb 20, 2014 at 1:14 PM, Jonathan Wakely wrote: > On 20 February 2014 15:31, Patrick Palka wrote: >> (I counted nearly 100 (non-debug) >> functions that could be made static in gcc, and 4 in libstdc++, by the >> way.) > > Which were the four in libstdc++? > > I only see __gslice_on_index and __concat_size_t. Those two along with _Rb_tree_rotate_left and _Rb_tree_rotate_right.
Re: Building GCC with -Wmissing-declarations and addressing its warnings
On 20 February 2014 15:31, Patrick Palka wrote: > (I counted nearly 100 (non-debug) > functions that could be made static in gcc, and 4 in libstdc++, by the > way.) Which were the four in libstdc++? I only see __gslice_on_index and __concat_size_t.
Re: Building GCC with -Wmissing-declarations and addressing its warnings
Patrick Palka writes: >> Maybe others will disagree and will think enabling >> -Wmissing-declarations would be a useful change, but I don't see the >> point. > > In my novice opinion, I think the flag helps keep source files tidy > and modular, and their interfaces well-defined. Its biggest benefit > is having the compiler inform you when a function should have been > marked static: marking a function static facilitates better > optimization and static analysis, and it helps convey the intent of > the function to the reader. +1 FWIW, though only for the compiler proper, since I wouldn't know either way about libstdc++. If GCC does become used as JIT in future then we might need to start worrying about ABI stability and so check the public symbols against a whitelist, like libstdc++ does. That's a stronger test, but probably too strong for GCC ATM. I think the other case you mentioned -- .c(c)s not including their own .hs -- is important too, especially since there's ongoing work to refactor the header files. In both cases, the option forces you to think about whether the routine really is public and so should be declared in a .h, or whether it's internal to the TU. Thanks, Richard
Re: Building GCC with -Wmissing-declarations and addressing its warnings
On Thu, Feb 20, 2014 at 7:42 AM, Jonathan Wakely wrote: > On 20 February 2014 10:02, Patrick Palka wrote: >> On Thu, Feb 20, 2014 at 2:16 AM, Jonathan Wakely >> wrote: >>> On 13 February 2014 20:47, Patrick Palka wrote: On a related note, would a patch to officially enable -Wmissing-declarations in the build process be well regarded? >>> >>> What would be the advantage? >> >> A missing declaration for an extern function definition likely means >> that the function should be marked static instead, so enabling the >> flag would help detect whether a function should otherwise be given >> static linkage. Isn't this especially important in libstdc++, where >> accidentally exposing an internal symbol in the library's ABI means >> having to keep the symbol around until the next ABI bump? > > It's not really an issue for libstdc++. All symbols are internal by > default and we only export specifically chosen symbols. If a new > symbol accidentally matches an existing pattern in the linker script > then it will be noticed by the testsuite ('make check-abi' in the > $objdir/$target/libstdc++-v3 dir) and we will make the pattern more > specific. Ah, ok, that makes sense. > > Making some functions static might be worthwhile, but for the other > ones referred to in this quote from your first email: > >> The rest of the >> fixes are on global function definitions whose declaration exists in a >> header file that was not included by the source file. To fix up these >> functions I simply included the relevant header file. > > The only advantage of this change is that if the definition is changed > without also changing the header then you might get a failure during > compilation rather than linking, but even that isn't guaranteed as the > new definition could be interpreted as an overload rather than an > incompatible declaration. Wouldn't the overload require a separate declaration, which would be missing, and thus the compiler would still emit the warning? > > Otherwise, including the header isn't *wrong* but it doesn't really > gain much, except silencing a warning, and that warning is only > emitted because you turned it on :-) And as you also said, some files > can't easily be fixed to avoid the warning. IMHO the simplest way to > solve the problem is not turn the warnings on! For what it's worth, I have fixed the files that I originally regarded as not easily fixable. They were pretty easy to fix after a little bit of thought. > > Maybe others will disagree and will think enabling > -Wmissing-declarations would be a useful change, but I don't see the > point. In my novice opinion, I think the flag helps keep source files tidy and modular, and their interfaces well-defined. Its biggest benefit is having the compiler inform you when a function should have been marked static: marking a function static facilitates better optimization and static analysis, and it helps convey the intent of the function to the reader. (I counted nearly 100 (non-debug) functions that could be made static in gcc, and 4 in libstdc++, by the way.)
Re: Building GCC with -Wmissing-declarations and addressing its warnings
On 20 February 2014 10:02, Patrick Palka wrote: > On Thu, Feb 20, 2014 at 2:16 AM, Jonathan Wakely > wrote: >> On 13 February 2014 20:47, Patrick Palka wrote: >>> On a related note, would a patch to officially enable >>> -Wmissing-declarations in the build process be well regarded? >> >> What would be the advantage? > > A missing declaration for an extern function definition likely means > that the function should be marked static instead, so enabling the > flag would help detect whether a function should otherwise be given > static linkage. Isn't this especially important in libstdc++, where > accidentally exposing an internal symbol in the library's ABI means > having to keep the symbol around until the next ABI bump? It's not really an issue for libstdc++. All symbols are internal by default and we only export specifically chosen symbols. If a new symbol accidentally matches an existing pattern in the linker script then it will be noticed by the testsuite ('make check-abi' in the $objdir/$target/libstdc++-v3 dir) and we will make the pattern more specific. Making some functions static might be worthwhile, but for the other ones referred to in this quote from your first email: > The rest of the > fixes are on global function definitions whose declaration exists in a > header file that was not included by the source file. To fix up these > functions I simply included the relevant header file. The only advantage of this change is that if the definition is changed without also changing the header then you might get a failure during compilation rather than linking, but even that isn't guaranteed as the new definition could be interpreted as an overload rather than an incompatible declaration. Otherwise, including the header isn't *wrong* but it doesn't really gain much, except silencing a warning, and that warning is only emitted because you turned it on :-) And as you also said, some files can't easily be fixed to avoid the warning. IMHO the simplest way to solve the problem is not turn the warnings on! Maybe others will disagree and will think enabling -Wmissing-declarations would be a useful change, but I don't see the point.
Re: Building GCC with -Wmissing-declarations and addressing its warnings
On Thu, Feb 20, 2014 at 2:16 AM, Jonathan Wakely wrote: > On 13 February 2014 20:47, Patrick Palka wrote: >> On a related note, would a patch to officially enable >> -Wmissing-declarations in the build process be well regarded? > > What would be the advantage? A missing declaration for an extern function definition likely means that the function should be marked static instead, so enabling the flag would help detect whether a function should otherwise be given static linkage. Isn't this especially important in libstdc++, where accidentally exposing an internal symbol in the library's ABI means having to keep the symbol around until the next ABI bump? > >> Since >> -Wmissing-prototypes is currently enabled, I assume it is the >> intention of the GCC devs to address these warnings, and that during >> the transition from a C to C++ bootstrap compiler a small oversight >> was made (that -Wmissing-prototypes is a no-op against C++ source >> files). > > The additional safety provided by -Wmissing-prototypes is already > guaranteed for C++. > > In C a missing prototype causes the compiler to guess, probably > incorrectly, how to call the function. > > In C++ a function cannot be called without a previous declaration and > the linker will notice if you declare a function with one signature > and define it differently. Good point.. -Wmissing-declarations does not provide additional safety to C++. Really the only benefit to the flag is the static thing mentioned above.
Re: Building GCC with -Wmissing-declarations and addressing its warnings
On 13 February 2014 20:47, Patrick Palka wrote: > On a related note, would a patch to officially enable > -Wmissing-declarations in the build process be well regarded? What would be the advantage? > Since > -Wmissing-prototypes is currently enabled, I assume it is the > intention of the GCC devs to address these warnings, and that during > the transition from a C to C++ bootstrap compiler a small oversight > was made (that -Wmissing-prototypes is a no-op against C++ source > files). The additional safety provided by -Wmissing-prototypes is already guaranteed for C++. In C a missing prototype causes the compiler to guess, probably incorrectly, how to call the function. In C++ a function cannot be called without a previous declaration and the linker will notice if you declare a function with one signature and define it differently.
Building GCC with -Wmissing-declarations and addressing its warnings
Hi everyone, I noticed that the GCC build process currently only uses the -Wmissing-prototypes flag, and not the -Wmissing-declarations flag. It seems that the former flag only works on C source files, which means that GCC's source files no longer benefit from this flag as they are now C++ files. The right flag to use, in this case, is -Wmissing-declarations, which works on both C and C++ source files. I decided to build GCC with this flag to see what kinds of warnings popped up, and to use these warnings to clean up the GCC source. I sifted through all the new warnings generated by -Wmissing-declarations during the build process and fixed the ones whose fixes were obvious. Most of my fixes are on global (non-debug) functions that are only referenced in the compilation unit in which they are defined. To fix these functions and to silence their warnings I have simply gave them static linkage. The rest of the fixes are on global function definitions whose declaration exists in a header file that was not included by the source file. To fix up these functions I simply included the relevant header file. The -Wmissing-declarations warnings that I did _not_ address are those emitted from the autogenerated gengtype header files, because their fixes are not trivial to me. They look like: In file included from ../../gcc/gcc/c/c-parser.c:14162:0: ./gt-c-c-parser.h: In function 'void gt_ggc_mx(c_token&)': ./gt-c-c-parser.h:50:1: warning: no previous declaration for 'void gt_ggc_mx(c_token&)' [-Wmissing-declarations] gt_ggc_mx (struct c_token& x_r ATTRIBUTE_UNUSED) I have also not addressed some of such warnings in predict.c and config/i386/i386.c because their fixes are not trivial to me either. Furthermore, I was not able to mark "static" any function that was used as a template argument to hash_table::traverse() because it seems that C++98 requires template argument pointers to have external linkage. (The C++11 standard relaxes this restriction, it seems.) The file var-tracking.c has many of such functions. Since I do not yet have a copyright assignment filed for GCC, I have omitted an actual code patch and instead provide you with a changelog that could be used to reconstruct the patch 100% if anyone is so inclined. Once my copyright assignment is filed, I will properly submit this patch if it is not yet done so by somebody else. On a related note, would a patch to officially enable -Wmissing-declarations in the build process be well regarded? Since -Wmissing-prototypes is currently enabled, I assume it is the intention of the GCC devs to address these warnings, and that during the transition from a C to C++ bootstrap compiler a small oversight was made (that -Wmissing-prototypes is a no-op against C++ source files). If the answer to the previous question is "yes" then how would one go about addressing the above gengtype-related warnings, if at all? Thanks for your time, Patrick * asan.c (asan_mem_ref_get_end): Make static. * calls.c: Include calls.h. * cfgexpand.c: Include cfgexpand.h. * cfgloop.c: Include tree-ssa-loop-niter.h. * cfgraphunit.c (decide_is_symbol_needed): Make static. * config/i386/i386.c (make_pass_insert_vzeroupper): Likewise. (ix86_avx_emit_vzeroupper): Likewise. * dwarf2out.c (init_addr_table_entry): Likewise. * gimple-builder.c: Include gimple-builder.h. * gimple-ssa-isolate-paths.c (isolate_path): Make static. * graphite.c (graphite_transform_loops): Likewise. * internal-fn.c (ubsan_expand_si_overflow_addsub_check): Make static. (ubsan_expand_si_overflow_neg_check): Likewise. (ubsan_expand_si_overflow_mul_check): Likewise. * ipa-devirt.c (hash_type_name): Likewise. (likely_target_p): Likewise. * ipa-inline-analysis.c (simple_edge_hints): Likewise. * ipa-profile.c (cmp_counts): Likewise. (contains_hot_call_p): Likewise. * ipa-prop.c (ipa_alloc_node_params): Likewise. (write_agg_replacement_chain): Likewise. * ipa.c (can_replace_by_local_alias): Likewise. * lto-streamer-out.c (output_symbol_p): Likewise. * omp-low.c (simd_clone_vector_of_formal_parm_types): Likewise. * print-tree.c: Include print-tree.h. * stmt.c: Include stmt.h. * stringpool.c: Include stringpool.h. * tree-cfg-cleanup.c: Include tree-cfg-cleanup.h. * tree-inline.c (redirect_all_calls): Make static. (freqs_to_count): Likewise. * tree-nested.c: Include tree-nested.h. * tree-predcom.c (tree_predictive_commoning): Make static. * tree-sra.c (ipa_sra_modify_function_body): Likewise. * tree-ssa-loop-im.c (movement_possibility): Likewise. (tree_ssa_lim): Likewise. * tree-ssa-loop-ivcanon.c (canonicalize_induction_variables): Likewise. (tree_unroll_loops_completely): Likewise. * tree-ssa-loop-prefet