libdw: dwarf_getsrcfiles should not imply dwarf_getsrclines

  dwarf_getsrcfiles causes line data to be read in addition to file data.
This is wasteful for programs which only need file or directory names.
Debuginfod server is one such example.

Fix this by moving the srcfile handling in read_srclines into a separate
function read_srcfiles.

read_srclines also no longer handles DW_LNE_define_file due to lack of
use and to simplify the separation of srcfile and srcline reading.

	* libdw/dwarf_getsrcfiles.c (dwarf_getsrcfiles): Replace
	dwarf_getsrclines and __libdw_getsrclines with
	__libdw_getsrcfiles.
	* libdw/dwarf_getsrclines.c (read_line_header): New function.
	(read_srcfiles): New function.
	(read_srclines): Move file reading into read_srcfiles.
	Remove DW_LNE_define_file handling.  Add parameter so
	that previously read srcfiles can be used if available.
	(__libdw_getsrclines): Call read_srcfiles if linesp is
	NULL.  Pass previously read srcfiles to read_srclines
	if available.
	(__libdw_getsrcfiles): New function.
	* libdw/dwarf_macro_getsrcfiles.c (dwarf_macro_getsrcfiles):
	Replace __libdw_getsrclines with __libdw_getsrcfiles.
	* libdw/libdwP.h (__libdw_getsrcfiles): New declaration.
	* tests/get-files.c: Verify that dwarf_getsrcfiles does
	not cause srclines to be read.
	* tests/get-lines.c: Verify that srclines can be read
	after srcfiles have been read.

https://sourceware.org/bugzilla/show_bug.cgi?id=27405

Signed-off-by: Aaron Merey <amerey@redhat.com>
---
 libdw/dwarf_getsrcfiles.c       |  24 +-
 libdw/dwarf_getsrclines.c       | 506 ++++++++++++++++++--------------
 libdw/dwarf_macro_getsrcfiles.c |   4 +-
 libdw/libdwP.h                  |  10 +
 tests/get-files.c               |   8 +
 tests/get-lines.c               |  20 +-
 6 files changed, 346 insertions(+), 226 deletions(-)
  

Hi Aaron,

On Thu, 2024-03-28 at 21:12 -0400, Aaron Merey wrote:
> dwarf_getsrcfiles causes line data to be read in addition to file data.
> This is wasteful for programs which only need file or directory names.
> Debuginfod server is one such example.
> 
> Fix this by moving the srcfile handling in read_srclines into a separate
> function read_srcfiles.

Very nice. On irc you said that this patch speeds up debuginfod server
rpm indexing by 5% and reduces max resident set size during indexing by
up to 75%. Which are impressive numbers. Please feel free to add those
to the commit message and/or add a NEWS entry.

> read_srclines also no longer handles DW_LNE_define_file due to lack of
> use and to simplify the separation of srcfile and srcline reading.

I did say so, and DWARF5 says "The DW_LNE_define_file operation defined
in earlier versions of DWARF is deprecated in DWARF Version 5." It also
isn't officially defined anymore: "Code 0x03 is reserved to allow
backward compatible support of the DW_LNE_define_file operation which
was defined in DWARF Version 4 and earlier." Also I haven't seen it used
ever. But I still wonder if completely rejecting it is the right thing
to do.

DWARF4 says about DW_LNE_define_file:

   The primary source file is described by an entry whose path name
   exactly matches that given in the DW_AT_name attribute in the
   compilation unit, and whose directory index is 0. The files are
   numbered, starting at 1, in the order in which they appear; the
   names in the header come before names defined by the
   DW_LNE_define_file instruction. These numbers are used in the file
   register of the state machine.

Which seems to imply that files defined with DW_LNE_define_file are
only used in the line program, not in the DIE attributes. So maybe we
can accept them and just add/expand the srcfiles when we see them (this
does mean we might have to dynamically grow the array after the fact,
but given this almost never happens it shouldn't really impact
performance normally).

You can of course say this is way too much work for a nice feature. If
so then do add to the NEWS entry that DWARF .debug_line containing
DW_LNE_define_file aren't accepted anymore.

> 	* libdw/dwarf_getsrcfiles.c (dwarf_getsrcfiles): Replace
> 	dwarf_getsrclines and __libdw_getsrclines with
> 	__libdw_getsrcfiles.
> 	* libdw/dwarf_getsrclines.c (read_line_header): New function.
> 	(read_srcfiles): New function.
> 	(read_srclines): Move file reading into read_srcfiles.
> 	Remove DW_LNE_define_file handling.  Add parameter so
> 	that previously read srcfiles can be used if available.
> 	(__libdw_getsrclines): Call read_srcfiles if linesp is
> 	NULL.  Pass previously read srcfiles to read_srclines
> 	if available.
> 	(__libdw_getsrcfiles): New function.

I did found it slightly confusing that __libdw_getsrclines still just
calls __libdw_getsrclines. But that is just a naming/indirection issue
which hopefully doesn't confuse anybody else looking at the
implementation.

> 	* libdw/dwarf_macro_getsrcfiles.c (dwarf_macro_getsrcfiles):
> 	Replace __libdw_getsrclines with __libdw_getsrcfiles.
> 	* libdw/libdwP.h (__libdw_getsrcfiles): New declaration.
> 	* tests/get-files.c: Verify that dwarf_getsrcfiles does
> 	not cause srclines to be read.
> 	* tests/get-lines.c: Verify that srclines can be read
> 	after srcfiles have been read.
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=27405
> 
> Signed-off-by: Aaron Merey <amerey@redhat.com>
> ---
>  libdw/dwarf_getsrcfiles.c       |  24 +-
>  libdw/dwarf_getsrclines.c       | 506 ++++++++++++++++++--------------
>  libdw/dwarf_macro_getsrcfiles.c |   4 +-
>  libdw/libdwP.h                  |  10 +
>  tests/get-files.c               |   8 +
>  tests/get-lines.c               |  20 +-
>  6 files changed, 346 insertions(+), 226 deletions(-)
> 
> diff --git a/libdw/dwarf_getsrcfiles.c b/libdw/dwarf_getsrcfiles.c
> index cd2e5b5a..24e4b7d2 100644
> --- a/libdw/dwarf_getsrcfiles.c
> +++ b/libdw/dwarf_getsrcfiles.c
> @@ -70,10 +70,9 @@ dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files, size_t *nfiles)
>  		{
>  		  /* We are only interested in the files, the lines will
>  		     always come from the skeleton.  */
> -		  res = __libdw_getsrclines (cu->dbg, dwp_off,
> +		  res = __libdw_getsrcfiles (cu->dbg, dwp_off,
>  					     __libdw_getcompdir (cudie),
> -					     cu->address_size, NULL,
> -					     &cu->files);
> +					     cu->address_size, &cu->files);
>  		}
>  	    }
>  	  else

