[PATCH for-8.2 0/2] ppc: get rid of free() (gitlab #1798)

[PATCH for-8.2 0/2] ppc: get rid of free() (gitlab #1798)

[Daniel Henrique Barboza]

Hello,

Here's some trivial changes following Peter's call to arms against
free() and friends in gitlab issue #1798 in an attempt to enforce
our memory management guidelines [1].

We only have 2 "free()" occurences that needs fixing in the ppc tree.
The hard part is to be dilligent to to not introduce new ones.

Michael, feel free to take it via qemu-trivial.


[1] https://www.qemu.org/docs/master/devel/style.html#low-level-memory-management 

Daniel Henrique Barboza (2):
  hw/ppc: use g_free() in spapr_tce_table_post_load()
  target/ppc: use g_free() in test_opcode_table()

 hw/ppc/spapr_iommu.c   | 2 +-
 target/ppc/translate.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.41.0

[PATCH for-8.2 1/2] hw/ppc: use g_free() in spapr_tce_table_post_load()

[Daniel Henrique Barboza]

tcet->mig_table is memcpy'ed from tcet->table, which in turn is created
via spapr_tce_alloc_table().

Use g_free() instead of free() to deallocate it.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index 63e34d457a..5e3973fc5f 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -248,7 +248,7 @@ static int spapr_tce_table_post_load(void *opaque, int version_id)
         memcpy(tcet->table, tcet->mig_table,
                tcet->nb_table * sizeof(tcet->table[0]));
 
-        free(tcet->mig_table);
+        g_free(tcet->mig_table);
         tcet->mig_table = NULL;
     }
 
-- 
2.41.0

[PATCH for-8.2 2/2] target/ppc: use g_free() in test_opcode_table()

[Daniel Henrique Barboza]

Use g_free(table[i]) instead of free(table[i]) to comply with QEMU low
level memory management guidelines.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 target/ppc/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index e6a0709066..d90535266e 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7129,7 +7129,7 @@ static int test_opcode_table(opc_handler_t **table, int len)
                 tmp = test_opcode_table(ind_table(table[i]),
                     PPC_CPU_INDIRECT_OPCODES_LEN);
                 if (tmp == 0) {
-                    free(table[i]);
+                    g_free(table[i]);
                     table[i] = &invalid_handler;
                 } else {
                     count++;
-- 
2.41.0

Re: [PATCH for-8.2 1/2] hw/ppc: use g_free() in spapr_tce_table_post_load()

[Peter Maydell]

On Fri, 28 Jul 2023 at 21:15, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
> tcet->mig_table is memcpy'ed from tcet->table,

The pointer is just copied, not memcpy'd.

> which in turn is created
> via spapr_tce_alloc_table().

You could mention that this uses g_new0() for the allocation.

> Use g_free() instead of free() to deallocate it.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/ppc/spapr_iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 63e34d457a..5e3973fc5f 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -248,7 +248,7 @@ static int spapr_tce_table_post_load(void *opaque, int version_id)
>          memcpy(tcet->table, tcet->mig_table,
>                 tcet->nb_table * sizeof(tcet->table[0]));
>
> -        free(tcet->mig_table);
> +        g_free(tcet->mig_table);
>          tcet->mig_table = NULL;
>      }

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH for-8.2 2/2] target/ppc: use g_free() in test_opcode_table()

[Peter Maydell]

