musl - Re: [PATCH v3] Build process uses script to add CFI directives to x86 asm

Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 13 May 2015 21:22:52 +0200
From: Szabolcs Nagy <nsz@...t70.net>
To: musl@...ts.openwall.com
Subject: Re: [PATCH v3] Build process uses script to add CFI
 directives to x86 asm

* Alex Dowad <alexinbeijing@...il.com> [2015-05-13 19:54:39 +0200]:
> 
> I've noticed that using tempfiles for the augmented asm has a drawback:
> In the source file/line debugging info generated by the assembler, it records
> the source file as "/tmp/<random-garbage>". Then, when you try to debug a program
> which is linked against the resulting musl, GDB tries to open "/tmp/<random-garbage>"
> to show in the source window.
> 
> Suggestions?? Perhaps generate .cfi.s files as Szabolcs suggested??
> 

you can use

 .file "foo.s"

> diff --git a/Makefile b/Makefile
> index 6559295..9aefd62 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -118,7 +118,7 @@ $(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 $@ $<
> +	tools/aswrap.sh $< $@ $(ARCH) "$(CC) $(CFLAGS_ALL_STATIC)"
>  

i think passing down the build command that way is not ok

" may be used inside CFLAGS (and there are other shell quote issues)
it hides the build command in the make output
it's not clear if the build correctly handles if CC fails

> +function get_const1() {
> +  # for instructions with 2 operands, get 1st operand (assuming it is constant)
> +  match($0, /-?(0x[0-9a-fA-F]+|[0-9]+),/)
> +  return parse_const(substr($0, RSTART, RLENGTH-1))
> +}

it only matches with immediate ,

i'd just clean the whitespaces up so you dont have to add \s* or \s+
to every regex (see below)

awk converts strings to numbers, but hex is unfortunately not guaranteed
to be supported (otherwise strtod conversion rules apply)

but you can implement parse_const(s) as

	sign = sub(/^-/,"",s)
	hex = sub(/^0x/,"",s)
	if (hex)
		n = hex2int(s)
	else
		n = s+0
	return sign ? -n : n

this does not handle binary (0b11) and octal (0123) asm consts
(i think you should check for those and emit a warning).

> +
> +{ print }
> +

i'd do it something like

{
	# print original line
	print

	# avoid generating .cfi based on comments
	gsub(/(#|\/\/).*/,"")

	# canonicalize whitespaces
	gsub(/\s+/," ")
	gsub(/ *, */,",")
	gsub(/ *: */,": ")
	sub(/ $/,"")
	sub(/^ /,"")

	# dont do anything with string consts
	if ($0 ~ /^\.(ascii|str).*"/)
		next

	# possibly handle /**/ and multiline string consts (split by \)
	# if you dont want to do that then check for it and print warning
	if ($0 ~ /\/\*/ || $0 ~ /\\/)
		print ARGV[0] ": warning: unhandled asm: " $0 > "/dev/stderr"
}

> +/^.global\s+\w+/ {
> +  globals[$2] = 1

may be spelt as .globl too

> +}

> +/pushl?/ {
> +  if (match($0, /\s+%(ax|bx|cx|dx|di|si|bp|sp)/))
> +    adjust_sp_offset(2)
> +  else
> +    adjust_sp_offset(4)
> +}

i think

 pushl $123
 push $123

are different

> diff --git a/tools/aswrap.sh b/tools/aswrap.sh
> new file mode 100755
> index 0000000..0afbd4e
> --- /dev/null
> +++ b/tools/aswrap.sh
> @@ -0,0 +1,15 @@
> +#!/bin/sh
> +# Run assembler to produce an object file, optionally applying other pre-processing steps
> +input=$1
> +output=$2
> +arch=$3
> +as=$4
> +
> +if [ -f "tools/add-cfi.awk.$arch" ]; then
> +  tmpfile=$(mktemp -t musl-aswrap-XXXXXX)
> +  awk -f tools/add-cfi.awk.$arch $input >$tmpfile
> +  mv $tmpfile $tmpfile.s
> +  input=$tmpfile.s
> +fi

set LC_ALL=C because you depend on collation order
in the awk script

or use [[:alnum:]] etc in the regex

> +$as -c -o $output $input
> \ No newline at end of file

add new lines at the end
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.