| Message ID | 20230911230909.1263308-1-gbiv@google.com |
|---|---|
| State | Accepted |
| Commit | 9c5eb9237cf6ae170086f0d4d87a025aa052cc9f |
| Headers | show |
| Series |
|
| Related | show |
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@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'... > + 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 >
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. :) George On Tue, Sep 12, 2023 at 7:22 AM Jacopo Mondi <jacopo.mondi@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@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'... > > > + 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 > >
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. I'll copy your reply below. > > George > > On Tue, Sep 12, 2023 at 7:22 AM Jacopo Mondi > <jacopo.mondi@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@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. > > > + 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 > > >
Hey Jacopo, On Thu, Sep 14, 2023 at 1:53 AM Jacopo Mondi <jacopo.mondi@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@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@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. > > > > > > + 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 > > > >
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@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@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@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@ideasonboard.com> 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 > > > > >
On Thu, Sep 14, 2023 at 9:32 AM Jacopo Mondi <jacopo.mondi@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@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@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@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@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). > > > > > > > > > > > > + 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 > > > > > >
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@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@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@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@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@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 > > > > > > >
On Thu, Sep 14, 2023 at 05:32:11PM +0200, Jacopo Mondi via libcamera-devel wrote: > On Thu, Sep 14, 2023 at 08:48:49AM -0600, George Burgess via libcamera-devel wrote: > > On Thu, Sep 14, 2023 at 1:53 AM Jacopo Mondi wrote: > > > 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. Ah, the usual top-posting vs. bottom-posting debate :-) > > > I'll copy your reply below. > > > > > > > On Tue, Sep 12, 2023 at 7:22 AM Jacopo Mondi wrote: > > > > > 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@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@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> But I'm wondering if we could simplify this by using compiler.get_define(). I'll give it a try on top. > > > > > > + 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
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 + 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
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 | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)