* [PATCH 1/3] in i386 CFI script, binary ops like ADD or AND modify 2nd operand, not 1st @ 2015-10-12 13:58 Alex Dowad 2015-10-12 13:58 ` [PATCH 2/3] fix matching errors related to i386 addressing modes in CFI generation script Alex Dowad 2015-10-12 13:59 ` [PATCH 3/3] add CFI generation script for x86_64 Alex Dowad 0 siblings, 2 replies; 5+ messages in thread From: Alex Dowad @ 2015-10-12 13:58 UTC (permalink / raw) To: musl thanks to R. Felker for noticing this problem. --- Fixed the issue with index registers being mistakenly identified as the destination for a binary op (thus causing their values to be treated as if overwritten). The fix is slightly hacky, but maintains brevity. The other option would be to write a long regex which is able to parse all x86 addressing modes. Also tweaked commit messages. Any other feedback will be appreciated, Alex Dowad tools/add-cfi.i386.awk | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/add-cfi.i386.awk b/tools/add-cfi.i386.awk index 5dc8794..fc0d8cf 100644 --- a/tools/add-cfi.i386.awk +++ b/tools/add-cfi.i386.awk @@ -185,12 +185,12 @@ function trashed(register) { # this does NOT exhaustively check for all possible instructions which could # overwrite a register value inherited from the caller (just the common ones) /mov.*,%e(ax|bx|cx|dx|si|di|bp)/ { trashed(get_reg2()) } -/(add|addl|sub|subl|and|or|xor|lea|sal|sar|shl|shr) %e(ax|bx|cx|dx|si|di|bp),/ { - trashed(get_reg1()) +/(add|addl|sub|subl|and|or|xor|lea|sal|sar|shl|shr).*,%e(ax|bx|cx|dx|si|di|bp)/ { + trashed(get_reg2()) } -/^i?mul [^,]*$/ { trashed("eax"); trashed("edx") } -/^i?mul %e(ax|bx|cx|dx|si|di|bp),/ { trashed(get_reg1()) } -/^i?div/ { trashed("eax"); trashed("edx") } +/^i?mul [^,]*$/ { trashed("eax"); trashed("edx") } +/^i?mul.*,%e(ax|bx|cx|dx|si|di|bp)/ { trashed(get_reg2()) } +/^i?div/ { trashed("eax"); trashed("edx") } /(dec|inc|not|neg|pop) %e(ax|bx|cx|dx|si|di|bp)/ { trashed(get_reg()) } /cpuid/ { trashed("eax"); trashed("ebx"); trashed("ecx"); trashed("edx") } -- 2.0.0.GIT ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/3] fix matching errors related to i386 addressing modes in CFI generation script 2015-10-12 13:58 [PATCH 1/3] in i386 CFI script, binary ops like ADD or AND modify 2nd operand, not 1st Alex Dowad @ 2015-10-12 13:58 ` Alex Dowad 2015-10-12 15:12 ` Rich Felker 2015-10-12 13:59 ` [PATCH 3/3] add CFI generation script for x86_64 Alex Dowad 1 sibling, 1 reply; 5+ messages in thread From: Alex Dowad @ 2015-10-12 13:58 UTC (permalink / raw) To: musl the regexps previously used to identify registers clobbered by MOVs, ADDs, and various other operations would erroneously match index registers. In other words, the following asm: mov $0, (%eax,%ebx,4) ...would cause EBX to be considered as overwritten, which might prevent a debugger from displaying a variable's value in a higher stack frame. thanks to Rich Felker for noticing this problem. --- tools/add-cfi.i386.awk | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/add-cfi.i386.awk b/tools/add-cfi.i386.awk index fc0d8cf..bd7932f 100644 --- a/tools/add-cfi.i386.awk +++ b/tools/add-cfi.i386.awk @@ -184,13 +184,13 @@ function trashed(register) { } # this does NOT exhaustively check for all possible instructions which could # overwrite a register value inherited from the caller (just the common ones) -/mov.*,%e(ax|bx|cx|dx|si|di|bp)/ { trashed(get_reg2()) } -/(add|addl|sub|subl|and|or|xor|lea|sal|sar|shl|shr).*,%e(ax|bx|cx|dx|si|di|bp)/ { +/mov.*,%e(ax|bx|cx|dx|si|di|bp)$/ { trashed(get_reg2()) } +/(add|addl|sub|subl|and|or|xor|lea|sal|sar|shl|shr).*,%e(ax|bx|cx|dx|si|di|bp)$/ { trashed(get_reg2()) } -/^i?mul [^,]*$/ { trashed("eax"); trashed("edx") } -/^i?mul.*,%e(ax|bx|cx|dx|si|di|bp)/ { trashed(get_reg2()) } -/^i?div/ { trashed("eax"); trashed("edx") } +/^i?mul [^,]*$/ { trashed("eax"); trashed("edx") } +/^i?mul.*,%e(ax|bx|cx|dx|si|di|bp)$/ { trashed(get_reg2()) } +/^i?div/ { trashed("eax"); trashed("edx") } /(dec|inc|not|neg|pop) %e(ax|bx|cx|dx|si|di|bp)/ { trashed(get_reg()) } /cpuid/ { trashed("eax"); trashed("ebx"); trashed("ecx"); trashed("edx") } -- 2.0.0.GIT ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] fix matching errors related to i386 addressing modes in CFI generation script 2015-10-12 13:58 ` [PATCH 2/3] fix matching errors related to i386 addressing modes in CFI generation script Alex Dowad @ 2015-10-12 15:12 ` Rich Felker 2015-10-12 18:30 ` Alex 0 siblings, 1 reply; 5+ messages in thread From: Rich Felker @ 2015-10-12 15:12 UTC (permalink / raw) To: musl On Mon, Oct 12, 2015 at 03:58:59PM +0200, Alex Dowad wrote: > the regexps previously used to identify registers clobbered by MOVs, ADDs, > and various other operations would erroneously match index registers. In other > words, the following asm: > > mov $0, (%eax,%ebx,4) > > ....would cause EBX to be considered as overwritten, which might prevent a > debugger from displaying a variable's value in a higher stack frame. > > thanks to Rich Felker for noticing this problem. > --- > tools/add-cfi.i386.awk | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/tools/add-cfi.i386.awk b/tools/add-cfi.i386.awk > index fc0d8cf..bd7932f 100644 > --- a/tools/add-cfi.i386.awk > +++ b/tools/add-cfi.i386.awk > @@ -184,13 +184,13 @@ function trashed(register) { > } > # this does NOT exhaustively check for all possible instructions which could > # overwrite a register value inherited from the caller (just the common ones) > -/mov.*,%e(ax|bx|cx|dx|si|di|bp)/ { trashed(get_reg2()) } > -/(add|addl|sub|subl|and|or|xor|lea|sal|sar|shl|shr).*,%e(ax|bx|cx|dx|si|di|bp)/ { > +/mov.*,%e(ax|bx|cx|dx|si|di|bp)$/ { trashed(get_reg2()) } > +/(add|addl|sub|subl|and|or|xor|lea|sal|sar|shl|shr).*,%e(ax|bx|cx|dx|si|di|bp)$/ { > trashed(get_reg2()) > } > -/^i?mul [^,]*$/ { trashed("eax"); trashed("edx") } > -/^i?mul.*,%e(ax|bx|cx|dx|si|di|bp)/ { trashed(get_reg2()) } > -/^i?div/ { trashed("eax"); trashed("edx") } > +/^i?mul [^,]*$/ { trashed("eax"); trashed("edx") } > +/^i?mul.*,%e(ax|bx|cx|dx|si|di|bp)$/ { trashed(get_reg2()) } > +/^i?div/ { trashed("eax"); trashed("edx") } > /(dec|inc|not|neg|pop) %e(ax|bx|cx|dx|si|di|bp)/ { trashed(get_reg()) } > /cpuid/ { trashed("eax"); trashed("ebx"); trashed("ecx"); trashed("edx") } Clever. At first I didn't see how this was fixing anything, with the .* still there, but given that you strip comments and extra whitespace, anchoring to the end with $ seems to work. While seeing them separately was useful for seeing how you fixed the bug, patches 1 and 2 should be merged for commit. All patch 2 is doing is fixing a bug that patch 1 introduces; together they just form a non-buggy version of "fix operand order". I can take care of the merging though. One other thing I noticed for future improvement: your patterns don't seem to catch instructions that modify just the low byte or half of a register. These are fairly uncommon in musl's i386 asm, but for x86_64, I would estimate a good 50% of register usage uses the 32-bit half (%e..) of a register rather than the full %r.., and your current script fails to mark these clobbers at all. Probably the regex should be something like %[er]?([abcd][xlh]|si|di|bp|...) - I don't recall the right form for the numbered x86_64 registers' low parts right off, though. Rich ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] fix matching errors related to i386 addressing modes in CFI generation script 2015-10-12 15:12 ` Rich Felker @ 2015-10-12 18:30 ` Alex 0 siblings, 0 replies; 5+ messages in thread From: Alex @ 2015-10-12 18:30 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 3230 bytes --] On Mon, Oct 12, 2015 at 5:12 PM, Rich Felker <dalias@libc.org> wrote: > On Mon, Oct 12, 2015 at 03:58:59PM +0200, Alex Dowad wrote: > > the regexps previously used to identify registers clobbered by MOVs, > ADDs, > > and various other operations would erroneously match index registers. In > other > > words, the following asm: > > > > mov $0, (%eax,%ebx,4) > > > > ....would cause EBX to be considered as overwritten, which might prevent > a > > debugger from displaying a variable's value in a higher stack frame. > > > > thanks to Rich Felker for noticing this problem. > > --- > > tools/add-cfi.i386.awk | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/tools/add-cfi.i386.awk b/tools/add-cfi.i386.awk > > index fc0d8cf..bd7932f 100644 > > --- a/tools/add-cfi.i386.awk > > +++ b/tools/add-cfi.i386.awk > > @@ -184,13 +184,13 @@ function trashed(register) { > > } > > # this does NOT exhaustively check for all possible instructions which > could > > # overwrite a register value inherited from the caller (just the common > ones) > > -/mov.*,%e(ax|bx|cx|dx|si|di|bp)/ { trashed(get_reg2()) } > > > -/(add|addl|sub|subl|and|or|xor|lea|sal|sar|shl|shr).*,%e(ax|bx|cx|dx|si|di|bp)/ > { > > +/mov.*,%e(ax|bx|cx|dx|si|di|bp)$/ { trashed(get_reg2()) } > > > +/(add|addl|sub|subl|and|or|xor|lea|sal|sar|shl|shr).*,%e(ax|bx|cx|dx|si|di|bp)$/ > { > > trashed(get_reg2()) > > } > > -/^i?mul [^,]*$/ { trashed("eax"); trashed("edx") } > > -/^i?mul.*,%e(ax|bx|cx|dx|si|di|bp)/ { trashed(get_reg2()) } > > -/^i?div/ { trashed("eax"); trashed("edx") } > > +/^i?mul [^,]*$/ { trashed("eax"); trashed("edx") } > > +/^i?mul.*,%e(ax|bx|cx|dx|si|di|bp)$/ { trashed(get_reg2()) } > > +/^i?div/ { trashed("eax"); trashed("edx") } > > /(dec|inc|not|neg|pop) %e(ax|bx|cx|dx|si|di|bp)/ { trashed(get_reg()) } > > /cpuid/ { trashed("eax"); trashed("ebx"); trashed("ecx"); > trashed("edx") } > > Clever. At first I didn't see how this was fixing anything, with the > .* still there, but given that you strip comments and extra > whitespace, anchoring to the end with $ seems to work. > > While seeing them separately was useful for seeing how you fixed the > bug, patches 1 and 2 should be merged for commit. All patch 2 is doing > is fixing a bug that patch 1 introduces; together they just form a > non-buggy version of "fix operand order". I can take care of the > merging though. > Let me do it. I'll merge them and send another patch series. One other thing I noticed for future improvement: your patterns don't > seem to catch instructions that modify just the low byte or half of a > register. These are fairly uncommon in musl's i386 asm, but for > x86_64, I would estimate a good 50% of register usage uses the 32-bit > half (%e..) of a register rather than the full %r.., and your current > script fails to mark these clobbers at all. Probably the regex should > be something like %[er]?([abcd][xlh]|si|di|bp|...) - I don't recall > the right form for the numbered x86_64 registers' low parts right off, > though. Thanks for bringing this out. I'll fix this too and send again. Alex [-- Attachment #2: Type: text/html, Size: 4254 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 3/3] add CFI generation script for x86_64 2015-10-12 13:58 [PATCH 1/3] in i386 CFI script, binary ops like ADD or AND modify 2nd operand, not 1st Alex Dowad 2015-10-12 13:58 ` [PATCH 2/3] fix matching errors related to i386 addressing modes in CFI generation script Alex Dowad @ 2015-10-12 13:59 ` Alex Dowad 1 sibling, 0 replies; 5+ messages in thread From: Alex Dowad @ 2015-10-12 13:59 UTC (permalink / raw) To: musl this makes debugging more pleasant when you have to dig into x86_64 assembly, since GCC can obtain accurate stack traces. it turns out that the parts we care about are almost exactly like i386, except for the register names! --- tools/add-cfi.x86_64.awk | 185 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 185 insertions(+) create mode 100644 tools/add-cfi.x86_64.awk diff --git a/tools/add-cfi.x86_64.awk b/tools/add-cfi.x86_64.awk new file mode 100644 index 0000000..173fb16 --- /dev/null +++ b/tools/add-cfi.x86_64.awk @@ -0,0 +1,185 @@ +# Insert GAS CFI directives ("control frame information") into x86-64 asm input + +BEGIN { + # don't put CFI data in the .eh_frame ELF section (which we don't keep) + print ".cfi_sections .debug_frame" + + # only emit CFI directives inside a function + in_function = 0 + + # emit .loc directives with line numbers from original source + printf ".file 1 \"%s\"\n", ARGV[1] + line_number = 0 + + # used to detect "call label; label:" trick + called = "" +} + +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)) +} +function get_reg() { + # only use if you already know there is 1 and only 1 register + match($0, /%r(ax|bx|cx|dx|si|di|bp|8|9|10|11|12|13|14|15)/) + return substr($0, RSTART+1, RLENGTH-1) +} +function get_reg1() { + # for instructions with 2 operands, get 1st operand (assuming it is register) + match($0, /%r(ax|bx|cx|dx|si|di|bp|8|9|10|11|12|13|14|15),/) + return substr($0, RSTART+1, RLENGTH-2) +} +function get_reg2() { + # for instructions with 2 operands, get 2nd operand (assuming it is register) + match($0, /,%r(ax|bx|cx|dx|si|di|bp|8|9|10|11|12|13|14|15)/) + return substr($0, RSTART+2, RLENGTH-2) +} + +function adjust_sp_offset(delta) { + if (in_function) + printf ".cfi_adjust_cfa_offset %d\n", delta +} + +{ + line_number = line_number + 1 + + # clean the input up before doing anything else + # delete comments + gsub(/(#|\/\/).*/, "") + + # canonicalize whitespace + gsub(/[ \t]+/, " ") # mawk doesn't understand \s + gsub(/ *, */, ",") + gsub(/ *: */, ": ") + gsub(/ $/, "") + gsub(/^ /, "") +} + +# check for assembler directives which we care about +/^\.(section|data|text)/ { + # a .cfi_startproc/.cfi_endproc pair should be within the same section + # otherwise, clang will choke when generating ELF output + if (in_function) { + print ".cfi_endproc" + in_function = 0 + } +} +/^\.type [a-zA-Z0-9_]+,\@function/ { + functions[substr($2, 1, length($2)-10)] = 1 +} +# not interested in assembler directives beyond this, just pass them through +/^\./ { + print + next +} + +/^[a-zA-Z0-9_]+:/ { + label = substr($1, 1, length($1)-1) # drop trailing : + + if (called == label) { + # note adjustment of stack pointer from "call label; label:" + adjust_sp_offset(8) + } + + if (functions[label]) { + if (in_function) + print ".cfi_endproc" + + in_function = 1 + print ".cfi_startproc" + + for (register in saved) + delete saved[register] + for (register in dirty) + delete dirty[register] + } + + # an instruction may follow on the same line, so continue processing +} + +/^$/ { next } + +{ + called = "" + printf ".loc 1 %d\n", line_number + print +} + +# KEEPING UP WITH THE STACK POINTER +# %rsp should only be adjusted by pushing/popping or adding/subtracting constants +# +/pushl?/ { + adjust_sp_offset(8) +} +/popl?/ { + adjust_sp_offset(-8) +} +/addl? \$-?(0x[0-9a-fA-F]+|[0-9]+),%rsp/ { adjust_sp_offset(-get_const1()) } +/subl? \$-?(0x[0-9a-fA-F]+|[0-9]+),%rsp/ { adjust_sp_offset(get_const1()) } + +/call/ { + if (match($0, /call [0-9]+f/)) # "forward" label + called = substr($0, RSTART+5, RLENGTH-6) + else if (match($0, /call [0-9a-zA-Z_]+/)) + called = substr($0, RSTART+5, RLENGTH-5) +} + +# TRACKING REGISTER VALUES FROM THE PREVIOUS STACK FRAME +# +/pushl? %r(ax|bx|cx|dx|si|di|bp|8|9|10|11|12|13|14|15)/ { # don't match "push (%reg)" + # if a register is being pushed, and its value has not changed since the + # beginning of this function, the pushed value can be used when printing + # local variables at the next level up the stack + # emit '.cfi_rel_offset' for that + + if (in_function) { + register = get_reg() + if (!saved[register] && !dirty[register]) { + printf ".cfi_rel_offset %s,0\n", register + saved[register] = 1 + } + } +} + +/movl? %r(ax|bx|cx|dx|si|di|bp|8|9|10|11|12|13|14|15),-?(0x[0-9a-fA-F]+|[0-9]+)?\(%rsp\)/ { + if (in_function) { + register = get_reg() + if (match($0, /-?(0x[0-9a-fA-F]+|[0-9]+)\(%rsp\)/)) { + offset = parse_const(substr($0, RSTART, RLENGTH-6)) + } else { + offset = 0 + } + if (!saved[register] && !dirty[register]) { + printf ".cfi_rel_offset %s,%d\n", register, offset + saved[register] = 1 + } + } +} + +# IF REGISTER VALUES ARE UNCEREMONIOUSLY TRASHED +# ...then we want to know about it. +# +function trashed(register) { + if (in_function && !saved[register] && !dirty[register]) { + printf ".cfi_undefined %s\n", register + } + dirty[register] = 1 +} +# this does NOT exhaustively check for all possible instructions which could +# overwrite a register value inherited from the caller (just the common ones) +/mov.*,%r(ax|bx|cx|dx|si|di|bp|8|9|10|11|12|13|14|15)$/ { trashed(get_reg2()) } +/(add|addl|sub|subl|and|or|xor|lea|sal|sar|shl|shr).*,%r(ax|bx|cx|dx|si|di|bp|8|9|10|11|12|13|14|15)$/ { + trashed(get_reg2()) +} +/^i?mul [^,]*$/ { trashed("rax"); trashed("rdx") } +/^i?mul.*,%r(ax|bx|cx|dx|si|di|bp|8|9|10|11|12|13|14|15)$/ { trashed(get_reg2()) } +/^i?div/ { trashed("rax"); trashed("rdx") } + +/(dec|inc|not|neg|pop) %r(ax|bx|cx|dx|si|di|bp|8|9|10|11|12|13|14|15)/ { trashed(get_reg()) } +/cpuid/ { trashed("rax"); trashed("rbx"); trashed("rcx"); trashed("rdx") } + +END { + if (in_function) + print ".cfi_endproc" +} -- 2.0.0.GIT ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-10-12 18:30 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-12 13:58 [PATCH 1/3] in i386 CFI script, binary ops like ADD or AND modify 2nd operand, not 1st Alex Dowad 2015-10-12 13:58 ` [PATCH 2/3] fix matching errors related to i386 addressing modes in CFI generation script Alex Dowad 2015-10-12 15:12 ` Rich Felker 2015-10-12 18:30 ` Alex 2015-10-12 13:59 ` [PATCH 3/3] add CFI generation script for x86_64 Alex Dowad
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).