diff mbox series

Prevent LTO section collision for a symbol name starting with '*'.

Message ID 55b4acda-9673-557b-5819-50bff07fa095@suse.cz
State New
Headers show
Series Prevent LTO section collision for a symbol name starting with '*'. | expand

Commit Message

Martin Liška Aug. 9, 2019, 1:57 p.m. UTC 
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?
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

Comments

Richard Biener Aug. 12, 2019, 9:45 a.m. UTC | #1 
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.  IIRC this is some target mangling applied to DECL_ASSEMBLER_NAME?
And section names cannot contain '*'?  Or do we need to actually
indentify '*fn' and 'fn' as the same?  For the testcase what is the clashing
symbol?  Can't we have many so that using "0" always is broken as well?

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
>
>
Martin Liška Aug. 12, 2019, 9:57 a.m. UTC | #2 
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() {}


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

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
>>
>>
Richard Biener Aug. 12, 2019, 10:39 a.m. UTC | #3 
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
> >>
> >>
>
Martin Liška Aug. 12, 2019, 10:43 a.m. UTC | #4 
On 8/12/19 12:39 PM, Richard Biener wrote:
> 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.

Ok, so please advise me how can we achieve that?

Martin
Richard Biener Aug. 12, 2019, 10:46 a.m. UTC | #5 
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
> > >>
> > >>
> >
Richard Biener Aug. 12, 2019, 10:49 a.m. UTC | #6 
On Mon, Aug 12, 2019 at 12:43 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 8/12/19 12:39 PM, Richard Biener wrote:
> > 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.
>
> Ok, so please advise me how can we achieve that?

I'd change lto_get_section_name to take the fndecl instead of name
and key on DECL_EXTERNAL/DECL_INLINE (or how we identify
extern gnu-inline functions), pre-pending a zero.

Richard.

> Martin
Richard Biener Aug. 12, 2019, 10:51 a.m. UTC | #7 
On Mon, Aug 12, 2019 at 12:49 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, Aug 12, 2019 at 12:43 PM Martin Liška <mliska@suse.cz> wrote:
> >
> > On 8/12/19 12:39 PM, Richard Biener wrote:
> > > 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.
> >
> > Ok, so please advise me how can we achieve that?
>
> I'd change lto_get_section_name to take the fndecl instead of name
> and key on DECL_EXTERNAL/DECL_INLINE (or how we identify
> extern gnu-inline functions), pre-pending a zero.

Or even "simpler"(?) by mangling DECL_ASSEMBLER_NAME for
these functions throughout GCC - they should never be emitted.

Honza?

Richard.

> Richard.
>
> > Martin
Jan Hubicka Aug. 15, 2019, 2:33 p.m. UTC | #8 
> 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.  IIRC this is some target mangling applied to DECL_ASSEMBLER_NAME?
DECL_ASSEMBLER_NAME works in a way that if it starts with "*"
it is copied verbatim to the linker ouptut.  If it is w/o "*"
then user_label_prefix is applied first, see 
symbol_table::assembler_names_equal_p

So if we skip "*" one can definitly construct testcases of different
function names ending up in same section especially when
user_label_prefix is non-empty, like on Windows I think it is "_".

> And section names cannot contain '*'?  Or do we need to actually
> indentify '*fn' and 'fn' as the same?  For the testcase what is the clashing
> symbol?  Can't we have many so that using "0" always is broken as well?

We may have duplicate symbols during the compile time->WPA streaming
since we do not do lto-symtab at compile time and user can use asm names
that way.  This is not limited to extern inlines, so it would be nice to
make this work reliably. I also plan support for keeping multiple
function bodies defined for one symbol in cases it is necessary (glibc
checking, when optimization flags are mismatches) for WPA->ltrans
streaming.

I was always considering option to simply use node->order ids to stream
sections.  Because of way node->order is merged we area always able to
recover the ID.

It is however kind of nice to see actual names in the objdump
of the LTO .o files.  I would not mind that much to see this go and it
would also save bit of space since symbol names can be long.

