bug#43725: 28.0.50; Include feature/native-comp into master

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi all,

this is to handle code-review and process whose final goal is to have
the feature/native-comp branch merged into master.

I'll try to make this as easy as possible implementing suggestions.
Also please feel free to install directly changes on the branch if you
feel.

Thanks in advance to the reviewers for the time.

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Lars Ingebrigtsen]

Andrea Corallo <akrl@sdf.org> writes:

> this is to handle code-review and process whose final goal is to have
> the feature/native-comp branch merged into master.

Was the patch against the trunk supposed to be included here?  If so,
something went wrong somewhere.  :-)

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

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Andrea Corallo <akrl@sdf.org> writes:
>
>> this is to handle code-review and process whose final goal is to have
>> the feature/native-comp branch merged into master.
>
> Was the patch against the trunk supposed to be included here?

Hi Lars, no it wasn't :)

In the sense that I'm not sure how do we prefer to proceed with the
review and I'm open for inputs on that.

I didn't know if every time I update the branch is worth posting here a
10K+ LOC patch, so I waited.  Also assuming the review will take some
time to go through I'm not really sure how this is practical.

OTOH as the branch is already in emacs.git should be trivial for
reviewers at any point in time to produce the diff and quote the parts
they like to discuss.

Slightly different but related: I wanted to mention that I think would
be really good if possible to retain the current history.  That is going
for a merge instead of applying a single patch.  I believe this would be
of great help for me to maintain the code in the future.

Please let me know how you prefer we proceed.

Thanks!

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Lars Ingebrigtsen]

Andrea Corallo <akrl@sdf.org> writes:

>> Was the patch against the trunk supposed to be included here?
>
> Hi Lars, no it wasn't :)
>
> In the sense that I'm not sure how do we prefer to proceed with the
> review and I'm open for inputs on that.

Right.  :-)

> I didn't know if every time I update the branch is worth posting here a
> 10K+ LOC patch, so I waited.  Also assuming the review will take some
> time to go through I'm not really sure how this is practical.
>
> OTOH as the branch is already in emacs.git should be trivial for
> reviewers at any point in time to produce the diff and quote the parts
> they like to discuss.

Sure, that's fine.  That would be

git diff origin/feature/native-comp..origin/master

?

> Slightly different but related: I wanted to mention that I think would
> be really good if possible to retain the current history.  That is going
> for a merge instead of applying a single patch.  I believe this would be
> of great help for me to maintain the code in the future.

Sure, sounds good.

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

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Andrea Corallo <akrl@sdf.org> writes:
>
>>> Was the patch against the trunk supposed to be included here?
>>
>> Hi Lars, no it wasn't :)
>>
>> In the sense that I'm not sure how do we prefer to proceed with the
>> review and I'm open for inputs on that.
>
> Right.  :-)
>
>> I didn't know if every time I update the branch is worth posting here a
>> 10K+ LOC patch, so I waited.  Also assuming the review will take some
>> time to go through I'm not really sure how this is practical.
>>
>> OTOH as the branch is already in emacs.git should be trivial for
>> reviewers at any point in time to produce the diff and quote the parts
>> they like to discuss.
>
> Sure, that's fine.  That would be
>
> git diff origin/feature/native-comp..origin/master
>
> ?

I think something like this would add into the diff all new commits that
where pushed to master.

One option is to do the same but against the last commit from master
included into the branch (it's easy to identify as just under the last
merge).  ATM would be:

git diff 6c0f1c26d2...origin/feature/native-comp

Other option is to just merge master into native-comp and do the diff
against master.

Not a git wizard so there could be even simpler ways to do that :)

>> Slightly different but related: I wanted to mention that I think would
>> be really good if possible to retain the current history.  That is going
>> for a merge instead of applying a single patch.  I believe this would be
>> of great help for me to maintain the code in the future.
>
> Sure, sounds good.

Great

Thanks

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> Cc: 43725@debbugs.gnu.org
> Date: Thu, 01 Oct 2020 07:51:16 +0000
> From: Andrea Corallo via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > git diff origin/feature/native-comp..origin/master
> >
> > ?
> 
> I think something like this would add into the diff all new commits that
> where pushed to master.
> 
> One option is to do the same but against the last commit from master
> included into the branch (it's easy to identify as just under the last
> merge).  ATM would be:
> 
> git diff 6c0f1c26d2...origin/feature/native-comp

I suggest this:

   git diff ...origin/feature/native-comp

That's how I always produce changes introduced by a branch that
diverged from the current branch.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 43725@debbugs.gnu.org
>> Date: Thu, 01 Oct 2020 07:51:16 +0000
>> From: Andrea Corallo via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> > git diff origin/feature/native-comp..origin/master
>> >
>> > ?
>> 
>> I think something like this would add into the diff all new commits that
>> where pushed to master.
>> 
>> One option is to do the same but against the last commit from master
>> included into the branch (it's easy to identify as just under the last
>> merge).  ATM would be:
>> 
>> git diff 6c0f1c26d2...origin/feature/native-comp
>
> I suggest this:
>
>    git diff ...origin/feature/native-comp
>
> That's how I always produce changes introduced by a branch that
> diverged from the current branch.

Wow that's very handy thanks.

Expanding what you have posted this should work regardless what is the
current checkouted branch:

git diff $(git merge-base origin/master origin/feature/native-comp)..origin/feature/native-comp

bug#43725: 28.0.50; Include feature/native-comp into master

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I suggest this:
>
>    git diff ...origin/feature/native-comp
>
> That's how I always produce changes introduced by a branch that
> diverged from the current branch.

Thanks; very handy.

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

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> Cc: 43725@debbugs.gnu.org
> Date: Wed, 30 Sep 2020 22:37:28 +0000
> From: Andrea Corallo via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> OTOH as the branch is already in emacs.git should be trivial for
> reviewers at any point in time to produce the diff and quote the parts
> they like to discuss.

That's what I intended to do, FWIW.

> Slightly different but related: I wanted to mention that I think would
> be really good if possible to retain the current history.  That is going
> for a merge instead of applying a single patch.  I believe this would be
> of great help for me to maintain the code in the future.

Fine by me.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi all,

FYI today I did some work to put the testsuite in shape also for the
native build (vanilla pass clean already).

The main challenge is related to the fact that the testsuite does large
use of primitive redefinition (tipically through `cl-letf').

As redefining primitives does not take effect in optimized code I
defined a macro (`advice-flet') with similar use but to advice instead
and put it in place in a numer of tests.  You'll see this work in
d07d7ab1a0 825e85b393.

I hope this approach is accettable (thought was good to ask for a
feedback), otherwise we can revert and find another solution.

ATM the testsuite for the native build runs still not clean, I'll finish
with cleaning it up.

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> Date: Fri, 02 Oct 2020 19:39:29 +0000
> From: Andrea Corallo via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> redefining primitives does not take effect in optimized code

Why doesn't it?  Can it be made to take effect, like it does in
interpreted code?

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Fri, 02 Oct 2020 19:39:29 +0000
>> From: Andrea Corallo via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> redefining primitives does not take effect in optimized code
>
> Why doesn't it?  Can it be made to take effect, like it does in
> interpreted code?

Mmmh, I think technically we could, similarly to what we do for the
advice, synthesize compile and install a trampoline.  This would read
the symbol-function and calls what's in inside.

This trampoline installation would be triggered inside Ffset.

So yeah I think we could, if that's the preferred way I can try this
way.

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: 43725@debbugs.gnu.org
> Date: Fri, 02 Oct 2020 19:58:32 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Date: Fri, 02 Oct 2020 19:39:29 +0000
> >> From: Andrea Corallo via "Bug reports for GNU Emacs,
> >>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >> 
> >> redefining primitives does not take effect in optimized code
> >
> > Why doesn't it?  Can it be made to take effect, like it does in
> > interpreted code?
> 
> Mmmh, I think technically we could, similarly to what we do for the
> advice, synthesize compile and install a trampoline.  This would read
> the symbol-function and calls what's in inside.
> 
> This trampoline installation would be triggered inside Ffset.
> 
> So yeah I think we could, if that's the preferred way I can try this
> way.

Lars, Stefan: do you agree that this is the preferred way?

bug#43725: 28.0.50; Include feature/native-comp into master

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Mmmh, I think technically we could, similarly to what we do for the
>> advice, synthesize compile and install a trampoline.  This would read
>> the symbol-function and calls what's in inside.
>> 
>> This trampoline installation would be triggered inside Ffset.
>> 
>> So yeah I think we could, if that's the preferred way I can try this
>> way.
>
> Lars, Stefan: do you agree that this is the preferred way?

I'm not really qualified to have an opinion here, but if this allows
redefining primitives, I'm all for it.  Redefining primitives is a
useful tool.

Would these trampolines be installed only if the primitives are
redefined, so there'd be no performance impact on code running normally?

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

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Mmmh, I think technically we could, similarly to what we do for the
>>> advice, synthesize compile and install a trampoline.  This would read
>>> the symbol-function and calls what's in inside.
>>> 
>>> This trampoline installation would be triggered inside Ffset.
>>> 
>>> So yeah I think we could, if that's the preferred way I can try this
>>> way.
>>
>> Lars, Stefan: do you agree that this is the preferred way?
>
> I'm not really qualified to have an opinion here, but if this allows
> redefining primitives, I'm all for it.  Redefining primitives is a
> useful tool.
>
> Would these trampolines be installed only if the primitives are
> redefined, so there'd be no performance impact on code running normally?

That's correct.

I did some experimentation today and also the implementation was easy as
the trampoline to be synthesized is exactly the same to what we
synthesize already for advising, essentially I just had to add the
proper trigger in fset.

At this point I'm also for going this way as it just reduce the
incompatibly surface and I don't see considerable downsides.

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Andrea Corallo via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> Mmmh, I think technically we could, similarly to what we do for the
>>>> advice, synthesize compile and install a trampoline.  This would read
>>>> the symbol-function and calls what's in inside.
>>>> 
>>>> This trampoline installation would be triggered inside Ffset.
>>>> 
>>>> So yeah I think we could, if that's the preferred way I can try this
>>>> way.
>>>
>>> Lars, Stefan: do you agree that this is the preferred way?
>>
>> I'm not really qualified to have an opinion here, but if this allows
>> redefining primitives, I'm all for it.  Redefining primitives is a
>> useful tool.
>>
>> Would these trampolines be installed only if the primitives are
>> redefined, so there'd be no performance impact on code running normally?
>
> That's correct.
>
> I did some experimentation today and also the implementation was easy as
> the trampoline to be synthesized is exactly the same to what we
> synthesize already for advising, essentially I just had to add the
> proper trigger in fset.
>
> At this point I'm also for going this way as it just reduce the
> incompatibly surface and I don't see considerable downsides.
>
>   Andrea

As my understanding is that that's the consensus I've pushed the revert
of those changes in the testsuite and made Ffset effective also for
redefining primitives.

FYI with the last tweaks `make check` is clean.

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Lars Ingebrigtsen]

