Alignment fault in perl on 64-bit OpenBSD octeon with clang

[afresh1]

Description

miniperl started crashing on OpenBSD octeon after clang's default changed to -fno-common.

Steps to Reproduce

visa@openbsd tracked down the specific variables involved, and deraadt@openbsd spent a full day inside perl.

With a small amount of work I (afresh1@openbsd) can get a machine set up for remote access that has these issues if someone needs access. I am available as afresh1 on IRC as well.

The following report was provided by deraadt@:

Take note of these two variables decls.

PERLVARA(G, hash_seed, PERL_HASH_SEED_BYTES, unsigned char) /* perl.c and hv.h */
#if defined(PERL_HASH_STATE_BYTES)
PERLVARA(G, hash_state, PERL_HASH_STATE_BYTES, unsigned char) /* perl.c and hv.h */
#endif

These produce PL_hash_seed and PL_hash_state variables.

In the majority of perl files the macros creates extern decls. In globals.o, the macros create storage instead. The storage is not just PERL_HASH_SEED_BYTES & PERL_HASH_STATE_BYTES bytes -- it is more complicated.

Since we switched to -fno-common model in clang by default, the seed and state variables have confirmed scope and can go directy into BSS in globals.o rather than being COMMON, and then having their placement be resolved at link-time. At link-time, they were being sorted and padded conservatively and ended up being aligned by pure luck.

Now that the compiler can decide to put them into BSS in the .o, they end up adjacent. Since PL_hash_seed has an awkward size, PL_hash_state thus is misaligned.

Observe,

00000de8 B PL_hash_seed
00000e04 B PL_hash_state

PL_hash_state is not at a 64bit aligned address. This is because both arrays are defined as char[], andPL_hash_seed does not have 64-bit aligned sizing.

In clang-generated assembly, the storage looks like this:

        .type   PL_hash_seed,@object    # @PL_hash_seed
        .globl  PL_hash_seed
PL_hash_seed:
        .space  28
        .size   PL_hash_seed, 28

        .type   PL_hash_state,@object   # @PL_hash_state
        .globl  PL_hash_state
PL_hash_state:
        .space  24612
        .size   PL_hash_state, 24612

The 28-byte PL_hash_seed allows the following object, which only has char[] alignment requirement, to become misaligned.

This is the cpp expansion of the storage allocating macros

 _Bool PL_hash_seed_set = (0);
 unsigned char PL_hash_seed[( 16 + (int)( 3 * sizeof(U32) ) )];

 unsigned char PL_hash_state[( 32 + ( ( 1 + ( 256 * 24 ) ) * sizeof(U32) ) )];

I tried to change the macros to correctly typed objects, but there are []-indexes and such happening throughout the hash macros & inline functions which would require more typecasts. That direction does not seem sane.

I also looked into using __atttribute__(__aligned__), but it fits poorly.

I don't see a way to fit the seed object into a union with a uint64_t, a bunch of other objects would also need the same union, since there is a lot of shared code.

A final idea is to over-allocate the seed storage, such that it is aligned. Or this could even use a roundup() macro?

Original patch that changed the 3 to a 4 to correct alignment on this particular architecture removed.

millert@openbsd said:

I think using a roundup() macro would be safest.

True, 12+4*32 becomes non-aligning on armv7.

This solves it for all architectures. I'm excessively rounding up, but the other perl cpp definitions (like IVSIZE) feel dirty and unsuitable.

As a 64-bit architecture, octeon chooses to use PERL_HASH_FUNC_STADTX.

There is a belief by several OpenBSD folks that the Linux kernel works around this and gcc seems to align things even without being told to, and some architectures may have ways to work around it. However these workarounds are likely very slow. kettenis@openbsd specifically points to "the DATA_ALIGNMENT macro in /usr/gnu/gcc/gcc/config/mips64.h".

Below is a patch that works around this issue for the current optimizations that llvm is applying but this is not a guaranteed fix as a change to the compiler or linker could sort things differently and break it again.

It would be best for perl to be using a correctly sized type and not relying on hacks to get the correct alignment. Deeper knowledge of the perl codebase seems to be required for that though.

There was a question where the 3 in the below code comes from as the initial patch just changed it to a 4 which worked on octeon, deraadt@ believes it is related to the SBOX32_MAX_LEN macro that was not always in scope and so this was a way to hack around that. Can you confirm?

Index: hv_func.h
===================================================================
RCS file: /cvs/src/gnu/usr.bin/perl/hv_func.h,v
retrieving revision 1.5
diff -u -p -u -r1.5 hv_func.h
--- hv_func.h   30 Dec 2019 02:13:41 -0000      1.5
+++ hv_func.h   10 Feb 2021 20:20:46 -0000
@@ -83,7 +83,8 @@

 #define _PERL_HASH_FUNC         "SBOX32_WITH_" __PERL_HASH_FUNC

-#define _PERL_HASH_SEED_BYTES   ( __PERL_HASH_SEED_BYTES + (int)( 3 * sizeof(U32) ) )
+#define _PERL_HASH_SEED_roundup(x, y)   ((((x)+((y)-1))/(y))*(y))
+#define _PERL_HASH_SEED_BYTES   ( _PERL_HASH_SEED_roundup(__PERL_HASH_SEED_BYTES + (int)( 3 * sizeof(U32)),
sizeof(U64) ) )

 #define _PERL_HASH_STATE_BYTES  \
     ( __PERL_HASH_STATE_BYTES + ( ( 1 + ( 256 * SBOX32_MAX_LEN ) ) * sizeof(U32) ) )

Expected behavior

Perl build completes without crashing.

Perl configuration

sysctl -n kern.version
OpenBSD 6.9-beta (GENERIC.MP) #464: Sat Feb  6 22:21:13 MST 2021
    deraadt@octeon.openbsd.org:/usr/src/sys/arch/octeon/compile/GENERIC.MP
tar xzf perl-5.32.1.tar.gz
cd perl-5.32.1
./Configure -de
...
make
... 
cc -Wl,-E -fstack-protector-strong -o miniperl opmini.o perlmini.o gv.o toke.o perly.o pad.o regcomp.o dump.o util.o mg.o reentr.o mro_core.o keywords.o hv.o av.o run.o pp_hot.o sv.o pp.o scope.o pp_ctl.o pp_sys.o doop.o doio.o regexec.o utf8.o taint.o deb.o universal.o globals.o perlio.o perlapi.o numeric.o mathoms.o locale.o pp_pack.o pp_sort.o caretx.o dquote.o time64.o miniperlmain.o -lm -lc
LD_LIBRARY_PATH=/home/afresh1/perl/perl-5.32.1/obj ./miniperl -w -Ilib -Idist/Exporter/lib -MExporter -e '<?>' || sh -c 'echo >&2 Failed to build miniperl. Please run make minitest; exit 1'
Bus error (core dumped)
Failed to build miniperl. Please run make minitest

[demerphq]

I will pick this up. I recommend you do NOT use STADTX_ in a distro build I recommend you use SIPHASH-1-3 or SIPHASH-2-4 instead.Both should be build options available to you. We will make STADTX deprecated in a future release.

Yves

On Thu, 11 Feb 2021 at 07:05, Andrew Fresh notifications@github.com wrote:

Description

miniperl started crashing on OpenBSD octeon after clang's default changed to -fno-common.

Steps to Reproduce

visa@openbsd tracked down the specific variables involved, and deraadt@openbsd spent a full day inside perl.

With a small amount of work I (afresh1@openbsd) can get a machine set up for remote access that has these issues if someone needs access. I am available as afresh1 on IRC as well.

The following report was provided by deraadt@:

Take note of these two variables decls https://github.com/Perl/perl5/blob/blead/perlvars.h#L266.

PERLVARA(G, hash_seed, PERL_HASH_SEED_BYTES, unsigned char) / perl.c and hv.h /

if defined(PERL_HASH_STATE_BYTES)

PERLVARA(G, hash_state, PERL_HASH_STATE_BYTES, unsigned char) / perl.c and hv.h /

endif

These produce PL_hash_seed and PL_hash_state variables.

In the majority of perl files the macros creates extern decls. In globals.o, the macros create storage instead. The storage is not just PERL_HASH_SEED_BYTES & PERL_HASH_STATE_BYTES bytes -- it is more complicated.

Since we switched to -fno-common model in clang by default, the seed and state variables have confirmed scope and can go directy into BSS in globals.o rather than being COMMON, and then having their placement be resolved at link-time. At link-time, they were being sorted and padded conservatively and ended up being aligned by pure luck.

Now that the compiler can decide to put them into BSS in the .o, they end up adjacent. Since PL_hash_seed has an awkward size, PL_hash_state thus is misaligned.

Observe,

00000de8 B PL_hash_seed 00000e04 B PL_hash_state

PL_hash_state is not at a 64bit aligned address. This is because both arrays are defined as char[], andPL_hash_seed does not have 64-bit aligned sizing.

In clang-generated assembly, the storage looks like this:

    .type   PL_hash_seed,@object    # @PL_hash_seed
    .globl  PL_hash_seed

PL_hash_seed: .space 28 .size PL_hash_seed, 28

    .type   PL_hash_state,@object   # @PL_hash_state
    .globl  PL_hash_state

PL_hash_state: .space 24612 .size PL_hash_state, 24612

The 28-byte PL_hash_seed allows the following object, which only has char[] alignment requirement, to become misaligned.

This is the cpp expansion of the storage allocating macros

_Bool PL_hash_seed_set = (0); unsigned char PL_hash_seed[( 16 + (int)( 3 * sizeof(U32) ) )];

unsigned char PL_hash_state[( 32 + ( ( 1 + ( 256 24 ) ) sizeof(U32) ) )];

I tried to change the macros to correctly typed objects, but there are []-indexes and such happening throughout the hash macros & inline functions which would require more typecasts. That direction does not seem sane.

I also looked into using atttribute(aligned), but it fits poorly.

I don't see a way to fit the seed object into a union with a uint64_t, a bunch of other objects would also need the same union, since there is a lot of shared code.

A final idea is to over-allocate the seed storage, such that it is aligned. Or this could even use a roundup() macro?

Original patch that changed the 3 to a 4 to correct alignment on this particular architecture removed.

millert@openbsd said:

I think using a roundup() macro would be safest.

True, 12+4*32 becomes non-aligning on armv7.

This solves it for all architectures. I'm excessively rounding up, but the other perl cpp definitions (like IVSIZE) feel dirty and unsuitable.

As a 64-bit architecture, octeon chooses to use PERL_HASH_FUNC_STADTX https://github.com/Perl/perl5/blob/blead/hv_func.h#L49.

There is a belief by several OpenBSD folks that the Linux kernel works around this and gcc seems to align things even without being told to, and some architectures may have ways to work around it. However these workarounds are likely very slow. kettenis@openbsd specifically points to "the DATA_ALIGNMENT macro in /usr/gnu/gcc/gcc/config/mips64.h".

Below is a patch that works around this issue for the current optimizations that llvm is applying but this is not a guaranteed fix as a change to the compiler orlinker could sort things differently and break it again.

It would be best for perl to be using the correct type and not relying on hacks to get the correct alignment. Deeper knowledge of the perl codebase seems to be required for that though.

There was a question where the 3 in the below code comes from as the initial patch just changed it to a 4 which worked on octeon, deraadt@ believes it is related to the SBOX32_MAX_LEN macro that was not always in scope and so this was a way to hack around that. Can you confirm?

Index: hv_func.h

RCS file: /cvs/src/gnu/usr.bin/perl/hv_func.h,v retrieving revision 1.5 diff -u -p -u -r1.5 hv_func.h --- hv_func.h 30 Dec 2019 02:13:41 -0000 1.5 +++ hv_func.h 10 Feb 2021 20:20:46 -0000 @@ -83,7 +83,8 @@

define _PERL_HASH_FUNC "SBOX32WITH" __PERL_HASH_FUNC

-#define _PERL_HASH_SEED_BYTES ( __PERL_HASH_SEED_BYTES + (int)( 3 sizeof(U32) ) ) +#define _PERL_HASH_SEED_roundup(x, y) ((((x)+((y)-1))/(y))(y)) +#define _PERL_HASH_SEED_BYTES ( _PERL_HASH_SEED_roundup(__PERL_HASH_SEED_BYTES + (int)( 3 * sizeof(U32)), sizeof(U64) ) )

define _PERL_HASH_STATE_BYTES \

 ( __PERL_HASH_STATE_BYTES + ( ( 1 + ( 256 * SBOX32_MAX_LEN ) ) * sizeof(U32) ) )

Expected behavior

Perl build completes without crashing.

Perl configuration

sysctl -n kern.version OpenBSD 6.9-beta (GENERIC.MP) #464: Sat Feb 6 22:21:13 MST 2021 deraadt@octeon.openbsd.org:/usr/src/sys/arch/octeon/compile/GENERIC.MP tar xzf perl-5.32.1.tar.gz cd perl-5.32.1 ./Configure -de ... make ... and I apparently deleted the log file with that output. I'll update in the morning after I generate a new one.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Perl/perl5/issues/18555, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZ5R6TC2ILDIZCVQ7IRQ3S6NXSHANCNFSM4XOIDKOA .

-- perl -Mre=debug -e "/just|another|perl|hacker/"

[demerphq]

I will need a few days to work through this as it is CNY right now. But i will look into it over the weekend.

Yves

On Thu, 11 Feb 2021 at 08:49, demerphq demerphq@gmail.com wrote:

I will pick this up. I recommend you do NOT use STADTX_ in a distro build I recommend you use SIPHASH-1-3 or SIPHASH-2-4 instead.Both should be build options available to you. We will make STADTX deprecated in a future release.

Yves

On Thu, 11 Feb 2021 at 07:05, Andrew Fresh notifications@github.com wrote:

Description

miniperl started crashing on OpenBSD octeon after clang's default changed to -fno-common.

Steps to Reproduce

visa@openbsd tracked down the specific variables involved, and deraadt@openbsd spent a full day inside perl.

With a small amount of work I (afresh1@openbsd) can get a machine set up for remote access that has these issues if someone needs access. I am available as afresh1 on IRC as well.

The following report was provided by deraadt@:

Take note of these two variables decls https://github.com/Perl/perl5/blob/blead/perlvars.h#L266.

PERLVARA(G, hash_seed, PERL_HASH_SEED_BYTES, unsigned char) / perl.c and hv.h /

if defined(PERL_HASH_STATE_BYTES)

PERLVARA(G, hash_state, PERL_HASH_STATE_BYTES, unsigned char) / perl.c and hv.h /

endif

These produce PL_hash_seed and PL_hash_state variables.

In the majority of perl files the macros creates extern decls. In globals.o, the macros create storage instead. The storage is not just PERL_HASH_SEED_BYTES & PERL_HASH_STATE_BYTES bytes -- it is more complicated.

Since we switched to -fno-common model in clang by default, the seed and state variables have confirmed scope and can go directy into BSS in globals.o rather than being COMMON, and then having their placement be resolved at link-time. At link-time, they were being sorted and padded conservatively and ended up being aligned by pure luck.

Now that the compiler can decide to put them into BSS in the .o, they end up adjacent. Since PL_hash_seed has an awkward size, PL_hash_state thus is misaligned.

Observe,

00000de8 B PL_hash_seed 00000e04 B PL_hash_state

PL_hash_state is not at a 64bit aligned address. This is because both arrays are defined as char[], andPL_hash_seed does not have 64-bit aligned sizing.

In clang-generated assembly, the storage looks like this:

    .type   PL_hash_seed,@object    # @PL_hash_seed
    .globl  PL_hash_seed

PL_hash_seed: .space 28 .size PL_hash_seed, 28

    .type   PL_hash_state,@object   # @PL_hash_state
    .globl  PL_hash_state

PL_hash_state: .space 24612 .size PL_hash_state, 24612

The 28-byte PL_hash_seed allows the following object, which only has char[] alignment requirement, to become misaligned.

This is the cpp expansion of the storage allocating macros

_Bool PL_hash_seed_set = (0); unsigned char PL_hash_seed[( 16 + (int)( 3 * sizeof(U32) ) )];

unsigned char PL_hash_state[( 32 + ( ( 1 + ( 256 24 ) ) sizeof(U32) ) )];

I tried to change the macros to correctly typed objects, but there are []-indexes and such happening throughout the hash macros & inline functions which would require more typecasts. That direction does not seem sane.

I also looked into using atttribute(aligned), but it fits poorly.

I don't see a way to fit the seed object into a union with a uint64_t, a bunch of other objects would also need the same union, since there is a lot of shared code.

A final idea is to over-allocate the seed storage, such that it is aligned. Or this could even use a roundup() macro?

Original patch that changed the 3 to a 4 to correct alignment on this particular architecture removed.

millert@openbsd said:

I think using a roundup() macro would be safest.

True, 12+4*32 becomes non-aligning on armv7.

This solves it for all architectures. I'm excessively rounding up, but the other perl cpp definitions (like IVSIZE) feel dirty and unsuitable.

As a 64-bit architecture, octeon chooses to use PERL_HASH_FUNC_STADTX https://github.com/Perl/perl5/blob/blead/hv_func.h#L49.

There is a belief by several OpenBSD folks that the Linux kernel works around this and gcc seems to align things even without being told to, and some architectures may have ways to work around it. However these workarounds are likely very slow. kettenis@openbsd specifically points to "the DATA_ALIGNMENT macro in /usr/gnu/gcc/gcc/config/mips64.h".

Below is a patch that works around this issue for the current optimizations that llvm is applying but this is not a guaranteed fix as a change to the compiler orlinker could sort things differently and break it again.

It would be best for perl to be using the correct type and not relying on hacks to get the correct alignment. Deeper knowledge of the perl codebase seems to be required for that though.

There was a question where the 3 in the below code comes from as the initial patch just changed it to a 4 which worked on octeon, deraadt@ believes it is related to the SBOX32_MAX_LEN macro that was not always in scope and so this was a way to hack around that. Can you confirm?

Index: hv_func.h

RCS file: /cvs/src/gnu/usr.bin/perl/hv_func.h,v retrieving revision 1.5 diff -u -p -u -r1.5 hv_func.h --- hv_func.h 30 Dec 2019 02:13:41 -0000 1.5 +++ hv_func.h 10 Feb 2021 20:20:46 -0000 @@ -83,7 +83,8 @@

define _PERL_HASH_FUNC "SBOX32WITH" __PERL_HASH_FUNC

-#define _PERL_HASH_SEED_BYTES ( __PERL_HASH_SEED_BYTES + (int)( 3 sizeof(U32) ) ) +#define _PERL_HASH_SEED_roundup(x, y) ((((x)+((y)-1))/(y))(y)) +#define _PERL_HASH_SEED_BYTES ( _PERL_HASH_SEED_roundup(__PERL_HASH_SEED_BYTES + (int)( 3 * sizeof(U32)), sizeof(U64) ) )

define _PERL_HASH_STATE_BYTES \

 ( __PERL_HASH_STATE_BYTES + ( ( 1 + ( 256 * SBOX32_MAX_LEN ) ) * sizeof(U32) ) )

Expected behavior

Perl build completes without crashing.

Perl configuration

sysctl -n kern.version OpenBSD 6.9-beta (GENERIC.MP) #464: Sat Feb 6 22:21:13 MST 2021 deraadt@octeon.openbsd.org:/usr/src/sys/arch/octeon/compile/GENERIC.MP tar xzf perl-5.32.1.tar.gz cd perl-5.32.1 ./Configure -de ... make ... and I apparently deleted the log file with that output. I'll update in the morning after I generate a new one.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Perl/perl5/issues/18555, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZ5R6TC2ILDIZCVQ7IRQ3S6NXSHANCNFSM4XOIDKOA .

-- perl -Mre=debug -e "/just|another|perl|hacker/"

-- perl -Mre=debug -e "/just|another|perl|hacker/"

[jkeenan]

Description

miniperl started crashing on OpenBSD octeon after clang's default changed to -fno-common.

For reference: http://www.openbsd.org/octeon.html. "OpenBSD/octeon is a port intended to run on MIPS64-based systems that utilize the Cavium OCTEON, OCTEON Plus, OCTEON II, and OCTEON III system on chips. "

[jkeenan]

@afresh1 reported this configuration information:

> **Description**
> 
> miniperl started crashing on OpenBSD octeon after clang's default changed to -fno-common.
> 

[snip]

> 
> ```
> sysctl -n kern.version
> OpenBSD 6.9-beta (GENERIC.MP) #464: Sat Feb  6 22:21:13 MST 2021
>     deraadt@octeon.openbsd.org:/usr/src/sys/arch/octeon/compile/GENERIC.MP
> tar xzf perl-5.32.1.tar.gz
> cd perl-5.32.1
> ./Configure -de
> ...
> make
> ... 
> cc -Wl,-E -fstack-protector-strong -o miniperl opmini.o perlmini.o gv.o toke.o perly.o pad.o regcomp.o dump.o util.o mg.o reentr.o mro_core.o keywords.o hv.o av.o run.o pp_hot.o sv.o pp.o scope.o pp_ctl.o pp_sys.o doop.o doio.o regexec.o utf8.o taint.o deb.o universal.o globals.o perlio.o perlapi.o numeric.o mathoms.o locale.o pp_pack.o pp_sort.o caretx.o dquote.o time64.o miniperlmain.o -lm -lc
> LD_LIBRARY_PATH=/home/afresh1/perl/perl-5.32.1/obj ./miniperl -w -Ilib -Idist/Exporter/lib -MExporter -e '<?>' || sh -c 'echo >&2 Failed to build miniperl. Please run make minitest; exit 1'
> Bus error (core dumped)
> Failed to build miniperl. Please run make minitest
> ```

Several questions:

• It appears you were working from the 5.32.1 tarball. Did you (or would you) get the same result with the 5.30 production release? (In other words, I'm trying to figure out whether there was a specific change in perl 5 source code that prompted this failure, or whether it was entirely due to a change in compilation switches on OpenBSD's part -- or some combination thereof.)

• Did your invocation of ./Configure have any other switches besides -de? If not, then where does -fno-common enter the situation?

• And once we've determined where -fno-common comes into play, what happens is we do not provide that switch?

• In an amd64 build of perl on OpenBSD, I'm accustomed to seeing this configuration:

  $ perl -V:config_args
  config_args='-dsE -Dopenbsd_distribution=defined -Dccflags=-DNO_LOCALE_NUMERIC -DNO_LOCALE_COLLATE -Dmksymlinks';

  ... where -Dopenbsd_distribution=defined is some special sauce on OpenBSD's side. What does the absence of that switch in your ./Configure call above signify (if anything)?

• Which version of clang are you using?

• While I suspect that OpenBSD has switched to clang as its default C-compiler, what happens if you try with gcc or g++ instead?

Thank you very much. Jim Keenan

[afresh1]

This does happen in perl 5.30.3 with OpenBSD system perl (where we originally saw it), and I assume with stock 5.30 as well. However I thought the report would be more useful with a recent plain version of perl, which also explains the lack of -Dopenbsd_distribution=defined and the other customary flags. Most of that special sauce is actually in hints/openbsd.sh, although there are some things I haven't gotten pushed upstream yet.

The -fno-common flag comes in with a switch to using it by default in clang, which uncovered the issue with these variables that are used as if they were aligned but don't request being aligned so the compiler doesn't know to do that and when this crash started. Without specifying -fno-common then this quote from deraadt@ is no longer in effect and we get the old optimizations where perl gets lucky and doesn't crash.

Since we switched to -fno-common model in clang by default, the seed and state variables have confirmed scope and can go directy into BSS in globals.o rather than being COMMON, and then having their placement be resolved at link-time. At link-time, they were being sorted and padded conservatively and ended up being aligned by pure luck.

Compiling with gcc avoids the alignment problems as gcc always aligns chars even when it is not requested.

gcc seems to align things even without being told to, [...] kettenis@openbsd specifically points to "the DATA_ALIGNMENT macro in /usr/gnu/gcc/gcc/config/mips64.h".

Compiling with gcc means that "it magically works".

$ clang -v
OpenBSD clang version 10.0.1 
Target: mips64-unknown-openbsd6.8
Thread model: posix
InstalledDir: /usr/bin

[afresh1]

@demerphq have you tried -Accflags=-DPERL_HASH_FUNC_SIPHASH?

In file included from hv.h:663,
from perl.h:3921,
from op.c:163:
hv_func.h: In function 'S_perl_hash_with_seed':
hv_func.h:107: warning: implicit declaration of function 'S_perl_siphash_seed_state'
hv_func.h:108: warning: implicit declaration of function 'S_perl_hash_siphash_2_4_with_state'
hv_func.h: At top level:
hv_func.h:194: warning: conflicting types for 'S_perl_siphash_seed_state'
hv_func.h:194: error: static declaration of 'S_perl_siphash_seed_state' follows non-static declaration
hv_func.h:107: error: previous implicit declaration of 'S_perl_siphash_seed_state' was here
hv_func.h:278: error: conflicting types for 'S_perl_hash_siphash_2_4_with_state'
hv_func.h:108: error: previous implicit declaration of 'S_perl_hash_siphash_2_4_with_state' was here

[demerphq]

On Fri, 12 Feb 2021, 03:55 Andrew Fresh, notifications@github.com wrote:

@demerphq https://github.com/demerphq have you tried -Accflags=-DPERL_HASH_FUNC_SIPHASH?

In file included from hv.h:663, from perl.h:3921, from op.c:163: hv_func.h: In function 'S_perl_hash_with_seed': hv_func.h:107: warning: implicit declaration of function 'S_perl_siphash_seed_state' hv_func.h:108: warning: implicit declaration of function 'S_perl_hash_siphash_2_4_with_state' hv_func.h: At top level: hv_func.h:194: warning: conflicting types for 'S_perl_siphash_seed_state' hv_func.h:194: error: static declaration of 'S_perl_siphash_seed_state' follows non-static declaration hv_func.h:107: error: previous implicit declaration of 'S_perl_siphash_seed_state' was here hv_func.h:278: error: conflicting types for 'S_perl_hash_siphash_2_4_with_state' hv_func.h:108: error: previous implicit declaration of 'S_perl_hash_siphash_2_4_with_state' was here

—

I do recollect that but I thought it was fixed. I suspect I rolled a fix but it did not get applied for some reason. I will investigate this whole mess tomorrow. Thanks for the reminder and sorry to suggest something that currently doesn't build.

Yves

You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/Perl/perl5/issues/18555#issuecomment-777936189, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZ5RZWTGNTHHVV5YEXKRTS6SKA3ANCNFSM4XOIDKOA .

[demerphq]

On Fri, 12 Feb 2021 at 09:01, demerphq demerphq@gmail.com wrote:

On Fri, 12 Feb 2021, 03:55 Andrew Fresh, notifications@github.com wrote:

@demerphq https://github.com/demerphq have you tried -Accflags=-DPERL_HASH_FUNC_SIPHASH?

In file included from hv.h:663, from perl.h:3921, from op.c:163: hv_func.h: In function 'S_perl_hash_with_seed': hv_func.h:107: warning: implicit declaration of function 'S_perl_siphash_seed_state' hv_func.h:108: warning: implicit declaration of function 'S_perl_hash_siphash_2_4_with_state' hv_func.h: At top level: hv_func.h:194: warning: conflicting types for 'S_perl_siphash_seed_state' hv_func.h:194: error: static declaration of 'S_perl_siphash_seed_state' follows non-static declaration hv_func.h:107: error: previous implicit declaration of 'S_perl_siphash_seed_state' was here hv_func.h:278: error: conflicting types for 'S_perl_hash_siphash_2_4_with_state' hv_func.h:108: error: previous implicit declaration of 'S_perl_hash_siphash_2_4_with_state' was here

—

I do recollect that but I thought it was fixed. I suspect I rolled a fix but it did not get applied for some reason. I will investigate this whole mess tomorrow. Thanks for the reminder and sorry to suggest something that currently doesn't build.

So all the required patches to get the hash function aligned and to answer the questions in this thread[1], along with some miscellaneous fixes I discovered along the way are contained in https://github.com/Perl/perl5/pull/18561

My patch is pretty close to yours, it is based on it in fact, but I took the liberty to address a few other issues, particularly related to 32 bit builds.

Patch 7ecd571465fad1de4f84ef24836c1612dac6a530 is a rebased version of the patch to make Siphash builds work which i thought had already been merged.

Please let me know if this now works as expected in terms of alignment. Thanks for the detailed bug report.

cheers Yves [1] Why 3? Because the SBOX stuff is initialized with a 3 x U32 vector.

-- perl -Mre=debug -e "/just|another|perl|hacker/"

[afresh1]

The problem with the workaround in the original patch is the compiler could re-arrange placement of objects in BSS such that U8 PL_hash_state[] gets put after a non-64-bit object, rather than immediately after a now-aligned PL_hash_seed[] and this solution doesn't ensure alignment.

The right thing is for each object to be declared with a correct size, so that each object demands alignment by the compiler for itself.

The best solution would be convincing the macros to assign the buffers as U64 P_hash_seed[size / sizeof(U64) ]; and U64 P_hash_state[size / sizeof(U64) ];.

[demerphq]

On Fri, 12 Feb 2021 at 19:01, Andrew Fresh notifications@github.com wrote:

The problem with the workaround in the original patch is the compiler could re-arrange placement of objects in BSS such that U8 PL_hash_state[] gets put after a non-64-bit object, rather than immediately after a now-aligned PL_hash_seed[] and this solution doesn't ensure alignment.

The right thing is for each object to be declared with a correct size, so that each object demands alignment by the compiler for itself.

The best solution would be convincing the macros to assign the buffers as U64 P_hash_seed[size / sizeof(U64) ]; and U64 P_hash_state[size / sizeof(U64) ];.

Understood. I will see what I can do. FWIW, if you try blead you should be able to build with SIPHASH now. (I recommend SIPHASH13 as faster and good enough for many purposes).

cheers, Yves

[demerphq]

On Fri, 12 Feb 2021 at 19:01, Andrew Fresh notifications@github.com wrote:

The problem with the workaround in the original patch is the compiler could re-arrange placement of objects in BSS such that U8 PL_hash_state[] gets put after a non-64-bit object, rather than immediately after a now-aligned PL_hash_seed[] and this solution doesn't ensure alignment.

The right thing is for each object to be declared with a correct size, so that each object demands alignment by the compiler for itself.

The best solution would be convincing the macros to assign the buffers as U64 P_hash_seed[size / sizeof(U64) ]; and U64 P_hash_state[size / sizeof(U64) ];.

Done in https://github.com/Perl/perl5/pull/18567

Thanks! yves

-- perl -Mre=debug -e "/just|another|perl|hacker/"

[demerphq]

If you can confirm that #18567 produced the desired result we can close this ticket.

[afresh1]

My octeon is pretty slow, but it seems to be working. Might take the rest of the weekend for my builds to complete but I will let you know.

[afresh1]

This seems to be doing what we need. Many thanks for the quick work!

[afresh1]

So, this seems to break ABI and cause at least mod_perl to fail (this was mod_perl-2.0.10):

modperl_perl.c: In function 'modperl_hash_seed_set':
modperl_perl.c:271: error: lvalue required as unary '&' operand
gmake[1]: *** [Makefile:112: modperl_perl.lo] Error 1

Looking at grep.metacpan it seems that hash_seed and hash_state are not used by anything else, but that could just be a failure of my searching.

[jkeenan]

So, this seems to break ABI and cause at least mod_perl to fail:

modperl_perl.c: In function 'modperl_hash_seed_set':
modperl_perl.c:271: error: lvalue required as unary '&' operand
gmake[1]: *** [Makefile:112: modperl_perl.lo] Error 1

Looking at grep.metacpan it seems that hash_seed and hash_state are not used by anything else, but that could just be a failure of my searching.

Can you tell whether this is something specific to octeon ... or to OpenBSD more generally ... or (just) generally? Can you provide a reduced case?

Thanks. jimk

[afresh1]

I tested this on sparc64, so I don't believe it is related to octeon. I think it is that mod_perl tries to use PL_hash_seed and somehow that doesn't exist. I expect you would be able to reproduce by trying to build mod_perl. I am not sure what a reduced case would look like, other than a new module that tried to use PL_hash_seed. I suppose it could be my regen of embedvar.h and perlapi.h, as it was easier for me to test building modules and stuff with my patched 5.32.1 since I don't know how to convince plenv to install a bleadperl. I will see if I can figure that out though.

EDIT: Look at that plenv install blead --as blead-2021-02-16 does what I want.

And with that, I can test mod_perl with blead perl.

This is perl 5, version 33, subversion 7 (v5.33.7 (4a026b43e97d13eff6b0baf1b644cf139d7f8ba2)) built for OpenBSD.amd64-openbsd

But, it still doesn't work right. Unfortunately I don't have a non-OpenBSD machine available with Apache installed to test another machine.

modperl_perl.c:271:16: error: cannot take the address of an rvalue of type
      'U8 *' (aka 'unsigned char *')
        memcpy(&PL_hash_seed, &MP_init_hash_seed,
               ^~~~~~~~~~~~~
2 warnings and 1 error generated.
*** Error 1 in src/modules/perl (Makefile:115 'modperl_perl.lo')
*** Error 2 in /tmp/hash/mod_perl-2.0.11 (Makefile:445 'modperl_lib')

[jkeenan]

So, this seems to break ABI and cause at least mod_perl to fail (this was mod_perl-2.0.10):

modperl_perl.c: In function 'modperl_hash_seed_set':
modperl_perl.c:271: error: lvalue required as unary '&' operand
gmake[1]: *** [Makefile:112: modperl_perl.lo] Error 1

Looking at grep.metacpan it seems that hash_seed and hash_state are not used by anything else, but that could just be a failure of my searching.

Please note that mod_perl is having testing problems across the board: http://matrix.cpantesters.org/?dist=mod_perl

[demerphq]

On Wed, 17 Feb 2021 at 04:50, Andrew Fresh notifications@github.com wrote:

I tested this on sparc64, so I don't believe it is related to octeon. I think it is that mod_perl tries to use PL_hash_seed https://metacpan.org/source/SHAY/mod_perl-2.0.11/src%2Fmodules%2Fperl%2Fmodperl_perl.c#L271 and somehow that doesn't exist. I expect you would be able to reproduce by trying to build mod_perl. I am not sure what a reduced case would look like, other than a new module that tried to use PL_hash_seed. I suppose it could be my regen of embedvar.h and perlapi.h https://github.com/afresh1/OpenBSD-perl/commit/6263d29707a5c1c2b8228efe70722ab79ef93205, as it was easier for me to test building modules and stuff with my patched 5.32.1 since I don't know how to convince plenv to install a bleadperl. I will see if I can figure that out though.

That code has been long broken. A long time ago I made it so that we do not directly use the seed in hashing after perl construct. We initialize the state from the seed, and then use the state directly. This means anycode that has been writing to the seed after perl_construct() was altering something that was no longer useful. We only keep the seed around so that we can tell users what it was.

I have to admit that code doesn't even make that much sense to me on the versions of perl it talks about, eg, 5.8.0 and 5.8.1, as IIRC perl_construct sets up some data structures that depend on the hash function, so altering it after perl_construct could leave a corrupted state. I remember that the state initialization logic had to be very very early. It isnt clear to me if anything writes to any hashes during Perl_construct, but several are definitely created and initialized. Anyway, I guess it doesn't matter. Ever since I split up hash initialization from hash execution this code has been basically a no-op or just an outright bug (eg reporting the wrong seed).

We would need to provide an API to set the hash seed to do what they want, and currently we don't have that API.

So I would say as far as Perl and this ticket is concerned this is not a bug that code is just wrong in many ways.

cheers, Yves

-- perl -Mre=debug -e "/just|another|perl|hacker/"

[afresh1]

Please note that mod_perl is having testing problems across the board: http://matrix.cpantesters.org/?dist=mod_perl

All those yellows are because nobody has Apache installed on their cpantester machines, but it is not very helpful.

In any case, that's the only thing on the CPAN I found using those variables, so if it isn't a problem then good news.

[sthen]

Are you expecting that the XS ABI should change with this? The problem I ran into that made it apparent was this:

perl:/usr/local/libdata/perl5/site_perl/amd64-openbsd/auto/Class/Load/XS/XS.so: undefined symbol 'PL_hash_state'

This didn't show up with @afresh1's grep and while the symbol doesn't show up in most .so's there are a few; on my workstation:

./auto/Package/Stash/XS/XS.so ./auto/Class/Load/XS/XS.so ./auto/Moose/Moose.so ./auto/XML/LibXML/LibXML.so

Presumably rebuilding packages built against libperl symbols will fix it (we can't easily identify just the affected ones but can trigger updates for all XS modules) but I just want to check that this is expected - openbsd can cope but it looks like this will affect all OS.

[jkeenan]

Are you expecting that the XS ABI should change with this? The problem I ran into that made it apparent was this:

perl:/usr/local/libdata/perl5/site_perl/amd64-openbsd/auto/Class/Load/XS/XS.so: undefined symbol 'PL_hash_state'

This didn't show up with @afresh1's grep and while the symbol doesn't show up in most .so's there are a few; on my workstation:

./auto/Package/Stash/XS/XS.so ./auto/Class/Load/XS/XS.so ./auto/Moose/Moose.so ./auto/XML/LibXML/LibXML.so

Presumably rebuilding packages built against libperl symbols will fix it (we can't easily identify just the affected ones but can trigger updates for all XS modules) but I just want to check that this is expected - openbsd can cope but it looks like this will affect all OS.

@sthen, can you supply more data as to the problem you observed? Was it also on octeon and also on OpenBSD? Can you reproduce the problem with a small example?

Thank you very much. Jim Keenan

[sthen]

On 2021/02/18 04:50, James E Keenan wrote:

Are you expecting that the XS ABI should change with this? The problem I ran into that made
it apparent was this:

perl:/usr/local/libdata/perl5/site_perl/amd64-openbsd/auto/Class/Load/XS/XS.so: undefined
symbol 'PL_hash_state'

This didn't show up with @afresh1's grep and while the symbol doesn't show up in most .so's
there are a few; on my workstation:

./auto/Package/Stash/XS/XS.so ./auto/Class/Load/XS/XS.so ./auto/Moose/Moose.so
./auto/XML/LibXML/LibXML.so

Presumably rebuilding packages built against libperl symbols will fix it (we can't easily
identify just the affected ones but can trigger updates for all XS modules) but I just want
to check that this is expected - openbsd can cope but it looks like this will affect all
OS.

@sthen, can you supply more data as to the problem you observed? Was it also on octeon and also on OpenBSD? Can you reproduce the problem with a small example?

Thank you very much. Jim Keenan

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.*

Hi Jim,

I don't have octeon machines, this was seen on amd64. We (openbsd) applied the patch to our tree (and have since backed it out while investigating). After updating to Perl binaries including this commit, some old compiled XS modules stopped working:

perl:/usr/local/libdata/perl5/site_perl/amd64-openbsd/auto/Class/Load/XS/XS.so: undefined symbol 'PL_hash_state'

The commit changes the PL_hash_state symbol in libperl to PL_hash_state_w (and similarly for PL_hash_seed). Some XS modules depend on the symbol; not all that many and I haven't identified what triggers it (I guess possibly those using PERL_HASH)

$ nm -s /usr/local/libdata/perl5/site_perl/amd64-openbsd/auto/Class/Load/XS/XS.so | grep PL_hash U PL_hash_state

and at least for the others I mentioned (Package::Stash::XS, Class::Load::XS, Moose, XML::LibXML).

I reverted the commit to get them running (I didn't have time to dig into it with a part-broken machine), but presumably they'll be fixed by recompiling against hv_func.h with the new macros i.e.

define PL_hash_seed ((U8 *)PL_hash_seed_w)

define PL_hash_state ((U8 *)PL_hash_state_w)

I guess the intention of doing the commit this way may have been to avoid changing type of an existing variable (thus quietly breaking ABI in a way that's hard to detect), so at least this is better as it's noisy ;)

As far as OpenBSD goes most of our users work with binary packages and we have mechanisms to force updates that we can use (as we would do for a Perl major version update). I'm not familiar with Perl's branching structure so don't know if this commit is currently destined to end up on a major release (which is OK I think) or a point release, if the latter then it might well be a problem.

[dur-randir]

In any case, that's the only thing on the CPAN I found using those variables, so if it isn't a problem then good news.

You should grep for PERLHASH([A-Z}+)?, which yields ~89 distributions. So while this commit breaks binary compatibility, but this only means that it can't be backported in upstream to 5.32 branch. If you can ship 5.32.next with all rebuilt modules, it shouldn't be a problem for you.

[jkeenan]

In any case, that's the only thing on the CPAN I found using those variables, so if it isn't a problem then good news.

You should grep for PERLHASH([A-Z}+)?, which yields ~89 distributions. So while this commit breaks binary compatibility, but this only means that it can't be backported in upstream to 5.32 branch. If you can ship 5.32.next with all rebuilt modules, it shouldn't be a problem for you.

I'm getting a bit confused here. Which commit are you referring to as "this commit"? @demerphq's recent merge https://github.com/Perl/perl5/commit/f43079cb514e3d0be0036424695438ae3fb58451? Or something in OpenBSD?

Thank you very much. Jim Keenan

[jkeenan]

You should grep for PERLHASH([A-Z}+)?

Should that be: PERLHASH([A-Z]+)?

[sthen]

On 2021/02/18 06:36, James E Keenan wrote:

    In any case, that's the only thing on the CPAN I found using those variables, so if it
    isn't a problem then good news.

You should grep for PERL_HASH(_[A-Z}+)?, which yields ~89 distributions. So while this
commit breaks binary compatibility, but this only means that it can't be backported in
upstream to 5.32 branch. If you can ship 5.32.next with all rebuilt modules, it shouldn't
be a problem for you.

I'm getting a bit confused here. Which commit are you referring to as "this commit"? @demerphq 's recent merge f43079c? Or something in OpenBSD?

Thank you very much. Jim Keenan

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.*

f43079c, which was then merged to OpenBSD.

As long as this doesn't get backported to 5.32.xx then I guess this is not a problem for Perl.

[afresh1]

In any case, that's the only thing on the CPAN I found using those variables, so if it isn't a problem then good news.

You should grep for PERLHASH([A-Z}+)?, which yields ~89 distributions. So [...] with all rebuilt modules, it shouldn't be a problem for you.

I was mostly worried about folks that used the variables that are now defines, not the ones that will work once recompiled, and so the question about mod_perl. We can bump the libperl version separately which will cause updates for all the xs modules in OpenBSD so agree, these are not a problem for us, and since you know that keeps it from being a candidate for a dot release on 5.32, it's also not a problem for you.

Many thanks for all your help and answering questions.