[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D55356#1327280 <https://reviews.llvm.org/D55356#1327280>, @clayborg wrote:

> In D55356#1327242 <https://reviews.llvm.org/D55356#1327242>, @labath wrote:
>
> > In D55356#1327224 <https://reviews.llvm.org/D55356#1327224>, @clayborg wrote:
> >
> > > In D55356#1327099 <https://reviews.llvm.org/D55356#1327099>, @labath wrote:
> > >
> > > > Actually, this now causes an lldb-mi test to fail, but it's not clear to me if the problem is in the test, or this patch. This issue happens when lldb-mi is printing the "library loaded" message after a module gets added to a not-yet-running target. It tries to print the load address by first getting the base address and then converting that to a load address.
> > > >
> > > > Before this patch, that would always fail, because well.. ELF and PECOFF had this function unimplemented, and for MachO the base address was section-relative, and so it wasn't resolved to a load address without the section being loaded. However, with this patch, in the ELF (and presumably PECOFF) case, the load address is not section-relative and so the `GetLoadAddress` function happily returns the address.
> > > >
> > > > Is this the expected behavior here? (i.e., object_file->GetLoadAddress().GetLoadAddress(target) returning a valid value even though the target is not running)
> > >
> > >
> > > Not unless someone has manually set the section load address in the test?
> >
> >
> > The test is not setting the load address manually. This simply falls out of how `Address::GetLoadAddress`  is implemented:
> >
> >   addr_t Address::GetLoadAddress(Target *target) const {
> >     SectionSP section_sp(GetSection());
> >     if (section_sp) {
> >       ...
> >     } else {
> >       // We don't have a section so the offset is the load address
> >       return m_offset;
> >     }
> >   }
> >
> >
> > So, where's the bug here? It's not clear to me how to change `Address::GetLoadAddress` to do something different than what it is doing now.
>
>
> So this is a bug really. When there is no section, we should specify what the m_offset is using lldb_private::AddressType in maybe a new ivar name "m_offset_type". Then GetBaseAddress() would specify eAddressTypeFile. And the above code would become:
>
>  
>
> addr_t Address::GetLoadAddress(Target *target) const {
>
>   SectionSP section_sp(GetSection());
>   if (section_sp) {
>     ...
>   } else if (m_offset_type == eAddressTypeLoad) {
>     // We don't have a section so the offset is the load address
>     return m_offset;
>   }
>
> }
>
>   We just need to be careful and see if we can not make lldb_private::Address get any bigger byte size wise if we can at all avoid it.


I must be missing something.  If there's a file around so that we can express this address relative to the file, why would it ever not be expressed as a section + offset?  If there's not a file around, then what does it mean to say this address ie eAddressTypeFile but we don't know the file?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55356/new/

https://reviews.llvm.org/D55356