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?
> > > +
> > > + 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
