Re: [RFC PATCH v3 09/10] lib: libos build scripts and documentation

[Hajime Tazaki <tazaki@sfc.wide.ad.jp>]

Hi Paul,

many thanks for your review. 

all the fixes will be on next patchset.
my comments are below.

At Mon, 20 Apr 2015 22:43:07 +0200,
Paul Bolle wrote:
> 
> Some random observations while I'm still trying to wrap my head around
> all this (which might take quite some time).
> 
> On Sun, 2015-04-19 at 22:28 +0900, Hajime Tazaki wrote:
> > --- /dev/null
> > +++ b/arch/lib/Kconfig
> > @@ -0,0 +1,124 @@
> > +menuconfig LIB
> > +       bool "LibOS-specific options"
> > +       def_bool n
> 
> This is the start of the Kconfig parse for lib. (That would basically
> still be true even if you didn't set KBUILD_KCONFIG, see below.) So why
> not do something like all arches do:
> 
> config LIB
> 	def_bool y
> 	select [...]
> 
> Ie, why would someone want to build for ARCH=lib and still not set LIB?

agreed. fixed.

> > +config EXPERIMENTAL
> > +	def_bool y
> 
> Unneeded: removed treewide in, I think, 2014.

thanks. fixed.

> > +config MMU
> > +        def_bool n
> 
> Add empty line.
> 
> > +config FPU
> > +        def_bool n
> 
> Ditto.

both are fixed.

> > +config KTIME_SCALAR
> > +       def_bool y
> 
> This one is unused.

deleted.

> > +config GENERIC_BUG
> > +	def_bool y
> > +	depends on BUG
> 
> Add empty line here.

fixed.

> > +config GENERIC_FIND_NEXT_BIT
> > +	def_bool y
> 
> This one is unused too.

deleted.

> > +config SLIB
> > +       def_bool y
> 
> You've also added SLIB to init/Kconfig in 02/10. But "make ARCH=lib
> *config" will never visit init/Kconfig, will it? And, apparently, none
> of SL[AOU]B are wanted for lib. So I think the entry for config SLIB in
> that file can be dropped (as other arches will never see it because it
> depends on LIB).
> 
> (Note that I haven't actually looked into all the Kconfig entries added
> above. Perhaps I might do that. But I'm pretty sure most of the time all
> I can say is: "I have no idea why this entry defaults to $VALUE".)

I intended to SLIB be a generic one, not only for the
arch/lib, as we discussed during v2 patch. 

but, you're right: for the moment, no one uses SLIB, we
don't visit init/Kconfig, I dropped config SLIB entry from
init/Kconfig.

> > +source "net/Kconfig"
> > +
> > +source "drivers/base/Kconfig"
> > +
> > +source "crypto/Kconfig"
> > +
> > +source "lib/Kconfig"
> > +
> > +
> 
> Trailing empty lines.

deleted. thanks.

> > diff --git a/arch/lib/Makefile b/arch/lib/Makefile
> > new file mode 100644
> > index 0000000..d8a0bf9
> > --- /dev/null
> > +++ b/arch/lib/Makefile
> > @@ -0,0 +1,251 @@
> > +ARCH_DIR := arch/lib
> > +SRCDIR=$(dir $(firstword $(MAKEFILE_LIST)))
> 
> Do you use SRCDIR?

no. deleted the line.

> > +DCE_TESTDIR=$(srctree)/tools/testing/libos/
> > +KBUILD_KCONFIG := arch/$(ARCH)/Kconfig
> 
> I think you copied this from arch/um/Makefile. But arch/um/ is, well,
> special. Why should lib not start the kconfig parse in the file named
> Kconfig? And if you want to start in arch/lib/Kconfig, it would be nice
> to add a mainmenu (just like arch/x86/um/Kconfig does).

right now, 'lib' only wants to eat arch/lib/Kconfig so that
build and link its wanted files instead of configurable one.

so I beilive arch/lib is also special as arch/um is.

I added a mainmenu btw. thanks.

> (I don't read Makefilese well enough to understand the rest of this
> file. I think it's scary.)

indeed. thank you again to review the cryptic files..

> When I did
>     make ARCH=lib menuconfig
> 
> I saw (among other things):
>     arch/lib/Makefile.print:41: target `trace/' given more than once in the same rule.
>     arch/lib/Makefile.print:41: target `trace/' given more than once in the same rule.
>     arch/lib/Makefile.print:41: target `trace/' given more than once in the same rule.
>     arch/lib/Makefile.print:41: target `trace/' given more than once in the same rule.
>     arch/lib/Makefile.print:41: target `lzo/' given more than once in the same rule.
(snip)
>     arch/lib/Makefile.print:41: target `ppp/' given more than once in the same rule.
>     arch/lib/Makefile.print:41: target `slip/' given more than once in the same rule.
> 
> I have no idea why. Unclean tree?

this was due to inappropriate handling of the internal
directory listing procedure. fixed.

> > +.PHONY : core
> > +.NOTPARALLEL : print $(subdirs) $(final-obj-m)
> 
> > --- /dev/null
> > +++ b/arch/lib/processor.mk
> > @@ -0,0 +1,7 @@
> > +PROCESSOR=$(shell uname -m)
> > +PROCESSOR_x86_64=64
> > +PROCESSOR_i686=32
> > +PROCESSOR_i586=32
> > +PROCESSOR_i386=32
> > +PROCESSOR_i486=32
> > +PROCESSOR_SIZE=$(PROCESSOR_$(PROCESSOR))
> 
> The rest of the tree appears to use BITS instead of PROCESSOR_SIZE. And
> I do hope there's a cleaner way for lib to set PROCESSOR_SIZE than this.

the variable PROCESSOR_SIZE is only used by
arch/lib/Makefile, with the following lines.

> +ifeq ($(PROCESSOR_SIZE),64)
> +CFLAGS+= -DCONFIG_64BIT
> +endif

Thus it eventually uses CONFIG_64BIT.

I think a cleaner way is to follow the way of arch/um, like
below: I deleted processor.mk and PROCESSOR_SIZE variable.

ifeq ($(SUBARCH),x86)
  ifeq ($(shell uname -m),x86_64)
	CFLAGS+= -DCONFIG_64BIT
endif

though it's not able to cross-compile yet.

again, thank you so much.
I'll be back very soon (v4 patch).

-- Hajime