[PATCH] softmmu/physmem: Hint notifier is not NULL in as_translate_for_iotlb()

[PATCH] softmmu/physmem: Hint notifier is not NULL in as_translate_for_iotlb()

[Philippe Mathieu-Daudé]

When using GCC 10.2 configured with --extra-cflags=-Os, we get:

  softmmu/physmem.c: In function ‘address_space_translate_for_iotlb’:
  softmmu/physmem.c:643:26: error: ‘notifier’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
    643 |         notifier->active = true;
        |                          ^
  softmmu/physmem.c:608:23: note: ‘notifier’ was declared here
    608 |     TCGIOMMUNotifier *notifier;
        |                       ^~~~~~~~

Insert assertions as hint to the compiler that 'notifier' can
not be NULL there.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Yet another hole in our CI.
---
 softmmu/physmem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 6301f4f0a5c..65602ed548e 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -605,7 +605,7 @@ static void tcg_register_iommu_notifier(CPUState *cpu,
      * when the IOMMU tells us the mappings we've cached have changed.
      */
     MemoryRegion *mr = MEMORY_REGION(iommu_mr);
-    TCGIOMMUNotifier *notifier;
+    TCGIOMMUNotifier *notifier = NULL;
     int i;
 
     for (i = 0; i < cpu->iommu_notifiers->len; i++) {
@@ -638,6 +638,7 @@ static void tcg_register_iommu_notifier(CPUState *cpu,
         memory_region_register_iommu_notifier(notifier->mr, &notifier->n,
                                               &error_fatal);
     }
+    assert(notifier != NULL);
 
     if (!notifier->active) {
         notifier->active = true;
-- 
2.26.2

Re: [PATCH] softmmu/physmem: Hint notifier is not NULL in as_translate_for_iotlb()

[Peter Maydell]

On Sun, 17 Jan 2021 at 16:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> When using GCC 10.2 configured with --extra-cflags=-Os, we get:
>
>   softmmu/physmem.c: In function ‘address_space_translate_for_iotlb’:
>   softmmu/physmem.c:643:26: error: ‘notifier’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>     643 |         notifier->active = true;
>         |                          ^
>   softmmu/physmem.c:608:23: note: ‘notifier’ was declared here
>     608 |     TCGIOMMUNotifier *notifier;
>         |                       ^~~~~~~~
>
> Insert assertions as hint to the compiler that 'notifier' can
> not be NULL there.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Yet another hole in our CI.
> ---
>  softmmu/physmem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 6301f4f0a5c..65602ed548e 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -605,7 +605,7 @@ static void tcg_register_iommu_notifier(CPUState *cpu,
>       * when the IOMMU tells us the mappings we've cached have changed.
>       */
>      MemoryRegion *mr = MEMORY_REGION(iommu_mr);
> -    TCGIOMMUNotifier *notifier;
> +    TCGIOMMUNotifier *notifier = NULL;
>      int i;
>
>      for (i = 0; i < cpu->iommu_notifiers->len; i++) {
> @@ -638,6 +638,7 @@ static void tcg_register_iommu_notifier(CPUState *cpu,
>          memory_region_register_iommu_notifier(notifier->mr, &notifier->n,
>                                                &error_fatal);
>      }
> +    assert(notifier != NULL);
>
>      if (!notifier->active) {
>          notifier->active = true;

Is the assert() necessary to prevent the compiler complaining?
Usually we don't bother if it's about to be dereferenced anyway.

thanks
-- PMM

Re: [PATCH] softmmu/physmem: Hint notifier is not NULL in as_translate_for_iotlb()

[Philippe Mathieu-Daudé]

On 1/17/21 5:47 PM, Peter Maydell wrote:
> On Sun, 17 Jan 2021 at 16:07, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> When using GCC 10.2 configured with --extra-cflags=-Os, we get:
>>
>>   softmmu/physmem.c: In function ‘address_space_translate_for_iotlb’:
>>   softmmu/physmem.c:643:26: error: ‘notifier’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>     643 |         notifier->active = true;
>>         |                          ^
>>   softmmu/physmem.c:608:23: note: ‘notifier’ was declared here
>>     608 |     TCGIOMMUNotifier *notifier;
>>         |                       ^~~~~~~~
>>
>> Insert assertions as hint to the compiler that 'notifier' can
>> not be NULL there.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> Yet another hole in our CI.
>> ---
>>  softmmu/physmem.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>> index 6301f4f0a5c..65602ed548e 100644
>> --- a/softmmu/physmem.c
>> +++ b/softmmu/physmem.c
>> @@ -605,7 +605,7 @@ static void tcg_register_iommu_notifier(CPUState *cpu,
>>       * when the IOMMU tells us the mappings we've cached have changed.
>>       */
>>      MemoryRegion *mr = MEMORY_REGION(iommu_mr);
>> -    TCGIOMMUNotifier *notifier;
>> +    TCGIOMMUNotifier *notifier = NULL;
>>      int i;
>>
>>      for (i = 0; i < cpu->iommu_notifiers->len; i++) {
>> @@ -638,6 +638,7 @@ static void tcg_register_iommu_notifier(CPUState *cpu,
>>          memory_region_register_iommu_notifier(notifier->mr, &notifier->n,
>>                                                &error_fatal);
>>      }
>> +    assert(notifier != NULL);
>>
>>      if (!notifier->active) {
>>          notifier->active = true;
> 
> Is the assert() necessary to prevent the compiler complaining?
> Usually we don't bother if it's about to be dereferenced anyway.

Yes you are right, the assert() is not necessary. Simply initializing
the value silents the error.

Regards,

Phil.