From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 77252 invoked by alias); 26 Apr 2017 23:09:50 -0000 Mailing-List: contact elfutils-devel-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Post: List-Help: List-Subscribe: Sender: elfutils-devel-owner@sourceware.org Received: (qmail 75737 invoked by uid 89); 26 Apr 2017 23:09:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.99.2 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY autolearn=ham version=3.3.2 spammy=loose, 9010 X-Spam-Status: No, score=-24.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY autolearn=ham version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on sourceware.org X-Spam-Level: X-HELO: gnu.wildebeest.org Message-ID: <1493248187.31726.92.camel@klomp.org> Subject: Re: [PATCH 5/5] Add frame pointer unwinding for aarch64 From: Mark Wielaard To: Ulf Hermann Cc: elfutils-devel@sourceware.org Date: Thu, 27 Apr 2017 14:29:00 -0000 In-Reply-To: <14b0cd1d-5737-2c7a-3fab-f197011c7fc6@qt.io> References: <1493124006.31726.33.camel@klomp.org> <1493124579-21017-1-git-send-email-mark@klomp.org> <1493124579-21017-5-git-send-email-mark@klomp.org> <1493125881.31726.44.camel@klomp.org> <3b0d6718-cf17-9ae1-b5f7-8c6413b8d3d2@qt.io> <1493217200.31726.59.camel@klomp.org> <14b0cd1d-5737-2c7a-3fab-f197011c7fc6@qt.io> Content-Type: multipart/mixed; boundary="=-4HzNtCCW8NjXsLGacN0f" X-Mailer: Evolution 3.12.11 (3.12.11-22.el7) Mime-Version: 1.0 X-SW-Source: 2017-q2/txt/msg00103.txt.bz2 --=-4HzNtCCW8NjXsLGacN0f Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-length: 2606 On Wed, 2017-04-26 at 17:27 +0200, Ulf Hermann wrote: > However, if you strip .eh_frame and .eh_frame_hdr from the exe (thus > triggering the fp unwinding on the first frame), you will see that it > skips sigusr2. At the same time it invents another frame 0x403f40 on > the main thread. Apparently pthread_join creates two stack frames. As > it correctly unwinds the rest, the latter seemed harmless to me. I am a little concerned about testing against an exec where .eh_frame is forcibly removed since that is an allocated section you are messing up the binary (which shows because the symbol table doesn't match anymore). It seems nicer to do the checks instead with a hacked up libdwfl/frame_unwind.c that simply doesn't handle cfi and so always uses the frame pointer unwinder: diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c index fb42c1a..6de64e5 100644 --- a/libdwfl/frame_unwind.c +++ b/libdwfl/frame_unwind.c @@ -539,6 +539,7 @@ new_unwound (Dwfl_Frame *state) static void handle_cfi (Dwfl_Frame *state, Dwarf_Addr pc, Dwarf_CFI *cfi, Dwarf_Addr b= ias) { + return; Dwarf_Frame *frame; if (INTUSE(dwarf_cfi_addrframe) (cfi, pc, &frame) !=3D 0) { You are right that in that case we loose/skip over sigusr2 from raise and end up at stdarg directly if we remove the pc & 0x1 check. But... that really is because we deliberately skip it. Proper/simple link-register/frame unwinding should say: - newPc =3D newLr & (~0x1); + newPc =3D lr; The newPc is the current link register, not the new one. With that we get the backtrace as expected. But... I now realize why you needed something like that in the case of mixed CFI/no-framepointer/no-CFI/framepointer code. Like we have in our testcase. In that case there is no good way to determine whether or not there really were proper frame pointers and/or how the previous frame was unwound. Our testcase is somewhat mean by using some signal/no-return code which, which is hard to properly unwind without full frame pointers or full CFI. And with the simpler code that doesn't try to guess whether or not to skip a frame you do end up with an extra siguser2 and/or main frame. We could try to be clever and realize the link register and pc are the same and then use the newLR instead as newPC. That however might just mean that it is a recursive call to the same function. So maybe the proper "fix" for that is to make our testcase a little less strict and allow the occasional extra frame instead of trying to make the frame pointer unwinder "extra smart". Maybe something like the attached patch? Cheers, Mark --=-4HzNtCCW8NjXsLGacN0f Content-Disposition: inline; filename="simple_fp_unwind.patch" Content-Type: text/x-patch; name="simple_fp_unwind.patch"; charset="UTF-8" Content-Transfer-Encoding: base64 Content-length: 4108 ZGlmZiAtLWdpdCBhL2JhY2tlbmRzL2FhcmNoNjRfdW53aW5kLmMgYi9iYWNr ZW5kcy9hYXJjaDY0X3Vud2luZC5jCmluZGV4IGNhYzRlYmQuLjE4YWFmOWEg MTAwNjQ0Ci0tLSBhL2JhY2tlbmRzL2FhcmNoNjRfdW53aW5kLmMKKysrIGIv YmFja2VuZHMvYWFyY2g2NF91bndpbmQuYwpAQCAtNjEsMjYgKzYxLDE1IEBA IEVCTEhPT0sodW53aW5kKSAoRWJsICplYmwgX19hdHRyaWJ1dGVfXyAoKHVu dXNlZCkpLCBEd2FyZl9BZGRyIHBjIF9fYXR0cmlidXRlX18KIAogICBEd2Fy Zl9Xb3JkIG5ld1BjLCBuZXdMciwgbmV3RnAsIG5ld1NwOwogCi0gIC8vIFRo ZSBpbml0aWFsIGZyYW1lIGlzIHNwZWNpYWwuIFdlIGFyZSBleHBlY3RlZCB0 byByZXR1cm4gbHIgZGlyZWN0bHkgaW4gdGhpcyBjYXNlLCBhbmQgd2UnbGwK LSAgLy8gY29tZSBiYWNrIHRvIHRoZSBzYW1lIGZyYW1lIGFnYWluIGluIHRo ZSBuZXh0IHJvdW5kLgotICBpZiAoKHBjICYgMHgxKSA9PSAwKQotICAgIHsK LSAgICAgIG5ld0xyID0gbHI7Ci0gICAgICBuZXdGcCA9IGZwOwotICAgICAg bmV3U3AgPSBzcDsKLSAgICB9Ci0gIGVsc2UKLSAgICB7Ci0gICAgICBpZiAo IXJlYWRmdW5jKGZwICsgTFJfT0ZGU0VULCAmbmV3THIsIGFyZykpCi0gICAg ICAgIG5ld0xyID0gMDsKLQotICAgICAgaWYgKCFyZWFkZnVuYyhmcCArIEZQ X09GRlNFVCwgJm5ld0ZwLCBhcmcpKQotICAgICAgICBuZXdGcCA9IDA7Ci0K LSAgICAgIG5ld1NwID0gZnAgKyBTUF9PRkZTRVQ7Ci0gICAgfQotCi0gIG5l d1BjID0gbmV3THIgJiAofjB4MSk7CisgIGlmICghcmVhZGZ1bmMoZnAgKyBM Ul9PRkZTRVQsICZuZXdMciwgYXJnKSkKKyAgICBuZXdMciA9IDA7CisKKyAg aWYgKCFyZWFkZnVuYyhmcCArIEZQX09GRlNFVCwgJm5ld0ZwLCBhcmcpKQor ICAgIG5ld0ZwID0gMDsKKworICBuZXdTcCA9IGZwICsgU1BfT0ZGU0VUOwor CisgIG5ld1BjID0gbHI7CiAgIGlmICghc2V0ZnVuYygtMSwgMSwgJm5ld1Bj LCBhcmcpKQogICAgIHJldHVybiBmYWxzZTsKIApAQCAtOTEsNiArODAsNSBA QCBFQkxIT09LKHVud2luZCkgKEVibCAqZWJsIF9fYXR0cmlidXRlX18gKCh1 bnVzZWQpKSwgRHdhcmZfQWRkciBwYyBfX2F0dHJpYnV0ZV9fCiAKICAgLy8g SWYgdGhlIGZwIGlzIGludmFsaWQsIHdlIG1pZ2h0IHN0aWxsIGhhdmUgYSB2 YWxpZCBsci4KICAgLy8gQnV0IGlmIHRoZSBmcCBpcyB2YWxpZCwgdGhlbiB0 aGUgc3RhY2sgc2hvdWxkIGJlIG1vdmluZyBpbiB0aGUgcmlnaHQgZGlyZWN0 aW9uLgotICAvLyBFeGNlcHQsIGlmIHRoaXMgaXMgdGhlIGluaXRpYWwgZnJh bWUuIFRoZW4gdGhlIHN0YWNrIGRvZXNuJ3QgbW92ZS4KLSAgcmV0dXJuIG5l d1BjICE9IDAgJiYgKGZwID09IDAgfHwgbmV3U3AgPiBzcCB8fCAocGMgJiAw eDEpID09IDApOworICByZXR1cm4gbmV3UGMgIT0gMCAmJiAoZnAgPT0gMCB8 fCBuZXdTcCA+IHNwKTsKIH0KZGlmZiAtLWdpdCBhL3Rlc3RzL2JhY2t0cmFj ZS1zdWJyLnNoIGIvdGVzdHMvYmFja3RyYWNlLXN1YnIuc2gKaW5kZXggYTMw M2UzMi4uOTczMWM0MyAxMDA2NDQKLS0tIGEvdGVzdHMvYmFja3RyYWNlLXN1 YnIuc2gKKysrIGIvdGVzdHMvYmFja3RyYWNlLXN1YnIuc2gKQEAgLTU5LDcg KzU5LDcgQEAgY2hlY2tfYmFja3RyYWNlZ2VuKCkKICMgSWdub3JlIGl0IGhl cmUgYXMgaXQgaXMgYSBidWcgb2YgT1MsIG5vdCBhIGJ1ZyBvZiBlbGZ1dGls cy4KIGNoZWNrX2VycigpCiB7Ci0gIGlmIFsgJChlZ3JlcCAtdiA8JDEgJ2R3 ZmxfdGhyZWFkX2dldGZyYW1lczogKE5vIERXQVJGIGluZm9ybWF0aW9uIGZv dW5kfG5vIG1hdGNoaW5nIGFkZHJlc3MgcmFuZ2V8YWRkcmVzcyBvdXQgb2Yg cmFuZ2UpJCcgXAorICBpZiBbICQoZWdyZXAgLXYgPCQxICdkd2ZsX3RocmVh ZF9nZXRmcmFtZXM6IChObyBEV0FSRiBpbmZvcm1hdGlvbiBmb3VuZHxubyBt YXRjaGluZyBhZGRyZXNzIHJhbmdlfGFkZHJlc3Mgb3V0IG9mIHJhbmdlfElu dmFsaWQgcmVnaXN0ZXIpJCcgXAogICAgICAgICAgfCB3YyAtYykgXAogICAg ICAgIC1lcSAwIF0KICAgdGhlbgpkaWZmIC0tZ2l0IGEvdGVzdHMvYmFja3Ry YWNlLmMgYi90ZXN0cy9iYWNrdHJhY2UuYwppbmRleCAxZmY2MzUzLi4yMWFi ZThhIDEwMDY0NAotLS0gYS90ZXN0cy9iYWNrdHJhY2UuYworKysgYi90ZXN0 cy9iYWNrdHJhY2UuYwpAQCAtOTAsNiArOTAsMTAgQEAgY2FsbGJhY2tfdmVy aWZ5IChwaWRfdCB0aWQsIHVuc2lnbmVkIGZyYW1lbm8sIER3YXJmX0FkZHIg cGMsCiAgICAgICByZXR1cm47CiAgICAgfQogICBEd2ZsX01vZHVsZSAqbW9k OworICAvKiBTZWUgY2FzZSA0LiBTcGVjaWFsIGNhc2UgdG8gaGVscCBvdXQg c2ltcGxlIGZyYW1lIHBvaW50ZXIgdW53aW5kZXJzLiAqLworICBzdGF0aWMg Ym9vbCBkdXBsaWNhdGVfc2lndXNyMiA9IGZhbHNlOworICBpZiAoZHVwbGlj YXRlX3NpZ3VzcjIpCisgICAgZnJhbWVuby0tOwogICBzdGF0aWMgYm9vbCBy ZWR1Y2VfZnJhbWVubyA9IGZhbHNlOwogICBpZiAocmVkdWNlX2ZyYW1lbm8p CiAgICAgZnJhbWVuby0tOwpAQCAtMTI1LDYgKzEyOSwxNCBAQCBjYWxsYmFj a192ZXJpZnkgKHBpZF90IHRpZCwgdW5zaWduZWQgZnJhbWVubywgRHdhcmZf QWRkciBwYywKIAl9CiAgICAgICAvKiBGQUxMVEhSVSAqLwogICAgIGNhc2Ug NDoKKyAgICAgIC8qIFNvbWUgc2ltcGxlIGZyYW1lIHVud2luZGVycyBnZXQg dGhpcyB3cm9uZyBhbmQgdGhpbmsgc2lndXNyMgorCSBpcyBjYWxsaW5nIGl0 c2VsZiBhZ2Fpbi4gQWxsb3cgaXQgYW5kIGp1c3QgcHJldGVuZCB0aGVyZSBp cworCSBhbiBleHRyYSBzaWd1c3IyIGZyYW1lLiAqLworICAgICAgaWYgKHN5 bW5hbWUgIT0gTlVMTCAmJiBzdHJjbXAgKHN5bW5hbWUsICJzaWd1c3IyIikg PT0gMCkKKwl7CisJICBkdXBsaWNhdGVfc2lndXNyMiA9IHRydWU7CisJICBi cmVhazsKKwl9CiAgICAgICBhc3NlcnQgKHN5bW5hbWUgIT0gTlVMTCAmJiBz dHJjbXAgKHN5bW5hbWUsICJzdGRhcmciKSA9PSAwKTsKICAgICAgIGJyZWFr OwogICAgIGNhc2UgNToK --=-4HzNtCCW8NjXsLGacN0f--