[RFA:] Fix PR37170: weak-1.c regression, second edition

[RFA:] Fix PR37170: weak-1.c regression, second edition

[Hans-Peter Nilsson]

Main points:
- #ifndef ASM_OUTPUT_EXTERNAL was around the weak-handling too.
- Inlined functions and emitted variables can be marked as weak;
  there must be no weak-*reference* annotation for them.
- output_operand doesn't catch all from code referenced
  symbol_ref's.

The need for the special Darwin hack was revealed by one of the
test-cases, I forgot which one, perhaps weak-1.c.  I hope all
this is temporarily, and that eventually the language front-ends
will stop calling assemble_external and that the expand pass
will handle all weak and external et al attributes.

The g++.dg/ext/inline1.C shows a case where an inlined function
is spuriously marked as weak (which with an earlier version of
this aptch broke on Darwin, whose assembler doesn't take kindly
to inconsistent weak annotations).  The testcase fails for
e.g. x86_64-unknown-linux-gnu.  The TREE_STATIC check fixes
this.  The gcc.dg/weak/weak-15 testcase doesn't really reveal
any flaws, it just covers a few holes in the weak-testing: I
missed tests that referenced a weak symbol, but offset, and the
address of a weak variable, but from data, not code.  Also in
there is kind-of an attempt to verify that the inlined-function-
marked-as-weak situation doesn't happen in C, though the code
calling assemble_external for that case was indeed specific to
C++.

I'd like to thank Dominique d'Humieres, Eric Weddington and
Andreas Tobler for testing various versions of this patch.

This version was bootstrapped and regtested on
x86_64-unknown-linux-gnu, i686-darwin9 and ppc-darwin9 and cross
to cris-elf.

Ok to commit?

gcc/testsuite:
	PR middle-end/37170
	* gcc.dg/weak/weak-15.c, g++.dg/ext/inline1.C: New tests.

gcc:
	PR middle-end/37170
	* final.c (mark_symbol_ref_as_used): New helper function.
	(output_operand): Instead of just looking inside MEMs for
	SYMBOL_REFs, use new helper function and for_each_rtx.
	* varasm.c (assemble_external): Move #ifndef ASM_OUTPUT_EXTERNAL
	to after weak-handling.  Return early also for decls with
	TREE_STATIC.  Make head comment more general.
	* config/darwin.c (machopic_output_indirection): Handle weak
	references here, like in assemble_external.


--- /dev/null	2008-02-22 17:23:55.052000250 +0100
+++ g++.dg/ext/inline1.C	2008-08-24 06:21:50.000000000 +0200
@@ -0,0 +1,34 @@
+// { dg-do compile }
+// { dg-options "-O" }
+// Make sure inlined non-outlined functions aren't marked weak.
+// We'd get a ".weak xyzzy" annotation trigged by the second declaration.
+
+// { dg-final { scan-assembler-not "weak\[^ \t\]*\[ \t\]_?xyzzy" } } 
+
+// The next check isn't really part of the actual test, just to make
+// sure there's no outline-copy of xyzzy, because if that really
+// happened, it *should* be marked linkonce or perhaps weak.
+// { dg-final { scan-assembler-not "xyzzy" } } 
+
+extern int x;
+extern void foo(void);
+extern void bar(void);
+
+extern "C" inline int xyzzy(int a)
+{
+  foo();
+  return a + x;
+}
+
+extern "C" int xyzzy(int);
+
+extern inline int plugh(int c)
+{
+  return xyzzy (c);
+}
+
+int y;
+void doit(int b)
+{
+  y = xyzzy (b) + plugh (b);
+}

--- /dev/null	2008-02-22 17:23:55.052000250 +0100
+++ gcc.dg/weak/weak-15.c	2008-08-25 03:41:31.000000000 +0200
@@ -0,0 +1,39 @@
+/* { dg-do compile } */
+/* { dg-require-weak "" } */
+/* { dg-options "-fno-common" } */
+
+/* { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?a" } } */
+/* { dg-final { scan-assembler-not "weak\[^ \t\]*\[ \t\]_?b" } } */
+/* { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?c" } } */
+/* { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?d" } } */
+
+#pragma weak a
+extern char a[];
+
+char *user_a(void)
+{
+  return a+1;
+}
+
+int x;
+int extern inline b(int y)
+{
+  return x+y;
+}
+
+extern int b(int y);
+
+int user_b(int z)
+{
+  return b(z);
+}
+
+#pragma weak c
+extern int c;
+
+int *user_c = &c;
+
+#pragma weak d
+extern char d[];
+
+char *user_d = &d[1];
Index: final.c
===================================================================
--- final.c	(revision 139624)
+++ final.c	(working copy)
@@ -3346,6 +3346,31 @@ output_asm_label (rtx x)
   assemble_name (asm_out_file, buf);
 }
 
