[PATCH 1/2] radix tree test suite: include kconfig.h with incomplete path

[PATCH 1/2] radix tree test suite: include kconfig.h with incomplete path

[Wei Yang]

The include path indicates it is the kconfig.h in tools/include/linux.

Let's use the same format as others for better reading.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: "Liam R. Howlett" <Liam.Howlett@oracle.com>
CC: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 tools/testing/radix-tree/linux/kernel.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/radix-tree/linux/kernel.h b/tools/testing/radix-tree/linux/kernel.h
index c0a2bb785b92..fab6f47a9472 100644
--- a/tools/testing/radix-tree/linux/kernel.h
+++ b/tools/testing/radix-tree/linux/kernel.h
@@ -11,7 +11,7 @@
 #include <linux/err.h>
 #include <linux/bitops.h>
 #include <linux/log2.h>
-#include "../../../include/linux/kconfig.h"
+#include <linux/kconfig.h>
 
 #define printk printf
 #define pr_err printk
-- 
2.34.1

[PATCH 2/2] radix tree test suite: remove the local copy of kconfig.h

[Wei Yang]

The local copy of kconfig.h includes the kconfig.h from kernel source,
which is almost the same as the one in tools/include/linux.

Remove the local copy and use the common one.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: "Liam R. Howlett" <Liam.Howlett@oracle.com>
CC: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 tools/testing/radix-tree/linux/kconfig.h | 1 -
 1 file changed, 1 deletion(-)
 delete mode 100644 tools/testing/radix-tree/linux/kconfig.h

diff --git a/tools/testing/radix-tree/linux/kconfig.h b/tools/testing/radix-tree/linux/kconfig.h
deleted file mode 100644
index 6c8675859913..000000000000
--- a/tools/testing/radix-tree/linux/kconfig.h
+++ /dev/null
@@ -1 +0,0 @@
-#include "../../../../include/linux/kconfig.h"
-- 
2.34.1

Re: [PATCH 1/2] radix tree test suite: include kconfig.h with incomplete path

[Liam R. Howlett]

* Wei Yang <richard.weiyang@gmail.com> [240712 03:42]:
> The include path indicates it is the kconfig.h in tools/include/linux.
> 
> Let's use the same format as others for better reading.

nack

This change stops using tools/testing/radix-tree/generated/autoconf.h
which is included from kconfig.h.

If you add an #error to that generated/autoconf.h header, then you will
see it will build with your change, but fails with the existing code.

So this is not equivalent.

> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: "Liam R. Howlett" <Liam.Howlett@oracle.com>
> CC: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  tools/testing/radix-tree/linux/kernel.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/radix-tree/linux/kernel.h b/tools/testing/radix-tree/linux/kernel.h
> index c0a2bb785b92..fab6f47a9472 100644
> --- a/tools/testing/radix-tree/linux/kernel.h
> +++ b/tools/testing/radix-tree/linux/kernel.h
> @@ -11,7 +11,7 @@
>  #include <linux/err.h>
>  #include <linux/bitops.h>
>  #include <linux/log2.h>
> -#include "../../../include/linux/kconfig.h"
> +#include <linux/kconfig.h>
>  
>  #define printk printf
>  #define pr_err printk
> -- 
> 2.34.1
> 

Re: [PATCH 1/2] radix tree test suite: include kconfig.h with incomplete path

[Wei Yang]

On Mon, Jul 15, 2024 at 12:17:44PM -0400, Liam R. Howlett wrote:
>* Wei Yang <richard.weiyang@gmail.com> [240712 03:42]:
>> The include path indicates it is the kconfig.h in tools/include/linux.
>> 
>> Let's use the same format as others for better reading.
>
>nack
>
>This change stops using tools/testing/radix-tree/generated/autoconf.h
>which is included from kconfig.h.
>
>If you add an #error to that generated/autoconf.h header, then you will
>see it will build with your change, but fails with the existing code.
>

Thanks for your review.

>So this is not equivalent.

BTW, if I am correct, the difference is introduced by patch #2. If I just apply
patch #1, the #error in generated/autoconf.h works.

Patch #1 looks good?

>
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> CC: "Liam R. Howlett" <Liam.Howlett@oracle.com>
>> CC: Matthew Wilcox (Oracle) <willy@infradead.org>
>> ---
>>  tools/testing/radix-tree/linux/kernel.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/tools/testing/radix-tree/linux/kernel.h b/tools/testing/radix-tree/linux/kernel.h
>> index c0a2bb785b92..fab6f47a9472 100644
>> --- a/tools/testing/radix-tree/linux/kernel.h
>> +++ b/tools/testing/radix-tree/linux/kernel.h
>> @@ -11,7 +11,7 @@
>>  #include <linux/err.h>
>>  #include <linux/bitops.h>
>>  #include <linux/log2.h>
>> -#include "../../../include/linux/kconfig.h"
>> +#include <linux/kconfig.h>
>>  
>>  #define printk printf
>>  #define pr_err printk
>> -- 
>> 2.34.1
>> 

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/2] radix tree test suite: include kconfig.h with incomplete path

