[PATCH 1/8 v4] Dead-field warning in structs at LTO-time
Martin Sebor
msebor@gmail.com
Thu Dec 10 16:53:51 GMT 2020
On 12/9/20 4:09 PM, Eric Gallager via Gcc-patches wrote:
> On Fri, Dec 4, 2020 at 4:58 AM Erick Ochoa <
> erick.ochoa@theobroma-systems.com> wrote:
>
>>
>> This commit includes the following components:
>>
>> Type-based escape analysis to determine structs that can be modified at
>> link-time.
>> Field access analysis to determine which fields are never read.
>>
>> The type-based escape analysis provides a list of types, that are not
>> visible outside of the current linking unit (e.g. parameter types of
>> external
>> functions).
>>
>> The field access analyses non-escaping structs for fields that
>> are not used in the linking unit and thus can be removed.
>>
>> 2020-11-04 Erick Ochoa <erick.ochoa@theobroma-systems.com>
>>
>> * Makefile.in: Add file to list of new sources.
>> * common.opt: Add new flags.
>> * ipa-type-escape-analysis.c: New file.
>> ---
>> gcc/Makefile.in | 1 +
>> gcc/common.opt | 8 +
>> gcc/ipa-type-escape-analysis.c | 3428 ++++++++++++++++++++++++++++++++
>> gcc/ipa-type-escape-analysis.h | 1152 +++++++++++
>> gcc/passes.def | 1 +
>> gcc/timevar.def | 1 +
>> gcc/tree-pass.h | 2 +
>> 7 files changed, 4593 insertions(+)
>> create mode 100644 gcc/ipa-type-escape-analysis.c
>> create mode 100644 gcc/ipa-type-escape-analysis.h
>>
>> diff --git a/gcc/Makefile.in b/gcc/Makefile.in
>> index 978a08f7b04..8b18c9217a2 100644
>> --- a/gcc/Makefile.in
>> +++ b/gcc/Makefile.in
>> @@ -1415,6 +1415,7 @@ OBJS = \
>> incpath.o \
>> init-regs.o \
>> internal-fn.o \
>> + ipa-type-escape-analysis.o \
>> ipa-cp.o \
>> ipa-sra.o \
>> ipa-devirt.o \
>> diff --git a/gcc/common.opt b/gcc/common.opt
>> index d4cbb2f86a5..85351738a29 100644
>> --- a/gcc/common.opt
>> +++ b/gcc/common.opt
>> @@ -3460,4 +3460,12 @@ fipa-ra
>> Common Report Var(flag_ipa_ra) Optimization
>> Use caller save register across calls if possible.
>> +fipa-type-escape-analysis
>> +Common Report Var(flag_ipa_type_escape_analysis) Optimization
>> +This flag is only used for debugging the type escape analysis
>> +
>> +Wdfa
>> +Common Var(warn_dfa) Init(1) Warning
>> +Warn about dead fields at link time.
>> +
>>
>
> I don't really like the name "-Wdfa" very much; could you maybe come up
> with a longer and more descriptive name instead? Say, "-Wunused-field" or
> "-Wunused-private-field" depending on the kind of field:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=72789
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92801
I second that. Also, invoke.texi needs to document the new option
and if it's only active with LTO it should mention that. I looked
to see how the warning is used so I have some more comments on that
code (I didn't review the rest of the patch).
>> +// Obtain nonescaping unaccessed fields
>> +static record_field_offset_map_t
>> +obtain_nonescaping_unaccessed_fields (tpartitions_t casting,
>> + record_field_map_t record_field_map)
>> +{
>> + bool has_fields_that_can_be_deleted = false;
>> + record_field_offset_map_t record_field_offset_map;
>> + for (std::map<tree, field_access_map_t>::iterator i
>> + = record_field_map.begin (),
>> + e = record_field_map.end ();
>> + i != e; ++i)
>> + {
>> + tree r_i = i->first;
>> + std::vector<tree> equivalence
>> + = find_equivalent_trees (r_i, record_field_map, casting);
>> + field_offsets_t field_offset;
>> + field_access_map_t original_field_map = record_field_map[r_i];
>> + keep_only_read_fields_from_field_map (original_field_map,
>> field_offset);
>> + keep_only_read_fields_from_equivalent_field_maps (equivalence,
>> + record_field_map,
>> + field_offset);
>> + // These map holds the following:
>> + // RECORD_TYPE -> unsigned (bit_pos_offset which has been read)
>> + record_field_offset_map[r_i] = field_offset;
>> + }
>> +
>> + // So now that we only have the FIELDS which are read,
>> + // we need to compute the complement...
>> +
>> + // Improve: This is tightly coupled, I need to decouple it...
>> + std::set<tree> to_erase;
>> + std::set<tree> to_keep;
>> + mark_escaping_types_to_be_deleted (record_field_offset_map, to_erase,
>> + casting);
>> + for (std::map<tree, field_offsets_t>::iterator i
>> + = record_field_offset_map.begin (),
>> + e = record_field_offset_map.end ();
>> + i != e; ++i)
>> + {
>> + tree record = i->first;
>> + const bool will_be_erased = to_erase.find (record) !=
>> to_erase.end ();
>> + // No need to compute which fields can be deleted if type is
>> escaping
>> + if (will_be_erased)
>> + continue;
>> +
>> + field_offsets_t field_offset = i->second;
>> + for (tree field = TYPE_FIELDS (record); field; field = DECL_CHAIN
>> (field))
>> + {
>> + unsigned f_offset = bitpos_of_field (field);
>> + bool in_set2 = field_offset.find (f_offset) != field_offset.end
>> ();
>> + if (in_set2)
>> + {
>> + field_offset.erase (f_offset);
>> + continue;
>> + }
>> + to_keep.insert (record);
>> + field_offset.insert (f_offset);
>> + has_fields_that_can_be_deleted = true;
>> + // NOTE: With anonymous fields this might be weird to print.
>> + log ("%s.%s may be deleted\n",
>> + type_stringifier::get_type_identifier (record).c_str (),
>> + type_stringifier::get_field_identifier (field).c_str ());
>> +
>> + if (OPT_Wdfa == 0) continue;
OPT_Wdfa is an enumerator so the above will never evaluate to true.
The warning() call checks to make sure a warning option is active
so checks like the above (which would otherwise use the warn_xxx
variable) aren't necessary, or really even reliable because they
don't faitfully reflect the state of warnings changed by #pragma
GCC diagnostic.
>> + // Anonymous fields? (Which the record can be!).
>> + warning (OPT_Wdfa, "RECORD_TYPE %qE has dead field %qE in
>> LTO.\n",
>> + record, field);
The warning() call should really be warning_at() with the location
of the field (DECL_SOURCE_LOCATION (field). Otherwise I don't think
the location of this warning would be meaningful.
The RECORD_TYPE bit is a GCC-internal detail that shouldn't be
exposed in user-visible diagnostics. If record is a type, %qT
should be used to format it (the directive mentions the class-id
('struct' or 'union' or 'class' in C++).
Warning text should also not end in a period or newline. Most
of these formatting conventions are summarized in
https://www.gnu.org/prep/standards/standards.html#Errors
and
https://gcc.gnu.org/codingconventions.html#Diagnostics
If the warning isn't effective without LTO then specifying
the option without -flto should trigger a warning pointing it
out. Otherwise users who expect it to work might be misled to
believe that their code is warning free.
Martin
More information about the Gcc-patches
mailing list