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.