Go front end review: files outside gofrontend/

Go front end review: files outside gofrontend/

[Joseph S. Myers]

I'll try to review the Go front-end files.  This message reviews those 
outside the gofrontend/ subdirectory, from 
svn://gcc.gnu.org/svn/gcc/branches/gccgo/gcc/go r166404:

Review of Make-lang.in:

> # C++ support.  This should really be in the top level Makefile.
> 
> ALL_CXXFLAGS = $(filter-out -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition -Wc++-compat, $(ALL_COMPILERFLAGS)) $(CXXFLAGS)

So put it there?  Isn't this relevant to general C++ building support?

> # Build Go export info generator.
> INTERNAL_CFLAGS += -DGO_DEBUGGING_INFO

What code is conditioned on this define?

> GCCGO_OBJS = $(GCC_OBJS) gospec.o intl.o prefix.o version.o
> gccgo$(exeext): $(GCCGO_OBJS) $(EXTRA_GCC_OBJS) $(LIBDEPS)
> 	$(CC) $(ALL_CFLAGS) $(LDFLAGS) -o $@ \
> 	  $(GCCGO_OBJS) $(EXTRA_GCC_OBJS) $(LIBS)

Shouldn't this use $(LINKER) and $(ALL_LINKERFLAGS)?

> go1$(exeext): $(GO_OBJS) attribs.o $(BACKEND) $(LIBDEPS)
> 	$(CXX) $(STATIC_LIBSTDCXX) $(ALL_CXXFLAGS) $(LDFLAGS) -o $@ \
> 	      $(GO_OBJS) attribs.o $(BACKEND) $(LIBS) $(BACKENDLIBS)

If someone uses the various configure options such as 
--with-host-libstdcxx to control linking with libstdc++ (and to link with 
a static libstdc++ with a compiler predating -static-libstdc++, say), how 
will that interact with this link command?  (The expectation with 
--with-host-libstdcxx is that linking ends up using the C compiler to 
avoid the driver doing the linking implicitly adding its own notion of the 
C++ standard library.)

> # Create a version of the g++ driver which calls the cross-compiler.

Not g++....

> gccgo-cross$(exeext): gccgo$(exeext)
> 	-rm -f gccgo-cross$(exeext)
> 	cp gccgo$(exeext) gccgo-cross$(exeext)

I don't actually see anything using this binary.

The install rules below:

> go.install-common: installdirs
> 	-rm -f $(DESTDIR)$(bindir)/$(GCCGO_INSTALL_NAME)$(exeext)
> 	-$(INSTALL_PROGRAM) gccgo$(exeext) $(DESTDIR)$(bindir)/$(GCCGO_INSTALL_NAME)$(exeext)
> 	-chmod a+x $(DESTDIR)$(bindir)/$(GCCGO_INSTALL_NAME)$(exeext)

are generally more sensible than the cargo-culted rules in other 
directories that involve checking for a -cross binary and possibly 
installing a non-functioning copy of the driver in $(gcc_tooldir)/bin/.  
But if you have these rules you shouldn't still have the -cross binary.  
And I think the *correct* rules everywhere ought to involve installing the 
$(target_noncanonical)-{gcc,g++,gccgo,etc.} driver everywhere (though most 
languages install one for both native and cross, this patch would not 
install one for gccgo for native) and not installing any copies in 
$(gcc_tooldir)/bin/ - furthermore, at least the $(INSTALL_PROGRAM) 
commands should not ignore errors.

> go.install-pdf:
> go.html:

The install-html hook appears to be missing.

> GO_SYSTEM_H = go/go-system.h $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) \
> 	$(DIAGNOSTIC_CORE_H) $(INPUT_H)

Limiting front-end dependence on target macros and tm.h is generally a 
good idea; putting tm.h into a header included everywhere is a less good 
idea.  What are the actual target macro dependencies in the Go front end?  
Can you contain them within files outside the gofrontend/ directory?

Review of go-c.h:

> extern int go_enable_dump(const char*);

Existing GCC and GNU style has the space before the open parenthesis.  I 
think this should apply at least to all the C/C++ code outside the 
gofrontend/ subdirectory.

Review of go-lang.c:

