Re: [PATCH v2 1/1] qemu-build: Add a qemu build option for enabling debug

[Fan Ni <fan.ni@gmx.us>]

On Fri, Aug 25, 2023 at 12:11:50PM -0700, Luis Chamberlain wrote:
> On Fri, Aug 25, 2023 at 11:17:44AM -0700, fan.ni@gmx.us wrote:
> > From: Fan Ni <fan.ni@gmx.us>
> >
> > Currently, qemu will always be compiled without enabling debug, which
> > means we will not be able to debug qemu source code with gdb. However,
> > building qemu with debug information included can be useful for qemu
> > development purpose. A new Kconfig option is added to allow user to enable
> > debug when build qemu. By default, it is disabled.
>
> Commit log looks good now.
>
> >
> > Signed-off-by: Fan Ni <fan.ni@samsung.com>
> > ---
> >  Makefile.build_qemu                          | 4 ++++
> >  playbooks/roles/build_qemu/defaults/main.yml | 1 +
> >  playbooks/roles/build_qemu/tasks/main.yml    | 2 +-
> >  vagrant/Kconfig                              | 8 ++++++++
> >  4 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile.build_qemu b/Makefile.build_qemu
> > index 7addd2f0..f2dad0ae 100644
> > --- a/Makefile.build_qemu
> > +++ b/Makefile.build_qemu
> > @@ -19,6 +19,10 @@ ifeq (y,$(CONFIG_TARGET_ARCH_PPC64LE))
> >  QEMU_BUILD_SETUP_ARGS += qemu_target="ppc64-softmmu"
> >  endif
> >
> > +ifeq (y,$(CONFIG_QEMU_BUILD_WITH_DEBUG_ENABLED))
> > +QEMU_BUILD_SETUP_ARGS += qemu_build_debug="--enable-debug"
> > +endif
> > +
>
> The variable can be qemu_build_debug_enable instead, and this
> should be replaced with a bool as well so 'True'.
>
> >  qemu: $(KDEVOPS_EXTRA_VARS)
> >  	@$(Q)ansible-playbook $(ANSIBLE_VERBOSE) --connection=local \
> >  		--inventory localhost, \
> > diff --git a/playbooks/roles/build_qemu/defaults/main.yml b/playbooks/roles/build_qemu/defaults/main.yml
> > index 006d669c..794cbd5f 100644
> > --- a/playbooks/roles/build_qemu/defaults/main.yml
> > +++ b/playbooks/roles/build_qemu/defaults/main.yml
> > @@ -12,3 +12,4 @@ qemu_git: "https://github.com/qemu/qemu.git"
> >  qemu_version: "v7.2.0-rc4"
> >  qemu_build_dir: "{{ qemu_data }}/build"
> >  qemu_target: "x86_64-softmmu"
> > +qemu_build_debug: False
>
> Note this is a bool, please rename it and add something like:
>
> qemu_build_debug_enable: False
> qemu_build_debug_enable_string: "--enable-debug"
> qemu_build_extra_strings: ""

Do we really need to add two variables for that? It seems we add an
unnecessary layer of check with qemu_build_debug_enable. I think we just need
a string like qemu_build_debug_enable_string. And leave it as "" here.
And when the configuration is turned on, set qemu_build_debug_enable_string to
"--enable-debug" and add build_debug_enable_string to the below
./configure command.

Fan

>
> > diff --git a/playbooks/roles/build_qemu/tasks/main.yml b/playbooks/roles/build_qemu/tasks/main.yml
> > index c826c9cf..d3427c56 100644
> > --- a/playbooks/roles/build_qemu/tasks/main.yml
> > +++ b/playbooks/roles/build_qemu/tasks/main.yml
> > @@ -74,7 +74,7 @@
> >      - build_qemu_now|bool
> >
> >  - name: Run configure for QEMU
> > -  command: "./configure --target-list={{ qemu_target }} --disable-download"
> > +  command: "./configure --target-list={{ qemu_target }} --disable-download {{qemu_build_debug}}"
> >    tags: [ 'qemu', 'configure' ]
> >    args:
> >      chdir: "{{ qemu_data }}"
>
> The way you have it this would run the following by default:
>
> ./configure --target-list=x86_64-softmmu --disable-download False
>
> Where I think you want:
>
> ./configure --target-list=x86_64-softmmu --disable-download
>
> A future goal is to modify kconfig to support outputting the yaml file
> so we don't have to muck around with the Makefiles to exand on the
> variables to let Makefile magic create our extra_vars.yaml file.
> When that happens we won't ever have to edit Makefiles to check
> for CONFIG variables and expand on things like QEMU_BUILD_SETUP_ARGS
> which get appended to in the end ANSIBLE_EXTRA_ARGS. This is now
> documented here:
>
> https://github.com/linux-kdevops/kdevops/blob/master/docs/how-extra-vars-generated.md
>
> Since our goal is to modify kconfig [0] to support outputting yaml file in
> addition to the .config file, the question would be for us:
>
>   what do we do today to minimize the amount of changes *today* so that when
>   this support does come through we won't have to go and remove all the
>   Makefile magic, or reduce that delta for new code?
>
> I think the answer for the above would be for you to add an ansible
> set_fact task which does the appending of the newly suggested default
> empty string I made qemu_build_extra_strings with qemu_build_debug_enable_string
> if qemu_build_debug_enable. For an example of a set_fact task which depends on
> a bool you can check commit ed8c04809df15c ("bootlinux: add support for
> "config-kdevops").
>
> With this, in the future, if you find the need, you could even
> dynamically expand the build arguments, so that if you run something
> like 'make qemu-configure' you could add arguments at the end to pass to
> ansible, like:
>
> make qemu-configure EXTRA_CONFIGURE_ARGS="--enable-foo"
>
> You can see how we made use of that on fstest with DYNAMIC_RUNTIME_VARS
> on workflows/fstests/Makefile. To support this you could add an empty
> dynamic string by default and the command line would override it, you'd
> then just always append this dynamic default string to the task with
> something like.
>
> - name: Run configure for QEMU
>   vars:
>     extra_configure_args="{{ qemu_build_extra_strings + ' ' + qemu_build_dynamic_extra_strings }}
>   command: "./configure --target-list={{ qemu_target }} {{ extra_configure_args }}"
>   tags: [ 'qemu', 'configure' ]
>     args:
>       chdir: "{{ qemu_data }}"
>
> [0] https://github.com/linux-kdevops/kconfig
>
>   Luis