[boot-wrapper PATCH v2 3/9] Makefile: Tell compiler to generate bare-metal code

Tue Jan 18 04:52:03 PST 2022

On Tue, 11 Jan 2022 11:30:18 +0000
Mark Rutland <mark.rutland at 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 at 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 at 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
> > > >     
> >   