[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 19 Nov 2015 00:22:09 +0000
From: Petr Hosek <phosek@...omium.org>
To: musl@...ts.openwall.com
Subject: Re: Support for out-of-tree build
On Wed, Nov 18, 2015 at 1:45 PM Rich Felker <dalias@...c.org> wrote:
> > -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.
>
I have merged ARCH_* and SUB_* into ARCH_*.
> > 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.
>
Correct, this would only make sense if objdir=. but then the implicit rules
wouldn't work so we don't need to support that scenario.
> > 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=/).
>
That's why I avoided using rm -rf $(objdir). I can imagine the use case for
setting $(objdir) to a different location, changing this to make it
hard-coded wouldn't be a problem if you prefer.
> > -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.
>
I agree, if we're not going to support the objdir=. scenario, we can remove
the symlink. It'll also simplify some of the directory dependencies.
> > -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...
>
That's fixed, well spotted.
> > -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.
>
Reverted and update to work with the new setup.
> > + $(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.
>
Removed.
> > +
> > +$(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.
>
These end up in crt/ even in the current upstream implementation.
> > -%.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.
>
Removed.
> > -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.
>
Fixed.
> > +# 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.
>
Removed.
> > +#
> > +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.
>
Check added and moved to the bottom. of the configure script.
Content of type "text/html" skipped
View attachment "support-out-of-tree-build.patch" of type "text/x-patch" (13615 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
