PR32219, weak hidden reference segfault

Hi, with reference to the old dicussion on PR 32219:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32219

It seems that a patch was submitted to put the DECL_WEAK check before
the visibility check, but that patch was never approved or applied, due
to concerns in the wording of surrounding comments:
http://gcc.gnu.org/ml/gcc-patches/2010-03/msg00666.html

That patch does solve the segfault in the PR, and happens to also solve
the other weak-hidden + section-anchor issue Nathan's other patch solves
(simply because it works the same way by rejecting DECL_WEAK inside
binds_local_p).

However, my own testing of the PR patch on recent trunk indicates a
regression: it filters out the TLS wrapper functions for C++11
thread_local variables, causing them to be called by @PLT, and failing a
few tests that check for this.

Looking into the generated code for:

extern void weakfun() __attribute__((weak,visibility("hidden")));
void foo ()
{
  if (weakfun) weakfun();
}

Under -O1 -m32 -fPIC, the address load and test will look like:

(insn 5 2 6 2 (set (reg/f:SI 60)
   (plus:SI (reg:SI 3 bx)
      (const:SI (unspec:SI [
        (symbol_ref/i:SI ("f") [flags 0x43]  <function_decl f>)
                    ] UNSPEC_GOTOFF)))) {*leasi}
     (expr_list:REG_EQUAL (symbol_ref/i:SI ("f") <function_decl f>)
        (nil)))

(insn 6 5 7 2 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg/f:SI 60)
            (const_int 0 [0]))) p.c:8 3 {*cmpsi_ccno_1}
     (nil))

(jump_insn 7 6 8 2 ...

However, the logic currently used in rtlanal.c:nonzero_address_p() only
test for "PIC-reg + <constant_p>", instead of more sophisticated
checking to expose the wrapped weak symbol_ref, thus confusing CSE to
eliminate the needed test.

The DECL_WEAK test upward movement in binds_local_p() works because when
bound non-local, x86 expand turns the address load into (mem (plus
(const (unspec ... UNSPEC_GOT)))) form, with the MEM helping to avoid
the above case.

Attached is a patch to make the nonzero_address_p() work for this case,
bootstrapped and tested on i686-linux without regressions.

I have the impression from the PR32219 discussion, that the solution
should be to make all weak-hidden-undefined symbols as potentially
non-local.  However, I don't think that is needed, no? As long as the
linker added zero value is in the same module, weak hidden semantics
should remain just the same...

Thanks,
Chung-Lin

2013-05-09  Chung-Lin Tang  <cltang@codesourcery.com>

	PR target/32219
	* rtlanal.c (nonzero_address_p): Robustify checking by look
        recursively into PIC constant offsets and (CONST (UNSPEC ...))
	expressions.

On Thu, May 09, 2013 at 05:52:26PM +0800, Chung-Lin Tang wrote:

>2013-05-09  Chung-Lin Tang  <cltang@codesourcery.com>
>
>	PR target/32219
>	* rtlanal.c (nonzero_address_p): Robustify checking by look
>        recursively into PIC constant offsets and (CONST (UNSPEC ...))
>	expressions.

>Index: rtlanal.c
>===================================================================
>--- rtlanal.c	(revision 198735)
>+++ rtlanal.c	(working copy)
>@@ -387,13 +387,22 @@ nonzero_address_p (const_rtx x)
>       return false;
> 
>     case CONST:
>-      return nonzero_address_p (XEXP (x, 0));
>+      {
>+	rtx base, offset;
>+	/* Peel away any constant offsets from the base symbol.  */
>+	split_const (CONST_CAST_RTX (x), &base, &offset);
>+	return nonzero_address_p (base);
>+      }
> 
>+    case UNSPEC:
>+      /* Reach for a contained symbol.  */
>+      return nonzero_address_p (XVECEXP (x, 0, 0));
>+
>     case PLUS:
>       /* Handle PIC references.  */
>       if (XEXP (x, 0) == pic_offset_table_rtx
> 	       && CONSTANT_P (XEXP (x, 1)))
>-	return true;
>+	return nonzero_address_p (XEXP (x, 1));
>       return false;
> 
>     case PRE_MODIFY:
>Index: testsuite/gcc.dg/visibility-21.c
>===================================================================
>--- testsuite/gcc.dg/visibility-21.c	(revision 0)
>+++ testsuite/gcc.dg/visibility-21.c	(revision 0)

Please put this into gcc.dg/torture/ instead to make sure it works on
all optimization levels.

>@@ -0,0 +1,12 @@
>+/* PR target/32219 */
>+/* { dg-do run } */
>+/* { dg-require-visibility "" } */
>+/* { dg-options "-O1 -fPIC" { target fpic } } */

I do not remember offhand if the whole test should
dg-require-effective-target fpic
>+
>+extern void f() __attribute__((weak,visibility("hidden")));
>+int main()
>+{
>+  if (f)
>+    f();
>+  return 0;
>+}

Thanks for taking care of this PR!
Bernhard

Chung-Lin Tang <cltang@codesourcery.com> writes:
> +    case UNSPEC:
> +      /* Reach for a contained symbol.  */
> +      return nonzero_address_p (XVECEXP (x, 0, 0));

I don't think this is safe.  UNSPECs really are unspecified :-),
so we can't assume that (unspec X) is nonzero simply because X is.

Richard