+/* Helper rtx-iteration-function for output_operand.  Marks
+   SYMBOL_REFs as referenced through use of assemble_external.  */
+
+static int
+mark_symbol_ref_as_used (rtx *xp, void *dummy ATTRIBUTE_UNUSED)
+{
+  rtx x = *xp;
+
+  /* If we have a used symbol, we may have to emit assembly
+     annotations corresponding to whether the symbol is external, weak
+     or has non-default visibility.  */
+  if (GET_CODE (x) == SYMBOL_REF)
+    {
+      tree t;
+
+      t = SYMBOL_REF_DECL (x);
+      if (t)
+	assemble_external (t);
+
+      return -1;
+    }
+
+  return 0;
+}
+
 /* Print operand X using machine-dependent assembler syntax.
    The macro PRINT_OPERAND is defined just to control this function.
    CODE is a non-digit that preceded the operand-number in the % spec,
@@ -3366,14 +3391,11 @@ output_operand (rtx x, int code ATTRIBUT
   gcc_assert (!x || !REG_P (x) || REGNO (x) < FIRST_PSEUDO_REGISTER);
 
   PRINT_OPERAND (asm_out_file, x, code);
-  if (x && MEM_P (x) && GET_CODE (XEXP (x, 0)) == SYMBOL_REF)
-    {
-      tree t;
-      x = XEXP (x, 0);
-      t = SYMBOL_REF_DECL (x);
-      if (t)
-	assemble_external (t);
-    }
+
+  if (x == NULL_RTX)
+    return;
+
+  for_each_rtx (&x, mark_symbol_ref_as_used, NULL);
 }
 
 /* Print a memory reference operand for address X
Index: varasm.c
===================================================================
--- varasm.c	(revision 139624)
+++ varasm.c	(working copy)
@@ -2290,9 +2290,10 @@ process_pending_assemble_externals (void
    to be emitted.  */
 static GTY(()) tree weak_decls;
 
-/* Output something to declare an external symbol to the assembler.
-   (Most assemblers don't need this, so we normally output nothing.)
-   Do nothing if DECL is not external.  */
+/* Output something to declare an external symbol to the assembler,
+   and qualifiers such as weakness.  (Most assemblers don't need
+   extern declaration, so we normally output nothing.)  Do nothing if
+   DECL is not external.  */
 
 void
 assemble_external (tree decl ATTRIBUTE_UNUSED)
@@ -2303,15 +2304,20 @@ assemble_external (tree decl ATTRIBUTE_U
      open.  If it's not, we should not be calling this function.  */
   gcc_assert (asm_out_file);
 
-#ifdef ASM_OUTPUT_EXTERNAL
-  if (!DECL_P (decl) || !DECL_EXTERNAL (decl) || !TREE_PUBLIC (decl))
+  if (!DECL_P (decl) || !DECL_EXTERNAL (decl) || !TREE_PUBLIC (decl)
+      /* Handle only actual external-only definitions, not e.g. extern
+	 inline code or variables for which storage has been
+	 allocated.  */
+      || TREE_STATIC (decl))
     return;
 
+  /* We want to output annotation for weak and external symbols at
+     very last to check if they are references or not.  */
+
   if (SUPPORTS_WEAK && DECL_WEAK (decl))
     weak_decls = tree_cons (NULL, decl, weak_decls);
 
-  /* We want to output external symbols at very last to check if they
-     are references or not.  */
+#ifdef ASM_OUTPUT_EXTERNAL
   pending_assemble_externals = tree_cons (0, decl,
 					  pending_assemble_externals);
 #endif
Index: config/darwin.c
===================================================================
--- config/darwin.c	(revision 139624)
+++ config/darwin.c	(working copy)
@@ -1002,6 +1002,30 @@ machopic_output_indirection (void **slot
       rtx init = const0_rtx;
 
       switch_to_section (darwin_sections[machopic_nl_symbol_ptr_section]);
+
+      /* Mach-O symbols are passed around in code through indirect
+	 references and the original symbol_ref hasn't passed through
+	 the generic handling and reference-catching in
+	 output_operand, so we need to manually mark weak references
+	 as such.  */
+      if (SYMBOL_REF_WEAK (symbol))
+	{
+	  tree decl = SYMBOL_REF_DECL (symbol);
+	  gcc_assert (DECL_P (decl));
+
+	  if (decl != NULL_TREE
+	      && DECL_EXTERNAL (decl) && TREE_PUBLIC (decl)
+	      /* Handle only actual external-only definitions, not
+		 e.g. extern inline code or variables for which
+		 storage has been allocated.  */
+	      && !TREE_STATIC (decl))
+	    {
+	      fputs ("\t.weak_reference ", asm_out_file);
+	      assemble_name (asm_out_file, sym_name);
+	      fputc ('\n', asm_out_file);
+	    }
+	}
+
       assemble_name (asm_out_file, ptr_name);
       fprintf (asm_out_file, ":\n");
 

brgds, H-P

Call for HPPA testers (was: [RFA:] Fix PR37170: weak-1.c regression, second edition)

[Hans-Peter Nilsson]

> Date: Wed, 27 Aug 2008 18:29:43 +0200
> From: Hans-Peter Nilsson <hp@axis.com>

> Main points:
> - #ifndef ASM_OUTPUT_EXTERNAL was around the weak-handling too.
> - Inlined functions and emitted variables can be marked as weak;
>   there must be no weak-*reference* annotation for them.
> - output_operand doesn't catch all from code referenced
>   symbol_ref's.

...but those were beside the original point, that it's not
enough to, in output_operand, look inside mems to see all
required symbol_refs.

As mentioned, revision 138310 (and its add-on for PR 36974) is
an attempt to fix one special case of this.  With the previous
patch applied, it's redundant but benevolent except for being
misleading.  My guess is that it "worked" where output_operand
failed only because the mem(*) is usually outside, not part of
the match_operand part in call insns (the one reaching
output_operand), and luckily the reported platforms had a
symbol_ref there in bootstrap and tests (and not e.g. a reg).

Can someone with an affected system (HPPA) pretty please test
this patch, reverting 138310 and the later change, on top of my
previous one?

I've already regtested it on x86_64-unknown-linux-gnu.

	* final.c (call_from_call_insn): Delete.
	(final_scan_insn): Don't attempt to dig out and call
	assemble_external for SYMBOL_REFs of call insns here.

(*) There's always a "spurious" mem around the address in the
generated (call ...); Michael Meissner speculates that it was
because RMS knew of machines that could execute code in
registers.

Index: final.c
===================================================================
--- final.c	(revision 139233)
+++ final.c	(working copy)
@@ -1744,34 +1744,6 @@
     }
 }
 
