Re: [PATCH 02/14] Add D frontend (GDC) implementation.

[David Malcolm <dmalcolm@redhat.com>]

On Tue, 2018-09-18 at 02:33 +0200, Iain Buclaw wrote:
> This patch adds the D front-end implementation, the only part of the
> compiler that interacts with GCC directly, and being the parts that I
> maintain, is something that I can talk about more directly.
> 
> For the actual code generation pass, that converts the front-end AST
> to GCC trees, most parts use a separate Visitor interfaces to do a
> certain kind of lowering, for instance, types.cc builds *_TYPE trees
> from AST Type's.  The Visitor class is part of the DMD front-end, and
> is defined in dfrontend/visitor.h.
> 
> There are also a few interfaces which have their headers in the DMD
> frontend, but are implemented here because they do something that
> requires knowledge of the GCC backend (d-target.cc), does something
> that may not be portable, or differ between D compilers
> (d-frontend.cc) or are a thin wrapper around something that is
> managed
> by GCC (d-diagnostic.cc).
> 
> Many high level operations result in generation of calls to D runtime
> library functions (runtime.def), all with require some kind of
> runtime
> type information (typeinfo.cc).  The compiler also generates
> functions
> for registering/deregistering compiled modules with the D runtime
> library (modules.cc).
> 
> As well as the D language having it's own built-in functions
> (intrinsics.cc), we also expose GCC builtins to D code via a
> `gcc.builtins' module (d-builtins.cc), and give special treatment to
> a
> number of UDAs that could be applied to functions (d-attribs.cc).
> 
> 
> That is roughly the high level jist of how things are currently
> organized.
> 
> ftp://ftp.gdcproject.org/patches/v4/02-v4-d-frontend-gdc.patch

Hi Iain.  I took at look at this patch, focusing on the diagnostics
side of things.

These are more suggestions than hard review blockers.

diff --git a/gcc/d/d-attribs.c b/gcc/d/d-attribs.c
new file mode 100644
index 00000000000..6c65b8cad9e
--- /dev/null
+++ b/gcc/d/d-attribs.c

I believe all new C++ source files are meant to be .cc, rather than .c,
so this should be d-attribs.cc, rather that d-attribs.c.

[...snip...]

diff --git a/gcc/d/d-codegen.cc b/gcc/d/d-codegen.cc
new file mode 100644
index 00000000000..c698890ba07
--- /dev/null
+++ b/gcc/d/d-codegen.cc

[...snip...]

+/* Return the GCC location for the D frontend location LOC.   */
+
+location_t
+get_linemap (const Loc& loc)
+{

I don't like the name "get_linemap", as it suggests to me that it's
getting the "struct line_map *" for LOC, rather than a location_t.

How about "get_location_t" instead, or "make_location_t"?  The latter
feels more appropriate, as it's doing non-trivial work.

+  location_t gcc_location = input_location;
+
+  if (loc.filename)
+    {
+      linemap_add (line_table, LC_ENTER, 0, loc.filename, loc.linnum);
+      linemap_line_start (line_table, loc.linnum, 0);
+      gcc_location = linemap_position_for_column (line_table, loc.charnum);
+      linemap_add (line_table, LC_LEAVE, 0, NULL, 0);
+    }
+
+  return gcc_location;
+}

[...snip...]

diff --git a/gcc/d/d-diagnostic.cc b/gcc/d/d-diagnostic.cc
new file mode 100644
index 00000000000..8b8a21e31b6
--- /dev/null
+++ b/gcc/d/d-diagnostic.cc
@@ -0,0 +1,347 @@
+/* d-diagnostics.cc -- D frontend interface to gcc diagnostics.
+   Copyright (C) 2017-2018 Free Software Foundation, Inc.
+
+GCC 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, or (at your option)
+any later version.
+
+GCC 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/>.  */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+
+#include "dmd/globals.h"
+#include "dmd/errors.h"
+
+#include "tree.h"
+#include "options.h"
+#include "diagnostic.h"
+
+#include "d-tree.h"
+
+
+/* Rewrite the format string FORMAT to deal with any format extensions not
+   supported by pp_format().  The result should be freed by the caller.  */
+static char *
+expand_format (const char *format)

Am I right in thinking this is to handle FORMAT strings coming from the
upstream D frontend, and this has its own formatting conventions?
Please can the leading comment have example(s) of the format, and what
it becomes (e.g. the backticks thing).

Maybe adopt a naming convention in the file, to distinguish d format
strings from pp format strings?  Maybe "d_format" vs "gcc_format"
?(though given the verbatim vs !verbatim below, am not sure how
feasible that is).

Maybe rename this function to expand_d_format??

(Might be nice to add some unittesting of this function via "selftest",
but that's definitely not a requirement; it's awkward to add right now)

+{
+  OutBuffer buf;
+  bool inbacktick = false;
+
+  for (const char *p = format; *p; )
+    {
+      while (*p != '\0' && *p != '%' && *p != '`')
+	{
+	  buf.writeByte (*p);
+	  p++;
+	}
+
+      if (*p == '\0')
+	break;
+
+      if (*p == '`')
+	{
+	  /* Text enclosed by `...` are translated as a quoted string.  */
+	  if (inbacktick)
+	    {
+	      buf.writestring ("%>");
+	      inbacktick = false;
+	    }
+	  else
+	    {
+	      buf.writestring ("%<");
+	      inbacktick = true;
+	    }
+	  p++;
+	  continue;
+	}
+
+      /* Check the conversion specification for unhandled flags.  */
+      buf.writeByte (*p);
+      p++;
+
+    Lagain:
+      switch (*p)
+	{
+	case '\0':
+	  /* Malformed format string.  */
+	  gcc_unreachable ();
+
+	case '-':
+	  /* Remove whitespace formatting. */
+	  p++;
+	  while (ISDIGIT (*p))
+	    p++;
+	  goto Lagain;
+
+	case '0':
+	  /* Remove zero padding from format string.  */
+	  while (ISDIGIT (*p))
+	    p++;
+	  goto Lagain;
+
+	case 'X':
+	  /* Hex format only supports lower-case.  */
+	  buf.writeByte ('x');
+	  p++;
+	  break;
+
+	default:
+	  break;
+	}
+    }
+
+  gcc_assert (!inbacktick);
+  return buf.extractString ();
+}
+
+/* Helper routine for all error routines.  Reports a diagnostic specified by
+   KIND at the explicit location LOC, where the message FORMAT has not yet
+   been translated by the gcc diagnostic routines.  */
+
+static void ATTRIBUTE_GCC_DIAG(3,0)
+d_diagnostic_report_diagnostic (const Loc& loc, int opt, const char *format,
+				va_list ap, diagnostic_t kind, bool verbatim)

