eval_pv fails to compile on non-gcc compilers with Perl < 5.031

[tglsfdc]

I've found that the eval_pv implementation added in PPPort 3.54 does not work on compilers that don't support gcc-like statements within expressions. Without BRACE_GROUPS, that macro looks like

#define eval_pv(p, croak_on_error) ((PL_Sv = Perl_eval_pv(aTHX_ p, 0)), (croak_on_error && (SvROK(ERRSV) || SvTRUE(ERRSV)) && (croak_sv(ERRSV), 1)), PL_Sv)

but croak_sv expands to something involving STMT_START { ... } STMT_END, so kaboom.

Observed with perl 5.12.5 and Solaris 11.3's acomp, and also with perl 5.8.9 and a really hoary HPUX compiler. Things were fine before we updated to a late-model ppport.h ...

[pali]

Would following patch fix your issue?

diff --git a/parts/inc/call b/parts/inc/call
index 351f8cc5547d..756378c7f453 100644
--- a/parts/inc/call
+++ b/parts/inc/call
@@ -51,7 +51,8 @@ __UNDEFINED__ PERL_LOADMOD_IMPORT_OPS   0x4
 #if defined(PERL_USE_GCC_BRACE_GROUPS)
 # define D_PPP_CROAK_IF_ERROR(cond) ({ SV *_errsv; ((cond) && (_errsv = ERRSV) && (SvROK(_errsv) || SvTRUE(_errsv)) && (croak_sv(_errsv), 1)); })
 #else
-# define D_PPP_CROAK_IF_ERROR(cond) ((cond) && (SvROK(ERRSV) || SvTRUE(ERRSV)) && (croak_sv(ERRSV), 1))
+  static void D_PPP_CROAK_IF_ERROR(int cond) { dTHX; SV *errsv; if (!cond) return; errsv = ERRSV; if (SvROK(errsv) || SvTRUE(errsv)) croak_sv(errsv); }
+# define D_PPP_CROAK_IF_ERROR(cond) D_PPP_CROAK_IF_ERROR(cond)
 #endif
 
 #ifndef G_METHOD

[tglsfdc]

As given, that doesn't seem to work for me: I get a bunch of errors/warnings that look like the proposed inline function isn't quite right.

cc: "ppport.h", line 15051: error 1000: Unexpected symbol: "void".
cc: "ppport.h", line 15051: error 1588: "my_perl" undefined.
cc: "ppport.h", line 15051: error 1532: Reference through a non-pointer.
cc: "ppport.h", line 15051: error 1532: Reference through a non-pointer.
cc: "ppport.h", line 15051: error 1527: Incompatible types in cast: Must cast from scalar to scalar or to void type.
cc: "ppport.h", line 15051: error 1532: Reference through a non-pointer.
cc: "ppport.h", line 15051: error 1532: Reference through a non-pointer.
cc: "ppport.h", line 15051: error 1532: Reference through a non-pointer.
cc: "ppport.h", line 15051: error 1532: Reference through a non-pointer.
cc: "ppport.h", line 15051: error 1532: Reference through a non-pointer.
cc: "ppport.h", line 15051: error 1532: Reference through a non-pointer.
cc: "ppport.h", line 15051: error 1532: Reference through a non-pointer.
cc: "ppport.h", line 15051: error 1554: Indirection must be through a pointer.
cc: "ppport.h", line 15051: error 1552: First expression of ?: must be arithmetic.
cc: "ppport.h", line 15051: warning 563: Argument #1 is not the correct type.
cc: "ppport.h", line 15051: warning 604: Pointers are not assignment-compatible.
cc: "ppport.h", line 15051: warning 563: Argument #2 is not the correct type.
cc: "ppport.h", line 15051: warning 563: Argument #1 is not the correct type.
cc: "ppport.h", line 15051: error 1532: Reference through a non-pointer.
cc: "ppport.h", line 15051: error 1532: Reference through a non-pointer.
cc: "ppport.h", line 15051: error 1527: Incompatible types in cast: Must cast from scalar to scalar or to void type.
cc: "ppport.h", line 15051: warning 604: Pointers are not assignment-compatible.
cc: "ppport.h", line 15051: warning 563: Argument #2 is not the correct type.
cc: "ppport.h", line 15051: warning 527: Integral value implicitly converted to pointer in assignment.
cc: "ppport.h", line 15051: warning 563: Argument #4 is not the correct type.
make: *** [plperl.o] Error 1

