Re: linux-next: kbuild tree build failure

Re: linux-next: kbuild tree build failure

[Milton Miller]

In-Reply-To: <Pine.LNX.4.64.0807101654080.6791@localhost.localdomain>

On Fri Jul 11 00:59:25 EST 2008, Roman Zippel wrote:
> On Thu, 10 Jul 2008, Michael Ellerman wrote:
>
> > Well yes :)  But I think that's because you're thinking of
> > "end-users" and I'm thinking of "users" like myself - ie. _I_ use
> > Kconfig and I do expect myself to be able to type a 64-bit address.
>
> That doesn't really answer my question, why you need this.
>
> > > > --- .config.orig  2008-07-08 09:30:00.000000000 +1000
> > > > +++ .config       2008-07-08 09:30:43.000000000 +1000
> > > > @@ -370,9 +370,8 @@
> > > >  CONFIG_HOTPLUG_PCI_RPA=m
> > > >  CONFIG_HOTPLUG_PCI_RPA_DLPAR=m
> > > >  # CONFIG_HAS_RAPIDIO is not set
> > > > -CONFIG_PAGE_OFFSET=0xc000000000000000
> > > > -CONFIG_KERNEL_START=0xc000000002000000
> > > > -CONFIG_PHYSICAL_START=0x02000000
> > > > +CONFIG_PAGE_OFFSET=0xc0000000
> > > > +CONFIG_PHYSICAL_START=0x2000000
> > >
> > > Why is this worse? These are constants, you're not supposed to 
> change them
> > > anyway.
> > > The remaining values are generated in page.h and should be the 
> same as
> > > before. If that isn't the case and this patch produces a nonworking
> > > kernel, I'd like to hear about it.
> >
> > You're right the built kernel is fine. So it's not a bug,
>

But its horrible code.

> Good, could someone please ack whether the powerpc changes are 
> acceptable?
>

Well, since no one else has said it,
NAK

The primary reason I object is this:
>>>> Index: linux-2.6/include/asm-powerpc/page.h
>>>> ===================================================================
>>>> --- linux-2.6.orig/include/asm-powerpc/page.h
>>>> +++ linux-2.6/include/asm-powerpc/page.h
>>>> @@ -67,9 +67,15 @@
>>>>   * If you want to test if something's a kernel address, use 
>>>> is_kernel_addr().
>>>>   */
>>>>
>>>> -#define KERNELBASE      ASM_CONST(CONFIG_KERNEL_START)
>>>> +#ifdef CONFIG_PPC64
>>>> +#define PAGE_OFFSET    (ASM_CONST(CONFIG_PAGE_OFFSET) << 32)
>>>> +#define KERNELBASE     
>>>> (PAGE_OFFSET+ASM_CONST(CONFIG_PHYSICAL_START))
>>>> +#define LOAD_OFFSET    PAGE_OFFSET
>>>> +#else
>>>> +#define KERNELBASE     ASM_CONST(CONFIG_KERNEL_START)
>>>>  #define PAGE_OFFSET    ASM_CONST(CONFIG_PAGE_OFFSET)
>>>> -#define LOAD_OFFSET    
>>>> ASM_CONST((CONFIG_KERNEL_START-CONFIG_PHYSICAL_START))
>>>> +#define LOAD_OFFSET    
>>>> (ASM_CONST(CONFIG_KERNEL_START)-ASM_CONST(CONFIG_PHYSICAL_START))
>>>> +#endif
>>>>
>>>>  #if defined(CONFIG_RELOCATABLE) && defined(CONFIG_FLATMEM)
>>>>  #ifndef __ASSEMBLY__

(1) #define PAGE_OFFSET    (ASM_CONST(CONFIG_PAGE_OFFSET) << 32)

It creates unreadable code, where two defines with almost the same name 
(the only difference being
the CONFIG_ prefix, which is often ignored when scanning) contains 
radically different values.

(2)  #define PAGE_OFFSET    ASM_CONST(CONFIG_PAGE_OFFSET)

It creates config variables that mean different things depending on 
other config variables
The 32 and 64 bit powerpc kernel share a common source, a config 
variable should be used for
only one purpose.



> > but I think it is nicer to have the real values in the .config.
>
> Why?

Mostly consistency between the different portions of the archticture.

As I remember, this code was adjusted and some of the defines moved 
from page.h
as part of the 32 bit relocatable kernel for 85xx booke ASMP support.

