Re: [PATCH] Improve debug info for various cases where we drop location info on the floor (PR debug/55608)

[Cary Coutant <ccoutant at google dot com>]

> Now that Stage 1 reopened, is this ok for 4.9?

Looks OK to me; I just had a few comments on the code readability.

> +  if (lookup_decl_die (decl))
> +    return rtl;
> +
> +  len = TREE_STRING_LENGTH (t);
> +  vec_safe_push (used_rtx_array, rtl);
> +  ref = new_die (DW_TAG_dwarf_procedure, comp_unit_die (), decl);
> +  array = (unsigned char *) ggc_alloc_atomic (len);
> +  memcpy (array, TREE_STRING_POINTER (t), len);
> +  l = new_loc_descr (DW_OP_implicit_value, len, 0);
> +  l->dw_loc_oprnd2.val_class = dw_val_class_vec;
> +  l->dw_loc_oprnd2.v.val_vec.length = len;
> +  l->dw_loc_oprnd2.v.val_vec.elt_size = 1;
> +  l->dw_loc_oprnd2.v.val_vec.array = array;
> +  add_AT_loc (ref, DW_AT_location, l);
> +  equate_decl_number_to_die (decl, ref);
> +  return rtl;

This is just a suggestion -- rewrite the above as:

  if (!lookup_decl_die (decl))
    {
      ...
    }

  return rtl;

This makes it more clear that you're actually returning the same thing
in either case, and that the creation of the dwarf procedure is just
a side effect. When I read through this the first time, I was expecting
the fall-through path to return something different just because of the
structure.

> -  for (; loc; loc = loc->dw_loc_next)
> +  bool start_of_dw_expr = true;
> +  for (; loc; start_of_dw_expr = (loc->dw_loc_opc == DW_OP_piece
> +                                 || loc->dw_loc_opc == DW_OP_bit_piece),
> +             loc = loc->dw_loc_next)

Another suggestion to make this a little less unwieldy:

  for (dw_loc_descr_ref prev = NULL; loc; prev = loc, loc = loc->dw_loc_next)
    ...
           if ((prev == NULL
                || prev->dw_loc_opc == DW_OP_piece
                || prev->dw_loc_opc == DW_OP_bit_piece)
               && loc->dw_loc_next
               && loc->dw_loc_next->dw_loc_opc == DW_OP_stack_value
               && !dwarf_strict)
             {
               ...
             }

Can you replace the { ... } with a function call? And a comment about
what you're doing here.

> +             if (l != NULL
> +                 && l->dw_loc_next == NULL
> +                 && l->dw_loc_opc == DW_OP_addr
> +                 && GET_CODE (l->dw_loc_oprnd1.v.val_addr) == SYMBOL_REF
> +                 && SYMBOL_REF_DECL (l->dw_loc_oprnd1.v.val_addr)
> +                 && a->dw_attr == DW_AT_location)
> +               {
> +                 ...
> +               }

Likewise here.

-cary