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


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