[PATCH kvm-unit-tests] cscope: fix database generation

[PATCH kvm-unit-tests] cscope: fix database generation

[Bandan Das]

The cscope.files that we generate doesn't include all
source files that are potentially interesting. We should
include all $(ARCH)es  and not the just the one configure
finds. Moreover, $(ARCH) expands to x86_64 which is not the
correct path for x86 sources. Generate cscope.files by searching
for all files starting from root.

While we are there, remove the unnecessary sed substitution
and modify find to include a few other file name extensions.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 Makefile  | 9 +++------
 configure | 2 ++
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 5d7506e..458d0f0 100644
--- a/Makefile
+++ b/Makefile
@@ -92,11 +92,8 @@ distclean: clean libfdt_clean
 	$(RM) lib/asm config.mak $(TEST_DIR)-run test.log msr.out cscope.*
 	$(RM) -r tests
 
-cscope: cscope_dirs = lib lib/libfdt lib/linux
-cscope: cscope_dirs += lib/$(ARCH)/asm lib/$(TEST_DIR)/asm lib/asm-generic
-cscope: cscope_dirs += $(TEST_DIR) lib/$(TEST_DIR) lib/$(ARCH)
 cscope:
-	$(RM) ./cscope.*
-	find -L $(cscope_dirs) -maxdepth 1 \
-		-name '*.[chsS]' -print | sed 's,^\./,,' | sort -u > ./cscope.files
+	$(RM) $(SRCDIR)/cscope.*
+	find -L $(SRCDIR) -maxdepth 3 \
+		-regex '.*\.\(c\|h\|cc\|S\|sh\|bash\|s\)$$' -print | sort -u > ./cscope.files
 	cscope -bk
diff --git a/configure b/configure
index ba6c55b..3153fb9 100755
--- a/configure
+++ b/configure
@@ -12,6 +12,7 @@ host=$arch
 cross_prefix=
 endian=""
 pretty_print_stacks=yes
+srcdir=`pwd`
 
 usage() {
     cat <<-EOF
@@ -159,4 +160,5 @@ TEST_DIR=$testdir
 FIRMWARE=$firmware
 ENDIAN=$endian
 PRETTY_PRINT_STACKS=$pretty_print_stacks
+SRCDIR=$srcdir
 EOF
-- 
2.5.5

Re: [PATCH kvm-unit-tests] cscope: fix database generation

[Andrew Jones]

On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote:
> 
> The cscope.files that we generate doesn't include all
> source files that are potentially interesting. We should
> include all $(ARCH)es  and not the just the one configure
> finds. Moreover, $(ARCH) expands to x86_64 which is not the
> correct path for x86 sources. Generate cscope.files by searching
> for all files starting from root.

No thanks :-) I'd rather not get hits for x86 and powerpc when
jumping around in arm code. It's true that $(ARCH) expands to
x86_64, but $(TEST_DIR) expands to x86, so the current find
does find all x86 source.

> 
> While we are there, remove the unnecessary sed substitution
> and modify find to include a few other file name extensions.

I can't remember why the sed was necessary, so it could maybe go
away. Does it hurt anything though?

