PING: [PATCH] PR target/35514: Gcc shoud generate symbol type for undefined symbol

PING: [PATCH] PR target/35514: Gcc shoud generate symbol type for undefined symbol

[H.J. Lu]

On Wed, Jul 8, 2015 at 1:35 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sun, Jul 5, 2015 at 2:54 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Sun, Jul 5, 2015 at 11:14 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> Update default_elf_asm_output_external to also output symbol type to
>>> help ELF linker to properly issue diagnostic message.  We don't output
>>> symbol type for reference to external TLS symbol since assembler will
>>> generate TLS symbol type based on TLS relocation and Solaris assembler
>>> only supports the @tls_obj type directive, not the @tls_object type
>>> directive used by GNU assmbler, which doesn't understand the @tls_obj
>>> type directive.
>>>
>>> Tested on Linux/x86-64.  OK for trunk?
>>>
>>> Thanks.
>>>
>>>
>>> H.J.
>>> ---
>>> gcc/
>>>
>>>         PR target/35514
>>>         * varasm.c (default_elf_asm_output_external): Also output symbol
>>>         type.
>>>
>>> gcc/testsuite/
>>>
>>>         PR target/35514
>>>         * lib/target-supports.exp (check_effective_target_elf): New.
>>>         * gcc.dg/pr35514-1.c: New file.
>>>         * gcc.dg/pr35514-2.c: Likewise.

>
> Here is the updated patch to adjust those tests by counting in
> the .type directive for ELF targets.  There is no regression on
> Linux/x86-64.  OK for trunk?
>

PING:

https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00652.html

-- 
H.J.

Re: PING: [PATCH] PR target/35514: Gcc shoud generate symbol type for undefined symbol

[Bernd Schmidt]

On 10/08/2015 01:10 PM, H.J. Lu wrote:
> On Wed, Jul 8, 2015 at 1:35 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Sun, Jul 5, 2015 at 2:54 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Sun, Jul 5, 2015 at 11:14 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> Update default_elf_asm_output_external to also output symbol type to
>>>> help ELF linker to properly issue diagnostic message.

What kind of diagnostic message? The ELF linker seems to have managed to 
do without for a long time. Is there some discussion of this on the 
binutils list that you can point to?

>>>>  We don't output
>>>> symbol type for reference to external TLS symbol since assembler will
>>>> generate TLS symbol type based on TLS relocation and Solaris assembler
>>>> only supports the @tls_obj type directive, not the @tls_object type
>>>> directive used by GNU assmbler, which doesn't understand the @tls_obj
>>>> type directive.

That information would have to go into a comment in the patch.


Bernd

Re: PING: [PATCH] PR target/35514: Gcc shoud generate symbol type for undefined symbol

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]

On Fri, Oct 9, 2015 at 4:11 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 10/08/2015 01:10 PM, H.J. Lu wrote:
>>
>> On Wed, Jul 8, 2015 at 1:35 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>
>>> On Sun, Jul 5, 2015 at 2:54 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>
>>>> On Sun, Jul 5, 2015 at 11:14 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>
>>>>> Update default_elf_asm_output_external to also output symbol type to
>>>>> help ELF linker to properly issue diagnostic message.
>
>
> What kind of diagnostic message? The ELF linker seems to have managed to do
> without for a long time. Is there some discussion of this on the binutils
> list that you can point to?

https://groups.google.com/forum/#!topic/generic-abi/_ZPPq_c8FSQ

>>>>>  We don't output
>>>>> symbol type for reference to external TLS symbol since assembler will
>>>>> generate TLS symbol type based on TLS relocation and Solaris assembler
>>>>> only supports the @tls_obj type directive, not the @tls_object type
>>>>> directive used by GNU assmbler, which doesn't understand the @tls_obj
>>>>> type directive.
>
>
> That information would have to go into a comment in the patch.
>

Here is the updated patch.

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-Output-ELF-symbol-type-for-undefined-symbols.patch --]
[-- Type: text/x-patch, Size: 13088 bytes --]

From 27684d5c6505f50a90358594343882e5e28e1f1b Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sun, 5 Jul 2015 08:58:28 -0700
Subject: [PATCH] Output ELF symbol type for undefined symbols

Update default_elf_asm_output_external to also output symbol type to
help ELF linker properly issue diagnostic message.  We don't output
symbol type for reference to external TLS symbol since assembler will
generate TLS symbol type based on TLS relocation and Solaris assembler
only supports the @tls_obj type directive, not the @tls_object type
directive used by GNU assmbler, which doesn't understand the @tls_obj
type directive.

gcc/

	PR target/35514
	* varasm.c (default_elf_asm_output_external): Also output symbol
	type.