-/* Given a CALL_INSN, find and return the nested CALL. */
-static rtx
-call_from_call_insn (rtx insn)
-{
-  rtx x;
-  gcc_assert (CALL_P (insn));
-  x = PATTERN (insn);
-
-  while (GET_CODE (x) != CALL)
-    {
-      switch (GET_CODE (x))
-	{
-	default:
-	  gcc_unreachable ();
-	case COND_EXEC:
-	  x = COND_EXEC_CODE (x);
-	  break;
-	case PARALLEL:
-	  x = XVECEXP (x, 0, 0);
-	  break;
-	case SET:
-	  x = XEXP (x, 1);
-	  break;
-	}
-    }
-  return x;
-}
-
 /* The final scan for one insn, INSN.
    Args are same as in `final', except that INSN
    is the insn being scanned.
@@ -2617,20 +2589,6 @@
 	targetm.asm_out.unwind_emit (asm_out_file, insn);
 #endif
 
-	if (CALL_P (insn))
-	  {
-	    rtx x = call_from_call_insn (insn);
-	    x = XEXP (x, 0);
-	    if (x && MEM_P (x) && GET_CODE (XEXP (x, 0)) == SYMBOL_REF)
-	      {
-		tree t;
-		x = XEXP (x, 0);
-		t = SYMBOL_REF_DECL (x);
-		if (t)
-		  assemble_external (t);
-	      }
-	  }
-
 	/* Output assembler code from the template.  */
 	output_asm_insn (templ, recog_data.operand);
 
@@ -3346,6 +3304,31 @@
   assemble_name (asm_out_file, buf);
 }
 
brgds, H-P

Re: Call for HPPA testers (was: [RFA:] Fix PR37170: weak-1.c regression, second edition)

[John David Anglin]

> Can someone with an affected system (HPPA) pretty please test
> this patch, reverting 138310 and the later change, on top of my
> previous one?

I'll try to give it a try this evening.  However, hppa is somewhat
sick.  I've been trying to fix a number of EH issues arising from
the use of CFI directions.  In addition, I'm seeing ICE's in
garbage collection that are breaking bootstrap (Ada and libstdc++).

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Re: Call for HPPA testers (was: [RFA:] Fix PR37170: weak-1.c regression, second edition)

[Hans-Peter Nilsson]

> Date: Wed, 27 Aug 2008 14:11:31 -0400 (EDT)
> From: "John David Anglin" <dave@hiauly1.hia.nrc.ca>

> > Can someone with an affected system (HPPA) pretty please test
> > this patch, reverting 138310 and the later change, on top of my

(I mean, just apply the two patches; no separate reverting or
patching needed.)

> > previous one?
> 
> I'll try to give it a try this evening.  However, hppa is somewhat
> sick.

If that happened recently, for testing this patch, any revision
after-and-including 139233 should do.

>  I've been trying to fix a number of EH issues arising from
> the use of CFI directions.  In addition, I'm seeing ICE's in
> garbage collection that are breaking bootstrap (Ada and libstdc++).

Thanks, but I now got Andreas Tobler testing it on
hppa2.0w-hp-hpux11.11 and hppa64-hp-hpux11.11.

Still, I guess testing for an old 32-bit-only hppa wouldn't
hurt, but only if there are some spare cycles.

brgds, H-P

Re: Call for HPPA testers

[Andreas Tobler]

