This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH] Merger of the "dmalcolm/jit" branch
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: David Malcolm <dmalcolm at redhat dot com>
- Cc: <gcc-patches at gcc dot gnu dot org>, <jit at gcc dot gnu dot org>
- Date: Tue, 23 Sep 2014 23:27:25 +0000
- Subject: Re: [PATCH] Merger of the "dmalcolm/jit" branch
- Authentication-results: sourceware.org; auth=none
- References: <1411497912 dot 16438 dot 27 dot camel at surprise>
Various *_finalize functions are missing comments explaining their
semantics. Also the return type should be on the line before the function
Shouldn't the jit.pdf, jit.install-html etc. Make-lang.in hooks actually
build / install the documentation for this JIT?
> +#include "config.h"
> +#include "system.h"
> +#include "ansidecl.h"
> +#include "coretypes.h"
The standard initial includes are config.h, system.h, coretypes.h.
system.h includes libiberty.h which includes ansidecl.h, so direct
ansidecl.h includes shouldn't be needed anywhere.
> diff --git a/gcc/jit/internal-api.c b/gcc/jit/internal-api.c
Should start with standard copyright and license header. This applies to
all sources in gcc/jit/.
recording::string::from_printf all use fixed-size buffers with vsnprintf
but no apparent reason to assume this can never result in truncation, and
missing API comments (lots of functions are missing such comments ...) to
state either the caller's responsibility to limit the length of the
result, or that the API provides for truncation. Unless there's a
definite reason truncation is needed, dynamic allocation should be used.
A patch was submitted a while back to add xasprintf and xvasprintf to
libiberty - <https://gcc.gnu.org/ml/gcc-patches/2009-11/msg01448.html> and
<https://gcc.gnu.org/ml/gcc-patches/2009-11/msg01449.html> (I don't know
if that's the most recent version), which could be resurrected.
The code for compiling a .s file should:
* use choose_tmpdir from libiberty rather than hardcoding /tmp (or,
better, create the files directly with make_temp_file, and delete them
* use libiberty's pexecute to run subprocesses, not "system" (building up
a string to pass to the shell always looks like a security hole, though in
this case it may in fact be safe);
* use the $(target_noncanonical)-gcc-$(version) name for the driver rather
than plain "gcc", to maximise the chance that it is actually the same
compiler the JIT library was built for (I realise you may not actually
depend on it being the same compiler, but that does seem best; in
principle in future it should be possible to load multiple copies of the
JIT library to JIT for different targets, so that code for an offload
accelerator can go through the JIT).
The documentation referring to the dmalcolm/jit branch will of course need
updating to refer to trunk (and GCC 5 and later releases) once this is on
Joseph S. Myers