>   /* The sizetype may be "unsigned long" or "unsigned long long".  */
>   if (TYPE_MODE (long_unsigned_type_node) == ptr_mode)
>     size_type_node = long_unsigned_type_node;
>   else if (TYPE_MODE (long_long_unsigned_type_node) == ptr_mode)
>     size_type_node = long_long_unsigned_type_node;
>   else
>     size_type_node = long_unsigned_type_node;

This exists in other front ends so isn't an obstacle to inclusion, but 
it's also ugly.  Hopefully at some point we'll transition macros such as 
SIZE_TYPE to evaluate to enum values rather than strings (and then convert 
them to hooks), so avoiding the need for such hacks for non-C-family front 
ends to work out a suitable sizetype, and also avoiding Fortran needing 
its own type name parsing code (get_typenode_from_name) to implement the C 
bindings.

>   set_sizetype(size_type_node);

Missing space.

> /* Initialize before parsing options.  */
> 
> static void
> go_langhook_init_options (
>     unsigned int argc ATTRIBUTE_UNUSED,
>     struct cl_decoded_option *decoded_options ATTRIBUTE_UNUSED)

As previously noted, use the init_options_struct hook as far as possible.

>   /* We default to always showing a column number.  */
>   flag_show_column = 1;

This is the GCC default now.  The variable (field in global_options) only 
exists for initialization in toplev.c:general_init and for the sake of 
reporting option state; the setting in global_dc is used for everything 
else.

> void
> go_preserve_from_gc(tree t)

Missing space.

Review of go-system.h:

> #include <tr1/unordered_map>

So you're requiring more than just the intersection of C++98 with the C++ 
language supported by some baseline GCC version (the agreed language for 
GCC-in-C++, I think)?  How widespread is <tr1/unordered_map> support in 
C++ implementations?  What was the first GCC version with it?  How hard 
would it be to provide a C++ equivalent of libiberty that provides such 
facilities on systems that don't have them (which seems analogous to how 
we handle missing C library facilities)?

Or do we believe this is not an issue as long as we don't use TR1 
facilities in code required for the C and C++ compilers, because we can 
have stricter requirements for building cross compilers and for native 
compilers a new libstdc++ will be built with the stage 1 C++ compiler?  
(If so, the documentation of prerequisites in install.texi still needs to 
say what GCC version is needed to build a Go cross compiler.)

> extern "C"

If GCC is built as C++, don't the various GCC headers you're including 
inside extern "C" actually describe C++ interfaces rather than C ones?  
How does this work?

> #include "tm.h"

(Remarked upon above.)

Review of gospec.c:

> #ifndef THREAD_LIBRARY
> #define THREAD_LIBRARY "pthread"
> #endif
> #ifndef THREAD_LIBRARY_PROFILE
> #define THREAD_LIBRARY_PROFILE THREAD_LIBRARY
> #endif
> 
> #ifndef LIBGO
> #define LIBGO "go"
> #endif
> #ifndef LIBGOBEGIN
> #define LIBGOBEGIN "gobegin"
> #endif
> #ifndef LIBGO_PROFILE
> #define LIBGO_PROFILE LIBGO
> #endif

By using #ifndef you're implicitly creating new target macros.  But I 
don't see any documentation of these macros in tm.texi.in on the branch.  
Do you really need to allow for these things to be changed by targets?  If 
so, the macros need documenting; if not, don't use #ifndef.

>   /* If nonzero, the user gave us the `-p' or `-pg' flag.  */
>   int saw_profile_flag = 0;

New code should be using bool for booleans, not int (and false/true not 
0/1).

I have no comments on files outside gofrontend/ not mentioned above.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Go front end review: files outside gofrontend/

[Joseph S. Myers]

A further comment on go-system.h:

It is always necessary for code in GCC to include config.h before any 
system headers.  Thus, the includes

#include <string>
#include <vector>
#include <tr1/unordered_map>
#include <iostream>
#include <cstdio>

are unsafe because they appear before the include of config.h.

In particular, feature test macros such as those for large-files support 
(which is now enabled in GCC) may be defined in config.h and may only be 
effective if defined before any system headers are included - and C++ 
system headers may include C system headers.

If the issue described in

// These must be included before the #poison declarations in system.h.

