clang static analysis reports
drivers/accel/qaic/qaic_data.c:610:2: warning: Undefined or garbage
value returned to caller [core.uninitialized.UndefReturn]
return ret;
^~~~~~~~~~
The ret variable is only set some of the time but is always returned.
So initialize ret to 0.
Fixes: ff13be830333 ("accel/qaic: Add datapath")
Signed-off-by: Tom Rix <[email protected]>
---
drivers/accel/qaic/qaic_data.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
index c0a574cd1b35..b46a16fb3080 100644
--- a/drivers/accel/qaic/qaic_data.c
+++ b/drivers/accel/qaic/qaic_data.c
@@ -591,7 +591,7 @@ static int qaic_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struc
struct qaic_bo *bo = to_qaic_bo(obj);
unsigned long offset = 0;
struct scatterlist *sg;
- int ret;
+ int ret = 0;
if (obj->import_attach)
return -EINVAL;
--
2.27.0
On 4/18/2023 1:20 PM, Tom Rix wrote:
> clang static analysis reports
> drivers/accel/qaic/qaic_data.c:610:2: warning: Undefined or garbage
> value returned to caller [core.uninitialized.UndefReturn]
> return ret;
> ^~~~~~~~~~
>
> The ret variable is only set some of the time but is always returned.
> So initialize ret to 0.
This does not appear to be entirely accurate to me.
Do you know what analysis Clang is doing? Is it limited to the function
itself?
remap_pfn_range, which initializes ret, will always run at-least once.
Feels more accurate to say that Clang cannot detect that ret will be
initialized, and we want to quiet the warning from the false positive.
> Fixes: ff13be830333 ("accel/qaic: Add datapath")
> Signed-off-by: Tom Rix <[email protected]>
> ---
> drivers/accel/qaic/qaic_data.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
> index c0a574cd1b35..b46a16fb3080 100644
> --- a/drivers/accel/qaic/qaic_data.c
> +++ b/drivers/accel/qaic/qaic_data.c
> @@ -591,7 +591,7 @@ static int qaic_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struc
> struct qaic_bo *bo = to_qaic_bo(obj);
> unsigned long offset = 0;
> struct scatterlist *sg;
> - int ret;
> + int ret = 0;
>
> if (obj->import_attach)
> return -EINVAL;
On Tue, Apr 18, 2023 at 1:46 PM Jeffrey Hugo <[email protected]> wrote:
>
> On 4/18/2023 1:20 PM, Tom Rix wrote:
> > clang static analysis reports
> > drivers/accel/qaic/qaic_data.c:610:2: warning: Undefined or garbage
> > value returned to caller [core.uninitialized.UndefReturn]
> > return ret;
> > ^~~~~~~~~~
> >
> > The ret variable is only set some of the time but is always returned.
> > So initialize ret to 0.
>
> This does not appear to be entirely accurate to me.
>
> Do you know what analysis Clang is doing? Is it limited to the function
> itself?
>
> remap_pfn_range, which initializes ret, will always run at-least once.
What happens if the loop body is never executed, say if `bo->sgt->sgl` is NULL?
>
> Feels more accurate to say that Clang cannot detect that ret will be
> initialized, and we want to quiet the warning from the false positive.
>
> > Fixes: ff13be830333 ("accel/qaic: Add datapath")
> > Signed-off-by: Tom Rix <[email protected]>
> > ---
> > drivers/accel/qaic/qaic_data.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
> > index c0a574cd1b35..b46a16fb3080 100644
> > --- a/drivers/accel/qaic/qaic_data.c
> > +++ b/drivers/accel/qaic/qaic_data.c
> > @@ -591,7 +591,7 @@ static int qaic_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struc
> > struct qaic_bo *bo = to_qaic_bo(obj);
> > unsigned long offset = 0;
> > struct scatterlist *sg;
> > - int ret;
> > + int ret = 0;
> >
> > if (obj->import_attach)
> > return -EINVAL;
>
--
Thanks,
~Nick Desaulniers
On 4/18/2023 2:48 PM, Nick Desaulniers wrote:
> On Tue, Apr 18, 2023 at 1:46 PM Jeffrey Hugo <[email protected]> wrote:
>>
>> On 4/18/2023 1:20 PM, Tom Rix wrote:
>>> clang static analysis reports
>>> drivers/accel/qaic/qaic_data.c:610:2: warning: Undefined or garbage
>>> value returned to caller [core.uninitialized.UndefReturn]
>>> return ret;
>>> ^~~~~~~~~~
>>>
>>> The ret variable is only set some of the time but is always returned.
>>> So initialize ret to 0.
>>
>> This does not appear to be entirely accurate to me.
>>
>> Do you know what analysis Clang is doing? Is it limited to the function
>> itself?
>>
>> remap_pfn_range, which initializes ret, will always run at-least once.
>
> What happens if the loop body is never executed, say if `bo->sgt->sgl` is NULL?
qaic_gem_object_mmap() doesn't get called unless a valid GEM object is
provided by userspace. For userspace to get a valid GEM object, it has
to go through qaic_create_bo_ioctl(). qaic_create_bo_ioctl() will not
return a valid GEM object unless sgt is allocated and initialized.
The loop body will execute at-least once. The if body will execute
at-least once. remap_pfn_range() will run at-least once. Therefore,
ret is always initialized.
This is how the code is intended to operate, and how it appears to be
implemented from what I see. Unless I'm missing something.
I can see how Clang might not be able to infer these things, but I don't
believe the code is broken. I feel that the commit text is unclear and
suggests that the code is infact, broken.
Userspace should not call mmap() in a critical path thus I don't see a
true performance concern here. So while my understanding of the coding
style is that redundant initialization of variables are to be avoided, I
think we can say that this is not redundant because it silences a
warning (because Clang is limited).
>> Feels more accurate to say that Clang cannot detect that ret will be
>> initialized, and we want to quiet the warning from the false positive.
>>
>>> Fixes: ff13be830333 ("accel/qaic: Add datapath")
>>> Signed-off-by: Tom Rix <[email protected]>
>>> ---
>>> drivers/accel/qaic/qaic_data.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
>>> index c0a574cd1b35..b46a16fb3080 100644
>>> --- a/drivers/accel/qaic/qaic_data.c
>>> +++ b/drivers/accel/qaic/qaic_data.c
>>> @@ -591,7 +591,7 @@ static int qaic_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struc
>>> struct qaic_bo *bo = to_qaic_bo(obj);
>>> unsigned long offset = 0;
>>> struct scatterlist *sg;
>>> - int ret;
>>> + int ret = 0;
>>>
>>> if (obj->import_attach)
>>> return -EINVAL;
>>
>
>