| Message ID | 20251113135840.610653-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Le jeudi 13 novembre 2025 à 14:58 +0100, Barnabás Pőcze a écrit : > `g_once_init_enter()` can trigger `-Wvolatile` in certain configurations, Sounds like you are running an outdated glib, I don't think this is true anymore. > so replace it by just a normal initialization since the required > synchronization is guaranteed since C++11. Additionally, switch to How is that even possible ? You can't turn all static variable into atomic operations without a major performance hit on TLS. That makes no sense to me. Also, GOnce is just a mutex, its fully reliable. > using `g_quark_from_static_string()` since a string literal is used. This is a different change, should be in a different patch. Nicolas > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/gstreamer/gstlibcameraallocator.cpp | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp > index d4492d994b..164f16b1be 100644 > --- a/src/gstreamer/gstlibcameraallocator.cpp > +++ b/src/gstreamer/gstlibcameraallocator.cpp > @@ -76,12 +76,7 @@ FrameWrap::~FrameWrap() > > GQuark FrameWrap::getQuark() > { > - static gsize frame_quark = 0; > - > - if (g_once_init_enter(&frame_quark)) { > - GQuark quark = g_quark_from_string("GstLibcameraFrameWrap"); > - g_once_init_leave(&frame_quark, quark); > - } > + static const GQuark frame_quark = g_quark_from_static_string("GstLibcameraFrameWrap"); > > return frame_quark; > }
2025. 11. 13. 15:16 keltezéssel, Nicolas Dufresne írta: > Hi Barnabás, > > Le jeudi 13 novembre 2025 à 14:58 +0100, Barnabás Pőcze a écrit : >> `g_once_init_enter()` can trigger `-Wvolatile` in certain configurations, > > Sounds like you are running an outdated glib, I don't think this is true > anymore. Ahh, sorry, the above part is wrong because I misremembered. It's not `g_once_init_enter()`, but how it was used in certain `G_DEFINE_TYPE*()` macros. This was indeed addressed in https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719. At least one build in the CI is affected by that, but that's unrelated. > >> so replace it by just a normal initialization since the required >> synchronization is guaranteed since C++11. Additionally, switch to > > How is that even possible ? You can't turn all static variable into atomic > operations without a major performance hit on TLS. That makes no sense to me. > Also, GOnce is just a mutex, its fully reliable. The compiler essentially inserts a pthread_once, call_once, etc. equivalent for the initialization part. See https://en.cppreference.com/w/cpp/language/storage_duration.html#Static_block_variables Regards, Barnabás Pőcze > >> using `g_quark_from_static_string()` since a string literal is used. > > This is a different change, should be in a different patch. > > Nicolas > >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/gstreamer/gstlibcameraallocator.cpp | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp >> index d4492d994b..164f16b1be 100644 >> --- a/src/gstreamer/gstlibcameraallocator.cpp >> +++ b/src/gstreamer/gstlibcameraallocator.cpp >> @@ -76,12 +76,7 @@ FrameWrap::~FrameWrap() >> >> GQuark FrameWrap::getQuark() >> { >> - static gsize frame_quark = 0; >> - >> - if (g_once_init_enter(&frame_quark)) { >> - GQuark quark = g_quark_from_string("GstLibcameraFrameWrap"); >> - g_once_init_leave(&frame_quark, quark); >> - } >> + static const GQuark frame_quark = g_quark_from_static_string("GstLibcameraFrameWrap"); >> >> return frame_quark; >> }
Le jeudi 13 novembre 2025 à 15:37 +0100, Barnabás Pőcze a écrit : > 2025. 11. 13. 15:16 keltezéssel, Nicolas Dufresne írta: > > Hi Barnabás, > > > > Le jeudi 13 novembre 2025 à 14:58 +0100, Barnabás Pőcze a écrit : > > > `g_once_init_enter()` can trigger `-Wvolatile` in certain configurations, > > > > Sounds like you are running an outdated glib, I don't think this is true > > anymore. > > Ahh, sorry, the above part is wrong because I misremembered. It's not `g_once_init_enter()`, > but how it was used in certain `G_DEFINE_TYPE*()` macros. This was indeed addressed > in https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719. At least one build in > the CI is affected by that, but that's unrelated. > > > > > > > so replace it by just a normal initialization since the required > > > synchronization is guaranteed since C++11. Additionally, switch to > > > > How is that even possible ? You can't turn all static variable into atomic > > operations without a major performance hit on TLS. That makes no sense to me. > > Also, GOnce is just a mutex, its fully reliable. > > The compiler essentially inserts a pthread_once, call_once, etc. equivalent for > the initialization part. See https://en.cppreference.com/w/cpp/language/storage_duration.html#Static_block_variables I didn't know about that, the "since C++11" is not clear though, this is part of a comment saying "or thread(since C++11)". Can we leave this code alone ? Any C/GObject reviewer will trip on the new construction, which utilize implicit locking. I don't think we have a justification to "simplify" this code. At best, you could make it more "C++" and use an explicit std::call_once instead of the GLib variant. Nicolas > > > Regards, > Barnabás Pőcze > > > > > > using `g_quark_from_static_string()` since a string literal is used. > > > > This is a different change, should be in a different patch. > > > > Nicolas > > > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > --- > > > src/gstreamer/gstlibcameraallocator.cpp | 7 +------ > > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp > > > index d4492d994b..164f16b1be 100644 > > > --- a/src/gstreamer/gstlibcameraallocator.cpp > > > +++ b/src/gstreamer/gstlibcameraallocator.cpp > > > @@ -76,12 +76,7 @@ FrameWrap::~FrameWrap() > > > > > > GQuark FrameWrap::getQuark() > > > { > > > - static gsize frame_quark = 0; > > > - > > > - if (g_once_init_enter(&frame_quark)) { > > > - GQuark quark = g_quark_from_string("GstLibcameraFrameWrap"); > > > - g_once_init_leave(&frame_quark, quark); > > > - } > > > + static const GQuark frame_quark = g_quark_from_static_string("GstLibcameraFrameWrap"); > > > > > > return frame_quark; > > > }
On Thu, Nov 13, 2025 at 02:58:40PM +0100, Barnabás Pőcze wrote: > `g_once_init_enter()` can trigger `-Wvolatile` in certain configurations, > so replace it by just a normal initialization since the required > synchronization is guaranteed since C++11. Additionally, switch to > using `g_quark_from_static_string()` since a string literal is used. Could you give an example of when this warning occurs ? I think you saw it when trying to compile with C++20, it's useful to record that information. > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/gstreamer/gstlibcameraallocator.cpp | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp > index d4492d994b..164f16b1be 100644 > --- a/src/gstreamer/gstlibcameraallocator.cpp > +++ b/src/gstreamer/gstlibcameraallocator.cpp > @@ -76,12 +76,7 @@ FrameWrap::~FrameWrap() > > GQuark FrameWrap::getQuark() > { > - static gsize frame_quark = 0; > - > - if (g_once_init_enter(&frame_quark)) { > - GQuark quark = g_quark_from_string("GstLibcameraFrameWrap"); > - g_once_init_leave(&frame_quark, quark); > - } > + static const GQuark frame_quark = g_quark_from_static_string("GstLibcameraFrameWrap"); > > return frame_quark; > }
2025. 11. 13. 16:18 keltezéssel, Laurent Pinchart írta: > On Thu, Nov 13, 2025 at 02:58:40PM +0100, Barnabás Pőcze wrote: >> `g_once_init_enter()` can trigger `-Wvolatile` in certain configurations, >> so replace it by just a normal initialization since the required >> synchronization is guaranteed since C++11. Additionally, switch to >> using `g_quark_from_static_string()` since a string literal is used. > > Could you give an example of when this warning occurs ? I think you saw > it when trying to compile with C++20, it's useful to record that > information. Please see my reply here: https://lists.libcamera.org/pipermail/libcamera-devel/2025-November/054715.html I have unfortunately misremembered the cause of the -Wvolatile warning. This part cannot trigger it. > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- >> src/gstreamer/gstlibcameraallocator.cpp | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp >> index d4492d994b..164f16b1be 100644 >> --- a/src/gstreamer/gstlibcameraallocator.cpp >> +++ b/src/gstreamer/gstlibcameraallocator.cpp >> @@ -76,12 +76,7 @@ FrameWrap::~FrameWrap() >> >> GQuark FrameWrap::getQuark() >> { >> - static gsize frame_quark = 0; >> - >> - if (g_once_init_enter(&frame_quark)) { >> - GQuark quark = g_quark_from_string("GstLibcameraFrameWrap"); >> - g_once_init_leave(&frame_quark, quark); >> - } >> + static const GQuark frame_quark = g_quark_from_static_string("GstLibcameraFrameWrap"); >> >> return frame_quark; >> } >
On Thu, Nov 13, 2025 at 09:48:59AM -0500, Nicolas Dufresne wrote: > Le jeudi 13 novembre 2025 à 15:37 +0100, Barnabás Pőcze a écrit : > > 2025. 11. 13. 15:16 keltezéssel, Nicolas Dufresne írta: > > > Le jeudi 13 novembre 2025 à 14:58 +0100, Barnabás Pőcze a écrit : > > > > `g_once_init_enter()` can trigger `-Wvolatile` in certain configurations, > > > > > > Sounds like you are running an outdated glib, I don't think this is true > > > anymore. > > > > Ahh, sorry, the above part is wrong because I misremembered. It's not `g_once_init_enter()`, > > but how it was used in certain `G_DEFINE_TYPE*()` macros. This was indeed addressed > > in https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719. At least one build in > > the CI is affected by that, but that's unrelated. > > > > > > so replace it by just a normal initialization since the required > > > > synchronization is guaranteed since C++11. Additionally, switch to > > > > > > How is that even possible ? You can't turn all static variable into atomic > > > operations without a major performance hit on TLS. That makes no sense to me. > > > Also, GOnce is just a mutex, its fully reliable. > > > > The compiler essentially inserts a pthread_once, call_once, etc. equivalent for > > the initialization part. See https://en.cppreference.com/w/cpp/language/storage_duration.html#Static_block_variables > > I didn't know about that, the "since C++11" is not clear though, this is part of > a comment saying "or thread(since C++11)". Can we leave this code alone ? Any > C/GObject reviewer will trip on the new construction, which utilize implicit > locking. I don't think we have a justification to "simplify" this code. At best, > you could make it more "C++" and use an explicit std::call_once instead of the > GLib variant. As far as I understand this change is meant to fix a failure in CI when compiling with C++20. I'm fine with std::call_once() to make it explicit if you prefer. > > > > using `g_quark_from_static_string()` since a string literal is used. > > > > > > This is a different change, should be in a different patch. > > > > > > Nicolas > > > > > > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > --- > > > > src/gstreamer/gstlibcameraallocator.cpp | 7 +------ > > > > 1 file changed, 1 insertion(+), 6 deletions(-) > > > > > > > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp > > > > index d4492d994b..164f16b1be 100644 > > > > --- a/src/gstreamer/gstlibcameraallocator.cpp > > > > +++ b/src/gstreamer/gstlibcameraallocator.cpp > > > > @@ -76,12 +76,7 @@ FrameWrap::~FrameWrap() > > > > > > > > GQuark FrameWrap::getQuark() > > > > { > > > > - static gsize frame_quark = 0; > > > > - > > > > - if (g_once_init_enter(&frame_quark)) { > > > > - GQuark quark = g_quark_from_string("GstLibcameraFrameWrap"); > > > > - g_once_init_leave(&frame_quark, quark); > > > > - } > > > > + static const GQuark frame_quark = g_quark_from_static_string("GstLibcameraFrameWrap"); > > > > > > > > return frame_quark; > > > > }
diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp index d4492d994b..164f16b1be 100644 --- a/src/gstreamer/gstlibcameraallocator.cpp +++ b/src/gstreamer/gstlibcameraallocator.cpp @@ -76,12 +76,7 @@ FrameWrap::~FrameWrap() GQuark FrameWrap::getQuark() { - static gsize frame_quark = 0; - - if (g_once_init_enter(&frame_quark)) { - GQuark quark = g_quark_from_string("GstLibcameraFrameWrap"); - g_once_init_leave(&frame_quark, quark); - } + static const GQuark frame_quark = g_quark_from_static_string("GstLibcameraFrameWrap"); return frame_quark; }
`g_once_init_enter()` can trigger `-Wvolatile` in certain configurations, so replace it by just a normal initialization since the required synchronization is guaranteed since C++11. Additionally, switch to using `g_quark_from_static_string()` since a string literal is used. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/gstreamer/gstlibcameraallocator.cpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)