On Thu, 2012-07-19 at 11:56 +0200, Mark Wielaard wrote:
On Wed, 2012-07-18 at 16:17 -0700, Roland McGrath wrote:
> This seems vaguely reasonable to me, but OTOH since in the mmap case we do
> eagerly set up the shdr pointers in file_read_elf, I think it makes sense
> to be consistent and do that in __libelf_readall too. It should also save
> a little memory to not malloc the ehdr and shdr copies. That means that
> __libelf_readall really ought to free the old ehdr copy (which is always
> cached) and the shdr copies if they were cached before. Most of the code
> can just be broken out of file_read_elf into a subroutine,
> e.g. __libelf_maybe_precache_headers. __libelf_readall will just need to
> free the old state and clear the shdr_malloced flag.
I thought about that, but was afraid about other (concurrent) users of
the already allocated data if we freed it and swapped in a new mmaped
version. So I decided to just keep what we have and only copy in the new
shdr data in load_shdr_wrlock () when requested. Do you think the memory
savings are significant enough to risk data races? It can probably be
done safely, but does require careful analysis for IMHO little gain.
Also note the somewhat related item in the TODO file:
** shdrs in read-only files
When reading (ELF_C_READ*) then there is no need to malloc Shdr
structure in elfXX_getshdr if file is mmaped and unaligned access
is allowed or the structure is aligned. Use ELF_F_MALLOCED flag
to differentiate.