From @andk

Not to sound ungrateful, but... the ticket is in state resolved?
Shouldn't it be open since it has broken more than half of CPAN?

Thanks to Dave Rolsky there is now at least a report for
Variable​::Magic​: https://rt.cpan.org/Ticket/Display.html?id=101410

Thanks to Dagfinn Ilmari Mannsåker there is now a
TIMB/DBI-1.632_90.tar.gz

But Glib and BerkeleyDB have not been reported or patched upstream.

Thanks,
--
andreas

From @cpansprout

On Sat Jan 10 14​:14​:47 2015, andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

Not to sound ungrateful, but... the ticket is in state resolved?

The topic of #123522 is to make perl more efficient by reducing the number of instructions for GvSVn, DEFSV, etc. That has been accomplished, so it is resolved.

I don’t know whether you intended to create a new ticket with your message, but you did (#123580), so it can serve that purpose. I’ll change the subject.

Shouldn't it be open since it has broken more than half of CPAN?

That much?

Thanks to Dave Rolsky there is now at least a report for
Variable​::Magic​: https://rt.cpan.org/Ticket/Display.html?id=101410

Thanks to Dagfinn Ilmari Mannsåker there is now a
TIMB/DBI-1.632_90.tar.gz

But Glib and BerkeleyDB have not been reported or patched upstream.

Do you know offhand whether these are all broken the same way, namely, through lvalue use of DEFSV?

I think we need to change that back, at least #ifndef PERL_CORE, and I plan to do so, soon.

--

Father Chrysostomos

The RT System itself - Status changed from 'new' to 'open'

From @bulk88

Father Chrysostomos via RT wrote​:

On Sat Jan 10 14​:14​:47 2015, andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

Not to sound ungrateful, but... the ticket is in state resolved?

The topic of #123522 is to make perl more efficient by reducing the number of instructions for GvSVn, DEFSV, etc. That has been accomplished, so it is resolved.

I don’t know whether you intended to create a new ticket with your message, but you did (#123580), so it can serve that purpose. I’ll change the subject.

Shouldn't it be open since it has broken more than half of CPAN?

That much?

Thanks to Dave Rolsky there is now at least a report for
Variable​::Magic​: https://rt.cpan.org/Ticket/Display.html?id=101410

Thanks to Dagfinn Ilmari Mannsåker there is now a
TIMB/DBI-1.632_90.tar.gz

But Glib and BerkeleyDB have not been reported or patched upstream.

Do you know offhand whether these are all broken the same way, namely, through lvalue use of DEFSV?

I think we need to change that back, at least #ifndef PERL_CORE, and I plan to do so, soon.

What do you plan exactly to do? There is a provision for the low or high
bits of a GPe (since a GPe is an offset into a GP struct, where every
element is atleast 4 byte aligned, so the low 2 bits are free, also
since 10*8= 80 (size of struct GP on 64 bit CPUs), the I8/U8 sign bit is
unused in a GPe, the reason why i care about GPe fitting into a char is
for CPU instruction set reasons [1]) to indicate whether to return an
lvalue SV** or a SV *. I didn't put in the lvalue SV ** feature since I
was hoping to fix the "DEFSV [or GvSVn(gv)] = newSV(0);" leak problem or
deprecate it away.

if GvSVn hypothetically will become

#define GvSVn(gv) (GvGP(gv)->gp_sv ? \
  GvGP(gv)->gp_sv : \
  *Perl_gv_add_by_type_p(aTHX_ (gv), GPe_SVL))

There is a memory read in the Perl_gv_add_by_type_p branch that can not
be optimized away by the CC even if the macro is only used as rvalue.
lvalue Perl_gv_add_by_type_p is still better than the old
Perl_gv_add_by_type which requires 2 memory reads after every
Perl_gv_add_by_type branch, one in "GvGP(gv)" and the 2nd one in
"->gp_sv". An older implementation I did had Perl_gv_add_by_type_p
returning the GP * always. The caller would then do
"*(void**)(gp+offset_to_member)" on it. On x86 you will have 1 byte of
machine code by doing "*(void**)(svp)" vs
"*(void**)(gp+offset_to_member)" since there is no offset, you dont save
any machine bytes on ARM due to its RISC design, "reg = reg;", "reg =
*(reg);" and "reg = *(reg+constant_offset)" are the same size.

[1]
-Older ARM cpus in "legacy" 32 bit mode, all constants must be 0-255,
followed by a rotate/shift count of 0-31 (for a constant 0-255, the
shift count is 0)
-Newer ARM cpus in "legacy" 32 bit mode, all constants must be 0-65535
-Newer ARM cpus in [new] "thumb" 16 bit mode, all constants must be 0-255
-X86 all constants are 1, 4 or 8 bytes, you save instruction/machine
code space by using a smaller constant
-on ARM, in C, if your C constant can not fit within the constants of
ARM asm language, the compiler will spit out "reg =
*(U32*)(instruction_pointer+offset_to_static_const_global_data);" and
loading the constant involved a memory read

For all ARM C compilers and x86 Visual C 2005 and newer, our sv_flags
could use some rearranging for the most common ones (IDK how to figure
that out) to sit at the lowest U8 of sv_flags.

#define SVf_IOK 0x00000100 /* has valid public integer value */
#define SVf_NOK 0x00000200 /* has valid public numeric value */
#define SVf_POK 0x00000400 /* has valid public pointer value */
#define SVf_ROK 0x00000800 /* has a valid reference pointer */

#define SVp_IOK 0x00001000 /* has valid non-public integer value */
#define SVp_NOK 0x00002000 /* has valid non-public numeric value */
#define SVp_POK 0x00004000 /* has valid non-public pointer value */
#define SVp_SCREAM 0x00008000 /* method name is DOES */

This is a quickly thought up scheme

move SvTYPE from 0x000000FF to 0xFF000000
move SVp_*OK flags from 0x0000F000 to
0x00000F00
and change #define PRIVSHIFT 4 /* (SVp_?OK >> PRIVSHIFT) == SVf_?OK */

SVs_GMG and SVs_OBJECT and SVf_UTF8 are moved from 0x00F00000 to
0x000000F0 , IDK what the 4th bit in 0x000000F0 should be, maybe
SVf_READONLY? IDK

SVf_*OK are moved from 0x00000F00 to 0x0000000F

From @cpansprout

On Sat Jan 10 18​:09​:11 2015, bulk88 wrote​:

Father Chrysostomos via RT wrote​:

Do you know offhand whether these are all broken the same way,
namely, through lvalue use of DEFSV?

I think we need to change that back, at least #ifndef PERL_CORE, and
I plan to do so, soon.

What do you plan exactly to do?

I had not quite decided yet. I was actually hoping you would take care of this. :-)

The thing is, originally all GVs had an SV, so DEFSV was equivalent to GvSV(PL_defgv). So SvREFCNT_dec(DEFSV), DEFSV = ... was perfectly logical and straightforward. When GVs stopped automatically holding SVs, then DEFSV needed to use GvSVn to preserve the existing expectation that DEFSV was always non-null. Since DEFSV assignment has always worked till now (when handled correctly), it seems wrong to remove it.

