Fix Eh delivery in partitioned functions

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

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.

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

> 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

Hmm, isn't the hash_set supposed to destruct itself at the end of scope?

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

Yep, that is a good idea.  I did not look into pass.def, just searched
for convenient place to do it before branch relaxation (it probably ought
to run after mdep reorg because no one knows what those will do with the
nop :)

Will re-test updated patch and commit.  Thanks a lot!

Honza

On Wed, 19 Jul 2017, Jan Hubicka wrote:

> > 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
> 
> Hmm, isn't the hash_set supposed to destruct itself at the end of scope?

Hmm, you are probably right ;)

> > 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?
> 
> Yep, that is a good idea.  I did not look into pass.def, just searched
> for convenient place to do it before branch relaxation (it probably ought
> to run after mdep reorg because no one knows what those will do with the
> nop :)
> 
> Will re-test updated patch and commit.  Thanks a lot!

Did you see the inline comments?

Thanks,
Richard.