[PATCH] LTO: get_section: add new argument

[PATCH] LTO: get_section: add new argument

[Martin Liška]

Hey.

During partial linking we ipa_prop_write_jump_functions twice from 2 IPA
pass (fnsummary and cp). That produces 2 compressed blocks in an ELF section
and then zstd complains as sections size does not correspond to the compressed
stream.

I'm adding both sanity check changes and the fix in ipa-prop.c.
I guess Martin and Honza can explain it in more detail?

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

Ready to be installed?
Thanks,
Martin

gcc/ChangeLog:

	PR lto/97508
	* langhooks.c (lhd_begin_section): Call get_section with
	not_existing = true.
	* output.h (get_section): Add new argument.
	* varasm.c (get_section): Fail when NOT_EXISTING is true
	and a section already exists.
	* ipa-prop.c (ipa_prop_write_jump_functions): Do not stream
	twice.
---
  gcc/ipa-prop.c  |  9 +++++++++
  gcc/langhooks.c |  2 +-
  gcc/output.h    |  3 ++-
  gcc/varasm.c    | 12 ++++++++++--
  4 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index a848f1db95e..d43fd2eee4f 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -5067,6 +5067,13 @@ ipa_prop_write_jump_functions (void)
    lto_symtab_encoder_iterator lsei;
    lto_symtab_encoder_t encoder;
  
+  /* The function can be called from 2 IPA_PASSES: "fnsummary" and "cp"
+     which happens in partial linking (-r).  Prevent double streaming
+     as reported in PR97508.  */
+  static bool already_stremed = false;
+  if (already_stremed)
+    return;
+
    if (!ipa_node_params_sum || !ipa_edge_args_sum)
      return;
  
@@ -5096,6 +5103,8 @@ ipa_prop_write_jump_functions (void)
    streamer_write_char_stream (ob->main_stream, 0);
    produce_asm (ob, NULL);
    destroy_output_block (ob);
+
+  already_stremed = true;
  }
  
  /* Read section in file FILE_DATA of length LEN with data DATA.  */
diff --git a/gcc/langhooks.c b/gcc/langhooks.c
index 8819a8859d4..d82f54251fd 100644
--- a/gcc/langhooks.c
+++ b/gcc/langhooks.c
@@ -790,7 +790,7 @@ lhd_begin_section (const char *name)
      saved_section = text_section;
  
    /* Create a new section and switch to it.  */
-  section = get_section (name, SECTION_DEBUG | SECTION_EXCLUDE, NULL);
+  section = get_section (name, SECTION_DEBUG | SECTION_EXCLUDE, NULL, true);
    switch_to_section (section);
  }
  
diff --git a/gcc/output.h b/gcc/output.h
index eb253c50329..2f2f1697fd8 100644
--- a/gcc/output.h
+++ b/gcc/output.h
@@ -523,7 +523,8 @@ extern GTY(()) bool in_cold_section_p;
  
  extern section *get_unnamed_section (unsigned int, void (*) (const void *),
  				     const void *);
-extern section *get_section (const char *, unsigned int, tree);
+extern section *get_section (const char *, unsigned int, tree,
+			     bool not_existing = false);
  extern section *get_named_section (tree, const char *, int);
  extern section *get_variable_section (tree, bool);
  extern void place_block_symbol (rtx);
diff --git a/gcc/varasm.c b/gcc/varasm.c
index ea0b59cf44a..207c9b077d1 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -277,10 +277,12 @@ get_noswitch_section (unsigned int flags, noswitch_section_callback callback)
  }
  
  /* Return the named section structure associated with NAME.  Create
-   a new section with the given fields if no such structure exists.  */
+   a new section with the given fields if no such structure exists.
+   When NOT_EXISTING, then fail if the section already exists.  */
  
  section *