Hans-Peter Nilsson wrote:
>> Date: Wed, 27 Aug 2008 18:29:43 +0200
>> From: Hans-Peter Nilsson <hp@axis.com>
> 
>> Main points:
>> - #ifndef ASM_OUTPUT_EXTERNAL was around the weak-handling too.
>> - Inlined functions and emitted variables can be marked as weak;
>>   there must be no weak-*reference* annotation for them.
>> - output_operand doesn't catch all from code referenced
>>   symbol_ref's.
> 
> ...but those were beside the original point, that it's not
> enough to, in output_operand, look inside mems to see all
> required symbol_refs.
> 
> As mentioned, revision 138310 (and its add-on for PR 36974) is
> an attempt to fix one special case of this.  With the previous
> patch applied, it's redundant but benevolent except for being
> misleading.  My guess is that it "worked" where output_operand
> failed only because the mem(*) is usually outside, not part of
> the match_operand part in call insns (the one reaching
> output_operand), and luckily the reported platforms had a
> symbol_ref there in bootstrap and tests (and not e.g. a reg).
> 
> Can someone with an affected system (HPPA) pretty please test
> this patch, reverting 138310 and the later change, on top of my
> previous one?
> 

Running. Approx 24h+.

The below snippet is superfluous.

> @@ -3346,6 +3304,31 @@
>    assemble_name (asm_out_file, buf);
>  }
>  


Thanks,
Andreas

Re: Call for HPPA testers

[Hans-Peter Nilsson]

> Date: Wed, 27 Aug 2008 20:24:37 +0200
> From: Andreas Tobler <andreast-list@fgznet.ch>

> The below snippet is superfluous.
> 
> > @@ -3346,6 +3304,31 @@
> >    assemble_name (asm_out_file, buf);
> >  }

Sorry, cruft from manual incomplete editing-out of the previous
patch. (gr?)

brgds, H-P

Re: Call for HPPA testers (was: [RFA:] Fix PR37170: weak-1.c regression, second edition)

[John David Anglin]

> Can someone with an affected system (HPPA) pretty please test
> this patch, reverting 138310 and the later change, on top of my
> previous one?

Doesn't work?  .IMPORT directives are no longer generated configure fails
for libiberty.  Was I supposed to revert the previous changes as well?

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Re: Call for HPPA testers (was: [RFA:] Fix PR37170: weak-1.c regression, second edition)

[Hans-Peter Nilsson]

> Date: Thu, 28 Aug 2008 00:15:12 -0400 (EDT)
> From: "John David Anglin" <dave@hiauly1.hia.nrc.ca>

> > Can someone with an affected system (HPPA) pretty please test
> > this patch, reverting 138310 and the later change, on top of my
> > previous one?
> 
> Doesn't work?  .IMPORT directives are no longer generated configure fails
> for libiberty.  Was I supposed to revert the previous changes as well?

I meant, just apply
<http://gcc.gnu.org/ml/gcc-patches/2008-08/msg02037.html> and
<http://gcc.gnu.org/ml/gcc-patches/2008-08/msg02044.html>
(the latter with the lines including and after the last @@
removed) to trunk.