You might be able to make something of the idea with some adjustments, but I don't know this code well enough to suggest exactly what.

[pali]

I updated patch, could you try it if it helps?

[tglsfdc]

After a bit more poking at it, the primary reason for the compile failures is that you missed doing the pTHX_/aTHX_ dance. For me, editing ppport.h like this:

-# define D_PPP_CROAK_IF_ERROR(cond) ((cond) && (SvROK(ERRSV) || SvTRUE(ERRSV)) && (croak_sv(ERRSV), 1))
+void D_PPP_CROAK_IF_ERROR(pTHX_ int cond) { SV *errsv; if (!cond) return; errsv = ERRSV; if (SvROK(errsv) || SvTRUE(errsv)) croak_sv(errsv); }
+# define D_PPP_CROAK_IF_ERROR(cond) D_PPP_CROAK_IF_ERROR(aTHX_ cond)

allows the compile to go through, but then (unsurprisingly) I get link-time errors about multiple modules defining D_PPP_CROAK_IF_ERROR. Not sure why the STATIC_INLINE bit doesn't work, but it doesn't compile with that.

[pali]

After a bit more poking at it, the primary reason for the compile failures is that you missed doing the pTHX_/aTHX_ dance.

That is why I updated patch with dTHX; line. Could you try if dTHX; is enough instead of using pTHX_ and aTHX_?

allows the compile to go through, but then (unsurprisingly) I get link-time errors about multiple modules defining D_PPP_CROAK_IF_ERROR. Not sure why the STATIC_INLINE bit doesn't work, but it doesn't compile with that.

That makes sense. D_PPP_CROAK_IF_ERROR has to be static. inline is just optimization. Could you try to replace STATIC_INLINE with just static keyword?

[tglsfdc]

Oh! My fault, I did not notice that you'd made that edit in addition to removal of the STATIC_INLINE bit. Yes, "dTHX;" works equally well as pTHX_/aTHX_, at least in my test case.

With the particular ancient compiler I'm testing, declaring the function as plain "static" works fine, with no unused-function warnings. I'm worried though that other compilers will whine about it in modules that use neither eval_pv nor eval_sv.

[tglsfdc]

