| Message ID | 20210701141011.785641-7-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Build system improvements | expand |
On 01.07.2021 16:09, 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. While I have to admit that I'm not really happy about these extra new files, I can see that one might view this as the less bad of two evils. > xen/Makefile | 11 ----------- > xen/Rules.mk | 2 +- > xen/arch/x86/mm/Makefile | 9 --------- > xen/arch/x86/mm/guest_walk.c | 3 --- > xen/arch/x86/mm/guest_walk_2.c | 2 ++ > xen/arch/x86/mm/guest_walk_3.c | 2 ++ > xen/arch/x86/mm/guest_walk_4.c | 2 ++ > xen/arch/x86/mm/hap/Makefile | 9 --------- > xen/arch/x86/mm/hap/guest_walk.c | 3 --- > xen/arch/x86/mm/hap/guest_walk_2level.c | 2 ++ > xen/arch/x86/mm/hap/guest_walk_3level.c | 2 ++ > xen/arch/x86/mm/hap/guest_walk_4level.c | 2 ++ Is there a particular reason you've kept the "level" in these three file names? Preferably with them shortened (and ideally dashes used everywhere in the new file names instead of underscores) Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On Wed, Jul 07, 2021 at 04:45:11PM +0200, Jan Beulich wrote: > On 01.07.2021 16:09, Anthony PERARD wrote: > > xen/Makefile | 11 ----------- > > xen/Rules.mk | 2 +- > > xen/arch/x86/mm/Makefile | 9 --------- > > xen/arch/x86/mm/guest_walk.c | 3 --- > > xen/arch/x86/mm/guest_walk_2.c | 2 ++ > > xen/arch/x86/mm/guest_walk_3.c | 2 ++ > > xen/arch/x86/mm/guest_walk_4.c | 2 ++ > > xen/arch/x86/mm/hap/Makefile | 9 --------- > > xen/arch/x86/mm/hap/guest_walk.c | 3 --- > > xen/arch/x86/mm/hap/guest_walk_2level.c | 2 ++ > > xen/arch/x86/mm/hap/guest_walk_3level.c | 2 ++ > > xen/arch/x86/mm/hap/guest_walk_4level.c | 2 ++ > > Is there a particular reason you've kept the "level" in these three > file names? Preferably with them shortened (and ideally dashes used > everywhere in the new file names instead of underscores) I had no reason to rename them. (Also, renaming them makes the patch bigger ;-) as we would rename the objects files.) But I can rename as part of the patch. Should I rename all of them (mm hap and shadow) to "guest-walk-$level.c" ? Or just "guest-$level.c" like in shadow/ ? On the other hand, it would probably be helpful to have the same basename for both the source and the .c that includes the source (e.g. "guest_walk.c" and "guest_walk_2.c" have "guest_walk" in common). So if we were to replace underscores by dashes, we should probably rename "guest_walk.c" to "guest-walk.c" as well. So I'll remove the "level" from the filenames in hap/ to start with in an update to this patch, it it worth it to do more that that? > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks,
On 12.07.2021 14:49, Anthony PERARD wrote: > On Wed, Jul 07, 2021 at 04:45:11PM +0200, Jan Beulich wrote: >> On 01.07.2021 16:09, Anthony PERARD wrote: >>> xen/Makefile | 11 ----------- >>> xen/Rules.mk | 2 +- >>> xen/arch/x86/mm/Makefile | 9 --------- >>> xen/arch/x86/mm/guest_walk.c | 3 --- >>> xen/arch/x86/mm/guest_walk_2.c | 2 ++ >>> xen/arch/x86/mm/guest_walk_3.c | 2 ++ >>> xen/arch/x86/mm/guest_walk_4.c | 2 ++ >>> xen/arch/x86/mm/hap/Makefile | 9 --------- >>> xen/arch/x86/mm/hap/guest_walk.c | 3 --- >>> xen/arch/x86/mm/hap/guest_walk_2level.c | 2 ++ >>> xen/arch/x86/mm/hap/guest_walk_3level.c | 2 ++ >>> xen/arch/x86/mm/hap/guest_walk_4level.c | 2 ++ >> >> Is there a particular reason you've kept the "level" in these three >> file names? Preferably with them shortened (and ideally dashes used >> everywhere in the new file names instead of underscores) > > I had no reason to rename them. (Also, renaming them makes the patch > bigger ;-) as we would rename the objects files.) > > But I can rename as part of the patch. Should I rename all of them (mm > hap and shadow) to "guest-walk-$level.c" ? Or just "guest-$level.c" like > in shadow/ ? On the other hand, it would probably be helpful to have the same > basename for both the source and the .c that includes the source (e.g. > "guest_walk.c" and "guest_walk_2.c" have "guest_walk" in common). So if > we were to replace underscores by dashes, we should probably rename > "guest_walk.c" to "guest-walk.c" as well. > > So I'll remove the "level" from the filenames in hap/ to start with in > an update to this patch, it it worth it to do more that that? I'd be okay with just the removal of "level"; switching to dashes would be nice imo, but I can see this being controversial - as you say, names would go slightly out of sync, and whether renaming the base files is warranted is hard to say. I'd be personally in favor, but I can see why people may prefer not to have those extra renames in here. Jan
diff --git a/xen/Makefile b/xen/Makefile index 89879fad4cb2..360b4a1d1867 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -476,17 +476,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 f05b2d3f0399..ede408efc515 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -139,7 +139,7 @@ cpp_flags = $(filter-out -Wa$(comma)%,$(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..32aef9b4ba26 100644 --- a/xen/arch/x86/mm/hap/Makefile +++ b/xen/arch/x86/mm/hap/Makefile @@ -4,12 +4,3 @@ obj-y += guest_walk_3level.o obj-y += guest_walk_4level.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_2level.c b/xen/arch/x86/mm/hap/guest_walk_2level.c new file mode 100644 index 000000000000..defcd59bc260 --- /dev/null +++ b/xen/arch/x86/mm/hap/guest_walk_2level.c @@ -0,0 +1,2 @@ +#define GUEST_PAGING_LEVELS 2 +#include "guest_walk.c" diff --git a/xen/arch/x86/mm/hap/guest_walk_3level.c b/xen/arch/x86/mm/hap/guest_walk_3level.c new file mode 100644 index 000000000000..1c9eca37741e --- /dev/null +++ b/xen/arch/x86/mm/hap/guest_walk_3level.c @@ -0,0 +1,2 @@ +#define GUEST_PAGING_LEVELS 3 +#include "guest_walk.c" diff --git a/xen/arch/x86/mm/hap/guest_walk_4level.c b/xen/arch/x86/mm/hap/guest_walk_4level.c new file mode 100644 index 000000000000..aa3900338a2d --- /dev/null +++ b/xen/arch/x86/mm/hap/guest_walk_4level.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;
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. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Notes: v6: - new patch to replace both from v5: xen,symbols: rework file symbols selection build: use if_changed to build mm/*/guest_%.o The changes in the patch for symbols.c was too complicated to explain, and I probably didn't realize one important fact about the tool: it deal with all *.o been concatenated to each other, without a way to figure out which symbol belong to which original file, and certainly no way to figure out if there's more than one ".file" symbol to choose from beside some fragile heuristic. xen/Makefile | 11 ----------- xen/Rules.mk | 2 +- xen/arch/x86/mm/Makefile | 9 --------- xen/arch/x86/mm/guest_walk.c | 3 --- xen/arch/x86/mm/guest_walk_2.c | 2 ++ xen/arch/x86/mm/guest_walk_3.c | 2 ++ xen/arch/x86/mm/guest_walk_4.c | 2 ++ xen/arch/x86/mm/hap/Makefile | 9 --------- xen/arch/x86/mm/hap/guest_walk.c | 3 --- xen/arch/x86/mm/hap/guest_walk_2level.c | 2 ++ xen/arch/x86/mm/hap/guest_walk_3level.c | 2 ++ xen/arch/x86/mm/hap/guest_walk_4level.c | 2 ++ xen/arch/x86/mm/shadow/Makefile | 9 --------- xen/arch/x86/mm/shadow/guest_2.c | 2 ++ xen/arch/x86/mm/shadow/guest_3.c | 2 ++ xen/arch/x86/mm/shadow/guest_4.c | 2 ++ xen/arch/x86/mm/shadow/multi.c | 3 --- xen/tools/symbols.c | 18 ++---------------- 18 files changed, 21 insertions(+), 64 deletions(-) create mode 100644 xen/arch/x86/mm/guest_walk_2.c create mode 100644 xen/arch/x86/mm/guest_walk_3.c create mode 100644 xen/arch/x86/mm/guest_walk_4.c create mode 100644 xen/arch/x86/mm/hap/guest_walk_2level.c create mode 100644 xen/arch/x86/mm/hap/guest_walk_3level.c create mode 100644 xen/arch/x86/mm/hap/guest_walk_4level.c create mode 100644 xen/arch/x86/mm/shadow/guest_2.c create mode 100644 xen/arch/x86/mm/shadow/guest_3.c create mode 100644 xen/arch/x86/mm/shadow/guest_4.c