Re: [PATCH v2] Add debug stream parameter to init_jit()

[Paulo César Pereira de Andrade]

Em qui., 9 de nov. de 2023 às 12:07, Paul Cercueil
<paul@crapouillou.net> escreveu:
>
> Hi Paulo,
>
> Le jeudi 09 novembre 2023 à 11:58 -0300, Paulo César Pereira de Andrade
> a écrit :
> > Em qui., 9 de nov. de 2023 às 10:42, Paul Cercueil
> > <paul@crapouillou.net> escreveu:
> > >
> > > Allow specifying where Lightning's messages and disassembly will be
> > > printed, instead of inconditionally using the error output.
> > >
> > > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >
> >   Hi Paul,
> >
> > > ---
> > > v2: init_jit() is now a macro that resolves to
> > > init_jit_with_debug().
> > > This allows keeping API compatibility with current software.
> > > ---
> > >  include/lightning.h.in          | 4 +++-
> > >  include/lightning/jit_private.h | 4 ++--
> > >  lib/jit_disasm.c                | 6 +++---
> > >  lib/jit_print.c                 | 4 ++--
> > >  lib/lightning.c                 | 7 +++++--
> > >  5 files changed, 15 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/include/lightning.h.in b/include/lightning.h.in
> > > index 6d51235..7e9e7e8 100644
> > > --- a/include/lightning.h.in
> > > +++ b/include/lightning.h.in
> > > @@ -23,6 +23,7 @@
> > >  #include <unistd.h>
> > >  #include <stdlib.h>
> > >  @MAYBE_INCLUDE_STDINT_H@
> > > +#include <stdio.h>
> > >  #include <string.h>
> > >  #include <pthread.h>
> > >
> > > @@ -1220,7 +1221,8 @@ typedef void  (*jit_free_func_ptr)
> > > (void*);
> > >  /*
> > >   * Prototypes
> > >   */
> > > -extern void init_jit(const char*);
> > > +extern void init_jit_with_debug(const char*,FILE*);
> > > +#define init_jit(progname) init_jit_with_debug(progname, NULL)
> >
> >   This still breaks abi. I understand Lightning does not have much
> > usage
> > and just recompiling the code should be good enough.
> >
> >   I can apply it, but it would require a full rebuild. After this
> > patch, existing
> > code would fail to load with an unresolved reference to init_jit,
> > until
> > rebuilt and relinked.
>
> Yes, I know. It is API-compatible, not ABI-compatible.
>
> But it wasn't really a concern before, was it? Looking at your commit
> 26f864b from last April, where you re-ordered the opcodes for no
> particular reason; that definitely did break the ABI.

  That depends on usage feedback :) Now I believe there are more active
users.

  I agree it was not so pretty, and should have been deferred to a newer
library major bump.

  I was about to commit v2, but did make the comment just to make sure you
did understand the problem. Sorry.

> Cheers,
> -Paul
>
> >
> > >  extern void finish_jit(void);
> > >
> > >  extern jit_state_t *jit_new_state(void);
> > > diff --git a/include/lightning/jit_private.h
> > > b/include/lightning/jit_private.h
> > > index 9f8caf6..a730d73 100644
> > > --- a/include/lightning/jit_private.h
> > > +++ b/include/lightning/jit_private.h
> > > @@ -873,8 +873,8 @@ _emit_ldxi_d(jit_state_t*, jit_int32_t,
> > > jit_int32_t, jit_word_t);
> > >  extern void
> > >  _emit_stxi_d(jit_state_t*, jit_word_t, jit_int32_t, jit_int32_t);
> > >
> > > -extern void jit_init_print(void);
> > > -extern void jit_init_debug(const char*);
> > > +extern void jit_init_print(FILE*);
> > > +extern void jit_init_debug(const char*, FILE*);
> > >  extern void jit_finish_debug(void);
> > >
> > >  extern void jit_init_note(void);
> > > diff --git a/lib/jit_disasm.c b/lib/jit_disasm.c
> > > index 456b4f6..90d90b0 100644
> > > --- a/lib/jit_disasm.c
> > > +++ b/lib/jit_disasm.c
> > > @@ -71,9 +71,9 @@ static int fprintf_styled(void * stream, enum
> > > disassembler_style style, const ch
> > >   * Implementation
> > >   */
> > >  void
> > > -jit_init_debug(const char *progname)
> > > +jit_init_debug(const char *progname, FILE *stream)
> > >  {
> > > -    jit_init_print();
> > > +    jit_init_print(stream);
> > >  #if DISASSEMBLER
> > >      bfd_init();
> > >
> > > @@ -89,7 +89,7 @@ jit_init_debug(const char *progname)
> > >      bfd_check_format(disasm_bfd, bfd_object);
> > >      bfd_check_format(disasm_bfd, bfd_archive);
> > >      if (!disasm_stream)
> > > -       disasm_stream = stderr;
> > > +       disasm_stream = stream;
> > >
> > >  #if BINUTILS_2_38
> > >      INIT_DISASSEMBLE_INFO(disasm_info, disasm_stream, fprintf,
> > > fprintf_styled);
> > > diff --git a/lib/jit_print.c b/lib/jit_print.c
> > > index 3a16c64..079112b 100644
> > > --- a/lib/jit_print.c
> > > +++ b/lib/jit_print.c
> > > @@ -75,10 +75,10 @@ static FILE *print_stream;
> > >   * Implementation
> > >   */
> > >  void
> > > -jit_init_print(void)
> > > +jit_init_print(FILE *stream)
> > >  {
> > >      if (!print_stream)
> > > -       print_stream = stderr;
> > > +       print_stream = stream;
> > >  }
> > >
> > >  void
> > > diff --git a/lib/lightning.c b/lib/lightning.c
> > > index 39c4fc2..af64c88 100644
> > > --- a/lib/lightning.c
> > > +++ b/lib/lightning.c
> > > @@ -181,10 +181,13 @@ _patch_register(jit_state_t *jit, jit_node_t
> > > *node, jit_node_t *link,
> > >   * Implementation
> > >   */
> > >  void
> > > -init_jit(const char *progname)
> > > +init_jit_with_debug(const char *progname, FILE *dbg_out)
> > >  {
> > > +    if (!dbg_out)
> > > +        dbg_out = stderr;
> > > +
> > >      jit_get_cpu();
> > > -    jit_init_debug(progname);
> > > +    jit_init_debug(progname, dbg_out);
> > >      jit_init_size();
> > >  }
> > >
> > > --
> > > 2.42.0
> > >

Thanks!
Paulo