[Wei Yang]

On Fri, Jul 12, 2024 at 07:41:50AM +0000, Wei Yang wrote:
>The include path indicates it is the kconfig.h in tools/include/linux.
>
>Let's use the same format as others for better reading.
>
>Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>CC: "Liam R. Howlett" <Liam.Howlett@oracle.com>
>CC: Matthew Wilcox (Oracle) <willy@infradead.org>
>---
> tools/testing/radix-tree/linux/kernel.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/tools/testing/radix-tree/linux/kernel.h b/tools/testing/radix-tree/linux/kernel.h
>index c0a2bb785b92..fab6f47a9472 100644
>--- a/tools/testing/radix-tree/linux/kernel.h
>+++ b/tools/testing/radix-tree/linux/kernel.h
>@@ -11,7 +11,7 @@
> #include <linux/err.h>
> #include <linux/bitops.h>
> #include <linux/log2.h>
>-#include "../../../include/linux/kconfig.h"
>+#include <linux/kconfig.h>

Liam

I found the original code maybe not correct, but my change log here is not
exact either.

If my understanding is correct. In radix-tree test suite, we want to use the
kconfig.h in local linux/ directory.

But the original format "../../../include/linux/kconfig.h" is the one in
tools/.

My misunderstanding is <linux/kconfig.h> points to tools/. But since we
specify -I. before -I../../include, so it points to the local kconfig.h. This
is what we expect to use. By accident, the code change here seems correct.

If you think my understanding is correct, I would send a v2 with proper
changelog to describe it.

> 
> #define printk printf
> #define pr_err printk
>-- 
>2.34.1

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/2] radix tree test suite: include kconfig.h with incomplete path

[Matthew Wilcox]

On Tue, Jul 16, 2024 at 02:15:43AM +0000, Wei Yang wrote:
> If you think my understanding is correct, I would send a v2 with proper
> changelog to describe it.

It seems like you're trying to change something that isn't broken.

Re: [PATCH 1/2] radix tree test suite: include kconfig.h with incomplete path

[Wei Yang]

On Tue, Jul 16, 2024 at 03:31:06AM +0100, Matthew Wilcox wrote:
>On Tue, Jul 16, 2024 at 02:15:43AM +0000, Wei Yang wrote:
>> If you think my understanding is correct, I would send a v2 with proper
>> changelog to describe it.
>
>It seems like you're trying to change something that isn't broken.

Currently we don't find the problem because these two kconfig.h is similar.
The only difference is the one in tools/ doesn't include "generated/autoconf.h".

And in related files, like include/linux/xarray.h, includes the correct
kconfig.h.

It is not broken, but doesn't mean it is correct, IMO.

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/2] radix tree test suite: include kconfig.h with incomplete path

[Liam R. Howlett]

* Wei Yang <richard.weiyang@gmail.com> [240716 03:56]:
> On Tue, Jul 16, 2024 at 03:31:06AM +0100, Matthew Wilcox wrote:
> >On Tue, Jul 16, 2024 at 02:15:43AM +0000, Wei Yang wrote:
> >> If you think my understanding is correct, I would send a v2 with proper
> >> changelog to describe it.
> >
> >It seems like you're trying to change something that isn't broken.
> 
> Currently we don't find the problem because these two kconfig.h is similar.
> The only difference is the one in tools/ doesn't include "generated/autoconf.h".

We need that include.

> 
> And in related files, like include/linux/xarray.h, includes the correct
> kconfig.h.
> 
> It is not broken, but doesn't mean it is correct, IMO.

This is really annoying because it works today and your change was
correct enough to look right, but it doesn't include the
generated/autoconf.h  - so it's not right.  It took a lot of time to
figure out that it isn't right.

Instead of forward progress, I am spending time debugging make and
compile considerations in test code that works today.  So, maybe it's
not exactly what you want to see in test code, but it doesn't mean it is
worth my time.

Don't change this until there is a reason to make the change.

Thanks,
Liam

Re: [PATCH 1/2] radix tree test suite: include kconfig.h with incomplete path

[Wei Yang]

On Tue, Jul 16, 2024 at 11:54:47AM -0400, Liam R. Howlett wrote:
>* Wei Yang <richard.weiyang@gmail.com> [240716 03:56]:
>> On Tue, Jul 16, 2024 at 03:31:06AM +0100, Matthew Wilcox wrote:
>> >On Tue, Jul 16, 2024 at 02:15:43AM +0000, Wei Yang wrote:
>> >> If you think my understanding is correct, I would send a v2 with proper
>> >> changelog to describe it.
>> >
>> >It seems like you're trying to change something that isn't broken.
>> 
>> Currently we don't find the problem because these two kconfig.h is similar.
>> The only difference is the one in tools/ doesn't include "generated/autoconf.h".
>
>We need that include.
>

I see your point, but I find current code has a chance not does do this.

