Re: [kvm-unit-tests PATCH v1 01/18] Makefile: Define __ASSEMBLY__ for assembly files

Andrew Jones
2024-02-15T19:13:37Z

On Thu, Feb 15, 2024 at 05:16:01PM +0000, Alexandru Elisei wrote:
> Hi Drew,
>
> On Thu, Feb 15, 2024 at 05:32:22PM +0100, Andrew Jones wrote:
> > On Thu, Feb 15, 2024 at 04:05:56PM +0000, Alexandru Elisei wrote:
> > > Hi Drew,
> > >
> > > On Mon, Jan 15, 2024 at 01:44:17PM +0100, Andrew Jones wrote:
> > > > On Thu, Nov 30, 2023 at 04:07:03AM -0500, Shaoqin Huang wrote:
> > > > > From: Alexandru Elisei <alexandru.elisei@arm.com>
> > > > >
> > > > > There are 25 header files today (found with grep -r "#ifndef __ASSEMBLY__)
> > > > > with functionality relies on the __ASSEMBLY__ prepocessor constant being
> > > > > correctly defined to work correctly. So far, kvm-unit-tests has relied on
> > > > > the assembly files to define the constant before including any header
> > > > > files which depend on it.
> > > > >
> > > > > Let's make sure that nobody gets this wrong and define it as a compiler
> > > > > constant when compiling assembly files. __ASSEMBLY__ is now defined for all
> > > > > .S files, even those that didn't set it explicitely before.
> > > > >
> > > > > Reviewed-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> > > > > Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
> > > > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > > > > Signed-off-by: Shaoqin Huang <shahuang@redhat.com>
> > > > > ---
> > > > > Makefile | 5 ++++-
> > > > > arm/cstart.S | 1 -
> > > > > arm/cstart64.S | 1 -
> > > > > powerpc/cstart64.S | 1 -
> > > > > 4 files changed, 4 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/Makefile b/Makefile
> > > > > index 602910dd..27ed14e6 100644
> > > > > --- a/Makefile
> > > > > +++ b/Makefile
> > > > > @@ -92,6 +92,9 @@ CFLAGS += -Woverride-init -Wmissing-prototypes -Wstrict-prototypes
> > > > >
> > > > > autodepend-flags = -MMD -MP -MF $(dir $*).$(notdir $*).d
> > > > >
> > > > > +AFLAGS = $(CFLAGS)
> > > > > +AFLAGS += -D__ASSEMBLY__
> > > > > +
> > > > > LDFLAGS += -nostdlib $(no_pie) -z noexecstack
> > > > >
> > > > > $(libcflat): $(cflatobjs)
> > > > > @@ -113,7 +116,7 @@ directories:
> > > > > @mkdir -p $(OBJDIRS)
> > > > >
> > > > > %.o: %.S
> > > > > - $(CC) $(CFLAGS) -c -nostdlib -o $@ $<
> > > > > + $(CC) $(AFLAGS) -c -nostdlib -o $@ $<
> > > >
> > > > I think we can drop the two hunks above from this patch and just rely on
> > > > the compiler to add __ASSEMBLY__ for us when compiling assembly files.
> > >
> > > I think the precompiler adds __ASSEMBLER__, not __ASSEMBLY__ [1]. Am I
> > > missing something?
> > >
> > > [1] https://gcc.gnu.org/onlinedocs/cpp/macros/predefined-macros.html#c.__ASSEMBLER__
> >
> > You're right. I'm not opposed to changing all the __ASSEMBLY__ references
> > to __ASSEMBLER__. I'll try to do that at some point unless you beat me to
> > it.
>
> Actually, I quite prefer the Linux style of using __ASSEMBLY__ instead of
> __ASSEMBLER__, because it makes reusing Linux files easier. That, and the
> habit formed by staring at Linux assembly files.

Those are good arguments and also saves the churn. OK, let's keep this
patch and __ASSEMBLY__

Thanks,
drew