On Sat, 2025-05-31 at 23:25 -0400, Jason Merrill wrote:
> From f57505c7c6fa05a14e285c14a81021655a43ccbc Mon Sep 17 00:00:00
> 2001
> From: Jason Merrill <ja...@redhat.com>
> Date: Wed, 20 Nov 2024 16:20:52 +0100
> Subject: [PATCH] c++: modules and #pragma diagnostic
> To: gcc-patches@gcc.gnu.org
>
> 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 give serializers direct access to the
> underlying
> data, as here with get_classification_history. 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.
>
> gcc/ChangeLog:
>
> * diagnostic.h (diagnostic_option_classifier): Friend
> diagnostic_context.
> (diagnostic_context::get_classification_history): New.
>
> gcc/cp/ChangeLog:
>
> * module.cc (module_state::write_diagnostic_classification):
> New.
> (module_state::write_begin): Call it.
> (module_state::read_diagnostic_classification): New.
> (module_state::read_initial): Call it.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/warn-spec-3_a.C: New test.
> * g++.dg/modules/warn-spec-3_b.C: New test.
> ---
> gcc/diagnostic.h | 10 +++
> gcc/cp/module.cc | 86
> +++++++++++++++++++-
> gcc/testsuite/g++.dg/modules/warn-spec-3_a.C | 20 +++++
> gcc/testsuite/g++.dg/modules/warn-spec-3_b.C | 8 ++
> 4 files changed, 123 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/g++.dg/modules/warn-spec-3_a.C
> create mode 100644 gcc/testsuite/g++.dg/modules/warn-spec-3_b.C
>
> diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h
> index cdd6f26ba2a..b6cab0d52ed 100644
> --- a/gcc/diagnostic.h
> +++ b/gcc/diagnostic.h
> @@ -307,6 +307,9 @@ private:
> diagnostic. */
> vec<diagnostic_classification_change_t> m_classification_history;
>
> + /* For diagnostic_context::get_classification_history, declared
> later. */
> + friend class diagnostic_context;
> +
> /* For pragma push/pop. */
> vec<int> m_push_list;
> };
> @@ -830,6 +833,13 @@ public:
> m_abort_on_error = val;
> }
>
> + /* Accessor for use in serialization, e.g. by C++ modules. */
> + auto &
> + get_classification_history ()
> + {
> + return m_option_classifier.m_classification_history;
> + }
> +
> private:
> void error_recursion () ATTRIBUTE_NORETURN;
>
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 2fa92b514d6..3f88baa7544 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -3879,6 +3879,9 @@ class GTY((chain_next ("%h.parent"), for_user))
> module_state {
> void write_macro_maps (elf_out *to, range_t &, unsigned *crc_ptr);
> bool read_macro_maps (line_map_uint_t);
>
> + void write_diagnostic_classification (elf_out *,
> diagnostic_context *, unsigned *);
> + bool read_diagnostic_classification (diagnostic_context *);
> +
> private:
> void write_define (bytes_out &, const cpp_macro *);
> cpp_macro *read_define (bytes_in &, cpp_reader *) const;
> @@ -18181,6 +18184,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->get_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);
> + }
> + }
> +
> + 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->get_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 ();
Thanks for updating the "friend" stuff.
> + if (kind == DK_POP)
> + opt += offset;
I'm wondering why the offset is applied to "opt" here? That feels
wrong to me, or, at least, I couldn't see what it's trying to do, which
got me thinking about the DK_POP case.
Does diagnostic_option_classifier's m_push_list get updated when
reading classification changes from the module?
What happens if you have a module that pushes a diagnostic
classification change, and later pops it? (analogous to a header that
temporarily suppresses a warning, if I understand correctly) If that's
reasonable to do, it would be good to have a test case for it, since I
don't think the existing test cases are covering the DK_POP case.
Probably less reasonable: what if a module pushes a diagnostics
classification change, but *doesn't* later pop it. Can the change be
popped by consumers of the module?
[...snip...]
Hope this is constructive; sorry if I'm missing something here
Dave
