Re: [SFN+LVU+IEPM v4 7/9] [LVU] Introduce location views

[Jakub Jelinek <jakub@redhat.com>]

On Tue, Dec 12, 2017 at 12:52:18AM -0200, Alexandre Oliva wrote:
> --- a/include/dwarf2.h
> +++ b/include/dwarf2.h
> @@ -298,6 +298,14 @@ enum dwarf_location_list_entry_type
>      DW_LLE_start_end = 0x07,
>      DW_LLE_start_length = 0x08,
>  
> +    /* <http://lists.dwarfstd.org/private.cgi/dwarf-discuss-dwarfstd.org/2017-April/004347.html>
> +       has the proposal for now; only available to list members.
> +
> +       A (possibly updated) copy of the proposal is available at
> +       <http://people.redhat.com/aoliva/papers/sfn/dwarf6-sfn-lvu.txt>.  */
> +    DW_LLE_GNU_view_pair = 0x09,
> +#define DW_LLE_view_pair DW_LLE_GNU_view_pair
> +

This looks wrong.  The proposal has not been accepted yet, so you
really can't know if DW_LLE_view_pair will be like that or whether it
will have value of 9.  Unfortunately, the DWARF standard doesn't specify a
vendor range for DW_LLE_* values.  I'd use 0xf0 or so, and don't pretend
there is DW_LLE_view_pair at all, just use DW_LLE_GNU_view_pair everywhere.
Jason, what do you think?

> --- a/gcc/dwarf2asm.c
> +++ b/gcc/dwarf2asm.c
> @@ -767,6 +767,33 @@ dw2_asm_output_data_sleb128 (HOST_WIDE_INT value,
>    va_end (ap);
>  }
>  
> +/* output symbol LAB1 as an unsigned LEB128 quantity.  */

Capital O in Output please.

> +static inline bool
> +dwarf2out_locviews_in_attribute ()
> +{
> +  return debug_variable_location_views
> +    && dwarf_version <= 5;

Formatting, should be
  return debug_variable_location_views && dwarf_version <= 5;
or
  return (debug_variable_location_views
	  && dwarf_version <= 5);
if it wouldn't fit (but it does).

> +static inline bool
> +dwarf2out_locviews_in_loclist ()
> +{
> +#ifndef DW_LLE_view_pair
> +  return false;
> +#else
> +  return debug_variable_location_views
> +    && dwarf_version >= 6;

Likewise.

> +
> +static bool
> +output_asm_line_debug_info (void)
> +{
> +  return DWARF2_ASM_VIEW_DEBUG_INFO
> +    || (DWARF2_ASM_LINE_DEBUG_INFO && !debug_variable_location_views);

Likewise.

> +  dw2_asm_output_data (1, DW_LLE_view_pair,
> +		       "DW_LLE_view_pair");

This also fits on a single line.

> +/* Output the dwarf version number.  */
> +
> +static void
> +output_dwarf_version ()
> +{
> +  /* ??? For now, if -gdwarf-6 is specified, we output version 5 with
> +     views in loclist.  That will change eventually.  */
> +  if (dwarf_version == 6)
> +    {
> +      static bool once;
> +      if (!once)
> +	{
> +	  warning (0,
> +		   "-gdwarf-6 is output as version 5 with incompatibilities");
> +	  once = true;
> +	}
> +      dw2_asm_output_data (2, 5, "DWARF version number");
> +    }

Do we really need to introduce -gdwarf-6 at this point?
-gdwarf-5 -gvariable-location-views should be sufficient, isn't it?
We don't know at all what will it look like in 3 or how many years.
My preference would be to keep all those dwarf_version == 6 related changes
out, including this output_dwarf_version function etc.

> +      const char *label = NOTE_DURING_CALL_P (loc_note)
> +	? last_postcall_label : last_label;

Again wrong formatting,
      const char *label
	= NOTE_DURING_CALL_P (loc_note) ? last_postcall_label : last_label;
is better.

> +  return !DECL_IGNORED_P (current_function_decl)
> +    && debug_variable_location_views
> +    && insn && GET_CODE (insn) == NOTE
> +    && NOTE_KIND (insn) == NOTE_INSN_VAR_LOCATION;

Formatting.

	Jakub