Correct byte compiler error/warning positions. The solution!

Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Emacs.

A short while ago, I mused on a solution to the incorrect positions we
get in byte compiler warning/error messages.  My proposed solution
turned out to be impractical fantasy - it didn't and couldn't work,
because of the difficulties of getting macros to play nicely.

However, I've now hit on the answer.

My 2018 solution to the problem _was_ a solution.  It just ran somewhere
around 8% more slowly than the original it was branched from, hence was
deemed unacceptable by other hackers here.

The reason for the slowdown was the modification to the EQ macro in
lisp.h.  It became necessary to check whether "symbols with position"
were enabled whenever the first == comparison didn't match.

A _LOT_ of these comparisons were for NILP, which is just #defined as EQ
(arg, Qnil).  However if NILP were to be defined directly as arg ==
Qnil, it would bypass the overhead in EQ.  Also required here is for the
reader to handle a literal "nil" specially, and return the
non-positioned symbol `nil' for it, rather than a positioned `nil'.

I've just tried out the above on my 2018 proposed code.  On a benchmark,
the new attempt ran 7.3% faster than the 2018 version.  It thus seems
likely that this strategem will give correct positions in warning
messages without such an unacceptable slowdown.

I'm going to try it out on an up to date repository.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Eli Zaretskii]

> Date: Fri, 26 Nov 2021 19:56:21 +0000
> From: Alan Mackenzie <acm@muc.de>
> 
> A _LOT_ of these comparisons were for NILP, which is just #defined as EQ
> (arg, Qnil).  However if NILP were to be defined directly as arg ==
> Qnil, it would bypass the overhead in EQ.

How can you use == when the operands could be structures?

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Eli.

On Sat, Nov 27, 2021 at 07:53:36 +0200, Eli Zaretskii wrote:
> > Date: Fri, 26 Nov 2021 19:56:21 +0000
> > From: Alan Mackenzie <acm@muc.de>

> > A _LOT_ of these comparisons were for NILP, which is just #defined as EQ
> > (arg, Qnil).  However if NILP were to be defined directly as arg ==
> > Qnil, it would bypass the overhead in EQ.

> How can you use == when the operands could be structures?

I meant that NILP will be implemented as a binary comparison against
zero, omitting the awkward test for symbols_with_position_enabled.

It would be something more like:

#define lisp_h_NILP(x) BASE_EQ (x, Qnil)

..  This works, and saves ~7% performance on a particular benchmark.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Eli Zaretskii]

> Date: Sat, 27 Nov 2021 09:31:05 +0000
> Cc: emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> Hello, Eli.
> 
> On Sat, Nov 27, 2021 at 07:53:36 +0200, Eli Zaretskii wrote:
> > > Date: Fri, 26 Nov 2021 19:56:21 +0000
> > > From: Alan Mackenzie <acm@muc.de>
> 
> > > A _LOT_ of these comparisons were for NILP, which is just #defined as EQ
> > > (arg, Qnil).  However if NILP were to be defined directly as arg ==
> > > Qnil, it would bypass the overhead in EQ.
> 
> > How can you use == when the operands could be structures?
> 
> I meant that NILP will be implemented as a binary comparison against
> zero, omitting the awkward test for symbols_with_position_enabled.

Please don't write code that assumes Qnil is zero.  (Or maybe I still
misunderstand what exactly you mean by the above.)

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Eli.

On Sat, Nov 27, 2021 at 12:07:10 +0200, Eli Zaretskii wrote:
> > Date: Sat, 27 Nov 2021 09:31:05 +0000
> > Cc: emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > Hello, Eli.

> > On Sat, Nov 27, 2021 at 07:53:36 +0200, Eli Zaretskii wrote:
> > > > Date: Fri, 26 Nov 2021 19:56:21 +0000
> > > > From: Alan Mackenzie <acm@muc.de>

> > > > A _LOT_ of these comparisons were for NILP, which is just #defined as EQ
> > > > (arg, Qnil).  However if NILP were to be defined directly as arg ==
> > > > Qnil, it would bypass the overhead in EQ.

> > > How can you use == when the operands could be structures?

> > I meant that NILP will be implemented as a binary comparison against
> > zero, omitting the awkward test for symbols_with_position_enabled.

> Please don't write code that assumes Qnil is zero.  (Or maybe I still
> misunderstand what exactly you mean by the above.)

Don't worry!  The new code merely assumes Qnil is a symbol, and leaves it
to the compiler to optimise for Qnil being binary zero.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Eli Zaretskii]

> Date: Sat, 27 Nov 2021 10:33:07 +0000
> Cc: emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > > I meant that NILP will be implemented as a binary comparison against
> > > zero, omitting the awkward test for symbols_with_position_enabled.
> 
> > Please don't write code that assumes Qnil is zero.  (Or maybe I still
> > misunderstand what exactly you mean by the above.)
> 
> Don't worry!  The new code merely assumes Qnil is a symbol, and leaves it
> to the compiler to optimise for Qnil being binary zero.

Once again, you cannot compare structures with == in portable C.  And
the Lisp_Object type can be struct.  As long as what you say above
doesn't mean that you compare with ==, I'm okay with what you propose.

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Eli.

On Sat, Nov 27, 2021 at 12:51:13 +0200, Eli Zaretskii wrote:
> > Date: Sat, 27 Nov 2021 10:33:07 +0000
> > Cc: emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > > I meant that NILP will be implemented as a binary comparison against
> > > > zero, omitting the awkward test for symbols_with_position_enabled.

> > > Please don't write code that assumes Qnil is zero.  (Or maybe I still
> > > misunderstand what exactly you mean by the above.)

> > Don't worry!  The new code merely assumes Qnil is a symbol, and leaves it
> > to the compiler to optimise for Qnil being binary zero.

> Once again, you cannot compare structures with == in portable C.  And
> the Lisp_Object type can be struct.  As long as what you say above
> doesn't mean that you compare with ==, I'm okay with what you propose.

Sorry, I was perhaps a wee bit too informal when I said "==".  The
actual code in lisp.h looks like this:

    #define lisp_h_NILP(x) ((XLI (x) == XLI (Qnil)))

..