gcc/testsuite/

	PR target/35514
	* g++.dg/fstack-protector-strong.C: Adjust scan-assembler-times
	for ELF targets.
	* g++.dg/opt/devirt2.C: Likewise.
	* g++.dg/pr49718.C: Likewise.
	* g++.dg/stackprotectexplicit2.C : Likewise.
	* gcc.dg/fstack-protector-strong.c: Likewise.
	* gcc.dg/tree-ssa/loop-1.c: Likewise.
	* gcc.target/i386/memcpy-strategy-3.c: Likewise.
	* gcc.target/i386/memset-strategy-1.c: Likewise.
	* gfortran.dg/bind_c_array_params_2.f90: Likewise.
	* gcc.dg/pr35514-1.c: New file.
	* gcc.dg/pr35514-2.c: Likewise.
	* lib/target-supports.exp (check_effective_target_elf): New.
---
 gcc/testsuite/g++.dg/fstack-protector-strong.C     |  3 ++-
 gcc/testsuite/g++.dg/opt/devirt2.C                 |  3 ++-
 gcc/testsuite/g++.dg/pr49718.C                     |  3 ++-
 gcc/testsuite/g++.dg/stackprotectexplicit2.C       |  3 ++-
 gcc/testsuite/gcc.dg/fstack-protector-strong.c     |  3 ++-
 gcc/testsuite/gcc.dg/pr35514-1.c                   | 16 +++++++++++++
 gcc/testsuite/gcc.dg/pr35514-2.c                   | 14 +++++++++++
 gcc/testsuite/gcc.dg/tree-ssa/loop-1.c             |  3 ++-
 gcc/testsuite/gcc.target/i386/memcpy-strategy-3.c  |  3 ++-
 gcc/testsuite/gcc.target/i386/memset-strategy-1.c  |  3 ++-
 .../gfortran.dg/bind_c_array_params_2.f90          |  3 ++-
 gcc/testsuite/lib/target-supports.exp              | 20 ++++++++++++++++
 gcc/varasm.c                                       | 28 ++++++++++++++++++----
 13 files changed, 91 insertions(+), 14 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr35514-1.c
 create mode 100644 gcc/testsuite/gcc.dg/pr35514-2.c

diff --git a/gcc/testsuite/g++.dg/fstack-protector-strong.C b/gcc/testsuite/g++.dg/fstack-protector-strong.C
index ae6d2fd..7be72fa 100644
--- a/gcc/testsuite/g++.dg/fstack-protector-strong.C
+++ b/gcc/testsuite/g++.dg/fstack-protector-strong.C
@@ -85,4 +85,5 @@ int foo7 (B *p)
   return p->return_slot ().a1;
 }
 
-/* { dg-final { scan-assembler-times "stack_chk_fail" 7 } } */
+/* { dg-final { scan-assembler-times "stack_chk_fail" 8 { target elf } } } */
+/* { dg-final { scan-assembler-times "stack_chk_fail" 7 { target { ! elf } } } } */
diff --git a/gcc/testsuite/g++.dg/opt/devirt2.C b/gcc/testsuite/g++.dg/opt/devirt2.C
index cf4842b..0ea54f6 100644
--- a/gcc/testsuite/g++.dg/opt/devirt2.C
+++ b/gcc/testsuite/g++.dg/opt/devirt2.C
@@ -5,7 +5,8 @@
 // { dg-additional-options "-mshort-calls" {target epiphany-*-*} }
 // Using -mno-abicalls avoids a R_MIPS_JALR .reloc.
 // { dg-additional-options "-mno-abicalls" { target mips*-*-* } }
-// { dg-final { scan-assembler-times "xyzzy" 2 { target { ! { alpha*-*-* hppa*-*-* ia64*-*-hpux* sparc*-*-* *-*-mingw* } } } } }
+// { dg-final { scan-assembler-times "xyzzy" 2 { target { ! elf } &&  target { ! { alpha*-*-* hppa*-*-* ia64*-*-hpux* sparc*-*-* *-*-mingw* } } } } }
+// { dg-final { scan-assembler-times "xyzzy" 3 { target { elf && { ! { alpha*-*-* hppa*-*-* ia64*-*-hpux* sparc*-*-* } } } } } }
 // For *-*-mingw* there is additionally one .def match
 // { dg-final { scan-assembler-times "xyzzy" 3 { target *-*-mingw* } } }
 // The IA64 and HPPA compilers generate external declarations in addition