If that doesn't work, can I see a testcase and the revision you
used?  On which hppa target (so I can build a crosscompiler)?
In the meantime, I'll try with the testcases you posted at the
earlier run at
<http://gcc.gnu.org/ml/gcc-patches/2008-07/msg01989.html> and
<http://gcc.gnu.org/ml/gcc-patches/2008-07/msg02024.html> and
for hppa-hpux (right, I should've done that before).

brgds, H-P

Re: Call for HPPA testers (was: [RFA:] Fix PR37170: weak-1.c regression, second edition)

[Hans-Peter Nilsson]

> Date: Thu, 28 Aug 2008 15:37:11 +0200
> From: Hans-Peter Nilsson <hp@axis.com>

> > Date: Thu, 28 Aug 2008 00:15:12 -0400 (EDT)
> > From: "John David Anglin" <dave@hiauly1.hia.nrc.ca>
> 
> > > Can someone with an affected system (HPPA) pretty please test
> > > this patch, reverting 138310 and the later change, on top of my
> > > previous one?
> > 
> > Doesn't work?  .IMPORT directives are no longer generated configure fails
> > for libiberty.  Was I supposed to revert the previous changes as well?
> 
> I meant, just apply
> <http://gcc.gnu.org/ml/gcc-patches/2008-08/msg02037.html> and
> <http://gcc.gnu.org/ml/gcc-patches/2008-08/msg02044.html>
> (the latter with the lines including and after the last @@
> removed) to trunk.
> 
> If that doesn't work, can I see a testcase and the revision you
> used?  On which hppa target (so I can build a crosscompiler)?
> In the meantime, I'll try with the testcases you posted at the
> earlier run at
> <http://gcc.gnu.org/ml/gcc-patches/2008-07/msg01989.html> and
> <http://gcc.gnu.org/ml/gcc-patches/2008-07/msg02024.html> and
> for hppa-hpux (right, I should've done that before).

For hppa1.1-hpux10 (*), with and without -O2, both test-cases
above produce identical assembly code for me before and after
the two patches applied, at revision 139233, with what at a
glance seems like sufficient .IMPORT directives.

(*) Plain hppa-hpux isn't supported so please mention an exact
tuple, canonical or as recognized by config.sub.

and thanks for testing!

brgds, H-P

Re: Call for HPPA testers (was: [RFA:] Fix PR37170: weak-1.c regression, second edition)

[John David Anglin]

> > I meant, just apply
> > <http://gcc.gnu.org/ml/gcc-patches/2008-08/msg02037.html> and
> > <http://gcc.gnu.org/ml/gcc-patches/2008-08/msg02044.html>
> > (the latter with the lines including and after the last @@
> > removed) to trunk.

It looks like there is still a problem.  I hit this error linking
libstdc++.sl.6.11:

/usr/ccs/bin/ld: Invalid symbol type for plabel (.libs/strstream.o, std::basic_i
ostream<char, std::char_traits<char> >::~basic_iostream()).

This is most likely caused by a missing .IMPORT directive.  The target
is hppa2.0w-hp-hpux11.11.  The revision is 139713.  I'll generate a
.ii tonight.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Re: Call for HPPA testers (was: [RFA:] Fix PR37170: weak-1.c regression, second edition)

[Andreas Tobler]

John David Anglin wrote:
>>> I meant, just apply
>>> <http://gcc.gnu.org/ml/gcc-patches/2008-08/msg02037.html> and
>>> <http://gcc.gnu.org/ml/gcc-patches/2008-08/msg02044.html>
>>> (the latter with the lines including and after the last @@
>>> removed) to trunk.
> 
> It looks like there is still a problem.  I hit this error linking
> libstdc++.sl.6.11:
> 
> /usr/ccs/bin/ld: Invalid symbol type for plabel (.libs/strstream.o, std::basic_i
> ostream<char, std::char_traits<char> >::~basic_iostream()).
> 
> This is most likely caused by a missing .IMPORT directive.  The target
> is hppa2.0w-hp-hpux11.11.  The revision is 139713.  I'll generate a
> .ii tonight.

Hm, I get SIGBUS when linking libstdc++ on hppa2.0w-hp-hpux11.11 
(r139659), but I have disable-checking, is this the same as the above?

hppa64-hp-hpux11.11 passed bootstrap (same rev), testing now gcc and 
later g++.

Andreas

Re: Call for HPPA testers (was: [RFA:] Fix PR37170: weak-1.c regression,

[John David Anglin]

> Hm, I get SIGBUS when linking libstdc++ on hppa2.0w-hp-hpux11.11 
> (r139659), but I have disable-checking, is this the same as the above?

Make sure you have the latest HP linker patch.  This used to
happen with old versions of the linker.  Also, make sure GAS is
up to date.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Re: Call for HPPA testers (was: [RFA:] Fix PR37170: weak-1.c regression, second edition)

[Hans-Peter Nilsson]

> Date: Thu, 28 Aug 2008 15:57:45 -0400 (EDT)
> From: "John David Anglin" <dave@hiauly1.hia.nrc.ca>

> It looks like there is still a problem.  I hit this error linking
> libstdc++.sl.6.11:

Is unpatched 139713 a working baseline for you?

> /usr/ccs/bin/ld: Invalid symbol type for plabel (.libs/strstream.o, std::basic_i
> ostream<char, std::char_traits<char> >::~basic_iostream()).
> 
> This is most likely caused by a missing .IMPORT directive.  The target
> is hppa2.0w-hp-hpux11.11.  The revision is 139713.  I'll generate a
> .ii tonight.

Please! thanks.

brgds, H-P

Re: Call for HPPA testers (was: [RFA:] Fix PR37170: weak-1.c regression, second edition)

[John David Anglin]

> Is unpatched 139713 a working baseline for you?

Results for a unpatched build of 139747 are here:
http://gcc.gnu.org/ml/gcc-testresults/2008-08/msg02665.html

It's not exactly a working baseline as there are no g++ failures in 4.3.
So, 'm not sure your proposed change is the root cause of libstdc++
failing to link:  The g++ fails look like:

FAIL: g++.dg/abi/layout2.C (test for excess errors)
Excess errors:
/usr/ccs/bin/ld: Unsatisfied symbols:
   vtable for B (first referenced in /var/tmp//ccIUeBeZ.o) (data)
   vtable for D (first referenced in /var/tmp//ccIUeBeZ.o) (data)

FAIL: g++.old-deja/g++.other/static14.C (test for excess errors)
Excess errors:
/usr/ccs/bin/ld: Unsatisfied symbols:
   basic_string::~basic_string() (first referenced in /var/tmp//ccAY9vKC.o) (code)
   Side::name()::sname     (first referenced in /var/tmp//ccAY9vKC.o) (data)

> > /usr/ccs/bin/ld: Invalid symbol type for plabel (.libs/strstream.o, std::basic_i
> > ostream<char, std::char_traits<char> >::~basic_iostream()).
> > 
> > This is most likely caused by a missing .IMPORT directive.  The target
> > is hppa2.0w-hp-hpux11.11.  The revision is 139713.  I'll generate a
> > .ii tonight.
> 
> Please! thanks.

Sorry, I forgot.  I'll have to redo a patched build.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Re: Call for HPPA testers (was: [RFA:] Fix PR37170: weak-1.c regression, second edition)

[Hans-Peter Nilsson]

> Date: Fri, 29 Aug 2008 12:50:49 -0400 (EDT)
> From: "John David Anglin" <dave@hiauly1.hia.nrc.ca>

> It's not exactly a working baseline as there are no g++ failures in 4.3.
> So, 'm not sure your proposed change is the root cause of libstdc++
> failing to link:  The g++ fails look like:

For this context, it's sufficient to know that the build fails
patched (with the first or both patches) where it worked
unpatched (with neither patch) for this revision.

> FAIL: g++.dg/abi/layout2.C (test for excess errors)
> Excess errors:
> /usr/ccs/bin/ld: Unsatisfied symbols:
>    vtable for B (first referenced in /var/tmp//ccIUeBeZ.o) (data)
>    vtable for D (first referenced in /var/tmp//ccIUeBeZ.o) (data)

Yeah, those should be fixed with the patches.

brgds, H-P

Re: Call for HPPA testers (was: [RFA:] Fix PR37170: weak-1.c regression, second edition)

[Andreas Tobler]

John David Anglin wrote:

>>> This is most likely caused by a missing .IMPORT directive.  The target
>>> is hppa2.0w-hp-hpux11.11.  The revision is 139713.  I'll generate a
>>> .ii tonight.
>> Please! thanks.
> 
> Sorry, I forgot.  I'll have to redo a patched build.

Attached it to 37170, hopefully usable :)

Andreas

Re: Call for HPPA testers (was: [RFA:] Fix PR37170: weak-1.c regression, second edition)

[John David Anglin]

> I meant, just apply
> <http://gcc.gnu.org/ml/gcc-patches/2008-08/msg02037.html> and
> <http://gcc.gnu.org/ml/gcc-patches/2008-08/msg02044.html>
> (the latter with the lines including and after the last @@
> removed) to trunk.

Ok, I only applied the later change.  I'll retest.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

[RFA:] Fix PR37170 (and PR37280): weak-1.c regression, third edition

[Hans-Peter Nilsson]

See <http://gcc.gnu.org/ml/gcc-patches/2008-08/msg02037.html>.
News since then: fix hppa-hpux regression.  Added test-case from PR37280.

To rehash the main points:
- #ifndef ASM_OUTPUT_EXTERNAL was around the weak-handling too.
- Inlined functions and emitted variables can be marked as weak;
  there must be no weak-*reference* annotation for them.
- Not all from code referenced symbol_ref's is caught by
  output_operand; some targets do weird stuff: darwin.
- For the symbol_ref's that actually pass through
  output_operand, it's not enough to look inside mems to find
  them (for you tree-huggers: what's passing through there is no
  longer just the DECL_RTL thingies).  This by itself was PR37280.

Tested native on hppa64-hpux11.11, x86_64-unknown-linux-gnu and
cross to cris-axis-elf.  (No test on Darwin this time, aside
from cross-testing a few cases, but then again, that port
doesn't define ASM_OUTPUT_EXTERNAL so results from previous
version *could* arguably carry over.)

gcc/testsuite:
	PR middle-end/37170
	PR middle-end/37280
	* gcc.dg/weak/weak-15.c, gcc.dg/weak/weak-16.c,
	g++.dg/ext/inline1.C: New tests.

gcc:
	PR middle-end/37170
	PR middle-end/37280
	* final.c (mark_symbol_ref_as_used): New helper function.
	(output_operand): Instead of just looking inside MEMs for
	SYMBOL_REFs, use new helper function and for_each_rtx.
	* varasm.c (assemble_external): Move #ifndef ASM_OUTPUT_EXTERNAL
	to after weak-handling.  Don't mark decls with TREE_STATIC as weak.
	Make head comment more general.
	* config/darwin.c (machopic_output_indirection): Handle weak
	references here, like in assemble_external.

--- /dev/null	2008-02-22 17:23:55.052000250 +0100
+++ g++.dg/ext/inline1.C	2008-08-24 06:21:50.000000000 +0200
@@ -0,0 +1,34 @@
+// { dg-do compile }
+// { dg-options "-O" }
+// Make sure inlined non-outlined functions aren't marked weak.
+// We'd get a ".weak xyzzy" annotation trigged by the second declaration.
+
+// { dg-final { scan-assembler-not "weak\[^ \t\]*\[ \t\]_?xyzzy" } } 
+
+// The next check isn't really part of the actual test, just to make
+// sure there's no outline-copy of xyzzy, because if that really
+// happened, it *should* be marked linkonce or perhaps weak.
+// { dg-final { scan-assembler-not "xyzzy" } } 
+
+extern int x;
+extern void foo(void);
+extern void bar(void);
+
+extern "C" inline int xyzzy(int a)
+{
+  foo();
+  return a + x;
+}
+
+extern "C" int xyzzy(int);
+
+extern inline int plugh(int c)
+{
+  return xyzzy (c);
+}
+
+int y;
+void doit(int b)
+{
+  y = xyzzy (b) + plugh (b);
+}

--- /dev/null	2008-02-22 17:23:55.052000250 +0100
+++ gcc.dg/weak/weak-15.c	2008-08-25 03:41:31.000000000 +0200
@@ -0,0 +1,39 @@
+/* { dg-do compile } */
+/* { dg-require-weak "" } */
+/* { dg-options "-fno-common" } */
+
+/* { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?a" } } */
+/* { dg-final { scan-assembler-not "weak\[^ \t\]*\[ \t\]_?b" } } */
+/* { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?c" } } */
+/* { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?d" } } */
+
+#pragma weak a
+extern char a[];
+
+char *user_a(void)
+{
+  return a+1;
+}
+
+int x;
+int extern inline b(int y)
+{
+  return x+y;
+}
+
+extern int b(int y);
+
+int user_b(int z)
+{
+  return b(z);
+}
+
+#pragma weak c
+extern int c;
+
+int *user_c = &c;
+
+#pragma weak d
+extern char d[];
+
+char *user_d = &d[1];
--- /dev/null	2008-02-22 17:23:55.052000250 +0100
+++ gcc.dg/weak/weak-16.c	2008-09-02 21:02:01.000000000 +0200
@@ -0,0 +1,21 @@
+/* From PR37280.  */
+/* { dg-do compile } */
+/* { dg-require-weak "" } */
+/* { dg-options "-fno-common -Os" } */
+/* { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?kallsyms_token_index" } } */
+/* { dg-final { scan-assembler "weak\[^ \t\]*\[ \t\]_?kallsyms_token_table" } } */
+
+extern int kallsyms_token_index[] __attribute__((weak));
+extern int kallsyms_token_table[] __attribute__((weak));
+void kallsyms_expand_symbol(int *result)
+{
+  int len = *result;
+  int *tptr;
+  while(len) {
+    tptr = &kallsyms_token_table[ kallsyms_token_index[*result] ];
+    len--;
+    while (*tptr) tptr++;
+    *tptr = 1;
+  }
+ *result = 0;
+}
Index: final.c
===================================================================
--- final.c	(revision 139925)
+++ final.c	(working copy)
@@ -3349,6 +3349,31 @@ output_asm_label (rtx x)
   assemble_name (asm_out_file, buf);
 }
 
+/* Helper rtx-iteration-function for output_operand.  Marks
+   SYMBOL_REFs as referenced through use of assemble_external.  */
+
+static int
+mark_symbol_ref_as_used (rtx *xp, void *dummy ATTRIBUTE_UNUSED)
+{
+  rtx x = *xp;
+
+  /* If we have a used symbol, we may have to emit assembly
+     annotations corresponding to whether the symbol is external, weak
+     or has non-default visibility.  */
+  if (GET_CODE (x) == SYMBOL_REF)
+    {
+      tree t;
+
+      t = SYMBOL_REF_DECL (x);
+      if (t)
+	assemble_external (t);
+
+      return -1;
+    }
+
+  return 0;
+}
+
 /* Print operand X using machine-dependent assembler syntax.
    The macro PRINT_OPERAND is defined just to control this function.
    CODE is a non-digit that preceded the operand-number in the % spec,
@@ -3369,14 +3394,11 @@ output_operand (rtx x, int code ATTRIBUT
   gcc_assert (!x || !REG_P (x) || REGNO (x) < FIRST_PSEUDO_REGISTER);
 
   PRINT_OPERAND (asm_out_file, x, code);
-  if (x && MEM_P (x) && GET_CODE (XEXP (x, 0)) == SYMBOL_REF)
-    {
-      tree t;
-      x = XEXP (x, 0);
-      t = SYMBOL_REF_DECL (x);
-      if (t)
-	assemble_external (t);
-    }
+
+  if (x == NULL_RTX)
+    return;
+
+  for_each_rtx (&x, mark_symbol_ref_as_used, NULL);
 }
 
 /* Print a memory reference operand for address X
Index: varasm.c
===================================================================
--- varasm.c	(revision 139925)
+++ varasm.c	(working copy)
@@ -2290,9 +2290,10 @@ process_pending_assemble_externals (void
    to be emitted.  */
 static GTY(()) tree weak_decls;
 
-/* Output something to declare an external symbol to the assembler.
-   (Most assemblers don't need this, so we normally output nothing.)
-   Do nothing if DECL is not external.  */
+/* Output something to declare an external symbol to the assembler,
+   and qualifiers such as weakness.  (Most assemblers don't need
+   extern declaration, so we normally output nothing.)  Do nothing if
+   DECL is not external.  */
 
 void
 assemble_external (tree decl ATTRIBUTE_UNUSED)
@@ -2303,15 +2304,22 @@ assemble_external (tree decl ATTRIBUTE_U
      open.  If it's not, we should not be calling this function.  */
   gcc_assert (asm_out_file);
 
-#ifdef ASM_OUTPUT_EXTERNAL
   if (!DECL_P (decl) || !DECL_EXTERNAL (decl) || !TREE_PUBLIC (decl))
     return;
 
-  if (SUPPORTS_WEAK && DECL_WEAK (decl))
+  /* We want to output annotation for weak and external symbols at
+     very last to check if they are references or not.  */
+
+  if (SUPPORTS_WEAK && DECL_WEAK (decl)
+      /* TREE_STATIC is a weird and abused creature which is not
+	 generally the right test for whether an entity has been
+	 locally emitted, inlined or otherwise not-really-extern, but
+	 for declarations that can be weak, it happens to be
+	 match.  */
+      && !TREE_STATIC (decl))
     weak_decls = tree_cons (NULL, decl, weak_decls);
 
-  /* We want to output external symbols at very last to check if they
-     are references or not.  */
+#ifdef ASM_OUTPUT_EXTERNAL
   pending_assemble_externals = tree_cons (0, decl,
 					  pending_assemble_externals);
 #endif
Index: config/darwin.c
===================================================================
--- config/darwin.c	(revision 139925)
+++ config/darwin.c	(working copy)
@@ -1002,6 +1002,30 @@ machopic_output_indirection (void **slot
       rtx init = const0_rtx;
 
       switch_to_section (darwin_sections[machopic_nl_symbol_ptr_section]);
+
+      /* Mach-O symbols are passed around in code through indirect
+	 references and the original symbol_ref hasn't passed through
+	 the generic handling and reference-catching in
+	 output_operand, so we need to manually mark weak references
+	 as such.  */
+      if (SYMBOL_REF_WEAK (symbol))
+	{
+	  tree decl = SYMBOL_REF_DECL (symbol);
+	  gcc_assert (DECL_P (decl));
+
+	  if (decl != NULL_TREE
+	      && DECL_EXTERNAL (decl) && TREE_PUBLIC (decl)
+	      /* Handle only actual external-only definitions, not
+		 e.g. extern inline code or variables for which
+		 storage has been allocated.  */
+	      && !TREE_STATIC (decl))
+	    {
+	      fputs ("\t.weak_reference ", asm_out_file);
+	      assemble_name (asm_out_file, sym_name);
+	      fputc ('\n', asm_out_file);
+	    }
+	}
+
       assemble_name (asm_out_file, ptr_name);
       fprintf (asm_out_file, ":\n");
 

brgds, H-P

Re: [RFA:] Fix PR37170 (and PR37280): weak-1.c regression, third edition

[Hans-Peter Nilsson]

Ping: <http://gcc.gnu.org/ml/gcc-patches/2008-09/msg00195.html>.

brgds, H-P

Re: [RFA:] Fix PR37170 (and PR37280): weak-1.c regression, third edition

[Rafael Espindola]

> Tested native on hppa64-hpux11.11, x86_64-unknown-linux-gnu and
> cross to cris-axis-elf.  (No test on Darwin this time, aside
> from cross-testing a few cases, but then again, that port
> doesn't define ASM_OUTPUT_EXTERNAL so results from previous
> version *could* arguably carry over.)

Just finished testing on powerpc-apple-darwin8.11.0. The results:

-----------------------------------------------------------------

Tests that now work, but didn't before:

g++.dg/ext/weak2.C scan-assembler weak[^ \t]*[ \t]_?_Z3foov


Tests that now work, but didn't before:

gcc.dg/darwin-weakimport-1.c scan-assembler weak_reference _a
gcc.dg/darwin-weakimport-2.c scan-assembler weak_reference _foo
gcc.dg/weak/weak-1.c scan-assembler weak[^ \t]*[ \t]_?j
gcc.dg/weak/weak-12.c scan-assembler weak[^ \t]*[ \t]_?foo
gcc.dg/weak/weak-2.c scan-assembler weak[^ \t]*[ \t]_?ffoo1a
gcc.dg/weak/weak-2.c scan-assembler weak[^ \t]*[ \t]_?ffoo1b
gcc.dg/weak/weak-2.c scan-assembler weak[^ \t]*[ \t]_?ffoo1c
gcc.dg/weak/weak-2.c scan-assembler weak[^ \t]*[ \t]_?ffoo1e
gcc.dg/weak/weak-4.c scan-assembler weak[^ \t]*[ \t]_?vfoo1a
gcc.dg/weak/weak-4.c scan-assembler weak[^ \t]*[ \t]_?vfoo1b
gcc.dg/weak/weak-4.c scan-assembler weak[^ \t]*[ \t]_?vfoo1c

-------------------------------------------------------------

Cheers,
-- 
Rafael Avila de Espindola

Google | Gordon House | Barrow Street | Dublin 4 | Ireland
Registered in Dublin, Ireland | Registration Number: 368047

Re: [RFA:] Fix PR37170 (and PR37280): weak-1.c regression, third edition

[Ian Lance Taylor]

Hans-Peter Nilsson <hans-peter.nilsson@axis.com> writes:

> gcc/testsuite:
> 	PR middle-end/37170
> 	PR middle-end/37280
> 	* gcc.dg/weak/weak-15.c, gcc.dg/weak/weak-16.c,
> 	g++.dg/ext/inline1.C: New tests.
>
> gcc:
> 	PR middle-end/37170
> 	PR middle-end/37280
> 	* final.c (mark_symbol_ref_as_used): New helper function.
> 	(output_operand): Instead of just looking inside MEMs for
> 	SYMBOL_REFs, use new helper function and for_each_rtx.
> 	* varasm.c (assemble_external): Move #ifndef ASM_OUTPUT_EXTERNAL
> 	to after weak-handling.  Don't mark decls with TREE_STATIC as weak.
> 	Make head comment more general.
> 	* config/darwin.c (machopic_output_indirection): Handle weak
> 	references here, like in assemble_external.

This is OK.

Thanks.

Ian