I haven't gotten very far in reviewing the patch set (because it's very
large), and I don't have any comments so far (because it all looks very
good :-)).  Just two minor comments on building and running:

If libgccjit isn't installed, configure says this:

checking for dlfunc... no
configure: error: Installed libgccjit has failed passing the smoke test.
You can verify it yourself compiling:
<https://gcc.gnu.org/onlinedocs/jit/intro/tutorial01.html>.
Please report the issue to your distribution.
Here instructions on how to compile and install libgccjit from source:
<https://gcc.gnu.org/wiki/JIT>.

Instead of saying that it isn't installed.

Also -- starting Emacs says "Compilation started.", and it seems like
it'll say that now and then when using Emacs, too.

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

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I haven't gotten very far in reviewing the patch set (because it's very
> large), and I don't have any comments so far (because it all looks very
> good :-)).  Just two minor comments on building and running:
>
> If libgccjit isn't installed, configure says this:
>
> checking for dlfunc... no
> configure: error: Installed libgccjit has failed passing the smoke test.
> You can verify it yourself compiling:
> <https://gcc.gnu.org/onlinedocs/jit/intro/tutorial01.html>.
> Please report the issue to your distribution.
> Here instructions on how to compile and install libgccjit from source:
> <https://gcc.gnu.org/wiki/JIT>.
>
> Instead of saying that it isn't installed.
>
> Also -- starting Emacs says "Compilation started.", and it seems like
> it'll say that now and then when using Emacs, too.

Hi Lars,

thanks for commenting, I pushed bd27257965 and 58d85f4dbb to address
your two suggestions.

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Lars Ingebrigtsen]

Andrea Corallo <akrl@sdf.org> writes:

> thanks for commenting, I pushed bd27257965 and 58d85f4dbb to address
> your two suggestions.

Thanks; works fine now.

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

bug#43725: 28.0.50; Include feature/native-comp into master

[Stefan Monnier]

> Lars, Stefan: do you agree that this is the preferred way?

Yes,


        Stefan

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi all,

I was wondering: is there anything I can do to help the progress of
this?

Regards

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Lars Ingebrigtsen]

Andrea Corallo <akrl@sdf.org> writes:

> I was wondering: is there anything I can do to help the progress of
> this?

