[Bug middle-end/103310] New: null comparison with a weak symbol eliminated

[Bug middle-end/103310] New: null comparison with a weak symbol eliminated

[msebor at gcc dot gnu.org]

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

            Bug ID: 103310
           Summary: null comparison with a weak symbol eliminated
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: msebor at gcc dot gnu.org
  Target Milestone: ---

The following test case was prompted by the discussion in the review of a
-Waddress enhancement:
  https://gcc.gnu.org/pipermail/gcc-patches/2021-November/584749.html

Before folding the test in call_alias() to true, GCC issues a -Waddress.  At
runtime the program then crashes.  In the review Jason suggests GCC should
reject the subsequent declaration of the alias with an error because it has
been used (and because the test for null may have been eliminated).

cat a.c && gcc -DUSE_ALIAS -O2 -Wall -c a.c && gcc -O2 -Wall a.c a.o && ./a.out
#if USE_ALIAS

extern void alias (void);

void call_alias (void)
{
  __builtin_printf ("in %s: alias = %p\n", __func__, alias);

  if (alias)
    alias ();
}

extern void alias (void)  __attribute__((weak));

#else

extern void alias (void)  __attribute__((weak));   // not defined

extern void call_alias (void);

int main (void)
{
  __builtin_printf ("in %s: alias = %p\n", __func__, alias);

  call_alias ();
}

#endif
a.c: In function ‘call_alias’:
a.c:9:7: warning: the address of ‘alias’ will always evaluate as ‘true’
[-Waddress]
    9 |   if (alias)
      |       ^~~~~
in main: alias = (nil)
in call_alias: alias = (nil)
Segmentation fault (core dumped)

[Bug c/103310] null comparison with a weak symbol eliminated

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|middle-end                  |c

--- Comment #1 from Martin Sebor <msebor at gcc dot gnu.org> ---
The test case below shows that storing the address of the alias in a local
pointer avoids the problem.  The dumps show that the problem is in the front
end which folds the test of the address of the weak symbol to true.

$ cat pr103310.c && gcc -Wall -O2 -S -fdump-tree-original=/dev/stdout
-fdump-tree-optimized=/dev/stdout pr103310.c
extern void alias (void);

void call_alias (void)
{
  __builtin_printf ("in %s: alias = %p\n", __func__, alias);

  if (alias)
    alias ();
}

void call_ptr_alias (void)
{
  void (*p)(void) = alias;

  __builtin_printf ("in %s: alias = %p\n", __func__, p);

  if (p)
    p ();
}

extern void alias (void)  __attribute__((weak));
pr103310.c: In function ‘call_alias’:
pr103310.c:7:7: warning: the address of ‘alias’ will always evaluate as ‘true’
[-Waddress]
    7 |   if (alias)
      |       ^~~~~

;; Function call_alias (null)
;; enabled by -tree-original


{
  static const char __func__[11] = "call_alias";

    static const char __func__[11] = "call_alias";
  __builtin_printf ((const char *) "in %s: alias = %p\n", (const char *)
&__func__, alias);
  if (1)
    {
      alias ();
    }
}


;; Function call_ptr_alias (null)
;; enabled by -tree-original


{
  void (*<T349>) (void) p = alias;
  static const char __func__[15] = "call_ptr_alias";

    static const char __func__[15] = "call_ptr_alias";
    void (*<T349>) (void) p = alias;
  __builtin_printf ((const char *) "in %s: alias = %p\n", (const char *)
&__func__, p);
  if (p != 0B)
    {
      p ();
    }
}


;; Function call_alias (call_alias, funcdef_no=0, decl_uid=1945, cgraph_uid=1,
symbol_order=0)

void call_alias ()
{
  static const char __func__[11] = "call_alias";

  <bb 2> [local count: 1073741824]:
  __builtin_printf ("in %s: alias = %p\n", &__func__, alias);
  alias (); [tail call]
  return;

}



;; Function call_ptr_alias (call_ptr_alias, funcdef_no=1, decl_uid=1949,
cgraph_uid=2, symbol_order=1)

