| Message ID | 20210926220103.1721355-2-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | user-mode: Avoid processing unnecessary meson rules | expand |
On 9/26/21 6:01 PM, Philippe Mathieu-Daudé wrote: > Reported-by: Warner Losh <imp@bsdimp.com> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > bsd-user/meson.build | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/bsd-user/meson.build b/bsd-user/meson.build > index 03695493408..a7607e1c884 100644 > --- a/bsd-user/meson.build > +++ b/bsd-user/meson.build > @@ -1,3 +1,7 @@ > +if not config_host.has_key('CONFIG_BSD') > + subdir_done() > +endif Why here and not in the parent meson.build? r~
On Sun, Sep 26, 2021 at 5:08 PM Richard Henderson < richard.henderson@linaro.org> wrote: > On 9/26/21 6:01 PM, Philippe Mathieu-Daudé wrote: > > Reported-by: Warner Losh <imp@bsdimp.com> > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > bsd-user/meson.build | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/bsd-user/meson.build b/bsd-user/meson.build > > index 03695493408..a7607e1c884 100644 > > --- a/bsd-user/meson.build > > +++ b/bsd-user/meson.build > > @@ -1,3 +1,7 @@ > > +if not config_host.has_key('CONFIG_BSD') > > + subdir_done() > > +endif > > Why here and not in the parent meson.build? > I have the same question... I never know where to 'filter' when it comes to meson... Waner
On 9/27/21 06:01, Philippe Mathieu-Daudé wrote: > Reported-by: Warner Losh <imp@bsdimp.com> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > bsd-user/meson.build | 4 ++++ > 1 file changed, 4 insertions(+) I'm newcomer here, but this is just 4 lines of Meson, and two similar usages already exist within QEMU proper so... Reviewed-by: WANG Xuerui <git@xen0n.name>
On 9/27/21 10:42, WANG Xuerui wrote: > On 9/27/21 06:01, Philippe Mathieu-Daudé wrote: >> Reported-by: Warner Losh <imp@bsdimp.com> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> bsd-user/meson.build | 4 ++++ >> 1 file changed, 4 insertions(+) > > I'm newcomer here, but this is just 4 lines of Meson, and two similar > usages already exist within QEMU proper so... > > Reviewed-by: WANG Xuerui <git@xen0n.name> > > Hmm, other replies pointed out that one can also make the top-level subdir() call conditional... And we seem to have more than 2 usages following the latter pattern instead. However putting the conditional in sub-directory makes the logic localized, which is arguably more friendly to someone casually browsing the source code. As both ways work, I think maybe being consistent and moving things to top-level meson.build has more value?
On 9/27/21 01:08, Richard Henderson wrote: > On 9/26/21 6:01 PM, Philippe Mathieu-Daudé wrote: >> Reported-by: Warner Losh <imp@bsdimp.com> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> bsd-user/meson.build | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/bsd-user/meson.build b/bsd-user/meson.build >> index 03695493408..a7607e1c884 100644 >> --- a/bsd-user/meson.build >> +++ b/bsd-user/meson.build >> @@ -1,3 +1,7 @@ >> +if not config_host.has_key('CONFIG_BSD') >> + subdir_done() >> +endif > > Why here and not in the parent meson.build? This is what Paolo recommended me to do last time I added a conditional inclusion. Personally I prefer having it in the call site rather than the callee (no need to read the callee to notice it isn't called). I guess this is for readability, to not clutter meson.build files more... Paolo, what is your preference?
On Sun, 26 Sept 2021 at 23:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > Reported-by: Warner Losh <imp@bsdimp.com> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > bsd-user/meson.build | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/bsd-user/meson.build b/bsd-user/meson.build > index 03695493408..a7607e1c884 100644 > --- a/bsd-user/meson.build > +++ b/bsd-user/meson.build > @@ -1,3 +1,7 @@ > +if not config_host.has_key('CONFIG_BSD') > + subdir_done() > +endif > + > bsd_user_ss.add(files( > 'bsdload.c', > 'elfload.c', So, what's the reason for this change? The commit messages and the cover letter don't really explain it. Is this fixing a bug (if so what?), a precaution to avoid possible future bugs, fixing a performance issue with how long meson takes to run (if so, how much effect does this have), or something else? thanks -- PMM
On Mon, Sep 27, 2021 at 11:15 AM Peter Maydell <peter.maydell@linaro.org> wrote: > On Sun, 26 Sept 2021 at 23:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > Reported-by: Warner Losh <imp@bsdimp.com> > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > bsd-user/meson.build | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/bsd-user/meson.build b/bsd-user/meson.build > > index 03695493408..a7607e1c884 100644 > > --- a/bsd-user/meson.build > > +++ b/bsd-user/meson.build > > @@ -1,3 +1,7 @@ > > +if not config_host.has_key('CONFIG_BSD') > > + subdir_done() > > +endif > > + > > bsd_user_ss.add(files( > > 'bsdload.c', > > 'elfload.c', > > > So, what's the reason for this change? https://lore.kernel.org/qemu-devel/CANCZdfprC16ezJQCWJmYEApX6eym9nxSOqAtBAGr+cziS4r2qw@mail.gmail.com/ linux-user/meson.build is evaluated on bsd, and bsd-user/meson.build on Linux. > The commit messages and > the cover letter don't really explain it. Is this fixing a bug > (if so what?), a precaution to avoid possible future bugs, > fixing a performance issue with how long meson takes to run (if > so, how much effect does this have), or something else? I'll wait for feedback from Paolo, then work on the explanation. Regards, Phil.
On Mon, Sep 27, 2021 at 12:01:02AM +0200, Philippe Mathieu-Daudé wrote: > Reported-by: Warner Losh <imp@bsdimp.com> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > bsd-user/meson.build | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/bsd-user/meson.build b/bsd-user/meson.build > index 03695493408..a7607e1c884 100644 > --- a/bsd-user/meson.build > +++ b/bsd-user/meson.build > @@ -1,3 +1,7 @@ > +if not config_host.has_key('CONFIG_BSD') > + subdir_done() > +endif > + > bsd_user_ss.add(files( > 'bsdload.c', > 'elfload.c', If we look at the big picture across the root meson.build, and this meson.build we have bsd_user_ss = ss.source_set() ... bsd_user_ss.add(files( 'bsdload.c', 'elfload.c', 'main.c', 'mmap.c', 'signal.c', 'strace.c', 'syscall.c', 'uaccess.c', )) ... bsd_user_ss.add(files('gdbstub.c')) specific_ss.add_all(when: 'CONFIG_BSD_USER', if_true: bsd_user_ss) So without this change, we're already correctly dropping bsd_user_ss in its entirity, when not on BSD. With this change, we're dropping some, but not all, of bsd_user_ss files - gdbstub.c remains. So this change on its own doesn't make a whole lot of sense. If we look at linux-user/meson.build though things are more complex. There we have alot of sub-dirs, and meson.biuld in those dirs adds generators for various files. So conceivably skipping linux-user will mean we won't auto-generate files we don't need on non-Linux. With that in mind, I think it makes conceptual sense to have this bsd-user/meson.build change, for the purpose of design consistency, even if it doesn't have any real world benefit for bsd-user today. Regards, Daniel
On Mon, 27 Sept 2021 at 10:40, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On Mon, Sep 27, 2021 at 11:15 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Sun, 26 Sept 2021 at 23:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > Reported-by: Warner Losh <imp@bsdimp.com> > > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > > --- > > > bsd-user/meson.build | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/bsd-user/meson.build b/bsd-user/meson.build > > > index 03695493408..a7607e1c884 100644 > > > --- a/bsd-user/meson.build > > > +++ b/bsd-user/meson.build > > > @@ -1,3 +1,7 @@ > > > +if not config_host.has_key('CONFIG_BSD') > > > + subdir_done() > > > +endif > > > + > > > bsd_user_ss.add(files( > > > 'bsdload.c', > > > 'elfload.c', > > > > > > So, what's the reason for this change? > > https://lore.kernel.org/qemu-devel/CANCZdfprC16ezJQCWJmYEApX6eym9nxSOqAtBAGr+cziS4r2qw@mail.gmail.com/ > > linux-user/meson.build is evaluated on bsd, and bsd-user/meson.build on Linux. True, but "meson.build is evaluated but just does nothing or adds files to a sourceset that isn't used" is pretty common (hw/pci/meson.build is evaluated even if we're not building a system with PCI support, for example). If there's a reason why bsd-user and linux-user are special we should explain it, I think. thanks -- PMM
On Mon, 27 Sept 2021 at 10:59, Daniel P. Berrangé <berrange@redhat.com> wrote: > If we look at linux-user/meson.build though things are more complex. > There we have alot of sub-dirs, and meson.biuld in those dirs adds > generators for various files. So conceivably skipping linux-user > will mean we won't auto-generate files we don't need on non-Linux. The top level meson.build doesn't call process on the syscall_nr_generators[] list unless we're building linux-user targets, so I don't think we will auto-generate anything we don't need. -- PMM
On Mon, Sep 27, 2021 at 4:08 AM Peter Maydell <peter.maydell@linaro.org> wrote: > On Mon, 27 Sept 2021 at 10:59, Daniel P. Berrangé <berrange@redhat.com> > wrote: > > If we look at linux-user/meson.build though things are more complex. > > There we have alot of sub-dirs, and meson.biuld in those dirs adds > > generators for various files. So conceivably skipping linux-user > > will mean we won't auto-generate files we don't need on non-Linux. > > The top level meson.build doesn't call process on the > syscall_nr_generators[] list unless we're building linux-user > targets, so I don't think we will auto-generate anything we > don't need. > The problem is that I'm trying to add some os-specific files to the bsd-user in a patch set I sent off. bsd-user compiles for different BSDs, and I'd like to start pulling in per-bsd files as I'm upstreaming. To do that, I have this construct in the bsd-user/meson.build file: # Pull in the OS-specific build glue, if any if fs.exists(targetos) subdir(targetos) endif primarily because the builds failed on Linux when it tried to find the non-existent bsd-user/linunx directory. The question came up on how to cope with this situation, and Philippe sent off this series as an alternative to that construct. The whole reason why we descend into the linux-user directory on BSD and into the bsd-user directory on linux is unclear to me as well. So this is preparing the ground for my future work of upstreaming. I'm agnostic as to how it's resolved, but it needs to be resolved. Warner
Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > On Mon, Sep 27, 2021 at 11:15 AM Peter Maydell <peter.maydell@linaro.org> wrote: >> On Sun, 26 Sept 2021 at 23:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> > Reported-by: Warner Losh <imp@bsdimp.com> >> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> > --- >> > bsd-user/meson.build | 4 ++++ >> > 1 file changed, 4 insertions(+) >> > >> > diff --git a/bsd-user/meson.build b/bsd-user/meson.build >> > index 03695493408..a7607e1c884 100644 >> > --- a/bsd-user/meson.build >> > +++ b/bsd-user/meson.build >> > @@ -1,3 +1,7 @@ >> > +if not config_host.has_key('CONFIG_BSD') >> > + subdir_done() >> > +endif >> > + >> > bsd_user_ss.add(files( >> > 'bsdload.c', >> > 'elfload.c', >> >> >> So, what's the reason for this change? > > https://lore.kernel.org/qemu-devel/CANCZdfprC16ezJQCWJmYEApX6eym9nxSOqAtBAGr+cziS4r2qw@mail.gmail.com/ > > linux-user/meson.build is evaluated on bsd, and bsd-user/meson.build on Linux. > >> The commit messages and >> the cover letter don't really explain it. Is this fixing a bug >> (if so what?), a precaution to avoid possible future bugs, >> fixing a performance issue with how long meson takes to run (if >> so, how much effect does this have), or something else? > > I'll wait for feedback from Paolo, then work on the explanation. Ping Paolo?
On 27/09/21 07:24, Philippe Mathieu-Daudé wrote: >> Why here and not in the parent meson.build? > This is what Paolo recommended me to do last time I added a > conditional inclusion. > > Personally I prefer having it in the call site rather than > the callee (no need to read the callee to notice it isn't > called). I guess this is for readability, to not clutter > meson.build? files more... Yes, pretty much. In this case it's quite obvious that bsd-user is BSD-only, but I prefer it if dir/meson.build has the knowledge of what goes on in dir/. That said, we're not terribly consistent, see have_block and have_tools, so either will be okay. Paolo > Paolo, what is your preference? >
On 27/09/21 11:54, Peter Maydell wrote: > True, but "meson.build is evaluated but just does nothing or > adds files to a sourceset that isn't used" is pretty common > (hw/pci/meson.build is evaluated even if we're not building > a system with PCI support, for example). Selection of files from hw/pci/meson.build is based on per-target definitions, so there's no way around when:/if_true:. (Technically, hw/pci/meson.build also as an if_false, so there's *really* no way around parsing it). Instead, when the definition is constant across all targets, it is possible to use either when:/if_true: or an "if" as in if have_user util_ss.add(files('selfmap.c')) endif or the various "if m[0].found()" found in directories that build shared modules. In this case I personally lean more towards the latter, but when:/if_true: is a little more compact so both are acceptable. Paolo
On 27/09/21 11:52, Daniel P. Berrangé wrote: > bsd_user_ss.add(files('gdbstub.c')) > specific_ss.add_all(when: 'CONFIG_BSD_USER', if_true: bsd_user_ss) > > > So without this change, we're already correctly dropping bsd_user_ss > in its entirity, when not on BSD. > > With this change, we're dropping some, but not all, of bsd_user_ss > files - gdbstub.c remains. > > So this change on its own doesn't make a whole lot of sense. Agreed; that said, the gdbstub.c files added by bsd_user_ss.add(files('gdbstub.c')) linux_user_ss.add(files('gdbstub.c', 'thunk.c')) are unnecessary because there is already specific_ss.add(files('cpu.c', 'disas.c', 'gdbstub.c'), capstone) So, with those two instances of gdbstub.c removed, both patches are Acked-by: Paolo Bonzini <pbonzini@redhat.com> Thanks, Paolo
Le 05/10/2021 à 21:26, Paolo Bonzini a écrit : > On 27/09/21 11:52, Daniel P. Berrangé wrote: >> bsd_user_ss.add(files('gdbstub.c')) >> specific_ss.add_all(when: 'CONFIG_BSD_USER', if_true: bsd_user_ss) >> >> >> So without this change, we're already correctly dropping bsd_user_ss >> in its entirity, when not on BSD. >> >> With this change, we're dropping some, but not all, of bsd_user_ss >> files - gdbstub.c remains. >> >> So this change on its own doesn't make a whole lot of sense. > > Agreed; that said, the gdbstub.c files added by > > bsd_user_ss.add(files('gdbstub.c')) > linux_user_ss.add(files('gdbstub.c', 'thunk.c')) > > are unnecessary because there is already > > specific_ss.add(files('cpu.c', 'disas.c', 'gdbstub.c'), capstone) > > So, with those two instances of gdbstub.c removed, both patches are > > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > I can take them if a v2 is sent updated accordingly... Thanks, Laurent
On Tue, Oct 5, 2021 at 2:41 PM Laurent Vivier <laurent@vivier.eu> wrote: > Le 05/10/2021 à 21:26, Paolo Bonzini a écrit : > > On 27/09/21 11:52, Daniel P. Berrangé wrote: > >> bsd_user_ss.add(files('gdbstub.c')) > >> specific_ss.add_all(when: 'CONFIG_BSD_USER', if_true: bsd_user_ss) > >> > >> > >> So without this change, we're already correctly dropping bsd_user_ss > >> in its entirity, when not on BSD. > >> > >> With this change, we're dropping some, but not all, of bsd_user_ss > >> files - gdbstub.c remains. > >> > >> So this change on its own doesn't make a whole lot of sense. > > > > Agreed; that said, the gdbstub.c files added by > > > > bsd_user_ss.add(files('gdbstub.c')) > > linux_user_ss.add(files('gdbstub.c', 'thunk.c')) > > > > are unnecessary because there is already > > > > specific_ss.add(files('cpu.c', 'disas.c', 'gdbstub.c'), capstone) > > > > So, with those two instances of gdbstub.c removed, both patches are > > > > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > > > > I can take them if a v2 is sent updated accordingly... > I'd planned on rolling them into the bsd-user patch set that I'd been working on that is almost ready to go in (this is I think the only remaining issue with the patch train, though I need to double check threads). I'd planned on doing that tomorrow, but would be happy to coordinate with you since one of them does touch linux-user. Warner
Le 05/10/2021 à 22:46, Warner Losh a écrit : > > > On Tue, Oct 5, 2021 at 2:41 PM Laurent Vivier <laurent@vivier.eu <mailto:laurent@vivier.eu>> wrote: > > Le 05/10/2021 à 21:26, Paolo Bonzini a écrit : > > On 27/09/21 11:52, Daniel P. Berrangé wrote: > >> bsd_user_ss.add(files('gdbstub.c')) > >> specific_ss.add_all(when: 'CONFIG_BSD_USER', if_true: bsd_user_ss) > >> > >> > >> So without this change, we're already correctly dropping bsd_user_ss > >> in its entirity, when not on BSD. > >> > >> With this change, we're dropping some, but not all, of bsd_user_ss > >> files - gdbstub.c remains. > >> > >> So this change on its own doesn't make a whole lot of sense. > > > > Agreed; that said, the gdbstub.c files added by > > > > bsd_user_ss.add(files('gdbstub.c')) > > linux_user_ss.add(files('gdbstub.c', 'thunk.c')) > > > > are unnecessary because there is already > > > > specific_ss.add(files('cpu.c', 'disas.c', 'gdbstub.c'), capstone) > > > > So, with those two instances of gdbstub.c removed, both patches are > > > > Acked-by: Paolo Bonzini <pbonzini@redhat.com <mailto:pbonzini@redhat.com>> > > > > I can take them if a v2 is sent updated accordingly... > > > I'd planned on rolling them into the bsd-user patch set that I'd been working on > that is almost ready to go in (this is I think the only remaining issue with the patch > train, though I need to double check threads). I'd planned on doing that tomorrow, > but would be happy to coordinate with you since one of them does touch linux-user. You can take both. Thanks, Laurent
diff --git a/bsd-user/meson.build b/bsd-user/meson.build index 03695493408..a7607e1c884 100644 --- a/bsd-user/meson.build +++ b/bsd-user/meson.build @@ -1,3 +1,7 @@ +if not config_host.has_key('CONFIG_BSD') + subdir_done() +endif + bsd_user_ss.add(files( 'bsdload.c', 'elfload.c',
Reported-by: Warner Losh <imp@bsdimp.com> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- bsd-user/meson.build | 4 ++++ 1 file changed, 4 insertions(+)