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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
