[libcamera-devel] [PATCH v2] meson: Don't set _FORTIFY_SOURCE for ChromeOS
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Sep 19 09:08:29 CEST 2023
Hi George
On Mon, Sep 18, 2023 at 09:56:46AM -0600, George Burgess wrote:
> On Thu, Sep 14, 2023 at 9:32 AM Jacopo Mondi
> <jacopo.mondi at ideasonboard.com> wrote:
> >
> > Hi George
> >
> > On Thu, Sep 14, 2023 at 08:48:49AM -0600, George Burgess via libcamera-devel wrote:
> > > Hey Jacopo,
> > >
> > > On Thu, Sep 14, 2023 at 1:53 AM Jacopo Mondi
> > > <jacopo.mondi at ideasonboard.com> wrote:
> > > >
> > > > Hi George
> > > >
> > > > On Tue, Sep 12, 2023 at 09:06:20AM -0600, George Burgess via libcamera-devel wrote:
> > > > > Hey,
> > > > >
> > > > > > Do you expect to have -D_FORTIFY_SOURCE and -U_FORTIFY_SOURCE in the
> > > > > > same command line, so that you have to reset has_fortify_define to
> > > > > > false if it was previously been set to true here below ?
> > > > >
> > > > > You're correct - the code as written intends to answer "will FORTIFY _be
> > > > > enabled_ by `cpp_args`?" If you'd like, I'm happy to reduce complexity and have
> > > > > the code determine whether `-D_FORTIFY_SOURCE*` was specified at all. Either
> > > > > would work equally well for ChromeOS. :)
> > > >
> > > > Please do not mangle the email content and try to reply where the
> > > > comment was made in the patch, otherwise it's hard to find context.
> > >
> > > Ah, is replying inline like this the preferred style? I'll try to do that going
> > > forward then - thanks for the tip.
> > >
> > > >
> > > > I'll copy your reply below.
> > > >
> > > > >
> > > > > George
> > > > >
> > > > > On Tue, Sep 12, 2023 at 7:22 AM Jacopo Mondi
> > > > > <jacopo.mondi at ideasonboard.com> wrote:
> > > > > >
> > > > > > Hi George
> > > > > >
> > > > > > On Mon, Sep 11, 2023 at 05:09:07PM -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 | 24 +++++++++++++++++++-----
> > > > > > > 1 file changed, 19 insertions(+), 5 deletions(-)
> > > > > > >
> > > > > > > diff --git a/meson.build b/meson.build
> > > > > > > index 7959b538..2e834263 100644
> > > > > > > --- a/meson.build
> > > > > > > +++ b/meson.build
> > > > > > > @@ -99,12 +99,26 @@ if cc.get_id() == 'clang'
> > > > > > > error('clang version is too old, libcamera requires 9.0 or newer')
> > > > > > > endif
> > > > > > >
> > > > > > > - # 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.
> > > > > > > + # Turn _FORTIFY_SOURCE by default on. This is needed on clang only as gcc
> > > > > > > + # enables it by default. FORTIFY will not work properly with `-O0`, and may
> > > > > > > + # result in macro redefinition errors if the user already has a setting for
> > > > > > > + # `-D_FORTIFY_SOURCE`. Do not enable FORTIFY in either of those cases.
> > > > > > > if get_option('optimization') != '0'
> > > > > > > - common_arguments += [
> > > > > > > - '-D_FORTIFY_SOURCE=2',
> > > > > > > - ]
> > > > > > > + has_fortify_define = false
> > > > > > > + # Assume that if the user requests a FORTIFY level in cpp_args, they
> > > > > > > + # do the same for c_args.
> > > > > > > + foreach flag : get_option('cpp_args')
> > > > > > > + if flag == '-U_FORTIFY_SOURCE'
> > > > > > > + has_fortify_define = false
> > > > > >
> > > > > > Do you expect to have -D_FORTIFY_SOURCE and -U_FORTIFY_SOURCE in the
> > > > > > same command line, so that you have to reset has_fortify_define to
> > > > > > false if it was previously been set to true here below ?
> > > > > >
> > > > > > Otherwise I might be missing why you have to set 'has_fortify_define =
> > > > > > false' when the variable is already initialized to 'false'...
> > > > > >
> > > > >
> > > > > You're correct - the code as written intends to answer "will FORTIFY _be
> > > > > enabled_ by `cpp_args`?" If you'd like, I'm happy to reduce complexity and have
> > > > > the code determine whether `-D_FORTIFY_SOURCE*` was specified at all. Either
> > > > > would work equally well for ChromeOS. :)
> > > >
> > > > As I read this the code is written to protect against the case where
> > > > -U_FORTIFY_SOURCE is specified in the same command line -after-
> > > > -D_FORTIFY_SOURCE, isn't it ?
> > > >
> > > > My comment was about the fact `has_fortify_define` is already
> > > > initialized to false, so there is not need for
> > > >
> > > > if flag == '-U_FORTIFY_SOURCE'
> > > > has_fortify_define = false
> > > >
> > > > Unless you don't expect the above described case.
> > > >
> > > > As command lines are usually generated by the build system and
> > > > multiple options to enable/disable a feature might be concatenated in
> > > > the same line, I'm not opposed to this, but I was wondering if this
> > > > was by design or not.
> > >
> > > Yes, this is by design, and I wrote it this way for the exact reason you
> > > mention: build flags can be gathered from many different places, and later ones
> > > take precedence over earlier ones. This logic tries to recognize that and handle
> > > it appropriately.
> > >
> >
> > Ok, just wanted to make sure this was by design, and I think
> > protecting against conflicting parameters makes sense!
> >
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
> > Thanks
> > j
>
> Thanks again for your help & review! Sorry for my ignorance of kernelish
> patching procedures, but now that this has been reviewed, is there a process I
> should follow to get it landed? I don't see my change in the history
> of https://git.libcamera.org/libcamera/libcamera.git/ yet (HEAD == 90e0fea6c).
>
Not yet, it needs one more R-b tag and then we can merge it.
Also, a minor nit, looking at the commit title, I would re-phrase that
to remove ChromeOS mention from there (there's not CrOS specific, if
not that CrOS uses a system-defined command line, but other systems
might do the same)
Thanks
j
>
> > > >
> > > >
> > > > > > > + elif flag.startswith('-D_FORTIFY_SOURCE=')
> > > > > > > + has_fortify_define = true
> > > > > > > + endif
> > > > > > > + endforeach
> > > > > > > + if not has_fortify_define
> > > > > > > + common_arguments += [
> > > > > > > + '-D_FORTIFY_SOURCE=2',
> > > > > > > + ]
> > > > > > > + endif
> > > > > > > endif
> > > > > > >
> > > > > > > # Use libc++ by default if available instead of libstdc++ when compiling
> > > > > > > --
> > > > > > > 2.42.0.283.g2d96d420d3-goog
> > > > > > >
More information about the libcamera-devel
mailing list