Anyhow, it's now working!  I can bootstrap the new code, although I
haven't tried it with native compilation, yet.  For timings, I've used
my favourite informal benchmark, M-; (time-scroll) on xdisp.c:

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
(defmacro time-it (&rest forms)
  "Time the running of a sequence of forms using `float-time'.
Call like this: \"M-: (time-it (foo ...) (bar ...) ...)\"."
  `(let ((start (float-time)))
    ,@forms
    (- (float-time) start)))

(defun time-scroll (&optional arg)
  (interactive "P")
  (message "%s"
           (time-it
            (condition-case nil
                (while t
                  (if arg (scroll-down) (scroll-up))
                  (sit-for 0))
              (error nil)))))
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

The timings I have for scrolling in the forward direction (each time
preceded by M-x garbage-collect, and typing/erasing a character to clear
the font-lock settings) are:

(master branch, no native compilation):
20.847s, 21.609s, 19.459s, 21.570s, 21.651

(New code with correct warning messages, no native compilation):
21.284s, 42.152s ????, 19.927s, 19.960s, 22.259s, 22.198, 22.209s.

The input to configure was the same in both cases, namely

  ./configure --with-gif=no --with-tiff=no --with-gpm.

, and the timings were done on the Linux console.

No, I don't understand that 42s run.  A lot of garbage collection,
perhaps?  But otherwise, the timings are broadly similar, with the new
code being slightly slower.  But the variations between runs is more
significant than the variation between versions.

On bug ~9109 from Roland Winkler:

(unwind-protect
    (let ((foo "foo"))
      (insert foo))
  (setq foo "bar"))

Byte compiling this with M-x compile-defun on master produces this
warning message:

  Buffer winkler.el:2:12: Warning: assignment to free variable `foo'

The position 2:12 is clearly wrong.

Byte compiling it with the new code produces:

  Buffer winkler.el:2:12:4:9: Warning: assignment to free variable
`#<symbol foo at 67>'.

There, there are two positions the (old, temporary) 2:12 and the (new,
correct) 4:9.  There is a bit of tidying up of the mechanism to be done
here, obviously, but the correct position is demonstrated.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Eli Zaretskii]

> Date: Sat, 27 Nov 2021 23:05:12 +0000
> Cc: emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> The timings I have for scrolling in the forward direction (each time
> preceded by M-x garbage-collect, and typing/erasing a character to clear
> the font-lock settings) are:
> 
> (master branch, no native compilation):
> 20.847s, 21.609s, 19.459s, 21.570s, 21.651
> 
> (New code with correct warning messages, no native compilation):
> 21.284s, 42.152s ????, 19.927s, 19.960s, 22.259s, 22.198, 22.209s.
> 
> The input to configure was the same in both cases, namely
> 
>   ./configure --with-gif=no --with-tiff=no --with-gpm.
> 
> , and the timings were done on the Linux console.
> 
> No, I don't understand that 42s run.  A lot of garbage collection,
> perhaps?

You should always record the time take by GC and subtract it from the
elapsed time.  I believe benchmark-run shows that time, so please use
it in the comparison.

Re: Correct byte compiler error/warning positions. The solution!

[Andrea Corallo]

Hi Alan,

I think would be interesting to have a comparative run also using
<https://elpa.gnu.org/packages/elisp-benchmarks.html>

  Andrea

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Eli.

On Sun, Nov 28, 2021 at 09:25:03 +0200, Eli Zaretskii wrote:
> > Date: Sat, 27 Nov 2021 23:05:12 +0000
> > Cc: emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > The timings I have for scrolling in the forward direction (each time
> > preceded by M-x garbage-collect, and typing/erasing a character to clear
> > the font-lock settings) are:

> > (master branch, no native compilation):
> > 20.847s, 21.609s, 19.459s, 21.570s, 21.651

> > (New code with correct warning messages, no native compilation):
> > 21.284s, 42.152s ????, 19.927s, 19.960s, 22.259s, 22.198, 22.209s.

> > The input to configure was the same in both cases, namely

> >   ./configure --with-gif=no --with-tiff=no --with-gpm.

> > , and the timings were done on the Linux console.

> > No, I don't understand that 42s run.  A lot of garbage collection,
> > perhaps?

> You should always record the time take by GC and subtract it from the
> elapsed time.  I believe benchmark-run shows that time, so please use
> it in the comparison.

OK, I've done that a few times, thanks.  There were 97 GCs in a run,
totalling around 5 seconds.  I'll try it more systematically some time.

I've become a little confused over the "==" in lisp_h_NILP that we were
discussing.  Omitting the XLI calls causes GCC 10.3.0 to build a smaller
executable.  (This was with debugging info switched off.)  I realise this
is not acceptable, since there are other systems this wouldn't compile
under.

Anyhow, I've committed the current state in the new branch
scratch/correct-warning-pos.  It should build and run OK, although I
haven't tried it out with native compilation, yet.  It is marginally
slower than master.  Maybe we can merge it into master some time for
Emacs 29.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Eli Zaretskii]

> Date: Mon, 29 Nov 2021 11:50:19 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: emacs-devel@gnu.org
> 
> Anyhow, I've committed the current state in the new branch
> scratch/correct-warning-pos.  It should build and run OK, although I
> haven't tried it out with native compilation, yet.  It is marginally
> slower than master.  Maybe we can merge it into master some time for
> Emacs 29.

Please show the benchmark results, so we could know how slower is
this.

Thanks.

Re: Correct byte compiler error/warning positions. The solution!

[Robert Pluim]

>>>>> On Mon, 29 Nov 2021 11:50:19 +0000, Alan Mackenzie <acm@muc.de> said:
    Alan> Anyhow, I've committed the current state in the new branch
    Alan> scratch/correct-warning-pos.  It should build and run OK, although I
    Alan> haven't tried it out with native compilation, yet.  It is marginally
    Alan> slower than master.  Maybe we can merge it into master some time for
    Alan> Emacs 29.

"Thou shalt listen to Eli's warnings about '==', else your branch
shall not compile"

./configure --enable-check-lisp-object-type --enable-checking

make[1]: Entering directory '/home/rpluim/repos/emacs-pos/src'
  CC       dispnew.o
In file included from dispnew.c:27:
lisp.h: In function ‘EQ’:
lisp.h:385:40: error: invalid operands to binary == (have ‘Lisp_Object’ and ‘Lisp_Object’)
  385 |         ? (XSYMBOL_WITH_POS((x)))->sym == (y)          \
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^~
      |                                  |
      |                                  Lisp_Object
lisp.h:1329:10: note: in expansion of macro ‘lisp_h_EQ’
 1329 |   return lisp_h_EQ (x, y);
      |          ^~~~~~~~~
lisp.h:388:15: error: invalid operands to binary == (have ‘Lisp_Object’ and ‘Lisp_Object’)
  387 |           && ((XSYMBOL_WITH_POS((x)))->sym                   \
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                      |
      |                                      Lisp_Object
  388 |               == (XSYMBOL_WITH_POS((y)))->sym)               \
      |               ^~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                         |
      |                                         Lisp_Object
lisp.h:1329:10: note: in expansion of macro ‘lisp_h_EQ’
 1329 |   return lisp_h_EQ (x, y);
      |          ^~~~~~~~~
lisp.h:391:18: error: invalid operands to binary == (have ‘Lisp_Object’ and ‘Lisp_Object’)
  391 |          && ((x) == ((XSYMBOL_WITH_POS ((y)))->sym))))))
      |                  ^~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                              |
      |                                              Lisp_Object
lisp.h:1329:10: note: in expansion of macro ‘lisp_h_EQ’
 1329 |   return lisp_h_EQ (x, y);
      |          ^~~~~~~~~
In file included from dispnew.c:27:
lisp.h:1330:1: warning: control reaches end of non-void function [-Wreturn-type]
 1330 | }
      | ^
make[1]: *** [Makefile:406: dispnew.o] Error 1

Robert
-- 

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Robert.

On Mon, Nov 29, 2021 at 14:24:47 +0100, Robert Pluim wrote:
> >>>>> On Mon, 29 Nov 2021 11:50:19 +0000, Alan Mackenzie <acm@muc.de> said:
>     Alan> Anyhow, I've committed the current state in the new branch
>     Alan> scratch/correct-warning-pos.  It should build and run OK, although I
>     Alan> haven't tried it out with native compilation, yet.  It is marginally
>     Alan> slower than master.  Maybe we can merge it into master some time for
>     Alan> Emacs 29.

> "Thou shalt listen to Eli's warnings about '==', else your branch
> shall not compile"

:-)

> ./configure --enable-check-lisp-object-type --enable-checking

> make[1]: Entering directory '/home/rpluim/repos/emacs-pos/src'
>   CC       dispnew.o
> In file included from dispnew.c:27:
> lisp.h: In function ‘EQ’:
> lisp.h:385:40: error: invalid operands to binary == (have ‘Lisp_Object’ and ‘Lisp_Object’)
>   385 |         ? (XSYMBOL_WITH_POS((x)))->sym == (y)          \
>       |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^~
>       |                                  |
>       |                                  Lisp_Object
> lisp.h:1329:10: note: in expansion of macro ‘lisp_h_EQ’
>  1329 |   return lisp_h_EQ (x, y);
>       |          ^~~~~~~~~
> lisp.h:388:15: error: invalid operands to binary == (have ‘Lisp_Object’ and ‘Lisp_Object’)
>   387 |           && ((XSYMBOL_WITH_POS((x)))->sym                   \
>       |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       |                                      |
>       |                                      Lisp_Object
>   388 |               == (XSYMBOL_WITH_POS((y)))->sym)               \
>       |               ^~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       |                                         |
>       |                                         Lisp_Object
> lisp.h:1329:10: note: in expansion of macro ‘lisp_h_EQ’
>  1329 |   return lisp_h_EQ (x, y);
>       |          ^~~~~~~~~
> lisp.h:391:18: error: invalid operands to binary == (have ‘Lisp_Object’ and ‘Lisp_Object’)
>   391 |          && ((x) == ((XSYMBOL_WITH_POS ((y)))->sym))))))
>       |                  ^~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       |                                              |
>       |                                              Lisp_Object
> lisp.h:1329:10: note: in expansion of macro ‘lisp_h_EQ’
>  1329 |   return lisp_h_EQ (x, y);
>       |          ^~~~~~~~~
> In file included from dispnew.c:27:
> lisp.h:1330:1: warning: control reaches end of non-void function [-Wreturn-type]
>  1330 | }
>       | ^
> make[1]: *** [Makefile:406: dispnew.o] Error 1

Yes.  I think the patch below will fix this.

I must admit, I didn't know about --enable-check-lisp-object-type.  When
I tried it out with the fixed lisp_h_EQ, I got lots of other warnings.


But this option generates slower code.  On a 20 second benchmark, the
code was ~0.5s slower than when compiled without the option.  This
probably shouldn't happen, and I don't know why it does.

I need to tidy up the backslashes too.

Anyhow, here's the patch which at least keeps the compiler messages as
warnings:



diff --git a/src/lisp.h b/src/lisp.h
index 08013e94d1..00d9843d6a 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -366,7 +366,7 @@ #define LISP_WORDS_ARE_POINTERS (EMACS_INT_MAX == INTPTR_MAX)
 
 #define lisp_h_PSEUDOVECTORP(a,code)                            \
   (lisp_h_VECTORLIKEP((a)) &&                                   \
-   ((XUNTAG ((a), Lisp_Vectorlike, union vectorlike_header)->size       \
+   ((XUNTAG ((a), Lisp_Vectorlike, union vectorlike_header)->size     \
      & (PSEUDOVECTOR_FLAG | PVEC_TYPE_MASK))                    \
     == (PSEUDOVECTOR_FLAG | ((code) << PSEUDOVECTOR_AREA_BITS))))
 
@@ -382,13 +382,13 @@ #define lisp_h_EQ(x, y) ((XLI ((x)) == XLI ((y)))       \
   || (symbols_with_pos_enabled    \
   && (SYMBOL_WITH_POS_P ((x))                        \
       ? BARE_SYMBOL_P ((y))                               \
-        ? (XSYMBOL_WITH_POS((x)))->sym == (y)          \
+        ? XLI (XSYMBOL_WITH_POS((x))->sym) == XLI (y)           \
         : SYMBOL_WITH_POS_P((y))                       \
-          && ((XSYMBOL_WITH_POS((x)))->sym                   \
-              == (XSYMBOL_WITH_POS((y)))->sym)               \
+          && (XLI (XSYMBOL_WITH_POS((x))->sym)                   \
+              == XLI (XSYMBOL_WITH_POS((y))->sym))               \
       : (SYMBOL_WITH_POS_P ((y))                     \
          && BARE_SYMBOL_P ((x))                           \
-         && ((x) == ((XSYMBOL_WITH_POS ((y)))->sym))))))
+         && (XLI (x) == XLI ((XSYMBOL_WITH_POS ((y)))->sym))))))
 
 #define lisp_h_FIXNUMP(x) \
    (! (((unsigned) (XLI (x) >> (USE_LSB_TAG ? 0 : FIXNUM_BITS)) \

> Robert
> -- 

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Eli.

On Mon, Nov 29, 2021 at 14:45:01 +0200, Eli Zaretskii wrote:
> > Date: Mon, 29 Nov 2021 11:50:19 +0000
> > From: Alan Mackenzie <acm@muc.de>
> > Cc: emacs-devel@gnu.org

> > Anyhow, I've committed the current state in the new branch
> > scratch/correct-warning-pos.  It should build and run OK, although I
> > haven't tried it out with native compilation, yet.  It is marginally
> > slower than master.  Maybe we can merge it into master some time for
> > Emacs 29.

> Please show the benchmark results, so we could know how slower is
> this.

The source for the benchmarking is:

(defun time-scroll-b (&optional arg)    ; For use in `benchmark-run'.
  (condition-case nil
      (while t
        (if arg (scroll-down) (scroll-up))
        (sit-for 0))
    (error nil)))

