This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [3/3] unconditionally call via SYMBOL_COMPUTED_OPS
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Tom Tromey <tromey at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 7 Feb 2013 17:32:33 +0100
- Subject: Re: [3/3] unconditionally call via SYMBOL_COMPUTED_OPS
- References: <871udlhzzb.fsf@fleche.redhat.com>
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 <dwarf2_locexpr_funcs>, ops_register = 0xfd7a20 <dwarf2_locexpr_funcs>}, 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 <dwarf2_locexpr_funcs>, ops_register = 0xfd7a20 <dwarf2_locexpr_funcs>}, 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