On Fri, 28 Jul 2023 at 21:47, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
> Use g_free(table[i]) instead of free(table[i]) to comply with QEMU low
> level memory management guidelines.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  target/ppc/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index e6a0709066..d90535266e 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7129,7 +7129,7 @@ static int test_opcode_table(opc_handler_t **table, int len)
>                  tmp = test_opcode_table(ind_table(table[i]),
>                      PPC_CPU_INDIRECT_OPCODES_LEN);
>                  if (tmp == 0) {
> -                    free(table[i]);
> +                    g_free(table[i]);
>                      table[i] = &invalid_handler;
>                  } else {
>                      count++;

Where is the allocation that this memory is free()ing? I
think it is the g_new() in create_new_table(), but the code
is a little complicated for me to understand...

thanks
-- PMM

Re: [PATCH for-8.2 0/2] ppc: get rid of free() (gitlab #1798)

[Peter Maydell]

On Fri, 28 Jul 2023 at 21:57, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
> Here's some trivial changes following Peter's call to arms against
> free() and friends in gitlab issue #1798 in an attempt to enforce
> our memory management guidelines [1].

To clarify, this isn't a "call to arms". The issue is marked up as
a "bite-sized task", which is to say that it's a potential easy
place to start for newcomers to the community who might be making
their first contribution to the codebase. The changes it suggests
aren't urgent; at most they're a nice-to-have, since glib
guarantees that you can mix malloc/free and g_malloc/g_free.

We've had this sitting around as a suggestion on the wiki page
for bite-sized-tasks for years, and occasionally people come
through and have a go at it. I wanted to clean up and expand
on the description of what we had in mind for the change, to
give those people a better chance of successfully completing
the task.

thanks
-- PMM

Re: [PATCH for-8.2 2/2] target/ppc: use g_free() in test_opcode_table()

[Daniel Henrique Barboza]

On 7/29/23 12:32, Peter Maydell wrote:
> On Fri, 28 Jul 2023 at 21:47, Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
>>
>> Use g_free(table[i]) instead of free(table[i]) to comply with QEMU low
>> level memory management guidelines.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   target/ppc/translate.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index e6a0709066..d90535266e 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -7129,7 +7129,7 @@ static int test_opcode_table(opc_handler_t **table, int len)
>>                   tmp = test_opcode_table(ind_table(table[i]),
>>                       PPC_CPU_INDIRECT_OPCODES_LEN);
>>                   if (tmp == 0) {
>> -                    free(table[i]);
>> +                    g_free(table[i]);
>>                       table[i] = &invalid_handler;
>>                   } else {
>>                       count++;
> 
> Where is the allocation that this memory is free()ing? I
> think it is the g_new() in create_new_table(), but the code
> is a little complicated for me to understand...

It's on create_new_table() in the same file:

static int create_new_table(opc_handler_t **table, unsigned char idx)
{
     opc_handler_t **tmp;

     tmp = g_new(opc_handler_t *, PPC_CPU_INDIRECT_OPCODES_LEN);
     fill_new_table(tmp, PPC_CPU_INDIRECT_OPCODES_LEN);
     table[idx] = (opc_handler_t *)((uintptr_t)tmp | PPC_INDIRECT);

     return 0;
}


I probably should've mentioned in the commit msg ...


Daniel


> 
> thanks
> -- PMM

Re: [PATCH for-8.2 0/2] ppc: get rid of free() (gitlab #1798)

[Daniel Henrique Barboza]

On 7/29/23 12:35, Peter Maydell wrote:
> On Fri, 28 Jul 2023 at 21:57, Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
>> Here's some trivial changes following Peter's call to arms against
>> free() and friends in gitlab issue #1798 in an attempt to enforce
>> our memory management guidelines [1].
> 
> To clarify, this isn't a "call to arms". The issue is marked up as
> a "bite-sized task", which is to say that it's a potential easy
> place to start for newcomers to the community who might be making
> their first contribution to the codebase. The changes it suggests
> aren't urgent; at most they're a nice-to-have, since glib
> guarantees that you can mix malloc/free and g_malloc/g_free.

I failed to realized it was a byte sized task :/ and my Coccinelle comment
in the bug makes me fell dumb hehe (given that Coccinelle is not newcomer
friendly).

> 
> We've had this sitting around as a suggestion on the wiki page
> for bite-sized-tasks for years, and occasionally people come
> through and have a go at it. I wanted to clean up and expand
> on the description of what we had in mind for the change, to
> give those people a better chance of successfully completing
> the task.

What we can do then, since I already sent these, is perhaps link these patches
as example/template in the gitlab issue later on.


Thanks,

Daniel

> 
> thanks
> -- PMM

Re: [PATCH for-8.2 0/2] ppc: get rid of free() (gitlab #1798)

[Michael Tokarev]

30.07.2023 20:13, Daniel Henrique Barboza wrote:
> 
> 
> On 7/29/23 12:35, Peter Maydell wrote:
>> On Fri, 28 Jul 2023 at 21:57, Daniel Henrique Barboza
>> <danielhb413@gmail.com> wrote:
>>> Here's some trivial changes following Peter's call to arms against
>>> free() and friends in gitlab issue #1798 in an attempt to enforce
>>> our memory management guidelines [1].
>>
>> To clarify, this isn't a "call to arms". The issue is marked up as
>> a "bite-sized task", which is to say that it's a potential easy
>> place to start for newcomers to the community who might be making
>> their first contribution to the codebase. The changes it suggests
>> aren't urgent; at most they're a nice-to-have, since glib
>> guarantees that you can mix malloc/free and g_malloc/g_free.
> 
> I failed to realized it was a byte sized task :/ and my Coccinelle comment
> in the bug makes me fell dumb hehe (given that Coccinelle is not newcomer
> friendly).
> 
>>
>> We've had this sitting around as a suggestion on the wiki page
>> for bite-sized-tasks for years, and occasionally people come
>> through and have a go at it. I wanted to clean up and expand
>> on the description of what we had in mind for the change, to
>> give those people a better chance of successfully completing
>> the task.
> 
> What we can do then, since I already sent these, is perhaps link these patches
> as example/template in the gitlab issue later on.

Applied to my trivial-patches branch adding suggested commit comment
fixes while at it, hopefully there's nothing more to do :)

Thanks,

/mjt