| Message ID | 20240527132428.250382-1-pobrn@protonmail.com |
|---|---|
| State | Accepted |
| Commit | c79fa47aac9b38865dab5e9f29030903bf460c46 |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2024-05-27 14:24:29) > GCC 14 thinks `rects` is a "possibly dangling reference to a temporary": > > /libcamera/src/android/camera_capabilities.cpp: In member function ‘int CameraCapabilities::initializeStaticMetadata()’: > /libcamera/src/android/camera_capabilities.cpp:1084:46: error: possibly dangling reference to a temporary [-Werror=dangling-reference] > 1084 | const Span<const Rectangle>& rects = > | ^~~~~ > /libcamera/src/android/camera_capabilities.cpp:1085:83: note: the temporary was destroyed at the end of the full expression ‘(& properties)->libcamera::ControlList::get<libcamera::Span<const libcamera::Rectangle> >(libcamera::properties::PixelArrayActiveAreas).std::optional<libcamera::Span<const libcamera::Rectangle> >::value_or<libcamera::Span<const libcamera::Rectangle> >(libcamera::Span<const libcamera::Rectangle>())’ > 1085 | properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{}); > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~ > > The return value of `value_or()` is indeed a temporary, but > binding it to a reference extends its lifetime. Avoid the > warning by not using a reference; this does not make much > difference since `value_or()` does not return a reference. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > src/android/camera_capabilities.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > index 6f4d48de..71043e12 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -1081,7 +1081,7 @@ int CameraCapabilities::initializeStaticMetadata() > } > > { > - const Span<const Rectangle> &rects = > + const Span<const Rectangle> rects = > properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{}); > std::vector<int32_t> data{ > static_cast<int32_t>(rects[0].x), > -- > 2.45.1 > >
On Mon, May 27, 2024 at 02:34:19PM +0100, Kieran Bingham wrote: > Quoting Barnabás Pőcze (2024-05-27 14:24:29) > > GCC 14 thinks `rects` is a "possibly dangling reference to a temporary": > > > > /libcamera/src/android/camera_capabilities.cpp: In member function ‘int CameraCapabilities::initializeStaticMetadata()’: > > /libcamera/src/android/camera_capabilities.cpp:1084:46: error: possibly dangling reference to a temporary [-Werror=dangling-reference] > > 1084 | const Span<const Rectangle>& rects = > > | ^~~~~ > > /libcamera/src/android/camera_capabilities.cpp:1085:83: note: the temporary was destroyed at the end of the full expression ‘(& properties)->libcamera::ControlList::get<libcamera::Span<const libcamera::Rectangle> >(libcamera::properties::PixelArrayActiveAreas).std::optional<libcamera::Span<const libcamera::Rectangle> >::value_or<libcamera::Span<const libcamera::Rectangle> >(libcamera::Span<const libcamera::Rectangle>())’ > > 1085 | properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{}); > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~ > > > > The return value of `value_or()` is indeed a temporary, but > > binding it to a reference extends its lifetime. Avoid the > > warning by not using a reference; this does not make much > > difference since `value_or()` does not return a reference. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I'm working on adding gcc 14 tests in our CI. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > --- > > src/android/camera_capabilities.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > > index 6f4d48de..71043e12 100644 > > --- a/src/android/camera_capabilities.cpp > > +++ b/src/android/camera_capabilities.cpp > > @@ -1081,7 +1081,7 @@ int CameraCapabilities::initializeStaticMetadata() > > } > > > > { > > - const Span<const Rectangle> &rects = > > + const Span<const Rectangle> rects = > > properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{}); > > std::vector<int32_t> data{ > > static_cast<int32_t>(rects[0].x),
On Mon, May 27, 2024 at 07:24:16PM +0300, Laurent Pinchart wrote: > On Mon, May 27, 2024 at 02:34:19PM +0100, Kieran Bingham wrote: > > Quoting Barnabás Pőcze (2024-05-27 14:24:29) > > > GCC 14 thinks `rects` is a "possibly dangling reference to a temporary": > > > > > > /libcamera/src/android/camera_capabilities.cpp: In member function ‘int CameraCapabilities::initializeStaticMetadata()’: > > > /libcamera/src/android/camera_capabilities.cpp:1084:46: error: possibly dangling reference to a temporary [-Werror=dangling-reference] > > > 1084 | const Span<const Rectangle>& rects = > > > | ^~~~~ > > > /libcamera/src/android/camera_capabilities.cpp:1085:83: note: the temporary was destroyed at the end of the full expression ‘(& properties)->libcamera::ControlList::get<libcamera::Span<const libcamera::Rectangle> >(libcamera::properties::PixelArrayActiveAreas).std::optional<libcamera::Span<const libcamera::Rectangle> >::value_or<libcamera::Span<const libcamera::Rectangle> >(libcamera::Span<const libcamera::Rectangle>())’ > > > 1085 | properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{}); > > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > The return value of `value_or()` is indeed a temporary, but > > > binding it to a reference extends its lifetime. Avoid the > > > warning by not using a reference; this does not make much > > > difference since `value_or()` does not return a reference. Actually, I'd like to update the commit message. The issue isn't so much that value_or() returns a temporary, but that the parameter passed to value_or() is destroyed at the end of the statement. Due to copy elision (I think), value_or() may return the instance it receives as a parameter. Barnabás, Would the following commit message work for you (and do you think it's accurate) ? -------- The return value of `value_or()` is a temporary, and binding it to a reference extends its lifetime. However, the parameter passed to the `value_or()` function is destroyed at the end of the statement. With copy elision, the function may effectively return the instance that it receives as a parameter, leading to a dangling reference to a temporary. Fix this by storing a copy of the value. As Span is a lightweight class, the overhead is negligible. -------- > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I'm working on adding gcc 14 tests in our CI. > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > --- > > > src/android/camera_capabilities.cpp | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > > > index 6f4d48de..71043e12 100644 > > > --- a/src/android/camera_capabilities.cpp > > > +++ b/src/android/camera_capabilities.cpp > > > @@ -1081,7 +1081,7 @@ int CameraCapabilities::initializeStaticMetadata() > > > } > > > > > > { > > > - const Span<const Rectangle> &rects = > > > + const Span<const Rectangle> rects = > > > properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{}); > > > std::vector<int32_t> data{ > > > static_cast<int32_t>(rects[0].x),
2024. május 27., hétfő 19:06 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta: > On Mon, May 27, 2024 at 07:24:16PM +0300, Laurent Pinchart wrote: > > On Mon, May 27, 2024 at 02:34:19PM +0100, Kieran Bingham wrote: > > > Quoting Barnabás Pőcze (2024-05-27 14:24:29) > > > > GCC 14 thinks `rects` is a "possibly dangling reference to a temporary": > > > > > > > > /libcamera/src/android/camera_capabilities.cpp: In member function ‘int CameraCapabilities::initializeStaticMetadata()’: > > > > /libcamera/src/android/camera_capabilities.cpp:1084:46: error: possibly dangling reference to a temporary [-Werror=dangling-reference] > > > > 1084 | const Span<const Rectangle>& rects = > > > > | ^~~~~ > > > > /libcamera/src/android/camera_capabilities.cpp:1085:83: note: the temporary was destroyed at the end of the full expression ‘(& properties)->libcamera::ControlList::get<libcamera::Span<const libcamera::Rectangle> >(libcamera::properties::PixelArrayActiveAreas).std::optional<libcamera::Span<const libcamera::Rectangle> >::value_or<libcamera::Span<const libcamera::Rectangle> >(libcamera::Span<const libcamera::Rectangle>())’ > > > > 1085 | properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{}); > > > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > The return value of `value_or()` is indeed a temporary, but > > > > binding it to a reference extends its lifetime. Avoid the > > > > warning by not using a reference; this does not make much > > > > difference since `value_or()` does not return a reference. > > Actually, I'd like to update the commit message. The issue isn't so much > that value_or() returns a temporary, but that the parameter passed to > value_or() is destroyed at the end of the statement. Due to copy elision > (I think), value_or() may return the instance it receives as a > parameter. I am not entirely sure about that. Note https://en.cppreference.com/w/cpp/utility/optional/value_or specifically: 2) Equivalent to bool(*this) ? std::move(**this) : static_cast<T>(std::forward<U>(default_value)). In my reading when returning the default value, a new value of type T is constructed from the argument (which is a forwarding reference). So in this specific case, I would say that we're constructing a `Span<const Rectangle>` from a `Span<const Rectangle>&&` (the argument), and returning that, so it is a new separate object from the argument. I am fairly certain GCC is in the wrong here, see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115229 I opened that issue after running into specific warning. Note that if you remove the `int *p` part from the code in the issue then the warning disappears. Regards, Barnabás Pőcze > > Barnabás, Would the following commit message work for you (and do you > think it's accurate) ? > > -------- > The return value of `value_or()` is a temporary, and binding it to a > reference extends its lifetime. However, the parameter passed to the > `value_or()` function is destroyed at the end of the statement. With > copy elision, the function may effectively return the instance that it > receives as a parameter, leading to a dangling reference to a temporary. > > Fix this by storing a copy of the value. As Span is a lightweight class, > the overhead is negligible. > -------- > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > I'm working on adding gcc 14 tests in our CI. > > > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > > --- > > > > src/android/camera_capabilities.cpp | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > > > > index 6f4d48de..71043e12 100644 > > > > --- a/src/android/camera_capabilities.cpp > > > > +++ b/src/android/camera_capabilities.cpp > > > > @@ -1081,7 +1081,7 @@ int CameraCapabilities::initializeStaticMetadata() > > > > } > > > > > > > > { > > > > - const Span<const Rectangle> &rects = > > > > + const Span<const Rectangle> rects = > > > > properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{}); > > > > std::vector<int32_t> data{ > > > > static_cast<int32_t>(rects[0].x), > > -- > Regards, > > Laurent Pinchart
Hi Barnabás, On Mon, May 27, 2024 at 06:32:04PM +0000, Barnabás Pőcze wrote: > 2024. május 27., hétfő 19:06 keltezéssel, Laurent Pinchart írta: > > On Mon, May 27, 2024 at 07:24:16PM +0300, Laurent Pinchart wrote: > > > On Mon, May 27, 2024 at 02:34:19PM +0100, Kieran Bingham wrote: > > > > Quoting Barnabás Pőcze (2024-05-27 14:24:29) > > > > > GCC 14 thinks `rects` is a "possibly dangling reference to a temporary": > > > > > > > > > > /libcamera/src/android/camera_capabilities.cpp: In member function ‘int CameraCapabilities::initializeStaticMetadata()’: > > > > > /libcamera/src/android/camera_capabilities.cpp:1084:46: error: possibly dangling reference to a temporary [-Werror=dangling-reference] > > > > > 1084 | const Span<const Rectangle>& rects = > > > > > | ^~~~~ > > > > > /libcamera/src/android/camera_capabilities.cpp:1085:83: note: the temporary was destroyed at the end of the full expression ‘(& properties)->libcamera::ControlList::get<libcamera::Span<const libcamera::Rectangle> >(libcamera::properties::PixelArrayActiveAreas).std::optional<libcamera::Span<const libcamera::Rectangle> >::value_or<libcamera::Span<const libcamera::Rectangle> >(libcamera::Span<const libcamera::Rectangle>())’ > > > > > 1085 | properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{}); > > > > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > The return value of `value_or()` is indeed a temporary, but > > > > > binding it to a reference extends its lifetime. Avoid the > > > > > warning by not using a reference; this does not make much > > > > > difference since `value_or()` does not return a reference. > > > > Actually, I'd like to update the commit message. The issue isn't so much > > that value_or() returns a temporary, but that the parameter passed to > > value_or() is destroyed at the end of the statement. Due to copy elision > > (I think), value_or() may return the instance it receives as a > > parameter. > > I am not entirely sure about that. Note https://en.cppreference.com/w/cpp/utility/optional/value_or > specifically: > > 2) Equivalent to bool(*this) ? std::move(**this) : static_cast<T>(std::forward<U>(default_value)). > > In my reading when returning the default value, a new value of type T > is constructed from the argument (which is a forwarding reference). > > So in this specific case, I would say that we're constructing a `Span<const Rectangle>` > from a `Span<const Rectangle>&&` (the argument), and returning that, so it > is a new separate object from the argument. > > I am fairly certain GCC is in the wrong here, see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115229 > I opened that issue after running into specific warning. Note that if you remove > the `int *p` part from the code in the issue then the warning disappears. I *think* you're right, but I can't make up my mind for sure here. I'm not even totally sure about which temporary gcc think the dangling reference relates to :-S I'll use your original commit message. > > Barnabás, Would the following commit message work for you (and do you > > think it's accurate) ? > > > > -------- > > The return value of `value_or()` is a temporary, and binding it to a > > reference extends its lifetime. However, the parameter passed to the > > `value_or()` function is destroyed at the end of the statement. With > > copy elision, the function may effectively return the instance that it > > receives as a parameter, leading to a dangling reference to a temporary. > > > > Fix this by storing a copy of the value. As Span is a lightweight class, > > the overhead is negligible. > > -------- > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > I'm working on adding gcc 14 tests in our CI. > > > > > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > > > --- > > > > > src/android/camera_capabilities.cpp | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > > > > > index 6f4d48de..71043e12 100644 > > > > > --- a/src/android/camera_capabilities.cpp > > > > > +++ b/src/android/camera_capabilities.cpp > > > > > @@ -1081,7 +1081,7 @@ int CameraCapabilities::initializeStaticMetadata() > > > > > } > > > > > > > > > > { > > > > > - const Span<const Rectangle> &rects = > > > > > + const Span<const Rectangle> rects = > > > > > properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{}); > > > > > std::vector<int32_t> data{ > > > > > static_cast<int32_t>(rects[0].x),
diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp index 6f4d48de..71043e12 100644 --- a/src/android/camera_capabilities.cpp +++ b/src/android/camera_capabilities.cpp @@ -1081,7 +1081,7 @@ int CameraCapabilities::initializeStaticMetadata() } { - const Span<const Rectangle> &rects = + const Span<const Rectangle> rects = properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{}); std::vector<int32_t> data{ static_cast<int32_t>(rects[0].x),
GCC 14 thinks `rects` is a "possibly dangling reference to a temporary": /libcamera/src/android/camera_capabilities.cpp: In member function ‘int CameraCapabilities::initializeStaticMetadata()’: /libcamera/src/android/camera_capabilities.cpp:1084:46: error: possibly dangling reference to a temporary [-Werror=dangling-reference] 1084 | const Span<const Rectangle>& rects = | ^~~~~ /libcamera/src/android/camera_capabilities.cpp:1085:83: note: the temporary was destroyed at the end of the full expression ‘(& properties)->libcamera::ControlList::get<libcamera::Span<const libcamera::Rectangle> >(libcamera::properties::PixelArrayActiveAreas).std::optional<libcamera::Span<const libcamera::Rectangle> >::value_or<libcamera::Span<const libcamera::Rectangle> >(libcamera::Span<const libcamera::Rectangle>())’ 1085 | properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{}); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~ The return value of `value_or()` is indeed a temporary, but binding it to a reference extends its lifetime. Avoid the warning by not using a reference; this does not make much difference since `value_or()` does not return a reference. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/android/camera_capabilities.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)