[Bug target/104901] New: gcc/config/rs6000/mm_malloc.h:46: incorrectLogicOperator

[Bug target/104901] New: gcc/config/rs6000/mm_malloc.h:46: incorrectLogicOperator

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104901

            Bug ID: 104901
           Summary: gcc/config/rs6000/mm_malloc.h:46:
                    incorrectLogicOperator
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dcb314 at hotmail dot com
  Target Milestone: ---

Static analyser cppcheck says:

gcc/config/rs6000/mm_malloc.h:46:37: warning: Logical conjunction always
evaluates to false: __alignment == __malloc_align && __alignment ==
__vec_align. [incorrectLogicOperator]

Suggest replace && with ||

[Bug target/104901] gcc/config/rs6000/mm_malloc.h:46: incorrectLogicOperator

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104901

David Binderman <dcb314 at hotmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pc at us dot ibm.com

--- Comment #1 from David Binderman <dcb314 at hotmail dot com> ---
git blame says:

efbb17db52af (Paul A. Clarke 2022-02-16 20:01:41 -0600 46)   if (__alignment ==
__malloc_align && __alignment == __vec_align)

[Bug target/104901] gcc/config/rs6000/mm_malloc.h:46: incorrectLogicOperator

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104901

--- Comment #2 from Andreas Schwab <schwab@linux-m68k.org> ---
Already present in the first commit 281de9c2d56.

[Bug target/104901] gcc/config/rs6000/mm_malloc.h:46: incorrectLogicOperator

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104901

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
It's a way to say a == b == c, no?  Not sure if that was intended of course,
but it seems so since we follow with

  if (__alignment < __vec_align)
    __alignment = __vec_align;

[Bug target/104901] gcc/config/rs6000/mm_malloc.h:46: incorrectLogicOperator

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104901

Segher Boessenkool <segher at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |segher at gcc dot gnu.org

--- Comment #4 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Yes, the code is correct as written.  There are many ways to avoid this warning
of course (and the warning is too smart for its own good, imnsho -- if it read
"__alignment == 16 && __alignment == 16" that would be a different thing, but
as-is this just invites us to write less clear (i.e, worse!) code to avoid the
warning.

On the other hand.  Paul, is this micro-optimisation useful at all, don't
posix_memalign and malloc end up the same under the covers anyway?

[Bug target/104901] gcc/config/rs6000/mm_malloc.h:46: incorrectLogicOperator

[pc at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104901

pc at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pc at gcc dot gnu.org

--- Comment #5 from pc at gcc dot gnu.org ---
(In reply to Segher Boessenkool from comment #4)
> is this micro-optimisation useful at all, don't
> posix_memalign and malloc end up the same under the covers anyway?

posix_memalign returns memory aligned to a specified power-of-2 alignment.
malloc returns memory aligned to some ABI minimum. (You already know this, I'm
sure.) The code will use malloc if it can, and posix_memalign otherwise. There
may be a slight advantage to using malloc instead of posix_memalign. The paths
are indeed different. I'm not sure why the floor is raised after determining
not to call malloc:
--
  if (__alignment == __malloc_align && __alignment == __vec_align)
    return malloc (__size);
  if (__alignment < __vec_align)
    __alignment = __vec_align;
--
(I probably would've written the code slightly differently.)

It appears to me that the identified code would be always false on a 32-bit
system, where __malloc_align would be computed as 64 bits, and _vec_align as
128 bits.  It would be always true on a 64-bit system (128 == 128).

All that being said, I'm not sure I see any problem with the code, and maybe
the analyzer is being a bit overzealous?

FWIW, the code was likely taken as an analog to gcc/config/i386/pmm_malloc.h:
--
  if (__alignment == 1)
    return malloc (__size);
  if (__alignment == 2 || (sizeof (void *) == 8 && __alignment == 4))
    __alignment = sizeof (void *);
  if (posix_memalign (&__ptr, __alignment, __size) == 0)
    return __ptr;
--

[Bug target/104901] gcc/config/rs6000/mm_malloc.h:46: incorrectLogicOperator

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104901

Segher Boessenkool <segher at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #6 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to pc from comment #5)
> The code will use malloc if it can, and posix_memalign otherwise.

If it knows it can based on some very simplistic criterion, yes.  Is this
a useful optimisation, or is this premature optimisation?

> There may be a slight advantage to using malloc instead of posix_memalign.
> The paths are indeed different. I'm not sure why the floor is raised after
> determining not to call malloc:
> --
>   if (__alignment == __malloc_align && __alignment == __vec_align)
>     return malloc (__size);
>   if (__alignment < __vec_align)
>     __alignment = __vec_align;
> --

If it does call malloc it *also* does this, it is just that __alignment
already is the same then (and this function does an early out, too) ;-)

> (I probably would've written the code slightly differently.)

Yeah.

> It appears to me that the identified code would be always false on a 32-bit
> system, where __malloc_align would be computed as 64 bits, and _vec_align as
> 128 bits.  It would be always true on a 64-bit system (128 == 128).

Likely, yes.

> All that being said, I'm not sure I see any problem with the code, and maybe
> the analyzer is being a bit overzealous?

That's my point, yes.

- * -

Ah.  This is some 3rd party checker.  Closing as INVALID.

[Bug target/104901] gcc/config/rs6000/mm_malloc.h:46: incorrectLogicOperator

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104901

--- Comment #7 from Andreas Schwab <schwab@linux-m68k.org> ---
I think it would still be worthwhile to avoid the antipattern (v == c1 && v ==
c2).

[Bug target/104901] gcc/config/rs6000/mm_malloc.h:46: incorrectLogicOperator

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104901

--- Comment #8 from Segher Boessenkool <segher at gcc dot gnu.org> ---
Sure, but c1 as well as c2 are not constants here!

[Bug target/104901] gcc/config/rs6000/mm_malloc.h:46: incorrectLogicOperator

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104901

--- Comment #9 from David Binderman <dcb314 at hotmail dot com> ---
(In reply to Andreas Schwab from comment #7)
> I think it would still be worthwhile to avoid the antipattern (v == c1 && v
> == c2).

+1

I haven't got a rs6000 box, but I suspect our old friend -Wlogical-op
would find this problem also.

[Bug target/104901] gcc/config/rs6000/mm_malloc.h:46: incorrectLogicOperator

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104901

--- Comment #10 from Andreas Schwab <schwab@linux-m68k.org> ---
The values depend on the target, but they are genuine compile time constants.

[Bug target/104901] gcc/config/rs6000/mm_malloc.h:46: incorrectLogicOperator

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104901

--- Comment #11 from Segher Boessenkool <segher at gcc dot gnu.org> ---
$ cat vcc.c
int f(int x) { return x == 31 && x == 42; }

-Wlogical-op gives

vcc.c: In function 'f':
vcc.c:1:31: warning: logical 'and' of mutually exclusive tests is always false
[-Wlogical-op]
    1 | int f(int x) { return x == 31 && x == 42; }
      |                               ^~

The mm_malloc.h code does not warn here (neither -m32 nor -m64 does).