unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / Atom feed
* bug#43725: 28.0.50; Include feature/native-comp into master
@ 2020-09-30 15:44 Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-09-30 17:27 ` Lars Ingebrigtsen
                   ` (2 more replies)
  0 siblings, 3 replies; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-09-30 15:44 UTC (permalink / raw)
  To: 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-09-30 15:44 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
@ 2020-09-30 17:27 ` Lars Ingebrigtsen
  2020-09-30 22:37   ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-02 19:39 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-11-25 18:35 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2 siblings, 1 reply; 61+ messages in thread
From: Lars Ingebrigtsen @ 2020-09-30 17:27 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-09-30 17:27 ` Lars Ingebrigtsen
@ 2020-09-30 22:37   ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-01  1:34     ` Lars Ingebrigtsen
  2020-10-01  2:40     ` Eli Zaretskii
  0 siblings, 2 replies; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-09-30 22:37 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-09-30 22:37   ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-10-01  1:34     ` Lars Ingebrigtsen
  2020-10-01  7:51       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-01  2:40     ` Eli Zaretskii
  1 sibling, 1 reply; 61+ messages in thread
From: Lars Ingebrigtsen @ 2020-10-01  1:34 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-09-30 22:37   ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-01  1:34     ` Lars Ingebrigtsen
@ 2020-10-01  2:40     ` Eli Zaretskii
  1 sibling, 0 replies; 61+ messages in thread
From: Eli Zaretskii @ 2020-10-01  2:40 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-10-01  1:34     ` Lars Ingebrigtsen
@ 2020-10-01  7:51       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-01 13:13         ` Eli Zaretskii
  0 siblings, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-10-01  7:51 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-10-01  7:51       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-10-01 13:13         ` Eli Zaretskii
  2020-10-01 13:40           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-01 15:55           ` Lars Ingebrigtsen
  0 siblings, 2 replies; 61+ messages in thread
From: Eli Zaretskii @ 2020-10-01 13:13 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-10-01 13:13         ` Eli Zaretskii
@ 2020-10-01 13:40           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-01 15:55           ` Lars Ingebrigtsen
  1 sibling, 0 replies; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-10-01 13:40 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-10-01 13:13         ` Eli Zaretskii
  2020-10-01 13:40           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-10-01 15:55           ` Lars Ingebrigtsen
  1 sibling, 0 replies; 61+ messages in thread
From: Lars Ingebrigtsen @ 2020-10-01 15:55 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 43725, Andrea Corallo

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-09-30 15:44 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
  2020-09-30 17:27 ` Lars Ingebrigtsen
@ 2020-10-02 19:39 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-02 19:46   ` Eli Zaretskii
  2020-11-25 18:35 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2 siblings, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-10-02 19:39 UTC (permalink / raw)
  To: 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-10-02 19:39 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-10-02 19:46   ` Eli Zaretskii
  2020-10-02 19:58     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 61+ messages in thread
From: Eli Zaretskii @ 2020-10-02 19:46 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-10-02 19:46   ` Eli Zaretskii
@ 2020-10-02 19:58     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-03  7:27       ` Eli Zaretskii
  0 siblings, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-10-02 19:58 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-10-02 19:58     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-10-03  7:27       ` Eli Zaretskii
  2020-10-03 17:23         ` Lars Ingebrigtsen
  2020-10-06 17:11         ` Stefan Monnier
  0 siblings, 2 replies; 61+ messages in thread