diff --git a/gcc/testsuite/g++.dg/pr49718.C b/gcc/testsuite/g++.dg/pr49718.C
index b1cc5de..da44372 100644
--- a/gcc/testsuite/g++.dg/pr49718.C
+++ b/gcc/testsuite/g++.dg/pr49718.C
@@ -2,7 +2,8 @@
 /* { dg-options "-O2 -finstrument-functions" } */
 /* { dg-additional-options "-mno-explicit-relocs" { target alpha*-*-* } } */
 /* { dg-additional-options "-mno-relax-pic-calls" { target mips*-*-* } } */
-/* { dg-final { scan-assembler-times "__cyg_profile_func_enter" 1 { target { ! { hppa*-*-hpux* } } } } } */
+/* { dg-final { scan-assembler-times "__cyg_profile_func_enter" 1 { target { ! elf } && target { ! { hppa*-*-hpux* } } } } } */
+/* { dg-final { scan-assembler-times "__cyg_profile_func_enter" 2 { target { elf && { ! { hppa*-*-hpux* } } } } } } */
 /* { dg-final { scan-assembler-times "__cyg_profile_func_enter,%r" 1 { target hppa*-*-hpux* } } } */
 
 #define NOINSTR __attribute__((no_instrument_function))
diff --git a/gcc/testsuite/g++.dg/stackprotectexplicit2.C b/gcc/testsuite/g++.dg/stackprotectexplicit2.C
index 9cf9ab9..0423056 100644
--- a/gcc/testsuite/g++.dg/stackprotectexplicit2.C
+++ b/gcc/testsuite/g++.dg/stackprotectexplicit2.C
@@ -24,4 +24,5 @@ int __attribute__((stack_protect)) c()
 }
 
 
-/* { dg-final { scan-assembler-times "stack_chk_fail" 2 } } */
\ No newline at end of file
+/* { dg-final { scan-assembler-times "stack_chk_fail" 3 { target elf } } } */
+/* { dg-final { scan-assembler-times "stack_chk_fail" 2 { target { ! elf } } } } */
diff --git a/gcc/testsuite/gcc.dg/fstack-protector-strong.c b/gcc/testsuite/gcc.dg/fstack-protector-strong.c
index 8e9d891..50ea5d0 100644
--- a/gcc/testsuite/gcc.dg/fstack-protector-strong.c
+++ b/gcc/testsuite/gcc.dg/fstack-protector-strong.c
@@ -154,4 +154,5 @@ void foo12 ()
   global3 ();
 }
 
-/* { dg-final { scan-assembler-times "stack_chk_fail" 12 } } */
+/* { dg-final { scan-assembler-times "stack_chk_fail" 13 { target elf } } } */
+/* { dg-final { scan-assembler-times "stack_chk_fail" 12 { target { ! elf } } } } */
diff --git a/gcc/testsuite/gcc.dg/pr35514-1.c b/gcc/testsuite/gcc.dg/pr35514-1.c
new file mode 100644
index 0000000..43eeec4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr35514-1.c
@@ -0,0 +1,16 @@
+/* { dg-do compile { target { elf } } } */
+/* { dg-options "-O2" } */
+/* { dg-final { scan-assembler ".type\[^\\n\]*object, \(%|@\)object" } } */
+/* { dg-final { scan-assembler ".type\[^\\n\]*function, \(%|@\)function" } } */
+
+/* Verify that we generate proper symbol types for external reference.  */
+
+extern int object;
+void function (void);
+
+int
+foo (void)
+{
+  function ();
+  return object;
+}
diff --git a/gcc/testsuite/gcc.dg/pr35514-2.c b/gcc/testsuite/gcc.dg/pr35514-2.c
new file mode 100644
index 0000000..d915bf5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr35514-2.c
@@ -0,0 +1,14 @@
+/* { dg-do compile { target { elf } } } */
+/* { dg-require-effective-target tls } */
+/* { dg-options "-O2" } */
+/* { dg-final { scan-assembler-not ".type\[^\\n\]*object, \(%|@\)object" } } */
+
+/* Verify that we generate proper symbol types for external reference.  */
+
+extern __thread int object;
+
+int
+foo (void)
+{
+  return object;
+}
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/loop-1.c b/gcc/testsuite/gcc.dg/tree-ssa/loop-1.c
index 9d18dd7..0903ea2 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/loop-1.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/loop-1.c
@@ -46,7 +46,8 @@ int xxx(void)
 /* CRIS keeps the address in a register.  */
 /* m68k sometimes puts the address in a register, depending on CPU and PIC.  */
 
