From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29596 invoked by alias); 11 Jun 2002 15:28:31 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 29569 invoked from network); 11 Jun 2002 15:28:28 -0000 Received: from unknown (HELO kerberos.suse.cz) (195.47.106.10) by sources.redhat.com with SMTP; 11 Jun 2002 15:28:28 -0000 Received: from chimera.suse.cz (chimera.suse.cz [10.20.0.2]) by kerberos.suse.cz (SuSE SMTP server) with ESMTP id 5DA0059D354; Tue, 11 Jun 2002 17:28:28 +0200 (CEST) Received: from suse.cz (naga.suse.cz [10.20.1.16]) by chimera.suse.cz (8.11.0/8.11.0/SuSE Linux 8.11.0-0.4) with ESMTP id g5BFSSE11034; Tue, 11 Jun 2002 17:28:28 +0200 X-Authentication-Warning: chimera.suse.cz: Host naga.suse.cz [10.20.1.16] claimed to be suse.cz Message-ID: <3D06171B.3040709@suse.cz> Date: Tue, 11 Jun 2002 08:28:00 -0000 From: Michal Ludvig Organization: SuSE CR User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.0) Gecko/20020529 X-Accept-Language: cs, cz, en MIME-Version: 1.0 To: Elena Zannoni Cc: Andrew Cagney , GDB Patches Subject: Re: [RFA] dwarf2cfi.c improvements References: <3CF79DA0.9040404@suse.cz> <3CFB727E.10005@cygnus.com> <3CFCA5ED.7010201@suse.cz> <15621.27804.562409.937859@localhost.redhat.com> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2002-06/txt/msg00173.txt.bz2 Elena Zannoni wrote: > Michal Ludvig writes: > > > > Well, this is the patch without unrelated message changes. Can I commit > > it? It seems to be quite stable in our everyday use... > > > > Michal, sorry, not as is. No problem, I'll rework it. > There are coding standard violations having to do with spaces before > '(' and after ')' in casts. Also, it is better to not have multiple > initializations in a single line, like this: > > ! char *start = NULL, *end = NULL, *frame_buffer = NULL; Changed. I've reindented the whole file. I hope running 'indent dwarf2cfi.c' is enough. > Conditional expressions are also flagged by the ARI. What does 'AR Index' mean and how do I run the checker? > Can I suggest that you split the pcrel changes into a separate patch? Well, I wouldn't like to. When you start parsing .eh_frame you need a working pcrel, otherwise it is useless. But without parsing .eh_frame you can't verify whether it works, because .debug_frame doesn't use relative addressing. May I ask you to approve both at once, please? > About pcrel, I don't like the use of the flag like you have below. > Could you find some other way to do it w/o passing the address of the > flag around? Could you maybe change the function to return 0/1 and > have the current return value as an out parameter instead? That is possible, but it would make read_encoded_pointer() behave differently from read_pointer(). I'll better introduce a new function like pointer_type() that would return an enum of possible types. > Or alternatively, could you have the function read_encoded_pointer > emit the warning itself? In some cases PC-relative addressing is valid, but sometimes it is not (or at least is not used). The warning is there only once. > After all, none of > the other error/warning messages are that detailed, they don't print > the PC value. I noticed that in a couple of the calls to > read_encoded_pointer the code doesn't care about emitting the warning, > why? Or add a pc parameter for the purpose of the warning. There are four occurences: - In DW_CFA_set_loc I don't (yet) handle pcrel addressing, because I have never seen it in .*_frame. That's why I print a warning - If I'll ever see it I'll implement support for it. - When reading pers_addr I don't care about the return value at all. I just need to advance the pointer to data. - Init_loc reading takes care of pcrel addressing. - fde->address_range is always an absolute value, but encoded in a pointer format. I don't care if it is in pcrel format or not - it's just a difference between two pointers. There are no other calls to read_encoded_pointer() in dwarf2cfi.c > About the eh_frame part of the patch. > I don't really like the use of eh_frame as you have in dwarf2_build_frame_info. > Could you try to figure out a simpler way to deal with the two cases? > You could use an enumeration for eh_frame. Well, yes, actually. But then I would have to change: if(eh_frame) ... to if(frame == EH_ONLY || frame == EH_SECOND) ... What about making local macros IS_DEBUG_FRAME(), IS_EH_FRAME(), IS_EH_FRAME_ONLY(), ...? > Could you do 2 passes, one for each section? Hm? I don't understand what you mean... > I am not able to apply your patch, which I usually do, to try a test > run. Is there anyway you can regenerate the patch? Maybe with a unified > diff instead? (it's just that I find it easier to read) > I would like to give you more intelligent comments about the eh_frame > part, but I need to apply the patch first. The cfi-huge5.diff was made against clean rev 1.7 of dwarf2cfi.c file. For me it applies cleanly against this revision. Michal Ludvig -- * SuSE CR, s.r.o * mludvig@suse.cz * +420 2 9654 5373 * http://www.suse.cz