I ran (benchmark-run (time-scroll-b)) five times on both versions of
Emacs, using the file src/xdisp.c from the version being tested, and
running on a Linux tty.  Between each run I did M-<, SPACE, pause ~5
seconds, C-_.

On the master branch I got the following timings:

    * - 1: (20.146470262 435 7.018855274999999)
    * - 2: (20.6936481 307 6.8447708129999985)
    * - 3: (20.748953179999997 303 6.931802685000001)
    * - 4: (20.754181744 303 6.932338166000001)
    * - 5: (20.746469523000002 304 6.927925281999997)

On the scratch/correct-warning-pos branch, I got these:

    * - 1: (20.200789011 446 7.2819411899999995)
    * - 2: (20.837616185999998 308 6.967083439000001)
    * - 3: (20.93961052 305 7.074547531)
    * - 4: (20.931170864 305 7.0736086979999975)
    * - 5: (20.853407755 304 7.029190317999998)

So, on this test the new branch appears to be around 1%, perhaps a
little less, slower than the master branch.

It is notable that the first run in each version is different from the
others, both in being a little faster, and having far more
garbage-collections.  I don't know why this is.  Maybe Emacs could be
marginally sped up by garbage collecting more frequently, but that's
speculation.

> Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Robert Pluim]

>>>>> On Mon, 29 Nov 2021 19:16:26 +0000, Alan Mackenzie <acm@muc.de> said:

    Alan> Yes.  I think the patch below will fix this.

    Alan> I must admit, I didn't know about --enable-check-lisp-object-type.  When
    Alan> I tried it out with the fixed lisp_h_EQ, I got lots of other warnings.

    Alan> But this option generates slower code.  On a 20 second benchmark, the
    Alan> code was ~0.5s slower than when compiled without the option.  This
    Alan> probably shouldn't happen, and I don't know why it does.

Itʼs a mandatory option to test when making changes to lisp object
representation, but you wouldnʼt use it in production, so it doesnʼt
matter if itʼs slightly slower.

    Alan> I need to tidy up the backslashes too.

    Alan> Anyhow, here's the patch which at least keeps the compiler messages as
    Alan> warnings:

Iʼm not getting any warnings at all with that patch, and the resulting
Emacs seems to work ok.

Robert
-- 

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Eli.

Ping?

On Mon, Nov 29, 2021 at 19:39:05 +0000, Alan Mackenzie wrote:
> On Mon, Nov 29, 2021 at 14:45:01 +0200, Eli Zaretskii wrote:
> > > Date: Mon, 29 Nov 2021 11:50:19 +0000
> > > From: Alan Mackenzie <acm@muc.de>
> > > Cc: emacs-devel@gnu.org

> > > Anyhow, I've committed the current state in the new branch
> > > scratch/correct-warning-pos.  It should build and run OK, although I
> > > haven't tried it out with native compilation, yet.  It is marginally
> > > slower than master.  Maybe we can merge it into master some time for
> > > Emacs 29.

> > Please show the benchmark results, so we could know how slower is
> > this.

> The source for the benchmarking is:

