Go front end review: files outside gofrontend/
Ian Lance Taylor
iant@google.com
Wed Nov 17 00:42:00 GMT 2010
"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
More information about the Gcc-patches
mailing list