Re: [PATCH v8] Build process uses script to add CFI directives to x86 asm

[Rich Felker <dalias@libc.org>]

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