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

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

[Basile Starynkevitch]

[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]


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 <http://lightning.h.in>
>     b/include/lightning.h.in <http://lightning.h.in>
>     > index 6d51235..25f685b 100644
>     > --- a/include/lightning.h.in <http://lightning.h.in>
>     > +++ b/include/lightning.h.in <http://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.

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.


Thanks

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

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

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

[Paul Cercueil]

Hi,

Le 16 octobre 2023 01:55:41 GMT+02:00, "Paulo César Pereira de Andrade" <paulo.cesar.pereira.de.andrade@gmail.com> a écrit :
>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.

I can make init_jit() a macro that would call a init_jit_dbg() function (the current init_jit() renamed) with stderr as parameter.

Cheers,
-Paul

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