[PATCH v5 0/5] P1689R5 support

[PATCH v5 0/5] P1689R5 support

[Ben Boeckel]

Hi,

This patch series adds initial support for ISO C++'s [P1689R5][], a
format for describing C++ module requirements and provisions based on
the source code. This is required because compiling C++ with modules is
not embarrassingly parallel and need to be ordered to ensure that
`import some_module;` can be satisfied in time by making sure that any
TU with `export import some_module;` is compiled first.

[P1689R5]: https://isocpp.org/files/papers/P1689R5.html

I've also added patches to include imported module CMI files and the
module mapper file as dependencies of the compilation. I briefly looked
into adding dependencies on response files as well, but that appeared to
need some code contortions to have a `class mkdeps` available before
parsing the command line or to keep the information around until one was
made.

I'd like feedback on the approach taken here with respect to the
user-visible flags. I'll also note that header units are not supported
at this time because the current `-E` behavior with respect to `import
<some_header>;` is to search for an appropriate `.gcm` file which is not
something such a "scan" can support. A new mode will likely need to be
created (e.g., replacing `-E` with `-fc++-module-scanning` or something)
where headers are looked up "normally" and processed only as much as
scanning requires.

FWIW, Clang as taken an alternate approach with its `clang-scan-deps`
tool rather than using the compiler directly.

Thanks,

--Ben

---
v4 -> v5:

- add dependency tracking for imported modules to `-MF`
- add dependency tracking for static module mapper files given to
  `-fmodule-mapper=`

v3 -> v4:

- add missing spaces between function names and arguments

v2 -> v3:

- changelog entries moved to commit messages
- documentation updated/added in the UTF-8 routine editing

v1 -> v2:

- removal of the `deps_write(extra)` parameter to option-checking where
  ndeeded
- default parameter of `cpp_finish(fdeps_stream = NULL)`
- unification of libcpp UTF-8 validity functions from v1
- test cases for flag parsing states (depflags-*) and p1689 output
  (p1689-*)

Ben Boeckel (5):
  libcpp: reject codepoints above 0x10FFFF
  libcpp: add a function to determine UTF-8 validity of a C string
  p1689r5: initial support
  c++modules: report imported CMI files as dependencies
  c++modules: report module mapper files as a dependency

 gcc/c-family/c-opts.cc                        |  40 +++-
 gcc/c-family/c.opt                            |  12 +
 gcc/cp/mapper-client.cc                       |   4 +
 gcc/cp/mapper-client.h                        |   1 +
 gcc/cp/module.cc                              |  23 +-
 gcc/doc/invoke.texi                           |  15 ++
 gcc/testsuite/g++.dg/modules/depflags-f-MD.C  |   2 +
 gcc/testsuite/g++.dg/modules/depflags-f.C     |   1 +
 gcc/testsuite/g++.dg/modules/depflags-fi.C    |   3 +
 gcc/testsuite/g++.dg/modules/depflags-fj-MD.C |   3 +
 gcc/testsuite/g++.dg/modules/depflags-fj.C    |   4 +
 .../g++.dg/modules/depflags-fjo-MD.C          |   4 +
 gcc/testsuite/g++.dg/modules/depflags-fjo.C   |   5 +
 gcc/testsuite/g++.dg/modules/depflags-fo-MD.C |   3 +
 gcc/testsuite/g++.dg/modules/depflags-fo.C    |   4 +
 gcc/testsuite/g++.dg/modules/depflags-j-MD.C  |   2 +
 gcc/testsuite/g++.dg/modules/depflags-j.C     |   3 +
 gcc/testsuite/g++.dg/modules/depflags-jo-MD.C |   3 +
 gcc/testsuite/g++.dg/modules/depflags-jo.C    |   4 +
 gcc/testsuite/g++.dg/modules/depflags-o-MD.C  |   2 +
 gcc/testsuite/g++.dg/modules/depflags-o.C     |   3 +
 gcc/testsuite/g++.dg/modules/modules.exp      |   1 +
 gcc/testsuite/g++.dg/modules/p1689-1.C        |  18 ++
 gcc/testsuite/g++.dg/modules/p1689-1.exp.json |  27 +++
 gcc/testsuite/g++.dg/modules/p1689-2.C        |  16 ++
 gcc/testsuite/g++.dg/modules/p1689-2.exp.json |  16 ++
 gcc/testsuite/g++.dg/modules/p1689-3.C        |  14 ++
 gcc/testsuite/g++.dg/modules/p1689-3.exp.json |  16 ++
 gcc/testsuite/g++.dg/modules/p1689-4.C        |  14 ++
 gcc/testsuite/g++.dg/modules/p1689-4.exp.json |  14 ++
 gcc/testsuite/g++.dg/modules/p1689-5.C        |  14 ++
 gcc/testsuite/g++.dg/modules/p1689-5.exp.json |  14 ++
 gcc/testsuite/g++.dg/modules/test-p1689.py    | 222 ++++++++++++++++++
 gcc/testsuite/lib/modules.exp                 |  71 ++++++
 libcpp/charset.cc                             |  28 ++-
 libcpp/include/cpplib.h                       |  12 +-
 libcpp/include/mkdeps.h                       |  17 +-
 libcpp/init.cc                                |  13 +-
 libcpp/internal.h                             |   2 +
 libcpp/mkdeps.cc                              | 149 +++++++++++-
 40 files changed, 789 insertions(+), 30 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-f-MD.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-f.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fi.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fj-MD.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fj.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fjo-MD.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fjo.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fo-MD.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fo.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-j-MD.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-j.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-jo-MD.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-jo.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-o-MD.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-o.C
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-1.C
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-1.exp.json
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-2.C
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-2.exp.json
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-3.C
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-3.exp.json
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-4.C
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-4.exp.json
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-5.C
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-5.exp.json
 create mode 100644 gcc/testsuite/g++.dg/modules/test-p1689.py
 create mode 100644 gcc/testsuite/lib/modules.exp


base-commit: 9d4c00cdaccc3decd07740e817387ce844ef3ac9
-- 
2.39.0

[PATCH v5 1/5] libcpp: reject codepoints above 0x10FFFF

[Ben Boeckel]

Unicode does not support such values because they are unrepresentable in
UTF-16.

libcpp/

	* charset.cc: Reject encodings of codepoints above 0x10FFFF.
	UTF-16 does not support such codepoints and therefore all
	Unicode rejects such values.

Signed-off-by: Ben Boeckel <ben.boeckel@kitware.com>
---
 libcpp/charset.cc | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/libcpp/charset.cc b/libcpp/charset.cc
index 3c47d4f868b..f7ae12ea5a2 100644
--- a/libcpp/charset.cc
+++ b/libcpp/charset.cc
@@ -158,6 +158,10 @@ struct _cpp_strbuf
    encoded as any of DF 80, E0 9F 80, F0 80 9F 80, F8 80 80 9F 80, or
    FC 80 80 80 9F 80.  Only the first is valid.
 
+   Additionally, Unicode declares that all codepoints above 0010FFFF are
+   invalid because they cannot be represented in UTF-16. As such, all 5- and
+   6-byte encodings are invalid.
+
    An implementation note: the transformation from UTF-16 to UTF-8, or
    vice versa, is easiest done by using UTF-32 as an intermediary.  */
 
