Re: [qemu-s390x] [PATCH v4 09/21] target: Do not include "exec/exec-all.h" if it is not necessary

[Michael S. Tsirkin]

On Wed, May 30, 2018 at 03:19:19AM -0300, Philippe Mathieu-Daudé wrote:
> Le mer. 30 mai 2018 02:50, Philippe Mathieu-Daudé <address@hidden> a écrit :
> 
>     On Wed, May 30, 2018 at 1:42 AM, Michael S. Tsirkin <address@hidden> 
> wrote:
>     > On Wed, May 30, 2018 at 12:12:55AM -0300, Philippe Mathieu-Daudé wrote:
>     >> Hi Cornelia,
>     >>
>     >> On 05/29/2018 08:40 AM, Cornelia Huck wrote:
>     >> > On Mon, 28 May 2018 20:27:07 -0300
>     >> > Philippe Mathieu-Daudé <address@hidden> wrote:
>     >> >
>     >> >> Code change produced with:
>     >> >>     $ git grep '#include "exec/exec-all.h"' | \
>     >> >>       cut -d: -f-1 | \
>     >> >>       xargs egrep -L "(cpu_address_space_init|cpu_loop_|tlb_|tb_|
>     GETPC|singlestep|TranslationBlock)" | \
>     >> >
>     >> > Hm, does this expression catch all files that need to include
>     >> > exec-all.h? The resulting patch seems fine, though.
>     >>
>     >> No, not all :/
>     >> I started with "(cpu_loop_|tlb_|tb_)" then kept brutebuilding until no
>     >> more errors appear. In 2 more steps I added "cpu_address_space_init|"
>     >> then "|GETPC|singlestep|TranslationBlock". Quick and dirty enough for 
> my
>     >> goal than trying to build a regex to explode function/struct names from
>     >> headers. This is a clever way to do it for long term command reuse 
> taken
>     >> from commit messages...
>     >
>     > Brutebuilding isn't a good way to find unused includes, some other 
> header
>     > might pull in an include you are trying to remove for its own purposes.
>     > If you want to try brutebuilding you must also verify that's
>     > not the case - e.g. look at the dependency file generated.
> 
> 
> I think I get it for headers, but here I'm only modifying .c source files. 

Same thing - if you call a function from a header, you should include
that header even if some other header happens to pull it in.
Otherwise someone might cleanup that other header and suddenly
your build breaks.



> I'll restrict the shell command to .c 

I don't think it matters.

> 
> 
>     Hmm you mean the .d files in the build dir?
> 
>     i.e.
>     $ find lm32-softmmu -name \*.d -exec fgrep -L exec-all.h {} \; | sed
>     -e 's/.*-softmmu\/\(.*\)d$/\1c/'
>     dump.c
>     numa.c
>     tcg/tcg-common.c
>     tcg/optimize.c
>     tcg/tcg-op-gvec.c
>     tcg/tcg-op-vec.c
>     accel/tcg/tcg-runtime-gvec.c
>     accel/tcg/tcg-all.c
>     accel/accel.c
>     accel/stubs/whpx-stub.c
>     accel/stubs/hvf-stub.c
>     accel/stubs/kvm-stub.c
>     accel/stubs/hax-stub.c
>     hw/lm32/milkymist.c
>     hw/lm32/lm32_boards.c
>     hw/core/generic-loader.c
>     ...
> 
>     Then look why each .c included it?
> 
>     >
>     >> >>       xargs sed -i.bak '/#include "exec\/exec-all.h"/d'
>     >> >>
>     >> >> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>