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

[Thomas Huth <thuth@redhat.com>]

On 16/08/2024 10.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.

I think I tend to disagree here. If users enabled saitize=undefined it with 
--extra-cflags, they also have to disable sanitize=function with 
--extra-cflags, so listing this unconditionally in our meson.build is a red 
herring when people are looking at the file.

  Thomas