@@ -216,7 +220,7 @@ one_utf8_to_cppchar (const uchar **inbufp, size_t *inbytesleftp,
   if (c <= 0x3FFFFFF && nbytes > 5) return EILSEQ;
 
   /* Make sure the character is valid.  */
-  if (c > 0x7FFFFFFF || (c >= 0xD800 && c <= 0xDFFF)) return EILSEQ;
+  if (c > 0x10FFFF || (c >= 0xD800 && c <= 0xDFFF)) return EILSEQ;
 
   *cp = c;
   *inbufp = inbuf;
@@ -320,7 +324,7 @@ one_utf32_to_utf8 (iconv_t bigend, const uchar **inbufp, size_t *inbytesleftp,
   s += inbuf[bigend ? 2 : 1] << 8;
   s += inbuf[bigend ? 3 : 0];
 
-  if (s >= 0x7FFFFFFF || (s >= 0xD800 && s <= 0xDFFF))
+  if (s > 0x10FFFF || (s >= 0xD800 && s <= 0xDFFF))
     return EILSEQ;
 
   rval = one_cppchar_to_utf8 (s, outbufp, outbytesleftp);
-- 
2.39.0

Re: [PATCH v5 1/5] libcpp: reject codepoints above 0x10FFFF

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 546 bytes --]

On 1/25/23 13:06, Ben Boeckel wrote:
> Unicode does not support such values because they are unrepresentable in
> UTF-16.
> 
> libcpp/
> 
> 	* charset.cc: Reject encodings of codepoints above 0x10FFFF.
> 	UTF-16 does not support such codepoints and therefore all
> 	Unicode rejects such values.

It seems that this causes a bunch of testsuite failures from tests that 
expect this limit to be checked elsewhere with a different diagnostic, 
so I think the easiest thing is to fold this into _cpp_valid_utf8_str 
instead, i.e.:

Make sense?

Jason

[-- Attachment #2: 0001-libcpp-add-a-function-to-determine-UTF-8-validity-of.patch --]
[-- Type: text/x-patch, Size: 2250 bytes --]

From 296e9d1e16533979d12bd98db2937e396a0796f3 Mon Sep 17 00:00:00 2001
From: Ben Boeckel <ben.boeckel@kitware.com>
Date: Sat, 10 Dec 2022 17:20:49 -0500
Subject: [PATCH] libcpp: add a function to determine UTF-8 validity of a C
 string
To: gcc-patches@gcc.gnu.org

This simplifies the interface for other UTF-8 validity detections when a
simple "yes" or "no" answer is sufficient.

libcpp/

	* charset.cc: Add `_cpp_valid_utf8_str` which determines whether
	a C string is valid UTF-8 or not.
	* internal.h: Add prototype for `_cpp_valid_utf8_str`.

Signed-off-by: Ben Boeckel <ben.boeckel@kitware.com>
---
 libcpp/internal.h |  2 ++
 libcpp/charset.cc | 24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/libcpp/internal.h b/libcpp/internal.h
index 9724676a8cd..48520901b2d 100644
--- a/libcpp/internal.h
+++ b/libcpp/internal.h
@@ -834,6 +834,8 @@ extern bool _cpp_valid_utf8 (cpp_reader *pfile,
 			     struct normalize_state *nst,
 			     cppchar_t *cp);
 
+extern bool _cpp_valid_utf8_str (const char *str);
+
 extern void _cpp_destroy_iconv (cpp_reader *);
 extern unsigned char *_cpp_convert_input (cpp_reader *, const char *,
 					  unsigned char *, size_t, size_t,
diff --git a/libcpp/charset.cc b/libcpp/charset.cc
index 3c47d4f868b..42a1b596c06 100644
--- a/libcpp/charset.cc
+++ b/libcpp/charset.cc
@@ -1864,6 +1864,30 @@ _cpp_valid_utf8 (cpp_reader *pfile,
   return true;
 }
 
+/*  Detect whether a C-string is a valid UTF-8-encoded set of bytes. Returns
+    `false` if any contained byte sequence encodes an invalid Unicode codepoint
+    or is not a valid UTF-8 sequence. Returns `true` otherwise. */
+
+extern bool
+_cpp_valid_utf8_str (const char *name)
+{
+  const uchar* in = (const uchar*)name;
+  size_t len = strlen (name);
+  cppchar_t cp;
+
+  while (*in)
+    {
+      if (one_utf8_to_cppchar (&in, &len, &cp))
+	return false;
+
+      /* one_utf8_to_cppchar doesn't check this limit.  */
+      if (cp > UCS_LIMIT)
+	return false;
+    }
+
+  return true;
+}
+
 /* Subroutine of convert_hex and convert_oct.  N is the representation
    in the execution character set of a numeric escape; write it into the
    string buffer TBUF and update the end-of-string pointer therein.  WIDE
-- 
2.31.1

Re: [PATCH v5 1/5] libcpp: reject codepoints above 0x10FFFF

[Ben Boeckel]

On Mon, Feb 13, 2023 at 10:53:17 -0500, Jason Merrill wrote:
> On 1/25/23 13:06, Ben Boeckel wrote:
> > Unicode does not support such values because they are unrepresentable in
> > UTF-16.
> > 
> > libcpp/
> > 
> > 	* charset.cc: Reject encodings of codepoints above 0x10FFFF.
> > 	UTF-16 does not support such codepoints and therefore all
> > 	Unicode rejects such values.
> 
> It seems that this causes a bunch of testsuite failures from tests that 
> expect this limit to be checked elsewhere with a different diagnostic, 
> so I think the easiest thing is to fold this into _cpp_valid_utf8_str 
> instead, i.e.:

Since then, `cpp_valid_utf8_p` has appeared and takes care of the
over-long encodings. The new patchset just checks for codepoints beyond
0x10FFFF and rejects them in this function (and the test suite matches
`master` results for me then).

--Ben

[PATCH v5 2/5] libcpp: add a function to determine UTF-8 validity of a C string

[Ben Boeckel]

This simplifies the interface for other UTF-8 validity detections when a
simple "yes" or "no" answer is sufficient.

libcpp/

	* charset.cc: Add `_cpp_valid_utf8_str` which determines whether
	a C string is valid UTF-8 or not.
	* internal.h: Add prototype for `_cpp_valid_utf8_str`.

Signed-off-by: Ben Boeckel <ben.boeckel@kitware.com>
---
 libcpp/charset.cc | 20 ++++++++++++++++++++
 libcpp/internal.h |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/libcpp/charset.cc b/libcpp/charset.cc
index f7ae12ea5a2..616be9d02ee 100644
--- a/libcpp/charset.cc
+++ b/libcpp/charset.cc
@@ -1868,6 +1868,26 @@ _cpp_valid_utf8 (cpp_reader *pfile,
   return true;
 }
 
+/*  Detect whether a C-string is a valid UTF-8-encoded set of bytes. Returns
+    `false` if any contained byte sequence encodes an invalid Unicode codepoint
+    or is not a valid UTF-8 sequence. Returns `true` otherwise. */
+
+extern bool
+_cpp_valid_utf8_str (const char *name)
+{
+  const uchar* in = (const uchar*)name;
+  size_t len = strlen (name);
+  cppchar_t cp;
+
+  while (*in)
+    {
+      if (one_utf8_to_cppchar (&in, &len, &cp))
+	return false;
+    }
+
+  return true;
+}
+
 /* Subroutine of convert_hex and convert_oct.  N is the representation
    in the execution character set of a numeric escape; write it into the
    string buffer TBUF and update the end-of-string pointer therein.  WIDE
diff --git a/libcpp/internal.h b/libcpp/internal.h
index 9724676a8cd..48520901b2d 100644
--- a/libcpp/internal.h
+++ b/libcpp/internal.h
@@ -834,6 +834,8 @@ extern bool _cpp_valid_utf8 (cpp_reader *pfile,
 			     struct normalize_state *nst,
 			     cppchar_t *cp);
 
+extern bool _cpp_valid_utf8_str (const char *str);
+
 extern void _cpp_destroy_iconv (cpp_reader *);
 extern unsigned char *_cpp_convert_input (cpp_reader *, const char *,
 					  unsigned char *, size_t, size_t,
-- 
2.39.0

Re: [PATCH v5 2/5] libcpp: add a function to determine UTF-8 validity of a C string

[David Malcolm]

On Wed, Jan 25, 2023 at 4:09 PM Ben Boeckel via Gcc <gcc@gcc.gnu.org> wrote:
>
> This simplifies the interface for other UTF-8 validity detections when a
> simple "yes" or "no" answer is sufficient.
>
> libcpp/
>
>         * charset.cc: Add `_cpp_valid_utf8_str` which determines whether
>         a C string is valid UTF-8 or not.
>         * internal.h: Add prototype for `_cpp_valid_utf8_str`.
>
> Signed-off-by: Ben Boeckel <ben.boeckel@kitware.com>

[going through patches in patchwork]

What's the status of this patch; did this ever get committed?

I see that Jason preapproved this via his review of "[PATCH v3 2/3]
libcpp: add a function to determine UTF-8 validity of a C string"

Thanks
Dave

Re: [PATCH v5 2/5] libcpp: add a function to determine UTF-8 validity of a C string

[Jason Merrill]

On 10/23/23 11:16, David Malcolm wrote:
> On Wed, Jan 25, 2023 at 4:09 PM Ben Boeckel via Gcc <gcc@gcc.gnu.org> wrote:
>>
>> This simplifies the interface for other UTF-8 validity detections when a
>> simple "yes" or "no" answer is sufficient.
>>
>> libcpp/
>>
>>          * charset.cc: Add `_cpp_valid_utf8_str` which determines whether
>>          a C string is valid UTF-8 or not.
>>          * internal.h: Add prototype for `_cpp_valid_utf8_str`.
>>
>> Signed-off-by: Ben Boeckel <ben.boeckel@kitware.com>
> 
> [going through patches in patchwork]
> 
> What's the status of this patch; did this ever get committed?

It was superseded.

Jason

Re: [PATCH v5 2/5] libcpp: add a function to determine UTF-8 validity of a C string

[David Malcolm]

On Mon, 2023-10-23 at 11:24 -0400, Jason Merrill wrote:
> On 10/23/23 11:16, David Malcolm wrote:
> > On Wed, Jan 25, 2023 at 4:09 PM Ben Boeckel via Gcc
> > <gcc@gcc.gnu.org> wrote:
> > > 
> > > This simplifies the interface for other UTF-8 validity detections
> > > when a
> > > simple "yes" or "no" answer is sufficient.
> > > 
> > > libcpp/
> > > 
> > >          * charset.cc: Add `_cpp_valid_utf8_str` which determines
> > > whether
> > >          a C string is valid UTF-8 or not.
> > >          * internal.h: Add prototype for `_cpp_valid_utf8_str`.
> > > 
> > > Signed-off-by: Ben Boeckel <ben.boeckel@kitware.com>
> > 
> > [going through patches in patchwork]
> > 
> > What's the status of this patch; did this ever get committed?
> 
> It was superseded.

Thanks; closed out in patchwork.

Dave

[PATCH v5 3/5] p1689r5: initial support

[Ben Boeckel]

This patch implements support for [P1689R5][] to communicate to a build
system the C++20 module dependencies to build systems so that they may
build `.gcm` files in the proper order.

Support is communicated through the following three new flags:

- `-fdeps-format=` specifies the format for the output. Currently named
  `p1689r5`.

- `-fdeps-file=` specifies the path to the file to write the format to.

- `-fdep-output=` specifies the `.o` that will be written for the TU
  that is scanned. This is required so that the build system can
  correlate the dependency output with the actual compilation that will
  occur.

CMake supports this format as of 17 Jun 2022 (to be part of 3.25.0)
using an experimental feature selection (to allow for future usage
evolution without committing to how it works today). While it remains
experimental, docs may be found in CMake's documentation for
experimental features.

Future work may include using this format for Fortran module
dependencies as well, however this is still pending work.

[P1689R5]: https://isocpp.org/files/papers/P1689R5.html
[cmake-experimental]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Help/dev/experimental.rst

TODO:

- header-unit information fields

Header units (including the standard library headers) are 100%
unsupported right now because the `-E` mechanism wants to import their
BMIs. A new mode (i.e., something more workable than existing `-E`
behavior) that mocks up header units as if they were imported purely
from their path and content would be required.

- non-utf8 paths

The current standard says that paths that are not unambiguously
represented using UTF-8 are not supported (because these cases are rare
and the extra complication is not worth it at this time). Future
versions of the format might have ways of encoding non-UTF-8 paths. For
now, this patch just doesn't support non-UTF-8 paths (ignoring the
"unambiguously represetable in UTF-8" case).

- figure out why junk gets placed at the end of the file

Sometimes it seems like the file gets a lot of `NUL` bytes appended to
it. It happens rarely and seems to be the result of some
`ftruncate`-style call which results in extra padding in the contents.
Noting it here as an observation at least.

libcpp/

	* include/cpplib.h: Add cpp_deps_format enum.
	(cpp_options): Add format field
	(cpp_finish): Add dependency stream parameter.
	* include/mkdeps.h (deps_add_module_target): Add new preprocessor
	parameter used for C++ module tracking.
	* init.cc (cpp_finish): Add new preprocessor parameter used for C++
	module tracking.
	* mkdeps.cc (mkdeps): Implement P1689R5 output.

gcc/

	* doc/invoke.texi: Document -fdeps-format=, -fdep-file=, and
	-fdep-output= flags.

gcc/c-family/

	* c-opts.cc (c_common_handle_option): Add fdeps_file variable and
	-fdeps-format=, -fdep-file=, and -fdep-output= parsing.
	* c.opt: Add -fdeps-format=, -fdep-file=, and -fdep-output= flags.

gcc/cp/

	* module.cc (preprocessed_module): Pass whether the module is
	exported to dependency tracking.

gcc/testsuite/

	* g++.dg/modules/depflags-f-MD.C: New test.
	* g++.dg/modules/depflags-f.C: New test.
	* g++.dg/modules/depflags-fi.C: New test.
	* g++.dg/modules/depflags-fj-MD.C: New test.
	* g++.dg/modules/depflags-fj.C: New test.
	* g++.dg/modules/depflags-fjo-MD.C: New test.
	* g++.dg/modules/depflags-fjo.C: New test.
	* g++.dg/modules/depflags-fo-MD.C: New test.
	* g++.dg/modules/depflags-fo.C: New test.
	* g++.dg/modules/depflags-j-MD.C: New test.
	* g++.dg/modules/depflags-j.C: New test.
	* g++.dg/modules/depflags-jo-MD.C: New test.
	* g++.dg/modules/depflags-jo.C: New test.
	* g++.dg/modules/depflags-o-MD.C: New test.
	* g++.dg/modules/depflags-o.C: New test.
	* g++.dg/modules/p1689-1.C: New test.
	* g++.dg/modules/p1689-1.exp.json: New test expectation.
	* g++.dg/modules/p1689-2.C: New test.
	* g++.dg/modules/p1689-2.exp.json: New test expectation.
	* g++.dg/modules/p1689-3.C: New test.
	* g++.dg/modules/p1689-3.exp.json: New test expectation.
	* g++.dg/modules/p1689-4.C: New test.
	* g++.dg/modules/p1689-4.exp.json: New test expectation.
	* g++.dg/modules/p1689-5.C: New test.
	* g++.dg/modules/p1689-5.exp.json: New test expectation.
	* g++.dg/modules/modules.exp: Load new P1689 library routines.
	* g++.dg/modules/test-p1689.py: New tool for validating P1689 output.
	* lib/modules.exp: Support for validating P1689 outputs.

Signed-off-by: Ben Boeckel <ben.boeckel@kitware.com>
---
 gcc/c-family/c-opts.cc                        |  40 +++-
 gcc/c-family/c.opt                            |  12 +
 gcc/cp/module.cc                              |   3 +-
 gcc/doc/invoke.texi                           |  15 ++
 gcc/testsuite/g++.dg/modules/depflags-f-MD.C  |   2 +
 gcc/testsuite/g++.dg/modules/depflags-f.C     |   1 +
 gcc/testsuite/g++.dg/modules/depflags-fi.C    |   3 +
 gcc/testsuite/g++.dg/modules/depflags-fj-MD.C |   3 +
 gcc/testsuite/g++.dg/modules/depflags-fj.C    |   4 +
 .../g++.dg/modules/depflags-fjo-MD.C          |   4 +
 gcc/testsuite/g++.dg/modules/depflags-fjo.C   |   5 +
 gcc/testsuite/g++.dg/modules/depflags-fo-MD.C |   3 +
 gcc/testsuite/g++.dg/modules/depflags-fo.C    |   4 +
 gcc/testsuite/g++.dg/modules/depflags-j-MD.C  |   2 +
 gcc/testsuite/g++.dg/modules/depflags-j.C     |   3 +
 gcc/testsuite/g++.dg/modules/depflags-jo-MD.C |   3 +
 gcc/testsuite/g++.dg/modules/depflags-jo.C    |   4 +
 gcc/testsuite/g++.dg/modules/depflags-o-MD.C  |   2 +
 gcc/testsuite/g++.dg/modules/depflags-o.C     |   3 +
 gcc/testsuite/g++.dg/modules/modules.exp      |   1 +
 gcc/testsuite/g++.dg/modules/p1689-1.C        |  18 ++
 gcc/testsuite/g++.dg/modules/p1689-1.exp.json |  27 +++
 gcc/testsuite/g++.dg/modules/p1689-2.C        |  16 ++
 gcc/testsuite/g++.dg/modules/p1689-2.exp.json |  16 ++
 gcc/testsuite/g++.dg/modules/p1689-3.C        |  14 ++
 gcc/testsuite/g++.dg/modules/p1689-3.exp.json |  16 ++
 gcc/testsuite/g++.dg/modules/p1689-4.C        |  14 ++
 gcc/testsuite/g++.dg/modules/p1689-4.exp.json |  14 ++
 gcc/testsuite/g++.dg/modules/p1689-5.C        |  14 ++
 gcc/testsuite/g++.dg/modules/p1689-5.exp.json |  14 ++
 gcc/testsuite/g++.dg/modules/test-p1689.py    | 222 ++++++++++++++++++
 gcc/testsuite/lib/modules.exp                 |  71 ++++++
 libcpp/include/cpplib.h                       |  12 +-
 libcpp/include/mkdeps.h                       |  17 +-
 libcpp/init.cc                                |  13 +-
 libcpp/mkdeps.cc                              | 149 +++++++++++-
 36 files changed, 745 insertions(+), 19 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-f-MD.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-f.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fi.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fj-MD.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fj.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fjo-MD.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fjo.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fo-MD.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fo.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-j-MD.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-j.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-jo-MD.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-jo.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-o-MD.C
 create mode 100644 gcc/testsuite/g++.dg/modules/depflags-o.C
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-1.C
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-1.exp.json
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-2.C
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-2.exp.json
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-3.C
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-3.exp.json
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-4.C
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-4.exp.json
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-5.C
 create mode 100644 gcc/testsuite/g++.dg/modules/p1689-5.exp.json
 create mode 100644 gcc/testsuite/g++.dg/modules/test-p1689.py
 create mode 100644 gcc/testsuite/lib/modules.exp

diff --git a/gcc/c-family/c-opts.cc b/gcc/c-family/c-opts.cc
index c68a2a27469..1c14ce3fe8e 100644
--- a/gcc/c-family/c-opts.cc
+++ b/gcc/c-family/c-opts.cc
@@ -77,6 +77,9 @@ static bool verbose;
 /* Dependency output file.  */
 static const char *deps_file;
 
+/* Enhanced dependency output file.  */
+static const char *fdeps_file;
+
 /* The prefix given by -iprefix, if any.  */
 static const char *iprefix;
 
@@ -360,6 +363,23 @@ c_common_handle_option (size_t scode, const char *arg, HOST_WIDE_INT value,
       deps_file = arg;
       break;
 
+    case OPT_fdep_format_:
+      if (!strcmp (arg, "p1689r5"))
+	cpp_opts->deps.format = DEPS_FMT_P1689R5;
+      else
+	error ("%<-fdep-format=%> unknown format %<%s%>", arg);
+      break;
+
+    case OPT_fdep_file_:
+      deps_seen = true;
+      fdeps_file = arg;
+      break;
+
+    case OPT_fdep_output_:
+      deps_seen = true;
+      defer_opt (code, arg);
+      break;
+
     case OPT_MF:
       deps_seen = true;
       deps_file = arg;
@@ -1272,6 +1292,7 @@ void
 c_common_finish (void)
 {
   FILE *deps_stream = NULL;
+  FILE *fdeps_stream = NULL;
 
   /* Note that we write the dependencies even if there are errors. This is
      useful for handling outdated generated headers that now trigger errors
@@ -1300,9 +1321,24 @@ c_common_finish (void)
      locations with input_location, which would be incorrect now.  */
   override_libcpp_locations = false;
 
+  if (cpp_opts->deps.format != DEPS_FMT_NONE)
+    {
+      if (!fdeps_file)
+	fdeps_stream = out_stream;
+      else if (fdeps_file[0] == '-' && fdeps_file[1] == '\0')
+	fdeps_stream = stdout;
+      else
+	{
+	  fdeps_stream = fopen (fdeps_file, "w");
+	  if (!fdeps_stream)
+	    fatal_error (input_location, "opening dependency file %s: %m",
+			 fdeps_file);
+	}
+    }
+
   /* For performance, avoid tearing down cpplib's internal structures
      with cpp_destroy ().  */
-  cpp_finish (parse_in, deps_stream);
+  cpp_finish (parse_in, deps_stream, fdeps_stream);
 
   if (deps_stream && deps_stream != out_stream && deps_stream != stdout
       && (ferror (deps_stream) || fclose (deps_stream)))
@@ -1374,6 +1410,8 @@ handle_deferred_opts (void)
 
 	if (opt->code == OPT_MT || opt->code == OPT_MQ)
 	  deps_add_target (deps, opt->arg, opt->code == OPT_MQ);
+	else if (opt->code == OPT_fdep_output_)
+	  deps_add_output (deps, opt->arg, true);
       }
 }
 
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index ef371ca8c26..630781fdf8a 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -256,6 +256,18 @@ MT
 C ObjC C++ ObjC++ Joined Separate MissingArgError(missing makefile target after %qs)
 -MT <target>	Add a target that does not require quoting.
 
+fdep-format=
+C ObjC C++ ObjC++ NoDriverArg Joined MissingArgError(missing format after %qs)
+Format for output dependency information.  Supported (\"p1689r5\").
+
+fdep-file=
+C ObjC C++ ObjC++ NoDriverArg Joined MissingArgError(missing output path after %qs)
+File for output dependency information.
+
+fdep-output=
+C ObjC C++ ObjC++ NoDriverArg Joined MissingArgError(missing path after %qs)
+-fdep-output=obj.o Output file for the compile step.
+
 P
 C ObjC C++ ObjC++
 Do not generate #line directives.
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index ac2fe66b080..ebd30f63d81 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -19832,7 +19832,8 @@ preprocessed_module (cpp_reader *reader)
 		  && (module->is_interface () || module->is_partition ()))
 		deps_add_module_target (deps, module->get_flatname (),
 					maybe_add_cmi_prefix (module->filename),
-					module->is_header());
+					module->is_header (),
+					module->is_exported ());
 	      else
 		deps_add_module_dep (deps, module->get_flatname ());
 	    }
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 06d77983e30..b61c3ebd3ec 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -2791,6 +2791,21 @@ is @option{-fpermitted-flt-eval-methods=c11}.  The default when in a GNU
 dialect (@option{-std=gnu11} or similar) is
 @option{-fpermitted-flt-eval-methods=ts-18661-3}.
 
+@item -fdep-file=@var{file}
+@opindex fdep-file
+Where to write structured dependency information.
+
+@item -fdep-format=@var{format}
+@opindex fdep-format
+The format to use for structured dependency information. @samp{p1689r5} is the
+only supported format right now.  Note that when this argument is specified, the
+output of @samp{-MF} is stripped of some information (namely C++ modules) so
+that it does not use extended makefile syntax not understood by most tools.
+
+@item -fdep-output=@var{file}
+@opindex fdep-output
+Analogous to @option{-MT} but for structured dependency information.
+
 @item -fplan9-extensions
 @opindex fplan9-extensions
 Accept some non-standard constructs used in Plan 9 code.
diff --git a/gcc/testsuite/g++.dg/modules/depflags-f-MD.C b/gcc/testsuite/g++.dg/modules/depflags-f-MD.C
new file mode 100644
index 00000000000..90e1c9983bd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/depflags-f-MD.C
@@ -0,0 +1,2 @@
+// { dg-additional-options -MD }
+// { dg-additional-options -fdep-format=p1689r5 }
diff --git a/gcc/testsuite/g++.dg/modules/depflags-f.C b/gcc/testsuite/g++.dg/modules/depflags-f.C
new file mode 100644
index 00000000000..6192300879d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/depflags-f.C
@@ -0,0 +1 @@
+// { dg-additional-options -fdep-format=p1689r5 }
diff --git a/gcc/testsuite/g++.dg/modules/depflags-fi.C b/gcc/testsuite/g++.dg/modules/depflags-fi.C
new file mode 100644
index 00000000000..4f649a11bdd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/depflags-fi.C
@@ -0,0 +1,3 @@
+// { dg-additional-options -fdep-format=invalid }
+
+// { dg-prune-output "error: '-fdep-format=' unknown format 'invalid'"  }
diff --git a/gcc/testsuite/g++.dg/modules/depflags-fj-MD.C b/gcc/testsuite/g++.dg/modules/depflags-fj-MD.C
new file mode 100644
index 00000000000..a361d81f37f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/depflags-fj-MD.C
@@ -0,0 +1,3 @@
+// { dg-additional-options -MD }
+// { dg-additional-options -fdep-file=depflags-3.json }
+// { dg-additional-options -fdep-format=p1689r5 }
diff --git a/gcc/testsuite/g++.dg/modules/depflags-fj.C b/gcc/testsuite/g++.dg/modules/depflags-fj.C
new file mode 100644
index 00000000000..4a140ec1f13
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/depflags-fj.C
@@ -0,0 +1,4 @@
+// { dg-additional-options -fdep-file=depflags-3.json }
+// { dg-additional-options -fdep-format=p1689r5 }
+
+// { dg-prune-output "error: to generate dependencies you must specify either '-M' or '-MM'" }
diff --git a/gcc/testsuite/g++.dg/modules/depflags-fjo-MD.C b/gcc/testsuite/g++.dg/modules/depflags-fjo-MD.C
new file mode 100644
index 00000000000..18d765211b4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/depflags-fjo-MD.C
@@ -0,0 +1,4 @@
+// { dg-additional-options -MD }
+// { dg-additional-options -fdep-file=depflags-3.json }
+// { dg-additional-options -fdep-output=depflags-1.C }
+// { dg-additional-options -fdep-format=p1689r5 }
diff --git a/gcc/testsuite/g++.dg/modules/depflags-fjo.C b/gcc/testsuite/g++.dg/modules/depflags-fjo.C
new file mode 100644
index 00000000000..6d239f63017
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/depflags-fjo.C
@@ -0,0 +1,5 @@
+// { dg-additional-options -fdep-file=depflags-3.json }
+// { dg-additional-options -fdep-output=depflags-1.C }
+// { dg-additional-options -fdep-format=p1689r5 }
+
+// { dg-prune-output "error: to generate dependencies you must specify either '-M' or '-MM'" }
diff --git a/gcc/testsuite/g++.dg/modules/depflags-fo-MD.C b/gcc/testsuite/g++.dg/modules/depflags-fo-MD.C
new file mode 100644
index 00000000000..a3a775b606a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/depflags-fo-MD.C
@@ -0,0 +1,3 @@
+// { dg-additional-options -MD }
+// { dg-additional-options -fdep-format=p1689r5 }
+// { dg-additional-options -fdep-output=depflags-1.C }
diff --git a/gcc/testsuite/g++.dg/modules/depflags-fo.C b/gcc/testsuite/g++.dg/modules/depflags-fo.C
new file mode 100644
index 00000000000..29839978e59
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/depflags-fo.C
@@ -0,0 +1,4 @@
+// { dg-additional-options -fdep-format=p1689r5 }
+// { dg-additional-options -fdep-output=depflags-1.C }
+
+// { dg-prune-output "error: to generate dependencies you must specify either '-M' or '-MM'" }
diff --git a/gcc/testsuite/g++.dg/modules/depflags-j-MD.C b/gcc/testsuite/g++.dg/modules/depflags-j-MD.C
new file mode 100644
index 00000000000..d95c8e6c2e6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/depflags-j-MD.C
@@ -0,0 +1,2 @@
+// { dg-additional-options -MD }
+// { dg-additional-options -fdep-file=depflags-3.json }
diff --git a/gcc/testsuite/g++.dg/modules/depflags-j.C b/gcc/testsuite/g++.dg/modules/depflags-j.C
new file mode 100644
index 00000000000..5f100b0f6e5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/depflags-j.C
@@ -0,0 +1,3 @@
+// { dg-additional-options -fdep-file=depflags-3.json }
+
+// { dg-prune-output "error: to generate dependencies you must specify either '-M' or '-MM'" }
diff --git a/gcc/testsuite/g++.dg/modules/depflags-jo-MD.C b/gcc/testsuite/g++.dg/modules/depflags-jo-MD.C
new file mode 100644
index 00000000000..44330794abc
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/depflags-jo-MD.C
@@ -0,0 +1,3 @@
+// { dg-additional-options -MD }
+// { dg-additional-options -fdep-file=depflags-3.json }
+// { dg-additional-options -fdep-output=depflags-1.C }
diff --git a/gcc/testsuite/g++.dg/modules/depflags-jo.C b/gcc/testsuite/g++.dg/modules/depflags-jo.C
new file mode 100644
index 00000000000..8eec6bba1d1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/depflags-jo.C
@@ -0,0 +1,4 @@
+// { dg-additional-options -fdep-file=depflags-3.json }
+// { dg-additional-options -fdep-output=depflags-1.C }
+
+// { dg-prune-output "error: to generate dependencies you must specify either '-M' or '-MM'" }
diff --git a/gcc/testsuite/g++.dg/modules/depflags-o-MD.C b/gcc/testsuite/g++.dg/modules/depflags-o-MD.C
new file mode 100644
index 00000000000..429f1f85684
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/depflags-o-MD.C
@@ -0,0 +1,2 @@
+// { dg-additional-options -MD }
+// { dg-additional-options -fdep-output=depflags-1.C }
diff --git a/gcc/testsuite/g++.dg/modules/depflags-o.C b/gcc/testsuite/g++.dg/modules/depflags-o.C
new file mode 100644
index 00000000000..9a7326cc812
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/depflags-o.C
@@ -0,0 +1,3 @@
+// { dg-additional-options -fdep-output=depflags-1.C }
+
+// { dg-prune-output "error: to generate dependencies you must specify either '-M' or '-MM'" }
diff --git a/gcc/testsuite/g++.dg/modules/modules.exp b/gcc/testsuite/g++.dg/modules/modules.exp
index 61994b05945..c06e1cbca8a 100644
--- a/gcc/testsuite/g++.dg/modules/modules.exp
+++ b/gcc/testsuite/g++.dg/modules/modules.exp
@@ -28,6 +28,7 @@
 # { dg-module-do [link|run] [xfail] [options] } # link [and run]
 
 load_lib g++-dg.exp
+load_lib modules.exp
 
 # If a testcase doesn't have special options, use these.
 global DEFAULT_CXXFLAGS
diff --git a/gcc/testsuite/g++.dg/modules/p1689-1.C b/gcc/testsuite/g++.dg/modules/p1689-1.C
new file mode 100644
index 00000000000..245e30d09ce
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/p1689-1.C
@@ -0,0 +1,18 @@
+// { dg-additional-options -E }
+// { dg-additional-options -MT }
+// { dg-additional-options p1689-1.json }
+// { dg-additional-options -MD }
+// { dg-additional-options -fmodules-ts }
+// { dg-additional-options -fdep-format=p1689r5 }
+// { dg-additional-options -fdep-output=p1689-1.o }
+// { dg-additional-options -fdep-file=p1689-1.json }
+
+// Export a module that uses modules, re-exports modules, and partitions.
+
+export module foo;
+export import foo:part1;
+import foo:part2;
+
+export import bar;
+
+// { dg-final { run-check-p1689-valid p1689-1.json p1689-1.exp.json } }
diff --git a/gcc/testsuite/g++.dg/modules/p1689-1.exp.json b/gcc/testsuite/g++.dg/modules/p1689-1.exp.json
new file mode 100644
index 00000000000..c5648ac7ae5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/p1689-1.exp.json
@@ -0,0 +1,27 @@
+{
+    "rules": [
+        {
+            "primary-output": "p1689-1.o",
+            "provides": [
+                {
+                    "logical-name": "foo",
+                    "is-interface": true
+                }
+            ],
+            "requires": [
+                "__P1689_unordered__",
+                {
+                    "logical-name": "bar"
+                },
+                {
+                    "logical-name": "foo:part1"
+                },
+                {
+                    "logical-name": "foo:part2"
+                }
+            ]
+        }
+    ],
+    "version": 0,
+    "revision": 0
+}
diff --git a/gcc/testsuite/g++.dg/modules/p1689-2.C b/gcc/testsuite/g++.dg/modules/p1689-2.C
new file mode 100644
index 00000000000..add07f59a0e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/p1689-2.C
@@ -0,0 +1,16 @@
+// { dg-additional-options -E }
+// { dg-additional-options -MT }
+// { dg-additional-options p1689-2.json }
+// { dg-additional-options -MD }
+// { dg-additional-options -fmodules-ts }
+// { dg-additional-options -fdep-format=p1689r5 }
+// { dg-additional-options -fdep-output=p1689-2.o }
+// { dg-additional-options -fdep-file=p1689-2.json }
+
+// Export a module partition that uses modules.
+
+export module foo:part1;
+
+#include <iostream>
+
+// { dg-final { run-check-p1689-valid p1689-2.json p1689-2.exp.json } }
diff --git a/gcc/testsuite/g++.dg/modules/p1689-2.exp.json b/gcc/testsuite/g++.dg/modules/p1689-2.exp.json
new file mode 100644
index 00000000000..6901172f277
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/p1689-2.exp.json
@@ -0,0 +1,16 @@
+{
+    "rules": [
+        {
+            "primary-output": "p1689-2.o",
+            "provides": [
+                {
+                    "logical-name": "foo:part1",
+                    "is-interface": true
+                }
+            ],
+            "requires": []
+        }
+    ],
+    "version": 0,
+    "revision": 0
+}
diff --git a/gcc/testsuite/g++.dg/modules/p1689-3.C b/gcc/testsuite/g++.dg/modules/p1689-3.C
new file mode 100644
index 00000000000..3482c2f4903
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/p1689-3.C
@@ -0,0 +1,14 @@
+// { dg-additional-options -E }
+// { dg-additional-options -MT }
+// { dg-additional-options p1689-3.json }
+// { dg-additional-options -MD }
+// { dg-additional-options -fmodules-ts }
+// { dg-additional-options -fdep-format=p1689r5 }
+// { dg-additional-options -fdep-output=p1689-3.o }
+// { dg-additional-options -fdep-file=p1689-3.json }
+
+// Provide a module partition.
+
+module foo:part2;
+
+// { dg-final { run-check-p1689-valid p1689-3.json p1689-3.exp.json } }
diff --git a/gcc/testsuite/g++.dg/modules/p1689-3.exp.json b/gcc/testsuite/g++.dg/modules/p1689-3.exp.json
new file mode 100644
index 00000000000..5a40beacd22
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/p1689-3.exp.json
@@ -0,0 +1,16 @@
+{
+    "rules": [
+        {
+            "primary-output": "p1689-3.o",
+            "provides": [
+                {
+                    "logical-name": "foo:part2",
+                    "is-interface": false
+                }
+            ],
+            "requires": []
+        }
+    ],
+    "version": 0,
+    "revision": 0
+}
diff --git a/gcc/testsuite/g++.dg/modules/p1689-4.C b/gcc/testsuite/g++.dg/modules/p1689-4.C
new file mode 100644
index 00000000000..88bac77a8f8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/p1689-4.C
@@ -0,0 +1,14 @@
+// { dg-additional-options -E }
+// { dg-additional-options -MT }
+// { dg-additional-options p1689-4.json }
+// { dg-additional-options -MD }
+// { dg-additional-options -fmodules-ts }
+// { dg-additional-options -fdep-format=p1689r5 }
+// { dg-additional-options -fdep-output=p1689-4.o }
+// { dg-additional-options -fdep-file=p1689-4.json }
+
+// Module implementation unit.
+
+module foo;
+
+// { dg-final { run-check-p1689-valid p1689-4.json p1689-4.exp.json } }
diff --git a/gcc/testsuite/g++.dg/modules/p1689-4.exp.json b/gcc/testsuite/g++.dg/modules/p1689-4.exp.json
new file mode 100644
index 00000000000..b119f5654b1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/p1689-4.exp.json
@@ -0,0 +1,14 @@
+{
+    "rules": [
+        {
+            "primary-output": "p1689-4.o",
+            "requires": []
+                {
+                    "logical-name": "foo"
+                }
+            ]
+        }
+    ],
+    "version": 0,
+    "revision": 0
+}
diff --git a/gcc/testsuite/g++.dg/modules/p1689-5.C b/gcc/testsuite/g++.dg/modules/p1689-5.C
new file mode 100644
index 00000000000..e985277368b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/p1689-5.C
@@ -0,0 +1,14 @@
+// { dg-additional-options -E }
+// { dg-additional-options -MT }
+// { dg-additional-options p1689-5.json }
+// { dg-additional-options -MD }
+// { dg-additional-options -fmodules-ts }
+// { dg-additional-options -fdep-format=p1689r5 }
+// { dg-additional-options -fdep-output=p1689-5.o }
+// { dg-additional-options -fdep-file=p1689-5.json }
+
+// Use modules, don't provide anything.
+
+import bar;
+
+// { dg-final { run-check-p1689-valid p1689-5.json p1689-5.exp.json } }
diff --git a/gcc/testsuite/g++.dg/modules/p1689-5.exp.json b/gcc/testsuite/g++.dg/modules/p1689-5.exp.json
new file mode 100644
index 00000000000..18704ac8820
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/p1689-5.exp.json
@@ -0,0 +1,14 @@
+{
+    "rules": [
+        {
+            "primary-output": "p1689-5.o",
+            "requires": [
+                {
+                    "logical-name": "bar"
+                }
+            ]
+        }
+    ],
+    "version": 0,
+    "revision": 0
+}
diff --git a/gcc/testsuite/g++.dg/modules/test-p1689.py b/gcc/testsuite/g++.dg/modules/test-p1689.py
new file mode 100644
index 00000000000..2f07cc361aa
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/test-p1689.py
@@ -0,0 +1,222 @@
+import json
+
+
+# Parameters.
+ALL_ERRORS = False
+REPLACEMENTS = {}
+
+
+def _print_path(path):
+    '''Format a JSON path for output.'''
+    return '/'.join(path)
+
+
+def _report_error(msg):
+    '''Report an error.'''
+    full_msg = 'ERROR: ' + msg
+    if ALL_ERRORS:
+        print(full_msg)
+    else:
+        raise RuntimeError(full_msg)
+
+
+def _error_type_mismatch(path, actual, expect):
+    '''Report that there is a type mismatch.'''
+    _report_error('type mismatch at %s: actual: "%s" expect: "%s"' % (_print_path(path), actual, expect))
+
+
+def _error_unknown_type(path, typ):
+    '''Report that there is an unknown type in the JSON object.'''
+    _report_error('unknown type at %s: "%s"' % (_print_path(path), typ))
+
+
+def _error_length_mismatch(path, actual, expect):
+    '''Report a length mismatch in an object or array.'''
+    _report_error('length mismatch at %s: actual: "%s" expect: "%s"' % (_print_path(path), actual, expect))
+
+
+def _error_unexpect_value(path, actual, expect):
+    '''Report a value mismatch.'''
+    _report_error('value mismatch at %s: actual: "%s" expect: "%s"' % (_print_path(path), actual, expect))
+
+
+def _error_extra_key(path, key):
+    '''Report on a key that is unexpected.'''
+    _report_error('extra key at %s: "%s"' % (_print_path(path), key))
+
+
+def _error_missing_key(path, key):
+    '''Report on a key that is missing.'''
+    _report_error('extra key at %s: %s' % (_print_path(path), key))
+
+
+def _compare_object(path, actual, expect):
+    '''Compare a JSON object.'''
+    is_ok = True
+
+    if not len(actual) == len(expect):
+        _error_length_mismatch(path, len(actual), len(expect))
+        is_ok = False
+
+    for key in actual:
+        if key not in expect:
+            _error_extra_key(path, key)
+            is_ok = False
+        else:
+            sub_error = compare_json(path + [key], actual[key], expect[key])
+            if sub_error:
+                is_ok = False
+
+    for key in expect:
+        if key not in actual:
+            _error_missing_key(path, key)
+            is_ok = False
+
+    return is_ok
+
+
+def _compare_array(path, actual, expect):
+    '''Compare a JSON array.'''
+    is_ok = True
+
+    if not len(actual) == len(expect):
+        _error_length_mismatch(path, len(actual), len(expect))
+        is_ok = False
+
+    for (idx, (a, e)) in enumerate(zip(actual, expect)):
+        sub_error = compare_json(path + [str(idx)], a, e)
+        if sub_error:
+            is_ok = False
+
+    return is_ok
+
+
+def _make_replacements(value):
+    for (old, new) in REPLACEMENTS.values():
+        value = value.replace(old, new)
+    return value
+
+
+def _compare_string(path, actual, expect):
+    '''Compare a JSON string supporting replacements in the expected output.'''
+    expect = _make_replacements(expect)
+
+    if not actual == expect:
+        _error_unexpect_value(path, actual, expect)
+        return False
+    else:
+        print('%s is ok: %s' % (_print_path(path), actual))
+    return True
+
+
+def _compare_number(path, actual, expect):
+    '''Compare a JSON integer.'''
+    if not actual == expect:
+        _error_unexpect_value(path, actual, expect)
+        return False
+    else:
+        print('%s is ok: %s' % (_print_path(path), actual))
+    return True
+
+
+def _inspect_ordering(arr):
+    req_ordering = True
+
+    if not arr:
+        return arr, req_ordering
+
+    if arr[0] == '__P1689_unordered__':
+        arr.pop(0)
+        req_ordering = False
+
+    return arr, req_ordering
+
+
+def compare_json(path, actual, expect):
+    actual_type = type(actual)
+    expect_type = type(expect)
+
+    is_ok = True
+
+    if not actual_type == expect_type:
+        _error_type_mismatch(path, actual_type, expect_type)
+        is_ok = False
+    elif actual_type == dict:
+        is_ok = _compare_object(path, actual, expect)
+    elif actual_type == list:
+        expect, req_ordering = _inspect_ordering(expect)
+        if not req_ordering:
+            actual = set(actual)
+            expect = set(expect)
+        is_ok = _compare_array(path, actual, expect)
+    elif actual_type == str:
+        is_ok = _compare_string(path, actual, expect)
+    elif actual_type == float:
+        is_ok = _compare_number(path, actual, expect)
+    elif actual_type == int:
+        is_ok = _compare_number(path, actual, expect)
+    elif actual_type == bool:
+        is_ok = _compare_number(path, actual, expect)
+    elif actual_type == type(None):
+        pass
+    else:
+        _error_unknown_type(path, actual_type)
+        is_ok = False
+
+    return is_ok
+
+
+def validate_p1689(actual, expect):
+    '''Validate a P1689 file against an expected output file.
+
+    Returns `False` if it fails, `True` if they are the same.
+    '''
+    with open(actual, 'r') as fin:
+        actual_content = fin.read()
+    with open(expect, 'r') as fin:
+        expect_content = fin.read()
+
+    actual_json = json.loads(actual_content)
+    expect_json = json.loads(expect_content)
+
+    return compare_json([], actual_json, expect_json)
+
+
+if __name__ == '__main__':
+    import sys
+
+    actual = None
+    expect = None
+
+    # Parse arguments.
+    args = sys.argv[1:]
+    while args:
+        # Take an argument.
+        arg = args.pop(0)
+
+        # Parse out replacement expressions.
+        if arg == '-r' or arg == '--replace':
+            replacement = args.pop(0)
+            (key, value) = replacement.split('=', maxsplit=1)
+            REPLACEMENTS[key] = value
+        # Flag to change how errors are reported.
+        elif arg == '-A' or arg == '--all':
+            ALL_ERRORS = True
+        # Required arguments.
+        elif arg == '-a' or arg == '--actual':
+            actual = args.pop(0)
+        elif arg == '-e' or arg == '--expect':
+            expect = args.pop(0)
+
+    # Validate that we have the required arguments.
+    if actual is None:
+        raise RuntimeError('missing "actual" file')
+    if expect is None:
+        raise RuntimeError('missing "expect" file')
+
+    # Do the actual work.
+    is_ok = validate_p1689(actual, expect)
+
+    # Fail if errors are found.
+    if not is_ok:
+        sys.exit(1)
diff --git a/gcc/testsuite/lib/modules.exp b/gcc/testsuite/lib/modules.exp
new file mode 100644
index 00000000000..c7cfda6aae4
--- /dev/null
+++ b/gcc/testsuite/lib/modules.exp
@@ -0,0 +1,71 @@
+#   Copyright (C) 1997-2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with GCC; see the file COPYING3.  If not see
+# <http://www.gnu.org/licenses/>.
+
+# Verify various kinds of gcov output: line counts, branch percentages,
+# and call return percentages.  None of this is language-specific.
+
+load_lib "target-supports.exp"
+
+#
+# clean-p1689-file -- delete a working file the compiler creates for p1689
+#
+# TESTCASE is the name of the test.
+# SUFFIX is file suffix
+
+proc clean-p1689-file { testcase suffix } {
+    set basename [file tail $testcase]
+    set base [file rootname $basename]
+    remote_file host delete $base.$suffix
+}
+
+#
+# clean-p1689 -- delete the working files the compiler creates for p1689
+#
+# TESTCASE is the name of the test.
+#
+proc clean-p1689 { testcase } {
+    clean-p1689-file $testcase "d"
+    clean-p1689-file $testcase "json"
+}
+
+# Call by dg-final to check a P1689 dependency file
+
+proc run-check-p1689-valid { depfile template } {
+    global srcdir subdir
+    # Extract the test file name from the arguments.
+    set testcase [file rootname [file tail $depfile]]
+
+    verbose "Running P1689 validation for $testcase in $srcdir/$subdir" 2
+    set testcase [remote_download host $testcase]
+
+    set pytest_script "test-p1689.py"
+    if { ![check_effective_target_recent_python3] } {
+      unsupported "$pytest_script python3 is missing"
+      return
+    }
+
+    verbose "running script" 1
+    spawn -noecho python3 $srcdir/$subdir/$pytest_script --all --actual $depfile --expect $srcdir/$subdir/$template
+
+    expect {
+      -re "ERROR: (\[^\r\n\]*)" {
+       fail $expect_out(0,string)
+       exp_continue
+      }
+    }
+
+    clean-p1689 $testcase
+}
diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h
index 8df071e1587..aa1c065104a 100644
--- a/libcpp/include/cpplib.h
+++ b/libcpp/include/cpplib.h
@@ -302,6 +302,9 @@ typedef CPPCHAR_SIGNED_T cppchar_signed_t;
 /* Style of header dependencies to generate.  */
 enum cpp_deps_style { DEPS_NONE = 0, DEPS_USER, DEPS_SYSTEM };
 
