On 6/3/25 7:02 PM, David Malcolm wrote:
On Sat, 2025-05-31 at 23:25 -0400, Jason Merrill wrote:
+ 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.
DK_POP returns to the state at the specified index:
/* For DK_POP, this is the index of the corresponding push (as stored
in m_push_list).
Otherwise, this is an option index. */
int option;
...and since we're appending to the vector here, the index in the
interface TU needs to be adjusted for any entries that might have
already been in the vector before we started appending. I've added more
comments.
Does diagnostic_option_classifier's m_push_list get updated when
reading classification changes from the module?
That's not necessary; m_push_list is only for remembering the index we
want to return to when we later see a pop, which is not relevant to an
importer.
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.
There's a lot of push/pop in the libstdc++ headers that this change was
motivated by, including <initializer_list> that's included by the new
test. So when writing out the changes from warn-spec-3_a.C we have
Diagnostic changes: 8
Index 0: ignored: -Wvariadic-macros
Index 1: ignored: -Wc++23-extensions
Index 2: pop from 0
Index 3: ignored: -Wvariadic-macros
Index 4: ignored: -Wc++11-extensions
Index 5: ignored: -Wc++23-extensions
Index 6: pop from 3
Index 7: ignored: -Winit-list-lifetime
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?
That's not something we want to support; importing a module should not
change the diagnostic configuration in the importer.
But yes, the reader should make sure that any changes are popped at the
end of an import. Fixed, and tested in warn-spec-3_c.C.
This revision also avoids re-exporting imported entries and adds detail
to -fdump-lang-module.
Jason
From 5421eebade9a9e25d951f0bed3e38f14486bf350 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.
libcpp/ChangeLog:
* line-map.cc (linemap_location_from_module_p): Add.
* include/line-map.h: Declare it.
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.
(dk_string, dump_dc_change): New.
gcc/testsuite/ChangeLog:
* g++.dg/modules/warn-spec-3_a.C: New test.
* g++.dg/modules/warn-spec-3_b.C: New test.
* g++.dg/modules/warn-spec-3_c.C: New test.
---
gcc/diagnostic.h | 10 ++
libcpp/include/line-map.h | 4 +
gcc/cp/module.cc | 177 ++++++++++++++++++-
gcc/testsuite/g++.dg/modules/warn-spec-3_a.C | 20 +++
gcc/testsuite/g++.dg/modules/warn-spec-3_b.C | 7 +
gcc/testsuite/g++.dg/modules/warn-spec-3_c.C | 12 ++
libcpp/line-map.cc | 11 ++
7 files changed, 240 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
create mode 100644 gcc/testsuite/g++.dg/modules/warn-spec-3_c.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/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 83ebbde474d..21a59af2236 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1111,6 +1111,10 @@ extern location_t linemap_module_loc
extern void linemap_module_reparent
(line_maps *, location_t loc, location_t new_parent);
+/* TRUE iff the location comes from a module import. */
+extern bool linemap_location_from_module_p
+ (const line_maps *, location_t);
+
/* Restore the linemap state such that the map at LWM-1 continues.
Return start location of the new map. */
extern location_t linemap_module_restore
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index e2ad2e88a6e..9de802461c1 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -3879,6 +3879,10 @@ 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 +18185,168 @@ module_state::write_ordinary_maps (elf_out *to, range_t &info,
dump.outdent ();
}
+/* Return the prefix to use for dumping a #pragma diagnostic change to DK. */
+
+static const char *
+dk_string (diagnostic_t dk)
+{
+ gcc_assert (dk > DK_UNSPECIFIED && dk < DK_LAST_DIAGNOSTIC_KIND);
+ if (dk == DK_IGNORED)
+ /* diagnostic.def has an empty string for ignored. */
+ return "ignored: ";
+ else
+ return get_diagnostic_kind_text (dk);
+}
+
+/* Dump one #pragma GCC diagnostic entry. */
+
+static bool
+dump_dc_change (unsigned index, unsigned opt, diagnostic_t dk)
+{
+ if (dk == DK_POP)
+ return dump (" Index %u: pop from %d", index, opt);
+ else
+ return dump (" Index %u: %s%s", index, dk_string (dk),
+ cl_options[opt].opt_text);
+}
+
+/* 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 ();
+
+ bytes_out sec (to);
+ if (sec.streaming_p ())
+ {
+ sec.begin ();
+ dump () && dump ("Writing diagnostic change locations");
+ dump.indent ();
+ }
+
+ unsigned len = changes.length ();
+
+ /* We don't want to write out any entries that came from one of our imports.
+ But then we need to adjust the total, and change DK_POP targets to match
+ the index in our actual output. So remember how many lines we had skipped
+ at each step, where -1 means this line itself is skipped. */
+ int skips = 0;
+ auto_vec<int> skips_at (len);
+ skips_at.safe_grow (len);
+
+ for (unsigned i = 0; i < len; ++i)
+ {
+ const auto &c = changes[i];
+ skips_at[i] = skips;
+ if (linemap_location_from_module_p (line_table, c.location))
+ {
+ ++skips;
+ skips_at[i] = -1;
+ continue;
+ }
+ }
+
+ if (sec.streaming_p ())
+ {
+ sec.u (len - skips);
+ dump () && dump ("Diagnostic changes: %u", len - skips);
+ }
+
+ for (unsigned i = 0; i < len; ++i)
+ {
+ if (skips_at[i] == -1)
+ continue;
+
+ const auto &c = changes[i];
+ write_location (sec, c.location);
+ if (sec.streaming_p ())
+ {
+ unsigned opt = c.option;
+ if (c.kind == DK_POP)
+ opt -= skips_at[opt];
+ sec.u (opt);
+ sec.u (c.kind);
+ dump () && dump_dc_change (i - skips_at[i], opt, 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 ();
+ int offset = changes.length ();
+ changes.reserve (len + 1);
+ 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)
+ /* For a pop, opt is the 'changes' index to return to. */
+ opt += offset;
+ changes.quick_push ({ loc, opt, kind });
+ dump () && dump_dc_change (changes.length () - 1, opt, kind);
+ }
+
+ /* Did the import pop all its diagnostic changes? */
+ bool last_was_reset = (len == 0);
+ if (len)
+ for (int i = changes.length () - 1; ; --i)
+ {
+ gcc_checking_assert (i >= offset);
+
+ const auto &c = changes[i];
+ if (c.kind != DK_POP)
+ break;
+ else if (c.option == offset)
+ {
+ last_was_reset = true;
+ break;
+ }
+ else
+ /* As in update_effective_level_from_pragmas, the loop will decrement
+ i so we actually jump to c.option - 1. */
+ i = c.option;
+ }
+ if (!last_was_reset)
+ {
+ /* It didn't, so add a pop at its last location to avoid affecting later
+ imports. */
+ location_t last_loc = ordinary_locs.first + ordinary_locs.second - 1;
+ changes.quick_push ({ last_loc, offset, DK_POP });
+ dump () && dump (" Adding final pop from index %d", offset);
+ }
+
+ 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)
{
@@ -19871,6 +20037,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));
@@ -20012,7 +20180,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);
@@ -20085,6 +20256,10 @@ module_state::read_initial (cpp_reader *reader)
else if (!read_ordinary_maps (config.ordinary_locs, config.loc_range_bits))
ok = false;
+ if (ok && have_locs && config.ordinary_locs
+ && !read_diagnostic_classification (global_dc))
+ ok = false;
+
/* Allocate the REMAP vector. */
slurp->alloc_remap (config.num_imports);
diff --git a/gcc/testsuite/g++.dg/modules/warn-spec-3_a.C b/gcc/testsuite/g++.dg/modules/warn-spec-3_a.C
new file mode 100644
index 00000000000..2e50303c41e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/warn-spec-3_a.C
@@ -0,0 +1,20 @@
+// { dg-do compile { target c++20 } }
+// { dg-additional-options -fmodules }
+// { dg-module-cmi M }
+
+module;
+
+#include <initializer_list>
+
+export module M;
+
+#pragma GCC diagnostic ignored "-Winit-list-lifetime"
+
+template <class T>
+struct myspan {
+ const T* p; unsigned s;
+ myspan (std::initializer_list<T> il)
+ : p (il.begin()), s (il.size()) { }
+};
+
+export void f(myspan<int>);
diff --git a/gcc/testsuite/g++.dg/modules/warn-spec-3_b.C b/gcc/testsuite/g++.dg/modules/warn-spec-3_b.C
new file mode 100644
index 00000000000..0a614f72bec
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/warn-spec-3_b.C
@@ -0,0 +1,7 @@
+// { dg-additional-options "-fmodules -fdump-lang-module" }
+
+// Test that we don't re-export the changes from M.
+// { dg-final { scan-lang-dump {Diagnostic changes: 0} module } }
+
+export module N;
+import M;
diff --git a/gcc/testsuite/g++.dg/modules/warn-spec-3_c.C b/gcc/testsuite/g++.dg/modules/warn-spec-3_c.C
new file mode 100644
index 00000000000..2e466c09c8b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/warn-spec-3_c.C
@@ -0,0 +1,12 @@
+// { dg-additional-options "-fmodules -fdump-lang-module" }
+
+// Test that we clean up the unpopped change in M.
+// { dg-final { scan-lang-dump {Adding final pop} module } }
+
+import N;
+import M;
+
+int main()
+{
+ f({24,42});
+}
diff --git a/libcpp/line-map.cc b/libcpp/line-map.cc
index 284af5781dc..33701b519e1 100644
--- a/libcpp/line-map.cc
+++ b/libcpp/line-map.cc
@@ -767,6 +767,17 @@ linemap_module_restore (line_maps *set, line_map_uint_t lwm)
return 0;
}
+/* TRUE iff the location comes from a module import. */
+
+bool
+linemap_location_from_module_p (const line_maps *set, location_t loc)
+{
+ const line_map_ordinary *map = linemap_ordinary_map_lookup (set, loc);
+ while (map && map->reason != LC_MODULE)
+ map = linemap_included_from_linemap (set, map);
+ return !!map;
+}
+
/* Returns TRUE if the line table set tracks token locations across
macro expansion, FALSE otherwise. */
--
2.49.0
