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

[Florian K. <fl...@ei...>]

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.

> @@ -4819,7 +4819,7 @@ static Bool handle_self_exe_open(SyscallStatus *status, const HChar *filename,
>   
>      /* Opening /proc/<pid>/exe or /proc/self/exe? */
>      VG_(sprintf)(name, "/proc/%d/exe", VG_(getpid)());
> -   if (!VG_STREQ(filename, name) && !VG_STREQ(filename, "/proc/self/exe"))
> +   if ((VG_(strcmp)(filename, name)!=0) && !VG_STREQ(filename, "/proc/self/exe"))

ditto.

> @@ -4910,7 +4910,7 @@ PRE(sys_open)
>   
>         VG_(sprintf)(name, "/proc/%d/cmdline", VG_(getpid)());
>         if (ML_(safe_to_deref)( arg1s, 1 )
> -          && (VG_STREQ(arg1s, name) || VG_STREQ(arg1s, "/proc/self/cmdline"))) {
> +          && (VG_(strcmp)(arg1s, name)==0 || VG_STREQ(arg1s, "/proc/self/cmdline"))) {
>            sres = VG_(dup)( VG_(cl_cmdline_fd) );
>            SET_STATUS_from_SysRes( sres );
>            if (!sr_isError(sres)) {

ditto

> @@ -5094,7 +5094,7 @@ PRE(sys_readlink)
>         HChar* arg1s = (HChar*) (Addr)ARG1;
>         VG_(sprintf)(name, PID_EXEPATH, VG_(getpid)());
>         if (ML_(safe_to_deref)(arg1s, 1)
> -          && (VG_STREQ(arg1s, name) || VG_STREQ(arg1s, SELF_EXEPATH))) {
> +          && (VG_(strcmp)(arg1s, name)==0 || VG_STREQ(arg1s, SELF_EXEPATH))) {

ditto

> 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?

    Florian