There is a provision for the low or
high
bits of a GPe (since a GPe is an offset into a GP struct, where every
element is atleast 4 byte aligned, so the low 2 bits are free, also
since 10*8= 80 (size of struct GP on 64 bit CPUs), the I8/U8 sign bit
is
unused in a GPe, the reason why i care about GPe fitting into a char
is
for CPU instruction set reasons [1]) to indicate whether to return an
lvalue SV** or a SV *. I didn't put in the lvalue SV ** feature since
I
was hoping to fix the "DEFSV [or GvSVn(gv)] = newSV(0);" leak problem
or
deprecate it away.

By fix do you mean patching the CPAN modules that assign to it? If you are willing to do that, then we can leave DEFSV as a string rvalue.

--

Father Chrysostomos

From @bulk88

Father Chrysostomos via RT wrote​:

On Sat Jan 10 18​:09​:11 2015, bulk88 wrote​:

Father Chrysostomos via RT wrote​:

Do you know offhand whether these are all broken the same way,
namely, through lvalue use of DEFSV?

I think we need to change that back, at least #ifndef PERL_CORE, and
I plan to do so, soon.
What do you plan exactly to do?

I had not quite decided yet. I was actually hoping you would take care of this. :-)

The thing is, originally all GVs had an SV, so DEFSV was equivalent to GvSV(PL_defgv). So SvREFCNT_dec(DEFSV), DEFSV = ... was perfectly logical and straightforward. When GVs stopped automatically holding SVs, then DEFSV needed to use GvSVn to preserve the existing expectation that DEFSV was always non-null. Since DEFSV assignment has always worked till now (when handled correctly), it seems wrong to remove it.

There is a provision for the low or
high
bits of a GPe (since a GPe is an offset into a GP struct, where every
element is atleast 4 byte aligned, so the low 2 bits are free, also
since 10*8= 80 (size of struct GP on 64 bit CPUs), the I8/U8 sign bit
is
unused in a GPe, the reason why i care about GPe fitting into a char
is
for CPU instruction set reasons [1]) to indicate whether to return an
lvalue SV** or a SV *. I didn't put in the lvalue SV ** feature since
I
was hoping to fix the "DEFSV [or GvSVn(gv)] = newSV(0);" leak problem
or
deprecate it away.

By fix do you mean patching the CPAN modules that assign to it? If you are willing to do that, then we can leave DEFSV as a string rvalue.

http​://perl5.git.perl.org/perl.git/commitdiff/55b5114f4ff694ab871173b736aa2d48bb095684
needs to be straigtened out. I dont want to fix the DEFSV cases unless
DEFSV's API is sane.

# define DEFSV_set(sv) \
  (SvREFCNT_dec(GvSV(PL_defgv)), GvSV(PL_defgv) = SvREFCNT_inc(sv))

and

# define DEFSV_set(sv) (GvSV(PL_defgv) = (sv))

have 2 very different descriptions for CPAN and PERL_CORE. Here are the
2 faux descriptions for perlapi/perlintern

DEFSV_set(sv) CPAN description

Sets C<*_{SCALAR}> to be C<SV * sv>. This takes ownership of one
reference count.

DEFSV_set(sv) CORE description.

Sets C<*_{SCALAR}> to be C<SV * sv>. The reference count on the SV will
be incremented by one.

In other parts of the Perl api we have "newRV_inc" and "newRV_noinc" so
there isn't confusion.

If DEFSV_set is fixed, the CPAN version will be

# define DEFSV_set(sv) \
  (SvREFCNT_dec(GvSV(PL_defgv)), GvSV(PL_defgv) = (sv))

with no refcnt++, and DEFSV_set is not available for PERL_CORE (all uses
replaced with a new "DEFSV_setinc").

Personally
# define DEFSV_set(sv) STMT_START { \
  SV * defsvsetx = GvSV(PL_defgv)); \
  GvSV(PL_defgv) = (sv); \
  SvREFCNT_dec(defsvsetx); \
  } STMT_END

would be better since GvSV doesn't need to execute twice, but it breaks
darkpan compatibility (or not really since we never documented DEFSV_set
as being an rvalue expression returning the original sv, AKA nobody ever
  wrote "sv_setpv(DEFSV_set(newSV(0)), "a string");", cpan grep shows
that doesn't exist http​://grep.cpan.me/?q=-file%3Appport.h+DEFSV_set ).

So can't fix DEFSV problems on CPAN until P5P straightens out what
DEFSV_set is supposed to do, which is the obvious replacement for lvalue
assigns to DEFSV, and backcompat with older perls. Unless someone has a
different proposal for an API to replace lvalue DEFSV assignment.

Note, there is a question on if DEFSV_set should be "GvSV(PL_defgv) =
(sv);" style "copying" or, "sv_setsv(GvSVn(PL_defgv), (sv));" style copying.

I've had thoughts about making $_ and $@​ and $! 's sv heads immortal and
put them next to

PERLVAR(I, sv_undef, SV)
PERLVAR(I, sv_no, SV)
PERLVAR(I, sv_yes, SV)

with special casing in S_gv_magicalize or S_gv_is_in_main to connect the
GvSV in *_/*@​/*! to the immortals. This will cause pp_gvsv to be
replaced with PP(pp_const) and DEFSV to be &(my_perl->Idefsv) (no memory
reads, just add a const offset to my_perl) instead of
my_perl->Idefgv->sv_u.svu_gp->gp_sv (3 memory reads). A better
optimization would be to replace pp_const with pp_immortal, currently
-e" print !!1;", uses pp_const which is a

OP *
Perl_pp_const(PerlInterpreter * my_perl)
{
  SV **sp = (my_perl->Istack_sp);
  do {
  do {
  if (((((my_perl->Istack_max) - sp < (int) (1)) ? (char) 1 :
(char) 0))) {
  sp = Perl_stack_grow(my_perl, sp, sp, (int) (1));
  ((void) sizeof(sp));
  }
  } while (0);

  *++sp =
  ((((SVOP *) ((my_perl->Iop)))->op_sv ? ((SVOP *)
((my_perl->Iop)))->
  op_sv : ((my_perl->Icurpad)[((my_perl->Iop))->op_targ])));
  } while (0);
  return ((my_perl->Istack_sp) = sp, (my_perl->Iop)->op_next);
}

instead pp_immortal takes a BASEOP (not SVOP), which is 4/8 bytes
smaller than SVOP. pp_immortal uses op_private in BASEOP as an index (we
are limited to 256 immortal SV heads) into an array of immortal SV heads
in the interp struct. Notice doing "*sp =
&(my_perl->Iimsv[PL_op->op_private])" has only 3 memory accesses and no
pad reads, write to "*sp" and read "PL_op" and read "op_private",
my_perl is a register pulled off C stack at pp function entry and sp is
a register. In unthreaded mode, the statement is "*sp =
PL_imsv[PL_op->op_private])", which has 3 memory accesses, write "*sp",
read "PL_op" and read "op_private". IDK about fesibility of using
op_targ instead of op_private.

From @andk

On Sat, 10 Jan 2015 14​:23​:48 -0800, "Father Chrysostomos via RT" <perlbug-followup@​perl.org> said​:

  > On Sat Jan 10 14​:14​:47 2015, andreas.koenig.7os6VVqR@​franz.ak.mind.de wrote​:

Not to sound ungrateful, but... the ticket is in state resolved?

  > The topic of #123522 is to make perl more efficient by reducing the
  > number of instructions for GvSVn, DEFSV, etc. That has been
  > accomplished, so it is resolved.

  > I don’t know whether you intended to create a new ticket with your
  > message, but you did (#123580), so it can serve that purpose. I’ll
  > change the subject.

I did not intend to open a new ticket, it just happened, but apparently
it was the right thing to do, thanks for the subject change!

Shouldn't it be open since it has broken more than half of CPAN?

  > That much?

Variable-Magic is depended on by thousands. I have no exact numbers but
my smoker has a sample of some 10000 distributions. Before this commit I
could smoke about 90% of those and since the commit I can smoke 20%.

Most prominent users of Variable-Magic are

  B-Hooks-EndOfScope->namespace-autoclean->Catalyst-Runtime
  ->Dist-Zilla
  ->namespace-clean ->Class-Load ->Moose

Thanks to Dave Rolsky there is now at least a report for
Variable​::Magic​: https://rt.cpan.org/Ticket/Display.html?id=101410

Thanks to Dagfinn Ilmari Mannsåker there is now a
TIMB/DBI-1.632_90.tar.gz

But Glib and BerkeleyDB have not been reported or patched upstream.

  > Do you know offhand whether these are all broken the same way,
  > namely, through lvalue use of DEFSV?

Yes, they are.

--
andreas

From @bulk88

Andreas Koenig wrote​:

Variable-Magic is depended on by thousands. I have no exact numbers but
my smoker has a sample of some 10000 distributions. Before this commit I
could smoke about 90% of those and since the commit I can smoke 20%.

Most prominent users of Variable-Magic are

B-Hooks-EndOfScope->namespace-autoclean->Catalyst-Runtime
->Dist-Zilla
->namespace-clean ->Class-Load ->Moose

Thanks to Dave Rolsky there is now at least a report for
Variable​::Magic​: https://rt.cpan.org/Ticket/Display.html?id=101410

Thanks to Dagfinn Ilmari Mannsåker there is now a
TIMB/DBI-1.632_90.tar.gz

But Glib and BerkeleyDB have not been reported or patched upstream.

Do you know offhand whether these are all broken the same way,
namely, through lvalue use of DEFSV?

Yes, they are.

Variable Magic is an ERRSV user, it has no affected uses of DEFSV. A
patch is up at
https://rt.cpan.org/Ticket/Display.html?id=101410&results=069ce8b814adfecc4eb0ae80198362fe
but 1 test fails, but it doesn't seem to be related to no more lvalue
ERRSV, because even if I make "ERRSV" token lvalue, the test still fails.

From perl@profvince.com

Variable Magic is an ERRSV user, it has no affected uses of DEFSV. A
patch is up at
https://rt.cpan.org/Ticket/Display.html?id=101410&results=069ce8b814adfecc4eb0ae80198362fe
but 1 test fails, but it doesn't seem to be related to no more lvalue
ERRSV, because even if I make "ERRSV" token lvalue, the test still fails.

I don't understand this change, and I don't understand your
explainations, so I don't feel confident with applying this change.

Is 819b139 such an improvement that we need to patch half of CPAN with
cryptic borderline-non-API changes that are likely to break with the
next core microoptimization?

Vincent

From @iabyn

On Sun, Jan 11, 2015 at 12​:57​:07PM +0100, Vincent Pit (VPIT) wrote​:

I don't understand this change, and I don't understand your explainations,
so I don't feel confident with applying this change.

Is 819b139 such an improvement that we need to patch half of CPAN with
cryptic borderline-non-API changes that are likely to break with the next
core microoptimization?

My understanding of the issue is that the original refactorisation /
optimisation patch caused, as a side-effect, for GvSVn(), GvAVn()
etc to become rvalue-only, whereas before they were assignable to.

Since ERRSV and DEFSV are defined (outside of core) as​:

  #define ERRSV GvSVn(PL_errgv)
  #define DEFSV GvSVn(PL_defgv)

this has the knock-on effect of making those macros rvalue-only too.

There are some issues with assigning to the Gv*Vn() macros; in particular,
when the macro creates a new SV in the relevant slot (the 'n' in GV*Vn
means populate the slot if it doesn't already exist), the new SV will be
immediately leaked. There are some further issues with the way DEFSV and
DEFSV_set are defined in and outside core, also leading to refcount
issues. These need to be addressed at some point,

HOWEVER, since we're getting close to contentious code-freeze time, I
propose that​:

for now, we modify the definitions of GvSVn(), GvAVn() etc so that
they become lvalue again, but leave the rest of the 819b139 refactoring in
place. This should fix the issues with CPAN code.

After 5.22 is released, we then revisit the issue, decide how we want to
alter Gv*Vn(), DEFSV etc, and what deprecation cycles if any we need.

I also wonder whether whether we should re-instate the gv_add_by_type()
function as back-compat wrapper (it was deleted and replaced with
gv_add_by_type_p()). It was listed is 'Ap', which means it is part of the
API, but without documentation. Some may regard this as meaning its not
actually part of the API, but I think the mere presence of 'A' means we've
made a promise of sorts. On the other hand, grep.cpan.me shows no usage
of it.

--
No matter how many dust sheets you use, you will get paint on the carpet.

From @cpansprout

On Tue Jan 13 03​:46​:14 2015, davem wrote​:

HOWEVER, since we're getting close to contentious code-freeze time, I
propose that​:

for now, we modify the definitions of GvSVn(), GvAVn() etc so that
they become lvalue again, but leave the rest of the 819b139 refactoring in
place. This should fix the issues with CPAN code.

After 5.22 is released, we then revisit the issue, decide how we want to
alter Gv*Vn(), DEFSV etc, and what deprecation cycles if any we need.

I agree. I was going to say the same myself, but Real Life got in the way of the message I was in the middle of drafting.

I also wonder whether whether we should re-instate the gv_add_by_type()
function as back-compat wrapper (it was deleted and replaced with
gv_add_by_type_p()). It was listed is 'Ap', which means it is part of the
API, but without documentation. Some may regard this as meaning its not
actually part of the API, but I think the mere presence of 'A' means we've
made a promise of sorts. On the other hand, grep.cpan.me shows no usage
of it.

There have been many cases of functions added to the API accidentally because someone copied A from another entry and nobody noticed soon enough. As long as there are no CPAN uses, I think we can retract that accidental promise.

--

Father Chrysostomos

From @wolfsage

On Tue, Jan 13, 2015 at 6​:45 AM, Dave Mitchell <davem@​iabyn.com> wrote​:

HOWEVER, since we're getting close to contentious code-freeze time, I
propose that​:

for now, we modify the definitions of GvSVn(), GvAVn() etc so that
they become lvalue again, but leave the rest of the 819b139 refactoring in
place. This should fix the issues with CPAN code.

This sounds like a good plan to me. Does anyone of this on their plate?

Thanks,

-- Matthew Horsfall (alh)

From @iabyn

On Wed, Jan 14, 2015 at 11​:06​:35AM -0500, Matthew Horsfall (alh) wrote​:

On Tue, Jan 13, 2015 at 6​:45 AM, Dave Mitchell <davem@​iabyn.com> wrote​:

HOWEVER, since we're getting close to contentious code-freeze time, I
propose that​:

for now, we modify the definitions of GvSVn(), GvAVn() etc so that
they become lvalue again, but leave the rest of the 819b139 refactoring in
place. This should fix the issues with CPAN code.

This sounds like a good plan to me. Does anyone of this on their plate?

I'll do it in the next day or two unless there's any further
discussion/changes.

--
No man treats a motor car as foolishly as he treats another human being.
When the car will not go, he does not attribute its annoying behaviour to
sin, he does not say, You are a wicked motorcar, and I shall not give you
any more petrol until you go. He attempts to find out what is wrong and
set it right.
  -- Bertrand Russell,
  Has Religion Made Useful Contributions to Civilization?

From @bulk88

Dave Mitchell wrote​:

for now, we modify the definitions of GvSVn(), GvAVn() etc so that
they become lvalue again, but leave the rest of the 819b139 refactoring in
place. This should fix the issues with CPAN code.

I'll make a patch implementing lval to Perl_gv_add_by_type_p, since I
had a suspicion this will happen.

So what is the list of things that need to become lval under #ifndef
PERL_CORE (AKA cpan right?)?

ERRSV yes, variable​::magic

DEFSV probably yes http​://grep.cpan.me/?q=-file%3Appport.h+DEFSV DBI
claims to have fixed it
perl5-dbi/dbi@7d32b92
there are some more modules on that list

GvSVn http​://grep.cpan.me/?q=-file%3Appport.h+GvSVn 1 use of lvalue at
http​://grep.cpan.me/?q=-file%3Appport.h%20GvSVn+dist=B-C

GvAVn no uses as lval http​://grep.cpan.me/?q=-file%3Appport.h+GvAVn

GvIOn no uses as lval http​://grep.cpan.me/?q=-file%3Appport.h+GvIOn

GvHVn no uses as lval http​://grep.cpan.me/?q=-file%3Appport.h+GvHVn

After 5.22 is released, we then revisit the issue, decide how we want to
alter Gv*Vn(), DEFSV etc, and what deprecation cycles if any we need.

ok

I also wonder whether whether we should re-instate the gv_add_by_type()
function as back-compat wrapper (it was deleted and replaced with
gv_add_by_type_p()). It was listed is 'Ap', which means it is part of the
API, but without documentation. Some may regard this as meaning its not
actually part of the API, but I think the mere presence of 'A' means we've
made a promise of sorts. On the other hand, grep.cpan.me shows no usage
of it.

"public API" is not a legal contract, the designation was haphazardly
added when embed.fnc was invented, if there is no cpan grep usage, it
means it was a bad API and nobody used it, I changed the symbol name of
gv_add_by_type just in case of darkpan code. If we get a bug report we
can add it to mathoms.c. I've been building my perls with -DNO_MATHOMS
for some months now without any problems.

mathoms gv_add_by_type is a variation of "#define gv_SVadd(gv)
(Perl_gv_add_by_type_p(aTHX_ (gv), GPe_SV), gv)"​: to implement if it
needs to be implemented

From @ilmari

bulk88 <bulk88@​hotmail.com> writes​:

Dave Mitchell wrote​:

for now, we modify the definitions of GvSVn(), GvAVn() etc so that
they become lvalue again, but leave the rest of the 819b139 refactoring in
place. This should fix the issues with CPAN code.

I'll make a patch implementing lval to Perl_gv_add_by_type_p, since I
had a suspicion this will happen.

That won't help as long as Gv[AHS]Vn remain ternaries, since using a
ternary operator as an lvalue is undefined behaviour¹ in C, and
explicitly forbidden by gcc².

[1]​: «If an attempt is made to modify the result of a conditional
  operator or to access it after the next sequence point, the
  behavior is undefined» - ISO.IEC 9899​:1999 (E) 6.5.15.4

[2]​: $ cat lvalue.c
  #include <stdio.h>
  int main (int argc, char *argv[]) {
  int a = 0, b = 0;
  (argc > 1 ? a : b) = 1;
  printf("a=%d, b=%d\n", a, b);
  }
  $ gcc -o lvalue lvalue.c
  lvalue.c​: In function ‘main’​:
  lvalue.c​:4​:24​: error​: lvalue required as left operand of assignment

--
"A disappointingly low fraction of the human race is,
at any given time, on fire." - Stig Sandbeck Mathisen

From @bulk88

bulk88 wrote​:

Dave Mitchell wrote​:

for now, we modify the definitions of GvSVn(), GvAVn() etc so that
they become lvalue again, but leave the rest of the 819b139
refactoring in
place. This should fix the issues with CPAN code.

I'll make a patch implementing lval to Perl_gv_add_by_type_p, since I
had a suspicion this will happen.

Patch attached. Tested on an unmodified Variable​::Magic

t\18-opinfo.t ............. 1/125
# Failed test 'get magic with op_info == 2 doesn't croak'
# at t\18-opinfo.t line 86.
# got​: 'Can't coerce GLOB to integer in transliteration (tr///)
at (eva
l 138) line 1.
# '
# expected​: ''
# { my $c = ""; cast $c, $wiz; $c =~ y/x/y/ }
# Looks like you failed 1 test of 125.
t\18-opinfo.t ............. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/125 subtests

I did some test builds of 18-opinfo.t failing, I narrowed it to between

http​://perl5.git.perl.org/perl.git/070d3cb6f06f7c7f0a932b57616cd28061cb96c0
(" perly.y​: Don’t call op_lvalue on refgen kid") which is 1 commit
before "refactor gv_add_by_type" (opinfo.t fails on " perly.y​: Don’t
call op_lvalue on refgen kid") and
http​://perl5.git.perl.org/perl.git/846dac6786c1ada87b95d0268c0a9772a4bd04fc
  ("add new release to perlhist" tag v5.21.7) (opinfo.t passes on "add
new release to perlhist"). So 18-opinfo.t failing is something else p5p
did and I can't comment on that test fail any further.

From @bulk88

From b227cc7c33967fd25d1ac7054659434669f08779 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Thu, 15 Jan 2015 20:14:27 -0500
Subject: [PATCH] add lvalue feature to Perl_gv_add_by_type_p

Part of [perl #123580] and [perl #123522] . This is used as minially as
possible for efficiency reasons, so only for CPAN, and only where breakage
occured from [perl #123522]. Perl_gv_add_by_type_p returns a SV ** instead
of a GP *, since on x86, 1 byte is saved in
"register_A = *(void**)register_B;" instruction vs
"register_A = *(void**)(register_B+0x10);" instruction.
---
 embed.fnc |    2 +-
 gv.c      |   23 ++++++++++++++---------
 gv.h      |   17 ++++++++++++++++-
 perl.h    |    8 ++++++--
 proto.h   |    2 +-
 5 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/embed.fnc b/embed.fnc
index 01a96d3..1444fd0 100644
--- a/embed.fnc
+++ b/embed.fnc
@@ -482,7 +482,7 @@ p	|char*	|getenv_len	|NN const char *env_elem|NN unsigned long *len
 pox	|void	|get_db_sub	|NULLOK SV **svp|NN CV *cv
 Ap	|void	|gp_free	|NULLOK GV* gv
 Ap	|GP*	|gp_ref		|NULLOK GP* gp
-Xp	|SV*	|gv_add_by_type_p|NN GV *gv|gv_add_type type
+Xp	|void*	|gv_add_by_type_p|NN GV *gv|gv_add_type type
 Apmb	|GV*	|gv_AVadd	|NULLOK GV *gv
 Apmb	|GV*	|gv_HVadd	|NULLOK GV *gv
 Apmb	|GV*	|gv_IOadd	|NULLOK GV* gv
diff --git a/gv.c b/gv.c
index 219502b..c4274e3 100644
--- a/gv.c
+++ b/gv.c
@@ -41,34 +41,39 @@ Perl stores its global variables.
 static const char S_autoload[] = "AUTOLOAD";
 #define S_autolen (sizeof("AUTOLOAD")-1)
 
-SV *
+/* returns a SV * (arg type is GPe_*V) or SV ** (arg type is GPe_*VL) */
+
+void *
 Perl_gv_add_by_type_p(pTHX_ GV *gv, gv_add_type type)
 {
     SV **where;
     SV * sv;
+    /* low bit, which cant be a GP struct offset due to alignment, is the lval
+       flag */
+    gv_add_type clean_type = type &~GPe_LVAL;
     PERL_ARGS_ASSERT_GV_ADD_BY_TYPE_P;
 
     if ( SvTYPE((const SV *)gv) != SVt_PVGV
          && SvTYPE((const SV *)gv) != SVt_PVLV
     ) {
 	const char *what;
-	if (type == GPe_IO) {
+	if (clean_type == GPe_IO) {
 	    /*
 	     * if it walks like a dirhandle, then let's assume that
 	     * this is a dirhandle.
 	     */
 	    what = OP_IS_DIRHOP(PL_op->op_type) ?
 		"dirhandle" : "filehandle";
-	} else if (type == GPe_HV) {
+	} else if (clean_type == GPe_HV) {
 	    what = "hash";
 	} else {
-	    what = type == GPe_AV ? "array" : "scalar";
+	    what = clean_type == GPe_AV ? "array" : "scalar";
 	}
 	/* diag_listed_as: Bad symbol for filehandle */
 	Perl_croak(aTHX_ "Bad symbol for %s", what);
     }
 
-    where = (SV **)((Size_t)GvGP(gv)+ (Size_t)type);
+    where = (SV **)((Size_t)GvGP(gv)+ (Size_t)clean_type);
 
     sv = *where;
     if (!sv) {
@@ -83,14 +88,14 @@ Perl_gv_add_by_type_p(pTHX_ GV *gv, gv_add_type type)
 #else
 #  error unknown pointer size
 #endif
-	svtype svtypevar = (svtype)addtype_to_svtype[PTRPTR2IDX(type)];
+	svtype svtypevar = (svtype)addtype_to_svtype[PTRPTR2IDX(clean_type)];
 
-	assert(PTRPTR2IDX(type) < sizeof(addtype_to_svtype));
+	assert(PTRPTR2IDX(clean_type) < sizeof(addtype_to_svtype));
 	sv = *where = newSV_type(svtypevar);
-	if (type == GPe_AV && memEQs(GvNAME(gv), GvNAMELEN(gv), "ISA"))
+	if (clean_type == GPe_AV && memEQs(GvNAME(gv), GvNAMELEN(gv), "ISA"))
 	    sv_magic(sv, (SV *)gv, PERL_MAGIC_isa, NULL, 0);
     }
-    return sv;
+    return type & GPe_LVAL ? where : sv;
 }
 
 GV *
diff --git a/gv.h b/gv.h
index 7792017..79d6e1c 100644
--- a/gv.h
+++ b/gv.h
@@ -103,9 +103,19 @@ Return the CV from the GV.
 #ifdef PERL_DONT_CREATE_GVSV
 #define GvSVn(gv)	(GvGP(gv)->gp_sv ? \
 			 GvGP(gv)->gp_sv : \
-			 Perl_gv_add_by_type_p(aTHX_ (gv), GPe_SV))
+			 (SV*)Perl_gv_add_by_type_p(aTHX_ (gv), GPe_SV))
+/* not public API, this is an assignable lval version of GvSVn,
+   it may be removed one day */
+#ifndef PERL_CORE
+#  define GvSVnL(gv)	(*(GvGP(gv)->gp_sv ? \
+			 &GvGP(gv)->gp_sv : \
+			 (SV**)Perl_gv_add_by_type_p(aTHX_ (gv), GPe_SVL)))
+#endif
 #else
 #define GvSVn(gv)	GvSV(gv)
