Re: [patch] Use mmap instead of obstack_alloc for dwarf debug sections.

[Tom Tromey <tromey@redhat.com>]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> Sorry, attached the bfd patch instead of the gdb one :-(
Paul> Here is the right one.

Thanks.  One point of order -- if you re-send a patch, re-send the
ChangeLog entry too.  Not a big deal, but it makes things simpler when
reviewing.  Thanks.

I saw in your original post that this patch helps on
memory-constrained machines; your example was 2G of debug info on a 4G
machine.  I just want to make sure that this doesn't hurt other cases;
I don't expect it would, but it is good to cross the t's...

I like how this patch cleans up some globals in dwarf2read.c.

I have a few comments, mostly nits, but one real one.

Paul> +struct dwarf2_section_info
Paul> +{
Paul> +  asection *asection;
Paul> +  gdb_byte *buffer;
Paul> +  bfd_size_type size;
Paul> +  int was_mmaped;

I would spell this "was_mmapped" (two "p"s).

Paul> +zlib_decompress_section (struct objfile *objfile, asection *sectp,
Paul> +                         gdb_byte **outbuf, bfd_size_type *outsize)

[...]

Paul> +  bfd_size_type compressed_size = bfd_get_section_size (sectp);
Paul> +  gdb_byte *compressed_buffer = xmalloc (compressed_size);
Paul> +  bfd_size_type uncompressed_size;
Paul> +  gdb_byte *uncompressed_buffer;
Paul> +  z_stream strm;
Paul> +  int rc;
Paul> +  int header_size = 12;
Paul> +
Paul> +  if (bfd_seek (abfd, sectp->filepos, SEEK_SET) != 0
Paul> +      || bfd_bread (compressed_buffer, compressed_size, abfd) != compressed_size)
Paul> +    error (_("Dwarf Error: Can't read DWARF data from '%s'"),
Paul> +           bfd_get_filename (abfd));

This isn't your problem -- but I noticed that this will leak
compressed_buffer if there is an error.  This code needs to use a
cleanup.

I'll fix this after your patch goes in.

Paul> +static void
Paul> +dwarf2_read_section (struct objfile *objfile, struct dwarf2_section_info *info)

[...]

Paul> +#ifdef HAVE_MMAP

[...]

Paul> +	{
Paul> +	  info->was_mmaped = 1;
Paul> +	  info->buffer = retbuf + (sectp->filepos & (pagesize - 1)) ;
Paul> +	  return;
Paul> +	}

[...]

Paul> +  /* When debugging .o files, we may need to apply relocations; see
Paul> +     http://sourceware.org/ml/gdb-patches/2002-04/msg00136.html .
Paul> +     We never compress sections in .o files, so we only need to
Paul> +     try this when the section is not compressed.  */
Paul> +  retbuf = symfile_relocate_debug_section (abfd, sectp, buf);

If we're using mmap then we seem to skip this relocation step.
This seems like a bug.

IIUC, this is an unusual case.  So perhaps one fix would be to
introduce a new symfile_debug_section_needs_relocate_p, then protect
the mmap branch with that.

Paul> +static void
Paul> +munmap_section_buffer (struct dwarf2_section_info *info)

[...]

Paul> +      gdb_assert (munmap ((void *)map_begin, map_length) == 0);

Space after the ")" of the cast to void *.

Tom