From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9682 invoked by alias); 13 Jun 2002 19:02:59 -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 9477 invoked from network); 13 Jun 2002 19:02:45 -0000 Received: from unknown (HELO kerberos.suse.cz) (195.47.106.10) by sources.redhat.com with SMTP; 13 Jun 2002 19:02:45 -0000 Received: from chimera.suse.cz (chimera.suse.cz [10.20.0.2]) by kerberos.suse.cz (SuSE SMTP server) with ESMTP id CFE0559D351 for ; Thu, 13 Jun 2002 21:02:44 +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 g5DJ2iE08460 for ; Thu, 13 Jun 2002 21:02:44 +0200 X-Authentication-Warning: chimera.suse.cz: Host naga.suse.cz [10.20.1.16] claimed to be suse.cz Message-ID: <3D08EC53.30302@suse.cz> Date: Thu, 13 Jun 2002 12:02: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: GDB Patches Subject: Re: [RFA] dwarf2cfi.c improvements Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2002-06/txt/msg00230.txt.bz2 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? > 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. > 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. > > - 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. > > - 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. 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. > > 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. Michal Ludvig -- * SuSE CR, s.r.o * mludvig@suse.cz * +420 2 9654 5373 * http://www.suse.cz