+/* Format of header dependencies to generate.  */
+enum cpp_deps_format { DEPS_FMT_NONE = 0, DEPS_FMT_P1689R5 };
+
 /* The possible normalization levels, from most restrictive to least.  */
 enum cpp_normalize_level {
   /* In NFKC.  */
@@ -589,6 +592,9 @@ struct cpp_options
     /* Style of header dependencies to generate.  */
     enum cpp_deps_style style;
 
+    /* Format of header dependencies to generate.  */
+    enum cpp_deps_format format;
+
     /* Assume missing files are generated files.  */
     bool missing_files;
 
@@ -1112,9 +1118,9 @@ extern void cpp_post_options (cpp_reader *);
 extern void cpp_init_iconv (cpp_reader *);
 
 /* Call this to finish preprocessing.  If you requested dependency
-   generation, pass an open stream to write the information to,
-   otherwise NULL.  It is your responsibility to close the stream.  */
-extern void cpp_finish (cpp_reader *, FILE *deps_stream);
+   generation, pass open stream(s) to write the information to,
+   otherwise NULL.  It is your responsibility to close the stream(s).  */
+extern void cpp_finish (cpp_reader *, FILE *deps_stream, FILE *fdeps_stream = NULL);
 
 /* Call this to release the handle at the end of preprocessing.  Any
    use of the handle after this function returns is invalid.  */
diff --git a/libcpp/include/mkdeps.h b/libcpp/include/mkdeps.h
index 920e2791334..33c7437a481 100644
--- a/libcpp/include/mkdeps.h
+++ b/libcpp/include/mkdeps.h
@@ -53,20 +53,29 @@ extern void deps_add_default_target (class mkdeps *, const char *);
 
 /* Adds a module target.  The module name and cmi name are copied.  */
 extern void deps_add_module_target (struct mkdeps *, const char *module,
-				    const char *cmi, bool is_header);
+				    const char *cmi, bool is_header,
+				    bool is_exported);
 
 /* Adds a module dependency.  The module name is copied.  */
 extern void deps_add_module_dep (struct mkdeps *, const char *module);
 
+/* Add an output.  */
+extern void deps_add_output (struct mkdeps *, const char *, bool);
+
 /* Add a dependency (appears on the right side of the colon) to the
    deps list.  Dependencies will be printed in the order that they
    were entered with this function.  By convention, the first
    dependency entered should be the primary source file.  */
 extern void deps_add_dep (class mkdeps *, const char *);
 
-/* Write out a deps buffer to a specified file.  The last argument
-   is the number of columns to word-wrap at (0 means don't wrap).  */
-extern void deps_write (const cpp_reader *, FILE *, unsigned int);
+/* Write out a deps buffer to a specified file.  The third argument
+   is the number of columns to word-wrap at (0 means don't wrap).
+   The last argument indicates whether to output extra information
+   (namely modules).  */
+extern void deps_write (const struct cpp_reader *, FILE *, unsigned int);
+
+/* Write out a deps buffer to a specified file in P1689R5 format.  */
+extern void deps_write_p1689r5 (const struct mkdeps *, FILE *);
 
 /* Write out a deps buffer to a file, in a form that can be read back
    with deps_restore.  Returns nonzero on error, in which case the
diff --git a/libcpp/init.cc b/libcpp/init.cc
index c508f06112a..d34fd6fdeef 100644
--- a/libcpp/init.cc
+++ b/libcpp/init.cc
@@ -855,7 +855,7 @@ read_original_directory (cpp_reader *pfile)
    Maybe it should also reset state, such that you could call
    cpp_start_read with a new filename to restart processing.  */
 void
-cpp_finish (cpp_reader *pfile, FILE *deps_stream)
+cpp_finish (struct cpp_reader *pfile, FILE *deps_stream, FILE *fdeps_stream)
 {
   /* Warn about unused macros before popping the final buffer.  */
   if (CPP_OPTION (pfile, warn_unused_macros))
@@ -869,8 +869,15 @@ cpp_finish (cpp_reader *pfile, FILE *deps_stream)
   while (pfile->buffer)
     _cpp_pop_buffer (pfile);
 
-  if (deps_stream)
-    deps_write (pfile, deps_stream, 72);
+  cpp_deps_format deps_format = CPP_OPTION (pfile, deps.format);
+  if (deps_format == DEPS_FMT_P1689R5 && fdeps_stream)
+    deps_write_p1689r5 (pfile->deps, fdeps_stream);
+
+  if (CPP_OPTION (pfile, deps.style) != DEPS_NONE
+      && deps_stream)
+    {
+      deps_write (pfile, deps_stream, 72);
+    }
 
   /* Report on headers that could use multiple include guards.  */
   if (CPP_OPTION (pfile, print_include_names))
diff --git a/libcpp/mkdeps.cc b/libcpp/mkdeps.cc
index 8f9585c3c0a..081287c94ba 100644
--- a/libcpp/mkdeps.cc
+++ b/libcpp/mkdeps.cc
@@ -81,7 +81,8 @@ public:
   };
 
   mkdeps ()
-    : module_name (NULL), cmi_name (NULL), is_header_unit (false), quote_lwm (0)
+    : primary_output (NULL), module_name (NULL), cmi_name (NULL)
+    , is_header_unit (false), is_exported (false), quote_lwm (0)
   {
   }
   ~mkdeps ()
@@ -90,6 +91,9 @@ public:
 
     for (i = targets.size (); i--;)
       free (const_cast <char *> (targets[i]));
+    free (const_cast <char *> (primary_output));
+    for (i = outputs.size (); i--;)
+      free (const_cast <char *> (outputs[i]));
     for (i = deps.size (); i--;)
       free (const_cast <char *> (deps[i]));
     for (i = vpath.size (); i--;)
@@ -103,6 +107,8 @@ public:
 public:
   vec<const char *> targets;
   vec<const char *> deps;
+  const char * primary_output;
+  vec<const char *> outputs;
   vec<velt> vpath;
   vec<const char *> modules;
 
@@ -110,6 +116,7 @@ public:
   const char *module_name;
   const char *cmi_name;
   bool is_header_unit;
+  bool is_exported;
   unsigned short quote_lwm;
 };
 
@@ -288,6 +295,21 @@ deps_add_default_target (class mkdeps *d, const char *tgt)
     }
 }
 
+/* Adds an output O.  We make a copy, so it need not be a permanent
+   string.  */
+void
+deps_add_output (struct mkdeps *d, const char *o, bool is_primary)
+{
+  o = apply_vpath (d, o);
+  if (is_primary)
+  {
+    if (d->primary_output)
+      d->outputs.push (d->primary_output);
+    d->primary_output = xstrdup (o);
+  } else
+    d->outputs.push (xstrdup (o));
+}
+
 void
 deps_add_dep (class mkdeps *d, const char *t)
 {
@@ -325,12 +347,13 @@ deps_add_vpath (class mkdeps *d, const char *vpath)
 
 void
 deps_add_module_target (struct mkdeps *d, const char *m,
-			const char *cmi, bool is_header_unit)
+			const char *cmi, bool is_header_unit, bool is_exported)
 {
   gcc_assert (!d->module_name);
   
   d->module_name = xstrdup (m);
   d->is_header_unit = is_header_unit;
+  d->is_exported = is_exported;
   d->cmi_name = xstrdup (cmi);
 }
 
@@ -395,10 +418,15 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
   if (colmax && colmax < 34)
     colmax = 34;
 
+  /* Write out C++ modules information if no other `-fdeps-format=`
+   * option is given. */
+  cpp_deps_format deps_format = CPP_OPTION (pfile, deps.format);
+  bool write_make_modules_deps = deps_format == DEPS_FMT_NONE;
+
   if (d->deps.size ())
     {
       column = make_write_vec (d->targets, fp, 0, colmax, d->quote_lwm);
-      if (CPP_OPTION (pfile, deps.modules) && d->cmi_name)
+      if (write_make_modules_deps && CPP_OPTION (pfile, deps.modules) && d->cmi_name)
 	column = make_write_name (d->cmi_name, fp, column, colmax);
       fputs (":", fp);
       column++;
@@ -412,7 +440,7 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
   if (!CPP_OPTION (pfile, deps.modules))
     return;
 
-  if (d->modules.size ())
+  if (write_make_modules_deps && d->modules.size ())
     {
       column = make_write_vec (d->targets, fp, 0, colmax, d->quote_lwm);
       if (d->cmi_name)
@@ -423,7 +451,7 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
       fputs ("\n", fp);
     }
 
-  if (d->module_name)
+  if (write_make_modules_deps && d->module_name)
     {
       if (d->cmi_name)
 	{
@@ -455,7 +483,7 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
 	}
     }
   
-  if (d->modules.size ())
+  if (write_make_modules_deps && d->modules.size ())
     {
       column = fprintf (fp, "CXX_IMPORTS +=");
       make_write_vec (d->modules, fp, column, colmax, 0, ".c++m");
@@ -468,11 +496,118 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
 /* Really we should be opening fp here.  */
 
 void
-deps_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
+deps_write (const struct cpp_reader *pfile, FILE *fp, unsigned int colmax)
 {
   make_write (pfile, fp, colmax);
 }
 
+static void
+p1689r5_write_filepath (const char *name, FILE *fp)
+{
+  if (_cpp_valid_utf8_str (name))
+    {
+      fputc ('"', fp);
+      for (const char* c = name; *c; c++)
+	{
+	  // Escape control characters.
+	  if (ISCNTRL (*c))
+	    fprintf (fp, "\\u%04x", *c);
+	  // JSON escape characters.
+	  else if (*c == '"' || *c == '\\')
+	    {
+	      fputc ('\\', fp);
+	      fputc (*c, fp);
+	    }
+	  // Everything else.
+	  else
+	    fputc (*c, fp);
+	}
+      fputc ('"', fp);
+    }
+  else
+    {
+      // TODO: print an error
+    }
+}
+
+static void
+p1689r5_write_vec (const mkdeps::vec<const char *> &vec, FILE *fp)
+{
+  for (unsigned ix = 0; ix != vec.size (); ix++)
+    {
+      p1689r5_write_filepath (vec[ix], fp);
+      if (ix < vec.size () - 1)
+	fputc (',', fp);
+      fputc ('\n', fp);
+    }
+}
+
+void
+deps_write_p1689r5 (const struct mkdeps *d, FILE *fp)
+{
+  fputs ("{\n", fp);
+
+  fputs ("\"rules\": [\n", fp);
+  fputs ("{\n", fp);
+
+  if (d->primary_output)
+    {
+      fputs ("\"primary-output\": ", fp);
+      p1689r5_write_filepath (d->primary_output, fp);
+      fputs (",\n", fp);
+    }
+
+  if (d->outputs.size ())
+    {
+      fputs ("\"outputs\": [\n", fp);
+      p1689r5_write_vec (d->outputs, fp);
+      fputs ("],\n", fp);
+    }
+
+  if (d->module_name)
+    {
+      fputs ("\"provides\": [\n", fp);
+      fputs ("{\n", fp);
+
+      fputs ("\"logical-name\": ", fp);
+      p1689r5_write_filepath (d->module_name, fp);
+      fputs (",\n", fp);
+
+      fprintf (fp, "\"is-interface\": %s\n", d->is_exported ? "true" : "false");
+
+      // TODO: header-unit information
+
+      fputs ("}\n", fp);
+      fputs ("],\n", fp);
+    }
+
+  fputs ("\"requires\": [\n", fp);
+  for (size_t i = 0; i < d->modules.size (); i++)
+    {
+      if (i != 0)
+	fputs (",\n", fp);
+      fputs ("{\n", fp);
+
+      fputs ("\"logical-name\": ", fp);
+      p1689r5_write_filepath (d->modules[i], fp);
+      fputs ("\n", fp);
+
+      // TODO: header-unit information
+
+      fputs ("}\n", fp);
+    }
+  fputs ("]\n", fp);
+
+  fputs ("}\n", fp);
+
+  fputs ("],\n", fp);
+
+  fputs ("\"version\": 0,\n", fp);
+  fputs ("\"revision\": 0\n", fp);
+
+  fputs ("}\n", fp);
+}
+
 /* Write out a deps buffer to a file, in a form that can be read back
    with deps_restore.  Returns nonzero on error, in which case the
    error number will be in errno.  */
-- 
2.39.0

Re: [PATCH v5 3/5] p1689r5: initial support

[Jason Merrill]

On 1/25/23 13:06, Ben Boeckel wrote:
> This patch implements support for [P1689R5][] to communicate to a build
> system the C++20 module dependencies to build systems so that they may
> build `.gcm` files in the proper order.

Thanks again.

> Support is communicated through the following three new flags:
> 
> - `-fdeps-format=` specifies the format for the output. Currently named
>    `p1689r5`.
> 
> - `-fdeps-file=` specifies the path to the file to write the format to.
> 
> - `-fdep-output=` specifies the `.o` that will be written for the TU
>    that is scanned. This is required so that the build system can
>    correlate the dependency output with the actual compilation that will
>    occur.

I notice that the actual flags are all -fdep-*, though some of them are 
-fdeps-* here, and the internal variables all seem to be fdeps_*.  I 
lean toward harmonizing on "deps", I think.

I don't love the three separate options, but I suppose it's fine.  I'd 
prefer "target" instead of "output".

It should be possible to omit both -file and -target and get reasonable 
defaults, like the ones for -MD/-MQ in gcc.cc:cpp_unique_options.

> CMake supports this format as of 17 Jun 2022 (to be part of 3.25.0)
> using an experimental feature selection (to allow for future usage
> evolution without committing to how it works today). While it remains
> experimental, docs may be found in CMake's documentation for
> experimental features.
> 
> Future work may include using this format for Fortran module
> dependencies as well, however this is still pending work.
> 
> [P1689R5]: https://isocpp.org/files/papers/P1689R5.html
> [cmake-experimental]: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Help/dev/experimental.rst
> 
> TODO:
> 
> - header-unit information fields
> 
> Header units (including the standard library headers) are 100%
> unsupported right now because the `-E` mechanism wants to import their
> BMIs. A new mode (i.e., something more workable than existing `-E`
> behavior) that mocks up header units as if they were imported purely
> from their path and content would be required.

I notice that the cpp dependency generation tries (in open_file_failed) 
to continue after encountering a missing file, is that not sufficient 
for header units?  Or adjustable to be sufficient?

> - non-utf8 paths
> 
> The current standard says that paths that are not unambiguously
> represented using UTF-8 are not supported (because these cases are rare
> and the extra complication is not worth it at this time). Future
> versions of the format might have ways of encoding non-UTF-8 paths. For
> now, this patch just doesn't support non-UTF-8 paths (ignoring the
> "unambiguously represetable in UTF-8" case).

typo "representable"

> - figure out why junk gets placed at the end of the file
> 
> Sometimes it seems like the file gets a lot of `NUL` bytes appended to
> it. It happens rarely and seems to be the result of some
> `ftruncate`-style call which results in extra padding in the contents.
> Noting it here as an observation at least.
> 
> libcpp/
> 
> 	* include/cpplib.h: Add cpp_deps_format enum.
> 	(cpp_options): Add format field
> 	(cpp_finish): Add dependency stream parameter.
> 	* include/mkdeps.h (deps_add_module_target): Add new preprocessor
> 	parameter used for C++ module tracking.
> 	* init.cc (cpp_finish): Add new preprocessor parameter used for C++
> 	module tracking.
> 	* mkdeps.cc (mkdeps): Implement P1689R5 output.
> 
> gcc/
> 
> 	* doc/invoke.texi: Document -fdeps-format=, -fdep-file=, and
> 	-fdep-output= flags.
> 
> gcc/c-family/
> 
> 	* c-opts.cc (c_common_handle_option): Add fdeps_file variable and
> 	-fdeps-format=, -fdep-file=, and -fdep-output= parsing.
> 	* c.opt: Add -fdeps-format=, -fdep-file=, and -fdep-output= flags.
> 
> gcc/cp/
> 
> 	* module.cc (preprocessed_module): Pass whether the module is
> 	exported to dependency tracking.
> 
> gcc/testsuite/
> 
> 	* g++.dg/modules/depflags-f-MD.C: New test.
> 	* g++.dg/modules/depflags-f.C: New test.
> 	* g++.dg/modules/depflags-fi.C: New test.
> 	* g++.dg/modules/depflags-fj-MD.C: New test.
> 	* g++.dg/modules/depflags-fj.C: New test.
> 	* g++.dg/modules/depflags-fjo-MD.C: New test.
> 	* g++.dg/modules/depflags-fjo.C: New test.
> 	* g++.dg/modules/depflags-fo-MD.C: New test.
> 	* g++.dg/modules/depflags-fo.C: New test.
> 	* g++.dg/modules/depflags-j-MD.C: New test.
> 	* g++.dg/modules/depflags-j.C: New test.
> 	* g++.dg/modules/depflags-jo-MD.C: New test.
> 	* g++.dg/modules/depflags-jo.C: New test.
> 	* g++.dg/modules/depflags-o-MD.C: New test.
> 	* g++.dg/modules/depflags-o.C: New test.
> 	* g++.dg/modules/p1689-1.C: New test.
> 	* g++.dg/modules/p1689-1.exp.json: New test expectation.
> 	* g++.dg/modules/p1689-2.C: New test.
> 	* g++.dg/modules/p1689-2.exp.json: New test expectation.
> 	* g++.dg/modules/p1689-3.C: New test.
> 	* g++.dg/modules/p1689-3.exp.json: New test expectation.
> 	* g++.dg/modules/p1689-4.C: New test.
> 	* g++.dg/modules/p1689-4.exp.json: New test expectation.
> 	* g++.dg/modules/p1689-5.C: New test.
> 	* g++.dg/modules/p1689-5.exp.json: New test expectation.
> 	* g++.dg/modules/modules.exp: Load new P1689 library routines.
> 	* g++.dg/modules/test-p1689.py: New tool for validating P1689 output.
> 	* lib/modules.exp: Support for validating P1689 outputs.
> 
> Signed-off-by: Ben Boeckel <ben.boeckel@kitware.com>
> ---
>   gcc/c-family/c-opts.cc                        |  40 +++-
>   gcc/c-family/c.opt                            |  12 +
>   gcc/cp/module.cc                              |   3 +-
>   gcc/doc/invoke.texi                           |  15 ++
>   gcc/testsuite/g++.dg/modules/depflags-f-MD.C  |   2 +
>   gcc/testsuite/g++.dg/modules/depflags-f.C     |   1 +
>   gcc/testsuite/g++.dg/modules/depflags-fi.C    |   3 +
>   gcc/testsuite/g++.dg/modules/depflags-fj-MD.C |   3 +
>   gcc/testsuite/g++.dg/modules/depflags-fj.C    |   4 +
>   .../g++.dg/modules/depflags-fjo-MD.C          |   4 +
>   gcc/testsuite/g++.dg/modules/depflags-fjo.C   |   5 +
>   gcc/testsuite/g++.dg/modules/depflags-fo-MD.C |   3 +
>   gcc/testsuite/g++.dg/modules/depflags-fo.C    |   4 +
>   gcc/testsuite/g++.dg/modules/depflags-j-MD.C  |   2 +
>   gcc/testsuite/g++.dg/modules/depflags-j.C     |   3 +
>   gcc/testsuite/g++.dg/modules/depflags-jo-MD.C |   3 +
>   gcc/testsuite/g++.dg/modules/depflags-jo.C    |   4 +
>   gcc/testsuite/g++.dg/modules/depflags-o-MD.C  |   2 +
>   gcc/testsuite/g++.dg/modules/depflags-o.C     |   3 +
>   gcc/testsuite/g++.dg/modules/modules.exp      |   1 +
>   gcc/testsuite/g++.dg/modules/p1689-1.C        |  18 ++
>   gcc/testsuite/g++.dg/modules/p1689-1.exp.json |  27 +++
>   gcc/testsuite/g++.dg/modules/p1689-2.C        |  16 ++
>   gcc/testsuite/g++.dg/modules/p1689-2.exp.json |  16 ++
>   gcc/testsuite/g++.dg/modules/p1689-3.C        |  14 ++
>   gcc/testsuite/g++.dg/modules/p1689-3.exp.json |  16 ++
>   gcc/testsuite/g++.dg/modules/p1689-4.C        |  14 ++
>   gcc/testsuite/g++.dg/modules/p1689-4.exp.json |  14 ++
>   gcc/testsuite/g++.dg/modules/p1689-5.C        |  14 ++
>   gcc/testsuite/g++.dg/modules/p1689-5.exp.json |  14 ++
>   gcc/testsuite/g++.dg/modules/test-p1689.py    | 222 ++++++++++++++++++
>   gcc/testsuite/lib/modules.exp                 |  71 ++++++
>   libcpp/include/cpplib.h                       |  12 +-
>   libcpp/include/mkdeps.h                       |  17 +-
>   libcpp/init.cc                                |  13 +-
>   libcpp/mkdeps.cc                              | 149 +++++++++++-
>   36 files changed, 745 insertions(+), 19 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-f-MD.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-f.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fi.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fj-MD.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fj.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fjo-MD.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fjo.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fo-MD.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fo.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-j-MD.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-j.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-jo-MD.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-jo.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-o-MD.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-o.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-1.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-1.exp.json
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-2.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-2.exp.json
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-3.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-3.exp.json
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-4.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-4.exp.json
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-5.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-5.exp.json
>   create mode 100644 gcc/testsuite/g++.dg/modules/test-p1689.py
>   create mode 100644 gcc/testsuite/lib/modules.exp
> 
> diff --git a/gcc/c-family/c-opts.cc b/gcc/c-family/c-opts.cc
> index c68a2a27469..1c14ce3fe8e 100644
> --- a/gcc/c-family/c-opts.cc
> +++ b/gcc/c-family/c-opts.cc
> @@ -77,6 +77,9 @@ static bool verbose;
>   /* Dependency output file.  */
>   static const char *deps_file;
>   
> +/* Enhanced dependency output file.  */

Maybe "structured", as in the docs?  It isn't really a direct 
enhancement of the makefile dependencies.

> +static const char *fdeps_file;
> +
>   /* The prefix given by -iprefix, if any.  */
>   static const char *iprefix;
>   
> @@ -360,6 +363,23 @@ c_common_handle_option (size_t scode, const char *arg, HOST_WIDE_INT value,
>         deps_file = arg;
>         break;
>   
> +    case OPT_fdep_format_:
> +      if (!strcmp (arg, "p1689r5"))
> +	cpp_opts->deps.format = DEPS_FMT_P1689R5;
> +      else
> +	error ("%<-fdep-format=%> unknown format %<%s%>", arg);
> +      break;
> +
> +    case OPT_fdep_file_:
> +      deps_seen = true;
> +      fdeps_file = arg;
> +      break;
> +
> +    case OPT_fdep_output_:
> +      deps_seen = true;
> +      defer_opt (code, arg);
> +      break;
> +
>       case OPT_MF:
>         deps_seen = true;
>         deps_file = arg;
> @@ -1272,6 +1292,7 @@ void
>   c_common_finish (void)
>   {
>     FILE *deps_stream = NULL;
> +  FILE *fdeps_stream = NULL;
>   
>     /* Note that we write the dependencies even if there are errors. This is
>        useful for handling outdated generated headers that now trigger errors
> @@ -1300,9 +1321,24 @@ c_common_finish (void)
>        locations with input_location, which would be incorrect now.  */
>     override_libcpp_locations = false;
>   
> +  if (cpp_opts->deps.format != DEPS_FMT_NONE)
> +    {
> +      if (!fdeps_file)
> +	fdeps_stream = out_stream;
> +      else if (fdeps_file[0] == '-' && fdeps_file[1] == '\0')
> +	fdeps_stream = stdout;

You probably want to check that deps_stream and fdeps_stream don't end 
up as the same stream.

> +      else
> +	{
> +	  fdeps_stream = fopen (fdeps_file, "w");
> +	  if (!fdeps_stream)
> +	    fatal_error (input_location, "opening dependency file %s: %m",
> +			 fdeps_file);
> +	}
> +    }
> +
>     /* For performance, avoid tearing down cpplib's internal structures
>        with cpp_destroy ().  */
> -  cpp_finish (parse_in, deps_stream);
> +  cpp_finish (parse_in, deps_stream, fdeps_stream);
>   
>     if (deps_stream && deps_stream != out_stream && deps_stream != stdout
>         && (ferror (deps_stream) || fclose (deps_stream)))
> @@ -1374,6 +1410,8 @@ handle_deferred_opts (void)
>   
>   	if (opt->code == OPT_MT || opt->code == OPT_MQ)
>   	  deps_add_target (deps, opt->arg, opt->code == OPT_MQ);
> +	else if (opt->code == OPT_fdep_output_)
> +	  deps_add_output (deps, opt->arg, true);

How about fdeps_add_target?

>         }
>   }
>   
> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> index ef371ca8c26..630781fdf8a 100644
> --- a/gcc/c-family/c.opt
> +++ b/gcc/c-family/c.opt
> @@ -256,6 +256,18 @@ MT
>   C ObjC C++ ObjC++ Joined Separate MissingArgError(missing makefile target after %qs)
>   -MT <target>	Add a target that does not require quoting.
>   
> +fdep-format=
> +C ObjC C++ ObjC++ NoDriverArg Joined MissingArgError(missing format after %qs)
> +Format for output dependency information.  Supported (\"p1689r5\").

I think we want "structured" here, as well.

> +fdep-file=
> +C ObjC C++ ObjC++ NoDriverArg Joined MissingArgError(missing output path after %qs)
> +File for output dependency information.
> +
> +fdep-output=
> +C ObjC C++ ObjC++ NoDriverArg Joined MissingArgError(missing path after %qs)
> +-fdep-output=obj.o Output file for the compile step.
> +
>   P
>   C ObjC C++ ObjC++
>   Do not generate #line directives.
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index ac2fe66b080..ebd30f63d81 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -19832,7 +19832,8 @@ preprocessed_module (cpp_reader *reader)
>   		  && (module->is_interface () || module->is_partition ()))
>   		deps_add_module_target (deps, module->get_flatname (),
>   					maybe_add_cmi_prefix (module->filename),
> -					module->is_header());
> +					module->is_header (),
> +					module->is_exported ());
>   	      else
>   		deps_add_module_dep (deps, module->get_flatname ());
>   	    }
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 06d77983e30..b61c3ebd3ec 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -2791,6 +2791,21 @@ is @option{-fpermitted-flt-eval-methods=c11}.  The default when in a GNU
>   dialect (@option{-std=gnu11} or similar) is
>   @option{-fpermitted-flt-eval-methods=ts-18661-3}.
>   
> +@item -fdep-file=@var{file}
> +@opindex fdep-file
> +Where to write structured dependency information.
> +
> +@item -fdep-format=@var{format}
> +@opindex fdep-format
> +The format to use for structured dependency information. @samp{p1689r5} is the
> +only supported format right now.  Note that when this argument is specified, the
> +output of @samp{-MF} is stripped of some information (namely C++ modules) so
> +that it does not use extended makefile syntax not understood by most tools.
> +
> +@item -fdep-output=@var{file}
> +@opindex fdep-output
> +Analogous to @option{-MT} but for structured dependency information.

Please add more detail about how these are intended to be used.