arises then I think the answer is for system.h to include some C++ 
headers, when compiling as C++, before the problem #poison declarations, 
just as it includes various C headers.  A suitable set of C++ header 
includes might also eliminate many of the #include <...> directives in 
individual files in the front end.  If a source file directly includes a 
<...> system header, there should be a clear reason why that include is 
not in system.h (for example, deliberately deciding that certain 
interfaces such as those from GMP and MPFR should only be used in certain 
parts of the compiler - especially where using a header may result in 
library dependencies not desired for programs such as the driver).

-- 
Joseph S. Myers
joseph@codesourcery.com

Go front end review: gofrontend/

[Joseph S. Myers]

This message covers files inside gofrontend/.

My previous comments about extern "C" use around includes, and about when 
system headers are included, apply generally here.  As a more specific 
comment, lex.cc includes <stdint.h>; you can't rely on all hosts having 
this header, but if they do then system.h will already have included it.  
The extern "C" around the include of filenames.h in import.cc is 
suspicious in its own way; libiberty headers such as that should contain 
their own extern "C" declaration, as indeed that one does (and in any case 
system.h already includes filenames.h so you don't need to include it 
again).

expressions.cc in various places passes a diagnostic argument of error_at 
to _().  error_at translates its gmsgid operand and it should not be 
passed to _() a second time.

In most places expressions.cc uses _() on the operand of 
this->report_error.  This is correct for the present definition of 
Expression::report_error which passes the string to a "%s" format.  But 
one "floating point constant truncated to integer" message does not use 
_().  And another message "left operand of %<<-%> must be channel" is 
actually a format string, which isn't going to work correctly with the 
present code.  I think Expression::report_error should pass the received 
string directly as a format string to error_at, so allowing %< and %> to 
work, and should be set up so that _() markup is not needed for exgettext 
to extract its argument for translation.  A similar format string issue 
appears in lex.cc,

          this->error("invalid character after %<\\%>");

You've mentioned that Go allows UTF-8 identifiers.  For printing these in 
diagnostics you need to pass them through identifier_to_locale so that 
they are printed in the user's locale character set, which may not be 
UTF-8.  (When the %E format is used, the core code handles this 
automatically.)

What target macros are the tm_p includes in expressions.cc and 
gogo-tree.cc needed for?  Can the uses of those target macros be 
abstracted from the front-end code?

The front end adds three new uses of atoi, which has undefined behavior if 
the input number is out of range.  Though the use of this function is a 
pre-existing condition (bug 44574) so we can't yet poison it in system.h, 
adding more uses seems bad and they should be easy to avoid.  On a similar 
note, strtol is used in lex.cc with no check for overflow (including a 
value outside the range of linenum_type = unsigned int, the type passed to 
linemap_add).

It is generally preferred to use %m in diagnostics instead of directly 
using strerror; if strerror is needed for some reason (e.g. passing to a 
printf function rather than to the GCC pretty-printing machinery; the 
latter has %m, the former may not), use the xstrerror wrapper instead.  
See my patch <http://gcc.gnu.org/ml/gcc-patches/2010-05/msg02226.html>.

In various places you open what I think may be binary files with O_RDONLY 
- don't you need to use O_BINARY as well (defining it to 0 if not already 
defined) for portability to Windows hosts?

In import.cc you form filenames in directories using a hardcoded '/'.  I 
think using this instead of DIR_SEPARATOR may be safe given that cpplib 
(append_file_to_dir) does it - but do you need the append_file_to_dir 
checks relating to "//"?

Does the ".o" handling in import.cc need to use TARGET_OBJECT_SUFFIX (and 
so a target macro dependency unless you abstract it out or make it a 
hook)?  Do the VMS people have any comments on this?