Honza
> 
> 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
> >
> >
Richard Biener Aug. 16, 2019, 8:55 a.m. UTC | #9 
On Thu, Aug 15, 2019 at 4:33 PM Jan Hubicka <hubicka@ucw.cz> 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.  IIRC this is some target mangling applied to DECL_ASSEMBLER_NAME?
> DECL_ASSEMBLER_NAME works in a way that if it starts with "*"
> it is copied verbatim to the linker ouptut.  If it is w/o "*"
> then user_label_prefix is applied first, see
> symbol_table::assembler_names_equal_p
>
> So if we skip "*" one can definitly construct testcases of different
> function names ending up in same section especially when
> user_label_prefix is non-empty, like on Windows I think it is "_".
>
> > And section names cannot contain '*'?  Or do we need to actually
> > indentify '*fn' and 'fn' as the same?  For the testcase what is the clashing
> > symbol?  Can't we have many so that using "0" always is broken as well?
>
> We may have duplicate symbols during the compile time->WPA streaming
> since we do not do lto-symtab at compile time and user can use asm names
> that way.  This is not limited to extern inlines, so it would be nice to
> make this work reliably. I also plan support for keeping multiple
> function bodies defined for one symbol in cases it is necessary (glibc
> checking, when optimization flags are mismatches) for WPA->ltrans
> streaming.
>
> I was always considering option to simply use node->order ids to stream
> sections.  Because of way node->order is merged we area always able to
> recover the ID.

Heh, that sounds like a nice idea indeed.

>
> It is however kind of nice to see actual names in the objdump
> of the LTO .o files.  I would not mind that much to see this go and it
> would also save bit of space since symbol names can be long.
>
> Honza
> >
> > 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
> > >
> > >
Martin Liška Aug. 23, 2019, 12:33 p.m. UTC | #10 
On 8/15/19 4:33 PM, Jan Hubicka 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.  IIRC this is some target mangling applied to DECL_ASSEMBLER_NAME?
> DECL_ASSEMBLER_NAME works in a way that if it starts with "*"
> it is copied verbatim to the linker ouptut.  If it is w/o "*"
> then user_label_prefix is applied first, see 
> symbol_table::assembler_names_equal_p
> 
> So if we skip "*" one can definitly construct testcases of different
> function names ending up in same section especially when
> user_label_prefix is non-empty, like on Windows I think it is "_".
> 
>> And section names cannot contain '*'?  Or do we need to actually
>> indentify '*fn' and 'fn' as the same?  For the testcase what is the clashing
>> symbol?  Can't we have many so that using "0" always is broken as well?
> 
> We may have duplicate symbols during the compile time->WPA streaming
> since we do not do lto-symtab at compile time and user can use asm names
> that way.  This is not limited to extern inlines, so it would be nice to
> make this work reliably. I also plan support for keeping multiple
> function bodies defined for one symbol in cases it is necessary (glibc
> checking, when optimization flags are mismatches) for WPA->ltrans
> streaming.
> 
> I was always considering option to simply use node->order ids to stream
> sections.  Because of way node->order is merged we area always able to
> recover the ID.

Looks good to me. The only small issue is that in lto-cgraph.c:

  1220    const char *section;
  1221    order = streamer_read_hwi (ib) + order_base;
  1222    clone_ref = streamer_read_hwi (ib);
...
  1245    node->order = order;
  1246    if (order >= symtab->order)
  1247      symtab->order = order + 1;

So in WPA, we shift order by order_base. For streaming purpose I'll need
something like symtab_node::lgen_order that will be streamer_read_hwi (ib).

Are you fine with that?

> 
> It is however kind of nice to see actual names in the objdump
> of the LTO .o files.  I would not mind that much to see this go and it
> would also save bit of space since symbol names can be long.

Or we can mix name.order as a section name?
Martin

> 
> Honza
>>
>> 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
>>>
>>>
Martin Liška Aug. 23, 2019, 12:57 p.m. UTC | #11 
On 8/23/19 2:33 PM, Martin Liška wrote:
> So in WPA, we shift order by order_base. For streaming purpose I'll need
> something like symtab_node::lgen_order that will be streamer_read_hwi (ib).

And we'll probably need to stream the original LGEN symtab_node::order
in order to read proper section during LTRANS. Am I right?

Martin
Jan Hubicka Aug. 23, 2019, 2:37 p.m. UTC | #12 
> On 8/23/19 2:33 PM, Martin Liška wrote:
> > So in WPA, we shift order by order_base. For streaming purpose I'll need
> > something like symtab_node::lgen_order that will be streamer_read_hwi (ib).
> 
> And we'll probably need to stream the original LGEN symtab_node::order
> in order to read proper section during LTRANS. Am I right?

Well, at WPA time you know node->order and from lto_file_data you know
how to recalculate node->order to original node->order at compile time.
During the section copying you can arrange it to be renamed to new
node->order which you then stream from WPA to ltrans.

Honza
> 
> Martin
Martin Liška Aug. 23, 2019, 2:53 p.m. UTC | #13 
On 8/23/19 4:37 PM, Jan Hubicka wrote:
>> On 8/23/19 2:33 PM, Martin Liška wrote:
>>> So in WPA, we shift order by order_base. For streaming purpose I'll need
>>> something like symtab_node::lgen_order that will be streamer_read_hwi (ib).
>>
>> And we'll probably need to stream the original LGEN symtab_node::order
>> in order to read proper section during LTRANS. Am I right?
> 
> Well, at WPA time you know node->order and from lto_file_data you know
> how to recalculate node->order to original node->order at compile time.

How can I recalculate that? One possible solution is to store order_base
for each struct lto_file_decl_data *file_data in input_node function.

> During the section copying you can arrange it to be renamed to new
> node->order which you then stream from WPA to ltrans.

That's a nice solution. Can you please point me to the code which
does that?

And what's your opinion about section naming scheme $name.$order.$file_hash?

Thanks,
Martin

> 
> Honza
>>
>> Martin
Jan Hubicka Aug. 23, 2019, 3:05 p.m. UTC | #14 
> On 8/23/19 4:37 PM, Jan Hubicka wrote:
> >> On 8/23/19 2:33 PM, Martin Liška wrote:
> >>> So in WPA, we shift order by order_base. For streaming purpose I'll need
> >>> something like symtab_node::lgen_order that will be streamer_read_hwi (ib).
> >>
> >> And we'll probably need to stream the original LGEN symtab_node::order
> >> in order to read proper section during LTRANS. Am I right?
> > 
> > Well, at WPA time you know node->order and from lto_file_data you know
> > how to recalculate node->order to original node->order at compile time.
> 
> How can I recalculate that? One possible solution is to store order_base
> for each struct lto_file_decl_data *file_data in input_node function.

Yep, i would keep order_base in file_data.
> 
> > During the section copying you can arrange it to be renamed to new
> > node->order which you then stream from WPA to ltrans.
> 
> That's a nice solution. Can you please point me to the code which
> does that?

copy_function_or_variable preserves section name but it seems like it
should be easy to invent new name there.
> 
> And what's your opinion about section naming scheme $name.$order.$file_hash?

Why you need the file hash? I hope the orders would be reasonably stable
way to code things w/o conflicts.

Honza
> 
> Thanks,
> Martin
> 
> > 
> > Honza
> >>
> >> Martin
>
Martin Liška Aug. 23, 2019, 3:14 p.m. UTC | #15 
On 8/23/19 5:05 PM, Jan Hubicka wrote:
>> On 8/23/19 4:37 PM, Jan Hubicka wrote:
>>>> On 8/23/19 2:33 PM, Martin Liška wrote:
>>>>> So in WPA, we shift order by order_base. For streaming purpose I'll need
>>>>> something like symtab_node::lgen_order that will be streamer_read_hwi (ib).
>>>>
>>>> And we'll probably need to stream the original LGEN symtab_node::order
>>>> in order to read proper section during LTRANS. Am I right?
>>>
>>> Well, at WPA time you know node->order and from lto_file_data you know
>>> how to recalculate node->order to original node->order at compile time.
>>
>> How can I recalculate that? One possible solution is to store order_base
>> for each struct lto_file_decl_data *file_data in input_node function.
> 
> Yep, i would keep order_base in file_data.

Fine.

>>
>>> During the section copying you can arrange it to be renamed to new
>>> node->order which you then stream from WPA to ltrans.
>>
>> That's a nice solution. Can you please point me to the code which
>> does that?
> 
> copy_function_or_variable preserves section name but it seems like it
> should be easy to invent new name there.

Great, thanks.

>>
>> And what's your opinion about section naming scheme $name.$order.$file_hash?
> 
> Why you need the file hash? I hope the orders would be reasonably stable
> way to code things w/o conflicts.

Sorry, I mean the hash which we use in LGEN even now. So e.g.:

readelf -S -W main.o
There are 17 section headers, starting at offset 0x958:

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
...
  [ 6] .gnu.lto_main.e4fa533f2618f732 PROGBITS        0000000000000000 00007c 000287 00   E  0   0  1

