| Message ID | 20230418192046.3235870-1-trix@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | accel/qaic: initialize ret variable to 0 | expand |
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 <trix@redhat.com> > --- > 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 <quic_jhugo@quicinc.com> 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 <trix@redhat.com> > > --- > > 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 4/18/2023 2:48 PM, Nick Desaulniers wrote: > On Tue, Apr 18, 2023 at 1:46 PM Jeffrey Hugo <quic_jhugo@quicinc.com> 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 <trix@redhat.com> >>> --- >>> 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; >> > >
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;
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 <trix@redhat.com> --- drivers/accel/qaic/qaic_data.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)