Re: [Valgrind-developers] [valgrind] Fix GCC -Waddress warnings

[Paul F. <pj...@wa...>]

Hi Florian

On 2026-03-13 23:58, Florian Krohm wrote:
> Hi Paul,
>
> On 3/13/26 23:27, Paul Floyd wrote:
>
>> diff --git a/coregrind/m_syswrap/syswrap-generic.c 
>> b/coregrind/m_syswrap/syswrap-generic.c
>> index 1cd592196b..3d31c53e72 100644
>> --- a/coregrind/m_syswrap/syswrap-generic.c
>> +++ b/coregrind/m_syswrap/syswrap-generic.c
>> @@ -4789,7 +4789,7 @@ static Bool handle_auxv_open(SyscallStatus 
>> *status, const HChar *filename,
>>        /* Opening /proc/<pid>/auxv or /proc/self/auxv? */
>>      VG_(sprintf)(name, "/proc/%d/auxv", VG_(getpid)());
>> -   if (!VG_STREQ(filename, name) && !VG_STREQ(filename, 
>> "/proc/self/auxv"))
>> +   if ((VG_(strcmp)(filename, name)!=0) && !VG_STREQ(filename, 
>> "/proc/self/auxv"))
>
> IIUC you changed !VG_STREQ(filename, name) to VG_(strcmp)(filename, 
> name)!=0 because 'name' cannot be NULL (and 'filename' is safe to 
> deref). That means the 2nd occurrence of VG_STREQ on that line is safe 
> to be changed to strcmp as well. Even though you did not get a warning 
> about that.
> I would find it less puzzling if the other VG_STREQ be changed as well.

I did consider turning off -Waddress. In all these case I expect the the 
compiler will just optimise away the redundant checks. But then I 
thought that for new code it is useful to avoid "if (true)" cases.

Then I tried a few things. Some nasty casts to hide the fact that these 
are arrays. I didn't like that. Then I tried an unchecked version of the 
macro, and I quickly realised that we would need 4 variants checking 
both, neither, only s1 and only s2. That was too complicated. So I 
switched to just using VG_(str[n]cmp).

I agree that this is less readable, particularly as str[n]cmp doesn't 
return true/false for string equality like the macros.

>
>> diff --git a/coregrind/m_ume/main.c b/coregrind/m_ume/main.c
>> @@ -72,9 +72,9 @@ extern double VG_(strtod)  ( const HChar* str, 
>> HChar** endptr );
>> ------------------------------------------------------------------ */
>>     /* Use this for normal null-termination-style string comparison. */
>> -#define VG_STREQ(s1,s2) ( (s1 != NULL && s2 != NULL \
>> +#define VG_STREQ(s1,s2) ( ((s1) != NULL && (s2) != NULL \
>>                              && VG_(strcmp)((s1),(s2))==0) ? True : 
>> False )
>> -#define VG_STREQN(n,s1,s2) ( (s1 != NULL && s2 != NULL \
>> +#define VG_STREQN(n,s1,s2) ( ((s1) != NULL && (s2) != NULL \
>>                                && VG_(strncmp)((s1),(s2),(n))==0) ? 
>> True : False )
>
> Yes! Macro arguments should always be parenthesised in the replacement 
> text.
> Did you use some tool for that or your eagle eyes? 


I saw that when I was playing around with casts and non checking macros. 
I did use claude.ai free tier to check the final diff for typos and 
silly mistakes.


A+

Paul