| Message ID | 20230220045523.3884-1-ecurtin@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Eric, 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@gmail.com> > Signed-off-by: Eric Curtin <ecurtin@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@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? -- 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; > > -- > 2.39.1 >
On Tue, Mar 28, 2023 at 10:13 AM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Eric, > > 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@gmail.com> > > Signed-off-by: Eric Curtin <ecurtin@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@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 > > -- > 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; > > > > -- > > 2.39.1 > >
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@gmail.com> > > > Signed-off-by: Eric Curtin <ecurtin@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@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()); > > > --- > > > 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; > > >
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@gmail.com> > > > > Signed-off-by: Eric Curtin <ecurtin@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@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. -- 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
On Thu, 13 Apr 2023 at 12:13, Kieran Bingham <kieran.bingham@ideasonboard.com> 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@gmail.com> > > > > > Signed-off-by: Eric Curtin <ecurtin@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@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! I prefer the patch here also for the record, more portable code (even though that may not be an issue for libcamera), could add a comment or something (but do we care enough?). But whatever you guys think :) > > Laurent, any objection? Lets get this resolved quickly now we know it's > not going to be fixed in gcc. > > -- > 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 >
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@gmail.com> > > > > > Signed-off-by: Eric Curtin <ecurtin@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@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. > > > > > --- > > > > > 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; > > > > >
On Thu, 13 Apr 2023 at 12:35, Eric Curtin <ecurtin@redhat.com> wrote: > > On Thu, 13 Apr 2023 at 12:13, Kieran Bingham > <kieran.bingham@ideasonboard.com> 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@gmail.com> > > > > > > Signed-off-by: Eric Curtin <ecurtin@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@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! > > I prefer the patch here also for the record, more portable code (even > though that may not be an issue for libcamera), could add a comment or > something (but do we care enough?). Scratch the we "could add a comment", it's there already. It's so long since I submitted this patch I forgot! Is mise le meas/Regards, Eric Curtin > > But whatever you guys think :) > > > > > Laurent, any objection? Lets get this resolved quickly now we know it's > > not going to be fixed in gcc. > > > > -- > > 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 > >
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@gmail.com> > > > > > > Signed-off-by: Eric Curtin <ecurtin@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@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
On Thu, Apr 13, 2023 at 4:37 AM Laurent Pinchart <laurent.pinchart@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@gmail.com> > > > > > > Signed-off-by: Eric Curtin <ecurtin@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@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
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;
../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@gmail.com> Signed-off-by: Eric Curtin <ecurtin@redhat.com> --- 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(-)