This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Fix Eh delivery in partitioned functions
On Wed, 19 Jul 2017, Jan Hubicka wrote:
> > > I think we could just output from generic code - I think it can be done by
> > > final_scan_insn. I don't know however if we have a way to tell if the section
> > > starts with a landing pad?
> >
> > Not sure either -- some insn note / bb note? Some flag on the label?
> > At least the latter should be easy to add if it's not there already.
> >
> > Richard.
>
> Hi,
> this is updated patch. I am now adding NOP_EXPR into the instruction stream.
> This is done before shorten branches so alignment tracking works there as
> expected.
> Landing pads are having PRESERVE flag set, but that is also true about named
> labels etc. So I think only safe way is to look them up from the EH tables
> which is not that hard. first_in_partition is now called on every landing
> pad in the cold section and it walks backward looking if it can be first. I added
> visited set to be sure it runs in linear time.
>
> Boostrapped/regtested x86_64-linux, OK?
It looks sensible. You leak the hash_set and I wonder if you can hook
it in pass_convert_to_eh_region_ranges instead which runs before
rest_of_handle_shorten_branches which means things can be entirely
contained in except.c?
> Honza
>
> * except.c (first_in_partition): New function.
> (maybe_add_nop_after_section_switch): New function.
> * except.h (maybe_add_nop_after_section_switch): Declare.
> * final.c (rest_of_handle_shorten_branches): Use it.
> Index: except.c
> ===================================================================
> --- except.c (revision 250312)
> +++ except.c (working copy)
> @@ -2724,6 +2724,60 @@ sjlj_size_of_call_site_table (void)
> return size;
> }
>
> +/* If L is first in the partition return NOTE_INSN_SWITCH_TEXT_SECTIONS
> + note which starts the section. */
> +rtx_insn *
> +first_in_partition (rtx_insn *l, hash_set<rtx_insn *> *visited)
Maybe rename to first_active_in_partition?
> +{
> + while (l != NULL_RTX)
> + {
> + if (visited->add (l))
> + return NULL;
given the insn stream is linear isn't it enough to add all cs->landing_pad
upfront and simply stop at them? Or mark them with a flag, clearing after
the work?
> + if (active_insn_p (l)
> + && GET_CODE (PATTERN (l)) != ASM_INPUT
> + && GET_CODE (PATTERN (l)) != ASM_OPERANDS)
> + return NULL;
a comment why we need to ignore ASMs would be nice.
> + else if (GET_CODE (l) == NOTE
> + && NOTE_KIND (l) == NOTE_INSN_SWITCH_TEXT_SECTIONS)
> + return l;
> + l = PREV_INSN (l);
> + }
> + gcc_unreachable ();
> +}
> +
> +/* Return true if NOP needs to be inserted after
> + NOTE_INSN_SWITCH_TEXT_SECTIONS. This is the case when section starts with
> + EH landing pad. Otherwise the landing pad offset will end up being 0 which
> + will be interpreted as no landing pad and exceptions will not be delivered.
> + */
> +
> +bool
> +maybe_add_nop_after_section_switch (void)
> +{
> + if (!crtl->uses_eh_lsda
> + || !crtl->eh.call_site_record_v[1])
> + return false;
> + int n = vec_safe_length (crtl->eh.call_site_record_v[1]);
> + hash_set<rtx_insn *> visited;
> +
> + for (int i = 0; i < n; ++i)
> + {
> + struct call_site_record_d *cs
> + = (*crtl->eh.call_site_record_v[1])[i];
> + if (cs->landing_pad)
> + {
> + rtx insn = first_in_partition (as_a <rtx_insn *> (cs->landing_pad),
> + &visited);
I wonder if inlining first_in_partition would make this more
understandable
> + if (insn)
> + {
> + emit_insn_after (gen_nop (), insn);
> + return true;
> + }
> + }
> + }
> + return false;
> +}
> +
> static void
> dw2_output_call_site_table (int cs_format, int section)
> {
> @@ -2749,8 +2803,10 @@ dw2_output_call_site_table (int cs_forma
> ASM_GENERATE_INTERNAL_LABEL (reg_end_lab, "LEHE", call_site_base + i);
>
> if (cs->landing_pad)
> - ASM_GENERATE_INTERNAL_LABEL (landing_pad_lab, "L",
> - CODE_LABEL_NUMBER (cs->landing_pad));
> + {
> + ASM_GENERATE_INTERNAL_LABEL (landing_pad_lab, "L",
> + CODE_LABEL_NUMBER (cs->landing_pad));
> + }
>
> /* ??? Perhaps use insn length scaling if the assembler supports
> generic arithmetic. */
> Index: except.h
> ===================================================================
> --- except.h (revision 250312)
> +++ except.h (working copy)
> @@ -283,6 +283,7 @@ extern eh_region get_eh_region_from_rtx
> extern eh_landing_pad get_eh_landing_pad_from_rtx (const_rtx);
>
> extern void finish_eh_generation (void);
> +extern bool maybe_add_nop_after_section_switch (void);
>
> struct GTY(()) throw_stmt_node {
> gimple *stmt;
> Index: final.c
> ===================================================================
> --- final.c (revision 250312)
> +++ final.c (working copy)
> @@ -4581,6 +4582,7 @@ static unsigned int
> rest_of_handle_shorten_branches (void)
> {
> /* Shorten branches. */
> + maybe_add_nop_after_section_switch ();
> shorten_branches (get_insns ());
> return 0;
> }
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)