Re: [PATCH] Maintain order of LTO sections

[Richard Guenther <rguenther at suse dot de>]

On Tue, 4 Oct 2011, Jan Hubicka wrote:

> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > Currently when reading in LTO sections from ld -r files they can
> > get randomly reordered based on hash tables and random IDs.
> > This causes reordering later when the final code is generated and
> > also makes crashes harder to reproduce.
> > 
> > This patch maintains explicit lists based on the input order and uses
> > those lists to preserve that order when starting the rest of the
> > LTO passes.
> > 
> > This is the first step to working -fno-toplevel-reorder for
> > LTO. But this needs more changes because the LTO partitioner
> > can still reorder.
> > 
> > This add two lists: one for the section and another one for
> > the file_decl_datas. This is needed because the sections are
> > walked twice through different data structures.
> > 
> > In addition some code becomes slightly cleaner because we don't need
> > to pass state through abstract callbacks anymore, but
> > can just use direct type safe calls.
> > 
> > Passes LTO bootstrap and testsuite on x86_64-linux. Ok?
> 
> Note that this is also needed to make lto-symtab decisions consistent with linker
> decisions. So it makes a lot of sense to me, but I can't approve it.
> I was running into funny cases where things got resolved differently on
> openoffice, so hopefully this will solve it.

Well, I'm not sure we should jump through too much hoops to make
-flto work with -fno-toplevel-reorder.  Simply because I think nothing
defines any "toplevel order" for multiple object files.  So, I think
both ld and gcc/collect2 need to first document they will never
break toplevel order (what about ld with -ffunction-sections and
-gc-sections? Or -relax?)

Richard.