Removing basic block 5
void call_ptr_alias ()
{
  static const char __func__[15] = "call_ptr_alias";

  <bb 2> [local count: 1073741824]:
  __builtin_printf ("in %s: alias = %p\n", &__func__, alias);
  if (alias != 0B)
    goto <bb 3>; [53.47%]
  else
    goto <bb 4>; [46.53%]

  <bb 3> [local count: 574129753]:
  alias (); [tail call]

  <bb 4> [local count: 1073741824]:
  return;

}

[Bug c/103310] null comparison with a weak symbol eliminated

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
                 CC|                            |jason at gcc dot gnu.org
   Last reconfirmed|                            |2021-11-18

--- Comment #2 from Jason Merrill <jason at gcc dot gnu.org> ---
The C++ front end rejects the testcase with "declared weak after used".  We set
refuse_visibility_changes under fold's tree_single_nonzero_warnv_p. 

Changing get_create to get in maybe_nonzero_address makes the C++ compiler
accept the testcase, and properly test whether alias is null.

This issue seems to go back to Honza's r5-3627, which changed symtab_node::get
to symtab_node::get_create in the code that became maybe_nonzero_address, so
that we decide early whether a particular function is weak or not.

This was done so that constant-evaluation could properly decide that the
address of a function is non-null.  But it's harmful when we do that for
speculative folding; we should only return a definitive answer, and set
refuse_visibility_changes, when a constant result is required.

It seems we need a way to tell fold that we really want a constant value, have
the C++ front end set that for manifestly-constant-evaluated expressions, and
only use get_create in that case.

But I also guess the issue that the C front end is both optimizing away the
test and not setting refuse_visibility_changes is a C front end issue, that
it's doing the optimization without involving fold.

[Bug c/103310] null comparison with a weak symbol eliminated

[cvs-commit at gcc dot gnu.org]

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

--- Comment #3 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:53caa4723d8de73fe21e63ba264082f3071b2887

commit r12-5696-g53caa4723d8de73fe21e63ba264082f3071b2887
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Nov 24 05:45:02 2021 -0500

    c++: constexpr, fold, weak redecl, fp/0 [PR103310]

    For PR61825, honza changed tree_single_nonzero_warnv_p to prevent a later
    declaration from marking a function as weak after we've determined that it
    wasn't weak before.  But we shouldn't do that for speculative folding; we
    should only do it when we actually need a constant value.  In C++, such a
    context is called "manifestly constant-evaluated".  In fold, this seems to
    correspond to the folding_initializer flag, since in C this situation only
    occurs in static initializers.

    This change makes nonzero-1.c well-formed; I've added a nonzero-1a.c to
    verify that we delete the null check eventually if there is no weak
    redeclaration.

    The varasm.c change is so that if we do get the weak redeclaration error,
we
    get it at the position of the weak declaration rather than the previous
    declaration.

    Using the FOLD_INIT paths also affects floating point arithmetic: notably,
    this makes floating point division by zero in a manifestly
    constant-evaluated context constant, as in a C static initializer.  I've
had
    some success convincing CWG that this is the right direction; C++ should
    follow C's floating point semantics more than we have been doing, and
Joseph
    says that the C policy is that Annex F overrides other parts of the
standard
    that say that some operations are undefined.  But since we're in stage 3,
    I'm only making this change with the new flag -fconstexpr-fp-except.  It
may
    turn on by default in a future release.

    I think this distinction is only relevant for binary operations; arithmetic
    for the floating point case, comparison for possibly non-zero addresses.

            PR c++/103310

    gcc/ChangeLog:

            * fold-const.c (maybe_nonzero_address): Use get_create or get
            depending on folding_initializer.
            (fold_binary_initializer_loc): New.
            * fold-const.h (fold_binary_initializer_loc): Declare.
            * varasm.c (mark_weak): Don't use the decl location.
            * doc/invoke.texi: Document -fconstexpr-fp-except.

    gcc/c-family/ChangeLog:

            * c.opt: Add -fconstexpr-fp-except.

    gcc/cp/ChangeLog:

            * constexpr.c (cxx_eval_binary_expression): Use
            fold_binary_initializer_loc if manifestly cxeval.

    gcc/testsuite/ChangeLog:

            * g++.dg/cpp0x/constexpr-fp-except1.C: New test.
            * g++.dg/cpp1z/constexpr-if36.C: New test.
            * gcc.dg/tree-ssa/nonzero-1.c: Now well-formed.
            * gcc.dg/tree-ssa/nonzero-1a.c: New test.