Re: [PATCH] Merger of the "dmalcolm/jit" branch

["Joseph S. Myers" <joseph@codesourcery.com>]

Various *_finalize functions are missing comments explaining their 
semantics.  Also the return type should be on the line before the function 
name.

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/.

dump::write, recording::context::add_error_va, 
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 
individual afterwards);

* 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 
trunk.

-- 
Joseph S. Myers
joseph@codesourcery.com