> (defun time-scroll-b (&optional arg)    ; For use in `benchmark-run'.
>   (condition-case nil
>       (while t
>         (if arg (scroll-down) (scroll-up))
>         (sit-for 0))
>     (error nil)))

> I ran (benchmark-run (time-scroll-b)) five times on both versions of
> Emacs, using the file src/xdisp.c from the version being tested, and
> running on a Linux tty.  Between each run I did M-<, SPACE, pause ~5
> seconds, C-_.

> On the master branch I got the following timings:

>     * - 1: (20.146470262 435 7.018855274999999)
>     * - 2: (20.6936481 307 6.8447708129999985)
>     * - 3: (20.748953179999997 303 6.931802685000001)
>     * - 4: (20.754181744 303 6.932338166000001)
>     * - 5: (20.746469523000002 304 6.927925281999997)

> On the scratch/correct-warning-pos branch, I got these:

>     * - 1: (20.200789011 446 7.2819411899999995)
>     * - 2: (20.837616185999998 308 6.967083439000001)
>     * - 3: (20.93961052 305 7.074547531)
>     * - 4: (20.931170864 305 7.0736086979999975)
>     * - 5: (20.853407755 304 7.029190317999998)

> So, on this test the new branch appears to be around 1%, perhaps a
> little less, slower than the master branch.

> It is notable that the first run in each version is different from the
> others, both in being a little faster, and having far more
> garbage-collections.  I don't know why this is.  Maybe Emacs could be
> marginally sped up by garbage collecting more frequently, but that's
> speculation.

> > Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Andrea Corallo]

Andrea Corallo <akrl@sdf.org> writes:

> Hi Alan,
>
> I think would be interesting to have a comparative run also using
> <https://elpa.gnu.org/packages/elisp-benchmarks.html>
>
>   Andrea

Adding on this,

given we should very carefully evaluate the performance impact (one
single benchmark is certainly not sufficient), I think we should stay
away for solutions adding performance cost to the run-time.

There's no question we'll have to pay a cost for this, but the other
solutions on the table (hashing conses or fat conses) are impacting only
the compile time and therefore IMO should definitely be preferred.

Regards

  Andrea

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Andrea.

On Wed, Dec 01, 2021 at 16:18:48 +0000, Andrea Corallo wrote:
> Andrea Corallo <akrl@sdf.org> writes:

> > Hi Alan,

> > I think would be interesting to have a comparative run also using
> > <https://elpa.gnu.org/packages/elisp-benchmarks.html>

> >   Andrea

> Adding on this,

> given we should very carefully evaluate the performance impact (one
> single benchmark is certainly not sufficient), I think we should stay
> away for solutions adding performance cost to the run-time.

Any help evaluating the performance of the scratch/correct-warning-pos
branch would certainly be appreciated.

> There's no question we'll have to pay a cost for this, but the other
> solutions on the table (hashing conses or fat conses) are impacting only
> the compile time and therefore IMO should definitely be preferred.

One great advantage of the current approach is that it's implemented
working code.  Well, almost (it doesn't yet work with native-compilation
enabled).

The disadvantage in performance, as measured so far by me, is so small
(less than 1%) as to be negligible.

The bug reports about warning positions being wrong have remained
unresolved for many years.  It is surely now time to fix them with
scratch/correct-warning-pos when it becomes stable.

If there come to be better ways of addressing the problem in the future,
then one of those ways could supersede the current proposal when it
becomes available.

> Regards

>   Andrea

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Eli Zaretskii]

> Date: Wed, 1 Dec 2021 15:58:26 +0000
> Cc: emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> Hello, Eli.
> 
> Ping?

Sorry: did you ask some question and wait for me to answer?  If so,
what was the question?

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Eli.

On Wed, Dec 01, 2021 at 18:49:23 +0200, Eli Zaretskii wrote:
> > Date: Wed, 1 Dec 2021 15:58:26 +0000
> > Cc: emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > Ping?

> Sorry: did you ask some question and wait for me to answer?  If so,
> what was the question?

You asked me for some benchmark figures comparing master with the
scratch/correct-warning-pos branch.  I supplied them, and they suggest
that the performance penalty in the new branch is small, around 1%.

I would like to merge this branch, when it becomes stable, into master,
thus finally resolving the bug of the wrong positions in warning
messages from the byte compiler.

Comments would be welcome.

Thanks!

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Lars Ingebrigtsen]

Alan Mackenzie <acm@muc.de> writes:

> I would like to merge this branch, when it becomes stable, into master,
> thus finally resolving the bug of the wrong positions in warning
> messages from the byte compiler.
>
> Comments would be welcome.

Would this mean that other parts of the byte compilation machinery would
need to be changed, since two symbols read while doing that won't be
`eq' any more?

Does the positions leak out from the symbols anywhere else, like when
doing reads outside the byte compilation process?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Correct byte compiler error/warning positions. The solution!

[Eli Zaretskii]

> Date: Wed, 1 Dec 2021 16:58:52 +0000
> Cc: emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> You asked me for some benchmark figures comparing master with the
> scratch/correct-warning-pos branch.  I supplied them, and they suggest
> that the performance penalty in the new branch is small, around 1%.

1% slowdown is insignificant.  But Andrea suggested to use an
additional suite of benchmarks, and I thought you were going to do
that as well.

> I would like to merge this branch, when it becomes stable, into master,
> thus finally resolving the bug of the wrong positions in warning
> messages from the byte compiler.

If all the benchmarks show a slowdown <= 1%, I'm okay with merging it.

Thanks.

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Eli.

On Wed, Dec 01, 2021 at 19:08:01 +0200, Eli Zaretskii wrote:
> > Date: Wed, 1 Dec 2021 16:58:52 +0000
> > Cc: emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > You asked me for some benchmark figures comparing master with the
> > scratch/correct-warning-pos branch.  I supplied them, and they suggest
> > that the performance penalty in the new branch is small, around 1%.

> 1% slowdown is insignificant.  But Andrea suggested to use an
> additional suite of benchmarks, and I thought you were going to do
> that as well.

I'm intending to.  At the moment, I'm looking at getting it working with
native-compilation.

> > I would like to merge this branch, when it becomes stable, into master,
> > thus finally resolving the bug of the wrong positions in warning
> > messages from the byte compiler.

> If all the benchmarks show a slowdown <= 1%, I'm okay with merging it.

Thanks!

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Lars.

On Wed, Dec 01, 2021 at 18:04:54 +0100, Lars Ingebrigtsen wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > I would like to merge this branch, when it becomes stable, into master,
> > thus finally resolving the bug of the wrong positions in warning
> > messages from the byte compiler.

> > Comments would be welcome.

> Would this mean that other parts of the byte compilation machinery would
> need to be changed, since two symbols read while doing that won't be
> `eq' any more?

The branch is working code, capable of bootstrapping Emacs, at least in
a non-native-code configuration.

lisp_h_EQ (in lisp.h) has been modified such that #<symbol foo at 100>
is `eq' #<symbol foo at 200>.

The compilation machinery is basically unchanged.

> Does the positions leak out from the symbols anywhere else, like when
> doing reads outside the byte compilation process?

`read' is as it always was.  There is a new function
`read-positioning-symbols' which returns symbols (apart from nil) as
symbols-with-position.  It is called only from the byte compiler.

Any such leakages are bugs to fix.  This we can do.

> -- 
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Lars Ingebrigtsen]

Alan Mackenzie <acm@muc.de> writes:

> lisp_h_EQ (in lisp.h) has been modified such that #<symbol foo at 100>
> is `eq' #<symbol foo at 200>.

Ah, I see.  I thought they'd differ.  Then I guess the fallout should be
minimal?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Correct byte compiler error/warning positions. The solution!

[Andrea Corallo]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> If all the benchmarks show a slowdown <= 1%, I'm okay with merging it.
>
> Thanks.

Another quick note, I think we should evaluate the impact not only with
different benchmarks but also using a native compiled build (ATM the
branch has no support for that).

Thanks

  Andrea

Re: Correct byte compiler error/warning positions. The solution!

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: Alan Mackenzie <acm@muc.de>, emacs-devel@gnu.org
> Date: Wed, 01 Dec 2021 17:53:03 +0000
> 
> Another quick note, I think we should evaluate the impact not only with
> different benchmarks but also using a native compiled build (ATM the
> branch has no support for that).

Alan said he is going to do that.

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Lars.

On Wed, Dec 01, 2021 at 18:38:57 +0100, Lars Ingebrigtsen wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > lisp_h_EQ (in lisp.h) has been modified such that #<symbol foo at 100>
> > is `eq' #<symbol foo at 200>.