From: Eli Zaretskii @ 2020-10-03  7:27 UTC (permalink / raw)
  To: Andrea Corallo, Lars Ingebrigtsen; +Cc: 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-10-03  7:27       ` Eli Zaretskii
@ 2020-10-03 17:23         ` Lars Ingebrigtsen
  2020-10-03 18:40           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-06 17:11         ` Stefan Monnier
  1 sibling, 1 reply; 61+ messages in thread
From: Lars Ingebrigtsen @ 2020-10-03 17:23 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Lars Ingebrigtsen, Stefan Monnier, 43725, Andrea Corallo

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-10-03 17:23         ` Lars Ingebrigtsen
@ 2020-10-03 18:40           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-06 16:39             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-10-03 18:40 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Eli Zaretskii, Stefan Monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-10-03 18:40           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-10-06 16:39             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-07  4:28               ` Lars Ingebrigtsen
  0 siblings, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-10-06 16:39 UTC (permalink / raw)
  To: 43725; +Cc: larsi, eliz, monnier

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-10-03  7:27       ` Eli Zaretskii
  2020-10-03 17:23         ` Lars Ingebrigtsen
@ 2020-10-06 17:11         ` Stefan Monnier
  1 sibling, 0 replies; 61+ messages in thread
From: Stefan Monnier @ 2020-10-06 17:11 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 43725, Andrea Corallo

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

Yes,


        Stefan






^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-10-06 16:39             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-10-07  4:28               ` Lars Ingebrigtsen
  2020-10-07 14:47                 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 61+ messages in thread
From: Lars Ingebrigtsen @ 2020-10-07  4:28 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: Stefan Monnier, 43725

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






^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-10-07  4:28               ` Lars Ingebrigtsen
@ 2020-10-07 14:47                 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-10-09  4:20                   ` Lars Ingebrigtsen
  0 siblings, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-10-07 14:47 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Eli Zaretskii, Stefan Monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-10-07 14:47                 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-10-09  4:20                   ` Lars Ingebrigtsen
  0 siblings, 0 replies; 61+ messages in thread
From: Lars Ingebrigtsen @ 2020-10-09  4:20 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: Stefan Monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-09-30 15:44 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
  2020-09-30 17:27 ` Lars Ingebrigtsen
  2020-10-02 19:39 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-11-25 18:35 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-11-26 10:10   ` Lars Ingebrigtsen
  2021-02-18 21:58   ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2 siblings, 2 replies; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-11-25 18:35 UTC (permalink / raw)
  To: 43725; +Cc: Eli Zaretskii, Lars Ingebrigtsen, monnier

Hi all,

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

Regards

  Andrea





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-11-25 18:35 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-11-26 10:10   ` Lars Ingebrigtsen
  2020-11-26 11:19     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-11-26 14:17     ` Eli Zaretskii
  2021-02-18 21:58   ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 2 replies; 61+ messages in thread
From: Lars Ingebrigtsen @ 2020-11-26 10:10 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-11-26 10:10   ` Lars Ingebrigtsen
@ 2020-11-26 11:19     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-11-26 11:28       ` Lars Ingebrigtsen
  2020-11-26 14:17     ` Eli Zaretskii
  1 sibling, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-11-26 11:19 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Eli Zaretskii, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-11-26 11:19     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-11-26 11:28       ` Lars Ingebrigtsen
  2020-11-26 12:02         ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 61+ messages in thread
From: Lars Ingebrigtsen @ 2020-11-26 11:28 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-11-26 11:28       ` Lars Ingebrigtsen
@ 2020-11-26 12:02         ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-12-04 22:28           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-11-26 12:02 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Eli Zaretskii, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-11-26 10:10   ` Lars Ingebrigtsen
  2020-11-26 11:19     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-11-26 14:17     ` Eli Zaretskii
  2020-11-26 21:15       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 1 reply; 61+ messages in thread
From: Eli Zaretskii @ 2020-11-26 14:17 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: monnier, 43725, akrl

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-11-26 14:17     ` Eli Zaretskii
@ 2020-11-26 21:15       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-11-27  7:16         ` Eli Zaretskii
  0 siblings, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-11-26 21:15 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Lars Ingebrigtsen, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-11-26 21:15       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-11-27  7:16         ` Eli Zaretskii
  2021-02-13 20:06           ` Eli Zaretskii
  0 siblings, 1 reply; 61+ messages in thread
From: Eli Zaretskii @ 2020-11-27  7:16 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-11-26 12:02         ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-12-04 22:28           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-12-06 13:13             ` Lars Ingebrigtsen
  0 siblings, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2020-12-04 22:28 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Eli Zaretskii, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-12-04 22:28           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2020-12-06 13:13             ` Lars Ingebrigtsen
  0 siblings, 0 replies; 61+ messages in thread
From: Lars Ingebrigtsen @ 2020-12-06 13:13 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-11-27  7:16         ` Eli Zaretskii
@ 2021-02-13 20:06           ` Eli Zaretskii
  2021-02-14 20:22             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
                               ` (2 more replies)
  0 siblings, 3 replies; 61+ messages in thread
