Building GCC with -Wmissing-declarations and addressing its warnings

Building GCC with -Wmissing-declarations and addressing its warnings

[Patrick Palka]

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<Type, Callback>()
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-prefetch.c (tree_ssa_prefetch_arrays):
        Likewise.
        * tree-ssa-loop-unswitch.c (tree_ssa_unswitch_loops): Likewise.
        * tree-ssa-threadupdate.c (ssa_fix_duplicate_block_edges):
        Likewise.
        * tree-vect-loop-manip.c (vect_create_cond_for_alias_checks):
        Likewise.
        * ubsan.c (tree_type_map_hash): Likewise.
        * varpool.c (varpool_call_variable_insertion_hooks): Likewise.

Re: Building GCC with -Wmissing-declarations and addressing its warnings

[Jonathan Wakely]

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.

Re: Building GCC with -Wmissing-declarations and addressing its warnings

[Patrick Palka]

On Thu, Feb 20, 2014 at 2:16 AM, Jonathan Wakely <jwakely.gcc@gmail.com> 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

[Jonathan Wakely]

On 20 February 2014 10:02, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Thu, Feb 20, 2014 at 2:16 AM, Jonathan Wakely <jwakely.gcc@gmail.com> 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

[Patrick Palka]

On Thu, Feb 20, 2014 at 7:42 AM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> On 20 February 2014 10:02, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> On Thu, Feb 20, 2014 at 2:16 AM, Jonathan Wakely <jwakely.gcc@gmail.com> 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

[Richard Sandiford]

Patrick Palka <patrick@parcs.ath.cx> 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

[Jonathan Wakely]

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]

On Thu, Feb 20, 2014 at 1:14 PM, Jonathan Wakely <jwakely.gcc@gmail.com> 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

[Jonathan Wakely]

On 20 February 2014 18:16, Patrick Palka wrote:
> On Thu, Feb 20, 2014 at 1:14 PM, Jonathan Wakely <jwakely.gcc@gmail.com> 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.)