-/* { dg-final { scan-assembler-times "foo" 5 { xfail hppa*-*-* ia64*-*-* sh*-*-* cris-*-* crisv32-*-* fido-*-* m68k-*-* i?86-*-mingw* i?86-*-cygwin* x86_64-*-mingw* visium-*-* } } } */
+/* { dg-final { scan-assembler-times "foo" 5 { target { ! elf } xfail hppa*-*-* ia64*-*-* sh*-*-* cris-*-* crisv32-*-* fido-*-* m68k-*-* i?86-*-mingw* i?86-*-cygwin* x86_64-*-mingw* visium-*-* } } } */
+/* { dg-final { scan-assembler-times "foo" 6 { target elf xfail hppa*-*-* ia64*-*-* sh*-*-* cris-*-* crisv32-*-* fido-*-* m68k-*-* visium-*-* } } } */
 /* { dg-final { scan-assembler-times "foo,%r" 5 { target hppa*-*-* } } } */
 /* { dg-final { scan-assembler-times "= foo"  5 { target ia64*-*-* } } } */
 /* { dg-final { scan-assembler-times "call\[ \t\]*_foo" 5 { target i?86-*-mingw* i?86-*-cygwin* } } } */
diff --git a/gcc/testsuite/gcc.target/i386/memcpy-strategy-3.c b/gcc/testsuite/gcc.target/i386/memcpy-strategy-3.c
index ddd1ef7..6553c12 100644
--- a/gcc/testsuite/gcc.target/i386/memcpy-strategy-3.c
+++ b/gcc/testsuite/gcc.target/i386/memcpy-strategy-3.c
@@ -1,6 +1,7 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -march=atom -mmemcpy-strategy=vector_loop:2000:align,libcall:-1:align" } */
-/* { dg-final { scan-assembler-times "memcpy" 2  } } */
+/* { dg-final { scan-assembler-times "memcpy" 3 { target elf } } } */
+/* { dg-final { scan-assembler-times "memcpy" 2 { target { ! elf } } } } */
 
 char a[2048];
 char b[2048];
diff --git a/gcc/testsuite/gcc.target/i386/memset-strategy-1.c b/gcc/testsuite/gcc.target/i386/memset-strategy-1.c
index d1b97c5..c249d1e 100644
--- a/gcc/testsuite/gcc.target/i386/memset-strategy-1.c
+++ b/gcc/testsuite/gcc.target/i386/memset-strategy-1.c
@@ -1,6 +1,7 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -march=atom -mmemset-strategy=libcall:-1:align" } */
-/* { dg-final { scan-assembler-times "memset" 2  } } */
+/* { dg-final { scan-assembler-times "memset" 3 { target elf }  } } */
+/* { dg-final { scan-assembler-times "memset" 2 { target { ! elf } } } } */
 
 char a[2048];
 void t (void)
diff --git a/gcc/testsuite/gfortran.dg/bind_c_array_params_2.f90 b/gcc/testsuite/gfortran.dg/bind_c_array_params_2.f90
index c8eb50d..19c6082 100644
--- a/gcc/testsuite/gfortran.dg/bind_c_array_params_2.f90
+++ b/gcc/testsuite/gfortran.dg/bind_c_array_params_2.f90
@@ -16,6 +16,7 @@ integer :: aa(4,4)
 call test(aa)
 end
 
-! { dg-final { scan-assembler-times "\[ \t\]\[$,_0-9\]*myBindC" 1 { target { ! { hppa*-*-* } } } } }
+! { dg-final { scan-assembler-times "\[ \t\]\[$,_0-9\]*myBindC" 1 { target { ! elf } && target { ! { hppa*-*-* } } } } }
+! { dg-final { scan-assembler-times "\[ \t\]\[$,_0-9\]*myBindC" 2 { target { elf && { ! { hppa*-*-* } } } } } }
 ! { dg-final { scan-assembler-times "myBindC,%r2" 1 { target { hppa*-*-* } } } }
 ! { dg-final { scan-tree-dump-times "test \\\(&parm\\." 1 "original" } }
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 4d5b0a3d..53af66d 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -6484,3 +6484,23 @@ proc check_effective_target_builtin_eh_return { } {
 	}
     } "" ]
 }
