Should `builtin::` functions forbid (or warn) about replacement?

[Paul "LeoNerd" Evans]

(Reposting in its own thread in case people missed it while buried in
the last one).

On Wed, 9 Oct 2024 10:29:33 +0100

Sub calls are normally based on looking up the CV in the GV at
@a = qw(a b c);
which prints "xyz";
Can this proposed optimisation cope with the sub being swapped out? If
not, do we document anywhere that you can't always rely on it for
builtin:::indexed?

It's already the case that most of the simple `builtin::` functions are
replaced by direct opcodes at compiletime, so such a replacement in the
GV already doesn't work.

E.g.

$ perl -E '*builtin::reftype = sub { "BOO" }; say builtin::reftype []'
Built-in function 'builtin::reftype' is experimental at -e line 1.
Prototype mismatch: sub builtin::reftype ($) vs none at -e line 1.
ARRAY

This is because most of those simple function CVs have a callchecker
that replaces the OP_ENTERSUB with a direct opcode implementation of
the behaviour. This makes them perform much faster at runtime, avoiding
that entersub overhead.

We should probably call that out more specifically in documentation.

I think it's not entirely unreasonable that any of the functions in the
"builtin::" space don't allow being replaced in such a fashion. In fact
perhaps we could probably go further in setting the READONLY flag on all
these GVs, to prevent that kind of thing?

[Paul "LeoNerd" Evans]

On Thu, 10 Oct 2024 18:43:46 +0100

Post by Paul "LeoNerd" Evans
We should probably call that out more specifically in documentation.
I think it's not entirely unreasonable that any of the functions in
the "builtin::" space don't allow being replaced in such a fashion.
In fact perhaps we could probably go further in setting the READONLY
flag on all these GVs, to prevent that kind of thing?

In the event that we decide "yes we should", I've prepared a PR.
Currently set as a draft but I'll undraft it if we decide to proceed:

https://github.com/Perl/perl5/pull/22657

[Tony Cook]

Post by Paul "LeoNerd" Evans
(Reposting in its own thread in case people missed it while buried in
the last one).
On Wed, 9 Oct 2024 10:29:33 +0100

Sub calls are normally based on looking up the CV in the GV at
@a = qw(a b c);
which prints "xyz";
Can this proposed optimisation cope with the sub being swapped out? If
not, do we document anywhere that you can't always rely on it for
builtin:::indexed?

It's already the case that most of the simple `builtin::` functions are
replaced by direct opcodes at compiletime, so such a replacement in the
GV already doesn't work.
E.g.
$ perl -E '*builtin::reftype = sub { "BOO" }; say builtin::reftype []'
Built-in function 'builtin::reftype' is experimental at -e line 1.
Prototype mismatch: sub builtin::reftype ($) vs none at -e line 1.
ARRAY
This is because most of those simple function CVs have a callchecker
that replaces the OP_ENTERSUB with a direct opcode implementation of
the behaviour. This makes them perform much faster at runtime, avoiding
that entersub overhead.
We should probably call that out more specifically in documentation.
I think it's not entirely unreasonable that any of the functions in the
"builtin::" space don't allow being replaced in such a fashion. In fact
perhaps we could probably go further in setting the READONLY flag on all
these GVs, to prevent that kind of thing?

I think the compile-checks you're doing are reasonable, the checks for
op overloads (like replacing readdir()) are similarly done at
compile-time, replacing an OP_READDIR with an OP_ENTERSUB.

Tony

[Paul "LeoNerd" Evans]

On Fri, 11 Oct 2024 10:26:54 +1100

Post by Tony Cook

Post by Paul "LeoNerd" Evans
We should probably call that out more specifically in documentation.
I think it's not entirely unreasonable that any of the functions in
the "builtin::" space don't allow being replaced in such a fashion.
In fact perhaps we could probably go further in setting the
READONLY flag on all these GVs, to prevent that kind of thing?

I think the compile-checks you're doing are reasonable,

OK. In that case, maybe the code as implemented is fine, but more
attention should be brought to it in documentation? Even if just a
small note to say "Hey, by the way, don't do this and expect it to
still work ..."

Post by Tony Cook
the checks for
op overloads (like replacing readdir()) are similarly done at
compile-time, replacing an OP_READDIR with an OP_ENTERSUB.

(I think either I am misreading that or you wrote that backwards ;) )

[Paul "LeoNerd" Evans]

On Thu, 10 Oct 2024 18:43:46 +0100

Post by Paul "LeoNerd" Evans
We should probably call that out more specifically in documentation.
I think it's not entirely unreasonable that any of the functions in
the "builtin::" space don't allow being replaced in such a fashion.
In fact perhaps we could probably go further in setting the READONLY
flag on all these GVs, to prevent that kind of thing?

Various comments and suggestions point out that we already have other
things that would break in similar situations, so perhaps outright
forbidding it with SvREADONLY is going a bit far.

In that light then, I have created a new alternative PR, that simply
documents "Hey you probably shouldn't do this..."

https://github.com/Perl/perl5/pull/22668

If we think this is a better approach, then that can be merged and then
I'll continue with the foreach+indexed optimisations.