diff mbox series

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

Message ID 20240816-function-v3-1-32ff225e550e@daynix.com (mailing list archive)
State New, archived
Headers show
Series [v3] meson: Use -fno-sanitize=function when available | expand

Commit Message

Akihiko Odaki Aug. 16, 2024, 6:22 a.m. UTC
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(+)


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

Best regards,

Comments

Thomas Huth Aug. 16, 2024, 7:03 a.m. UTC | #1
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
Akihiko Odaki Aug. 16, 2024, 7:12 a.m. UTC | #2
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
Thomas Huth Aug. 16, 2024, 7:27 a.m. UTC | #3
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
Akihiko Odaki Aug. 16, 2024, 7:30 a.m. UTC | #4
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
Thomas Huth Aug. 16, 2024, 8:03 a.m. UTC | #5
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
Richard Henderson Aug. 16, 2024, 8:05 a.m. UTC | #6
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~
Akihiko Odaki Aug. 16, 2024, 8:21 a.m. UTC | #7
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.
Thomas Huth Aug. 16, 2024, 8:24 a.m. UTC | #8
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
Akihiko Odaki Aug. 16, 2024, 8:27 a.m. UTC | #9
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.
Richard Henderson Aug. 16, 2024, 8:46 a.m. UTC | #10
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~
Akihiko Odaki Aug. 16, 2024, 8:51 a.m. UTC | #11
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
Thomas Huth Aug. 16, 2024, 8:51 a.m. UTC | #12
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
diff mbox series

Patch

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)