[libcamera-devel] [PATCH] meson: Really define _FORTIFY_SOURCE for optimised builds

[Kieran Bingham]

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 at 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 at ideasonboard.com>
>>
>>>  add_project_arguments(cpp_arguments, language : 'cpp')
>>>  add_project_link_arguments(cpp_arguments, language : 'cpp')
>>>
> 

-- 
Regards
--
Kieran