From: Eli Zaretskii @ 2021-02-13 20:06 UTC (permalink / raw)
  To: akrl; +Cc: larsi, monnier, 43725

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?





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-13 20:06           ` Eli Zaretskii
@ 2021-02-14 20:22             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-16 21:13             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-18 21:56             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2 siblings, 0 replies; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-02-14 20:22 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-13 20:06           ` Eli Zaretskii
  2021-02-14 20:22             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-02-16 21:13             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-17 15:18               ` Eli Zaretskii
  2021-02-18 20:32               ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-18 21:56             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2 siblings, 2 replies; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-02-16 21:13 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-16 21:13             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-02-17 15:18               ` Eli Zaretskii
  2021-02-18 21:22                 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-18 20:32               ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 1 reply; 61+ messages in thread
From: Eli Zaretskii @ 2021-02-17 15:18 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-16 21:13             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-17 15:18               ` Eli Zaretskii
@ 2021-02-18 20:32               ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-18 20:36                 ` Eli Zaretskii
  2021-02-19 12:53                 ` Pip Cet
  1 sibling, 2 replies; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-02-18 20:32 UTC (permalink / raw)
  To: 43725; +Cc: eliz, larsi, monnier

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-18 20:32               ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-02-18 20:36                 ` Eli Zaretskii
  2021-02-18 20:53                   ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-19 12:53                 ` Pip Cet
  1 sibling, 1 reply; 61+ messages in thread
From: Eli Zaretskii @ 2021-02-18 20:36 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-18 20:36                 ` Eli Zaretskii
@ 2021-02-18 20:53                   ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-19  7:51                     ` Eli Zaretskii
  0 siblings, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-02-18 20:53 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 43725, larsi, monnier

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-17 15:18               ` Eli Zaretskii
@ 2021-02-18 21:22                 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-19  8:08                   ` Eli Zaretskii
  0 siblings, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-02-18 21:22 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-13 20:06           ` Eli Zaretskii
  2021-02-14 20:22             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-16 21:13             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-02-18 21:56             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-19  8:14               ` Eli Zaretskii
  2 siblings, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-02-18 21:56 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2020-11-25 18:35 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2020-11-26 10:10   ` Lars Ingebrigtsen
@ 2021-02-18 21:58   ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-19  8:16     ` Eli Zaretskii
  1 sibling, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-02-18 21:58 UTC (permalink / raw)
  To: 43725; +Cc: eliz, larsi, monnier

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

  Andrea





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-18 20:53                   ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-02-19  7:51                     ` Eli Zaretskii
  2021-02-19 11:13                       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 61+ messages in thread
From: Eli Zaretskii @ 2021-02-19  7:51 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-18 21:22                 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-02-19  8:08                   ` Eli Zaretskii
  0 siblings, 0 replies; 61+ messages in thread
From: Eli Zaretskii @ 2021-02-19  8:08 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-18 21:56             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-02-19  8:14               ` Eli Zaretskii
  0 siblings, 0 replies; 61+ messages in thread
From: Eli Zaretskii @ 2021-02-19  8:14 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-18 21:58   ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-02-19  8:16     ` Eli Zaretskii
  2021-02-22 20:08       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 61+ messages in thread
From: Eli Zaretskii @ 2021-02-19  8:16 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-19  7:51                     ` Eli Zaretskii
@ 2021-02-19 11:13                       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-19 12:23                         ` Eli Zaretskii
  0 siblings, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-02-19 11:13 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 43725, larsi, monnier

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-19 11:13                       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-02-19 12:23                         ` Eli Zaretskii
  2021-02-19 22:03                           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 61+ messages in thread
