| Message ID | 1417418467.3387.89.camel@yam-132-YW-E178-FTW |
|---|---|
| State | New |
| Headers | show |
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.
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
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 >
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
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.
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.)
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);