From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/7939 Path: news.gmane.org!not-for-mail From: Alex 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 21:06:16 +0200 Message-ID: References: <1433493558-9027-1-git-send-email-alexinbeijing@gmail.com> <20150614043725.GL17573@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=089e0122e6aa19ca8005187f09d3 X-Trace: ger.gmane.org 1434308796 22681 80.91.229.3 (14 Jun 2015 19:06:36 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 14 Jun 2015 19:06:36 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-7952-gllmg-musl=m.gmane.org@lists.openwall.com Sun Jun 14 21:06:31 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 1Z4DEk-0008MK-BH for gllmg-musl@m.gmane.org; Sun, 14 Jun 2015 21:06:30 +0200 Original-Received: (qmail 3399 invoked by uid 550); 14 Jun 2015 19:06:28 -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 3378 invoked from network); 14 Jun 2015 19:06:28 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; bh=D0Yi2pBZznRYQ9oF1Js08bv0N7/pwSutd2mq2Twy5oE=; b=HoOK0Brbv6xIE9pJqsPCB0Tego0qXO73PEuM+V27G5+U2Vmy709hH0N9KC96DroA0F y/lhRaeAuTBPvyvT87gMcSVjnxJMXpabR2AELlmuTFJvS8AP7U6pK2QXwfUFN9pqW3ap 9VydwdgFhm0rMe17xmFW7YnrL47NogyYoxmRSxLLL+vQviHkU4E4wyvH3wAi/C0271Gk 9EdUoetyCeq7j5KTcpVXiTjGvgdbUxM2TQBNfWDE8EhcgCDaaufZRz/OaJztm4HjLqtQ fitonEHN5u/kbIOVsayMofEiiNwZSAoar41NIJTBxLn43wRzOEhLoD7BU+AUQkJKPEkH reyA== X-Received: by 10.50.109.138 with SMTP id hs10mr16649622igb.48.1434308776149; Sun, 14 Jun 2015 12:06:16 -0700 (PDT) In-Reply-To: <20150614043725.GL17573@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:7939 Archived-At: --089e0122e6aa19ca8005187f09d3 Content-Type: text/plain; charset=UTF-8 Thanks for the reply! Comments below: On Sun, Jun 14, 2015 at 6:37 AM, Rich Felker wrote: > 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.) > The reason for not passing CFLAGS is because clang chokes on "-g" when assembling code with CFI directives. I also thought that ASFLAGS might be a useful customization point for people who want to edit config.mak to create a custom build. But you are the judge of that. Since it seems that CFLAGS is needed, would it be acceptable to bypass the issue by saying that clang users simply won't be able to do debug builds of musl until their compiler is fixed? The current state of LLVM's CFI generation is so bad that debug builds probably won't be useful anyways. If that is a sticking point, I might put together a patch for LLVM and see if they want it. Unfortunately, I have already discovered a bunch of other problems with LLVM which would be nice to fix, but time for developing and polishing patches is limited... As an aside, I admire the fact that you picked up on that subtle regression. The standard of code quality and attention to detail on this project is very high, as compared to other open-source projects I have worked on. Kudos to all the contributors! 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? > I'm not an AWKer, but from what I have read, apparently "awk -f script1.awk -f script2.awk" is the equivalent of concatenating "script1.awk" and "script2.awk", so shared functions can easily be put in a common file. It seems that the amount of shared code will be small, however. Actually, the entire script for x86-32 is already fairly small. I feel that anything more sophisticated than picking a script based on arch would just be complicating matters for little benefit. If it turns out that I am wrong, the commonalities can be abstracted out later. At that time, with several such preprocessing scripts available to look at, it will be clearer what and how to abstract. > > > # > > +# 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. > OK. It was written like this because "echo '.cfi_startproc\n.cfi_endproc'" didn't work on BusyBox ash. But it seems that printf is fine. Will revise. Thanks, AD --089e0122e6aa19ca8005187f09d3 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Thanks for the reply! Comments below:

On Sun, Jun 14, 2015 at 6:37 AM, R= ich Felker <dalias@libc.org> wrote:
On Fri, Jun 05, 2015 at 10:39:18AM +0200, Alex Do= wad 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))))
>=C2=A0 =C2=A0 =C2=A0 =C2=A0$(CC) $(CFLAGS_ALL_STATIC) -c -o $@ $(dir $&= lt;)$(shell cat $<)
>
>=C2=A0 %.o: $(ARCH)/%.s
> -=C2=A0 =C2=A0 =C2=A0$(CC) $(CFLAGS_ALL_STATIC) -c -o $@ $<
> +ifeq ($(ADD_CFI),yes)
> +=C2=A0 =C2=A0 =C2=A0LC_ALL=3DC awk -f tools/add-cfi.$(ARCH).awk $<= | $(CC) $(ASFLAGS) -x assembler -c -o $@ -
> +else
> +=C2=A0 =C2=A0 =C2=A0$(CC) $(ASFLAGS) -c -o $@ $<
> +endif

Removing $(CFLAGS_STATIC_ALL) here is a regression. -Wa,--noexecstac= k
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<= br> 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.)

The reas= on for not passing CFLAGS is because clang chokes on "-g" when as= sembling code with CFI directives. I also thought that ASFLAGS might be a u= seful customization point for people who want to edit config.mak to create = a custom build. But you are the judge of that.

Sin= ce it seems that CFLAGS is needed, would it be acceptable to bypass the iss= ue by saying that clang users simply won't be able to do debug builds o= f musl until their compiler is fixed? The current state of LLVM's CFI g= eneration is so bad that debug builds probably won't be useful anyways.=

If that is a sticking point, I might put together= a patch for LLVM and see if they want it. Unfortunately, I have already di= scovered a bunch of other problems with LLVM which would be nice to fix, bu= t time for developing and polishing patches is limited...

As an aside, I admire the fact that you picked up on that subtle re= gression. The standard of code quality and attention to detail on this proj= ect is very high, as compared to other open-source projects I have worked o= n. Kudos to all the contributors!

As for the naming (tools/add-cfi.$(ARCH).awk), I'm not opposed to this<= br> 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?

I'm not an AWKer, but fr= om what I have read, apparently "awk -f script1.awk -f script2.awk&quo= t; is the equivalent of concatenating "script1.awk" and "scr= ipt2.awk", so shared functions can easily be put in a common file.

It seems that the amount of shared code will be small= , however. Actually, the entire script for x86-32 is already fairly small. = I feel that anything more sophisticated than picking a script based on arch= would just be complicating matters for little benefit.

If it turns out that I am wrong, the commonalities can be abstracted = out later. At that time, with several such preprocessing scripts available = to look at, it will be clearer what and how to abstract.
=C2=A0

>=C2=A0 #
> +# 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 de= bugging information... "
> +if fnmatch '-g*|*\ -g*' "$CFLAGS_AUTO" && > +=C2=A0 =C2=A0test -f "tools/add-cfi.$ARCH.awk" && > +=C2=A0 =C2=A0echo ".cfi_startproc
> +.cfi_endproc" | $CC -x assembler -c -o /dev/null -
> +then
> +=C2=A0 ADD_CFI=3Dyes
> +else
> +=C2=A0 ADD_CFI=3Dno
> +fi
> +printf "%s\n" "$ADD_CFI"
> +
> +#

This test looks nice and robust. I'd mildly prefer:

=C2=A0 printf '.cfi_startproc\n.cfi_endproc\n'

to avoid the multi-line string with echo, but that's a tiny detail.
=

OK. It was written like this because "= ;echo '.cfi_startproc\n.cfi_endproc'" didn't work on BusyB= ox ash. But it seems that printf is fine. Will revise.

=
Thanks, AD
--089e0122e6aa19ca8005187f09d3--