[Bug c/104633] New: [12 Regression] -Winfinite-recursion diagnoses fortify wrappers

[Bug c/104633] New: [12 Regression] -Winfinite-recursion diagnoses fortify wrappers

[rguenth at gcc dot gnu.org]

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

            Bug ID: 104633
           Summary: [12 Regression] -Winfinite-recursion diagnoses fortify
                    wrappers
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rguenth at gcc dot gnu.org
  Target Milestone: ---

extern inline __attribute__((always_inline))
int memcmp (const void * p, const void *q, unsigned long size)
{
  return __builtin_memcmp (p, q, size);
}

are diagnosed with

t.c: In function 'memcmp':
t.c:2:5: warning: infinite recursion detected [-Winfinite-recursion]
    2 | int memcmp (const void * p, const void *q, unsigned long size)
      |     ^~~~~~
t.c:4:10: note: recursive call
    4 |   return __builtin_memcmp (p, q, size);
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This pattern happens in glibc fortify wrappers (but need -Wsystem-headers in
addition to -Wall).  It's reportedly also triggering for kernel wrappers
in its fortify-string.h which does not get the benefit of doubt via
-Wsystem-headers.

These kind of wrappers are not recursions (I think the kernel doesn't have
'extern' on the inline), inline instances will not be called itself and
__builtin_XXX are never inline expanded(?).

[Bug c/104633] [12 Regression] -Winfinite-recursion diagnoses fortify wrappers

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic
   Target Milestone|---                         |12.0
                 CC|                            |msebor at gcc dot gnu.org

[Bug c/104633] [12 Regression] -Winfinite-recursion diagnoses fortify wrappers

[rguenth at gcc dot gnu.org]

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

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Note the inline function needs __attribute__((gnu_inline)) or -fgnu89-inline to
not compile to an endless recursion, but we then still get the undesired
diagnostic.

[Bug c/104633] [12 Regression] -Winfinite-recursion diagnoses fortify wrappers

[rguenth at gcc dot gnu.org]

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

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
The following is a valid use of extern inline I think

extern int memcmp (const void * p, const void *q, unsigned long size);

extern inline __attribute__((always_inline,gnu_inline))
int memcmp (const void * p, const void *q, unsigned long size)
{
  return memcmp (p, q, size);
}

int foo (const void * p, const void *q)
{
  return memcmp (p, q, 4);
}

I think the user needs to avoid name-lookup issues here by using an alias
for the call to memcmp in the inline wrapper which might also avoid the
diagnostic.  That __builtin_memcmp finds the memcmp definition is quite
unfortunate and makes it not usable as a way to avoid using an alias.

[Bug c/104633] [12 Regression] -Winfinite-recursion diagnoses fortify wrappers

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #2)
> The following is a valid use of extern inline I think
> 
> extern int memcmp (const void * p, const void *q, unsigned long size);
> 
> extern inline __attribute__((always_inline,gnu_inline))
> int memcmp (const void * p, const void *q, unsigned long size)
> {
>   return memcmp (p, q, size);
> }

I think the above isn't actually valid, the compiler should still inline it
infinitely.  This is an infinite recursion.
extern inline __attribute__((always_inline,gnu_inline))
int memcmp (const void * p, const void *q, unsigned long size)
{
  return __builtin_memcmp (p, q, size);
}
is ok, __builtin_memcmp there doesn't mean it should use the user memcmp
inline, it should handle it as the builtin and if it decides it wants to call,
will call memcmp (but the out of line one).

What glibc uses are either the call __builtin_whatever forms, or call an alias,
say:
extern int __memcmp_alias (const void *, const void *, unsigned long) __asm
("memcmp");

extern inline __attribute__((always_inline,gnu_inline))
int memcmp (const void * p, const void *q, unsigned long size)
{
  return __memcmp_alias (p, q, size);
}
This one is also fine, it should call the external function, not the inline
recursively.
So, -Winfinite-recursion shouldn't warn about these 2 forms and should warn
about the #c2 case.

[Bug c/104633] [12 Regression] -Winfinite-recursion diagnoses fortify wrappers

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-02-22
     Ever confirmed|0                           |1
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 52491
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52491&action=edit
gcc12-pr104633.patch

Untested fix.

[Bug c/104633] [12 Regression] -Winfinite-recursion diagnoses fortify wrappers

[egallager at gcc dot gnu.org]

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

Eric Gallager <egallager at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |egallager at gcc dot gnu.org
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=56888

--- Comment #5 from Eric Gallager <egallager at gcc dot gnu.org> ---
I'd think it'd be worth checking to see if this affects the cases from bug
56888, too...

[Bug c/104633] [12 Regression] -Winfinite-recursion diagnoses fortify wrappers

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:c8cb5098c7854a1ed07e85c6165ef0c348d6df1d

commit r12-7358-gc8cb5098c7854a1ed07e85c6165ef0c348d6df1d
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Feb 23 12:03:55 2022 +0100

    warn-recursion: Don't warn for __builtin_calls in gnu_inline extern inline
functions [PR104633]

    The first two testcases show different ways how e.g. the glibc
    _FORTIFY_SOURCE wrappers are implemented, and on Winfinite-recursion-3.c
    the new -Winfinite-recursion warning emits a false positive warning.

    It is a false positive because when a builtin with 2 names is called
    through the __builtin_ name (but not all builtins have a name prefixed
    exactly like that) from extern inline function with gnu_inline semantics,
    it doesn't mean the compiler will ever attempt to use the user inline
    wrapper for the call, the __builtin_ just does what the builtin function
    is expected to do and either expands into some compiler generated code,
    or if the compiler decides to emit a call it will use an actual definition
    of the function, but that is not the extern inline gnu_inline function
    which is never emitted out of line.
    Compared to that, in Winfinite-recursion-5.c the extern inline gnu_inline
    wrapper calls the builtin by the same name as the function's name and in
    that case it is infinite recursion, we actuall try to inline the recursive
    call and also error because the recursion is infinite during inlining;
    without always_inline we wouldn't error but it is still infinite recursion,
    the user has no control on how many recursive calls we actually inline.

    2022-02-22  Jakub Jelinek  <jakub@redhat.com>

            PR c/104633
            * gimple-warn-recursion.cc
(pass_warn_recursion::find_function_exit):
            Don't warn about calls to corresponding builtin from extern inline
            gnu_inline wrappers.

            * gcc.dg/Winfinite-recursion-3.c: New test.
            * gcc.dg/Winfinite-recursion-4.c: New test.
            * gcc.dg/Winfinite-recursion-5.c: New test.

[Bug c/104633] [12 Regression] -Winfinite-recursion diagnoses fortify wrappers

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.