> 
> Honza
> > 
> > gcc/lto/:
> > 
> > 2011-10-02   Andi Kleen <ak@linux.intel.com>
> > 
> > 	* lto-object.c (lto_obj_add_section_data): Add list.
> > 	(lto_obj_add_section): Fill in list.
> > 	(ltoobj_build_section_table): Pass through list.
> > 	* lto.c (file_data_list): Declare.
> > 	(create_subid_section_table): Pass arguments directly.
> > 	Fill in list of file_datas.
> > 	(lwstate): Delete.
> > 	(lto_create_files_from_ids): Pass in direct arguments.
> > 	Don't maintain list.
> > 	(lto_file_read): Use explicit section and file data lists.
> > 	(lto_read_all_file_options): Pass in section_list.
> > 	* lto.h (lto_obj_build_section_table): Add list.
> > 	(lto_section_slot): Add next.
> > 	(lto_section_list): Declare.
> > 
> > diff --git a/gcc/lto/lto-object.c b/gcc/lto/lto-object.c
> > index 3be58de..daf3bd0 100644
> > --- a/gcc/lto/lto-object.c
> > +++ b/gcc/lto/lto-object.c
> > @@ -204,6 +204,8 @@ struct lto_obj_add_section_data
> >    htab_t section_hash_table;
> >    /* The offset of this file.  */
> >    off_t base_offset;
> > +  /* List in linker order */
> > +  struct lto_section_list *list;
> >  };
> >  
> >  /* This is called for each section in the file.  */
> > @@ -218,6 +220,7 @@ lto_obj_add_section (void *data, const char *name, off_t offset,
> >    char *new_name;
> >    struct lto_section_slot s_slot;
> >    void **slot;
> > +  struct lto_section_list *list = loasd->list;
> >  
> >    if (strncmp (name, LTO_SECTION_NAME_PREFIX,
> >  	       strlen (LTO_SECTION_NAME_PREFIX)) != 0)
> > @@ -228,12 +231,21 @@ lto_obj_add_section (void *data, const char *name, off_t offset,
> >    slot = htab_find_slot (section_hash_table, &s_slot, INSERT);
> >    if (*slot == NULL)
> >      {
> > -      struct lto_section_slot *new_slot = XNEW (struct lto_section_slot);
> > +      struct lto_section_slot *new_slot = XCNEW (struct lto_section_slot);
> >  
> >        new_slot->name = new_name;
> >        new_slot->start = loasd->base_offset + offset;
> >        new_slot->len = length;
> >        *slot = new_slot;
> > +
> > +      if (list != NULL)
> > +        {
> > +          if (!list->first)
> > +            list->first = new_slot;
> > +          if (list->last)
> > +            list->last->next = new_slot;
> > +          list->last = new_slot;
> > +        }
> >      }
> >    else
> >      {
> > @@ -248,7 +260,7 @@ lto_obj_add_section (void *data, const char *name, off_t offset,
> >     the start and size of each section in the .o file.  */
> >  
> >  htab_t
> > -lto_obj_build_section_table (lto_file *lto_file)
> > +lto_obj_build_section_table (lto_file *lto_file, struct lto_section_list *list)
> >  {
> >    struct lto_simple_object *lo = (struct lto_simple_object *) lto_file;
> >    htab_t section_hash_table;
> > @@ -261,6 +273,7 @@ lto_obj_build_section_table (lto_file *lto_file)
> >    gcc_assert (lo->sobj_r != NULL && lo->sobj_w == NULL);
> >    loasd.section_hash_table = section_hash_table;
> >    loasd.base_offset = lo->base.offset;
> > +  loasd.list = list;
> >    errmsg = simple_object_find_sections (lo->sobj_r, lto_obj_add_section,
> >  					&loasd, &err);
> >    if (errmsg != NULL)
> > diff --git a/gcc/lto/lto.c b/gcc/lto/lto.c
> > index 778e33e..a77eeb4 100644
> > --- a/gcc/lto/lto.c
> > +++ b/gcc/lto/lto.c
> > @@ -1052,6 +1052,12 @@ lto_resolution_read (splay_tree file_ids, FILE *resolution, lto_file *file)
> >      }
> >  }
> >  
> > +/* List of file_decl_datas */
> > +struct file_data_list
> > +  {
> > +    struct lto_file_decl_data *first, *last;
> > +  };
> > +
> >  /* Is the name for a id'ed LTO section? */
> >  
> >  static int 
> > @@ -1068,11 +1074,10 @@ lto_section_with_id (const char *name, unsigned HOST_WIDE_INT *id)
> >  /* Create file_data of each sub file id */
> >  
> >  static int 
> > -create_subid_section_table (void **slot, void *data)
> > +create_subid_section_table (struct lto_section_slot *ls, splay_tree file_ids,
> > +                            struct file_data_list *list)
> >  {
> >    struct lto_section_slot s_slot, *new_slot;
> > -  struct lto_section_slot *ls = *(struct lto_section_slot **)slot;
> > -  splay_tree file_ids = (splay_tree)data;
> >    unsigned HOST_WIDE_INT id;
> >    splay_tree_node nd;
> >    void **hash_slot;
> > @@ -1095,6 +1100,13 @@ create_subid_section_table (void **slot, void *data)
> >        file_data->id = id;
> >        file_data->section_hash_table = lto_obj_create_section_hash_table ();;
> >        lto_splay_tree_insert (file_ids, id, file_data);
> > +
> > +      /* Maintain list in linker order */
> > +      if (!list->first)
> > +        list->first = file_data;
> > +      if (list->last)
> > +        list->last->next = file_data;
> > +      list->last = file_data;
> >      }
> >  
> >    /* Copy section into sub module hash table */
> > @@ -1129,27 +1141,17 @@ lto_file_finalize (struct lto_file_decl_data *file_data, lto_file *file)
> >    lto_free_section_data (file_data, LTO_section_decls, NULL, data, len);
> >  }
> >  
> > -struct lwstate
> > -{
> > -  lto_file *file;
> > -  struct lto_file_decl_data **file_data;
> > -  int *count;
> > -};
> > -
> > -/* Traverse ids and create a list of file_datas out of it. */      
> > +/* Finalize FILE_DATA in FILE and increase COUNT. */
> >  
> > -static int lto_create_files_from_ids (splay_tree_node node, void *data)
> > +static int 
> > +lto_create_files_from_ids (lto_file *file, struct lto_file_decl_data *file_data, 
> > +			   int *count)
> >  {
> > -  struct lwstate *lw = (struct lwstate *)data;
> > -  struct lto_file_decl_data *file_data = (struct lto_file_decl_data *)node->value;
> > -
> > -  lto_file_finalize (file_data, lw->file);
> > +  lto_file_finalize (file_data, file);
> >    if (cgraph_dump_file)
> >      fprintf (cgraph_dump_file, "Creating file %s with sub id " HOST_WIDE_INT_PRINT_HEX "\n", 
> >  	     file_data->file_name, file_data->id);
> > -  file_data->next = *lw->file_data;
> > -  *lw->file_data = file_data;
> > -  (*lw->count)++;
> > +  (*count)++;
> >    return 0;
> >  }
> >  
> > @@ -1166,29 +1168,31 @@ lto_file_read (lto_file *file, FILE *resolution_file, int *count)
> >    struct lto_file_decl_data *file_data = NULL;
> >    splay_tree file_ids;
> >    htab_t section_hash_table;
> > -  struct lwstate state;
> > -  
> > -  section_hash_table = lto_obj_build_section_table (file);
> > +  struct lto_section_slot *section;
> > +  struct file_data_list file_list;
> > +  struct lto_section_list section_list;
> > + 
> > +  memset (&section_list, 0, sizeof (struct lto_section_list)); 
> > +  section_hash_table = lto_obj_build_section_table (file, &section_list);
> >  
> >    /* Find all sub modules in the object and put their sections into new hash
> >       tables in a splay tree. */
> >    file_ids = lto_splay_tree_new ();
> > -  htab_traverse (section_hash_table, create_subid_section_table, file_ids);
> > -  
> > +  memset (&file_list, 0, sizeof (struct file_data_list));
> > +  for (section = section_list.first; section != NULL; section = section->next)
> > +    create_subid_section_table (section, file_ids, &file_list);
> > +
> >    /* Add resolutions to file ids */
> >    lto_resolution_read (file_ids, resolution_file, file);
> >  
> > -  /* Finalize each lto file for each submodule in the merged object
> > -     and create list for returning. */
> > -  state.file = file;
> > -  state.file_data = &file_data;
> > -  state.count = count;
> > -  splay_tree_foreach (file_ids, lto_create_files_from_ids, &state);
> > -    
> > +  /* Finalize each lto file for each submodule in the merged object */
> > +  for (file_data = file_list.first; file_data != NULL; file_data = file_data->next)
> > +    lto_create_files_from_ids (file, file_data, count);
> > + 
> >    splay_tree_delete (file_ids);
> >    htab_delete (section_hash_table);
> >  
> > -  return file_data;
> > +  return file_list.first;
> >  }
> >  
> >  #if HAVE_MMAP_FILE && HAVE_SYSCONF && defined _SC_PAGE_SIZE
> > @@ -2427,7 +2431,7 @@ lto_read_all_file_options (void)
> >  
> >        file_data = XCNEW (struct lto_file_decl_data);
> >        file_data->file_name = file->filename;
> > -      file_data->section_hash_table = lto_obj_build_section_table (file);
> > +      file_data->section_hash_table = lto_obj_build_section_table (file, NULL);
> >  
> >        lto_read_file_options (file_data);
> >  
> > diff --git a/gcc/lto/lto.h b/gcc/lto/lto.h
> > index 8110ace..43fcca6 100644
> > --- a/gcc/lto/lto.h
> > +++ b/gcc/lto/lto.h
> > @@ -43,7 +43,8 @@ extern void lto_read_all_file_options (void);
> >  /* In lto-elf.c or lto-coff.c  */
> >  extern lto_file *lto_obj_file_open (const char *filename, bool writable);
> >  extern void lto_obj_file_close (lto_file *file);
> > -extern htab_t lto_obj_build_section_table (lto_file *file);
> > +struct lto_section_list;
> > +extern htab_t lto_obj_build_section_table (lto_file *file, struct lto_section_list *list);
> >  extern htab_t lto_obj_create_section_hash_table (void);
> >  extern void lto_obj_begin_section (const char *name);
> >  extern void lto_obj_append_data (const void *data, size_t len, void *block);
> > @@ -58,6 +59,13 @@ struct lto_section_slot
> >    const char *name;
> >    intptr_t start;
> >    size_t len;
> > +  struct lto_section_slot *next;
> > +};
> > +
> > +/* A list of section slots */
> > +struct lto_section_list
> > +{
> > +  struct lto_section_slot *first, *last;
> >  };
> >  
> >  int64_t lto_parse_hex (const char *p);
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer