| Message ID | 20210824105038.1257926-6-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Build system improvements, now with out-of-tree build! | expand |
On 24.08.2021 12:49, Anthony PERARD wrote: > This replace the use of a single .c file use for multiple .o file by > creating multiple .c file including the first one. > > There's quite a few issues with trying to build more than one object > file from a single source file: there's is a duplication of the make > rules to generate those targets; there is an additional ".file" symbol > added in order to differentiate between the object files; and the > tools/symbols have an heuristic to try to pick up the right ".file". > > This patch adds new .c source file which avoid the need to add a > second ".file" symbol and thus avoid the need to deal with those > issues. > > Also remove __OBJECT_FILE__ from $(CC) command line as it isn't used > anywhere anymore. And remove the macro "build-intermediate" since the > generic rules for single targets can be used. > > And rename the objects in mm/hap/ to remove the extra "level". > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Hmm, when replying to 00/51 I didn't recall I gave an R-b for this one already. I'd like to restrict it some: It should be taken to stand for the technical correctness of the change. Nevertheless I'm not really happy with the introduction of the various tiny source files. I've meanwhile been wondering: Can't these be generated (in the build tree, as soon as that's possible to be separate) rather than getting put in the repo? Jan
On Tue, Sep 07, 2021 at 08:14:14AM +0200, Jan Beulich wrote: > On 24.08.2021 12:49, Anthony PERARD wrote: > > This replace the use of a single .c file use for multiple .o file by > > creating multiple .c file including the first one. > > > > There's quite a few issues with trying to build more than one object > > file from a single source file: there's is a duplication of the make > > rules to generate those targets; there is an additional ".file" symbol > > added in order to differentiate between the object files; and the > > tools/symbols have an heuristic to try to pick up the right ".file". > > > > This patch adds new .c source file which avoid the need to add a > > second ".file" symbol and thus avoid the need to deal with those > > issues. > > > > Also remove __OBJECT_FILE__ from $(CC) command line as it isn't used > > anywhere anymore. And remove the macro "build-intermediate" since the > > generic rules for single targets can be used. > > > > And rename the objects in mm/hap/ to remove the extra "level". > > > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Hmm, when replying to 00/51 I didn't recall I gave an R-b for this one > already. I'd like to restrict it some: It should be taken to stand for > the technical correctness of the change. Nevertheless I'm not really > happy with the introduction of the various tiny source files. I've > meanwhile been wondering: Can't these be generated (in the build tree, > as soon as that's possible to be separate) rather than getting put in > the repo? Do we really need to generated those never to be change tiny source file? Do we really need to make the build system a lot more complicated? With those tiny source file in a different directory to the main source file, we need to add "-I" to CFLAGS. (source tree vs build tree.) I don't like preparation phase, I'd rather do just-in-time generation of those tiny file (if we really have too...) as to let make organize itself on when to do things. That mean, extra targets on how to generate the files, and telling make that those would be intermediate target shouldn't be deleted to avoid the final object from been regenerated over and over again (I'm not sure why the rebuild of tiny source file happen when they are intermediate, maybe because FORCE prerequisite on %.o). Can't we commit this patch as is? What kind of issue is there with those tiny source files? Should we add a warning in those tiny source files, something like "No modification of this file allowed, it's part of the build system." ? Thanks,
Anthony PERARD writes ("Re: [XEN PATCH v7 05/51] x86/mm: avoid building multiple .o from a single .c file"):
> On Tue, Sep 07, 2021 at 08:14:14AM +0200, Jan Beulich wrote:
> > Hmm, when replying to 00/51 I didn't recall I gave an R-b for this one
> > already. I'd like to restrict it some: It should be taken to stand for
> > the technical correctness of the change. Nevertheless I'm not really
> > happy with the introduction of the various tiny source files. I've
> > meanwhile been wondering: Can't these be generated (in the build tree,
> > as soon as that's possible to be separate) rather than getting put in
> > the repo?
>
> Do we really need to generated those never to be change tiny source
> file? Do we really need to make the build system a lot more complicated?
I'm not an x86 maintainer, but my 2p anyway:
I think the handful of tiny source files is probably better than some
contraption in the build system. Much less risk of something funny
and confusing going on.
We could reduce the number of copies of the same text by making the
copies of guest_walk*.c in hap/ be symlinks to ../guest_walk*.c.
> Can't we commit this patch as is? What kind of issue is there with those
> tiny source files? Should we add a warning in those tiny source files,
> something like "No modification of this file allowed, it's part of the
> build system." ?
I don't think we need any such warning. No-one is going to take that
tiny file and try to edit it to put functionality in it, and if they
do it will be spotted on review.
Thanks,
Ian.
On 08.09.2021 13:14, Anthony PERARD wrote: > On Tue, Sep 07, 2021 at 08:14:14AM +0200, Jan Beulich wrote: >> On 24.08.2021 12:49, Anthony PERARD wrote: >>> This replace the use of a single .c file use for multiple .o file by >>> creating multiple .c file including the first one. >>> >>> There's quite a few issues with trying to build more than one object >>> file from a single source file: there's is a duplication of the make >>> rules to generate those targets; there is an additional ".file" symbol >>> added in order to differentiate between the object files; and the >>> tools/symbols have an heuristic to try to pick up the right ".file". >>> >>> This patch adds new .c source file which avoid the need to add a >>> second ".file" symbol and thus avoid the need to deal with those >>> issues. >>> >>> Also remove __OBJECT_FILE__ from $(CC) command line as it isn't used >>> anywhere anymore. And remove the macro "build-intermediate" since the >>> generic rules for single targets can be used. >>> >>> And rename the objects in mm/hap/ to remove the extra "level". >>> >>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> >> Hmm, when replying to 00/51 I didn't recall I gave an R-b for this one >> already. I'd like to restrict it some: It should be taken to stand for >> the technical correctness of the change. Nevertheless I'm not really >> happy with the introduction of the various tiny source files. I've >> meanwhile been wondering: Can't these be generated (in the build tree, >> as soon as that's possible to be separate) rather than getting put in >> the repo? > > Do we really need to generated those never to be change tiny source > file? Do we really need to make the build system a lot more complicated? > > With those tiny source file in a different directory to the main source > file, we need to add "-I" to CFLAGS. (source tree vs build tree.) > > I don't like preparation phase, I'd rather do just-in-time generation of > those tiny file (if we really have too...) as to let make organize > itself on when to do things. That mean, extra targets on how to generate > the files, and telling make that those would be intermediate target > shouldn't be deleted to avoid the final object from been regenerated > over and over again (I'm not sure why the rebuild of tiny source file > happen when they are intermediate, maybe because FORCE prerequisite on > %.o). > > Can't we commit this patch as is? I'll consider another time. > What kind of issue is there with those tiny source files? To me they're ugly and their presence is at least mildly confusing. And apparently I'm not the only one thinking that way, or else such tiny stubs would have been put there right when introducing these multiply built sources. > Should we add a warning in those tiny source files, > something like "No modification of this file allowed, it's part of the > build system." ? Not sure this would make much of a difference. Them getting touched later on is not a primary concern of mine. In fact once they're there, I don't see why they couldn't be extended. At least in shadow code there might be functions which could live there, reducing #ifdef-ary. Jan
On Wed, Sep 08, 2021 at 02:01:43PM +0200, Jan Beulich wrote: > On 08.09.2021 13:14, Anthony PERARD wrote: > > What kind of issue is there with those tiny source files? > > To me they're ugly and their presence is at least mildly confusing. > And apparently I'm not the only one thinking that way, or else such > tiny stubs would have been put there right when introducing these > multiply built sources. Well, at the time when this was introduced, tools/symbols didn't exist, so avoiding the stubs might have been ok by then, but it meant to duplicated %.o:%.c rules in the Makefile... Now, we have tools/symbols, a hack put into the source file to add a ".file instruction", a heuristic in tools/symbols to pick up the ".file" we want, and a workaround to change in behavior of binutils. This is a lot of complexity to avoid introducing those extra source files... complexity that were needed to by added _after_ the initial introduction of multiply built sources, due to changes in the build system. My patches have attempted to remove all the complexity, and having ugly small source files is the price to pay. And you want to add back some complexity in the build system just to avoid those tiny files? I mean hypervisor and build system are complex software to write, do we really need to add complexity at every opportunity?
On 08/09/2021 12:44, Ian Jackson wrote: > Anthony PERARD writes ("Re: [XEN PATCH v7 05/51] x86/mm: avoid building multiple .o from a single .c file"): >> On Tue, Sep 07, 2021 at 08:14:14AM +0200, Jan Beulich wrote: >>> Hmm, when replying to 00/51 I didn't recall I gave an R-b for this one >>> already. I'd like to restrict it some: It should be taken to stand for >>> the technical correctness of the change. Nevertheless I'm not really >>> happy with the introduction of the various tiny source files. I've >>> meanwhile been wondering: Can't these be generated (in the build tree, >>> as soon as that's possible to be separate) rather than getting put in >>> the repo? >> Do we really need to generated those never to be change tiny source >> file? Do we really need to make the build system a lot more complicated? > I'm not an x86 maintainer, but my 2p anyway: > > I think the handful of tiny source files is probably better than some > contraption in the build system. Much less risk of something funny > and confusing going on. I agree. This patch is definitely an improvement on the status quo. > We could reduce the number of copies of the same text by making the > copies of guest_walk*.c in hap/ be symlinks to ../guest_walk*.c. The two guest_walk's are totally different logic. Adding a symlink would be reintroducing "something funny and confusing". > >> Can't we commit this patch as is? What kind of issue is there with those >> tiny source files? Should we add a warning in those tiny source files, >> something like "No modification of this file allowed, it's part of the >> build system." ? > I don't think we need any such warning. No-one is going to take that > tiny file and try to edit it to put functionality in it, and if they > do it will be spotted on review. Agreed. FTR, this patch is Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> and fit to be committed.
diff --git a/xen/Makefile b/xen/Makefile index 4ceb02d37441..f35a4d84f7cd 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -482,17 +482,6 @@ _MAP: %/: FORCE $(MAKE) -f $(BASEDIR)/Rules.mk -C $* built_in.o built_in_bin.o -build-intermediate = $(eval $(call build-intermediate-closure,$(1))) -define build-intermediate-closure -$(1): FORCE - $(MAKE) -f $(BASEDIR)/Rules.mk -C $$(@D) $$(@F) -endef - -$(foreach base,arch/x86/mm/guest_walk_% \ - arch/x86/mm/hap/guest_walk_%level \ - arch/x86/mm/shadow/guest_%, \ - $(foreach ext,o i s,$(call build-intermediate,$(base).$(ext)))) - .PHONY: cloc cloc: $(eval tmpfile := $(shell mktemp)) diff --git a/xen/Rules.mk b/xen/Rules.mk index 3f61682ceab7..48ae519d0153 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -139,7 +139,7 @@ cpp_flags = $(filter-out -Wa$(comma)% -flto,$(1)) # Calculation of flags, first the generic flags, then the arch specific flags, # and last the flags modified for a target or a directory. -c_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_CFLAGS) '-D__OBJECT_FILE__="$@"' +c_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_CFLAGS) a_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_AFLAGS) include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk diff --git a/xen/arch/x86/mm/Makefile b/xen/arch/x86/mm/Makefile index 2818c066f76a..6b7882d992bb 100644 --- a/xen/arch/x86/mm/Makefile +++ b/xen/arch/x86/mm/Makefile @@ -10,12 +10,3 @@ obj-$(CONFIG_MEM_SHARING) += mem_sharing.o obj-y += p2m.o obj-$(CONFIG_HVM) += p2m-ept.o p2m-pod.o p2m-pt.o obj-y += paging.o - -guest_walk_%.o: guest_walk.c Makefile - $(CC) $(c_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ - -guest_walk_%.i: guest_walk.c Makefile - $(CPP) $(call cpp_flags,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ - -guest_walk_%.s: guest_walk.c Makefile - $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@ diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c index 30d83cf1e0e6..b9f607272c39 100644 --- a/xen/arch/x86/mm/guest_walk.c +++ b/xen/arch/x86/mm/guest_walk.c @@ -21,9 +21,6 @@ * along with this program; If not, see <http://www.gnu.org/licenses/>. */ -/* Allow uniquely identifying static symbols in the 3 generated objects. */ -asm(".file \"" __OBJECT_FILE__ "\""); - #include <xen/types.h> #include <xen/mm.h> #include <xen/paging.h> diff --git a/xen/arch/x86/mm/guest_walk_2.c b/xen/arch/x86/mm/guest_walk_2.c new file mode 100644 index 000000000000..defcd59bc260 --- /dev/null +++ b/xen/arch/x86/mm/guest_walk_2.c @@ -0,0 +1,2 @@ +#define GUEST_PAGING_LEVELS 2 +#include "guest_walk.c" diff --git a/xen/arch/x86/mm/guest_walk_3.c b/xen/arch/x86/mm/guest_walk_3.c new file mode 100644 index 000000000000..1c9eca37741e --- /dev/null +++ b/xen/arch/x86/mm/guest_walk_3.c @@ -0,0 +1,2 @@ +#define GUEST_PAGING_LEVELS 3 +#include "guest_walk.c" diff --git a/xen/arch/x86/mm/guest_walk_4.c b/xen/arch/x86/mm/guest_walk_4.c new file mode 100644 index 000000000000..aa3900338a2d --- /dev/null +++ b/xen/arch/x86/mm/guest_walk_4.c @@ -0,0 +1,2 @@ +#define GUEST_PAGING_LEVELS 4 +#include "guest_walk.c" diff --git a/xen/arch/x86/mm/hap/Makefile b/xen/arch/x86/mm/hap/Makefile index c6d296b51720..8ef54b1faa50 100644 --- a/xen/arch/x86/mm/hap/Makefile +++ b/xen/arch/x86/mm/hap/Makefile @@ -1,15 +1,6 @@ obj-y += hap.o -obj-y += guest_walk_2level.o -obj-y += guest_walk_3level.o -obj-y += guest_walk_4level.o +obj-y += guest_walk_2.o +obj-y += guest_walk_3.o +obj-y += guest_walk_4.o obj-y += nested_hap.o obj-y += nested_ept.o - -guest_walk_%level.o: guest_walk.c Makefile - $(CC) $(c_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ - -guest_walk_%level.i: guest_walk.c Makefile - $(CPP) $(call cpp_flags,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ - -guest_walk_%level.s: guest_walk.c Makefile - $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@ diff --git a/xen/arch/x86/mm/hap/guest_walk.c b/xen/arch/x86/mm/hap/guest_walk.c index f59ebc84a290..832a8058471e 100644 --- a/xen/arch/x86/mm/hap/guest_walk.c +++ b/xen/arch/x86/mm/hap/guest_walk.c @@ -18,9 +18,6 @@ * this program; If not, see <http://www.gnu.org/licenses/>. */ -/* Allow uniquely identifying static symbols in the 3 generated objects. */ -asm(".file \"" __OBJECT_FILE__ "\""); - #include <xen/domain_page.h> #include <xen/paging.h> #include <xen/sched.h> diff --git a/xen/arch/x86/mm/hap/guest_walk_2.c b/xen/arch/x86/mm/hap/guest_walk_2.c new file mode 100644 index 000000000000..defcd59bc260 --- /dev/null +++ b/xen/arch/x86/mm/hap/guest_walk_2.c @@ -0,0 +1,2 @@ +#define GUEST_PAGING_LEVELS 2 +#include "guest_walk.c" diff --git a/xen/arch/x86/mm/hap/guest_walk_3.c b/xen/arch/x86/mm/hap/guest_walk_3.c new file mode 100644 index 000000000000..1c9eca37741e --- /dev/null +++ b/xen/arch/x86/mm/hap/guest_walk_3.c @@ -0,0 +1,2 @@ +#define GUEST_PAGING_LEVELS 3 +#include "guest_walk.c" diff --git a/xen/arch/x86/mm/hap/guest_walk_4.c b/xen/arch/x86/mm/hap/guest_walk_4.c new file mode 100644 index 000000000000..aa3900338a2d --- /dev/null +++ b/xen/arch/x86/mm/hap/guest_walk_4.c @@ -0,0 +1,2 @@ +#define GUEST_PAGING_LEVELS 4 +#include "guest_walk.c" diff --git a/xen/arch/x86/mm/shadow/Makefile b/xen/arch/x86/mm/shadow/Makefile index fd64b4dda925..b4a1620b6920 100644 --- a/xen/arch/x86/mm/shadow/Makefile +++ b/xen/arch/x86/mm/shadow/Makefile @@ -5,12 +5,3 @@ obj-$(CONFIG_PV) += pv.o guest_4.o else obj-y += none.o endif - -guest_%.o: multi.c Makefile - $(CC) $(c_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ - -guest_%.i: multi.c Makefile - $(CPP) $(call cpp_flags,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ - -guest_%.s: multi.c Makefile - $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@ diff --git a/xen/arch/x86/mm/shadow/guest_2.c b/xen/arch/x86/mm/shadow/guest_2.c new file mode 100644 index 000000000000..288b229982b0 --- /dev/null +++ b/xen/arch/x86/mm/shadow/guest_2.c @@ -0,0 +1,2 @@ +#define GUEST_PAGING_LEVELS 2 +#include "multi.c" diff --git a/xen/arch/x86/mm/shadow/guest_3.c b/xen/arch/x86/mm/shadow/guest_3.c new file mode 100644 index 000000000000..04e17b0b8adc --- /dev/null +++ b/xen/arch/x86/mm/shadow/guest_3.c @@ -0,0 +1,2 @@ +#define GUEST_PAGING_LEVELS 3 +#include "multi.c" diff --git a/xen/arch/x86/mm/shadow/guest_4.c b/xen/arch/x86/mm/shadow/guest_4.c new file mode 100644 index 000000000000..c0c5d3cb11ad --- /dev/null +++ b/xen/arch/x86/mm/shadow/guest_4.c @@ -0,0 +1,2 @@ +#define GUEST_PAGING_LEVELS 4 +#include "multi.c" diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c index 8bb028c2e2fa..7207fcf9e75f 100644 --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -20,9 +20,6 @@ * along with this program; If not, see <http://www.gnu.org/licenses/>. */ -/* Allow uniquely identifying static symbols in the 3 generated objects. */ -asm(".file \"" __OBJECT_FILE__ "\""); - #include <xen/types.h> #include <xen/mm.h> #include <xen/trace.h> diff --git a/xen/tools/symbols.c b/xen/tools/symbols.c index 0b124526165d..710e9785d348 100644 --- a/xen/tools/symbols.c +++ b/xen/tools/symbols.c @@ -84,7 +84,6 @@ static int read_symbol(FILE *in, struct sym_entry *s) { char str[500], type[20] = ""; char *sym, stype; - static enum { symbol, single_source, multi_source } last; static char *filename; int rc = -1; @@ -118,24 +117,11 @@ static int read_symbol(FILE *in, struct sym_entry *s) */ input_format == fmt_sysv && !*type && stype == '?' && sym && sym[1] && strchr("cSsoh", sym[1]) && !sym[2])) { - /* - * gas prior to binutils commit fbdf9406b0 (expected to appear - * in 2.27) outputs symbol table entries resulting from .file - * in reverse order. If we get two consecutive file symbols, - * prefer the first one if that names an object file or has a - * directory component (to cover multiply compiled files). - */ - bool multi = strchr(str, '/') || (sym && sym[1] == 'o'); - - if (multi || last != multi_source) { - free(filename); - filename = *str ? strdup(str) : NULL; - } - last = multi ? multi_source : single_source; + free(filename); + filename = *str ? strdup(str) : NULL; goto skip_tail; } - last = symbol; rc = -1; sym = str;