[libcamera-devel] [PATCH] meson: Don't set _FORTIFY_SOURCE for ChromeOS

[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 at 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