[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 19 Nov 2015 23:09:55 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: Support for out-of-tree build
Thanks! Have you tested it? I get:
$ mkdir foo
$ cd foo
$ ../configure
$ make
make: *** No rule to make target `obj/include/bits', needed by `obj/include/bits/alltypes.h'. Stop.
I think this means there's still some cruft using the old include/bits
symlink rather than the proper pathname. See below:
On Thu, Nov 19, 2015 at 12:22:09AM +0000, Petr Hosek wrote:
> > > 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.
Did you mean "can't imagine a use"? To me this looks roughly like a
"second level of out-of-tree support" that's probably not useful, and
could be error-prone e.g. if somebody tries to get clever and use
objdir=., or could be dangerous (objdir=). I think it's best just to
hard-code it.
> > > +
> > > +$(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.
I don't follow. In current upstream musl, crt*.o end up in crt/ and
then get copied from there to lib/. But if, after the out-of-tree
patch, some object files end up in crt/$(ARCH)/ instead of crt/, we
need to make sure they make it to lib/ correctly.
Now for comments on the new patch:
> +objdir = obj
Let's remove objdir entirely and just use obj/.
> -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 = $(patsubst $(srcdir)/%.c,%.o,$(BASE_SRCS))
> +ARCH_SRCS = $(wildcard $(srcdir)/src/*/$(ARCH)/*.s $(srcdir)/src/*/$(ARCH)$(ASMSUBARCH)/*.sub)
> +ARCH_OBJS = $(patsubst $(srcdir)/%.sub,%.o,$(patsubst $(srcdir)/%.s,%.o,$(ARCH_SRCS)))
> +EXCLUDE_OBJS = $(patsubst $(srcdir)/%,%,$(subst /$(ARCH)$(ASMSUBARCH)/,/,$(subst /$(ARCH)/,/,$(patsubst $(srcdir)/%,%,$(ARCH_OBJS)))))
> +OBJS = $(addprefix $(objdir)/, $(filter-out $(EXCLUDE_OBJS), $(BASE_OBJS)) $(ARCH_OBJS) $(SUB_OBJS))
Not important at all, but REPLACED_OBJS might be a better name to
signify to people reading the makefile what this is about.
> -ARCH_INCLUDES = $(wildcard arch/$(ARCH)/bits/*.h)
> -ALL_INCLUDES = $(sort $(wildcard include/*.h include/*/*.h) $(GENH) $(ARCH_INCLUDES:arch/$(ARCH)/%=include/%))
> +ARCH_INCLUDES = $(wildcard $(srcdir)/arch/$(ARCH)/bits/*.h)
> +INCLUDES = $(wildcard $(srcdir)/include/*.h $(srcdir)/include/*/*.h)
> +ALL_INCLUDES = $(sort $(INCLUDES:$(srcdir)/%=%) $(GENH:$(objdir)/%=%) $(ARCH_INCLUDES:$(srcdir)/arch/$(ARCH)/%=include/%))
This is confusing, but probably no worse than what was already there.
> EMPTY_LIB_NAMES = m rt pthread crypt util xnet resolv dl
> EMPTY_LIBS = $(EMPTY_LIB_NAMES:%=lib/lib%.a)
> @@ -64,10 +71,22 @@ LDSO_PATHNAME = $(syslibdir)/ld-musl-$(ARCH)$(SUBARCH).so.1
>
> all: $(ALL_LIBS) $(ALL_TOOLS)
>
> +$(ALL_LIBS): | lib/
> +$(ALL_TOOLS): | tools/
> +$(CRT_LIBS:lib/%=$(objdir)/crt/%): | $(objdir)/crt/
> +$(OBJS) $(LOBJS): | $(sort $(dir $(OBJS)))
> +$(GENH): | $(objdir)/include/bits/
> +$(GENH_INT): | $(objdir)/src/internal/
> +
> +SRC_DIRS = $(sort $(dir $(ALL_LIBS) $(ALL_TOOLS) $(OBJS) $(GENH) $(GENH_INT)) $(addprefix $(objdir)/, crt/ include/))
> +
> +$(SRC_DIRS):
> + mkdir -p $@
> +
It looks like you're missing mkdir rules for everything but $(SRC_DIRS).
> -include/bits:
> - @test "$(ARCH)" || { echo "Please set ARCH in config.mak before running make." ; exit 1 ; }
> - ln -sf ../arch/$(ARCH)/bits $@
Removing the bits symlink is ok, but we need to preserve the logic
that prevents trying to build without ARCH set. Perhaps something
like:
ifeq ($(ARCH),)
$(error Please run configure or set ARCH in config.mak before running make)
endif
just after the inclusion of config.mak.
> 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)
> +$(NOSSP_SRCS:%.c=$(objdir)/%.o) $(NOSSP_SRCS:%.c=$(objdir)/%.lo): CFLAGS_ALL += $(CFLAGS_NOSSP)
This looks much better.
> -$(CRT_LIBS:lib/%=crt/%): CFLAGS_ALL += -DCRT
> +$(CRT_LIBS:lib/%=$(objdir)/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.
> +#$(dir $(patsubst %/,%,$(dir $(1))))$(notdir $(1:.s=.o)): $(1)
I suppose this comment was meant to be removed?
> -%.o: $(ARCH)$(ASMSUBARCH)/%.sub
> +$(objdir)/%.o: $(srcdir)/%.sub
> $(CC) $(CFLAGS_ALL_STATIC) -c -o $@ $(dir $<)$(shell cat $<)
Not new in your patch, but I wonder if using $$(cat $<) would make
more sense than $(shell cat $<) -- does the latter risk getting
evaluated even when the rule isn't being run?
> -lib/%.o: crt/%.o
> +lib/%.o: $(objdir)/crt/%.o
> cp $< $@
I don't see how this gets arch versions of crti.o/crtn.o. I think you
must be producing the dummy ones from the empty .c files, which won't
work. Actually once this is all done the dummy .c files there can
probably be removed, or we could remove them right away to eliminate
the need for another filter-out here (but I don't think that's really
needed anyway since it's a small fixed set of files we can hard-code).
BTW that made me realize one semantic change in how the new build
system treats arch files: src/*/$(ARCH)/*.s (and .sub and whatever
other types we add) will get used now even if there's no .c file by
the same name in the parent directory. This actually looks to be
really nice -- we can eliminate dummy .c files that are empty and only
have them when the generic implementation works for some/most archs.
We can also move the arch/$(ARCH)/src/* files to appropriate
src/*/$(ARCH) locations based on what code they go with.
> +test -f Makefile || ln -sf $srcdir/Makefile .
Is there a reason you do it this way rather than checking for
srcdir=.? The reason I ask is that existence of a stray makefile
that's not a symlink to the musl makefile should probably be an error
here rather than silently ignored. But maybe there are more things to
consider..
Rich
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.
