New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
5.29.4 - re/subst.t crashes perl on 64-bit Windows #16729
Comments
From @sisyphusHi, On 64-bit builds, subst.t behaves perfectly up to and including test 236: ok 236 - vivifying stash elem in $that::{elem} =~ s//.../e At that point, however, perl immediately crashes - and I haven't yet found The same crash occurs if I run the test script outside of Test::Harness. No problem with that test script on 32-bit builds. Cheers, |
From @tonycozOn Sun, Oct 21, 2018 at 02:03:53AM -0700, sisyphus (via RT) wrote:
I didn't see it on 64-bit builds with CCTYPE=MSVC90. Could you supply the -V information please? Tony |
The RT System itself - Status changed from 'new' to 'open' |
From @sisyphusOn Sun, 21 Oct 2018 04:19:18 -0700, tonyc wrote:
Cheers, |
From @sisyphusThis bug might be specific to my mingw-w64 port of gcc-8.1.0 - though there was no such problem with 5.29.3. If (using gcc-8.1.0) I skip the offending "read-only COW =~ s/does not match// should croak" test, that test script then runs ok and all tests except for IO/Socket.t (see #133604) pass. However,I have now detected that t/run/fresh_perl.t is behaving strangely. .... with all test passing. Cheers, |
From @sisyphusOn Sun, 21 Oct 2018 21:54:41 -0700, sisyphus@cpan.org wrote:
Just a little bit more about this problem with t/run/fresh_perl.t on perl-5.29.4: 1) It's happening only on x64 builds whose nvtype is double - there's no such issue if nvtype is long double; Needs some more poking. Cheers, |
From @tonycozOn Sun, Oct 21, 2018 at 09:54:41PM -0700, sisyphus@cpan.org via RT wrote:
I can't reproduce either failure: J:\dev\perl\git\perl\t>.\perl -I..\lib -V:gccversion J:\dev\perl\git\perl\t>.\perl harness run\fresh_perl.t J:\dev\perl\git\perl\t>.\perl -I..\lib -V:ptrsize J:\dev\perl\git\perl\t>.\perl harness re\subst.t I didn't get a popup from fresh_perl.t I started from a clean tree, and built with: gmake -j2 CFG=Debug test-prep Tony |
From @sisyphusOn Mon, 22 Oct 2018 17:20:50 -0700, tonyc wrote:
I've just now checked and "CFG=Debug" avoids both issues for me, too. I'm still seeing these issues with a plain 'gmake -j2' (with no additional args), followed by: cd .. I think there's a fair chance you'll see the same. And I doubt that the '-j2' arg has any bearing on the issues as I don't normally specify any '-j*' arg when building. Cheers, |
From @tonycozBuilding with a default CFG, or by eliminating -DDEBUGGING I can
and the fresh_perl test:
Tony |
From @sisyphusHaven't made any progress with this. Other than that, all I have is a couple of one-liners (based on the t/run/fresh_perl.t and t/re/subst.t failures) that demonstrate the problem: C:\>perl -le "for('A') {s/b/c/}" C:\>perl -le "for(__PACKAGE__) {s/b/c/}" C:\> In both cases a "perl.exe has stopped working" pop-up appears after the error message has been presented, and the process doesn't exit until the pop-up has been acknowledged. One heisenbuggy aspect that I noticed was that if I preceded the above code with a Devel::Peek::Dump() of an arbitrary string, then there was no crash to contend with. That is, this one liner (for example) behaves exactly as it ought: C:\>perl -MDevel::Peek -le "Dump('x'); for('A') {s/b/c/}" C:\> Problem still persists in 5.29.5. Cheers, |
From @tonycozOn Sat, 01 Dec 2018 23:46:44 -0800, sisyphus@cpan.org wrote:
After some fumbling, this bisected to: 69aa5eb is the first bad commit win32: define HAS_BUILTIN_EXPECT on MinGW :040000 040000 b4bfa77c7d830364d15c8004a70fb0e05bdff24e 6c7cd94c61bee93056f74b169328b169583428bd M win32 which implies a bug in gcc, since all this does is turn: EXPECT(expr,val) from (expr) to __builtin_expect(expr,val) and EXPECT(expr,val) is only used in the LIKELY() and UNLIKELY() macros: #define LIKELY(cond) EXPECT(cBOOL(cond),TRUE) __builtin_expect() is documented at https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html Reverting the above commit prevented the crash for me. Tony |
From @sisyphusOn Sun, 02 Dec 2018 20:52:42 -0800, tonyc wrote:
Reverting those changes to win32/config.gc and win32/config_H.gc also works for me - and I'll continue to make those changes whenever I build perl using my mingw-w64 port of 64-bit gcc-8.1.0. (I already patch those 2 files for my own builds as a result of #133582, anyway.) If anyone comes up with a C program that demonstrates this gcc bug, please let me know as I'd like to file a bug report for it with the mingw-w64 project. Thanks Tony. Cheers, |
From @xenuOn Tue, 04 Dec 2018 03:31:29 -0800
I also am trying to find the cause of the crash, but I'm having no luck Inline Patchdiff --git a/sv.h b/sv.h
index f3392b08ec..58c3a8f33c 100644
--- a/sv.h
+++ b/sv.h
@@ -2055,7 +2055,7 @@ properly null terminated. Equivalent to sv_setpvs(""), but more efficient.
#define SvUNLOCK(sv) PL_unlockhook(aTHX_ sv)
#define SvDESTROYABLE(sv) PL_destroyhook(aTHX_ sv)
-#define SvGETMAGIC(x) ((void)(UNLIKELY(SvGMAGICAL(x)) && mg_get(x)))
+#define SvGETMAGIC(x) ((void)((SvGMAGICAL(x)) && mg_get(x)))
#define SvSETMAGIC(x) STMT_START { if (UNLIKELY(SvSMAGICAL(x))) mg_set(x); } STMT_END
#define SvSetSV_and(dst,src,finally) \ |
From @khwilliamsonOn 12/4/18 7:02 AM, Tomasz Konojacki wrote:
In the meantime, it seems that a hints directive could be used on mingw |
From @sisyphusOn Tue, 04 Dec 2018 07:26:12 -0800, public@khwilliamson.com wrote:
Hi, My gcc-8.1.0 was built by the mingw-w64 project, and today I've also built latest devel release (5.29.9) using a gcc-8.2.1 toolchain that was built by the msys2 project. That build of perl also experienced the same issues as the gcc-8.1.0 build, and those issues were again resolved when I reverted the changes to win32/config.gc and win32/config_H.gc. So we can now say that this issue afflicts both 64-bit gcc-8.1.0 (with mingw runtime version 6.0) and 64-bit gcc-8.2.1 (with mingw runtime version 7.0). Ought this be deemed a blocker for 5.30 ? Personally, I've no objection to continuing to revert win32/config.gc and win32/config_H.gc for my local builds. Cheers, |
From @khwilliamsonOn Mon, 01 Apr 2019 19:31:47 -0700, sisyphus@cpan.org wrote:
I added this to the 5.30 blockers list, to make sure it gets adequate attention -- |
From @iabynOn Mon, Apr 01, 2019 at 08:01:04PM -0700, Karl Williamson via RT wrote:
I think perhaps for 5.30.0 we should revert the entirely of the The commit I propose reverting in full is: commit 69aa5eb win32: define HAS_BUILTIN_EXPECT on MinGW Affected files ... Differences ... Inline Patchdiff --git a/win32/config.gc b/win32/config.gc
index 4477ebc04c..56f367f91d 100644
--- a/win32/config.gc
+++ b/win32/config.gc
@@ -117,7 +117,7 @@ d_bsdgetpgrp='undef'
d_bsdsetpgrp='undef'
d_builtin_add_overflow='undef'
d_builtin_choose_expr='undef'
-d_builtin_expect='undef'
+d_builtin_expect='define'
d_builtin_mul_overflow='undef'
d_builtin_sub_overflow='undef'
d_c99_variadic_macros='undef'
diff --git a/win32/config_H.gc b/win32/config_H.gc
index 87e90bac4b..756d4a7cf2 100644
--- a/win32/config_H.gc
+++ b/win32/config_H.gc
@@ -2319,7 +2319,7 @@
* Can we handle GCC builtin for telling that certain values are more
* likely
*/
-/*#define HAS_BUILTIN_EXPECT / **/
+#define HAS_BUILTIN_EXPECT /**/
/*#define HAS_BUILTIN_CHOOSE_EXPR / **/
/* HAS_C99_VARIADIC_MACROS:
-- "Emacs isn't a bad OS once you get used to it. |
From @xenuOn Tue, 23 Apr 2019 16:43:50 +0100
Yeah, I think it's the best course of action. I hoped we would find the |
From @iabynOn Tue, Apr 23, 2019 at 11:03:35PM +0200, Tomasz Konojacki wrote:
Now reverted with v5.29.10-13-gabd494f123. I'll move it to the 5.32.0 -- |
From @jkeenanOn Wed, 24 Apr 2019 11:32:38 GMT, davem wrote:
Have we identified the cause of the problem reported in this ticket (such that we can report the bug externally)? Thank you very much. -- |
From @sisyphusAFAIK, although we know how to trigger it, we haven't yet identified the I did spend some time a while back trying to create a C script that would Using the mingw64 port of gcc-8.3.0 that ships with StrawberryPerl-5.30.0, Cheers, On Fri, May 31, 2019 at 10:43 PM James E Keenan via RT <
|
From @xenuOn Sat, 1 Jun 2019, at 03:20, sisyphus wrote:
My findings so far: 1. The attached patch makes it not crash with HAS_BUILTIN_EXPECT commit *not* reverted. It removes LIKELY() from SvGETMAGIC() inside pp_subst(). 2. I have attached assembly code of pp_hot.c with and without my patch applied. The difference between them is really tiny: --- a/C:/Users/xenu/Documents/pp_hot/crashing/pp_hot.s Unless I'm missing something, this change is completely harmless. The only difference is Perl_croak_no_modify() call being moved a bit, but the control flow in both versions is *exactly* the same. That makes me suspect that it's a bug in the linker, which is probably the worst possible scenario, because linkers are extremely hard to debug :( |
From @xenumake_it_not_crash.patchdiff --git a/pp_hot.c b/pp_hot.c
index 2df5df8303..9ecb0b678f 100644
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -4146,7 +4146,7 @@ PP(pp_subst)
EXTEND(SP,1);
}
- SvGETMAGIC(TARG); /* must come before cow check */
+ ((void)((SvGMAGICAL(TARG)) && mg_get(TARG))); /* must come before cow check */
#ifdef PERL_ANY_COW
/* note that a string might get converted to COW during matching */
was_cow = cBOOL(SvIsCOW(TARG));
|
I've created a meta-ticket around the general issue of this type of failure in mingw at #17521 |
This patch wasn't meant to be applied, its purpose was to demonstrate the issue. Anyway, there's nothing we can do about this issue in time for 5.32. We still don't have a minimal code sample, so we can't report it to binutils (or gcc) developers. |
Based on the above comment, I have removed this as a 5.32 blocker |
What is the current status of this? @xenu |
Migrated from rt.perl.org#133603 (status was 'open')
Searchable as RT133603$
The text was updated successfully, but these errors were encountered: