Bug 495112 - s390x: GCC miscompiles coredump-elf.c
Summary: s390x: GCC miscompiles coredump-elf.c
Status: RESOLVED NOT A BUG
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (other bugs)
Version First Reported In: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-10-20 21:24 UTC by Florian Krohm
Modified: 2024-10-21 09:21 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
Reproducer (813 bytes, text/x-csrc)
2024-10-20 21:24 UTC, Florian Krohm
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Krohm 2024-10-20 21:24:33 UTC
Created attachment 175067 [details]
Reproducer

The symptom is this compiler warning:

In function ‘fill_prstatus’,
    inlined from ‘dump_one_thread’ at m_coredump/coredump-elf.c:804:7:
m_coredump/coredump-elf.c:451:32: warning: array subscript ‘struct vki_user_regs_struct[0]
’ is partly outside array bounds of ‘struct vki_elf_prstatus[1]’ [-Warray-bounds=]
  451 | #  define DO(n)  regs->gprs[n] = arch->vex.guest_r##n
      |                  ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
m_coredump/coredump-elf.c:452:4: note: in expansion of macro ‘DO’
  452 |    DO(0);  DO(1);  DO(2);  DO(3);  DO(4);  DO(5);  DO(6);  DO(7);
      |    ^~
m_coredump/coredump-elf.c: In function ‘dump_one_thread’:
m_coredump/coredump-elf.c:774:28: note: at offset 112 into object ‘prstatus’ of size 336
  774 |    struct vki_elf_prstatus prstatus;
      |                            ^~~~~~~~
....
and many more for every invocation of the DO macro.

The warning is incorrect because it is not struct vki_user_regs_struct that
is being indexed but vki_user_regs_struct::gprs. I condensed a small reproducer.
Taking a closer look it turns out that the reproducer gets miscompiled.
In the .s file there should be a reference to the global variable "vexstuff" but
it is missing. I guess it's reasonable to conclude that coredump-elf.c will get
miscompiled as well.

This occurs both with gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0 as well
as stock 14.2.0 (latest released version). One of the folks at IBM maintaining the 
s390x backend for GCC confirmed that this is a known bug.

The fix is to reduce the optimisation level with a function attribute like so:

diff --git a/coregrind/m_coredump/coredump-elf.c b/coregrind/m_coredump/coredump-elf.c
index a4632d9e2..22c6d4946 100644
--- a/coregrind/m_coredump/coredump-elf.c
+++ b/coregrind/m_coredump/coredump-elf.c
@@ -241,6 +241,9 @@ static void fill_prpsinfo(const ThreadState *tst,
 }
 #endif
 
+#if __GNUC__*10000 + __GNUC_MINOR__*100 + __GNUC__PATCHLEVEL__ <= 140200
+__attribute__((optimize(1)))
+#endif
 static void fill_prstatus(const ThreadState *tst, 
 			  /*OUT*/struct vki_elf_prstatus *prs, 
 			  const vki_siginfo_t *si)
Comment 1 Tom Hughes 2024-10-20 21:34:00 UTC
Shouldn't this be reported to gcc rather than trying to workaround it here?
Comment 2 Florian Krohm 2024-10-21 06:34:04 UTC
(In reply to Tom Hughes from comment #1)
> Shouldn't this be reported to gcc rather than trying to workaround it here?

Yes, definitely. And GCC is the place where it ought to be fixed. As the s390x gcc liaison told me: 
this is an instance of a known bug. So I would assume that they have a PR for that in their bugzilla.
I'd argue that the proposed patch should be applied nevertheless not so much because it makes 
the warning disappear but because of the miscompile. 
Assuming that the bug is fixed in the the next GCC release, will it be backported to 11.4.0? I do not know.
That GCC version is shipped with Ubuntu 22.4 and probably still in widespread use.
Comment 3 Florian Krohm 2024-10-21 09:21:49 UTC
I stand corrected. As was explained to me: there is no miscompile here. Just an invalid warning which is
what the GCC PR refers to.
There are no references to that 'vexsstuff'  variable in the reproducer because those assignments
are dead. They assign to a variable whose value is never used. Hence GCC throws them out. Nice!

Now, as much as I like a clean build -- I dislike it even more to clutter the code with pragmas and ifdeffery
to suppress the warning. So I'm content leaving things as they are.
Sorry for the noise.