Bug 2338812

Are you sure it is gcc?
I've built isa-l in rawhide mock (where it failed with the given test) and f41 (where it passed).
In the rawhide chroot, I've run igzip_rand_test against the f41 libisal.so.2 (i.e. built with gcc 14.2.1), it still failed.
In the 41 chroot, I've run igzip_rand_test against the rawhide libisal.so.2 (i.e. built with gcc 15.0.1), it still succeeded.
That would point to the igzip_rand_test binary.
But, cross-copying those binaries doesn't change anything, even rawhide built igzip_rand_test with LD_LIBRARY_PATH against
rawhide built libisal.so.2 in the 41 chroot succeeds, while 41 built igzip_rand_test with LD_LIBRARY_PATH against 41 built libisal.so.2 fails.

So, in particular I've copied the .libs/libisal.so* and igzip/.libs/igzip_rand_test from both buildroots to /tmp/ of both buildroots.
In 41 buildroot
[mockbuild@da1a45a5b1e94ac9b1f689925e3c5204 ~]$ LD_LIBRARY_PATH=/tmp/rawhide/ /tmp/rawhide/igzip_rand_test 
igzip_rand_test stateless:              Pass
igzip_rand_test stateless FULL_FLUSH:   Pass
igzip_rand_test stateful  NO_FLUSH:     Pass
igzip_rand_test stateful  SYNC_FLUSH:   Pass
igzip_rand_test stateful  FULL_FLUSH:   Pass
igzip_rand_test stateful  Change Flush: Pass
igzip_rand_test large input             Pass
igzip_rand_test inflate   Std Vectors:  Pass
igzip rand test finished: All tests passed
[mockbuild@da1a45a5b1e94ac9b1f689925e3c5204 ~]$ LD_LIBRARY_PATH=/tmp/41/ /tmp/41/igzip_rand_test 
igzip_rand_test stateless:              Pass
igzip_rand_test stateless FULL_FLUSH:   Pass
igzip_rand_test stateful  NO_FLUSH:     Pass
igzip_rand_test stateful  SYNC_FLUSH:   Pass
igzip_rand_test stateful  FULL_FLUSH:   Pass
igzip_rand_test stateful  Change Flush: Pass
igzip_rand_test large input             Pass
igzip_rand_test inflate   Std Vectors:  Pass
igzip rand test finished: All tests passed
In the rawhide chroot
[mockbuild@1310715f95354bd5b8749b91e252348f ~]$ LD_LIBRARY_PATH=/tmp/rawhide /tmp/rawhide/igzip_rand_test 
igzip_rand_test stateless:              Pass
igzip_rand_test stateless FULL_FLUSH:   error: invalid header
Fail
igzip rand test finished: Some tests failed
[mockbuild@1310715f95354bd5b8749b91e252348f ~]$ LD_LIBRARY_PATH=/tmp/41 /tmp/41/igzip_rand_test 
igzip_rand_test stateless:              Pass
igzip_rand_test stateless FULL_FLUSH:   error: invalid header
Fail
igzip rand test finished: Some tests failed
strace tells all it opens are the libisal.so.2 and glibc.
openat(AT_FDCWD, "/tmp/41/libisal.so.2", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
So the only thing that changed is glibc.

And now same buildroot, /tmp/old is the fc41 ld64.so.1 and libc.so.6 and /tmp/new fc64 ld64.so.1 and libc.so.6:
[mockbuild@1310715f95354bd5b8749b91e252348f ~]$ /tmp/old/ld64.so.1 --library-path /tmp/old:/tmp/41 /tmp/41/igzip_rand_test 
igzip_rand_test stateless:              Pass
igzip_rand_test stateless FULL_FLUSH:   Pass
igzip_rand_test stateful  NO_FLUSH:     Pass
igzip_rand_test stateful  SYNC_FLUSH:   Pass
igzip_rand_test stateful  FULL_FLUSH:   Pass
igzip_rand_test stateful  Change Flush: Pass
igzip_rand_test large input             Pass
igzip_rand_test inflate   Std Vectors:  Pass
igzip rand test finished: All tests passed
[mockbuild@1310715f95354bd5b8749b91e252348f ~]$ /tmp/new/ld64.so.1 --library-path /tmp/new:/tmp/41 /tmp/41/igzip_rand_test 
igzip_rand_test stateless:              Pass
igzip_rand_test stateless FULL_FLUSH:   error: invalid header
Fail
igzip rand test finished: Some tests failed
and
[mockbuild@3950c92442f34105bfd9728c2fe4f87b tmp]$ /tmp/old/ld64.so.1 --library-path /tmp/old:/tmp/rawhide /tmp/rawhide/igzip_rand_test
igzip_rand_test stateless:              Pass
igzip_rand_test stateless FULL_FLUSH:   Pass
igzip_rand_test stateful  NO_FLUSH:     Pass
igzip_rand_test stateful  SYNC_FLUSH:   Pass
igzip_rand_test stateful  FULL_FLUSH:   Pass
igzip_rand_test stateful  Change Flush: Pass
igzip_rand_test large input             Pass
igzip_rand_test inflate   Std Vectors:  Pass
igzip rand test finished: All tests passed
[mockbuild@3950c92442f34105bfd9728c2fe4f87b tmp]$ /tmp/new/ld64.so.1 --library-path /tmp/new:/tmp/rawhide /tmp/rawhide/igzip_rand_test
igzip_rand_test stateless:              Pass
igzip_rand_test stateless FULL_FLUSH:   error: invalid header
Fail
igzip rand test finished: Some tests failed
in the other buildroot.  So, it is glibc 2.40-17.s390x vs. glibc-2.40.9000-28.fc42 that changes things, not the version of gcc.
And, both glibc 2.40-17.s390x and glibc 2.40.900-28.fc42.s390x have been built with gcc 14.2.1.
Of course it might be some UB in the package, but I don't see how gcc is involved here at all.

I'll try to bisect it on the glibc side.

Surprisingly, it's not one of my TLS changes.

I suspect there is an integer size mismatch somewhere in this change, and that breaks things on big endian:

commit d5bceac99d24af1131b90027dab267e437b65cd1 (HEAD)
Author: Sam James <sam>
Date:   Tue Dec 10 01:21:46 2024 +0000

    stdlib: random_r: fix unaligned access in initstate and initstate_r [BZ #30584]
    
    The initstate{,_r} interfaces are documented in BSD as needing an aligned
    array of 32-bit values, but neither POSIX nor glibc's own documentation
    require it to be aligned. glibc's documentation says it "should" be a power
    of 2, but not must.
    
    Use memcpy to read and write to `state` to handle such an unaligned
    argument.
    
    Co-authored-by: Adhemerval Zanella <adhemerval.zanella>
    Reviewed-by: Florian Weimer <fweimer>

This fixes it:

diff --git a/stdlib/random_r.c b/stdlib/random_r.c
index 605e96983c..64f3e402aa 100644
--- a/stdlib/random_r.c
+++ b/stdlib/random_r.c
@@ -390,9 +390,8 @@ __random_r (struct random_data *buf, int32_t *result)
       int32_t *end_ptr = buf->end_ptr;
       uint32_t val;
 
-      val = read_state (rptr, 0);
-      int32_t t = read_state (fptr, 0);
-      write_state (fptr, 0, t + val);
+      val = read_state (fptr, 0) + read_state (rptr, 0);
+      write_state (fptr, 0, val);
       /* Chucking least random bit.  */
       *result = val >> 1;
       ++fptr;

It's clearly a bug in the original commit, which made this change:

-      val = *fptr += (uint32_t) *rptr;
+      val = read_state (rptr, 0);
+      int32_t t = read_state (fptr, 0);
+      write_state (fptr, 0, t + val);

I would like to better understand why this makes a difference, and why only on s390x.

(In reply to Florian Weimer from comment #5)
> +      val = read_state (fptr, 0) + read_state (rptr, 0);

Does it care which read_state happens first?

(In reply to Jakub Jelinek from comment #6)
> (In reply to Florian Weimer from comment #5)
> > +      val = read_state (fptr, 0) + read_state (rptr, 0);
> 
> Does it care which read_state happens first?

No, I don't think the original code had a sequence point violation or race condition.

The issue is that the sequence of random numbers changes due to this bug, and the test case igzip/igzip_rand_test.c now selects level 3 here instead of level 0:

int
test_compress_stateless(uint8_t *in_data, uint32_t in_size, uint32_t flush_type)
{
        int ret = IGZIP_COMP_OK;
        uint32_t z_size, overflow, gzip_flag, level, hist_bits;
        uint8_t *z_buf = NULL;
        uint8_t *in_buf = NULL;

        gzip_flag = rand() % 5;
        hist_bits = rand() % 16;
        level = get_rand_level();
…
}

If I change the level to 3 manually on s390x, the test fails with the old glibc, too. But forcing this change into the x86-64 build doesn't make the test fail on that architecture. The s390x gzip accelerator does not seem to be used. Maybe it's the over way round: the x86-64 assembler pieces deal with this test change better than the C implementation that is used on s390x.

I think val = read_state (fptr, 0) + read_state (rptr, 0); is bad, because the function returns int32_t and I think the point of the old code was to do the addition in unsigned type to avoid UB on overflow.
So better either val = read_state (fptr, 0); val += read_state (rptr, 0); or the other way around, or explicit (uint32_t) cast somewhere.

As for isa-l, isn't there a bug then in the testcase as well?  Give me any (pseudo)random number you like as long as it is not this and that because I don't handle that.

(In reply to Jakub Jelinek from comment #8)
> I think val = read_state (fptr, 0) + read_state (rptr, 0); is bad, because
> the function returns int32_t and I think the point of the old code was to do
> the addition in unsigned type to avoid UB on overflow.
> So better either val = read_state (fptr, 0); val += read_state (rptr, 0); or
> the other way around, or explicit (uint32_t) cast somewhere.
> 
> As for isa-l, isn't there a bug then in the testcase as well?  Give me any
> (pseudo)random number you like as long as it is not this and that because I
> don't handle that.

Fair point, I'll tweak it.

Posted upstream:

[PATCH 1/2] stdlib: Fix unintended change to the random_r implementation
<https://inbox.sourceware.org/libc-alpha/76b56012252d98de3ffacb5f8e3b2adfd6ee6ede.1737403568.git.fweimer@redhat.com/>

(In reply to Jakub Jelinek from comment #8)
> As for isa-l, isn't there a bug then in the testcase as well?  Give me any
> (pseudo)random number you like as long as it is not this and that because I
> don't handle that.

The isa-l issue has been fixed uosream:
https://github.com/intel/isa-l/issues/311