> Ah, I see.  I thought they'd differ.  Then I guess the fallout should be
> minimal?

Indeed.  :-)

> -- 
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Andrea.

On Wed, Dec 01, 2021 at 17:53:03 +0000, Andrea Corallo wrote:
> Eli Zaretskii <eliz@gnu.org> writes:

> [...]

> > If all the benchmarks show a slowdown <= 1%, I'm okay with merging it.

> > Thanks.

> Another quick note, I think we should evaluate the impact not only with
> different benchmarks but also using a native compiled build (ATM the
> branch has no support for that).

The change to the scratch/correct-warning-pos branch to work with native
compilation is probably quite small, but I don't know the native code
compiler (comp.el, etc.) at all.

Would you help me with it, please.

The mechanism of the change was introducing @dfn{symbols with position}.
These are embodied in src/lisp.h with a new type tag
PVEC_SYMBOL_WITH_POS, and the type struct Lisp_Symbol_With_Pos.

The most pertinent changes in the branch are likewise those in
src/lisp.h.  There, there's a new flag variable,
symbols_with_pos_enabled, which is tested in the macros lisp_h_EQ,
lisp_h_SYMBOLP, and in the inline function XSYMBOL.  There are new
"primitive" macros, lisp_h_BASE_EQ, lisp_h_BARE_SYMBOL_P, and the inline
function XBARE_SYMBOL.  There are a few other things too, like
lisp_h_SYMBOL_WITH_POS_P.

All these changes can be seen with a git diff between the branch head
and the branch point in the master branch.

Thanks in advance for the help!

> Thanks

>   Andrea

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Andrea Corallo]

Alan Mackenzie <acm@muc.de> writes:

> Hello, Andrea.

Hi Alan,

> On Wed, Dec 01, 2021 at 17:53:03 +0000, Andrea Corallo wrote:
>> Eli Zaretskii <eliz@gnu.org> writes:
>
>> [...]
>
>> > If all the benchmarks show a slowdown <= 1%, I'm okay with merging it.
>
>> > Thanks.
>
>> Another quick note, I think we should evaluate the impact not only with
>> different benchmarks but also using a native compiled build (ATM the
>> branch has no support for that).
>
> The change to the scratch/correct-warning-pos branch to work with native
> compilation is probably quite small,

Not so sure about that

> but I don't know the native code
> compiler (comp.el, etc.) at all.
>
> Would you help me with it, please.

Sure

> The mechanism of the change was introducing @dfn{symbols with position}.
> These are embodied in src/lisp.h with a new type tag
> PVEC_SYMBOL_WITH_POS, and the type struct Lisp_Symbol_With_Pos.
>
> The most pertinent changes in the branch are likewise those in
> src/lisp.h.  There, there's a new flag variable,
> symbols_with_pos_enabled, which is tested in the macros lisp_h_EQ,
> lisp_h_SYMBOLP, and in the inline function XSYMBOL.  There are new
> "primitive" macros, lisp_h_BASE_EQ, lisp_h_BARE_SYMBOL_P, and the inline
> function XBARE_SYMBOL.  There are a few other things too, like
> lisp_h_SYMBOL_WITH_POS_P.
>
> All these changes can be seen with a git diff between the branch head
> and the branch point in the master branch.

The modifications needed are all and only going into comp.c.

The function you have to extend is 'emit_EQ'.  You'll see we have an
emit_* function for each corresponding macro/inline function used, ex:
'emit_XLI', 'emit_XCONS' etc...

You'll have to define all the new one needed in order to use them use
them in the new 'emit_EQ'.

Best Regards

  Andrea

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Andrea.

On Thu, Dec 02, 2021 at 16:31:55 +0000, Andrea Corallo wrote:
> Alan Mackenzie <acm@muc.de> writes:

> Hi Alan,

> > On Wed, Dec 01, 2021 at 17:53:03 +0000, Andrea Corallo wrote:
> >> Eli Zaretskii <eliz@gnu.org> writes:

> >> [...]

> >> > If all the benchmarks show a slowdown <= 1%, I'm okay with merging it.

> >> > Thanks.

> >> Another quick note, I think we should evaluate the impact not only with
> >> different benchmarks but also using a native compiled build (ATM the
> >> branch has no support for that).

> > The change to the scratch/correct-warning-pos branch to work with native
> > compilation is probably quite small,

> Not so sure about that

:-)

> > but I don't know the native code
> > compiler (comp.el, etc.) at all.

> > Would you help me with it, please.

> Sure

> > The mechanism of the change was introducing @dfn{symbols with position}.
> > These are embodied in src/lisp.h with a new type tag
> > PVEC_SYMBOL_WITH_POS, and the type struct Lisp_Symbol_With_Pos.

> > The most pertinent changes in the branch are likewise those in
> > src/lisp.h.  There, there's a new flag variable,
> > symbols_with_pos_enabled, which is tested in the macros lisp_h_EQ,
> > lisp_h_SYMBOLP, and in the inline function XSYMBOL.  There are new
> > "primitive" macros, lisp_h_BASE_EQ, lisp_h_BARE_SYMBOL_P, and the inline
> > function XBARE_SYMBOL.  There are a few other things too, like
> > lisp_h_SYMBOL_WITH_POS_P.

> > All these changes can be seen with a git diff between the branch head
> > and the branch point in the master branch.

> The modifications needed are all and only going into comp.c.

OK.

> The function you have to extend is 'emit_EQ'.  You'll see we have an
> emit_* function for each corresponding macro/inline function used, ex:
> 'emit_XLI', 'emit_XCONS' etc...

OK.

> You'll have to define all the new one needed in order to use them use
> them in the new 'emit_EQ'.

Thanks.  I'm currently going through the tutorial at
https://gcc.gnu.org/onlinedocs/jit/intro/tutorial....html, so it may be a
day or two before I start asking you questions.  ;-)

I can see already I'll need an emit_BASE_EQ (a renaming of the current
emit_EQ), and one or two others.

Hopefully, we'll get this up and running in a few days, or a week or two
at most.

> Best Regards

>   Andrea

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Andrea.

On Thu, Dec 02, 2021 at 20:35:50 +0000, Alan Mackenzie wrote:
> On Thu, Dec 02, 2021 at 16:31:55 +0000, Andrea Corallo wrote:
> > Alan Mackenzie <acm@muc.de> writes:

> > Hi Alan,

[ .... ]

> > The modifications needed are all and only going into comp.c.

> OK.

> > The function you have to extend is 'emit_EQ'.  You'll see we have an
> > emit_* function for each corresponding macro/inline function used, ex:
> > 'emit_XLI', 'emit_XCONS' etc...

> OK.

> > You'll have to define all the new one needed in order to use them use
> > them in the new 'emit_EQ'.

> Thanks.  I'm currently going through the tutorial at
> https://gcc.gnu.org/onlinedocs/jit/intro/tutorial....html, so it may be a
> day or two before I start asking you questions.  ;-)

For extending emit_EQ, I've got the following embryonic scheme:

1/- Import the C variable symbols_with_pos_enabled:

 gcc_jit_lvalue *syms_with_pos_enabled =
               gcc_jit_context_new_global
                 (comp.ctxt, NULL,
                  GCC_JIT_GLOBAL_IMPORTED,
                  gcc_jit_get_type (comp.ctxt, GCC_JIT_TYPE_BOOL),
                  "symbols_with_pos_enabled");

