| Message ID | 20201007092239.41704-2-tomi.valkeinen@iki.fi |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi, On 07/10/2020 10:22, Tomi Valkeinen wrote: > I get the warnings related to psabi with: > > gcc 9.3.0 "arm-buildroot-linux-gnueabihf-g++.br_real (Buildroot 2020.08-24-gc5c5f1fa46) 9.3.0" > > Remove the check for gcc-9, so that -Wno-psabi is used on all gcc > versions over 7.1. It doesn't seem unreasonable to keep this disabled for now. Should it be < 10 instead though? We're not *yet* worried about abi stability anyway, but perhaps we'll need to check through some of this when we do consider stabilising. It might be that we end up requiring a greater minimum compiler or such to prevent us hitting the changes that were affected. But for now ... With some consideration of if <10 is more applicable or not, and eitherway after that: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi> > --- > meson.build | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/meson.build b/meson.build > index dd88eea..16f1c6f 100644 > --- a/meson.build > +++ b/meson.build > @@ -75,8 +75,7 @@ if cc.get_id() == 'gcc' > # passing on ARM platforms. This generates a large number of messages > # during compilation with gcc >=7.1 until gcc 9. Silence them. > if (host_machine.cpu_family() == 'arm' and > - cc.version().version_compare('>=7.1') and > - cc.version().version_compare('<9')) > + cc.version().version_compare('>=7.1')) > cpp_arguments += [ > '-Wno-psabi', > ] >
Hi Tomi and Kieran, On Wed, Oct 07, 2020 at 10:45:40AM +0100, Kieran Bingham wrote: > On 07/10/2020 10:22, Tomi Valkeinen wrote: > > I get the warnings related to psabi with: > > > > gcc 9.3.0 "arm-buildroot-linux-gnueabihf-g++.br_real (Buildroot 2020.08-24-gc5c5f1fa46) 9.3.0" > > > > Remove the check for gcc-9, so that -Wno-psabi is used on all gcc > > versions over 7.1. > > It doesn't seem unreasonable to keep this disabled for now. > > Should it be < 10 instead though? I was going to suggest the same, for the exact same reasons :-) > We're not *yet* worried about abi stability anyway, but perhaps we'll > need to check through some of this when we do consider stabilising. > > It might be that we end up requiring a greater minimum compiler or such > to prevent us hitting the changes that were affected. But for now ... > > With some consideration of if <10 is more applicable or not, and > eitherway after that: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi> > > --- > > meson.build | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/meson.build b/meson.build > > index dd88eea..16f1c6f 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -75,8 +75,7 @@ if cc.get_id() == 'gcc' > > # passing on ARM platforms. This generates a large number of messages > > # during compilation with gcc >=7.1 until gcc 9. Silence them. > > if (host_machine.cpu_family() == 'arm' and > > - cc.version().version_compare('>=7.1') and > > - cc.version().version_compare('<9')) > > + cc.version().version_compare('>=7.1')) > > cpp_arguments += [ > > '-Wno-psabi', > > ]
On 07/10/2020 16:29, Laurent Pinchart wrote: > Hi Tomi and Kieran, > > On Wed, Oct 07, 2020 at 10:45:40AM +0100, Kieran Bingham wrote: >> On 07/10/2020 10:22, Tomi Valkeinen wrote: >>> I get the warnings related to psabi with: >>> >>> gcc 9.3.0 "arm-buildroot-linux-gnueabihf-g++.br_real (Buildroot 2020.08-24-gc5c5f1fa46) 9.3.0" >>> >>> Remove the check for gcc-9, so that -Wno-psabi is used on all gcc >>> versions over 7.1. >> >> It doesn't seem unreasonable to keep this disabled for now. >> >> Should it be < 10 instead though? > > I was going to suggest the same, for the exact same reasons :-) So what is the reason, I didn't quite get that. Do we not get the warnings on gcc 10? Why not just enable -Wno-psabi if the compiler supports it? Tomi
On Wed, Oct 07, 2020 at 04:45:04PM +0300, Tomi Valkeinen wrote: > On 07/10/2020 16:29, Laurent Pinchart wrote: > > Hi Tomi and Kieran, > > > > On Wed, Oct 07, 2020 at 10:45:40AM +0100, Kieran Bingham wrote: > >> On 07/10/2020 10:22, Tomi Valkeinen wrote: > >>> I get the warnings related to psabi with: > >>> > >>> gcc 9.3.0 "arm-buildroot-linux-gnueabihf-g++.br_real (Buildroot 2020.08-24-gc5c5f1fa46) 9.3.0" > >>> > >>> Remove the check for gcc-9, so that -Wno-psabi is used on all gcc > >>> versions over 7.1. > >> > >> It doesn't seem unreasonable to keep this disabled for now. > >> > >> Should it be < 10 instead though? > > > > I was going to suggest the same, for the exact same reasons :-) > > So what is the reason, I didn't quite get that. > > Do we not get the warnings on gcc 10? > > Why not just enable -Wno-psabi if the compiler supports it? In order to be notified of future ABI changes. Currently we get warned that an ABI we use changed on gcc-7.1, and that's alright. If we suppress the warning unconditionally, once gcc 11 gets released for example, it may warn us of ABI changes introduced in gcc 11. I would expect the gcc 7.1 ABI warning to get disabled at some point. I wonder if there's a way to disable ABI warnings of gcc <= 7 only.
On 07/10/2020 16:47, Laurent Pinchart wrote: > On Wed, Oct 07, 2020 at 04:45:04PM +0300, Tomi Valkeinen wrote: >> On 07/10/2020 16:29, Laurent Pinchart wrote: >>> Hi Tomi and Kieran, >>> >>> On Wed, Oct 07, 2020 at 10:45:40AM +0100, Kieran Bingham wrote: >>>> On 07/10/2020 10:22, Tomi Valkeinen wrote: >>>>> I get the warnings related to psabi with: >>>>> >>>>> gcc 9.3.0 "arm-buildroot-linux-gnueabihf-g++.br_real (Buildroot 2020.08-24-gc5c5f1fa46) 9.3.0" >>>>> >>>>> Remove the check for gcc-9, so that -Wno-psabi is used on all gcc >>>>> versions over 7.1. >>>> >>>> It doesn't seem unreasonable to keep this disabled for now. >>>> >>>> Should it be < 10 instead though? >>> >>> I was going to suggest the same, for the exact same reasons :-) >> >> So what is the reason, I didn't quite get that. >> >> Do we not get the warnings on gcc 10? >> >> Why not just enable -Wno-psabi if the compiler supports it? > > In order to be notified of future ABI changes. Currently we get warned > that an ABI we use changed on gcc-7.1, and that's alright. If we > suppress the warning unconditionally, once gcc 11 gets released for > example, it may warn us of ABI changes introduced in gcc 11. I would > expect the gcc 7.1 ABI warning to get disabled at some point. I wonder > if there's a way to disable ABI warnings of gcc <= 7 only. Hmm... So is the suggestion to keep increasing the gcc version number in meson.build whenever someone tries a more recent gcc version and gets the warning spam? And if in the future there's a new ABI change, we hit the warnings, and then we again add a gcc version range where we add -Wno-psabi to get rid of the warnings? This sounds a bit silly. Perhaps I'm missing something. I'm not sure what is the point with these warnings if the solution is always to add -Wno-psabi, which silences all psabi changes. Tomi
Hi Tomi, On 07/10/2020 15:17, Tomi Valkeinen wrote: > On 07/10/2020 16:47, Laurent Pinchart wrote: >> On Wed, Oct 07, 2020 at 04:45:04PM +0300, Tomi Valkeinen wrote: >>> On 07/10/2020 16:29, Laurent Pinchart wrote: >>>> Hi Tomi and Kieran, >>>> >>>> On Wed, Oct 07, 2020 at 10:45:40AM +0100, Kieran Bingham wrote: >>>>> On 07/10/2020 10:22, Tomi Valkeinen wrote: >>>>>> I get the warnings related to psabi with: >>>>>> >>>>>> gcc 9.3.0 "arm-buildroot-linux-gnueabihf-g++.br_real (Buildroot 2020.08-24-gc5c5f1fa46) 9.3.0" >>>>>> >>>>>> Remove the check for gcc-9, so that -Wno-psabi is used on all gcc >>>>>> versions over 7.1. >>>>> >>>>> It doesn't seem unreasonable to keep this disabled for now. >>>>> >>>>> Should it be < 10 instead though? >>>> >>>> I was going to suggest the same, for the exact same reasons :-) >>> >>> So what is the reason, I didn't quite get that. >>> >>> Do we not get the warnings on gcc 10? >>> >>> Why not just enable -Wno-psabi if the compiler supports it? >> >> In order to be notified of future ABI changes. Currently we get warned >> that an ABI we use changed on gcc-7.1, and that's alright. If we >> suppress the warning unconditionally, once gcc 11 gets released for >> example, it may warn us of ABI changes introduced in gcc 11. I would >> expect the gcc 7.1 ABI warning to get disabled at some point. I wonder >> if there's a way to disable ABI warnings of gcc <= 7 only. > > Hmm... So is the suggestion to keep increasing the gcc version number in meson.build whenever > someone tries a more recent gcc version and gets the warning spam? Currently, Yes ;-) > And if in the future there's a new ABI change, we hit the warnings, and then we again add a gcc > version range where we add -Wno-psabi to get rid of the warnings? Yes ... until the point we declare ourselves ABI stable. At that point, things change. Extending the -Wno-psabi is papering over the cracks we don't care about. But when we want to be ABI stable - those cracks are suddenly important. > This sounds a bit silly. Perhaps I'm missing something. I'm not sure what is the point with these > warnings if the solution is always to add -Wno-psabi, which silences all psabi changes. I haven't checked/seen the recent warnings, but from my understanding they say something like "If you compile the library with GCC 6, and the application with GCC 7 - it will break in magical ways you would not expect" Which could happen for instance if the library was provided by the distribution, and an individual compiled an application with a different tool-chain version... We currently have a minimum GCC version of 7, so we don't care about that, and we're happy to ignore the warnings. If GCC one day decides to add an ABI break at gcc 10 however, that statement would change to: "If you compile the library with GCC 9, and an application with GCC 10 - It will break in magical ways you would not expect" and that would be more problematic for us. So we're happy to defer the warnings for now, but we can't ignore any future additions. > Tomi > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi, On 08/10/2020 12:18, Kieran Bingham wrote: >> This sounds a bit silly. Perhaps I'm missing something. I'm not sure what is the point with these >> warnings if the solution is always to add -Wno-psabi, which silences all psabi changes. > > > I haven't checked/seen the recent warnings, but from my understanding > they say something like > > "If you compile the library with GCC 6, and the application with GCC 7 > - it will break in magical ways you would not expect" note: parameter passing for argument of type ‘std::_Rb_tree_const_iterator<std::pair<const libcamera::PixelFormat, libcamera::PixelFormatInfo> >’ changed in GCC 7.1 > Which could happen for instance if the library was provided by the > distribution, and an individual compiled an application with a different > tool-chain version... > > We currently have a minimum GCC version of 7, so we don't care about > that, and we're happy to ignore the warnings. But... What does libcamera's minimum gcc version affect? A library libcamera uses against could be compiled with earlier gcc, no? Or if we consider the ABI from libcamera, libcamera could be compiled on gcc 7, and the application on gcc 6. > If GCC one day decides to add an ABI break at gcc 10 however, that > statement would change to: > > "If you compile the library with GCC 9, and an application with GCC 10 > - It will break in magical ways you would not expect" > > and that would be more problematic for us. > > So we're happy to defer the warnings for now, but we can't ignore any > future additions. But how do we separate the two different ABI breakages? There's just -Wno-psabi. If I understand what you say correctly, we'd like to see the new warning, but not the old one. And... So what's the fix? Rewrite all the code affected by the ABI change in some other way, so that you don't hit the ABI change? Is that even realistic? Isn't the only real fix just requiring the people to compile libraries and apps with gcc versions that are ABI compatible? I don't think I get this whole thing =). But I'm fine with increasing the version check to < 10, as I'm currently using gcc 9. I'll probably switch my buildroot to gcc 10 at some point in the near future, and then I can send a new patch. Tomi
Hi Tomi, On Thu, Oct 08, 2020 at 01:27:30PM +0300, Tomi Valkeinen wrote: > On 08/10/2020 12:18, Kieran Bingham wrote: > > >> This sounds a bit silly. Perhaps I'm missing something. I'm not sure what is the point with these > >> warnings if the solution is always to add -Wno-psabi, which silences all psabi changes. > > > > > > I haven't checked/seen the recent warnings, but from my understanding > > they say something like > > > > "If you compile the library with GCC 6, and the application with GCC 7 > > - it will break in magical ways you would not expect" > > note: parameter passing for argument of type ‘std::_Rb_tree_const_iterator<std::pair<const > libcamera::PixelFormat, libcamera::PixelFormatInfo> >’ changed in GCC 7.1 > > > Which could happen for instance if the library was provided by the > > distribution, and an individual compiled an application with a different > > tool-chain version... > > > > We currently have a minimum GCC version of 7, so we don't care about > > that, and we're happy to ignore the warnings. > > But... What does libcamera's minimum gcc version affect? A library > libcamera uses against could be compiled with earlier gcc, no? > > Or if we consider the ABI from libcamera, libcamera could be compiled > on gcc 7, and the application on gcc 6. > > > If GCC one day decides to add an ABI break at gcc 10 however, that > > statement would change to: > > > > "If you compile the library with GCC 9, and an application with GCC 10 > > - It will break in magical ways you would not expect" > > > > and that would be more problematic for us. > > > > So we're happy to defer the warnings for now, but we can't ignore any > > future additions. > > But how do we separate the two different ABI breakages? There's just > -Wno-psabi. If I understand what you say correctly, we'd like to see > the new warning, but not the old one. That's the idea, but it doesn't seem to be possible. > And... So what's the fix? Rewrite all the code affected by the ABI > change in some other way, so that you don't hit the ABI change? Is > that even realistic? Isn't the only real fix just requiring the people > to compile libraries and apps with gcc versions that are ABI > compatible? > > I don't think I get this whole thing =). But I'm fine with increasing > the version check to < 10, as I'm currently using gcc 9. I'll probably > switch my buildroot to gcc 10 at some point in the near future, and > then I can send a new patch. The idea is that I'd like to keep an eye on ABI breakages introduced by gcc, in a "just for my information" fashion for the moment. I don't know at this point how such breakages would even be handled (if at all). "Just for my information" is probably not a good enough excuse to annoy everybody with warnings when using new compiler versions though. I'd thus agree to drop the version check. I wonder how the gcc developers had envisioned this being used though, if there's a warning (which is actually not even a warning, but a note), I imagine it was put there with a target audience in mind. Or maybe this is just a mess and nobody should really pay attention :-)
On 09/10/2020 04:37, Laurent Pinchart wrote: >> I don't think I get this whole thing =). But I'm fine with increasing >> the version check to < 10, as I'm currently using gcc 9. I'll probably >> switch my buildroot to gcc 10 at some point in the near future, and >> then I can send a new patch. > > The idea is that I'd like to keep an eye on ABI breakages introduced by > gcc, in a "just for my information" fashion for the moment. I don't know > at this point how such breakages would even be handled (if at all). > > "Just for my information" is probably not a good enough excuse to annoy > everybody with warnings when using new compiler versions though. I'd > thus agree to drop the version check. I wonder how the gcc developers > had envisioned this being used though, if there's a warning (which is > actually not even a warning, but a note), I imagine it was put there > with a target audience in mind. Or maybe this is just a mess and nobody > should really pay attention :-) Ok. Yes, I don't get this either. What would make sense is, e.g. "-Wno-psabi=1,4", which would disable warnings for ABI breakages #1 and #4, but allow all the other warnings. What I did in kms++ was just: if (cpp.get_id() == 'gcc' and host_machine.cpu_family() == 'arm' and cpp.has_argument('-Wno-psabi')) cpp_arguments += [ '-Wno-psabi', ] endif It's silly to always disable the warning, but I haven't found much info about it while googling, so currently I'm guessing it's the "this is just a mess" case. Tomi
Hi Tomi, On Fri, Oct 09, 2020 at 10:03:39AM +0300, Tomi Valkeinen wrote: > On 09/10/2020 04:37, Laurent Pinchart wrote: > > >> I don't think I get this whole thing =). But I'm fine with increasing > >> the version check to < 10, as I'm currently using gcc 9. I'll probably > >> switch my buildroot to gcc 10 at some point in the near future, and > >> then I can send a new patch. > > > > The idea is that I'd like to keep an eye on ABI breakages introduced by > > gcc, in a "just for my information" fashion for the moment. I don't know > > at this point how such breakages would even be handled (if at all). > > > > "Just for my information" is probably not a good enough excuse to annoy > > everybody with warnings when using new compiler versions though. I'd > > thus agree to drop the version check. I wonder how the gcc developers > > had envisioned this being used though, if there's a warning (which is > > actually not even a warning, but a note), I imagine it was put there > > with a target audience in mind. Or maybe this is just a mess and nobody > > should really pay attention :-) > > Ok. Yes, I don't get this either. What would make sense is, e.g. > "-Wno-psabi=1,4", which would disable warnings for ABI breakages #1 > and #4, but allow all the other warnings. > > What I did in kms++ was just: > > if (cpp.get_id() == 'gcc' and > host_machine.cpu_family() == 'arm' and > cpp.has_argument('-Wno-psabi')) > > cpp_arguments += [ > '-Wno-psabi', > ] > endif > > It's silly to always disable the warning, but I haven't found much > info about it while googling, so currently I'm guessing it's the "this > is just a mess" case. I think I'm getting convinced that this is indeed the right way forward for now. Kieran, what do you think ?
Hi Laurent, On 12/10/2020 00:52, Laurent Pinchart wrote: > Hi Tomi, > > On Fri, Oct 09, 2020 at 10:03:39AM +0300, Tomi Valkeinen wrote: >> On 09/10/2020 04:37, Laurent Pinchart wrote: >> >>>> I don't think I get this whole thing =). But I'm fine with increasing >>>> the version check to < 10, as I'm currently using gcc 9. I'll probably >>>> switch my buildroot to gcc 10 at some point in the near future, and >>>> then I can send a new patch. >>> >>> The idea is that I'd like to keep an eye on ABI breakages introduced by >>> gcc, in a "just for my information" fashion for the moment. I don't know >>> at this point how such breakages would even be handled (if at all). >>> >>> "Just for my information" is probably not a good enough excuse to annoy >>> everybody with warnings when using new compiler versions though. I'd >>> thus agree to drop the version check. I wonder how the gcc developers >>> had envisioned this being used though, if there's a warning (which is >>> actually not even a warning, but a note), I imagine it was put there >>> with a target audience in mind. Or maybe this is just a mess and nobody >>> should really pay attention :-) >> >> Ok. Yes, I don't get this either. What would make sense is, e.g. >> "-Wno-psabi=1,4", which would disable warnings for ABI breakages #1 >> and #4, but allow all the other warnings. >> >> What I did in kms++ was just: >> >> if (cpp.get_id() == 'gcc' and >> host_machine.cpu_family() == 'arm' and >> cpp.has_argument('-Wno-psabi')) >> >> cpp_arguments += [ >> '-Wno-psabi', >> ] >> endif >> >> It's silly to always disable the warning, but I haven't found much >> info about it while googling, so currently I'm guessing it's the "this >> is just a mess" case. > > I think I'm getting convinced that this is indeed the right way forward > for now. Kieran, what do you think ? The part of all this that I don't like is that GCC doesn't (seem to) have a way to disable (/acknowledge) the ABI change from v7, to ignore 'just that set'. So indeed, it means that these warnings will currently persist forever - and then get blurred in with any additional ones. Which is quite frustrating. Along with the lack of control that we have over what tools actually build our library in distro's etc, I fear this might simply be a problem we defer to the distributions. It's not something that can be handled stand-alone at the library level. I don't particularly want to wave the 'someone else's problem' - but there are simply external factors that are outside of our control for this issue - so we can't hope to solve them all. I don't mind disabling it.
On Mon, Oct 12, 2020 at 11:44:34AM +0100, Kieran Bingham wrote: > On 12/10/2020 00:52, Laurent Pinchart wrote: > > On Fri, Oct 09, 2020 at 10:03:39AM +0300, Tomi Valkeinen wrote: > >> On 09/10/2020 04:37, Laurent Pinchart wrote: > >> > >>>> I don't think I get this whole thing =). But I'm fine with increasing > >>>> the version check to < 10, as I'm currently using gcc 9. I'll probably > >>>> switch my buildroot to gcc 10 at some point in the near future, and > >>>> then I can send a new patch. > >>> > >>> The idea is that I'd like to keep an eye on ABI breakages introduced by > >>> gcc, in a "just for my information" fashion for the moment. I don't know > >>> at this point how such breakages would even be handled (if at all). > >>> > >>> "Just for my information" is probably not a good enough excuse to annoy > >>> everybody with warnings when using new compiler versions though. I'd > >>> thus agree to drop the version check. I wonder how the gcc developers > >>> had envisioned this being used though, if there's a warning (which is > >>> actually not even a warning, but a note), I imagine it was put there > >>> with a target audience in mind. Or maybe this is just a mess and nobody > >>> should really pay attention :-) > >> > >> Ok. Yes, I don't get this either. What would make sense is, e.g. > >> "-Wno-psabi=1,4", which would disable warnings for ABI breakages #1 > >> and #4, but allow all the other warnings. > >> > >> What I did in kms++ was just: > >> > >> if (cpp.get_id() == 'gcc' and > >> host_machine.cpu_family() == 'arm' and > >> cpp.has_argument('-Wno-psabi')) > >> > >> cpp_arguments += [ > >> '-Wno-psabi', > >> ] > >> endif > >> > >> It's silly to always disable the warning, but I haven't found much > >> info about it while googling, so currently I'm guessing it's the "this > >> is just a mess" case. > > > > I think I'm getting convinced that this is indeed the right way forward > > for now. Kieran, what do you think ? > > The part of all this that I don't like is that GCC doesn't (seem to) > have a way to disable (/acknowledge) the ABI change from v7, to ignore > 'just that set'. > > So indeed, it means that these warnings will currently persist forever - > and then get blurred in with any additional ones. Which is quite > frustrating. > > Along with the lack of control that we have over what tools actually > build our library in distro's etc, I fear this might simply be a problem > we defer to the distributions. It's not something that can be handled > stand-alone at the library level. I don't particularly want to wave the > 'someone else's problem' - but there are simply external factors that > are outside of our control for this issue - so we can't hope to solve > them all. > > I don't mind disabling it. I'll push Tomi's patch with a comment updated to remove gcc < 9.
diff --git a/meson.build b/meson.build index dd88eea..16f1c6f 100644 --- a/meson.build +++ b/meson.build @@ -75,8 +75,7 @@ if cc.get_id() == 'gcc' # passing on ARM platforms. This generates a large number of messages # during compilation with gcc >=7.1 until gcc 9. Silence them. if (host_machine.cpu_family() == 'arm' and - cc.version().version_compare('>=7.1') and - cc.version().version_compare('<9')) + cc.version().version_compare('>=7.1')) cpp_arguments += [ '-Wno-psabi', ]
I get the warnings related to psabi with: gcc 9.3.0 "arm-buildroot-linux-gnueabihf-g++.br_real (Buildroot 2020.08-24-gc5c5f1fa46) 9.3.0" Remove the check for gcc-9, so that -Wno-psabi is used on all gcc versions over 7.1. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi> --- meson.build | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)