On Mon, 23 Oct 2023 at 18:18, Jan Hubicka <hubicka@ucw.cz> wrote:

> Hello,
> thanks for the patch.
>
> Overall it looks in right direction except for the code duplication in
> output_die and friends.
> > +/* Given a die and id, produce the appropriate abbreviations
> > +   directly to lto object file */
> > +
> > +static void
> > +output_die_abbrevs_to_object_file(unsigned long abbrev_id, dw_die_ref
> > abbrev)
> > +{
> > +  unsigned ix;
> > +  dw_attr_node *a_attr;
> > +
> > +  output_data_uleb128_to_object_file(abbrev_id);
> > +  output_data_uleb128_to_object_file(abbrev->die_tag);
> > +
> > +
> > +  if (abbrev->die_child != NULL)
> > +    output_data_to_object_file(1,DW_children_yes);
> > +  else
> > +    output_data_to_object_file(1,DW_children_no);
> > +
> > +  for (ix = 0; vec_safe_iterate (abbrev->die_attr, ix, &a_attr); ix++)
> > +    {
> > +      output_data_uleb128_to_object_file(a_attr->dw_attr);
> > +      output_value_format_to_object_file(a_attr);
> > +      if (value_format (a_attr) == DW_FORM_implicit_const)
> > + {
> > +  if (AT_class (a_attr) == dw_val_class_file_implicit)
> > +    {
> > +      int f = maybe_emit_file (a_attr->dw_attr_val.v.val_file);
> > + output_data_sleb128_to_object_file(f);
> > +    }
> > +  else
> > +      output_data_sleb128_to_object_file(a_attr->dw_attr_val.v.val_int);
> > + }
> > +    }
> > +
> > +  output_data_to_object_file (1, 0);
> > +  output_data_to_object_file (1, 0);
>
> So this basically renames dw2_asm_output_data to
> output_data_to_object_file and similarly for other output functions.
>
> What would be main problems of making dw2_asm_* functions to do the
> right thing when outputting to object file?
> Either by conditionals or turning them to virtual functions/hooks as
> Richi suggested?
>
I think it's doable via conditionals. Can you explain the second approach
in more detail?


>
> It may be performance critical how quickly we sput out the bytecode.
> In future we may templateize this, but right now it is likely premature
> optimization.
>
Cool.

> >
> > +struct lto_simple_object
> lto_simple_object is declared in lto frontend.  Why do you need to
> duplicate it here?
>
> It looks like adding relocations should be abstracted by lto API,
> so you don't need to look inside this structure that is
> lto/lto-object.cc only.
>
I should have taken this approach, but instead, I exposed simple objects to
dwarf2out.
That's the reason to duplicate the above struct. I will take care of this
while refactoring
and abstracting it by lto API


>
> > +/* Output one line number table into the .debug_line section.  */
> > +
> > +static void
> > +output_one_line_info_table (dw_line_info_table *table)
> It is hard to tell from the diff.  Did you just moved these functions
> earlier in source file?
>
Yeah. I will refactor the dwarf2out soon to clear these confusions.

-- 
Rishi


>
> Honza
>