The 32 bit kernel has advanced options to change the VMA split, which 
enable
direct user input when explicitly defined.  That allows us to not need 
patches
for the embedded boards who need some other split than 3G/1G.  So there
is the reason that we have this directly specified in Kconfig at all.

While the 64 bit kernel doesn't need to actually change the page 
offset, as we
don't support the full 64 bits of the real address anyways (in fact, the
archtiecture prevents us from doing so) and therefore don't need to 
adjust
the effective address spilt between user and kernel.

But introducing config variables that mean different things is 
UNMAINTAINABLE.

Also, I remember, CONFIG_PAGE_OFFSET is used by the linker script and 
previously
page.h was conditionally included.   Does it always include page.h now?



On a seperate note,
>>>>  config PINT3_ASSIGN
>>>>         hex "PINT3_ASSIGN"
>>>>         depends on PINTx_REASSIGN
>>>> -       default 0x02020303
>>>> +       default 0x2020303

is harder to read.   The value is a list of 4 1 byte values, but you 
have hidden the first nibble making parsing the rest of the value hard.

>>>>
>>>>  config IRAM_SIZE
>>>>         hex "Internal memory size (hex)"
>>>>         depends on (CHIP_M32700 || CHIP_M32102 || CHIP_VDEC2 || 
>>>> CHIP_OPSP || CHIP_M32104) && DISCONTIGMEM
>>>> -       default "00080000" if CHIP_M32700
>>>> -       default "00010000" if CHIP_M32102 || CHIP_OPSP || 
>>>> CHIP_M32104
>>>> -       default "00008000" if CHIP_VDEC2
>>>> +       default "0x80000" if CHIP_M32700
>>>> +       default "0x10000" if CHIP_M32102 || CHIP_OPSP || CHIP_M32104
>>>> +       default "0x8000" if CHIP_VDEC2

Likewize, I find it easier to mentally check the order of magnitude and 
compare sizes when they have
leading zeros and are right aligned.


Going to another email in the thread,

On Fri Jul 11 00:52:25 EST 2008, Roman Zippel wrpte:
> On Tue, 8 Jul 2008, Sam Ravnborg wrote:
> > We use Kconfig for a mixture of user editable values and fixed
> > configuration values.
> > And I agree that asking the user to input a 64 bit number is not 
> usefull.
> >
> > But keeping support for 64 bit values is what I would consider
> > expected functionality.
...
> > > This would also ease on any portability issues
> > > (kconfig is compiled with the host compiler not the target 
> compiler).
> >
> > We use strtol() in a few places in symbol.c already where we do an
> > implicit conversion to int. Why did this not cause us problems 
> before?
> >
> > Is it because these code paths are only triggered when we deal with 
> ranges?
> > If so we could 'fix' strdup_type() to not use strto{,u}l() so it
> > is 64 bit clean and we are back to old behaviour.
>
> Ranges are the primary reason I made it consistent with this.
> If we really wanted to support 64bit numbers, it would create only more
> problems. First you have to make sure that on every build host (i.e 
> also
> non-Linux) strtoll() is available. Then how it should these numbers be
> represented? On 32bit these may need a 'll' postfix, but the powerpc
> example already shows, that there are different requirements, so they 
> use
> ASM_CONST for that. How should this postprecessing be integrated into
> kconfig?

I think the architectures will already do something like ASM_CONST in
their C code when its needed.  This should not be the responsibility of
kbuild.

If you are worried about users tring to set values that are too high,
then make the types be hex8, hex16, hex32, and hex64.

milton

Re: linux-next: kbuild tree build failure

[Roman Zippel]

Hi,

On Sat, 12 Jul 2008, Milton Miller wrote:

> (1) #define PAGE_OFFSET    (ASM_CONST(CONFIG_PAGE_OFFSET) << 32)
> 
> It creates unreadable code, where two defines with almost the same name (the
> only difference being
> the CONFIG_ prefix, which is often ignored when scanning) contains radically
> different values.
> 
> (2)  #define PAGE_OFFSET    ASM_CONST(CONFIG_PAGE_OFFSET)

Giving it different names is not really difficult. Any objections to 
CONFIG_PAGE_HIGH_OFFSET?

