[testsuite] Fix multiple definitions of _init

[testsuite] Fix multiple definitions of _init

[Oleg Endo]

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

Hi,

When running the testsuite on a sh-elf configuration, some test cases
fail due to multiple definitions of the function '_init'.  This is
because on sh-elf every function is automatically prefixed with a '_'
char.  When defining a C function 'init' in some code, the actual name
becomes '_init' and this conflicts with the _init function in libgcc
(crti.S).  While the behavior of adding the '_' prefix is debatable, the
testsuite failures can be easily fixed by the attached patch.
Is this OK for trunk?

Cheers,
Oleg

gcc/testsuite/ChangeLog:

	* gcc.c-torture/execute/20120919-1.c (init): Rename to initt.
	* gcc.c-torture/execute/pr42248.c: Likewise.
	* gcc.c-torture/execute/pr53688.c: Likewise.
	* gcc.c-torture/execute/pr42614.c: Likewise.

[-- Attachment #2: testsuite_init_func.patch --]
[-- Type: text/x-patch, Size: 2419 bytes --]

Index: gcc/testsuite/gcc.c-torture/execute/20120919-1.c
===================================================================
--- gcc/testsuite/gcc.c-torture/execute/20120919-1.c	(revision 218199)
+++ gcc/testsuite/gcc.c-torture/execute/20120919-1.c	(working copy)
@@ -9,9 +9,9 @@
 
 extern void abort(void);
 
-void init (int *n, int *dummy) __attribute__ ((noinline,noclone));
+void initt (int *n, int *dummy) __attribute__ ((noinline,noclone));
 
-void init (int *n, int *dummy)
+void initt (int *n, int *dummy)
 {
   if(0 == n) dummy[0] = 0;
 }
@@ -20,7 +20,7 @@
 {
   int dummy[1532];
   int i = -1, n = 1, s = 0;
-  init (&n, dummy);
+  initt (&n, dummy);
   while (i < n) {
     if (i == 0) {
       if (pd[i] > 0) {
Index: gcc/testsuite/gcc.c-torture/execute/pr42248.c
===================================================================
--- gcc/testsuite/gcc.c-torture/execute/pr42248.c	(revision 218199)
+++ gcc/testsuite/gcc.c-torture/execute/pr42248.c	(working copy)
@@ -12,7 +12,7 @@
 }
 
 void
-init (Scf10 *p, _Complex double y)
+initt (Scf10 *p, _Complex double y)
 {
   p->a = y;
 }
@@ -20,7 +20,7 @@
 int
 main ()
 {
-  init (&g1s, (_Complex double)1);
+  initt (&g1s, (_Complex double)1);
   check (g1s, (_Complex double)1);
 
   return 0;
Index: gcc/testsuite/gcc.c-torture/execute/pr53688.c
===================================================================
--- gcc/testsuite/gcc.c-torture/execute/pr53688.c	(revision 218199)
+++ gcc/testsuite/gcc.c-torture/execute/pr53688.c	(working copy)
@@ -5,7 +5,7 @@
 } p;
 
 void __attribute__((noinline,noclone))
-init()
+initt()
 {
   __builtin_memcpy (p.part1, "FOOBARFOO", sizeof (p.part1));
   __builtin_memcpy (p.part2, "SPEC CPU", sizeof (p.part2));
@@ -15,7 +15,7 @@
 {
   char *x;
   int c;
-  init();
+  initt();
   __builtin_memcpy (&headline[0], p.part1, 9);
   c = 9;
   x = &headline[0];
Index: gcc/testsuite/gcc.c-torture/execute/pr42614.c
===================================================================
--- gcc/testsuite/gcc.c-torture/execute/pr42614.c	(revision 218199)
+++ gcc/testsuite/gcc.c-torture/execute/pr42614.c	(working copy)
@@ -12,7 +12,7 @@
   TEntry data[2];
 } TTable;
 
-TTable *init ()
+TTable *initt ()
 {
   return malloc(sizeof(TTable));
 }
@@ -54,7 +54,7 @@
 main ()
 {
   unsigned char index = 0;
-  TTable *table_p = init();
+  TTable *table_p = initt();
   TEntry work;
 
   inlined_wrong (&(table_p->data[1]), 1);

Re: [testsuite] Fix multiple definitions of _init

[Richard Biener]

On Mon, Dec 1, 2014 at 8:21 AM, Oleg Endo <oleg.endo@t-online.de> wrote:
> Hi,
>
> When running the testsuite on a sh-elf configuration, some test cases
> fail due to multiple definitions of the function '_init'.  This is
> because on sh-elf every function is automatically prefixed with a '_'
> char.  When defining a C function 'init' in some code, the actual name
> becomes '_init' and this conflicts with the _init function in libgcc
> (crti.S).  While the behavior of adding the '_' prefix is debatable, the
> testsuite failures can be easily fixed by the attached patch.
> Is this OK for trunk?

The testcases are certainly valid C testcases and thus sh-elf is buggy
here.  As '_init' is a reserved identifier it shouldn't mangle 'init' as
'_init'.

Do you never run into "real" applications naming a function 'init'?

Richard.

> Cheers,
> Oleg
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.c-torture/execute/20120919-1.c (init): Rename to initt.
>         * gcc.c-torture/execute/pr42248.c: Likewise.
>         * gcc.c-torture/execute/pr53688.c: Likewise.
>         * gcc.c-torture/execute/pr42614.c: Likewise.

Re: [testsuite] Fix multiple definitions of _init

[Oleg Endo]

On Mon, 2014-12-01 at 12:09 +0100, Richard Biener wrote:
> On Mon, Dec 1, 2014 at 8:21 AM, Oleg Endo <oleg.endo@t-online.de> wrote:
> > Hi,
> >
> > When running the testsuite on a sh-elf configuration, some test cases
> > fail due to multiple definitions of the function '_init'.  This is
> > because on sh-elf every function is automatically prefixed with a '_'
> > char.  When defining a C function 'init' in some code, the actual name
> > becomes '_init' and this conflicts with the _init function in libgcc
> > (crti.S).  While the behavior of adding the '_' prefix is debatable, the
> > testsuite failures can be easily fixed by the attached patch.
> > Is this OK for trunk?
> 
> The testcases are certainly valid C testcases and thus sh-elf is buggy
> here.  As '_init' is a reserved identifier it shouldn't mangle 'init' as
> '_init'.
> 
> Do you never run into "real" applications naming a function 'init'?

Looking for USER_LABEL_PREFIX shows that the '_' prefix mangling scheme
is being used by quite some targets/configs.  So it's not just sh-elf,
but other bare metal configs and their ABIs, where 'init' gets mangled
to '_init' and there's a '.global _init' in libgcc.  Yes, it does break
valid standard C code 'void init (void)', but it's been doing that for
quite a while.

I didn't mean to open a can of worms because of 4 test cases showing up
in sh-elf sim tests.  From what I can see, those 4 cases would also pop
up for other configs, not only sh-elf.  Anyway, the function names in
those test cases don't matter at all, do they?

Cheers,
Oleg

Re: [testsuite] Fix multiple definitions of _init

[Richard Biener]

On Mon, Dec 1, 2014 at 11:48 PM, Oleg Endo <oleg.endo@t-online.de> wrote:
> On Mon, 2014-12-01 at 12:09 +0100, Richard Biener wrote:
>> On Mon, Dec 1, 2014 at 8:21 AM, Oleg Endo <oleg.endo@t-online.de> wrote:
>> > Hi,
>> >
>> > When running the testsuite on a sh-elf configuration, some test cases
>> > fail due to multiple definitions of the function '_init'.  This is
>> > because on sh-elf every function is automatically prefixed with a '_'
>> > char.  When defining a C function 'init' in some code, the actual name
>> > becomes '_init' and this conflicts with the _init function in libgcc
>> > (crti.S).  While the behavior of adding the '_' prefix is debatable, the
>> > testsuite failures can be easily fixed by the attached patch.
>> > Is this OK for trunk?
>>
>> The testcases are certainly valid C testcases and thus sh-elf is buggy
>> here.  As '_init' is a reserved identifier it shouldn't mangle 'init' as
>> '_init'.
>>
>> Do you never run into "real" applications naming a function 'init'?
>
> Looking for USER_LABEL_PREFIX shows that the '_' prefix mangling scheme
> is being used by quite some targets/configs.  So it's not just sh-elf,
> but other bare metal configs and their ABIs, where 'init' gets mangled
> to '_init' and there's a '.global _init' in libgcc.  Yes, it does break
> valid standard C code 'void init (void)', but it's been doing that for
> quite a while.

Oh, and I wonder why the global init isn't then __init (two underscores),
as obviously on those targets symbols with _ prefix are not in the
implementation namespace.

> I didn't mean to open a can of worms because of 4 test cases showing up
> in sh-elf sim tests.  From what I can see, those 4 cases would also pop
> up for other configs, not only sh-elf.  Anyway, the function names in
> those test cases don't matter at all, do they?

No, but it looks like papering over a problem.  After all we could have
a valid testcase checking for exactly this situation - that a C function
named 'init' works fine.

Joseph may have more experience with how targets should setup
USER_LABEL_PREFIX to avoid this situation.

Richard.

> Cheers,
> Oleg
>

Re: [testsuite] Fix multiple definitions of _init

[Oleg Endo]

On Tue, 2014-12-02 at 11:22 +0100, Richard Biener wrote:
> On Mon, Dec 1, 2014 at 11:48 PM, Oleg Endo <oleg.endo@t-online.de> wrote:
> > On Mon, 2014-12-01 at 12:09 +0100, Richard Biener wrote:
> >> On Mon, Dec 1, 2014 at 8:21 AM, Oleg Endo <oleg.endo@t-online.de> wrote:
> >> > Hi,
> >> >
> >> > When running the testsuite on a sh-elf configuration, some test cases
> >> > fail due to multiple definitions of the function '_init'.  This is
> >> > because on sh-elf every function is automatically prefixed with a '_'
> >> > char.  When defining a C function 'init' in some code, the actual name
> >> > becomes '_init' and this conflicts with the _init function in libgcc
> >> > (crti.S).  While the behavior of adding the '_' prefix is debatable, the
> >> > testsuite failures can be easily fixed by the attached patch.
> >> > Is this OK for trunk?
> >>
> >> The testcases are certainly valid C testcases and thus sh-elf is buggy
> >> here.  As '_init' is a reserved identifier it shouldn't mangle 'init' as
> >> '_init'.
> >>
> >> Do you never run into "real" applications naming a function 'init'?
> >
> > Looking for USER_LABEL_PREFIX shows that the '_' prefix mangling scheme
> > is being used by quite some targets/configs.  So it's not just sh-elf,
> > but other bare metal configs and their ABIs, where 'init' gets mangled
> > to '_init' and there's a '.global _init' in libgcc.  Yes, it does break
> > valid standard C code 'void init (void)', but it's been doing that for
> > quite a while.
> 
> Oh, and I wonder why the global init isn't then __init (two underscores),
> as obviously on those targets symbols with _ prefix are not in the
> implementation namespace.

My guess:  Because it's written in asm, and there it's not known whether
a _ prefix should be used or not.  It's an ABI/convention.  BTW it's not
only the init function, but potentially several others.  E.g.
newlib/libc/sys/sh/crt0.S refers to other symbols such as _stack,
_edata, _end, _main, _exit.  Thus, having something global named "stack"
in a C program will potentially clash.  So while we maybe could use
USER_LABEL_PREFIX in libgcc to add the prefix, there would still be
issues with things defined in e.g. newlib or the linker script.

For bare metal configs / programs having some additional
reserved/prohibited names is not a big deal, as there are often other
restrictions which are even worse.  I guess the issue isn't that bad.
Out of all the gcc test cases, I hit only 4.  In the context of gcc sim
testing, one option could be to use a special toolchain config, which
does not do the _ prefixing.  However, there's no such configure option
and the only way to change it is to change the USER_LABEL_PREFIX
definition, or add a subtarget which doesn't prefix _.  Another option
could be adding something to the global configure stuff so that the
option -fno-leading-underscore can be turned on by default for the
resulting compiler.

Cheers,
Oleg

Re: [testsuite] Fix multiple definitions of _init

[Mike Stump]

On Nov 30, 2014, at 11:21 PM, Oleg Endo <oleg.endo@t-online.de> wrote:
> When running the testsuite on a sh-elf configuration, some test cases
> fail due to multiple definitions of the function '_init'.  This is
> because on sh-elf every function is automatically prefixed with a '_'
> char.  When defining a C function 'init' in some code, the actual name
> becomes '_init' and this conflicts with the _init function in libgcc
> (crti.S).  While the behavior of adding the '_' prefix is debatable, the
> testsuite failures can be easily fixed by the attached patch.
> Is this OK for trunk?

No.  It is reasonable for the test suite to fail when the implementation of gcc is wrong (unclean) or newlib startup code is wrong (unclean).  Since that is what happened, the fix is to fix the cleanliness problem.

I’ve read through the thread…  I think the sh newlib port has a trivial bug and should be fixed.  Either, add an underscore or remove one, just like _all_ the other ports.  There is no complexity here, just a typo (or someone copied the wrong port).

When newlib/libc/sys/sh/crt0.S is fixed, and libgcc/config/sh/crt1.S is fixed, then these test cases will pass.

USER_LABEL_PREFIX  is not the issue.  Let me quote from the code:

        .long   _atexit
        .long   _init

This is wrong.

Also, _fini needs to be fixed as well.  It is unfortunate that the fix involves two packages, but, such is life.

Re: [testsuite] Fix multiple definitions of _init

[Joseph Myers]

On Tue, 2 Dec 2014, Richard Biener wrote:

> Joseph may have more experience with how targets should setup
> USER_LABEL_PREFIX to avoid this situation.

See e.g. config/arm/lib1funcs.S:

#define SYM(x) CONCAT1 (__USER_LABEL_PREFIX__, x)

(and the associated macro definition of CONCAT1 that uses, etc.).  If you 
have .S sources that may be used on targets both with and without a 
prefix, you should do something similar.  (The ELF gABI says "External C 
symbols have the same names in C and object files' symbol tables.", so ELF 
targets using leading underscores on C symbol names are going against the 
gABI.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [testsuite] Fix multiple definitions of _init

[Oleg Endo]

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

On Tue, 2014-12-02 at 10:23 -0800, Mike Stump wrote:
> No.  It is reasonable for the test suite to fail when the
> implementation of gcc is wrong (unclean) or newlib startup code is
> wrong (unclean).  Since that is what happened, the fix is to fix the
> cleanliness problem.
> 
> I’ve read through the thread…  I think the sh newlib port has a trivial
> bug and should be fixed.  Either, add an underscore or remove one, just
> like _all_ the other ports.  There is no complexity here, just a typo
> (or someone copied the wrong port).
> 
> When newlib/libc/sys/sh/crt0.S is fixed, and libgcc/config/sh/crt1.S is
> fixed, then these test cases will pass.
> 
> USER_LABEL_PREFIX  is not the issue.  Let me quote from the code:
> 
>         .long   _atexit         .long   _init
> 
> This is wrong.
> 
> Also, _fini needs to be fixed as well.  It is unfortunate that the fix
> involves two packages, but, such is life.

On Tue, 2014-12-02 at 21:55 +0000, Joseph Myers wrote:
> See e.g. config/arm/lib1funcs.S:
> 
> #define SYM(x) CONCAT1 (__USER_LABEL_PREFIX__, x)
> 
> (and the associated macro definition of CONCAT1 that uses, etc.).  If
> you have .S sources that may be used on targets both with and without
> a prefix, you should do something similar.  (The ELF gABI says
> "External C symbols have the same names in C and object files' symbol
> tables.", so ELF targets using leading underscores on C symbol names
> are going against the gABI.)

Thanks for the comments.  I've tried the attached patch on my newlib
config and it seems to fix the problem.  Newlib doesn't seem to be in
need for patching for non-SH64/SH5 in this case.

Kaz, could you please check if the patch doesn't break anything on
sh4-linux?  If so, I'd like to commit this to trunk.

I'm a bit puzzled though.  In libgcc/arm/crti.S there are also _init and
_fini function names.  It seems that an arm-eabi configuration doesn't
add the _ prefix.  But if it did, that'd be the same problem as on SH I
guess?

I still think there should be a configure option to control/override the
target defined default value of USER_LABEL_PREFIX.  But that's another
story.

Cheers,
Oleg


[-- Attachment #2: sh_libgcc_init_fini.patch --]
[-- Type: text/x-patch, Size: 3421 bytes --]

Index: libgcc/config/sh/crti.S
===================================================================
--- libgcc/config/sh/crti.S	(revision 218464)
+++ libgcc/config/sh/crti.S	(working copy)
@@ -22,6 +22,7 @@
 see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 <http://www.gnu.org/licenses/>.  */
 
+#include "crt.h"
 
 /* The code in sections .init and .fini is supposed to be a single
    regular function.  The function in .init is called directly from
@@ -44,8 +45,8 @@
 #else
 	.p2align 1
 #endif
-	.global	 _init
-_init:
+	.global	 GLOBAL(_init)
+GLOBAL(_init):
 #if __SHMEDIA__
 	addi	r15, -16, r15
 	st.q	r15, 8, r14
@@ -89,8 +90,8 @@
 #else
 	.p2align 1
 #endif
-	.global  _fini
-_fini:	
+	.global  GLOBAL(_fini)
+GLOBAL(_fini):	
 #if __SHMEDIA__
 	addi	r15, -16, r15
 	st.q	r15, 8, r14
Index: libgcc/config/sh/crt1.S
===================================================================
--- libgcc/config/sh/crt1.S	(revision 218464)
+++ libgcc/config/sh/crt1.S	(working copy)
@@ -22,6 +22,7 @@
 see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 <http://www.gnu.org/licenses/>.  */
 
+#include "crt.h"
 
 #ifdef MMU_SUPPORT
 	/* Section used for exception/timer interrupt stack area */
@@ -420,7 +421,7 @@
 #endif /* MMU_SUPPORT */
 
 	pt/l	.Lzero_bss_loop, tr0
-	pt/l	_init, tr5
+	pt/l	GLOBAL(_init), tr5
 	pt/l	___setup_argv_and_call_main, tr6
 	pt/l	_exit, tr7
 
@@ -452,7 +453,7 @@
 
 	! arrange for exit to call fini
 	pt/l	_atexit, tr1
-	LOAD_ADDR (_fini, r2)
+	LOAD_ADDR (GLOBAL(_fini), r2)
 	blink	tr1, r18
 
 	! call init
@@ -850,9 +851,9 @@
 atexit_k:
 	.long	_atexit
 init_k:
-	.long	_init
+	.long	GLOBAL(_init)
 fini_k:
-	.long	_fini
+	.long	GLOBAL(_fini)
 #ifdef VBR_SETUP
 old_vbr_k:
 	.long	old_vbr
@@ -1116,9 +1117,7 @@
 #if defined(__SH_FPU_ANY__)
 	.balign 4
 pervading_precision_k:
-#define CONCAT1(A,B) A##B
-#define CONCAT(A,B) CONCAT1(A,B)
-	.long CONCAT(__USER_LABEL_PREFIX__,__fpscr_values)+4
+	.long GLOBAL(__fpscr_values)+4
 #endif
 #else
 	mov.l 2f, r0     ! Load the old vbr setting (if any).
Index: libgcc/config/sh/crt.h
===================================================================
--- libgcc/config/sh/crt.h	(revision 0)
+++ libgcc/config/sh/crt.h	(revision 0)
@@ -0,0 +1,29 @@
+/* Copyright (C) 2014 Free Software Foundation, Inc.
+
+This file is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by the
+Free Software Foundation; either version 3, or (at your option) any
+later version.
+
+This file is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+General Public License for more details.
+
+Under Section 7 of GPL version 3, you are granted additional
+permissions described in the GCC Runtime Library Exception, version
+3.1, as published by the Free Software Foundation.
+
+You should have received a copy of the GNU General Public License and
+a copy of the GCC Runtime Library Exception along with this program;
+see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
+<http://www.gnu.org/licenses/>.  */
+
+#ifndef __USER_LABEL_PREFIX__
+#error  __USER_LABEL_PREFIX__ not defined
+#endif
+
+#define CONCAT(a, b) CONCAT2(a, b)
+#define CONCAT2(a, b) a ## b
+
+#define GLOBAL(X) CONCAT(__USER_LABEL_PREFIX__,X)

Re: [testsuite] Fix multiple definitions of _init

[Kaz Kojima]

Oleg Endo <oleg.endo@t-online.de> wrote:
> Kaz, could you please check if the patch doesn't break anything on
> sh4-linux?  If so, I'd like to commit this to trunk.

Build and test ok on sh4-unknown-linux-gnu.

Regards,
	kaz

Re: [testsuite] Fix multiple definitions of _init

[Oleg Endo]

On Mon, 2014-12-08 at 14:51 +0900, Kaz Kojima wrote:
> Oleg Endo <oleg.endo@t-online.de> wrote:
> > Kaz, could you please check if the patch doesn't break anything on
> > sh4-linux?  If so, I'd like to commit this to trunk.
> 
> Build and test ok on sh4-unknown-linux-gnu.
> 

Now also tested here with 
make -k check RUNTESTFLAGS="--target_board=sh-sim
\{-m2/-ml,-m2/-mb,-m2a/-mb,-m4/-ml,-m4/-mb,-m4a/-ml,-m4a/-mb}"

Committed as r218807.

Cheers,
Oleg