From: Eli Zaretskii @ 2021-02-19 12:23 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-18 20:32               ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-18 20:36                 ` Eli Zaretskii
@ 2021-02-19 12:53                 ` Pip Cet
  2021-02-19 13:37                   ` Eli Zaretskii
  1 sibling, 1 reply; 61+ messages in thread
From: Pip Cet @ 2021-02-19 12:53 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, 43725, Stefan Monnier

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.





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-19 12:53                 ` Pip Cet
@ 2021-02-19 13:37                   ` Eli Zaretskii
  2021-02-19 14:16                     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 61+ messages in thread
From: Eli Zaretskii @ 2021-02-19 13:37 UTC (permalink / raw)
  To: Pip Cet; +Cc: larsi, monnier, 43725, akrl

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-19 13:37                   ` Eli Zaretskii
@ 2021-02-19 14:16                     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-19 14:23                       ` Pip Cet
  0 siblings, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-02-19 14:16 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, 43725, Pip Cet, monnier

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-19 14:16                     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-02-19 14:23                       ` Pip Cet
  2021-02-19 14:35                         ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 61+ messages in thread
From: Pip Cet @ 2021-02-19 14:23 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, 43725, Stefan Monnier

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?





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-19 14:23                       ` Pip Cet
@ 2021-02-19 14:35                         ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-19 14:59                           ` Eli Zaretskii
  2021-02-19 15:00                           ` Pip Cet
  0 siblings, 2 replies; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-02-19 14:35 UTC (permalink / raw)
  To: Pip Cet; +Cc: Eli Zaretskii, larsi, 43725, Stefan Monnier

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... :)





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-19 14:35                         ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-02-19 14:59                           ` Eli Zaretskii
  2021-02-19 15:00                           ` Pip Cet
  1 sibling, 0 replies; 61+ messages in thread
From: Eli Zaretskii @ 2021-02-19 14:59 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, pipcet, 43725, monnier

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-19 14:35                         ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-19 14:59                           ` Eli Zaretskii
@ 2021-02-19 15:00                           ` Pip Cet
  2021-02-19 15:11                             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-19 17:33                             ` Andy Moreton
  1 sibling, 2 replies; 61+ messages in thread
From: Pip Cet @ 2021-02-19 15:00 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, 43725, Stefan Monnier

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-19 15:00                           ` Pip Cet
@ 2021-02-19 15:11                             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-19 17:33                             ` Andy Moreton
  1 sibling, 0 replies; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-02-19 15:11 UTC (permalink / raw)
  To: Pip Cet; +Cc: Eli Zaretskii, larsi, 43725, Stefan Monnier

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-19 15:00                           ` Pip Cet
  2021-02-19 15:11                             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-02-19 17:33                             ` Andy Moreton
  1 sibling, 0 replies; 61+ messages in thread
From: Andy Moreton @ 2021-02-19 17:33 UTC (permalink / raw)
  To: 43725

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






^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-19 12:23                         ` Eli Zaretskii
@ 2021-02-19 22:03                           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-20  6:57                             ` Eli Zaretskii
  0 siblings, 1 reply; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-02-19 22:03 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-19 22:03                           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-02-20  6:57                             ` Eli Zaretskii
  0 siblings, 0 replies; 61+ messages in thread
From: Eli Zaretskii @ 2021-02-20  6:57 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-19  8:16     ` Eli Zaretskii
@ 2021-02-22 20:08       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-22 20:51         ` Pip Cet
  2021-02-23  3:23         ` Eli Zaretskii
  0 siblings, 2 replies; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-02-22 20:08 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-22 20:08       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-02-22 20:51         ` Pip Cet
  2021-02-23  3:23         ` Eli Zaretskii
  1 sibling, 0 replies; 61+ messages in thread
