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

Tom Tromey
2009-06-16T19:19:25Z

>>>>> "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