Created attachment 58464 [details] Patch that implements the described proposal. Version: 3.7 SVN (using Devel) OS: All Because of how the VALGRIND_DO_CLIENT_REQUEST() macro has been defined, instantiating a variable to store the result of the client request is necessary even if that result won't be used. One way to get rid of these superfluous temporary variables and the associated gcc 4.6 "set but not used" warnings is to swap the roles of VALGRIND_DO_CLIENT_REQUEST() and VALGRIND_DO_CLIENT_REQUEST_EXPR(). The attached patch implements this approach. Open issues: - The attached patch so far has been tested only on x86, amd64 and ppc and not yet on any of the other supported architectures. - Since the new VALGRIND_DO_CLIENT_REQUEST_EXPR() for gcc is implemented as a statement expression, ignoring the return value of a client request can trigger a pesky "value computed is not used" warning. Reproducible: Always
Created attachment 58548 [details] Patch that implements the described proposal, v2.
I have been experimenting with a further change on top of the attached patch that makes the VALGRIND_DO_CLIENT_REQUEST_EXPR() macro invoke an inline function called valgrind_do_client_request_expr(). While that helps to get rid of the "value computed is not used" warning, that change breaks many regression tests because the call stacks in Valgrind error reports differ. Is it acceptable that the call stacks change ? An example of such a call stack change: --- addressable.stderr.exp 2011-03-05 17:16:41.000000000 +0100 +++ addressable.stderr.out 2011-04-20 20:09:34.000000000 +0200 @@ -9,7 +9,8 @@ For counts of detected and suppressed errors, rerun with: -v ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) Unaddressable byte(s) found during client check request - at 0x........: test2 (addressable.c:48) + at 0x........: valgrind_do_client_request_expr (valgrind.h:...) + by 0x........: test2 (addressable.c:48) by 0x........: main (addressable.c:125) Address 0x........ is not stack'd, malloc'd or (recently) free'd Which is caused by this valgrind.h change: @@ -236,24 +242,42 @@ typedef #define VALGRIND_DO_CLIENT_REQUEST_EXPR( \ _zzq_default, _zzq_request, \ _zzq_arg1, _zzq_arg2, _zzq_arg3, _zzq_arg4, _zzq_arg5) \ - __extension__ \ - ({volatile unsigned int _zzq_args[6]; \ - volatile unsigned int _zzq_result; \ - _zzq_args[0] = (unsigned int)(_zzq_request); \ - _zzq_args[1] = (unsigned int)(_zzq_arg1); \ - _zzq_args[2] = (unsigned int)(_zzq_arg2); \ - _zzq_args[3] = (unsigned int)(_zzq_arg3); \ - _zzq_args[4] = (unsigned int)(_zzq_arg4); \ - _zzq_args[5] = (unsigned int)(_zzq_arg5); \ - __asm__ volatile(__SPECIAL_INSTRUCTION_PREAMBLE \ - /* %EDX = client_request ( %EAX ) */ \ - "xchgl %%ebx,%%ebx" \ - : "=d" (_zzq_result) \ - : "a" (&_zzq_args[0]), "0" (_zzq_default) \ - : "cc", "memory" \ - ); \ - _zzq_result; \ - }) + valgrind_do_client_request_expr((unsigned int)(_zzq_default), \ + (_zzq_request), (unsigned int)(_zzq_arg1), \ + (unsigned int)(_zzq_arg2), (unsigned int)(_zzq_arg3), \ + (unsigned int)(_zzq_arg4), (unsigned int)(_zzq_arg5)) + +static __inline__ unsigned int +valgrind_do_client_request_expr(unsigned int _zzq_default, + unsigned int _zzq_request, + unsigned int _zzq_arg1, unsigned int _zzq_arg2, + unsigned int _zzq_arg3, unsigned int _zzq_arg4, + unsigned int _zzq_arg5); +static __inline__ unsigned int +valgrind_do_client_request_expr(unsigned int _zzq_default, + unsigned int _zzq_request, + unsigned int _zzq_arg1, unsigned int _zzq_arg2, + unsigned int _zzq_arg3, unsigned int _zzq_arg4, + unsigned int _zzq_arg5) +{ + volatile unsigned int _zzq_args[6]; + volatile unsigned int _zzq_result; + + _zzq_args[0] = _zzq_request; + _zzq_args[1] = _zzq_arg1; + _zzq_args[2] = _zzq_arg2; + _zzq_args[3] = _zzq_arg3; + _zzq_args[4] = _zzq_arg4; + _zzq_args[5] = _zzq_arg5; + __asm__ volatile(__SPECIAL_INSTRUCTION_PREAMBLE + /* %EDX = client_request ( %EAX ) */ + "xchgl %%ebx,%%ebx" + : "=d" (_zzq_result) + : "a" (&_zzq_args[0]), "0" (_zzq_default) + : "cc", "memory" + ); + return _zzq_result; +} #define VALGRIND_GET_NR_CONTEXT(_zzq_rlval) \ { volatile OrigFn* _zzq_orig = &(_zzq_rlval);
Created attachment 59246 [details] Patch that implements the described proposal, v3. Modified the macros for invoking client requests such that no "return value not ignored as it should be" warnings are triggered. That patch has two annoying side effects unfortunately: - The output of many regression tests changes. - Call stacks generated from inside a client request become dependent on the gcc version. As an example, here is the diff for the regression test memcheck/tests/badfree between gcc 4.5 (amd64) and gcc 4.3 (ppc): $ cat memcheck/tests/badfree.stderr.diff --- badfree.stderr.exp 2011-04-21 14:49:18.000000000 -0400 +++ badfree.stderr.out 2011-04-23 06:48:46.000000000 -0400 @@ -1,10 +1,10 @@ Invalid free() / delete / delete[] / realloc() - at 0x........: free (valgrind.h:...) + at 0x........: free (vg_replace_malloc.c:...) by 0x........: main (badfree.c:12) Address 0x........ is not stack'd, malloc'd or (recently) free'd Invalid free() / delete / delete[] / realloc() - at 0x........: free (valgrind.h:...) + at 0x........: free (vg_replace_malloc.c:...) by 0x........: main (badfree.c:15) Address 0x........ is on thread 1's stack
Bart, did you see the macros DO_CREQ_{v,W}_{W,WW,WWW,etc} in helgrind/hg_intercepts.c? These provide a convenient way to write client requests and require the __attribute__((unused)) to be placed only in these macros, and nowhere else. They also don't change the stack traces AFAIK.
(In reply to comment #4) > Bart, did you see the macros DO_CREQ_{v,W}_{W,WW,WWW,etc} in > helgrind/hg_intercepts.c? These provide a convenient way to > write client requests and require the __attribute__((unused)) > to be placed only in these macros, and nowhere else. They also > don't change the stack traces AFAIK. I'm not sure that I understood the above comment - have you noticed that in the attached patch the Helgrind DO_CREQ_{v,W}_W* macros have been modified such that it is no longer necessary to use __attribute__((unused)) ?
Created attachment 59548 [details] Patch that implements the described proposal, v4. I've found a simpler approach than the inline functions introduced in v3 to suppress the warnings generated by gcc 4.5 on the regression test programs: insert a few void casts (about 10). That means that version four of the patch is much closer to version two than to version 3.
*** Bug 272986 has been marked as a duplicate of this bug. ***
Applied as r11755 on the trunk.