Re: [testsuite] Fix multiple definitions of _init
On Mon, 2014-12-08 at 14:51 +0900, Kaz Kojima wrote:
Oleg Endo oleg.e...@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
Re: [testsuite] Fix multiple definitions of _init
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 +, 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 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
Re: [testsuite] Fix multiple definitions of _init
Oleg Endo oleg.e...@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
On Mon, Dec 1, 2014 at 11:48 PM, Oleg Endo oleg.e...@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.e...@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
On Tue, 2014-12-02 at 11:22 +0100, Richard Biener wrote: On Mon, Dec 1, 2014 at 11:48 PM, Oleg Endo oleg.e...@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.e...@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
On Nov 30, 2014, at 11:21 PM, Oleg Endo oleg.e...@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
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 jos...@codesourcery.com
Re: [testsuite] Fix multiple definitions of _init
On Mon, Dec 1, 2014 at 8:21 AM, Oleg Endo oleg.e...@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
On Mon, 2014-12-01 at 12:09 +0100, Richard Biener wrote: On Mon, Dec 1, 2014 at 8:21 AM, Oleg Endo oleg.e...@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
[testsuite] Fix multiple definitions of _init
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.
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);
