On Tue, 27 May 2025, David Malcolm wrote:
> On Fri, 2025-05-23 at 16:58 -0400, Jason Merrill wrote:
> > On 4/14/25 9:57 AM, Jason Merrill wrote:
> > > On 1/9/25 10:00 PM, Jason Merrill wrote:
> > > > Tested x86_64-pc-linux-gnu. Is the diagnostic.h change OK for
> > > > trunk?
> > >
> > > Ping?
> >
> > Ping.
>
> Sorry for the delay in responding; comments below...
>
> >
> > > > -- 8< --
> > > >
> > > > To respect the #pragma diagnostic lines in libstdc++ headers when
> > > > compiling
> > > > with module std, we need to represent them in the module.
> > > >
> > > > I think it's reasonable to make module_state a friend of
> > > > diagnostic_option_classifier to allow direct access to the data.
> > > > This
> > > > is a
> > > > different approach from how Jakub made PCH streaming members of
> > > > diagnostic_option_classifier, but it seems to me that modules
> > > > handling
> > > > belongs in module.cc.
>
> Putting it in module.cc looks good to me, though perhaps it should be
> just a friend of diagnostic_option_classifier but not of
> diagnostic_context? Could the functions take a
> diagnostic_option_classifier rather than a diagnostic_context?
> diagnostic_context is something of a "big blob" of a class.
>
> [...snip...]
>
> > > > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > > > index 78fb21dc22f..49c9c092163 100644
> > > > --- a/gcc/cp/module.cc
> > > > +++ b/gcc/cp/module.cc
>
> [...snip...]
>
> > > > @@ -17637,6 +17640,78 @@ module_state::write_ordinary_maps (elf_out
> > > > *to, range_t &info,
> > > > dump.outdent ();
> > > > }
> > > > +/* Write out any #pragma GCC diagnostic info to the .dgc section. */
> > > > +
> > > > +void
> > > > +module_state::write_diagnostic_classification (elf_out *to,
> > > > + diagnostic_context *dc,
> > > > + unsigned *crc_p)
> > > > +{
> > > > + auto &changes = dc->m_option_classifier.m_classification_history;
> > > > +
> > > > + dump () && dump ("Writing diagnostic change locations");
> > > > + dump.indent ();
> > > > +
> > > > + bytes_out sec (to);
> > > > + if (sec.streaming_p ())
> > > > + sec.begin ();
> > > > +
> > > > + unsigned len = changes.length ();
> > > > + dump () && dump ("Diagnostic changes: %u", len);
> > > > + if (sec.streaming_p ())
> > > > + sec.u (len);
> > > > +
> > > > + for (const auto &c: changes)
> > > > + {
> > > > + write_location (sec, c.location);
> > > > + if (sec.streaming_p ())
> > > > + {
> > > > + sec.u (c.option);
> > > > + sec.u (c.kind);
> > > > + }
> > > > + }
>
> I confess I don't fully understand the module code yet - in particular
> the streaming vs non-streaming distinction. What are the "if
> (sec.streaming_p ())" guards doing here? It looks it can be false if
> the param "elf_out *to" is null (can that happen?), and if it's false,
> then this function essentially becomes a no-op. Is that what we want?
When streaming_p is false then we're not serializing, we're just doing
dependency analysis, the same walking code is used for both. The trees_out
class definition has the following comment:
/* The walk is used for three similar purposes:
1. The initial scan for dependencies.
2. Once dependencies have been found, ordering them.
3. Writing dependencies to file (streaming_p).
For cases where it matters, these accessers can be used to determine
which state we're in. */
But it seems we don't need to check it here, since streaming_p will always
be true at the current call sites of write_diagnostic_classification?
>
>
> > > > +
> > > > + if (sec.streaming_p ())
> > > > + sec.end (to, to->name (MOD_SNAME_PFX ".dgc"), crc_p);
> > > > + dump.outdent ();
> > > > +}
> > > > +
> > > > +/* Read any #pragma GCC diagnostic info from the .dgc section. */
> > > > +
> > > > +bool
> > > > +module_state::read_diagnostic_classification (diagnostic_context *dc)
> > > > +{
> > > > + bytes_in sec;
> > > > +
> > > > + if (!sec.begin (loc, from (), MOD_SNAME_PFX ".dgc"))
> > > > + return false;
> > > > +
> > > > + dump () && dump ("Reading diagnostic change locations");
> > > > + dump.indent ();
> > > > +
> > > > + unsigned len = sec.u ();
> > > > + dump () && dump ("Diagnostic changes: %u", len);
> > > > +
> > > > + auto &changes = dc->m_option_classifier.m_classification_history;
> > > > + unsigned offset = changes.length ();
> > > > + changes.reserve (len);
> > > > + for (unsigned i = 0; i < len; ++i)
> > > > + {
> > > > + location_t loc = read_location (sec);
> > > > + int opt = sec.u ();
> > > > + diagnostic_t kind = (diagnostic_t) sec.u ();
> > > > + if (kind == DK_POP)
> > > > + opt += offset;
> > > > + changes.quick_push ({ loc, opt, kind });
> > > > + }
> > > > +
> > > > + dump.outdent ();
> > > > + if (!sec.end (from ()))
> > > > + return false;
> > > > +
> > > > + return true;
> > > > +}
> > > > +
> > > > void
> > > > module_state::write_macro_maps (elf_out *to, range_t &info, unsigned
> > > > *crc_p)
> > > > {
> > > > @@ -19231,6 +19306,8 @@ module_state::write_begin (elf_out *to,
> > > > cpp_reader *reader,
> > > > if (is_header ())
> > > > macros = prepare_macros (reader);
> > > > + write_diagnostic_classification (nullptr, global_dc, nullptr);
> > > > +
> > > > config.num_imports = mod_hwm;
> > > > config.num_partitions = modules->length () - mod_hwm;
> > > > auto map_info = write_prepare_maps (&config, bool
> > > > (config.num_partitions));
> > > > @@ -19372,7 +19449,10 @@ module_state::write_begin (elf_out *to,
> > > > cpp_reader *reader,
> > > > /* Write the line maps. */
> > > > if (config.ordinary_locs)
> > > > - write_ordinary_maps (to, map_info, bool (config.num_partitions),
> > > > &crc);
> > > > + {
> > > > + write_ordinary_maps (to, map_info, bool
> > > > (config.num_partitions), &crc);
> > > > + write_diagnostic_classification (to, global_dc, &crc);
> > > > + }
> > > > if (config.macro_locs)
> > > > write_macro_maps (to, map_info, &crc);
> > > > @@ -19505,6 +19585,10 @@ module_state::read_initial (cpp_reader *reader)
> > > > else if (!read_macro_maps (config.macro_locs))
> > > > ok = false;
> > > > + if (ok && have_locs && config.ordinary_locs
> > > > + && !read_diagnostic_classification (global_dc))
> > > > + ok = false;
> > > > +
> > > > /* Note whether there's an active initializer. */
> > > > active_init_p = !is_header () && bool (config.active_init);
>
> [...snip...]
>
> Hope this is constructive.
>
> Dave
>
>