From: Pip Cet @ 2021-02-22 20:51 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, 43725, Stefan Monnier

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.





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-22 20:08       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2021-02-22 20:51         ` Pip Cet
@ 2021-02-23  3:23         ` Eli Zaretskii
  2021-02-26 19:31           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 1 reply; 61+ messages in thread
From: Eli Zaretskii @ 2021-02-23  3:23 UTC (permalink / raw)
  To: Andrea Corallo; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

* bug#43725: 28.0.50; Include feature/native-comp into master
  2021-02-23  3:23         ` Eli Zaretskii
@ 2021-02-26 19:31           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 0 replies; 61+ messages in thread
From: Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-02-26 19:31 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, monnier, 43725

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





^ permalink raw reply	[flat|nested] 61+ messages in thread

end of thread, other threads:[~2021-02-26 19:31 UTC | newest]

Thread overview: 61+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-30 15:44 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
2020-09-30 17:27 ` Lars Ingebrigtsen
2020-09-30 22:37   ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-10-01  1:34     ` Lars Ingebrigtsen
2020-10-01  7:51       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-10-01 13:13         ` Eli Zaretskii
2020-10-01 13:40           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-10-01 15:55           ` Lars Ingebrigtsen
2020-10-01  2:40     ` Eli Zaretskii
2020-10-02 19:39 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-10-02 19:46   ` Eli Zaretskii
2020-10-02 19:58     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-10-03  7:27       ` Eli Zaretskii
2020-10-03 17:23         ` Lars Ingebrigtsen
2020-10-03 18:40           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-10-06 16:39             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-10-07  4:28               ` Lars Ingebrigtsen
2020-10-07 14:47                 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-10-09  4:20                   ` Lars Ingebrigtsen
2020-10-06 17:11         ` Stefan Monnier
2020-11-25 18:35 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-11-26 10:10   ` Lars Ingebrigtsen
2020-11-26 11:19     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-11-26 11:28       ` Lars Ingebrigtsen
2020-11-26 12:02         ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-12-04 22:28           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-12-06 13:13             ` Lars Ingebrigtsen
2020-11-26 14:17     ` Eli Zaretskii
2020-11-26 21:15       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2020-11-27  7:16         ` Eli Zaretskii
2021-02-13 20:06           ` Eli Zaretskii
2021-02-14 20:22             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-02-16 21:13             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-02-17 15:18               ` Eli Zaretskii
2021-02-18 21:22                 ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-02-19  8:08                   ` Eli Zaretskii
2021-02-18 20:32               ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-02-18 20:36                 ` Eli Zaretskii
2021-02-18 20:53                   ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-02-19  7:51                     ` Eli Zaretskii
2021-02-19 11:13                       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-02-19 12:23                         ` Eli Zaretskii
2021-02-19 22:03                           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-02-20  6:57                             ` Eli Zaretskii
2021-02-19 12:53                 ` Pip Cet
2021-02-19 13:37                   ` Eli Zaretskii
2021-02-19 14:16                     ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-02-19 14:23                       ` Pip Cet
2021-02-19 14:35                         ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-02-19 14:59                           ` Eli Zaretskii
2021-02-19 15:00                           ` Pip Cet
2021-02-19 15:11                             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-02-19 17:33                             ` Andy Moreton
2021-02-18 21:56             ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-02-19  8:14               ` Eli Zaretskii
2021-02-18 21:58   ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-02-19  8:16     ` Eli Zaretskii
2021-02-22 20:08       ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-02-22 20:51         ` Pip Cet
2021-02-23  3:23         ` Eli Zaretskii
2021-02-26 19:31           ` Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors

unofficial mirror of bug-gnu-emacs@gnu.org 

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/emacs-bugs/0 emacs-bugs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 emacs-bugs emacs-bugs/ https://yhetil.org/emacs-bugs \
		bug-gnu-emacs@gnu.org
	public-inbox-index emacs-bugs

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.emacs.bugs
	nntp://news.gmane.io/gmane.emacs.bugs


AGPL code for this site: git clone http://ou63pmih66umazou.onion/public-inbox.git