Re: Support for out-of-tree build

[Rich Felker <dalias@libc.org>]

On Wed, Nov 18, 2015 at 08:19:28PM +0000, Petr Hosek wrote:
> > Discussion on irc, along with some basic testing, showed that this
> > does not work. The obvious solution with $(subst ...) also does not
> > work because it's plausible that $(srcdir) also contains the substring
> > /$(ARCH)/, in which case it would get messed up. However I did find a
> > form that seems to work reliably and that's not hideous:
> >
> > $(patsubst %,$(srcdir)/%,$(subst /$(ARCH)/,/,$(patsubst
> > $(srcdir)/%,%,$(ARCH_OBJS))))
> >
> > That yields the list of object files to filter out, which can then be
> > passed into $(filter-out ...).
> >
> 
> I've reworked the patch following our discussion on IRC by placing all
> object files into a directory which enables the use of implicit rules. The
> rest of the logic is similar to what you proposed as well as to my previous
> patches: constructing the list of objects and then filtering out files for
> which (sub)architecture versions are available.

I've tried to review it in detail this time, with comments inline
below.

> -SRCS = $(sort $(wildcard src/*/*.c arch/$(ARCH)/src/*.c))
> -OBJS = $(SRCS:.c=.o)
> +BASE_SRCS = $(sort $(wildcard $(srcdir)/src/*/*.c $(srcdir)/arch/$(ARCH)/src/*.c))
> +BASE_OBJS = $(BASE_SRCS:$(srcdir)/%.c=%.o)
> +ARCH_SRCS = $(wildcard $(srcdir)/src/*/$(ARCH)/*.s)
> +ARCH_OBJS = $(ARCH_SRCS:$(srcdir)/%.s=%.o)
> +SUB_SRCS = $(wildcard $(srcdir)/src/*/$(ARCH)$(ASMSUBARCH)/*.sub)
> +SUB_OBJS = $(SUB_SRCS:$(srcdir)/%.sub=%.o)
> +EXCLUDE_ARCH_OBJS = $(patsubst $(srcdir)/%,%,$(subst /$(ARCH)/,/,$(patsubst $(srcdir)/%,%,$(ARCH_OBJS))))
> +EXCLUDE_SUB_OBJS = $(patsubst $(srcdir)/%,%,$(subst /$(ARCH)$(ASMSUBARCH)/,/,$(patsubst $(srcdir)/%,%,$(SUB_OBJS))))
> +OBJS = $(addprefix $(objdir)/, $(filter-out $(EXCLUDE_SUB_OBJS) $(EXCLUDE_ARCH_OBJS), $(BASE_OBJS)) $(ARCH_OBJS) $(SUB_OBJS))

Why not just put all the arch files (whether they come from .s or
.sub, or in the future .c or .S) together in ARCH_OBJS? I think this
can all be done with no intermediate *_SRCS vars (and probably even
without SRCS) with some explicit $(patsubst...) etc. Actually I'm a
bit surprised if things like this work:

	$(BASE_SRCS:$(srcdir)/%.c=%.o)

I had trouble with % in the middle of the pattern not doing what I
expected it to.

>  CFLAGS_ALL = $(CFLAGS_C99FSE)
> -CFLAGS_ALL += -D_XOPEN_SOURCE=700 -I./arch/$(ARCH) -I./src/internal -I./include
> +CFLAGS_ALL += -D_XOPEN_SOURCE=700 -I$(srcdir)/arch/$(ARCH) $(sort -I$(objdir)/src/internal -I$(srcdir)/src/internal) $(sort -I$(objdir)/include -I$(srcdir)/include)

What is the $(sort...) for now? Before I thought it was to remove
duplicates, but now they will not be duplicates unless you're trying
to support objdir=. -- is that your intent? I don't think it will
work based on our discussions yesterday.

>  clean:
> -	rm -f crt/*.o
> +	rm -f $(objdir)/crt/*.o
>  	rm -f $(OBJS)
>  	rm -f $(LOBJS)
>  	rm -f $(ALL_LIBS) lib/*.[ao] lib/*.so

This can probably just be rm -rf obj, but perhaps we should wait to
make that change. And seeing this, I'm mildly in favor of hard-coding
obj/ (everywhere) rather than using $(objdir)/ simply because the
latter is really dangerous if you override it on the command line with
an unwanted setting (like make clean objdir=/).

> -include/bits:
> +$(objdir)/include/bits:
>  	@test "$(ARCH)" || { echo "Please set ARCH in config.mak before running make." ; exit 1 ; }
>  	ln -sf ../arch/$(ARCH)/bits $@

I think this may be problematic -- for an in-tree build, you'll get a
bits symlink in the source include dir, and then out-of-tree builds
might use it. Include order might prevent that but it sounds fragile.
We should probably eliminate the bits symlink here.

> -include/bits/alltypes.h.in: include/bits
> -
> -include/bits/alltypes.h: include/bits/alltypes.h.in include/alltypes.h.in tools/mkalltypes.sed
> -	sed -f tools/mkalltypes.sed include/bits/alltypes.h.in include/alltypes.h.in > $@
> +$(objdir)/include/bits/alltypes.h: $(srcdir)/arch/$(ARCH)/bits/alltypes.h.in $(srcdir)/include/alltypes.h.in $(srcdir)/tools/mkalltypes.sed $(objdir)/include/bits
> +	sed -f $(srcdir)/tools/mkalltypes.sed $(srcdir)/arch/$(ARCH)/bits/alltypes.h.in $(srcdir)/include/alltypes.h.in > $@

So the generated alltypes.h goes in obj/include/bits? But the
installation rule does not seem to get it from there. It seems to only
look in arch/$(ARCH)/bits/%. I guess the above symlink causes them to
be the same, but I don't think there's actually a dependency in the
makefile to let make know that...

> -src/internal/version.h: $(wildcard VERSION .git)
> -	printf '#define VERSION "%s"\n' "$$(sh tools/version.sh)" > $@
> +$(objdir)/src/internal/version.h: $(wildcard $(srcdir)/VERSION $(srcdir)/.git)
> +	printf '#define VERSION "%s"\n' "$$(cd $(srcdir); sh tools/version.sh)" > $@
>  
> -src/internal/version.lo: src/internal/version.h
> +$(objdir)/src/internal/version.lo: $(objdir)/src/internal/version.h
>  
> -crt/rcrt1.o src/ldso/dlstart.lo src/ldso/dynlink.lo: src/internal/dynlink.h arch/$(ARCH)/reloc.h
> +$(objdir)/crt/rcrt1.o $(objdir)/src/ldso/dlstart.lo $(objdir)/src/ldso/dynlink.lo: $(srcdir)/src/internal/dynlink.h $(srcdir)/arch/$(ARCH)/reloc.h
>  
> -crt/crt1.o crt/Scrt1.o crt/rcrt1.o src/ldso/dlstart.lo: $(wildcard arch/$(ARCH)/crt_arch.h)
> +$(objdir)/crt/crt1.o $(objdir)/crt/scrt1.o $(objdir)/crt/rcrt1.o $(objdir)/src/ldso/dlstart.lo: $(srcdir)/arch/$(ARCH)/crt_arch.h
>  
> -crt/rcrt1.o: src/ldso/dlstart.c
> +$(objdir)/crt/rcrt1.o: $(srcdir)/src/ldso/dlstart.c
>  
> -crt/Scrt1.o crt/rcrt1.o: CFLAGS_ALL += -fPIC
> +$(objdir)/crt/Scrt1.o $(objdir)/crt/rcrt1.o: CFLAGS_ALL += -fPIC
>  
> -OPTIMIZE_SRCS = $(wildcard $(OPTIMIZE_GLOBS:%=src/%))
> -$(OPTIMIZE_SRCS:%.c=%.o) $(OPTIMIZE_SRCS:%.c=%.lo): CFLAGS += -O3
> +OPTIMIZE_SRCS = $(wildcard $(OPTIMIZE_GLOBS:%=$(srcdir)/src/%))
> +$(OPTIMIZE_SRCS:$(srcdir)/%.c=$(objdir)/%.o) $(OPTIMIZE_SRCS:$(srcdir)/%.c=$(objdir)/%.lo): CFLAGS += -O3
>  
>  MEMOPS_SRCS = src/string/memcpy.c src/string/memmove.c src/string/memcmp.c src/string/memset.c
> -$(MEMOPS_SRCS:%.c=%.o) $(MEMOPS_SRCS:%.c=%.lo): CFLAGS_ALL += $(CFLAGS_MEMOPS)
> +$(MEMOPS_SRCS:%.c=$(objdir)/%.o) $(MEMOPS_SRCS:%.c=$(objdir)/%.lo): CFLAGS_ALL += $(CFLAGS_MEMOPS)
>  
>  NOSSP_SRCS = $(wildcard crt/*.c) \
> -	src/env/__libc_start_main.c src/env/__init_tls.c \
> -	src/thread/__set_thread_area.c src/env/__stack_chk_fail.c \
> -	src/string/memset.c src/string/memcpy.c \
> -	src/ldso/dlstart.c src/ldso/dynlink.c
> -$(NOSSP_SRCS:%.c=%.o) $(NOSSP_SRCS:%.c=%.lo): CFLAGS_ALL += $(CFLAGS_NOSSP)
> -
> -$(CRT_LIBS:lib/%=crt/%): CFLAGS_ALL += -DCRT
> -
> -# This incantation ensures that changes to any subarch asm files will
> -# force the corresponding object file to be rebuilt, even if the implicit
> -# rule below goes indirectly through a .sub file.
> -define mkasmdep
> -$(dir $(patsubst %/,%,$(dir $(1))))$(notdir $(1:.s=.o)): $(1)
> -endef
> -$(foreach s,$(wildcard src/*/$(ARCH)*/*.s),$(eval $(call mkasmdep,$(s))))

Simply removing this will break dependency-based regeneration of
object files that use .sub files when the underlying .s file changes.
Maybe we don't care since .sub files will be removed soon, but it
should be noted as a regression for the time being, or we should just
transform this for out-of-tree support.

> +	$(srcdir)/src/env/__libc_start_main.c $(srcdir)/src/env/__init_tls.c \
> +	$(srcdir)/src/thread/__set_thread_area.c $(srcdir)/src/env/__stack_chk_fail.c \
> +	$(srcdir)/src/string/memset.c $(srcdir)/src/string/memcpy.c \
> +	$(srcdir)/src/ldso/dlstart.c $(srcdir)/src/ldso/dynlink.c

It might make sense to use a function to apply a prefix to all of
these rather than repeating $(srcdir)/ over and over.

> +$(NOSSP_SRCS:$(srcdir)/%.c=$(objdir)/%.o) $(NOSSP_SRCS:$(srcdir)/%.c=$(objdir)/%.lo): CFLAGS_ALL += $(CFLAGS_NOSSP)

Actually I think we don't even want $(srcdir) there to begin with.
It's just removed immediately.

> +
> +$(CRT_LIBS:lib/%=$(objdir)/crt/%): CFLAGS_ALL += -DCRT

I'm not clear on whether crti.o/crtn.o end up in crt/ or crt/$(ARCH)/
but if it's the latter we need to handle this somehow.

> -%.lo: $(ARCH)$(ASMSUBARCH)/%.sub
> -	$(CC) $(CFLAGS_ALL_SHARED) -c -o $@ $(dir $<)$(shell cat $<)
> +$(objdir)/%.lo: $(srcdir)/%.sub
> +	 $(CC) $(CFLAGS_ALL_SHARED) -c -o $@ $(dir $<)$(shell cat $<)

Looks like you got a spurious space in there.

> -lib/%.o: crt/%.o
> +lib/%.o: $(objdir)/crt/%.o
>  	cp $< $@

This also depends on crti.o/crtn.o not being in an arch subdir.

> +$(DESTDIR)$(includedir)/bits/%: $(srcdir)/arch/$(ARCH)/bits/%
> +	$(INSTALL) -D -m 644 $< $@
> +
>  $(DESTDIR)$(includedir)/bits/%: arch/$(ARCH)/bits/%
>  	$(INSTALL) -D -m 644 $< $@

This is the rule I mentioned above that doesn't seem to have any way
for make to know how to generate arch/$(ARCH)/bits/alltypes.h unless
it already happened to be generated.

> +# Get the musl source dir for out-of-tree builds

I don't recall if I've followed this principle myself, but it probably
makes sense to avoid writing musl explicitly all over the configure
script in case someone goes and reuses parts of it for another
project.

> +#
> +if test -z "$srcdir" ; then
> +srcdir="${0%/configure}"
> +stripdir srcdir
> +fi
> +abs_builddir="$(pwd)" || fail "$0: cannot determine working directory"
> +abs_srcdir="$(cd $srcdir && pwd)" || fail "$0: invalid source directory $srcdir"
> +test "$abs_srcdir" = "$abs_builddir" && srcdir=.
> +ln -sf $srcdir/Makefile .

Doesn't this do ln -sf ./Makefile . when srcdir=.? That needs to be
suppressed, and the ln should probably not happen until the configure
script has "succeeded", at the same time config.mak is generated.

Rich