2/- Extend emit_EQ with lots of nested gcc_jit_context_new_comparison's
containing GCC_JIT_BINARY_OP_LOGICAL_OR/ANDs, something like this:

  emit_EQ (gcc_jit_rvalue *x, gcc_jit_rvalue *y)
  {
    emit_comment ("EQ");

    return gcc_jit_context_new_comparison (
             comp.ctxt,
             NULL,
             GIT_JIT_BINARY_OP_LOGICAL_OR (
               gcc_jit_context_new_comparison
                 (comp.ctxt,
                  NULL,
                  GCC_JIT_COMPARISON_EQ,
                  emit_XLI (x),
                  emit_XLI (y)),
               syms_with_pos_enabled
                 ....
                 ....
               ) ;
  }

2a/- I think I would need C macros called something like ELN_AND and
ELN_OR to make this half-way readable.

3/- Have several fragments of gcc_jit code which could be plugged into
the above, things representing BARE_SYMBOL_P (x), SYMBOL_WITH_POS_P (x),
and so on.

Can you let me know what you think of this approach, please?  That is,
before I put a lot of work into it.  Is it broadly a good way of doing
the job?

> I can see already I'll need an emit_BASE_EQ (a renaming of the current
> emit_EQ), and one or two others.

> Hopefully, we'll get this up and running in a few days, or a week or two
> at most.

I can't help noticing the lack of anything like SYMBOLP in comp.c.  Is
this just because nothing needs it?  Or is there some other method in
operation, like using the inline function in lisp.h somehow, for
example?

> > Best Regards

> >   Andrea

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Andrea Corallo]

Alan Mackenzie <acm@muc.de> writes:

> Hello, Andrea.
>
> On Thu, Dec 02, 2021 at 20:35:50 +0000, Alan Mackenzie wrote:
>> On Thu, Dec 02, 2021 at 16:31:55 +0000, Andrea Corallo wrote:
>> > Alan Mackenzie <acm@muc.de> writes:
>
>> > Hi Alan,
>
> [ .... ]
>
>> > The modifications needed are all and only going into comp.c.
>
>> OK.
>
>> > The function you have to extend is 'emit_EQ'.  You'll see we have an
>> > emit_* function for each corresponding macro/inline function used, ex:
>> > 'emit_XLI', 'emit_XCONS' etc...
>
>> OK.
>
>> > You'll have to define all the new one needed in order to use them use
>> > them in the new 'emit_EQ'.
>
>> Thanks.  I'm currently going through the tutorial at
>> https://gcc.gnu.org/onlinedocs/jit/intro/tutorial....html, so it may be a
>> day or two before I start asking you questions.  ;-)

Hi Alan,

> For extending emit_EQ, I've got the following embryonic scheme:
>
> 1/- Import the C variable symbols_with_pos_enabled:
>
>  gcc_jit_lvalue *syms_with_pos_enabled =
>                gcc_jit_context_new_global
>                  (comp.ctxt, NULL,
>                   GCC_JIT_GLOBAL_IMPORTED,
>                   gcc_jit_get_type (comp.ctxt, GCC_JIT_TYPE_BOOL),
>                   "symbols_with_pos_enabled");

Yep something like.  PS We already have 'comp.bool_type' to use.

> 2/- Extend emit_EQ with lots of nested gcc_jit_context_new_comparison's
> containing GCC_JIT_BINARY_OP_LOGICAL_OR/ANDs, something like this:
>
>   emit_EQ (gcc_jit_rvalue *x, gcc_jit_rvalue *y)
>   {
>     emit_comment ("EQ");
>
>     return gcc_jit_context_new_comparison (
>              comp.ctxt,
>              NULL,
>              GIT_JIT_BINARY_OP_LOGICAL_OR (
>                gcc_jit_context_new_comparison
>                  (comp.ctxt,
>                   NULL,
>                   GCC_JIT_COMPARISON_EQ,
>                   emit_XLI (x),
>                   emit_XLI (y)),
>                syms_with_pos_enabled
>                  ....
>                  ....
>                ) ;
>   }
>
> 2a/- I think I would need C macros called something like ELN_AND and
> ELN_OR to make this half-way readable.
>
> 3/- Have several fragments of gcc_jit code which could be plugged into
> the above, things representing BARE_SYMBOL_P (x), SYMBOL_WITH_POS_P (x),
> and so on.
>
> Can you let me know what you think of this approach, please?  That is,
> before I put a lot of work into it.  Is it broadly a good way of doing
> the job?

I think it could be a good idea but I believe there's no need to use
macros here, we could have just functions return rvalues no?

I'm not a big fan of C macros and I try not to use them whem possible.

>> I can see already I'll need an emit_BASE_EQ (a renaming of the current
>> emit_EQ), and one or two others.
>
>> Hopefully, we'll get this up and running in a few days, or a week or two
>> at most.
>
> I can't help noticing the lack of anything like SYMBOLP in comp.c.  Is
> this just because nothing needs it?

Correct.

> Or is there some other method in
> operation, like using the inline function in lisp.h somehow, for
> example?

Nope.

