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