Where did the lists of Unicode letters and digits in lex.cc come from?  
Some particular Unicode version?  A generator program?  Does Go follow the 
recommendations of ISO/IEC TR 10176:2003 regarding what characters to 
allow in identifiers?  (That TR is freely available from 
<http://standards.iso.org/ittf/PubliclyAvailableStandards/index.html>.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Go front end review: files outside gofrontend/

[Ian Lance Taylor]

"Joseph S. Myers" <joseph@codesourcery.com> writes:

> I'll try to review the Go front-end files.  This message reviews those 
> outside the gofrontend/ subdirectory, from 
> svn://gcc.gnu.org/svn/gcc/branches/gccgo/gcc/go r166404:

Thanks very much for the review.


> Review of Make-lang.in:
>
>> # C++ support.  This should really be in the top level Makefile.
>> 
>> ALL_CXXFLAGS = $(filter-out -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition -Wc++-compat, $(ALL_COMPILERFLAGS)) $(CXXFLAGS)
>
> So put it there?  Isn't this relevant to general C++ building support?

Removed.


>> # Build Go export info generator.
>> INTERNAL_CFLAGS += -DGO_DEBUGGING_INFO
>
> What code is conditioned on this define?

None anymore.  Removed.


>> GCCGO_OBJS = $(GCC_OBJS) gospec.o intl.o prefix.o version.o
>> gccgo$(exeext): $(GCCGO_OBJS) $(EXTRA_GCC_OBJS) $(LIBDEPS)
>> 	$(CC) $(ALL_CFLAGS) $(LDFLAGS) -o $@ \
>> 	  $(GCCGO_OBJS) $(EXTRA_GCC_OBJS) $(LIBS)
>
> Shouldn't this use $(LINKER) and $(ALL_LINKERFLAGS)?

Yes.  Changed.


>> go1$(exeext): $(GO_OBJS) attribs.o $(BACKEND) $(LIBDEPS)
>> 	$(CXX) $(STATIC_LIBSTDCXX) $(ALL_CXXFLAGS) $(LDFLAGS) -o $@ \
>> 	      $(GO_OBJS) attribs.o $(BACKEND) $(LIBS) $(BACKENDLIBS)
>
> If someone uses the various configure options such as 
> --with-host-libstdcxx to control linking with libstdc++ (and to link with 
> a static libstdc++ with a compiler predating -static-libstdc++, say), how 
> will that interact with this link command?  (The expectation with 
> --with-host-libstdcxx is that linking ends up using the C compiler to 
> avoid the driver doing the linking implicitly adding its own notion of the 
> C++ standard library.)

I have a pending configure patch which will permit removing
$(STATIC_LIBSTDCXX), and will let this link be entirely controlled by
--with-host-libstdcxx, --with-stage1-ldflags, and friends.

http://gcc.gnu.org/ml/gcc-patches/2010-11/msg01639.html


>> # Create a version of the g++ driver which calls the cross-compiler.
>
> Not g++....

Removed.


>> gccgo-cross$(exeext): gccgo$(exeext)
>> 	-rm -f gccgo-cross$(exeext)
>> 	cp gccgo$(exeext) gccgo-cross$(exeext)
>
> I don't actually see anything using this binary.

Removed.


> The install rules below:
>
>> go.install-common: installdirs
>> 	-rm -f $(DESTDIR)$(bindir)/$(GCCGO_INSTALL_NAME)$(exeext)
>> 	-$(INSTALL_PROGRAM) gccgo$(exeext) $(DESTDIR)$(bindir)/$(GCCGO_INSTALL_NAME)$(exeext)
>> 	-chmod a+x $(DESTDIR)$(bindir)/$(GCCGO_INSTALL_NAME)$(exeext)
>
> are generally more sensible than the cargo-culted rules in other 
> directories that involve checking for a -cross binary and possibly 
> installing a non-functioning copy of the driver in $(gcc_tooldir)/bin/.  
> But if you have these rules you shouldn't still have the -cross binary.  
> And I think the *correct* rules everywhere ought to involve installing the 
> $(target_noncanonical)-{gcc,g++,gccgo,etc.} driver everywhere (though most 
> languages install one for both native and cross, this patch would not 
> install one for gccgo for native) and not installing any copies in 
> $(gcc_tooldir)/bin/ - furthermore, at least the $(INSTALL_PROGRAM) 
> commands should not ignore errors.

This rule does install gccgo for native, so I'm not sure what you mean
there.  I changed this to not ignore the exit status of the
$(INSTALL_PROGRAM), and I removed the chmod since most installs don't do
it.


>> go.install-pdf:
>> go.html:
>
> The install-html hook appears to be missing.

Added.


>> GO_SYSTEM_H = go/go-system.h $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) \
>> 	$(DIAGNOSTIC_CORE_H) $(INPUT_H)
>
> Limiting front-end dependence on target macros and tm.h is generally a 
> good idea; putting tm.h into a header included everywhere is a less good 
> idea.  What are the actual target macro dependencies in the Go front end?  
> Can you contain them within files outside the gofrontend/ directory?