> On a seperate note,
> > > > >  config PINT3_ASSIGN
> > > > >         hex "PINT3_ASSIGN"
> > > > >         depends on PINTx_REASSIGN
> > > > > -       default 0x02020303
> > > > > +       default 0x2020303
> 
> is harder to read.   The value is a list of 4 1 byte values, but you have
> hidden the first nibble making parsing the rest of the value hard.

Sam mentioned that already, but that's a situation where the warning can 
be relaxed.

> If you are worried about users tring to set values that are too high,
> then make the types be hex8, hex16, hex32, and hex64.

It's not this, I value consistency as much as you and the values are 
sometimes used as integers, so a working range is needed. Using simple 
integers keeps things much simpler and as the ASM_CONST example shows any 
bigger values are not necessarily directly usable anyway.

bye, Roman

Re: linux-next: kbuild tree build failure

[Milton Miller]

Hi Roman.

I saw your reply on the list archives but can not find
it in my inbox.

On Sun Jul 13 at 09:21:08 EST 2008, Roman Zippel wrote:
> On Sat, 12 Jul 2008, Milton Miller wrote:
>
>> (1) #define PAGE_OFFSET    (ASM_CONST(CONFIG_PAGE_OFFSET) << 32)
>>
>> It creates unreadable code, where two defines with almost the same
> name (the
>> only difference being
>> the CONFIG_ prefix, which is often ignored when scanning) contains
> radically
>> different values.
>>
>> (2)  #define PAGE_OFFSET    ASM_CONST(CONFIG_PAGE_OFFSET)
>
> Giving it different names is not really difficult. Any objections to
> CONFIG_PAGE_HIGH_OFFSET?

Once you remove the symmetry and add the ifdef to page.h, we need to
reevaluate its presence in Kconfig.   I can't think of a reason to
have a partial value, and therefore would instead say take out the
config variable on PPC64 as the PPC32 address split reason is moot.

>
>> On a seperate note,
>>>>>>  config PINT3_ASSIGN
>>>>>>         hex "PINT3_ASSIGN"
>>>>>>         depends on PINTx_REASSIGN
>>>>>> -       default 0x02020303
>>>>>> +       default 0x2020303
>>
>> is harder to read.   The value is a list of 4 1 byte values, but you
>> have hidden the first nibble making parsing the rest of the value 
>> hard.
>
> Sam mentioned that already, but that's a situation where the warning 
> can
> be relaxed.

The warning can be relaxed?   What are you talking about?

I was trying to make a case for leading zeros without actually stating
what I was asking for.

You changes did s/(0x)?0*/0x/, y/A-F/a-f/ -- that is you made all hex
constants conform to "0x%x".   I was arguing for 0x%8.8x, or just 
leaving
them as formatted.

>
>> If you are worried about users tring to set values that are too high,
>> then make the types be hex8, hex16, hex32, and hex64.
>
> It's not this, I value consistency as much as you and the values are
> sometimes used as integers, so a working range is needed. Using simple
> integers keeps things much simpler and as the ASM_CONST example shows 
> any
> bigger values are not necessarily directly usable anyway.

How many places want to range check hex numbers?   Is it just setting
the number of digits input?  If strtoull is to hard, can we just do
the check as strings of digits?

If we keep this new restriction on hex numbers, it needs to be in
the Documentation, even if its prefixed by "currently".

milton

linux-next: kbuild tree build failure

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

Hi Sam,

Today's linux-next build (powerpc ppc64_defconfig) failed like this:

powerpc-linux-ld:arch/powerpc/kernel/vmlinux.lds:190: syntax error

