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

[Kieran Bingham]

Quoting Laurent Pinchart (2023-04-13 12:37:10)
> 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.

Except at that point, GCC-13 will be included in the build-matrix, as a
supported compiler... so it's not going to 'sneak in'.

--
Kieran


> 
> > > > > > ---
> > > > > > 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