From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18397 invoked by alias); 7 Feb 2013 16:32:47 -0000 Received: (qmail 18355 invoked by uid 22791); 7 Feb 2013 16:32:43 -0000 X-SWARE-Spam-Status: No, hits=-5.4 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_DB,TW_XF X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 07 Feb 2013 16:32:38 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r17GWbYd022866 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 7 Feb 2013 11:32:38 -0500 Received: from host2.jankratochvil.net (ovpn-116-18.ams2.redhat.com [10.36.116.18]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r17GWXBZ021397 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 7 Feb 2013 11:32:36 -0500 Date: Thu, 07 Feb 2013 16:32:00 -0000 From: Jan Kratochvil To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [3/3] unconditionally call via SYMBOL_COMPUTED_OPS Message-ID: <20130207163233.GA15297@host2.jankratochvil.net> References: <871udlhzzb.fsf@fleche.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <871udlhzzb.fsf@fleche.redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) 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: 2013-02/txt/msg00180.txt.bz2 On Wed, 16 Jan 2013 17:08:56 +0100, Tom Tromey wrote: > There are some FIXME comments scattered about the code concerning the > unconditional use of SYMBOL_COMPUTED_OPS. > > I think the idea here was that we could remove LOC_COMPUTED entirely, > and just have a computed ops vector alongside some other LOC_* constant. > > I didn't go quite that far, but I made it so that if the symbol has > computed ops, they are called unconditionally. Then I removed the > FIXMEs. > > Patch #2 fixed the DWARF frame base issue mentioned in these comments by > adding the "reader datum" form of synthetic address classes. I did this > in patch #2, and not here, because splitting this differently made the > patches uglier -- introducing new code only to delete it again. This patch removes comments which are still valid even after this patch. And this patch 3/3 IMO makes it worse by using SYMBOL_COMPUTED_OPS without checking SYMBOL_CLASS first, it is rather a "reverse-cleanup" patch. I had problems cleaning up patch 2/2 now because issues brought in by this patch 3/3 and I find this patch 3/3 is the inappropriate one. The problem is that functions have no real DW_AT_location. They have only DW_AT_low_pc + DW_AT_high_pc (or other variants we know), these are stored in SYMBOL_BLOCK_VALUE thanks to LOC_BLOCK. That would be complete and fine. But there is additionally SYMBOL_COMPUTED_OPS and SYMBOL_LOCATION_BATON present there which make no sense there, the location is already complete by SYMBOL_BLOCK_VALUE+LOC_BLOCK. SYMBOL_COMPUTED_OPS+SYMBOL_LOCATION_BATON there describe DW_AT_frame_base but that is just one of the many additional attributes. (gdb) p *((struct symtab *) 0x22d0f80).blockvector.block[2].function $1 = {ginfo = {name = 0x22b4dbd "main", value = {ivalue = 36507008, block = 0x22d0d80, bytes = 0x22d0d80 "\360\004@", address = 36507008, common_block = 0x22d0d80, chain = 0x22d0d80}, language_specific = {mangled_lang = {demangled_name = 0x0}, cplus_specific = 0x0}, language = language_c, section = 0, obj_section = 0x0}, type = 0x22c7a70, symtab = 0x22d0f80, domain = VAR_DOMAIN, aclass = LOC_BLOCK, is_argument = 0, is_inlined = 0, is_cplus_template_function = 0, line = 1, ops = {ops_computed = 0xfd7a20 , ops_register = 0xfd7a20 }, aux_value = 0x22d0cd0, hash_next = 0x0} <1><2d>: Abbrev Number: 2 (DW_TAG_subprogram) <2e> DW_AT_name : (indirect string, offset: 0x7c): main <34> DW_AT_type : <0x5b> <38> DW_AT_low_pc : 0x4004f0 <40> DW_AT_high_pc : 0x10 <48> DW_AT_frame_base : 1 byte block: 9c (DW_OP_call_frame_cfa) When you compare it with an autovariable it uses LOC_COMPUTED, therefore symbol.ginfo.value is unused. And there is correctly (because it is LOC_COMPUTED) used SYMBOL_COMPUTED_OPS+SYMBOL_LOCATION_BATON which really describe location of the autovariable itself. (gdb) p *((struct symtab *) 0x22d0f80).blockvector.block[2].function.ginfo.value.block.dict.data.linear.syms[0] $6 = {ginfo = {name = 0x22c050f "i", value = {ivalue = 0, block = 0x0, bytes = 0x0, address = 0, common_block = 0x0, chain = 0x0}, language_specific = { mangled_lang = {demangled_name = 0x0}, cplus_specific = 0x0}, language = language_c, section = 0, obj_section = 0x0}, type = 0x22ccb00, symtab = 0x22d0f80, domain = VAR_DOMAIN, aclass = LOC_COMPUTED, is_argument = 0, is_inlined = 0, is_cplus_template_function = 0, line = 1, ops = { ops_computed = 0xfd7a20 , ops_register = 0xfd7a20 }, aux_value = 0x22d0d60, hash_next = 0x0} <2><4e>: Abbrev Number: 3 (DW_TAG_variable) <4f> DW_AT_name : i <53> DW_AT_type : <0x5b> <57> DW_AT_location : 2 byte block: 91 6c (DW_OP_fbreg: -20) We could talk differently if SYMBOL_COMPUTED_OPS would apply for all LOC_*, the full virtualization. But that would require resolving first the inappropriate reuse of SYMBOL_COMPUTED_OPS for DW_AT_frame_base in function symbols. Without the full virtualization / unconditional calls via SYMBOL_COMPUTED_OPS I do not understand this patch and I am against it. Thanks, Jan