The current method to include 32 bit C boot code is:
- compile each function we want to use into a separate object file;
- each function is compiled with -fpic option;
- convert these object files to binary files. This operation removes GOP
which we don't want in the executable;
- a small assembly part in each file add the entry point;
- code can't have external references, all possible variables are passed
by value or pointer;
- include these binary files in head.S.
There are currently some limitations:
- code is compiled separately, it's not possible to share a function
(like memcpy) between different functions to use;
- although code is compiled with -fpic there's no certainty there are
no relocations, specifically data ones. This can lead into hard to
find bugs;
- it's hard to add a simple function;
- having to pass external variables makes hard to do multiple things
otherwise functions would require a lot of parameters so code would
have to be split into multiple functions which is not easy.
Current change extends the current process:
- all object files are linked together before getting converted making
possible to share code between the function we want to call;
- a single object file is generated with all functions to use and
exported symbols to easily call;
- variables to use are declared in linker script and easily used inside
C code. Declaring them manually could be annoying but makes also
easier to check them. Using external pointers can be still an issue if
they are not fixed. If an external symbol is not declared this gives a
link error.
Some details of the implementation:
- C code is compiled with -fpic flags (as before);
- object files from C code are linked together;
- the single bundled object file is linked with 2 slightly different
script files to generate 2 bundled object files;
- the 2 bundled object files are converted to binary removing the need
for global offset tables;
- a Python script is used to generate assembly source from the 2
binaries;
- the single assembly file is compiled to generate final bundled object
file;
- to detect possible unwanted relocation in data/code code is generated
with different addresses. This is enforced starting .text section at
different positions and adding a fixed "gap" at the beginning.
This makes sure code and data is position independent;
- to detect used symbols in data/code symbols are placed in .text
section at different offsets (based on the line in the linker script).
This is needed as potentially a reference to a symbol is converted to
a reference to the containing section so multiple symbols could be
converted to reference to same symbol (section name) and we need to
distinguish them;
- --orphan-handling=error option to linker is used to make sure we
account for all possible sections from C code;
Current limitations:
- the main one is the lack of support for 64 bit code. It would make
sure that even the code used for 64 bit (at the moment EFI code) is
code and data position independent. We cannot assume that code that
came from code compiled for 32 bit and compiled for 64 bit is code and
data position independent, different compiler options lead to
different code/data.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
Changes since v2:
- removed W^X limitation, allowing data;
- added some comments to python script;
- added extension to python script;
- added header to generated assembly code from python script;
- added starting symbol to generated assembly code from python script
to make disassembly more clear;
- other minor style changes to python script.
---
xen/arch/x86/boot/.gitignore | 5 +-
xen/arch/x86/boot/Makefile | 38 +++-
.../x86/boot/{build32.lds => build32.lds.S} | 35 ++-
xen/arch/x86/boot/cmdline.c | 12 -
xen/arch/x86/boot/head.S | 12 -
xen/arch/x86/boot/reloc.c | 14 --
xen/tools/combine_two_binaries.py | 207 ++++++++++++++++++
7 files changed, 268 insertions(+), 55 deletions(-)
rename xen/arch/x86/boot/{build32.lds => build32.lds.S} (70%)
create mode 100755 xen/tools/combine_two_binaries.py
diff --git a/xen/arch/x86/boot/.gitignore b/xen/arch/x86/boot/.gitignore
index a379db7988..ebad650e5c 100644
--- a/xen/arch/x86/boot/.gitignore
+++ b/xen/arch/x86/boot/.gitignore
@@ -1,3 +1,4 @@
/mkelf32
-/*.bin
-/*.lnk
+/build32.*.lds
+/built_in_32.*.bin
+/built_in_32.*.map
diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 1199291d2b..23ad274c89 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -1,4 +1,5 @@
obj-bin-y += head.o
+obj-bin-y += built_in_32.o
obj32 := cmdline.32.o
obj32 += reloc.32.o
@@ -9,9 +10,6 @@ targets += $(obj32)
obj32 := $(addprefix $(obj)/,$(obj32))
-$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
-$(obj)/head.o: $(obj32:.32.o=.bin)
-
CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
$(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -mregparm=3
@@ -25,14 +23,34 @@ $(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
$(obj)/%.32.o: $(src)/%.c FORCE
$(call if_changed_rule,cc_o_c)
+orphan-handling-$(call ld-option,--orphan-handling=error) := --orphan-handling=error
LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))
-%.bin: %.lnk
- $(OBJCOPY) -j .text -O binary $< $@
-
-%.lnk: %.32.o $(src)/build32.lds
- $(LD32) -N -T $(filter %.lds,$^) -o $@ $<
-
-clean-files := *.lnk *.bin
+$(obj)/build32.final.lds: AFLAGS-y += -DFINAL
+$(obj)/build32.other.lds $(obj)/build32.final.lds: $(src)/build32.lds.S FORCE
+ $(call if_changed_dep,cpp_lds_S)
+
+# link all 32bit objects together
+$(obj)/built_in_32.tmp.o: $(obj32)
+ $(LD32) -r -o $@ $^
+
+$(obj)/built_in_32.%.bin: $(obj)/build32.%.lds $(obj)/built_in_32.tmp.o
+## link bundle with a given layout
+ $(LD32) $(orphan-handling-y) -N -T $< -Map $(obj)/built_in_32.$(*F).map -o $(obj)/built_in_32.$(*F).o $(obj)/built_in_32.tmp.o
+## extract binaries from object
+ $(OBJCOPY) -j .text -O binary $(obj)/built_in_32.$(*F).o $@
+ rm -f $(obj)/built_in_32.$(*F).o
+
+# generate final object file combining and checking above binaries
+$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin
+ $(PYTHON) $(srctree)/tools/combine_two_binaries.py \
+ --script $(obj)/build32.final.lds \
+ --bin1 $(obj)/built_in_32.other.bin \
+ --bin2 $(obj)/built_in_32.final.bin \
+ --map $(obj)/built_in_32.final.map \
+ --exports cmdline_parse_early,reloc \
+ --output $@
+
+clean-files := built_in_32.*.bin built_in_32.*.map build32.*.lds
diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds.S
similarity index 70%
rename from xen/arch/x86/boot/build32.lds
rename to xen/arch/x86/boot/build32.lds.S
index 56edaa727b..50c167aef0 100644
--- a/xen/arch/x86/boot/build32.lds
+++ b/xen/arch/x86/boot/build32.lds.S
@@ -15,22 +15,47 @@
* with this program. If not, see <http://www.gnu.org/licenses/>.
*/
-ENTRY(_start)
+#ifdef FINAL
+# define GAP 0
+# define MULT 0
+# define TEXT_START
+#else
+# define GAP 0x010200
+# define MULT 1
+# define TEXT_START 0x408020
+#endif
+# define DECLARE_IMPORT(name) name = . + (__LINE__ * MULT)
+
+ENTRY(dummy_start)
SECTIONS
{
/* Merge code and data into one section. */
- .text : {
+ .text TEXT_START : {
+ /* Silence linker warning, we are not going to use it */
+ dummy_start = .;
+
+ /* Declare below any symbol name needed.
+ * Each symbol should be on its own line.
+ * It looks like a tedious work but we make sure the things we use.
+ * Potentially they should be all variables. */
+ DECLARE_IMPORT(__base_relocs_start);
+ DECLARE_IMPORT(__base_relocs_end);
+ . = . + GAP;
*(.text)
*(.text.*)
- *(.data)
- *(.data.*)
*(.rodata)
*(.rodata.*)
+ *(.data)
+ *(.data.*)
*(.bss)
*(.bss.*)
}
-
+ /DISCARD/ : {
+ *(.comment)
+ *(.comment.*)
+ *(.note.*)
+ }
/* Dynamic linkage sections. Collected simply so we can check they're empty. */
.got : {
*(.got)
diff --git a/xen/arch/x86/boot/cmdline.c b/xen/arch/x86/boot/cmdline.c
index fc9241ede9..196c580e91 100644
--- a/xen/arch/x86/boot/cmdline.c
+++ b/xen/arch/x86/boot/cmdline.c
@@ -18,18 +18,6 @@
* Linux kernel source (linux/lib/string.c).
*/
-/*
- * This entry point is entered from xen/arch/x86/boot/head.S with:
- * - %eax = &cmdline,
- * - %edx = &early_boot_opts.
- */
-asm (
- " .text \n"
- " .globl _start \n"
- "_start: \n"
- " jmp cmdline_parse_early \n"
- );
-
#include <xen/compiler.h>
#include <xen/kconfig.h>
#include <xen/macros.h>
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index c4de1dfab5..e0776e3896 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -759,18 +759,6 @@ trampoline_setup:
/* Jump into the relocated trampoline. */
lret
- /*
- * cmdline and reloc are written in C, and linked to be 32bit PIC with
- * entrypoints at 0 and using the fastcall convention.
- */
-FUNC_LOCAL(cmdline_parse_early)
- .incbin "cmdline.bin"
-END(cmdline_parse_early)
-
-FUNC_LOCAL(reloc)
- .incbin "reloc.bin"
-END(reloc)
-
ENTRY(trampoline_start)
#include "trampoline.S"
ENTRY(trampoline_end)
diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
index 8c58affcd9..94b078d7b1 100644
--- a/xen/arch/x86/boot/reloc.c
+++ b/xen/arch/x86/boot/reloc.c
@@ -12,20 +12,6 @@
* Daniel Kiper <daniel.kiper@oracle.com>
*/
-/*
- * This entry point is entered from xen/arch/x86/boot/head.S with:
- * - %eax = MAGIC,
- * - %edx = INFORMATION_ADDRESS,
- * - %ecx = TOPMOST_LOW_MEMORY_STACK_ADDRESS.
- * - 0x04(%esp) = BOOT_VIDEO_INFO_ADDRESS.
- */
-asm (
- " .text \n"
- " .globl _start \n"
- "_start: \n"
- " jmp reloc \n"
- );
-
#include <xen/compiler.h>
#include <xen/macros.h>
#include <xen/types.h>
diff --git a/xen/tools/combine_two_binaries.py b/xen/tools/combine_two_binaries.py
new file mode 100755
index 0000000000..138c59287e
--- /dev/null
+++ b/xen/tools/combine_two_binaries.py
@@ -0,0 +1,207 @@
+#!/usr/bin/env python3
+
+from __future__ import print_function
+import argparse
+import re
+import struct
+import sys
+
+parser = argparse.ArgumentParser(description='Generate assembly file to merge into other code.')
+parser.add_argument('--script', dest='script',
+ required=True,
+ help='Linker script for extracting symbols')
+parser.add_argument('--bin1', dest='bin1',
+ required=True,
+ help='First binary')
+parser.add_argument('--bin2', dest='bin2',
+ required=True,
+ help='Second binary')
+parser.add_argument('--output', dest='output',
+ help='Output file')
+parser.add_argument('--map', dest='mapfile',
+ help='Map file to read for symbols to export')
+parser.add_argument('--exports', dest='exports',
+ help='Symbols to export')
+parser.add_argument('--section-header', dest='section_header',
+ default='.section .init.text, "ax", @progbits',
+ help='Section header declaration')
+parser.add_argument('-v', '--verbose',
+ action='store_true')
+args = parser.parse_args()
+
+gap = 0x010200
+text_diff = 0x408020
+
+# Parse linker script for external symbols to use.
+# Next regex matches expanded DECLARE_IMPORT lines in linker script.
+symbol_re = re.compile(r'\s+(\S+)\s*=\s*\.\s*\+\s*\((\d+)\s*\*\s*0\s*\)\s*;')
+symbols = {}
+lines = {}
+for line in open(args.script):
+ m = symbol_re.match(line)
+ if not m:
+ continue
+ (name, line_num) = (m.group(1), int(m.group(2)))
+ if line_num == 0:
+ raise Exception("Invalid line number found:\n\t" + line)
+ if line_num in symbols:
+ raise Exception("Symbol with this line already present:\n\t" + line)
+ if name in lines:
+ raise Exception("Symbol with this name already present:\n\t" + name)
+ symbols[line_num] = name
+ lines[name] = line_num
+
+exports = []
+if args.exports is not None:
+ exports = dict([(name, None) for name in args.exports.split(',')])
+
+# Parse mapfile, look for ther symbols we want to export.
+if args.mapfile is not None:
+ symbol_re = re.compile(r'\s{15,}0x([0-9a-f]+)\s+(\S+)\n')
+ for line in open(args.mapfile):
+ m = symbol_re.match(line)
+ if not m or m.group(2) not in exports:
+ continue
+ addr = int(m.group(1), 16)
+ exports[m.group(2)] = addr
+for (name, addr) in exports.items():
+ if addr is None:
+ raise Exception("Required export symbols %s not found" % name)
+
+file1 = open(args.bin1, 'rb')
+file2 = open(args.bin2, 'rb')
+file1.seek(0, 2)
+size1 = file1.tell()
+file2.seek(0, 2)
+size2 = file2.tell()
+if size1 > size2:
+ file1, file2 = file2, file1
+ size1, size2 = size2, size1
+if size2 != size1 + gap:
+ raise Exception('File sizes do not match')
+
+file1.seek(0, 0)
+data1 = file1.read(size1)
+file2.seek(gap, 0)
+data2 = file2.read(size1)
+
+max_line = max(symbols.keys())
+
+def to_int32(n):
+ '''Convert a number to signed 32 bit integer truncating if needed'''
+ mask = (1 << 32) - 1
+ h = 1 << 31
+ return (n & mask) ^ h - h
+
+i = 0
+references = {}
+internals = 0
+while i <= size1 - 4:
+ n1 = struct.unpack('<I', data1[i:i+4])[0]
+ n2 = struct.unpack('<I', data2[i:i+4])[0]
+ i += 1
+ # The two numbers are the same, no problems
+ if n1 == n2:
+ continue
+ # Try to understand why they are different
+ diff = to_int32(n1 - n2)
+ if diff == -gap: # this is an internal relocation
+ pos = i - 1
+ print("Internal relocation found at position %#x "
+ "n1=%#x n2=%#x diff=%#x" % (pos, n1, n2, diff),
+ file=sys.stderr)
+ i += 3
+ internals += 1
+ if internals >= 10:
+ break
+ continue
+ # This is a relative relocation to a symbol, accepted, code/data is
+ # relocatable.
+ if diff < gap and diff >= gap - max_line:
+ n = gap - diff
+ symbol = symbols.get(n)
+ # check we have a symbol
+ if symbol is None:
+ raise Exception("Cannot find symbol for line %d" % n)
+ pos = i - 1
+ if args.verbose:
+ print('Position %#x %d %s' % (pos, n, symbol), file=sys.stderr)
+ i += 3
+ references[pos] = symbol
+ continue
+ # First byte is the same, move to next byte
+ if diff & 0xff == 0 and i <= size1 - 4:
+ continue
+ # Probably a type of relocation we don't want or support
+ pos = i - 1
+ suggestion = ''
+ symbol = symbols.get(-diff - text_diff)
+ if symbol is not None:
+ suggestion = " Maybe %s is not defined as hidden?" % symbol
+ raise Exception("Unexpected difference found at %#x "
+ "n1=%#x n2=%#x diff=%#x gap=%#x.%s" % \
+ (pos, n1, n2, diff, gap, suggestion))
+if internals != 0:
+ raise Exception("Previous relocations found")
+
+def line_bytes(buf, out):
+ '''Output an assembly line with all bytes in "buf"'''
+ # Python 2 compatibility
+ if type(buf) == str:
+ print("\t.byte " + ','.join([str(ord(c)) for c in buf]), file=out)
+ else:
+ print("\t.byte " + ','.join([str(n) for n in buf]), file=out)
+
+def part(start, end, out):
+ '''Output bytes of "data" from "start" to "end"'''
+ while start < end:
+ e = min(start + 16, end)
+ line_bytes(data1[start:e], out)
+ start = e
+
+def reference(pos, out):
+ name = references[pos]
+ n = struct.unpack('<I', data1[pos:pos+4])[0]
+ sign = '+'
+ if n >= (1 << 31):
+ n -= (1 << 32)
+ n += pos
+ if n < 0:
+ n = -n
+ sign = '-'
+ print("\t.hidden %s\n"
+ "\t.long %s %s %#x - ." % (name, name, sign, n),
+ file=out)
+
+def output(out):
+ prev = 0
+ exports_by_addr = {}
+ for (sym, addr) in exports.items():
+ exports_by_addr.setdefault(addr, []).append(sym)
+ positions = list(references.keys())
+ positions += list(exports_by_addr.keys())
+ for pos in sorted(positions):
+ part(prev, pos, out)
+ prev = pos
+ if pos in references:
+ reference(pos, out)
+ prev = pos + 4
+ if pos in exports_by_addr:
+ for sym in exports_by_addr[pos]:
+ print("\t.global %s\n"
+ "\t.hidden %s\n"
+ "%s:" % (sym, sym, sym),
+ file=out)
+ part(prev, size1, out)
+
+out = sys.stdout
+if args.output is not None:
+ out = open(args.output, 'w')
+print('''/*
+ * File autogenerated by combine_two_binaries.py DO NOT EDIT
+ */''', file=out)
+print('\t' + args.section_header, file=out)
+print('obj_start:', file=out)
+output(out)
+print('\n\t.section\t.note.GNU-stack,"",@progbits', file=out)
+out.flush()
--
2.34.1On 14/10/2024 9:53 am, Frediano Ziglio wrote:
> diff --git a/xen/tools/combine_two_binaries.py b/xen/tools/combine_two_binaries.py
> new file mode 100755
> index 0000000000..138c59287e
> --- /dev/null
> +++ b/xen/tools/combine_two_binaries.py
> @@ -0,0 +1,207 @@
>
> +if args.output is not None:
> + out = open(args.output, 'w')
> +print('''/*
> + * File autogenerated by combine_two_binaries.py DO NOT EDIT
> + */''', file=out)
> +print('\t' + args.section_header, file=out)
> +print('obj_start:', file=out)
obj32_start:
This symbol survives into the main Xen binary, where "obj_start" is
entirely devoid of context.
~Andrew
On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> index 1199291d2b..23ad274c89 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,4 +1,5 @@
> obj-bin-y += head.o
> +obj-bin-y += built_in_32.o
I don't quite like that this new object name is "built_in_32.o",
It's really closed to "built_in.*" which is used by Rules.mk to collect
all objects in a subdirectory. I don't really have a better suggestion at
the moment.
> @@ -9,9 +10,6 @@ targets += $(obj32)
>
> obj32 := $(addprefix $(obj)/,$(obj32))
>
> -$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
> -$(obj)/head.o: $(obj32:.32.o=.bin)
> -
> CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
> $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
> CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -mregparm=3
> @@ -25,14 +23,34 @@ $(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
> $(obj)/%.32.o: $(src)/%.c FORCE
> $(call if_changed_rule,cc_o_c)
>
> +orphan-handling-$(call ld-option,--orphan-handling=error) := --orphan-handling=error
> LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
> LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
> LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))
>
> -%.bin: %.lnk
> - $(OBJCOPY) -j .text -O binary $< $@
> -
> -%.lnk: %.32.o $(src)/build32.lds
> - $(LD32) -N -T $(filter %.lds,$^) -o $@ $<
> -
> -clean-files := *.lnk *.bin
> +$(obj)/build32.final.lds: AFLAGS-y += -DFINAL
> +$(obj)/build32.other.lds $(obj)/build32.final.lds: $(src)/build32.lds.S FORCE
> + $(call if_changed_dep,cpp_lds_S)
> +
> +# link all 32bit objects together
> +$(obj)/built_in_32.tmp.o: $(obj32)
> + $(LD32) -r -o $@ $^
> +
> +$(obj)/built_in_32.%.bin: $(obj)/build32.%.lds $(obj)/built_in_32.tmp.o
> +## link bundle with a given layout
Could you put the comment aligned with the rest of the recipe? That way
it won't visually separate the rule into several pieces. You can
prefix it with @ so it doesn't show in build logs:
@# comments
> + $(LD32) $(orphan-handling-y) -N -T $< -Map $(obj)/built_in_32.$(*F).map -o $(obj)/built_in_32.$(*F).o $(obj)/built_in_32.tmp.o
I think this wants to be: -T $(filter %.lds,$^) -Map $(patsubst %.bin,%.map,$@) -o $(patsubst %.bin,%.o,$@) $(filter %.o,$^)
:-(, maybe that's lots of $(patsubst,), not sure which is better between
$(patsubst,) and using the stem $*.
Also, if something tries to use built_in_32.tmp.bin, we have a rule that
remove it's prerequisite.
BTW, everything is kind of temporary in a build system, beside the few
files that we want to install on a machine, so having a target named
with "*tmp*" isn't great. But having a rule that create "*tmp*" file but
remove them before the end of its recipe is fine (or those *tmp* aren't
use outside of this recipe).
> +## extract binaries from object
> + $(OBJCOPY) -j .text -O binary $(obj)/built_in_32.$(*F).o $@
> + rm -f $(obj)/built_in_32.$(*F).o
> +
> +# generate final object file combining and checking above binaries
> +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin
So, "other" isn't part of "final", I don't really know what those two
things contains so naming wise I can't suggest anything useful.
But, is "built_in_32.S" really only depends on those two .bin? SHouldn't
it get regenerated if the script changes, or the .lds that --script
option seems to use? What is the "--map" option, an input or output?
But I guess we can ignore the ".map" file because it's part of the
".bin".
Another thing that might be useful to do, is to use the "if_changed"
make macro, that way if the command line of the script changes, make
can remake the output. But maybe it's a bit complicated for this recipe
because it doesn't uses $< or $^.
> + $(PYTHON) $(srctree)/tools/combine_two_binaries.py \
> + --script $(obj)/build32.final.lds \
> + --bin1 $(obj)/built_in_32.other.bin \
> + --bin2 $(obj)/built_in_32.final.bin \
> + --map $(obj)/built_in_32.final.map \
> + --exports cmdline_parse_early,reloc \
> + --output $@
> +
> +clean-files := built_in_32.*.bin built_in_32.*.map build32.*.lds
> diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds.S
> similarity index 70%
> rename from xen/arch/x86/boot/build32.lds
> rename to xen/arch/x86/boot/build32.lds.S
> index 56edaa727b..50c167aef0 100644
> --- a/xen/arch/x86/boot/build32.lds
> +++ b/xen/arch/x86/boot/build32.lds.S
> @@ -15,22 +15,47 @@
> * with this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> -ENTRY(_start)
> +#ifdef FINAL
> +# define GAP 0
> +# define MULT 0
> +# define TEXT_START
> +#else
> +# define GAP 0x010200
> +# define MULT 1
> +# define TEXT_START 0x408020
> +#endif
> +# define DECLARE_IMPORT(name) name = . + (__LINE__ * MULT)
Is ^ this a stray space?
Overall, it's kind of mostly style comment that I had, so feel free to
ignore most of them.
Cheers,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD
<anthony.perard@vates.tech> wrote:
>
> On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
> > diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> > index 1199291d2b..23ad274c89 100644
> > --- a/xen/arch/x86/boot/Makefile
> > +++ b/xen/arch/x86/boot/Makefile
> > @@ -1,4 +1,5 @@
> > obj-bin-y += head.o
> > +obj-bin-y += built_in_32.o
>
> I don't quite like that this new object name is "built_in_32.o",
> It's really closed to "built_in.*" which is used by Rules.mk to collect
> all objects in a subdirectory. I don't really have a better suggestion at
> the moment.
>
It was cbundle.o before, but people preferred built_in_32.o.
It's a collection of object files like built_in.o so it does not seem
so bad to me.
But seen other replies, some other people prefer "bundle".
> > @@ -9,9 +10,6 @@ targets += $(obj32)
> >
> > obj32 := $(addprefix $(obj)/,$(obj32))
> >
> > -$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
> > -$(obj)/head.o: $(obj32:.32.o=.bin)
> > -
> > CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
> > $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
> > CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -mregparm=3
> > @@ -25,14 +23,34 @@ $(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
> > $(obj)/%.32.o: $(src)/%.c FORCE
> > $(call if_changed_rule,cc_o_c)
> >
> > +orphan-handling-$(call ld-option,--orphan-handling=error) := --orphan-handling=error
> > LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
> > LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
> > LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))
> >
> > -%.bin: %.lnk
> > - $(OBJCOPY) -j .text -O binary $< $@
> > -
> > -%.lnk: %.32.o $(src)/build32.lds
> > - $(LD32) -N -T $(filter %.lds,$^) -o $@ $<
> > -
> > -clean-files := *.lnk *.bin
> > +$(obj)/build32.final.lds: AFLAGS-y += -DFINAL
> > +$(obj)/build32.other.lds $(obj)/build32.final.lds: $(src)/build32.lds.S FORCE
> > + $(call if_changed_dep,cpp_lds_S)
> > +
> > +# link all 32bit objects together
> > +$(obj)/built_in_32.tmp.o: $(obj32)
> > + $(LD32) -r -o $@ $^
> > +
> > +$(obj)/built_in_32.%.bin: $(obj)/build32.%.lds $(obj)/built_in_32.tmp.o
> > +## link bundle with a given layout
>
> Could you put the comment aligned with the rest of the recipe? That way
> it won't visually separate the rule into several pieces. You can
> prefix it with @ so it doesn't show in build logs:
>
> @# comments
>
Yes, they look more or less the same but technically pretty different.
The "## XXX" is a comment for make command, the "\t@#comment" is a way
to tell make to not print the command before launching it so make will
launch a shell command "# comment".
Yes, indentation does not make it clear. Not that launching a shell to
execute a comment will take a long time. On the other hand, here that
comment is more for the rule than for the shell. Maybe something like
target:
command \
# do something
(personally I found it even more ugly)
> > + $(LD32) $(orphan-handling-y) -N -T $< -Map $(obj)/built_in_32.$(*F).map -o $(obj)/built_in_32.$(*F).o $(obj)/built_in_32.tmp.o
>
> I think this wants to be: -T $(filter %.lds,$^) -Map $(patsubst %.bin,%.map,$@) -o $(patsubst %.bin,%.o,$@) $(filter %.o,$^)
>
> :-(, maybe that's lots of $(patsubst,), not sure which is better between
> $(patsubst,) and using the stem $*.
>
Not strong about stem or patsubst.
The 2 filters seem good, they suggest lds for the script and objects
for the input, which makes sense.
> Also, if something tries to use built_in_32.tmp.bin, we have a rule that
> remove it's prerequisite.
>
> BTW, everything is kind of temporary in a build system, beside the few
> files that we want to install on a machine, so having a target named
> with "*tmp*" isn't great. But having a rule that create "*tmp*" file but
> remove them before the end of its recipe is fine (or those *tmp* aren't
> use outside of this recipe).
>
Mumble... yes, I think the XX.tmp.o was a temporary internal rule file.
So we still don't agree on one name, and now we want to find also
another, tricky.
More or less if it can help, one is a 32 bit object file that bundle
together multiple 32 bits object files while the other is the
conversion of the 32 bits bundle file to 64 bits.
XXX_32.o and XXX_32as64.o ??
> > +## extract binaries from object
> > + $(OBJCOPY) -j .text -O binary $(obj)/built_in_32.$(*F).o $@
> > + rm -f $(obj)/built_in_32.$(*F).o
> > +
> > +# generate final object file combining and checking above binaries
> > +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin
>
> So, "other" isn't part of "final", I don't really know what those two
> things contains so naming wise I can't suggest anything useful.
>
> But, is "built_in_32.S" really only depends on those two .bin? SHouldn't
> it get regenerated if the script changes, or the .lds that --script
> option seems to use? What is the "--map" option, an input or output?
Both map file and linker script are dependency of the bin files so no problems.
Yes, potentially you want to generate if the python script change.
Potentially also if Makefile, make, ld or any other command change,
but that possibly is kind of "we don't usually care".
> But I guess we can ignore the ".map" file because it's part of the
> ".bin".
>
Yes, I suppose they are, although we can make it explicit both
generating them and using as dependencies.
> Another thing that might be useful to do, is to use the "if_changed"
> make macro, that way if the command line of the script changes, make
> can remake the output. But maybe it's a bit complicated for this recipe
> because it doesn't uses $< or $^.
>
I usually simply add Makefile to the dependencies. It works too.
> > + $(PYTHON) $(srctree)/tools/combine_two_binaries.py \
> > + --script $(obj)/build32.final.lds \
> > + --bin1 $(obj)/built_in_32.other.bin \
> > + --bin2 $(obj)/built_in_32.final.bin \
> > + --map $(obj)/built_in_32.final.map \
> > + --exports cmdline_parse_early,reloc \
> > + --output $@
> > +
> > +clean-files := built_in_32.*.bin built_in_32.*.map build32.*.lds
> > diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds.S
> > similarity index 70%
> > rename from xen/arch/x86/boot/build32.lds
> > rename to xen/arch/x86/boot/build32.lds.S
> > index 56edaa727b..50c167aef0 100644
> > --- a/xen/arch/x86/boot/build32.lds
> > +++ b/xen/arch/x86/boot/build32.lds.S
> > @@ -15,22 +15,47 @@
> > * with this program. If not, see <http://www.gnu.org/licenses/>.
> > */
> >
> > -ENTRY(_start)
> > +#ifdef FINAL
> > +# define GAP 0
> > +# define MULT 0
> > +# define TEXT_START
> > +#else
> > +# define GAP 0x010200
> > +# define MULT 1
> > +# define TEXT_START 0x408020
> > +#endif
> > +# define DECLARE_IMPORT(name) name = . + (__LINE__ * MULT)
>
> Is ^ this a stray space?
>
Yes, a single ASCII character 32. Surely we don't want tabs. Other
parts of the file use 2 spaces indentation, others 8 spaces. Or are
you suggesting to not indent them?
>
> Overall, it's kind of mostly style comment that I had, so feel free to
> ignore most of them.
>
Naming can be cumbersome but usually helps readability.
> Cheers,
>
> --
>
> Anthony Perard | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech
>
Frediano
On Mon, Oct 14, 2024 at 05:32:26PM +0100, Frediano Ziglio wrote:
> On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD
> <anthony.perard@vates.tech> wrote:
> >
> > On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote:
> > > diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> > > index 1199291d2b..23ad274c89 100644
> > > --- a/xen/arch/x86/boot/Makefile
> > > +++ b/xen/arch/x86/boot/Makefile
> > > @@ -1,4 +1,5 @@
> > > obj-bin-y += head.o
> > > +obj-bin-y += built_in_32.o
> >
> > I don't quite like that this new object name is "built_in_32.o",
> > It's really closed to "built_in.*" which is used by Rules.mk to collect
> > all objects in a subdirectory. I don't really have a better suggestion at
> > the moment.
> >
>
> It was cbundle.o before, but people preferred built_in_32.o.
> It's a collection of object files like built_in.o so it does not seem
> so bad to me.
> But seen other replies, some other people prefer "bundle".
Yeah, I guess it's ok (built_in_32 that is; now that I gave a better
review of the rest of the Makefile changes).
> > > @@ -9,9 +10,6 @@ targets += $(obj32)
> > >
> > > obj32 := $(addprefix $(obj)/,$(obj32))
> > >
> > > -$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj)
> > > -$(obj)/head.o: $(obj32:.32.o=.bin)
> > > -
> > > CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS))
> > > $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS))
> > > CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -mregparm=3
> > > @@ -25,14 +23,34 @@ $(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
> > > $(obj)/%.32.o: $(src)/%.c FORCE
> > > $(call if_changed_rule,cc_o_c)
> > >
> > > +orphan-handling-$(call ld-option,--orphan-handling=error) := --orphan-handling=error
> > > LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments
> > > LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y)
> > > LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))
> > >
> > > -%.bin: %.lnk
> > > - $(OBJCOPY) -j .text -O binary $< $@
> > > -
> > > -%.lnk: %.32.o $(src)/build32.lds
> > > - $(LD32) -N -T $(filter %.lds,$^) -o $@ $<
> > > -
> > > -clean-files := *.lnk *.bin
> > > +$(obj)/build32.final.lds: AFLAGS-y += -DFINAL
> > > +$(obj)/build32.other.lds $(obj)/build32.final.lds: $(src)/build32.lds.S FORCE
> > > + $(call if_changed_dep,cpp_lds_S)
Could you add:
targets += build32.final.lds build32.other.lds
otherwise, the if_changes macro doesn't work, and the target keeps been
rebuilt. `make V=2` will tell you the same thing.
> > > +# link all 32bit objects together
> > > +$(obj)/built_in_32.tmp.o: $(obj32)
> > > + $(LD32) -r -o $@ $^
> > > +
> > > +$(obj)/built_in_32.%.bin: $(obj)/build32.%.lds $(obj)/built_in_32.tmp.o
> > > +## link bundle with a given layout
> >
> > Could you put the comment aligned with the rest of the recipe? That way
> > it won't visually separate the rule into several pieces. You can
> > prefix it with @ so it doesn't show in build logs:
> >
> > @# comments
> >
>
> Yes, they look more or less the same but technically pretty different.
> The "## XXX" is a comment for make command, the "\t@#comment" is a way
> to tell make to not print the command before launching it so make will
> launch a shell command "# comment".
> Yes, indentation does not make it clear. Not that launching a shell to
> execute a comment will take a long time. On the other hand, here that
> comment is more for the rule than for the shell. Maybe something like
>
> target:
> command \
> # do something
>
> (personally I found it even more ugly)
How about removing the comments? I mean the command line is
self-explanatory here. Same thing about the objcopy comment.
(Or a comment before the rule, when really needed)
> > > + $(LD32) $(orphan-handling-y) -N -T $< -Map $(obj)/built_in_32.$(*F).map -o $(obj)/built_in_32.$(*F).o $(obj)/built_in_32.tmp.o
> >
> > I think this wants to be: -T $(filter %.lds,$^) -Map $(patsubst %.bin,%.map,$@) -o $(patsubst %.bin,%.o,$@) $(filter %.o,$^)
> >
> > :-(, maybe that's lots of $(patsubst,), not sure which is better between
> > $(patsubst,) and using the stem $*.
> >
>
> Not strong about stem or patsubst.
Actually, I found a better suggestion (after reading the previous path
which remind me about an existing feature that I don't really use), the
use of Substitution References
(https://www.gnu.org/software/make/manual/make.html#Substitution-Refs)
This is a shorter version than patsubst, but works very well here, the
recipe can look like this:
$(LD32) $(orphan-handling-y) -N -T $< -Map $(@:bin=map) -o $(@:bin=o) $(filter %.o,$^)
$(OBJCOPY) -j .text -O binary $(@:bin=o) $@
rm -f $(@:bin=o)
(It is likely fine to keep the reference to the lds script as $<,
instead of using $(filter,).)
BTW, do we need the rules that generate the input of this rules
(built_in_32.tmp.o that is), or can this one takes all 32bit objects as
input?
> The 2 filters seem good, they suggest lds for the script and objects
> for the input, which makes sense.
> > Also, if something tries to use built_in_32.tmp.bin, we have a rule that
> > remove it's prerequisite.
> >
> > BTW, everything is kind of temporary in a build system, beside the few
> > files that we want to install on a machine, so having a target named
> > with "*tmp*" isn't great. But having a rule that create "*tmp*" file but
> > remove them before the end of its recipe is fine (or those *tmp* aren't
> > use outside of this recipe).
> >
>
> Mumble... yes, I think the XX.tmp.o was a temporary internal rule file.
> So we still don't agree on one name, and now we want to find also
> another, tricky.
> More or less if it can help, one is a 32 bit object file that bundle
> together multiple 32 bits object files while the other is the
> conversion of the 32 bits bundle file to 64 bits.
> XXX_32.o and XXX_32as64.o ??
We may not need the rule for it :-).
> > > +## extract binaries from object
> > > + $(OBJCOPY) -j .text -O binary $(obj)/built_in_32.$(*F).o $@
> > > + rm -f $(obj)/built_in_32.$(*F).o
> > > +
> > > +# generate final object file combining and checking above binaries
> > > +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin
> >
> > So, "other" isn't part of "final", I don't really know what those two
> > things contains so naming wise I can't suggest anything useful.
Instead of "other", is "control" (like in science experiment where you
have a control group), or "offseted" (which seems to be how this second
binary is constructed) would be better names for this *.bin? It seems
the script take both input and play the game of the 7 differences, to
find clues about the location of some symbols, right?.
> > But, is "built_in_32.S" really only depends on those two .bin? SHouldn't
> > it get regenerated if the script changes, or the .lds that --script
> > option seems to use? What is the "--map" option, an input or output?
>
> Both map file and linker script are dependency of the bin files so no problems.
> Yes, potentially you want to generate if the python script change.
> Potentially also if Makefile, make, ld or any other command change,
> but that possibly is kind of "we don't usually care".
Actually, for the hypervisor, we usually care about changes which could
lead to a different output. While changes of build toolchain aren't
really track (even if there's a patch that have a small improvement
toward it that was never committed), many other changes are been tracked
and act upon. The macros $(if_changes*,) from Linux's Kbuild are a great
help.
If the build system doesn't regenerate targets when a developer try to
debug some complex problem, it isn't going to help. We can easily have a
situation where the developer adds printf earlier and earlier and
earlier to try to find out where Xen crash, to find out later that the
build system was working against them by not rebuilding the file that
should be rebuilt. So if we can avoid some frustration and time lost,
it's better :-).
> > But I guess we can ignore the ".map" file because it's part of the
> > ".bin".
> >
>
> Yes, I suppose they are, although we can make it explicit both
> generating them and using as dependencies.
Having a rule that generate several targets is complicated to write in
GNU Make 3.80, the rule is more likely to be called several time in
parallel. So let's ignored the .map file, as it should only be used
along side the .bin.
> > Another thing that might be useful to do, is to use the "if_changed"
> > make macro, that way if the command line of the script changes, make
> > can remake the output. But maybe it's a bit complicated for this recipe
> > because it doesn't uses $< or $^.
> >
>
> I usually simply add Makefile to the dependencies. It works too.
Having "Makefile" as prerequisite is both too much and not enough. This
lead to rebuild due to unrelated changes in the Makefile and if the
command line change (like more command line option modified in a variable
in a different Makefile or the environment) the target doesn't gets
rebuilt.
That's where $(if_changed,) helps.
> > > + $(PYTHON) $(srctree)/tools/combine_two_binaries.py \
> > > + --script $(obj)/build32.final.lds \
> > > + --bin1 $(obj)/built_in_32.other.bin \
> > > + --bin2 $(obj)/built_in_32.final.bin \
> > > + --map $(obj)/built_in_32.final.map \
> > > + --exports cmdline_parse_early,reloc \
> > > + --output $@
I can think of one example where $(if_changed,) is going to really help,
by looking at this command line:
One does update the .c file to add a function that they like to
export, run `make`, realize they forgot to update the makefile so
update it, run `make`, it's still doesn't work...
Maybe run `make clean; make`, or something else...
So, could you use $(if_changed,) ?
Probably:
quiet_cmd_combine = GEN $@
cmd_combine = $(PYTHON) ...
$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin FORCE
$(call if_changes,combine)
targets += built_in_32.S
GEN, for generate, or it could be PY instead, because python script can
be slow to compile which could explain why the build system output is
making a pause on this target (on slow machines that is). Or it could be
COMBINE, or something else, but it's not really necessary to explain,
the target name is often enough to figure out what's happening, when
needed.
> > > +
> > > +clean-files := built_in_32.*.bin built_in_32.*.map build32.*.lds
> > > diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds.S
> > > similarity index 70%
> > > rename from xen/arch/x86/boot/build32.lds
> > > rename to xen/arch/x86/boot/build32.lds.S
> > > index 56edaa727b..50c167aef0 100644
> > > --- a/xen/arch/x86/boot/build32.lds
> > > +++ b/xen/arch/x86/boot/build32.lds.S
> > > @@ -15,22 +15,47 @@
> > > * with this program. If not, see <http://www.gnu.org/licenses/>.
> > > */
> > >
> > > -ENTRY(_start)
> > > +#ifdef FINAL
> > > +# define GAP 0
> > > +# define MULT 0
> > > +# define TEXT_START
> > > +#else
> > > +# define GAP 0x010200
> > > +# define MULT 1
> > > +# define TEXT_START 0x408020
So I've read the python script a bit, and notice that the two values GAP
and TEXT_START are also written verbatim in it, without comment, so
looks like magic number there. Could you at least put a comment in the
python script? (Something thing that could be better were if the
Makefile itself provided those number to both the lds script and the
python script, but I don't know if it's possible.)
> > > +#endif
> > > +# define DECLARE_IMPORT(name) name = . + (__LINE__ * MULT)
> >
> > Is ^ this a stray space?
> >
>
> Yes, a single ASCII character 32. Surely we don't want tabs. Other
> parts of the file use 2 spaces indentation, others 8 spaces. Or are
> you suggesting to not indent them?
Yes, that what "stray" imply, also I mean only the last line as imply by
the use singular. Sorry, I could have try to be a bit more precise.
All the indentation within the #ifdef #endif are fine, but why is
#define still indented after the last #endif?
> >
> > Overall, it's kind of mostly style comment that I had, so feel free to
> > ignore most of them.
> >
>
> Naming can be cumbersome but usually helps readability.
Yes, also sorry for the half-baked previous review, it could have been better.
Cheers,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
On Tue, Oct 15, 2024 at 2:51 PM Anthony PERARD <anthony.perard@vates.tech> wrote: > > On Mon, Oct 14, 2024 at 05:32:26PM +0100, Frediano Ziglio wrote: > > On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD > > <anthony.perard@vates.tech> wrote: > > > > > > On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote: > > > > diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile > > > > index 1199291d2b..23ad274c89 100644 > > > > --- a/xen/arch/x86/boot/Makefile > > > > +++ b/xen/arch/x86/boot/Makefile > > > > @@ -1,4 +1,5 @@ > > > > obj-bin-y += head.o > > > > +obj-bin-y += built_in_32.o > > > > > > I don't quite like that this new object name is "built_in_32.o", > > > It's really closed to "built_in.*" which is used by Rules.mk to collect > > > all objects in a subdirectory. I don't really have a better suggestion at > > > the moment. > > > > > > > It was cbundle.o before, but people preferred built_in_32.o. > > It's a collection of object files like built_in.o so it does not seem > > so bad to me. > > But seen other replies, some other people prefer "bundle". > > Yeah, I guess it's ok (built_in_32 that is; now that I gave a better > review of the rest of the Makefile changes). > Renamed to built-in-32 for CODING_STYLE > > > > @@ -9,9 +10,6 @@ targets += $(obj32) > > > > > > > > obj32 := $(addprefix $(obj)/,$(obj32)) > > > > > > > > -$(obj)/head.o: AFLAGS-y += -Wa$(comma)-I$(obj) > > > > -$(obj)/head.o: $(obj32:.32.o=.bin) > > > > - > > > > CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS)) > > > > $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS)) > > > > CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float -mregparm=3 > > > > @@ -25,14 +23,34 @@ $(obj32): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic > > > > $(obj)/%.32.o: $(src)/%.c FORCE > > > > $(call if_changed_rule,cc_o_c) > > > > > > > > +orphan-handling-$(call ld-option,--orphan-handling=error) := --orphan-handling=error > > > > LDFLAGS_DIRECT-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments > > > > LDFLAGS_DIRECT += $(LDFLAGS_DIRECT-y) > > > > LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT)) > > > > > > > > -%.bin: %.lnk > > > > - $(OBJCOPY) -j .text -O binary $< $@ > > > > - > > > > -%.lnk: %.32.o $(src)/build32.lds > > > > - $(LD32) -N -T $(filter %.lds,$^) -o $@ $< > > > > - > > > > -clean-files := *.lnk *.bin > > > > +$(obj)/build32.final.lds: AFLAGS-y += -DFINAL > > > > +$(obj)/build32.other.lds $(obj)/build32.final.lds: $(src)/build32.lds.S FORCE > > > > + $(call if_changed_dep,cpp_lds_S) > > Could you add: > > targets += build32.final.lds build32.other.lds > Added > otherwise, the if_changes macro doesn't work, and the target keeps been > rebuilt. `make V=2` will tell you the same thing. > > > > > +# link all 32bit objects together > > > > +$(obj)/built_in_32.tmp.o: $(obj32) > > > > + $(LD32) -r -o $@ $^ > > > > + > > > > +$(obj)/built_in_32.%.bin: $(obj)/build32.%.lds $(obj)/built_in_32.tmp.o > > > > +## link bundle with a given layout > > > > > > Could you put the comment aligned with the rest of the recipe? That way > > > it won't visually separate the rule into several pieces. You can > > > prefix it with @ so it doesn't show in build logs: > > > > > > @# comments > > > > > > > Yes, they look more or less the same but technically pretty different. > > The "## XXX" is a comment for make command, the "\t@#comment" is a way > > to tell make to not print the command before launching it so make will > > launch a shell command "# comment". > > Yes, indentation does not make it clear. Not that launching a shell to > > execute a comment will take a long time. On the other hand, here that > > comment is more for the rule than for the shell. Maybe something like > > > > target: > > command \ > > # do something > > > > (personally I found it even more ugly) > > How about removing the comments? I mean the command line is > self-explanatory here. Same thing about the objcopy comment. > (Or a comment before the rule, when really needed) > Moved on top of the rule (slightly changed). > > > > + $(LD32) $(orphan-handling-y) -N -T $< -Map $(obj)/built_in_32.$(*F).map -o $(obj)/built_in_32.$(*F).o $(obj)/built_in_32.tmp.o > > > > > > I think this wants to be: -T $(filter %.lds,$^) -Map $(patsubst %.bin,%.map,$@) -o $(patsubst %.bin,%.o,$@) $(filter %.o,$^) > > > > > > :-(, maybe that's lots of $(patsubst,), not sure which is better between > > > $(patsubst,) and using the stem $*. > > > > > > > Not strong about stem or patsubst. > > Actually, I found a better suggestion (after reading the previous path > which remind me about an existing feature that I don't really use), the > use of Substitution References > (https://www.gnu.org/software/make/manual/make.html#Substitution-Refs) > > This is a shorter version than patsubst, but works very well here, the > recipe can look like this: > $(LD32) $(orphan-handling-y) -N -T $< -Map $(@:bin=map) -o $(@:bin=o) $(filter %.o,$^) > $(OBJCOPY) -j .text -O binary $(@:bin=o) $@ > rm -f $(@:bin=o) > > (It is likely fine to keep the reference to the lds script as $<, > instead of using $(filter,).) > Done > BTW, do we need the rules that generate the input of this rules > (built_in_32.tmp.o that is), or can this one takes all 32bit objects as > input? > Better not to do it In some conditions it can generate slightly different results (like different object alignments) making the algorithm fail. > > The 2 filters seem good, they suggest lds for the script and objects > > for the input, which makes sense. > > > > Also, if something tries to use built_in_32.tmp.bin, we have a rule that > > > remove it's prerequisite. > > > > > > BTW, everything is kind of temporary in a build system, beside the few > > > files that we want to install on a machine, so having a target named > > > with "*tmp*" isn't great. But having a rule that create "*tmp*" file but > > > remove them before the end of its recipe is fine (or those *tmp* aren't > > > use outside of this recipe). > > > > > > > Mumble... yes, I think the XX.tmp.o was a temporary internal rule file. > > So we still don't agree on one name, and now we want to find also > > another, tricky. > > More or less if it can help, one is a 32 bit object file that bundle > > together multiple 32 bits object files while the other is the > > conversion of the 32 bits bundle file to 64 bits. > > XXX_32.o and XXX_32as64.o ?? > > We may not need the rule for it :-). > Not sure if we are asking a change and how. > > > > +## extract binaries from object > > > > + $(OBJCOPY) -j .text -O binary $(obj)/built_in_32.$(*F).o $@ > > > > + rm -f $(obj)/built_in_32.$(*F).o > > > > + > > > > +# generate final object file combining and checking above binaries > > > > +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin > > > > > > So, "other" isn't part of "final", I don't really know what those two > > > things contains so naming wise I can't suggest anything useful. > > Instead of "other", is "control" (like in science experiment where you > have a control group), or "offseted" (which seems to be how this second > binary is constructed) would be better names for this *.bin? It seems > the script take both input and play the game of the 7 differences, to > find clues about the location of some symbols, right?. > I don't know the game and I think people not familiar with it won't find new names more readable but less. Not saying that current names are good, they just need to be located at different addresses with some "magic" in the middle. > > > But, is "built_in_32.S" really only depends on those two .bin? SHouldn't > > > it get regenerated if the script changes, or the .lds that --script > > > option seems to use? What is the "--map" option, an input or output? > > > > Both map file and linker script are dependency of the bin files so no problems. > > Yes, potentially you want to generate if the python script change. > > Potentially also if Makefile, make, ld or any other command change, > > but that possibly is kind of "we don't usually care". > > Actually, for the hypervisor, we usually care about changes which could > lead to a different output. While changes of build toolchain aren't > really track (even if there's a patch that have a small improvement > toward it that was never committed), many other changes are been tracked > and act upon. The macros $(if_changes*,) from Linux's Kbuild are a great > help. > > If the build system doesn't regenerate targets when a developer try to > debug some complex problem, it isn't going to help. We can easily have a > situation where the developer adds printf earlier and earlier and > earlier to try to find out where Xen crash, to find out later that the > build system was working against them by not rebuilding the file that > should be rebuilt. So if we can avoid some frustration and time lost, > it's better :-). > > > > But I guess we can ignore the ".map" file because it's part of the > > > ".bin". > > > > > > > Yes, I suppose they are, although we can make it explicit both > > generating them and using as dependencies. > > Having a rule that generate several targets is complicated to write in > GNU Make 3.80, the rule is more likely to be called several time in > parallel. So let's ignored the .map file, as it should only be used > along side the .bin. > > > > Another thing that might be useful to do, is to use the "if_changed" > > > make macro, that way if the command line of the script changes, make > > > can remake the output. But maybe it's a bit complicated for this recipe > > > because it doesn't uses $< or $^. > > > > > > > I usually simply add Makefile to the dependencies. It works too. > > Having "Makefile" as prerequisite is both too much and not enough. This > lead to rebuild due to unrelated changes in the Makefile and if the > command line change (like more command line option modified in a variable > in a different Makefile or the environment) the target doesn't gets > rebuilt. > > That's where $(if_changed,) helps. > > > > > + $(PYTHON) $(srctree)/tools/combine_two_binaries.py \ > > > > + --script $(obj)/build32.final.lds \ > > > > + --bin1 $(obj)/built_in_32.other.bin \ > > > > + --bin2 $(obj)/built_in_32.final.bin \ > > > > + --map $(obj)/built_in_32.final.map \ > > > > + --exports cmdline_parse_early,reloc \ > > > > + --output $@ > > I can think of one example where $(if_changed,) is going to really help, > by looking at this command line: > One does update the .c file to add a function that they like to > export, run `make`, realize they forgot to update the makefile so > update it, run `make`, it's still doesn't work... > Maybe run `make clean; make`, or something else... > > So, could you use $(if_changed,) ? > Probably: > quiet_cmd_combine = GEN $@ > cmd_combine = $(PYTHON) ... > $(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin FORCE > $(call if_changes,combine) > targets += built_in_32.S > > GEN, for generate, or it could be PY instead, because python script can > be slow to compile which could explain why the build system output is > making a pause on this target (on slow machines that is). Or it could be > COMBINE, or something else, but it's not really necessary to explain, > the target name is often enough to figure out what's happening, when > needed. > It just looks more complicated to me. It happened to me, if you don't use the exported symbol no reason to have it exported from the object, if you use it the link will fail and it won't take long to fix it. > > > > + > > > > +clean-files := built_in_32.*.bin built_in_32.*.map build32.*.lds > > > > diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds.S > > > > similarity index 70% > > > > rename from xen/arch/x86/boot/build32.lds > > > > rename to xen/arch/x86/boot/build32.lds.S > > > > index 56edaa727b..50c167aef0 100644 > > > > --- a/xen/arch/x86/boot/build32.lds > > > > +++ b/xen/arch/x86/boot/build32.lds.S > > > > @@ -15,22 +15,47 @@ > > > > * with this program. If not, see <http://www.gnu.org/licenses/>. > > > > */ > > > > > > > > -ENTRY(_start) > > > > +#ifdef FINAL > > > > +# define GAP 0 > > > > +# define MULT 0 > > > > +# define TEXT_START > > > > +#else > > > > +# define GAP 0x010200 > > > > +# define MULT 1 > > > > +# define TEXT_START 0x408020 > > So I've read the python script a bit, and notice that the two values GAP > and TEXT_START are also written verbatim in it, without comment, so > looks like magic number there. Could you at least put a comment in the > python script? (Something thing that could be better were if the > Makefile itself provided those number to both the lds script and the > python script, but I don't know if it's possible.) > It can be done I suppose: - 2 macros in Makefile - pass values into linker scripts using variables - 2 options for Python script - pass values to Python script > > > > +#endif > > > > +# define DECLARE_IMPORT(name) name = . + (__LINE__ * MULT) > > > > > > Is ^ this a stray space? > > > > > > > Yes, a single ASCII character 32. Surely we don't want tabs. Other > > parts of the file use 2 spaces indentation, others 8 spaces. Or are > > you suggesting to not indent them? > > Yes, that what "stray" imply, also I mean only the last line as imply by > the use singular. Sorry, I could have try to be a bit more precise. > > All the indentation within the #ifdef #endif are fine, but why is > #define still indented after the last #endif? > Oh.. that single space. Historic, copy&paste, there were 2 defines in the past one for each block. Removed. > > > > > > Overall, it's kind of mostly style comment that I had, so feel free to > > > ignore most of them. > > > > > > > Naming can be cumbersome but usually helps readability. > > Yes, also sorry for the half-baked previous review, it could have been better. > > Cheers, > > -- > > Anthony Perard | Vates XCP-ng Developer > > XCP-ng & Xen Orchestra - Vates solutions > > web: https://vates.tech > Frediano
On Wed, Oct 16, 2024 at 09:33:32AM +0100, Frediano Ziglio wrote: > On Tue, Oct 15, 2024 at 2:51 PM Anthony PERARD <anthony.perard@vates.tech> wrote: > > On Mon, Oct 14, 2024 at 05:32:26PM +0100, Frediano Ziglio wrote: > > > On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD <anthony.perard@vates.tech> wrote: > > > > On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote: > > BTW, do we need the rules that generate the input of this rules > > (built_in_32.tmp.o that is), or can this one takes all 32bit objects as > > input? > > > > Better not to do it In some conditions it can generate slightly > different results (like different object alignments) making the > algorithm fail. Ok. Thanks for the explanation. > > > > > +# generate final object file combining and checking above binaries > > > > > +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin > > > > > > > > So, "other" isn't part of "final", I don't really know what those two > > > > things contains so naming wise I can't suggest anything useful. > > > > Instead of "other", is "control" (like in science experiment where you > > have a control group), or "offseted" (which seems to be how this second > > binary is constructed) would be better names for this *.bin? It seems > > the script take both input and play the game of the 7 differences, to > > find clues about the location of some symbols, right?. > > > > I don't know the game and I think people not familiar with it won't > find new names more readable but less. Sorry, the "game" as nothing to do with the name I've proposed. I was just asking if the script take both *.bin and was looking for differences. (The game of 7 differences is simple: there's two similar pictures and you just look for the 7 differences between them, that's it.) > Not saying that current names are good, they just need to be located > at different addresses with some "magic" in the middle. Well to me "other" evoke a binary that contains functions that are not in "final", but instead they both contain the sames functions with slight variation of placement in the file (with added offset, gap), as I understand. But if you don't like my proposal, so be it. > > I can think of one example where $(if_changed,) is going to really help, > > by looking at this command line: > > One does update the .c file to add a function that they like to > > export, run `make`, realize they forgot to update the makefile so > > update it, run `make`, it's still doesn't work... > > Maybe run `make clean; make`, or something else... > > > > So, could you use $(if_changed,) ? > > Probably: > > quiet_cmd_combine = GEN $@ > > cmd_combine = $(PYTHON) ... > > $(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin FORCE > > $(call if_changes,combine) > > targets += built_in_32.S > > > > GEN, for generate, or it could be PY instead, because python script can > > be slow to compile which could explain why the build system output is > > making a pause on this target (on slow machines that is). Or it could be > > COMBINE, or something else, but it's not really necessary to explain, > > the target name is often enough to figure out what's happening, when > > needed. > > > > It just looks more complicated to me. I'm sorry if writing makefile is complicated. GNU make doesn't help with writing build system that work well, especially when doing incremental builds. So we need to use more complicated construction, especially for a complex project like Xen. Cheers, -- Anthony Perard | Vates XCP-ng Developer XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
On Wed, Oct 16, 2024 at 12:25 PM Anthony PERARD <anthony.perard@vates.tech> wrote: > > On Wed, Oct 16, 2024 at 09:33:32AM +0100, Frediano Ziglio wrote: > > On Tue, Oct 15, 2024 at 2:51 PM Anthony PERARD <anthony.perard@vates.tech> wrote: > > > On Mon, Oct 14, 2024 at 05:32:26PM +0100, Frediano Ziglio wrote: > > > > On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD <anthony.perard@vates.tech> wrote: > > > > > On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote: > > > > BTW, do we need the rules that generate the input of this rules > > > (built_in_32.tmp.o that is), or can this one takes all 32bit objects as > > > input? > > > > > > > Better not to do it In some conditions it can generate slightly > > different results (like different object alignments) making the > > algorithm fail. > > Ok. Thanks for the explanation. > > > > > > > +# generate final object file combining and checking above binaries > > > > > > +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin > > > > > > > > > > So, "other" isn't part of "final", I don't really know what those two > > > > > things contains so naming wise I can't suggest anything useful. > > > > > > Instead of "other", is "control" (like in science experiment where you > > > have a control group), or "offseted" (which seems to be how this second > > > binary is constructed) would be better names for this *.bin? It seems > > > the script take both input and play the game of the 7 differences, to > > > find clues about the location of some symbols, right?. > > > > > > > I don't know the game and I think people not familiar with it won't > > find new names more readable but less. > > Sorry, the "game" as nothing to do with the name I've proposed. I was > just asking if the script take both *.bin and was looking for > differences. > > (The game of 7 differences is simple: there's two similar pictures and > you just look for the 7 differences between them, that's it.) > > > Not saying that current names are good, they just need to be located > > at different addresses with some "magic" in the middle. > > Well to me "other" evoke a binary that contains functions that are not > in "final", but instead they both contain the sames functions with > slight variation of placement in the file (with added offset, gap), as I > understand. But if you don't like my proposal, so be it. > What about "base" and "offsetted" ? I don't know why "offsetted" sounds weird to me but I didn't find anything better. I hope some native English speaker could come with a better proposal. > > > I can think of one example where $(if_changed,) is going to really help, > > > by looking at this command line: > > > One does update the .c file to add a function that they like to > > > export, run `make`, realize they forgot to update the makefile so > > > update it, run `make`, it's still doesn't work... > > > Maybe run `make clean; make`, or something else... > > > > > > So, could you use $(if_changed,) ? > > > Probably: > > > quiet_cmd_combine = GEN $@ > > > cmd_combine = $(PYTHON) ... > > > $(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin FORCE > > > $(call if_changes,combine) > > > targets += built_in_32.S > > > > > > GEN, for generate, or it could be PY instead, because python script can > > > be slow to compile which could explain why the build system output is > > > making a pause on this target (on slow machines that is). Or it could be > > > COMBINE, or something else, but it's not really necessary to explain, > > > the target name is often enough to figure out what's happening, when > > > needed. > > > > > > > It just looks more complicated to me. > > I'm sorry if writing makefile is complicated. GNU make doesn't help with > writing build system that work well, especially when doing incremental > builds. So we need to use more complicated construction, especially for > a complex project like Xen. > It was more a balance consideration. Considering the cases that seem to solve (and your case did not much apply) I don't feel that worth. Also, dependency to Makefile would solve without additional macros and indirection. Do you mind posting a full working change? Frediano
On Wed, Oct 16, 2024 at 04:05:00PM +0100, Frediano Ziglio wrote:
> On Wed, Oct 16, 2024 at 12:25 PM Anthony PERARD > <anthony.perard@vates.tech> wrote:
> > On Wed, Oct 16, 2024 at 09:33:32AM +0100, Frediano Ziglio wrote:
> > > On Tue, Oct 15, 2024 at 2:51 PM Anthony PERARD <anthony.perard@vates.tech> wrote:
> > > > I can think of one example where $(if_changed,) is going to really help,
> > > > by looking at this command line:
> > > > One does update the .c file to add a function that they like to
> > > > export, run `make`, realize they forgot to update the makefile so
> > > > update it, run `make`, it's still doesn't work...
> > > > Maybe run `make clean; make`, or something else...
> > > >
> > > > So, could you use $(if_changed,) ?
> > > > Probably:
> > > > quiet_cmd_combine = GEN $@
> > > > cmd_combine = $(PYTHON) ...
> > > > $(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin FORCE
> > > > $(call if_changes,combine)
> > > > targets += built_in_32.S
> > > >
> > > > GEN, for generate, or it could be PY instead, because python script can
> > > > be slow to compile which could explain why the build system output is
> > > > making a pause on this target (on slow machines that is). Or it could be
> > > > COMBINE, or something else, but it's not really necessary to explain,
> > > > the target name is often enough to figure out what's happening, when
> > > > needed.
> > > >
> > >
> > > It just looks more complicated to me.
> >
> > I'm sorry if writing makefile is complicated. GNU make doesn't help with
> > writing build system that work well, especially when doing incremental
> > builds. So we need to use more complicated construction, especially for
> > a complex project like Xen.
> >
>
> It was more a balance consideration. Considering the cases that seem
> to solve (and your case did not much apply) I don't feel that worth.
> Also, dependency to Makefile would solve without additional macros and
> indirection. Do you mind posting a full working change?
Sure, here it is (I notice I've misspell the macro name in what I've written
earlier):
quiet_cmd_combine = GEN $@
cmd_combine = \
$(PYTHON) $(srctree)/tools/combine_two_binaries.py \
--script $(obj)/build32.final.lds \
--bin1 $(obj)/built_in_32.other.bin \
--bin2 $(obj)/built_in_32.final.bin \
--map $(obj)/built_in_32.final.map \
--exports cmdline_parse_early,reloc \
--output $@
targets += built_in_32.S
$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin \
$(srctree)/tools/combine_two_binaries.py FORCE
$(call if_changed,combine)
Cheers,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
On Wed, Oct 16, 2024 at 4:05 PM Frediano Ziglio <frediano.ziglio@cloud.com> wrote: > > On Wed, Oct 16, 2024 at 12:25 PM Anthony PERARD > <anthony.perard@vates.tech> wrote: > > > > On Wed, Oct 16, 2024 at 09:33:32AM +0100, Frediano Ziglio wrote: > > > On Tue, Oct 15, 2024 at 2:51 PM Anthony PERARD <anthony.perard@vates.tech> wrote: > > > > On Mon, Oct 14, 2024 at 05:32:26PM +0100, Frediano Ziglio wrote: > > > > > On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD <anthony.perard@vates.tech> wrote: > > > > > > On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote: > > > > > > BTW, do we need the rules that generate the input of this rules > > > > (built_in_32.tmp.o that is), or can this one takes all 32bit objects as > > > > input? > > > > > > > > > > Better not to do it In some conditions it can generate slightly > > > different results (like different object alignments) making the > > > algorithm fail. > > > > Ok. Thanks for the explanation. > > > > > > > > > +# generate final object file combining and checking above binaries > > > > > > > +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin > > > > > > > > > > > > So, "other" isn't part of "final", I don't really know what those two > > > > > > things contains so naming wise I can't suggest anything useful. > > > > > > > > Instead of "other", is "control" (like in science experiment where you > > > > have a control group), or "offseted" (which seems to be how this second > > > > binary is constructed) would be better names for this *.bin? It seems > > > > the script take both input and play the game of the 7 differences, to > > > > find clues about the location of some symbols, right?. > > > > > > > > > > I don't know the game and I think people not familiar with it won't > > > find new names more readable but less. > > > > Sorry, the "game" as nothing to do with the name I've proposed. I was > > just asking if the script take both *.bin and was looking for > > differences. > > > > (The game of 7 differences is simple: there's two similar pictures and > > you just look for the 7 differences between them, that's it.) > > > > > Not saying that current names are good, they just need to be located > > > at different addresses with some "magic" in the middle. > > > > Well to me "other" evoke a binary that contains functions that are not > > in "final", but instead they both contain the sames functions with > > slight variation of placement in the file (with added offset, gap), as I > > understand. But if you don't like my proposal, so be it. > > > > What about "base" and "offsetted" ? I don't know why "offsetted" > sounds weird to me but I didn't find anything better. I hope some > native English speaker could come with a better proposal. > What about "base" and "bias"/"biased" ? Frediano
On 17/10/2024 11:58 am, Frediano Ziglio wrote: > On Wed, Oct 16, 2024 at 4:05 PM Frediano Ziglio > <frediano.ziglio@cloud.com> wrote: >> On Wed, Oct 16, 2024 at 12:25 PM Anthony PERARD >> <anthony.perard@vates.tech> wrote: >>> On Wed, Oct 16, 2024 at 09:33:32AM +0100, Frediano Ziglio wrote: >>>> On Tue, Oct 15, 2024 at 2:51 PM Anthony PERARD <anthony.perard@vates.tech> wrote: >>>>> On Mon, Oct 14, 2024 at 05:32:26PM +0100, Frediano Ziglio wrote: >>>>>> On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD <anthony.perard@vates.tech> wrote: >>>>>>> On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote: >>>>>>>> +# generate final object file combining and checking above binaries >>>>>>>> +$(obj)/built_in_32.S: $(obj)/built_in_32.other.bin $(obj)/built_in_32.final.bin >>>>>>> So, "other" isn't part of "final", I don't really know what those two >>>>>>> things contains so naming wise I can't suggest anything useful. >>>>> Instead of "other", is "control" (like in science experiment where you >>>>> have a control group), or "offseted" (which seems to be how this second >>>>> binary is constructed) would be better names for this *.bin? It seems >>>>> the script take both input and play the game of the 7 differences, to >>>>> find clues about the location of some symbols, right?. >>>>> >>>> I don't know the game and I think people not familiar with it won't >>>> find new names more readable but less. >>> Sorry, the "game" as nothing to do with the name I've proposed. I was >>> just asking if the script take both *.bin and was looking for >>> differences. >>> >>> (The game of 7 differences is simple: there's two similar pictures and >>> you just look for the 7 differences between them, that's it.) >>> >>>> Not saying that current names are good, they just need to be located >>>> at different addresses with some "magic" in the middle. >>> Well to me "other" evoke a binary that contains functions that are not >>> in "final", but instead they both contain the sames functions with >>> slight variation of placement in the file (with added offset, gap), as I >>> understand. But if you don't like my proposal, so be it. >>> >> What about "base" and "offsetted" ? I don't know why "offsetted" >> sounds weird to me but I didn't find anything better. I hope some >> native English speaker could come with a better proposal. >> > What about "base" and "bias"/"biased" ? A plain "offset" would be fine in this context. "offsetted" is indeed wrong English. ~Andrew
On 14.10.2024 18:32, Frediano Ziglio wrote: > On Mon, Oct 14, 2024 at 4:31 PM Anthony PERARD > <anthony.perard@vates.tech> wrote: >> >> On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote: >>> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile >>> index 1199291d2b..23ad274c89 100644 >>> --- a/xen/arch/x86/boot/Makefile >>> +++ b/xen/arch/x86/boot/Makefile >>> @@ -1,4 +1,5 @@ >>> obj-bin-y += head.o >>> +obj-bin-y += built_in_32.o >> >> I don't quite like that this new object name is "built_in_32.o", >> It's really closed to "built_in.*" which is used by Rules.mk to collect >> all objects in a subdirectory. I don't really have a better suggestion at >> the moment. >> > > It was cbundle.o before, but people preferred built_in_32.o. > It's a collection of object files like built_in.o so it does not seem > so bad to me. > But seen other replies, some other people prefer "bundle". Well, I for one don't really _prefer_ bundle. I merely see it as a possible option to address Anthony's name ambiguity concern. >>> + $(LD32) $(orphan-handling-y) -N -T $< -Map $(obj)/built_in_32.$(*F).map -o $(obj)/built_in_32.$(*F).o $(obj)/built_in_32.tmp.o >> >> I think this wants to be: -T $(filter %.lds,$^) -Map $(patsubst %.bin,%.map,$@) -o $(patsubst %.bin,%.o,$@) $(filter %.o,$^) >> >> :-(, maybe that's lots of $(patsubst,), not sure which is better between >> $(patsubst,) and using the stem $*. >> > > Not strong about stem or patsubst. > The 2 filters seem good, they suggest lds for the script and objects > for the input, which makes sense. > >> Also, if something tries to use built_in_32.tmp.bin, we have a rule that >> remove it's prerequisite. >> >> BTW, everything is kind of temporary in a build system, beside the few >> files that we want to install on a machine, so having a target named >> with "*tmp*" isn't great. But having a rule that create "*tmp*" file but >> remove them before the end of its recipe is fine (or those *tmp* aren't >> use outside of this recipe). >> > > Mumble... yes, I think the XX.tmp.o was a temporary internal rule file. > So we still don't agree on one name, and now we want to find also > another, tricky. > More or less if it can help, one is a 32 bit object file that bundle > together multiple 32 bits object files while the other is the > conversion of the 32 bits bundle file to 64 bits. > XXX_32.o and XXX_32as64.o ?? Whatever the eventual name (I don't care all that much), just one request: Dashes instead of underscores please. Jan
On 14/10/2024 4:31 pm, Anthony PERARD wrote: > On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote: >> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile >> index 1199291d2b..23ad274c89 100644 >> --- a/xen/arch/x86/boot/Makefile >> +++ b/xen/arch/x86/boot/Makefile >> @@ -1,4 +1,5 @@ >> obj-bin-y += head.o >> +obj-bin-y += built_in_32.o > I don't quite like that this new object name is "built_in_32.o", > It's really closed to "built_in.*" which is used by Rules.mk to collect > all objects in a subdirectory. I don't really have a better suggestion at > the moment. Similarity was the point of suggesting it. It is the end result of "the 32bit objects we've done magic with to shoehorn into 64bit", and a improvement on "cbundle" from earlier iterations. ~Andrew
On 14.10.2024 17:31, Anthony PERARD wrote: > On Mon, Oct 14, 2024 at 09:53:28AM +0100, Frediano Ziglio wrote: >> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile >> index 1199291d2b..23ad274c89 100644 >> --- a/xen/arch/x86/boot/Makefile >> +++ b/xen/arch/x86/boot/Makefile >> @@ -1,4 +1,5 @@ >> obj-bin-y += head.o >> +obj-bin-y += built_in_32.o > > I don't quite like that this new object name is "built_in_32.o", > It's really closed to "built_in.*" which is used by Rules.mk to collect > all objects in a subdirectory. I don't really have a better suggestion at > the moment. blob32.o? bundle32.o? Jan
© 2016 - 2024 Red Hat, Inc.