Re: [SFN+LVU+IEPM v4 9/9] [IEPM] Introduce inline entry point markers

[Jeff Law <law at redhat dot com>]

On 12/11/2017 07:54 PM, Alexandre Oliva wrote:
> On Nov 10, 2017, Alexandre Oliva <aoliva@redhat.com> wrote:
> 
>> Output DW_AT_entry_pc based on markers.
> 
> Here's an updated version of the patch.
> 
> [IEPM] Introduce inline entry point markers
> 
> 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.
> 	(GEN_RTX_DEBUG_MARKER_INLINE_ENTRY_PAT): New.
> 	* 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/dwarf2out.c b/gcc/dwarf2out.c
> index a88b4cdf6e25..df2f6d92c6fc 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -23533,6 +23579,42 @@ add_high_low_attributes (tree stmt, dw_die_ref die)
>  {
>    char label[MAX_ARTIFICIAL_LABEL_BYTES];
>  
> +  if (inline_entry_data **iedp
> +      = !inline_entry_data_table ? NULL
> +      : inline_entry_data_table->find_slot_with_hash (stmt,
> +						      htab_hash_pointer (stmt),
> +						      NO_INSERT))
> +    {
> +      inline_entry_data *ied = *iedp;
> +      gcc_assert (MAY_HAVE_DEBUG_MARKER_INSNS);
> +      gcc_assert (inlined_function_outer_scope_p (stmt));
> +      ASM_GENERATE_INTERNAL_LABEL (label, ied->label_pfx, ied->label_num);
> +      add_AT_lbl_id (die, DW_AT_entry_pc, label);
> +
> +      if (debug_variable_location_views && !ZERO_VIEW_P (ied->view))
> +	{
> +	  if (!output_asm_line_debug_info ())
> +	    add_AT_unsigned (die, DW_AT_GNU_entry_view, ied->view);
> +	  else
> +	    {
> +	      ASM_GENERATE_INTERNAL_LABEL (label, "LVU", ied->view);
> +	      /* FIXME: this will resolve to a small number.  Could we
> +		 possibly emit smaller data?  Ideally we'd emit a
> +		 uleb128, but that would make the size of DIEs
> +		 impossible for the compiler to compute, since it's
> +		 the assembler that computes the value of the view
> +		 label in this case.  Ideally, we'd have a single form
> +		 encompassing both the address and the view, and
> +		 indirecting them through a table might make things
> +		 easier, but even that would be more wasteful,
> +		 space-wise, than what we have now.  */
> +	      add_AT_lbl_id (die, DW_AT_GNU_entry_view, label);
Do you have a sense of whether or not this really matters in practice?
how much bigger do we get due when we enable LVU?

In general I'm more concerned with getting the data we need into the
dwarf2 records than I am with optimizing its size.  I'm not saying we
should ignore the size, but



> +
> +/* Called during final while assembling the marker of the entry point
> +   for an inlined function.  */
> +
> +static void
> +dwarf2out_inline_entry (tree block)
> +{
> +  gcc_assert (DECL_P (block_ultimate_origin (block)));
> +
> +  /* Sanity check the block tree.  This would catch a case in which
> +     BLOCK got removed from the tree reachable from the outermost
> +     lexical block, but got retained in markers.  It would still link
> +     back to its parents, but some ancestor would be missing a link
> +     down the path to the sub BLOCK.  If the block got removed, its
> +     BLOCK_NUMBER will not be a usable value.  */
> +  gcc_checking_assert (block_within_block_p (block,
> +					     DECL_INITIAL
> +					     (current_function_decl),
> +					     true));
> +
> +  gcc_assert (inlined_function_outer_scope_p (block));
> +  gcc_assert (!BLOCK_DIE (block));
> +
> +  /* If we can't represent it, don't bother.  */
> +  if (!(dwarf_version >= 3 || !dwarf_strict))
> +    return;
Consider moving this check earlier.  I don't think it's a hard
requirement, so if you put it after the asserts for a reason, then leave
it has is.


Generally I think this is fine (it's much simpler than the dwarf2 bits
of the LVU patch.

Jeff