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

[Kieran Bingham]

Quoting Kieran Bingham (2023-02-18 23:00:15)
> Quoting Eric Curtin via libcamera-devel (2023-02-14 13:34:11)
> > On Tue, 14 Feb 2023 at 13:24, Laurent Pinchart
> > <laurent.pinchart at ideasonboard.com> wrote:
> > >
> > > Hi Eric,
> > >
> > > (CC'ing Barnabás)
> > >
> > > Thank you for the patch.
> > >
> > > On Tue, Feb 14, 2023 at 12:14:49PM +0000, Eric Curtin via libcamera-devel wrote:
> > > > ../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
> > >
> > > This seems to be a false positive from an unreleased gcc version. Could
> > > you report the issue to upstream gcc ? I'd like to get it fixed if it's
> > 
> > Reported here:
> > 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107532
> > 
> > I added another poke.
> > 
> > > indeed a false positive, or at least get confirmation from them that
> > > they won't fix it.
> > >
> 
> Let's add this here then:
> 
> Bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107532

So it seems from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107532#c13
that the GCC-13 'bug fix' fixed the minimal test case, but possibly not
the false positive in libcamera.

Can anyone else test to confirm?

--
Kieran


> 
> > > > Co-developed-by: Khem Raj <raj.khem at gmail.com>
> > > > Signed-off-by: Eric Curtin <ecurtin at redhat.com>
> > > > ---
> > > > Changes in v2:
> > > >
> > > > - Added const
> > > > - Made patch mergeable by accounting for new directory structure
> > > > ---
> > > >  src/apps/cam/file_sink.cpp | 9 ++++-----
> > > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp
> > > > index b32aad24..d41569c9 100644
> > > > --- a/src/apps/cam/file_sink.cpp
> > > > +++ b/src/apps/cam/file_sink.cpp
> > > > @@ -114,13 +114,12 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,
> > > >       }
> > > >
> > > >       for (unsigned int i = 0; i < buffer->planes().size(); ++i) {
> 
> I'd be tempted to add some sort of explainer here in a comment, but
> maybe that's overkill if the bug does get fixed before GCC-13 is
> released...?
> 
> > > > -             const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];
> > > > -
> > > > +             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
> 
> Otherwise, the change itself seems fair, and does the 'same thing'...
> so:
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > > >                                 << " larger than plane size " << data.size()
> > > >                                 << std::endl;
> > > >
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> > >
> >