+
+# Returns 1 if the target is ELF.
+
+proc check_effective_target_elf { } {
+    global check_effective_target_elf_saved
+    global tool
+
+    if [info exists check_effective_target_elf_saved] {
+        verbose "check_effective_target_elf returning saved $check_effective_target_elf_saved" 2
+    } else {
+	set objformat [gcc_target_object_format]
+	if { $objformat == "elf" } {
+	    set check_effective_target_elf_saved 1
+	} else {
+	    set check_effective_target_elf_saved 0
+	}
+        verbose "check_effective_target_elf returning $check_effective_target_elf_saved" 2
+    }
+    return $check_effective_target_elf_saved
+}
diff --git a/gcc/varasm.c b/gcc/varasm.c
index f1564bc..85f0a23 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -7494,7 +7494,7 @@ elf_record_gcc_switches (print_switch_type type, const char * name)
 }
 
 /* Emit text to declare externally defined symbols. It is needed to
-   properly support non-default visibility.  */
+   properly support non-default visibility and specify symbol type.  */
 void
 default_elf_asm_output_external (FILE *file ATTRIBUTE_UNUSED,
 				 tree decl,
@@ -7503,10 +7503,28 @@ default_elf_asm_output_external (FILE *file ATTRIBUTE_UNUSED,
   /* We output the name if and only if TREE_SYMBOL_REFERENCED is
      set in order to avoid putting out names that are never really
      used.  Always output visibility specified in the source.  */
-  if (TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl))
-      && (DECL_VISIBILITY_SPECIFIED (decl)
-	  || targetm.binds_local_p (decl)))
-    maybe_assemble_visibility (decl);
+  if (TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl)))
+    {
+      if (DECL_VISIBILITY_SPECIFIED (decl)
+	  || targetm.binds_local_p (decl))
+	maybe_assemble_visibility (decl);
+
+#ifdef ASM_OUTPUT_TYPE_DIRECTIVE
+      /* We don't output symbol type for reference to external TLS
+	 symbol since assembler will generate TLS symbol type based
+	 on TLS relocation and Solaris assembler only supports the
+	 @tls_obj type directive, not the @tls_object type directive
+	 used by GNU assmbler, which doesn't understand the @tls_obj
+	 type directive.  */
+      if (TREE_CODE (decl) == FUNCTION_DECL)
+	ASM_OUTPUT_TYPE_DIRECTIVE (file, name, "function");
+      else if (TREE_CODE (decl) == VAR_DECL)
+	{
+	  if (!DECL_THREAD_LOCAL_P (decl))
+	    ASM_OUTPUT_TYPE_DIRECTIVE (file, name, "object");
+	}
+#endif
+    }
 }
 
 /* The default hook for TARGET_ASM_OUTPUT_SOURCE_FILENAME.  */
-- 
2.4.3

Re: PING: [PATCH] PR target/35514: Gcc shoud generate symbol type for undefined symbol

[Bernd Schmidt]

On 10/09/2015 02:45 PM, H.J. Lu wrote:
> What kind of diagnostic message? The ELF linker seems to have managed to do
>> without for a long time. Is there some discussion of this on the binutils
>> list that you can point to?
>
> https://groups.google.com/forum/#!topic/generic-abi/_ZPPq_c8FSQ

Hmm, near the end we have this message from Cary:

>> So I assume that it is incorrect for gcc(1) to mark an undefined function
>
>> reference as STT_NOTYPE. I think that is the bug HJ pointed out.
>
> No, I don't think it's incorrect at all. It's merely a
> quality-of-implementation issue: setting the undef to STT_FUNC or
> STT_OBJECT allows the linker to diagnose a mismatch between reference
> and definition, but, traditionally, Unix linkers have always been
> happy to bind symbols without checking types, and I'll bet there's
> still plenty of code that depends on that.

So there does not appear to be consensus quite yet. Cc'ing Cary in case 
he has additional input.


Bernd

Re: PING: [PATCH] PR target/35514: Gcc shoud generate symbol type for undefined symbol

[H.J. Lu]

On Fri, Oct 9, 2015 at 5:50 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 10/09/2015 02:45 PM, H.J. Lu wrote:
>>
>> What kind of diagnostic message? The ELF linker seems to have managed to
>> do
>>>
>>> without for a long time. Is there some discussion of this on the binutils
>>> list that you can point to?
>>
>>
>> https://groups.google.com/forum/#!topic/generic-abi/_ZPPq_c8FSQ
>
>
> Hmm, near the end we have this message from Cary:
>
>>> So I assume that it is incorrect for gcc(1) to mark an undefined function
>>
>>
>>> reference as STT_NOTYPE. I think that is the bug HJ pointed out.
>>
>>
>> No, I don't think it's incorrect at all. It's merely a
>> quality-of-implementation issue: setting the undef to STT_FUNC or
>> STT_OBJECT allows the linker to diagnose a mismatch between reference
>> and definition, but, traditionally, Unix linkers have always been
>> happy to bind symbols without checking types, and I'll bet there's
>> still plenty of code that depends on that.
>
>
> So there does not appear to be consensus quite yet. Cc'ing Cary in case he
> has additional input.
>

Everyone agrees that it is a quality-of-implementation issue.


-- 
H.J.