Given the call to expand_format for the !verbatim case, it seems that
ATTRIBUTE_GCC_DIAG isn't necessarily quite right here, as FORMAT would
then be a D format string.  But it's probably close enough for now.

+{
+  va_list argp;
+  va_copy (argp, ap);
+
+  if (loc.filename || !verbatim)
+    {
+      rich_location rich_loc (line_table, get_linemap (loc));
+      diagnostic_info diagnostic;
+      char *xformat = expand_format (format);
+
+      diagnostic_set_info (&diagnostic, xformat, &argp, &rich_loc, kind);
+      if (opt != 0)
+	diagnostic.option_index = opt;
+
+      diagnostic_report_diagnostic (global_dc, &diagnostic);
+      free (xformat);
+    }
+  else
+    {
+      /* Write verbatim messages with no location direct to stream.  */
+      text_info text;
+      text.err_no = errno;
+      text.args_ptr = &argp;
+      text.format_spec = expand_format (format);
+      text.x_data = NULL;
+
+      pp_format_verbatim (global_dc->printer, &text);
+      pp_newline_and_flush (global_dc->printer);
+    }
+
+  va_end (argp);
+}
+
+/* Print a hard error message with explicit location LOC, increasing the
+   global or gagged error count.  */
+
+void ATTRIBUTE_GCC_DIAG(2,3)
+error (const Loc& loc, const char *format, ...)
+{
+  va_list ap;
+  va_start (ap, format);
+  verror (loc, format, ap);
+  va_end (ap);
+}
+
+void ATTRIBUTE_GCC_DIAG(2,0)
+verror (const Loc& loc, const char *format, va_list ap,
+	const char *p1, const char *p2, const char *)

This one needs a leading comment: what's the deal with P1 and P2?

