Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On 5 October 2012 11:35, Richard Guenther richard.guent...@gmail.com wrote: On Fri, Oct 5, 2012 at 12:07 PM, Iain Buclaw ibuc...@ubuntu.com wrote: On 5 October 2012 01:06, Joseph S. Myers jos...@codesourcery.com wrote: On Thu, 4 Oct 2012, Iain Buclaw wrote: The only patches to gcc proper are documentation-related and adding the D frontend / libphobos to configure and make files. I would have thought that these would typically only be included with the actual front-end? Looking back at my previous review comments, I suggested that you might need to split up c-common.[ch] so that certain parts of attribute handling could be shared with D, because duplicate code copied from elsewhere in GCC was not an appropriate implementation approach. Have you then eliminated the duplicate code in some other way that does not involve splitting up those files so code can be shared? Ah, no; thanks for reminding me of this. The code duplicated from c-common.[ch] are the handlers for C __attributes__, however gdc doesn't use all of them because some just don't have a fitting place eg: gnu_inline, artificial. Would the best approach be to move all handle_* functions and any helper functions into a new source file that can be shared between frontends, and define two new frontend hooks, LANG_HOOK_ATTRIBUTE_TABLE and LANG_HOOK_FORMAT_ATTRIBUTE_TABLE ? Btw, the LTO frontend also has most of the stuff duplicated ... (see lto/lto-lang.c). Not sure why ... Richard. Looks like LTO's frontend has the relevant attributes duplicated in order to support the attributes used for GCC builtins (const, pure, nothrow, transaction_pure, etc...). Probably only these handlers that could move to a common frontend location, and keep the rest as part of c-family. Regards, -- Iain Buclaw *(p e ? p++ : p) = (c 0x0f) + '0';
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On 5 October 2012 01:06, Joseph S. Myers jos...@codesourcery.com wrote: On Thu, 4 Oct 2012, Iain Buclaw wrote: The only patches to gcc proper are documentation-related and adding the D frontend / libphobos to configure and make files. I would have thought that these would typically only be included with the actual front-end? Looking back at my previous review comments, I suggested that you might need to split up c-common.[ch] so that certain parts of attribute handling could be shared with D, because duplicate code copied from elsewhere in GCC was not an appropriate implementation approach. Have you then eliminated the duplicate code in some other way that does not involve splitting up those files so code can be shared? Ah, no; thanks for reminding me of this. The code duplicated from c-common.[ch] are the handlers for C __attributes__, however gdc doesn't use all of them because some just don't have a fitting place eg: gnu_inline, artificial. Would the best approach be to move all handle_* functions and any helper functions into a new source file that can be shared between frontends, and define two new frontend hooks, LANG_HOOK_ATTRIBUTE_TABLE and LANG_HOOK_FORMAT_ATTRIBUTE_TABLE ? Regards -- Iain Buclaw *(p e ? p++ : p) = (c 0x0f) + '0';
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On Fri, Oct 5, 2012 at 12:07 PM, Iain Buclaw ibuc...@ubuntu.com wrote: On 5 October 2012 01:06, Joseph S. Myers jos...@codesourcery.com wrote: On Thu, 4 Oct 2012, Iain Buclaw wrote: The only patches to gcc proper are documentation-related and adding the D frontend / libphobos to configure and make files. I would have thought that these would typically only be included with the actual front-end? Looking back at my previous review comments, I suggested that you might need to split up c-common.[ch] so that certain parts of attribute handling could be shared with D, because duplicate code copied from elsewhere in GCC was not an appropriate implementation approach. Have you then eliminated the duplicate code in some other way that does not involve splitting up those files so code can be shared? Ah, no; thanks for reminding me of this. The code duplicated from c-common.[ch] are the handlers for C __attributes__, however gdc doesn't use all of them because some just don't have a fitting place eg: gnu_inline, artificial. Would the best approach be to move all handle_* functions and any helper functions into a new source file that can be shared between frontends, and define two new frontend hooks, LANG_HOOK_ATTRIBUTE_TABLE and LANG_HOOK_FORMAT_ATTRIBUTE_TABLE ? Btw, the LTO frontend also has most of the stuff duplicated ... (see lto/lto-lang.c). Not sure why ... Richard. Regards -- Iain Buclaw *(p e ? p++ : p) = (c 0x0f) + '0';
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On Fri, 5 Oct 2012, Iain Buclaw wrote: Would the best approach be to move all handle_* functions and any helper functions into a new source file that can be shared between frontends, and define two new frontend hooks, LANG_HOOK_ATTRIBUTE_TABLE and LANG_HOOK_FORMAT_ATTRIBUTE_TABLE ? I don't know what the best approach would be. It's up to you to work out a clean design (that is, a design that makes logical sense as a way to implement the relevant features - starting from the features, not the existing code, as a justification for the design) and justify in your patch posting why you think it's the right way to refactor this code so it can be shared. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On Oct 4, 2012, at 6:06 AM, Iain Buclaw ibuc...@ubuntu.com wrote: I would like to get a bump on this. It's been a while, and there have been quite a number of changes since the initial post that address many of the issues raised. Rather than reposting patches, someone mentioned attaching changelog, well, here it is. Repository is still located here: https://github.com/D-Programming-GDC/GDC Would it be possible to have a re-newed review? You don't ask, you post. If you have independent patches to the rest of gcc that improve it, I'd suggest posting those as separate patches and getting those in. This give you a base onto which to slot in the front-end.
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On Thu, 4 Oct 2012, Iain Buclaw wrote: The only patches to gcc proper are documentation-related and adding the D frontend / libphobos to configure and make files. I would have thought that these would typically only be included with the actual front-end? Looking back at my previous review comments, I suggested that you might need to split up c-common.[ch] so that certain parts of attribute handling could be shared with D, because duplicate code copied from elsewhere in GCC was not an appropriate implementation approach. Have you then eliminated the duplicate code in some other way that does not involve splitting up those files so code can be shared? -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On 27 June 2012 19:17, Mike Stump mikest...@comcast.net wrote: On Jun 27, 2012, at 7:45 AM, Iain Buclaw wrote: I do have a question though, what is available for the transition of development from git to svn? Other than a lot of ready and getting used to the various switches and commands on my part. Why transition? Quite a few people around here use git on a day to day basis and just push and pull to/from svn as they see fit. gcc has a read-only git repo you can track and pull from. For pushing into svn, you can use git to do that as well (dcommit). You'll want to read up on work flows on the net... as dcommit and merges require a little extra caution that isn't obvious. I did not know of this, thanks. I'll be sure to look it up. -- Iain Buclaw *(p e ? p++ : p) = (c 0x0f) + '0';
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On 19 June 2012 17:20, Joseph S. Myers jos...@codesourcery.com wrote: On Mon, 18 Jun 2012, Iain Buclaw wrote: [PATCH 1/4]: The D compiler frontend - gcc/d Only selectively reviewed, but here are some comments: diff -Naur gcc-4.8-20120617/gcc/d/asmstmt.cc gcc-4.8/gcc/d/asmstmt.cc --- gcc-4.8-20120617/gcc/d/asmstmt.cc 1970-01-01 01:00:00.0 +0100 +++ gcc-4.8/gcc/d/asmstmt.cc 2012-06-05 13:42:09.044876794 +0100 @@ -0,0 +1,2731 @@ +// asmstmt.cc -- D frontend for GCC. +// Originally contributed by David Friedman +// Maintained by Iain Buclaw + +// GCC is free software; you can redistribute it and/or modify it under Every file more than ten lines long needs a copyright notice as well as the license notice. See http://www.gnu.org/prep/maintain/html_node/Copyright-Notices.html for instructions, including the case of multiple copyright holders - though if there are any significant (more than fifteen lines of copyrightable text or so) contributors not assigning copyright to the FSF then special approval from the FSF will be needed to include the front end. I would say that the files in dfrontend/ need copyright and license notices as well, though not necessarily in exactly GNU form. Thus, you will need to get Digital Mars to approve appropriate notices for those files (aav.c is the first I see that's lacking such a notice but is long enough to need one; likewise async.c, gnuc.c, speller.c; rmem.c just says All Rights Reserved and needs a proper license notice like other files; likewise rmem.h). I have raised this with Walter, and the licensing has been fixed in all frontend code. +#ifdef TARGET_80387 +#include d-asm-i386.h +#else +#define D_NO_INLINE_ASM_AT_ALL +#endif Ugh. We want to move away from target macros, and this isn't even a proper target macro. It would be better to define target hooks for the D inline asm support - possibly with a D-specific hook structure, like the C hooks structure. (Even if you avoid needing copyright assignments for the front end itself, such hook implementations will probably need to be assigned.) This code has been removed entirely. +/* Apple GCC extends ASM_EXPR to five operands; cannot use build4. */ I don't see why that should be in the least relevant to a contribution to FSF GCC. If you can do things in a more natural way in FSF GCC, then do so. Now use build5, similar to other frontends. Each function in the GCC-specific parts of the code should have a comment on it, explaining the semantics of the function, its operands and its return value if any. Am working on this. For new code in GCC, it's better to use snprintf than sprintf. Have fixed this. Thanks. +extern void decode_options (struct gcc_options *, struct gcc_options *, Please use appropriate headers rather than local declarations of GCC functions. +// d-bi-attr.h -- D frontend for GCC. This file looks like it's largely copied from elsewhere in GCC. In such a case, please work out a better way to refactor the code so that it can be shared rather than duplicated. (Again, such common code will no doubt need full copyright assignments.) I don't know whether your assignment Assigns Past and Future Changes to the GNU D Compiler (GDC) covers changes elsewhere in GCC. But I expect a general assignment for GCC to be needed for any refactoring involved in adapting common code for use in D. (And such refactoring would be a new contribution so there shouldn't be any issues with unknown previous contributors without assignments - those would only arise if significant amounts of previously written D front-end code are being moved into common code.) It's copied as including c-common.c / .h causes problems with a fair number of references pulled in that need to be stubbed out - also, some GCC function attributes that we use do not make any sense to have in D code (eg: gnu_inline, artificial, cleanup). It could certainly be possible though ... Will need to review this in more detail. +#if D_VA_LIST_TYPE_VOIDPTR Please avoid #if conditionals on anything that could be a target property. It's generally better to use if conditionals instead of #if, so that all cases are checked for syntax in all compiles. I see #if conditions on defines such as V2 and V1 as well. Unless something is an *existing* target macro or configure macro in GCC, use if conditions and ensure that the macro is defined to true or false values (rather than defined or not defined). But if a macro is always defined, or never defined, then just avoiding the conditionals may be better. Have remove this from the gdc glue. The gcc/d/dfrontend/readme.txt says: +These sources are free, they are redistributable and modifiable +under the terms of the GNU General Public License (attached as gpl.txt), +or the Artistic License (attached as artistic.txt). But that license is GPLv2. We need an explicit notice (approved by the copyright
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On 19 June 2012 17:08, Steven Bosscher stevenb@gmail.com wrote: Many functions have no leading comment, and other GNU coding standard requirements are not followed either. Those should IMHO be fixed also, before this front end can be accepted. To separate this from the other listed items. I am aware of certain things that are in definite need to spot check over. (NB: The coding convention was originally KR, then changed over to Allman about 2 years ago, and recently changed over to GNU-like via several vim macros I wrote to carry out the quick re-format job). As well as the GCC coding convention, I would like to also know what of the C++ Coding convention is an absolutely *must* be followed to the letter on the wiki? As it states at the top that it is only a proposed set of coding conventions to be used when writing GCC in C++, and I am well aware that in GDC I do not follow some items, such as All data members should have names which end with an underscore. http://gcc.gnu.org/wiki/CppConventions Regards -- Iain Buclaw *(p e ? p++ : p) = (c 0x0f) + '0';
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On Wed, 27 Jun 2012, Iain Buclaw wrote: It's copied as including c-common.c / .h causes problems with a fair number of references pulled in that need to be stubbed out - also, some GCC function attributes that we use do not make any sense to have in D code (eg: gnu_inline, artificial, cleanup). It could certainly be possible though ... Will need to review this in more detail. Quite possibly you need to split up c-common so that the parts that can also be shared with D are in separate files. The D frontend is completely independent of the GCC backend, and any alterations are purely for portability (eg, the use of real_t rather than long double for the representation of floats). There is no If for portability, I'd hope they wouldn't need to be conditional - rather, the common repository used for all the compilers using the dfrontend code should be able to have them, unconditionally, and another such compiler might have a typedef of real_t to long double if that's what that other compiler wishes to use. Hopefully you can work with the people maintaining other such compilers so that there can genuinely be a shared, portable source base for the shared code, in a public repository used by all those maintainers, without conditionals based on which compiler it's used in, and with that shared source base only using an absolute minimum of headers from whatever compiler it's used in (so only minimal GCC headers when used in GCC, etc.). Likewise, have removed it as is in fact no longer required. The optimize #undef remains for the time being as it conflicts with the name of a member in the D frontend sources. If the D frontend followed the C++ Coding Conventions as outlined in gcc.gnu.org/wiki/CppConventions then this wouldn't be an issue. Though I don't think it has an obligation to being essentially disconnected from calling any GCC code. But it ought to be possible to stop the shared D front end from including the relevant GCC headers at all, if it has a clean interface to the rest of GCC Have removed all alloca handling from GDC and replaced with simply including libiberty.h. system.h includes libiberty.h, so direct inclusion of libiberty.h shouldn't be needed (unless you are trying to avoid using system.h in code shared with other D compilers). https://github.com/D-Programming-GDC/GDC/commits/master I do have a question though, what is available for the transition of development from git to svn? Other than a lot of ready and getting used to the various switches and commands on my part. Once there is a front end ready to commit and approved in technical and GNU policy terms, you'd just do svn add on the files to add them to trunk. It's up to you how you handle keeping the dfrontend/ changes in sync with an external shared repository (with all changes going to the external repository first); Ian Taylor may have some automation for that issue for Go. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On 27 June 2012 16:47, Joseph S. Myers jos...@codesourcery.com wrote: On Wed, 27 Jun 2012, Iain Buclaw wrote: It's copied as including c-common.c / .h causes problems with a fair number of references pulled in that need to be stubbed out - also, some GCC function attributes that we use do not make any sense to have in D code (eg: gnu_inline, artificial, cleanup). It could certainly be possible though ... Will need to review this in more detail. Quite possibly you need to split up c-common so that the parts that can also be shared with D are in separate files. The D frontend is completely independent of the GCC backend, and any alterations are purely for portability (eg, the use of real_t rather than long double for the representation of floats). There is no If for portability, I'd hope they wouldn't need to be conditional - rather, the common repository used for all the compilers using the dfrontend code should be able to have them, unconditionally, and another such compiler might have a typedef of real_t to long double if that's what that other compiler wishes to use. Hopefully you can work with the people maintaining other such compilers so that there can genuinely be a shared, portable source base for the shared code, in a public repository used by all those maintainers, without conditionals based on which compiler it's used in, and with that shared source base only using an absolute minimum of headers from whatever compiler it's used in (so only minimal GCC headers when used in GCC, etc.). In some ways, some elements of the code is already shared, and I've have no trouble sending patches for GDC-specific elements provided that they are wrapped around #ifdef IN_GCC. Likewise, have removed it as is in fact no longer required. The optimize #undef remains for the time being as it conflicts with the name of a member in the D frontend sources. If the D frontend followed the C++ Coding Conventions as outlined in gcc.gnu.org/wiki/CppConventions then this wouldn't be an issue. Though I don't think it has an obligation to being essentially disconnected from calling any GCC code. But it ought to be possible to stop the shared D front end from including the relevant GCC headers at all, if it has a clean interface to the rest of GCC It's the other way round, actually. It's the GDC code that errors on compilation from including the main header (mars.h) in the D frontend. The dfrontend code does not touch d-gcc-includes.h, and I can add in a compile #error to assert that is always the case now that I'm using IN_GCC_FRONTEND to build the GDC sources. Have removed all alloca handling from GDC and replaced with simply including libiberty.h. system.h includes libiberty.h, so direct inclusion of libiberty.h shouldn't be needed (unless you are trying to avoid using system.h in code shared with other D compilers). I'd rather not be including headers in GCC into the D frontend, however useful they may be. One reason for this is that system.h poisons at least one function that the D frontend uses (strdup in rmem.c). Although I'm sure the maintainers would be happy to accept any patches I send if the D frontend must be compliant and not use these banned functions in GCC. https://github.com/D-Programming-GDC/GDC/commits/master I do have a question though, what is available for the transition of development from git to svn? Other than a lot of ready and getting used to the various switches and commands on my part. Once there is a front end ready to commit and approved in technical and GNU policy terms, you'd just do svn add on the files to add them to trunk. It's up to you how you handle keeping the dfrontend/ changes in sync with an external shared repository (with all changes going to the external repository first); Ian Taylor may have some automation for that issue for Go. OK, thanks. As the D frontend goes through a sometimes experimental development process between each release, I'd rather have it so that I merge the frontend into GDC as each release happens, instead of keeping in constant sync. This is how I handle such merges at the moment, although it doesn't help if you require a track of all changes made to the D frontend. Regards -- Iain Buclaw *(p e ? p++ : p) = (c 0x0f) + '0';
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On Wed, 27 Jun 2012, Iain Buclaw wrote: OK, thanks. As the D frontend goes through a sometimes experimental development process between each release, I'd rather have it so that I merge the frontend into GDC as each release happens, instead of keeping in constant sync. This is how I handle such merges at the moment, although it doesn't help if you require a track of all changes made to the D frontend. As long as you don't make changes to the dfrontend code directly in the GCC repository (only import versions from another repository, verbatim) I think that would work (importing from releases / release branches in the upstream repository rather than always the development mainline). Likewise any other directories taken verbatim from some external repository shared between D compilers. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On Jun 27, 2012, at 7:45 AM, Iain Buclaw wrote: I do have a question though, what is available for the transition of development from git to svn? Other than a lot of ready and getting used to the various switches and commands on my part. Why transition? Quite a few people around here use git on a day to day basis and just push and pull to/from svn as they see fit. gcc has a read-only git repo you can track and pull from. For pushing into svn, you can use git to do that as well (dcommit). You'll want to read up on work flows on the net... as dcommit and merges require a little extra caution that isn't obvious. As the D frontend goes through a sometimes experimental development process between each release, I'd rather have it so that I merge the frontend into GDC as each release happens, instead of keeping in constant sync. You can evolve when and how often you push and pull...
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On Tue, Jun 26, 2012 at 2:43 AM, Iain Buclaw ibuclaw at ubuntu dot com wrote: On 19 June 2012 17:08, Steven Bosscher stevenb dot gcc at gmail dot com wrote: BTW you also include output.h in those two files, and I am about two patches away from adding output.h to the list of headers that no front end should ever include (a front end should never have to write out assembly). Can you please check what you need output.h for, and fix this? What are you calling targetm.asm_out.output_mi_thunk and targetm.asm_out.generate_internal_label for? Thunks and aliases should go through cgraphunit. (NB: This also means that this front end cannot work with LTO. IMHO we shouldn't let in new front ends that don't work with LTO.) I tried switching, but unfortunately it broke the code generation of D thunks. D requires any class that inherits from an interface to output thunks for that interface regardless of how far back it is and whether it being external to the current module. However, the GCC backend as far as I can tell only outputs aliases when the function is output as well. So if there is no function to output no thunk will be generated. This leaves a handful of undefined references to thunks that were defined in the D frontend, but discarded by the backend. Whereas forcing the output using output_mi_thunk means the thunk is emitted and does not cause issues. So removing output.h will be a bit of a problem for me with no obvious way around it. The obvious way around it is to work with this community to fix the problem. You will want to talk with Jan Hubicka about this, see if he can help. Ciao! Steven
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On 19 June 2012 17:08, Steven Bosscher stevenb@gmail.com wrote: Hello, I had a very quick look through the gdc_frontend patch. Below are a couple of comments on it: http://www.gdcproject.org/files/gdc_frontend.patch.gz [PATCH 1/4]: The D compiler frontend - gcc/d How did you test this? You include rtl.h/expr.h in d-builtins.c and d-gcc-includes.h, which should both be in ALL_HOST_FRONTEND_OBJS and fail to build because IN_GCC_FRONTEND is defined and GCC_RTL_H is poisoned. See system.h: /* Front ends should never have to include middle-end headers. Enforce this by poisoning the header double-include protection defines. */ #ifdef IN_GCC_FRONTEND #pragma GCC poison GCC_RTL_H GCC_EXCEPT_H GCC_EXPR_H #endif Do you somehow bypass the normal build system? Or maybe you don't include system.h? Either way, front ends should never have to include RTL headers. It seems that IN_GCC_FRONTEND was never defined throughout. I did not realise this. Have corrected it now in Make-lang.in and removed all code that depended on it. BTW you also include output.h in those two files, and I am about two patches away from adding output.h to the list of headers that no front end should ever include (a front end should never have to write out assembly). Can you please check what you need output.h for, and fix this? What are you calling targetm.asm_out.output_mi_thunk and targetm.asm_out.generate_internal_label for? Thunks and aliases should go through cgraphunit. (NB: This also means that this front end cannot work with LTO. IMHO we shouldn't let in new front ends that don't work with LTO.) I tried switching, but unfortunately it broke the code generation of D thunks. D requires any class that inherits from an interface to output thunks for that interface regardless of how far back it is and whether it being external to the current module. However, the GCC backend as far as I can tell only outputs aliases when the function is output as well. So if there is no function to output no thunk will be generated. This leaves a handful of undefined references to thunks that were defined in the D frontend, but discarded by the backend. Whereas forcing the output using output_mi_thunk means the thunk is emitted and does not cause issues. So removing output.h will be a bit of a problem for me with no obvious way around it. Many functions have no leading comment, and other GNU coding standard requirements are not followed either. Those should IMHO be fixed also, before this front end can be accepted. Most functions are of the same name but from different classes, eg: toElem, toIR, toCtype, toObjFile are a few of the main functions which cover the GCC code generation of all expressions and statements passed from the D frontend. There is this comment: +/* GCC does not support jumps from asm statements. This isn't really true anymore, as your patch also notes: + -- + %% Fix for GCC-4.5+ + GCC now accepts a 5th operand, ASM_LABELS. (...) + For prior versions of gcc, this requires a backpatch. It seems to me that if this front end is contributed, handling of prior version of gcc isn't necessary anymore - that code should just be removed. + + case Op_de: +#ifndef TARGET_80387 +#define XFmode TFmode +#endif + mode = XFmode; // not TFmode What is this hack for? This is not the way to find the right mode for this operation. +#ifdef TARGET_80387 +#include d-asm-i386.h +#else +#define D_NO_INLINE_ASM_AT_ALL +#endif + +/* Apple GCC extends ASM_EXPR to five operands; cannot use build4. */ Idem here. And Apple GCC is irrelevant too, if this front end lands on FSF trunk. What is d/d-asm-i386.h for? It looks like i386 is a special case throughout the front end. After some discussion, I have remove the D Inline Asm implementation completely from GDC, along with the backend headers that it depended on, so this file is no more, along with the i386 special cases. In d-gcc-tree.h: +// normally include config.h (hconfig.h, tconfig.h?), but that +// includes things that cause problems, so... + +union tree_node; +typedef union tree_node *tree; See coretypes.h. Thanks for the tip! Have switched it over. -- Iain Buclaw *(p e ? p++ : p) = (c 0x0f) + '0';
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On Mon, 18 Jun 2012, Iain Buclaw wrote: These series of patches are for the D compiler frontend for inclusion into GCC. http://www.gdcproject.org/files/gdc_frontend.patch.gz http://www.gdcproject.org/files/gdc_libphobos.patch.gz http://www.gdcproject.org/files/gdc_testsuite.patch.gz http://www.gdcproject.org/files/gdc_gcc.patch.gz Please provide GNU ChangeLog entries for each patch, for each relevant ChangeLog file. It would be best to post those in plain text to the list, even if the patches themselves are too big. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
Hello, I had a very quick look through the gdc_frontend patch. Below are a couple of comments on it: http://www.gdcproject.org/files/gdc_frontend.patch.gz [PATCH 1/4]: The D compiler frontend - gcc/d How did you test this? You include rtl.h/expr.h in d-builtins.c and d-gcc-includes.h, which should both be in ALL_HOST_FRONTEND_OBJS and fail to build because IN_GCC_FRONTEND is defined and GCC_RTL_H is poisoned. See system.h: /* Front ends should never have to include middle-end headers. Enforce this by poisoning the header double-include protection defines. */ #ifdef IN_GCC_FRONTEND #pragma GCC poison GCC_RTL_H GCC_EXCEPT_H GCC_EXPR_H #endif Do you somehow bypass the normal build system? Or maybe you don't include system.h? Either way, front ends should never have to include RTL headers. BTW you also include output.h in those two files, and I am about two patches away from adding output.h to the list of headers that no front end should ever include (a front end should never have to write out assembly). Can you please check what you need output.h for, and fix this? What are you calling targetm.asm_out.output_mi_thunk and targetm.asm_out.generate_internal_label for? Thunks and aliases should go through cgraphunit. (NB: This also means that this front end cannot work with LTO. IMHO we shouldn't let in new front ends that don't work with LTO.) Many functions have no leading comment, and other GNU coding standard requirements are not followed either. Those should IMHO be fixed also, before this front end can be accepted. There is this comment: +/* GCC does not support jumps from asm statements. This isn't really true anymore, as your patch also notes: + -- + %% Fix for GCC-4.5+ + GCC now accepts a 5th operand, ASM_LABELS. (...) + For prior versions of gcc, this requires a backpatch. It seems to me that if this front end is contributed, handling of prior version of gcc isn't necessary anymore - that code should just be removed. + + case Op_de: +#ifndef TARGET_80387 +#define XFmode TFmode +#endif + mode = XFmode; // not TFmode What is this hack for? This is not the way to find the right mode for this operation. +#ifdef TARGET_80387 +#include d-asm-i386.h +#else +#define D_NO_INLINE_ASM_AT_ALL +#endif + +/* Apple GCC extends ASM_EXPR to five operands; cannot use build4. */ Idem here. And Apple GCC is irrelevant too, if this front end lands on FSF trunk. What is d/d-asm-i386.h for? It looks like i386 is a special case throughout the front end. In d-gcc-tree.h: +// normally include config.h (hconfig.h, tconfig.h?), but that +// includes things that cause problems, so... + +union tree_node; +typedef union tree_node *tree; See coretypes.h. Ciao! Steven
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On Mon, 18 Jun 2012, Iain Buclaw wrote: [PATCH 1/4]: The D compiler frontend - gcc/d Only selectively reviewed, but here are some comments: diff -Naur gcc-4.8-20120617/gcc/d/asmstmt.cc gcc-4.8/gcc/d/asmstmt.cc --- gcc-4.8-20120617/gcc/d/asmstmt.cc 1970-01-01 01:00:00.0 +0100 +++ gcc-4.8/gcc/d/asmstmt.cc2012-06-05 13:42:09.044876794 +0100 @@ -0,0 +1,2731 @@ +// asmstmt.cc -- D frontend for GCC. +// Originally contributed by David Friedman +// Maintained by Iain Buclaw + +// GCC is free software; you can redistribute it and/or modify it under Every file more than ten lines long needs a copyright notice as well as the license notice. See http://www.gnu.org/prep/maintain/html_node/Copyright-Notices.html for instructions, including the case of multiple copyright holders - though if there are any significant (more than fifteen lines of copyrightable text or so) contributors not assigning copyright to the FSF then special approval from the FSF will be needed to include the front end. I would say that the files in dfrontend/ need copyright and license notices as well, though not necessarily in exactly GNU form. Thus, you will need to get Digital Mars to approve appropriate notices for those files (aav.c is the first I see that's lacking such a notice but is long enough to need one; likewise async.c, gnuc.c, speller.c; rmem.c just says All Rights Reserved and needs a proper license notice like other files; likewise rmem.h). +#ifdef TARGET_80387 +#include d-asm-i386.h +#else +#define D_NO_INLINE_ASM_AT_ALL +#endif Ugh. We want to move away from target macros, and this isn't even a proper target macro. It would be better to define target hooks for the D inline asm support - possibly with a D-specific hook structure, like the C hooks structure. (Even if you avoid needing copyright assignments for the front end itself, such hook implementations will probably need to be assigned.) +/* Apple GCC extends ASM_EXPR to five operands; cannot use build4. */ I don't see why that should be in the least relevant to a contribution to FSF GCC. If you can do things in a more natural way in FSF GCC, then do so. Each function in the GCC-specific parts of the code should have a comment on it, explaining the semantics of the function, its operands and its return value if any. For new code in GCC, it's better to use snprintf than sprintf. +extern void decode_options (struct gcc_options *, struct gcc_options *, Please use appropriate headers rather than local declarations of GCC functions. +// d-bi-attr.h -- D frontend for GCC. This file looks like it's largely copied from elsewhere in GCC. In such a case, please work out a better way to refactor the code so that it can be shared rather than duplicated. (Again, such common code will no doubt need full copyright assignments.) I don't know whether your assignment Assigns Past and Future Changes to the GNU D Compiler (GDC) covers changes elsewhere in GCC. But I expect a general assignment for GCC to be needed for any refactoring involved in adapting common code for use in D. (And such refactoring would be a new contribution so there shouldn't be any issues with unknown previous contributors without assignments - those would only arise if significant amounts of previously written D front-end code are being moved into common code.) +#if D_VA_LIST_TYPE_VOIDPTR Please avoid #if conditionals on anything that could be a target property. It's generally better to use if conditionals instead of #if, so that all cases are checked for syntax in all compiles. I see #if conditions on defines such as V2 and V1 as well. Unless something is an *existing* target macro or configure macro in GCC, use if conditions and ensure that the macro is defined to true or false values (rather than defined or not defined). But if a macro is always defined, or never defined, then just avoiding the conditionals may be better. The gcc/d/dfrontend/readme.txt says: +These sources are free, they are redistributable and modifiable +under the terms of the GNU General Public License (attached as gpl.txt), +or the Artistic License (attached as artistic.txt). But that license is GPLv2. We need an explicit notice (approved by the copyright holder) saying that *any later version* may be used. If Digital Mars wishes to license the separately maintained dfrontend/ code under GPLv2+ rather than GPLv3+, that's fine, just like the gofrontend/ code is under a permissive license - but it needs to be explicit that any later version may be used. I haven't studied the details of the dfrontend/ code. But if you are to follow the Go model - separately maintained code for the front end proper that may be used verbatim in multiple compilers, with the code outside dfrontend/ doing everything related to interfacing with GCC, and only what's related to interfacing with GCC - then the +/* NOTE: This file has been patched
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On Mon, 18 Jun 2012, Iain Buclaw wrote: http://www.gdcproject.org/files/gdc_libphobos.patch.gz Same comments as before about FSF postal addresses. Although runtime libraries need not be assigned to the FSF (as per the GCC Mission Statement), all significant files should still have copyright and license notices (approved by all significant contributors) so that people know the free software terms under which they may be used. E.g., libphobos/libdruntime/config/x3.c appears to be missing such notices. Without a license (or a dedication to the public domain), a file is presumptively copyright and has no license for anyone to use it at all. +if true; then if true seems odd; if you have a good reason for it, you need to comment it. +# generated automatically by aclocal 1.9.6 -*- Autoconf -*- Please use the standard documented autoconf/automake versions for GCC (autoconf 2.64, automake 1.11.1). diff -Naur gcc-4.8-20120617/libphobos/autom4te.cache/output.0 gcc-4.8/libphobos/autom4te.cache/output.0 We don't check in autom4te.cache directories. +# libphobos is usually a symlink to gcc/d/phobos, so libphobos/.. No it's not. No runtime libraries should go under gcc/ any more at all. +dnl Copied from libstdc++-v3/acinclude.m4. Indeed, multilib will not work Refactor into the config/ directory, don't copy. \ No newline at end of file Add any missing newlines to text files in all patches. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH, gdc] - Merging gdc (GNU D Compiler) into gcc
On Mon, 18 Jun 2012, Iain Buclaw wrote: http://www.gdcproject.org/files/gdc_testsuite.patch.gz I have no comments on this patch for now. -- Joseph S. Myers jos...@codesourcery.com