Oh ... the reason PERL_STATIC_INLINE fails is that I'm testing against perl 5.8.9, which if I'm reading the chart right lacks that symbol (and I don't see it in grepping the perl headers, either). So plain "static" might be the only way.

[pali]

Could you check if macro PERL_STATIC_INLINE is defined? In some of my modules I have at the beginning:

#ifndef PERL_STATIC_INLINE
#define PERL_STATIC_INLINE static
#endif

So I have feeling that Devel::PPPort does not define PERL_STATIC_INLINE for old perl versions with old compilers.

[tglsfdc]

Indeed, there is no such definition in ppport.h.

[pali]

Ok! So seems that Devel::PPPort needs to backport PERL_STATIC_INLINE definition and then it should work.

[tglsfdc]

I'm still a bit worried about the unused-function-warning aspect, but maybe the set of cases where you'd get one is small enough to not bother about. Most people should be developing against versions that have PERL_STATIC_INLINE, and having that properly defined should be enough to prevent a warning.

[pali]

Could you try following patch for backporting PERL_STATIC_INLINE macro?

diff --git a/parts/inc/misc b/parts/inc/misc
index 6d3edbc5af6d..a74e8cf8a017 100644
--- a/parts/inc/misc
+++ b/parts/inc/misc
@@ -19,6 +19,7 @@ MUTABLE_PTR
 NVTYPE
 PERLIO_FUNCS_CAST
 PERLIO_FUNCS_DECL
+PERL_STATIC_INLINE
 PERL_UNUSED_ARG
 PERL_UNUSED_CONTEXT
 PERL_UNUSED_DECL
@@ -38,6 +39,12 @@ ASSUME
 
 =implementation
 
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
+__UNDEFINED__ PERL_STATIC_INLINE static inline
+#else
+__UNDEFINED__ PERL_STATIC_INLINE static
+#endif
+
 __UNDEFINED__ cBOOL(cbool) ((cbool) ? (bool)1 : (bool)0)
 __UNDEFINED__ OpHAS_SIBLING(o)      (cBOOL((o)->op_sibling))
 __UNDEFINED__ OpSIBLING(o)          (0 + (o)->op_sibling)

Ad unused-function-warning, we are dealing with old compilers, I would not spend too much time how to mute false-positive warnings for these compilers. Just to ensure that code is correct and accepted by older compilers. If you need to check that code is correct, just use some new compiler...

[tglsfdc]

Sorry, I'm not really set up to build Devel::PPPort from source. But I can confirm that editing ppport.h like this passes my tests:
ppport.patch.txt

[pali]

Pull request with fix is here: #217
Hopefully it is enough for fixing this issue.

[tglsfdc]

Thanks!

[atoomic]

Thanks a lot @tglsfdc for confirming that the current patch solves your issue.
This is going to be in the next release.

@pali thanks for staying on top of it!

[khwilliamson]

Testing with all the suite without brace groups, failed in 5.9.3

#define sv_len_utf8(sv) (PL_Sv = (sv), do { if (SvGMAGICAL(x)) mg_get(x); } while(0), sv_len_utf8_nomg(PL_Sv))

is the expansion that fails, with

error: expected expression before ‘do’

I'm not good at this part of C, figuring out what could get it to work

[atoomic]

I was not able to install 5.9.3 and I wonder how much we should care about old development versions.
I was able to test the suggested patch using 5.10.1 and did not noticed any issues at this point.

[khwilliamson]

As I told atoomic privately, usually a problem will exist in all previous versions to the one it is first found in. I looked at just the official versions, and sure enough, this bug exists in the first such one prior to 5.9.3; which is 5.8.8

[khwilliamson]

The bottom line is we've been testing only with gcc all this time, and it is not the only compiler that people use on older perls

[khwilliamson]

I am working on testing other possible problems. So far I have found one more failure which is easy to fix

[khwilliamson]

The other problem I found plus this one

#define sv_len_utf8(sv) (PL_Sv = (sv), do { if (SvGMAGICAL(x)) mg_get(x); } while(0), sv_len_utf8_nomg(PL_Sv))

are the only ones that surfaced when run without gcc brace groups. The other fix is easy; I haven't stared at this to see what to do about it. Feel free to figure it out.

[tglsfdc]

Use a ?: expression?

(PL_Sv = (sv), SvGMAGICAL(x) ? mg_get(x) : (void)0, sv_len_utf8_nomg(PL_Sv))

But I don't understand this macro. Where is "x" coming from?

[khwilliamson]

On 2/14/22 22:18, Tom Lane wrote: Use a ?: expression? (PL_Sv = (sv), SvGMAGICAL(x) ? mg_get(x) : (void)0, sv_len_utf8_nomg(PL_Sv)) But I don't understand this macro. Where is "x" coming from?

It is missing the beginning #define foo(x) (PL_Sv = (sv), SvGMAGICAL(x) ? mg_get(x) : (void)0, sv_len_utf8_nomg(PL_Sv))

[tglsfdc]

Hm ... well, the double evaluation of the macro argument is bad practice, but it was like that already.

[pali]

@tglsfdc: Look! @khwilliamson wrote it incompletely, sv_len_utf8 macro is defined as:

#define SvGETMAGIC(x) do { if (SvGMAGICAL(x)) mg_get(x); } while (0)
#define sv_len_utf8_nomg(sv) (PL_Sv = (sv), (SvUTF8(PL_Sv) ? Perl_sv_len_utf8(aTHX_ (!SvGMAGICAL(PL_Sv) ? PL_Sv : sv_mortalcopy_flags(PL_Sv, SV_NOSTEAL))) : (SvPV_nomg(PL_Sv, PL_na), PL_na)))
#define sv_len_utf8(sv) (PL_Sv = (sv), SvGETMAGIC(PL_Sv), sv_len_utf8_nomg(PL_Sv))

where PL_Sv is global scratchpad variable of type SV *

Most of the perl macros evaluates its argument more than once, and this practice is documented the official in perlapi documentation, so this is not issue.

[tglsfdc]

Got it. So this should be enough:

#define SvGETMAGIC(x) (SvGMAGICAL(x) ? mg_get(x) : (void) 0)

I didn't find the definition of mg_get in a quick look, so I'm not sure if the cast to void is right.

[pali]

mg_get() is documented in perldoc perlapi that returns int. In header files it is defined as:

#define mg_get(a) Perl_mg_get(aTHX_ a)
PERL_CALLCONV int Perl_mg_get(pTHX_ SV* sv);

SvGETMAGIC is documented that it should have signature as void SvGETMAGIC(SV* sv)

In Perl source code is SvGETMAGIC defined as:

#define SvGETMAGIC(x) ((void)(UNLIKELY(SvGMAGICAL(x)) && mg_get(x)))

So what about defining it in Devel::PPPort in the same way? Probably with dropped UNLIKELY.

[khwilliamson]

I am working on testing other possible problems. So far I have found one more failure which is easy to fix

[khwilliamson]

I would not have thought to go looking for an updated definition, so I really appreciate pali's having done so.

Changing to use this definition causes it to compile, but the sv_len_utf8 (with and without _nomg) tests fail. undef is being returned. I am starting to look at this.

[khwilliamson]

I have been busy on other things, but also beating my head against the other issues that were uncovered by this. It turns out that sv_len_utf8_nomg() has likely never been a public function. I know not why. That's what threw me off.

@pali submitted a patch to make it visible 303ccc0. That patch doesn't work without brace groups.

And, do we need it.? No module that doesn't include ppport.h can see it, because it is not publicly available. In other words, this is a function that is only furnished by Devel::PPPort, not by perl.

[tglsfdc]

I see that embed.h defines sv_len_utf8_nomg only #ifdef PERL_CORE, so I agree it's not meant to be public. But ppport.h is using it to implement sv_len_utf8, so maybe you have to have it for that?

[pali]

Look at these two macros:

__UNDEFINED__ newSVsv_flags(sv, flags) ((PL_Sv = newSV(0)), sv_setsv_flags(PL_Sv, (sv), (flags)), PL_Sv)
__UNDEFINED__ sv_mortalcopy_flags(sv, flags) sv_2mortal(newSVsv_flags((sv), (flags)))

It means that it is not possible to call newSVsv_flags(PL_Sv, ...) and therefore also not possible to call sv_mortalcopy_flags(PL_Sv, ...) because newSVsv_flags overwrites PL_Sv variable.

Changing to use this definition causes it to compile, but the sv_len_utf8 (with and without _nomg) tests fail. undef is being returned. I am starting to look at this.

But sv_len_utf8_nomg is doing it and so it does not work. And sv_len_utf8 calls sv_len_utf8_nomg. That is why it is broken.

How to solve it? I have an idea about suboptimal implementation which will call other functions even when not needed, but I think it could provide correct result:

__UNDEFINED__ sv_len_utf8_nomg(sv) sv_len_utf8(sv_mortalcopy_flags((sv), SV_NOSTEAL))

Any idea? Tests and comments are welcome.

[khwilliamson]

#218 should fix this. Feel free to comment and test

[pali]

Approved.