>> 
>> And in related files, like include/linux/xarray.h, includes the correct
>> kconfig.h.
>> 
>> It is not broken, but doesn't mean it is correct, IMO.
>
>This is really annoying because it works today and your change was
>correct enough to look right, but it doesn't include the
>generated/autoconf.h  - so it's not right.  It took a lot of time to
>figure out that it isn't right.
>

I am not sure you have read my second mail.

Here is what I find.

I add a warning in tools/include/linux/kconfig.h.

diff --git a/tools/include/linux/kconfig.h b/tools/include/linux/kconfig.h
index 13b86bd3b746..12d3ed5f16b2 100644
--- a/tools/include/linux/kconfig.h
+++ b/tools/include/linux/kconfig.h
@@ -2,6 +2,8 @@
 #ifndef _TOOLS_LINUX_KCONFIG_H
 #define _TOOLS_LINUX_KCONFIG_H
 
+#warning include local kconfig.h
+

Then I run the command:

$make maple.o
cc -I. -I../../include -I../../../lib -g -Og -Wall -D_LGPL_SOURCE -fsanitize=address -fsanitize=undefined   -c -o maple.o maple.c
In file included from ./linux/kernel.h:14,
                 from ../../include/linux/list.h:7,
                 from ./linux/../../../../include/linux/radix-tree.h:13,
                 from ./linux/radix-tree.h:5,
                 from test.h:4,
                 from maple.c:11:
./linux/../../../include/linux/kconfig.h:5:2: warning: #warning include local kconfig.h [-Wcpp]
    5 | #warning include local kconfig.h
      |  ^~~~~~~

This means current code may include tools/include/linux/kconfig.h, which doesn't
include generated/autoconf.h.

After I change to <linux/kconfig.h> in kernel.h, the warning message is gone.

Current behavior is not what you expected, right? We have a chance to include
the kconfig.h which doesn't include generated/autoconf.h. 

Hope not waste your time.

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/2] radix tree test suite: include kconfig.h with incomplete path

[Wei Yang]

On Wed, Jul 17, 2024 at 12:34:32AM +0000, Wei Yang wrote:
>On Tue, Jul 16, 2024 at 11:54:47AM -0400, Liam R. Howlett wrote:
>>* Wei Yang <richard.weiyang@gmail.com> [240716 03:56]:
>>> On Tue, Jul 16, 2024 at 03:31:06AM +0100, Matthew Wilcox wrote:
>>> >On Tue, Jul 16, 2024 at 02:15:43AM +0000, Wei Yang wrote:
>>> >> If you think my understanding is correct, I would send a v2 with proper
>>> >> changelog to describe it.
>>> >
>>> >It seems like you're trying to change something that isn't broken.
>>> 
>>> Currently we don't find the problem because these two kconfig.h is similar.
>>> The only difference is the one in tools/ doesn't include "generated/autoconf.h".
>>
>>We need that include.
>>
>
>I see your point, but I find current code has a chance not does do this.
>
>>> 
>>> And in related files, like include/linux/xarray.h, includes the correct
>>> kconfig.h.
>>> 
>>> It is not broken, but doesn't mean it is correct, IMO.
>>
>>This is really annoying because it works today and your change was
>>correct enough to look right, but it doesn't include the
>>generated/autoconf.h  - so it's not right.  It took a lot of time to
>>figure out that it isn't right.
>>
>
>I am not sure you have read my second mail.
>
>Here is what I find.
>
>I add a warning in tools/include/linux/kconfig.h.
>
>diff --git a/tools/include/linux/kconfig.h b/tools/include/linux/kconfig.h
>index 13b86bd3b746..12d3ed5f16b2 100644
>--- a/tools/include/linux/kconfig.h
>+++ b/tools/include/linux/kconfig.h
>@@ -2,6 +2,8 @@
> #ifndef _TOOLS_LINUX_KCONFIG_H
> #define _TOOLS_LINUX_KCONFIG_H
> 
>+#warning include local kconfig.h
>+
>
>Then I run the command:
>
>$make maple.o
>cc -I. -I../../include -I../../../lib -g -Og -Wall -D_LGPL_SOURCE -fsanitize=address -fsanitize=undefined   -c -o maple.o maple.c
>In file included from ./linux/kernel.h:14,
>                 from ../../include/linux/list.h:7,
>                 from ./linux/../../../../include/linux/radix-tree.h:13,
>                 from ./linux/radix-tree.h:5,
>                 from test.h:4,
>                 from maple.c:11:
>./linux/../../../include/linux/kconfig.h:5:2: warning: #warning include local kconfig.h [-Wcpp]
>    5 | #warning include local kconfig.h
>      |  ^~~~~~~
>
>This means current code may include tools/include/linux/kconfig.h, which doesn't
>include generated/autoconf.h.
>

Liam,

Do you think what I found is real?

Look forward your insight.

>After I change to <linux/kconfig.h> in kernel.h, the warning message is gone.
>
>Current behavior is not what you expected, right? We have a chance to include
>the kconfig.h which doesn't include generated/autoconf.h. 
>
>Hope not waste your time.
>
>-- 
>Wei Yang
>Help you, Help me

-- 
Wei Yang
Help you, Help me