Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
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 > From b865024ebdf1c89cf88dbc23d694b6568fb540b7 Mon Sep 17 00:00:00 2001 From: Martin Liska Date: Mon, 26 Aug 2019 11:59:23 +0200 Subject: [PATCH] Use symtab_node::order in LTO sections with body. gcc/ChangeLog: 2019-10-30 Martin Liska PR lto/91393 PR lto/88220 * cgraph.c (cgraph_node::get_create): Overwrite node->order from a first_clone in order to get proper LTO section in LTO stream. (cgraph_node::get_untransformed_body): Use lto_get_section_data where symtab_node::order must be provided. * cgraphclones.c (cgraph_node::find_replacement): Update also symbol order. * ipa-fnsummary.c (ipa_fn_summary_read): Use new function lto_get_summary_section_data. * ipa-hsa.c (ipa_hsa_read_summary): Likewise. * ipa-icf.c (sem_item_optimizer::read_summary): Likewise. * ipa-prop.c (ipa_prop_read_jump_functions): Likewise. (ipcp_read_transformation_summaries): Likewise. * ipa-sra.c (ipa_sra_read_summary): Likewise. * lto-cgraph.c (input_node): Add also order_base. (input_varpool_node): Likewise. (input_cgraph_1): Assign the order_base. (input_cgraph_opt_summary): Use new lto_get_summary_section_data. * lto-opts.c (lto_write_options): Pass new argument. * lto-section-in.c (lto_get_section_data): Add new argumente order. (lto_get_summary_section_data): New. (lto_get_raw_section_data): Add order argument. (lto_create_simple_input_block): Likewise. * lto-section-out.c (lto_destroy_simple_output_block): Likewise. * lto-streamer-in.c (lto_input_toplevel_asms): Use lto_get_summary_section_data. (lto_input_mode_table): Likewise. * lto-streamer-out.c (produce_asm): Pass symtab_node::order. (lto_output_toplevel_asms): Pass new argument. (copy_function_or_variable): Likewise. (produce_lto_section):Likewise. (produce_symtab): Likewise. (lto_write_mode_table): Likewise. (produce_asm_for_decls): Likewise. * lto-streamer.c (lto_get_section_name): Concat symbol name and symbol order. * lto-streamer.h (lto_get_section_data): Add order argument. (lto_get_summary_section_data): New. (lto_get_raw_section_data): Add order argument. (lto_get_section_name): Likewise. * varpool.c (varpool_node::get_constructor): Pass order argument. gcc/lto/ChangeLog: 2019-10-30 Martin Liska PR lto/91393 PR lto/88220 * lto-common.c (lto_file_finalize): Use lto_get_summary_section_data. (get_section_data): Add ord
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
> 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
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
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
From 04b80f0bceba93bb43c0c1cb378b431a297232d2 Mon Sep 17 00:00:00 2001
From: Martin Liska
Date: Mon, 26 Aug 2019 11:59:23 +0200
Subject: [PATCH] Use symtab_node::order in LTO sections with body.
---
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
diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index d47d4128b1c..34333b9377a 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -547,6 +547,7 @@ cgraph_node::get_create (tree decl)
{
first_clone->clone_of = node;
node->clones = first_clone;
+ node->order = first_clone->order;
symtab->symtab_prevail_in_asm_name_hash (node);
node->decl->decl_with_vis.symtab_node = node;
if (dump_file)
@@ -3546,12 +3547,17 @@ cgraph_node::get_untransformed_body (void)
struct lto_in_decl_state *decl_state
= lto_get_function_in_decl_state (file_data, decl);
+ cgraph_node *origin = this;
+ while (origin->clone_of)
+origin = origin->clone_of;
+
+ int stream_order = origin->order - file_data->order_base;
data = lto_get_section_data (file_data, LTO_section_function_body,
- name, &len, decl_state->compressed);
+ name, stream_order, &len,
+ decl_state->compressed);
if (!data)
-fatal_error (input_location, "%s: section %s is missing",
- file_data->file_name,
- name);
+fatal_error (input_location, "%s: section %s.%d is missing",
+ file_data->file_name, name, stream_order);
gcc_assert (DECL_STRUCT_FUNCTION (decl) == NULL);
diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index 087b5a26280..5b92fc1da53 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -670,6 +670,11 @@ cgraph_node::find_replacement (void)
n->clone_of = next_inline_clone;
n = n->next_sibling_clone;
}
+
+ /* Update order in order to be able to find a LTO section
+ with function body. */
+ replacement->order = order;
+
return replacement;
}
else
diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
index 0d38e42546d..4fb141ff107 100644
--- a/gcc/ipa-fnsummary.c
+++ b/gcc/ipa-fnsummary.c
@@ -3714,7 +3714,7 @@ ipa_fn_summary_read (void)
size_t len;
const char *data = lto_get_section_data (file_data,
LTO_section_ipa_fn_summary,
- NULL, &len);
+ NULL, 0, &len);
if (data)
inline_read_section (file_data, data, len);
else
diff --git a/gcc/ipa-hsa.c b/gcc/ipa-hsa.c
index 8af1d734d85..c9739fa6135 100644
--- a/gcc/ipa-hsa.c
+++ b/gcc/ipa-hsa.c
@@ -278,7 +278,7 @@ ipa_hsa_read_summary (void)
{
size_t len;
const char *data = lto_get_section_data (file_data, LTO_section_ipa_hsa,
- NULL, &len);
+ NULL, 0, &len);
if (data)
ipa_hsa_read_section (file_data, data, len);
diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index 066288d04ba..6e79047d56a 100644
--- a/gcc/ipa-icf.c
+++
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
@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
>
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
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
>From 61603b9a995d6cab7938ad2b0e2a14fde3d02308 Mon Sep 17 00:00:00 2001
From: Martin Liska
Date: Mon, 26 Aug 2019 11:59:23 +0200
Subject: [PATCH] Use symtab_node::order in LTO sections with body.
---
gcc/cgraph.c | 13 +
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/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 ---
17 files changed, 85 insertions(+), 49 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/lto/pr91393_0.c
diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 843891e9e56..9f86c2bdd10 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -3602,12 +3602,17 @@ cgraph_node::get_untransformed_body (void)
struct lto_in_decl_state *decl_state
= lto_get_function_in_decl_state (file_data, decl);
+ cgraph_node *origin = this;
+ while (origin->clone_of)
+origin = origin->clone_of;
+
+ int stream_order = origin->order - file_data->order_base;
data = lto_get_section_data (file_data, LTO_section_function_body,
- name, &len, decl_state->compressed);
+ name, stream_order, &len,
+ decl_state->compressed);
if (!data)
-fatal_error (input_location, "%s: section %s is missing",
- file_data->file_name,
- name);
+fatal_error (input_location, "%s: section %s.%d is missing",
+ file_data->file_name, name, stream_order);
gcc_assert (DECL_STRUCT_FUNCTION (decl) == NULL);
diff --git a/gcc/cgraphclones.c b/gcc/cgraphclones.c
index fa753697c78..f47c50fbb00 100644
--- a/gcc/cgraphclones.c
+++ b/gcc/cgraphclones.c
@@ -772,6 +772,11 @@ cgraph_node::find_replacement (void)
n->clone_of = next_inline_clone;
n = n->next_sibling_clone;
}
+
+ /* Update order in order to be able to find a LTO section
+ with function body. */
+ replacement->order = order;
+
return replacement;
}
else
diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
index 1bf1806eaf8..b7e0571cdfc 100644
--- a/gcc/ipa-fnsummary.c
+++ b/gcc/ipa-fnsummary.c
@@ -3463,7 +3463,7 @@ ipa_fn_summary_read (void)
size_t len;
const char *data = lto_get_section_data (file_data,
LTO_section_ipa_fn_summary,
- NULL, &len);
+ NULL, 0, &len);
if (data)
inline_read_section (file_data, data, len);
else
diff --git a/gcc/ipa-hsa.c b/gcc/ipa-hsa.c
index 8af1d734d85..c9739fa6135 100644
--- a/gcc/ipa-hsa.c
+++ b/gcc/ipa-hsa.c
@@ -278,7 +278,7 @@ ipa_hsa_read_summary (void)
{
size_t len;
const char *data = lto_get_section_data (file_data, LTO_section_ipa_hsa,
- NULL, &len);
+ NULL, 0, &len);
if (data)
ipa_hsa_read_section (file_data, data, len);
diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index c9c3cb4a331..41663821d36 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -2387,7 +2387,7 @@ sem_item_optimizer::read_summary (void)
{
size_t len;
const char *data = lto_get_section_data (file_data,
- LTO_section_ipa_icf, NULL, &len);
+ LTO_section_ipa_icf, NULL, 0, &len);
if (data)
read_section (file_data, data, len);
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index a23aa2590a0..9b36d96fd2a 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -4592,7 +4592,9 @@ ipa_prop_read_jump_functions (void)
while ((file_data = file_data_vec[j++]))
{
size_t len;
- const char *data = lto_get_section_data (file_data, LTO_section_jump_functions, NULL, &len);
+ const char *data = lto_get_s
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
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) @0x7769f708 >>> 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 >>> >>
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
> 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) @0x7769f708 > > 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 > > >
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
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) @0x7769f708 > 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 >
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
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) @0x7769f708
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
>From d4607fe7ce11bc5e35583ee545979d000dec3233 Mon Sep 17 00:00:00 2001
From: Martin Liska
Date: Mon, 26 Aug 2019 11:59:23 +0200
Subject: [PATCH] WIP v2.
---
gcc/cgraph.c | 4 +++-
gcc/ipa-fnsummary.c | 2 +-
gcc/ipa-hsa.c| 2 +-
gcc/ipa-icf.c| 2 +-
gcc/ipa-prop.c | 4 ++--
gcc/lto-cgraph.c | 13 +
gcc/lto-opts.c | 2 +-
gcc/lto-section-in.c | 11 ++-
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 | 12 ++--
gcc/testsuite/gcc.dg/lto/pr91393_0.c | 11 +++
gcc/varpool.c| 2 +-
16 files changed, 67 insertions(+), 44 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/lto/pr91393_0.c
diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index ea8ab38d806..7de4b427a87 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -3602,8 +3602,10 @@ cgraph_node::get_untransformed_body (void)
struct lto_in_decl_state *decl_state
= lto_get_function_in_decl_state (file_data, decl);
+ int stream_order = clone_of != NULL ? clone_of->order : order;
data = lto_get_section_data (file_data, LTO_section_function_body,
- name, &len, decl_state->compressed);
+ name, stream_order - file_data->order_base, &len,
+ decl_state->compressed);
if (!data)
fatal_error (input_location, "%s: section %s is missing",
file_data->file_name,
diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
index 278bf606661..424aea73780 100644
--- a/gcc/ipa-fnsummary.c
+++ b/gcc/ipa-fnsummary.c
@@ -3354,7 +3354,7 @@ ipa_fn_summary_read (void)
size_t len;
const char *data = lto_get_section_data (file_data,
LTO_section_ipa_fn_summary,
- NULL, &len);
+ NULL, 0, &len);
if (data)
inline_read_section (file_data, data, len);
else
diff --git a/gcc/ipa-hsa.c b/gcc/ipa-hsa.c
index 8af1d734d85..c9739fa6135 100644
--- a/gcc/ipa-hsa.c
+++ b/gcc/ipa-hsa.c
@@ -278,7 +278,7 @@ ipa_hsa_read_summary (void)
{
size_t len;
const char *data = lto_get_section_data (file_data, LTO_section_ipa_hsa,
- NULL, &len);
+ NULL, 0, &len);
if (data)
ipa_hsa_read_section (file_data, data, len);
diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index c9c3cb4a331..41663821d36 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -2387,7 +2387,7 @@ sem_item_optimizer::read_summary (void)
{
size_t len;
const char *data = lto_get_section_data (file_data,
- LTO_section_ipa_icf, NULL, &len);
+ LTO_section_ipa_icf, NULL, 0, &len);
if (data)
read_section (file_data, data, len);
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 1a0e12e6c0c..3a4a48df5df 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -4600,7 +4600,7 @@ ipa_prop_read_jump_functions (void)
while ((file_data = file_data_vec[j++]))
{
size_t len;
- const char *data = lto_get_section_data (file_data, LTO_section_jump_functions, NULL, &len);
+ const char *data = lto_get_section_data (file_data, LTO_section_jump_functions, NULL, 0, &len);
if (data)
ipa_prop_read_section (file_data, data, len);
@@ -4845,7 +4845,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);
if (data)
read_replacements_section (file_data, data, len);
}
diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
index bc0f0107333..aeabed08f04 100644
--- a/gcc/lto-cgraph.c
+++ b
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
> > 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 TypeAddress OffSize ES > Flg Lk Inf Al > ... > [ 6] .gnu.lto_main.e4fa533f2618f732 PROGBITS 7c > 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 > >> >
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
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 TypeAddress OffSize ES Flg Lk Inf Al ... [ 6] .gnu.lto_main.e4fa533f2618f732 PROGBITS 7c 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 >>
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
> 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 >
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
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
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
> 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
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
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
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 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: 1220const char *section; 1221order = streamer_read_hwi (ib) + order_base; 1222clone_ref = streamer_read_hwi (ib); ... 1245node->order = order; 1246if (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 >>> >>> 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 >>> >>> 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 >>> >>>
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
On Thu, Aug 15, 2019 at 4:33 PM Jan Hubicka wrote: > > > On Fri, Aug 9, 2019 at 3:57 PM Martin Liška 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 > > > > > > 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 > > > > > > 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 > > > > > >
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
> On Fri, Aug 9, 2019 at 3:57 PM Martin Liška 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 > > > > 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 > > > > 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 > > > >
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
On Mon, Aug 12, 2019 at 12:49 PM Richard Biener wrote: > > On Mon, Aug 12, 2019 at 12:43 PM Martin Liška 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
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
On Mon, Aug 12, 2019 at 12:43 PM Martin Liška 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
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
On Mon, Aug 12, 2019 at 12:39 PM Richard Biener
wrote:
>
> On Mon, Aug 12, 2019 at 11:57 AM Martin Liška wrote:
> >
> > On 8/12/19 11:45 AM, Richard Biener wrote:
> > > On Fri, Aug 9, 2019 at 3:57 PM Martin Liška 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
> > >>
> > >> 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
> > >>
> > >> 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
> > >>
> > >>
> >
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
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
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
On Mon, Aug 12, 2019 at 11:57 AM Martin Liška wrote:
>
> On 8/12/19 11:45 AM, Richard Biener wrote:
> > On Fri, Aug 9, 2019 at 3:57 PM Martin Liška 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
> >>
> >> 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
> >>
> >> 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
> >>
> >>
>
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
On 8/12/19 11:45 AM, Richard Biener wrote:
> On Fri, Aug 9, 2019 at 3:57 PM Martin Liška 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
>>
>> 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
>>
>> 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
>>
>>
Re: [PATCH] Prevent LTO section collision for a symbol name starting with '*'.
On Fri, Aug 9, 2019 at 3:57 PM Martin Liška 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 > > 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 > > 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 > >
[PATCH] Prevent LTO section collision for a symbol name starting with '*'.
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
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
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
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 000..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;
+}