OK.

> @@ -89,12 +88,19 @@ dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files, size_t *nfiles)
>  	}
>        else
>  	{
> -	  Dwarf_Lines *lines;
> -	  size_t nlines;
> -
> -	  /* Let the more generic function do the work.  It'll create more
> -	     data but that will be needed in an real program anyway.  */
> -	  res = INTUSE(dwarf_getsrclines) (cudie, &lines, &nlines);
> +	  /* The die must have a statement list associated.  */
> +	  Dwarf_Attribute stmt_list_mem;
> +	  Dwarf_Attribute *stmt_list = INTUSE(dwarf_attr) (cudie, DW_AT_stmt_list,
> +							   &stmt_list_mem);
> +
> +	  Dwarf_Off debug_line_offset;
> +	  if (__libdw_formptr (stmt_list, IDX_debug_line, DWARF_E_NO_DEBUG_LINE,
> +			       NULL, &debug_line_offset) == NULL)
> +	    return -1;
> +
> +	  res = __libdw_getsrcfiles (cu->dbg, debug_line_offset,
> +				     __libdw_getcompdir (cudie),
> +				     cu->address_size, &cu->files);
>  	}
>      }
>    else if (cu->files != (void *) -1l)

OK.

> diff --git a/libdw/dwarf_getsrclines.c b/libdw/dwarf_getsrclines.c
> index 69e10c7b..4eda13f4 100644
> --- a/libdw/dwarf_getsrclines.c
> +++ b/libdw/dwarf_getsrclines.c
> @@ -77,6 +77,28 @@ compare_lines (const void *a, const void *b)
>      : 0;
>  }
>  
> +/* Decoded .debug_line program header.  */
> +struct line_header
> +{
> +  /* Header entries */
> +  Dwarf_Word unit_length;
> +  unsigned int length;
> +  uint_fast16_t version;
> +  size_t line_address_size;
> +  size_t segment_selector_size;
> +  Dwarf_Word header_length;
> +  const unsigned char *header_start;
> +  uint_fast8_t minimum_instr_len;
> +  uint_fast8_t max_ops_per_instr;
> +  uint_fast8_t default_is_stmt;
> +  int_fast8_t line_base;
> +  uint_fast8_t line_range;
> +  uint_fast8_t opcode_base;
> +  const uint8_t *standard_opcode_lengths;
> +  unsigned int debug_str_offset;  /* CUBIN only */
> +  size_t files_start;
> +};
> +
>  struct line_state
>  {
>    Dwarf_Word addr;
> @@ -155,127 +177,81 @@ add_new_line (struct line_state *state, struct linelist *new_line)
>    return false;
>  }
>  
> +/* Cache the .debug_line header.  Return 0 if sucessful, otherwise set
> +   libdw errno and return -1.  */
> +
>  static int
> -read_srclines (Dwarf *dbg,
> -	       const unsigned char *linep, const unsigned char *lineendp,
> -	       const char *comp_dir, unsigned address_size,
> -	       Dwarf_Lines **linesp, Dwarf_Files **filesp)
> +read_line_header (Dwarf *dbg, unsigned address_size,
> +		  const unsigned char *linep, const unsigned char *lineendp,
> +		  struct line_header *lh)
>  {
> -  int res = -1;
> -
> -  struct filelist *filelist = NULL;
> -  size_t nfilelist = 0;
> -  size_t ndirlist = 0;
> -
> -  /* If there are a large number of lines, files or dirs don't blow up
> -     the stack.  Stack allocate some entries, only dynamically malloc
> -     when more than MAX.  */
> -#define MAX_STACK_ALLOC 4096
> -#define MAX_STACK_LINES (MAX_STACK_ALLOC / 2)
> -#define MAX_STACK_FILES (MAX_STACK_ALLOC / 4)
> -#define MAX_STACK_DIRS  (MAX_STACK_ALLOC / 16)
> -
> -  /* Initial statement program state (except for stmt_list, see below).  */
> -  struct line_state state =
> -    {
> -      .linelist = NULL,
> -      .nlinelist = 0,
> -      .addr = 0,
> -      .op_index = 0,
> -      .file = 1,
> -      /* We only store int but want to check for overflow (see SET above).  */
> -      .line = 1,
> -      .column = 0,
> -      .basic_block = false,
> -      .prologue_end = false,
> -      .epilogue_begin = false,
> -      .isa = 0,
> -      .discriminator = 0,
> -      .context = 0,
> -      .function_name = 0
> -    };
> -
> -  /* The dirs normally go on the stack, but if there are too many
> -     we alloc them all.  Set up stack storage early, so we can check on
> -     error if we need to free them or not.  */
> -  struct dirlist
> -  {
> -    const char *dir;
> -    size_t len;
> -  };
> -  struct dirlist dirstack[MAX_STACK_DIRS];
> -  struct dirlist *dirarray = dirstack;
> +  const unsigned char *line_start = linep;
>  
>    if (unlikely (linep + 4 > lineendp))
> -    {
> -    invalid_data:
> -      __libdw_seterrno (DWARF_E_INVALID_DEBUG_LINE);
> -      goto out;
> -    }
> +    goto invalid_data;
>  
> -  Dwarf_Word unit_length = read_4ubyte_unaligned_inc (dbg, linep);
> -  unsigned int length = 4;
> -  if (unlikely (unit_length == DWARF3_LENGTH_64_BIT))
> +  lh->unit_length = read_4ubyte_unaligned_inc (dbg, linep);
> +  lh->length = 4;
> +  if (unlikely (lh->unit_length == DWARF3_LENGTH_64_BIT))
>      {
>        if (unlikely (linep + 8 > lineendp))
>  	goto invalid_data;
> -      unit_length = read_8ubyte_unaligned_inc (dbg, linep);
> -      length = 8;
> +      lh->unit_length = read_8ubyte_unaligned_inc (dbg, linep);
> +      lh->length = 8;
>      }
>  
>    /* Check whether we have enough room in the section.  */
> -  if (unlikely (unit_length > (size_t) (lineendp - linep)))
> +  if (unlikely (lh->unit_length > (size_t) (lineendp - linep)))
>      goto invalid_data;
> -  lineendp = linep + unit_length;
> +  lineendp = linep + lh->unit_length;
>  
>    /* The next element of the header is the version identifier.  */
>    if ((size_t) (lineendp - linep) < 2)
>      goto invalid_data;
> -  uint_fast16_t version = read_2ubyte_unaligned_inc (dbg, linep);
> -  if (unlikely (version < 2) || unlikely (version > 5))
> +  lh->version = read_2ubyte_unaligned_inc (dbg, linep);
> +  if (unlikely (lh->version < 2) || unlikely (lh->version > 5))
>      {
>        __libdw_seterrno (DWARF_E_VERSION);
> -      goto out;
> +      return -1;
>      }
>  
>    /* DWARF5 explicitly lists address and segment_selector sizes.  */
> -  if (version >= 5)
> +  if (lh->version >= 5)
>      {
>        if ((size_t) (lineendp - linep) < 2)
>  	goto invalid_data;
> -      size_t line_address_size = *linep++;
> -      size_t segment_selector_size = *linep++;
> -      if (line_address_size != address_size || segment_selector_size != 0)
> +      lh->line_address_size = *linep++;
> +      lh->segment_selector_size = *linep++;
> +      if (lh->line_address_size != address_size || lh->segment_selector_size != 0)
>  	goto invalid_data;
>      }
>  
>    /* Next comes the header length.  */
> -  Dwarf_Word header_length;
> -  if (length == 4)
> +  if (lh->length == 4)
>      {
>        if ((size_t) (lineendp - linep) < 4)
>  	goto invalid_data;
> -      header_length = read_4ubyte_unaligned_inc (dbg, linep);
> +      lh->header_length = read_4ubyte_unaligned_inc (dbg, linep);
>      }
>    else
>      {
>        if ((size_t) (lineendp - linep) < 8)
>  	goto invalid_data;
> -      header_length = read_8ubyte_unaligned_inc (dbg, linep);
> +      lh->header_length = read_8ubyte_unaligned_inc (dbg, linep);
>      }
> -  const unsigned char *header_start = linep;
> +  lh->header_start = linep;
>  
>    /* Next the minimum instruction length.  */
> -  uint_fast8_t minimum_instr_len = *linep++;
> +  lh->minimum_instr_len = *linep++;
>  
>    /* Next the maximum operations per instruction, in version 4 format.  */
> -  uint_fast8_t max_ops_per_instr = 1;
> -  if (version >= 4)
> +  lh->max_ops_per_instr = 1;
> +  if (lh->version >= 4)
>      {
>        if (unlikely ((size_t) (lineendp - linep) < 1))
>  	goto invalid_data;
> -      max_ops_per_instr = *linep++;
> -      if (unlikely (max_ops_per_instr == 0))
> +      lh->max_ops_per_instr = *linep++;
> +      if (unlikely (lh->max_ops_per_instr == 0))
>  	goto invalid_data;
>      }
>  
> @@ -285,23 +261,73 @@ read_srclines (Dwarf *dbg,
>  
>    /* Then the flag determining the default value of the is_stmt
>       register.  */
> -  uint_fast8_t default_is_stmt = *linep++;
> +  lh->default_is_stmt = *linep++;
>  
>    /* Now the line base.  */
> -  int_fast8_t line_base = (int8_t) *linep++;
> +  lh->line_base = (int8_t) *linep++;
>  
>    /* And the line range.  */
> -  uint_fast8_t line_range = *linep++;
> +  lh->line_range = *linep++;
>  
>    /* The opcode base.  */
> -  uint_fast8_t opcode_base = *linep++;
> +  lh->opcode_base = *linep++;
>  
>    /* Remember array with the standard opcode length (-1 to account for
>       the opcode with value zero not being mentioned).  */
> -  const uint8_t *standard_opcode_lengths = linep - 1;
> -  if (unlikely (lineendp - linep < opcode_base - 1))
> +  lh->standard_opcode_lengths = linep - 1;
> +  if (unlikely (lineendp - linep < lh->opcode_base - 1))
>      goto invalid_data;
> -  linep += opcode_base - 1;
> +  linep += lh->opcode_base - 1;
> +
> +  /* Record beginning of the file information.  */
> +  lh->files_start = (size_t) (linep - line_start);
> +
> +  return 0;
> +
> +invalid_data:
> +  __libdw_seterrno (DWARF_E_INVALID_DEBUG_LINE);
> +  return -1;
> +}

OK, this now just fills in the new  struct line_header argument.

> +/* If there are a large number of lines, files or dirs don't blow up
> +   the stack.  Stack allocate some entries, only dynamically malloc
> +   when more than MAX.  */
> +#define MAX_STACK_ALLOC 4096
> +#define MAX_STACK_LINES (MAX_STACK_ALLOC / 2)
> +#define MAX_STACK_FILES (MAX_STACK_ALLOC / 4)
> +#define MAX_STACK_DIRS  (MAX_STACK_ALLOC / 16)
> +
> +static int
> +read_srcfiles (Dwarf *dbg,
> +	       const unsigned char *linep, const unsigned char *lineendp,
> +	       const char *comp_dir, unsigned address_size,
> +	       Dwarf_Files **filesp, struct line_header *lh)
> +{
> +  struct line_header lh_local;
> +
> +  if (lh == NULL)
> +    {
> +      if (read_line_header (dbg, address_size, linep, lineendp, &lh_local) != 0)
> +	return -1;
> +      lh = &lh_local;
> +    }
> +
> +  int res = -1;
> +
> +  struct filelist *filelist = NULL;
> +  size_t nfilelist = 0;
> +  size_t ndirlist = 0;
> +
> +  /* The dirs normally go on the stack, but if there are too many
> +     we alloc them all.  Set up stack storage early, so we can check on
> +     error if we need to free them or not.  */
> +  struct dirlist
> +  {
> +    const char *dir;
> +    size_t len;
> +  };
> +  struct dirlist dirstack[MAX_STACK_DIRS];
> +  struct dirlist *dirarray = dirstack;
>  
>    /* To read DWARF5 dir and file lists we need to know the forms.  For
>       now we skip everything, except the DW_LNCT_path and
> @@ -311,12 +337,18 @@ read_srclines (Dwarf *dbg,
>    unsigned char form_path = -1; /* Which forms is DW_LNCT_path.  */
>    unsigned char form_idx = -1;  /* And which is DW_LNCT_directory_index.  */
>  
> +  /* Set lineendp to the end of the line information.  */
> +  lineendp = linep + lh->length + lh->unit_length;
> +
> +  /* Advance linep to the beginning of the header's srcfile information.  */
> +  linep += lh->files_start;
> +
>    /* To read/skip form data.  */
>    Dwarf_CU fake_cu = {
>      .dbg = dbg,
>      .sec_idx = IDX_debug_line,
>      .version = 5,
> -    .offset_size = length,
> +    .offset_size = lh->length,
>      .address_size = address_size,
>      .startp = (void *) linep,
>      .endp = (void *) lineendp,
> @@ -324,7 +356,7 @@ read_srclines (Dwarf *dbg,
>  
>    /* First count the entries.  */
>    size_t ndirs = 0;
> -  if (version < 5)
> +  if (lh->version < 5)
>      {
>        const unsigned char *dirp = linep;
>        while (dirp < lineendp && *dirp != 0)
> @@ -395,7 +427,7 @@ read_srclines (Dwarf *dbg,
>  
>    /* Entry zero is implicit for older versions, but explicit for 5+.  */
>    struct dirlist comp_dir_elem;
> -  if (version < 5)
> +  if (lh->version < 5)
>      {
>        /* First comes the list of directories.  Add the compilation
>  	 directory first since the index zero is used for it.  */
> @@ -482,7 +514,7 @@ read_srclines (Dwarf *dbg,
>    fl; })
>  
>    /* Now read the files.  */
> -  if (version < 5)
> +  if (lh->version < 5)
>      {
>        if (unlikely (linep >= lineendp))
>  	goto invalid_data;
> @@ -662,29 +694,111 @@ read_srclines (Dwarf *dbg,
>  	}
>      }
>  
> -  unsigned int debug_str_offset = 0;
> -  if (unlikely (linep == header_start + header_length - 4))
> +  if (unlikely (linep == lh->header_start + lh->header_length - 4))
>      {
>        /* CUBINs contain an unsigned 4-byte offset */
> -      debug_str_offset = read_4ubyte_unaligned_inc (dbg, linep);
> +      lh->debug_str_offset = read_4ubyte_unaligned_inc (dbg, linep);
>      }
>  
>    /* Consistency check.  */
> -  if (unlikely (linep != header_start + header_length))
> +  if (unlikely (linep != lh->header_start + lh->header_length))
> +    goto invalid_data;
> +
> +  /* Put all the files in an array.  */
> +  Dwarf_Files *files = libdw_alloc (dbg, Dwarf_Files,
> +				    sizeof (Dwarf_Files)
> +				    + nfilelist * sizeof (Dwarf_Fileinfo)
> +				    + (ndirlist + 1) * sizeof (char *),
> +				    1);
> +  const char **dirs = (void *) &files->info[nfilelist];
> +
> +  struct filelist *fileslist = filelist;
> +  files->nfiles = nfilelist;
> +  for (size_t n = nfilelist; n > 0; n--)
> +    {
> +      files->info[n - 1] = fileslist->info;
> +      fileslist = fileslist->next;
> +    }
> +  assert (fileslist == NULL);
> +
> +  /* Put all the directory strings in an array.  */
> +  files->ndirs = ndirlist;
> +  for (unsigned int i = 0; i < ndirlist; ++i)
> +    dirs[i] = dirarray[i].dir;
> +  dirs[ndirlist] = NULL;
> +
> +  /* Pass the file data structure to the caller.  */
> +  if (filesp != NULL)
> +    *filesp = files;
> +
> +  res = 0;
> +  goto out;
> +
> +invalid_data:
> +  __libdw_seterrno (DWARF_E_INVALID_DEBUG_LINE);
> +
> +out:
> +  if (dirarray != dirstack)
> +    free (dirarray);
> +  for (size_t i = MAX_STACK_FILES; i < nfilelist; i++)
>      {
> -      __libdw_seterrno (DWARF_E_INVALID_DWARF);
> -      goto out;
> +      struct filelist *fl = filelist->next;
> +      free (filelist);
> +      filelist = fl;
>      }
>  
> +  return res;
> +}


And then this does the filelist part, also possibly using/filling in a
new struct line_header argument.

> +static int
> +read_srclines (Dwarf *dbg,
> +	       const unsigned char *linep, const unsigned char *lineendp,
> +	       const char *comp_dir, unsigned address_size,
> +	       Dwarf_Lines **linesp, Dwarf_Files **filesp,
> +	       bool read_files)
> +{
> +  struct line_header lh;
> +
> +  if (read_line_header (dbg, address_size, linep, lineendp, &lh) != 0)
> +    return -1;
> +
> +  /* If read_files is true then reuse cached Dwarf_Files.  */
> +  if (read_files && read_srcfiles (dbg, linep, lineendp, comp_dir,
> +				   address_size, filesp, &lh) != 0)
> +    return -1;
> +
> +  int res = -1;
> +
> +  /* Initial statement program state (except for stmt_list, see below).  */
> +  struct line_state state =
> +    {
> +      .linelist = NULL,
> +      .nlinelist = 0,
> +      .addr = 0,
> +      .op_index = 0,
> +      .file = 1,
> +      /* We only store int but want to check for overflow (see SET above).  */
> +      .line = 1,
> +      .column = 0,
> +      .basic_block = false,
> +      .prologue_end = false,
> +      .epilogue_begin = false,
> +      .isa = 0,
> +      .discriminator = 0,
> +      .context = 0,
> +      .function_name = 0
> +    };
> +
>    /* We are about to process the statement program.  Most state machine
>       registers have already been initialize above.  Just add the is_stmt
>       default. See 6.2.2 in the v2.1 specification.  */
> -  state.is_stmt = default_is_stmt;
> +  state.is_stmt = lh.default_is_stmt;
>  
>    /* Apply the "operation advance" from a special opcode or
>       DW_LNS_advance_pc (as per DWARF4 6.2.5.1).  */
>  #define advance_pc(op_advance) \
> -  run_advance_pc (&state, op_advance, minimum_instr_len, max_ops_per_instr)
> +  run_advance_pc (&state, op_advance, lh.minimum_instr_len, \
> +		  lh.max_ops_per_instr)
>  
>    /* Process the instructions.  */
>  
> @@ -697,12 +811,21 @@ read_srclines (Dwarf *dbg,
>  			   ? &llstack[state.nlinelist]		\
>  			   : malloc (sizeof (struct linelist)));	\
>      if (unlikely (ll == NULL))					\
> -      goto no_mem;						\
> +    {									\
> +	__libdw_seterrno (DWARF_E_NOMEM);				\
> +	goto out;							\
> +    }									\
>      state.end_sequence = end_seq;				\
>      if (unlikely (add_new_line (&state, ll)))			\
>        goto invalid_data;						\
>    } while (0)
>  
> +  /* Set lineendp to the end of the line information.  */
> +  lineendp = linep + lh.length + lh.unit_length;
> +
> +  /* Set linep to the beginning of the line information.  */
> +  linep = lh.header_start + lh.header_length;
> +
>    while (linep < lineendp)
>      {
>        unsigned int opcode;
> @@ -713,9 +836,9 @@ read_srclines (Dwarf *dbg,
>        opcode = *linep++;
>  
>        /* Is this a special opcode?  */
> -      if (likely (opcode >= opcode_base))
> +      if (likely (opcode >= lh.opcode_base))
>  	{
> -	  if (unlikely (line_range == 0))
> +	  if (unlikely (lh.line_range == 0))
>  	    goto invalid_data;
>  
>  	  /* Yes.  Handling this is quite easy since the opcode value
> @@ -724,12 +847,12 @@ read_srclines (Dwarf *dbg,
>  	     opcode = (desired line increment - line_base)
>  		       + (line_range * address advance) + opcode_base
>  	  */
> -	  int line_increment = (line_base
> -				+ (opcode - opcode_base) % line_range);
> +	  int line_increment = (lh.line_base
> +				+ (opcode - lh.opcode_base) % lh.line_range);
>  
>  	  /* Perform the increments.  */
>  	  state.line += line_increment;
> -	  advance_pc ((opcode - opcode_base) / line_range);
> +	  advance_pc ((opcode - lh.opcode_base) / lh.line_range);
>  
>  	  /* Add a new line with the current state machine values.  */
>  	  NEW_LINE (0);
> @@ -768,7 +891,7 @@ read_srclines (Dwarf *dbg,
>  	      state.file = 1;
>  	      state.line = 1;
>  	      state.column = 0;
> -	      state.is_stmt = default_is_stmt;
> +	      state.is_stmt = lh.default_is_stmt;
>  	      state.basic_block = false;
>  	      state.prologue_end = false;
>  	      state.epilogue_begin = false;
> @@ -790,63 +913,9 @@ read_srclines (Dwarf *dbg,
>  		goto out;
>  	      break;
>  
> -	    case DW_LNE_define_file:
> -	      {
> -		char *fname = (char *) linep;
> -		uint8_t *endp = memchr (linep, '\0', lineendp - linep);
> -		if (endp == NULL)
> -		  goto invalid_data;
> -		size_t fnamelen = endp - linep;
> -		linep = endp + 1;
> -
> -		unsigned int diridx;
> -		if (unlikely (linep >= lineendp))
> -		  goto invalid_data;
> -		get_uleb128 (diridx, linep, lineendp);
> -		if (unlikely (diridx >= ndirlist))
> -		  {
> -		    __libdw_seterrno (DWARF_E_INVALID_DIR_IDX);
> -		    goto invalid_data;
> -		  }
> -		Dwarf_Word mtime;
> -		if (unlikely (linep >= lineendp))
> -		  goto invalid_data;
> -		get_uleb128 (mtime, linep, lineendp);
> -		Dwarf_Word filelength;
> -		if (unlikely (linep >= lineendp))
> -		  goto invalid_data;
> -		get_uleb128 (filelength, linep, lineendp);
> -
> -		struct filelist *new_file = NEW_FILE ();
> -		if (fname[0] == '/')
> -		  new_file->info.name = fname;
> -		else
> -		  {
> -		    new_file->info.name =
> -		      libdw_alloc (dbg, char, 1, (dirarray[diridx].len + 1
> -						  + fnamelen + 1));
> -		    char *cp = new_file->info.name;
> -
> -		    if (dirarray[diridx].dir != NULL)
> -		      /* This value could be NULL in case the
> -			 DW_AT_comp_dir was not present.  We
> -			 cannot do much in this case.  Just
> -			 keep the file relative.  */
> -		      {
> -			cp = stpcpy (cp, dirarray[diridx].dir);
> -			*cp++ = '/';
> -		      }
> -		    strcpy (cp, fname);
> -		  }
> -
> -		new_file->info.mtime = mtime;
> -		new_file->info.length = filelength;
> -	      }
> -	      break;

So here I would suggest we still parse it and possibly add it the the
(already existing) srcfiles (dynamically extending it?).

>  	    case DW_LNE_set_discriminator:
>  	      /* Takes one ULEB128 parameter, the discriminator.  */
> -	      if (unlikely (standard_opcode_lengths[opcode] != 1))
> +	      if (unlikely (lh.standard_opcode_lengths[opcode] != 1))
>  		goto invalid_data;
>  
>  	      if (unlikely (linep >= lineendp))
> @@ -861,14 +930,14 @@ read_srclines (Dwarf *dbg,
>  	      if (unlikely (linep >= lineendp))
>  		goto invalid_data;
>  	      get_uleb128 (state.function_name, linep, lineendp);
> -	      state.function_name += debug_str_offset;
> +	      state.function_name += lh.debug_str_offset;
>  	      break;
>  
>  	    case DW_LNE_NVIDIA_set_function_name:
>  	      if (unlikely (linep >= lineendp))
>  		goto invalid_data;
>  	      get_uleb128 (state.function_name, linep, lineendp);
> -	      state.function_name += debug_str_offset;
> +	      state.function_name += lh.debug_str_offset;
>  	      break;
>  
>  	    default:
> @@ -886,7 +955,7 @@ read_srclines (Dwarf *dbg,
>  	    {
>  	    case DW_LNS_copy:
>  	      /* Takes no argument.  */
> -	      if (unlikely (standard_opcode_lengths[opcode] != 0))
> +	      if (unlikely (lh.standard_opcode_lengths[opcode] != 0))
>  		goto invalid_data;
>  
>  	      /* Add a new line with the current state machine values.  */
> @@ -902,7 +971,7 @@ read_srclines (Dwarf *dbg,
>  	    case DW_LNS_advance_pc:
>  	      /* Takes one uleb128 parameter which is added to the
>  		 address.  */
> -	      if (unlikely (standard_opcode_lengths[opcode] != 1))
> +	      if (unlikely (lh.standard_opcode_lengths[opcode] != 1))
>  		goto invalid_data;
>  
>  	      if (unlikely (linep >= lineendp))
> @@ -914,7 +983,7 @@ read_srclines (Dwarf *dbg,
>  	    case DW_LNS_advance_line:
>  	      /* Takes one sleb128 parameter which is added to the
>  		 line.  */
> -	      if (unlikely (standard_opcode_lengths[opcode] != 1))
> +	      if (unlikely (lh.standard_opcode_lengths[opcode] != 1))
>  		goto invalid_data;
>  
>  	      if (unlikely (linep >= lineendp))
> @@ -925,7 +994,7 @@ read_srclines (Dwarf *dbg,
>  
>  	    case DW_LNS_set_file:
>  	      /* Takes one uleb128 parameter which is stored in file.  */
> -	      if (unlikely (standard_opcode_lengths[opcode] != 1))
> +	      if (unlikely (lh.standard_opcode_lengths[opcode] != 1))
>  		goto invalid_data;
>  
>  	      if (unlikely (linep >= lineendp))
> @@ -936,7 +1005,7 @@ read_srclines (Dwarf *dbg,
>  
>  	    case DW_LNS_set_column:
>  	      /* Takes one uleb128 parameter which is stored in column.  */
> -	      if (unlikely (standard_opcode_lengths[opcode] != 1))
> +	      if (unlikely (lh.standard_opcode_lengths[opcode] != 1))
>  		goto invalid_data;
>  
>  	      if (unlikely (linep >= lineendp))
> @@ -947,7 +1016,7 @@ read_srclines (Dwarf *dbg,
>  
>  	    case DW_LNS_negate_stmt:
>  	      /* Takes no argument.  */
> -	      if (unlikely (standard_opcode_lengths[opcode] != 0))
> +	      if (unlikely (lh.standard_opcode_lengths[opcode] != 0))
>  		goto invalid_data;
>  
>  	      state.is_stmt = 1 - state.is_stmt;
> @@ -955,7 +1024,7 @@ read_srclines (Dwarf *dbg,
>  
>  	    case DW_LNS_set_basic_block:
>  	      /* Takes no argument.  */
> -	      if (unlikely (standard_opcode_lengths[opcode] != 0))
> +	      if (unlikely (lh.standard_opcode_lengths[opcode] != 0))
>  		goto invalid_data;
>  
>  	      state.basic_block = true;
> @@ -963,19 +1032,19 @@ read_srclines (Dwarf *dbg,
>  
>  	    case DW_LNS_const_add_pc:
>  	      /* Takes no argument.  */
> -	      if (unlikely (standard_opcode_lengths[opcode] != 0))
> +	      if (unlikely (lh.standard_opcode_lengths[opcode] != 0))
>  		goto invalid_data;
>  
> -	      if (unlikely (line_range == 0))
> +	      if (unlikely (lh.line_range == 0))
>  		goto invalid_data;
>  
> -	      advance_pc ((255 - opcode_base) / line_range);
> +	      advance_pc ((255 - lh.opcode_base) / lh.line_range);
>  	      break;
>  
>  	    case DW_LNS_fixed_advance_pc:
>  	      /* Takes one 16 bit parameter which is added to the
>  		 address.  */
> -	      if (unlikely (standard_opcode_lengths[opcode] != 1)
> +	      if (unlikely (lh.standard_opcode_lengths[opcode] != 1)
>  		  || unlikely (lineendp - linep < 2))
>  		goto invalid_data;
>  
> @@ -985,7 +1054,7 @@ read_srclines (Dwarf *dbg,
>  
>  	    case DW_LNS_set_prologue_end:
>  	      /* Takes no argument.  */
> -	      if (unlikely (standard_opcode_lengths[opcode] != 0))
> +	      if (unlikely (lh.standard_opcode_lengths[opcode] != 0))
>  		goto invalid_data;
>  
>  	      state.prologue_end = true;
> @@ -993,7 +1062,7 @@ read_srclines (Dwarf *dbg,
>  
>  	    case DW_LNS_set_epilogue_begin:
>  	      /* Takes no argument.  */
> -	      if (unlikely (standard_opcode_lengths[opcode] != 0))
> +	      if (unlikely (lh.standard_opcode_lengths[opcode] != 0))
>  		goto invalid_data;
>  
>  	      state.epilogue_begin = true;
> @@ -1001,7 +1070,7 @@ read_srclines (Dwarf *dbg,
>  
>  	    case DW_LNS_set_isa:
>  	      /* Takes one uleb128 parameter which is stored in isa.  */
> -	      if (unlikely (standard_opcode_lengths[opcode] != 1))
> +	      if (unlikely (lh.standard_opcode_lengths[opcode] != 1))
>  		goto invalid_data;
>  
>  	      if (unlikely (linep >= lineendp))
> @@ -1015,7 +1084,7 @@ read_srclines (Dwarf *dbg,
>  	  /* This is a new opcode the generator but not we know about.
>  	     Read the parameters associated with it but then discard
>  	     everything.  Read all the parameters for this opcode.  */
> -	  for (int n = standard_opcode_lengths[opcode]; n > 0; --n)
> +	  for (int n = lh.standard_opcode_lengths[opcode]; n > 0; --n)
>  	    {
>  	      if (unlikely (linep >= lineendp))
>  		goto invalid_data;
> @@ -1027,33 +1096,6 @@ read_srclines (Dwarf *dbg,
>  	}
>      }
>  
> -  /* Put all the files in an array.  */
> -  Dwarf_Files *files = libdw_alloc (dbg, Dwarf_Files,
> -				    sizeof (Dwarf_Files)
> -				    + nfilelist * sizeof (Dwarf_Fileinfo)
> -				    + (ndirlist + 1) * sizeof (char *),
> -				    1);
> -  const char **dirs = (void *) &files->info[nfilelist];
> -
> -  struct filelist *fileslist = filelist;
> -  files->nfiles = nfilelist;
> -  for (size_t n = nfilelist; n > 0; n--)
> -    {
> -      files->info[n - 1] = fileslist->info;
> -      fileslist = fileslist->next;
> -    }
> -  assert (fileslist == NULL);
> -
> -  /* Put all the directory strings in an array.  */
> -  files->ndirs = ndirlist;
> -  for (unsigned int i = 0; i < ndirlist; ++i)
> -    dirs[i] = dirarray[i].dir;
> -  dirs[ndirlist] = NULL;
> -
> -  /* Pass the file data structure to the caller.  */
> -  if (filesp != NULL)
> -    *filesp = files;
> -
>    size_t buf_size = (sizeof (Dwarf_Lines)
>  		     + (sizeof (Dwarf_Line) * state.nlinelist));
>    void *buf = libdw_alloc (dbg, Dwarf_Lines, buf_size, 1);
> @@ -1087,7 +1129,7 @@ read_srclines (Dwarf *dbg,
>    for (size_t i = 0; i < state.nlinelist; ++i)
>      {
>        lines->info[i] = sortlines[i]->line;
> -      lines->info[i].files = files;
> +      lines->info[i].files = *filesp;
>      }
>  
>    /* Make sure the highest address for the CU is marked as end_sequence.
> @@ -1102,8 +1144,12 @@ read_srclines (Dwarf *dbg,
>  
>    /* Success.  */
>    res = 0;
> +  goto out;
> +
> +invalid_data:
> +  __libdw_seterrno (DWARF_E_INVALID_DEBUG_LINE);
>  
> - out:
> +out:
>    /* Free malloced line records, if any.  */
>    for (size_t i = MAX_STACK_LINES; i < state.nlinelist; i++)
>      {
> @@ -1111,14 +1157,6 @@ read_srclines (Dwarf *dbg,
>        free (state.linelist);
>        state.linelist = ll;
>      }
> -  if (dirarray != dirstack)
> -    free (dirarray);
> -  for (size_t i = MAX_STACK_FILES; i < nfilelist; i++)
> -    {
> -      struct filelist *fl = filelist->next;
> -      free (filelist);
> -      filelist = fl;
> -    }
>  
>    return res;
>  }

OK, so this now only does srclines.

> @@ -1148,6 +1186,7 @@ __libdw_getsrclines (Dwarf *dbg, Dwarf_Off debug_line_offset,
>  					files_lines_compare);
>    if (found == NULL)
>      {
> +      /* This .debug_line is being read for the first time.  */
>        Elf_Data *data = __libdw_checked_get_data (dbg, IDX_debug_line);
>        if (data == NULL
>  	  || __libdw_offset_in_section (dbg, IDX_debug_line,
> @@ -1160,8 +1199,13 @@ __libdw_getsrclines (Dwarf *dbg, Dwarf_Off debug_line_offset,
>        struct files_lines_s *node = libdw_alloc (dbg, struct files_lines_s,
>  						sizeof *node, 1);
>  
> -      if (read_srclines (dbg, linep, lineendp, comp_dir, address_size,
> -			 &node->lines, &node->files) != 0)
> +      /* If linesp is NULL then read srcfiles without reading srclines.  */
> +      if (linesp == NULL
> +	  && read_srcfiles (dbg, linep, lineendp, comp_dir, address_size,
> +			    &node->files, NULL) != 0)
> +	    return -1;
> +      else if (read_srclines (dbg, linep, lineendp, comp_dir, address_size,
> +			 &node->lines, &node->files, true) != 0)
>  	return -1;
>  
>        node->debug_line_offset = debug_line_offset;
> @@ -1173,6 +1217,30 @@ __libdw_getsrclines (Dwarf *dbg, Dwarf_Off debug_line_offset,
>  	  return -1;
>  	}
>      }
> +  else if (*found != NULL && (*found)->files != 0)
> +    {
> +      /* Srcfiles were already read from this .debug_line.  Now read
> +	 srclines.  */
> +      Elf_Data *data = __libdw_checked_get_data (dbg, IDX_debug_line);
> +      if (data == NULL
> +	  || __libdw_offset_in_section (dbg, IDX_debug_line,
> +					debug_line_offset, 1) != 0)
> +	return -1;
> +
> +      const unsigned char *linep = data->d_buf + debug_line_offset;
> +      const unsigned char *lineendp = data->d_buf + data->d_size;
> +
> +      struct files_lines_s *node = *found;
> +
> +      if (read_srclines (dbg, linep, lineendp, comp_dir, address_size,
> +			 &node->lines, &node->files, false) != 0)
> +	return -1;
> +    }
> +  else
> +    {
> +      __libdw_seterrno (DWARF_E_INVALID_DEBUG_LINE);
> +      return -1;
> +    }
>  
>    if (linesp != NULL)
>      *linesp = (*found)->lines;
> @@ -1183,6 +1251,16 @@ __libdw_getsrclines (Dwarf *dbg, Dwarf_Off debug_line_offset,
>    return 0;
>  }

It is called __libdw_getsrclines but uses the above split out functions
to read either.

> +int
> +internal_function
> +__libdw_getsrcfiles (Dwarf *dbg, Dwarf_Off debug_line_offset,
> +		     const char *comp_dir, unsigned address_size,
> +		     Dwarf_Files **filesp)
> +{
> +  return __libdw_getsrclines (dbg, debug_line_offset, comp_dir,
> +			      address_size, NULL, filesp);
> +}
> +
>  /* Get the compilation directory, if any is set.  */
>  const char *
>  __libdw_getcompdir (Dwarf_Die *cudie)

OK, calling __libdw_getsrclines with linesp NULL.
Slightly confusing for the casual reader though.

> diff --git a/libdw/dwarf_macro_getsrcfiles.c b/libdw/dwarf_macro_getsrcfiles.c
> index 11c587af..5e02935d 100644
> --- a/libdw/dwarf_macro_getsrcfiles.c
> +++ b/libdw/dwarf_macro_getsrcfiles.c
> @@ -74,8 +74,8 @@ dwarf_macro_getsrcfiles (Dwarf *dbg, Dwarf_Macro *macro,
>  	 the same unit through dwarf_getsrcfiles, and the file names
>  	 will be broken.  */
>  
> -      if (__libdw_getsrclines (table->dbg, line_offset, table->comp_dir,
> -			       table->address_size, NULL, &table->files) < 0)
> +      if (__libdw_getsrcfiles (table->dbg, line_offset, table->comp_dir,
> +			       table->address_size, &table->files) < 0)
>  	table->files = (void *) -1;
>      }

OK.

> diff --git a/libdw/libdwP.h b/libdw/libdwP.h
> index c1c84ed3..e55ff50a 100644
> --- a/libdw/libdwP.h
> +++ b/libdw/libdwP.h
> @@ -1108,6 +1108,16 @@ int __libdw_getsrclines (Dwarf *dbg, Dwarf_Off debug_line_offset,
>    internal_function
>    __nonnull_attribute__ (1);
>  
> +/* Load .debug_line unit at DEBUG_LINE_OFFSET.  COMP_DIR is a value of
> +   DW_AT_comp_dir or NULL if that attribute is not available.  Caches
> +   the loaded unit and set *FILESP with loaded information.  Returns 0
> +   for success or a negative value for failure.  */
> +int __libdw_getsrcfiles (Dwarf *dbg, Dwarf_Off debug_line_offset,
> +			 const char *comp_dir, unsigned address_size,
> +			 Dwarf_Files **filesp)
> +  internal_function
> +  __nonnull_attribute__ (1);
> +
>  /* Load and return value of DW_AT_comp_dir from CUDIE.  */
>  const char *__libdw_getcompdir (Dwarf_Die *cudie);

OK.

> diff --git a/tests/get-files.c b/tests/get-files.c
> index 04091733..fa65aa93 100644
> --- a/tests/get-files.c
> +++ b/tests/get-files.c
> @@ -24,6 +24,7 @@
>  #include ELFUTILS_HEADER(dw)
>  #include <stdio.h>
>  #include <unistd.h>
> +#include "../libdw/libdwP.h"
>  
>  
>  int
> @@ -76,6 +77,13 @@ main (int argc, char *argv[])
>  	      break;
>  	    }
>  
> +	  if (die->cu->lines != NULL)
> +	    {
> +	      printf ("%s: dwarf_getsrcfiles should not get lines\n", argv[cnt]);
> +	      result = 1;
> +	      break;
> +	    }
> +
>  	  const char *const *dirs;
>  	  size_t ndirs;
>  	  if (dwarf_getsrcdirs (files, &dirs, &ndirs) != 0)
> diff --git a/tests/get-lines.c b/tests/get-lines.c
> index 188d0162..77fb3c54 100644
> --- a/tests/get-lines.c
> +++ b/tests/get-lines.c
> @@ -26,6 +26,7 @@
>  #include <stdio.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include "../libdw/libdwP.h"
>  
>  
>  int
> @@ -69,6 +70,24 @@ main (int argc, char *argv[])
>  	    }
>  	  old_cuoff = cuoff;
>  
> +	  Dwarf_Files *files;
> +	  size_t nfiles;
> +
> +	  /* Get files first to test that lines are read separately.  */
> +	  if (dwarf_getsrcfiles (&die, &files, &nfiles) != 0)
> +	    {
> +	      printf ("%s: cannot get files\n", argv[cnt]);
> +	      result = 1;
> +	      break;
> +	    }
> +
> +	  if (die.cu->lines != NULL)
> +	    {
> +	      printf ("%s: dwarf_getsrcfiles should not get lines\n", argv[cnt]);
> +	      result = 1;
> +	      break;
> +	    }
> +
>  	  Dwarf_Lines *lb;
>  	  size_t nlb;
>  	  if (dwarf_getsrclines (&die, &lb, &nlb) != 0)
> @@ -103,7 +122,6 @@ main (int argc, char *argv[])
>  
>  	      /* Getting the file path through the Dwarf_Files should
>  		 result in the same path.  */
> -	      Dwarf_Files *files;
>  	      size_t idx;
>  	      if (dwarf_line_file (l, &files, &idx) != 0)
>  		{

OK. Tests do use libdwP.h internals. Which isn't great. But OK in this
case because there is no other way to test this functionality.

tests/run-get-files.sh and tests/run-get-lines.sh also contain
testfiles using dwo and dwp files, which all still pass.

Thanks,

Mark