Bug 430429 - valgrind.h doesn't compile on s390x with clang
Summary: valgrind.h doesn't compile on s390x with clang
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (other bugs)
Version First Reported In: unspecified
Platform: Other Linux
: NOR major
Target Milestone: ---
Assignee: Andreas Arnez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-12-15 15:16 UTC by Avi Kivity
Modified: 2021-03-23 11:08 UTC (History)
3 users (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Avi Kivity 2020-12-15 15:16:04 UTC
SUMMARY

Given a file test.c with the content

---8<--begin content---8<----
#include <valgrind/valgrind.h>
---8<---end content----8<----

It fails to compile on s390x:

 clang -o test.o test.c
In file included from test.c:1:
/usr/include/valgrind/valgrind.h:6768:47: error: unsupported inline asm: input with type 'int' matching output with type 'volatile unsigned long'
   _qzz_res = VALGRIND_DO_CLIENT_REQUEST_EXPR(0,
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
/usr/include/valgrind/valgrind.h:879:49: note: expanded from macro 'VALGRIND_DO_CLIENT_REQUEST_EXPR'
                    : "a" (&_zzq_args[0]), "0" (_zzq_default)    \
                                                ^~~~~~~~~~~~
/usr/include/valgrind/valgrind.h:6807:47: error: unsupported inline asm: input with type 'int' matching output with type 'volatile unsigned long'
   _qzz_res = VALGRIND_DO_CLIENT_REQUEST_EXPR(0,
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
/usr/include/valgrind/valgrind.h:879:49: note: expanded from macro 'VALGRIND_DO_CLIENT_REQUEST_EXPR'
                    : "a" (&_zzq_args[0]), "0" (_zzq_default)    \
                                                ^~~~~~~~~~~~
2 errors generated.



STEPS TO REPRODUCE
1. Create a file with the provided content
2. Compile it on s390x with clang


OBSERVED RESULT


Errors

EXPECTED RESULT

No errors


SOFTWARE/OS VERSIONS
Linux, Fedora 33, valgrind-3.16.1-8.fc33.s390x, clang-11.0.0-2.fc33.s390x

ADDITIONAL INFORMATION
Comment 1 Jonathan Albrecht 2021-02-19 14:51:37 UTC
This seems very similar to https://bugs.kde.org/show_bug.cgi?id=342008 which just added an explicit cast for _zzq_default.

I tried the same fix for s390x with:

diff --git a/include/valgrind.h b/include/valgrind.h
index d33dd3093..04a747c7a 100644
--- a/include/valgrind.h
+++ b/include/valgrind.h
@@ -876,7 +876,8 @@ typedef
                     /* results = r3 */                           \
                     "lgr %0, 3\n\t"                              \
                     : "=d" (_zzq_result)                         \
-                    : "a" (&_zzq_args[0]), "0" (_zzq_default)    \
+                    : "a" (&_zzq_args[0]),                       \
+                      "0" ((unsigned long int)_zzq_default)      \
                     : "cc", "2", "3", "memory"                   \
                    );                                            \
    _zzq_result;                                                  \

and it fixes the compile error but I don't know if that's sufficient.

FWIW, I built valgrind from source using gcc and ran the reg tests both with and without the change and got the same results.
Comment 2 Andreas Arnez 2021-02-24 19:15:48 UTC
(In reply to Jonathan Albrecht from comment #1)
> [...]
> I tried the same fix for s390x with:
> 
> diff --git a/include/valgrind.h b/include/valgrind.h
> index d33dd3093..04a747c7a 100644
> [...]
FWIW, the patch looks OK to me, and I don't see why it shouldn't be sufficient for fixing this error.

@Julian: Isn't the patch necessary for other 64-bit platforms as well then?  Such as ppc64le or amd64?
Comment 3 Paul Floyd 2021-03-04 20:31:11 UTC
There is certainly no problem on FreeBSD. Just double checked with clang 10 and clang 12 (don't have 11 handy).

I have done builds with clang on Fedora and I don't remember seeing any such issues. Will confirm tomorrow.
Comment 4 Julian Seward 2021-03-05 09:46:42 UTC
(In reply to Andreas Arnez from comment #2)

@Andreas: if the patch works on s390, I think you should land it.

> @Julian: Isn't the patch necessary for other 64-bit platforms as well then? 
> Such as ppc64le or amd64?

Well, we have no reports of such problems on either ppc64le or amd64.
Though looking at the code, I fundamentally agree with you .. I don't
see why s390 should have a problem here but not the others.
Comment 5 Paul Floyd 2021-03-05 10:41:19 UTC
Quick confirmation for amd64 Fedora. No problems with clang 11 or clang 13 (built from git head).

In fact, even with -Weverything I only get a few warnings about the use of leading double underscores. That is pretty good as -Weverything often generates many reams of warnings.
Comment 6 Julian Seward 2021-03-05 10:57:37 UTC
(In reply to Paul Floyd from comment #5)
> Quick confirmation for amd64 Fedora. No problems with clang 11 or clang 13

Thanks.

(general comment, not suggesting any action right now): Looking at valgrind.h
now, I am thinking that it is a mistake for us to use `unsigned long int` as
the basic machine-word type for 64-bit targets.  Perhaps we should change those
en masse to `unsigned long long int`, so as to avoid any ambiguity in the
type-sizes.  TBH I am not sure why it uses `unsigned long int` at all.
Comment 7 Andreas Arnez 2021-03-09 16:39:38 UTC
(In reply to Julian Seward from comment #4)
> @Andreas: if the patch works on s390, I think you should land it.
Done.  Pushed as 484b7dd1e862b1624cb8e7aa453df277da4f7a15.
Comment 8 Avi Kivity 2021-03-12 14:33:28 UTC
Maybe thanks for fixing the bug. Please consider a patch release so I can lobby my distribution for updating its valgrind.
Comment 9 Avi Kivity 2021-03-12 14:34:10 UTC
(Many and unconditional thanks, not maybe thanks).
Comment 10 Andreas Arnez 2021-03-23 11:08:05 UTC
Valgrind 3.17.0 has been released, and it contains the fix for this Bug.  See the announcement:
  https://sourceforge.net/p/valgrind/mailman/message/37246072/