Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by load_at()

[Alexander Graf]

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:address@hidden
>> Sent: Thursday, November 22, 2012 2:42 AM
>> To: Stuart Yoder
>> Cc: Yin Olivia-R63875; address@hidden; address@hidden
>> 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<address@hidden>
>> wrote:
>>>> Signed-off-by: Olivia Yin<address@hidden>
>>>> ---
>>>>  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
>> 
> 
>