I moved the tm.h dependency to the specific files which require it.

> Review of go-c.h:
>
>> extern int go_enable_dump(const char*);
>
> Existing GCC and GNU style has the space before the open parenthesis.  I 
> think this should apply at least to all the C/C++ code outside the 
> gofrontend/ subdirectory.

Fixed.


> Review of go-lang.c:
>
>>   /* The sizetype may be "unsigned long" or "unsigned long long".  */
>>   if (TYPE_MODE (long_unsigned_type_node) == ptr_mode)
>>     size_type_node = long_unsigned_type_node;
>>   else if (TYPE_MODE (long_long_unsigned_type_node) == ptr_mode)
>>     size_type_node = long_long_unsigned_type_node;
>>   else
>>     size_type_node = long_unsigned_type_node;
>
> This exists in other front ends so isn't an obstacle to inclusion, but 
> it's also ugly.  Hopefully at some point we'll transition macros such as 
> SIZE_TYPE to evaluate to enum values rather than strings (and then convert 
> them to hooks), so avoiding the need for such hacks for non-C-family front 
> ends to work out a suitable sizetype, and also avoiding Fortran needing 
> its own type name parsing code (get_typenode_from_name) to implement the C 
> bindings.

I didn't do anything here.


>>   set_sizetype(size_type_node);
>
> Missing space.

Fixed.

>> /* Initialize before parsing options.  */
>> 
>> static void
>> go_langhook_init_options (
>>     unsigned int argc ATTRIBUTE_UNUSED,
>>     struct cl_decoded_option *decoded_options ATTRIBUTE_UNUSED)
>
> As previously noted, use the init_options_struct hook as far as possible.

Fixed.


>>   /* We default to always showing a column number.  */
>>   flag_show_column = 1;
>
> This is the GCC default now.  The variable (field in global_options) only 
> exists for initialization in toplev.c:general_init and for the sake of 
> reporting option state; the setting in global_dc is used for everything 
> else.

Removed setting flag_show_column.


>> void
>> go_preserve_from_gc(tree t)
>
> Missing space.

Fixed.


> Review of go-system.h:
>
>> #include <tr1/unordered_map>
>
> So you're requiring more than just the intersection of C++98 with the C++ 
> language supported by some baseline GCC version (the agreed language for 
> GCC-in-C++, I think)?  How widespread is <tr1/unordered_map> support in 
> C++ implementations?  What was the first GCC version with it?  How hard 
> would it be to provide a C++ equivalent of libiberty that provides such 
> facilities on systems that don't have them (which seems analogous to how 
> we handle missing C library facilities)?
>
> Or do we believe this is not an issue as long as we don't use TR1 
> facilities in code required for the C and C++ compilers, because we can 
> have stricter requirements for building cross compilers and for native 
> compilers a new libstdc++ will be built with the stage 1 C++ compiler?  
> (If so, the documentation of prerequisites in install.texi still needs to 
> say what GCC version is needed to build a Go cross compiler.)

I changed this to use configure tests for what is available, along the
same lines as gold.


>> extern "C"
>
> If GCC is built as C++, don't the various GCC headers you're including 
> inside extern "C" actually describe C++ interfaces rather than C ones?  
> How does this work?

It works now.


> Review of gospec.c:
>
>> #ifndef THREAD_LIBRARY
>> #define THREAD_LIBRARY "pthread"
>> #endif
>> #ifndef THREAD_LIBRARY_PROFILE
>> #define THREAD_LIBRARY_PROFILE THREAD_LIBRARY
>> #endif
>> 
>> #ifndef LIBGO
>> #define LIBGO "go"
>> #endif
>> #ifndef LIBGOBEGIN
>> #define LIBGOBEGIN "gobegin"
>> #endif
>> #ifndef LIBGO_PROFILE
>> #define LIBGO_PROFILE LIBGO
>> #endif
>
> By using #ifndef you're implicitly creating new target macros.  But I 
> don't see any documentation of these macros in tm.texi.in on the branch.  
> Do you really need to allow for these things to be changed by targets?  If 
> so, the macros need documenting; if not, don't use #ifndef.

