Re: [patch rfc] Eliminate extract_address

[Andrew Cagney <ac131313@redhat.com>]

> On May 21, 12:19pm, Andrew Cagney wrote:
> 
> 
>> Kevin?
>> 
>> The other option would be deprecate it, but I'd prefer not to as when 
>> pratical elimination is always better than deprecation.
> 
> 
> Yes, I agree with this sentiment.
> 
> If you are confident that those of us reading the code will be able to
> determine that it is actually an address that's being extracted, then
> I have no further objections.  If there's any place where it's unclear,
> then I suggest the addition of a comment.

I'll do this (which means I'll re-do the change piece meal) and commit 
over comming days.

Andrew


>> Andrew
>> 
>> 
> 
>> >> First, the return types are different.  extract_address() returns
>> >> CORE_ADDR while extract_unsigned_integer returns ULONGEST.  If
>> >> we were to encounter a scenario where this is a problem, it's easier
>> >> to fix a wrapper (extract_address()) instead of the myriad places in
>> >> the code which presently call extract_address().  (This point is
>> >> probably moot because I suspect we already have a lot of code which
>> >> assumes that CORE_ADDR may be interchanged with LONGEST or ULONGEST
>> >> anyway.)
> 
>> > 
>> > 
>> > sizeof(CORE_ADDR) <= sizeof(ULONGEST) so this isn't a problem.
>> > 
>> > 
>> > Do we have a gdb_assert() somewhere to ensure that this is the case?
>> > (This could happen at initialization time...)
>> > 
>> > Magic in "defs.h" does it.  An assert wouldn't hurt.
>> > 
> 
>> >> Second, having function calls to extract_address() provides
>> >> information to the reader that you don't get by having calls to
>> >> extract_unsigned_integer().  It tells the reader that we're expecting
>> >> to get an address and not an integer.  This really helps when someone
>> >> reading gdb's code is wondering about what the thing is that's being
>> >> extracted.
> 
>> > 
>> > 
>> > The extract_address function doesn't extract an address, it extracts an unsigned integer.
>> > On the MIPS, extract_address needs to sign extend.  On the d10v, extract address needs to know the address space.
>> > 
>> > 
>> > Yes, I understand that.  Doing the substitution you propose will make
>> > it more difficult to make the correct fix (of using extract_typed_address)
>> > at a later time.
>> > 
>> > 
>> > If the code needs to extract an address it can use extract_typed_address which corectly handles all these cases.
>> > 
>> > 
>> > Yes.
>> > 
>> > 
>> > Is it a good thing?  It eliminates a lie.
>> > 
>> > 
>> > At the expense of making the code marginally less comprehensible and
>> > making it more difficult to identify the potential cases where
>> > extract_typed_address() should be used instead.
>> > 
>> > I think it makes it more comprehensible - it is now very clear exactly how the value is being obtained.  The ``extract_address'' function gives the misleading impression that it is correctly extracting an address, and that (per MIPS and d10v) isn't the case.
>> > 
>> > It also takes away the assumption that extract_address can, some how, be made cross architecture.
>> > 
>> > Or have all of those cases already been identified?  If so, then I
>> > withdraw my objection.  (Though I still like having "address" in the
>> > function name to help to document what it is that's being extracted.)
>> > 
>> > It tinkers with the following:
>> > 
>> > - ada/jv-* where things are pretty broken
>> > 
>> > - dwarf2 which is extracting/assuming an an unsigned integer
>> > 
>> > - unsigned_pointer_to_address making its implementation consistent with signed_pointer_to_address
>> > 
>> > - solib* where it is now (worryingly) clear what the code is doing.
>> > 
>> > - stack.c where it's printing out an integer value
>> > 
>> > After that, it's all target dependant code.
>> > 
>> > Andrew
>> > 
>> > 
>> > 
> 
>> 
>>-- End of excerpt from Andrew Cagney
> 
> 
> 
>