Re: [PATCH v3] meson: Use -fno-sanitize=function when available

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2024/08/16 17:46, Richard Henderson wrote:
> On 8/16/24 18:27, Akihiko Odaki wrote:
>> On 2024/08/16 17:24, Thomas Huth wrote:
>>> On 16/08/2024 10.21, Akihiko Odaki wrote:
>>>> On 2024/08/16 17:03, Thomas Huth wrote:
>>>>> On 16/08/2024 09.30, Akihiko Odaki wrote:
>>>>>> On 2024/08/16 16:27, Thomas Huth wrote:
>>>>>>> On 16/08/2024 09.12, Akihiko Odaki wrote:
>>>>>>>> On 2024/08/16 16:03, Thomas Huth wrote:
>>>>>>>>> On 16/08/2024 08.22, Akihiko Odaki wrote:
>>>>>>>>>> Commit 23ef50ae2d0c (".gitlab-ci.d/buildtest.yml: Use
>>>>>>>>>> -fno-sanitize=function in the clang-system job") adds
>>>>>>>>>> -fno-sanitize=function for the CI but doesn't add the flag in the
>>>>>>>>>> other context. Add it to meson.build for such. It is not 
>>>>>>>>>> removed from
>>>>>>>>>> .gitlab-ci.d/buildtest.yml because -fno-sanitize=function in 
>>>>>>>>>> meson.build
>>>>>>>>>> does not affect --extra-cflags due to argument ordering.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>>>>>>> ---
>>>>>>>>>> Changes in v3:
>>>>>>>>>> - I was not properly dropping the change of 
>>>>>>>>>> .gitlab-ci.d/buildtest.yml
>>>>>>>>>>    but only updated the message. v3 fixes this. (Thomas Huth)
>>>>>>>>>> - Link to v2: https://lore.kernel.org/r/20240729-function- 
>>>>>>>>>> v2-1-2401ab18b30b@daynix.com
>>>>>>>>>>
>>>>>>>>>> Changes in v2:
>>>>>>>>>> - Dropped the change of: .gitlab-ci.d/buildtest.yml
>>>>>>>>>> - Link to v1: 
>>>>>>>>>> https://lore.kernel.org/r/20240714-function-v1-1- 
>>>>>>>>>> cc2acb4171ba@daynix.com
>>>>>>>>>> ---
>>>>>>>>>>   meson.build | 1 +
>>>>>>>>>>   1 file changed, 1 insertion(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/meson.build b/meson.build
>>>>>>>>>> index 5613b62a4f42..a4169c572ba9 100644
>>>>>>>>>> --- a/meson.build
>>>>>>>>>> +++ b/meson.build
>>>>>>>>>> @@ -609,6 +609,7 @@ if host_os != 'openbsd' and \
>>>>>>>>>>   endif
>>>>>>>>>>   qemu_common_flags += 
>>>>>>>>>> cc.get_supported_arguments(hardening_flags)
>>>>>>>>>> +qemu_common_flags += 
>>>>>>>>>> cc.get_supported_arguments('-fno-sanitize=function')
>>>>>>>>>
>>>>>>>>> As I mentioned in my last mail: I think it would make sense to 
>>>>>>>>> move this at the end of the "if get_option('tsan')" block in 
>>>>>>>>> meson.build, since this apparently only fixes the use of 
>>>>>>>>> "--enable-sanitizers", and cannot fix the "--extra-cflags" that 
>>>>>>>>> a user might have specified?
>>>>>>>>
>>>>>>>> Sorry, I missed it. It cannot fix --extra-cflags, but it should 
>>>>>>>> be able to fix compiler flags specified by compiler distributor.
>>>>>>>
>>>>>>> Oh, you mean that there are distros that enable 
>>>>>>> -fsanitize=function by default? Can you name one? If so, I think 
>>>>>>> that information should go into the patch description...?
>>>>>>
>>>>>> No, it is just a precaution.
>>>>>
>>>>> Ok. I don't think any normal distro will enable this by default 
>>>>> since this impacts performance of the programs, so it's either the 
>>>>> user specifying --enable-sanitizers or the user specifying 
>>>>> --extra-cflags="-fsanitize=...". In the latter case, your patch 
>>>>> does not help. In the former case, I think this setting should go 
>>>>> into the same code block as where we set -fsanitize=undefined in 
>>>>> our meson.build file, so that it is clear where it belongs to.
>>>>
>>>> It does not look like -fno-sanitize=function belongs to the code 
>>>> block to me. Putting - fno-sanitize=function in the code block will 
>>>> make it seem to say that we should disable function sanitizer 
>>>> because the user requests to enable sanitizers, which makes little 
>>>> sense.
>>>
>>> As far as I understood, -fsanitize=undefine turns on 
>>> -fsanitize=function, too, or did I get that wrong?
>>> If not, how did you run into this problem? How did you enable the 
>>> function sanitizer if not using --enable-sanitizers ?
>>
>> The point is we don't care who enables sanitizers, and unconditonally 
>> setting -fno- sanitize=function will clarify that.
>>
> 
> Argument ordering is important.  You cannot just drop this in the middle 
> of meson.build and expect anything reasonable to happen.

That is a good point. We should add -fno-sanitize=function immediately 
after -fsanitize=undefined; I will submit v4 with that change.

Regards,
Akihiko Odaki