Caused by commit 59d59d97e34c6593bec43666679b28da5d583958 ("Improve
vmlinux.lds.h support for arch specific linker scripts") which was
clearly not build tested - don't do that! There is a missing \ at the end
of a line in the definition of the HEAD_SECTION macro.

I have used the version of the kbuild tree from next-20090605.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: linux-next: kbuild tree build failure

[Sam Ravnborg]

On Tue, Jun 09, 2009 at 11:48:20AM +1000, Stephen Rothwell wrote:
> Hi Sam,
> 
> Today's linux-next build (powerpc ppc64_defconfig) failed like this:
> 
> powerpc-linux-ld:arch/powerpc/kernel/vmlinux.lds:190: syntax error
> 
> Caused by commit 59d59d97e34c6593bec43666679b28da5d583958 ("Improve
> vmlinux.lds.h support for arch specific linker scripts") which was
> clearly not build tested - don't do that! There is a missing \ at the end
> of a line in the definition of the HEAD_SECTION macro.
> 
> I have used the version of the kbuild tree from next-20090605.

My bad - obviously.
I will fix when the kids are sleeping (as in a few hours from now).

	Sam

Re: linux-next: kbuild tree build failure

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 321 bytes --]

Hi Sam,

On Tue, 9 Jun 2009 18:15:12 +0200 Sam Ravnborg <sam@ravnborg.org> wrote:
>
> I will fix when the kids are sleeping (as in a few hours from now).

Thanks.  I won't be needing it for about 7 hours.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: linux-next: kbuild tree build failure

[Sam Ravnborg]

On Wed, Jun 10, 2009 at 02:19:59AM +1000, Stephen Rothwell wrote:
> Hi Sam,
> 
> On Tue, 9 Jun 2009 18:15:12 +0200 Sam Ravnborg <sam@ravnborg.org> wrote:
> >
> > I will fix when the kids are sleeping (as in a few hours from now).
> 
> Thanks.  I won't be needing it for about 7 hours.

Fixed now (and build tested all changes this time).

Thanks,
	Sam

Re: linux-next: kbuild tree build failure

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 266 bytes --]

Hi Sam,

On Tue, 9 Jun 2009 23:04:27 +0200 Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Fixed now (and build tested all changes this time).

Excellent!

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

linux-next: kbuild tree build failure

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

Hi Sam,

Today's linux-next build (x86_64 allmodconfig) failed like this:

In file included from arch/x86/kernel/syscall_64.c:28:
arch/x86/include/asm/unistd_64.h:665: error: array index in initializer exceeds array bounds
arch/x86/include/asm/unistd_64.h:665: error: (near initialization for 'sys_call_table')
arch/x86/include/asm/unistd_64.h:665: warning: excess elements in array initializer
arch/x86/include/asm/unistd_64.h:665: warning: (near initialization for 'sys_call_table')

Exposed by commit a2e2725541fad72416326798c2d7fa4dafb7d337 ("net:
Introduce recvmmsg socket syscall").  include/asm/asm-offsets.h did not
get updated even though arch/x86/kernel/asm-offsets.s did.  Ah,
generated/asm-offsets.h has been updated but my build tree still has the
(generated in a previous build) include/asm/asm-offsets.h which is
presumably found before the new arch/x86/include/asm/asm-offsets.h
introduced by commit fe5ff47a5d79ca99ec7e3155eb19469b44905b2c ("kbuild:
move asm-offsets.h to include/generated").

Sam, can you think of a better migration path?

I removed include/asm/asm_offsets.h from my object tree and everything
builds ok.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: linux-next: kbuild tree build failure

[Sam Ravnborg]

On Wed, Oct 14, 2009 at 12:20:24PM +1100, Stephen Rothwell wrote:
> Hi Sam,
> 
> Today's linux-next build (x86_64 allmodconfig) failed like this:
> 
> In file included from arch/x86/kernel/syscall_64.c:28:
> arch/x86/include/asm/unistd_64.h:665: error: array index in initializer exceeds array bounds
> arch/x86/include/asm/unistd_64.h:665: error: (near initialization for 'sys_call_table')
> arch/x86/include/asm/unistd_64.h:665: warning: excess elements in array initializer
> arch/x86/include/asm/unistd_64.h:665: warning: (near initialization for 'sys_call_table')
> 
> Exposed by commit a2e2725541fad72416326798c2d7fa4dafb7d337 ("net:
> Introduce recvmmsg socket syscall").  include/asm/asm-offsets.h did not
> get updated even though arch/x86/kernel/asm-offsets.s did.  Ah,
> generated/asm-offsets.h has been updated but my build tree still has the
> (generated in a previous build) include/asm/asm-offsets.h which is
> presumably found before the new arch/x86/include/asm/asm-offsets.h
> introduced by commit fe5ff47a5d79ca99ec7e3155eb19469b44905b2c ("kbuild:
> move asm-offsets.h to include/generated").
> 
> Sam, can you think of a better migration path?

I once thought of forefully removing include/asm to avoid this.
Maybe we could let "make clean" remove include/asm -
I expect this would have helped in this situation?

I'm planning to redo the serie within the next couple of weeks anyway.

	Sam