| Message ID | 20191121041913.16202-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | a8d03129033080c4c42a3f44792b9f7db206e300 |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 21/11/2019 04:19, Laurent Pinchart wrote: > Commit 965c5bf7fbf5 ("meson: Define _FORTIFY_SOURCE for optimised > builds") tried to define _FORTIFY_SOURCE for optimised builds with > clang, but updated the common_arguments after it was used. This resulted > in the _FORTIFY_SOURCE option not being applied. Fix it. > > Fixes: 965c5bf7fbf5 ("meson: Define _FORTIFY_SOURCE for optimised builds") > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > meson.build | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/meson.build b/meson.build > index 72ad7c8b493b..0a222ba96dcb 100644 > --- a/meson.build > +++ b/meson.build > @@ -35,8 +35,8 @@ common_arguments = [ > '-include', 'config.h', > ] > > -c_arguments = common_arguments > -cpp_arguments = common_arguments > +c_arguments = [] > +cpp_arguments = [] c_arguments and cpp_arguments are not used until after the code block below. Do we really need to define them as empty arrays here? I /think/ we can just move the assignment to below? (where you do an addition instead) > > if cc.get_id() == 'clang' > # Turn _FORTIFY_SOURCE by default on optimised builds (as it requires -O1 > @@ -56,6 +56,9 @@ if cc.get_id() == 'clang' > endif > endif > > +c_arguments += common_arguments > +cpp_arguments += common_arguments > +> add_project_arguments(c_arguments, language : 'c') Eeep - and that's so blindingly obvious when you look at 965c5bf7fbf5, as this line is clearly in the hunk. I'm sorry I missed it. (of course it's always easier to spot something that you know is there). With either the empty arrays declared above if that's necessary (or just preferred) or directly assigned lower down if it's appropriate: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > add_project_arguments(cpp_arguments, language : 'cpp') > add_project_link_arguments(cpp_arguments, language : 'cpp') >
Hi Kieran, On Thu, Nov 21, 2019 at 09:17:50AM +0000, Kieran Bingham wrote: > On 21/11/2019 04:19, Laurent Pinchart wrote: > > Commit 965c5bf7fbf5 ("meson: Define _FORTIFY_SOURCE for optimised > > builds") tried to define _FORTIFY_SOURCE for optimised builds with > > clang, but updated the common_arguments after it was used. This resulted > > in the _FORTIFY_SOURCE option not being applied. Fix it. > > > > Fixes: 965c5bf7fbf5 ("meson: Define _FORTIFY_SOURCE for optimised builds") > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > meson.build | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/meson.build b/meson.build > > index 72ad7c8b493b..0a222ba96dcb 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -35,8 +35,8 @@ common_arguments = [ > > '-include', 'config.h', > > ] > > > > -c_arguments = common_arguments > > -cpp_arguments = common_arguments > > +c_arguments = [] > > +cpp_arguments = [] > > c_arguments and cpp_arguments are not used until after the code block > below. Do we really need to define them as empty arrays here? > > I /think/ we can just move the assignment to below? > (where you do an addition instead) I could do that for cpp_arguments, but cpp_arguments is extended in the if cc.get_id() == 'clang' block. I could assign it there, but then the += might complain later. I'd rather keep an empty array at the beginning and += everywhere to make it easily extensible and avoid future issues. > > if cc.get_id() == 'clang' > > # Turn _FORTIFY_SOURCE by default on optimised builds (as it requires -O1 > > @@ -56,6 +56,9 @@ if cc.get_id() == 'clang' > > endif > > endif > > > > +c_arguments += common_arguments > > +cpp_arguments += common_arguments > > +> add_project_arguments(c_arguments, language : 'c') > > Eeep - and that's so blindingly obvious when you look at 965c5bf7fbf5, > as this line is clearly in the hunk. I'm sorry I missed it. > > (of course it's always easier to spot something that you know is there). > > With either the empty arrays declared above if that's necessary (or just > preferred) or directly assigned lower down if it's appropriate: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > add_project_arguments(cpp_arguments, language : 'cpp') > > add_project_link_arguments(cpp_arguments, language : 'cpp') > >
Hi Laurent, Thanks for your work. On 2019-11-21 06:19:13 +0200, Laurent Pinchart wrote: > Commit 965c5bf7fbf5 ("meson: Define _FORTIFY_SOURCE for optimised > builds") tried to define _FORTIFY_SOURCE for optimised builds with > clang, but updated the common_arguments after it was used. This resulted > in the _FORTIFY_SOURCE option not being applied. Fix it. > > Fixes: 965c5bf7fbf5 ("meson: Define _FORTIFY_SOURCE for optimised builds") > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > meson.build | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/meson.build b/meson.build > index 72ad7c8b493b..0a222ba96dcb 100644 > --- a/meson.build > +++ b/meson.build > @@ -35,8 +35,8 @@ common_arguments = [ > '-include', 'config.h', > ] > > -c_arguments = common_arguments > -cpp_arguments = common_arguments > +c_arguments = [] > +cpp_arguments = [] > > if cc.get_id() == 'clang' > # Turn _FORTIFY_SOURCE by default on optimised builds (as it requires -O1 > @@ -56,6 +56,9 @@ if cc.get_id() == 'clang' > endif > endif > > +c_arguments += common_arguments > +cpp_arguments += common_arguments > + > add_project_arguments(c_arguments, language : 'c') > add_project_arguments(cpp_arguments, language : 'cpp') > add_project_link_arguments(cpp_arguments, language : 'cpp') > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On 21/11/2019 09:33, Laurent Pinchart wrote: > Hi Kieran, > > On Thu, Nov 21, 2019 at 09:17:50AM +0000, Kieran Bingham wrote: >> On 21/11/2019 04:19, Laurent Pinchart wrote: >>> Commit 965c5bf7fbf5 ("meson: Define _FORTIFY_SOURCE for optimised >>> builds") tried to define _FORTIFY_SOURCE for optimised builds with >>> clang, but updated the common_arguments after it was used. This resulted >>> in the _FORTIFY_SOURCE option not being applied. Fix it. >>> >>> Fixes: 965c5bf7fbf5 ("meson: Define _FORTIFY_SOURCE for optimised builds") >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> meson.build | 7 +++++-- >>> 1 file changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/meson.build b/meson.build >>> index 72ad7c8b493b..0a222ba96dcb 100644 >>> --- a/meson.build >>> +++ b/meson.build >>> @@ -35,8 +35,8 @@ common_arguments = [ >>> '-include', 'config.h', >>> ] >>> >>> -c_arguments = common_arguments >>> -cpp_arguments = common_arguments >>> +c_arguments = [] >>> +cpp_arguments = [] >> >> c_arguments and cpp_arguments are not used until after the code block >> below. Do we really need to define them as empty arrays here? >> >> I /think/ we can just move the assignment to below? >> (where you do an addition instead) > > I could do that for cpp_arguments, but cpp_arguments is extended in the > if cc.get_id() == 'clang' block. I could assign it there, but then the > += might complain later. I'd rather keep an empty array at the beginning > and += everywhere to make it easily extensible and avoid future issues. Declaring early is good. Looks like it can be pushed :D -- Kieran > >>> if cc.get_id() == 'clang' >>> # Turn _FORTIFY_SOURCE by default on optimised builds (as it requires -O1 >>> @@ -56,6 +56,9 @@ if cc.get_id() == 'clang' >>> endif >>> endif >>> >>> +c_arguments += common_arguments >>> +cpp_arguments += common_arguments >>> +> add_project_arguments(c_arguments, language : 'c') >> >> Eeep - and that's so blindingly obvious when you look at 965c5bf7fbf5, >> as this line is clearly in the hunk. I'm sorry I missed it. >> >> (of course it's always easier to spot something that you know is there). >> >> With either the empty arrays declared above if that's necessary (or just >> preferred) or directly assigned lower down if it's appropriate: >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> add_project_arguments(cpp_arguments, language : 'cpp') >>> add_project_link_arguments(cpp_arguments, language : 'cpp') >>> >
diff --git a/meson.build b/meson.build index 72ad7c8b493b..0a222ba96dcb 100644 --- a/meson.build +++ b/meson.build @@ -35,8 +35,8 @@ common_arguments = [ '-include', 'config.h', ] -c_arguments = common_arguments -cpp_arguments = common_arguments +c_arguments = [] +cpp_arguments = [] if cc.get_id() == 'clang' # Turn _FORTIFY_SOURCE by default on optimised builds (as it requires -O1 @@ -56,6 +56,9 @@ if cc.get_id() == 'clang' endif endif +c_arguments += common_arguments +cpp_arguments += common_arguments + add_project_arguments(c_arguments, language : 'c') add_project_arguments(cpp_arguments, language : 'cpp') add_project_link_arguments(cpp_arguments, language : 'cpp')
Commit 965c5bf7fbf5 ("meson: Define _FORTIFY_SOURCE for optimised builds") tried to define _FORTIFY_SOURCE for optimised builds with clang, but updated the common_arguments after it was used. This resulted in the _FORTIFY_SOURCE option not being applied. Fix it. Fixes: 965c5bf7fbf5 ("meson: Define _FORTIFY_SOURCE for optimised builds") Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- meson.build | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)