From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/7933 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH v8] Build process uses script to add CFI directives to x86 asm Date: Sun, 14 Jun 2015 00:37:25 -0400 Message-ID: <20150614043725.GL17573@brightrain.aerifal.cx> References: <1433493558-9027-1-git-send-email-alexinbeijing@gmail.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1434256662 31265 80.91.229.3 (14 Jun 2015 04:37:42 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 14 Jun 2015 04:37:42 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-7946-gllmg-musl=m.gmane.org@lists.openwall.com Sun Jun 14 06:37:41 2015 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1Z3zfx-0001jT-Ds for gllmg-musl@m.gmane.org; Sun, 14 Jun 2015 06:37:41 +0200 Original-Received: (qmail 17833 invoked by uid 550); 14 Jun 2015 04:37:40 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 17812 invoked from network); 14 Jun 2015 04:37:39 -0000 Content-Disposition: inline In-Reply-To: <1433493558-9027-1-git-send-email-alexinbeijing@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:7933 Archived-At: 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