Tip, while developing I suggest you to dump the pseudo C code generated
using `native-comp-debug' > 1 to verify it looks the way it is expected.

Best Regards

  Andrea

Re: Correct byte compiler error/warning positions. The solution!

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> Date: Sat, 04 Dec 2021 19:22:02 +0000
> 
> I think it could be a good idea but I believe there's no need to use
> macros here, we could have just functions return rvalues no?
> 
> I'm not a big fan of C macros and I try not to use them whem possible.

Macros punish unoptimized builds less severely than functions.

Re: Correct byte compiler error/warning positions. The solution!

[Andrea Corallo]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
>> Date: Sat, 04 Dec 2021 19:22:02 +0000
>> 
>> I think it could be a good idea but I believe there's no need to use
>> macros here, we could have just functions return rvalues no?
>> 
>> I'm not a big fan of C macros and I try not to use them whem possible.
>
> Macros punish unoptimized builds less severely than functions.

Yep, but in this case I'm sure the perf delta is not measurable
therefore IMO functions should be preferred.

  Andrea

Re: Correct byte compiler error/warning positions. The solution!

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: acm@muc.de, emacs-devel@gnu.org
> Date: Sat, 04 Dec 2021 19:55:28 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Andrea Corallo <akrl@sdf.org>
> >> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> >> Date: Sat, 04 Dec 2021 19:22:02 +0000
> >> 
> >> I think it could be a good idea but I believe there's no need to use
> >> macros here, we could have just functions return rvalues no?
> >> 
> >> I'm not a big fan of C macros and I try not to use them whem possible.
> >
> > Macros punish unoptimized builds less severely than functions.
> 
> Yep, but in this case I'm sure the perf delta is not measurable
> therefore IMO functions should be preferred.

I was responding to a much more general dislike of macros that you
expressed above, and wanted to explain why we do use macros in Emacs.

Re: Correct byte compiler error/warning positions. The solution!

[Andrea Corallo]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: acm@muc.de, emacs-devel@gnu.org
>> Date: Sat, 04 Dec 2021 19:55:28 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Andrea Corallo <akrl@sdf.org>
>> >> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
>> >> Date: Sat, 04 Dec 2021 19:22:02 +0000
>> >> 
>> >> I think it could be a good idea but I believe there's no need to use
>> >> macros here, we could have just functions return rvalues no?
>> >> 
>> >> I'm not a big fan of C macros and I try not to use them whem possible.
>> >
>> > Macros punish unoptimized builds less severely than functions.
>> 
>> Yep, but in this case I'm sure the perf delta is not measurable
>> therefore IMO functions should be preferred.
>
> I was responding to a much more general dislike of macros that you
> expressed above, and wanted to explain why we do use macros in Emacs.

I see, and understand why we use them, where we use them.

Actually I've got the explanation when I proposed a patch to remove them
some time ago :D :D And that's one reason why my phrase finished with
"when possible".

  Andrea

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Andrea.

On Sat, Dec 04, 2021 at 19:22:02 +0000, Andrea Corallo wrote:
> Alan Mackenzie <acm@muc.de> writes:

[ .... ]

> Hi Alan,

> > For extending emit_EQ, I've got the following embryonic scheme:

> > 1/- Import the C variable symbols_with_pos_enabled:

> >  gcc_jit_lvalue *syms_with_pos_enabled =
> >                gcc_jit_context_new_global
> >                  (comp.ctxt, NULL,
> >                   GCC_JIT_GLOBAL_IMPORTED,
> >                   gcc_jit_get_type (comp.ctxt, GCC_JIT_TYPE_BOOL),
> >                   "symbols_with_pos_enabled");

> Yep something like.  PS We already have 'comp.bool_type' to use.

[ .... ]

> > Can you let me know what you think of this approach, please?  That is,
> > before I put a lot of work into it.  Is it broadly a good way of doing
> > the job?

> I think it could be a good idea but I believe there's no need to use
> macros here, we could have just functions return rvalues no?

Yes, no macros needed.  :-)

> I'm not a big fan of C macros and I try not to use them whem possible.

[ .... ]

I have a problem at the moment, which could be a big problem.  How do I
refer to a Lisp variable from jit generated code?

In particular, I need read-access to symbols-with-pos-enabled, or more
precisely to globals.f_symbols_with_pos_enabled.

The gcc jit product seems to go out of its way to make such access as
difficult as possible.  There is a way to get an integer out of "C space"
into the jit mechanism, but this cannot be cast to a pointer type, and
there is no similar mechanism to get a pointer out of C space.  In fact,
gcc jit seems to restrict the language it supports to the equivalent of
Pascal, and makes pointer arithmetic impossible.

I tried declaring "globals" as a global variable to be imported into jit
space, but the loader doesn't know about "globals".

So, how can I get access to globals.f_symbols_with_pos_enabled?

Thanks in advance!

> Best Regards

>   Andrea

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Andrea Corallo]

Alan Mackenzie <acm@muc.de> writes:

> Hello, Andrea.
>
> On Sat, Dec 04, 2021 at 19:22:02 +0000, Andrea Corallo wrote:
>> Alan Mackenzie <acm@muc.de> writes:
>
> [ .... ]
>
>> Hi Alan,
>
>> > For extending emit_EQ, I've got the following embryonic scheme:
>
>> > 1/- Import the C variable symbols_with_pos_enabled:
>
>> >  gcc_jit_lvalue *syms_with_pos_enabled =
>> >                gcc_jit_context_new_global
>> >                  (comp.ctxt, NULL,
>> >                   GCC_JIT_GLOBAL_IMPORTED,
>> >                   gcc_jit_get_type (comp.ctxt, GCC_JIT_TYPE_BOOL),
>> >                   "symbols_with_pos_enabled");
>
>> Yep something like.  PS We already have 'comp.bool_type' to use.
>
> [ .... ]
>
>> > Can you let me know what you think of this approach, please?  That is,
>> > before I put a lot of work into it.  Is it broadly a good way of doing
>> > the job?
>
>> I think it could be a good idea but I believe there's no need to use
>> macros here, we could have just functions return rvalues no?
>
> Yes, no macros needed.  :-)
>
>> I'm not a big fan of C macros and I try not to use them whem possible.
>
> [ .... ]
>
> I have a problem at the moment, which could be a big problem.  How do I
> refer to a Lisp variable from jit generated code?
>
> In particular, I need read-access to symbols-with-pos-enabled, or more
> precisely to globals.f_symbols_with_pos_enabled.
>
> The gcc jit product seems to go out of its way to make such access as
> difficult as possible.  There is a way to get an integer out of "C space"
> into the jit mechanism, but this cannot be cast to a pointer type, and
> there is no similar mechanism to get a pointer out of C space.  In fact,
> gcc jit seems to restrict the language it supports to the equivalent of
> Pascal, and makes pointer arithmetic impossible.
>
> I tried declaring "globals" as a global variable to be imported into jit
> space, but the loader doesn't know about "globals".
>
> So, how can I get access to globals.f_symbols_with_pos_enabled?
>
> Thanks in advance!


Hi Alan,

I think the way should be done is that one declare in the .eln a global
variable as a (bool *), say 'f_symbols_with_pos_enabled_ref'.  Then
during eln load time we set into that the correct address of
'globals.f_symbols_with_pos_enabled' so it can be used as
'*f_symbols_with_pos_enabled_ref' by the generated code.

We do something very similar for the Emacs global var 'current_thread'.
In the eln we have a global variable named "current_thread_reloc" where
we store the address of 'current_thread'.  You can see we set this value
during eln load in 'load_comp_unit'.

Just grep CURRENT_THREAD_RELOC_SYM and you should find the relevant
pieces of code you are interested in.

Best Regards

  Andrea

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Andrea.

On Wed, Dec 15, 2021 at 09:33:45 +0000, Andrea Corallo wrote:
> Alan Mackenzie <acm@muc.de> writes:

[ .... ]

> > I have a problem at the moment, which could be a big problem.  How do I
> > refer to a Lisp variable from jit generated code?

> > In particular, I need read-access to symbols-with-pos-enabled, or more
> > precisely to globals.f_symbols_with_pos_enabled.

[ .... ]

> > I tried declaring "globals" as a global variable to be imported into jit
> > space, but the loader doesn't know about "globals".

> > So, how can I get access to globals.f_symbols_with_pos_enabled?

> > Thanks in advance!


> Hi Alan,

> I think the way should be done is that one declare in the .eln a global
> variable as a (bool *), say 'f_symbols_with_pos_enabled_ref'.  Then
> during eln load time we set into that the correct address of
> 'globals.f_symbols_with_pos_enabled' so it can be used as
> '*f_symbols_with_pos_enabled_ref' by the generated code.

Thanks, I have done this, and have thus progressed further.  It is a
shame we need the extra indirection, but it shouldn't cost too much in
run time.

By the way, congratulations on using three stars in the declaration
struct thread_state ***current_thread_reloc;.  That's not something one
sees very often.  :-)

> We do something very similar for the Emacs global var 'current_thread'.
> In the eln we have a global variable named "current_thread_reloc" where
> we store the address of 'current_thread'.  You can see we set this value
> during eln load in 'load_comp_unit'.

> Just grep CURRENT_THREAD_RELOC_SYM and you should find the relevant
> pieces of code you are interested in.

Done.

I now have another problem, and that's when bootstrap-emacs is trying to
load comp.{el,elc,eln} (I'm not sure which), and is in the (defconst
comp-known-func-cstr-h ...).  The (Lisp) backtrace I see looks like this:

Debugger entered--Lisp error: (error "Attempt to modify read-only object" --cl-block-comp-cstr-union-1-no-mem--)
  comp-cstr-union-1-no-mem(t #s(comp-cstr :typeset (number) :valset nil :range nil :neg nil) #s(comp-cstr :typeset (marker) :valset nil :range nil :neg nil))
  comp-cstr-union-1(t #s(comp-cstr :typeset (t) :valset nil :range nil :neg nil) #s(comp-cstr :typeset (number) :valset nil :range nil :neg nil) #s(comp-cstr :typeset (marker) :valset nil :range nil :neg nil))
  comp-cstr-union(#s(comp-cstr :typeset (t) :valset nil :range nil :neg nil) #s(comp-cstr :typeset (number) :valset nil :range nil :neg nil) #s(comp-cstr :typeset (marker) :valset nil :range nil :neg nil))
  comp-cstr-union-make(#s(comp-cstr :typeset (number) :valset nil :range nil :neg nil) #s(comp-cstr :typeset (marker) :valset nil :range nil :neg nil))
  comp-type-spec-to-cstr((or number marker) t)
  #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_51>((or number marker))
  comp-type-spec-to-cstr((function ((or number marker) (or number marker)) number))
  byte-code("\302 \30\303\304\305\"\11\306\211\211\211\211\5:\2038\0\5@\262\3\2\211A\262\4\242\262\5\2@\262\4\307\4!\262\2\310\5\3\6\11#\210\5A\262\6..." [comp-ctxt comp-known-type-specifiers make-comp-cstr-ctxt make-hash-table :test eq ni\l comp-type-spec-to-cstr puthash] 11)
  (defconst comp-known-func-cstr-h (byte-code "\302 \30\303\304\305\"\11\306\211\211\211\211\5:\2038\0\5@\262\3\2\211A\262\4\242\262\5\2@\262\4\307\4!\262\2\310\5\3\6\11#\210\5A\262\6..." [comp-ctxt comp-known-type-specifiers make-comp-cst\r-ctxt make-hash-table :test eq nil comp-type-spec-to-cstr puthash] 11) "Hash table function -> `comp-constraint'.")
  load("comp" nil t)

