From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10393 invoked by alias); 18 Jun 2012 09:23:02 -0000 Received: (qmail 10382 invoked by uid 22791); 18 Jun 2012 09:23:01 -0000 X-SWARE-Spam-Status: No, hits=-2.6 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED X-Spam-Check-By: sourceware.org Received: from mel.act-europe.fr (HELO mel.act-europe.fr) (194.98.77.210) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 18 Jun 2012 09:22:44 +0000 Received: from localhost (localhost [127.0.0.1]) by filtered-smtp.eu.adacore.com (Postfix) with ESMTP id DE0D9290011; Mon, 18 Jun 2012 11:22:48 +0200 (CEST) Received: from mel.act-europe.fr ([127.0.0.1]) by localhost (smtp.eu.adacore.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id bWAKHVWKjasi; Mon, 18 Jun 2012 11:22:48 +0200 (CEST) Received: from ulanbator.act-europe.fr (ulanbator.act-europe.fr [10.10.1.67]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by mel.act-europe.fr (Postfix) with ESMTP id CA15C290007; Mon, 18 Jun 2012 11:22:48 +0200 (CEST) Subject: Re: [RFA] Add Windows x64 SEH unwinder Mime-Version: 1.0 (Apple Message framework v1278) Content-Type: text/plain; charset=us-ascii From: Tristan Gingold In-Reply-To: <20120615180604.GW18729@adacore.com> Date: Mon, 18 Jun 2012 09:23:00 -0000 Cc: "gdb-patches@sourceware.org ml" , Kai Tietz Content-Transfer-Encoding: quoted-printable Message-Id: <011965D1-A7E2-46C6-BB87-14EF38EA7E41@adacore.com> References: <20120615180604.GW18729@adacore.com> To: Joel Brobecker X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-06/txt/msg00573.txt.bz2 On Jun 15, 2012, at 8:06 PM, Joel Brobecker wrote: >> This patch adds an unwinder that reads these data. The main advantage >> is that gdb is now able to unwind through code compiled with other >> compilers (and through system libraries). >=20 > I see you beat me to this task, and you didn't even tell me you started > :-). Thanks for doing it! >=20 >> I haven't run the gdb testsuite on Windows x64 (I don't know if this >> is doable), but I have manually tested it on a few executables, >> including gdb itself. >=20 > It would be good if you could test those changes against AdaCore's > testsuite, which we know runs well on x64. It's not as complete as > the official testsuite, but will already test quite a bit, and will > definitely be better than nothing. Ok, will do it. > Because you prepend the unwinder, I think that this unwinder will > take precedence over the DWARF unwinder (right?), and so it's > important we get a minimum amount of confidence before we apply it > (hence the testing suggestion above). We are also trying to get ready > to branch 7.5, and I don't think I would feel all that comfortable > applying this now. [...] >> +/* Try to recognize and decode an epilogue sequence. */ >> + >> +static int >> +x64_frame_decode_epilogue (struct frame_info *this_frame, >> + struct x64_frame_cache *cache) >=20 > I am amazed that one would still need to do manual epilogue > detection and associated by-hand instruction scanning when > using any unwind info. But I assume the info is not complete > enough and there is no other way? No, this is simply part of the specs; but there are only a few patterns to = consider. [...] >> + /* Allow the user to break this loop. */ >> + if (quit_flag) >> + return 0; >=20 > Why not use the QUIT macro? I assume that you want to avoid > the call to the "quit" function, and return 0 instead. But > that seems very odd to me, and maybe even wrong, because you'll > be continuing the unwind even though the user pressed ctrl-c, > with unpredictable results. Ok. [...] >> + for (j =3D 0; ; j++) >> + { >> + struct external_pex64_unwind_info ex_ui; >> + gdb_byte insns[2 * 256]; >=20 > I am a little concerned about this magic number. Can you explain > how you came to it? The multiplication seems to suggest that there > is a logic behind it. I added a comment about that. [...] >> + /* Find the entry. >> + Note: it might be a better idea to ask directly to the kernel. Th= is >> + will handle dynamically added entries (for JIT engines). */ >=20 > I am fine with the current approach, but this makes me wondering > how we would do that, and why we are not doing it? Asking the kernel > from a tdep file means that we'd have to define another xfer object > kind, so that's the first obstacle... Yes, yet another xfer object will do it... >> + if (unwind_info & 1) >=20 > ? I have adjust the comment about this (may not be in the second version just= posted). >> + if (target_read_memory (image_base + (unwind_info & ~1), >> + (char *) &d, sizeof (d)) !=3D 0) > ^^^^^^^^ (gdb_byte *) >=20 > I am realizing I might have missed other cases of casting to > "char *". I fixed all of them (at least in my current tree). Thank you for your extensive review, Tristan