Re: [PATCH v4 4/6] elf: Add _bfd_elf_mmap_section and _bfd_elf_munmap_section_contents

[Alan Modra <amodra@gmail.com>]

On Wed, Mar 06, 2024 at 03:23:59PM -0800, H.J. Lu wrote:
> +void
> +_bfd_elf_munmap_section_contents (asection *sec ATTRIBUTE_UNUSED,
> +				  void *contents)
> +{
> +  /* NB: Since _bfd_elf_munmap_section_contents is called like free,
> +     CONTENTS may be NULL.  */
> +  if (contents == NULL)
> +    return;
> +
> +#ifdef USE_MMAP
> +  if (sec->mmapped_p)
> +    {
> +      /* NB: Don't free CONTENTS if it has been cached.  */
> +      if (elf_section_data (sec)->this_hdr.contents == contents)
> +	return;

Why the above test?  This function must have such a test before it is
called if the contents might be cached.  Hmm, unless you have some
place where contents are cached only when mmapped?  (I didn't see such
a case.)

> +      /* When bfd_mmap_local returns (void *) -1 on an unknown input,
> +	 CONTENTS is malloced and CONTENTS_ADDR is set to NULL.  */

The above comment adds confusion.  As far as I can see there isn't a
case where bfd_mmap_local returns -1 causing contents to be malloc'd.
Delete the comment, or mention _bfd_elf_mmap_section behaviour here.
BTW, why does this function have _contents in the name while
_bfd_elf_mmap_section does not?

> +      if (elf_section_data (sec)->contents_addr != NULL)
> +	{
> +	  /* NB: CONTENTS_ADDR and CONTENTS_SIZE must be valid.  */
> +	  if (munmap (elf_section_data (sec)->contents_addr,
> +		      elf_section_data (sec)->contents_size) != 0)
> +	    abort ();
> +	  sec->mmapped_p = 0;
> +	  sec->contents = NULL;
> +	  elf_section_data (sec)->contents_addr = NULL;
> +	  elf_section_data (sec)->contents_size = 0;
> +	  return;
> +	}
> +    }
> +#endif
> +
> +  free (contents);
> +}

> diff --git a/bfd/libbfd.c b/bfd/libbfd.c
> index 237f91e5c97..c847c4f0180 100644
> --- a/bfd/libbfd.c
> +++ b/bfd/libbfd.c
> @@ -21,6 +21,7 @@
>  
>  #include "sysdep.h"
>  #include "bfd.h"
> +#include "elf-bfd.h"
>  #include "libbfd.h"
>  #include "objalloc.h"
>  
> @@ -1202,6 +1203,19 @@ _bfd_generic_get_section_contents (bfd *abfd,
>        return false;
>      }
>  
> +#ifdef USE_MMAP
> +  if (section->mmapped_p
> +      && (section->contents != NULL || location != NULL))
> +    {
> +      _bfd_error_handler
> +	/* xgettext:c-format */
> +	(_("%pB: mapped section %pA has non-NULLL buffer"),

typo

> +	 abfd, section);
> +      bfd_set_error (bfd_error_invalid_operation);
> +      return false;
> +    }
> +#endif
> +
>    sz = bfd_get_section_limit_octets (abfd, section);
>    if (offset + count < count
>        || offset + count > sz
> @@ -1214,8 +1228,49 @@ _bfd_generic_get_section_contents (bfd *abfd,
>        return false;
>      }
>  
> -  if (bfd_seek (abfd, section->filepos + offset, SEEK_SET) != 0
> -      || bfd_read (location, count, abfd) != count)
> +  if (bfd_seek (abfd, section->filepos + offset, SEEK_SET) != 0)
> +    return false;
> +
> +#ifdef USE_MMAP
> +  if (section->mmapped_p)
> +    {
> +      if (location != 0
> +	  || bfd_get_flavour (abfd) != bfd_target_elf_flavour)
> +	abort ();
> +
> +      int prot = ((section->reloc_count == 0)
> +		  ? PROT_READ : PROT_READ | PROT_WRITE);
> +
> +      location = bfd_mmap_local
> +	(abfd, count, prot, &elf_section_data (section)->contents_addr,
> +	 &elf_section_data (section)->contents_size);
> +
> +      if (location == NULL)
> +	return false;
> +
> +      /* Check for the unknown input file.  */

Not "the unknown input file" but rather "iovec not supporting mmap".

> +      if (location != (void *) -1)
> +	{
> +	  section->contents = location;
> +	  return true;
> +	}
> +
> +      /* Malloc the buffer and call bfd_read.  */
> +      location = (bfd_byte *) bfd_malloc (count);
> +      if (location == NULL)
> +	{
> +	  if (bfd_get_error () == bfd_error_no_memory)
> +	    _bfd_error_handler
> +	      /* xgettext:c-format */
> +	      (_("error: %pB(%pA) is too large (%#" PRIx64 " bytes)"),
> +	       abfd, section, (uint64_t) count);
> +	  return false;
> +	}
> +      section->contents = location;
> +    }
> +#endif
> +
> +  if (bfd_read (location, count, abfd) != count)
>      return false;
>  
>    return true;

OK with those things fixed.

-- 
Alan Modra
Australia Development Lab, IBM