-get_section (const char *name, unsigned int flags, tree decl)
+get_section (const char *name, unsigned int flags, tree decl,
+	     bool not_existing)
  {
    section *sect, **slot;
  
@@ -297,6 +299,12 @@ get_section (const char *name, unsigned int flags, tree decl)
      }
    else
      {
+      if (not_existing)
+	{
+	  error ("Section already exists: %qs", name);
+	  gcc_unreachable ();
+	}
+
        sect = *slot;
        /* It is fine if one of the sections has SECTION_NOTYPE as long as
           the other has none of the contrary flags (see the logic at the end
-- 
2.28.0

Re: [PATCH] LTO: get_section: add new argument

[Jan Hubicka]

> Hey.
> 
> During partial linking we ipa_prop_write_jump_functions twice from 2 IPA
> pass (fnsummary and cp). That produces 2 compressed blocks in an ELF section
> and then zstd complains as sections size does not correspond to the compressed
> stream.
> 
> I'm adding both sanity check changes and the fix in ipa-prop.c.
> I guess Martin and Honza can explain it in more detail?
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?
> Thanks,
> Martin
> 
> gcc/ChangeLog:
> 
> 	PR lto/97508
> 	* langhooks.c (lhd_begin_section): Call get_section with
> 	not_existing = true.
> 	* output.h (get_section): Add new argument.
> 	* varasm.c (get_section): Fail when NOT_EXISTING is true
> 	and a section already exists.
> 	* ipa-prop.c (ipa_prop_write_jump_functions): Do not stream
> 	twice.

I think the streaming should happen only from ipa-fnsummary.
Oriignally ipa-prop was ipa-cp only, then indirect inlining was added,
but these days we have specialized analysis pass and thus ipa-prop
should be intergrated to it.

Honza
> ---
>  gcc/ipa-prop.c  |  9 +++++++++
>  gcc/langhooks.c |  2 +-
>  gcc/output.h    |  3 ++-
>  gcc/varasm.c    | 12 ++++++++++--
>  4 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> index a848f1db95e..d43fd2eee4f 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -5067,6 +5067,13 @@ ipa_prop_write_jump_functions (void)
>    lto_symtab_encoder_iterator lsei;
>    lto_symtab_encoder_t encoder;
> +  /* The function can be called from 2 IPA_PASSES: "fnsummary" and "cp"
> +     which happens in partial linking (-r).  Prevent double streaming
> +     as reported in PR97508.  */
> +  static bool already_stremed = false;
> +  if (already_stremed)
> +    return;
> +
>    if (!ipa_node_params_sum || !ipa_edge_args_sum)
>      return;
> @@ -5096,6 +5103,8 @@ ipa_prop_write_jump_functions (void)
>    streamer_write_char_stream (ob->main_stream, 0);
>    produce_asm (ob, NULL);
>    destroy_output_block (ob);
> +
> +  already_stremed = true;
>  }
>  /* Read section in file FILE_DATA of length LEN with data DATA.  */
> diff --git a/gcc/langhooks.c b/gcc/langhooks.c
> index 8819a8859d4..d82f54251fd 100644
> --- a/gcc/langhooks.c
> +++ b/gcc/langhooks.c
> @@ -790,7 +790,7 @@ lhd_begin_section (const char *name)
>      saved_section = text_section;
>    /* Create a new section and switch to it.  */
> -  section = get_section (name, SECTION_DEBUG | SECTION_EXCLUDE, NULL);
> +  section = get_section (name, SECTION_DEBUG | SECTION_EXCLUDE, NULL, true);
>    switch_to_section (section);
>  }
> diff --git a/gcc/output.h b/gcc/output.h
> index eb253c50329..2f2f1697fd8 100644
> --- a/gcc/output.h
> +++ b/gcc/output.h
> @@ -523,7 +523,8 @@ extern GTY(()) bool in_cold_section_p;
>  extern section *get_unnamed_section (unsigned int, void (*) (const void *),
>  				     const void *);
> -extern section *get_section (const char *, unsigned int, tree);
> +extern section *get_section (const char *, unsigned int, tree,
> +			     bool not_existing = false);
>  extern section *get_named_section (tree, const char *, int);
>  extern section *get_variable_section (tree, bool);
>  extern void place_block_symbol (rtx);
> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index ea0b59cf44a..207c9b077d1 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -277,10 +277,12 @@ get_noswitch_section (unsigned int flags, noswitch_section_callback callback)
>  }
>  /* Return the named section structure associated with NAME.  Create
> -   a new section with the given fields if no such structure exists.  */
> +   a new section with the given fields if no such structure exists.
> +   When NOT_EXISTING, then fail if the section already exists.  */
>  section *
> -get_section (const char *name, unsigned int flags, tree decl)
> +get_section (const char *name, unsigned int flags, tree decl,
> +	     bool not_existing)
>  {
>    section *sect, **slot;
> @@ -297,6 +299,12 @@ get_section (const char *name, unsigned int flags, tree decl)
>      }
>    else
>      {
> +      if (not_existing)
> +	{
> +	  error ("Section already exists: %qs", name);
> +	  gcc_unreachable ();
> +	}
> +
>        sect = *slot;
>        /* It is fine if one of the sections has SECTION_NOTYPE as long as
>           the other has none of the contrary flags (see the logic at the end
> -- 
> 2.28.0
> 

Re: [PATCH] LTO: get_section: add new argument

[Martin Liška]

On 10/21/20 12:06 PM, Jan Hubicka wrote:
> I think the streaming should happen only from ipa-fnsummary.
> Oriignally ipa-prop was ipa-cp only, then indirect inlining was added,
> but these days we have specialized analysis pass and thus ipa-prop
> should be intergrated to it.

All right, there's a WIP patch but it ICEs at various places:

gcc main.o
during IPA pass: fnsummary
lto1: internal compiler error: Segmentation fault
0xc909ff crash_signal
	/home/marxin/Programming/gcc/gcc/toplev.c:330
0x7ffff788e6bf ???
	/usr/src/debug/glibc-2.32-1.1.x86_64/signal/../sysdeps/unix/sysv/linux/x86_64/sigaction.c:0
0xa7cfbd hash_table_mod1(unsigned int, unsigned int)
	/home/marxin/Programming/gcc/gcc/hash-table.h:344
0xa7cfbd hash_table<hash_map<int_hash<int, 0, -1>, ipa_node_params*, simple_hashmap_traits<default_hash_traits<int_hash<int, 0, -1> >, ipa_node_params*> >::hash_entry, false, xcallocator>::find_with_hash(int const&, unsigned int)
	/home/marxin/Programming/gcc/gcc/hash-table.h:911
0xa79216 hash_map<int_hash<int, 0, -1>, ipa_node_params*, simple_hashmap_traits<default_hash_traits<int_hash<int, 0, -1> >, ipa_node_params*> >::get(int const&)
	/home/marxin/Programming/gcc/gcc/hash-map.h:185
0xa79216 function_summary<ipa_node_params*>::get(cgraph_node*)
	/home/marxin/Programming/gcc/gcc/symbol-summary.h:163
0xa79216 inline_read_section
	/home/marxin/Programming/gcc/gcc/ipa-fnsummary.c:4314
0xa79ee0 ipa_fn_summary_read
	/home/marxin/Programming/gcc/gcc/ipa-fnsummary.c:4478
0xbc0ead ipa_read_summaries_1
	/home/marxin/Programming/gcc/gcc/passes.c:2844
0x7e31aa read_cgraph_and_symbols(unsigned int, char const**)
	/home/marxin/Programming/gcc/gcc/lto/lto-common.c:2919
0x7cb6e2 lto_main()
	/home/marxin/Programming/gcc/gcc/lto/lto.c:625

Can you please you or Martin finish the patch?
Thanks,
Martin

Re: [PATCH] LTO: get_section: add new argument

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 2052 bytes --]

On 10/21/20 1:17 PM, Martin Liška wrote:
> On 10/21/20 12:06 PM, Jan Hubicka wrote:
>> I think the streaming should happen only from ipa-fnsummary.
>> Oriignally ipa-prop was ipa-cp only, then indirect inlining was added,
>> but these days we have specialized analysis pass and thus ipa-prop
>> should be intergrated to it.
> 
> All right, there's a WIP patch but it ICEs at various places:
> 
> gcc main.o
> during IPA pass: fnsummary
> lto1: internal compiler error: Segmentation fault
> 0xc909ff crash_signal
>      /home/marxin/Programming/gcc/gcc/toplev.c:330
> 0x7ffff788e6bf ???
>      /usr/src/debug/glibc-2.32-1.1.x86_64/signal/../sysdeps/unix/sysv/linux/x86_64/sigaction.c:0
> 0xa7cfbd hash_table_mod1(unsigned int, unsigned int)
>      /home/marxin/Programming/gcc/gcc/hash-table.h:344
> 0xa7cfbd hash_table<hash_map<int_hash<int, 0, -1>, ipa_node_params*, simple_hashmap_traits<default_hash_traits<int_hash<int, 0, -1> >, ipa_node_params*> >::hash_entry, false, xcallocator>::find_with_hash(int const&, unsigned int)
>      /home/marxin/Programming/gcc/gcc/hash-table.h:911
> 0xa79216 hash_map<int_hash<int, 0, -1>, ipa_node_params*, simple_hashmap_traits<default_hash_traits<int_hash<int, 0, -1> >, ipa_node_params*> >::get(int const&)
>      /home/marxin/Programming/gcc/gcc/hash-map.h:185
> 0xa79216 function_summary<ipa_node_params*>::get(cgraph_node*)
>      /home/marxin/Programming/gcc/gcc/symbol-summary.h:163
> 0xa79216 inline_read_section
>      /home/marxin/Programming/gcc/gcc/ipa-fnsummary.c:4314
> 0xa79ee0 ipa_fn_summary_read
>      /home/marxin/Programming/gcc/gcc/ipa-fnsummary.c:4478
> 0xbc0ead ipa_read_summaries_1
>      /home/marxin/Programming/gcc/gcc/passes.c:2844
> 0x7e31aa read_cgraph_and_symbols(unsigned int, char const**)
>      /home/marxin/Programming/gcc/gcc/lto/lto-common.c:2919
> 0x7cb6e2 lto_main()
>      /home/marxin/Programming/gcc/gcc/lto/lto.c:625
> 
> Can you please you or Martin finish the patch?
> Thanks,
> Martin

... adding missing patch.

Martin

[-- Attachment #2: 0001-LTO-get_section-add-new-argument.patch --]
[-- Type: text/x-patch, Size: 3822 bytes --]

From 8c765ebad21da7f34a5038b4df8c4d29fb391055 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Wed, 21 Oct 2020 11:11:03 +0200
Subject: [PATCH] LTO: get_section: add new argument

gcc/ChangeLog:

	PR lto/97508
	* langhooks.c (lhd_begin_section): Call get_section with
	not_existing = true.
	* output.h (get_section): Add new argument.
	* varasm.c (get_section): Fail when NOT_EXISTING is true
	and a section already exists.
	* ipa-cp.c (ipcp_write_summary): Remove.
	(ipcp_read_summary): Likewise.
---
 gcc/ipa-cp.c    | 20 ++------------------
 gcc/langhooks.c |  2 +-
 gcc/output.h    |  3 ++-
 gcc/varasm.c    | 12 ++++++++++--
 4 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 2152f9e5876..db87329bc0c 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -5943,22 +5943,6 @@ ipcp_generate_summary (void)
     ipa_analyze_node (node);
 }
 
-/* Write ipcp summary for nodes in SET.  */
-
-static void
-ipcp_write_summary (void)
-{
-  ipa_prop_write_jump_functions ();
-}
-
-/* Read ipcp summary.  */
-
-static void
-ipcp_read_summary (void)
-{
-  ipa_prop_read_jump_functions ();
-}
-
 namespace {
 
 const pass_data pass_data_ipa_cp =
@@ -5980,8 +5964,8 @@ public:
   pass_ipa_cp (gcc::context *ctxt)
     : ipa_opt_pass_d (pass_data_ipa_cp, ctxt,
 		      ipcp_generate_summary, /* generate_summary */
-		      ipcp_write_summary, /* write_summary */
-		      ipcp_read_summary, /* read_summary */
+		      NULL, /* write_summary */
+		      NULL, /* read_summary */
 		      ipcp_write_transformation_summaries, /*
 		      write_optimization_summary */
 		      ipcp_read_transformation_summaries, /*
diff --git a/gcc/langhooks.c b/gcc/langhooks.c
index 8819a8859d4..d82f54251fd 100644
--- a/gcc/langhooks.c
+++ b/gcc/langhooks.c
@@ -790,7 +790,7 @@ lhd_begin_section (const char *name)
     saved_section = text_section;
 
   /* Create a new section and switch to it.  */
-  section = get_section (name, SECTION_DEBUG | SECTION_EXCLUDE, NULL);
+  section = get_section (name, SECTION_DEBUG | SECTION_EXCLUDE, NULL, true);
   switch_to_section (section);
 }
 
diff --git a/gcc/output.h b/gcc/output.h
index eb253c50329..2f2f1697fd8 100644
--- a/gcc/output.h
+++ b/gcc/output.h
@@ -523,7 +523,8 @@ extern GTY(()) bool in_cold_section_p;
 
 extern section *get_unnamed_section (unsigned int, void (*) (const void *),
 				     const void *);
-extern section *get_section (const char *, unsigned int, tree);
+extern section *get_section (const char *, unsigned int, tree,
+			     bool not_existing = false);
 extern section *get_named_section (tree, const char *, int);
 extern section *get_variable_section (tree, bool);
 extern void place_block_symbol (rtx);
diff --git a/gcc/varasm.c b/gcc/varasm.c
index ea0b59cf44a..207c9b077d1 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -277,10 +277,12 @@ get_noswitch_section (unsigned int flags, noswitch_section_callback callback)
 }
 
 /* Return the named section structure associated with NAME.  Create
-   a new section with the given fields if no such structure exists.  */
+   a new section with the given fields if no such structure exists.
+   When NOT_EXISTING, then fail if the section already exists.  */
 
 section *
-get_section (const char *name, unsigned int flags, tree decl)
+get_section (const char *name, unsigned int flags, tree decl,
+	     bool not_existing)
 {
   section *sect, **slot;
 
@@ -297,6 +299,12 @@ get_section (const char *name, unsigned int flags, tree decl)
     }
   else
     {
+      if (not_existing)
+	{
+	  error ("Section already exists: %qs", name);
+	  gcc_unreachable ();
+	}
+
       sect = *slot;
       /* It is fine if one of the sections has SECTION_NOTYPE as long as
          the other has none of the contrary flags (see the logic at the end
-- 
2.28.0

Re: [PATCH] LTO: get_section: add new argument

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 157 bytes --]

All right, there's a patch that works for me.

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

Ready to be installed?
Thanks,
Martin

[-- Attachment #2: 0001-LTO-get_section-add-new-argument.patch --]
[-- Type: text/x-patch, Size: 4863 bytes --]

From 33c58cab6bc0d779b11e7ffb36bfb485d73d6816 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Wed, 21 Oct 2020 11:11:03 +0200
Subject: [PATCH] LTO: get_section: add new argument

gcc/ChangeLog:

	PR lto/97508
	* langhooks.c (lhd_begin_section): Call get_section with
	not_existing = true.
	* output.h (get_section): Add new argument.
	* varasm.c (get_section): Fail when NOT_EXISTING is true
	and a section already exists.
	* ipa-cp.c (ipcp_write_summary): Remove.
	(ipcp_read_summary): Likewise.
	* ipa-fnsummary.c (ipa_fn_summary_read): Always read jump
	functions summary.
	(ipa_fn_summary_write): Always stream it.
---
 gcc/ipa-cp.c        | 20 ++------------------
 gcc/ipa-fnsummary.c |  6 ++----
 gcc/langhooks.c     |  2 +-
 gcc/output.h        |  3 ++-
 gcc/varasm.c        | 12 ++++++++++--
 5 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 2152f9e5876..db87329bc0c 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -5943,22 +5943,6 @@ ipcp_generate_summary (void)
     ipa_analyze_node (node);
 }
 
-/* Write ipcp summary for nodes in SET.  */
-
-static void
-ipcp_write_summary (void)
-{
-  ipa_prop_write_jump_functions ();
-}
-
-/* Read ipcp summary.  */
-
-static void
-ipcp_read_summary (void)
-{
-  ipa_prop_read_jump_functions ();
-}
-
 namespace {
 
 const pass_data pass_data_ipa_cp =
@@ -5980,8 +5964,8 @@ public:
   pass_ipa_cp (gcc::context *ctxt)
     : ipa_opt_pass_d (pass_data_ipa_cp, ctxt,
 		      ipcp_generate_summary, /* generate_summary */
-		      ipcp_write_summary, /* write_summary */
-		      ipcp_read_summary, /* read_summary */
+		      NULL, /* write_summary */
+		      NULL, /* read_summary */
 		      ipcp_write_transformation_summaries, /*
 		      write_optimization_summary */
 		      ipcp_read_transformation_summaries, /*
diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
index 9e3eda4d3cb..6479a03f2c6 100644
--- a/gcc/ipa-fnsummary.c
+++ b/gcc/ipa-fnsummary.c
@@ -4466,6 +4466,7 @@ ipa_fn_summary_read (void)
   struct lto_file_decl_data *file_data;
   unsigned int j = 0;
 
+  ipa_prop_read_jump_functions ();
   ipa_fn_summary_alloc ();
 
   while ((file_data = file_data_vec[j++]))
@@ -4484,8 +4485,6 @@ ipa_fn_summary_read (void)
 		     "ipa inline summary is missing in input file");
     }
   ipa_register_cgraph_hooks ();
-  if (!flag_ipa_cp)
-    ipa_prop_read_jump_functions ();
 
   gcc_assert (ipa_fn_summaries);
   ipa_fn_summaries->enable_insertion_hook ();
@@ -4628,8 +4627,7 @@ ipa_fn_summary_write (void)
   produce_asm (ob, NULL);
   destroy_output_block (ob);
 
-  if (!flag_ipa_cp)
-    ipa_prop_write_jump_functions ();
+  ipa_prop_write_jump_functions ();
 }
 
 
diff --git a/gcc/langhooks.c b/gcc/langhooks.c
index 8819a8859d4..d82f54251fd 100644
--- a/gcc/langhooks.c
+++ b/gcc/langhooks.c
@@ -790,7 +790,7 @@ lhd_begin_section (const char *name)
     saved_section = text_section;
 
   /* Create a new section and switch to it.  */
-  section = get_section (name, SECTION_DEBUG | SECTION_EXCLUDE, NULL);
+  section = get_section (name, SECTION_DEBUG | SECTION_EXCLUDE, NULL, true);
   switch_to_section (section);
 }
 
diff --git a/gcc/output.h b/gcc/output.h
index eb253c50329..2f2f1697fd8 100644
--- a/gcc/output.h
+++ b/gcc/output.h
@@ -523,7 +523,8 @@ extern GTY(()) bool in_cold_section_p;
 
 extern section *get_unnamed_section (unsigned int, void (*) (const void *),
 				     const void *);
-extern section *get_section (const char *, unsigned int, tree);
+extern section *get_section (const char *, unsigned int, tree,
+			     bool not_existing = false);
 extern section *get_named_section (tree, const char *, int);
 extern section *get_variable_section (tree, bool);
 extern void place_block_symbol (rtx);
diff --git a/gcc/varasm.c b/gcc/varasm.c
index ea0b59cf44a..207c9b077d1 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -277,10 +277,12 @@ get_noswitch_section (unsigned int flags, noswitch_section_callback callback)
 }
 
 /* Return the named section structure associated with NAME.  Create
-   a new section with the given fields if no such structure exists.  */
+   a new section with the given fields if no such structure exists.
+   When NOT_EXISTING, then fail if the section already exists.  */
 
 section *
-get_section (const char *name, unsigned int flags, tree decl)
+get_section (const char *name, unsigned int flags, tree decl,
+	     bool not_existing)
 {
   section *sect, **slot;
 
@@ -297,6 +299,12 @@ get_section (const char *name, unsigned int flags, tree decl)
     }
   else
     {
+      if (not_existing)
+	{
+	  error ("Section already exists: %qs", name);
+	  gcc_unreachable ();
+	}
+
       sect = *slot;
       /* It is fine if one of the sections has SECTION_NOTYPE as long as
          the other has none of the contrary flags (see the logic at the end
-- 
2.28.0

Re: [PATCH] LTO: get_section: add new argument

[Martin Jambor]

Hi,

On Thu, Oct 22 2020, Martin Liška wrote:
> All right, there's a patch that works for me.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
> Thanks,
> Martin
> From 33c58cab6bc0d779b11e7ffb36bfb485d73d6816 Mon Sep 17 00:00:00 2001
> From: Martin Liska <mliska@suse.cz>
> Date: Wed, 21 Oct 2020 11:11:03 +0200
> Subject: [PATCH] LTO: get_section: add new argument
>
> gcc/ChangeLog:
>
> 	PR lto/97508
> 	* langhooks.c (lhd_begin_section): Call get_section with
> 	not_existing = true.
> 	* output.h (get_section): Add new argument.
> 	* varasm.c (get_section): Fail when NOT_EXISTING is true
> 	and a section already exists.
> 	* ipa-cp.c (ipcp_write_summary): Remove.
> 	(ipcp_read_summary): Likewise.
> 	* ipa-fnsummary.c (ipa_fn_summary_read): Always read jump
> 	functions summary.
> 	(ipa_fn_summary_write): Always stream it.

the ipa bits are OK.  One nit...

[...]

> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index ea0b59cf44a..207c9b077d1 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -297,6 +299,12 @@ get_section (const char *name, unsigned int flags, tree decl)
>      }
>    else
>      {
> +      if (not_existing)
> +	{
> +	  error ("Section already exists: %qs", name);

...is that I think this should be internal_error.  I am not sure what
difference it makes in practice, if any, though.

Thanks,

Martin


> +	  gcc_unreachable ();
> +	}
> +
>        sect = *slot;
>        /* It is fine if one of the sections has SECTION_NOTYPE as long as
>           the other has none of the contrary flags (see the logic at the end
> -- 
> 2.28.0

Re: [PATCH] LTO: get_section: add new argument

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 232 bytes --]

On 10/22/20 1:42 PM, Martin Jambor wrote:
> ...is that I think this should be internal_error.  I am not sure what
> difference it makes in practice, if any, though.

You are right, using internal_error is a better approach.

Martin

[-- Attachment #2: 0001-LTO-get_section-add-new-argument.patch --]
[-- Type: text/x-patch, Size: 4834 bytes --]

From e6ab47df74113e7eae3160d5f9c2f7a7e724feb1 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Wed, 21 Oct 2020 11:11:03 +0200
Subject: [PATCH] LTO: get_section: add new argument

gcc/ChangeLog:

	PR lto/97508
	* langhooks.c (lhd_begin_section): Call get_section with
	not_existing = true.
	* output.h (get_section): Add new argument.
	* varasm.c (get_section): Fail when NOT_EXISTING is true
	and a section already exists.
	* ipa-cp.c (ipcp_write_summary): Remove.
	(ipcp_read_summary): Likewise.
	* ipa-fnsummary.c (ipa_fn_summary_read): Always read jump
	functions summary.
	(ipa_fn_summary_write): Always stream it.
---
 gcc/ipa-cp.c        | 20 ++------------------
 gcc/ipa-fnsummary.c |  6 ++----
 gcc/langhooks.c     |  2 +-
 gcc/output.h        |  3 ++-
 gcc/varasm.c        |  9 +++++++--
 5 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 2152f9e5876..db87329bc0c 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -5943,22 +5943,6 @@ ipcp_generate_summary (void)
     ipa_analyze_node (node);
 }
 
-/* Write ipcp summary for nodes in SET.  */
-
-static void
-ipcp_write_summary (void)
-{
-  ipa_prop_write_jump_functions ();
-}
-
-/* Read ipcp summary.  */
-
-static void
-ipcp_read_summary (void)
-{
-  ipa_prop_read_jump_functions ();
-}
-
 namespace {
 
 const pass_data pass_data_ipa_cp =
@@ -5980,8 +5964,8 @@ public:
   pass_ipa_cp (gcc::context *ctxt)
     : ipa_opt_pass_d (pass_data_ipa_cp, ctxt,
 		      ipcp_generate_summary, /* generate_summary */
-		      ipcp_write_summary, /* write_summary */
-		      ipcp_read_summary, /* read_summary */
+		      NULL, /* write_summary */
+		      NULL, /* read_summary */
 		      ipcp_write_transformation_summaries, /*
 		      write_optimization_summary */
 		      ipcp_read_transformation_summaries, /*
diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
index 9e3eda4d3cb..6479a03f2c6 100644
--- a/gcc/ipa-fnsummary.c
+++ b/gcc/ipa-fnsummary.c
@@ -4466,6 +4466,7 @@ ipa_fn_summary_read (void)
   struct lto_file_decl_data *file_data;
   unsigned int j = 0;
 
+  ipa_prop_read_jump_functions ();
   ipa_fn_summary_alloc ();
 
   while ((file_data = file_data_vec[j++]))
@@ -4484,8 +4485,6 @@ ipa_fn_summary_read (void)
 		     "ipa inline summary is missing in input file");
     }
   ipa_register_cgraph_hooks ();
-  if (!flag_ipa_cp)
-    ipa_prop_read_jump_functions ();
 
   gcc_assert (ipa_fn_summaries);
   ipa_fn_summaries->enable_insertion_hook ();
@@ -4628,8 +4627,7 @@ ipa_fn_summary_write (void)
   produce_asm (ob, NULL);
   destroy_output_block (ob);
 
-  if (!flag_ipa_cp)
-    ipa_prop_write_jump_functions ();
+  ipa_prop_write_jump_functions ();
 }
 
 
diff --git a/gcc/langhooks.c b/gcc/langhooks.c
index 8819a8859d4..d82f54251fd 100644
--- a/gcc/langhooks.c
+++ b/gcc/langhooks.c
@@ -790,7 +790,7 @@ lhd_begin_section (const char *name)
     saved_section = text_section;
 
   /* Create a new section and switch to it.  */
-  section = get_section (name, SECTION_DEBUG | SECTION_EXCLUDE, NULL);
+  section = get_section (name, SECTION_DEBUG | SECTION_EXCLUDE, NULL, true);
   switch_to_section (section);
 }
 
diff --git a/gcc/output.h b/gcc/output.h
index eb253c50329..2f2f1697fd8 100644
--- a/gcc/output.h
+++ b/gcc/output.h
@@ -523,7 +523,8 @@ extern GTY(()) bool in_cold_section_p;
 
 extern section *get_unnamed_section (unsigned int, void (*) (const void *),
 				     const void *);
-extern section *get_section (const char *, unsigned int, tree);
+extern section *get_section (const char *, unsigned int, tree,
+			     bool not_existing = false);
 extern section *get_named_section (tree, const char *, int);
 extern section *get_variable_section (tree, bool);
 extern void place_block_symbol (rtx);
diff --git a/gcc/varasm.c b/gcc/varasm.c
index ea0b59cf44a..435c7b348a5 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -277,10 +277,12 @@ get_noswitch_section (unsigned int flags, noswitch_section_callback callback)
 }
 
 /* Return the named section structure associated with NAME.  Create
-   a new section with the given fields if no such structure exists.  */
+   a new section with the given fields if no such structure exists.
+   When NOT_EXISTING, then fail if the section already exists.  */
 
 section *
-get_section (const char *name, unsigned int flags, tree decl)
+get_section (const char *name, unsigned int flags, tree decl,
+	     bool not_existing)
 {
   section *sect, **slot;
 
@@ -297,6 +299,9 @@ get_section (const char *name, unsigned int flags, tree decl)
     }
   else
     {
+      if (not_existing)
+	internal_error ("Section already exists: %qs", name);
+
       sect = *slot;
       /* It is fine if one of the sections has SECTION_NOTYPE as long as
          the other has none of the contrary flags (see the logic at the end
-- 
2.28.0

Re: [PATCH] LTO: get_section: add new argument

[Martin Liška]

On 10/22/20 2:14 PM, Martin Liška wrote:
> On 10/22/20 1:42 PM, Martin Jambor wrote:
>> ...is that I think this should be internal_error.  I am not sure what
>> difference it makes in practice, if any, though.
> 
> You are right, using internal_error is a better approach.
> 
> Martin

Is also the rest of the patch correct?

Thanks,
Martin

Re: [PATCH] LTO: get_section: add new argument

[Jan Hubicka]

> From 33c58cab6bc0d779b11e7ffb36bfb485d73d6816 Mon Sep 17 00:00:00 2001
> From: Martin Liska <mliska@suse.cz>
> Date: Wed, 21 Oct 2020 11:11:03 +0200
> Subject: [PATCH] LTO: get_section: add new argument
> 
> gcc/ChangeLog:
> 
> 	PR lto/97508
> 	* langhooks.c (lhd_begin_section): Call get_section with
> 	not_existing = true.
> 	* output.h (get_section): Add new argument.
> 	* varasm.c (get_section): Fail when NOT_EXISTING is true
> 	and a section already exists.
> 	* ipa-cp.c (ipcp_write_summary): Remove.
> 	(ipcp_read_summary): Likewise.
> 	* ipa-fnsummary.c (ipa_fn_summary_read): Always read jump
> 	functions summary.
> 	(ipa_fn_summary_write): Always stream it.

OK with ...
> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index ea0b59cf44a..207c9b077d1 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -277,10 +277,12 @@ get_noswitch_section (unsigned int flags, noswitch_section_callback callback)
>  }
>  
>  /* Return the named section structure associated with NAME.  Create
> -   a new section with the given fields if no such structure exists.  */
> +   a new section with the given fields if no such structure exists.
> +   When NOT_EXISTING, then fail if the section already exists.  */
>  
>  section *
> -get_section (const char *name, unsigned int flags, tree decl)
> +get_section (const char *name, unsigned int flags, tree decl,
> +	     bool not_existing)
>  {
>    section *sect, **slot;
>  
> @@ -297,6 +299,12 @@ get_section (const char *name, unsigned int flags, tree decl)
>      }
>    else
>      {
> +      if (not_existing)
> +	{
> +	  error ("Section already exists: %qs", name);
> +	  gcc_unreachable ();
> +	}

internal_error here?
OK, I see that you do checking in the get_section that is not lto
streaming only.  I guess in that case you also want to do same checking
in the place we produce .o file directly (during WPA->ltrans streaming).

Honza
> +
>        sect = *slot;
>        /* It is fine if one of the sections has SECTION_NOTYPE as long as
>           the other has none of the contrary flags (see the logic at the end
> -- 
> 2.28.0
> 

Re: [PATCH] LTO: get_section: add new argument

[Martin Liška]

On 10/29/20 1:51 PM, Jan Hubicka wrote:
> 
>>  From 33c58cab6bc0d779b11e7ffb36bfb485d73d6816 Mon Sep 17 00:00:00 2001
>> From: Martin Liska <mliska@suse.cz>
>> Date: Wed, 21 Oct 2020 11:11:03 +0200
>> Subject: [PATCH] LTO: get_section: add new argument
>>
>> gcc/ChangeLog:
>>
>> 	PR lto/97508
>> 	* langhooks.c (lhd_begin_section): Call get_section with
>> 	not_existing = true.
>> 	* output.h (get_section): Add new argument.
>> 	* varasm.c (get_section): Fail when NOT_EXISTING is true
>> 	and a section already exists.
>> 	* ipa-cp.c (ipcp_write_summary): Remove.
>> 	(ipcp_read_summary): Likewise.
>> 	* ipa-fnsummary.c (ipa_fn_summary_read): Always read jump
>> 	functions summary.
>> 	(ipa_fn_summary_write): Always stream it.
> 
> OK with ...
>> diff --git a/gcc/varasm.c b/gcc/varasm.c
>> index ea0b59cf44a..207c9b077d1 100644
>> --- a/gcc/varasm.c
>> +++ b/gcc/varasm.c
>> @@ -277,10 +277,12 @@ get_noswitch_section (unsigned int flags, noswitch_section_callback callback)
>>   }
>>   
>>   /* Return the named section structure associated with NAME.  Create
>> -   a new section with the given fields if no such structure exists.  */
>> +   a new section with the given fields if no such structure exists.
>> +   When NOT_EXISTING, then fail if the section already exists.  */
>>   
>>   section *
>> -get_section (const char *name, unsigned int flags, tree decl)
>> +get_section (const char *name, unsigned int flags, tree decl,
>> +	     bool not_existing)
>>   {
>>     section *sect, **slot;
>>   
>> @@ -297,6 +299,12 @@ get_section (const char *name, unsigned int flags, tree decl)
>>       }
>>     else
>>       {
>> +      if (not_existing)
>> +	{
>> +	  error ("Section already exists: %qs", name);
>> +	  gcc_unreachable ();
>> +	}
> 
> internal_error here?

Yep! Thanks for review.

> OK, I see that you do checking in the get_section that is not lto
> streaming only.  I guess in that case you also want to do same checking
> in the place we produce .o file directly (during WPA->ltrans streaming).

You are right, it's the following call stack:

#0  simple_object_write_create_section (sobj=0x305e930, name=0x305c200 ".gnu.lto_.ipa_modref.38127cbcd9999426", align=3, errmsg=0x7fffffffd928, err=0x7fffffffd924) at /home/marxin/Programming/gcc/libiberty/simple-object.c:462
#1  0x000000000096e044 in lto_obj_begin_section (name=0x305c200 ".gnu.lto_.ipa_modref.38127cbcd9999426") at /home/marxin/Programming/gcc/gcc/lto/lto-object.c:333
#2  0x0000000000dbc70e in lto_begin_section (name=0x305c200 ".gnu.lto_.ipa_modref.38127cbcd9999426", compress=false) at /home/marxin/Programming/gcc/gcc/lto-section-out.c:68
#3  0x0000000000db153d in produce_asm (ob=0x307a3d0, fn=0x0) at /home/marxin/Programming/gcc/gcc/lto-streamer-out.c:2206
#4  0x0000000000c78275 in (anonymous namespace)::modref_write () at /home/marxin/Programming/gcc/gcc/ipa-modref.c:1372
#5  0x0000000000ea4ae0 in ipa_write_optimization_summaries_1 (pass=0x30715d0, state=0x305ed40) at /home/marxin/Programming/gcc/gcc/passes.c:2785
#6  0x0000000000ea4be8 in ipa_write_optimization_summaries (encoder=0x305bce0) at /home/marxin/Programming/gcc/gcc/passes.c:2818
#7  0x000000000096996f in stream_out (temp_filename=0x305b280 "./a.ltrans0.o", encoder=0x305bce0, part=0) at /home/marxin/Programming/gcc/gcc/lto/lto.c:172
#8  0x0000000000969af0 in stream_out_partitions_1 (temp_filename=0x305b280 "./a.ltrans0.o", blen=10, min=0, max=1) at /home/marxin/Programming/gcc/gcc/lto/lto.c:218
#9  0x0000000000969b4d in stream_out_partitions (temp_filename=0x305b280 "./a.ltrans0.o", blen=10, min=0, max=1, last=true) at /home/marxin/Programming/gcc/gcc/lto/lto.c:235
#10 0x000000000096a274 in lto_wpa_write_files () at /home/marxin/Programming/gcc/gcc/lto/lto.c:394
#11 0x000000000096a75d in do_whole_program_analysis () at /home/marxin/Programming/gcc/gcc/lto/lto.c:537
#12 0x000000000096a86b in lto_main () at /home/marxin/Programming/gcc/gcc/lto/lto.c:637
#13 0x000000000102bc6c in compile_file () at /home/marxin/Programming/gcc/gcc/toplev.c:460
#14 0x000000000102eff4 in do_compile () at /home/marxin/Programming/gcc/gcc/toplev.c:2321
#15 0x000000000102f2eb in toplev::main (this=0x7fffffffdd3e, argc=17, argv=0x30496d0) at /home/marxin/Programming/gcc/gcc/toplev.c:2460
#16 0x000000000216ff50 in main (argc=17, argv=0x7fffffffde48) at /home/marxin/Programming/gcc/gcc/main.c:39

I'm going to prepare one another patch for it and I'm going to install
this patch.

Martin

> 
> Honza
>> +
>>         sect = *slot;
>>         /* It is fine if one of the sections has SECTION_NOTYPE as long as
>>            the other has none of the contrary flags (see the logic at the end
>> -- 
>> 2.28.0
>>
> 

[PATCH] LTO: get_section: add new argument

[Martin Liška]

One more backport I've just tested:

gcc/ChangeLog:

	PR lto/97508
	* langhooks.c (lhd_begin_section): Call get_section with
	not_existing = true.
	* output.h (get_section): Add new argument.
	* varasm.c (get_section): Fail when NOT_EXISTING is true
	and a section already exists.
	* ipa-cp.c (ipcp_write_summary): Remove.
	(ipcp_read_summary): Likewise.
	* ipa-fnsummary.c (ipa_fn_summary_read): Always read jump
	functions summary.
	(ipa_fn_summary_write): Always stream it.

(cherry picked from commit 568de14d2e74cfdd600b8995ff6ac08c98ddef48)
---
  gcc/ipa-cp.c        | 20 ++------------------
  gcc/ipa-fnsummary.c |  6 ++----
  gcc/langhooks.c     |  2 +-
  gcc/output.h        |  3 ++-
  gcc/varasm.c        |  9 +++++++--
  5 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index c7867dbed9b..b1f0881bd70 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -5946,22 +5946,6 @@ ipcp_generate_summary (void)
      ipa_analyze_node (node);
  }
  
-/* Write ipcp summary for nodes in SET.  */
-
-static void
-ipcp_write_summary (void)
-{
-  ipa_prop_write_jump_functions ();
-}
-
-/* Read ipcp summary.  */
-
-static void
-ipcp_read_summary (void)
-{
-  ipa_prop_read_jump_functions ();
-}
-
  namespace {
  
  const pass_data pass_data_ipa_cp =
@@ -5983,8 +5967,8 @@ public:
    pass_ipa_cp (gcc::context *ctxt)
      : ipa_opt_pass_d (pass_data_ipa_cp, ctxt,
  		      ipcp_generate_summary, /* generate_summary */
-		      ipcp_write_summary, /* write_summary */
-		      ipcp_read_summary, /* read_summary */
+		      NULL, /* write_summary */
+		      NULL, /* read_summary */
  		      ipcp_write_transformation_summaries, /*
  		      write_optimization_summary */
  		      ipcp_read_transformation_summaries, /*
diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
index 55a0b272a96..e07c9b3bba0 100644
--- a/gcc/ipa-fnsummary.c
+++ b/gcc/ipa-fnsummary.c
@@ -4346,6 +4346,7 @@ ipa_fn_summary_read (void)
    struct lto_file_decl_data *file_data;
    unsigned int j = 0;
  
+  ipa_prop_read_jump_functions ();
    ipa_fn_summary_alloc ();
  
    while ((file_data = file_data_vec[j++]))
@@ -4364,8 +4365,6 @@ ipa_fn_summary_read (void)
  		     "ipa inline summary is missing in input file");
      }
    ipa_register_cgraph_hooks ();
-  if (!flag_ipa_cp)
-    ipa_prop_read_jump_functions ();
  
    gcc_assert (ipa_fn_summaries);
    ipa_fn_summaries->enable_insertion_hook ();
@@ -4500,8 +4499,7 @@ ipa_fn_summary_write (void)
    produce_asm (ob, NULL);
    destroy_output_block (ob);
  
-  if (!flag_ipa_cp)
-    ipa_prop_write_jump_functions ();
+  ipa_prop_write_jump_functions ();
  }
  
  
diff --git a/gcc/langhooks.c b/gcc/langhooks.c
index 5e3216da631..70a554c4447 100644
--- a/gcc/langhooks.c
+++ b/gcc/langhooks.c
@@ -777,7 +777,7 @@ lhd_begin_section (const char *name)
      saved_section = text_section;
  
    /* Create a new section and switch to it.  */
-  section = get_section (name, SECTION_DEBUG | SECTION_EXCLUDE, NULL);
+  section = get_section (name, SECTION_DEBUG | SECTION_EXCLUDE, NULL, true);
    switch_to_section (section);
  }
  
diff --git a/gcc/output.h b/gcc/output.h
index eb253c50329..2f2f1697fd8 100644
--- a/gcc/output.h
+++ b/gcc/output.h
@@ -523,7 +523,8 @@ extern GTY(()) bool in_cold_section_p;
  
  extern section *get_unnamed_section (unsigned int, void (*) (const void *),
  				     const void *);
-extern section *get_section (const char *, unsigned int, tree);
+extern section *get_section (const char *, unsigned int, tree,
+			     bool not_existing = false);
  extern section *get_named_section (tree, const char *, int);
  extern section *get_variable_section (tree, bool);
  extern void place_block_symbol (rtx);
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 5bf4e96a773..0e7531926f8 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -276,10 +276,12 @@ get_noswitch_section (unsigned int flags, noswitch_section_callback callback)
  }
  
  /* Return the named section structure associated with NAME.  Create
-   a new section with the given fields if no such structure exists.  */
+   a new section with the given fields if no such structure exists.
+   When NOT_EXISTING, then fail if the section already exists.  */
  
  section *
-get_section (const char *name, unsigned int flags, tree decl)
+get_section (const char *name, unsigned int flags, tree decl,
+	     bool not_existing)
  {
    section *sect, **slot;
  
@@ -296,6 +298,9 @@ get_section (const char *name, unsigned int flags, tree decl)
      }
    else
      {
+      if (not_existing)
+	internal_error ("Section already exists: %qs", name);
+
        sect = *slot;
        /* It is fine if one of the sections has SECTION_NOTYPE as long as
           the other has none of the contrary flags (see the logic at the end
-- 
2.29.0