Re: [PATCH 9/9] [IEPM] Introduce inline entry point markers

[Jeff Law <law at redhat dot com>]

On 09/30/2017 03:08 AM, Alexandre Oliva wrote:
> Output DW_AT_entry_pc based on markers.
> 
> Introduce DW_AT_GNU_entry_view as a DWARF extension.
> 
> If views are enabled are we're not in strict compliance mode, output
> DW_AT_GNU_entry_view if it might be nonzero.
> 
> This patch depends on SFN and LVU patchsets, and on the IEPM patch that
> introduces the inline_entry debug hook.
> 
> for  include/ChangeLog
> 
> 	* dwarf2.def (DW_AT_GNU_entry_view): New.
> 
> for  gcc/ChangeLog
> 
> 	* cfgexpand.c (expand_gimple_basic_block): Handle inline entry
> 	markers.
> 	* dwarf2out.c (dwarf2_debug_hooks): Enable inline_entry hook.
> 	(BLOCK_INLINE_ENTRY_LABEL): New.
> 	(dwarf2out_var_location): Disregard inline entry markers.
> 	(inline_entry_data): New struct.
> 	(inline_entry_data_hasher): New hashtable type.
> 	(inline_entry_data_hasher::hash): New.
> 	(inline_entry_data_hasher::equal): New.
> 	(inline_entry_data_table): New variable.
> 	(add_high_low_attributes): Add DW_AT_entry_pc and
> 	DW_AT_GNU_entry_view attributes if a pending entry is found
> 	in inline_entry_data_table.  Add old entry_pc attribute only
> 	if debug nonbinding markers are disabled.
> 	(gen_inlined_subroutine_die): Set BLOCK_DIE if nonbinding
> 	markers are enabled.
> 	(block_within_block_p, dwarf2out_inline_entry): New.
> 	(dwarf2out_finish): Check that no entries remained in
> 	inline_entry_data_table.
> 	* final.c (reemit_insn_block_notes): Handle inline entry notes.
> 	(final_scan_insn, notice_source_line): Likewise.
> 	(rest_of_clean_state): Skip inline entry markers.
> 	* gimple-pretty-print.c (dump_gimple_debug): Handle inline entry
> 	markers.
> 	* gimple.c (gimple_build_debug_inline_entry): New.
> 	* gimple.h (enum gimple_debug_subcode): Add
> 	GIMPLE_DEBUG_INLINE_ENTRY.
> 	(gimple_build_debug_inline_entry): Declare.
> 	(gimple_debug_inline_entry_p): New.
> 	(gimple_debug_nonbind_marker_p): Adjust.
> 	* insn-notes.def (INLINE_ENTRY): New.
> 	* print-rtl.c (rtx_writer::print_rtx_operand_code_0): Handle
> 	inline entry marker notes.
> 	(print_insn): Likewise.
> 	* rtl.h	(NOTE_MARKER_P): Add INLINE_ENTRY support.
> 	(INSN_DEBUG_MARKER_KIND): Likewise.
> 	* tree-inline.c	(expand_call_inline): Build and insert
> 	debug_inline_entry stmt.
> 	* tree-ssa-live.c (remove_unused_scope_block_p): Preserve
> 	inline entry blocks early, if nonbind markers are enabled.
> 	(dump_scope_block): Dump fragment info.
> 	* var-tracking.c (reemit_marker_as_note): Handle inline entry note.
> 	* doc/gimple.texi (gimple_debug_inline_entry_p): New.
> 	(gimple_build_debug_inline_entry): New.
> 	* doc/invoke.texi (gstatement-frontiers, gno-statement-frontiers):
> 	Enable/disable inline entry points too.
> 	* doc/rtl.texi (NOTE_INSN_INLINE_ENTRY): New.
> 	(DEBUG_INSN): Describe inline entry markers.
> ---
> 
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index c854ffd..8ccdafd 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -5691,6 +5691,15 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls)
>  		goto delink_debug_stmt;
>  	      else if (gimple_debug_begin_stmt_p (stmt))
>  		val = gen_rtx_DEBUG_MARKER (VOIDmode);
> +	      else if (gimple_debug_inline_entry_p (stmt))
> +		{
> +		  tree block = gimple_block (stmt);
> +
> +		  if (block)
> +		    val = gen_rtx_DEBUG_MARKER (BLKmode);
> +		  else
> +		    goto delink_debug_stmt;
> +		}
>  	      else
>  		gcc_unreachable ();
So again, I haven't looked at prior patches.  It seems to me like we're
abusing the mode of the debug marker rtx here.  Is that really the best
way to do things?  Would it be better to have different RTXs for the
marker or real arguments?  Or what about keying on the existence of
INSN_LOCATION?!?  Something just feels a bit icky here.



> +/* Check whether BLOCK, a lexical block, is nested within OUTER, or is
> +   OUTER itself.  */
> +static bool
> +block_within_block_p (tree block, tree outer)
> +{
> +  if (block == outer)
> +    return true;
> +
> +  /* Quickly check that OUTER is up BLOCK's supercontext chain.  */
> +  for (tree context = BLOCK_SUPERCONTEXT (block);
> +       context != outer;
> +       context = BLOCK_SUPERCONTEXT (context))
> +    if (!context || TREE_CODE (context) != BLOCK)
> +      return false;
> +
> +  /* Now check that each block is actually referenced by its
> +     parent.  */
So this seemed reasonable up to here.  Based on the name and function
comment, I'd think at this point you'd be able to return true.  We found
OUTER in BLOCK's supercontext chain.

But you've got another blob of code.  So it seems like you're check more
than what the name/function comment imply.

Otherwise this patch seems pretty reasonable.

jeff