Remove OBJF_REORDERED

  OBJF_REORDERED is set for nearly every object format.  And, despite
the ominous warnings here and there, it does not seem very expensive.
This patch removes the flag entirely.
---
 gdb/buildsym.c      | 20 ++++++++------------
 gdb/coffread.c      |  5 -----
 gdb/elfread.c       |  4 ----
 gdb/machoread.c     |  4 +---
 gdb/mdebugread.c    | 29 -----------------------------
 gdb/objfile-flags.h | 22 +++++++---------------
 gdb/psymtab.c       |  3 +--
 gdb/symfile.c       |  2 +-
 gdb/symtab.c        | 17 +++++------------
 gdb/xcoffread.c     |  5 -----
 10 files changed, 23 insertions(+), 88 deletions(-)
  

On Sun, Feb 26, 2023 at 8:00 PM Tom Tromey <tom@tromey.com> wrote:

> OBJF_REORDERED is set for nearly every object format.  And, despite
> the ominous warnings here and there, it does not seem very expensive.
> This patch removes the flag entirely.
> ---
>  gdb/buildsym.c      | 20 ++++++++------------
>  gdb/coffread.c      |  5 -----
>  gdb/elfread.c       |  4 ----
>  gdb/machoread.c     |  4 +---
>  gdb/mdebugread.c    | 29 -----------------------------
>  gdb/objfile-flags.h | 22 +++++++---------------
>  gdb/psymtab.c       |  3 +--
>  gdb/symfile.c       |  2 +-
>  gdb/symtab.c        | 17 +++++------------
>  gdb/xcoffread.c     |  5 -----
>  10 files changed, 23 insertions(+), 88 deletions(-)
>
> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> index dbc81727cb1..8b6cb616d71 100644
>
>
I think it's a nice simplification and a reasonable thing to do.
  

Tom Tromey <tom@tromey.com> writes:

> OBJF_REORDERED is set for nearly every object format.  And, despite
> the ominous warnings here and there, it does not seem very expensive.
> This patch removes the flag entirely.
> ---
>  gdb/buildsym.c      | 20 ++++++++------------
>  gdb/coffread.c      |  5 -----
>  gdb/elfread.c       |  4 ----
>  gdb/machoread.c     |  4 +---
>  gdb/mdebugread.c    | 29 -----------------------------
>  gdb/objfile-flags.h | 22 +++++++---------------
>  gdb/psymtab.c       |  3 +--
>  gdb/symfile.c       |  2 +-
>  gdb/symtab.c        | 17 +++++------------
>  gdb/xcoffread.c     |  5 -----
>  10 files changed, 23 insertions(+), 88 deletions(-)
>
> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> index dbc81727cb1..8b6cb616d71 100644
> --- a/gdb/buildsym.c
> +++ b/gdb/buildsym.c
> @@ -789,10 +789,7 @@ buildsym_compunit::end_compunit_symtab_get_static_block (CORE_ADDR end_addr,
>  	}
>      }
>  
> -  /* Reordered executables may have out of order pending blocks; if
> -     OBJF_REORDERED is true, then sort the pending blocks.  */

Might be nice to keep a truncated version of this comment, otherwise you
need to dig through the body of the `if` to figure out what this is all
about.

