From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30854 invoked by alias); 19 Jun 2002 13:48:29 -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 30787 invoked from network); 19 Jun 2002 13:48:22 -0000 Received: from unknown (HELO cygnus.com) (205.180.83.203) by sources.redhat.com with SMTP; 19 Jun 2002 13:48:22 -0000 Received: from localhost.redhat.com (remus.sfbay.redhat.com [172.16.27.252]) by runyon.cygnus.com (8.8.7-cygnus/8.8.7) with ESMTP id GAA17691 for ; Wed, 19 Jun 2002 06:48:21 -0700 (PDT) Received: by localhost.redhat.com (Postfix, from userid 469) id E86E4107D4; Wed, 19 Jun 2002 09:47:39 -0400 (EDT) From: Elena Zannoni MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <15632.35707.805336.444363@localhost.redhat.com> Date: Wed, 19 Jun 2002 06:48:00 -0000 To: Michal Ludvig Cc: gdb-patches@sources.redhat.com Subject: Re: [RFA] dwarf2cfi.c improvements In-Reply-To: <3D08EB06.9030200@suse.cz> References: <3CF79DA0.9040404@suse.cz> <3CFB727E.10005@cygnus.com> <3CFCA5ED.7010201@suse.cz> <15621.27804.562409.937859@localhost.redhat.com> <3D06171B.3040709@suse.cz> <15622.25798.905178.867470@localhost.redhat.com> <3D08EB06.9030200@suse.cz> X-SW-Source: 2002-06/txt/msg00363.txt.bz2 Michal Ludvig writes: > Elena Zannoni wrote: > > Michal Ludvig writes: > > > > 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. > > > > OK. But still, the warning should come from inside the function > > itself. > > I have introducend new function pointer_encoding() that the function > calling read_encoded_pointer() may invoke to see what was returned and > how to handle it. I'm convinced, that it's the caller's responsibility > to deal with different PE's (pointer encodings), because sometimes some > of them are valid, sometimes not. If I'd just pass base_addr into the > read_enc_ptr() how would I know whether it was handled correctly. There > might be a situation, that the pointer would be encoded either in pcrel > or in funcrel format - then I should decide afterwards how to compute > the base address, shouldn't I? > Much better. This makes the code a bit easier to understand. The previous version of the function was trying to do too much. > > I was thinking you needed to do it outside, to have the > > fs->pc available to be printed in the warning, but that value actually > > comes from inside the function itself, so it doesn't need to be separate. > > > > > - When reading pers_addr I don't care about the return value at all. I > > > just need to advance the pointer to data. > > > > Actually, do you need to call this function at all, if all you need to > > do is eat up some data? This is really overloading the function. > > Could you just call one of the read_* functions? > > No, because it depends on PE how much data should I eat up. I can have > another function that would give me the number of bytes occupied by a > given PE, but I don't believe it's a good idea. It's not called that > often, that it could speed things up. > No need anymore, with the pointer_encoding function. > > But I see that gcc does > > some similar trick. BTW that function deals with alignment and the gdb > > version doesn't (yet?). > > Hmm I didn't notice. I'll look at it. But it works as it is. > Keep it in mind if you see strange behaviors. > > > - Init_loc reading takes care of pcrel addressing. > > > > Yes, the bit that does it though, was inside read_encoded_pointer, and > > now it's not, and you also need to do an additional check, so I would > > prefer doing it as it was before. > > It wasn't correct, anyway. I have written my reasons against this > approach above. > > > Could you instead add a new parmeter *base* instead of the flag? In > > the DW_CFA_set_loc case, base would be NULL while here it would be > > base_offset, so you could still distinguish the two cases. > > How can I know how to compute base_addr when I don't know whether the > encoding is pcrel, textrel, funcrel, ...? I know these aren't used _now_ > but at least hypothetically are possible. > Right. OK. > > > - 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. > > > > So that would work anyway, right? > > I must only know in how many bytes it's encoded so that I advance the > data pointer correctly. The value itself, either if encoded as pcrel, is > always absolute. At least I hope so :-) > > > > I don't undersand this change: > > *** 530,541 **** > > } > > > > if (ret != 0) > > ! switch (encoding & 0xf0) > > { > > case DW_EH_PE_absptr: > > break; > > case DW_EH_PE_pcrel: > > ! ret += (CORE_ADDR) *p; > > break; > > case DW_EH_PE_textrel: > > case DW_EH_PE_datarel: > > --- 531,543 ---- > > } > > > > if (ret != 0) > > ! { > > ! switch (encoding & 0x70) > > { > > case DW_EH_PE_absptr: > > break; > > case DW_EH_PE_pcrel: > > ! if (flag_pcrel) *flag_pcrel = 1; > > break; > > case DW_EH_PE_textrel: > > case DW_EH_PE_datarel: > > *************** read_encoded_pointer (bfd *abfd, char ** > > *** 544,549 **** > > --- 546,555 ---- > > internal_error (__FILE__, __LINE__, > > "read_encoded_pointer: unknown pointer encoding"); > > } > > + > > + if (encoding & DW_EH_PE_indirect) > > + warning ("CFI: Unsupported pointer encoding: DW_EH_PE_indirect"); > > + } > > > > return ret; > > } > > > > Why are you changing the mask from 0xf0 to 0x70 in the switch to avoid > > catching DW_EH_PE_indirect? Could you just add it to the case statement? > > I can't because DW_EH_PE_indirect is independent on DW_EH_PE_*rel. Both > PE_pcrel and PE_pcrel_indirect are possible. In fact, encoding variable > contains three flags: size, encoding and (in)direct flag. > Ah, I see. 3 masks for 3 different things. Is there any way you can add a comment about this? > All of them must be handled separately. > > > > > 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) ... > > > > > > > That is the idea, yes. It would be much more understandable to the > > reader. > > I have changed computing of this parameter in dwarf2_build_frame_info so > that it is more readable now, but I'd leave 'eh_frame' as it is, because > in most cases I don't care whether it's 1 or 2. The hack with > eh_frame==2 is there just for speeding up things when looking for > duplicate FDEs. I eh_frame is the only section parsed, then I don't have > to be afraid that we already have a given FDE and don't have to check it > at all. > Could you add a comment explaining the differences between the eh_frame section and the debug_frame section? Only a couple, as far as I know. > > > What about making local macros IS_DEBUG_FRAME(), IS_EH_FRAME(), > > > IS_EH_FRAME_ONLY(), ...? > > > > > > > How would those behave different from enums? If I am understanding your > > suggestion correctly, that is (macros are usually not introduced > > unless absolutely necessary). > > IS_EH_FRAME would be true for both possible cases with eh_frame. > As I already said - usually I don't care whether it's parsed first or > after debug_frame. > > > Could you send me a unified diff? (don't need to send it to the list > > just to me). I'd appreciate that. > > Yes. Included. It's against cvs rev 1.9. > A few little things: > + > +enum ptr_encoding > +pointer_encoding (unsigned char encoding) > +{ > + int ret; > > + if (encoding & DW_EH_PE_indirect) > + warning ("CFI: Unsupported pointer encoding: DW_EH_PE_indirect"); > + > + switch (encoding & 0x70) > + { > + case DW_EH_PE_absptr: > + case DW_EH_PE_pcrel: > + case DW_EH_PE_textrel: > + case DW_EH_PE_datarel: > + case DW_EH_PE_funcrel: > + ret = encoding & 0x70; > + break; > + default: > + internal_error (__FILE__, __LINE__, > + "read_encoded_pointer: unknown pointer encoding"); The function name should be pointer_encoding. > +static void > +parse_frame_info (struct objfile *objfile, file_ptr frame_offset, > + unsigned int frame_size, int eh_frame) > { > bfd *abfd = objfile->obfd; > + asection *curr_section_ptr; > char *start = NULL; > char *end = NULL; > - int from_eh = 0; > + char *frame_buffer = NULL; > + char *curr_section_name, *aug_data; > + struct cie_unit *last_cie = NULL; > + int last_dup_fde = 0, aug_len, i; Please put these on separate lines. > + if (eh_frame && > + (cie->offset == > + (unit_offset + bytes_read - cie_id))) Please reformat this line. > + for (i = 0; eh_frame == 2 && i < fde_chunks.elems; i++) Could you include the for loop in an if statement, instead of adding the eh_frame==2 test? > + int after_debug_frame=0; Spaces. ChangeLog? Other than that approved. Elena