So my question should be if we want $name.$order as a section name?

Martin

> 
> Honza
>>
>> Thanks,
>> Martin
>>
>>>
>>> Honza
>>>>
>>>> Martin
>>
Jan Hubicka Aug. 23, 2019, 10:36 p.m. UTC | #16 
> 
> Sorry, I mean the hash which we use in LGEN even now. So e.g.:
> 
> readelf -S -W main.o
> There are 17 section headers, starting at offset 0x958:
> 
> Section Headers:
>   [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
> ...
>   [ 6] .gnu.lto_main.e4fa533f2618f732 PROGBITS        0000000000000000 00007c 000287 00   E  0   0  1
> 
> So my question should be if we want $name.$order as a section name?

Aha, I see.  Those hashes are there to let ld -r to link LTO files.
Different sections will just get copied into the output file and will
happen to work.  Yes, I think we want to keep this hash.

Honza
> 
> Martin
> 
> > 
> > Honza
> >>
> >> Thanks,
> >> Martin
> >>
> >>>
> >>> Honza
> >>>>
> >>>> Martin
> >>
>
Martin Liška Aug. 26, 2019, 10:04 a.m. UTC | #17 
Ok. I have a semi-working patch that has issues for inline clones.
When we call cgraph_node::get_untransformed_body for an inline clone,
then one needs to use clone_of->order to find proper LTO stream.

What's more problematic is that such clone can be expanded:

f/12 (f) @0x7ffff769f708
  Type: function definition analyzed
  Visibility: external public
  References: mumble.lto_priv.0/8 (write)
  Referring: 
  Read from file: /tmp/cciAkXHp.ltrans1.o
  Function f/12 is inline copy in main/0
  Availability: local
  Function flags: count:1073741824 (estimated locally) local nonfreeing_fn executed_once
  Called by: main/0 (inlined) (1073741824 (estimated locally),1.00 per call) 
  Calls: 

and lost. So we end up with an orphan and we ICE with:

/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/lto/20081112_0.c: In function ‘main’:
/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/lto/20081112_0.c:10:3: fatal error: /tmp/cciAkXHp.ltrans1.o: section f is missing

So usage of symtab_node::order seems awkward to me :/

Martin
Martin Liška Sept. 9, 2019, 12:39 p.m. UTC | #18 
PING^1

On 8/26/19 12:04 PM, Martin Liška wrote:
> Ok. I have a semi-working patch that has issues for inline clones.
> When we call cgraph_node::get_untransformed_body for an inline clone,
> then one needs to use clone_of->order to find proper LTO stream.
> 
> What's more problematic is that such clone can be expanded:
> 
> f/12 (f) @0x7ffff769f708
>   Type: function definition analyzed
>   Visibility: external public
>   References: mumble.lto_priv.0/8 (write)
>   Referring: 
>   Read from file: /tmp/cciAkXHp.ltrans1.o
>   Function f/12 is inline copy in main/0
>   Availability: local
>   Function flags: count:1073741824 (estimated locally) local nonfreeing_fn executed_once
>   Called by: main/0 (inlined) (1073741824 (estimated locally),1.00 per call) 
>   Calls: 
> 
> and lost. So we end up with an orphan and we ICE with:
> 
> /home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/lto/20081112_0.c: In function ‘main’:
> /home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/lto/20081112_0.c:10:3: fatal error: /tmp/cciAkXHp.ltrans1.o: section f is missing
> 
> So usage of symtab_node::order seems awkward to me :/
> 
> Martin
>
Jan Hubicka Sept. 9, 2019, 2:33 p.m. UTC | #19 
> PING^1
> 
> On 8/26/19 12:04 PM, Martin Liška wrote:
> > Ok. I have a semi-working patch that has issues for inline clones.
> > When we call cgraph_node::get_untransformed_body for an inline clone,
> > then one needs to use clone_of->order to find proper LTO stream.

This seems OK to me - when using inline clone we really look for a body
of its master, so that seems OK.
> > 
> > What's more problematic is that such clone can be expanded:
> > 
> > f/12 (f) @0x7ffff769f708
> >   Type: function definition analyzed
> >   Visibility: external public
> >   References: mumble.lto_priv.0/8 (write)
> >   Referring: 
> >   Read from file: /tmp/cciAkXHp.ltrans1.o
> >   Function f/12 is inline copy in main/0
> >   Availability: local
> >   Function flags: count:1073741824 (estimated locally) local nonfreeing_fn executed_once
> >   Called by: main/0 (inlined) (1073741824 (estimated locally),1.00 per call) 
> >   Calls: 
> > 
> > and lost. So we end up with an orphan and we ICE with:

We do some work on removing unnecesary master clone when function is
fully inlined and I guess in that case you lose the order info.
One option would be to copy order into all inline clones (it does not
have very good meaning there) or do that when reshaping the tree.
This is done in cgraph_node::remove at the place clone_of is
manipulated.
This is probably bit cleaner.
> > 
> > /home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/lto/20081112_0.c: In function ‘main’:
> > /home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/lto/20081112_0.c:10:3: fatal error: /tmp/cciAkXHp.ltrans1.o: section f is missing
> > 
> > So usage of symtab_node::order seems awkward to me :/
> > 
> > Martin
> > 
>
Martin Liška Sept. 11, 2019, 11:38 a.m. UTC | #20 
On 9/9/19 4:33 PM, Jan Hubicka wrote:
>> PING^1
>>
>> On 8/26/19 12:04 PM, Martin Liška wrote:
>>> Ok. I have a semi-working patch that has issues for inline clones.
>>> When we call cgraph_node::get_untransformed_body for an inline clone,
>>> then one needs to use clone_of->order to find proper LTO stream.
> 
> This seems OK to me - when using inline clone we really look for a body
> of its master, so that seems OK.

Ok!

>>>
>>> What's more problematic is that such clone can be expanded:
>>>
>>> f/12 (f) @0x7ffff769f708
>>>   Type: function definition analyzed
>>>   Visibility: external public
>>>   References: mumble.lto_priv.0/8 (write)
>>>   Referring: 
>>>   Read from file: /tmp/cciAkXHp.ltrans1.o
>>>   Function f/12 is inline copy in main/0
>>>   Availability: local
>>>   Function flags: count:1073741824 (estimated locally) local nonfreeing_fn executed_once
>>>   Called by: main/0 (inlined) (1073741824 (estimated locally),1.00 per call) 
>>>   Calls: 
>>>
>>> and lost. So we end up with an orphan and we ICE with:
> 
> We do some work on removing unnecesary master clone when function is
> fully inlined and I guess in that case you lose the order info.
> One option would be to copy order into all inline clones (it does not
> have very good meaning there) or do that when reshaping the tree.

What do you mean by "reshaping the tree"?

> This is done in cgraph_node::remove at the place clone_of is
> manipulated.

The inline_clone manipulation happens in cgraph_node::find_replacement where
we manipulate the clone_of.

Martin

> This is probably bit cleaner.
>>>
>>> /home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/lto/20081112_0.c: In function ‘main’:
>>> /home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/lto/20081112_0.c:10:3: fatal error: /tmp/cciAkXHp.ltrans1.o: section f is missing
>>>
>>> So usage of symtab_node::order seems awkward to me :/
>>>
>>> Martin
>>>
>>
Martin Liška Sept. 18, 2019, 10:14 a.m. UTC | #21 
On 9/11/19 1:38 PM, Martin Liška wrote:
> The inline_clone manipulation happens in cgraph_node::find_replacement where
> we manipulate the clone_of.

I fixed that but there's a similar situation which goes other way around:

cgraph_node *
cgraph_node::get_create (tree decl)
{
  cgraph_node *first_clone = cgraph_node::get (decl);

  if (first_clone && !first_clone->global.inlined_to)
    return first_clone;

  cgraph_node *node = cgraph_node::create (decl);
  if (first_clone)
    {
      first_clone->clone_of = node;

Here we come up with a new parent and this->clone_of is set to the parent.
We ought to come cgraph_node::order here, but I don't like.
Right now cgraph_node::order is a way how one can identify a node in IPA dumps.

The patch is breaking that. I'm not sure we want the patch right now.
Martin
Martin Liška Oct. 22, 2019, 11:09 a.m. UTC | #22 
@Honza: PING^1

On 9/18/19 12:14 PM, Martin Liška wrote:
> On 9/11/19 1:38 PM, Martin Liška wrote:
>> The inline_clone manipulation happens in cgraph_node::find_replacement where
>> we manipulate the clone_of.
> 
> I fixed that but there's a similar situation which goes other way around:
> 
> cgraph_node *
> cgraph_node::get_create (tree decl)
> {
>   cgraph_node *first_clone = cgraph_node::get (decl);
> 
>   if (first_clone && !first_clone->global.inlined_to)
>     return first_clone;
> 
>   cgraph_node *node = cgraph_node::create (decl);
>   if (first_clone)
>     {
>       first_clone->clone_of = node;
> 
> Here we come up with a new parent and this->clone_of is set to the parent.
> We ought to come cgraph_node::order here, but I don't like.
> Right now cgraph_node::order is a way how one can identify a node in IPA dumps.
> 
> The patch is breaking that. I'm not sure we want the patch right now.
> Martin
>
Martin Liška Oct. 29, 2019, 11:31 a.m. UTC | #23 
On 10/22/19 1:09 PM, Martin Liška wrote:
> @Honza: PING^1
> 
> On 9/18/19 12:14 PM, Martin Liška wrote:
>> On 9/11/19 1:38 PM, Martin Liška wrote:
>>> The inline_clone manipulation happens in cgraph_node::find_replacement where
>>> we manipulate the clone_of.
>>
>> I fixed that but there's a similar situation which goes other way around:
>>
>> cgraph_node *
>> cgraph_node::get_create (tree decl)
>> {
>>   cgraph_node *first_clone = cgraph_node::get (decl);
>>
>>   if (first_clone && !first_clone->global.inlined_to)
>>     return first_clone;
>>
>>   cgraph_node *node = cgraph_node::create (decl);
>>   if (first_clone)
>>     {
>>       first_clone->clone_of = node;
>>
>> Here we come up with a new parent and this->clone_of is set to the parent.
>> We ought to come cgraph_node::order here, but I don't like.
>> Right now cgraph_node::order is a way how one can identify a node in IPA dumps.
>>
>> The patch is breaking that. I'm not sure we want the patch right now.
>> Martin
>>
> 

Ok, I've got a version that is working right.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin
Jan Hubicka Oct. 29, 2019, 2:24 p.m. UTC | #24 
>  gcc/cgraph.c                         | 14 ++++++++++----
>  gcc/cgraphclones.c                   |  5 +++++
>  gcc/ipa-fnsummary.c                  |  2 +-
>  gcc/ipa-hsa.c                        |  2 +-
>  gcc/ipa-icf.c                        |  2 +-
>  gcc/ipa-prop.c                       |  6 ++++--
>  gcc/ipa-sra.c                        |  2 +-
>  gcc/lto-cgraph.c                     | 13 +++++--------
>  gcc/lto-opts.c                       |  2 +-
>  gcc/lto-section-in.c                 | 12 +++++++-----
>  gcc/lto-section-out.c                |  2 +-
>  gcc/lto-streamer-in.c                |  4 ++--
>  gcc/lto-streamer-out.c               | 21 ++++++++++++---------
>  gcc/lto-streamer.c                   |  9 +++++++--
>  gcc/lto-streamer.h                   | 10 +++++++---
>  gcc/lto/lto-common.c                 | 13 +++++++------
>  gcc/testsuite/gcc.dg/lto/pr91393_0.c | 11 +++++++++++
>  gcc/varpool.c                        |  7 ++++---
>  18 files changed, 87 insertions(+), 50 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/lto/pr91393_0.c
> 
> @@ -4863,7 +4865,7 @@ ipcp_read_transformation_summaries (void)
>        size_t len;
>        const char *data = lto_get_section_data (file_data,
>  					       LTO_section_ipcp_transform,
> -					       NULL, &len);
> +					       NULL, 0, &len);

I wonder if we can get prettier interface for this. Perhaps just a
wrapper lto_get_summary_section_data that does not need so many
parameters.
> --- a/gcc/varpool.c
> +++ b/gcc/varpool.c
> @@ -299,11 +299,12 @@ varpool_node::get_constructor (void)
>  	 = lto_get_function_in_decl_state (file_data, decl);
>  
>    data = lto_get_section_data (file_data, LTO_section_function_body,
> -			       name, &len, decl_state->compressed);
> +			       name, order - file_data->order_base,
> +			       &len, decl_state->compressed);
>    if (!data)
> -    fatal_error (input_location, "%s: section %s is missing",
> +    fatal_error (input_location, "%s: section %s.%d is missing",
>  		 file_data->file_name,
> -		 name);
> +		 name, order - file_data->order_base);

Are we now going to get addition .xy indexes for summary sections too?

Patch looks OK.

Honza
Martin Liška Oct. 30, 2019, 10:08 a.m. UTC | #25 
On 10/29/19 3:24 PM, Jan Hubicka wrote:
>>  gcc/cgraph.c                         | 14 ++++++++++----
>>  gcc/cgraphclones.c                   |  5 +++++
>>  gcc/ipa-fnsummary.c                  |  2 +-
>>  gcc/ipa-hsa.c                        |  2 +-
>>  gcc/ipa-icf.c                        |  2 +-
>>  gcc/ipa-prop.c                       |  6 ++++--
>>  gcc/ipa-sra.c                        |  2 +-
>>  gcc/lto-cgraph.c                     | 13 +++++--------
>>  gcc/lto-opts.c                       |  2 +-
>>  gcc/lto-section-in.c                 | 12 +++++++-----
>>  gcc/lto-section-out.c                |  2 +-
>>  gcc/lto-streamer-in.c                |  4 ++--
>>  gcc/lto-streamer-out.c               | 21 ++++++++++++---------
>>  gcc/lto-streamer.c                   |  9 +++++++--
>>  gcc/lto-streamer.h                   | 10 +++++++---
>>  gcc/lto/lto-common.c                 | 13 +++++++------
>>  gcc/testsuite/gcc.dg/lto/pr91393_0.c | 11 +++++++++++
>>  gcc/varpool.c                        |  7 ++++---
>>  18 files changed, 87 insertions(+), 50 deletions(-)
>>  create mode 100644 gcc/testsuite/gcc.dg/lto/pr91393_0.c
>>
>> @@ -4863,7 +4865,7 @@ ipcp_read_transformation_summaries (void)
>>        size_t len;
>>        const char *data = lto_get_section_data (file_data,
>>  					       LTO_section_ipcp_transform,
>> -					       NULL, &len);
>> +					       NULL, 0, &len);
> 
> I wonder if we can get prettier interface for this. Perhaps just a
> wrapper lto_get_summary_section_data that does not need so many
> parameters.

Yes, you are right and I've fixed that in updated version of the patch
that I'm going to install.

>> --- a/gcc/varpool.c
>> +++ b/gcc/varpool.c
>> @@ -299,11 +299,12 @@ varpool_node::get_constructor (void)
>>  	 = lto_get_function_in_decl_state (file_data, decl);
>>  
>>    data = lto_get_section_data (file_data, LTO_section_function_body,
>> -			       name, &len, decl_state->compressed);
>> +			       name, order - file_data->order_base,
>> +			       &len, decl_state->compressed);
>>    if (!data)
>> -    fatal_error (input_location, "%s: section %s is missing",
>> +    fatal_error (input_location, "%s: section %s.%d is missing",
>>  		 file_data->file_name,
>> -		 name);
>> +		 name, order - file_data->order_base);
> 
> Are we now going to get addition .xy indexes for summary sections too?

No, these will remain without any .xy extension.

> 
> Patch looks OK.

Thanks,
Martin

> 
> Honza
>
diff mbox series

Patch

diff --git a/gcc/lto-streamer.c b/gcc/lto-streamer.c
index bd0126faebb..ffcaae516a5 100644
--- a/gcc/lto-streamer.c
+++ b/gcc/lto-streamer.c
@@ -124,9 +124,18 @@  lto_get_section_name (int section_type, const char *name, struct lto_file_decl_d
     {
       gcc_assert (name != NULL);
       if (name[0] == '*')
-	name++;
-      add = name;
-      sep = "";
+	{
+	  /* Symbols starting with '*' can clash with a symbol
+	     that has the same name.  Use then zero as one can't
+	     use digits at the beginning of identifiers.  */
+	  sep = "0";
+	  add = name + 1;
+	}
+      else
+	{
+	  add = name;
+	  sep = "";
+	}
     }
   else if (section_type < LTO_N_SECTION_TYPES)
     {
diff --git a/gcc/testsuite/gcc.dg/lto/pr91393_0.c b/gcc/testsuite/gcc.dg/lto/pr91393_0.c
new file mode 100644
index 00000000000..43b2426c86b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/lto/pr91393_0.c
@@ -0,0 +1,11 @@ 
+void __open_alias(int, ...) __asm__("open");
+void __open_alias(int flags, ...) {}
+extern __inline __attribute__((__gnu_inline__)) int open() {}
+struct {
+  void *func;
+} a = {open};
+
+int main()
+{
+  return 0;
+}