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.