miniperl fails with SIGBUS on sparc (usethreads+use64bitint) #12978
Comments
From @ntyniThis is a bug report for perl from Niko Tyni <ntyni@debian.org>, [ Sorry about the timing, it took a week for our sparc build server to get 5.18.0 build fails on sparc with -Dusethreads -Duse64bitint. ./miniperl -w -Ilib -MExporter -e '<?>' || /usr/bin/make minitest An example backtrace: This is also Perlbugging this with a non-threaded build. All the tests pass with that. ./Configure -Doptimize="-g -O0" -des -Dusethreads -Duse64bitint and both -Dusethreads -Duse64bitint are needed to trigger it. Seen with gcc 4.4, 4.6 and 4.8, at both -O0 and -O2. Bisecting says the first bad commit is commit 8be227a CV-based slab allocation for ops Flags: Site configuration information for perl 5.18.0: Configured by ntyni at Sun May 19 16:39:10 UTC 2013. Summary of my perl5 (revision 5 version 18 subversion 0) configuration: Locally applied patches: @INC for perl 5.18.0: Environment for perl 5.18.0: |
From @LeontOn Sun, May 19, 2013 at 9:50 PM, Niko Tyni <perlbug-followup@perl.org> wrote:
This smells like an alignment issue.
That makes perfect sense with my theory. Probably that allocator Leon |
|
The RT System itself - Status changed from 'new' to 'open' |
From @nwc10On Mon, May 20, 2013 at 05:11:38AM +0200, Leon Timmermans wrote:
Agree. Been caught out by these before.
I think it needs to be to the alignment of the largest thing in an OP. struct pmop { But really, that "IV" should be something else. "STRLEN", I think, or even We can't change that for 5.18.x. But as well as the alignment thing, we Nicholas Clark |
From PeterCMartini@GMail.comOn Sun, May 19, 2013 at 3:50 PM, Niko Tyni <perlbug-followup@perl.org> wrote:
Interesting - I don't get the same result when building with those -bash-3.2$ gcc --version -bash-3.2$ uname -a I'm rebuilding fresh now to see if I can get it to choke, and will
|
From PeterCMartini@GMail.comFinished smoking on my Solaris 10 / SPARC box, and the same config I'll see if I can get debian installed and running on this same
All tests successful. Summary of my perl5 (revision 5 version 18 subversion 0) configuration: Characteristics of this binary (from libperl): |
From @nwc10On Mon, May 20, 2013 at 08:19:42AM +0100, Nicholas Clark wrote:
I can replicate this on one of the sparc linux machines on the GCC compile sizeof(struct op) = 20 __alignof__(struct op) = 4 I get identical output from a mips machine, but it doesn't BUS ERROR. I'm thinking about the least worst way to fix this. I notice that 1) almost nothing on CPAN uses the macros NewOp() and NewOpSz() So I infer that no code out there calls the slab allocator for arbitrary So I'm wondering if that's the way to go for maint-5.18 Whilst it's cleaner, I'm wary of the idea of aligning everything to 8 Nicholas Clark |
From olga.kryzhanovska@gmail.comAll RISC architectures prefer the natural alignment of data types, On Solaris/SPARC the Sun Studio compiler always uses the fast Olga On Tue, May 21, 2013 at 8:14 PM, Nicholas Clark <nick@ccl4.org> wrote:
-- |
From @doughera88On Tue, 21 May 2013, Nicholas Clark wrote:
I was able to replicate it on an old Sparc Solaris system. It compiles
Yes, I suspect so.
It could perhaps be protected by #ifdef __sparc__/#endif conditionals. I may be able to look at this late next week. -- |
From @nwc10On Tue, May 21, 2013 at 05:02:38PM -0400, Andy Dougherty wrote:
Interesting. So newer gcc is generating instructions that require alignment,
I don't think that this is correct, as I've read that mips can enable SIGBUS
I hope to have something sooner than that. Nicholas Clark |
From @doughera88On Wed, 22 May 2013, Nicholas Clark wrote:
Old just means it's all I have handy. (Specifically UltraSparc 10,
That would be great. I can test things sooner, but not create them. -- |
From @ntyniOn Wed, May 22, 2013 at 12:33:28PM +0100, Nicholas Clark wrote:
Earlier cases of similar alignment problems on sparc that I've seen
FWIW ARM also has (probably different) alignment requirements, but http://jsolano.net/2012/09/06/arm-unaligned-data-access-and-floating-point-in-linux/ I don't think there's a similar sysctl for Sparc / Linux (or Solaris
Thanks for looking at this. I'm happy to test anything you come up with. |
From @nwc10On Wed, May 22, 2013 at 10:39:53PM +0300, Niko Tyni wrote:
I had an insight on the tram. All sorts of complex hacks are complex...
Try this. It's against blead. The comment in the diff hunk will conflict Inline Patchdiff --git a/op.c b/op.c
index 96a06b1..341b087 100644
--- a/op.c
+++ b/op.c
@@ -175,6 +175,19 @@ Perl_Slab_Alloc(pTHX_ size_t sz)
|| (CvSTART(PL_compcv) && !CvSLABBED(PL_compcv)))
return PerlMemShared_calloc(1, sz);
+#if defined(USE_ITHREADS) && IVSIZE > U32SIZE
+ /* Work around a goof with alignment on our part. For sparc32 (and
+ possibly other architectures), if built with -Duse64bitint, the IV
+ op_pmoffset in struct pmop should be 8 byte aligned, but the slab
+ allocator is only providing 4 byte alignment. The real fix is to change
+ the IV to a type the same size as a pointer, such as size_t, but we
+ can't do that without breaking the ABI, which is a no-no in a maint
+ release. So instead, simply allocate struct pmop directly, which will be
+ suitably aligned: */
+ if (sz == sizeof(struct pmop))
+ return PerlMemShared_calloc(1, sz);
+#endif
+
/* While the subroutine is under construction, the slabs are accessed via
CvSTART(), to avoid needing to expand PVCV by one pointer for something
unneeded at runtime. Once a subroutine is constructed, the slabs are
Would be nice to test this with & without -Duse64bitint on as many Nicholas Clark |
From @doughera88On Wed, 22 May 2013, Nicholas Clark wrote:
I like that. It's clean and clever, but not too clever. So far, it built for me on Solaris/SPARC with gcc-4.6.0 -Dusethreads -Duse64bitint On the first two, all tests passed except op/threads.t. Rerunning just On the third one, it's in the middle of building extensions. This is slow -- |
From @ntyniOn Wed, May 22, 2013 at 01:26:03PM -0700, Nicholas Clark via RT wrote:
I tested with v5.18.0 and a "backported" version of the patch. All tests armhf (ARM hard-float, 32bit) I'm attaching 'perl -V' output for all of those in case someone is curious. |
From @ntyniSummary of my perl5 (revision 5 version 18 subversion 0) configuration: Characteristics of this binary (from libperl): Characteristics of this binary (from libperl): |
From @ntyniSummary of my perl5 (revision 5 version 18 subversion 0) configuration: Characteristics of this binary (from libperl): Characteristics of this binary (from libperl): |
From @ntyniSummary of my perl5 (revision 5 version 18 subversion 0) configuration: Characteristics of this binary (from libperl): Characteristics of this binary (from libperl): |
From @ntyniSummary of my perl5 (revision 5 version 18 subversion 0) configuration: Characteristics of this binary (from libperl): Characteristics of this binary (from libperl): |
From @ntyniSummary of my perl5 (revision 5 version 18 subversion 0) configuration: Characteristics of this binary (from libperl): Characteristics of this binary (from libperl): |
From @ntyniSummary of my perl5 (revision 5 version 18 subversion 0) configuration: Characteristics of this binary (from libperl): Characteristics of this binary (from libperl): |
From @ntyniSummary of my perl5 (revision 5 version 18 subversion 0) configuration: Characteristics of this binary (from libperl): Characteristics of this binary (from libperl): |
From @ntyniSummary of my perl5 (revision 5 version 18 subversion 0) configuration: Characteristics of this binary (from libperl): Characteristics of this binary (from libperl): |
From @nwc10On Fri, May 24, 2013 at 10:30:09AM +0300, Niko Tyni wrote:
Thanks. It also passed on everything I tried it on. I've pushed it to blead. Sadly the two available sparc machines at the GCC compile farm are scheduled Nicholas Clark |
From @doughera88On Fri, 24 May 2013, Nicholas Clark wrote:
Thank you. So far it's working fine for me and I anticipate no problems. Meanwhile, what would you think about tweaking it like this: Inline Patchdiff --git a/op.c b/op.c
index 792e8d6..fd69c56 100644
--- a/op.c
+++ b/op.c
@@ -175,7 +175,7 @@ Perl_Slab_Alloc(pTHX_ size_t sz)
|| (CvSTART(PL_compcv) && !CvSLABBED(PL_compcv)))
return PerlMemShared_calloc(1, sz);
-#if defined(USE_ITHREADS) && IVSIZE > U32SIZE
+#if defined(USE_ITHREADS) && IVSIZE > U32SIZE && !defined(__x86_64__)
/* Work around a goof with alignment on our part. For sparc32 (and
possibly other architectures), if built with -Duse64bitint, the IV
op_pmoffset in struct pmop should be 8 byte aligned, but the slab
my $i = "abcdefg"; my $k; ran slightly quicker on x86_64 (differences were consistently at the 2-3
That would be nice. I can continue to test on SPARC, but the machine -- |
From @nwc10On Sat, May 25, 2013 at 10:57:47AM -0400, Andy Dougherty wrote:
Good point. But I think that that's more restrictive than it needs to be. #if defined(USE_ITHREADS) && IVSIZE > U32SIZE && IVSIZE > PTRSIZE (but I have not yet tested it)
and would avoid that cost on other 64 bit platforms.
That's a test doing work at runtime, which is affected by a compile-time Nicholas Clark |
From @doughera88On Sat, 25 May 2013, Nicholas Clark wrote:
Yes, that would catch the relevant SPARC case (-Duse64bitint on an
Correct. With current blead on Linux x86_64, every op allocation still if(sz == sizeof(struct pmop) and even though the test is always false for that program, the cummulative Anyway, it works as is. I think your suggested change should be an -- |
From @craigberryOn Sat, May 25, 2013 at 10:04 AM, Nicholas Clark <nick@ccl4.org> wrote:
FWIW, this would be true on VMS Alpha or Itanium configured with |
From @nwc10On Sat, May 25, 2013 at 10:41:50AM -0500, Craig A. Berry wrote:
Meaning that right now 5.18.0 built on VMS Alpha or Itanium with Nicholas Clark |
From @nwc10On Sat, May 25, 2013 at 11:26:40AM -0400, Andrew Dougherty wrote:
I pushed this tweak as smoke-me/nicholas/rt-118055-1 commit f61a27cade956988ec6c5ff53b8aff3492ca5839 Improved struct pmop alignment fix - avoid the slow path on 64 bit systems. Inline Patchdiff --git a/op.c b/op.c
index 792e8d6..c5dbb68 100644
--- a/op.c
+++ b/op.c
@@ -175,7 +175,7 @@ Perl_Slab_Alloc(pTHX_ size_t sz)
|| (CvSTART(PL_compcv) && !CvSLABBED(PL_compcv)))
return PerlMemShared_calloc(1, sz);
-#if defined(USE_ITHREADS) && IVSIZE > U32SIZE
+#if defined(USE_ITHREADS) && IVSIZE > U32SIZE && IVSIZE > PTRSIZE
/* Work around a goof with alignment on our part. For sparc32 (and
possibly other architectures), if built with -Duse64bitint, the IV
op_pmoffset in struct pmop should be 8 byte aligned, but the slab
With the change as shown, that code is no longer compiled on any platform with I tested 4 configurations on Sparc, Mips and Arm Linux, and all were (still) Nicholas Clark |
From @craigberryOn Mon, May 27, 2013 at 9:08 AM, Nicholas Clark <nick@ccl4.org> wrote:
Sorry, I was responding to that one line of code and not the context of the Just out of paranoia I did a build and run of the test suite for the blead 2013-05-27.15:45:01 24ee355 smoke-me/nicholas/rt-118055-1 2013-05-26.06:50:17 and the build and test times on an otherwise-idle Itanium system were |
From @nwc10On Mon, May 27, 2013 at 05:13:13PM -0500, Craig A. Berry wrote:
That's useful, but what I'd hope for, given that the initial version of the So it works on VMS Itanium. Which is good. But I'm sort of curious, is it Actually, "state of tests for v5.18.0 on VMS" would be useful to know. Nicholas Clark |
From @doughera88On Sat, 25 May 2013, Nicholas Clark wrote:
I know it's already been pushed and tested elsewhere, but I'm happy to gcc-4.6.0 gcc-4.1.0 -Dusethreads -Duse64bitint gcc-3.4.3 -Dusethreads -Duse64bitint cc -Dusethreads Nicely done. I think this (along with the corresponding comment patches, -- |
From @craigberryOn Tue, May 28, 2013 at 3:10 AM, Nicholas Clark <nick@ccl4.org> wrote:
Is it not c2a50dd that would be the cut-off point? $ git log --oneline -n500 | grep -C2 c2a50dd Based on that assumption I built blead@1a72e16 and the full build and run Actually, "state of tests for v5.18.0 on VMS" would be useful to know. Working on it. |
From @craigberryOn Wed, May 29, 2013 at 7:41 AM, Craig A. Berry <craig.a.berry@gmail.com>wrote:
VMS smoke reports are now working again: http://www.nntp.perl.org/group/perl.daily-build.reports/2013/05/msg143101.html That's blead. There were a few more Module::Build failures in 5.18.0 |
From @rjbs* "Craig A. Berry" <craig.a.berry@gmail.com> [2013-05-30T22:45:37]
\o/ Great news, thanks very much, Craig! -- |
From @nwc10On Fri, May 31, 2013 at 07:27:16AM -0400, Ricardo Signes wrote:
\o/ Nicholas Clark |
From @iabynOn Mon, May 27, 2013 at 03:20:40PM +0100, Nicholas Clark wrote:
I've just cherry-picked c2a50dd into maint-5.18 as However, I'm not sure of the status of the smoke-me/nicholas/rt-118055-1 -- |
From @jmdhOn Sat, Jun 15, 2013 at 06:15:36AM -0700, Dave Mitchell via RT wrote:
F
We have applied http://perl5.git.perl.org/perl.git/commit/f61a27cade956988ec6c5ff53b8aff3492ca5839 (from smoke-me/nicholas/rt-118055-1, although for some reason gitweb Dominic. |
From @ntyniOn Sat, Jun 15, 2013 at 02:14:37PM +0100, Dave Mitchell wrote:
Thanks to both of you!
FWIW we applied them both for Debian "experimental" 5.18.0 packages, I'm pretty sure I tested f61a27cade95698 manually on sparc and it was http://buildd.debian.org/status/package.php?p=perl&suite=experimental (the latter is for architectures that are not yet or no longer part of |
From @jmdhOn Sat, Jun 15, 2013 at 04:35:33PM +0300, Niko Tyni wrote:
The build results for sparc are now in and it the build is succeeding https://buildd.debian.org/status/fetch.php?pkg=perl&arch=sparc&ver=5.18.0-3&stamp=1371738826 Dominic |
From @iabynOn Sun, Jun 23, 2013 at 12:29:05PM +0100, Dominic Hargreaves wrote:
I've now cherry-picked the improved fix into maint-5.18. -- |
From @jmdhOn Mon, Jun 24, 2013 at 02:28:59PM -0700, Dave Mitchell via RT wrote:
Yes, I believe so - thank you! Dominic. |
From @cpansproutOn Mon Jun 24 15:23:27 2013, dom wrote:
Does this remaining issue change that? On Mon May 20 00:20:46 2013, nicholas wrote:
We usually use PADOFFSET for array indices, but whatever.
Any reason I shouldn�t change the IV in blead and remove the workaround -- Father Chrysostomos |
From @cpansproutOn Mon Jun 24 18:16:21 2013, sprout wrote:
I have done that in commit 784e50c. -- Father Chrysostomos |
|
@cpansprout - Status changed from 'open' to 'resolved' |
From @nwc10On Sat, Jul 06, 2013 at 06:01:52PM -0700, Father Chrysostomos via RT wrote:
Thanks. PADOFFSET looks like a better idea than the other types I suggested. Sorry for the delay in replying. Been away and too much going on. Trying to Nicholas Clark |
Migrated from rt.perl.org#118055 (status was 'resolved')
Searchable as RT118055$
The text was updated successfully, but these errors were encountered: