This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.


On Mon, Aug 12, 2019 at 12:39 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, Aug 12, 2019 at 11:57 AM Martin Liška <mliska@suse.cz> wrote:
> >
> > On 8/12/19 11:45 AM, Richard Biener wrote:
> > > On Fri, Aug 9, 2019 at 3:57 PM Martin Liška <mliska@suse.cz> wrote:
> > >>
> > >> Hi.
> > >>
> > >> The patch is about prevention of LTO section name clashing.
> > >> Now we have a situation where body of 2 functions is streamed
> > >> into the same ELF section. Then we'll end up with smashed data.
> > >>
> > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> > >>
> > >> Ready to be installed?
> > >
> > > I think the comment should mention why we skip a leading '*'
> > > at all.
> >
> > git blame helps us here:
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42531
> >
> >
> > > IIRC this is some target mangling applied to DECL_ASSEMBLER_NAME?
> >
> > Yes, it's prepended here:
> > set_user_assembler_name
> >
> > > And section names cannot contain '*'?
> >
> > As seen in the PR, not.
> >
> > > Or do we need to actually
> > > indentify '*fn' and 'fn' as the same?
> >
> > No, they should be identified as different symbols.
> >
> > >  For the testcase what is the clashing
> > > symbol?
> >
> > void __open_alias(int, ...) __asm__("open"); - aka *open
> > and:
> > +extern __inline __attribute__((__gnu_inline__)) int open() {}
>
> Hmm, so for
>
> void __open_alias(int, ...) __asm__("open");
> void __open_alias(int flags, ...) {}
>
> a non-LTO compile will output __open_alias with the symbol "open".
> Then we have
>
> extern __inline __attribute__((__gnu_inline__)) int open() {}
>
> which is the "other" body for 'open' but it shouldn't really be output
> to the symbol table.  Still we want to emit its body for the purpose
> of inlining.  So IMHO the fix is not to do magic '0' appending for
> the user-asm-name case but instead "mangling" of extern inline
> bodies because those are the speciality causing the issue in the end.
>
> >
> > >  Can't we have many so that using "0" always is broken as well?
> >
> > If I'll define 2 symbols that alias to a same asm name, I'll get:
> > $ cat clash.c
> > void __open_alias(int, ...) __asm__("open");
> > void __open_alias2(int, ...) __asm__("open");
> > void __open_alias(int flags, ...) {}
> > void __open_alias2(int flags, ...) {}
> > extern __inline __attribute__((__gnu_inline__)) int open() {}
> > struct {
> >   void *func;
> > } a = {open};
> >
> > int main()
> > {
> >   return 0;
> > }
> >
> > $ gcc clash.c -flto
> > lto1: fatal error: missing resolution data for *open
> > compilation terminated.
> >
> > Which is a reasonable error message to me.
>
> Here I don't agree, earlier diagnostic of such invalid testcase
> would be welcome instead.  If you build w/o LTO you'll get
>
> /tmp/ccjjlMhr.s: Assembler messages:
> /tmp/ccjjlMhr.s:40: Error: symbol `open' is already defined
>
> IMHO this is invalid (undiagnosed) C.

Btw, with -flto we also only get a single function section for
both (not sure if the bytecode ends up sensible).

> Richard.
>
>
>
> > Martin
> >
> > >
> > > Richard.
> > >
> > >> Thanks,
> > >> Martin
> > >>
> > >> gcc/ChangeLog:
> > >>
> > >> 2019-08-09  Martin Liska  <mliska@suse.cz>
> > >>
> > >>         PR lto/91393
> > >>         PR lto/88220
> > >>         * lto-streamer.c (lto_get_section_name): Replace '*' leading
> > >>         character with '0'.
> > >>
> > >> gcc/testsuite/ChangeLog:
> > >>
> > >> 2019-08-09  Martin Liska  <mliska@suse.cz>
> > >>
> > >>         PR lto/91393
> > >>         PR lto/88220
> > >>         * gcc.dg/lto/pr91393_0.c: New test.
> > >> ---
> > >>  gcc/lto-streamer.c                   | 15 ++++++++++++---
> > >>  gcc/testsuite/gcc.dg/lto/pr91393_0.c | 11 +++++++++++
> > >>  2 files changed, 23 insertions(+), 3 deletions(-)
> > >>  create mode 100644 gcc/testsuite/gcc.dg/lto/pr91393_0.c
> > >>
> > >>
> >


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]