Re: perl_5.32.0~rc1-1 FTBFS on m68k (experimental)
Hi Adrian,
On Thu, Jun 18, 2020 at 10:46 PM John Paul Adrian Glaubitz
wrote:
> On 6/18/20 12:10 PM, John Paul Adrian Glaubitz wrote:
> > Reported upstream [1].
>
> It's an alignment issue and can be trivially fixed with this patch:
>
> diff --git a/op.h b/op.h
> index fc21f03cda..480c95245b 100644
> --- a/op.h
> +++ b/op.h
> @@ -698,7 +698,7 @@ struct opslot {
> U16 opslot_size;/* size of this slot (in pointers) */
> U16 opslot_offset; /* offset from start of slab (in ptr
> units) */
> OP opslot_op; /* the op itself */
> -};
> +} __attribute__ ((aligned (4)));
>
> struct opslab {
> OPSLAB * opslab_next;/* next slab */
In the mean time, you changed this to add explicit padding instead:
https://github.com/Perl/perl5/issues/17871
> diff --git a/op.h b/op.h
> index fc21f03cda..fb9f538e23 100644
> --- a/op.h
> +++ b/op.h
> @@ -714,6 +714,7 @@ struct opslab {
> # ifdef PERL_DEBUG_READONLY_OPS
> bool opslab_readonly;
> # endif
> +U16 opslab_padding;/* padding to ensure proper
> alignment */
> OPSLOT opslab_slots; /* slots begin here */
> };
I take it PERL_DEBUG_READONLY_OPS is enabled?
Hence the padding should be moved inside the #ifdef,
Furthermore, sizeof(bool) = 1, right? So you still have an implicit
hole, and it would be better to add 3 bytes of explicit padding
instead one 16-bit quantity.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: perl_5.32.0~rc1-1 FTBFS on m68k (experimental)
> On Jun 19, 2020, at 10:02 AM, Geert Uytterhoeven wrote:
>
> Hi Adrian,
>
>> On Thu, Jun 18, 2020 at 10:46 PM John Paul Adrian Glaubitz
>> wrote:
>>> On 6/18/20 12:10 PM, John Paul Adrian Glaubitz wrote:
>>> Reported upstream [1].
>>
>> It's an alignment issue and can be trivially fixed with this patch:
>>
>> diff --git a/op.h b/op.h
>> index fc21f03cda..480c95245b 100644
>> --- a/op.h
>> +++ b/op.h
>> @@ -698,7 +698,7 @@ struct opslot {
>> U16 opslot_size;/* size of this slot (in pointers) */
>> U16 opslot_offset; /* offset from start of slab (in ptr
>> units) */
>> OP opslot_op; /* the op itself */
>> -};
>> +} __attribute__ ((aligned (4)));
>>
>> struct opslab {
>> OPSLAB * opslab_next;/* next slab */
>
> In the mean time, you changed this to add explicit padding instead:
>
> https://github.com/Perl/perl5/issues/17871
>
>> diff --git a/op.h b/op.h
>> index fc21f03cda..fb9f538e23 100644
>> --- a/op.h
>> +++ b/op.h
>> @@ -714,6 +714,7 @@ struct opslab {
>> # ifdef PERL_DEBUG_READONLY_OPS
>> bool opslab_readonly;
>> # endif
>> +U16 opslab_padding;/* padding to ensure proper
>> alignment */
>> OPSLOT opslab_slots; /* slots begin here */
>> };
>
> I take it PERL_DEBUG_READONLY_OPS is enabled?
> Hence the padding should be moved inside the #ifdef,
> Furthermore, sizeof(bool) = 1, right? So you still have an implicit
> hole, and it would be better to add 3 bytes of explicit padding
> instead one 16-bit quantity.
No, I didn’t take that #ifdef into consideration.
And I’m confused now.
Add the three bytes how? Inside the #ifdef makes no sense as that wouldn’t fix
release builds.
Adrian
Re: perl_5.32.0~rc1-1 FTBFS on m68k (experimental)
Hi Adrian,
On Fri, Jun 19, 2020 at 10:20 AM John Paul Adrian Glaubitz
wrote:
> > On Jun 19, 2020, at 10:02 AM, Geert Uytterhoeven
> > wrote:
> >> On Thu, Jun 18, 2020 at 10:46 PM John Paul Adrian Glaubitz
> >> wrote:
> >>> On 6/18/20 12:10 PM, John Paul Adrian Glaubitz wrote:
> >>> Reported upstream [1].
> >>
> >> It's an alignment issue and can be trivially fixed with this patch:
> >>
> >> diff --git a/op.h b/op.h
> >> index fc21f03cda..480c95245b 100644
> >> --- a/op.h
> >> +++ b/op.h
> >> @@ -698,7 +698,7 @@ struct opslot {
> >> U16 opslot_size;/* size of this slot (in pointers) */
> >> U16 opslot_offset; /* offset from start of slab (in ptr
> >> units) */
> >> OP opslot_op; /* the op itself */
> >> -};
> >> +} __attribute__ ((aligned (4)));
> >>
> >> struct opslab {
> >> OPSLAB * opslab_next;/* next slab */
> >
> > In the mean time, you changed this to add explicit padding instead:
> >
> > https://github.com/Perl/perl5/issues/17871
> >
> >> diff --git a/op.h b/op.h
> >> index fc21f03cda..fb9f538e23 100644
> >> --- a/op.h
> >> +++ b/op.h
> >> @@ -714,6 +714,7 @@ struct opslab {
> >> # ifdef PERL_DEBUG_READONLY_OPS
> >> bool opslab_readonly;
> >> # endif
> >> +U16 opslab_padding;/* padding to ensure
> >> proper alignment */
> >> OPSLOT opslab_slots; /* slots begin here */
> >> };
> >
> > I take it PERL_DEBUG_READONLY_OPS is enabled?
> > Hence the padding should be moved inside the #ifdef,
> > Furthermore, sizeof(bool) = 1, right? So you still have an implicit
> > hole, and it would be better to add 3 bytes of explicit padding
> > instead one 16-bit quantity.
>
> No, I didn’t take that #ifdef into consideration.
>
> And I’m confused now.
>
> Add the three bytes how? Inside the #ifdef makes no sense as that wouldn’t
> fix release builds.
So release builds fail, too?
And they don't set PERL_DEBUG_READONLY_OPS?
If PERL_DEBUG_READONLY_OPS is not set, I see no reason why
the padding would be needed.
Disclaimer: looking at the 5.26 version from Ubuntu, as github seems
to be down.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: perl_5.32.0~rc1-1 FTBFS on m68k (experimental)
On 6/19/20 10:42 AM, Geert Uytterhoeven wrote: > So release builds fail, too? Well, the build fails during the stage1 build. > And they don't set PERL_DEBUG_READONLY_OPS? > > If PERL_DEBUG_READONLY_OPS is not set, I see no reason why > the padding would be needed. It might be set during stage1, but not stage2. I will have to look at it again. Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer - glaub...@debian.org `. `' Freie Universitaet Berlin - glaub...@physik.fu-berlin.de `-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Re: perl_5.32.0~rc1-1 FTBFS on m68k (experimental)
On 6/19/20 10:47 AM, John Paul Adrian Glaubitz wrote:
> It might be set during stage1, but not stage2.
>
> I will have to look at it again.
I just tried this change and it doesn't fix the problem:
diff --git a/op.h b/op.h
index fc21f03cda..ca3102d7d2 100644
--- a/op.h
+++ b/op.h
@@ -713,6 +713,8 @@ struct opslab {
units) */
# ifdef PERL_DEBUG_READONLY_OPS
bool opslab_readonly;
+U16 _padding1; /* additional padding to ensure
opslab is 32-bit aligned */
+U8 _padding2; /* when PERL_DEBUG_READONLY_OPS is
set */
# endif
OPSLOT opslab_slots; /* slots begin here */
};
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - glaub...@debian.org
`. `' Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
`-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Re: perl_5.32.0~rc1-1 FTBFS on m68k (experimental)
Hi Adrian,
On Fri, Jun 19, 2020 at 10:52 AM John Paul Adrian Glaubitz
wrote:
> On 6/19/20 10:47 AM, John Paul Adrian Glaubitz wrote:
> > It might be set during stage1, but not stage2.
> >
> > I will have to look at it again.
>
> I just tried this change and it doesn't fix the problem:
That's actually expected ;-)
> --- a/op.h
> +++ b/op.h
> @@ -713,6 +713,8 @@ struct opslab {
> units) */
> # ifdef PERL_DEBUG_READONLY_OPS
> bool opslab_readonly;
implicit padding of 1 byte
> +U16 _padding1; /* additional padding to ensure
> opslab is 32-bit aligned */
> +U8 _padding2; /* when PERL_DEBUG_READONLY_OPS is
> set */
implicit padding of 1 byte
> # endif
> OPSLOT opslab_slots; /* slots begin here */
> };
Either invert the order of the two paddings, or replace them by a single one:
U8 _padding[3];
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: perl_5.32.0~rc1-1 FTBFS on m68k (experimental)
On 6/19/20 11:03 AM, Geert Uytterhoeven wrote:
> Either invert the order of the two paddings, or replace them by a single one:
>
> U8 _padding[3];
I just tried this variant and it didn't help:
diff --git a/op.h b/op.h
index fc21f03cda..703be6b3f2 100644
--- a/op.h
+++ b/op.h
@@ -713,6 +713,7 @@ struct opslab {
units) */
# ifdef PERL_DEBUG_READONLY_OPS
bool opslab_readonly;
+U8 _padding[3];/* additional padding to ensure opslab
is 32-bit aligned */
# endif
OPSLOT opslab_slots; /* slots begin here */
};
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - glaub...@debian.org
`. `' Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
`-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Re: perl_5.32.0~rc1-1 FTBFS on m68k (experimental)
Le 19/06/2020 à 11:08, John Paul Adrian Glaubitz a écrit :
> On 6/19/20 11:03 AM, Geert Uytterhoeven wrote:
>> Either invert the order of the two paddings, or replace them by a single one:
>>
>> U8 _padding[3];
> I just tried this variant and it didn't help:
>
> diff --git a/op.h b/op.h
> index fc21f03cda..703be6b3f2 100644
> --- a/op.h
> +++ b/op.h
> @@ -713,6 +713,7 @@ struct opslab {
> units) */
> # ifdef PERL_DEBUG_READONLY_OPS
> bool opslab_readonly;
> +U8 _padding[3];/* additional padding to ensure
> opslab is 32-bit aligned */
> # endif
> OPSLOT opslab_slots; /* slots begin here */
> };
>
> Adrian
>
Can't you move "opslab_slots" before "# ifdef PERL_DEBUG_READONLY_OPS"?
Thanks,
Laurent
Re: perl_5.32.0~rc1-1 FTBFS on m68k (experimental)
Hi Laurent!
On 6/19/20 11:33 AM, Laurent Vivier wrote:
>
> Can't you move "opslab_slots" before "# ifdef PERL_DEBUG_READONLY_OPS"?
I just tried this variant, it also crashes:
diff --git a/op.h b/op.h
index fc21f03cda..69f74843bf 100644
--- a/op.h
+++ b/op.h
@@ -711,10 +711,12 @@ struct opslab {
U16 opslab_free_space; /* space available in this slab
for allocating new ops (in ptr
units) */
+OPSLOT opslab_slots; /* slots begin here */
# ifdef PERL_DEBUG_READONLY_OPS
bool opslab_readonly;
+U8 _padding[3];
# endif
-OPSLOT opslab_slots; /* slots begin here */
+
};
# define OPSLOT_HEADER STRUCT_OFFSET(OPSLOT, opslot_op)
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - glaub...@debian.org
`. `' Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
`-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Re: perl_5.32.0~rc1-1 FTBFS on m68k (experimental)
Hi!
This works, both with PERL_DEBUG_READONLY_OPS and without:
diff --git a/op.h b/op.h
index fc21f03cda..fc7e73fef4 100644
--- a/op.h
+++ b/op.h
@@ -713,7 +713,9 @@ struct opslab {
units) */
# ifdef PERL_DEBUG_READONLY_OPS
bool opslab_readonly;
+U8 opslab_padding1[3];
# endif
+U16opslab_padding2;
OPSLOT opslab_slots; /* slots begin here */
};
Note that sizeof(OPSLOT) is 24.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - glaub...@debian.org
`. `' Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
`-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Re: perl_5.32.0~rc1-1 FTBFS on m68k (experimental)
Hi!
On 6/19/20 12:08 PM, John Paul Adrian Glaubitz wrote:
> This works, both with PERL_DEBUG_READONLY_OPS and without:
Updated patch attached.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - glaub...@debian.org
`. `' Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
`-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
>From 65bed710c6b990285d7feae63e8e9b8c6f8ab8ec Mon Sep 17 00:00:00 2001
From: John Paul Adrian Glaubitz
Date: Fri, 19 Jun 2020 12:30:57 +0200
Subject: [PATCH] op.h: Add additional padding to struct opslab to ensure
proper alignment
On m68k, the natural alignment is 16 bits which causes the opslab_opslot
member of "struct opslab" to be aligned at a 16-bit offset. Other 32-bit
and 64-bit architectures have a natural alignment of at least 32 bits, so
the offset is always guaranteed to be at least 32-bit-aligned.
Fix this by adding additional padding bytes before the opslab_opslot
member, both for cases when PERL_DEBUG_READONLY_OPS defined and not
defined to ensure the offset of oplab_slots is always 32-bit-aligned.
On architectures which have a natural alignment of at least 32 bits,
the padding does not affect the alignment, offsets or struct size.
---
op.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/op.h b/op.h
index fc21f03cda..b620983d38 100644
--- a/op.h
+++ b/op.h
@@ -713,7 +713,9 @@ struct opslab {
units) */
# ifdef PERL_DEBUG_READONLY_OPS
bool opslab_readonly;
+U8 opslab_padding1[3]; /* padding to ensure that opslab_slots is always */
# endif
+U16 opslab_padding2; /* located at an offset with 32-bit alignment */
OPSLOT opslab_slots; /* slots begin here */
};
--
2.27.0
Re: perl_5.32.0~rc1-1 FTBFS on m68k (experimental)
Hi Adrian,
On Fri, Jun 19, 2020 at 12:08 PM John Paul Adrian Glaubitz
wrote:
> This works, both with PERL_DEBUG_READONLY_OPS and without:
>
> diff --git a/op.h b/op.h
> index fc21f03cda..fc7e73fef4 100644
> --- a/op.h
> +++ b/op.h
> @@ -713,7 +713,9 @@ struct opslab {
> units) */
> # ifdef PERL_DEBUG_READONLY_OPS
> bool opslab_readonly;
> +U8 opslab_padding1[3];
> # endif
> +U16opslab_padding2;
> OPSLOT opslab_slots; /* slots begin here */
> };
So now you have 5 bytes of padding if PERL_DEBUG_READONLY_OPS,
while I guess 1 would be sufficient?
What about
# ifdef PERL_DEBUG_READONLY_OPS
bool opslab_readonly;
U8 opslab_padding;
#else
U16opslab_padding;
# endif
?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: perl_5.32.0~rc1-1 FTBFS on m68k (experimental)
Hi Geert! On 6/19/20 1:19 PM, Geert Uytterhoeven wrote: > So now you have 5 bytes of padding if PERL_DEBUG_READONLY_OPS, > while I guess 1 would be sufficient? > > What about > > # ifdef PERL_DEBUG_READONLY_OPS > bool opslab_readonly; > U8 opslab_padding; > #else > U16opslab_padding; > # endif > > ? Yes, that's a good idea and that also works as expected. I'll use that. Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer - glaub...@debian.org `. `' Freie Universitaet Berlin - glaub...@physik.fu-berlin.de `-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Re: Bug#963108: perl: Please include minor patch to fix FTBFS on m68k
On 6/19/20 8:37 AM, John Paul Adrian Glaubitz wrote:
> On 6/19/20 8:22 AM, John Paul Adrian Glaubitz wrote:
>> The attached patch fixes the problem by adding an additional 16 bits padding
>> before the opslot member which causes the alignment of opslot to be 32 bits.
>
> Attaching a patch with a better commit message for explanation.
Third version of the patch which includes Laurent's and Geert's feedback and
which
is hopefully the version that gets merged upstream.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - glaub...@debian.org
`. `' Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
`-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
>From 89acf85c3c8943081b5dcd5d3c8bcc2384d8f8b7 Mon Sep 17 00:00:00 2001
From: John Paul Adrian Glaubitz
Date: Fri, 19 Jun 2020 16:40:38 +0200
Subject: [PATCH] op.h: Add additional padding to struct opslab to ensure
proper alignment
On m68k, the natural alignment is 16 bits which causes the opslab_opslot
member of struct opslab to be aligned at a 16-bit offset. Other 32-bit
and 64-bit architectures have a natural alignment of at least 32 bits, so
the offset is always guaranteed to be at least 32-bit-aligned.
Fix this by adding additional padding bytes before the opslab_opslot
member, both for cases when PERL_DEBUG_READONLY_OPS defined and not
defined to ensure the offset of oplab_slots is always 32-bit-aligned.
On architectures which have a natural alignment of at least 32 bits,
the padding does not affect the alignment, offsets or struct size.
---
AUTHORS | 1 +
op.h| 3 +++
2 files changed, 4 insertions(+)
diff --git a/AUTHORS b/AUTHORS
index 1a4680027f..e4ea405d98 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -651,6 +651,7 @@ John Macdonald
John Malmberg
John Nolan
John P. Linderman
+John Paul Adrian Glaubitz
John Peacock
John Pfuntner
John Poltorak
diff --git a/op.h b/op.h
index fc21f03cda..b9f6da82c9 100644
--- a/op.h
+++ b/op.h
@@ -713,6 +713,9 @@ struct opslab {
units) */
# ifdef PERL_DEBUG_READONLY_OPS
bool opslab_readonly;
+U8 opslab_padding; /* padding to ensure that opslab_slots is always */
+# else
+U16 opslab_padding; /* located at an offset with 32-bit alignment */
# endif
OPSLOT opslab_slots; /* slots begin here */
};
--
2.27.0
