[PINGx2]: [VTA, PR41473] drop NULL locations from lists

[Jack Howarth <howarth at bromo dot med dot uc dot edu>]

   Can someone please review and commit this patch so
that darwin can retain full debug code in FSF gcc?
      Thanks in advance.
               Jack

On Tue, Dec 01, 2009 at 03:14:24PM -0500, Jack Howarth wrote:
> Alexandre,
>      Can we get this patch which, when coupled with...
> 
> http://gcc.gnu.org/ml/gcc-patches/2009-11/msg01452.html
> 
> , completely eliminates the crashes in dsymutil on darwin
> into gcc trunk? Thanks in advance.
>                  Jack
> 
> 
> On Tue, Nov 24, 2009 at 12:39:12AM -0200, Alexandre Oliva wrote:
> > On Nov 23, 2009, Cary Coutant <ccoutant@google.com> wrote:
> > 
> > > Jakub wrote:
> > >>> Are you sure the dropping of single_element_loc_list call in the
> > >>> loc-> first && loc->last == loc->first case is a good idea?
> > 
> > It wasn't.  I didn't realize it was needed for the block-form
> > optimization to work.  However, I think the optimization is a bit
> > excessive.  We don't want to say a variable is at a certain location
> > throughout its lifetime just because it happens to be there on a narrow
> > range of PCs.
> > 
> > > And I replied:
> > >> I don't think that would mean the same thing: a single-element loc
> > >> list describes a variable whose value is known only for that one
> > >> range, where a block form location expression describes a variable
> > >> that can always be found at the given location.
> > 
> > > Hmmm, it looks like add_AT_location_description() is already going to
> > > convert single-element loc lists into a simple block-form location
> > > expression. In the case of a single-element location list where the
> > > range is constrained, is that really the right thing to do?
> > 
> > If there aren't any other entries, we could probably assume they're
> > equivalent, i.e., that the variable is set when we enter its scope, and
> > remains live till the end.  This may not always hold, though: if the
> > variable is set later in the scope, we lose.
> > 
> > Nevertheless, I've (partially) restored this functionality in the patch
> > below.  It retains var_loc_notes with NULL locs until the time of
> > converting lists to the internal dwarf representation, and then it only
> > applies the optimization if there's really no more than a single entry
> > in the incoming location list.
> > 
> > I've also fixed an assertion check I ran into on
> > x86_64-linux-gnu/libgcc: a VOIDmode CONST_DOUBLE failed the mode ==
> > GET_MODE newly-introduced assertion, so I realized it to accept incoming
> > non-VOID modes with VOIDmode consts.
> > 
> > This one has passed bootstrap compare on x86_64-linux-gnu already,
> > building target libs now.
> > 
> 
> > for  gcc/ChangeLog
> > from  Alexandre Oliva  <aoliva@redhat.com>
> > 
> > 	PR debug/41473
> > 	* dwarf2out.c (add_var_loc_to_decl): Don't drop initial empty
> > 	locations.
> > 	(new_loc_list): Drop gensym arg.  Move generation of ll_symbol...
> > 	(gen_llsym): ... here.  New function.
> > 	(add_loc_descr_to_loc_list): Removed.
> > 	(loc_descriptor): Infer mode from CONST_DOUBLEs and CONST_VECTORs.
> > 	(single_element_loc_list): Removed.
> > 	(dw_loc_list): Don't create entries without a location.  Don't
> > 	special-case the first node of the list, only single nodes.
> > 	(single_element_loc_list_p): Simplify.
> > 	(loc_list_from_tree): Don't use DECL_RTL if loc_list is nonempty.
> > 	(add_location_or_const_value_attribute): Test var loc for NULL.
> > 	(convert_cfa_to_fb_loc_list): Adjust calls to new new_loc_list,
> > 	call gen_llsym if needed.
> > 
> > Index: gcc/dwarf2out.c
> > ===================================================================
> > --- gcc/dwarf2out.c.orig	2009-11-23 18:12:26.000000000 -0200
> > +++ gcc/dwarf2out.c	2009-11-23 23:41:40.000000000 -0200
> > @@ -6172,10 +6172,7 @@ static void gen_generic_params_dies (tre
> >  static void splice_child_die (dw_die_ref, dw_die_ref);
> >  static int file_info_cmp (const void *, const void *);
> >  static dw_loc_list_ref new_loc_list (dw_loc_descr_ref, const char *,
> > -				     const char *, const char *, unsigned);
> > -static void add_loc_descr_to_loc_list (dw_loc_list_ref *, dw_loc_descr_ref,
> > -				       const char *, const char *,
> > -				       const char *);
> > +				     const char *, const char *);
> >  static void output_loc_list (dw_loc_list_ref);
> >  static char *gen_internal_sym (const char *);
> >  
> > @@ -7788,8 +7785,7 @@ add_var_loc_to_decl (tree decl, struct v
> >  	  temp->last = loc;
> >  	}
> >      }
> > -  /* Do not add empty location to the beginning of the list.  */
> > -  else if (NOTE_VAR_LOCATION_LOC (loc->var_loc_note) != NULL_RTX)
> > +  else
> >      {
> >        temp->first = loc;
> >        temp->last = loc;
> > @@ -10297,12 +10293,11 @@ output_die_symbol (dw_die_ref die)
> >  }
> >  
> >  /* Return a new location list, given the begin and end range, and the
> > -   expression. gensym tells us whether to generate a new internal symbol for
> > -   this location list node, which is done for the head of the list only.  */
> > +   expression.  */
> >  
> >  static inline dw_loc_list_ref
> >  new_loc_list (dw_loc_descr_ref expr, const char *begin, const char *end,
> > -	      const char *section, unsigned int gensym)
> > +	      const char *section)
> >  {
> >    dw_loc_list_ref retlist = GGC_CNEW (dw_loc_list_node);
> >  
> > @@ -10310,27 +10305,18 @@ new_loc_list (dw_loc_descr_ref expr, con
> >    retlist->end = end;
> >    retlist->expr = expr;
> >    retlist->section = section;
> > -  if (gensym)
> > -    retlist->ll_symbol = gen_internal_sym ("LLST");
> >  
> >    return retlist;
> >  }
> >  
> > -/* Add a location description expression to a location list.  */
> > +/* Generate a new internal symbol for this location list node, if it
> > +   hasn't got one yet.  */
> >  
> >  static inline void
> > -add_loc_descr_to_loc_list (dw_loc_list_ref *list_head, dw_loc_descr_ref descr,
> > -			   const char *begin, const char *end,
> > -			   const char *section)
> > +gen_llsym (dw_loc_list_ref list)
> >  {
> > -  dw_loc_list_ref *d;
> > -
> > -  /* Find the end of the chain.  */
> > -  for (d = list_head; (*d) != NULL; d = &(*d)->dw_loc_next)
> > -    ;
> > -
> > -  /* Add a new location list node to the list.  */
> > -  *d = new_loc_list (descr, begin, end, section, 0);
> > +  gcc_assert (!list->ll_symbol);
> > +  list->ll_symbol = gen_internal_sym ("LLST");
> >  }
> >  
> >  /* Output the location list given to us.  */
> > @@ -13650,15 +13636,17 @@ loc_descriptor (rtx rtl, enum machine_mo
> >        break;
> >  
> >      case CONST_DOUBLE:
> > +      if (mode == VOIDmode)
> > +	mode = GET_MODE (rtl);
> > +
> >        if (mode != VOIDmode && (dwarf_version >= 4 || !dwarf_strict))
> >  	{
> > +	  gcc_assert (mode == GET_MODE (rtl) || VOIDmode == GET_MODE (rtl));
> > +
> >  	  /* Note that a CONST_DOUBLE rtx could represent either an integer
> >  	     or a floating-point constant.  A CONST_DOUBLE is used whenever
> >  	     the constant requires more than one word in order to be
> >  	     adequately represented.  We output CONST_DOUBLEs as blocks.  */
> > -	  if (GET_MODE (rtl) != VOIDmode)
> > -	    mode = GET_MODE (rtl);
> > -
> >  	  loc_result = new_loc_descr (DW_OP_implicit_value,
> >  				      GET_MODE_SIZE (mode), 0);
> >  	  if (SCALAR_FLOAT_MODE_P (mode))
> > @@ -13684,6 +13672,9 @@ loc_descriptor (rtx rtl, enum machine_mo
> >        break;
> >  
> >      case CONST_VECTOR:
> > +      if (mode == VOIDmode)
> > +	mode = GET_MODE (rtl);
> > +
> >        if (mode != VOIDmode && (dwarf_version >= 4 || !dwarf_strict))
> >  	{
> >  	  unsigned int elt_size = GET_MODE_UNIT_SIZE (GET_MODE (rtl));
> > @@ -13692,7 +13683,7 @@ loc_descriptor (rtx rtl, enum machine_mo
> >  	  unsigned int i;
> >  	  unsigned char *p;
> >  
> > -	  mode = GET_MODE (rtl);
> > +	  gcc_assert (mode == GET_MODE (rtl) || VOIDmode == GET_MODE (rtl));
> >  	  switch (GET_MODE_CLASS (mode))
> >  	    {
> >  	    case MODE_VECTOR_INT:
> > @@ -13839,14 +13830,6 @@ decl_by_reference_p (tree decl)
> >  	  && DECL_BY_REFERENCE (decl));
> >  }
> >  
> > -/* Return single element location list containing loc descr REF.  */
> > -
> > -static dw_loc_list_ref
> > -single_element_loc_list (dw_loc_descr_ref ref)
> > -{
> > -  return new_loc_list (ref, NULL, NULL, NULL, 0);
> > -}
> > -
> >  /* Helper function for dw_loc_list.  Compute proper Dwarf location descriptor
> >     for VARLOC.  */
> >  
> > @@ -13928,20 +13911,21 @@ dw_loc_list_1 (tree loc, rtx varloc, int
> >    return descr;
> >  }
> >  
> > -/* Return dwarf representation of location list representing for
> > -   LOC_LIST of DECL.  WANT_ADDRESS has the same meaning as in
> > -   loc_list_from_tree function.  */
> > +/* Return the dwarf representation of the location list LOC_LIST of
> > +   DECL.  WANT_ADDRESS has the same meaning as in loc_list_from_tree
> > +   function.  */
> >  
> >  static dw_loc_list_ref
> > -dw_loc_list (var_loc_list * loc_list, tree decl, int want_address)
> > +dw_loc_list (var_loc_list *loc_list, tree decl, int want_address)
> >  {
> >    const char *endname, *secname;
> > -  dw_loc_list_ref list;
> >    rtx varloc;
> >    enum var_init_status initialized;
> >    struct var_loc_node *node;
> >    dw_loc_descr_ref descr;
> >    char label_id[MAX_ARTIFICIAL_LABEL_BYTES];
> > +  dw_loc_list_ref list = NULL;
> > +  dw_loc_list_ref *listp = &list;
> >  
> >    /* Now that we know what section we are using for a base,
> >       actually construct the list of locations.
> > @@ -13954,26 +13938,9 @@ dw_loc_list (var_loc_list * loc_list, tr
> >       This means we have to special case the last node, and generate
> >       a range of [last location start, end of function label].  */
> >  
> > -  node = loc_list->first;
> >    secname = secname_for_decl (decl);
> >  
> > -  if (NOTE_VAR_LOCATION_LOC (node->var_loc_note))
> > -    initialized = NOTE_VAR_LOCATION_STATUS (node->var_loc_note);
> > -  else
> > -    initialized = VAR_INIT_STATUS_INITIALIZED;
> > -  varloc = NOTE_VAR_LOCATION (node->var_loc_note);
> > -  descr = dw_loc_list_1 (decl, varloc, want_address, initialized);
> > -
> > -  if (loc_list && loc_list->first != loc_list->last)
> > -    list = new_loc_list (descr, node->label, node->next->label, secname, 1);
> > -  else
> > -    return single_element_loc_list (descr);
> > -  node = node->next;
> > -
> > -  if (!node)
> > -    return NULL;
> > -
> > -  for (; node->next; node = node->next)
> > +  for (node = loc_list->first; node->next; node = node->next)
> >      if (NOTE_VAR_LOCATION_LOC (node->var_loc_note) != NULL_RTX)
> >        {
> >  	/* The variable has a location between NODE->LABEL and
> > @@ -13981,28 +13948,46 @@ dw_loc_list (var_loc_list * loc_list, tr
> >  	initialized = NOTE_VAR_LOCATION_STATUS (node->var_loc_note);
> >  	varloc = NOTE_VAR_LOCATION (node->var_loc_note);
> >  	descr = dw_loc_list_1 (decl, varloc, want_address, initialized);
> > -	add_loc_descr_to_loc_list (&list, descr,
> > -				   node->label, node->next->label, secname);
> > +	if (descr)
> > +	  {
> > +	    *listp = new_loc_list (descr, node->label, node->next->label,
> > +				   secname);
> > +	    listp = &(*listp)->dw_loc_next;
> > +	  }
> >        }
> >  
> >    /* If the variable has a location at the last label
> >       it keeps its location until the end of function.  */
> >    if (NOTE_VAR_LOCATION_LOC (node->var_loc_note) != NULL_RTX)
> >      {
> > -      if (!current_function_decl)
> > -	endname = text_end_label;
> > -      else
> > -	{
> > -	  ASM_GENERATE_INTERNAL_LABEL (label_id, FUNC_END_LABEL,
> > -				       current_function_funcdef_no);
> > -	  endname = ggc_strdup (label_id);
> > -	}
> > -
> >        initialized = NOTE_VAR_LOCATION_STATUS (node->var_loc_note);
> >        varloc = NOTE_VAR_LOCATION (node->var_loc_note);
> >        descr = dw_loc_list_1 (decl, varloc, want_address, initialized);
> > -      add_loc_descr_to_loc_list (&list, descr, node->label, endname, secname);
> > +      if (descr)
> > +	{
> > +	  if (!current_function_decl)
> > +	    endname = text_end_label;
> > +	  else
> > +	    {
> > +	      ASM_GENERATE_INTERNAL_LABEL (label_id, FUNC_END_LABEL,
> > +					   current_function_funcdef_no);
> > +	      endname = ggc_strdup (label_id);
> > +	    }
> > +
> > +	  *listp = new_loc_list (descr, node->label, endname, secname);
> > +	  listp = &(*listp)->dw_loc_next;
> > +	}
> >      }
> > +
> > +  /* Try to avoid the overhead of a location list emitting a location
> > +     expression instead, but only if we didn't have more than one
> > +     location entry in the first place.  If some entries were not
> > +     representable, we don't want to pretend a single entry that was
> > +     applies to the entire scope in which the variable is
> > +     available.  */
> > +  if (list && loc_list->first->next)
> > +    gen_llsym (list);
> > +
> >    return list;
> >  }
> >  
> > @@ -14012,7 +13997,8 @@ dw_loc_list (var_loc_list * loc_list, tr
> >  static bool
> >  single_element_loc_list_p (dw_loc_list_ref list)
> >  {
> > -  return (!list->dw_loc_next && !list->begin && !list->end);
> > +  gcc_assert (!list->dw_loc_next || list->ll_symbol);
> > +  return !list->ll_symbol;
> >  }
> >  
> >  /* To each location in list LIST add loc descr REF.  */
> > @@ -14312,9 +14298,9 @@ loc_list_from_tree (tree loc, int want_a
> >  	rtx rtl;
> >  	var_loc_list *loc_list = lookup_decl_loc (loc);
> >  
> > -	if (loc_list && loc_list->first
> > -	    && (list_ret = dw_loc_list (loc_list, loc, want_address)))
> > +	if (loc_list && loc_list->first)
> >  	  {
> > +	    list_ret = dw_loc_list (loc_list, loc, want_address);
> >  	    have_address = want_address != 0;
> >  	    break;
> >  	  }
> > @@ -14725,7 +14711,7 @@ loc_list_from_tree (tree loc, int want_a
> >  	add_loc_descr_to_each (list_ret, new_loc_descr (op, size, 0));
> >      }
> >    if (ret)
> > -    list_ret = single_element_loc_list (ret);
> > +    list_ret = new_loc_list (ret, NULL, NULL, NULL);
> >  
> >    return list_ret;
> >  }
> > @@ -15719,17 +15705,19 @@ add_location_or_const_value_attribute (d
> >       a constant value.  That way we are better to use add_const_value_attribute
> >       rather than expanding constant value equivalent.  */
> >    loc_list = lookup_decl_loc (decl);
> > -  if (loc_list && loc_list->first && loc_list->first == loc_list->last)
> > +  if (loc_list && loc_list->first && loc_list->first == loc_list->last
> > +      && loc_list->first->var_loc_note
> > +      && NOTE_VAR_LOCATION (loc_list->first->var_loc_note)
> > +      && NOTE_VAR_LOCATION_LOC (loc_list->first->var_loc_note))
> >      {
> >        enum var_init_status status;
> >        struct var_loc_node *node;
> >  
> >        node = loc_list->first;
> >        status = NOTE_VAR_LOCATION_STATUS (node->var_loc_note);
> > -      rtl = NOTE_VAR_LOCATION (node->var_loc_note);
> > -      if (GET_CODE (rtl) == VAR_LOCATION
> > -	  && GET_CODE (XEXP (rtl, 1)) != PARALLEL)
> > -	rtl = XEXP (XEXP (rtl, 1), 0);
> > +      rtl = NOTE_VAR_LOCATION_LOC (node->var_loc_note);
> > +      if (GET_CODE (rtl) != PARALLEL)
> > +	rtl = XEXP (rtl, 0);
> >        if ((CONSTANT_P (rtl) || GET_CODE (rtl) == CONST_STRING)
> >  	  && add_const_value_attribute (die, rtl))
> >  	 return true;
> > @@ -16016,8 +16004,7 @@ convert_cfa_to_fb_loc_list (HOST_WIDE_IN
> >  	if (!cfa_equal_p (&last_cfa, &next_cfa))
> >  	  {
> >  	    *list_tail = new_loc_list (build_cfa_loc (&last_cfa, offset),
> > -				       start_label, last_label, section,
> > -				       list == NULL);
> > +				       start_label, last_label, section);
> >  
> >  	    list_tail = &(*list_tail)->dw_loc_next;
> >  	    last_cfa = next_cfa;
> > @@ -16038,14 +16025,16 @@ convert_cfa_to_fb_loc_list (HOST_WIDE_IN
> >    if (!cfa_equal_p (&last_cfa, &next_cfa))
> >      {
> >        *list_tail = new_loc_list (build_cfa_loc (&last_cfa, offset),
> > -				 start_label, last_label, section,
> > -				 list == NULL);
> > +				 start_label, last_label, section);
> >        list_tail = &(*list_tail)->dw_loc_next;
> >        start_label = last_label;
> >      }
> > +
> >    *list_tail = new_loc_list (build_cfa_loc (&next_cfa, offset),
> > -			     start_label, fde->dw_fde_end, section,
> > -			     list == NULL);
> > +			     start_label, fde->dw_fde_end, section);
> > +
> > +  if (list && list->dw_loc_next)
> > +    gen_llsym (list);
> >  
> >    return list;
> >  }
> 
> > 
> > -- 
> > Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> > You must be the change you wish to see in the world. -- Gandhi
> > Be Free! -- http://FSFLA.org/   FSF Latin America board member
> > Free Software Evangelist      Red Hat Brazil Compiler Engineer