On Fri, Jun 05, 2015 at 10:39:18AM +0200, Alex Dowad wrote:
> 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.