Re: [3/3] unconditionally call via SYMBOL_COMPUTED_OPS

[Jan Kratochvil <jan.kratochvil@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