| Message ID | 20260527171041.110173-1-simonparri@ganzeria.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Simon, Thank you for your patch, Quoting Simon Parri (2026-05-27 18:10:37) > The function PiSPCameraData::beOutputDequeue in pipelines/rpi/pisp.cpp > contains a variable "index" that is initialized in the loop along with the > variable "stream". When -ftree-vrp is not enabled, GCC cannot analyze the > code to determine that after "ASSERT(stream)", "index" will always be > initialized. Since it cannot determine that "index" will always be > initialized, it emits a warning, which is elevated to an error. > > If the rpi/pisp pipeline is to be built, and the compiler is GCC, add > -ftree-vrp to project arguments for C++. > > This is not needed for Clang, since Clang always does the appropriate level of > code analysis (even at -O0). > --- > > Notes: > I would like advice on whether using "add_project_arguments" is the correct > thing to do here. It feels somewhat wrong to me because all the other > flags (as far as I can tell) are set via common_arguments / c_arguments / > cpp_arguments. However, since I need to check the enabled pipelines, I put the check > after the code that sets the "pipelines" variable. Indeed, I don't think I would tackle this through a global flag. This will now change how all parts are built depending on whether RPi is included. > Or is my approach wrong to begin with? Should I be adjusting the code itself > instead of adding compiler flags? In my opinion, adding compiler flags is the > correct approach, since the code already works as-is with Clang. I haven't looked at if the code itself could be updated, but at least I would say that if we need a specific compile flag it should only be associated against the single file impacted ideally. Perhaps at least we could push this down into the pipeline handler meson.build itself. I think src/libcamera/pipeline/rpi/pisp/meson.build should be the right place and this file wouldn't be parsed if the pisp isn't enabled. -- Regards Kieran > > Thank you for your attention. > > meson.build | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/meson.build b/meson.build > index 9cd73462f..0344c55bb 100644 > --- a/meson.build > +++ b/meson.build > @@ -241,6 +241,10 @@ else > pipelines = wanted_pipelines > endif > > +if cc.get_id() == 'gcc' and pipelines.contains('rpi/pisp') > + add_project_arguments('-ftree-vrp', language : 'cpp') > +endif > + > # Tests require the vimc pipeline handler, include it automatically when tests > # are enabled. > if get_option('test') > -- > 2.51.2 >
Hi Kieran, Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Hi Simon, > > Thank you for your patch, Thank you for taking a look at it. > Quoting Simon Parri (2026-05-27 18:10:37) >> The function PiSPCameraData::beOutputDequeue in pipelines/rpi/pisp.cpp >> contains a variable "index" that is initialized in the loop along with the >> variable "stream". When -ftree-vrp is not enabled, GCC cannot analyze the >> code to determine that after "ASSERT(stream)", "index" will always be >> initialized. Since it cannot determine that "index" will always be >> initialized, it emits a warning, which is elevated to an error. >> >> If the rpi/pisp pipeline is to be built, and the compiler is GCC, add >> -ftree-vrp to project arguments for C++. >> >> This is not needed for Clang, since Clang always does the appropriate level of >> code analysis (even at -O0). >> --- >> >> Notes: >> I would like advice on whether using "add_project_arguments" is the correct >> thing to do here. It feels somewhat wrong to me because all the other >> flags (as far as I can tell) are set via common_arguments / c_arguments / >> cpp_arguments. However, since I need to check the enabled pipelines, I put the check >> after the code that sets the "pipelines" variable. > > Indeed, I don't think I would tackle this through a global flag. This > will now change how all parts are built depending on whether RPi is > included. Ah, I see I forgot to make it clear that the patch only enables the flag if rpi/pisp is in the list of pipelines to be built. I agree that it would not be appropriate to enable it unconditionally. >> Or is my approach wrong to begin with? Should I be adjusting the code itself >> instead of adding compiler flags? In my opinion, adding compiler flags is the >> correct approach, since the code already works as-is with Clang. > > I haven't looked at if the code itself could be updated, but at least I > would say that if we need a specific compile flag it should only be > associated against the single file impacted ideally. As far as I can tell, Meson does not support adding flags per file, only per project. Upstream suggests defining a separate static library for files that should have separate flags, which is not a great solution in my opinion. > Perhaps at least we could push this down into the pipeline handler > meson.build itself. > > I think src/libcamera/pipeline/rpi/pisp/meson.build should be the right > place and this file wouldn't be parsed if the pisp isn't enabled. Actually, that was the first place I tried to put it. It seems to me that adding it there is difficult, since by the time the pipeline handler's meson.build runs there are already targets declared, and meson does not allow adding flags when there are already targets declared. Should I try rearranging the subdir() calls in src/libcamera/build.meson to try to get src/libcamera/pipeline/rpi/pisp/meson.build to run before any targets are declared? I’m not sure how to go about that. Thanks again for the review. (Kieran, sorry for the double-send, I forgot to reply to all.) -- Simon Parri
Hi 2026. 05. 28. 17:01 keltezéssel, Simon Parri írta: > Hi Kieran, > > Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > >> Hi Simon, >> >> Thank you for your patch, > > Thank you for taking a look at it. > >> Quoting Simon Parri (2026-05-27 18:10:37) >>> The function PiSPCameraData::beOutputDequeue in pipelines/rpi/pisp.cpp >>> contains a variable "index" that is initialized in the loop along with the >>> variable "stream". When -ftree-vrp is not enabled, GCC cannot analyze the >>> code to determine that after "ASSERT(stream)", "index" will always be >>> initialized. Since it cannot determine that "index" will always be >>> initialized, it emits a warning, which is elevated to an error. >>> >>> If the rpi/pisp pipeline is to be built, and the compiler is GCC, add >>> -ftree-vrp to project arguments for C++. >>> >>> This is not needed for Clang, since Clang always does the appropriate level of >>> code analysis (even at -O0). >>> --- >>> >>> Notes: >>> I would like advice on whether using "add_project_arguments" is the correct >>> thing to do here. It feels somewhat wrong to me because all the other >>> flags (as far as I can tell) are set via common_arguments / c_arguments / >>> cpp_arguments. However, since I need to check the enabled pipelines, I put the check >>> after the code that sets the "pipelines" variable. >> >> Indeed, I don't think I would tackle this through a global flag. This >> will now change how all parts are built depending on whether RPi is >> included. > > Ah, I see I forgot to make it clear that the patch only enables the flag > if rpi/pisp is in the list of pipelines to be built. I agree that it > would not be appropriate to enable it unconditionally. > >>> Or is my approach wrong to begin with? Should I be adjusting the code itself >>> instead of adding compiler flags? In my opinion, adding compiler flags is the >>> correct approach, since the code already works as-is with Clang. >> >> I haven't looked at if the code itself could be updated, but at least I >> would say that if we need a specific compile flag it should only be >> associated against the single file impacted ideally. The code has been changed to avoid such warnings already multiple times: * https://patchwork.libcamera.org/patch/20454/ * https://patchwork.libcamera.org/patch/22997/ so I don't think that's is problematic. > > As far as I can tell, Meson does not support adding flags per file, only > per project. Upstream suggests defining a separate static library for > files that should have separate flags, which is not a great solution in > my opinion. > >> Perhaps at least we could push this down into the pipeline handler >> meson.build itself. >> >> I think src/libcamera/pipeline/rpi/pisp/meson.build should be the right >> place and this file wouldn't be parsed if the pisp isn't enabled. > > Actually, that was the first place I tried to put it. It seems to me > that adding it there is difficult, since by the time the pipeline > handler's meson.build runs there are already targets declared, and meson > does not allow adding flags when there are already targets declared. > > Should I try rearranging the subdir() calls in src/libcamera/build.meson > to try to get src/libcamera/pipeline/rpi/pisp/meson.build to run before > any targets are declared? I’m not sure how to go about that. If a compiler flag is to be added, I think the static library approach is better than trying to rearrange things so that `add_project_arguments()` can be done from the subdir at the appropriate time. > > Thanks again for the review. > > (Kieran, sorry for the double-send, I forgot to reply to all.) > -- > Simon Parri
Hello, Barnabás, Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > Hi > > 2026. 05. 28. 17:01 keltezéssel, Simon Parri írta: >> Hi Kieran, >> Kieran Bingham <kieran.bingham@ideasonboard.com> writes: >> >>> Hi Simon, >>> >>> Thank you for your patch, >> Thank you for taking a look at it. >> >>> Quoting Simon Parri (2026-05-27 18:10:37) >>>> The function PiSPCameraData::beOutputDequeue in pipelines/rpi/pisp.cpp >>>> contains a variable "index" that is initialized in the loop along with the >>>> variable "stream". When -ftree-vrp is not enabled, GCC cannot analyze the >>>> code to determine that after "ASSERT(stream)", "index" will always be >>>> initialized. Since it cannot determine that "index" will always be >>>> initialized, it emits a warning, which is elevated to an error. >>>> >>>> If the rpi/pisp pipeline is to be built, and the compiler is GCC, add >>>> -ftree-vrp to project arguments for C++. >>>> >>>> This is not needed for Clang, since Clang always does the appropriate level of >>>> code analysis (even at -O0). >>>> --- >>>> >>>> Notes: >>>> I would like advice on whether using "add_project_arguments" is the correct >>>> thing to do here. It feels somewhat wrong to me because all the other >>>> flags (as far as I can tell) are set via common_arguments / c_arguments / >>>> cpp_arguments. However, since I need to check the enabled pipelines, I put the check >>>> after the code that sets the "pipelines" variable. >>> >>> Indeed, I don't think I would tackle this through a global flag. This >>> will now change how all parts are built depending on whether RPi is >>> included. >> Ah, I see I forgot to make it clear that the patch only enables the flag >> if rpi/pisp is in the list of pipelines to be built. I agree that it >> would not be appropriate to enable it unconditionally. >> >>>> Or is my approach wrong to begin with? Should I be adjusting the code itself >>>> instead of adding compiler flags? In my opinion, adding compiler flags is the >>>> correct approach, since the code already works as-is with Clang. >>> >>> I haven't looked at if the code itself could be updated, but at least I >>> would say that if we need a specific compile flag it should only be >>> associated against the single file impacted ideally. > > The code has been changed to avoid such warnings already multiple times: > > * https://patchwork.libcamera.org/patch/20454/ > * https://patchwork.libcamera.org/patch/22997/ > > so I don't think that's is problematic. Ah, I see. In that case it does seem easier to simply initialize "index" in this case as well. Should it simply be initialized to 0? >> As far as I can tell, Meson does not support adding flags per file, only >> per project. Upstream suggests defining a separate static library for >> files that should have separate flags, which is not a great solution in >> my opinion. >> >>> Perhaps at least we could push this down into the pipeline handler >>> meson.build itself. >>> >>> I think src/libcamera/pipeline/rpi/pisp/meson.build should be the right >>> place and this file wouldn't be parsed if the pisp isn't enabled. >> Actually, that was the first place I tried to put it. It seems to me >> that adding it there is difficult, since by the time the pipeline >> handler's meson.build runs there are already targets declared, and meson >> does not allow adding flags when there are already targets declared. >> Should I try rearranging the subdir() calls in src/libcamera/build.meson >> to try to get src/libcamera/pipeline/rpi/pisp/meson.build to run before >> any targets are declared? I’m not sure how to go about that. > > If a compiler flag is to be added, I think the static library approach is > better than trying to rearrange things so that `add_project_arguments()` > can be done from the subdir at the appropriate time. If that’s the route to go I can look into it, but initializing the variable seems simpler to me. >> Thanks again for the review. >> (Kieran, sorry for the double-send, I forgot to reply to all.) >> -- >> Simon Parri -- Simon Parri
On Thu, Jun 04, 2026 at 11:35:32PM +0200, Simon Parri wrote: > Barnabás Pőcze writes: > > 2026. 05. 28. 17:01 keltezéssel, Simon Parri írta: > >> Kieran Bingham writes: > >>> Quoting Simon Parri (2026-05-27 18:10:37) > >>>> The function PiSPCameraData::beOutputDequeue in pipelines/rpi/pisp.cpp > >>>> contains a variable "index" that is initialized in the loop along with the > >>>> variable "stream". When -ftree-vrp is not enabled, GCC cannot analyze the > >>>> code to determine that after "ASSERT(stream)", "index" will always be > >>>> initialized. Since it cannot determine that "index" will always be > >>>> initialized, it emits a warning, which is elevated to an error. > >>>> > >>>> If the rpi/pisp pipeline is to be built, and the compiler is GCC, add > >>>> -ftree-vrp to project arguments for C++. > >>>> > >>>> This is not needed for Clang, since Clang always does the appropriate level of > >>>> code analysis (even at -O0). > >>>> --- > >>>> > >>>> Notes: > >>>> I would like advice on whether using "add_project_arguments" is the correct > >>>> thing to do here. It feels somewhat wrong to me because all the other > >>>> flags (as far as I can tell) are set via common_arguments / c_arguments / > >>>> cpp_arguments. However, since I need to check the enabled pipelines, I put the check > >>>> after the code that sets the "pipelines" variable. > >>> > >>> Indeed, I don't think I would tackle this through a global flag. This > >>> will now change how all parts are built depending on whether RPi is > >>> included. > >> Ah, I see I forgot to make it clear that the patch only enables the flag > >> if rpi/pisp is in the list of pipelines to be built. I agree that it > >> would not be appropriate to enable it unconditionally. > >> > >>>> Or is my approach wrong to begin with? Should I be adjusting the code itself > >>>> instead of adding compiler flags? In my opinion, adding compiler flags is the > >>>> correct approach, since the code already works as-is with Clang. > >>> > >>> I haven't looked at if the code itself could be updated, but at least I > >>> would say that if we need a specific compile flag it should only be > >>> associated against the single file impacted ideally. > > > > The code has been changed to avoid such warnings already multiple times: > > > > * https://patchwork.libcamera.org/patch/20454/ > > * https://patchwork.libcamera.org/patch/22997/ > > > > so I don't think that's is problematic. > > Ah, I see. In that case it does seem easier to simply initialize "index" > in this case as well. Should it simply be initialized to 0? Seems good to me. > >> As far as I can tell, Meson does not support adding flags per file, only > >> per project. Upstream suggests defining a separate static library for > >> files that should have separate flags, which is not a great solution in > >> my opinion. > >> > >>> Perhaps at least we could push this down into the pipeline handler > >>> meson.build itself. > >>> > >>> I think src/libcamera/pipeline/rpi/pisp/meson.build should be the right > >>> place and this file wouldn't be parsed if the pisp isn't enabled. > >> Actually, that was the first place I tried to put it. It seems to me > >> that adding it there is difficult, since by the time the pipeline > >> handler's meson.build runs there are already targets declared, and meson > >> does not allow adding flags when there are already targets declared. > >> Should I try rearranging the subdir() calls in src/libcamera/build.meson > >> to try to get src/libcamera/pipeline/rpi/pisp/meson.build to run before > >> any targets are declared? I’m not sure how to go about that. > > > > If a compiler flag is to be added, I think the static library approach is > > better than trying to rearrange things so that `add_project_arguments()` > > can be done from the subdir at the appropriate time. > > If that’s the route to go I can look into it, but initializing the > variable seems simpler to me. Yes, let's initialize the variable. Will you submit a patch ? > >> Thanks again for the review. > >> (Kieran, sorry for the double-send, I forgot to reply to all.)
diff --git a/meson.build b/meson.build index 9cd73462f..0344c55bb 100644 --- a/meson.build +++ b/meson.build @@ -241,6 +241,10 @@ else pipelines = wanted_pipelines endif +if cc.get_id() == 'gcc' and pipelines.contains('rpi/pisp') + add_project_arguments('-ftree-vrp', language : 'cpp') +endif + # Tests require the vimc pipeline handler, include it automatically when tests # are enabled. if get_option('test')
The function PiSPCameraData::beOutputDequeue in pipelines/rpi/pisp.cpp contains a variable "index" that is initialized in the loop along with the variable "stream". When -ftree-vrp is not enabled, GCC cannot analyze the code to determine that after "ASSERT(stream)", "index" will always be initialized. Since it cannot determine that "index" will always be initialized, it emits a warning, which is elevated to an error. If the rpi/pisp pipeline is to be built, and the compiler is GCC, add -ftree-vrp to project arguments for C++. This is not needed for Clang, since Clang always does the appropriate level of code analysis (even at -O0). --- Notes: I would like advice on whether using "add_project_arguments" is the correct thing to do here. It feels somewhat wrong to me because all the other flags (as far as I can tell) are set via common_arguments / c_arguments / cpp_arguments. However, since I need to check the enabled pipelines, I put the check after the code that sets the "pipelines" variable. Or is my approach wrong to begin with? Should I be adjusting the code itself instead of adding compiler flags? In my opinion, adding compiler flags is the correct approach, since the code already works as-is with Clang. Thank you for your attention. meson.build | 4 ++++ 1 file changed, 4 insertions(+)