There's a lot of code, and very little of it is in areas where I have
much knowledge, so I'm not much help here, I'm afraid.  (I think I read
about a fifth of the diff and didn't have any comments.)

I wonder whether Stefan of Eli has found time to take a look at it?

There's a few compilation warnings in the native-comp tree that hasn't
been fixed yet (mostly due to ;;;###autoload stuff)?  That should be
fixed before merging, I think.

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

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Andrea Corallo <akrl@sdf.org> writes:
>
>> I was wondering: is there anything I can do to help the progress of
>> this?
>
> There's a lot of code, and very little of it is in areas where I have
> much knowledge, so I'm not much help here, I'm afraid.  (I think I read
> about a fifth of the diff and didn't have any comments.)
>
> I wonder whether Stefan of Eli has found time to take a look at it?
>
> There's a few compilation warnings in the native-comp tree that hasn't
> been fixed yet (mostly due to ;;;###autoload stuff)?  That should be
> fixed before merging, I think.

Hi Lars,

thanks for the reply.

Are these warnings related to compiler specific files or related to
other compilation units?

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Lars Ingebrigtsen]

Andrea Corallo <akrl@sdf.org> writes:

> Are these warnings related to compiler specific files or related to
> other compilation units?

I'm not quite sure I understand the question, but it's things like:

 ELC+ELN   gnus/message.elc

In end of data:
message.el:8873:1: Warning: the function ‘safe-date-to-time’ might not be
    defined at runtime.

And:

;;;###autoload
(defun safe-date-to-time (date)
  ...)

I don't see any patterns to which ;;;###autoloads are "missing" in
native-comp.

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

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Andrea Corallo <akrl@sdf.org> writes:
>
>> Are these warnings related to compiler specific files or related to
>> other compilation units?
>
> I'm not quite sure I understand the question, but it's things like:
>
>  ELC+ELN   gnus/message.elc
>
> In end of data:
> message.el:8873:1: Warning: the function ‘safe-date-to-time’ might not be
>     defined at runtime.
>
> And:
>
> ;;;###autoload
> (defun safe-date-to-time (date)
>   ...)
>
> I don't see any patterns to which ;;;###autoloads are "missing" in
> native-comp.

Interesting, I've completely missed this :/

Gonna look into it

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Andrea Corallo via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> Andrea Corallo <akrl@sdf.org> writes:
>>
>>> Are these warnings related to compiler specific files or related to
>>> other compilation units?
>>
>> I'm not quite sure I understand the question, but it's things like:
>>
>>  ELC+ELN   gnus/message.elc
>>
>> In end of data:
>> message.el:8873:1: Warning: the function ‘safe-date-to-time’ might not be
>>     defined at runtime.
>>
>> And:
>>
>> ;;;###autoload
>> (defun safe-date-to-time (date)
>>   ...)
>>
>> I don't see any patterns to which ;;;###autoloads are "missing" in
>> native-comp.
>
> Interesting, I've completely missed this :/
>
> Gonna look into it
>
>   Andrea

Hi Lars,

quite happy to mention that dcfd367d28 should fix the issue of the
spurious warnings.  A little less proud of the fact that took me good
part of the day to track this stupid bug down :/

Anyway I've build the branch with and without --with-nativecomp and
(after some clean-up on the vanilla build) I count the same number of
warnings now on both builds.

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Lars Ingebrigtsen]

Andrea Corallo <akrl@sdf.org> writes:

> quite happy to mention that dcfd367d28 should fix the issue of the
> spurious warnings.  A little less proud of the fact that took me good
> part of the day to track this stupid bug down :/

:-)

Thanks; I did a new AOT build, and I can confirm that all these warnings
are gone.

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

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 43725@debbugs.gnu.org,  Eli Zaretskii <eliz@gnu.org>,
>   monnier@iro.umontreal.ca
> Date: Thu, 26 Nov 2020 11:10:11 +0100
> 
> > I was wondering: is there anything I can do to help the progress of
> > this?
> 
> There's a lot of code, and very little of it is in areas where I have
> much knowledge, so I'm not much help here, I'm afraid.  (I think I read
> about a fifth of the diff and didn't have any comments.)
> 
> I wonder whether Stefan of Eli has found time to take a look at it?

It's on my todo, so I will get to it eventually.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Cc: 43725@debbugs.gnu.org,  Eli Zaretskii <eliz@gnu.org>,
>>   monnier@iro.umontreal.ca
>> Date: Thu, 26 Nov 2020 11:10:11 +0100
>> 
>> > I was wondering: is there anything I can do to help the progress of
>> > this?
>> 
>> There's a lot of code, and very little of it is in areas where I have
>> much knowledge, so I'm not much help here, I'm afraid.  (I think I read
>> about a fifth of the diff and didn't have any comments.)
>> 
>> I wonder whether Stefan of Eli has found time to take a look at it?
>
> It's on my todo, so I will get to it eventually.

Please while assessing the priorities of your todo, account for the fact
that with the current user-base of the branch the flow of "important"
bugs is ATM zeroed.  If we want to have this in 28 would be wise to have
the larger coverage earlier than later to increase the verification
surface.

Thanks

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  43725@debbugs.gnu.org,
>   monnier@iro.umontreal.ca
> Date: Thu, 26 Nov 2020 21:15:09 +0000
> 
> >> I wonder whether Stefan of Eli has found time to take a look at it?
> >
> > It's on my todo, so I will get to it eventually.
> 
> Please while assessing the priorities of your todo, account for the fact
> that with the current user-base of the branch the flow of "important"
> bugs is ATM zeroed.  If we want to have this in 28 would be wise to have
> the larger coverage earlier than later to increase the verification
> surface.

Will do, thanks.

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

Andrea, can you please explain why there are so many changes in Lisp
files on the branch?

For example, what are changes like this one about:

  +(declare-function subr-native-lambda-list "data.c")

Or this:

  +   ((and (featurep 'nativecomp)
  +         (subrp def)
  +         (listp (subr-native-lambda-list def)))
  +    (subr-native-lambda-list def))

Or this:

  +  ;; Never native compile to allow cc-defs.el:2345 hack.
  +  (declare (speed -1))

This will not work on MS-Windows, you need to use path-separator to do
it portably:

  +    (when (featurep 'nativecomp)
  +      (defvar comp-eln-load-path)
  +      (let ((path-env (getenv "EMACSNATIVELOADPATH")))
  +        (when path-env
  +          (dolist (path (split-string path-env ":")) <<<<<<<<<<<<<
  +            (unless (string= "" path)
  +              (push path comp-eln-load-path)))))
  +      (push (concat user-emacs-directory "eln-cache/") comp-eln-load-path))

I'm not sure I understand this addition to epaths.nt:

  +/* Like PATH_LOADSEARCH, but contains the relative path from the
  +   installation directory.
  +*/
  +#define PATH_REL_LOADSEARCH ""

Can you explain how PATH_REL_LOADSEARCH is supposed to work, and for
what purpose it was introduced?

A few minor comments about the C code:

This is unsafe, because a fixnum can be larger than PTRDIFF_MAX:

  +static gcc_jit_lvalue *
  +emit_mvar_lval (Lisp_Object mvar)
  +{
  +  Lisp_Object mvar_slot = CALL1I (comp-mvar-slot, mvar);
  +
  +  if (EQ (mvar_slot, Qscratch))
  +    {
  +      if (!comp.scratch)
  +       comp.scratch = gcc_jit_function_new_local (comp.func,
  +                                                  NULL,
  +                                                  comp.lisp_obj_type,
  +                                                  "scratch");
  +      return comp.scratch;
  +    }
  +
  +  return comp.frame[XFIXNUM (mvar_slot)];  <<<<<<<<<<<<<<<<<<<<
  +}

Likewise, this is unsafe because a fixnum can be larger than INT_MAX:

  +  if (!FIXNUMP (idx))
  +    xsignal1 (Qnative_ice,
  +             build_string ("inconsistent data relocation container"));
  +  reloc.idx = gcc_jit_context_new_rvalue_from_int (comp.ctxt,
  +                                                  comp.ptrdiff_type,
  +                                                  XFIXNUM (idx)); <<<<<<<<

(There are several more calls with the same problem.)

Several comparisons like this one:

  +  if (val != (long) val)

are IMO better written as

  if (val > LONG_MAX || val < LONG_MIN)

Here, wouldn't it be better to have an assertion that there are no
more than 6 elements in the list:

  +  Lisp_Object arg[6];
  +
  +  Lisp_Object p = XCDR (insn);
  +  ptrdiff_t i = 0;
  +  FOR_EACH_TAIL (p)
  +    {
  +      if (i == sizeof (arg) / sizeof (Lisp_Object))
  +       break;
  +      arg[i++] = XCAR (p);
  +    }

If there are more than 6, we will be writing beyond the end of the
arg[] array.

This is nonportable:

  +  if (!noninteractive)
  +    {
  +      sigset_t blocked;
  +      /* Gcc doesn't like being interrupted at all.  */
  +      block_input ();
  +      sigemptyset (&blocked);
  +      sigaddset (&blocked, SIGALRM);
  +      sigaddset (&blocked, SIGINT);
  +#ifdef USABLE_SIGIO
  +      sigaddset (&blocked, SIGIO);
  +#endif
  +      pthread_sigmask (SIG_BLOCK, &blocked, &saved_sigset); <<<<<<<<<<<
  +      count = SPECPDL_INDEX ();
  +      record_unwind_protect_void (restore_sigmask);
  +    }

We shouldn't use pthread_sigmask unconditionally, we should use it
only on Posix platforms.  Can you explain why the signals here should
be blocked?  What happens if they aren't, and a signal arrives while
the compilation runs?  I'm asking because on MS-Windows blocking
signals with sigaddset/sigmask doesn't really work, so the question is
what if anything should be done here on Windows.

Here, 'i' could be ptrdiff_t, no need to use EMACS_INT:

  +  EMACS_INT d_vec_len = XFIXNUM (Flength (comp_u->data_vec));
  +  for (EMACS_INT i = 0; i < d_vec_len; i++)
  +    if (!EQ (data_relocs[i],  AREF (comp_u->data_vec, i)))
  +      return false;
  +
  +  d_vec_len = XFIXNUM (Flength (comp_u->data_impure_vec));
  +  for (EMACS_INT i = 0; i < d_vec_len; i++)

This should have a big FIXME, since it means the name of the DLL is in
two different places, and if we need to support a differently-named
DLL, we are in trouble:

  +#ifdef WINDOWSNT
  +  /* We may need to load libgccjit when dumping before term/w32-win.el
  +     defines `dynamic-library-alist`. This will fail if that variable
  +     is empty, so add libgccjit-0.dll to it.  */
  +  if (will_dump_p ())
  +    Vdynamic_library_alist = list1 (list2 (Qgccjit,
  +                                           build_string ("libgccjit-0.dll")));

Is there a more elegant way of resolving this difficulty?

This lacks the ENCODE_FILE part:

  +           char *fname = SSDATA (concat2 (Vinvocation_directory,
  +                                          XCAR (comp_u->file)));
  +           FILE *file;
  +           if ((file = fopen (fname, "r")))

And why are you using fopen instead of emacs_fopen?

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> Andrea, can you please explain why there are so many changes in Lisp
> files on the branch?
>
> For example, what are changes like this one about:
>
>   +(declare-function subr-native-lambda-list "data.c")

`subr-native-lambda-list' returns the lambda list for a dynamic native
compiled function or t if lexically scoped.

This is available only building "--with-nativecomp", this is to remove a
bytecompiler warning for vanilla build.  

> Or this:
>
>   +   ((and (featurep 'nativecomp)
>   +         (subrp def)
>   +         (listp (subr-native-lambda-list def)))
>   +    (subr-native-lambda-list def))

This is to retrive the lambda list of dynamic scoped native compiled
lisp function so that `help-function-arglist' can return it.

> Or this:
>
>   +  ;; Never native compile to allow cc-defs.el:2345 hack.
>   +  (declare (speed -1))

cc-defs does a dirty hack where is capturing the bytecode object of
`cc-bytecomp-compiling-or-loading'.

From cc-defs.el:
========
;; Ugly hack to pull in the definition of `cc-bytecomp-compiling-or-loading'
;; from cc-bytecomp to make it available at loadtime.  This is the same
;; mechanism used in cc-mode.el for `c-populate-syntax-table'.
(defalias 'cc-bytecomp-compiling-or-loading
  (cc-eval-when-compile
    (let ((f (symbol-function 'cc-bytecomp-compiling-or-loading)))
      (if (byte-code-function-p f) f (byte-compile f)))))
========

Native code cannot go through the read print loop so this hack can't
work native compiling the function, speed -1 ensure the function is
present in the eln file but in form of bytecode.

> This will not work on MS-Windows, you need to use path-separator to do
> it portably:
>
>   +    (when (featurep 'nativecomp)
>   +      (defvar comp-eln-load-path)
>   +      (let ((path-env (getenv "EMACSNATIVELOADPATH")))
>   +        (when path-env
>   +          (dolist (path (split-string path-env ":")) <<<<<<<<<<<<<
>   +            (unless (string= "" path)
>   +              (push path comp-eln-load-path)))))
>   +      (push (concat user-emacs-directory "eln-cache/") comp-eln-load-path))

Fixed by 31416495ad.

I'll keep on working through your points the coming week, sincere thanks
for reviewing.

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Addressing some easy part of the review to close the day.

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> This is unsafe, because a fixnum can be larger than PTRDIFF_MAX:
>
>   +static gcc_jit_lvalue *
>   +emit_mvar_lval (Lisp_Object mvar)
>   +{
>   +  Lisp_Object mvar_slot = CALL1I (comp-mvar-slot, mvar);
>   +
>   +  if (EQ (mvar_slot, Qscratch))
>   +    {
>   +      if (!comp.scratch)
>   +       comp.scratch = gcc_jit_function_new_local (comp.func,
>   +                                                  NULL,
>   +                                                  comp.lisp_obj_type,
>   +                                                  "scratch");
>   +      return comp.scratch;
>   +    }
>   +
>   +  return comp.frame[XFIXNUM (mvar_slot)];  <<<<<<<<<<<<<<<<<<<<
>   +}

Fixed by 543e6e664c

> Likewise, this is unsafe because a fixnum can be larger than INT_MAX:
>
>   +  if (!FIXNUMP (idx))
>   +    xsignal1 (Qnative_ice,
>   +             build_string ("inconsistent data relocation container"));
>   +  reloc.idx = gcc_jit_context_new_rvalue_from_int (comp.ctxt,
>   +                                                  comp.ptrdiff_type,
>   +                                                  XFIXNUM (idx)); <<<<<<<<
>
> (There are several more calls with the same problem.)

Should we never trust in C a value coming from a Lisp_Object even if is
supposed to be constructed on purpose?

> Several comparisons like this one:
>
>   +  if (val != (long) val)
>
> are IMO better written as
>
>   if (val > LONG_MAX || val < LONG_MIN)

Fixed by 72e4a22391

> Here, wouldn't it be better to have an assertion that there are no
> more than 6 elements in the list:
>
>   +  Lisp_Object arg[6];
>   +
>   +  Lisp_Object p = XCDR (insn);
>   +  ptrdiff_t i = 0;
>   +  FOR_EACH_TAIL (p)
>   +    {
>   +      if (i == sizeof (arg) / sizeof (Lisp_Object))
>   +       break;
>   +      arg[i++] = XCAR (p);
>   +    }

This way we can have insns longer than 6 operands but we don't load
them.  These are tipically comment insn we use as a debug note therefore
not relevant here (code generation).

> This is nonportable:
>
>   +  if (!noninteractive)
>   +    {
>   +      sigset_t blocked;
>   +      /* Gcc doesn't like being interrupted at all.  */
>   +      block_input ();
>   +      sigemptyset (&blocked);
>   +      sigaddset (&blocked, SIGALRM);
>   +      sigaddset (&blocked, SIGINT);
>   +#ifdef USABLE_SIGIO
>   +      sigaddset (&blocked, SIGIO);
>   +#endif
>   +      pthread_sigmask (SIG_BLOCK, &blocked, &saved_sigset); <<<<<<<<<<<
>   +      count = SPECPDL_INDEX ();
>   +      record_unwind_protect_void (restore_sigmask);
>   +    }
>
> We shouldn't use pthread_sigmask unconditionally, we should use it
> only on Posix platforms.  Can you explain why the signals here should
> be blocked?  What happens if they aren't, and a signal arrives while
> the compilation runs?  I'm asking because on MS-Windows blocking
> signals with sigaddset/sigmask doesn't really work, so the question is
> what if anything should be done here on Windows.

IIRC the compilation was crashing.  Actually we should be able to get
rid of this piece of code.  ATM we always run in a non interactive
(typically child) process compilations so this code is not exercised
anymore.

Removed by 21858596f0

> Here, 'i' could be ptrdiff_t, no need to use EMACS_INT:
>
>   +  EMACS_INT d_vec_len = XFIXNUM (Flength (comp_u->data_vec));
>   +  for (EMACS_INT i = 0; i < d_vec_len; i++)
>   +    if (!EQ (data_relocs[i],  AREF (comp_u->data_vec, i)))
>   +      return false;
>   +
>   +  d_vec_len = XFIXNUM (Flength (comp_u->data_impure_vec));
>   +  for (EMACS_INT i = 0; i < d_vec_len; i++)

Fixed by 7b676861dd

Thanks

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
> Date: Tue, 16 Feb 2021 21:13:07 +0000
> 
> Addressing some easy part of the review to close the day.

Thanks.

> > Likewise, this is unsafe because a fixnum can be larger than INT_MAX:
> >
> >   +  if (!FIXNUMP (idx))
> >   +    xsignal1 (Qnative_ice,
> >   +             build_string ("inconsistent data relocation container"));
> >   +  reloc.idx = gcc_jit_context_new_rvalue_from_int (comp.ctxt,
> >   +                                                  comp.ptrdiff_type,
> >   +                                                  XFIXNUM (idx)); <<<<<<<<
> >
> > (There are several more calls with the same problem.)
> 
> Should we never trust in C a value coming from a Lisp_Object even if is
> supposed to be constructed on purpose?

We can trust that if we indeed make the value explicitly, but if
there's at least a tiny chance to get a value that's too large, at
least an assertion would be a good idea.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
>> Date: Tue, 16 Feb 2021 21:13:07 +0000
>> 
>> Addressing some easy part of the review to close the day.
>
> Thanks.
>
>> > Likewise, this is unsafe because a fixnum can be larger than INT_MAX:
>> >
>> >   +  if (!FIXNUMP (idx))
>> >   +    xsignal1 (Qnative_ice,
>> >   +             build_string ("inconsistent data relocation container"));
>> >   +  reloc.idx = gcc_jit_context_new_rvalue_from_int (comp.ctxt,
>> >   +                                                  comp.ptrdiff_type,
>> >   +                                                  XFIXNUM (idx)); <<<<<<<<
>> >
>> > (There are several more calls with the same problem.)
>> 
>> Should we never trust in C a value coming from a Lisp_Object even if is
>> supposed to be constructed on purpose?
>
> We can trust that if we indeed make the value explicitly, but if
> there's at least a tiny chance to get a value that's too large, at
> least an assertion would be a good idea.

That's sensible.  185121da69 and b1bab6e073 are introducing assertions
for the highlighted cases and the other one I've spotted.

Thanks

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
> Date: Thu, 18 Feb 2021 21:22:54 +0000
> 
> > We can trust that if we indeed make the value explicitly, but if
> > there's at least a tiny chance to get a value that's too large, at
> > least an assertion would be a good idea.
> 
> That's sensible.  185121da69 and b1bab6e073 are introducing assertions
> for the highlighted cases and the other one I've spotted.

Thanks.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Andrea Corallo via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> Addressing some easy part of the review to close the day.
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> [...]
>> Several comparisons like this one:
>>
>>   +  if (val != (long) val)
>>
>> are IMO better written as
>>
>>   if (val > LONG_MAX || val < LONG_MIN)
>
> Fixed by 72e4a22391

I noticed that with the suggested fix applied in configurations where
'val' can't exceed a long on the positive side GCC (trunk from some time
ago) is complaining emitting the following warning:

comp.c:1174:22: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
 1174 |   if (val > LONG_MAX || val < LONG_MIN)
      |                      ^~

Not sure what's the best way to silence it or if we want to revert to
the previous formulation.

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, larsi@gnus.org, monnier@iro.umontreal.ca,
>         43725@debbugs.gnu.org
> Date: Thu, 18 Feb 2021 20:32:06 +0000
> 
> >> Several comparisons like this one:
> >>
> >>   +  if (val != (long) val)
> >>
> >> are IMO better written as
> >>
> >>   if (val > LONG_MAX || val < LONG_MIN)
> >
> > Fixed by 72e4a22391
> 
> I noticed that with the suggested fix applied in configurations where
> 'val' can't exceed a long on the positive side GCC (trunk from some time
> ago) is complaining emitting the following warning:
> 
> comp.c:1174:22: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
>  1174 |   if (val > LONG_MAX || val < LONG_MIN)
>       |                      ^~
> 
> Not sure what's the best way to silence it or if we want to revert to
> the previous formulation.

You can either condition that by WIDE_EMACS_INT, or add a comparison
between LONG_MAX and INT_MAX.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: Eli Zaretskii <eliz@gnu.org>, larsi@gnus.org, monnier@iro.umontreal.ca,
>>         43725@debbugs.gnu.org
>> Date: Thu, 18 Feb 2021 20:32:06 +0000
>> 
>> >> Several comparisons like this one:
>> >>
>> >>   +  if (val != (long) val)
>> >>
>> >> are IMO better written as
>> >>
>> >>   if (val > LONG_MAX || val < LONG_MIN)
>> >
>> > Fixed by 72e4a22391
>> 
>> I noticed that with the suggested fix applied in configurations where
>> 'val' can't exceed a long on the positive side GCC (trunk from some time
>> ago) is complaining emitting the following warning:
>> 
>> comp.c:1174:22: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
>>  1174 |   if (val > LONG_MAX || val < LONG_MIN)
>>       |                      ^~
>> 
>> Not sure what's the best way to silence it or if we want to revert to
>> the previous formulation.

Hi Eli,

sorry just to make sure I understand the suggestions:

> You can either condition that by WIDE_EMACS_INT,

Put it under #ifdef I guess.

> or add a comparison > between LONG_MAX and INT_MAX.

Not sure I understand how you'd write this.

Thanks

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: bug-gnu-emacs@gnu.org, larsi@gnus.org, monnier@iro.umontreal.ca,
>         43725@debbugs.gnu.org
> Date: Thu, 18 Feb 2021 20:53:05 +0000
> 
> >> comp.c:1174:22: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
> >>  1174 |   if (val > LONG_MAX || val < LONG_MIN)
> >>       |                      ^~
> >> 
> >> Not sure what's the best way to silence it or if we want to revert to
> >> the previous formulation.
> 
> Hi Eli,
> 
> sorry just to make sure I understand the suggestions:
> 
> > You can either condition that by WIDE_EMACS_INT,
> 
> Put it under #ifdef I guess.
> 
> > or add a comparison > between LONG_MAX and INT_MAX.
> 
> Not sure I understand how you'd write this.

Maybe it will be easier to do something like this instead:

  #if EMACS_INT_MAX > LONG_MAX
    return emit_rvalue_from_unsigned_long_long (...
  #else
    return gcc_jit_context_new_rvalue_from_long (...
  #endif

Does that work well?

Btw, why does the 'long long' variant say "unsigned", while the 'long'
variant doesn't?

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: bug-gnu-emacs@gnu.org, larsi@gnus.org, monnier@iro.umontreal.ca,
>>         43725@debbugs.gnu.org
>> Date: Thu, 18 Feb 2021 20:53:05 +0000
>> 
>> >> comp.c:1174:22: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
>> >>  1174 |   if (val > LONG_MAX || val < LONG_MIN)
>> >>       |                      ^~
>> >> 
>> >> Not sure what's the best way to silence it or if we want to revert to
>> >> the previous formulation.
>> 
>> Hi Eli,
>> 
>> sorry just to make sure I understand the suggestions:
>> 
>> > You can either condition that by WIDE_EMACS_INT,
>> 
>> Put it under #ifdef I guess.
>> 
>> > or add a comparison > between LONG_MAX and INT_MAX.
>> 
>> Not sure I understand how you'd write this.
>
> Maybe it will be easier to do something like this instead:
>
>   #if EMACS_INT_MAX > LONG_MAX
>     return emit_rvalue_from_unsigned_long_long (...
>   #else
>     return gcc_jit_context_new_rvalue_from_long (...
>   #endif
>
> Does that work well?

Unfortunately this is a compile time (for the native compiler)
decision.  If val fits into a long we want to emit simply that.

At this point I'm starting to think that the original formulation is
probably the lesser of evils.

> Btw, why does the 'long long' variant say "unsigned", while the 'long'
> variant doesn't?


'emit_rvalue_from_unsigned_long_long' was added by me as libgccjit so
far has no long long support.  As this is shifting the numeric argument
to emit the code that re-create the long long equivalent using an
unsigned was the natural choice to avoid UB.

Admittedly we could just cast inside
'emit_rvalue_from_unsigned_long_long' and rename it into
'emit_rvalue_from_long_long' but I'm not sure is that important.

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: bug-gnu-emacs@gnu.org, larsi@gnus.org, monnier@iro.umontreal.ca,
>         43725@debbugs.gnu.org
> Date: Fri, 19 Feb 2021 11:13:14 +0000
> 
> >   #if EMACS_INT_MAX > LONG_MAX
> >     return emit_rvalue_from_unsigned_long_long (...
> >   #else
> >     return gcc_jit_context_new_rvalue_from_long (...
> >   #endif
> >
> > Does that work well?
> 
> Unfortunately this is a compile time (for the native compiler)
> decision.  If val fits into a long we want to emit simply that.

OK, then how about my original proposal, viz.:

  if (EMACS_INT_MAX > LONG_MAX)
    {
      if (val > LONG_MAX || val < LONG_MIN)
        ...
      else
        ...
    }

You could also #ifdef this conditioned on WINDOWSNT, since MS-Windows
is the only platform where this matters.

> At this point I'm starting to think that the original formulation is
> probably the lesser of evils.

Believe me, it isn't.  For starters, it is not clear what it does.

> 'emit_rvalue_from_unsigned_long_long' was added by me as libgccjit so
> far has no long long support.  As this is shifting the numeric argument
> to emit the code that re-create the long long equivalent using an
> unsigned was the natural choice to avoid UB.
> 
> Admittedly we could just cast inside
> 'emit_rvalue_from_unsigned_long_long' and rename it into
> 'emit_rvalue_from_long_long' but I'm not sure is that important.

Either that, or some comment would be good enough.

Thanks.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: bug-gnu-emacs@gnu.org, larsi@gnus.org, monnier@iro.umontreal.ca,
>>         43725@debbugs.gnu.org
>> Date: Fri, 19 Feb 2021 11:13:14 +0000
>>
>> >   #if EMACS_INT_MAX > LONG_MAX
>> >     return emit_rvalue_from_unsigned_long_long (...
>> >   #else
>> >     return gcc_jit_context_new_rvalue_from_long (...
>> >   #endif
>> >
>> > Does that work well?
>>
>> Unfortunately this is a compile time (for the native compiler)
>> decision.  If val fits into a long we want to emit simply that.
>
> OK, then how about my original proposal, viz.:
>
>   if (EMACS_INT_MAX > LONG_MAX)
>     {
>       if (val > LONG_MAX || val < LONG_MIN)
>         ...
>       else
>         ...
>     }
>
> You could also #ifdef this conditioned on WINDOWSNT, since MS-Windows
> is the only platform where this matters.

Okay 14e6268d14 is arranging things as follow:

 #ifdef WIDE_EMACS_INT
   if (val > LONG_MAX || val < LONG_MIN)
     return emit_rvalue_from_unsigned_long_long (comp.emacs_uint_type, val);
 #endif
   return gcc_jit_context_new_rvalue_from_long (comp.ctxt,
                                                comp.emacs_uint_type,
                                                val);


>> At this point I'm starting to think that the original formulation is
>> probably the lesser of evils.
>
> Believe me, it isn't.  For starters, it is not clear what it does.
>
>> 'emit_rvalue_from_unsigned_long_long' was added by me as libgccjit so
>> far has no long long support.  As this is shifting the numeric argument
>> to emit the code that re-create the long long equivalent using an
>> unsigned was the natural choice to avoid UB.
>>
>> Admittedly we could just cast inside
>> 'emit_rvalue_from_unsigned_long_long' and rename it into
>> 'emit_rvalue_from_long_long' but I'm not sure is that important.
>
> Either that, or some comment would be good enough.

I realized we already had also the signed variant so with 92fe7a91f4 I
removed the unsigned one and we always use 'emit_rvalue_from_long_long'
now.

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
> Date: Fri, 19 Feb 2021 22:03:32 +0000
> 
> Okay 14e6268d14 is arranging things as follow:
> 
>  #ifdef WIDE_EMACS_INT
>    if (val > LONG_MAX || val < LONG_MIN)
>      return emit_rvalue_from_unsigned_long_long (comp.emacs_uint_type, val);
>  #endif
>    return gcc_jit_context_new_rvalue_from_long (comp.ctxt,
>                                                 comp.emacs_uint_type,
>                                                 val);

Thanks.

> >> 'emit_rvalue_from_unsigned_long_long' was added by me as libgccjit so
> >> far has no long long support.  As this is shifting the numeric argument
> >> to emit the code that re-create the long long equivalent using an
> >> unsigned was the natural choice to avoid UB.
> >>
> >> Admittedly we could just cast inside
> >> 'emit_rvalue_from_unsigned_long_long' and rename it into
> >> 'emit_rvalue_from_long_long' but I'm not sure is that important.
> >
> > Either that, or some comment would be good enough.
> 
> I realized we already had also the signed variant so with 92fe7a91f4 I
> removed the unsigned one and we always use 'emit_rvalue_from_long_long'
> now.

Great, thanks.

bug#43725: 28.0.50; Include feature/native-comp into master

[Pip Cet]

On Thu, Feb 18, 2021 at 8:40 PM Andrea Corallo via Bug reports for GNU
Emacs, the Swiss army knife of text editors <bug-gnu-emacs@gnu.org>
wrote:
> I noticed that with the suggested fix applied in configurations where
> 'val' can't exceed a long on the positive side GCC (trunk from some time
> ago) is complaining emitting the following warning:
>
> comp.c:1174:22: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
>  1174 |   if (val > LONG_MAX || val < LONG_MIN)
>       |                      ^~
>
> Not sure what's the best way to silence it or if we want to revert to
> the previous formulation.

Maybe it's me, but I had a hard time parsing that warning. IIUC, the
tests are both trivially false? And oring two trivially false
conditions shouldn't trigger a warning, should it?

Sounds like it's GCC that needs to be fixed.

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Fri, 19 Feb 2021 12:53:43 +0000
> Cc: 43725@debbugs.gnu.org, eliz@gnu.org, larsi@gnus.org, 
> 	Stefan Monnier <monnier@iro.umontreal.ca>
> 
> > comp.c:1174:22: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
> >  1174 |   if (val > LONG_MAX || val < LONG_MIN)
> >       |                      ^~
> >
> > Not sure what's the best way to silence it or if we want to revert to
> > the previous formulation.
> 
> Maybe it's me, but I had a hard time parsing that warning. IIUC, the
> tests are both trivially false? And oring two trivially false
> conditions shouldn't trigger a warning, should it?

No, I think it tries to say that val is always either greater than
LONG_MAX or smaller than LONG_MIN.

> Sounds like it's GCC that needs to be fixed.

Could be.  But I've given up on GCC's tendency to warn about perfectly
valid programs.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Pip Cet <pipcet@gmail.com>
>> Date: Fri, 19 Feb 2021 12:53:43 +0000
>> Cc: 43725@debbugs.gnu.org, eliz@gnu.org, larsi@gnus.org, 
>> 	Stefan Monnier <monnier@iro.umontreal.ca>
>> 
>> > comp.c:1174:22: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
>> >  1174 |   if (val > LONG_MAX || val < LONG_MIN)
>> >       |                      ^~
>> >
>> > Not sure what's the best way to silence it or if we want to revert to
>> > the previous formulation.
>> 
>> Maybe it's me, but I had a hard time parsing that warning. IIUC, the
>> tests are both trivially false? And oring two trivially false
>> conditions shouldn't trigger a warning, should it?
>
> No, I think it tries to say that val is always either greater than
> LONG_MAX or smaller than LONG_MIN.

Exactly, the warning is about the fact that all the other conditions
will never be evaluated.  The phrasing is admittedly quite cryptic tho.

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Pip Cet]

On Fri, Feb 19, 2021 at 2:16 PM Andrea Corallo <akrl@sdf.org> wrote:
> >> > comp.c:1174:22: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
> >> >  1174 |   if (val > LONG_MAX || val < LONG_MIN)
> >> >       |                      ^~
> >> >
> >> > Not sure what's the best way to silence it or if we want to revert to
> >> > the previous formulation.
> >>
> >> Maybe it's me, but I had a hard time parsing that warning. IIUC, the
> >> tests are both trivially false? And oring two trivially false
> >> conditions shouldn't trigger a warning, should it?
> >
> > No, I think it tries to say that val is always either greater than
> > LONG_MAX or smaller than LONG_MIN.
>
> Exactly, the warning is about the fact that all the other conditions
> will never be evaluated.  The phrasing is admittedly quite cryptic tho.

I thought you said this was a setup where the argument _does_ fit a long?

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Pip Cet <pipcet@gmail.com> writes:

> On Fri, Feb 19, 2021 at 2:16 PM Andrea Corallo <akrl@sdf.org> wrote:
>> >> > comp.c:1174:22: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
>> >> >  1174 |   if (val > LONG_MAX || val < LONG_MIN)
>> >> >       |                      ^~
>> >> >
>> >> > Not sure what's the best way to silence it or if we want to revert to
>> >> > the previous formulation.
>> >>
>> >> Maybe it's me, but I had a hard time parsing that warning. IIUC, the
>> >> tests are both trivially false? And oring two trivially false
>> >> conditions shouldn't trigger a warning, should it?
>> >
>> > No, I think it tries to say that val is always either greater than
>> > LONG_MAX or smaller than LONG_MIN.
>>
>> Exactly, the warning is about the fact that all the other conditions
>> will never be evaluated.  The phrasing is admittedly quite cryptic tho.
>
> I thought you said this was a setup where the argument _does_ fit a long?

There we are dispatching if 'val' can be expressed or not with a long.

'val' is an EMACS_INT so depending on the configuration it might fit in
a long by definition, when this happen GCC sees the first condition in
or as always true etc etc... :)

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, 43725@debbugs.gnu.org, larsi@gnus.org,
>         Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 19 Feb 2021 14:35:43 +0000
> 
> >> > No, I think it tries to say that val is always either greater than
> >> > LONG_MAX or smaller than LONG_MIN.
> >>
> >> Exactly, the warning is about the fact that all the other conditions
> >> will never be evaluated.  The phrasing is admittedly quite cryptic tho.
> >
> > I thought you said this was a setup where the argument _does_ fit a long?
> 
> There we are dispatching if 'val' can be expressed or not with a long.
> 
> 'val' is an EMACS_INT so depending on the configuration it might fit in
> a long by definition, when this happen GCC sees the first condition in
> or as always true etc etc... :)

The condition would be always true if it were written like this:

  if (val >= LONG_MIN && val <= LONG_MAX)

But that wasn't what the code was saying, it was the exact opposite of
this condition.  So I think GCC was actually complaining about the
'else' clause.

bug#43725: 28.0.50; Include feature/native-comp into master

[Pip Cet]

On Fri, Feb 19, 2021 at 2:35 PM Andrea Corallo <akrl@sdf.org> wrote:
> > On Fri, Feb 19, 2021 at 2:16 PM Andrea Corallo <akrl@sdf.org> wrote:
> >> >> > comp.c:1174:22: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
> >> >> >  1174 |   if (val > LONG_MAX || val < LONG_MIN)
> >> >> >       |                      ^~
> >> >> >
> >> >> > Not sure what's the best way to silence it or if we want to revert to
> >> >> > the previous formulation.
> >> >>
> >> >> Maybe it's me, but I had a hard time parsing that warning. IIUC, the
> >> >> tests are both trivially false? And oring two trivially false
> >> >> conditions shouldn't trigger a warning, should it?
> >> >
> >> > No, I think it tries to say that val is always either greater than
> >> > LONG_MAX or smaller than LONG_MIN.
> >>
> >> Exactly, the warning is about the fact that all the other conditions
> >> will never be evaluated.  The phrasing is admittedly quite cryptic tho.
> >
> > I thought you said this was a setup where the argument _does_ fit a long?
>
> There we are dispatching if 'val' can be expressed or not with a long.
>
> 'val' is an EMACS_INT so depending on the configuration it might fit in
> a long by definition, when this happen GCC sees the first condition in
> or as always true etc etc... :)

So val is greater than LONG_MAX? How can this happen?

I've got some initial results here that seem to show a serious bug in
my backend in this situation, and I hope it's not a gcc bug...

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Pip Cet <pipcet@gmail.com> writes:

> On Fri, Feb 19, 2021 at 2:35 PM Andrea Corallo <akrl@sdf.org> wrote:
>> > On Fri, Feb 19, 2021 at 2:16 PM Andrea Corallo <akrl@sdf.org> wrote:
>> >> >> > comp.c:1174:22: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
>> >> >> >  1174 |   if (val > LONG_MAX || val < LONG_MIN)
>> >> >> >       |                      ^~
>> >> >> >
>> >> >> > Not sure what's the best way to silence it or if we want to revert to
>> >> >> > the previous formulation.
>> >> >>
>> >> >> Maybe it's me, but I had a hard time parsing that warning. IIUC, the
>> >> >> tests are both trivially false? And oring two trivially false
>> >> >> conditions shouldn't trigger a warning, should it?
>> >> >
>> >> > No, I think it tries to say that val is always either greater than
>> >> > LONG_MAX or smaller than LONG_MIN.
>> >>
>> >> Exactly, the warning is about the fact that all the other conditions
>> >> will never be evaluated.  The phrasing is admittedly quite cryptic tho.
>> >
>> > I thought you said this was a setup where the argument _does_ fit a long?
>>
>> There we are dispatching if 'val' can be expressed or not with a long.
>>
>> 'val' is an EMACS_INT so depending on the configuration it might fit in
>> a long by definition, when this happen GCC sees the first condition in
>> or as always true etc etc... :)
>
> So val is greater than LONG_MAX? How can this happen?

Apologies I was wrong, Eli's theory from his last message sounds like
the correct one.

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Andy Moreton]

On Fri 19 Feb 2021, Pip Cet wrote:

> On Fri, Feb 19, 2021 at 2:35 PM Andrea Corallo <akrl@sdf.org> wrote:
>> > On Fri, Feb 19, 2021 at 2:16 PM Andrea Corallo <akrl@sdf.org> wrote:
>> >> >> > comp.c:1174:22: warning: logical ‘or’ of collectively exhaustive tests is always true [-Wlogical-op]
>> >> >> >  1174 |   if (val > LONG_MAX || val < LONG_MIN)
>> >> >> >       |                      ^~
>> >> >> >
>> >> >> > Not sure what's the best way to silence it or if we want to revert to
>> >> >> > the previous formulation.
>> >> >>
>> >> >> Maybe it's me, but I had a hard time parsing that warning. IIUC, the
>> >> >> tests are both trivially false? And oring two trivially false
>> >> >> conditions shouldn't trigger a warning, should it?
>> >> >
>> >> > No, I think it tries to say that val is always either greater than
>> >> > LONG_MAX or smaller than LONG_MIN.
>> >>
>> >> Exactly, the warning is about the fact that all the other conditions
>> >> will never be evaluated.  The phrasing is admittedly quite cryptic tho.
>> >
>> > I thought you said this was a setup where the argument _does_ fit a long?
>>
>> There we are dispatching if 'val' can be expressed or not with a long.
>>
>> 'val' is an EMACS_INT so depending on the configuration it might fit in
>> a long by definition, when this happen GCC sees the first condition in
>> or as always true etc etc... :)
>
> So val is greater than LONG_MAX? How can this happen?

A 32bit emacs configured with "--with-wide-int" where EMACS_INT is wider
than long.

    AndyM

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> This should have a big FIXME, since it means the name of the DLL is in
> two different places, and if we need to support a differently-named
> DLL, we are in trouble:

Done with 805cae572a.

>   +#ifdef WINDOWSNT
>   +  /* We may need to load libgccjit when dumping before term/w32-win.el
>   +     defines `dynamic-library-alist`. This will fail if that variable
>   +     is empty, so add libgccjit-0.dll to it.  */
>   +  if (will_dump_p ())
>   +    Vdynamic_library_alist = list1 (list2 (Qgccjit,
>   +                                           build_string ("libgccjit-0.dll")));
>
> Is there a more elegant way of resolving this difficulty?

I've no precise idea.  I guess `dynamic-library-alist` should be
initialized before any compilation can be triggered, not sure how easy
is that, is also something I can't test and as a consequence develop.

> This lacks the ENCODE_FILE part:
>
>   +           char *fname = SSDATA (concat2 (Vinvocation_directory,
>   +                                          XCAR (comp_u->file)));
>   +           FILE *file;
>   +           if ((file = fopen (fname, "r")))
>
> And why are you using fopen instead of emacs_fopen?

2110a3faf7 fix both suggestons.

Thanks

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
> Date: Thu, 18 Feb 2021 21:56:47 +0000
> 
> >   +#ifdef WINDOWSNT
> >   +  /* We may need to load libgccjit when dumping before term/w32-win.el
> >   +     defines `dynamic-library-alist`. This will fail if that variable
> >   +     is empty, so add libgccjit-0.dll to it.  */
> >   +  if (will_dump_p ())
> >   +    Vdynamic_library_alist = list1 (list2 (Qgccjit,
> >   +                                           build_string ("libgccjit-0.dll")));
> >
> > Is there a more elegant way of resolving this difficulty?
> 
> I've no precise idea.  I guess `dynamic-library-alist` should be
> initialized before any compilation can be triggered, not sure how easy
> is that, is also something I can't test and as a consequence develop.

If libgccjit DLL must be loaded early on, perhaps it shouldn't use the
Vdynamic_library_alist machinery at all, but have its own loading
code?

Anyway, we can delay this cleanup until the branch is merged with
master, that's why I asked for  FIXME.

> > This lacks the ENCODE_FILE part:
> >
> >   +           char *fname = SSDATA (concat2 (Vinvocation_directory,
> >   +                                          XCAR (comp_u->file)));
> >   +           FILE *file;
> >   +           if ((file = fopen (fname, "r")))
> >
> > And why are you using fopen instead of emacs_fopen?
> 
> 2110a3faf7 fix both suggestons.

Thanks.  For the future: it is important to go through existing Emacs
APIs that accept file names, because on Windows we must convert UTF-8
encoded file names to UTF-16, so as to provide full support for
non-ASCII file names and directory names.  This conversion happens in
w32.c, which defines wrappers for functions like fopen, so directly
calling those C APIs is generally a bug waiting to happen.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Side question:  do we all agree that the current configure flag
'--with-nativecomp' should be '--with-native-comp'?

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: 43725@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>,
>         Lars Ingebrigtsen
>  <larsi@gnus.org>, monnier@iro.umontreal.ca
> Date: Thu, 18 Feb 2021 21:58:32 +0000
> 
> Side question:  do we all agree that the current configure flag
> '--with-nativecomp' should be '--with-native-comp'?

Yes, '--with-native-comp' sounds better.  Maybe even
'--with-native-compilation', even though it's longer.  (But since this
option will become ON by default, users will not have to type it in
most cases.)

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: 43725@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>,
>>         Lars Ingebrigtsen
>>  <larsi@gnus.org>, monnier@iro.umontreal.ca
>> Date: Thu, 18 Feb 2021 21:58:32 +0000
>> 
>> Side question:  do we all agree that the current configure flag
>> '--with-nativecomp' should be '--with-native-comp'?
>
> Yes, '--with-native-comp' sounds better.  Maybe even
> '--with-native-compilation', even though it's longer.  (But since this
> option will become ON by default, users will not have to type it in
> most cases.)

I pushed the change for having '--with-native-comp' but coming here to
acknowledge that I recalled that also '--with-native-compilation' was
suggested.  Cause I think is important to change it only once I reverted
the commit.

Thinking about if we don't need to use shortcuts also
'--with-native-compiler' might be okay WDYT?

Lets just pick the one we like and I'll make the change.

Thanks

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Pip Cet]

On Mon, Feb 22, 2021 at 8:09 PM Andrea Corallo via Bug reports for GNU
Emacs, the Swiss army knife of text editors <bug-gnu-emacs@gnu.org>
wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> >> From: Andrea Corallo <akrl@sdf.org>
> > Yes, '--with-native-comp' sounds better.  Maybe even
> > '--with-native-compilation', even though it's longer.  (But since this
> > option will become ON by default, users will not have to type it in
> > most cases.)
>
> I pushed the change for having '--with-native-comp' but coming here to
> acknowledge that I recalled that also '--with-native-compilation' was
> suggested.  Cause I think is important to change it only once I reverted
> the commit.

I'm not sure about the --with-, though.
--enable-native-comp{,ilation,iler} might be more consistent with
other configures. (and --with-libgccjit would go with it)

> Thinking about if we don't need to use shortcuts also
> '--with-native-compiler' might be okay WDYT?
>
> Lets just pick the one we like and I'll make the change.

I still think it would be good to split out the ability to run
natively-compiled files from the ability to generate them. We should
plan for configure options for that.

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: 43725@debbugs.gnu.org, larsi@gnus.org, monnier@iro.umontreal.ca
> Date: Mon, 22 Feb 2021 20:08:22 +0000
> 
> Thinking about if we don't need to use shortcuts also
> '--with-native-compiler' might be okay WDYT?

That sounds less appropriate to me.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: 43725@debbugs.gnu.org, larsi@gnus.org, monnier@iro.umontreal.ca
>> Date: Mon, 22 Feb 2021 20:08:22 +0000
>> 
>> Thinking about if we don't need to use shortcuts also
>> '--with-native-compiler' might be okay WDYT?
>
> That sounds less appropriate to me.
>

Okay, 42fc752a14 changes the configure flag into
'--with-native-compilation'.

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Date: Fri, 26 Feb 2021 19:31:20 +0000
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
> 
> >> Thinking about if we don't need to use shortcuts also
> >> '--with-native-compiler' might be okay WDYT?
> >
> > That sounds less appropriate to me.
> >
> 
> Okay, 42fc752a14 changes the configure flag into
> '--with-native-compilation'.

Thanks.

Looking at a running Emacs, I see that it loads the *.eln files that
are preloaded.  I would expect those to be dumped into the emacs.pdmp
file, as it happens with *.elc files.  If all the stuff is dumped into
emacs.pdmp and loaded from there, why does Emacs still need to load
those *.eln files as shared libraries?

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Date: Fri, 26 Feb 2021 19:31:20 +0000
>> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
>> 
>> >> Thinking about if we don't need to use shortcuts also
>> >> '--with-native-compiler' might be okay WDYT?
>> >
>> > That sounds less appropriate to me.
>> >
>> 
>> Okay, 42fc752a14 changes the configure flag into
>> '--with-native-compilation'.
>
> Thanks.
>
> Looking at a running Emacs, I see that it loads the *.eln files that
> are preloaded.  I would expect those to be dumped into the emacs.pdmp
> file, as it happens with *.elc files.  If all the stuff is dumped into
> emacs.pdmp and loaded from there, why does Emacs still need to load
> those *.eln files as shared libraries?

For loaded you mean entering in 'load_comp_unit'?

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
> Date: Tue, 09 Mar 2021 19:28:19 +0000
> 
> > Looking at a running Emacs, I see that it loads the *.eln files that
> > are preloaded.  I would expect those to be dumped into the emacs.pdmp
> > file, as it happens with *.elc files.  If all the stuff is dumped into
> > emacs.pdmp and loaded from there, why does Emacs still need to load
> > those *.eln files as shared libraries?
> 
> For loaded you mean entering in 'load_comp_unit'?

I looked at the shared libraries loaded by the program as shown by the
GDB "info sharedlibrary" command.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
>> Date: Tue, 09 Mar 2021 19:28:19 +0000
>> 
>> > Looking at a running Emacs, I see that it loads the *.eln files that
>> > are preloaded.  I would expect those to be dumped into the emacs.pdmp
>> > file, as it happens with *.elc files.  If all the stuff is dumped into
>> > emacs.pdmp and loaded from there, why does Emacs still need to load
>> > those *.eln files as shared libraries?
>> 
>> For loaded you mean entering in 'load_comp_unit'?
>
> I looked at the shared libraries loaded by the program as shown by the
> GDB "info sharedlibrary" command.

Okay, that's normal.  The pdumper is dumping only the pseudovector
representing the CU.  When the image is loaded pdumper calls
'load_comp_unit' to have the dynamic linker map again the .eln into
memory.

I think here [1] at the time I wrote something about the reasons why I
ended-up with this solution, but the long story short is that the only
affordable and reliable way to have this job done is to ask the dynamic
linker to do it.

  Andrea

[1] <http://akrl.sdf.org/gccemacs.html#orgc59fae6>

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
> Date: Tue, 09 Mar 2021 20:09:03 +0000
> 
> >> > Looking at a running Emacs, I see that it loads the *.eln files that
> >> > are preloaded.  I would expect those to be dumped into the emacs.pdmp
> >> > file, as it happens with *.elc files.  If all the stuff is dumped into
> >> > emacs.pdmp and loaded from there, why does Emacs still need to load
> >> > those *.eln files as shared libraries?
> >> 
> >> For loaded you mean entering in 'load_comp_unit'?
> >
> > I looked at the shared libraries loaded by the program as shown by the
> > GDB "info sharedlibrary" command.
> 
> Okay, that's normal.  The pdumper is dumping only the pseudovector
> representing the CU.  When the image is loaded pdumper calls
> 'load_comp_unit' to have the dynamic linker map again the .eln into
> memory.

This means that, unlike with preloaded *.elc files, one cannot delete
the preloaded *.eln files once the pdumper file is created, because
the *.eln files are still needed each time Emacs starts.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
>> Date: Tue, 09 Mar 2021 20:09:03 +0000
>> 
>> >> > Looking at a running Emacs, I see that it loads the *.eln files that
>> >> > are preloaded.  I would expect those to be dumped into the emacs.pdmp
>> >> > file, as it happens with *.elc files.  If all the stuff is dumped into
>> >> > emacs.pdmp and loaded from there, why does Emacs still need to load
>> >> > those *.eln files as shared libraries?
>> >> 
>> >> For loaded you mean entering in 'load_comp_unit'?
>> >
>> > I looked at the shared libraries loaded by the program as shown by the
>> > GDB "info sharedlibrary" command.
>> 
>> Okay, that's normal.  The pdumper is dumping only the pseudovector
>> representing the CU.  When the image is loaded pdumper calls
>> 'load_comp_unit' to have the dynamic linker map again the .eln into
>> memory.
>
> This means that, unlike with preloaded *.elc files, one cannot delete
> the preloaded *.eln files once the pdumper file is created, because
> the *.eln files are still needed each time Emacs starts.

Precisely.

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

If I do this:

   $ touch lisp/emacs-lisp/comp.el
   $ ./src/emacs -Q
   C-h f apropos RET

then I see that Emacs starts natively-compiling comp.el, even though I
already have a comp.eln file in the eln-cache.

This is unlike what happens with the .elc files, where Emacs will
never byte-compile a file without my say-so.  I could perhaps
understand the automatic compilation if there was no .eln file
anywhere in sight, but why should Emacs automatically compile Lisp
files it loads when a .eln file for them does exist?

Does this happen with any .el file, or is comp.el special in some
sense?

One situation where this gets in the way is when I make some changes
in a .el file because I'm testing something or debugging some problem.
In those cases I usually load a .el file manually and later either
undo the changes or make them permanent, and re-byte-compile at that
time.  But with natively-compilation it sounds like I've lost control
on when the file is compiled and which version of it is compiled.

Is this a reasonable default behavior?  Maybe it is reasonable for
users who just use Emacs.  But for developers that constantly make
changes in .el files this could be a nuisance, at least sometimes.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> If I do this:
>
>    $ touch lisp/emacs-lisp/comp.el
>    $ ./src/emacs -Q
>    C-h f apropos RET
>
> then I see that Emacs starts natively-compiling comp.el, even though I
> already have a comp.eln file in the eln-cache.
>
> This is unlike what happens with the .elc files, where Emacs will
> never byte-compile a file without my say-so.  I could perhaps
> understand the automatic compilation if there was no .eln file
> anywhere in sight, but why should Emacs automatically compile Lisp
> files it loads when a .eln file for them does exist?
>
> Does this happen with any .el file, or is comp.el special in some
> sense?
>
> One situation where this gets in the way is when I make some changes
> in a .el file because I'm testing something or debugging some problem.
> In those cases I usually load a .el file manually and later either
> undo the changes or make them permanent, and re-byte-compile at that
> time.  But with natively-compilation it sounds like I've lost control
> on when the file is compiled and which version of it is compiled.
>
> Is this a reasonable default behavior?  Maybe it is reasonable for
> users who just use Emacs.  But for developers that constantly make
> changes in .el files this could be a nuisance, at least sometimes.

Hi Eli,

this is not expected and I cannot reproduce it here.

The file is hashed using the content + its filename so access and
modification times should not come into play.  Are we sure the file
content wasn't modified?  Or we might be possibly looking at different
issue here.

Thanks

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
> Date: Wed, 10 Mar 2021 15:45:49 +0000
> 
> this is not expected and I cannot reproduce it here.

OK, so just to be sure we are taking about the same scenario:

  . I touch comp.el while a comp-NNNNN.eln already exists in
    ~/.emacs.d/eln-cache.
  . I then start "emacs -Q" and invoke "C-h f" for some function whose
    .el file doesn't yet have a .eln file.  For example,

        C-h f ruler-mode RET

  . What I see next is that Emacs starts compiling in the background,
    and I later find that both ruler-mode.el and comp.el have been
    natively-compiled.

Hmmm... now I see that it doesn't happen every time.  So I guess some
additional factor is at work here, indeed.

> The file is hashed using the content + its filename so access and
> modification times should not come into play.  Are we sure the file
> content wasn't modified?

The file wasn't modified, that's for sure.

> Or we might be possibly looking at different issue here.

Probably.  I'll try to step through the code, but could you give me
some pointers: where do we decide whether to native-compile a file
that we load?

Thanks.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
>> Date: Wed, 10 Mar 2021 15:45:49 +0000
>> 
>> this is not expected and I cannot reproduce it here.
>
> OK, so just to be sure we are taking about the same scenario:
>
>   . I touch comp.el while a comp-NNNNN.eln already exists in
>     ~/.emacs.d/eln-cache.
>   . I then start "emacs -Q" and invoke "C-h f" for some function whose
>     .el file doesn't yet have a .eln file.  For example,
>
>         C-h f ruler-mode RET
>
>   . What I see next is that Emacs starts compiling in the background,
>     and I later find that both ruler-mode.el and comp.el have been
>     natively-compiled.
>
> Hmmm... now I see that it doesn't happen every time.  So I guess some
> additional factor is at work here, indeed.
>
>> The file is hashed using the content + its filename so access and
>> modification times should not come into play.  Are we sure the file
>> content wasn't modified?
>
> The file wasn't modified, that's for sure.
>
>> Or we might be possibly looking at different issue here.
>
> Probably.  I'll try to step through the code, but could you give me
> some pointers: where do we decide whether to native-compile a file
> that we load?

The triggering point we are interested in here should be
'maybe_defer_native_compilation' (called by 'Fdefalias').

Thanks

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Andrea Corallo via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Andrea Corallo <akrl@sdf.org>
>>> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
>>> Date: Wed, 10 Mar 2021 15:45:49 +0000
>>> 
>>> this is not expected and I cannot reproduce it here.
>>
>> OK, so just to be sure we are taking about the same scenario:
>>
>>   . I touch comp.el while a comp-NNNNN.eln already exists in
>>     ~/.emacs.d/eln-cache.
>>   . I then start "emacs -Q" and invoke "C-h f" for some function whose
>>     .el file doesn't yet have a .eln file.  For example,
>>
>>         C-h f ruler-mode RET
>>
>>   . What I see next is that Emacs starts compiling in the background,
>>     and I later find that both ruler-mode.el and comp.el have been
>>     natively-compiled.
>>
>> Hmmm... now I see that it doesn't happen every time.  So I guess some
>> additional factor is at work here, indeed.
>>
>>> The file is hashed using the content + its filename so access and
>>> modification times should not come into play.  Are we sure the file
>>> content wasn't modified?
>>
>> The file wasn't modified, that's for sure.
>>
>>> Or we might be possibly looking at different issue here.
>>
>> Probably.  I'll try to step through the code, but could you give me
>> some pointers: where do we decide whether to native-compile a file
>> that we load?
>
> The triggering point we are interested in here should be
> 'maybe_defer_native_compilation' (called by 'Fdefalias').

Sorry thinking about I believe for this case would be more interesting
to see why 'maybe_swap_for_eln' is failing to find the corresponding
.eln.  This is called in 'openp'.

Thanks

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

Andrea, I see in comp.c and comp.el you many times use 'concat'
instead of expand-file-name to construct a file name from the leading
directories and a basename.  Sometimes even a mix of the two.  Here's
one example:

  return Fexpand_file_name (filename,
			    concat2 (Ffile_name_as_directory (base_dir),
				     Vcomp_native_version_dir));

Is there a reason for using 'concat' in these cases?  E.g., the above
could be written as

  return Fexpand_file_name (filename
  	 		    Fexpand_file_name (Vcomp_native_version_dir,
			   		       base_dir));

which also makes only 2 function calls instead of 3.  Am I missing
something?

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> Andrea, I see in comp.c and comp.el you many times use 'concat'
> instead of expand-file-name to construct a file name from the leading
> directories and a basename.  Sometimes even a mix of the two.  Here's
> one example:
>
>   return Fexpand_file_name (filename,
> 			    concat2 (Ffile_name_as_directory (base_dir),
> 				     Vcomp_native_version_dir));
>
> Is there a reason for using 'concat' in these cases?  E.g., the above
> could be written as
>
>   return Fexpand_file_name (filename
>   	 		    Fexpand_file_name (Vcomp_native_version_dir,
> 			   		       base_dir));
>
> which also makes only 2 function calls instead of 3.  Am I missing
> something?

I guess is more likely that *I* missed something here :)

Probably in this case I thought that because I've used
Ffile_name_as_directory its output could just be concatenated without
having to use Fexpand_file_name.

Is this practice incorrect or dangerous?

Thanks

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
> Date: Sun, 14 Mar 2021 19:06:01 +0000
> 
> Probably in this case I thought that because I've used
> Ffile_name_as_directory its output could just be concatenated without
> having to use Fexpand_file_name.
> 
> Is this practice incorrect or dangerous?

It's "not recommended".  The reason is that expand-file-name deals
with irregularities such as multiple consecutive slashes, unibyte vs
multibyte strings, drive letters on MS-Windows, etc., and 'concat'
doesn't.

I will go over the code and replace 'concat' where needed.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
>> Date: Sun, 14 Mar 2021 19:06:01 +0000
>> 
>> Probably in this case I thought that because I've used
>> Ffile_name_as_directory its output could just be concatenated without
>> having to use Fexpand_file_name.
>> 
>> Is this practice incorrect or dangerous?
>
> It's "not recommended".  The reason is that expand-file-name deals
> with irregularities such as multiple consecutive slashes, unibyte vs
> multibyte strings, drive letters on MS-Windows, etc., and 'concat'
> doesn't.

I probably thought there was safe by construction but I see your point.

> I will go over the code and replace 'concat' where needed.

Thanks (again)

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

One issue that I have in my notes about native-comp is the issue with
input encoding of the source we submit to the native-compiler.

The issue is this: our Lisp files are generally encoded in UTF-8.  But
there's no guarantee that GCC expects the source to be encoded in
UTF-8.  If the text we submit to libgccjit for compilation is in the
same encoding as the original .el files, then shouldn't we tell GCC
its actual encoding, to make sure non-ASCII text aren't mangled?

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> One issue that I have in my notes about native-comp is the issue with
> input encoding of the source we submit to the native-compiler.
>
> The issue is this: our Lisp files are generally encoded in UTF-8.  But
> there's no guarantee that GCC expects the source to be encoded in
> UTF-8.  If the text we submit to libgccjit for compilation is in the
> same encoding as the original .el files, then shouldn't we tell GCC
> its actual encoding, to make sure non-ASCII text aren't mangled?

The only generated text we are passing to libgccjit is feed to
'gcc_jit_context_new_string_literal'.  This text is generated from
'Fprin1_to_string' that we use to essentially render in form of text
immediate objects.  I thought this was disconnected from the encoding of
the source file, am I wrong?

Thanks

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
> Date: Mon, 15 Mar 2021 15:43:07 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > The issue is this: our Lisp files are generally encoded in UTF-8.  But
> > there's no guarantee that GCC expects the source to be encoded in
> > UTF-8.  If the text we submit to libgccjit for compilation is in the
> > same encoding as the original .el files, then shouldn't we tell GCC
> > its actual encoding, to make sure non-ASCII text aren't mangled?
> 
> The only generated text we are passing to libgccjit is feed to
> 'gcc_jit_context_new_string_literal'.

What kind of "string literals" go this way?  Are these constant
strings from Lisp, or can some other data types from Lisp end up being
passed like that?

> This text is generated from 'Fprin1_to_string' that we use to
> essentially render in form of text immediate objects.  I thought
> this was disconnected from the encoding of the source file, am I
> wrong?

If the original text is from Lisp strings in the Lisp source file,
then it isn't disconnected.  Those strings could include non-ASCII
characters, and they come out in UTF-8 (well, almost, modulo the raw
bytes etc.).

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrea Corallo <akrl@sdf.org>
>> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
>> Date: Mon, 15 Mar 2021 15:43:07 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > The issue is this: our Lisp files are generally encoded in UTF-8.  But
>> > there's no guarantee that GCC expects the source to be encoded in
>> > UTF-8.  If the text we submit to libgccjit for compilation is in the
>> > same encoding as the original .el files, then shouldn't we tell GCC
>> > its actual encoding, to make sure non-ASCII text aren't mangled?
>> 
>> The only generated text we are passing to libgccjit is feed to
>> 'gcc_jit_context_new_string_literal'.
>
> What kind of "string literals" go this way?  Are these constant
> strings from Lisp, or can some other data types from Lisp end up being
> passed like that?

This are essentially all Lisp objects that we'd put in the constant
vector for the bytecode case.

>> This text is generated from 'Fprin1_to_string' that we use to
>> essentially render in form of text immediate objects.  I thought
>> this was disconnected from the encoding of the source file, am I
>> wrong?
>
> If the original text is from Lisp strings in the Lisp source file,
> then it isn't disconnected.  Those strings could include non-ASCII
> characters, and they come out in UTF-8 (well, almost, modulo the raw
> bytes etc.).

In december I've fixed a bug for multibyte strings adding also a
testcase for that.  This was bug#45342 and the commit fixing and adding
the testcase is 72c1a41573.

I'm no big expert into this area so you might want to have a look to the
commit to suggets if this covers already the case you are suggesting or
not.

Thanks!

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
> Date: Mon, 15 Mar 2021 20:00:17 +0000
> 
> >> This text is generated from 'Fprin1_to_string' that we use to
> >> essentially render in form of text immediate objects.  I thought
> >> this was disconnected from the encoding of the source file, am I
> >> wrong?
> >
> > If the original text is from Lisp strings in the Lisp source file,
> > then it isn't disconnected.  Those strings could include non-ASCII
> > characters, and they come out in UTF-8 (well, almost, modulo the raw
> > bytes etc.).
> 
> In december I've fixed a bug for multibyte strings adding also a
> testcase for that.  This was bug#45342 and the commit fixing and adding
> the testcase is 72c1a41573.
> 
> I'm no big expert into this area so you might want to have a look to the
> commit to suggets if this covers already the case you are suggesting or
> not.

SGTM, so I guess we can see this issue closed, until and unless we see
some specific bug reports.

Thanks.

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

We have the command byte-compile-file, but we don't have its
equivalent native-compile command.  Should we have a
native-compile-file command?  I think we should, and I think it
should run synchronously, like byte-compile-file, i.e. it should run
the compilation in the Emacs session where the command was invoked,
not in a subprocess.

WDYT?

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> We have the command byte-compile-file, but we don't have its
> equivalent native-compile command.  Should we have a
> native-compile-file command?  I think we should, and I think it
> should run synchronously, like byte-compile-file, i.e. it should run
> the compilation in the Emacs session where the command was invoked,
> not in a subprocess.
>
> WDYT?

`native-compile' invoked with a string as a FUNCTION-OR-FILE parameter
accomplish that, is this sufficient?

Thanks

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> From: Andrea Corallo <akrl@sdf.org>
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
> Date: Sun, 21 Mar 2021 08:32:59 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > We have the command byte-compile-file, but we don't have its
> > equivalent native-compile command.  Should we have a
> > native-compile-file command?  I think we should, and I think it
> > should run synchronously, like byte-compile-file, i.e. it should run
> > the compilation in the Emacs session where the command was invoked,
> > not in a subprocess.
> >
> > WDYT?
> 
> `native-compile' invoked with a string as a FUNCTION-OR-FILE parameter
> accomplish that, is this sufficient?

It could be, but it isn't a command.  And please look at
byte-compile-file: it offers useful features, such as compiling the
file visited in the current buffer by default.

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> Date: Sun, 14 Mar 2021 21:51:38 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org
> 
> I will go over the code and replace 'concat' where needed.

Done.

bug#43725: 28.0.50; Include feature/native-comp into master

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi all,

shall I close this bug?

  Andrea

bug#43725: 28.0.50; Include feature/native-comp into master

[Eli Zaretskii]

> Date: Sat, 01 May 2021 18:24:18 +0000
> From:  Andrea Corallo via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Hi all,
> 
> shall I close this bug?

It's about time.