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

[Richard Biener <richard dot guenther at gmail dot com>]

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.

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
> >>
> >>
>