On 26.11.2012, at 02:53, Yin Olivia-R63875 wrote:
> Hi Stuart & Alex,
>
> "syms" and "str" could not be free since "symbols" is a global variable which
> need stay in the memory during the whole life of VM. So it will not be free
> and
> reload when reset.
Ah, that's used for the debug log output, right?
> The only change is to the previous patch of elf loader (hw/elf_ops.h) as
> below:
>
> @@ -232,7 +235,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
> if (pentry)
> *pentry = (uint64_t)(elf_sword)ehdr.e_entry;
>
> - glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
> + if (!reset) {
> + glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
> + }
>
> size = ehdr.e_phnum * sizeof(phdr[0]);
> lseek(fd, ehdr.e_phoff, SEEK_SET);
>
> Do you think it is reasonable?
I think semantically we want to only load the symbols the first time we load
the binary (read: not on reset), yes. Where does the reset variable you're
using there come from?
Alex
>
> Best Regards,
> Olivia
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:ag...@suse.de]
>> Sent: Thursday, November 22, 2012 2:42 AM
>> To: Stuart Yoder
>> Cc: Yin Olivia-R63875; qemu-devel@nongnu.org; qemu-...@nongnu.org
>> Subject: Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by
>> load_at()
>>
>> On 11/21/2012 07:39 PM, Stuart Yoder wrote:
>>> On Wed, Nov 21, 2012 at 8:38 AM, Olivia Yin<hong-hua....@freescale.com>
>> wrote:
>>>> Signed-off-by: Olivia Yin<hong-hua....@freescale.com>
>>>> ---
>>>> hw/elf_ops.h | 2 ++
>>>> 1 files changed, 2 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/hw/elf_ops.h b/hw/elf_ops.h index b346861..9c76a75
>>>> 100644
>>>> --- a/hw/elf_ops.h
>>>> +++ b/hw/elf_ops.h
>>>> @@ -178,6 +178,8 @@ static int glue(load_symbols, SZ)(struct elfhdr
>> *ehdr, int fd, int must_swab,
>>>> s->disas_strtab = str;
>>>> s->next = syminfos;
>>>> syminfos = s;
>>>> + g_free(syms);
>>>> + g_free(str);
>>>> g_free(shdr_table);
>>>> return 0;
>>>> fail:
>>> Olivia, as Alex pointed out there are references to syms and str in
>>> the struct "s"....so you can't just free those I don't think.
>>>
>>> The problem that leaves us with is that on every reset when we call
>>> load_elf() that we re-load and re-malloc space for the symbols.
>>>
>>> I think the solution may be to factor out the call to load_symbols()
>>> from load_elf(). It looks like what load_symbols does in the end is
>>> set the variable syminfos to point to the loaded symbol info.
>>>
>>> If you factor load_symbols() out then in load_elf_32/64() you would do
>>> something like:
>>> elf_phy_loader_32/64()
>>> load_symbols_32/64().
>>>
>>> We don't need to be reloading symbols on every reset.
>>>
>>> Alex, does that make sense?
>>
>> We can also mandate the caller of load_symbols to free the respective
>> data :)
>>
>>
>> Alex
>>
>
>