> -
> -  if ((m_objfile->flags & OBJF_REORDERED) && m_pending_blocks)
> +  if (m_pending_blocks != nullptr)
>      {
>        struct pending_block *pb;
>  
> @@ -903,15 +900,14 @@ buildsym_compunit::end_compunit_symtab_with_blockvector
>  		return (ln1.pc < ln2.pc);
>  	      };
>  
> -	  /* Like the pending blocks, the line table may be scrambled in
> -	     reordered executables.  Sort it if OBJF_REORDERED is true.  It
> -	     is important to preserve the order of lines at the same
> -	     address, as this maintains the inline function caller/callee
> +	  /* Like the pending blocks, the line table may be scrambled
> +	     in reordered executables.  Sort it.  It is important to
> +	     preserve the order of lines at the same address, as this
> +	     maintains the inline function caller/callee
>  	     relationships, this is why std::stable_sort is used.  */
> -	  if (m_objfile->flags & OBJF_REORDERED)
> -	    std::stable_sort (subfile->line_vector_entries.begin (),
> -			      subfile->line_vector_entries.end (),
> -			      lte_is_less_than);
> +	  std::stable_sort (subfile->line_vector_entries.begin (),
> +			    subfile->line_vector_entries.end (),
> +			    lte_is_less_than);
>  	}
>  
>        /* Allocate a symbol table if necessary.  */
> diff --git a/gdb/coffread.c b/gdb/coffread.c
> index 45542bc2432..8be9a50076d 100644
> --- a/gdb/coffread.c
> +++ b/gdb/coffread.c
> @@ -485,11 +485,6 @@ coff_symfile_init (struct objfile *objfile)
>  {
>    /* Allocate struct to keep track of the symfile.  */
>    coff_objfile_data_key.emplace (objfile);
> -
> -  /* COFF objects may be reordered, so set OBJF_REORDERED.  If we
> -     find this causes a significant slowdown in gdb then we could
> -     set it in the debug symbol readers only when necessary.  */
> -  objfile->flags |= OBJF_REORDERED;
>  }
>  
>  /* This function is called for every section; it finds the outer
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index ca684aab57e..e9b0844d9d9 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -1382,10 +1382,6 @@ elf_symfile_finish (struct objfile *objfile)
>  static void
>  elf_symfile_init (struct objfile *objfile)
>  {
> -  /* ELF objects may be reordered, so set OBJF_REORDERED.  If we
> -     find this causes a significant slowdown in gdb then we could
> -     set it in the debug symbol readers only when necessary.  */
> -  objfile->flags |= OBJF_REORDERED;
>  }
>  
>  /* Implementation of `sym_get_probes', as documented in symfile.h.  */
> diff --git a/gdb/machoread.c b/gdb/machoread.c
> index 49cf4fd38fd..e8cae810788 100644
> --- a/gdb/machoread.c
> +++ b/gdb/machoread.c
> @@ -81,7 +81,6 @@ macho_new_init (struct objfile *objfile)
>  static void
>  macho_symfile_init (struct objfile *objfile)
>  {
> -  objfile->flags |= OBJF_REORDERED;
>  }
>  
>  /* Add symbol SYM to the minimal symbol table of OBJFILE.  */
> @@ -586,8 +585,7 @@ macho_add_oso_symfile (oso_el *oso, const gdb_bfd_ref_ptr &abfd,
>    symbol_file_add_from_bfd
>      (abfd, name, symfile_flags & ~(SYMFILE_MAINLINE | SYMFILE_VERBOSE),
>       NULL,
> -     main_objfile->flags & (OBJF_REORDERED | OBJF_SHARED
> -			    | OBJF_READNOW | OBJF_USERLOADED),
> +     main_objfile->flags & (OBJF_SHARED | OBJF_READNOW | OBJF_USERLOADED),
>       main_objfile);
>  }
>  
> diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
> index 793795c903a..26735176e8d 100644
> --- a/gdb/mdebugread.c
> +++ b/gdb/mdebugread.c
> @@ -3673,35 +3673,6 @@ parse_partial_symbols (minimal_symbol_reader &reader,
>  			   textlow_not_set);
>        includes_used = 0;
>        dependencies_used = 0;
> -
> -      /* The objfile has its functions reordered if this partial symbol
> -	 table overlaps any other partial symbol table.
> -	 We cannot assume a reordered objfile if a partial symbol table
> -	 is contained within another partial symbol table, as partial symbol
> -	 tables for include files with executable code are contained
> -	 within the partial symbol table for the including source file,
> -	 and we do not want to flag the objfile reordered for these cases.
> -
> -	 This strategy works well for Irix-5.2 shared libraries, but we
> -	 might have to use a more elaborate (and slower) algorithm for
> -	 other cases.  */
> -      save_pst = fdr_to_pst[f_idx].pst;
> -      if (save_pst != NULL
> -	  && save_pst->text_low_valid
> -	  && !(objfile->flags & OBJF_REORDERED))
> -	{
> -	  for (partial_symtab *iter : partial_symtabs->range ())
> -	    {
> -	      if (save_pst != iter
> -		  && save_pst->raw_text_low () >= iter->raw_text_low ()
> -		  && save_pst->raw_text_low () < iter->raw_text_high ()
> -		  && save_pst->raw_text_high () > iter->raw_text_high ())
> -		{
> -		  objfile->flags |= OBJF_REORDERED;
> -		  break;
> -		}
> -	    }
> -	}
>      }
>  
>    /* Now scan the FDRs for dependencies.  */
> diff --git a/gdb/objfile-flags.h b/gdb/objfile-flags.h
> index 7cc45684f66..9dee2ee51a0 100644
> --- a/gdb/objfile-flags.h
> +++ b/gdb/objfile-flags.h
> @@ -27,22 +27,14 @@
>  
>  enum objfile_flag : unsigned
>    {
> -    /* When an object file has its functions reordered (currently
> -       Irix-5.2 shared libraries exhibit this behaviour), we will need
> -       an expensive algorithm to locate a partial symtab or symtab via
> -       an address.  To avoid this penalty for normal object files, we
> -       use this flag, whose setting is determined upon symbol table
> -       read in.  */
> -    OBJF_REORDERED = 1 << 0,	/* Functions are reordered */
> -
>      /* Distinguish between an objfile for a shared library and a
>         "vanilla" objfile.  This may come from a target's
>         implementation of the solib interface, from add-symbol-file, or
>         any other mechanism that loads dynamic objects.  */
> -    OBJF_SHARED = 1 << 1,	/* From a shared library */
> +    OBJF_SHARED = 1 << 0,	/* From a shared library */
>  
>      /* User requested that this objfile be read in it's entirety.  */
> -    OBJF_READNOW = 1 << 2,	/* Immediate full read */
> +    OBJF_READNOW = 1 << 1,	/* Immediate full read */
>  
>      /* This objfile was created because the user explicitly caused it
>         (e.g., used the add-symbol-file command).  This bit offers a
> @@ -50,24 +42,24 @@ enum objfile_flag : unsigned
>         longer valid (i.e., are associated with an old inferior), but
>         to preserve ones that the user explicitly loaded via the
>         add-symbol-file command.  */
> -    OBJF_USERLOADED = 1 << 3,	/* User loaded */
> +    OBJF_USERLOADED = 1 << 2,	/* User loaded */
>  
>      /* Set if we have tried to read partial symtabs for this objfile.
>         This is used to allow lazy reading of partial symtabs.  */
> -    OBJF_PSYMTABS_READ = 1 << 4,
> +    OBJF_PSYMTABS_READ = 1 << 3,
>  
>      /* Set if this is the main symbol file (as opposed to symbol file
>         for dynamically loaded code).  */
> -    OBJF_MAINLINE = 1 << 5,
> +    OBJF_MAINLINE = 1 << 4,
>  
>      /* ORIGINAL_NAME and OBFD->FILENAME correspond to text description
>         unrelated to filesystem names.  It can be for example
>         "<image in memory>".  */
> -    OBJF_NOT_FILENAME = 1 << 6,
> +    OBJF_NOT_FILENAME = 1 << 5,
>  
>      /* User requested that we do not read this objfile's symbolic
>         information.  */
> -    OBJF_READNEVER = 1 << 7,
> +    OBJF_READNEVER = 1 << 6,
>    };
>  
>  DEF_ENUM_FLAGS_TYPE (enum objfile_flag, objfile_flags);
> diff --git a/gdb/psymtab.c b/gdb/psymtab.c
> index c9fd6f5f20e..e88f30a946f 100644
> --- a/gdb/psymtab.c
> +++ b/gdb/psymtab.c
> @@ -102,8 +102,7 @@ find_pc_sect_psymtab_closer (struct objfile *objfile,
>       many partial symbol tables containing the PC, but
>       we want the partial symbol table that contains the
>       function containing the PC.  */
> -  if (!(objfile->flags & OBJF_REORDERED)
> -      && section == NULL)  /* Can't validate section this way.  */
> +  if (section == nullptr)  /* Can't validate section this way.  */

Any idea what the "Can't validate section this way." comment is about?

It has been in the git repository since the import commit (c906108c2147)
in 1999.

It seems to indicate the nullptr check is the wrong thing to do, but
without more information, and given the code seems fine, I wonder if we
should just drop the comment?

Otherwise, this looks like a nice cleanup.

Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>      return pst;
>  
>    if (msymbol.minsym == NULL)
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 373f5592107..bb9981a4634 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1149,7 +1149,7 @@ symbol_file_add_separate (const gdb_bfd_ref_ptr &bfd, const char *name,
>  
>    symbol_file_add_with_addrs
>      (bfd, name, symfile_flags, &sap,
> -     objfile->flags & (OBJF_REORDERED | OBJF_SHARED | OBJF_READNOW
> +     objfile->flags & (OBJF_SHARED | OBJF_READNOW
>  		       | OBJF_USERLOADED | OBJF_MAINLINE),
>       objfile);
>  }
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index f26dd2cb187..507d7986727 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -2842,18 +2842,11 @@ find_pc_sect_compunit_symtab (CORE_ADDR pc, struct obj_section *section)
>  	  /* In order to better support objfiles that contain both
>  	     stabs and coff debugging info, we continue on if a psymtab
>  	     can't be found.  */
> -	  if ((obj_file->flags & OBJF_REORDERED) != 0)
> -	    {
> -	      struct compunit_symtab *result;
> -
> -	      result
> -		= obj_file->find_pc_sect_compunit_symtab (msymbol,
> -							  pc,
> -							  section,
> -							  0);
> -	      if (result != NULL)
> -		return result;
> -	    }
> +	  struct compunit_symtab *result
> +	    = obj_file->find_pc_sect_compunit_symtab (msymbol, pc,
> +						      section, 0);
> +	  if (result != nullptr)
> +	    return result;
>  
>  	  if (section != 0)
>  	    {
> diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c
> index bd6f6521c4c..66a954bb8b1 100644
> --- a/gdb/xcoffread.c
> +++ b/gdb/xcoffread.c
> @@ -1789,11 +1789,6 @@ xcoff_symfile_init (struct objfile *objfile)
>  {
>    /* Allocate struct to keep track of the symfile.  */
>    xcoff_objfile_data_key.emplace (objfile);
> -
> -  /* XCOFF objects may be reordered, so set OBJF_REORDERED.  If we
> -     find this causes a significant slowdown in gdb then we could
> -     set it in the debug symbol readers only when necessary.  */
> -  objfile->flags |= OBJF_REORDERED;
>  }
>  
>  /* Perform any local cleanups required when we are done with a particular
> -- 
> 2.39.1
  

On 2/26/23 13:58, Tom Tromey wrote:
> OBJF_REORDERED is set for nearly every object format.  And, despite
> the ominous warnings here and there, it does not seem very expensive.
> This patch removes the flag entirely.

That seems reasonable to me.  I don't have any numbers to back it up,
but I'm guessing that sort algorithms used in practice are pretty fast
for already sorted data.

Simon
  

>> -  /* Reordered executables may have out of order pending blocks; if
>> -     OBJF_REORDERED is true, then sort the pending blocks.  */

Andrew> Might be nice to keep a truncated version of this comment, otherwise you
Andrew> need to dig through the body of the `if` to figure out what this is all
Andrew> about.

I added:

  /* Executables may have out of order pending blocks; sort the
     pending blocks.  */

>> diff --git a/gdb/psymtab.c b/gdb/psymtab.c
>> index c9fd6f5f20e..e88f30a946f 100644
>> --- a/gdb/psymtab.c
>> +++ b/gdb/psymtab.c
>> @@ -102,8 +102,7 @@ find_pc_sect_psymtab_closer (struct objfile *objfile,
>> many partial symbol tables containing the PC, but
>> we want the partial symbol table that contains the
>> function containing the PC.  */
>> -  if (!(objfile->flags & OBJF_REORDERED)
>> -      && section == NULL)  /* Can't validate section this way.  */
>> +  if (section == nullptr)  /* Can't validate section this way.  */

Andrew> Any idea what the "Can't validate section this way." comment is about?

Nope.

Andrew> It seems to indicate the nullptr check is the wrong thing to do, but
Andrew> without more information, and given the code seems fine, I wonder if we
Andrew> should just drop the comment?

I removed it.

I'm not really sure that section can ever be null here, but who knows.
psymtab is on life support IMO.

Thanks for the review.

Tom