+#ifndef PERL_CORE
+#  define GvSVnL(gv)	GvSV(gv)
+#endif
 #endif
 
 #define GvREFCNT(gv)	(GvGP(gv)->gp_refcnt)
@@ -288,10 +298,15 @@ Return the CV from the GV.
 
 /* used by Perl_gv_add_by_type_p for option checking, low bits are free here*/
 typedef enum {
+    GPe_LVAL = 1, /* low bit can't be an offset */
     GPe_SV = STRUCT_OFFSET(GP, gp_sv),
     GPe_IO = STRUCT_OFFSET(GP, gp_io),
     GPe_HV = STRUCT_OFFSET(GP, gp_hv),
     GPe_AV = STRUCT_OFFSET(GP, gp_av),
+    GPe_SVL = STRUCT_OFFSET(GP, gp_sv) | GPe_LVAL,
+    GPe_IOL = STRUCT_OFFSET(GP, gp_io) | GPe_LVAL,
+    GPe_HVL = STRUCT_OFFSET(GP, gp_hv) | GPe_LVAL,
+    GPe_AVL = STRUCT_OFFSET(GP, gp_av) | GPe_LVAL,
 } gv_add_type;
 
 #define gv_AVadd(gv) (Perl_gv_add_by_type_p(aTHX_ (gv), GPe_AV), gv)