+{
+  if (!global.gag || global.params.showGaggedErrors)
+    {
+      char *xformat;
+
+      /* Build string and emit.  */
+      if (p2 != NULL)
+	xformat = xasprintf ("%s %s %s", p1, p2, format);
+      else if (p1 != NULL)
+	xformat = xasprintf ("%s %s", p1, format);
+      else
+	xformat = xasprintf ("%s", format);
+
+      d_diagnostic_report_diagnostic (loc, 0, xformat, ap,
+				      global.gag ? DK_ANACHRONISM : DK_ERROR,
+				      false);
+      free (xformat);
+    }
+
+  if (global.gag)
+    global.gaggedErrors++;
+
+  global.errors++;
+}
+
+/* Print supplementary message about the last error with explicit location LOC.
+   This doesn't increase the global error count.  */
+
+void ATTRIBUTE_GCC_DIAG(2,3)
+errorSupplemental (const Loc& loc, const char *format, ...)
+{
+  va_list ap;
+  va_start (ap, format);
+  verrorSupplemental (loc, format, ap);
+  va_end (ap);
+}
+
+void ATTRIBUTE_GCC_DIAG(2,0)
+verrorSupplemental (const Loc& loc, const char *format, va_list ap)
+{
+  if (global.gag && !global.params.showGaggedErrors)
+    return;
+
+  d_diagnostic_report_diagnostic (loc, 0, format, ap, DK_NOTE, false);
+}
+
+/* Print a warning message with explicit location LOC, increasing the
+   global warning count.  */
+
+void ATTRIBUTE_GCC_DIAG(2,3)
+warning (const Loc& loc, const char *format, ...)
+{
+  va_list ap;
+  va_start (ap, format);
+  vwarning (loc, format, ap);
+  va_end (ap);
+}
+
+void ATTRIBUTE_GCC_DIAG(2,0)
+vwarning (const Loc& loc, const char *format, va_list ap)
+{
+  if (global.params.warnings && !global.gag)
+    {
+      /* Warnings don't count if gagged.  */
+      if (global.params.warnings == 1)

What's the magic "1" value above?  Can it be an enum?  (or is this
something from the upstream parsing code?)

+	global.warnings++;
+
+      d_diagnostic_report_diagnostic (loc, 0, format, ap, DK_WARNING, false);
+    }
+}
+
+/* Print supplementary message about the last warning with explicit location
+   LOC.  This doesn't increase the global warning count.  */
+
+void ATTRIBUTE_GCC_DIAG(2,3)
+warningSupplemental (const Loc& loc, const char *format, ...)
+{
+  va_list ap;
+  va_start (ap, format);
+  vwarningSupplemental (loc, format, ap);
+  va_end (ap);
+}
+
+void ATTRIBUTE_GCC_DIAG(2,0)
+vwarningSupplemental (const Loc& loc, const char *format, va_list ap)
+{
+  if (!global.params.warnings || global.gag)
+    return;
+
+  d_diagnostic_report_diagnostic (loc, 0, format, ap, DK_NOTE, false);
+}
+
+/* Print a deprecation message with explicit location LOC, increasing the
+   global warning or error count depending on how deprecations are treated.  */
+
+void ATTRIBUTE_GCC_DIAG(2,3)
+deprecation (const Loc& loc, const char *format, ...)
+{
+  va_list ap;
+  va_start (ap, format);
+  vdeprecation (loc, format, ap);
+  va_end (ap);
+}
+
+void ATTRIBUTE_GCC_DIAG(2,0)
+vdeprecation (const Loc& loc, const char *format, va_list ap,
+	      const char *p1, const char *p2)
+{

Again: please can this have a leading comment explaining P1 and P2.

+  if (global.params.useDeprecated == 0)
+    verror (loc, format, ap, p1, p2);
+  else if (global.params.useDeprecated == 2 && !global.gag)

Can these "0" and "2" values be enums?

+    {
+      char *xformat;
+
+      /* Build string and emit.  */
+      if (p2 != NULL)
+	xformat = xasprintf ("%s %s %s", p1, p2, format);
+      else if (p1 != NULL)
+	xformat = xasprintf ("%s %s", p1, format);
+      else
+	xformat = xasprintf ("%s", format);
+
+      d_diagnostic_report_diagnostic (loc, OPT_Wdeprecated, xformat, ap,
+				      DK_WARNING, false);
+      free (xformat);
+    }
+}
+
+/* Print supplementary message about the last deprecation with explicit
+   location LOC.  This does not increase the global error count.  */
+
+void ATTRIBUTE_GCC_DIAG(2,3)
+deprecationSupplemental (const Loc& loc, const char *format, ...)
+{
+  va_list ap;
+  va_start (ap, format);
+  vdeprecationSupplemental (loc, format, ap);
+  va_end (ap);
+}
+
+void ATTRIBUTE_GCC_DIAG(2,0)
+vdeprecationSupplemental (const Loc& loc, const char *format, va_list ap)
+{
+  if (global.params.useDeprecated == 0)
+    verrorSupplemental (loc, format, ap);
+  else if (global.params.useDeprecated == 2 && !global.gag)
+    d_diagnostic_report_diagnostic (loc, 0, format, ap, DK_NOTE, false);

As before for the "0" and "2".

+}
+
+/* Print a verbose message with explicit location LOC.  */
+
+void ATTRIBUTE_GCC_DIAG(2, 3)
+message (const Loc& loc, const char *format, ...)
+{
+  va_list ap;
+  va_start (ap, format);
+  vmessage (loc, format, ap);
+  va_end (ap);
+}
+
+void ATTRIBUTE_GCC_DIAG(2,0)
+vmessage(const Loc& loc, const char *format, va_list ap)
+{
+  d_diagnostic_report_diagnostic (loc, 0, format, ap, DK_NOTE, true);
+}
+
+/* Same as above, but doesn't take a location argument.  */
+
+void ATTRIBUTE_GCC_DIAG(1, 2)
+message (const char *format, ...)
+{
+  va_list ap;
+  va_start (ap, format);
+  vmessage (Loc (), format, ap);
+  va_end (ap);
+}
+
+/* Call this after printing out fatal error messages to clean up and
+   exit the compiler.  */
+
+void
+fatal (void)
+{
+  exit (FATAL_EXIT_CODE);
+}
[...snip...]

diff --git a/gcc/d/d-spec.c b/gcc/d/d-spec.c
new file mode 100644
index 00000000000..6a68ebbcf24
--- /dev/null
+++ b/gcc/d/d-spec.c

As above, this new C++ file should be .cc, rather than .c.

[...snip...]

Hope this is constructive
Dave