DWARF: turn dw_loc_descr_node field into hash map for frame offset check

Hello,

As discussed on
<https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01708.html>, this change
removes a field in the dw_loc_descr_node structure so we can get rid of
the CHECKING_P macro usage.

This field was used to perform consistency checks for frame offset in
DWARF procedures. As a replacement, this commit turns the "visited
nodes" set in resolve_args_picking_1 into a map that remembers for each
dw_loc_descr_node the frame offset associated to it, so that the
consistency check is still operational.

Boostrapped and regtested on x86_64-linux. Ok to commit? Thank you in
advance!
---
 gcc/dwarf2out.c | 37 +++++++++++++++++++------------------
 gcc/dwarf2out.h |  6 ------
 2 files changed, 19 insertions(+), 24 deletions(-)

On Wed, Apr 27, 2016 at 10:03 AM, Pierre-Marie de Rodat
<derodat@adacore.com> wrote:
> Hello,
>
> As discussed on
> <https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01708.html>, this change
> removes a field in the dw_loc_descr_node structure so we can get rid of
> the CHECKING_P macro usage.
>
> This field was used to perform consistency checks for frame offset in
> DWARF procedures. As a replacement, this commit turns the "visited
> nodes" set in resolve_args_picking_1 into a map that remembers for each
> dw_loc_descr_node the frame offset associated to it, so that the
> consistency check is still operational.
>
> Boostrapped and regtested on x86_64-linux. Ok to commit? Thank you in

Ok.

Thanks,
Richard.

> advance!
> ---
>  gcc/dwarf2out.c | 37 +++++++++++++++++++------------------
>  gcc/dwarf2out.h |  6 ------
>  2 files changed, 19 insertions(+), 24 deletions(-)
>
> diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
> index 0bbff87..463863d 100644
> --- a/gcc/dwarf2out.c
> +++ b/gcc/dwarf2out.c
> @@ -1325,9 +1325,6 @@ new_loc_descr (enum dwarf_location_atom op, unsigned HOST_WIDE_INT oprnd1,
>    dw_loc_descr_ref descr = ggc_cleared_alloc<dw_loc_descr_node> ();
>
>    descr->dw_loc_opc = op;
> -#if CHECKING_P
> -  descr->dw_loc_frame_offset = -1;
> -#endif
>    descr->dw_loc_oprnd1.val_class = dw_val_class_unsigned_const;
>    descr->dw_loc_oprnd1.val_entry = NULL;
>    descr->dw_loc_oprnd1.v.val_unsigned = oprnd1;
> @@ -15353,12 +15350,14 @@ is_handled_procedure_type (tree type)
>           && int_size_in_bytes (type) <= DWARF2_ADDR_SIZE);
>  }
>
> -/* Helper for resolve_args_picking.  Stop when coming across VISITED nodes.  */
> +/* Helper for resolve_args_picking: do the same but stop when coming across
> +   visited nodes.  For each node we visit, register in FRAME_OFFSETS the frame
> +   offset *before* evaluating the corresponding operation.  */
>
>  static bool
>  resolve_args_picking_1 (dw_loc_descr_ref loc, unsigned initial_frame_offset,
>                         struct dwarf_procedure_info *dpi,
> -                       hash_set<dw_loc_descr_ref> &visited)
> +                       hash_map<dw_loc_descr_ref, unsigned> &frame_offsets)
>  {
>    /* The "frame_offset" identifier is already used to name a macro... */
>    unsigned frame_offset_ = initial_frame_offset;
> @@ -15366,19 +15365,18 @@ resolve_args_picking_1 (dw_loc_descr_ref loc, unsigned initial_frame_offset,
>
>    for (l = loc; l != NULL;)
>      {
> +      bool existed;
> +      unsigned &l_frame_offset = frame_offsets.get_or_insert (l, &existed);
> +
>        /* If we already met this node, there is nothing to compute anymore.  */
> -      if (visited.add (l))
> +      if (existed)
>         {
> -#if CHECKING_P
>           /* Make sure that the stack size is consistent wherever the execution
>              flow comes from.  */
> -         gcc_assert ((unsigned) l->dw_loc_frame_offset == frame_offset_);
> -#endif
> +         gcc_assert ((unsigned) l_frame_offset == frame_offset_);
>           break;
>         }
> -#if CHECKING_P
> -      l->dw_loc_frame_offset = frame_offset_;
> -#endif
> +      l_frame_offset = frame_offset_;
>
>        /* If needed, relocate the picking offset with respect to the frame
>          offset. */
> @@ -15601,7 +15599,7 @@ resolve_args_picking_1 (dw_loc_descr_ref loc, unsigned initial_frame_offset,
>         {
>         case DW_OP_bra:
>           if (!resolve_args_picking_1 (l->dw_loc_next, frame_offset_, dpi,
> -                                      visited))
> +                                      frame_offsets))
>             return false;
>           /* Fall through... */
>
> @@ -15623,17 +15621,20 @@ resolve_args_picking_1 (dw_loc_descr_ref loc, unsigned initial_frame_offset,
>
>  /* Make a DFS over operations reachable through LOC (i.e. follow branch
>     operations) in order to resolve the operand of DW_OP_pick operations that
> -   target DWARF procedure arguments (DPI).  Stop at already visited nodes.
> -   INITIAL_FRAME_OFFSET is the frame offset *before* LOC is executed.  Return
> -   if all relocations were successful.  */
> +   target DWARF procedure arguments (DPI).  INITIAL_FRAME_OFFSET is the frame
> +   offset *before* LOC is executed.  Return if all relocations were
> +   successful.  */
>
>  static bool
>  resolve_args_picking (dw_loc_descr_ref loc, unsigned initial_frame_offset,
>                       struct dwarf_procedure_info *dpi)
>  {
> -  hash_set<dw_loc_descr_ref> visited;
> +  /* Associate to all visited operations the frame offset *before* evaluating
> +     this operation.  */
> +  hash_map<dw_loc_descr_ref, unsigned> frame_offsets;
>
> -  return resolve_args_picking_1 (loc, initial_frame_offset, dpi, visited);
> +  return resolve_args_picking_1 (loc, initial_frame_offset, dpi,
> +                                frame_offsets);
>  }
>
>  /* Try to generate a DWARF procedure that computes the same result as FNDECL.
> diff --git a/gcc/dwarf2out.h b/gcc/dwarf2out.h
> index 91b3d6b..abf0550 100644
> --- a/gcc/dwarf2out.h
> +++ b/gcc/dwarf2out.h
> @@ -239,12 +239,6 @@ struct GTY((chain_next ("%h.dw_loc_next"))) dw_loc_descr_node {
>       frame offset.  */
>    unsigned int frame_offset_rel : 1;
>    int dw_loc_addr;
> -#if CHECKING_P
> -  /* When translating a function into a DWARF procedure, contains the frame
> -     offset *before* evaluating this operation.  It is -1 when not yet
> -     initialized.  */
> -  int dw_loc_frame_offset;
> -#endif
>    dw_val_node dw_loc_oprnd1;
>    dw_val_node dw_loc_oprnd2;
>  };
> --
> 2.7.4
>

On 04/27/2016 01:31 PM, Richard Biener wrote:
> Ok.
>
> Thanks,
> Richard.

Thank you for the very quick feedback! I just commited the change.