Fixed.


>>   /* If nonzero, the user gave us the `-p' or `-pg' flag.  */
>>   int saw_profile_flag = 0;
>
> New code should be using bool for booleans, not int (and false/true not 
> 0/1).

Fixed.

All these changes are committed to gccgo branch.

Thanks again for the review.

Ian

Re: Go front end review: files outside gofrontend/

[Joseph S. Myers]

On Tue, 16 Nov 2010, Ian Lance Taylor wrote:

> > are generally more sensible than the cargo-culted rules in other 
> > directories that involve checking for a -cross binary and possibly 
> > installing a non-functioning copy of the driver in $(gcc_tooldir)/bin/.  
> > But if you have these rules you shouldn't still have the -cross binary.  
> > And I think the *correct* rules everywhere ought to involve installing the 
> > $(target_noncanonical)-{gcc,g++,gccgo,etc.} driver everywhere (though most 
> > languages install one for both native and cross, this patch would not 
> > install one for gccgo for native) and not installing any copies in 
> > $(gcc_tooldir)/bin/ - furthermore, at least the $(INSTALL_PROGRAM) 
> > commands should not ignore errors.
> 
> This rule does install gccgo for native, so I'm not sure what you mean
> there.  I changed this to not ignore the exit status of the

What I'm referring to is the installation of $(target_noncanonical)-gccgo.  
For example, there's a $(target_noncanonical)-gcc binary for native as 
well as for cross.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Go front end review: files outside gofrontend/

[Ian Lance Taylor]

"Joseph S. Myers" <joseph@codesourcery.com> writes:

> On Tue, 16 Nov 2010, Ian Lance Taylor wrote:
>
>> > are generally more sensible than the cargo-culted rules in other 
>> > directories that involve checking for a -cross binary and possibly 
>> > installing a non-functioning copy of the driver in $(gcc_tooldir)/bin/.  
>> > But if you have these rules you shouldn't still have the -cross binary.  
>> > And I think the *correct* rules everywhere ought to involve installing the 
>> > $(target_noncanonical)-{gcc,g++,gccgo,etc.} driver everywhere (though most 
>> > languages install one for both native and cross, this patch would not 
>> > install one for gccgo for native) and not installing any copies in 
>> > $(gcc_tooldir)/bin/ - furthermore, at least the $(INSTALL_PROGRAM) 
>> > commands should not ignore errors.
>> 
>> This rule does install gccgo for native, so I'm not sure what you mean
>> there.  I changed this to not ignore the exit status of the
>
> What I'm referring to is the installation of $(target_noncanonical)-gccgo.  
> For example, there's a $(target_noncanonical)-gcc binary for native as 
> well as for cross.

Oh yeah.  Sorry for being dense.  I committed a patch to the gccgo
branch to fix this.  Thanks.

Ian

Re: Go front end review: files outside gofrontend/

[Ian Lance Taylor]

"Joseph S. Myers" <joseph@codesourcery.com> writes:

> It is always necessary for code in GCC to include config.h before any 
> system headers.  Thus, the includes
>
> #include <string>
> #include <vector>
> #include <tr1/unordered_map>
> #include <iostream>
> #include <cstdio>
>
> are unsafe because they appear before the include of config.h.

This is now done reliably in the frontend.

Thanks.

Ian

Re: Go front end review: gofrontend/

[Ian Lance Taylor]

"Joseph S. Myers" <joseph@codesourcery.com> writes:

> This message covers files inside gofrontend/.

Thanks for the detailed review.


> My previous comments about extern "C" use around includes, and about when 
> system headers are included, apply generally here.  As a more specific 
> comment, lex.cc includes <stdint.h>; you can't rely on all hosts having 
> this header, but if they do then system.h will already have included it.  
> The extern "C" around the include of filenames.h in import.cc is 
> suspicious in its own way; libiberty headers such as that should contain 
> their own extern "C" declaration, as indeed that one does (and in any case 
> system.h already includes filenames.h so you don't need to include it 
> again).

All fixed.


> expressions.cc in various places passes a diagnostic argument of error_at 
> to _().  error_at translates its gmsgid operand and it should not be 
> passed to _() a second time.

Fixed.


