Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upglob in threads is broken in perl5.18 #13291
Comments
From trast@inf.ethz.chIt seems <glob*> contains a race when used in parallel in threads. This ----- 8< ----- use warnings; use threads; my $nthread = 20; sub work { my @threads; foreach my $t (@threads) { There are two kinds of errors shown: a) Attempt to free unreferenced scalar: SV 0xd1e738, Perl interpreter: 0xdf8900 at /home/thomas/tmp/perl-thread-glob-test.pl line 13. b) Thread 1 terminated abnormally: sv_upgrade from type 255 down to type 11 at perl-thread-glob-test.pl line 13. Followed by a segmentation fault or (rarely) a bus error. It does not matter whether the glob actually matches any files, but The segfault looks like this: #0 0x0000000000499720 in Perl_hv_common (my_perl=0x8f7e80, hv=0x889720, keysv=keysv@entry=0x0, or sometimes just starts at the iterate() frame: #0 0x00007ffff68b569b in iterate (my_perl=0xcbad60, globber=0x7ffff68b5c60 <csh_glob>) at Glob.xs:96 vincent on #perl ran the test script on many perl versions (thanks!) and $ for p in ~/perl/builds/bin/perl5*thr* ; do echo $p ; $p x.pl ; done --- perlbug -d --- Flags: This perlbug was built using Perl 5.16.2 - Mon Mar 11 11:01:56 UTC 2013 Site configuration information for perl 5.16.2: Configured by abuild at Mon Mar 11 10:54:42 UTC 2013. Summary of my perl5 (revision 5 version 16 subversion 2) configuration: Locally applied patches: @INC for perl 5.16.2: Environment for perl 5.16.2: -- |
From @cpansproutOn Wed May 01 04:55:31 2013, trast@inf.ethz.ch wrote:
I can produce this bug on Mac OS X as well. I also see ‘Attempt to free nonexistent shared string’, ‘Assertion I have tried to diagnose this, but I am stumped. It has nothing to do with readdir. I get the same thing with <{a}>, but The bug can’t be in glob_expand. I changed glob_expand to return early, But I can’t see anything wrong with the code. Nothing uses globals. -- Father Chrysostomos |
|
The RT System itself - Status changed from 'new' to 'open' |
From @cpansproutOn Sat Jul 06 17:53:30 2013, sprout wrote:
I meant globextend. -- Father Chrysostomos |
From @nthykierCreated by @nthykierDear perl porters, When Debian migrated to Perl5.18.1, I started to experience random """ The test case itself does not crash, but it does spew out warnings """ Based on that, I replaced my calls to "glob" in the threads and my script hugmeir also suggested that he believed this bug to also affect blead ~Niels Perl Info |
From @cpansproutOn Fri Sep 20 04:09:25 2013, niels@thykier.net wrote:
Interesting. Could this be the same as #117823? -- Father Chrysostomos |
|
The RT System itself - Status changed from 'new' to 'open' |
From @nthykierOn 2013-09-20 17:39, Father Chrysostomos via RT wrote:
To be honest, I am not sure. When I discovered the issue, I ran a gdb ~Niels [0] An extra data point is the assertion triggered. In the Debian bug debugperl: hv.c:356: Perl_hv_common: Assertion `((svtype)((hv)->sv_flags But RT#117823 lists: Glob.xs:232: csh_glob: Assertion `((svtype)((entries)->sv_flags & 0xff)) [1] E.g. both bugs involves calling "glob" from threads. My original [DebBug] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=723805 |
From @nthykier |
From @HugmeirOn Fri, Sep 20, 2013 at 12:39 PM, Father Chrysostomos via RT <
Yes. The problem for both is that x_GLOB_ENTRIES should be thread-local, This patch solves the issue: Inline Patchdiff --git a/ext/File-Glob/Glob.xs b/ext/File-Glob/Glob.xs
index b3705b3..181dbbc 100644
--- a/ext/File-Glob/Glob.xs
+++ b/ext/File-Glob/Glob.xs
@@ -396,6 +396,13 @@ PPCODE:
iterate(aTHX_ doglob_iter_wrapper);
SPAGAIN;
+void
+CLONE(...)
+CODE:
+ PERL_UNUSED_ARG(items);
+ MY_CXT_CLONE;
+ MY_CXT.x_GLOB_ENTRIES = NULL;
+
BOOT:
{
#ifndef PERL_EXTERNAL_GLOB
./perl -Ilib -Mthreads -E 'sub foo {scalar glob("*")} foo(); say Will return "Artistic" four times, rather than "Artristic" followed by the |
From @cpansproutOn Fri Sep 20 16:31:00 2013, Hugmeir wrote:
Yes, the latter. That would be backward-compatible with when File::Glob Thank you for tackling this. This is entirely my fault, as I was the -- Father Chrysostomos |
From @HugmeirOn Fri, Sep 20, 2013 at 9:16 PM, Father Chrysostomos via RT <
I'm afraid I don't know how to implement this properly! Unlike dup magic, Inline Patchdiff --git a/ext/File-Glob/Glob.xs b/ext/File-Glob/Glob.xs
index 181dbbc..118d88e 100644
--- a/ext/File-Glob/Glob.xs
+++ b/ext/File-Glob/Glob.xs
@@ -9,6 +9,9 @@
#define MY_CXT_KEY "File::Glob::_guts" XS_VERSION
typedef struct {
+#ifdef USE_ITHREADS
+ tTHX interp;
+#endif
int x_GLOB_ERROR;
HV * x_GLOB_ENTRIES;
Perl_ophook_t x_GLOB_OLD_OPHOOK;
@@ -396,12 +399,32 @@ PPCODE:
iterate(aTHX_ doglob_iter_wrapper);
SPAGAIN;
+#ifdef USE_ITHREADS
+
void
CLONE(...)
+INIT:
+ HV *glob_entries_clone = NULL;
CODE:
PERL_UNUSED_ARG(items);
- MY_CXT_CLONE;
- MY_CXT.x_GLOB_ENTRIES = NULL;
+ {
+ dMY_CXT;
+ if ( MY_CXT.x_GLOB_ENTRIES ) {
+ CLONE_PARAMS param;
+ param.stashes = NULL;
+ param.flags = 0;
+ param.proto_perl = MY_CXT.interp;
+
+ glob_entries_clone =
BOOT: And with that: AUTHORS So, it works, but no clue if this is the proper way of doing it. use Config; BEGIN { [*] At least, that's the impression that I got when I was looking some |
From @cpansproutOn Fri Sep 20 18:05:44 2013, Hugmeir wrote:
Ah, but File::Glob is *not* normal code!
It has often bothered me that a CLONE method in pure Perl can’t really If what you have below works, and works reliably (it looks fine to me), Is it possible to write this in a way that is binary-compatible (for
Probably.
-- Father Chrysostomos |
From @cpansproutOn Fri Sep 20 20:19:01 2013, sprout wrote:
That gives the wrong impression. Let’s try again: I believe (and hope) the proposed changes are binary-compatible. If it -- Father Chrysostomos |
From @HugmeirOn Sat, Sep 21, 2013 at 12:19 AM, Father Chrysostomos via RT <
Heh, fair enough.
I wasn't sure if the new tests would work on VMS -- most of the other glob
This I do not know. |
From @cpansproutOn Fri Sep 20 23:45:43 2013, Hugmeir wrote:
Your test assumes that the output will be sorted, but does not even use Concerning your earlier Test::More comment and my response, I don’t
As long as XS extensions compiled with 5.18.[10] continue to work with BTW, we should probably add a new API to perl for registering clone -- Father Chrysostomos |
From @HugmeirOn Sat, Sep 21, 2013 at 10:56 AM, Father Chrysostomos via RT <
Ah, I see, thanks. From the docs, I had assumed that it always gave sorted
While that's the case right now, the problem is that it's a potential
+50 |
From @cpansproutOn Sat Sep 21 07:40:33 2013, Hugmeir wrote:
I think perlfunc/glob needs to state that glob is not implemented via -- Father Chrysostomos |
From @craigberryOn Sat, Sep 21, 2013 at 12:51 PM, Father Chrysostomos via RT
I have done a smoke run of smoke-me/hugmeir/dup_glob_state, results <http://www.nntp.perl.org/group/perl.daily-build.reports/2013/09/msg151124.html> I mention that here because unless you have the SHA1s memorized, there
It uses the results of LIB$FIND_FILE, documented at: <http://h71000.www7.hp.com/doc/82final/5932/5932pro_017.html#fin_f> but I see no mention of sorting there. As far as I've ever seen,
Does perlfunc/glob really need to say how it's implemented? To me
On VMS, glob does not split its arguments on whitespace: $ perl -e "print glob('*.c');" I don't know when the split on whitespace feature was introduced but Whitespace in the pattern is assumed to be part of the filename, and $ create file^ with_space.txt where "^_" is the canonical form of a caret-escaped space character. |
From @HugmeirOn Sat, Sep 21, 2013 at 10:56 AM, Father Chrysostomos via RT <
Actually. I've hit a bit of a snag here. How come bsd_glob() doesn't keep use 5.010; say "Default glob:"; say "\nbsd_glob:"; say "\ncsh_glob:"; __END__ bsd_glob: csh_glob: Also, by doing a 'use File::Glob qw(:bsd_glob)', CORE::glob gets replaced I can work around this by using csh_glob in those tests, which both keeps
|
From kaffeetisch@gmx.deOn 21.09.2013 15:56, Father Chrysostomos via RT wrote:
Note that there is a kind of API for SV-specific cloning: the MGf_DUP |
From @cpansproutOn Sat Sep 21 19:48:07 2013, craig.a.berry@gmail.com wrote:
I don’t think it’s important. My main point was that the test intended
Currently we state that it is implemented via File::Glob and one can see
av.cdeb.cdoio.cdoop.cdquote_static.cdump.cgenerate_uudmap.cglobals.cgv.chv.cinline_invlist.ckeywords.clocale.cmadly.cmalloc.cmathoms.cmg.cmg_names.c miniperlmain.cmro.cnumeric.cop.coverload.cpacksizetables.cpad.cperl.cperlapi.cperlio.cperlmain.cPERLMINI.Cperly.cpp.cpp_ctl.cpp_hot.cpp_pack.cpp_sor t.cpp_sys.creentr.cregcomp.cregexec.crun.cscope.csv.ctaint.ctime64.ctoke.cuniversal.cutf8.cutil.cvms.c
perl 3, which introduced glob.
-- Father Chrysostomos |
From @cpansproutOn Sun Sep 22 02:28:24 2013, Hugmeir wrote:
It’s an historical mess. It used to be that keeping state and splitting I suggested adding the :bsd_glob tag (since bsd_glob was known not to But File::Glob’s interface was a confusing mess to begin with, and we -- Father Chrysostomos |
From @epaCraig A. Berry <craig.a.berry <at> gmail.com> writes:
AFAIK, glob splitting on whitespace was an accident of its original Personally I'm not a fan of this behaviour - it tends to cause bugs when -- |
From @HugmeirOn Sat, Sep 21, 2013 at 11:47 PM, Craig A. Berry <craig.a.berry@gmail.com>wrote:
Thanks! I would've entirely missed that. Looks like under threads, the new
I think that it would belong in perlport, but perlport for glob points to
Honestly that seems more useful than splitting on whitespace. Windows uses |
From @ikegamiOn Tue, Sep 24, 2013 at 9:32 AM, Brian Fraser <fraserbn@gmail.com> wrote:
Wouldn't be a big issue if you could escape the space, but you can't. Gotta
|
From @ikegamiOn Tue, Sep 24, 2013 at 1:36 PM, Eric Brine <ikegami@adaelis.com> wrote:
Let me rephrase that. You can't *on Windows*. There's no problem escaping $ perl -E'say for glob "a\ b/*"' |
From @maukeOn 24.09.2013 14:27, Ed Avis wrote:
Yes, please! You can easily get the old behavior of glob "foo bar" with -- |
From @ikegamiOn Tue, Sep 24, 2013 at 3:02 PM, Lukas Mai <plokinom@gmail.com> wrote:
Why break backwards compatibility just to save yourself from escaping |
From @TuxOn Tue, 24 Sep 2013 21:02:45 +0200, Lukas Mai <plokinom@gmail.com>
No, please not. That would break foreach my $filename (<a b c d* ef* >) { which would extend to quite a long and ugly line when you have to -- |
From @craigberryOn Tue, Sep 24, 2013 at 8:32 AM, Brian Fraser <fraserbn@gmail.com> wrote:
Here ha go: <http://www.nntp.perl.org/group/perl.daily-build.reports/2013/09/msg151317.html> The test failure I think you're interested in looks like: $ perl ../ext/File-Glob/t/threads.t And the gotcha is that on VMS, all files have extensions, even those Inline Patch--- ext/File-Glob/t/threads.t;-0 2013-09-22 05:26:48 -0500
+++ ext/File-Glob/t/threads.t 2013-09-25 07:08:29 -0500
@@ -28,7 +28,7 @@ use File::Glob qw(csh_glob);
my($dir) = tempdir(CLEANUP => 1)
or die "Could not create temporary directory";
-my @temp_files = qw(1_file 2_file 3_file);
+my @temp_files = qw(1_file.tmp 2_file.tmp 3_file.tmp);
for my $file (@temp_files) {
open my $fh, ">", File::Spec->catfile($dir, $file)
or die "Could not create file $dir/$file: $!";
which gets the test to pass: $ perl ../ext/File-Glob/t/threads.t |
From @HugmeirOn Wed, Sep 25, 2013 at 9:14 AM, Craig A. Berry <craig.a.berry@gmail.com>wrote:
Ooh, thank you! I'll push the branch with those changes later today. |
From @maukeOn 25.09.2013 08:07, H.Merijn Brand wrote:
for my $filename (map glob, qw<a b c d* ef*>) { -- |
|
@Hugmeir - Status changed from 'open' to 'resolved' |
From @HugmeirOn Wed, Sep 25, 2013 at 9:51 AM, Brian Fraser <fraserbn@gmail.com> wrote:
For some loose definition of "today." Fixed in facf34e on blead. |
From @nwc10On Wed, Sep 25, 2013 at 09:51:42AM -0300, Brian Fraser wrote:
In theory you could run it on emulated hardware, as there is a "hobbyist I have an account, but I'm not sure what the current procedure is to get Nicholas Clark |
From @craigberryOn Fri, Sep 27, 2013 at 6:22 AM, Nicholas Clark <nick@ccl4.org> wrote:
There are various Alpha emulators around and I think they all have a
And I believe it's a pretty underpowered ten-year-old Integrity
I think all you have to do is drop a note to OpenSource DOT OpenVMS AT |
From @cpansproutOn Fri Sep 27 03:59:08 2013, Hugmeir wrote:
Is there any chance of backporting that to 5.18 and 5.16? -- Father Chrysostomos |
From @HugmeirOn Sat, Sep 28, 2013 at 10:31 AM, Father Chrysostomos via RT <
+1 to the cherrypick. This is a regression from 5.14; the commit applies |
From @rjbs* Brian Fraser <fraserbn@gmail.com> [2013-09-28T09:45:14]
Please apply them the to maint branches. I'll be posting about 5.18.2 soon. -- |
From @nwc10On Fri, Sep 27, 2013 at 09:01:51AM -0500, Craig A. Berry wrote:
I've run it on a 32 bit system that only has 16M of real RAM and 96M of swap. (No-one said that it was *fast*. Nor was it quiet. But it completed) You could probably build Perl 5 natively on most smartphones released in the Nicholas Clark |
From @craigberryOn Fri, Sep 27, 2013 at 5:58 AM, Brian Fraser <fraserbn@gmail.com> wrote:
Actually it wasn't in facf34e, but it now is in 43ed1b7 :-). |
From @ppisarDne Wed, 01 May 2013 04:55:31 -0700, trast@inf.ethz.ch napsal(a):
It looks like the facf34e fix for this issue broke "forks" module <https://rt.cpan.org/Public/Bug/Display.html?id=123248>. A forked child crashes in threads->isthread() now. |
Migrated from rt.perl.org#119897 (status was 'resolved')
Searchable as RT119897$