Bug 1697302

setting __attribute__((optimize(("O0")))) for Skein_512::initial_block() (https://github.com/randombit/botan/blob/master/src/lib/hash/skein/skein_512.cpp#L70) makes the tests pass again, I'm looking further ...

void Skein_512::initial_block()
   {
   const uint8_t zeros[64] = { 0 };

   m_threefish->set_key(zeros, sizeof(zeros));

   // ASCII("SHA3") followed by version (0x0001) code
   uint8_t config_str[32] = { 0x53, 0x48, 0x41, 0x33, 0x01, 0x00, 0 };

==>       the problem is the array doesn't contain zeros after ^^^ on s390x

   store_le(uint32_t(m_output_bits), config_str + 8);

   reset_tweak(SKEIN_CONFIG, true);
   ubi_512(config_str, sizeof(config_str));


for upstream code see https://github.com/randombit/botan/blob/master/src/lib/hash/skein/skein_512.cpp#L77

from a gdb session on rawhide/s390x
(gdb) where
#0  Botan::Skein_512::initial_block (this=this@entry=0x10b76d0) at src/lib/hash/skein/skein_512.cpp:72
#1  0x000003fffdb697e0 in Botan::Skein_512::Skein_512 (this=0x10b76d0, arg_output_bits=<optimized out>, arg_personalization=...) at src/lib/hash/skein/skein_512.cpp:25
#2  0x000003fffdb5278a in Botan::HashFunction::create (algo_spec="Skein-512", provider="") at /usr/include/c++/9/ext/new_allocator.h:80
#3  0x000000000102d4e4 in Botan_CLI::Hash::go (this=0x10b6b70) at /usr/include/c++/9/bits/basic_string.h:263
#4  0x0000000001022808 in Botan_CLI::Command::run (this=0x10b6b70, params=std::vector of length 1, capacity 1 = {...}) at src/cli/cli.cpp:75
#5  0x00000000010191d0 in main (argc=<optimized out>, argv=0x3fffffff008) at /usr/include/c++/9/bits/unique_ptr.h:357
(gdb) n
73	   const uint8_t zeros[64] = { 0 };
(gdb) p zeros
$1 = "\000\000\000\000\001\vv\320\000\000\003\377\375\372_@\000\000\000\000\001\vv\350\000\000\003\377\375\371a\\\000\000\003\377\377\377\346H\000\000\000\000\000\000\004-\000\000\003\377\377\377\362\332\000\000\000\000\000\000\000"
(gdb) n
357	      get() const noexcept
(gdb) 
78	   uint8_t config_str[32] = { 0x53, 0x48, 0x41, 0x33, 0x01, 0x00, 0 };
(gdb) l
73	   const uint8_t zeros[64] = { 0 };
74	
75	   m_threefish->set_key(zeros, sizeof(zeros));
76	
77	   // ASCII("SHA3") followed by version (0x0001) code
78	   uint8_t config_str[32] = { 0x53, 0x48, 0x41, 0x33, 0x01, 0x00, 0 };
79	   store_le(uint32_t(m_output_bits), config_str + 8);
80	
81	   reset_tweak(SKEIN_CONFIG, true);
82	   ubi_512(config_str, sizeof(config_str));
(gdb) p zeros
$2 = '\000' <repeats 63 times>
(gdb) n
79	   store_le(uint32_t(m_output_bits), config_str + 8);
(gdb) p config_str
$3 = "SHA3\001\000\000\000\000\000\000\000\000\000\002\000\000\000\000\000\001\vv\320\000\000\003\377\375\364", <incomplete sequence \310>

Created attachment 1553647 [details]
preprocessed source file

What g++ options are used to compile it when it is miscompiled?

g++ -fPIC -fvisibility=hidden -fstack-protector -m64 -pthread -std=c++11 -D_REENTRANT -O3 -Wall -Wextra -Wpedantic -Wstrict-aliasing -Wcast-align -Wmissing-declarations -Wpointer-arith -Wcast-qual -Wzero-as-null-pointer-constant -Wnon-virtual-dtor  -Ibuild/include -Ibuild/include/external -c src/lib/hash/skein/skein_512.cpp -o build/obj/lib/hash_skein_512.o

^^^ this is from an upstream build, using Fedora flags gives the same (miscompiled) result

I think I can reproduce it with C -O2 -march=zEC12 -mtune=z13:
__attribute__((noipa)) void
bar (char *p)
{
  int i;
  for (i = 0; i < 6; i++)
    if (p[i] != "foobar"[i])
      __builtin_abort ();
  for (; i < 32; i++)
    if (p[i] != '\0')
      __builtin_abort ();
}

__attribute__((noipa)) void
foo (unsigned int x)
{
  char s[32] = { 'f', 'o', 'o', 'b', 'a', 'r', 0 };
  ((unsigned int *) s)[2] = __builtin_bswap32 (x);
  bar (s);
}

int
main ()
{
  foo (0);
  return 0;
}

(though, haven't tried to actually ssh somewhere and run it, just eyeballed dumps).
Will now bisect and fix.  In any case, looks like the bug is that the MEM used to clear the remainder of the array (last 24 bytes) claims to have size 1 instead of either 24 (exact size) or no size (conservative):
(insn 7 6 0 (parallel [
            (set (mem/c:BLK (plus:DI (reg/f:DI 55 virtual-stack-vars)
                        (const_int -24 [0xffffffffffffffe8])) [0 s+8 S1 A64])
                (const_int 0 [0]))
            (use (const_int 23 [0x17]))
            (use (const:BLK (unspec:BLK [
                            (const_int 0 [0])
                        ] UNSPEC_INSN)))
            (clobber (scratch:DI))
            (clobber (reg:CC 33 %cc))
        ]) "rh1697302.c":16:8 -1
     (nil))
from expansion or
(insn 19 6 15 2 (parallel [
            (set (mem/c:BLK (plus:DI (reg/f:DI 15 %r15)
                        (const_int 168 [0xa8])) [0 s+8 S1 A64])
                (const_int 0 [0]))
            (use (const_int 24 [0x18]))
            (clobber (reg:CC 33 %cc))
        ]) "rh1697302.c":16:8 1752 {*xc_zero}
     (nil))
after splitting.  It should use S24 or no S...

(In reply to Jakub Jelinek from comment #6)
> I think I can reproduce it with C -O2 -march=zEC12 -mtune=z13:
> __attribute__((noipa)) void
> bar (char *p)
> {
>   int i;
>   for (i = 0; i < 6; i++)
>     if (p[i] != "foobar"[i])
>       __builtin_abort ();
>   for (; i < 32; i++)
>     if (p[i] != '\0')
>       __builtin_abort ();
> }
> 
> __attribute__((noipa)) void
> foo (unsigned int x)
> {
>   char s[32] = { 'f', 'o', 'o', 'b', 'a', 'r', 0 };
>   ((unsigned int *) s)[2] = __builtin_bswap32 (x);
>   bar (s);
> }
> 
> int
> main ()
> {
>   foo (0);
>   return 0;
> }
> 
> (though, haven't tried to actually ssh somewhere and run it, just eyeballed
> dumps).

just checked and it really aborts

Started with my r268957 aka PR66152 change, though I bet it is a backend latent bug before that.

Actually no, the bug was in my patch.

Thanks Jakub, builds fine now with gcc 9.1