> In most places expressions.cc uses _() on the operand of 
> this->report_error.  This is correct for the present definition of 
> Expression::report_error which passes the string to a "%s" format.  But 
> one "floating point constant truncated to integer" message does not use 
> _().  And another message "left operand of %<<-%> must be channel" is 
> actually a format string, which isn't going to work correctly with the 
> present code.  I think Expression::report_error should pass the received 
> string directly as a format string to error_at, so allowing %< and %> to 
> work, and should be set up so that _() markup is not needed for exgettext 
> to extract its argument for translation.  A similar format string issue 
> appears in lex.cc,
>
>           this->error("invalid character after %<\\%>");

Fixed, although I didn't attempt to make Expression::report_error and
friends handle markup directly.


> You've mentioned that Go allows UTF-8 identifiers.  For printing these in 
> diagnostics you need to pass them through identifier_to_locale so that 
> they are printed in the user's locale character set, which may not be 
> UTF-8.  (When the %E format is used, the core code handles this 
> automatically.)

Done.


> What target macros are the tm_p includes in expressions.cc and 
> gogo-tree.cc needed for?  Can the uses of those target macros be 
> abstracted from the front-end code?

In expressions.cc it is ADJUST_FIELD_ALIGN.  I removed the #include of
tm_p.h from gogo-tree.cc, but I note in passing that it #include's tm.h
because of OBJECT_FORMAT_ELF.


> The front end adds three new uses of atoi, which has undefined behavior if 
> the input number is out of range.  Though the use of this function is a 
> pre-existing condition (bug 44574) so we can't yet poison it in system.h, 
> adding more uses seems bad and they should be easy to avoid.  On a similar 
> note, strtol is used in lex.cc with no check for overflow (including a 
> value outside the range of linenum_type = unsigned int, the type passed to 
> linemap_add).

I changed atoi to strtol and added overflow checks.


> It is generally preferred to use %m in diagnostics instead of directly 
> using strerror; if strerror is needed for some reason (e.g. passing to a 
> printf function rather than to the GCC pretty-printing machinery; the 
> latter has %m, the former may not), use the xstrerror wrapper instead.  
> See my patch <http://gcc.gnu.org/ml/gcc-patches/2010-05/msg02226.html>.

Changed to use %m.


> In various places you open what I think may be binary files with O_RDONLY 
> - don't you need to use O_BINARY as well (defining it to 0 if not already 
> defined) for portability to Windows hosts?

O_BINARY added.


> In import.cc you form filenames in directories using a hardcoded '/'.  I 
> think using this instead of DIR_SEPARATOR may be safe given that cpplib 
> (append_file_to_dir) does it - but do you need the append_file_to_dir 
> checks relating to "//"?

I implemented the same algorithm as append_file_to_dir.  I don't know
whether it matters but it can't hurt.


> Does the ".o" handling in import.cc need to use TARGET_OBJECT_SUFFIX (and 
> so a target macro dependency unless you abstract it out or make it a 
> hook)?  Do the VMS people have any comments on this?

I have punted on this for now as I'm unsure about the current search
algorithm.  In practice most code uses a .a file or a .gox file, not a
.o file.


> Where did the lists of Unicode letters and digits in lex.cc come from?  
> Some particular Unicode version?  A generator program?  Does Go follow the 
> recommendations of ISO/IEC TR 10176:2003 regarding what characters to 
> allow in identifiers?  (That TR is freely available from 
> <http://standards.iso.org/ittf/PubliclyAvailableStandards/index.html>.)

Those lists come from Unicode 5.2.0.

I don't think Go follows the exact recommendations of that TR.  The Go
spec (http://golang.org/doc/go_spec.html) says

    Identifiers name program entities such as variables and types. An
    identifier is a sequence of one or more letters and digits.  The
    first character in an identifier must be a letter.

unicode_char   = /* an arbitrary Unicode code point */ .
unicode_letter = /* a Unicode code point classified as "Letter" */ .
unicode_digit  = /* a Unicode code point classified as "Digit" */ .

letter        = unicode_letter | "_" .

identifier = letter { letter | unicode_digit } .

The TR seems to recommend accepting some combining characters, but since
they are not classed as "Letter" Go does not currently accept them in
identifiers.


Thanks again for the review.  All changes committed to the  gccgo
branch.

Ian