[libcamera-devel] [PATCH v3] cam: file_sink: Fixes following errors with gcc-13

[Khem Raj]

On Thu, Apr 13, 2023 at 4:37 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> On Thu, Apr 13, 2023 at 12:13:19PM +0100, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2023-04-13 12:07:02)
> > > On Tue, Mar 28, 2023 at 10:26:58AM -0700, Khem Raj wrote:
> > > > On Tue, Mar 28, 2023 at 10:13 AM Kieran Bingham wrote:
> > > > > Quoting Eric Curtin (2023-02-20 04:55:24)
> > > > > > ../git/src/cam/file_sink.cpp:92:45: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
> > > > > >    92 |                 const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];
> > > > > >       |                                             ^~~~
> > > > > > ../git/src/cam/file_sink.cpp:92:81: note: the temporary was destroyed at the end of the full expression '(& buffer->libcamera::FrameBuffer::metadata())->libcamera::FrameMetadata::planes().libcamera::Span<const libcamera::FrameMetadata::Plane>::operator[](i)'
> > > > > >    92 |                 const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];
> > > > > >       |                                                                                 ^
> > > > > > cc1plus: all warnings being treated as errors
> > > > > >
> > > > > > Co-developed-by: Khem Raj <raj.khem at gmail.com>
> > > > > > Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> > > > >
> > > > > I'm still hoping that holding off a little on this will ensure it's
> > > > > fixed in the compiler before GCC-13 is released - but I think I can also
> > > > > already add:
> > > > >
> > > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > >
> > > > > And given a fix was added to gcc-13, but it didn't fix /this/ we may be
> > > > > getting to the point that we'll simply have to merge this. I guess it
> > > > > depends on when gcc-13 is actually released?
> > > >
> > > > In a months time. It perhaps is not going to be fixed by atleast the
> > > > first 13.x release. Maybe subsequent point releases
> > > > might have it
> > >
> > > It looks like this won't be fixed in gcc :-(
> > >
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108165#c9
> > >
> > > Their recommendation is to use a pragma to disable the warning:
> > >
> > > diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp
> > > index b32aad247b8b..5fb075261a18 100644
> > > --- a/src/apps/cam/file_sink.cpp
> > > +++ b/src/apps/cam/file_sink.cpp
> > > @@ -114,7 +114,10 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,
> > >         }
> > >
> > >         for (unsigned int i = 0; i < buffer->planes().size(); ++i) {
> > > +#pragma GCC diagnostic push
> > > +#pragma GCC diagnostic ignored "-Wdangling-reference"
> > >                 const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];
> > > +#pragma GCC diagnostic pop
> > >
> > >                 Span<uint8_t> data = image->data(i);
> > >                 unsigned int length = std::min<unsigned int>(meta.bytesused, data.size());
> >
> > I think I'd prefer to apply the patch suggested here than add a pragma!
> >
> > Laurent, any objection? Lets get this resolved quickly now we know it's
> > not going to be fixed in gcc.
>
> There are pros and cons (as always). The pragma is self-documenting,
> which I like. Even with the comment below, future refactoring may
> reintroduce the problem.

I would prefer to patch it and help compiler. This also makes it
portable across multiple versions of gcc
and other compilers even if it means some restrictions on how it is written.
future refactoring will hopefully use gcc in some capacity and you
will run into problem early
>
> > > > > > ---
> > > > > > Changes in v3:
> > > > > >
> > > > > > - Added comment explaining change made because of false negative in gcc
> > > > > >
> > > > > > Changes in v2:
> > > > > >
> > > > > > - Added const
> > > > > > - Made patch mergeable by accounting for new directory structure
> > > > > > ---
> > > > > >  src/apps/cam/file_sink.cpp | 14 +++++++++-----
> > > > > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp
> > > > > > index b32aad24..01846cdb 100644
> > > > > > --- a/src/apps/cam/file_sink.cpp
> > > > > > +++ b/src/apps/cam/file_sink.cpp
> > > > > > @@ -114,13 +114,17 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,
> > > > > >         }
> > > > > >
> > > > > >         for (unsigned int i = 0; i < buffer->planes().size(); ++i) {
> > > > > > -               const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];
> > > > > > -
> > > > > > +               /*
> > > > > > +                * formerly "const FrameMetadata::Plane &"
> > > > > > +                * causing false negative (gcc 13):
> > > > > > +                * "possibly dangling reference to a temporary"
> > > > > > +                */
> > > > > > +               const unsigned int bytesused = buffer->metadata().planes()[i].bytesused;
> > > > > >                 Span<uint8_t> data = image->data(i);
> > > > > > -               unsigned int length = std::min<unsigned int>(meta.bytesused, data.size());
> > > > > > +               const unsigned int length = std::min<unsigned int>(bytesused, data.size());
> > > > > >
> > > > > > -               if (meta.bytesused > data.size())
> > > > > > -                       std::cerr << "payload size " << meta.bytesused
> > > > > > +               if (bytesused > data.size())
> > > > > > +                       std::cerr << "payload size " << bytesused
> > > > > >                                   << " larger than plane size " << data.size()
> > > > > >                                   << std::endl;
> > > > > >
>
> --
> Regards,
>
> Laurent Pinchart