The error seems to mean there was an attempt to write into pure memory,
and that the thing being written was the cl-block tag generated by the
(cl-defun comp-cstr-union-1-no-mem ...).

I thought I'd traced it to the `setcdr' form in cl--block-throw (in
cl-macs.el), but when I tried commenting out the CHECK_IMPURE test from
Fsetcdr (in data.c), I still got the same error and backtrace.

Would you help me with this error, please.  Thanks!

> Best Regards

>   Andrea

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Correct byte compiler error/warning positions. The solution!

[Andrea Corallo]

Alan Mackenzie <acm@muc.de> writes:

[...]

> Thanks, I have done this, and have thus progressed further.  It is a
> shame we need the extra indirection, but it shouldn't cost too much in
> run time.
>
> By the way, congratulations on using three stars in the declaration
> struct thread_state ***current_thread_reloc;.  That's not something one
> sees very often.  :-)

Well... I'm a *** programmer :D :x

>> We do something very similar for the Emacs global var 'current_thread'.
>> In the eln we have a global variable named "current_thread_reloc" where
>> we store the address of 'current_thread'.  You can see we set this value
>> during eln load in 'load_comp_unit'.
>
>> Just grep CURRENT_THREAD_RELOC_SYM and you should find the relevant
>> pieces of code you are interested in.
>
> Done.
>
> I now have another problem, and that's when bootstrap-emacs is trying to
> load comp.{el,elc,eln} (I'm not sure which), and is in the (defconst
> comp-known-func-cstr-h ...).  The (Lisp) backtrace I see looks like this:
>
> Debugger entered--Lisp error: (error "Attempt to modify read-only object" --cl-block-comp-cstr-union-1-no-mem--)
>   comp-cstr-union-1-no-mem(t #s(comp-cstr :typeset (number) :valset nil :range nil :neg nil) #s(comp-cstr :typeset (marker) :valset nil :range nil :neg nil))
>   comp-cstr-union-1(t #s(comp-cstr :typeset (t) :valset nil :range nil
> :neg nil) #s(comp-cstr :typeset (number) :valset nil :range nil :neg
> nil) #s(comp-cstr :typeset (marker) :valset nil :range nil :neg nil))
>   comp-cstr-union(#s(comp-cstr :typeset (t) :valset nil :range nil
> :neg nil) #s(comp-cstr :typeset (number) :valset nil :range nil :neg
> nil) #s(comp-cstr :typeset (marker) :valset nil :range nil :neg nil))
>   comp-cstr-union-make(#s(comp-cstr :typeset (number) :valset nil :range nil :neg nil) #s(comp-cstr :typeset (marker) :valset nil :range nil :neg nil))
>   comp-type-spec-to-cstr((or number marker) t)
>   #<subr F616e6f6e796d6f75732d6c616d626461_anonymous_lambda_51>((or number marker))
>   comp-type-spec-to-cstr((function ((or number marker) (or number marker)) number))
>   byte-code("\302
> \30\303\304\305\"\11\306\211\211\211\211\5:\2038\0\5@\262\3\2\211A\262\4\242\262\5\2@\262\4\307\4!\262\2\310\5\3\6\11#\210\5A\262\6..."
> [comp-ctxt comp-known-type-specifiers make-comp-cstr-ctxt
> make-hash-table :test eq ni\l comp-type-spec-to-cstr puthash] 11)
>   (defconst comp-known-func-cstr-h (byte-code "\302
> \30\303\304\305\"\11\306\211\211\211\211\5:\2038\0\5@\262\3\2\211A\262\4\242\262\5\2@\262\4\307\4!\262\2\310\5\3\6\11#\210\5A\262\6..."
> [comp-ctxt comp-known-type-specifiers make-comp-cst\r-ctxt
> make-hash-table :test eq nil comp-type-spec-to-cstr puthash] 11) "Hash
> table function -> `comp-constraint'.")
>   load("comp" nil t)
>
> The error seems to mean there was an attempt to write into pure memory,
> and that the thing being written was the cl-block tag generated by the
> (cl-defun comp-cstr-union-1-no-mem ...).
>
> I thought I'd traced it to the `setcdr' form in cl--block-throw (in
> cl-macs.el), but when I tried commenting out the CHECK_IMPURE test from
> Fsetcdr (in data.c), I still got the same error and backtrace.
>
> Would you help me with this error, please.  Thanks!

I suspect the error we see here is indeed induced by the code newly
introduced but is non sufficiently directly connected with it to
understand what's the issue there.

What I do suggest is that you bootstrap a non native compiled Emacs but
with native compilation support.  Then you use this to native compile a
simple eln that leverage your new generated code.  Note that with
native-comp-debug >= 2 you should be able to step into the eln with the
debugger.

To bootstrap a non native compiled Emacs but with native compilation
support one has to manually hack the /lisp/Makefile.in to have it
produce always and only .elc files instead of the .eln (IIRC two rules
have to be touched.). 

Another alternative approach might be using your temacs to produce and
debug your eln to be tested.

Note: in both cases you will also have to set
`native-comp-deferred-compilation' to nil to avoid jit compilations.

Best Regards

  Andrea

>> Best Regards
>
>>   Andrea

-- 
akrl@sdf.org

Re: Correct byte compiler error/warning positions. The solution!

[Alan Mackenzie]

Hello, Andrea.

On Mon, Dec 20, 2021 at 08:24:00 +0000, Andrea Corallo wrote:
> Alan Mackenzie <acm@muc.de> writes:

[ .... ]

> > I now have another problem, and that's when bootstrap-emacs is trying to
> > load comp.{el,elc,eln} (I'm not sure which), and is in the (defconst
> > comp-known-func-cstr-h ...).  The (Lisp) backtrace I see looks like this:

[ .... ]

> > The error seems to mean there was an attempt to write into pure memory,
> > and that the thing being written was the cl-block tag generated by the
> > (cl-defun comp-cstr-union-1-no-mem ...).

> > I thought I'd traced it to the `setcdr' form in cl--block-throw (in
> > cl-macs.el), but when I tried commenting out the CHECK_IMPURE test from
> > Fsetcdr (in data.c), I still got the same error and backtrace.

> > Would you help me with this error, please.  Thanks!

> I suspect the error we see here is indeed induced by the code newly
> introduced but is non sufficiently directly connected with it to
> understand what's the issue there.

> What I do suggest is that you bootstrap a non native compiled Emacs but
> with native compilation support.  Then you use this to native compile a
> simple eln that leverage your new generated code.  Note that with
> native-comp-debug >= 2 you should be able to step into the eln with the
> debugger.

> To bootstrap a non native compiled Emacs but with native compilation
> support one has to manually hack the /lisp/Makefile.in to have it
> produce always and only .elc files instead of the .eln (IIRC two rules
> have to be touched.). 

> Another alternative approach might be using your temacs to produce and
> debug your eln to be tested.

> Note: in both cases you will also have to set
> `native-comp-deferred-compilation' to nil to avoid jit compilations.

I've managed to nail this problem.  I got bootstrap-emacs into gdb, with
a breakpoint at pure_write_error3 (a variant of pure_write_error).  In
the backtrace, code-cstr-union-1-no-mem thought it was calling
push_handler, but was actually calling pure_write_error.

In my confusion, I had added entries to helper_link_table and also in
declare_runtime_imported_funcs, but the two were not in sync.  There was
no comment saying that these two structures had to be kept matching
eachother.

Now that I've fixed that, I've got deep into a make bootstrap, but
haven't quite managed to complete it.  After that, there are quite a few
things to tidy up.

So, I think I'm going to finish it soon, but probably not before
Christmas.

Cheers!

> Best Regards

>   Andrea

> -- 
> akrl@sdf.org

-- 
Alan Mackenzie (Nuremberg, Germany).