From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32198 invoked by alias); 23 Feb 2009 18:12:50 -0000 Received: (qmail 32189 invoked by uid 22791); 23 Feb 2009 18:12:49 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,SARE_MSGID_LONG40,SPF_PASS X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (216.239.33.17) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 23 Feb 2009 18:12:44 +0000 Received: from spaceape10.eur.corp.google.com (spaceape10.eur.corp.google.com [172.28.16.144]) by smtp-out.google.com with ESMTP id n1NICf5k004548 for ; Mon, 23 Feb 2009 18:12:41 GMT Received: from wf-out-1314.google.com (wfc28.prod.google.com [10.142.3.28]) by spaceape10.eur.corp.google.com with ESMTP id n1NICWVZ008955 for ; Mon, 23 Feb 2009 10:12:39 -0800 Received: by wf-out-1314.google.com with SMTP id 28so2539970wfc.17 for ; Mon, 23 Feb 2009 10:12:38 -0800 (PST) MIME-Version: 1.0 Received: by 10.142.144.16 with SMTP id r16mr2079958wfd.349.1235412758879; Mon, 23 Feb 2009 10:12:38 -0800 (PST) In-Reply-To: <20090223010759.GA30997@adacore.com> References: <20090205030257.8A6073A6B7A@localhost> <8ac60eac0902061837p5885b812j8a26669e799702e1@mail.gmail.com> <8ac60eac0902181458g39dfbce9k63c3329528b0aad5@mail.gmail.com> <20090223010759.GA30997@adacore.com> Date: Mon, 23 Feb 2009 18:36:00 -0000 Message-ID: <8ac60eac0902231012lb42bcb1q8b8cf19ad2ac192@mail.gmail.com> Subject: Re: [patch] Fix a crash when displaying variables from shared library. From: Paul Pluzhnikov To: Joel Brobecker Cc: Tom Tromey , Pedro Alves , gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-System-Of-Record: true 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: 2009-02/txt/msg00448.txt.bz2 On Sun, Feb 22, 2009 at 5:07 PM, Joel Brobecker wrote: >> + /* Can't re-parse the expression. Disable this display item. */ > > Minor style issue: You need to have two spaces after each period > (one in the middle, and one at the end, before the "*/"). Done. >> +/* Answer 1 if "d" uses "solib" (and will become dangling when "solib" >> + is unloaded), otherwise answer 0. */ > > If you don't mind, I think using "Return 1" instead of "Answer 1" would > be more consistent with the other descriptions. Done. > Another minor style correction: In GDB, we refer to the function > parameters by using their names in ALL_CAPS, and without the quotes. > So, in your case, you would write: > > /* Return 1 if D uses SOLIB (and will become dangling [...] */ Done. >> + if (d->block != NULL >> + && addr_low <= d->block->startaddr && d->block->endaddr <= addr_high) >> + return 1; > > I suggest you use solib_address instead of doing the check yourself. > As mentioned by Daniel in another thread, shared libraries on SVR4 > systems occupy a contiguous address block, but this is not the case > of DLLs where the data and text sections might be separate. > I verified that solib_address should handle the DLL case. Done. >> + for (i = 0; i < d->exp->nelts; i++) >> + { >> + union exp_element *elts = d->exp->elts; >> + if (elts[i].opcode == OP_VAR_VALUE) > > I'm afraid this isn't going to work for more complex structures... > The problem is that you might be reading an undefined field of > union exp_element. Imagine for instance that you have an expression > that looks like this: "foo->bar". > > At one point, you'll encounter the following elements: > > [i ] -> STRUCTOP_PTR > [i+1] -> A string > [i+2] -> STRUCTOP_PTR > > Iterating over the expression, you'll ignore the element at index i, > and then check the opcode of the element at i+1, which is the wrong > field of the enum to access in this case... I was afraid of that ... > I can't think of a way of doing what you're trying to do off the top > of my head. I'll have to think about it a little more. Perhaps others > will have suggestions... Or perhaps we'll have to attack the problem > with a different angle, I'm not very familiar with how "display" > expressions are handled... One way I think this could be fixed is to refactor e.g. dump_subexp_body_standard() to use "visitor pattern", and then use a different callback for display_uses_solib_p(). Perhaps refactoring should be done first as a separate patch? A simpler way is to cut/paste/modify dump_subexp_body_standard(), but there is (IMHO) already too much code duplication between it and e.g. evaluate_subexp_standard(), so I'd rather not introduce a 3rd copy. Another way would be to replace malloc()/realloc()s in parse_exp_in_context() with calloc()s, so that all "unused" .opcode's become OP_NULLs. This appears tricky, and there are language hooks which may need similar fixes. >> +gdb_test "run" "3: c_global = 43\\r\\nwarning: .*b_global.*\\r\\n1: a_global = 41" "after rebuild" > > Can this be changed to use either one of the runto routines, or maybe > gdb_run_cmd if one of the above doesn't work in this case? Note that this test is skipped when "is_remote target". Is there still a need to use runto? Is it better to use runto and *not* skip the test for remote targets? >> +gdb_test "run" "6: a_static = 46\\r\\n4: main_global = 44\\r\\n.*" > > Same here. Thanks, -- Paul Pluzhnikov