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

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

[Akihiko Odaki]

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')
 
 add_global_arguments(qemu_common_flags, native: false, language: all_languages)
 add_global_link_arguments(qemu_ldflags, native: false, language: all_languages)

---
base-commit: 93b799fafd9170da3a79a533ea6f73a18de82e22
change-id: 20240714-function-7d32c723abbc

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

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

[Thomas Huth]

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?

  Thomas

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

[Akihiko Odaki]

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.

Regards,
Akihiko Odaki

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

[Thomas Huth]

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...?

  Thomas

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

[Akihiko Odaki]

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.

Regards,
Akihiko Odaki

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

[Thomas Huth]

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.

  Thomas

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

[Akihiko Odaki]

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.

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

[Thomas Huth]

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 ?

  Thomas

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

[Akihiko Odaki]

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.

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

[Richard Henderson]

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.


r~

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

[Akihiko Odaki]

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

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

[Thomas Huth]

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

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

[Richard Henderson]

On 8/16/24 17: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...?

AFAICS, -fsanitize=function is enabled by -fsanitize=undefined.

Anyway, see also

https://lore.kernel.org/qemu-devel/20240813095216.306555-1-richard.henderson@linaro.org/


r~