>   @item -fplan9-extensions
>   @opindex fplan9-extensions
>   Accept some non-standard constructs used in Plan 9 code.
> diff --git a/gcc/testsuite/g++.dg/modules/depflags-f-MD.C b/gcc/testsuite/g++.dg/modules/depflags-f-MD.C
> new file mode 100644
> index 00000000000..90e1c9983bd
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/depflags-f-MD.C
> @@ -0,0 +1,2 @@
> +// { dg-additional-options -MD }
> +// { dg-additional-options -fdep-format=p1689r5 }
> diff --git a/gcc/testsuite/g++.dg/modules/depflags-f.C b/gcc/testsuite/g++.dg/modules/depflags-f.C
> new file mode 100644
> index 00000000000..6192300879d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/depflags-f.C
> @@ -0,0 +1 @@
> +// { dg-additional-options -fdep-format=p1689r5 }
> diff --git a/gcc/testsuite/g++.dg/modules/depflags-fi.C b/gcc/testsuite/g++.dg/modules/depflags-fi.C
> new file mode 100644
> index 00000000000..4f649a11bdd
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/depflags-fi.C
> @@ -0,0 +1,3 @@
> +// { dg-additional-options -fdep-format=invalid }
> +
> +// { dg-prune-output "error: '-fdep-format=' unknown format 'invalid'"  }
> diff --git a/gcc/testsuite/g++.dg/modules/depflags-fj-MD.C b/gcc/testsuite/g++.dg/modules/depflags-fj-MD.C
> new file mode 100644
> index 00000000000..a361d81f37f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/depflags-fj-MD.C
> @@ -0,0 +1,3 @@
> +// { dg-additional-options -MD }
> +// { dg-additional-options -fdep-file=depflags-3.json }
> +// { dg-additional-options -fdep-format=p1689r5 }
> diff --git a/gcc/testsuite/g++.dg/modules/depflags-fj.C b/gcc/testsuite/g++.dg/modules/depflags-fj.C
> new file mode 100644
> index 00000000000..4a140ec1f13
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/depflags-fj.C
> @@ -0,0 +1,4 @@
> +// { dg-additional-options -fdep-file=depflags-3.json }
> +// { dg-additional-options -fdep-format=p1689r5 }
> +
> +// { dg-prune-output "error: to generate dependencies you must specify either '-M' or '-MM'" }
> diff --git a/gcc/testsuite/g++.dg/modules/depflags-fjo-MD.C b/gcc/testsuite/g++.dg/modules/depflags-fjo-MD.C
> new file mode 100644
> index 00000000000..18d765211b4
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/depflags-fjo-MD.C
> @@ -0,0 +1,4 @@
> +// { dg-additional-options -MD }
> +// { dg-additional-options -fdep-file=depflags-3.json }
> +// { dg-additional-options -fdep-output=depflags-1.C }
> +// { dg-additional-options -fdep-format=p1689r5 }
> diff --git a/gcc/testsuite/g++.dg/modules/depflags-fjo.C b/gcc/testsuite/g++.dg/modules/depflags-fjo.C
> new file mode 100644
> index 00000000000..6d239f63017
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/depflags-fjo.C
> @@ -0,0 +1,5 @@
> +// { dg-additional-options -fdep-file=depflags-3.json }
> +// { dg-additional-options -fdep-output=depflags-1.C }
> +// { dg-additional-options -fdep-format=p1689r5 }
> +
> +// { dg-prune-output "error: to generate dependencies you must specify either '-M' or '-MM'" }
> diff --git a/gcc/testsuite/g++.dg/modules/depflags-fo-MD.C b/gcc/testsuite/g++.dg/modules/depflags-fo-MD.C
> new file mode 100644
> index 00000000000..a3a775b606a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/depflags-fo-MD.C
> @@ -0,0 +1,3 @@
> +// { dg-additional-options -MD }
> +// { dg-additional-options -fdep-format=p1689r5 }
> +// { dg-additional-options -fdep-output=depflags-1.C }
> diff --git a/gcc/testsuite/g++.dg/modules/depflags-fo.C b/gcc/testsuite/g++.dg/modules/depflags-fo.C
> new file mode 100644
> index 00000000000..29839978e59
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/depflags-fo.C
> @@ -0,0 +1,4 @@
> +// { dg-additional-options -fdep-format=p1689r5 }
> +// { dg-additional-options -fdep-output=depflags-1.C }
> +
> +// { dg-prune-output "error: to generate dependencies you must specify either '-M' or '-MM'" }
> diff --git a/gcc/testsuite/g++.dg/modules/depflags-j-MD.C b/gcc/testsuite/g++.dg/modules/depflags-j-MD.C
> new file mode 100644
> index 00000000000..d95c8e6c2e6
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/depflags-j-MD.C
> @@ -0,0 +1,2 @@
> +// { dg-additional-options -MD }
> +// { dg-additional-options -fdep-file=depflags-3.json }
> diff --git a/gcc/testsuite/g++.dg/modules/depflags-j.C b/gcc/testsuite/g++.dg/modules/depflags-j.C
> new file mode 100644
> index 00000000000..5f100b0f6e5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/depflags-j.C
> @@ -0,0 +1,3 @@
> +// { dg-additional-options -fdep-file=depflags-3.json }
> +
> +// { dg-prune-output "error: to generate dependencies you must specify either '-M' or '-MM'" }
> diff --git a/gcc/testsuite/g++.dg/modules/depflags-jo-MD.C b/gcc/testsuite/g++.dg/modules/depflags-jo-MD.C
> new file mode 100644
> index 00000000000..44330794abc
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/depflags-jo-MD.C
> @@ -0,0 +1,3 @@
> +// { dg-additional-options -MD }
> +// { dg-additional-options -fdep-file=depflags-3.json }
> +// { dg-additional-options -fdep-output=depflags-1.C }
> diff --git a/gcc/testsuite/g++.dg/modules/depflags-jo.C b/gcc/testsuite/g++.dg/modules/depflags-jo.C
> new file mode 100644
> index 00000000000..8eec6bba1d1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/depflags-jo.C
> @@ -0,0 +1,4 @@
> +// { dg-additional-options -fdep-file=depflags-3.json }
> +// { dg-additional-options -fdep-output=depflags-1.C }
> +
> +// { dg-prune-output "error: to generate dependencies you must specify either '-M' or '-MM'" }
> diff --git a/gcc/testsuite/g++.dg/modules/depflags-o-MD.C b/gcc/testsuite/g++.dg/modules/depflags-o-MD.C
> new file mode 100644
> index 00000000000..429f1f85684
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/depflags-o-MD.C
> @@ -0,0 +1,2 @@
> +// { dg-additional-options -MD }
> +// { dg-additional-options -fdep-output=depflags-1.C }
> diff --git a/gcc/testsuite/g++.dg/modules/depflags-o.C b/gcc/testsuite/g++.dg/modules/depflags-o.C
> new file mode 100644
> index 00000000000..9a7326cc812
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/depflags-o.C
> @@ -0,0 +1,3 @@
> +// { dg-additional-options -fdep-output=depflags-1.C }
> +
> +// { dg-prune-output "error: to generate dependencies you must specify either '-M' or '-MM'" }
> diff --git a/gcc/testsuite/g++.dg/modules/modules.exp b/gcc/testsuite/g++.dg/modules/modules.exp
> index 61994b05945..c06e1cbca8a 100644
> --- a/gcc/testsuite/g++.dg/modules/modules.exp
> +++ b/gcc/testsuite/g++.dg/modules/modules.exp
> @@ -28,6 +28,7 @@
>   # { dg-module-do [link|run] [xfail] [options] } # link [and run]
>   
>   load_lib g++-dg.exp
> +load_lib modules.exp
>   
>   # If a testcase doesn't have special options, use these.
>   global DEFAULT_CXXFLAGS
> diff --git a/gcc/testsuite/g++.dg/modules/p1689-1.C b/gcc/testsuite/g++.dg/modules/p1689-1.C
> new file mode 100644
> index 00000000000..245e30d09ce
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/p1689-1.C
> @@ -0,0 +1,18 @@
> +// { dg-additional-options -E }
> +// { dg-additional-options -MT }
> +// { dg-additional-options p1689-1.json }
> +// { dg-additional-options -MD }
> +// { dg-additional-options -fmodules-ts }
> +// { dg-additional-options -fdep-format=p1689r5 }
> +// { dg-additional-options -fdep-output=p1689-1.o }
> +// { dg-additional-options -fdep-file=p1689-1.json }

Note that with dg-additional-options you can put multiple flags in "" 
rather than put each on its own line.

So, I take it this is the common use case you have in mind, generating 
Make dependencies for the p1689 file?  When are you thinking the Make 
dependencies for the .o are generated?  At build time?

I'm a bit surprised you're using .json rather than an extension that 
indicates what the information is.

