search

Perl / perl5

šŸŖ The Perl programming language
https://dev.perl.org/perl5/
Other
1.94k stars 554 forks source link

Bleadperl v5.21.7-259-g819b139 breaks half of CPAN #14410

Closed p5pRT closed 9 years ago

p5pRT commented 9 years ago

Migrated from rt.perl.org#123580 (status was 'resolved')

Searchable as RT123580$

p5pRT commented 9 years ago

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

p5pRT commented 9 years ago

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

p5pRT commented 9 years ago

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

p5pRT commented 9 years ago

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

p5pRT commented 9 years ago

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

p5pRT commented 9 years ago

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.

p5pRT commented 9 years ago

From @andk

On Sat\, 10 Jan 2015 14​:23​:48 -0800\, "Father Chrysostomos via RT" \perlbug\-followup@&#8203;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

p5pRT commented 9 years ago

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.

p5pRT commented 9 years ago

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

p5pRT commented 9 years ago

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.

p5pRT commented 9 years ago

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

p5pRT commented 9 years ago

From @wolfsage

On Tue\, Jan 13\, 2015 at 6​:45 AM\, Dave Mitchell \davem@&#8203;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)

p5pRT commented 9 years ago

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@&#8203;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?

p5pRT commented 9 years ago

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 https://github.com/perl5-dbi/dbi/commit/7d32b921169c4f7bb842e4ee0a3c3c484a3deb74 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

p5pRT commented 9 years ago

From @ilmari

bulk88 \bulk88@&#8203;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

p5pRT commented 9 years ago

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.

p5pRT commented 9 years ago

From @bulk88

0001-add-lvalue-feature-to-Perl_gv_add_by_type_p.patch ```diff From b227cc7c33967fd25d1ac7054659434669f08779 Mon Sep 17 00:00:00 2001 From: Daniel Dragan 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 ```
p5pRT commented 9 years ago

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.

p5pRT commented 9 years ago

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 819b139db33e2022424694e381422766903d4f65 - 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)

p5pRT commented 9 years ago

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

p5pRT commented 9 years ago

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 819b139db33e2022424694e381422766903d4f65 - 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)

p5pRT commented 9 years ago

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

p5pRT commented 9 years ago

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

p5pRT commented 9 years ago

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!"

p5pRT commented 9 years ago

From @rjbs

* Dave Mitchell \davem@&#8203;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

p5pRT commented 9 years ago

From @tonycoz

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

* Dave Mitchell \davem@&#8203;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

p5pRT commented 9 years ago

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