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.
I decided to go with the original assert-relaxation approach and not do
the optimization at this time. I did add the suggestion to the TODO
file. Any volunteers? I did make all other changes suggested by Roland.
Patch as committed and pushed attached.
Cheers,
Mark