diff --git a/perl.h b/perl.h
index 12e7e12..046bbdb 100644
--- a/perl.h
+++ b/perl.h
@@ -1267,7 +1267,11 @@ EXTERN_C char *crypt(const char *, const char *);
 #   define RESTORE_ERRNO  (errno = saved_errno)
 #endif
 
-#define ERRSV GvSVn(PL_errgv)
+#ifdef PERL_CORE
+#  define ERRSV GvSVn(PL_errgv)
+#else
+#  define ERRSV GvSVnL(PL_errgv)
+#endif
 
 /* contains inlined gv_add_by_type */
 #define CLEAR_ERRSV() STMT_START {					\
@@ -1301,7 +1305,7 @@ EXTERN_C char *crypt(const char *, const char *);
 	GvSV(PL_defgv) = NULL           \
     )
 #else
-# define DEFSV GvSVn(PL_defgv)
+# define DEFSV GvSVnL(PL_defgv)
 # define DEFSV_set(sv) (GvSV(PL_defgv) = (sv))
 # define SAVE_DEFSV SAVESPTR(GvSV(PL_defgv))
 #endif
diff --git a/proto.h b/proto.h
index 986d835..e09c4d3 100644
--- a/proto.h
+++ b/proto.h
@@ -1391,7 +1391,7 @@ PERL_CALLCONV UV	Perl_grok_oct(pTHX_ const char* start, STRLEN* len_p, I32* flag
 /* PERL_CALLCONV GV*	Perl_gv_AVadd(pTHX_ GV *gv); */
 /* PERL_CALLCONV GV*	Perl_gv_HVadd(pTHX_ GV *gv); */
 /* PERL_CALLCONV GV*	Perl_gv_IOadd(pTHX_ GV* gv); */
-PERL_CALLCONV SV*	Perl_gv_add_by_type_p(pTHX_ GV *gv, gv_add_type type)
+PERL_CALLCONV void*	Perl_gv_add_by_type_p(pTHX_ GV *gv, gv_add_type type)
 			__attribute__nonnull__(pTHX_1);
 #define PERL_ARGS_ASSERT_GV_ADD_BY_TYPE_P	\
 	assert(gv)
-- 
1.7.9.msysgit.0

From @iabyn

On Thu, Jan 15, 2015 at 10​:24​:45PM -0500, bulk88 wrote​:

bulk88 wrote​:

Dave Mitchell wrote​:

for now, we modify the definitions of GvSVn(), GvAVn() etc so that
they become lvalue again, but leave the rest of the 819b139 refactoring
in
place. This should fix the issues with CPAN code.

I'll make a patch implementing lval to Perl_gv_add_by_type_p, since I had
a suspicion this will happen.

Patch attached. Tested on an unmodified Variable​::Magic

But this patch doesn't doesn't revert GvSVn(), GvAVn() etc to being
lvalues as I suggested.

--
The optimist believes that he lives in the best of all possible worlds.
As does the pessimist.

From @wolfsage

On the off chance that this doens't get resolved in time with
sufficient testing before release on Tuesday, I've created a branch
that reverts
819b139 -
smoke-me/alh/revert-for-123580 - and I plan to apply this before
release. I think that's the only commit that needed reverting, and on
that branch Variable​::Magic builds for me (where it still doesn't in
blead.)

If we have to revert it, I think it can be brought back in for 5.23.1,
but otherwise would be a contentious change? Not sure.

Cheers,

-- Matthew Horsfall (alh)

From @bulk88

Dave Mitchell wrote​:

On Thu, Jan 15, 2015 at 10​:24​:45PM -0500, bulk88 wrote​:

bulk88 wrote​:

Dave Mitchell wrote​:

for now, we modify the definitions of GvSVn(), GvAVn() etc so that
they become lvalue again, but leave the rest of the 819b139 refactoring
in
place. This should fix the issues with CPAN code.
I'll make a patch implementing lval to Perl_gv_add_by_type_p, since I had
a suspicion this will happen.
Patch attached. Tested on an unmodified Variable​::Magic

But this patch doesn't doesn't revert GvSVn(), GvAVn() etc to being
lvalues as I suggested.

All of the breakage reports so far are about ERRSV and DEFSV not being
lvals. Lets not penalize all the rval Gv**n users on CPAN for the
non-existent lval Gv**n users. There is also the issue of that the
longer a bad API is available, the more likely some random person will
use the bad API (like SvPV_set instead of sv_setpvn) making it a bigger
burden to deprecate/remove in the future. If someone comes with a report
of Gv**n as Gv**n not being lval in existing CPAN code, then we can turn
Gv**n individually (GvSVn or GvAVn or GvHVn or GvIOn, not all 4 in 1
shot) back to lval before 5.22.0 ships. I put in the provisions for
GvAVnL and GvHVnL to exist in the gv_add_type enum table. I didn't
create GvAVnL and GvHVnL since someone might get the idea to use them
between today and may 2016 and then demand that we keep them as part of
the public API. If someone has a better naming convention for the macros
other than "L" postfix please speak up. I couldn't use "LV" since that
would be confusing with the PVLV SV type. And lower case "l" means it is
not the first letter of an abbreivated word (IDK what "Vnl" is an
abbreviation for). Perl macros already have the "_const" postfix for the
+0 rval only versions of macros (see SvRV_const and SvPVX_const). But
there is no opposite term of "_const".

2 cpan greps for lvalue of Gv**n returns exactly 1 module

http​://grep.cpan.me/?q=%26\%28*Gv\w\wn\s*\%28 nothing

http​://grep.cpan.me/?q=Gv\w\wn\s*\%28.%2B\%29\s*%3D 1 module

From @jkeenan

On Sat Jan 17 09​:19​:38 2015, alh wrote​:

On the off chance that this doens't get resolved in time with
sufficient testing before release on Tuesday, I've created a branch
that reverts
819b139 -
smoke-me/alh/revert-for-123580 - and I plan to apply this before
release. I think that's the only commit that needed reverting, and on
that branch Variable​::Magic builds for me (where it still doesn't in
blead.)

If we have to revert it, I think it can be brought back in for 5.23.1,
but otherwise would be a contentious change? Not sure.

Building perl from the reversion branch also enabled me to build Variable​::Magic, which I could not do in blead. Comparisons attached.

--
James E Keenan (jkeenan@​cpan.org)

From @jkeenan

[revert-for-123580] 54 $ ./bin/cpan
Terminal does not support AddHistory.
Redundant argument in sprintf at /home/jkeenan/testing/revert-for-123580/lib/perl5/5.21.8/CPAN.pm line 314.

cpan shell -- CPAN exploration and modules installation (v2.05)
Enter 'h' for help.

cpan[1]> test Variable​::Magic
Reading '/home/jkeenan/.cpan/Metadata'
  Database was generated on Mon, 10 Nov 2014 23​:17​:02 GMT
\C is deprecated in regex; marked by <-- HERE in m/(\C <-- HERE )/ at /home/jkeenan/perl5/lib/perl5/URI/Escape.pm line 205, <FIN> line 1.
Fetching with LWP​:
http​://www.cpan.org/authors/01mailrc.txt.gz
Reading '/home/jkeenan/.cpan/sources/authors/01mailrc.txt.gz'
............................................................................DONE
Fetching with LWP​:
http​://www.cpan.org/modules/02packages.details.txt.gz
Reading '/home/jkeenan/.cpan/sources/modules/02packages.details.txt.gz'
  Database was generated on Sat, 17 Jan 2015 19​:29​:02 GMT
............................................................................DONE
Fetching with LWP​:
http​://www.cpan.org/modules/03modlist.data.gz
Reading '/home/jkeenan/.cpan/sources/modules/03modlist.data.gz'
DONE
Writing /home/jkeenan/.cpan/Metadata
Running test for module 'Variable​::Magic'
Fetching with LWP​:
http​://www.cpan.org/authors/id/V/VP/VPIT/Variable-Magic-0.55.tar.gz
Fetching with LWP​:
http​://www.cpan.org/authors/id/V/VP/VPIT/CHECKSUMS
Checksum for /home/jkeenan/.cpan/sources/authors/id/V/VP/VPIT/Variable-Magic-0.55.tar.gz ok
Scanning cache /home/jkeenan/.cpan/build for sizes
............................................................................DONE
'YAML' not installed, will not store persistent state
Configuring V/VP/VPIT/Variable-Magic-0.55.tar.gz with Makefile.PL
Checking if this is ActiveState Perl 5.8.8 build 822 or higher... no
Checking if this is gcc 3.4 on Windows trying to link against an import library... no
Checking if your kit is complete...
Looks good
Generating a Unix-style Makefile
Writing Makefile for Variable​::Magic
Writing MYMETA.yml and MYMETA.json
  VPIT/Variable-Magic-0.55.tar.gz
  /home/jkeenan/testing/revert-for-123580/bin/perl Makefile.PL INSTALLDIRS=site -- OK
Running make for V/VP/VPIT/Variable-Magic-0.55.tar.gz
cp lib/Variable/Magic.pm blib/lib/Variable/Magic.pm
Running Mkbootstrap for Variable​::Magic ()
chmod 644 "Magic.bs"
"/home/jkeenan/testing/revert-for-123580/bin/perl" "/home/jkeenan/testing/revert-for-123580/lib/perl5/5.21.8/ExtUtils/xsubpp" -typemap "/home/jkeenan/testing/revert-for-123580/lib/perl5/5.21.8/ExtUtils/typemap" Magic.xs > Magic.xsc && mv Magic.xsc Magic.c
cc -c -fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -O2 -DVERSION=\"0.55\" -DXS_VERSION=\"0.55\" -fPIC "-I/home/jkeenan/testing/revert-for-123580/lib/perl5/5.21.8/x86_64-linux/CORE" Magic.c
rm -f blib/arch/auto/Variable/Magic/Magic.so
cc -shared -O2 -L/usr/local/lib -fstack-protector Magic.o -o blib/arch/auto/Variable/Magic/Magic.so \
  \
 
chmod 755 blib/arch/auto/Variable/Magic/Magic.so
"/home/jkeenan/testing/revert-for-123580/bin/perl" -MExtUtils​::Command​::MM -e 'cp_nonempty' -- Magic.bs blib/arch/auto/Variable/Magic/Magic.bs 644
Manifying 1 pod document
  VPIT/Variable-Magic-0.55.tar.gz
  /usr/bin/make -- OK
Running make test
Running Mkbootstrap for Variable​::Magic ()
chmod 644 "Magic.bs"
PERL_DL_NONLAZY=1 "/home/jkeenan/testing/revert-for-123580/bin/perl" "-MExtUtils​::Command​::MM" "-MTest​::Harness" "-e" "undef *Test​::Harness​::Switches; test_harness(0, 'blib/lib', 'blib/arch')" t/*.t
t/00-load.t ....... 1/1 # Testing Variable​::Magic 0.55, Perl 5.021008 (no patchlevel), /home/jkeenan/testing/revert-for-123580/bin/perl
t/00-load.t ....... ok
t/01-import.t ..... ok
t/02-constants.t .. ok
t/10-simple.t ..... ok
t/11-multiple.t ... ok
t/13-data.t ....... ok
t/14-callbacks.t .. ok
t/15-self.t ....... ok
t/16-huf.t ........ # Using Hash​::Util​::FieldHash 1.15
t/16-huf.t ........ ok
t/17-ctl.t ........ 1/96 # Using Capture​::Tiny 0.24
t/17-ctl.t ........ ok
t/18-opinfo.t ..... ok
t/20-get.t ........ ok
t/21-set.t ........ ok
t/22-len.t ........ ok
t/23-clear.t ...... ok
t/24-free.t ....... ok
t/25-copy.t ....... 1/48 # Using Tie​::Array 1.06
# Using Tie​::Hash 1.05
t/25-copy.t ....... ok
t/27-local.t ...... ok
t/28-uvar.t ....... 1/75 # Using Tie​::Hash 1.05
t/28-uvar.t ....... ok
t/30-scalar.t ..... 1/76 # Using Tie​::Array 1.06
t/30-scalar.t ..... ok
t/31-array.t ...... ok
t/32-hash.t ....... ok
t/33-code.t ....... ok
t/34-glob.t ....... # Using Symbol 1.07
t/34-glob.t ....... ok
t/35-stash.t ...... ok
t/40-threads.t .... skipped​: This Variable​::Magic isn't thread safe
t/41-clone.t ...... skipped​: This Variable​::Magic isn't thread safe
t/80-leaks.t ...... ok
All tests successful.
Files=28, Tests=1252, 2 wallclock secs ( 0.18 usr 0.02 sys + 0.77 cusr 0.09 csys = 1.06 CPU)
Result​: PASS
  VPIT/Variable-Magic-0.55.tar.gz
  /usr/bin/make test -- OK

cpan[2]> bye
Terminal does not support GetHistory.
Lockfile removed.
[revert-for-123580] 55 $ cd ../blead
[blead] 56 $ pwd
/home/jkeenan/testing/blead
[blead] 57 $ ./bin/cpan
Terminal does not support AddHistory.
Redundant argument in sprintf at /home/jkeenan/testing/blead/lib/perl5/5.21.8/CPAN.pm line 314.

cpan shell -- CPAN exploration and modules installation (v2.05)
Enter 'h' for help.

cpan[1]> test Variable​::Magic
Reading '/home/jkeenan/.cpan/Metadata'
  Database was generated on Sat, 17 Jan 2015 19​:29​:02 GMT
Running test for module 'Variable​::Magic'
Checksum for /home/jkeenan/.cpan/sources/authors/id/V/VP/VPIT/Variable-Magic-0.55.tar.gz ok
Scanning cache /home/jkeenan/.cpan/build for sizes
............................................................................DONE
'YAML' not installed, will not store persistent state
Configuring V/VP/VPIT/Variable-Magic-0.55.tar.gz with Makefile.PL
Checking if this is ActiveState Perl 5.8.8 build 822 or higher... no
Checking if this is gcc 3.4 on Windows trying to link against an import library... no
Checking if your kit is complete...
Looks good
Generating a Unix-style Makefile
Writing Makefile for Variable​::Magic
Writing MYMETA.yml and MYMETA.json
  VPIT/Variable-Magic-0.55.tar.gz
  /home/jkeenan/testing/blead/bin/perl Makefile.PL INSTALLDIRS=site -- OK
Running make for V/VP/VPIT/Variable-Magic-0.55.tar.gz
cp lib/Variable/Magic.pm blib/lib/Variable/Magic.pm
Running Mkbootstrap for Variable​::Magic ()
chmod 644 "Magic.bs"
"/home/jkeenan/testing/blead/bin/perl" "/home/jkeenan/testing/blead/lib/perl5/5.21.8/ExtUtils/xsubpp" -typemap "/home/jkeenan/testing/blead/lib/perl5/5.21.8/ExtUtils/typemap" Magic.xs > Magic.xsc && mv Magic.xsc Magic.c
cc -c -fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -O2 -DVERSION=\"0.55\" -DXS_VERSION=\"0.55\" -fPIC "-I/home/jkeenan/testing/blead/lib/perl5/5.21.8/x86_64-linux/CORE" Magic.c
Magic.xs​: In function ‘vmg_call_sv’​:
Magic.xs​:289​:11​: error​: lvalue required as left operand of assignment
  ERRSV = newSV(0);
  ^
Magic.xs​:305​:10​: error​: lvalue required as left operand of assignment
  ERRSV = old_err;
  ^
Magic.xs​:330​:10​: error​: lvalue required as left operand of assignment
  ERRSV = old_err;
  ^
Magic.xs​: In function ‘vmg_propagate_errsv_free’​:
Magic.xs​:1393​:17​: error​: lvalue required as left operand of assignment
  ERRSV = mg->mg_obj;
  ^
make​: *** [Magic.o] Error 1
  VPIT/Variable-Magic-0.55.tar.gz
  /usr/bin/make -- NOT OK
Failed during this command​:
VPIT/Variable-Magic-0.55.tar.gz : make NO

cpan[2]> bye
Terminal does not support GetHistory.
Lockfile removed.

From @bulk88

On Sat Jan 17 11​:43​:42 2015, jkeenan wrote​:

Building perl from the reversion branch also enabled me to build
Variable​::Magic, which I could not do in blead. Comparisons attached.

Obviously it won't build with blead since blead is still "broken". Try blead with "[PATCH] add lvalue feature to Perl_gv_add_by_type_p" patch in this ticket.

--
bulk88 ~ bulk88 at hotmail.com

From @iabyn

On Sat, Jan 17, 2015 at 01​:12​:00PM -0500, bulk88 wrote​:

Dave Mitchell wrote​:

On Thu, Jan 15, 2015 at 10​:24​:45PM -0500, bulk88 wrote​:

bulk88 wrote​:

Dave Mitchell wrote​:

for now, we modify the definitions of GvSVn(), GvAVn() etc so that
they become lvalue again, but leave the rest of the 819b139 refactoring
in
place. This should fix the issues with CPAN code.
I'll make a patch implementing lval to Perl_gv_add_by_type_p, since I had
a suspicion this will happen.
Patch attached. Tested on an unmodified Variable​::Magic

But this patch doesn't doesn't revert GvSVn(), GvAVn() etc to being
lvalues as I suggested.

All of the breakage reports so far are about ERRSV and DEFSV not being
lvals. Lets not penalize all the rval Gv**n users on CPAN for the
non-existent lval Gv**n users. There is also the issue of that the longer a
bad API is available, the more likely some random person will use the bad
API (like SvPV_set instead of sv_setpvn) making it a bigger burden to
deprecate/remove in the future. If someone comes with a report of Gv**n as
Gv**n not being lval in existing CPAN code, then we can turn Gv**n
individually (GvSVn or GvAVn or GvHVn or GvIOn, not all 4 in 1 shot) back to
lval before 5.22.0 ships. I put in the provisions for GvAVnL and GvHVnL to
exist in the gv_add_type enum table. I didn't create GvAVnL and GvHVnL since
someone might get the idea to use them between today and may 2016 and then
demand that we keep them as part of the public API. If someone has a better
naming convention for the macros other than "L" postfix please speak up. I
couldn't use "LV" since that would be confusing with the PVLV SV type. And
lower case "l" means it is not the first letter of an abbreivated word (IDK
what "Vnl" is an abbreviation for). Perl macros already have the "_const"
postfix for the +0 rval only versions of macros (see SvRV_const and
SvPVX_const). But there is no opposite term of "_const".

2 cpan greps for lvalue of Gv**n returns exactly 1 module

http​://grep.cpan.me/?q=%26\%28*Gv\w\wn\s*\%28 nothing

http​://grep.cpan.me/?q=Gv\w\wn\s*\%28.%2B\%29\s*%3D 1 module

Those greps done for your original patch would have shown that nothing on
CPAN was going to break, and yet half of CPAN broke.

I now vote for complete reversion of the original patch, then worry about
it again post 5.22. We are 3 days away from the contentious commit
deadline, and I regard this issue as contentious.

--
Modern art​:
  "That's easy, I could have done that!"
  "Ah, but you didn't!"

From @rjbs

* Dave Mitchell <davem@​iabyn.com> [2015-01-17T16​:29​:42]

Those greps done for your original patch would have shown that nothing on
CPAN was going to break, and yet half of CPAN broke.

I now vote for complete reversion of the original patch, then worry about
it again post 5.22. We are 3 days away from the contentious commit
deadline, and I regard this issue as contentious.

The issue is certainly contentious. If someone (anyone) produces a patch that
restores the old lvalue-ness-es without being a simple reversion, swell.
Otherwise, this has to be reverted until the new blead. Maint-0 stability
practically lives and dies by smoke and BBC reports **and we are not getting
those now**. This issue is a continuing red flashing light in my bedroom.

--
rjbs

From @tonycoz

On Sat Jan 17 18​:57​:08 2015, perl.p5p@​rjbs.manxome.org wrote​:

* Dave Mitchell <davem@​iabyn.com> [2015-01-17T16​:29​:42]

Those greps done for your original patch would have shown that
nothing on
CPAN was going to break, and yet half of CPAN broke.

I now vote for complete reversion of the original patch, then worry
about
it again post 5.22. We are 3 days away from the contentious commit
deadline, and I regard this issue as contentious.

The issue is certainly contentious. If someone (anyone) produces a
patch that
restores the old lvalue-ness-es without being a simple reversion,
swell.
Otherwise, this has to be reverted until the new blead. Maint-0
stability
practically lives and dies by smoke and BBC reports **and we are not
getting
those now**. This issue is a continuing red flashing light in my
bedroom.

v5.21.7-259-g819b139 was reverted in v5.21.7-436-g13c59d4.

All of Variable​::Magic, Glib. BerkeleyDB and DBI all pass with blead.

I'm marking this ticket resolved.

Tony

@tonycoz - Status changed from 'open' to 'resolved'