Looks like you've added .cc, .sh, and .bash. I don't want to mix
scripts with C source. As for api/*.{cc|hh}, we could change the
API configure variable to a path ('./api') instead of a boolean,
and then use it in the current cscope line, along with adding
both .cc and .hh to the name extension list. (On a side note, we
should probably create ./api its own makefile and look into
getting it to work with other architectures at some point).

Thanks,
drew

> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  Makefile  | 9 +++------
>  configure | 2 ++
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 5d7506e..458d0f0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -92,11 +92,8 @@ distclean: clean libfdt_clean
>  	$(RM) lib/asm config.mak $(TEST_DIR)-run test.log msr.out cscope.*
>  	$(RM) -r tests
>  
> -cscope: cscope_dirs = lib lib/libfdt lib/linux
> -cscope: cscope_dirs += lib/$(ARCH)/asm lib/$(TEST_DIR)/asm lib/asm-generic
> -cscope: cscope_dirs += $(TEST_DIR) lib/$(TEST_DIR) lib/$(ARCH)
>  cscope:
> -	$(RM) ./cscope.*
> -	find -L $(cscope_dirs) -maxdepth 1 \
> -		-name '*.[chsS]' -print | sed 's,^\./,,' | sort -u > ./cscope.files
> +	$(RM) $(SRCDIR)/cscope.*
> +	find -L $(SRCDIR) -maxdepth 3 \
> +		-regex '.*\.\(c\|h\|cc\|S\|sh\|bash\|s\)$$' -print | sort -u > ./cscope.files
>  	cscope -bk
> diff --git a/configure b/configure
> index ba6c55b..3153fb9 100755
> --- a/configure
> +++ b/configure
> @@ -12,6 +12,7 @@ host=$arch
>  cross_prefix=
>  endian=""
>  pretty_print_stacks=yes
> +srcdir=`pwd`
>  
>  usage() {
>      cat <<-EOF
> @@ -159,4 +160,5 @@ TEST_DIR=$testdir
>  FIRMWARE=$firmware
>  ENDIAN=$endian
>  PRETTY_PRINT_STACKS=$pretty_print_stacks
> +SRCDIR=$srcdir
>  EOF
> -- 
> 2.5.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests] cscope: fix database generation

[Bandan Das]

Andrew Jones <drjones@redhat.com> writes:

> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote:
>> 
>> The cscope.files that we generate doesn't include all
>> source files that are potentially interesting. We should
>> include all $(ARCH)es  and not the just the one configure
>> finds. Moreover, $(ARCH) expands to x86_64 which is not the
>> correct path for x86 sources. Generate cscope.files by searching
>> for all files starting from root.
>
> No thanks :-) I'd rather not get hits for x86 and powerpc when

I don't think that's a good idea. Just because you don't like to
see x86 bits when jumping around in arm code doesn't mean you should
hide all references to a given function. And if the arm code is
segregated enough that there are no common functions, you won't
see them anyway. 

> jumping around in arm code. It's true that $(ARCH) expands to
> x86_64, but $(TEST_DIR) expands to x86, so the current find
> does find all x86 source.

Yep, you are right. It still prints a warning message about non-existent
dirs though which needs to be fixed.

>> 
>> While we are there, remove the unnecessary sed substitution
>> and modify find to include a few other file name extensions.
>
> I can't remember why the sed was necessary, so it could maybe go
> away. Does it hurt anything though?

No, it doesn't hurt anything. Looks like it was meant to replace
"./" with "". So, I really don't know what it was meant to do
because that substitution doesn't seem right either.

> Looks like you've added .cc, .sh, and .bash. I don't want to mix
> scripts with C source. As for api/*.{cc|hh}, we could change the
> API configure variable to a path ('./api') instead of a boolean,
> and then use it in the current cscope line, along with adding
> both .cc and .hh to the name extension list. (On a side note, we
> should probably create ./api its own makefile and look into
> getting it to work with other architectures at some point).

Ok, this I agree with. We can separate the scripts from the c sources
and probably have a variable that sets adding them. But please let's
not make it difficult to write arm/powerpc tests just because someone's on
x86 :)

> Thanks,
> drew
>
>> 
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  Makefile  | 9 +++------
>>  configure | 2 ++
>>  2 files changed, 5 insertions(+), 6 deletions(-)
>> 
>> diff --git a/Makefile b/Makefile
>> index 5d7506e..458d0f0 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -92,11 +92,8 @@ distclean: clean libfdt_clean
>>  	$(RM) lib/asm config.mak $(TEST_DIR)-run test.log msr.out cscope.*
>>  	$(RM) -r tests
>>  
>> -cscope: cscope_dirs = lib lib/libfdt lib/linux
>> -cscope: cscope_dirs += lib/$(ARCH)/asm lib/$(TEST_DIR)/asm lib/asm-generic
>> -cscope: cscope_dirs += $(TEST_DIR) lib/$(TEST_DIR) lib/$(ARCH)
>>  cscope:
>> -	$(RM) ./cscope.*
>> -	find -L $(cscope_dirs) -maxdepth 1 \
>> -		-name '*.[chsS]' -print | sed 's,^\./,,' | sort -u > ./cscope.files
>> +	$(RM) $(SRCDIR)/cscope.*
>> +	find -L $(SRCDIR) -maxdepth 3 \
>> +		-regex '.*\.\(c\|h\|cc\|S\|sh\|bash\|s\)$$' -print | sort -u > ./cscope.files
>>  	cscope -bk
>> diff --git a/configure b/configure
>> index ba6c55b..3153fb9 100755
>> --- a/configure
>> +++ b/configure
>> @@ -12,6 +12,7 @@ host=$arch
>>  cross_prefix=
>>  endian=""
>>  pretty_print_stacks=yes
>> +srcdir=`pwd`
>>  
>>  usage() {
>>      cat <<-EOF
>> @@ -159,4 +160,5 @@ TEST_DIR=$testdir
>>  FIRMWARE=$firmware
>>  ENDIAN=$endian
>>  PRETTY_PRINT_STACKS=$pretty_print_stacks
>> +SRCDIR=$srcdir
>>  EOF
>> -- 
>> 2.5.5
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests] cscope: fix database generation

[Radim Krčmář]

2016-04-19 13:08-0400, Bandan Das:
> Andrew Jones <drjones@redhat.com> writes:
>> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote:
>>> 
>>> The cscope.files that we generate doesn't include all
>>> source files that are potentially interesting. We should
>>> include all $(ARCH)es  and not the just the one configure
>>> finds. Moreover, $(ARCH) expands to x86_64 which is not the
>>> correct path for x86 sources. Generate cscope.files by searching
>>> for all files starting from root.
>>
>> No thanks :-) I'd rather not get hits for x86 and powerpc when
> 
> I don't think that's a good idea. Just because you don't like to
> see x86 bits when jumping around in arm code doesn't mean you should
> hide all references to a given function. And if the arm code is
> segregated enough that there are no common functions, you won't
> see them anyway. 
> [...]                                               But please let's
> not make it difficult to write arm/powerpc tests just because someone's on
> x86 :)

In the kernel, environment can override configuration, so you'd do

  % ARCH=arm make cscope

and it doesn't matter what arch is being compiled.  I think this would
be a better solution for kvm-unit-tests too.

I agree with Drew that other arches would mostly clutter searches if
they were included by default, but we can also add a rule in case
indexing all arches is desired (like when changing the prototype of some
generic function?).

Re: [PATCH kvm-unit-tests] cscope: fix database generation

[Bandan Das]

Radim Krčmář <rkrcmar@redhat.com> writes:

> 2016-04-19 13:08-0400, Bandan Das:
>> Andrew Jones <drjones@redhat.com> writes:
>>> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote:
>>>> 
>>>> The cscope.files that we generate doesn't include all
>>>> source files that are potentially interesting. We should
>>>> include all $(ARCH)es  and not the just the one configure
>>>> finds. Moreover, $(ARCH) expands to x86_64 which is not the
>>>> correct path for x86 sources. Generate cscope.files by searching
>>>> for all files starting from root.
>>>
>>> No thanks :-) I'd rather not get hits for x86 and powerpc when
>> 
>> I don't think that's a good idea. Just because you don't like to
>> see x86 bits when jumping around in arm code doesn't mean you should
>> hide all references to a given function. And if the arm code is
>> segregated enough that there are no common functions, you won't
>> see them anyway. 
>> [...]                                               But please let's
>> not make it difficult to write arm/powerpc tests just because someone's on
>> x86 :)
>
> In the kernel, environment can override configuration, so you'd do
>
>   % ARCH=arm make cscope
>
> and it doesn't matter what arch is being compiled.  I think this would
> be a better solution for kvm-unit-tests too.
>
> I agree with Drew that other arches would mostly clutter searches if
> they were included by default, but we can also add a rule in case

Since you mentioned the kernel, it looks like both the kernel/qemu
include all by default which makes sense to me. As I mentioned above,
you definitely don't want to modify a common library function only to
realize that you forgot to modify all sites. Looking at all sites adds
visibility, not introduce clutter.

> indexing all arches is desired (like when changing the prototype of some
> generic function?).

kvm-unit-tests is small enough that this can be the default. That said, this
is a trivial change and I am happy to live with it in my local branch. (Unless
ofcourse both Linux and Qemu decide to make this default behavior too!)

Re: [PATCH kvm-unit-tests] cscope: fix database generation

[Radim Krčmář]

2016-04-19 14:58-0400, Bandan Das:
> Radim Krčmář <rkrcmar@redhat.com> writes:
>> 2016-04-19 13:08-0400, Bandan Das:
>>> Andrew Jones <drjones@redhat.com> writes:
>>>> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote:
>>>>> 
>>>>> The cscope.files that we generate doesn't include all
>>>>> source files that are potentially interesting. We should
>>>>> include all $(ARCH)es  and not the just the one configure
>>>>> finds. Moreover, $(ARCH) expands to x86_64 which is not the
>>>>> correct path for x86 sources. Generate cscope.files by searching
>>>>> for all files starting from root.
>>>>
>>>> No thanks :-) I'd rather not get hits for x86 and powerpc when
>>> 
>>> I don't think that's a good idea. Just because you don't like to
>>> see x86 bits when jumping around in arm code doesn't mean you should
>>> hide all references to a given function. And if the arm code is
>>> segregated enough that there are no common functions, you won't
>>> see them anyway. 
>>> [...]                                               But please let's
>>> not make it difficult to write arm/powerpc tests just because someone's on
>>> x86 :)
>>
>> In the kernel, environment can override configuration, so you'd do
>>
>>   % ARCH=arm make cscope
>>
>> and it doesn't matter what arch is being compiled.  I think this would
>> be a better solution for kvm-unit-tests too.
>>
>> I agree with Drew that other arches would mostly clutter searches if
>> they were included by default, but we can also add a rule in case
> 
> Since you mentioned the kernel, it looks like both the kernel/qemu
> include all by default which makes sense to me.

qemu does, kernel doesn't (e.g. search for sys_sigreturn).

>                                                 As I mentioned above,
> you definitely don't want to modify a common library function only to
> realize that you forgot to modify all sites. Looking at all sites adds
> visibility, not introduce clutter.
> 
>> indexing all arches is desired (like when changing the prototype of some
>> generic function?).
> 
> kvm-unit-tests is small enough that this can be the default.

I don't see a reason to add results that we'd then have to mentally
filter regardless of their amount.

>                                                              That said, this
> is a trivial change and I am happy to live with it in my local branch. (Unless
> ofcourse both Linux and Qemu decide to make this default behavior too!)

I think we'll agree on something that can be merged -- the present
situation is bad.

For me, "include all" is a matter of `cscope -bk **/*.[chS]` and getting
only arch-specific cscope is harder, so having an option to generate
cscope just for target arch would be nice.

(I don't care about the default much, `ARCH=$sth make cscope` is easy
 to type if the default included all.)

Re: [PATCH kvm-unit-tests] cscope: fix database generation

[Bandan Das]

Radim Krčmář <rkrcmar@redhat.com> writes:

> 2016-04-19 14:58-0400, Bandan Das:
>> Radim Krčmář <rkrcmar@redhat.com> writes:
>>> 2016-04-19 13:08-0400, Bandan Das:
>>>> Andrew Jones <drjones@redhat.com> writes:
>>>>> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote:
>>>>>> 
>>>>>> The cscope.files that we generate doesn't include all
>>>>>> source files that are potentially interesting. We should
>>>>>> include all $(ARCH)es  and not the just the one configure
>>>>>> finds. Moreover, $(ARCH) expands to x86_64 which is not the
>>>>>> correct path for x86 sources. Generate cscope.files by searching
>>>>>> for all files starting from root.
>>>>>
>>>>> No thanks :-) I'd rather not get hits for x86 and powerpc when
>>>> 
>>>> I don't think that's a good idea. Just because you don't like to
>>>> see x86 bits when jumping around in arm code doesn't mean you should
>>>> hide all references to a given function. And if the arm code is
>>>> segregated enough that there are no common functions, you won't
>>>> see them anyway. 
>>>> [...]                                               But please let's
>>>> not make it difficult to write arm/powerpc tests just because someone's on
>>>> x86 :)
>>>
>>> In the kernel, environment can override configuration, so you'd do
>>>
>>>   % ARCH=arm make cscope
>>>
>>> and it doesn't matter what arch is being compiled.  I think this would
>>> be a better solution for kvm-unit-tests too.
>>>
>>> I agree with Drew that other arches would mostly clutter searches if
>>> they were included by default, but we can also add a rule in case
>> 
>> Since you mentioned the kernel, it looks like both the kernel/qemu
>> include all by default which makes sense to me.
>
> qemu does, kernel doesn't (e.g. search for sys_sigreturn).

Not exactly, arch specific(arch/*) files are excluded but there are
others such as sound/* include/* that are not all x86 specific. The
arch segregation is helpful since there would be functions with similar
names (and similar functionality). That's not the case for every file
in x86/ and arm/ of kvm-unit-tests.

>
>>                                                 As I mentioned above,
>> you definitely don't want to modify a common library function only to
>> realize that you forgot to modify all sites. Looking at all sites adds
>> visibility, not introduce clutter.
>> 
>>> indexing all arches is desired (like when changing the prototype of some
>>> generic function?).
>> 
>> kvm-unit-tests is small enough that this can be the default.
>
> I don't see a reason to add results that we'd then have to mentally
> filter regardless of their amount.
>
>>                                                              That said, this
>> is a trivial change and I am happy to live with it in my local branch. (Unless
>> ofcourse both Linux and Qemu decide to make this default behavior too!)
>
> I think we'll agree on something that can be merged -- the present
> situation is bad.

It's not that bad, actually. We are simply arguing over personal
preferences here. Changing my workflow a bit is not hard. :)

> For me, "include all" is a matter of `cscope -bk **/*.[chS]` and getting
> only arch-specific cscope is harder, so having an option to generate
> cscope just for target arch would be nice.

Yes, the option is definitely nice but making it the default, not so much.

> (I don't care about the default much, `ARCH=$sth make cscope` is easy
>  to type if the default included all.)
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests] cscope: fix database generation

[Andrew Jones]

On Tue, Apr 19, 2016 at 03:44:50PM -0400, Bandan Das wrote:
> Radim Krčmář <rkrcmar@redhat.com> writes:
> 
> > 2016-04-19 14:58-0400, Bandan Das:
> >> Radim Krčmář <rkrcmar@redhat.com> writes:
> >>> 2016-04-19 13:08-0400, Bandan Das:
> >>>> Andrew Jones <drjones@redhat.com> writes:
> >>>>> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote:
> >>>>>> 
> >>>>>> The cscope.files that we generate doesn't include all
> >>>>>> source files that are potentially interesting. We should
> >>>>>> include all $(ARCH)es  and not the just the one configure
> >>>>>> finds. Moreover, $(ARCH) expands to x86_64 which is not the
> >>>>>> correct path for x86 sources. Generate cscope.files by searching
> >>>>>> for all files starting from root.
> >>>>>
> >>>>> No thanks :-) I'd rather not get hits for x86 and powerpc when
> >>>> 
> >>>> I don't think that's a good idea. Just because you don't like to
> >>>> see x86 bits when jumping around in arm code doesn't mean you should
> >>>> hide all references to a given function. And if the arm code is
> >>>> segregated enough that there are no common functions, you won't
> >>>> see them anyway. 
> >>>> [...]                                               But please let's
> >>>> not make it difficult to write arm/powerpc tests just because someone's on
> >>>> x86 :)

How does it make it more difficult to write arm/powerpc tests on x86?
I do it all the time. Of course you need to run ./configure --arch=<ARCH>
first, but you need to do that to compile anyway.

> >>>
> >>> In the kernel, environment can override configuration, so you'd do
> >>>
> >>>   % ARCH=arm make cscope
> >>>
> >>> and it doesn't matter what arch is being compiled.  I think this would
> >>> be a better solution for kvm-unit-tests too.

This is what kvm-unit-tests does, except it takes ARCH from config.mak,
instead of making you specify it, and there is no 'everything' option,
default or otherwise right now.

> >>>
> >>> I agree with Drew that other arches would mostly clutter searches if
> >>> they were included by default, but we can also add a rule in case
> >> 
> >> Since you mentioned the kernel, it looks like both the kernel/qemu
> >> include all by default which makes sense to me.
> >
> > qemu does, kernel doesn't (e.g. search for sys_sigreturn).
> 
> Not exactly, arch specific(arch/*) files are excluded but there are
> others such as sound/* include/* that are not all x86 specific. The
> arch segregation is helpful since there would be functions with similar
> names (and similar functionality). That's not the case for every file
> in x86/ and arm/ of kvm-unit-tests.

kvm-unit-tests' cscope find collects all common files and all arch
specific files, just as you've described about the kernel. Please give
an example of where you couldn't find something you needed with this
find, so that I know what the problem is. Also, if there is a problem,
we can likely extend the find just a bit, rather than changing it to a
wholesale search.

> 
> >
> >>                                                 As I mentioned above,
> >> you definitely don't want to modify a common library function only to
> >> realize that you forgot to modify all sites. Looking at all sites adds
> >> visibility, not introduce clutter.

Steps to modifying a common function's call sites
  1) git grep <function>
  2) compile test [at least] all architectures

> >> 
> >>> indexing all arches is desired (like when changing the prototype of some
> >>> generic function?).
> >> 
> >> kvm-unit-tests is small enough that this can be the default.
> >
> > I don't see a reason to add results that we'd then have to mentally
> > filter regardless of their amount.
> >
> >>                                                              That said, this
> >> is a trivial change and I am happy to live with it in my local branch. (Unless
> >> ofcourse both Linux and Qemu decide to make this default behavior too!)
> >
> > I think we'll agree on something that can be merged -- the present
> > situation is bad.
> 
> It's not that bad, actually. We are simply arguing over personal
> preferences here. Changing my workflow a bit is not hard. :)
> 
> > For me, "include all" is a matter of `cscope -bk **/*.[chS]` and getting
> > only arch-specific cscope is harder, so having an option to generate
> > cscope just for target arch would be nice.

Yes, generating cscope just for the target arch is nice, and that's
kvm-unit-tests default, as it makes the most sense for the common
unit test developer, i.e. somebody not extending the framework, but
rather just writing an arch-specific unit test.

> 
> Yes, the option is definitely nice but making it the default, not so much.

As should be clear by now, IMHO, the default is not only nice, but correct.
Feel free to add a cscope-all target though.

> 
> > (I don't care about the default much, `ARCH=$sth make cscope` is easy
> >  to type if the default included all.)

I can live with that too, but I do care about defaults, and defaults
should target the common cases. I haven't yet seen enough justification
as to why seeing everything (whether you want to or not) is the best
common case for kvm-unit-tests' cscope generation.

Thanks,
drew

Re: [PATCH kvm-unit-tests] cscope: fix database generation

[Bandan Das]

Andrew Jones <drjones@redhat.com> writes:

> On Tue, Apr 19, 2016 at 03:44:50PM -0400, Bandan Das wrote:
>> Radim Krčmář <rkrcmar@redhat.com> writes:
>> 
>> > 2016-04-19 14:58-0400, Bandan Das:
>> >> Radim Krčmář <rkrcmar@redhat.com> writes:
>> >>> 2016-04-19 13:08-0400, Bandan Das:
>> >>>> Andrew Jones <drjones@redhat.com> writes:
>> >>>>> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote:
>> >>>>>> 
>> >>>>>> The cscope.files that we generate doesn't include all
>> >>>>>> source files that are potentially interesting. We should
>> >>>>>> include all $(ARCH)es  and not the just the one configure
>> >>>>>> finds. Moreover, $(ARCH) expands to x86_64 which is not the
>> >>>>>> correct path for x86 sources. Generate cscope.files by searching
>> >>>>>> for all files starting from root.
>> >>>>>
>> >>>>> No thanks :-) I'd rather not get hits for x86 and powerpc when
>> >>>> 
>> >>>> I don't think that's a good idea. Just because you don't like to
>> >>>> see x86 bits when jumping around in arm code doesn't mean you should
>> >>>> hide all references to a given function. And if the arm code is
>> >>>> segregated enough that there are no common functions, you won't
>> >>>> see them anyway. 
>> >>>> [...]                                               But please let's
>> >>>> not make it difficult to write arm/powerpc tests just because someone's on
>> >>>> x86 :)
>
> How does it make it more difficult to write arm/powerpc tests on x86?
> I do it all the time. Of course you need to run ./configure --arch=<ARCH>
> first, but you need to do that to compile anyway.
>
>> >>>
>> >>> In the kernel, environment can override configuration, so you'd do
>> >>>
>> >>>   % ARCH=arm make cscope
>> >>>
>> >>> and it doesn't matter what arch is being compiled.  I think this would
>> >>> be a better solution for kvm-unit-tests too.
>
> This is what kvm-unit-tests does, except it takes ARCH from config.mak,
> instead of making you specify it, and there is no 'everything' option,
> default or otherwise right now.
>
>> >>>
>> >>> I agree with Drew that other arches would mostly clutter searches if
>> >>> they were included by default, but we can also add a rule in case
>> >> 
>> >> Since you mentioned the kernel, it looks like both the kernel/qemu
>> >> include all by default which makes sense to me.
>> >
>> > qemu does, kernel doesn't (e.g. search for sys_sigreturn).
>> 
>> Not exactly, arch specific(arch/*) files are excluded but there are
>> others such as sound/* include/* that are not all x86 specific. The
>> arch segregation is helpful since there would be functions with similar
>> names (and similar functionality). That's not the case for every file
>> in x86/ and arm/ of kvm-unit-tests.
>
> kvm-unit-tests' cscope find collects all common files and all arch
> specific files, just as you've described about the kernel. Please give
> an example of where you couldn't find something you needed with this
> find, so that I know what the problem is. Also, if there is a problem,

Sorry, you are right. Looks like I was looking at stale files (iotable.c/h)
which don't exist anymore. So, I think the current setup works for me
too since I mostly work on x86. For other times, when I do have to certain
things in other arches, I will just run cscope by hand.

> we can likely extend the find just a bit, rather than changing it to a
> wholesale search.
>
>> 
...
>> 
>> Yes, the option is definitely nice but making it the default, not so much.
>
> As should be clear by now, IMHO, the default is not only nice, but correct.
> Feel free to add a cscope-all target though.

Yeah, I can live with this too. IMHO, the default is a matter of preference, there's
no question of correctness.

>> 
>> > (I don't care about the default much, `ARCH=$sth make cscope` is easy
>> >  to type if the default included all.)
>
> I can live with that too, but I do care about defaults, and defaults
> should target the common cases. I haven't yet seen enough justification
> as to why seeing everything (whether you want to or not) is the best
> common case for kvm-unit-tests' cscope generation.

Me neither, I am not convinced yet how including all files obstructs your workflow
but I am certain living with the defaults won't obstruct mine.

> Thanks,
> drew

Re: [PATCH kvm-unit-tests] cscope: fix database generation

[Andrew Jones]

On Wed, Apr 20, 2016 at 10:31:24AM -0400, Bandan Das wrote:
> Andrew Jones <drjones@redhat.com> writes:
> 
> > On Tue, Apr 19, 2016 at 03:44:50PM -0400, Bandan Das wrote:
> >> Radim Krčmář <rkrcmar@redhat.com> writes:
> >> 
> >> > 2016-04-19 14:58-0400, Bandan Das:
> >> >> Radim Krčmář <rkrcmar@redhat.com> writes:
> >> >>> 2016-04-19 13:08-0400, Bandan Das:
> >> >>>> Andrew Jones <drjones@redhat.com> writes:
> >> >>>>> On Mon, Apr 18, 2016 at 07:04:55PM -0400, Bandan Das wrote:
> >> >>>>>> 
> >> >>>>>> The cscope.files that we generate doesn't include all
> >> >>>>>> source files that are potentially interesting. We should
> >> >>>>>> include all $(ARCH)es  and not the just the one configure
> >> >>>>>> finds. Moreover, $(ARCH) expands to x86_64 which is not the
> >> >>>>>> correct path for x86 sources. Generate cscope.files by searching
> >> >>>>>> for all files starting from root.
> >> >>>>>
> >> >>>>> No thanks :-) I'd rather not get hits for x86 and powerpc when
> >> >>>> 
> >> >>>> I don't think that's a good idea. Just because you don't like to
> >> >>>> see x86 bits when jumping around in arm code doesn't mean you should
> >> >>>> hide all references to a given function. And if the arm code is
> >> >>>> segregated enough that there are no common functions, you won't
> >> >>>> see them anyway. 
> >> >>>> [...]                                               But please let's
> >> >>>> not make it difficult to write arm/powerpc tests just because someone's on
> >> >>>> x86 :)
> >
> > How does it make it more difficult to write arm/powerpc tests on x86?
> > I do it all the time. Of course you need to run ./configure --arch=<ARCH>
> > first, but you need to do that to compile anyway.
> >
> >> >>>
> >> >>> In the kernel, environment can override configuration, so you'd do
> >> >>>
> >> >>>   % ARCH=arm make cscope
> >> >>>
> >> >>> and it doesn't matter what arch is being compiled.  I think this would
> >> >>> be a better solution for kvm-unit-tests too.
> >
> > This is what kvm-unit-tests does, except it takes ARCH from config.mak,
> > instead of making you specify it, and there is no 'everything' option,
> > default or otherwise right now.
> >
> >> >>>
> >> >>> I agree with Drew that other arches would mostly clutter searches if
> >> >>> they were included by default, but we can also add a rule in case
> >> >> 
> >> >> Since you mentioned the kernel, it looks like both the kernel/qemu
> >> >> include all by default which makes sense to me.
> >> >
> >> > qemu does, kernel doesn't (e.g. search for sys_sigreturn).
> >> 
> >> Not exactly, arch specific(arch/*) files are excluded but there are
> >> others such as sound/* include/* that are not all x86 specific. The
> >> arch segregation is helpful since there would be functions with similar
> >> names (and similar functionality). That's not the case for every file
> >> in x86/ and arm/ of kvm-unit-tests.
> >
> > kvm-unit-tests' cscope find collects all common files and all arch
> > specific files, just as you've described about the kernel. Please give
> > an example of where you couldn't find something you needed with this
> > find, so that I know what the problem is. Also, if there is a problem,
> 
> Sorry, you are right. Looks like I was looking at stale files (iotable.c/h)
> which don't exist anymore. So, I think the current setup works for me
> too since I mostly work on x86. For other times, when I do have to certain
> things in other arches, I will just run cscope by hand.

Why run it by hand? Configure the arch you want to focus on, and then do
'make cscope'. Or, if you really need to view all, then submit a patch
adding a new makefile target, and then run that.

> 
> > we can likely extend the find just a bit, rather than changing it to a
> > wholesale search.
> >
> >> 
> ...
> >> 
> >> Yes, the option is definitely nice but making it the default, not so much.
> >
> > As should be clear by now, IMHO, the default is not only nice, but correct.
> > Feel free to add a cscope-all target though.
> 
> Yeah, I can live with this too. IMHO, the default is a matter of preference, there's
> no question of correctness.

Selection of defaults is hard. Somebody always feels it wrong. However
if you look at what works best for the largest audience, then I believe
that selection is the _correct_ default. I could be wrong about this
one, but I don't think so ;-)

> 
> >> 
> >> > (I don't care about the default much, `ARCH=$sth make cscope` is easy
> >> >  to type if the default included all.)
> >
> > I can live with that too, but I do care about defaults, and defaults
> > should target the common cases. I haven't yet seen enough justification
> > as to why seeing everything (whether you want to or not) is the best
> > common case for kvm-unit-tests' cscope generation.
> 
> Me neither, I am not convinced yet how including all files obstructs your workflow
> but I am certain living with the defaults won't obstruct mine.

It doesn't obstruct it. It adds useless overhead. More mental filtering,
as Radim described it.

drew

Re: [PATCH kvm-unit-tests] cscope: fix database generation

[Paolo Bonzini]

Yeah, kvm-unit-tests flamewars!  Testing has really taken off.

Paolo

On 19/04/2016 01:04, Bandan Das wrote:
> 
> The cscope.files that we generate doesn't include all
> source files that are potentially interesting. We should
> include all $(ARCH)es  and not the just the one configure
> finds. Moreover, $(ARCH) expands to x86_64 which is not the
> correct path for x86 sources. Generate cscope.files by searching
> for all files starting from root.
> 
> While we are there, remove the unnecessary sed substitution
> and modify find to include a few other file name extensions.
> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  Makefile  | 9 +++------
>  configure | 2 ++
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 5d7506e..458d0f0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -92,11 +92,8 @@ distclean: clean libfdt_clean
>  	$(RM) lib/asm config.mak $(TEST_DIR)-run test.log msr.out cscope.*
>  	$(RM) -r tests
>  
> -cscope: cscope_dirs = lib lib/libfdt lib/linux
> -cscope: cscope_dirs += lib/$(ARCH)/asm lib/$(TEST_DIR)/asm lib/asm-generic
> -cscope: cscope_dirs += $(TEST_DIR) lib/$(TEST_DIR) lib/$(ARCH)
>  cscope:
> -	$(RM) ./cscope.*
> -	find -L $(cscope_dirs) -maxdepth 1 \
> -		-name '*.[chsS]' -print | sed 's,^\./,,' | sort -u > ./cscope.files
> +	$(RM) $(SRCDIR)/cscope.*
> +	find -L $(SRCDIR) -maxdepth 3 \
> +		-regex '.*\.\(c\|h\|cc\|S\|sh\|bash\|s\)$$' -print | sort -u > ./cscope.files
>  	cscope -bk
> diff --git a/configure b/configure
> index ba6c55b..3153fb9 100755
> --- a/configure
> +++ b/configure
> @@ -12,6 +12,7 @@ host=$arch
>  cross_prefix=
>  endian=""
>  pretty_print_stacks=yes
> +srcdir=`pwd`
>  
>  usage() {
>      cat <<-EOF
> @@ -159,4 +160,5 @@ TEST_DIR=$testdir
>  FIRMWARE=$firmware
>  ENDIAN=$endian
>  PRETTY_PRINT_STACKS=$pretty_print_stacks
> +SRCDIR=$srcdir
>  EOF
>