[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150614043725.GL17573@brightrain.aerifal.cx>
Date: Sun, 14 Jun 2015 00:37:25 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH v8] Build process uses script to add CFI
directives to x86 asm
On Fri, Jun 05, 2015 at 10:39:18AM +0200, Alex Dowad wrote:
> Dear musl devs,
>
> Fixed one bug. Otherwise everything looks good in testing.
>
> Thanks, AD
Sorry it's taken me a while to get back to this. I'm working on the
nommu/sh2 stuff, byte-based C locale, and several other things that
have come up, but I definitely want to get to the CFI patch in this
release cycle. A few comments:
> Makefile | 12 ++-
> configure | 20 +++++
> tools/add-cfi.i386.awk | 227 +++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 257 insertions(+), 2 deletions(-)
> create mode 100644 tools/add-cfi.i386.awk
>
> diff --git a/Makefile b/Makefile
> index 2eb7b30..9b55fd8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -120,7 +120,11 @@ $(foreach s,$(wildcard src/*/$(ARCH)*/*.s),$(eval $(call mkasmdep,$(s))))
> $(CC) $(CFLAGS_ALL_STATIC) -c -o $@ $(dir $<)$(shell cat $<)
>
> %.o: $(ARCH)/%.s
> - $(CC) $(CFLAGS_ALL_STATIC) -c -o $@ $<
> +ifeq ($(ADD_CFI),yes)
> + LC_ALL=C awk -f tools/add-cfi.$(ARCH).awk $< | $(CC) $(ASFLAGS) -x assembler -c -o $@ -
> +else
> + $(CC) $(ASFLAGS) -c -o $@ $<
> +endif
Removing $(CFLAGS_STATIC_ALL) here is a regression. -Wa,--noexecstack
is necessary to prevent the kernel from giving us an executable stack
when asm files are linked. We could move it to a separate ASFLAGS, but
the patch doesn't do this, and unless there's a real need to avoid
passing CFLAGS, I'd rather not add more vars. (In this case, needing
the new var would be a silent security regression for anyone building
without re-running configure.)
As for the naming (tools/add-cfi.$(ARCH).awk), I'm not opposed to this
and the configure test for it is nice, but I wonder if there will be
significant code duplication between versions of this script for
different archs that would make it preferable to take the arch as an
argument. What do you think? Or does awk have an easy #include-like
mechanism?
> #
> +# Preprocess asm files to add extra debugging information if debug is
> +# enabled, our assembler supports the needed directives, and the
> +# preprocessing script has been written for our architecture.
> +#
> +printf "checking whether we should preprocess assembly to add debugging information... "
> +if fnmatch '-g*|*\ -g*' "$CFLAGS_AUTO" &&
> + test -f "tools/add-cfi.$ARCH.awk" &&
> + echo ".cfi_startproc
> +.cfi_endproc" | $CC -x assembler -c -o /dev/null -
> +then
> + ADD_CFI=yes
> +else
> + ADD_CFI=no
> +fi
> +printf "%s\n" "$ADD_CFI"
> +
> +#
This test looks nice and robust. I'd mildly prefer:
printf '.cfi_startproc\n.cfi_endproc\n'
to avoid the multi-line string with echo, but that's a tiny detail.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
