From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45835 invoked by alias); 29 Jun 2019 10:15:33 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 45808 invoked by uid 89); 29 Jun 2019 10:15:32 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-7.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,KAM_SHORT,T_FILL_THIS_FORM_SHORT autolearn=ham version=3.3.1 spammy=rabbit, hole, ChangeLogs, Mind X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.110.172) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 29 Jun 2019 10:15:24 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 60BF22B; Sat, 29 Jun 2019 03:15:19 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.39]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id DE4753F706; Sat, 29 Jun 2019 03:15:18 -0700 (PDT) From: Richard Sandiford To: Gaius Mulley Mail-Followup-To: Gaius Mulley ,gcc-patches@gcc.gnu.org, richard.sandiford@arm.com Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH, Modula-2 (C/C++/D/F/Go/Jit)] (Register spec fn) (v2) References: <87k1doxqhv.fsf@j228-gm.comp.glam.ac.uk> Date: Sat, 29 Jun 2019 10:15:00 -0000 In-Reply-To: <87k1doxqhv.fsf@j228-gm.comp.glam.ac.uk> (Gaius Mulley's message of "Fri, 14 Jun 2019 14:09:48 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2019-06/txt/msg01885.txt.bz2 Thanks for the patch and sorry for the slow reply. Gaius Mulley writes: > Hello, > > here is version two of the patches which introduce Modula-2 into the > GCC trunk. The patches include: > > (*) a patch to allow all front ends to register a lang spec function. > (included are patches for all front ends to provide an empty > callback function). > (*) patch diffs to allow the Modula-2 front end driver to be > built using GCC Makefile and friends. > > The compressed tarball includes: > > (*) gcc/m2 (compiler driver and lang-spec stuff for Modula-2). > Including the need for registering lang spec functions. > (*) gcc/testsuite/gm2 (a Modula-2 dejagnu test to ensure that > the gm2 driver is built and can understands --version). > > These patches have been re-written after taking on board the comments > found in this thread: > > https://gcc.gnu.org/ml/gcc-patches/2013-11/msg02620.html Echoing a point Joseph made there: does this approach to linking work with LTO, especially given the auto-generated main module? I don't think that should hold up the patch, just curious. > it is a revised patch set from: > > https://gcc.gnu.org/ml/gcc-patches/2019-06/msg00220.html > > I've run make bootstrap and run the regression tests on trunk and no > extra failures occur for all languages touched in the ChangeLog. > > I'm currently tracking gcc trunk and gcc-9 with gm2 (which works well > with amd64/arm64/i386) - these patches are currently simply for the > driver to minimise the patch size. There are also > 1800 tests in a > dejagnu testsuite for gm2 which can be included at some future time. It looks like the current GCC driver code is a bit of a half-transition towards a more C++ way of doing things. If we were going to embrace that fully, I guess each frontend driver would get the opportunity to subclass "driver" and override functions where appropriate. It then wouldn't be necessary to add each new hook to every frontend driver (and force other out-of-tree frontends to do the same). But that isn't how things work now, and going down that particular rabbit hole shouldn't be a requirement for this patch. So IMO the approach you're taking is fine. The same goes for the new functions that you're exporting. Ideally they'd be (protected?) member functions of "driver", but the internal callers are at points where the driver object is no longer to hand. Again, exposing functions follows existing practice so IMO is fine. > Here are the proposed patches and ChangeLogs and new files (gm2-v2.tar.gz) > (after the patches): > > ./ChangeLog > Sorry for the changelog nits, but: > 14-06-2019 Gaius Mulley Should be: 2019-06-14 Should only be two spaces between your name and email address. (Not that I care, I just work here :-)) > * configure.ac (GM2_FOR_BUILD): Added. > (GM2_FOR_TARGET): Added. > Request build driver program gm2. Lines should be indented to under the "*" rather than two spaces beyond. > * Makefile.def (GM2_FOR_TARGET): Added. > (GM2FLAGS_FOR_TARGET): Added. Assign GM2, > GM2_FOR_BUILD, GM2_FOR_TARGET and GM2FLAGS. > Pass variables to make. Add new language Modula-2 > (m2). > * Makefile.tpl (GM2FLAGS): Added. (GM2) Added. > (GM2_FOR_BUILD) Added. Missing newline before "GM2". Looks like some of the Makefile.def entries belong in Makefile.tpl instead. E.g. GM2_FOR_BUILD is only mentioned in Makefile.tpl. Reordering things slightly... > @@ -1655,6 +1659,10 @@ > { 0, 0 } > }; > > +/* front end registered spec functions */ > +static struct spec_function *lang_spec_functions = NULL; > +static unsigned int lang_spec_functions_length = 0; > + > static int processing_spec_function; > > /* Add appropriate libgcc specs to OBSTACK, taking into account > [...] > +/* Allow the front end to register a spec function. */ > + > +void > +fe_add_spec_function (const char *name, const char *(*func) (int, const char **)) > +{ > + const struct spec_function *f = lookup_spec_function (name); > + struct spec_function *fl; > + unsigned int i; > + > + if (f != NULL) > + fatal_error (input_location, "spec function (%s) already registered", name); > + > + if (lang_spec_functions == NULL) > + lang_spec_functions_length = 1; > + > + lang_spec_functions_length++; > + fl = (struct spec_function *) xmalloc (sizeof (const struct spec_function)*lang_spec_functions_length); > + for (i=0; i + fl[i] = lang_spec_functions[i]; > + free (lang_spec_functions); > + lang_spec_functions = fl; > + > + lang_spec_functions[lang_spec_functions_length-2].name = name; > + lang_spec_functions[lang_spec_functions_length-2].func = func; > + lang_spec_functions[lang_spec_functions_length-1].name = NULL; > + lang_spec_functions[lang_spec_functions_length-1].func = NULL; > +} > + This would be simpler if you make lang_spec_functions a vec. > @@ -3725,6 +3733,70 @@ > setenv ("SOURCE_DATE_EPOCH", source_date_epoch, 0); > } > > +/* Save an option OPT with N_ARGS arguments in array ARGS, marking it > + as validated if VALIDATED. */ > + > +void > +fe_save_switch (const char *opt, size_t n_args, const char *const *args, > + bool validated, bool known) > +{ > + save_switch (opt, n_args, args, validated, known); > +} > + > +/* fe_B_prefix allow the front end to add its own -B option. */ > + > +void > +fe_B_prefix (const char *arg) > +{ > + handle_OPT_B (arg); > +} It'd be simpler to expose save_switch and handle_OPT_B directly rather than use wrappers. The "fe_" prefix isn't used for other public functions. > + > +/* Handle the -B option by adding the prefix to exec, startfile and > + include search paths. */ > + > +static > +void handle_OPT_B (const char *arg) Formatting, should be: static void handle_OPT_B (const char *arg) (although I guess no longer static after the above). > +/* Mark a source file as compiled. */ > + > +void > +fe_remove_infile (const char *name) > +{ > + int max = n_infiles + lang_specific_extra_outfiles; > + int i; > + > + for (i = 0; i < max; i++) > + if (filename_cmp (name, infiles[i].name) == 0) > + infiles[i].compiled = true; > +} AFAICT this is only used here: ------------------------------------------------------------------------ /* no_objects return an empty string, but also remove all objects from the command line. */ extern void fe_remove_infile (const char *); static const char * no_objects (int argc ATTRIBUTE_UNUSED, const char *argv[] ATTRIBUTE_UNUSED) { object_list *o; for (o = head_objects; o != NULL; o = o->next) fe_remove_infile (o->name); return NULL; } ------------------------------------------------------------------------ which looks unnecessarily quadratic. Is there a simpler way to do this? Either way, the name fe_remove_infile seems inconsistent with the comment and with what the code actually does. > --- gcc-versionno-orig/gcc/fortran/gfortranspec.c 2019-05-28 22:27:32.773238257 +0100 > +++ gcc-versionno/gcc/fortran/gfortranspec.c 2019-05-29 12:10:01.323261736 +0100 > @@ -448,3 +448,9 @@ > > /* Number of extra output files that lang_specific_pre_link may generate. */ > int lang_specific_extra_outfiles = 0; /* Not used for F77. */ > + > +/* lang_register_spec_functions. Not used for F77. */ > +void > +lang_register_spec_functions (void) > +{ > +} Heh, the (existing) F77 comments look a bit of out of date :-) Some comments about the tar file (wasn't sure whether this was part of the submission yet, or whether you were just including it because of Joseph's request in the earlier thread): All in-tree configure files are now called configure.ac, so it would be good if gm2 did the same. ------------------------------------------------------------------------ AC_INIT(gm2config.h.in, 1.9.1, gm2@nongnu.org) ------------------------------------------------------------------------ Would this stay the same after the merge, or would the Savannah version of gm2 no longer be maintained? ------------------------------------------------------------------------ #ifndef MATH_LIBRARY_PROFILE #define MATH_LIBRARY_PROFILE MATH_LIBRARY #endif #ifndef LIBSTDCXX #define LIBSTDCXX "stdc++" #endif #ifndef LIBSTDCXX_PROFILE #define LIBSTDCXX_PROFILE LIBSTDCXX #endif #ifndef LIBSTDCXX_STATIC #define LIBSTDCXX_STATIC NULL #endif ------------------------------------------------------------------------ These macros in gm2spec.c don't seem to be used. "stdc++" in particular is hard-coded further down. ------------------------------------------------------------------------ /* assert, a simple assertion function. */ static void assert (int b) { if (!b) { printf ("assert failed in gm2spec.c"); exit (1); } } ------------------------------------------------------------------------ Better to use gcc_assert. ------------------------------------------------------------------------ #if defined(DEBUGGING) static void printOption (const char *desc, struct cl_decoded_option **in_decoded_options, int i) { ------------------------------------------------------------------------ Realise it's only a debugging function, but should be print_option. ------------------------------------------------------------------------ /* The last entry in libraryName must be the longest string in the list. This is the -flibs=name. */ static const char *libraryName[maxlib] = { "iso", "pim", "ulm", "min", "log", "cor" }; ------------------------------------------------------------------------ Some comments like this aren't properly indented (but most are). Mind having a quick scan and fix? Realise it'll be a bit tedious, sorry. Why does the last entry need to be the longest? It only seems to be used in a strcmp loop, and the list is separated by commas. The corresponding option documentation says: ------------------------------------------------------------------------ flibs= Modula-2 Joined specify the library order, currently legal entries include: logitech, min, pim-coroutine, ulm, iso ------------------------------------------------------------------------ but it sounds like the actual values are: log, min, pim, ulm and iso, is that right? Would be worth clarifying the documentation line if so. ------------------------------------------------------------------------ /* get_archive_name, return the corresponding archive name given the library name. */ static const char * get_archive_name (const char *library) { libs i; for (i = iso; i < maxlib; i = (libs) ((int)i + 1)) if (strcmp (libraryName[i], library) == 0) return archiveName[i]; return NULL; } /* build_archive, returns a string containing the a path to the archive defined by, libpath, s, and, dialectLib. */ static char * build_archive (const char *library) { if (library != NULL) { const char *a = get_archive_name (library); if (a != NULL) { char *s = (char *)xmalloc (strlen (a) + 1); strcpy (s, a); return s; } } return NULL; } ------------------------------------------------------------------------ The comment above build_archive seems to be a bit mangled. It looks like this code silently ignores unrecognised -flibs= options. Is that the intended behaviour? I couldn't see where the valid values were enforced. ------------------------------------------------------------------------ else if (gm2_prefix != NULL && !seen_fmakeall0) /* no need to issue a warning if seen_fmakeall0 as the parent will have set COMPILER_PATH and LIBRARY_PATH because of GM2_ROOT and users should not be using -fmakeall0 as it is an internal option. */ fprintf (stderr, "warning it is not advisible to set " GM2_PREFIX_ENV " as well as either " LIBRARY_PATH_ENV " or COMPILER_PATH\n"); ------------------------------------------------------------------------ This should be a proper warning, using the diagnostic machinery. Posting the tar file definitely helped to show how these things are used. But one thing I'm still not sure about is why gm2 needs it's own -B-related code, and why for example it needs: ------------------------------------------------------------------------ /* find_executable_path, if argv0 references an executable filename then use this path. */ static const char * find_executable_path (const char *argv0) { if (access (argv0, X_OK) == 0) { const char *n = strrchr (argv0, DIR_SEPARATOR); /* strip off the program name from argv0, but leave the DIR_SEPARATOR. */ if (n != NULL) { char *copy = xstrdup (argv0); char *n = strrchr (copy, DIR_SEPARATOR); n[1] = (char)0; return copy; } } return NULL; } ------------------------------------------------------------------------ GCC supports a lot of (maybe too many?) different installation layouts, so it would be good to reuse the existing search code and self-relocation code if at all possible. That might mean exposing more functions to the frontend driver, but IMO that'd still be preferable. Thanks, Richard