| Message ID | 20211222181607.1203191-4-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Various (build system) fixes | expand |
On Wed, Dec 22, 2021 at 06:16:01PM +0000, Andre Przywara wrote: > Our GCC invocation does not provide many parameters, which lets the > toolchain fill in its own default setup. > In case of a native build or when using a full-featured cross-compiler, > this probably means Linux userland, which is not what we want for a > bare-metal application like boot-wrapper. > > Tell the compiler to forget about those standard settings, and only use > what we explicitly ask for. In particular that means to not use toolchain > provided libraries and headers, since they might pull in more code than > we want, and might not run well in the boot-wrapper environment. > > This also enables optimisation, since it produces much better code and > tends to avoid problems due to missing inlining, for instance. I'd appreciate if we could add some explanation for these (just in the commit message is fine), as e.g. it's not clear to me why -fno-toplevel-reorder is necessary. Thanks, Mark. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > Makefile.am | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/Makefile.am b/Makefile.am > index 3e970a3..d9ad6d1 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -124,9 +124,14 @@ CHOSEN_NODE := chosen { \ > > CPPFLAGS += $(INITRD_FLAGS) > CFLAGS += -I$(top_srcdir)/include/ -I$(top_srcdir)/$(ARCH_SRC)/include/ > -CFLAGS += -Wall -fomit-frame-pointer > +CFLAGS += -Wall -Wstrict-prototypes -fomit-frame-pointer > +CFLAGS += -ffreestanding -nostdinc -nostdlib > +CFLAGS += -fno-common -fno-strict-aliasing -fno-stack-protector > +CFLAGS += -fno-toplevel-reorder > +CFLAGS += -fno-unwind-tables -fno-asynchronous-unwind-tables > CFLAGS += -ffunction-sections -fdata-sections > CFLAGS += -fno-pic -fno-pie > +CFLAGS += -Os > LDFLAGS += --gc-sections > > OBJ := $(addprefix $(ARCH_SRC),$(ARCH_OBJ)) $(addprefix $(COMMON_SRC),$(COMMON_OBJ)) > -- > 2.25.1 >
On Fri, 7 Jan 2022 13:53:50 +0000 Mark Rutland <mark.rutland@arm.com> wrote: Hi Mark, > On Wed, Dec 22, 2021 at 06:16:01PM +0000, Andre Przywara wrote: > > Our GCC invocation does not provide many parameters, which lets the > > toolchain fill in its own default setup. > > In case of a native build or when using a full-featured cross-compiler, > > this probably means Linux userland, which is not what we want for a > > bare-metal application like boot-wrapper. > > > > Tell the compiler to forget about those standard settings, and only use > > what we explicitly ask for. In particular that means to not use toolchain > > provided libraries and headers, since they might pull in more code than > > we want, and might not run well in the boot-wrapper environment. > > > > This also enables optimisation, since it produces much better code and > > tends to avoid problems due to missing inlining, for instance. > > I'd appreciate if we could add some explanation for these (just in the commit > message is fine), as e.g. it's not clear to me why -fno-toplevel-reorder is > necessary. Yeah, I was expecting some discussion about this. I went over most options from the manpage a while ago (for some unrelated baremetal code project), and decided for each option whether it would make sense or not. TBH I don't remember every detail from that, but I can certainly just drop the less critical options from that list. And I would be happy to add some rationale for each of them, but wonder what the best place would be? I'd rather do this in a file than in a commit message, would you prefer comments in Makefile.am, or in README or a separate file, with a pointer from Makefile.am. Or in a commit message anyway? Cheers, Andre > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > Makefile.am | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/Makefile.am b/Makefile.am > > index 3e970a3..d9ad6d1 100644 > > --- a/Makefile.am > > +++ b/Makefile.am > > @@ -124,9 +124,14 @@ CHOSEN_NODE := chosen { \ > > > > CPPFLAGS += $(INITRD_FLAGS) > > CFLAGS += -I$(top_srcdir)/include/ -I$(top_srcdir)/$(ARCH_SRC)/include/ > > -CFLAGS += -Wall -fomit-frame-pointer > > +CFLAGS += -Wall -Wstrict-prototypes -fomit-frame-pointer > > +CFLAGS += -ffreestanding -nostdinc -nostdlib > > +CFLAGS += -fno-common -fno-strict-aliasing -fno-stack-protector > > +CFLAGS += -fno-toplevel-reorder > > +CFLAGS += -fno-unwind-tables -fno-asynchronous-unwind-tables > > CFLAGS += -ffunction-sections -fdata-sections > > CFLAGS += -fno-pic -fno-pie > > +CFLAGS += -Os > > LDFLAGS += --gc-sections > > > > OBJ := $(addprefix $(ARCH_SRC),$(ARCH_OBJ)) $(addprefix $(COMMON_SRC),$(COMMON_OBJ)) > > -- > > 2.25.1 > >
On Fri, Jan 07, 2022 at 02:38:11PM +0000, Andre Przywara wrote: > On Fri, 7 Jan 2022 13:53:50 +0000 > Mark Rutland <mark.rutland@arm.com> wrote: > > Hi Mark, > > > On Wed, Dec 22, 2021 at 06:16:01PM +0000, Andre Przywara wrote: > > > Our GCC invocation does not provide many parameters, which lets the > > > toolchain fill in its own default setup. > > > In case of a native build or when using a full-featured cross-compiler, > > > this probably means Linux userland, which is not what we want for a > > > bare-metal application like boot-wrapper. > > > > > > Tell the compiler to forget about those standard settings, and only use > > > what we explicitly ask for. In particular that means to not use toolchain > > > provided libraries and headers, since they might pull in more code than > > > we want, and might not run well in the boot-wrapper environment. > > > > > > This also enables optimisation, since it produces much better code and > > > tends to avoid problems due to missing inlining, for instance. > > > > I'd appreciate if we could add some explanation for these (just in the commit > > message is fine), as e.g. it's not clear to me why -fno-toplevel-reorder is > > necessary. > > Yeah, I was expecting some discussion about this. I went over most options > from the manpage a while ago (for some unrelated baremetal code project), > and decided for each option whether it would make sense or not. TBH I > don't remember every detail from that, but I can certainly just drop the > less critical options from that list. Yes please. To be clear, I'm only suggesting dropping a few of these, and I think the others just needs some explanation. I'm certain we *should* add: -ffreestanding -nostdlib ... and I'd be happy to take a patch adding just those as an immediate fix. I think the folllowing are reasonable but need some explanation: -Wstrict-prototypes // Just to catch accidents -fno-stack-protector // Because we dont have any init/support code -fno-strict-aliasing // Do we rely on type punning today? -fno-unwind-tables // Because we don't consume the result -fno-asynchronous-unwind-tables // Because we don't consume the result I don't understand why we need: -fno-common -fno-toplevel-reorder -Os ... and I suspect we can drop those unless we have some rationale. I'm torn over: -nostdinc ... as IIUC that's just to catch accidental includes, but necessitates the prior patch adding copies of some headers. We could arguably leave that as-is and just be careful. > And I would be happy to add some rationale for each of them, but wonder > what the best place would be? I'd rather do this in a file than in a > commit message, would you prefer comments in Makefile.am, or in README or > a separate file, with a pointer from Makefile.am. Or in a commit message > anyway? I think within the Makefile would be best if you're happy to organise it, e.g. # The boot-wrapper is a non-hosted environment and isn't linked # with standard libraries. CFLAGS += -ffreestanding -nostdlib # Avoid the pointless creation og a GOT CFLAGS += -fno-pic -fno-pie ... does that work for you? Thanks, Mark. > > Cheers, > Andre > > > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > --- > > > Makefile.am | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/Makefile.am b/Makefile.am > > > index 3e970a3..d9ad6d1 100644 > > > --- a/Makefile.am > > > +++ b/Makefile.am > > > @@ -124,9 +124,14 @@ CHOSEN_NODE := chosen { \ > > > > > > CPPFLAGS += $(INITRD_FLAGS) > > > CFLAGS += -I$(top_srcdir)/include/ -I$(top_srcdir)/$(ARCH_SRC)/include/ > > > -CFLAGS += -Wall -fomit-frame-pointer > > > +CFLAGS += -Wall -Wstrict-prototypes -fomit-frame-pointer > > > +CFLAGS += -ffreestanding -nostdinc -nostdlib > > > +CFLAGS += -fno-common -fno-strict-aliasing -fno-stack-protector > > > +CFLAGS += -fno-toplevel-reorder > > > +CFLAGS += -fno-unwind-tables -fno-asynchronous-unwind-tables > > > CFLAGS += -ffunction-sections -fdata-sections > > > CFLAGS += -fno-pic -fno-pie > > > +CFLAGS += -Os > > > LDFLAGS += --gc-sections > > > > > > OBJ := $(addprefix $(ARCH_SRC),$(ARCH_OBJ)) $(addprefix $(COMMON_SRC),$(COMMON_OBJ)) > > > -- > > > 2.25.1 > > > >
On Tue, 11 Jan 2022 11:30:18 +0000 Mark Rutland <mark.rutland@arm.com> wrote: Hi Mark, > On Fri, Jan 07, 2022 at 02:38:11PM +0000, Andre Przywara wrote: > > On Fri, 7 Jan 2022 13:53:50 +0000 > > Mark Rutland <mark.rutland@arm.com> wrote: > > > > Hi Mark, > > > > > On Wed, Dec 22, 2021 at 06:16:01PM +0000, Andre Przywara wrote: > > > > Our GCC invocation does not provide many parameters, which lets the > > > > toolchain fill in its own default setup. > > > > In case of a native build or when using a full-featured cross-compiler, > > > > this probably means Linux userland, which is not what we want for a > > > > bare-metal application like boot-wrapper. > > > > > > > > Tell the compiler to forget about those standard settings, and only use > > > > what we explicitly ask for. In particular that means to not use toolchain > > > > provided libraries and headers, since they might pull in more code than > > > > we want, and might not run well in the boot-wrapper environment. > > > > > > > > This also enables optimisation, since it produces much better code and > > > > tends to avoid problems due to missing inlining, for instance. > > > > > > I'd appreciate if we could add some explanation for these (just in the commit > > > message is fine), as e.g. it's not clear to me why -fno-toplevel-reorder is > > > necessary. > > > > Yeah, I was expecting some discussion about this. I went over most options > > from the manpage a while ago (for some unrelated baremetal code project), > > and decided for each option whether it would make sense or not. TBH I > > don't remember every detail from that, but I can certainly just drop the > > less critical options from that list. > > Yes please. > > To be clear, I'm only suggesting dropping a few of these, and I think the > others just needs some explanation. > > I'm certain we *should* add: > > -ffreestanding > -nostdlib > > ... and I'd be happy to take a patch adding just those as an immediate fix. > > I think the folllowing are reasonable but need some explanation: > > -Wstrict-prototypes // Just to catch accidents Agreed. > -fno-stack-protector // Because we dont have any init/support code ... and it breaks the link, as we learned. > -fno-strict-aliasing // Do we rely on type punning today? I don't know for sure, but it seems like that's what this kind of low level software would like to do? Plus the kernel uses it also. > -fno-unwind-tables // Because we don't consume the result > -fno-asynchronous-unwind-tables // Because we don't consume the result Plus those two prevent sections of rather pointless data to be emitted into the ELF file. This blew up my own bare metal code considerably, though I don't see it in the boot-wrapper. > I don't understand why we need: > > -fno-common I think most projects use that to put uninitialised global variables in .BSS, to save space in .data and thus the image file. I see it used in all of Linux, U-Boot and TF-A. With the kernel image absolutely dwarfing any actual boot-wrapper code in our ELF file, I think we are not particularly sensitive about code/file size, though, are we? > -fno-toplevel-reorder Yeah, this one is nonsense, I think I had this as a workaround to one particular problem. Will drop it. > -Os AFAICT we don't use any optimisation at the moment, which typically generates horrible code. And IIRC at the least the kernel relies on some optimisation (due to some inlining tricks?), I wonder if we sooner or later inherit this requirement? > ... and I suspect we can drop those unless we have some rationale. > > I'm torn over: > > -nostdinc > > ... as IIUC that's just to catch accidental includes, but necessitates the > prior patch adding copies of some headers. We could arguably leave that as-is > and just be careful. Yeah, I see the point, this probably drops too much. I am just concerned that the standard header files these days pull in far too much *code* even, and then create some dependencies on libraries? For certain division operations, for instance? So looking at the *current* code, I see only stdint.h and stddef.h from the standard headers to be included. I'd say we can live with these coming from the toolchain. The only problem I see is when this gets extended, and someone pulls in string.h, for instance. IIUC modern toolchains put in quite some optimisation in the string routines, which might not go well in the boot-wrapper environment (NEON?). But for now we should indeed keep the standard headers, and drop our own stdint.h. I will send a patch with the remaining flags, plus some rationale as comments, similar to what you sketched already. Cheers, Andre > > And I would be happy to add some rationale for each of them, but wonder > > what the best place would be? I'd rather do this in a file than in a > > commit message, would you prefer comments in Makefile.am, or in README or > > a separate file, with a pointer from Makefile.am. Or in a commit message > > anyway? > > I think within the Makefile would be best if you're happy to organise it, e.g. > > # The boot-wrapper is a non-hosted environment and isn't linked > # with standard libraries. > CFLAGS += -ffreestanding -nostdlib > > # Avoid the pointless creation og a GOT > CFLAGS += -fno-pic -fno-pie > > ... does that work for you? > > Thanks, > Mark. > > > > > Cheers, > > Andre > > > > > > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > --- > > > > Makefile.am | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/Makefile.am b/Makefile.am > > > > index 3e970a3..d9ad6d1 100644 > > > > --- a/Makefile.am > > > > +++ b/Makefile.am > > > > @@ -124,9 +124,14 @@ CHOSEN_NODE := chosen { \ > > > > > > > > CPPFLAGS += $(INITRD_FLAGS) > > > > CFLAGS += -I$(top_srcdir)/include/ -I$(top_srcdir)/$(ARCH_SRC)/include/ > > > > -CFLAGS += -Wall -fomit-frame-pointer > > > > +CFLAGS += -Wall -Wstrict-prototypes -fomit-frame-pointer > > > > +CFLAGS += -ffreestanding -nostdinc -nostdlib > > > > +CFLAGS += -fno-common -fno-strict-aliasing -fno-stack-protector > > > > +CFLAGS += -fno-toplevel-reorder > > > > +CFLAGS += -fno-unwind-tables -fno-asynchronous-unwind-tables > > > > CFLAGS += -ffunction-sections -fdata-sections > > > > CFLAGS += -fno-pic -fno-pie > > > > +CFLAGS += -Os > > > > LDFLAGS += --gc-sections > > > > > > > > OBJ := $(addprefix $(ARCH_SRC),$(ARCH_OBJ)) $(addprefix $(COMMON_SRC),$(COMMON_OBJ)) > > > > -- > > > > 2.25.1 > > > > > >
On Tue, 18 Jan 2022 at 13:53, Andre Przywara <andre.przywara@arm.com> wrote: > > On Tue, 11 Jan 2022 11:30:18 +0000 > Mark Rutland <mark.rutland@arm.com> wrote: > > > I don't understand why we need: > > > > -fno-common > > I think most projects use that to put uninitialised global variables in > .BSS, to save space in .data and thus the image file. I see it used in all > of Linux, U-Boot and TF-A. With the kernel image absolutely dwarfing any > actual boot-wrapper code in our ELF file, I think we are not particularly > sensitive about code/file size, though, are we? > I'd keep this for correctness, otherwise unrelated uninitialized globals with external linkage that happen to have the same name will be merged into a single variable, instead of causing a duplicate symbol error at link time (which is usually the preferred result)
diff --git a/Makefile.am b/Makefile.am index 3e970a3..d9ad6d1 100644 --- a/Makefile.am +++ b/Makefile.am @@ -124,9 +124,14 @@ CHOSEN_NODE := chosen { \ CPPFLAGS += $(INITRD_FLAGS) CFLAGS += -I$(top_srcdir)/include/ -I$(top_srcdir)/$(ARCH_SRC)/include/ -CFLAGS += -Wall -fomit-frame-pointer +CFLAGS += -Wall -Wstrict-prototypes -fomit-frame-pointer +CFLAGS += -ffreestanding -nostdinc -nostdlib +CFLAGS += -fno-common -fno-strict-aliasing -fno-stack-protector +CFLAGS += -fno-toplevel-reorder +CFLAGS += -fno-unwind-tables -fno-asynchronous-unwind-tables CFLAGS += -ffunction-sections -fdata-sections CFLAGS += -fno-pic -fno-pie +CFLAGS += -Os LDFLAGS += --gc-sections OBJ := $(addprefix $(ARCH_SRC),$(ARCH_OBJ)) $(addprefix $(COMMON_SRC),$(COMMON_OBJ))
Our GCC invocation does not provide many parameters, which lets the toolchain fill in its own default setup. In case of a native build or when using a full-featured cross-compiler, this probably means Linux userland, which is not what we want for a bare-metal application like boot-wrapper. Tell the compiler to forget about those standard settings, and only use what we explicitly ask for. In particular that means to not use toolchain provided libraries and headers, since they might pull in more code than we want, and might not run well in the boot-wrapper environment. This also enables optimisation, since it produces much better code and tends to avoid problems due to missing inlining, for instance. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- Makefile.am | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)