[openssl.org #2562] Adding cfi/fpo records to asm (fix backtrace when debugging)

[yoni londner via RT]

Hi,

This patch fix backtrace when using EBP by adding debug record, that allow
the debugger to figure out the backtrace correctly.
source tree: 
openssl-1.0.0d.tar.gz
see
http://old.nabble.com/-PATCH--cfi-fpo-directives-in-md5-assembly-code-td31939273.html

Yoni.

Hi,This patch fix backtrace when using EBP by adding debug record, that allow the debugger to figure out the backtrace correctly.source tree:  openssl-1.0.0d.tar.gz

see http://old.nabble.com/-PATCH--cfi-fpo-directives-in-md5-assembly-code-td31939273.htmlYoni.


cfi-fpo-3.patch
Description: Binary data

Fwd: [PATCH] cfi/fpo directives in md5 assembly code

[yoni londner]

>> So, how can we promote this patch to be commited?
>>

> It is recommended to register the request on request traker at
> http://www.openssl.org/support/rt.html

Ok. I sent an email to r...@openssl.org
Thanks Gilles

Yoni.
__
OpenSSL Project http://www.openssl.org
Development Mailing List   openssl-dev@openssl.org
Automated List Manager   majord...@openssl.org

Re: [PATCH] cfi/fpo directives in md5 assembly code

[yoni londner]

So, how can we promote this patch to be commited?

On Fri, Jul 8, 2011 at 09:45, Wim Lewis  wrote:

> Well, I did some testing with the slightly-modified patch (debian squeeze
> and openbsd 4.9), and confirmed that this produces an .eh_frame which allows
> gdb to walk the stack successfully if the program is stopped in or
> singlestepped through md5_block_asm_data_order(). Some notes, though:
>
> - Not all versions of gas have the .cfi_foo directives. In fact,
> .cfi_sections only showed up in 2.21, and the other directives used here
> were added in 2.15.  Debian's reasonably up-to-date, but OBSD4.9 has 2.15,
> and Apple's dev tools ship with a decade-old 1.x version of gas. So the
> config script will presumably need to be able to partially or completely
> disable CFI generation. On the bright side, the .cfi_sections directive
> isn't actually needed; the default behavior (generating eh_frame but not
> debug_frame) is appropriate.
>
> - x86nasm.pl needed stubs for these directives as well. I have no idea
> what NASM supports in this area so I made both the CFI and FPO directives
> no-ops.
>
> - OpenSSL compiles with -fomit-frame-pointer on both of these machines,
> which means even the gcc-generated code is not backtraceable unless you add
> -funwind-tables. IMHO, the config script should add -funwind-tables unless
> disk space is at a premium (it doesn't affect the generated code) ---
> unwindability is handy to have. And by the same token, if the C code is
> compiled with -fomit-frame-pointer but no unwind tables, then it would make
> sense to leave the unwind information out of the asm code as well.
>
> - The perlasm scripts already keep track of the stack depth (to find args
> and such), which should make it possible to generate a lot of the CFI
> directives automatically. If this patch is accepted I think it's worth
> trying to make the perlasm code handle more CFI generation and then fix up
> the other frame-pointer-less asm routines as well.
>
>
> Here's the slightly-amended patch:
>
>
>

Re: [PATCH] cfi/fpo directives in md5 assembly code

[yoni londner]

> I wasn't proposing that the other changes had to be done now --- just
> noting that the lack of unwind information seems to be a problem that most
> of the assembly files have. I think the extra registers' unwind info for the
> md5 asm is worth including now, though, since it's a tiny enhancement.
>
You are, of course, right, but I do not have the time to add this now.

>
>
> I'll be away for a while, but I'll see if I can test the md5 patches and
> possibly look at some other asm files when I get back.
>
> That would be great.

Re: [PATCH] cfi/fpo directives in md5 assembly code

[yoni londner]

>
>
> I agree entirely, but why not fix the other registers while we're at it?
> I've attached a version of your diff with the extra registers' unwind info
> added--- untested, unfortunately--- it'll also need a
>
>  sub ::cfi_restore { &::emit(".cfi_restore",@_); }
>
> in x86gas.pl and the corresponding stub for MASM.
>
>
> Several of the other assembly files could use the same treatment as well:
> md5-x86_64.pl uses %rbp to point to one of its arguments, sha1-586.pl uses
> %ebp as a scratch register, etc.
>
>
>
I currently do not have the time to continue working on it.
Since I tested my patch carefully, and since it makes the code better, maybe
we can merge it to trunk, and continue to work on your suggestions later on.
Sound good?

Re: [PATCH] cfi/fpo directives in md5 assembly code

[yoni londner]

On Mon, Jun 27, 2011 at 22:20, Wim Lewis  wrote:

>
> On 27 Jun 2011, at 9:27 AM, yoni londner wrote:
> > As you know, on 32bit systems, when using EBP for anything other than
> holding the stack base, it is very difficult to get reasonable backtrace.
> > this can be fixed if directing the compiler to add a debug record which
> tells (at runtime) where we keep EBP value.
> > So, I added this record (FPO in ml.exe and cfi in gcc), and now we can
> debug/get backtrace at runtime.
> > I also fixed source file name, so gdb find's it.
> > Patch is attached (against openssl-1.0.0d.tar.gz), and I hope you will
> merge it to trunk.
>
> This seems like a good thing to fix.
>
> I have some questions/comments:
>
> 1. Would it be better to use ".cfi_startproc simple"? The GAS documentation
> doesn't actually say what opcodes are emitted by cfi_startproc vs. simple
> (and I haven't taken the time to check), but I'd expect a frameless leaf
> procedure like this one not to want the default opcodes emitted for a normal
> procedure. I could be wrong.
>

> 2. We could add .cfi_offset directives for the other callee-saved registers
> as well (EBX, ESI, EDI).
>
>
>
1. I looked at a generated assembler from gcc. So I am not 100% sure what is
the 'most correct', but this is what gcc emits.

2. This is possible, but the frame is the most important. I didn't see those
additional registers being a problem when debugging (the debugger may be
incorrect about values of local variables in parent function).


>
>
> __
> OpenSSL Project http://www.openssl.org
> Development Mailing List   openssl-dev@openssl.org
> Automated List Manager   majord...@openssl.org
>

[PATCH] cfi/fpo directives in md5 assembly code

[yoni londner]

Hi,

As you know, on 32bit systems, when using EBP for anything other than
holding the stack base, it is very difficult to get reasonable backtrace.
this can be fixed if directing the compiler to add a debug record which
tells (at runtime) where we keep EBP value.
So, I added this record (FPO in ml.exe and cfi in gcc), and now we can
debug/get backtrace at runtime.
I also fixed source file name, so gdb find's it.
Patch is attached (against openssl-1.0.0d.tar.gz), and I hope you will merge
it to trunk.

Yoni Londner.


cfi_fpo.diff
Description: Binary data