| Message ID | 20240816-function-v3-1-32ff225e550e@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] meson: Use -fno-sanitize=function when available | expand |
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
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
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
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
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
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~
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.
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
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.
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~
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
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 --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)
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,