Re: [PATCH 2/4] Populates jit-reader.h.in

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Sanjoy" == Sanjoy Das <sanjoy@playingwithpointers.com> writes:

Sanjoy> Adds the interface to be implemented by the JIT debug-info
Sanjoy> readers and the API they can use to jit-reader.h.in.

This looks pretty good.

I like the comments quite a bit, thanks for making the extra effort
here.

Sanjoy> +/* Return status codes. */
Sanjoy> +enum {
Sanjoy> +  GDB_FAIL = 0,
Sanjoy> +  GDB_SUCCESS = 1
Sanjoy> +};

Give this enum a tag and then use it as the return type in those
functions currently documented as returning GDB_FAIL or GDB_SUCCESS.

Sanjoy> +#define GDB_MAX_REGISTER_SIZE 64 /* Mirrors the internal GDB definition. */

A concern I have is that we may bump this value in future versions of
GDB.  (IIUC, internally it is really just a convenience so we can
allocate temporary arrays of the right size on the stack.)

I think it would be good to try to address this problem before it bites.
One way to do that would be to put the register size into one of the
structures, rather than having a constant.  E.g., there could be another
field in gdb_reg_value; or maybe some other way.

Sanjoy> +  /* For internal use. */
Sanjoy> +  void *private;

It isn't clear whose internal use these fields are for -- gdb or the jit
plugin?  I suggest changing each one to read "For use by GDB" or "For
use by the plugin" as appropriate.

I didn't see anything in the patch series to actually wire up the JIT
code, so it is hard to fully review this patch.  (I vaguely recall
seeing this in the earlier series -- but a resubmit should ideally be
complete.)

Sanjoy> +extern struct gdb_reader_funcs *gdb_init_reader (void);

It seems to me that we could put the API version into the returned
gdb_reader_funcs, to be checked by GDB.  Then we don't need a special
#define as in patch #1, just an ordinary version number.

Tom