> +// Export a module that uses modules, re-exports modules, and partitions.
> +
> +export module foo;
> +export import foo:part1;
> +import foo:part2;
> +
> +export import bar;
> +
> +// { dg-final { run-check-p1689-valid p1689-1.json p1689-1.exp.json } }
> diff --git a/gcc/testsuite/g++.dg/modules/p1689-1.exp.json b/gcc/testsuite/g++.dg/modules/p1689-1.exp.json
> new file mode 100644
> index 00000000000..c5648ac7ae5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/p1689-1.exp.json
> @@ -0,0 +1,27 @@
> +{
> +    "rules": [
> +        {
> +            "primary-output": "p1689-1.o",
> +            "provides": [
> +                {
> +                    "logical-name": "foo",
> +                    "is-interface": true
> +                }
> +            ],
> +            "requires": [
> +                "__P1689_unordered__",
> +                {
> +                    "logical-name": "bar"
> +                },
> +                {
> +                    "logical-name": "foo:part1"
> +                },
> +                {
> +                    "logical-name": "foo:part2"
> +                }
> +            ]
> +        }
> +    ],
> +    "version": 0,
> +    "revision": 0
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/p1689-2.C b/gcc/testsuite/g++.dg/modules/p1689-2.C
> new file mode 100644
> index 00000000000..add07f59a0e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/p1689-2.C
> @@ -0,0 +1,16 @@
> +// { dg-additional-options -E }
> +// { dg-additional-options -MT }
> +// { dg-additional-options p1689-2.json }
> +// { dg-additional-options -MD }
> +// { dg-additional-options -fmodules-ts }
> +// { dg-additional-options -fdep-format=p1689r5 }
> +// { dg-additional-options -fdep-output=p1689-2.o }
> +// { dg-additional-options -fdep-file=p1689-2.json }
> +
> +// Export a module partition that uses modules.
> +
> +export module foo:part1;
> +
> +#include <iostream>
> +
> +// { dg-final { run-check-p1689-valid p1689-2.json p1689-2.exp.json } }
> diff --git a/gcc/testsuite/g++.dg/modules/p1689-2.exp.json b/gcc/testsuite/g++.dg/modules/p1689-2.exp.json
> new file mode 100644
> index 00000000000..6901172f277
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/p1689-2.exp.json
> @@ -0,0 +1,16 @@
> +{
> +    "rules": [
> +        {
> +            "primary-output": "p1689-2.o",
> +            "provides": [
> +                {
> +                    "logical-name": "foo:part1",
> +                    "is-interface": true
> +                }
> +            ],
> +            "requires": []
> +        }
> +    ],
> +    "version": 0,
> +    "revision": 0
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/p1689-3.C b/gcc/testsuite/g++.dg/modules/p1689-3.C
> new file mode 100644
> index 00000000000..3482c2f4903
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/p1689-3.C
> @@ -0,0 +1,14 @@
> +// { dg-additional-options -E }
> +// { dg-additional-options -MT }
> +// { dg-additional-options p1689-3.json }
> +// { dg-additional-options -MD }
> +// { dg-additional-options -fmodules-ts }
> +// { dg-additional-options -fdep-format=p1689r5 }
> +// { dg-additional-options -fdep-output=p1689-3.o }
> +// { dg-additional-options -fdep-file=p1689-3.json }
> +
> +// Provide a module partition.
> +
> +module foo:part2;
> +
> +// { dg-final { run-check-p1689-valid p1689-3.json p1689-3.exp.json } }
> diff --git a/gcc/testsuite/g++.dg/modules/p1689-3.exp.json b/gcc/testsuite/g++.dg/modules/p1689-3.exp.json
> new file mode 100644
> index 00000000000..5a40beacd22
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/p1689-3.exp.json
> @@ -0,0 +1,16 @@
> +{
> +    "rules": [
> +        {
> +            "primary-output": "p1689-3.o",
> +            "provides": [
> +                {
> +                    "logical-name": "foo:part2",
> +                    "is-interface": false
> +                }
> +            ],
> +            "requires": []
> +        }
> +    ],
> +    "version": 0,
> +    "revision": 0
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/p1689-4.C b/gcc/testsuite/g++.dg/modules/p1689-4.C
> new file mode 100644
> index 00000000000..88bac77a8f8
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/p1689-4.C
> @@ -0,0 +1,14 @@
> +// { dg-additional-options -E }
> +// { dg-additional-options -MT }
> +// { dg-additional-options p1689-4.json }
> +// { dg-additional-options -MD }
> +// { dg-additional-options -fmodules-ts }
> +// { dg-additional-options -fdep-format=p1689r5 }
> +// { dg-additional-options -fdep-output=p1689-4.o }
> +// { dg-additional-options -fdep-file=p1689-4.json }
> +
> +// Module implementation unit.
> +
> +module foo;
> +
> +// { dg-final { run-check-p1689-valid p1689-4.json p1689-4.exp.json } }
> diff --git a/gcc/testsuite/g++.dg/modules/p1689-4.exp.json b/gcc/testsuite/g++.dg/modules/p1689-4.exp.json
> new file mode 100644
> index 00000000000..b119f5654b1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/p1689-4.exp.json
> @@ -0,0 +1,14 @@
> +{
> +    "rules": [
> +        {
> +            "primary-output": "p1689-4.o",
> +            "requires": []
> +                {
> +                    "logical-name": "foo"
> +                }
> +            ]
> +        }
> +    ],
> +    "version": 0,
> +    "revision": 0
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/p1689-5.C b/gcc/testsuite/g++.dg/modules/p1689-5.C
> new file mode 100644
> index 00000000000..e985277368b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/p1689-5.C
> @@ -0,0 +1,14 @@
> +// { dg-additional-options -E }
> +// { dg-additional-options -MT }
> +// { dg-additional-options p1689-5.json }
> +// { dg-additional-options -MD }
> +// { dg-additional-options -fmodules-ts }
> +// { dg-additional-options -fdep-format=p1689r5 }
> +// { dg-additional-options -fdep-output=p1689-5.o }
> +// { dg-additional-options -fdep-file=p1689-5.json }
> +
> +// Use modules, don't provide anything.
> +
> +import bar;
> +
> +// { dg-final { run-check-p1689-valid p1689-5.json p1689-5.exp.json } }
> diff --git a/gcc/testsuite/g++.dg/modules/p1689-5.exp.json b/gcc/testsuite/g++.dg/modules/p1689-5.exp.json
> new file mode 100644
> index 00000000000..18704ac8820
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/p1689-5.exp.json
> @@ -0,0 +1,14 @@
> +{
> +    "rules": [
> +        {
> +            "primary-output": "p1689-5.o",
> +            "requires": [
> +                {
> +                    "logical-name": "bar"
> +                }
> +            ]
> +        }
> +    ],
> +    "version": 0,
> +    "revision": 0
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/test-p1689.py b/gcc/testsuite/g++.dg/modules/test-p1689.py
> new file mode 100644
> index 00000000000..2f07cc361aa
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/test-p1689.py
> @@ -0,0 +1,222 @@
> +import json
> +
> +
> +# Parameters.
> +ALL_ERRORS = False
> +REPLACEMENTS = {}
> +
> +
> +def _print_path(path):
> +    '''Format a JSON path for output.'''
> +    return '/'.join(path)
> +
> +
> +def _report_error(msg):
> +    '''Report an error.'''
> +    full_msg = 'ERROR: ' + msg
> +    if ALL_ERRORS:
> +        print(full_msg)
> +    else:
> +        raise RuntimeError(full_msg)
> +
> +
> +def _error_type_mismatch(path, actual, expect):
> +    '''Report that there is a type mismatch.'''
> +    _report_error('type mismatch at %s: actual: "%s" expect: "%s"' % (_print_path(path), actual, expect))
> +
> +
> +def _error_unknown_type(path, typ):
> +    '''Report that there is an unknown type in the JSON object.'''
> +    _report_error('unknown type at %s: "%s"' % (_print_path(path), typ))
> +
> +
> +def _error_length_mismatch(path, actual, expect):
> +    '''Report a length mismatch in an object or array.'''
> +    _report_error('length mismatch at %s: actual: "%s" expect: "%s"' % (_print_path(path), actual, expect))
> +
> +
> +def _error_unexpect_value(path, actual, expect):
> +    '''Report a value mismatch.'''
> +    _report_error('value mismatch at %s: actual: "%s" expect: "%s"' % (_print_path(path), actual, expect))
> +
> +
> +def _error_extra_key(path, key):
> +    '''Report on a key that is unexpected.'''
> +    _report_error('extra key at %s: "%s"' % (_print_path(path), key))
> +
> +
> +def _error_missing_key(path, key):
> +    '''Report on a key that is missing.'''
> +    _report_error('extra key at %s: %s' % (_print_path(path), key))
> +
> +
> +def _compare_object(path, actual, expect):
> +    '''Compare a JSON object.'''
> +    is_ok = True
> +
> +    if not len(actual) == len(expect):
> +        _error_length_mismatch(path, len(actual), len(expect))
> +        is_ok = False
> +
> +    for key in actual:
> +        if key not in expect:
> +            _error_extra_key(path, key)
> +            is_ok = False
> +        else:
> +            sub_error = compare_json(path + [key], actual[key], expect[key])
> +            if sub_error:
> +                is_ok = False
> +
> +    for key in expect:
> +        if key not in actual:
> +            _error_missing_key(path, key)
> +            is_ok = False
> +
> +    return is_ok
> +
> +
> +def _compare_array(path, actual, expect):
> +    '''Compare a JSON array.'''
> +    is_ok = True
> +
> +    if not len(actual) == len(expect):
> +        _error_length_mismatch(path, len(actual), len(expect))
> +        is_ok = False
> +
> +    for (idx, (a, e)) in enumerate(zip(actual, expect)):
> +        sub_error = compare_json(path + [str(idx)], a, e)
> +        if sub_error:
> +            is_ok = False
> +
> +    return is_ok
> +
> +
> +def _make_replacements(value):
> +    for (old, new) in REPLACEMENTS.values():
> +        value = value.replace(old, new)
> +    return value
> +
> +
> +def _compare_string(path, actual, expect):
> +    '''Compare a JSON string supporting replacements in the expected output.'''
> +    expect = _make_replacements(expect)
> +
> +    if not actual == expect:
> +        _error_unexpect_value(path, actual, expect)
> +        return False
> +    else:
> +        print('%s is ok: %s' % (_print_path(path), actual))
> +    return True
> +
> +
> +def _compare_number(path, actual, expect):
> +    '''Compare a JSON integer.'''
> +    if not actual == expect:
> +        _error_unexpect_value(path, actual, expect)
> +        return False
> +    else:
> +        print('%s is ok: %s' % (_print_path(path), actual))
> +    return True
> +
> +
> +def _inspect_ordering(arr):
> +    req_ordering = True
> +
> +    if not arr:
> +        return arr, req_ordering
> +
> +    if arr[0] == '__P1689_unordered__':
> +        arr.pop(0)
> +        req_ordering = False
> +
> +    return arr, req_ordering
> +
> +
> +def compare_json(path, actual, expect):
> +    actual_type = type(actual)
> +    expect_type = type(expect)
> +
> +    is_ok = True
> +
> +    if not actual_type == expect_type:
> +        _error_type_mismatch(path, actual_type, expect_type)
> +        is_ok = False
> +    elif actual_type == dict:
> +        is_ok = _compare_object(path, actual, expect)
> +    elif actual_type == list:
> +        expect, req_ordering = _inspect_ordering(expect)
> +        if not req_ordering:
> +            actual = set(actual)
> +            expect = set(expect)
> +        is_ok = _compare_array(path, actual, expect)
> +    elif actual_type == str:
> +        is_ok = _compare_string(path, actual, expect)
> +    elif actual_type == float:
> +        is_ok = _compare_number(path, actual, expect)
> +    elif actual_type == int:
> +        is_ok = _compare_number(path, actual, expect)
> +    elif actual_type == bool:
> +        is_ok = _compare_number(path, actual, expect)
> +    elif actual_type == type(None):
> +        pass
> +    else:
> +        _error_unknown_type(path, actual_type)
> +        is_ok = False
> +
> +    return is_ok
> +
> +
> +def validate_p1689(actual, expect):
> +    '''Validate a P1689 file against an expected output file.
> +
> +    Returns `False` if it fails, `True` if they are the same.
> +    '''
> +    with open(actual, 'r') as fin:
> +        actual_content = fin.read()
> +    with open(expect, 'r') as fin:
> +        expect_content = fin.read()
> +
> +    actual_json = json.loads(actual_content)
> +    expect_json = json.loads(expect_content)
> +
> +    return compare_json([], actual_json, expect_json)
> +
> +
> +if __name__ == '__main__':
> +    import sys
> +
> +    actual = None
> +    expect = None
> +
> +    # Parse arguments.
> +    args = sys.argv[1:]
> +    while args:
> +        # Take an argument.
> +        arg = args.pop(0)
> +
> +        # Parse out replacement expressions.
> +        if arg == '-r' or arg == '--replace':
> +            replacement = args.pop(0)
> +            (key, value) = replacement.split('=', maxsplit=1)
> +            REPLACEMENTS[key] = value
> +        # Flag to change how errors are reported.
> +        elif arg == '-A' or arg == '--all':
> +            ALL_ERRORS = True
> +        # Required arguments.
> +        elif arg == '-a' or arg == '--actual':
> +            actual = args.pop(0)
> +        elif arg == '-e' or arg == '--expect':
> +            expect = args.pop(0)
> +
> +    # Validate that we have the required arguments.
> +    if actual is None:
> +        raise RuntimeError('missing "actual" file')
> +    if expect is None:
> +        raise RuntimeError('missing "expect" file')
> +
> +    # Do the actual work.
> +    is_ok = validate_p1689(actual, expect)
> +
> +    # Fail if errors are found.
> +    if not is_ok:
> +        sys.exit(1)
> diff --git a/gcc/testsuite/lib/modules.exp b/gcc/testsuite/lib/modules.exp
> new file mode 100644
> index 00000000000..c7cfda6aae4
> --- /dev/null
> +++ b/gcc/testsuite/lib/modules.exp
> @@ -0,0 +1,71 @@
> +#   Copyright (C) 1997-2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with GCC; see the file COPYING3.  If not see
> +# <http://www.gnu.org/licenses/>.
> +
> +# Verify various kinds of gcov output: line counts, branch percentages,
> +# and call return percentages.  None of this is language-specific.
> +
> +load_lib "target-supports.exp"
> +
> +#
> +# clean-p1689-file -- delete a working file the compiler creates for p1689
> +#
> +# TESTCASE is the name of the test.
> +# SUFFIX is file suffix
> +
> +proc clean-p1689-file { testcase suffix } {
> +    set basename [file tail $testcase]
> +    set base [file rootname $basename]
> +    remote_file host delete $base.$suffix
> +}
> +
> +#
> +# clean-p1689 -- delete the working files the compiler creates for p1689
> +#
> +# TESTCASE is the name of the test.
> +#
> +proc clean-p1689 { testcase } {
> +    clean-p1689-file $testcase "d"
> +    clean-p1689-file $testcase "json"
> +}
> +
> +# Call by dg-final to check a P1689 dependency file
> +
> +proc run-check-p1689-valid { depfile template } {
> +    global srcdir subdir
> +    # Extract the test file name from the arguments.
> +    set testcase [file rootname [file tail $depfile]]
> +
> +    verbose "Running P1689 validation for $testcase in $srcdir/$subdir" 2
> +    set testcase [remote_download host $testcase]
> +
> +    set pytest_script "test-p1689.py"
> +    if { ![check_effective_target_recent_python3] } {
> +      unsupported "$pytest_script python3 is missing"
> +      return
> +    }
> +
> +    verbose "running script" 1
> +    spawn -noecho python3 $srcdir/$subdir/$pytest_script --all --actual $depfile --expect $srcdir/$subdir/$template
> +
> +    expect {
> +      -re "ERROR: (\[^\r\n\]*)" {
> +       fail $expect_out(0,string)
> +       exp_continue
> +      }
> +    }
> +
> +    clean-p1689 $testcase
> +}
> diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h
> index 8df071e1587..aa1c065104a 100644
> --- a/libcpp/include/cpplib.h
> +++ b/libcpp/include/cpplib.h
> @@ -302,6 +302,9 @@ typedef CPPCHAR_SIGNED_T cppchar_signed_t;
>   /* Style of header dependencies to generate.  */
>   enum cpp_deps_style { DEPS_NONE = 0, DEPS_USER, DEPS_SYSTEM };
>   
> +/* Format of header dependencies to generate.  */
> +enum cpp_deps_format { DEPS_FMT_NONE = 0, DEPS_FMT_P1689R5 };

 From the earlier review:

> > Why not add this to cpp_deps_style?
>
> It is orthogonal. The `-M` flags and `-fdeps-*` flags are similar, but
> `-fdeps-*` dependencies are fundamentally different than `-M`
> dependencies. The comment does need updated though.

Makes sense, but "cpp_deps_style" and "cpp_deps_format" don't sound 
orthogonal, they sound overlapping.  Maybe just call it 
"cpp_fdeps_format" to match all the other fdeps, and fix the comment; 
maybe just add "structured" here, too.

> `-M` is about discovered dependencies: those that you find out while
> doing work. `-fdep-*` is about ordering dependencies: extracting
> information from file content in order to even order future work around.

I'm not sure I see the distinction; Makefiles also express ordering 
dependencies.  In both cases, you want to find out from the files what 
order you will want to process them in when building the project.

For modules, there's currently a problem arising from the lack of either 
on-demand BMI generation (apart from Nathan's P1602 prototype) or 
fallback to consuming modules as source directly, so indeed it's 
currently important to support a separate discovery phase.  But that 
seems like a separate question from the output file format; the 
discovery phase could emit Makefile dependencies and not the p1689 
format, if that makes more sense for the individual project.

For some projects, I imagine the discovery commands could also be the 
same as the build commands, and you just need to keep trying until the 
build succeeds.  I wonder if there's a way to automatically retry if 
dependency files got updated...

>   /* The possible normalization levels, from most restrictive to least.  */
>   enum cpp_normalize_level {
>     /* In NFKC.  */
> @@ -589,6 +592,9 @@ struct cpp_options
>       /* Style of header dependencies to generate.  */
>       enum cpp_deps_style style;
>   
> +    /* Format of header dependencies to generate.  */
> +    enum cpp_deps_format format;

And this should probably be fdeps_format.

>       /* Assume missing files are generated files.  */
>       bool missing_files;
>   
> @@ -1112,9 +1118,9 @@ extern void cpp_post_options (cpp_reader *);
>   extern void cpp_init_iconv (cpp_reader *);
>   
>   /* Call this to finish preprocessing.  If you requested dependency
> -   generation, pass an open stream to write the information to,
> -   otherwise NULL.  It is your responsibility to close the stream.  */
> -extern void cpp_finish (cpp_reader *, FILE *deps_stream);
> +   generation, pass open stream(s) to write the information to,
> +   otherwise NULL.  It is your responsibility to close the stream(s).  */
> +extern void cpp_finish (cpp_reader *, FILE *deps_stream, FILE *fdeps_stream = NULL);
>   
>   /* Call this to release the handle at the end of preprocessing.  Any
>      use of the handle after this function returns is invalid.  */
> diff --git a/libcpp/include/mkdeps.h b/libcpp/include/mkdeps.h
> index 920e2791334..33c7437a481 100644
> --- a/libcpp/include/mkdeps.h
> +++ b/libcpp/include/mkdeps.h
> @@ -53,20 +53,29 @@ extern void deps_add_default_target (class mkdeps *, const char *);
>   
>   /* Adds a module target.  The module name and cmi name are copied.  */
>   extern void deps_add_module_target (struct mkdeps *, const char *module,
> -				    const char *cmi, bool is_header);
> +				    const char *cmi, bool is_header,
> +				    bool is_exported);
>   
>   /* Adds a module dependency.  The module name is copied.  */
>   extern void deps_add_module_dep (struct mkdeps *, const char *module);
>   
> +/* Add an output.  */
> +extern void deps_add_output (struct mkdeps *, const char *, bool);
> +
>   /* Add a dependency (appears on the right side of the colon) to the
>      deps list.  Dependencies will be printed in the order that they
>      were entered with this function.  By convention, the first
>      dependency entered should be the primary source file.  */
>   extern void deps_add_dep (class mkdeps *, const char *);
>   
> -/* Write out a deps buffer to a specified file.  The last argument
> -   is the number of columns to word-wrap at (0 means don't wrap).  */
> -extern void deps_write (const cpp_reader *, FILE *, unsigned int);
> +/* Write out a deps buffer to a specified file.  The third argument
> +   is the number of columns to word-wrap at (0 means don't wrap).
> +   The last argument indicates whether to output extra information
> +   (namely modules).  */
> +extern void deps_write (const struct cpp_reader *, FILE *, unsigned int);

Seems like this change isn't needed anymore.

> +/* Write out a deps buffer to a specified file in P1689R5 format.  */
> +extern void deps_write_p1689r5 (const struct mkdeps *, FILE *);
>   
>   /* Write out a deps buffer to a file, in a form that can be read back
>      with deps_restore.  Returns nonzero on error, in which case the
> diff --git a/libcpp/init.cc b/libcpp/init.cc
> index c508f06112a..d34fd6fdeef 100644
> --- a/libcpp/init.cc
> +++ b/libcpp/init.cc
> @@ -855,7 +855,7 @@ read_original_directory (cpp_reader *pfile)
>      Maybe it should also reset state, such that you could call
>      cpp_start_read with a new filename to restart processing.  */
>   void
> -cpp_finish (cpp_reader *pfile, FILE *deps_stream)
> +cpp_finish (struct cpp_reader *pfile, FILE *deps_stream, FILE *fdeps_stream)
>   {
>     /* Warn about unused macros before popping the final buffer.  */
>     if (CPP_OPTION (pfile, warn_unused_macros))
> @@ -869,8 +869,15 @@ cpp_finish (cpp_reader *pfile, FILE *deps_stream)
>     while (pfile->buffer)
>       _cpp_pop_buffer (pfile);
>   
> -  if (deps_stream)
> -    deps_write (pfile, deps_stream, 72);
> +  cpp_deps_format deps_format = CPP_OPTION (pfile, deps.format);
> +  if (deps_format == DEPS_FMT_P1689R5 && fdeps_stream)
> +    deps_write_p1689r5 (pfile->deps, fdeps_stream);
> +
> +  if (CPP_OPTION (pfile, deps.style) != DEPS_NONE
> +      && deps_stream)

Why does the Make handling need to change?  deps_stream should be null 
if we aren't emitting Make deps, right?

> +    {
> +      deps_write (pfile, deps_stream, 72);
> +    }
>   
>     /* Report on headers that could use multiple include guards.  */
>     if (CPP_OPTION (pfile, print_include_names))
> diff --git a/libcpp/mkdeps.cc b/libcpp/mkdeps.cc
> index 8f9585c3c0a..081287c94ba 100644
> --- a/libcpp/mkdeps.cc
> +++ b/libcpp/mkdeps.cc
> @@ -81,7 +81,8 @@ public:
>     };
>   
>     mkdeps ()
> -    : module_name (NULL), cmi_name (NULL), is_header_unit (false), quote_lwm (0)
> +    : primary_output (NULL), module_name (NULL), cmi_name (NULL)
> +    , is_header_unit (false), is_exported (false), quote_lwm (0)
>     {
>     }
>     ~mkdeps ()
> @@ -90,6 +91,9 @@ public:
>   
>       for (i = targets.size (); i--;)
>         free (const_cast <char *> (targets[i]));
> +    free (const_cast <char *> (primary_output));
> +    for (i = outputs.size (); i--;)
> +      free (const_cast <char *> (outputs[i]));
>       for (i = deps.size (); i--;)
>         free (const_cast <char *> (deps[i]));
>       for (i = vpath.size (); i--;)
> @@ -103,6 +107,8 @@ public:
>   public:
>     vec<const char *> targets;
>     vec<const char *> deps;
> +  const char * primary_output;
> +  vec<const char *> outputs;

fdeps_targets, I think

>     vec<velt> vpath;
>     vec<const char *> modules;
>   
> @@ -110,6 +116,7 @@ public:
>     const char *module_name;
>     const char *cmi_name;
>     bool is_header_unit;
> +  bool is_exported;
>     unsigned short quote_lwm;
>   };
>   
> @@ -288,6 +295,21 @@ deps_add_default_target (class mkdeps *d, const char *tgt)
>       }
>   }
>   
> +/* Adds an output O.  We make a copy, so it need not be a permanent
> +   string.  */
> +void
> +deps_add_output (struct mkdeps *d, const char *o, bool is_primary)

As mentioned above, I think this should be fdeps_add_target.  And please 
add more commentary about why this wouldn't just be the same as -MT.

> +{
> +  o = apply_vpath (d, o);
> +  if (is_primary)
> +  {
> +    if (d->primary_output)
> +      d->outputs.push (d->primary_output);
> +    d->primary_output = xstrdup (o);
> +  } else
> +    d->outputs.push (xstrdup (o));
> +}
> +
>   void
>   deps_add_dep (class mkdeps *d, const char *t)
>   {
> @@ -325,12 +347,13 @@ deps_add_vpath (class mkdeps *d, const char *vpath)
>   
>   void
>   deps_add_module_target (struct mkdeps *d, const char *m,
> -			const char *cmi, bool is_header_unit)
> +			const char *cmi, bool is_header_unit, bool is_exported)
>   {
>     gcc_assert (!d->module_name);
>     
>     d->module_name = xstrdup (m);
>     d->is_header_unit = is_header_unit;
> +  d->is_exported = is_exported;
>     d->cmi_name = xstrdup (cmi);
>   }
>   
> @@ -395,10 +418,15 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
>     if (colmax && colmax < 34)
>       colmax = 34;
>   
> +  /* Write out C++ modules information if no other `-fdeps-format=`
> +   * option is given. */

Stray *

> +  cpp_deps_format deps_format = CPP_OPTION (pfile, deps.format);
> +  bool write_make_modules_deps = deps_format == DEPS_FMT_NONE;

Maybe && CPP_OPTION (pfile, deps.modules)?

> +
>     if (d->deps.size ())
>       {
>         column = make_write_vec (d->targets, fp, 0, colmax, d->quote_lwm);
> -      if (CPP_OPTION (pfile, deps.modules) && d->cmi_name)
> +      if (write_make_modules_deps && CPP_OPTION (pfile, deps.modules) && d->cmi_name)

So you don't need to check it here.

>   	column = make_write_name (d->cmi_name, fp, column, colmax);
>         fputs (":", fp);
>         column++;
> @@ -412,7 +440,7 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
>     if (!CPP_OPTION (pfile, deps.modules))

And then this can be if (!write_make_modules_deps) and you don't need to 
check the flag in the rest of the ifs.

>       return;
>   
> -  if (d->modules.size ())
> +  if (write_make_modules_deps && d->modules.size ())
>       {
>         column = make_write_vec (d->targets, fp, 0, colmax, d->quote_lwm);
>         if (d->cmi_name)
> @@ -423,7 +451,7 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
>         fputs ("\n", fp);
>       }
>   
> -  if (d->module_name)
> +  if (write_make_modules_deps && d->module_name)
>       {
>         if (d->cmi_name)
>   	{
> @@ -455,7 +483,7 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
>   	}
>       }
>     
> -  if (d->modules.size ())
> +  if (write_make_modules_deps && d->modules.size ())
>       {
>         column = fprintf (fp, "CXX_IMPORTS +=");
>         make_write_vec (d->modules, fp, column, colmax, 0, ".c++m");
> @@ -468,11 +496,118 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
>   /* Really we should be opening fp here.  */
>   
>   void
> -deps_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
> +deps_write (const struct cpp_reader *pfile, FILE *fp, unsigned int colmax)

As with the prototype, this change seems unneeded.

>   {
>     make_write (pfile, fp, colmax);
>   }
>   
> +static void
> +p1689r5_write_filepath (const char *name, FILE *fp)
> +{
> +  if (_cpp_valid_utf8_str (name))
> +    {
> +      fputc ('"', fp);
> +      for (const char* c = name; *c; c++)
> +	{
> +	  // Escape control characters.
> +	  if (ISCNTRL (*c))
> +	    fprintf (fp, "\\u%04x", *c);
> +	  // JSON escape characters.
> +	  else if (*c == '"' || *c == '\\')
> +	    {
> +	      fputc ('\\', fp);
> +	      fputc (*c, fp);
> +	    }
> +	  // Everything else.
> +	  else
> +	    fputc (*c, fp);
> +	}
> +      fputc ('"', fp);
> +    }
> +  else
> +    {
> +      // TODO: print an error
> +    }
> +}
> +
> +static void
> +p1689r5_write_vec (const mkdeps::vec<const char *> &vec, FILE *fp)
> +{
> +  for (unsigned ix = 0; ix != vec.size (); ix++)
> +    {
> +      p1689r5_write_filepath (vec[ix], fp);
> +      if (ix < vec.size () - 1)
> +	fputc (',', fp);
> +      fputc ('\n', fp);
> +    }
> +}
> +
> +void
> +deps_write_p1689r5 (const struct mkdeps *d, FILE *fp)
> +{
> +  fputs ("{\n", fp);
> +
> +  fputs ("\"rules\": [\n", fp);
> +  fputs ("{\n", fp);
> +
> +  if (d->primary_output)
> +    {
> +      fputs ("\"primary-output\": ", fp);
> +      p1689r5_write_filepath (d->primary_output, fp);
> +      fputs (",\n", fp);
> +    }
> +
> +  if (d->outputs.size ())
> +    {
> +      fputs ("\"outputs\": [\n", fp);
> +      p1689r5_write_vec (d->outputs, fp);
> +      fputs ("],\n", fp);
> +    }
> +
> +  if (d->module_name)
> +    {
> +      fputs ("\"provides\": [\n", fp);
> +      fputs ("{\n", fp);
> +
> +      fputs ("\"logical-name\": ", fp);
> +      p1689r5_write_filepath (d->module_name, fp);
> +      fputs (",\n", fp);
> +
> +      fprintf (fp, "\"is-interface\": %s\n", d->is_exported ? "true" : "false");
> +
> +      // TODO: header-unit information
> +
> +      fputs ("}\n", fp);
> +      fputs ("],\n", fp);
> +    }
> +
> +  fputs ("\"requires\": [\n", fp);
> +  for (size_t i = 0; i < d->modules.size (); i++)
> +    {
> +      if (i != 0)
> +	fputs (",\n", fp);
> +      fputs ("{\n", fp);
> +
> +      fputs ("\"logical-name\": ", fp);
> +      p1689r5_write_filepath (d->modules[i], fp);
> +      fputs ("\n", fp);
> +
> +      // TODO: header-unit information
> +
> +      fputs ("}\n", fp);
> +    }
> +  fputs ("]\n", fp);
> +
> +  fputs ("}\n", fp);
> +
> +  fputs ("],\n", fp);
> +
> +  fputs ("\"version\": 0,\n", fp);
> +  fputs ("\"revision\": 0\n", fp);
> +
> +  fputs ("}\n", fp);
> +}

Is there a reason not to use the gcc/json.h interface for JSON output?

Jason

Re: [PATCH v5 3/5] p1689r5: initial support

[Ben Boeckel]

On Tue, Feb 14, 2023 at 16:50:27 -0500, Jason Merrill wrote:
> I notice that the actual flags are all -fdep-*, though some of them are 
> -fdeps-* here, and the internal variables all seem to be fdeps_*.  I 
> lean toward harmonizing on "deps", I think.

Done.

> I don't love the three separate options, but I suppose it's fine.  I'd 
> prefer "target" instead of "output".

Done.

> It should be possible to omit both -file and -target and get reasonable 
> defaults, like the ones for -MD/-MQ in gcc.cc:cpp_unique_options.

`file` can be omitted (the `output_stream` will be used then). I *think*
I see that adding:

    %{fdeps_file:-fdeps-file=%{!o:%b.ddi}%{o*:%.ddi%*}}

would at least do for `-fdeps-file` defaults? I don't know if there's a
reasonable default for `-fdeps-target=` though given that this command
line has no information about the object file that will be used (`-o` is
used for preprocessor output since we're leaning on `-E` here).

--Ben

Re: [PATCH v5 3/5] p1689r5: initial support

[Jason Merrill]

On 5/12/23 10:24, Ben Boeckel wrote:
> On Tue, Feb 14, 2023 at 16:50:27 -0500, Jason Merrill wrote:
>> I notice that the actual flags are all -fdep-*, though some of them are
>> -fdeps-* here, and the internal variables all seem to be fdeps_*.  I
>> lean toward harmonizing on "deps", I think.
> 
> Done.
> 
>> I don't love the three separate options, but I suppose it's fine.  I'd
>> prefer "target" instead of "output".
> 
> Done.
> 
>> It should be possible to omit both -file and -target and get reasonable
>> defaults, like the ones for -MD/-MQ in gcc.cc:cpp_unique_options.
> 
> `file` can be omitted (the `output_stream` will be used then). I *think*
> I see that adding:
> 
>      %{fdeps_file:-fdeps-file=%{!o:%b.ddi}%{o*:%.ddi%*}}

%{!fdeps-file: but yes.

> would at least do for `-fdeps-file` defaults? I don't know if there's a
> reasonable default for `-fdeps-target=` though given that this command
> line has no information about the object file that will be used (`-o` is
> used for preprocessor output since we're leaning on `-E` here).

I would think it could default to %b.o?

I had quite a few more comments on the v5 patch that you didn't respond 
to here or address in the v6 patch; did your mail client hide them from you?

Jason

Re: [PATCH v5 3/5] p1689r5: initial support

[Ben Boeckel]

On Mon, Jun 19, 2023 at 17:33:58 -0400, Jason Merrill wrote:
> On 5/12/23 10:24, Ben Boeckel wrote:
> > `file` can be omitted (the `output_stream` will be used then). I *think*
> > I see that adding:
> > 
> >      %{fdeps_file:-fdeps-file=%{!o:%b.ddi}%{o*:%.ddi%*}}
> 
> %{!fdeps-file: but yes.
> 
> > would at least do for `-fdeps-file` defaults? I don't know if there's a
> > reasonable default for `-fdeps-target=` though given that this command
> > line has no information about the object file that will be used (`-o` is
> > used for preprocessor output since we're leaning on `-E` here).
> 
> I would think it could default to %b.o?

I suppose that could work, yes.

> I had quite a few more comments on the v5 patch that you didn't respond 
> to here or address in the v6 patch; did your mail client hide them from you?

Oof. Sorry, I saw large chunks of quoting and apparently assumed the
rest was fine (I usually do aggressive trimming when doing that style of
review). I see them now. Will go through and include in v7.

--Ben

Re: [PATCH v5 3/5] p1689r5: initial support

[Ben Boeckel]

On Tue, Feb 14, 2023 at 16:50:27 -0500, Jason Merrill wrote:
> On 1/25/23 13:06, Ben Boeckel wrote:
> > - header-unit information fields
> > 
> > Header units (including the standard library headers) are 100%
> > unsupported right now because the `-E` mechanism wants to import their
> > BMIs. A new mode (i.e., something more workable than existing `-E`
> > behavior) that mocks up header units as if they were imported purely
> > from their path and content would be required.
> 
> I notice that the cpp dependency generation tries (in open_file_failed) 
> to continue after encountering a missing file, is that not sufficient 
> for header units?  Or adjustable to be sufficient?

No. Header units can introduce macros which can be used to modify the
set of modules that are imported. Included headers are "discovered"
dependencies and don't modify the build graph (just add more files that
trigger a rebuild) and can be collected during compilation. Module
dependencies are needed to get the build correct in the first place in
order to:

- order module compilations in the build graph so that imported modules
  are ready before anything using them; and
- computing the set of flags needed for telling the compiler where
  imported modules' CMI files should be located.

> > - non-utf8 paths
> > 
> > The current standard says that paths that are not unambiguously
> > represented using UTF-8 are not supported (because these cases are rare
> > and the extra complication is not worth it at this time). Future
> > versions of the format might have ways of encoding non-UTF-8 paths. For
> > now, this patch just doesn't support non-UTF-8 paths (ignoring the
> > "unambiguously represetable in UTF-8" case).
> 
> typo "representable"

Fixed.

> > diff --git a/gcc/c-family/c-opts.cc b/gcc/c-family/c-opts.cc
> > index c68a2a27469..1c14ce3fe8e 100644
> > --- a/gcc/c-family/c-opts.cc
> > +++ b/gcc/c-family/c-opts.cc
> > @@ -77,6 +77,9 @@ static bool verbose;
> >   /* Dependency output file.  */
> >   static const char *deps_file;
> >   
> > +/* Enhanced dependency output file.  */
> 
> Maybe "structured", as in the docs?  It isn't really a direct 
> enhancement of the makefile dependencies.

Agreed. I'll also add a link to p1689r5 as a comment for what
"structured" means where it is parsed out.

> > +  if (cpp_opts->deps.format != DEPS_FMT_NONE)
> > +    {
> > +      if (!fdeps_file)
> > +	fdeps_stream = out_stream;
> > +      else if (fdeps_file[0] == '-' && fdeps_file[1] == '\0')
> > +	fdeps_stream = stdout;
> 
> You probably want to check that deps_stream and fdeps_stream don't end 
> up as the same stream.

Hmm. But `stdout` is probably fine to use for both though. Basically:

    if (fdeps_stream == out_stream && fdeps_stream != stdout)
      make_diagnostic_noise ();

> > @@ -1374,6 +1410,8 @@ handle_deferred_opts (void)
> >   
> >   	if (opt->code == OPT_MT || opt->code == OPT_MQ)
> >   	  deps_add_target (deps, opt->arg, opt->code == OPT_MQ);
> > +	else if (opt->code == OPT_fdep_output_)
> > +	  deps_add_output (deps, opt->arg, true);
> 
> How about fdeps_add_target?

Renamed.

> > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> > index ef371ca8c26..630781fdf8a 100644
> > --- a/gcc/c-family/c.opt
> > +++ b/gcc/c-family/c.opt
> > @@ -256,6 +256,18 @@ MT
> >   C ObjC C++ ObjC++ Joined Separate MissingArgError(missing makefile target after %qs)
> >   -MT <target>	Add a target that does not require quoting.
> >   
> > +fdep-format=
> > +C ObjC C++ ObjC++ NoDriverArg Joined MissingArgError(missing format after %qs)
> > +Format for output dependency information.  Supported (\"p1689r5\").
> 
> I think we want "structured" here, as well.

Fixed.

> > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > index 06d77983e30..b61c3ebd3ec 100644
> > --- a/gcc/doc/invoke.texi
> > +++ b/gcc/doc/invoke.texi
> > @@ -2791,6 +2791,21 @@ is @option{-fpermitted-flt-eval-methods=c11}.  The default when in a GNU
> >   dialect (@option{-std=gnu11} or similar) is
> >   @option{-fpermitted-flt-eval-methods=ts-18661-3}.
> >   
> > +@item -fdep-file=@var{file}
> > +@opindex fdep-file
> > +Where to write structured dependency information.
> > +
> > +@item -fdep-format=@var{format}
> > +@opindex fdep-format
> > +The format to use for structured dependency information. @samp{p1689r5} is the
> > +only supported format right now.  Note that when this argument is specified, the
> > +output of @samp{-MF} is stripped of some information (namely C++ modules) so
> > +that it does not use extended makefile syntax not understood by most tools.
> > +
> > +@item -fdep-output=@var{file}
> > +@opindex fdep-output
> > +Analogous to @option{-MT} but for structured dependency information.
> 
> Please add more detail about how these are intended to be used.

Will do.

> > diff --git a/gcc/testsuite/g++.dg/modules/p1689-1.C b/gcc/testsuite/g++.dg/modules/p1689-1.C
> > new file mode 100644
> > index 00000000000..245e30d09ce
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/p1689-1.C
> > @@ -0,0 +1,18 @@
> > +// { dg-additional-options -E }
> > +// { dg-additional-options -MT }
> > +// { dg-additional-options p1689-1.json }
> > +// { dg-additional-options -MD }
> > +// { dg-additional-options -fmodules-ts }
> > +// { dg-additional-options -fdep-format=p1689r5 }
> > +// { dg-additional-options -fdep-output=p1689-1.o }
> > +// { dg-additional-options -fdep-file=p1689-1.json }
> 
> Note that with dg-additional-options you can put multiple flags in "" 
> rather than put each on its own line.

Ah, that would indeed help.

> So, I take it this is the common use case you have in mind, generating 
> Make dependencies for the p1689 file?  When are you thinking the Make 
> dependencies for the .o are generated?  At build time?

Yes. If an included file changes, the scanning should be performed
again. The compilation will have its own `-MF` as well (which should
point to the same files plus the CMI files it ends up reading).

> I'm a bit surprised you're using .json rather than an extension that 
> indicates what the information is.

I can change that; the filename doesn't *really* matter (e.g., CMake
uses `.ddi` for "dynamic dependency information").

> > diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h
> > index 8df071e1587..aa1c065104a 100644
> > --- a/libcpp/include/cpplib.h
> > +++ b/libcpp/include/cpplib.h
> > @@ -302,6 +302,9 @@ typedef CPPCHAR_SIGNED_T cppchar_signed_t;
> >   /* Style of header dependencies to generate.  */
> >   enum cpp_deps_style { DEPS_NONE = 0, DEPS_USER, DEPS_SYSTEM };
> >   
> > +/* Format of header dependencies to generate.  */
> > +enum cpp_deps_format { DEPS_FMT_NONE = 0, DEPS_FMT_P1689R5 };
> 
>  From the earlier review:
> 
> > > Why not add this to cpp_deps_style?
> >
> > It is orthogonal. The `-M` flags and `-fdeps-*` flags are similar, but
> > `-fdeps-*` dependencies are fundamentally different than `-M`
> > dependencies. The comment does need updated though.
> 
> Makes sense, but "cpp_deps_style" and "cpp_deps_format" don't sound 
> orthogonal, they sound overlapping.  Maybe just call it 
> "cpp_fdeps_format" to match all the other fdeps, and fix the comment; 
> maybe just add "structured" here, too.

Also needs to mention that it is for module dependencies, not header
dependencies.

> > `-M` is about discovered dependencies: those that you find out while
> > doing work. `-fdep-*` is about ordering dependencies: extracting
> > information from file content in order to even order future work around.
> 
> I'm not sure I see the distinction; Makefiles also express ordering 
> dependencies.  In both cases, you want to find out from the files what 
> order you will want to process them in when building the project.

Makefiles can express ordering dependencies, but not the `-M` snippets;
these are for files that, if changed, should trigger a rebuild. This is
fundamentally different than module dependencies which instead indicate
which *compiles* (or CMI generation if using a 2-phase setup) need to
complete before compilation (or CMI generation) of the scanned TU can be
performed. Generally generated headers will be ordered manually in the
build system description. However, maintaining that same level for
in-source dependency information on a per-source level is a *far* higher
burden.

> For modules, there's currently a problem arising from the lack of either 
> on-demand BMI generation (apart from Nathan's P1602 prototype) or 
> fallback to consuming modules as source directly, so indeed it's 
> currently important to support a separate discovery phase.  But that 
> seems like a separate question from the output file format; the 
> discovery phase could emit Makefile dependencies and not the p1689 
> format, if that makes more sense for the individual project.

I don't know how to specify header unit dependencies in Makefile syntax
meaningfully. For example, how does one specify that a header named
`<foo.h>`, found at `/full/path/to/foo.h` is required using this syntax?
Note that one or the other is not suitable as importing `<to/foo.h>` may
find the same file.

Not to mention that there are problems with some paths not being
representable (or needing lots of escaping rules to be considered), and
encoding specifications in Makefiles.

In any case, if something does pop up and *really* prefers Makefile
syntax over JSON, `-fdeps-format=make` could be made. This is all
internal build system stuff, so if anything does start caring, I suspect
it will be `automake` (though one would also need to communicate with
other compilers if that's really wanted…which are all likely to support
P1689 already).

> For some projects, I imagine the discovery commands could also be the 
> same as the build commands, and you just need to keep trying until the 
> build succeeds.  I wonder if there's a way to automatically retry if 
> dependency files got updated...

This is the socket or fifo option for the `-fmodule-mapper=` flag. The
problem is that there is an unbounded number of compilations that can be
suspended while the build tool "searches" for one that claims to supply
a module being requested. If it does not actually exist (or is "hidden"
behind some circular dependency), all available compilation rules must
be attempted before saying "nope, doesn't exist". I don't think it's
really scalable myself.

> > @@ -589,6 +592,9 @@ struct cpp_options
> >       /* Style of header dependencies to generate.  */
> >       enum cpp_deps_style style;
> >   
> > +    /* Format of header dependencies to generate.  */
> > +    enum cpp_deps_format format;
> 
> And this should probably be fdeps_format.

Done

> > diff --git a/libcpp/include/mkdeps.h b/libcpp/include/mkdeps.h
> > index 920e2791334..33c7437a481 100644
> > --- a/libcpp/include/mkdeps.h
> > +++ b/libcpp/include/mkdeps.h
> > @@ -53,20 +53,29 @@ extern void deps_add_default_target (class mkdeps *, const char *);
> >   
> >   /* Adds a module target.  The module name and cmi name are copied.  */
> >   extern void deps_add_module_target (struct mkdeps *, const char *module,
> > -				    const char *cmi, bool is_header);
> > +				    const char *cmi, bool is_header,
> > +				    bool is_exported);
> >   
> >   /* Adds a module dependency.  The module name is copied.  */
> >   extern void deps_add_module_dep (struct mkdeps *, const char *module);
> >   
> > +/* Add an output.  */
> > +extern void deps_add_output (struct mkdeps *, const char *, bool);
> > +
> >   /* Add a dependency (appears on the right side of the colon) to the
> >      deps list.  Dependencies will be printed in the order that they
> >      were entered with this function.  By convention, the first
> >      dependency entered should be the primary source file.  */
> >   extern void deps_add_dep (class mkdeps *, const char *);
> >   
> > -/* Write out a deps buffer to a specified file.  The last argument
> > -   is the number of columns to word-wrap at (0 means don't wrap).  */
> > -extern void deps_write (const cpp_reader *, FILE *, unsigned int);
> > +/* Write out a deps buffer to a specified file.  The third argument
> > +   is the number of columns to word-wrap at (0 means don't wrap).
> > +   The last argument indicates whether to output extra information
> > +   (namely modules).  */
> > +extern void deps_write (const struct cpp_reader *, FILE *, unsigned int);
> 
> Seems like this change isn't needed anymore.

Indeed.

> > +/* Write out a deps buffer to a specified file in P1689R5 format.  */
> > +extern void deps_write_p1689r5 (const struct mkdeps *, FILE *);
> >   
> >   /* Write out a deps buffer to a file, in a form that can be read back
> >      with deps_restore.  Returns nonzero on error, in which case the
> > diff --git a/libcpp/init.cc b/libcpp/init.cc
> > index c508f06112a..d34fd6fdeef 100644
> > --- a/libcpp/init.cc
> > +++ b/libcpp/init.cc
> > @@ -855,7 +855,7 @@ read_original_directory (cpp_reader *pfile)
> >      Maybe it should also reset state, such that you could call
> >      cpp_start_read with a new filename to restart processing.  */
> >   void
> > -cpp_finish (cpp_reader *pfile, FILE *deps_stream)
> > +cpp_finish (struct cpp_reader *pfile, FILE *deps_stream, FILE *fdeps_stream)
> >   {
> >     /* Warn about unused macros before popping the final buffer.  */
> >     if (CPP_OPTION (pfile, warn_unused_macros))
> > @@ -869,8 +869,15 @@ cpp_finish (cpp_reader *pfile, FILE *deps_stream)
> >     while (pfile->buffer)
> >       _cpp_pop_buffer (pfile);
> >   
> > -  if (deps_stream)
> > -    deps_write (pfile, deps_stream, 72);
> > +  cpp_deps_format deps_format = CPP_OPTION (pfile, deps.format);
> > +  if (deps_format == DEPS_FMT_P1689R5 && fdeps_stream)
> > +    deps_write_p1689r5 (pfile->deps, fdeps_stream);
> > +
> > +  if (CPP_OPTION (pfile, deps.style) != DEPS_NONE
> > +      && deps_stream)
> 
> Why does the Make handling need to change?  deps_stream should be null 
> if we aren't emitting Make deps, right?

Yes. This might be leftovers from the first time I did it as a `-M` flag
(before I realized that `-M` as-is is still useful even for the
scanning).

> > diff --git a/libcpp/mkdeps.cc b/libcpp/mkdeps.cc
> > index 8f9585c3c0a..081287c94ba 100644
> > --- a/libcpp/mkdeps.cc
> > +++ b/libcpp/mkdeps.cc
> > @@ -103,6 +107,8 @@ public:
> >   public:
> >     vec<const char *> targets;
> >     vec<const char *> deps;
> > +  const char * primary_output;
> > +  vec<const char *> outputs;
> 
> fdeps_targets, I think

Done

> > @@ -288,6 +295,21 @@ deps_add_default_target (class mkdeps *d, const char *tgt)
> >       }
> >   }
> >   
> > +/* Adds an output O.  We make a copy, so it need not be a permanent
> > +   string.  */
> > +void
> > +deps_add_output (struct mkdeps *d, const char *o, bool is_primary)
> 
> As mentioned above, I think this should be fdeps_add_target.  And please 
> add more commentary about why this wouldn't just be the same as -MT.

Done.

> > @@ -395,10 +418,15 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
> >     if (colmax && colmax < 34)
> >       colmax = 34;
> >   
> > +  /* Write out C++ modules information if no other `-fdeps-format=`
> > +   * option is given. */
> 
> Stray *

Done.

> > +  cpp_deps_format deps_format = CPP_OPTION (pfile, deps.format);
> > +  bool write_make_modules_deps = deps_format == DEPS_FMT_NONE;
> 
> Maybe && CPP_OPTION (pfile, deps.modules)?
> 
> > +
> >     if (d->deps.size ())
> >       {
> >         column = make_write_vec (d->targets, fp, 0, colmax, d->quote_lwm);
> > -      if (CPP_OPTION (pfile, deps.modules) && d->cmi_name)
> > +      if (write_make_modules_deps && CPP_OPTION (pfile, deps.modules) && d->cmi_name)
> 
> So you don't need to check it here.

Done.

> > @@ -412,7 +440,7 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
> >     if (!CPP_OPTION (pfile, deps.modules))
> 
> And then this can be if (!write_make_modules_deps) and you don't need to 
> check the flag in the rest of the ifs.

Indeed. Done.

> > @@ -468,11 +496,118 @@ make_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
> >   /* Really we should be opening fp here.  */
> >   
> >   void
> > -deps_write (const cpp_reader *pfile, FILE *fp, unsigned int colmax)
> > +deps_write (const struct cpp_reader *pfile, FILE *fp, unsigned int colmax)
> 
> As with the prototype, this change seems unneeded.

Done.

> > <snip JSON output diff>
> 
> Is there a reason not to use the gcc/json.h interface for JSON output?

This is `libcpp`; is that not a dependency cycle?

--Ben

Re: [PATCH v5 3/5] p1689r5: initial support

[Jason Merrill]

On 6/20/23 15:46, Ben Boeckel wrote:
> On Tue, Feb 14, 2023 at 16:50:27 -0500, Jason Merrill wrote:
>> On 1/25/23 13:06, Ben Boeckel wrote:

>>> Header units (including the standard library headers) are 100%
>>> unsupported right now because the `-E` mechanism wants to import their
>>> BMIs. A new mode (i.e., something more workable than existing `-E`
>>> behavior) that mocks up header units as if they were imported purely
>>> from their path and content would be required.
>> >> I notice that the cpp dependency generation tries (in open_file_failed)
>> to continue after encountering a missing file, is that not sufficient 
>> for header units?  Or adjustable to be sufficient?
> 
> No. Header units can introduce macros which can be used to modify the
> set of modules that are imported. Included headers are "discovered"
> dependencies and don't modify the build graph (just add more files that
> trigger a rebuild) and can be collected during compilation. Module
> dependencies are needed to get the build correct in the first place in
> order to:
> 
> - order module compilations in the build graph so that imported modules
>   are ready before anything using them; and
> - computing the set of flags needed for telling the compiler where
>   imported modules' CMI files should be located.

So if the header unit CMI isn't available during dependency generation, 
would it be better to just #include the header?

>>> +  if (cpp_opts->deps.format != DEPS_FMT_NONE)
>>> +    {
>>> +      if (!fdeps_file)
>>> +	fdeps_stream = out_stream;
>>> +      else if (fdeps_file[0] == '-' && fdeps_file[1] == '\0')
>>> +	fdeps_stream = stdout;
>>
>> You probably want to check that deps_stream and fdeps_stream don't end
>> up as the same stream.
> 
> Hmm. But `stdout` is probably fine to use for both though. Basically:
> 
>      if (fdeps_stream == out_stream && fdeps_stream != stdout)
>        make_diagnostic_noise ();

(fdeps_stream == deps_stream, but sure, that's reasonable.

>> So, I take it this is the common use case you have in mind, generating
>> Make dependencies for the p1689 file?  When are you thinking the Make
>> dependencies for the .o are generated?  At build time?
> 
> Yes. If an included file changes, the scanning should be performed
> again. The compilation will have its own `-MF` as well (which should
> point to the same files plus the CMI files it ends up reading).
> 
>> I'm a bit surprised you're using .json rather than an extension that
>> indicates what the information is.
> 
> I can change that; the filename doesn't *really* matter (e.g., CMake
> uses `.ddi` for "dynamic dependency information").

That works.

>>> `-M` is about discovered dependencies: those that you find out while
>>> doing work. `-fdep-*` is about ordering dependencies: extracting
>>> information from file content in order to even order future work around.
>>
>> I'm not sure I see the distinction; Makefiles also express ordering
>> dependencies.  In both cases, you want to find out from the files what
>> order you will want to process them in when building the project.
> 
> Makefiles can express ordering dependencies, but not the `-M` snippets;
> these are for files that, if changed, should trigger a rebuild. This is > fundamentally different than module dependencies which instead indicate
> which *compiles* (or CMI generation if using a 2-phase setup) need to
> complete before compilation (or CMI generation) of the scanned TU can be
> performed. Generally generated headers will be ordered manually in the
> build system description. However, maintaining that same level for
> in-source dependency information on a per-source level is a *far* higher
> burden.

The main difference I see is that the CMI might not exist yet.  As you 
say, we don't want to require people to write all the dependencies by 
hand, but that just means we need to be able to generate the 
dependencies automatically.  In the Make-only model I'm thinking of, one 
would collect dependencies on an initial failing build, and then start 
over from the beginning again with the dependencies we discovered.  It's 
the same two-phase scan and build, but one that uses the same compile 
commands for both phases.

Anyway, this isn't an objection to this patch, just another model I also 
want to support.

>>> <snip JSON output diff>
>>
>> Is there a reason not to use the gcc/json.h interface for JSON output?
> 
> This is `libcpp`; is that not a dependency cycle?

Ah, indeed.  We could move it to libiberty, but it would need 
significant adjustments to remove its dependencies on other stuff in 
gcc/.  So maybe just add a TODO comment about that, along with adding 
comments before the functions.

Jason

Re: [PATCH v5 3/5] p1689r5: initial support

[Ben Boeckel]

On Fri, Jun 23, 2023 at 14:31:17 -0400, Jason Merrill wrote:
> On 6/20/23 15:46, Ben Boeckel wrote:
> > On Tue, Feb 14, 2023 at 16:50:27 -0500, Jason Merrill wrote:
> >> On 1/25/23 13:06, Ben Boeckel wrote:
> 
> >>> Header units (including the standard library headers) are 100%
> >>> unsupported right now because the `-E` mechanism wants to import their
> >>> BMIs. A new mode (i.e., something more workable than existing `-E`
> >>> behavior) that mocks up header units as if they were imported purely
> >>> from their path and content would be required.
> >> >> I notice that the cpp dependency generation tries (in open_file_failed)
> >> to continue after encountering a missing file, is that not sufficient 
> >> for header units?  Or adjustable to be sufficient?
> > 
> > No. Header units can introduce macros which can be used to modify the
> > set of modules that are imported. Included headers are "discovered"
> > dependencies and don't modify the build graph (just add more files that
> > trigger a rebuild) and can be collected during compilation. Module
> > dependencies are needed to get the build correct in the first place in
> > order to:
> > 
> > - order module compilations in the build graph so that imported modules
> >   are ready before anything using them; and
> > - computing the set of flags needed for telling the compiler where
> >   imported modules' CMI files should be located.
> 
> So if the header unit CMI isn't available during dependency generation, 
> would it be better to just #include the header?

It's not so simple: the preprocessor state needs to isolate out
`LOCAL_ONLY` from this case:

```
#define LOCAL_ONLY 1
import <some/header.h>; // The preprocessing of this should *not* see
                        // `LOCAL_ONLY`.
```

> > Hmm. But `stdout` is probably fine to use for both though. Basically:
> > 
> >      if (fdeps_stream == out_stream && fdeps_stream != stdout)
> >        make_diagnostic_noise ();
> 
> (fdeps_stream == deps_stream, but sure, that's reasonable.

Done.

> >> So, I take it this is the common use case you have in mind, generating
> >> Make dependencies for the p1689 file?  When are you thinking the Make
> >> dependencies for the .o are generated?  At build time?
> > 
> > Yes. If an included file changes, the scanning should be performed
> > again. The compilation will have its own `-MF` as well (which should
> > point to the same files plus the CMI files it ends up reading).
> > 
> >> I'm a bit surprised you're using .json rather than an extension that
> >> indicates what the information is.
> > 
> > I can change that; the filename doesn't *really* matter (e.g., CMake
> > uses `.ddi` for "dynamic dependency information").
> 
> That works.

Done.

> >>> `-M` is about discovered dependencies: those that you find out while
> >>> doing work. `-fdep-*` is about ordering dependencies: extracting
> >>> information from file content in order to even order future work around.
> >>
> >> I'm not sure I see the distinction; Makefiles also express ordering
> >> dependencies.  In both cases, you want to find out from the files what
> >> order you will want to process them in when building the project.
> > 
> > Makefiles can express ordering dependencies, but not the `-M` snippets;
> > these are for files that, if changed, should trigger a rebuild. This is > fundamentally different than module dependencies which instead indicate
> > which *compiles* (or CMI generation if using a 2-phase setup) need to
> > complete before compilation (or CMI generation) of the scanned TU can be
> > performed. Generally generated headers will be ordered manually in the
> > build system description. However, maintaining that same level for
> > in-source dependency information on a per-source level is a *far* higher
> > burden.
> 
> The main difference I see is that the CMI might not exist yet.  As you 
> say, we don't want to require people to write all the dependencies by 
> hand, but that just means we need to be able to generate the 
> dependencies automatically.  In the Make-only model I'm thinking of, one 
> would collect dependencies on an initial failing build, and then start 
> over from the beginning again with the dependencies we discovered.  It's 
> the same two-phase scan and build, but one that uses the same compile 
> commands for both phases.

It's a potentially unbounded set of phases:

- 2 phases per tool that is built that generates other module-using
  code for other tools:

    - scan files for toolA
    - build files for toolA
    - scan files written by toolA (for toolB)
    - build files written by toolA (for toolB)
    - …

- if a referenced module does not exist, knowing when one is "done" is
  difficult (an import cycle would appear like this because while module
  X does exist, it depends on Y which itself claims a dependency on X).

*Something* needs to interpret the information being provided in the
`-fdeps-file=` and communicate back to the build tool what is going on.
This requires:

- some coordination of where each TU will store these files (e.g., CMake
  has a per-target directory for storing such things and the collator
  knows which TUs belong to which targets to tell everything about
  them); and
- some kind of coordination for the target-level dependency graph (i.e.,
  if libA depends-on only libB, a TU from libA using a module "from"
  libB is ok, but using one "from" libC is bad and the same if libB uses
  a module "from" libA).

Therefore any automation will need to have an idea of the overall build
graph and an understanding of "yes, module X exists, but this TU is not
allowed to use it because the graph doesn't allow that edge". I don't
think a "drop in" tool to any arbitrary `Makefile`-using project can
exist as there's just not enough structure in a `Makefile` to really
require such information. `automake` can provide one, however, because
(AFAIK) it *does* have an idea about these kinds of things (though it
may need to be more structured than it is today).

The `libcody` GNU Make support patch doesn't have this structured
information and (AFAIK) will happily let any TU import any module from
any other TU even if the linker is going to be unhappy later. Again, I
think we can give users (and they deserve) better error messages than
"<mangled module initializer symbol> not found".

> Anyway, this isn't an objection to this patch, just another model I also 
> want to support.

The old Intel Fortran "how to build with modules" docs used to have a
"run `make` until it works" instruction step. I think we can do better
these days (yes, it means that pure-`make` is far harder than it used to
be).

> >>> <snip JSON output diff>
> >>
> >> Is there a reason not to use the gcc/json.h interface for JSON output?
> > 
> > This is `libcpp`; is that not a dependency cycle?
> 
> Ah, indeed.  We could move it to libiberty, but it would need 
> significant adjustments to remove its dependencies on other stuff in 
> gcc/.  So maybe just add a TODO comment about that, along with adding 
> comments before the functions.

Done.

--Ben

[PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Ben Boeckel]

They affect the build, so report them via `-MF` mechanisms.

gcc/cp/

	* module.cc (do_import): Report imported CMI files as
	dependencies.

Signed-off-by: Ben Boeckel <ben.boeckel@kitware.com>
---
 gcc/cp/module.cc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index ebd30f63d81..dbd1b721616 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -18966,6 +18966,8 @@ module_state::do_import (cpp_reader *reader, bool outermost)
       dump () && dump ("CMI is %s", file);
       if (note_module_cmi_yes || inform_cmi_p)
 	inform (loc, "reading CMI %qs", file);
+      /* Add the CMI file to the dependency tracking. */
+      deps_add_dep (cpp_get_deps (reader), file);
       fd = open (file, O_RDONLY | O_CLOEXEC | O_BINARY);
       e = errno;
     }
-- 
2.39.0

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Jason Merrill]

On 1/25/23 13:06, Ben Boeckel wrote:
> They affect the build, so report them via `-MF` mechanisms.
> 
> gcc/cp/
> 
> 	* module.cc (do_import): Report imported CMI files as
> 	dependencies.

Both this and the mapper dependency patch seem to cause most of the 
modules testcases to crash; please remember to run the regression tests 
(https://gcc.gnu.org/contribute.html#testing)

> Signed-off-by: Ben Boeckel <ben.boeckel@kitware.com>
> ---
>   gcc/cp/module.cc | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index ebd30f63d81..dbd1b721616 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -18966,6 +18966,8 @@ module_state::do_import (cpp_reader *reader, bool outermost)
>         dump () && dump ("CMI is %s", file);
>         if (note_module_cmi_yes || inform_cmi_p)
>   	inform (loc, "reading CMI %qs", file);
> +      /* Add the CMI file to the dependency tracking. */
> +      deps_add_dep (cpp_get_deps (reader), file);
>         fd = open (file, O_RDONLY | O_CLOEXEC | O_BINARY);
>         e = errno;
>       }

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Ben Boeckel]

On Mon, Feb 13, 2023 at 13:33:50 -0500, Jason Merrill wrote:
> Both this and the mapper dependency patch seem to cause most of the 
> modules testcases to crash; please remember to run the regression tests 
> (https://gcc.gnu.org/contribute.html#testing)

Fixed for v6. `cpp_get_deps` can return `NULL` which `deps_add_dep`
assumes to not be true; fixed by checking before calling.

--Ben

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Jason Merrill]

On 1/25/23 16:06, Ben Boeckel wrote:
> They affect the build, so report them via `-MF` mechanisms.

Why isn't this covered by the existing code in preprocessed_module?

> gcc/cp/
> 
> 	* module.cc (do_import): Report imported CMI files as
> 	dependencies.
> 
> Signed-off-by: Ben Boeckel <ben.boeckel@kitware.com>
> ---
>   gcc/cp/module.cc | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index ebd30f63d81..dbd1b721616 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -18966,6 +18966,8 @@ module_state::do_import (cpp_reader *reader, bool outermost)
>         dump () && dump ("CMI is %s", file);
>         if (note_module_cmi_yes || inform_cmi_p)
>   	inform (loc, "reading CMI %qs", file);
> +      /* Add the CMI file to the dependency tracking. */
> +      deps_add_dep (cpp_get_deps (reader), file);
>         fd = open (file, O_RDONLY | O_CLOEXEC | O_BINARY);
>         e = errno;
>       }

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Ben Boeckel]

On Thu, Jun 22, 2023 at 17:21:42 -0400, Jason Merrill wrote:
> On 1/25/23 16:06, Ben Boeckel wrote:
> > They affect the build, so report them via `-MF` mechanisms.
> 
> Why isn't this covered by the existing code in preprocessed_module?

It appears as though it is neutered in patch 3 where
`write_make_modules_deps` is used in `make_write` (or will use that name
in v7 once I finish up testing). This logic cannot be used for p1689
output because it assumes the location and names of CMI files (`.c++m`)
that will be necessary (it is related to the `CXX_IMPORTS +=` GNU
make/libcody extensions that will, e.g., cause `ninja` to choke if it is
read from `-MF` output as it uses "fancier" Makefile syntax than tools
that are not actually `make` are going to be willing to support). This
codepath is the *actual* filename being read at compile time and is
relevant at all times; it may duplicate what `preprocessed_module` sets
up.

I'm also realizing that this is why I need to pass `-fdeps-format=p1689`
when compiling…there may need to be another, more idiomatic, way to
disable this additional syntax usage in `-MF` output.

--Ben

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Nathan Sidwell]

On 6/22/23 22:45, Ben Boeckel wrote:
> On Thu, Jun 22, 2023 at 17:21:42 -0400, Jason Merrill wrote:
>> On 1/25/23 16:06, Ben Boeckel wrote:
>>> They affect the build, so report them via `-MF` mechanisms.
>>
>> Why isn't this covered by the existing code in preprocessed_module?
> 
> It appears as though it is neutered in patch 3 where
> `write_make_modules_deps` is used in `make_write` (or will use that name

Why do you want to record the transitive modules? I would expect just noting the 
ones with imports directly in the TU would suffice (i.e check the 'outermost' arg)

nathan


-- 
Nathan Sidwell

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Ben Boeckel]

On Fri, Jun 23, 2023 at 08:12:41 -0400, Nathan Sidwell wrote:
> On 6/22/23 22:45, Ben Boeckel wrote:
> > On Thu, Jun 22, 2023 at 17:21:42 -0400, Jason Merrill wrote:
> >> On 1/25/23 16:06, Ben Boeckel wrote:
> >>> They affect the build, so report them via `-MF` mechanisms.
> >>
> >> Why isn't this covered by the existing code in preprocessed_module?
> > 
> > It appears as though it is neutered in patch 3 where
> > `write_make_modules_deps` is used in `make_write` (or will use that name
> 
> Why do you want to record the transitive modules? I would expect just noting the 
> ones with imports directly in the TU would suffice (i.e check the 'outermost' arg)

FWIW, only GCC has "fat" modules. MSVC and Clang both require the
transitive closure to be passed. The idea there is to minimize the size
of individual module files.

If GCC only reads the "fat" modules, then only those should be recorded.
If it reads other modules, they should be recorded as well.

--Ben

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Jason Merrill]

On 6/25/23 12:36, Ben Boeckel wrote:
> On Fri, Jun 23, 2023 at 08:12:41 -0400, Nathan Sidwell wrote:
>> On 6/22/23 22:45, Ben Boeckel wrote:
>>> On Thu, Jun 22, 2023 at 17:21:42 -0400, Jason Merrill wrote:
>>>> On 1/25/23 16:06, Ben Boeckel wrote:
>>>>> They affect the build, so report them via `-MF` mechanisms.
>>>>
>>>> Why isn't this covered by the existing code in preprocessed_module?
>>>
>>> It appears as though it is neutered in patch 3 where
>>> `write_make_modules_deps` is used in `make_write` (or will use that name
>>
>> Why do you want to record the transitive modules? I would expect just noting the
>> ones with imports directly in the TU would suffice (i.e check the 'outermost' arg)
> 
> FWIW, only GCC has "fat" modules. MSVC and Clang both require the
> transitive closure to be passed. The idea there is to minimize the size
> of individual module files.
> 
> If GCC only reads the "fat" modules, then only those should be recorded.
> If it reads other modules, they should be recorded as well.

But wouldn't the transitive modules be dependencies of the direct 
imports, so (re)building the direct imports would first require building 
the transitive modules anyway?  Expressing the transitive closure of 
dependencies for each importer seems redundant when it can be easily 
derived from the direct dependencies of each module.

Jason

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Nathan Sidwell]

On 7/18/23 16:52, Jason Merrill wrote:
> On 6/25/23 12:36, Ben Boeckel wrote:
>> On Fri, Jun 23, 2023 at 08:12:41 -0400, Nathan Sidwell wrote:
>>> On 6/22/23 22:45, Ben Boeckel wrote:
>>>> On Thu, Jun 22, 2023 at 17:21:42 -0400, Jason Merrill wrote:
>>>>> On 1/25/23 16:06, Ben Boeckel wrote:
>>>>>> They affect the build, so report them via `-MF` mechanisms.
>>>>>
>>>>> Why isn't this covered by the existing code in preprocessed_module?
>>>>
>>>> It appears as though it is neutered in patch 3 where
>>>> `write_make_modules_deps` is used in `make_write` (or will use that name
>>>
>>> Why do you want to record the transitive modules? I would expect just noting the
>>> ones with imports directly in the TU would suffice (i.e check the 'outermost' 
>>> arg)
>>
>> FWIW, only GCC has "fat" modules. MSVC and Clang both require the
>> transitive closure to be passed. The idea there is to minimize the size
>> of individual module files.
>>
>> If GCC only reads the "fat" modules, then only those should be recorded.
>> If it reads other modules, they should be recorded as well.

Please explain what you mean by fat modules.  There seems to be confusion.

> 
> But wouldn't the transitive modules be dependencies of the direct imports, so 
> (re)building the direct imports would first require building the transitive 
> modules anyway?  Expressing the transitive closure of dependencies for each 
> importer seems redundant when it can be easily derived from the direct 
> dependencies of each module.
> 
> Jason
> 

-- 
Nathan Sidwell

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Ben Boeckel]

On Tue, Jul 18, 2023 at 16:52:44 -0400, Jason Merrill wrote:
> On 6/25/23 12:36, Ben Boeckel wrote:
> > On Fri, Jun 23, 2023 at 08:12:41 -0400, Nathan Sidwell wrote:
> >> On 6/22/23 22:45, Ben Boeckel wrote:
> >>> On Thu, Jun 22, 2023 at 17:21:42 -0400, Jason Merrill wrote:
> >>>> On 1/25/23 16:06, Ben Boeckel wrote:
> >>>>> They affect the build, so report them via `-MF` mechanisms.
> >>>>
> >>>> Why isn't this covered by the existing code in preprocessed_module?
> >>>
> >>> It appears as though it is neutered in patch 3 where
> >>> `write_make_modules_deps` is used in `make_write` (or will use that name
> >>
> >> Why do you want to record the transitive modules? I would expect just noting the
> >> ones with imports directly in the TU would suffice (i.e check the 'outermost' arg)
> > 
> > FWIW, only GCC has "fat" modules. MSVC and Clang both require the
> > transitive closure to be passed. The idea there is to minimize the size
> > of individual module files.
> > 
> > If GCC only reads the "fat" modules, then only those should be recorded.
> > If it reads other modules, they should be recorded as well.

For clarification, given:

* a.cppm
```
export module a;
```

* b.cppm
```
export module b;
import a;
```

* use.cppm
```
import b;
```

in a "fat" module setup, `use.cppm` only needs to be told about
`b.cmi` because it contains everything that an importer needs to know
about the `a` module (reachable types, re-exported bits, whatever). With
the "thin" modules, `a.cmi` must be specified when compiling `use.cppm`
to satisfy anything that may be required transitively (e.g., a return
type of some `b` symbol is from `a`). MSVC and Clang (17+) use this
model. I don't know MSVC's rationale, but Clang's is to make CMIs
relocatable by not embedding paths to dependent modules in CMIs. This
should help caching and network transfer sizes in distributed builds.
LLVM/Clang discussion:

    https://discourse.llvm.org/t/c-20-modules-should-the-bmis-contain-paths-to-their-dependent-bmis/70422
    https://github.com/llvm/llvm-project/issues/62707

Maybe I'm missing how this *actually* works in GCC as I've really only
interacted with it through the command line, but I've not needed to
mention `a.cmi` when compiling `use.cppm`. Is `a.cmi` referenced and
read through some embedded information in `b.cmi` or does `b.cmi`
include enough information to not need to read it at all? If the former,
distributed builds are going to have a problem knowing what files to
send just from the command line (I'll call this "implicit thin"). If the
latter, that is the "fat" CMI that I'm thinking of.

> But wouldn't the transitive modules be dependencies of the direct 
> imports, so (re)building the direct imports would first require building 
> the transitive modules anyway?  Expressing the transitive closure of 
> dependencies for each importer seems redundant when it can be easily 
> derived from the direct dependencies of each module.

I'm not concerned whether it is transitive or not, really. If a file is
read, it should be reported here regardless of the reason. Note that
caching mechanisms may skip actually *doing* the reading, but the
dependencies should still be reported from the cached results as-if the
real work had been performed.

--Ben

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Nathan Sidwell]

On 7/18/23 20:01, Ben Boeckel wrote:
> On Tue, Jul 18, 2023 at 16:52:44 -0400, Jason Merrill wrote:
>> On 6/25/23 12:36, Ben Boeckel wrote:
>>> On Fri, Jun 23, 2023 at 08:12:41 -0400, Nathan Sidwell wrote:
>>>> On 6/22/23 22:45, Ben Boeckel wrote:
>>>>> On Thu, Jun 22, 2023 at 17:21:42 -0400, Jason Merrill wrote:
>>>>>> On 1/25/23 16:06, Ben Boeckel wrote:
>>>>>>> They affect the build, so report them via `-MF` mechanisms.
>>>>>>
>>>>>> Why isn't this covered by the existing code in preprocessed_module?
>>>>>
>>>>> It appears as though it is neutered in patch 3 where
>>>>> `write_make_modules_deps` is used in `make_write` (or will use that name
>>>>
>>>> Why do you want to record the transitive modules? I would expect just noting the
>>>> ones with imports directly in the TU would suffice (i.e check the 'outermost' arg)
>>>
>>> FWIW, only GCC has "fat" modules. MSVC and Clang both require the
>>> transitive closure to be passed. The idea there is to minimize the size
>>> of individual module files.
>>>
>>> If GCC only reads the "fat" modules, then only those should be recorded.
>>> If it reads other modules, they should be recorded as well.
> 
> For clarification, given:
> 
> * a.cppm
> ```
> export module a;
> ```
> 
> * b.cppm
> ```
> export module b;
> import a;
> ```
> 
> * use.cppm
> ```
> import b;
> ```
> 
> in a "fat" module setup, `use.cppm` only needs to be told about
> `b.cmi` because it contains everything that an importer needs to know
> about the `a` module (reachable types, re-exported bits, whateve > With
> the "thin" modules, `a.cmi` must be specified when compiling `use.cppm`
> to satisfy anything that may be required transitively (e.g., a return

GCC is neither of these descriptions.  a CMI does not contain the transitive 
closure of its imports.  It contains an import table.  That table lists the 
transitive closure of its imports (it needs that closure to do remapping), and 
that table contains the CMI pathnames of the direct imports.  Those pathnames 
are absolute, if the mapper provded an absolute pathm or relative to the CMI repo.

The rationale here is that if you're building a CMI, Foo, which imports a bunch 
of modules, those imported CMIs will have the same (relative) location in this 
compilation and in compilations importing Foo (why would you move them?) Note 
this is NOT inhibiting relocatable builds, because of the CMI repo.


> Maybe I'm missing how this *actually* works in GCC as I've really only
> interacted with it through the command line, but I've not needed to
> mention `a.cmi` when compiling `use.cppm`. Is `a.cmi` referenced and
> read through some embedded information in `b.cmi` or does `b.cmi`
> include enough information to not need to read it at all? If the former,
> distributed builds are going to have a problem knowing what files to
> send just from the command line (I'll call this "implicit thin"). If the
> latter, that is the "fat" CMI that I'm thinking of.

please don't use perjorative terms like 'fat' and 'thin'.

> 
>> But wouldn't the transitive modules be dependencies of the direct
>> imports, so (re)building the direct imports would first require building
>> the transitive modules anyway?  Expressing the transitive closure of
>> dependencies for each importer seems redundant when it can be easily
>> derived from the direct dependencies of each module.
> 
> I'm not concerned whether it is transitive or not, really. If a file is
> read, it should be reported here regardless of the reason. Note that
> caching mechanisms may skip actually *doing* the reading, but the
> dependencies should still be reported from the cached results as-if the
> real work had been performed.
> 
> --Ben

-- 
Nathan Sidwell

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Ben Boeckel]

On Wed, Jul 19, 2023 at 17:11:08 -0400, Nathan Sidwell wrote:
> GCC is neither of these descriptions.  a CMI does not contain the transitive 
> closure of its imports.  It contains an import table.  That table lists the 
> transitive closure of its imports (it needs that closure to do remapping), and 
> that table contains the CMI pathnames of the direct imports.  Those pathnames 
> are absolute, if the mapper provded an absolute pathm or relative to the CMI repo.
> 
> The rationale here is that if you're building a CMI, Foo, which imports a bunch 
> of modules, those imported CMIs will have the same (relative) location in this 
> compilation and in compilations importing Foo (why would you move them?) Note 
> this is NOT inhibiting relocatable builds, because of the CMI repo.

But it is inhibiting distributed builds because the distributing tool
would need to know:

- what CMIs are actually imported (here, "read the module mapper file"
  (in CMake's case, this is only the modules that are needed; a single
  massive mapper file for an entire project would have extra entries) or
  "act as a proxy for the socket/program specified" for other
  approaches);
- read the CMIs as it sends to the remote side to gather any other CMIs
  that may be needed (recursively);

Contrast this with the MSVC and Clang (17+) mechanism where the command
line contains everything that is needed and a single bolus can be sent.

And relocatable is probably fine. How does it interact with reproducible
builds? Or are GCC CMIs not really something anyone should consider for
installation (even as a "here, maybe this can help consumers"
mechanism)?

> On 7/18/23 20:01, Ben Boeckel wrote:
> > Maybe I'm missing how this *actually* works in GCC as I've really only
> > interacted with it through the command line, but I've not needed to
> > mention `a.cmi` when compiling `use.cppm`. Is `a.cmi` referenced and
> > read through some embedded information in `b.cmi` or does `b.cmi`
> > include enough information to not need to read it at all? If the former,
> > distributed builds are going to have a problem knowing what files to
> > send just from the command line (I'll call this "implicit thin"). If the
> > latter, that is the "fat" CMI that I'm thinking of.
> 
> please don't use perjorative terms like 'fat' and 'thin'.

Sorry, I was internally analogizing to "thinLTO".

--Ben

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Nathan Sidwell]

On 7/19/23 20:47, Ben Boeckel wrote:
> On Wed, Jul 19, 2023 at 17:11:08 -0400, Nathan Sidwell wrote:
>> GCC is neither of these descriptions.  a CMI does not contain the transitive
>> closure of its imports.  It contains an import table.  That table lists the
>> transitive closure of its imports (it needs that closure to do remapping), and
>> that table contains the CMI pathnames of the direct imports.  Those pathnames
>> are absolute, if the mapper provded an absolute pathm or relative to the CMI repo.
>>
>> The rationale here is that if you're building a CMI, Foo, which imports a bunch
>> of modules, those imported CMIs will have the same (relative) location in this
>> compilation and in compilations importing Foo (why would you move them?) Note
>> this is NOT inhibiting relocatable builds, because of the CMI repo.
> 
> But it is inhibiting distributed builds because the distributing tool
> would need to know:
> 
> - what CMIs are actually imported (here, "read the module mapper file"
>    (in CMake's case, this is only the modules that are needed; a single
>    massive mapper file for an entire project would have extra entries) or
>    "act as a proxy for the socket/program specified" for other
>    approaches);

This information is in the machine (& human) README section of the CMI.

> - read the CMIs as it sends to the remote side to gather any other CMIs
>    that may be needed (recursively);
> 
> Contrast this with the MSVC and Clang (17+) mechanism where the command
> line contains everything that is needed and a single bolus can be sent.

um, the build system needs to create that command line? Where does the build 
system get that information?  IIUC it'll need to read some file(s) to do that.


> And relocatable is probably fine. How does it interact with reproducible
> builds? Or are GCC CMIs not really something anyone should consider for
> installation (even as a "here, maybe this can help consumers"
> mechanism)?

Module CMIs should be considered a cacheable artifact.  They are neither object 
files nor source files.

> 
>> On 7/18/23 20:01, Ben Boeckel wrote:
>>> Maybe I'm missing how this *actually* works in GCC as I've really only
>>> interacted with it through the command line, but I've not needed to
>>> mention `a.cmi` when compiling `use.cppm`. Is `a.cmi` referenced and
>>> read through some embedded information in `b.cmi` or does `b.cmi`
>>> include enough information to not need to read it at all? If the former,
>>> distributed builds are going to have a problem knowing what files to
>>> send just from the command line (I'll call this "implicit thin"). If the
>>> latter, that is the "fat" CMI that I'm thinking of.
>>
>> please don't use perjorative terms like 'fat' and 'thin'.
> 
> Sorry, I was internally analogizing to "thinLTO".
> 
> --Ben

-- 
Nathan Sidwell

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Ben Boeckel]

On Thu, Jul 20, 2023 at 17:00:32 -0400, Nathan Sidwell wrote:
> On 7/19/23 20:47, Ben Boeckel wrote:
> > But it is inhibiting distributed builds because the distributing tool
> > would need to know:
> > 
> > - what CMIs are actually imported (here, "read the module mapper file"
> >    (in CMake's case, this is only the modules that are needed; a single
> >    massive mapper file for an entire project would have extra entries) or
> >    "act as a proxy for the socket/program specified" for other
> >    approaches);
> 
> This information is in the machine (& human) README section of the CMI.

Ok. That leaves it up to distributing build tools to figure out at
least.

> > - read the CMIs as it sends to the remote side to gather any other CMIs
> >    that may be needed (recursively);
> > 
> > Contrast this with the MSVC and Clang (17+) mechanism where the command
> > line contains everything that is needed and a single bolus can be sent.
> 
> um, the build system needs to create that command line? Where does the build 
> system get that information?  IIUC it'll need to read some file(s) to do that.

It's chained through the P1689 information in the collator as needed. No
extra files need to be read (at least with CMake's approach); certainly
not CMI files.

> > And relocatable is probably fine. How does it interact with reproducible
> > builds? Or are GCC CMIs not really something anyone should consider for
> > installation (even as a "here, maybe this can help consumers"
> > mechanism)?
> 
> Module CMIs should be considered a cacheable artifact.  They are neither object 
> files nor source files.

Sure, cachable sounds fine. What about the installation?

--Ben

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Nathan Sidwell]

On 7/21/23 10:57, Ben Boeckel wrote:
> On Thu, Jul 20, 2023 at 17:00:32 -0400, Nathan Sidwell wrote:
>> On 7/19/23 20:47, Ben Boeckel wrote:
>>> But it is inhibiting distributed builds because the distributing tool
>>> would need to know:
>>>
>>> - what CMIs are actually imported (here, "read the module mapper file"
>>>     (in CMake's case, this is only the modules that are needed; a single
>>>     massive mapper file for an entire project would have extra entries) or
>>>     "act as a proxy for the socket/program specified" for other
>>>     approaches);
>>
>> This information is in the machine (& human) README section of the CMI.
> 
> Ok. That leaves it up to distributing build tools to figure out at
> least.
> 
>>> - read the CMIs as it sends to the remote side to gather any other CMIs
>>>     that may be needed (recursively);
>>>
>>> Contrast this with the MSVC and Clang (17+) mechanism where the command
>>> line contains everything that is needed and a single bolus can be sent.
>>
>> um, the build system needs to create that command line? Where does the build
>> system get that information?  IIUC it'll need to read some file(s) to do that.
> 
> It's chained through the P1689 information in the collator as needed. No
> extra files need to be read (at least with CMake's approach); certainly
> not CMI files.

It occurs to me that the model I am envisioning is similar to CMake's object 
libraries.  Object libraries are a convenient name for a bunch of object files. 
IIUC they're linked by naming the individual object files (or I think the could 
be implemented as a static lib linked with --whole-archive path/to/libfoo.a 
-no-whole-archive.  But for this conversation consider them a bunch of separate 
object files with a convenient group name.

Consider also that object libraries could themselves contain object libraries (I 
don't know of they can, but it seems like a useful concept).  Then one could 
create an object library from a collection of object files and object libraries 
(recursively).  CMake would handle the transitive gtaph.

Now, allow an object library to itself have some kind of tangible, on-disk 
representation.  *BUT* not like a static library -- it doesn't include the 
object files.


Now that immediately maps onto modules.

CMI: Object library
Direct imports: Direct object libraries of an object library

This is why I don't understand the need explicitly indicate the indirect imports 
of a CMI.  CMake knows them, because it knows the graph.

> 
>>> And relocatable is probably fine. How does it interact with reproducible
>>> builds? Or are GCC CMIs not really something anyone should consider for
>>> installation (even as a "here, maybe this can help consumers"
>>> mechanism)?
>>
>> Module CMIs should be considered a cacheable artifact.  They are neither object
>> files nor source files.
> 
> Sure, cachable sounds fine. What about the installation?
> 
> --Ben

-- 
Nathan Sidwell

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Ben Boeckel]

On Fri, Jul 21, 2023 at 16:23:07 -0400, Nathan Sidwell wrote:
> It occurs to me that the model I am envisioning is similar to CMake's object 
> libraries.  Object libraries are a convenient name for a bunch of object files. 
> IIUC they're linked by naming the individual object files (or I think the could 
> be implemented as a static lib linked with --whole-archive path/to/libfoo.a 
> -no-whole-archive.  But for this conversation consider them a bunch of separate 
> object files with a convenient group name.

Yes, `--whole-archive` would work great if it had any kind of
portability across CMake's platform set.

> Consider also that object libraries could themselves contain object libraries (I 
> don't know of they can, but it seems like a useful concept).  Then one could 
> create an object library from a collection of object files and object libraries 
> (recursively).  CMake would handle the transitive gtaph.

I think this detail is relevant, but you can use
`$<TARGET_OBJECT:subobjlib>` as an `INTERFACE` sources and it would act
like that, but it is an explicit thing. Instead, `OBJECT` libraries
*only* provide their objects to targets that *directly* link them. If
not, given this:

    A (OBJECT library)
    B (library of some kind; links PUBLIC to A)
    C (links to B)

If `A` has things like linker flags (or, more likely, libraries) as part
of its usage requirements, C will get them on is link line. However, if
OBJECT files are transitive in the same way, the linker (on most
platforms at least) chokes because it now has duplicates of all of A's
symbols: those from the B library and those from A's objects on the link
line.

> Now, allow an object library to itself have some kind of tangible, on-disk 
> representation.  *BUT* not like a static library -- it doesn't include the 
> object files.
> 
> 
> Now that immediately maps onto modules.
> 
> CMI: Object library
> Direct imports: Direct object libraries of an object library
> 
> This is why I don't understand the need explicitly indicate the indirect imports 
> of a CMI.  CMake knows them, because it knows the graph.

Sure, *CMake* knows them, but the *build tool* needs to be told
(typically `make` or `ninja`) because it is what is actually executing
the build graph. The way this is communicated is via `-MF` files and
that's what I'm providing in this patch. Note that `ninja` does not
allow rules to specify such dependencies for other rules than the one it
is reading the file for.

--Ben

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Jason Merrill]

On 7/23/23 20:26, Ben Boeckel wrote:
> On Fri, Jul 21, 2023 at 16:23:07 -0400, Nathan Sidwell wrote:
>> It occurs to me that the model I am envisioning is similar to CMake's object
>> libraries.  Object libraries are a convenient name for a bunch of object files.
>> IIUC they're linked by naming the individual object files (or I think the could
>> be implemented as a static lib linked with --whole-archive path/to/libfoo.a
>> -no-whole-archive.  But for this conversation consider them a bunch of separate
>> object files with a convenient group name.
> 
> Yes, `--whole-archive` would work great if it had any kind of
> portability across CMake's platform set.
> 
>> Consider also that object libraries could themselves contain object libraries (I
>> don't know of they can, but it seems like a useful concept).  Then one could
>> create an object library from a collection of object files and object libraries
>> (recursively).  CMake would handle the transitive gtaph.
> 
> I think this detail is relevant, but you can use
> `$<TARGET_OBJECT:subobjlib>` as an `INTERFACE` sources and it would act
> like that, but it is an explicit thing. Instead, `OBJECT` libraries
> *only* provide their objects to targets that *directly* link them. If
> not, given this:
> 
>      A (OBJECT library)
>      B (library of some kind; links PUBLIC to A)
>      C (links to B)
> 
> If `A` has things like linker flags (or, more likely, libraries) as part
> of its usage requirements, C will get them on is link line. However, if
> OBJECT files are transitive in the same way, the linker (on most
> platforms at least) chokes because it now has duplicates of all of A's
> symbols: those from the B library and those from A's objects on the link
> line.
> 
>> Now, allow an object library to itself have some kind of tangible, on-disk
>> representation.  *BUT* not like a static library -- it doesn't include the
>> object files.
>>
>>
>> Now that immediately maps onto modules.
>>
>> CMI: Object library
>> Direct imports: Direct object libraries of an object library
>>
>> This is why I don't understand the need explicitly indicate the indirect imports
>> of a CMI.  CMake knows them, because it knows the graph.
> 
> Sure, *CMake* knows them, but the *build tool* needs to be told
> (typically `make` or `ninja`) because it is what is actually executing
> the build graph. The way this is communicated is via `-MF` files and
> that's what I'm providing in this patch. Note that `ninja` does not
> allow rules to specify such dependencies for other rules than the one it
> is reading the file for.

But since the direct imports need to be rebuilt themselves if the 
transitive imports change, the build graph should be the same whether or 
not the transitive imports are repeated?  Either way, if a transitive 
import changes you need to rebuild the direct import and then the importer.

I guess it shouldn't hurt to have the transitive imports in the -MF 
file, as long as they aren't also in the p1689 file, so I'm not 
particularly opposed to this change, but I don't see how it makes a 
practical difference.

Jason

Re: [PATCH v5 4/5] c++modules: report imported CMI files as dependencies

[Ben Boeckel]

On Thu, Jul 27, 2023 at 18:13:48 -0700, Jason Merrill wrote:
> On 7/23/23 20:26, Ben Boeckel wrote:
> > Sure, *CMake* knows them, but the *build tool* needs to be told
> > (typically `make` or `ninja`) because it is what is actually executing
> > the build graph. The way this is communicated is via `-MF` files and
> > that's what I'm providing in this patch. Note that `ninja` does not
> > allow rules to specify such dependencies for other rules than the one it
> > is reading the file for.
> 
> But since the direct imports need to be rebuilt themselves if the 
> transitive imports change, the build graph should be the same whether or 
> not the transitive imports are repeated?  Either way, if a transitive 
> import changes you need to rebuild the direct import and then the importer.

I suppose I have seen enough bad build systems that don't do everything
correctly that I'm interested in creating "pits of success" rather than
"well, you didn't get thing X 100% correct, so you're screwed here too".

The case that I think is most likely here is that someone has a
"superbuild" with 3 projects A, B, and C where C uses B and B uses A. At
the top-level the superbuild exposes just "make projectA
projectB projectC"-granularity (rather than a combined build graph; they
may use different build systems) and then users go into some projectC
directly and forget to update projectB after updating projectA (known to
all use the same compiler/flags so that CMI sharing is possible). The
build it still broken, but ideally they get notified in some useful way
when rebuilding the TU rather than…whatever ends up catching the problem
incidentally.

> I guess it shouldn't hurt to have the transitive imports in the -MF 
> file, as long as they aren't also in the p1689 file, so I'm not 
> particularly opposed to this change, but I don't see how it makes a 
> practical difference.

Correct. The P1689 shouldn't even know about transitive imports (well,
maybe from header units?) as it just records "I saw an `import`
statement" and should never look up CMI files (indeed, we would need
another scanning step to know what CMI files to create for the P1689
scan if they were necessary…).

--Ben

[PATCH v5 5/5] c++modules: report module mapper files as a dependency

[Ben Boeckel]

It affects the build, and if used as a static file, can reliably be
tracked using the `-MF` mechanism.

gcc/cp/:

	* mapper-client.cc, mapper-client.h (open_module_client): Accept
	dependency tracking and track module mapper files as
	dependencies.
	* module.cc (make_mapper, get_mapper): Pass the dependency
	tracking class down.

Signed-off-by: Ben Boeckel <ben.boeckel@kitware.com>
---
 gcc/cp/mapper-client.cc |  4 ++++
 gcc/cp/mapper-client.h  |  1 +
 gcc/cp/module.cc        | 18 +++++++++---------
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/gcc/cp/mapper-client.cc b/gcc/cp/mapper-client.cc
index 39e80df2d25..0ce5679d659 100644
--- a/gcc/cp/mapper-client.cc
+++ b/gcc/cp/mapper-client.cc
@@ -34,6 +34,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "diagnostic-core.h"
 #include "mapper-client.h"
 #include "intl.h"
+#include "mkdeps.h"
 
 #include "../../c++tools/resolver.h"
 
@@ -132,6 +133,7 @@ spawn_mapper_program (char const **errmsg, std::string &name,
 
 module_client *
 module_client::open_module_client (location_t loc, const char *o,
+				   class mkdeps *deps,
 				   void (*set_repo) (const char *),
 				   char const *full_program_name)
 {
@@ -285,6 +287,8 @@ module_client::open_module_client (location_t loc, const char *o,
 	  errmsg = "opening";
 	else
 	  {
+	    /* Add the mapper file to the dependency tracking. */
+	    deps_add_dep (deps, name.c_str ());
 	    if (int l = r->read_tuple_file (fd, ident, false))
 	      {
 		if (l > 0)
diff --git a/gcc/cp/mapper-client.h b/gcc/cp/mapper-client.h
index b32723ce296..a3b0b8adc51 100644
--- a/gcc/cp/mapper-client.h
+++ b/gcc/cp/mapper-client.h
@@ -55,6 +55,7 @@ public:
 
 public:
   static module_client *open_module_client (location_t loc, const char *option,
+					    class mkdeps *,
 					    void (*set_repo) (const char *),
 					    char const *);
   static void close_module_client (location_t loc, module_client *);
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index dbd1b721616..37066bf072b 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -3969,12 +3969,12 @@ static GTY(()) vec<tree, va_gc> *partial_specializations;
 /* Our module mapper (created lazily).  */
 module_client *mapper;
 
-static module_client *make_mapper (location_t loc);
-inline module_client *get_mapper (location_t loc)
+static module_client *make_mapper (location_t loc, class mkdeps *deps);
+inline module_client *get_mapper (location_t loc, class mkdeps *deps)
 {
   auto *res = mapper;
   if (!res)
-    res = make_mapper (loc);
+    res = make_mapper (loc, deps);
   return res;
 }
 
@@ -14031,7 +14031,7 @@ get_module (const char *ptr)
 /* Create a new mapper connecting to OPTION.  */
 
 module_client *
-make_mapper (location_t loc)
+make_mapper (location_t loc, class mkdeps *deps)
 {
   timevar_start (TV_MODULE_MAPPER);
   const char *option = module_mapper_name;
@@ -14039,7 +14039,7 @@ make_mapper (location_t loc)
     option = getenv ("CXX_MODULE_MAPPER");
 
   mapper = module_client::open_module_client
-    (loc, option, &set_cmi_repo,
+    (loc, option, deps, &set_cmi_repo,
      (save_decoded_options[0].opt_index == OPT_SPECIAL_program_name)
      && save_decoded_options[0].arg != progname
      ? save_decoded_options[0].arg : nullptr);
@@ -19503,7 +19503,7 @@ maybe_translate_include (cpp_reader *reader, line_maps *lmaps, location_t loc,
   dump.push (NULL);
 
   dump () && dump ("Checking include translation '%s'", path);
-  auto *mapper = get_mapper (cpp_main_loc (reader));
+  auto *mapper = get_mapper (cpp_main_loc (reader), cpp_get_deps (reader));
 
   size_t len = strlen (path);
   path = canonicalize_header_name (NULL, loc, true, path, len);
@@ -19619,7 +19619,7 @@ module_begin_main_file (cpp_reader *reader, line_maps *lmaps,
 static void
 name_pending_imports (cpp_reader *reader)
 {
-  auto *mapper = get_mapper (cpp_main_loc (reader));
+  auto *mapper = get_mapper (cpp_main_loc (reader), cpp_get_deps (reader));
 
   if (!vec_safe_length (pending_imports))
     /* Not doing anything.  */
@@ -20089,7 +20089,7 @@ init_modules (cpp_reader *reader)
 
   if (!flag_module_lazy)
     /* Get the mapper now, if we're not being lazy.  */
-    get_mapper (cpp_main_loc (reader));
+    get_mapper (cpp_main_loc (reader), cpp_get_deps (reader));
 
   if (!flag_preprocess_only)
     {
@@ -20299,7 +20299,7 @@ late_finish_module (cpp_reader *reader,  module_processing_cookie *cookie,
 
   if (!errorcount)
     {
-      auto *mapper = get_mapper (cpp_main_loc (reader));
+      auto *mapper = get_mapper (cpp_main_loc (reader), cpp_get_deps (reader));
       mapper->ModuleCompiled (state->get_flatname ());
     }
   else if (cookie->cmi_name)
-- 
2.39.0

Re: [PATCH v5 5/5] c++modules: report module mapper files as a dependency

[Jason Merrill]

On 1/25/23 16:06, Ben Boeckel wrote:
> It affects the build, and if used as a static file, can reliably be
> tracked using the `-MF` mechanism.

Hmm, this seems a bit like making all .o depend on the Makefile; it 
shouldn't be necessary to rebuild all TUs that use modules when we add 
another module to the mapper file.  What is your expected use case for 
this dependency?

> gcc/cp/:
> 
> 	* mapper-client.cc, mapper-client.h (open_module_client): Accept
> 	dependency tracking and track module mapper files as
> 	dependencies.
> 	* module.cc (make_mapper, get_mapper): Pass the dependency
> 	tracking class down.
> 
> Signed-off-by: Ben Boeckel <ben.boeckel@kitware.com>
> ---
>   gcc/cp/mapper-client.cc |  4 ++++
>   gcc/cp/mapper-client.h  |  1 +
>   gcc/cp/module.cc        | 18 +++++++++---------
>   3 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/gcc/cp/mapper-client.cc b/gcc/cp/mapper-client.cc
> index 39e80df2d25..0ce5679d659 100644
> --- a/gcc/cp/mapper-client.cc
> +++ b/gcc/cp/mapper-client.cc
> @@ -34,6 +34,7 @@ along with GCC; see the file COPYING3.  If not see
>   #include "diagnostic-core.h"
>   #include "mapper-client.h"
>   #include "intl.h"
> +#include "mkdeps.h"
>   
>   #include "../../c++tools/resolver.h"
>   
> @@ -132,6 +133,7 @@ spawn_mapper_program (char const **errmsg, std::string &name,
>   
>   module_client *
>   module_client::open_module_client (location_t loc, const char *o,
> +				   class mkdeps *deps,
>   				   void (*set_repo) (const char *),
>   				   char const *full_program_name)
>   {
> @@ -285,6 +287,8 @@ module_client::open_module_client (location_t loc, const char *o,
>   	  errmsg = "opening";
>   	else
>   	  {
> +	    /* Add the mapper file to the dependency tracking. */
> +	    deps_add_dep (deps, name.c_str ());
>   	    if (int l = r->read_tuple_file (fd, ident, false))
>   	      {
>   		if (l > 0)
> diff --git a/gcc/cp/mapper-client.h b/gcc/cp/mapper-client.h
> index b32723ce296..a3b0b8adc51 100644
> --- a/gcc/cp/mapper-client.h
> +++ b/gcc/cp/mapper-client.h
> @@ -55,6 +55,7 @@ public:
>   
>   public:
>     static module_client *open_module_client (location_t loc, const char *option,
> +					    class mkdeps *,
>   					    void (*set_repo) (const char *),
>   					    char const *);
>     static void close_module_client (location_t loc, module_client *);
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index dbd1b721616..37066bf072b 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -3969,12 +3969,12 @@ static GTY(()) vec<tree, va_gc> *partial_specializations;
>   /* Our module mapper (created lazily).  */
>   module_client *mapper;
>   
> -static module_client *make_mapper (location_t loc);
> -inline module_client *get_mapper (location_t loc)
> +static module_client *make_mapper (location_t loc, class mkdeps *deps);
> +inline module_client *get_mapper (location_t loc, class mkdeps *deps)
>   {
>     auto *res = mapper;
>     if (!res)
> -    res = make_mapper (loc);
> +    res = make_mapper (loc, deps);
>     return res;
>   }
>   
> @@ -14031,7 +14031,7 @@ get_module (const char *ptr)
>   /* Create a new mapper connecting to OPTION.  */
>   
>   module_client *
> -make_mapper (location_t loc)
> +make_mapper (location_t loc, class mkdeps *deps)
>   {
>     timevar_start (TV_MODULE_MAPPER);
>     const char *option = module_mapper_name;
> @@ -14039,7 +14039,7 @@ make_mapper (location_t loc)
>       option = getenv ("CXX_MODULE_MAPPER");
>   
>     mapper = module_client::open_module_client
> -    (loc, option, &set_cmi_repo,
> +    (loc, option, deps, &set_cmi_repo,
>        (save_decoded_options[0].opt_index == OPT_SPECIAL_program_name)
>        && save_decoded_options[0].arg != progname
>        ? save_decoded_options[0].arg : nullptr);
> @@ -19503,7 +19503,7 @@ maybe_translate_include (cpp_reader *reader, line_maps *lmaps, location_t loc,
>     dump.push (NULL);
>   
>     dump () && dump ("Checking include translation '%s'", path);
> -  auto *mapper = get_mapper (cpp_main_loc (reader));
> +  auto *mapper = get_mapper (cpp_main_loc (reader), cpp_get_deps (reader));
>   
>     size_t len = strlen (path);
>     path = canonicalize_header_name (NULL, loc, true, path, len);
> @@ -19619,7 +19619,7 @@ module_begin_main_file (cpp_reader *reader, line_maps *lmaps,
>   static void
>   name_pending_imports (cpp_reader *reader)
>   {
> -  auto *mapper = get_mapper (cpp_main_loc (reader));
> +  auto *mapper = get_mapper (cpp_main_loc (reader), cpp_get_deps (reader));
>   
>     if (!vec_safe_length (pending_imports))
>       /* Not doing anything.  */
> @@ -20089,7 +20089,7 @@ init_modules (cpp_reader *reader)
>   
>     if (!flag_module_lazy)
>       /* Get the mapper now, if we're not being lazy.  */
> -    get_mapper (cpp_main_loc (reader));
> +    get_mapper (cpp_main_loc (reader), cpp_get_deps (reader));
>   
>     if (!flag_preprocess_only)
>       {
> @@ -20299,7 +20299,7 @@ late_finish_module (cpp_reader *reader,  module_processing_cookie *cookie,
>   
>     if (!errorcount)
>       {
> -      auto *mapper = get_mapper (cpp_main_loc (reader));
> +      auto *mapper = get_mapper (cpp_main_loc (reader), cpp_get_deps (reader));
>         mapper->ModuleCompiled (state->get_flatname ());
>       }
>     else if (cookie->cmi_name)

Re: [PATCH v5 5/5] c++modules: report module mapper files as a dependency

[Ben Boeckel]

On Fri, Jun 23, 2023 at 10:44:11 -0400, Jason Merrill wrote:
> On 1/25/23 16:06, Ben Boeckel wrote:
> > It affects the build, and if used as a static file, can reliably be
> > tracked using the `-MF` mechanism.
> 
> Hmm, this seems a bit like making all .o depend on the Makefile; it 

Technically this is true: the command line for the TU lives in said
Makefile; if I updated it, a new TU would be really nice. This is a
long-standing limitation of `make` though. FWIW, `ninja` fixes it by
tracking the command line used and CMake's Makefiles generator handles
it by storing TU flags in an included file and depending on that file
from the TU output.

> shouldn't be necessary to rebuild all TUs that use modules when we add 
> another module to the mapper file.

If I change it from:

```
mod.a   mod.a.cmi
```

to:

```
mod.a   mod.a.replace.cmi
```

I'd expect a recompile. As with anything, this depends on the
granularity of the mapper files. A global mapper file is very similar to
a global response file and given that we don't have line-change
granularity dependency tracking…

>                                     What is your expected use case for 
> this dependency?

CMake, at least, uses a per-TU mapper file, so any build system using a
similar strategy handling the above case would only affect TUs that
actually list `mod.a`.

--Ben

Re: [PATCH v5 0/5] P1689R5 support

[Ben Boeckel]

On Wed, Jan 25, 2023 at 16:06:31 -0500, Ben Boeckel wrote:
> This patch series adds initial support for ISO C++'s [P1689R5][], a
> format for describing C++ module requirements and provisions based on
> the source code. This is required because compiling C++ with modules is
> not embarrassingly parallel and need to be ordered to ensure that
> `import some_module;` can be satisfied in time by making sure that any
> TU with `export import some_module;` is compiled first.
> 
> [P1689R5]: https://isocpp.org/files/papers/P1689R5.html
> 
> I've also added patches to include imported module CMI files and the
> module mapper file as dependencies of the compilation. I briefly looked
> into adding dependencies on response files as well, but that appeared to
> need some code contortions to have a `class mkdeps` available before
> parsing the command line or to keep the information around until one was
> made.
> 
> I'd like feedback on the approach taken here with respect to the
> user-visible flags. I'll also note that header units are not supported
> at this time because the current `-E` behavior with respect to `import
> <some_header>;` is to search for an appropriate `.gcm` file which is not
> something such a "scan" can support. A new mode will likely need to be
> created (e.g., replacing `-E` with `-fc++-module-scanning` or something)
> where headers are looked up "normally" and processed only as much as
> scanning requires.
> 
> FWIW, Clang as taken an alternate approach with its `clang-scan-deps`
> tool rather than using the compiler directly.

Ping? It'd be nice to have this supported in at least GCC 14 (since it
missed 13).

Thanks,

--Ben

Re: [PATCH v5 0/5] P1689R5 support

[Harald Anlauf]

Hi Ben,

Am 25.01.23 um 22:06 schrieb Ben Boeckel via Gcc-patches:
> Hi,
>
> This patch series adds initial support for ISO C++'s [P1689R5][], a
> format for describing C++ module requirements and provisions based on
> the source code. This is required because compiling C++ with modules is
> not embarrassingly parallel and need to be ordered to ensure that
> `import some_module;` can be satisfied in time by making sure that any
> TU with `export import some_module;` is compiled first.
>
> [P1689R5]: https://isocpp.org/files/papers/P1689R5.html

while that paper mentions Fortran, the patch in its present version
does not seem to implement anything related to Fortran and does not
touch the gfortran frontend.  Or am I missing anything?  Otherwise,
could you give an example how it would be used with Fortran?

Thus I'd say that it is OK from the gfortran side.

Thanks,
Harald

> I've also added patches to include imported module CMI files and the
> module mapper file as dependencies of the compilation. I briefly looked
> into adding dependencies on response files as well, but that appeared to
> need some code contortions to have a `class mkdeps` available before
> parsing the command line or to keep the information around until one was
> made.
>
> I'd like feedback on the approach taken here with respect to the
> user-visible flags. I'll also note that header units are not supported
> at this time because the current `-E` behavior with respect to `import
> <some_header>;` is to search for an appropriate `.gcm` file which is not
> something such a "scan" can support. A new mode will likely need to be
> created (e.g., replacing `-E` with `-fc++-module-scanning` or something)
> where headers are looked up "normally" and processed only as much as
> scanning requires.
>
> FWIW, Clang as taken an alternate approach with its `clang-scan-deps`
> tool rather than using the compiler directly.
>
> Thanks,
>
> --Ben
>
> ---
> v4 -> v5:
>
> - add dependency tracking for imported modules to `-MF`
> - add dependency tracking for static module mapper files given to
>    `-fmodule-mapper=`
>
> v3 -> v4:
>
> - add missing spaces between function names and arguments
>
> v2 -> v3:
>
> - changelog entries moved to commit messages
> - documentation updated/added in the UTF-8 routine editing
>
> v1 -> v2:
>
> - removal of the `deps_write(extra)` parameter to option-checking where
>    ndeeded
> - default parameter of `cpp_finish(fdeps_stream = NULL)`
> - unification of libcpp UTF-8 validity functions from v1
> - test cases for flag parsing states (depflags-*) and p1689 output
>    (p1689-*)
>
> Ben Boeckel (5):
>    libcpp: reject codepoints above 0x10FFFF
>    libcpp: add a function to determine UTF-8 validity of a C string
>    p1689r5: initial support
>    c++modules: report imported CMI files as dependencies
>    c++modules: report module mapper files as a dependency
>
>   gcc/c-family/c-opts.cc                        |  40 +++-
>   gcc/c-family/c.opt                            |  12 +
>   gcc/cp/mapper-client.cc                       |   4 +
>   gcc/cp/mapper-client.h                        |   1 +
>   gcc/cp/module.cc                              |  23 +-
>   gcc/doc/invoke.texi                           |  15 ++
>   gcc/testsuite/g++.dg/modules/depflags-f-MD.C  |   2 +
>   gcc/testsuite/g++.dg/modules/depflags-f.C     |   1 +
>   gcc/testsuite/g++.dg/modules/depflags-fi.C    |   3 +
>   gcc/testsuite/g++.dg/modules/depflags-fj-MD.C |   3 +
>   gcc/testsuite/g++.dg/modules/depflags-fj.C    |   4 +
>   .../g++.dg/modules/depflags-fjo-MD.C          |   4 +
>   gcc/testsuite/g++.dg/modules/depflags-fjo.C   |   5 +
>   gcc/testsuite/g++.dg/modules/depflags-fo-MD.C |   3 +
>   gcc/testsuite/g++.dg/modules/depflags-fo.C    |   4 +
>   gcc/testsuite/g++.dg/modules/depflags-j-MD.C  |   2 +
>   gcc/testsuite/g++.dg/modules/depflags-j.C     |   3 +
>   gcc/testsuite/g++.dg/modules/depflags-jo-MD.C |   3 +
>   gcc/testsuite/g++.dg/modules/depflags-jo.C    |   4 +
>   gcc/testsuite/g++.dg/modules/depflags-o-MD.C  |   2 +
>   gcc/testsuite/g++.dg/modules/depflags-o.C     |   3 +
>   gcc/testsuite/g++.dg/modules/modules.exp      |   1 +
>   gcc/testsuite/g++.dg/modules/p1689-1.C        |  18 ++
>   gcc/testsuite/g++.dg/modules/p1689-1.exp.json |  27 +++
>   gcc/testsuite/g++.dg/modules/p1689-2.C        |  16 ++
>   gcc/testsuite/g++.dg/modules/p1689-2.exp.json |  16 ++
>   gcc/testsuite/g++.dg/modules/p1689-3.C        |  14 ++
>   gcc/testsuite/g++.dg/modules/p1689-3.exp.json |  16 ++
>   gcc/testsuite/g++.dg/modules/p1689-4.C        |  14 ++
>   gcc/testsuite/g++.dg/modules/p1689-4.exp.json |  14 ++
>   gcc/testsuite/g++.dg/modules/p1689-5.C        |  14 ++
>   gcc/testsuite/g++.dg/modules/p1689-5.exp.json |  14 ++
>   gcc/testsuite/g++.dg/modules/test-p1689.py    | 222 ++++++++++++++++++
>   gcc/testsuite/lib/modules.exp                 |  71 ++++++
>   libcpp/charset.cc                             |  28 ++-
>   libcpp/include/cpplib.h                       |  12 +-
>   libcpp/include/mkdeps.h                       |  17 +-
>   libcpp/init.cc                                |  13 +-
>   libcpp/internal.h                             |   2 +
>   libcpp/mkdeps.cc                              | 149 +++++++++++-
>   40 files changed, 789 insertions(+), 30 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-f-MD.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-f.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fi.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fj-MD.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fj.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fjo-MD.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fjo.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fo-MD.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fo.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-j-MD.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-j.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-jo-MD.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-jo.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-o-MD.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/depflags-o.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-1.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-1.exp.json
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-2.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-2.exp.json
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-3.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-3.exp.json
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-4.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-4.exp.json
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-5.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/p1689-5.exp.json
>   create mode 100644 gcc/testsuite/g++.dg/modules/test-p1689.py
>   create mode 100644 gcc/testsuite/lib/modules.exp
>
>
> base-commit: 9d4c00cdaccc3decd07740e817387ce844ef3ac9

Re: [PATCH v5 0/5] P1689R5 support

[Ben Boeckel]

On Thu, Feb 02, 2023 at 21:24:12 +0100, Harald Anlauf wrote:
> Am 25.01.23 um 22:06 schrieb Ben Boeckel via Gcc-patches:
> > Hi,
> >
> > This patch series adds initial support for ISO C++'s [P1689R5][], a
> > format for describing C++ module requirements and provisions based on
> > the source code. This is required because compiling C++ with modules is
> > not embarrassingly parallel and need to be ordered to ensure that
> > `import some_module;` can be satisfied in time by making sure that any
> > TU with `export import some_module;` is compiled first.
> >
> > [P1689R5]: https://isocpp.org/files/papers/P1689R5.html
> 
> while that paper mentions Fortran, the patch in its present version
> does not seem to implement anything related to Fortran and does not
> touch the gfortran frontend.  Or am I missing anything?  Otherwise,
> could you give an example how it would be used with Fortran?

Correct. Still trying to put the walls back together after modules
KoolAid Man'd their way into the build graph structure :) . Being able
to drop our Fortran parser (well, we'd have to drop support for Fortran
compilers that exist today…so maybe in 2075 or something) and rely on
compilers to tell us the information would be amazing though :) .

FWIW, the initial revision of the patchset did touch the gfortran
frontend, but the new parameter is now defaulted and therefore the
callsite doesn't need an update anymore. I still thought it worthwhile
to keep the Fortran side aware of what is going on in the space.

The link to Fortran comes up because the build graph problem is
isomorphic (Fortran supports exporting multiple modules from a single
TU, but it's not relevant at the graph level; it's the zero -> any case
that is hard), CMake "solved" it already, and C++ is going to have a
*lot* more "I want to consume $other_project's modules using my favorite
compiler/flags" than seems to happen in Fortran. If you're interested,
this is the paper showing how we do it:

    https://mathstuf.fedorapeople.org/fortran-modules/fortran-modules.html

> Thus I'd say that it is OK from the gfortran side.

Eventually we'll like to get gfortran supporting this type of scanning,
but…as above.

Thanks,

--Ben

Re: [PATCH v5 0/5] P1689R5 support

[Andrew Pinski]

On Wed, Jan 25, 2023 at 1:07 PM Ben Boeckel via Fortran
<fortran@gcc.gnu.org> wrote:
>
> Hi,
>
> This patch series adds initial support for ISO C++'s [P1689R5][], a
> format for describing C++ module requirements and provisions based on
> the source code. This is required because compiling C++ with modules is
> not embarrassingly parallel and need to be ordered to ensure that
> `import some_module;` can be satisfied in time by making sure that any
> TU with `export import some_module;` is compiled first.


I like how folks are complaining that GCC outputs POSIX makefile
syntax from GCC's dependency files which are supposed to be in POSIX
Makefile syntax.
It seems like rather the build tools are people like to use are not
understanding POSIX makefile syntax any more rather.
Also I am not a fan of json, it is too verbose for no use. Maybe it is
time to go back to standardizing a new POSIX makefile syntax rather
than changing C++ here.

Thanks,
Andrew Pinski

>
> [P1689R5]: https://isocpp.org/files/papers/P1689R5.html
>
> I've also added patches to include imported module CMI files and the
> module mapper file as dependencies of the compilation. I briefly looked
> into adding dependencies on response files as well, but that appeared to
> need some code contortions to have a `class mkdeps` available before
> parsing the command line or to keep the information around until one was
> made.
>
> I'd like feedback on the approach taken here with respect to the
> user-visible flags. I'll also note that header units are not supported
> at this time because the current `-E` behavior with respect to `import
> <some_header>;` is to search for an appropriate `.gcm` file which is not
> something such a "scan" can support. A new mode will likely need to be
> created (e.g., replacing `-E` with `-fc++-module-scanning` or something)
> where headers are looked up "normally" and processed only as much as
> scanning requires.
>
> FWIW, Clang as taken an alternate approach with its `clang-scan-deps`
> tool rather than using the compiler directly.
>
> Thanks,
>
> --Ben
>
> ---
> v4 -> v5:
>
> - add dependency tracking for imported modules to `-MF`
> - add dependency tracking for static module mapper files given to
>   `-fmodule-mapper=`
>
> v3 -> v4:
>
> - add missing spaces between function names and arguments
>
> v2 -> v3:
>
> - changelog entries moved to commit messages
> - documentation updated/added in the UTF-8 routine editing
>
> v1 -> v2:
>
> - removal of the `deps_write(extra)` parameter to option-checking where
>   ndeeded
> - default parameter of `cpp_finish(fdeps_stream = NULL)`
> - unification of libcpp UTF-8 validity functions from v1
> - test cases for flag parsing states (depflags-*) and p1689 output
>   (p1689-*)
>
> Ben Boeckel (5):
>   libcpp: reject codepoints above 0x10FFFF
>   libcpp: add a function to determine UTF-8 validity of a C string
>   p1689r5: initial support
>   c++modules: report imported CMI files as dependencies
>   c++modules: report module mapper files as a dependency
>
>  gcc/c-family/c-opts.cc                        |  40 +++-
>  gcc/c-family/c.opt                            |  12 +
>  gcc/cp/mapper-client.cc                       |   4 +
>  gcc/cp/mapper-client.h                        |   1 +
>  gcc/cp/module.cc                              |  23 +-
>  gcc/doc/invoke.texi                           |  15 ++
>  gcc/testsuite/g++.dg/modules/depflags-f-MD.C  |   2 +
>  gcc/testsuite/g++.dg/modules/depflags-f.C     |   1 +
>  gcc/testsuite/g++.dg/modules/depflags-fi.C    |   3 +
>  gcc/testsuite/g++.dg/modules/depflags-fj-MD.C |   3 +
>  gcc/testsuite/g++.dg/modules/depflags-fj.C    |   4 +
>  .../g++.dg/modules/depflags-fjo-MD.C          |   4 +
>  gcc/testsuite/g++.dg/modules/depflags-fjo.C   |   5 +
>  gcc/testsuite/g++.dg/modules/depflags-fo-MD.C |   3 +
>  gcc/testsuite/g++.dg/modules/depflags-fo.C    |   4 +
>  gcc/testsuite/g++.dg/modules/depflags-j-MD.C  |   2 +
>  gcc/testsuite/g++.dg/modules/depflags-j.C     |   3 +
>  gcc/testsuite/g++.dg/modules/depflags-jo-MD.C |   3 +
>  gcc/testsuite/g++.dg/modules/depflags-jo.C    |   4 +
>  gcc/testsuite/g++.dg/modules/depflags-o-MD.C  |   2 +
>  gcc/testsuite/g++.dg/modules/depflags-o.C     |   3 +
>  gcc/testsuite/g++.dg/modules/modules.exp      |   1 +
>  gcc/testsuite/g++.dg/modules/p1689-1.C        |  18 ++
>  gcc/testsuite/g++.dg/modules/p1689-1.exp.json |  27 +++
>  gcc/testsuite/g++.dg/modules/p1689-2.C        |  16 ++
>  gcc/testsuite/g++.dg/modules/p1689-2.exp.json |  16 ++
>  gcc/testsuite/g++.dg/modules/p1689-3.C        |  14 ++
>  gcc/testsuite/g++.dg/modules/p1689-3.exp.json |  16 ++
>  gcc/testsuite/g++.dg/modules/p1689-4.C        |  14 ++
>  gcc/testsuite/g++.dg/modules/p1689-4.exp.json |  14 ++
>  gcc/testsuite/g++.dg/modules/p1689-5.C        |  14 ++
>  gcc/testsuite/g++.dg/modules/p1689-5.exp.json |  14 ++
>  gcc/testsuite/g++.dg/modules/test-p1689.py    | 222 ++++++++++++++++++
>  gcc/testsuite/lib/modules.exp                 |  71 ++++++
>  libcpp/charset.cc                             |  28 ++-
>  libcpp/include/cpplib.h                       |  12 +-
>  libcpp/include/mkdeps.h                       |  17 +-
>  libcpp/init.cc                                |  13 +-
>  libcpp/internal.h                             |   2 +
>  libcpp/mkdeps.cc                              | 149 +++++++++++-
>  40 files changed, 789 insertions(+), 30 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/modules/depflags-f-MD.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/depflags-f.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fi.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fj-MD.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fj.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fjo-MD.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fjo.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fo-MD.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/depflags-fo.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/depflags-j-MD.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/depflags-j.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/depflags-jo-MD.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/depflags-jo.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/depflags-o-MD.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/depflags-o.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/p1689-1.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/p1689-1.exp.json
>  create mode 100644 gcc/testsuite/g++.dg/modules/p1689-2.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/p1689-2.exp.json
>  create mode 100644 gcc/testsuite/g++.dg/modules/p1689-3.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/p1689-3.exp.json
>  create mode 100644 gcc/testsuite/g++.dg/modules/p1689-4.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/p1689-4.exp.json
>  create mode 100644 gcc/testsuite/g++.dg/modules/p1689-5.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/p1689-5.exp.json
>  create mode 100644 gcc/testsuite/g++.dg/modules/test-p1689.py
>  create mode 100644 gcc/testsuite/lib/modules.exp
>
>
> base-commit: 9d4c00cdaccc3decd07740e817387ce844ef3ac9
> --
> 2.39.0
>

Re: [PATCH v5 0/5] P1689R5 support

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 1201 bytes --]

On Fri, 3 Feb 2023, 04:09 Andrew Pinski via Gcc, <gcc@gcc.gnu.org> wrote:

> On Wed, Jan 25, 2023 at 1:07 PM Ben Boeckel via Fortran
> <fortran@gcc.gnu.org> wrote:
> >
> > Hi,
> >
> > This patch series adds initial support for ISO C++'s [P1689R5][], a
> > format for describing C++ module requirements and provisions based on
> > the source code. This is required because compiling C++ with modules is
> > not embarrassingly parallel and need to be ordered to ensure that
> > `import some_module;` can be satisfied in time by making sure that any
> > TU with `export import some_module;` is compiled first.
>
>
> I like how folks are complaining that GCC outputs POSIX makefile
> syntax from GCC's dependency files which are supposed to be in POSIX
> Makefile syntax.
> It seems like rather the build tools are people like to use are not
> understanding POSIX makefile syntax any more rather.
> Also I am not a fan of json, it is too verbose for no use. Maybe it is
> time to go back to standardizing a new POSIX makefile syntax rather
> than changing C++ here.
>


That would take a decade or more. It's too late for POSIX 202x and the pace
that POSIX agrees on makefile features is incredibly slow.

Re: [PATCH v5 0/5] P1689R5 support

[Jonathan Wakely]

On Fri, 3 Feb 2023 at 08:58, Jonathan Wakely wrote:
>
>
>
> On Fri, 3 Feb 2023, 04:09 Andrew Pinski via Gcc, <gcc@gcc.gnu.org> wrote:
>>
>> On Wed, Jan 25, 2023 at 1:07 PM Ben Boeckel via Fortran
>> <fortran@gcc.gnu.org> wrote:
>> >
>> > Hi,
>> >
>> > This patch series adds initial support for ISO C++'s [P1689R5][], a
>> > format for describing C++ module requirements and provisions based on
>> > the source code. This is required because compiling C++ with modules is
>> > not embarrassingly parallel and need to be ordered to ensure that
>> > `import some_module;` can be satisfied in time by making sure that any
>> > TU with `export import some_module;` is compiled first.
>>
>>
>> I like how folks are complaining that GCC outputs POSIX makefile
>> syntax from GCC's dependency files which are supposed to be in POSIX
>> Makefile syntax.
>> It seems like rather the build tools are people like to use are not
>> understanding POSIX makefile syntax any more rather.
>> Also I am not a fan of json, it is too verbose for no use. Maybe it is
>> time to go back to standardizing a new POSIX makefile syntax rather
>> than changing C++ here.
>
>
>
> That would take a decade or more. It's too late for POSIX 202x and the pace that POSIX agrees on makefile features is incredibly slow.

Also, name+=value is *not* POSIX make syntax today, that's an
extension. That's why the tools don't always support it.
So I don't think it's true that GCC's dependency files are in POSIX syntax.

POSIX 202x does add support for it, but it will take some time for it
to be supported everywhere.

Re: [PATCH v5 0/5] P1689R5 support

[Ben Boeckel]

On Fri, Feb 03, 2023 at 09:10:21 +0000, Jonathan Wakely wrote:
> On Fri, 3 Feb 2023 at 08:58, Jonathan Wakely wrote:
> > On Fri, 3 Feb 2023, 04:09 Andrew Pinski via Gcc, <gcc@gcc.gnu.org> wrote:
> >> On Wed, Jan 25, 2023 at 1:07 PM Ben Boeckel via Fortran
> >> <fortran@gcc.gnu.org> wrote:
> >> > This patch series adds initial support for ISO C++'s [P1689R5][], a
> >> > format for describing C++ module requirements and provisions based on
> >> > the source code. This is required because compiling C++ with modules is
> >> > not embarrassingly parallel and need to be ordered to ensure that
> >> > `import some_module;` can be satisfied in time by making sure that any
> >> > TU with `export import some_module;` is compiled first.
> >>
> >> I like how folks are complaining that GCC outputs POSIX makefile
> >> syntax from GCC's dependency files which are supposed to be in POSIX
> >> Makefile syntax.
> >> It seems like rather the build tools are people like to use are not
> >> understanding POSIX makefile syntax any more rather.
> >> Also I am not a fan of json, it is too verbose for no use. Maybe it is
> >> time to go back to standardizing a new POSIX makefile syntax rather
> >> than changing C++ here.

I'm not complaining that dependency files are in POSIX (or even
POSIX-to-be) syntax. The information requires a bit more structure than
some variable assignments and I don't expect anything trying to read
them to start trying to understand `VAR_$(DEREF)=` and the behaviors of
`:=` versus `=` assignment to get this reliably.

> > That would take a decade or more. It's too late for POSIX 202x and
> > the pace that POSIX agrees on makefile features is incredibly slow.
> 
> Also, name+=value is *not* POSIX make syntax today, that's an
> extension. That's why the tools don't always support it.
> So I don't think it's true that GCC's dependency files are in POSIX syntax.
> 
> POSIX 202x does add support for it, but it will take some time for it
> to be supported everywhere.

Additionally, while the *syntax* might be supported, encoding all of
P1689 in it would require additional work (e.g., key/value variable
assignments or something). Batch scanning would also be…interesting.
Also note that the imported modules' location cannot be known before
scanning in general, so all you get are "logical names" that you need a
collator to link up with other scan results anyways. Tools such as
`make` and `ninja` cannot know, in general, how to do this linking
between arbitrary targets (e.g., there may be a debug and release build
of the same module in the graph and knowing which to use requires
higher-level info about the entire build graph; modules may also be
considered "private" and not accessible everywhere and therefore should
also not be hooked up across different target boundaries).

While the `CXX_MODULES +=` approach can work for simple cases (a
pseudo-implicit build), it is quite insufficient for the general case.

--Ben