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

[Paulo César Pereira de Andrade]

Em sáb., 14 de out. de 2023 às 04:04, Basile Starynkevitch
<basile@starynkevitch.net> escreveu:
>
>
> On 10/13/23 20:15, Paulo César Pereira de Andrade wrote:
>
> Hi Paul. Please sendo the v2 and I will apply the patch during the weekend.
>
> Em qua, 11 de out de 2023 12:57, Paul Cercueil <paul@crapouillou.net> 
> escreveu:
>>
>> Ooops, I missed to update the source files in check/.
>>
>> I'll send a V2.
>>
>> -Paul
>>
>> Le mercredi 11 octobre 2023 à 17:47 +0200, Paul Cercueil a écrit :
>> > Allow specifying where Lightning's messages and disassembly will be
>> > printed, instead of inconditionally using the error output.
>> > diff --git a/include/lightning.h.in b/include/lightning.h.in
>> > index 6d51235..25f685b 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,7 @@ typedef void
>> > (*jit_free_func_ptr)        (void*);
>> >  /*
>> >   * Prototypes
>> >   */
>> > -extern void init_jit(const char*);
>> > +extern void init_jit(const char*,FILE*);
>> >  extern void finish_jit(void);
>> >
>
>
> This patch is interesting and provides an interesting feature. However, it is 
> changing the public API of GNU lightning in an incompatible way.

  I did not like it much when first looking at it due to the clear abi break.
  But the feature is very useful. It just sets the FILE* where binutils is
told to print the disassembly, as well as where debug printed by Lightning
is also output.

> For projects (like the RefPerSys open source inference engine on 
> https://github.com/RefPerSys/RefPerSys/ ....) which are using GNU lightning 
> it would be very convenient to have a preprocessor convention (inspired by 
> libgccjit on https://gcc.gnu.org/onlinedocs/jit/ ...) which enables users of 
> GNU lightning to test it.
>
>
> What about adding in include/lightning.h.in some preprocessor symbols like
>
> #define LIB_LIGHTNING_API 1
>
> and increment that number every time the external API of GNU lightning is 
> changing?
>
> This would make using GNU lightning easier.

  I believe this is good, but should be avoided and only done when
really required. For now the proper approach should be to make a
new function call, to set the disasm_stream pointer in jit_disasm.c.

> Thanks
>
> --
> Basile Starynkevitch
>  <basile@starynkevitch.net>
> (only mine opinions / les opinions sont miennes uniquement)
> 92340 Bourg-la-Reine, France
> web page: starynkevitch.net/Basile/

Thanks,
Paulo