| Message ID | 20230907161259.2942654-1-gbiv@google.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi George On Thu, Sep 07, 2023 at 10:12:59AM -0600, George Burgess IV via libcamera-devel wrote: > ChromeOS is moving to a platform default of `_FORTIFY_SOURCE=3`, and > this definition conflicts with that: > > ``` > <command line>:4:9: error: '_FORTIFY_SOURCE' macro redefined > [-Werror,-Wmacro-redefined] > ``` I think we can drop the ``` ? > > Rather than adding logic to keep up with their local configuration, it > seems best to leave setting _FORTIFY_SOURCE on ChromeOS up to ChromeOS. > > Signed-off-by: George Burgess IV <gbiv@google.com> > --- > meson.build | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/meson.build b/meson.build > index 7959b538..109923ac 100644 > --- a/meson.build > +++ b/meson.build > @@ -101,7 +101,8 @@ if cc.get_id() == 'clang' > > # Turn _FORTIFY_SOURCE by default on optimised builds (as it requires -O1 > # or higher). This is needed on clang only as gcc enables it by default. > - if get_option('optimization') != '0' > + # CrOS sets their preferred FORTIFY level in platform-level CFLAGS. Not a native speaker here but, is "sets" and "their" in the same phrase ok ? Shouldn't this be either ("set" and "their") or ("sets" and "its") ? > + if get_option('optimization') != '0' and get_option('android_platform') != 'cros' Minor nitpicking apart, I think the patch is fine. Just curious to know if switching to FORTIFY_SOURCE=3 will generate new bug reports :) Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > common_arguments += [ > '-D_FORTIFY_SOURCE=2', > ] > -- > 2.42.0.283.g2d96d420d3-goog >
On Thu, Sep 07, 2023 at 10:12:59AM -0600, George Burgess IV via libcamera-devel wrote: > ChromeOS is moving to a platform default of `_FORTIFY_SOURCE=3`, and > this definition conflicts with that: > > ``` > <command line>:4:9: error: '_FORTIFY_SOURCE' macro redefined > [-Werror,-Wmacro-redefined] > ``` > > Rather than adding logic to keep up with their local configuration, it > seems best to leave setting _FORTIFY_SOURCE on ChromeOS up to ChromeOS. > > Signed-off-by: George Burgess IV <gbiv@google.com> > --- > meson.build | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/meson.build b/meson.build > index 7959b538..109923ac 100644 > --- a/meson.build > +++ b/meson.build > @@ -101,7 +101,8 @@ if cc.get_id() == 'clang' > > # Turn _FORTIFY_SOURCE by default on optimised builds (as it requires -O1 > # or higher). This is needed on clang only as gcc enables it by default. > - if get_option('optimization') != '0' > + # CrOS sets their preferred FORTIFY level in platform-level CFLAGS. Someone else may do the same. I wonder, would it be possible here to add -D_FORTIFY_SOURCES only if not already set in CFLAGS ? > + if get_option('optimization') != '0' and get_option('android_platform') != 'cros' > common_arguments += [ > '-D_FORTIFY_SOURCE=2', > ]
Thank you both for the quick feedback! > I think we can drop the ``` ? Done > Not a native speaker here but, is "sets" and "their" in the same > phrase ok ? Shouldn't this be either ("set" and "their") or ("sets" and > "its") ? Yeah, I agree the wording here was strange on my part. Fixed. > Minor nitpicking apart, I think the patch is fine. Just curious to > know if switching to FORTIFY_SOURCE=3 will generate new bug reports :) Maybe? :) The switch has been pretty clean for us so far. > Someone else may do the same. I wonder, would it be possible here to add > -D_FORTIFY_SOURCES only if not already set in CFLAGS ? ChromeOS is... a bit weird. Our ${CC}, ${CXX}, etc are actually wrappers that apply global CFLAGS (among other things). This makes it pretty hard to detect the complete set of flags are actually being set for a given build. *That said*, I can probably add a nop -D_FORTIFY_SOURCE=3 to the actual ${CFLAGS} for this package in ChromeOS, so this change can be made more generally applicable. I'll send v2 once I get the chance to test that. George On Fri, Sep 8, 2023 at 9:08 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Thu, Sep 07, 2023 at 10:12:59AM -0600, George Burgess IV via libcamera-devel wrote: > > ChromeOS is moving to a platform default of `_FORTIFY_SOURCE=3`, and > > this definition conflicts with that: > > > > ``` > > <command line>:4:9: error: '_FORTIFY_SOURCE' macro redefined > > [-Werror,-Wmacro-redefined] > > ``` > > > > Rather than adding logic to keep up with their local configuration, it > > seems best to leave setting _FORTIFY_SOURCE on ChromeOS up to ChromeOS. > > > > Signed-off-by: George Burgess IV <gbiv@google.com> > > --- > > meson.build | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/meson.build b/meson.build > > index 7959b538..109923ac 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -101,7 +101,8 @@ if cc.get_id() == 'clang' > > > > # Turn _FORTIFY_SOURCE by default on optimised builds (as it requires -O1 > > # or higher). This is needed on clang only as gcc enables it by default. > > - if get_option('optimization') != '0' > > + # CrOS sets their preferred FORTIFY level in platform-level CFLAGS. > > Someone else may do the same. I wonder, would it be possible here to add > -D_FORTIFY_SOURCES only if not already set in CFLAGS ? > > > + if get_option('optimization') != '0' and get_option('android_platform') != 'cros' > > common_arguments += [ > > '-D_FORTIFY_SOURCE=2', > > ] > > -- > Regards, > > Laurent Pinchart
On Fri, Sep 08, 2023 at 09:48:15AM -0600, George Burgess wrote: > Thank you both for the quick feedback! > > > I think we can drop the ``` ? > > Done > > > Not a native speaker here but, is "sets" and "their" in the same > > phrase ok ? Shouldn't this be either ("set" and "their") or ("sets" and > > "its") ? > > Yeah, I agree the wording here was strange on my part. Fixed. > > > Minor nitpicking apart, I think the patch is fine. Just curious to > > know if switching to FORTIFY_SOURCE=3 will generate new bug reports :) > > Maybe? :) The switch has been pretty clean for us so far. > > > Someone else may do the same. I wonder, would it be possible here to add > > -D_FORTIFY_SOURCES only if not already set in CFLAGS ? > > ChromeOS is... a bit weird. Our ${CC}, ${CXX}, etc are actually wrappers that > apply global CFLAGS (among other things). This makes it pretty hard to detect > the complete set of flags are actually being set for a given build. > > *That said*, I can probably add a nop -D_FORTIFY_SOURCE=3 to the actual > ${CFLAGS} for this package in ChromeOS, so this change can be made more > generally applicable. I'll send v2 once I get the chance to test that. Thank you, much appreciated. > On Fri, Sep 8, 2023 at 9:08 AM Laurent Pinchart wrote: > > On Thu, Sep 07, 2023 at 10:12:59AM -0600, George Burgess IV via libcamera-devel wrote: > > > ChromeOS is moving to a platform default of `_FORTIFY_SOURCE=3`, and > > > this definition conflicts with that: > > > > > > ``` > > > <command line>:4:9: error: '_FORTIFY_SOURCE' macro redefined > > > [-Werror,-Wmacro-redefined] > > > ``` > > > > > > Rather than adding logic to keep up with their local configuration, it > > > seems best to leave setting _FORTIFY_SOURCE on ChromeOS up to ChromeOS. > > > > > > Signed-off-by: George Burgess IV <gbiv@google.com> > > > --- > > > meson.build | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/meson.build b/meson.build > > > index 7959b538..109923ac 100644 > > > --- a/meson.build > > > +++ b/meson.build > > > @@ -101,7 +101,8 @@ if cc.get_id() == 'clang' > > > > > > # Turn _FORTIFY_SOURCE by default on optimised builds (as it requires -O1 > > > # or higher). This is needed on clang only as gcc enables it by default. > > > - if get_option('optimization') != '0' > > > + # CrOS sets their preferred FORTIFY level in platform-level CFLAGS. > > > > Someone else may do the same. I wonder, would it be possible here to add > > -D_FORTIFY_SOURCES only if not already set in CFLAGS ? > > > > > + if get_option('optimization') != '0' and get_option('android_platform') != 'cros' > > > common_arguments += [ > > > '-D_FORTIFY_SOURCE=2', > > > ]
diff --git a/meson.build b/meson.build index 7959b538..109923ac 100644 --- a/meson.build +++ b/meson.build @@ -101,7 +101,8 @@ if cc.get_id() == 'clang' # Turn _FORTIFY_SOURCE by default on optimised builds (as it requires -O1 # or higher). This is needed on clang only as gcc enables it by default. - if get_option('optimization') != '0' + # CrOS sets their preferred FORTIFY level in platform-level CFLAGS. + if get_option('optimization') != '0' and get_option('android_platform') != 'cros' common_arguments += [ '-D_FORTIFY_SOURCE=2', ]
ChromeOS is moving to a platform default of `_FORTIFY_SOURCE=3`, and this definition conflicts with that: ``` <command line>:4:9: error: '_FORTIFY_SOURCE' macro redefined [-Werror,-Wmacro-redefined] ``` Rather than adding logic to keep up with their local configuration, it seems best to leave setting _FORTIFY_SOURCE on ChromeOS up to ChromeOS. Signed-off-by: George Burgess IV <gbiv@google.com> --- meson.build | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)