[Lldb-commits] [PATCH] PECOFF

[Virgile Bello]

I see, sorry about that, I realized I got confused because Section also
have a "file_vm_addr" in its constructor (in .h, in .cpp it becomes
"file_addr"), accessible through GetFileAddress(). But this one seems to
actually be a VM space address, not a real file offset.
Some examples:
- In ObjectFile, comments for parameter file_vm_addr/GetFileAddress() when
creating Section: // File VM address == addresses as they are found in the
object file
- SetSectionLoadAddress (section_sp, section_sp->GetFileAddress() + offset)
< Using GetFileAddress to compute Section load address

Since Section used GetFileAddress() for VM addresses as written in
assembly, I thought GetFileOffset() would also be in VM space as well (I
didn't notice GetFileAddress vs GetFileOffset, thinking all GetFile would
work in the same address space).

Anyway, thanks for the review, now I understand what's wrong and I will
change it. However I still need this VM Base Address to be available for my
ObjectFile to be available externally, so that lldbProcessWindows can apply
module offset. As an example, if PECOFF module is supposed to be loaded at
VM Base Address = 0x400000 but actually loaded at 0x500000, debugger needs
to call module->SetLoadAddress(offset = 0x500000 - 0x400000), so that all
Sections are shifted properly.

Note that all addresses within PECOFF will be written in VM addr space as
if module is loaded at 0x400000, which is why it should be considered the
Module VM Address, same as Section's file_vm_addr/GetFileAddress() is
currently being used for Section VM address offseting.

So, if I understood right:
- Is it OK to add a function such as ObjectFile::GetVirtualFileAddress() or
something like that to represent this concept? If yes, what name?
- In that case, shouldn't Section::GetFileOffset be renamed
to GetVirtualFileAddress() as well, to avoid future confusion between VM
address and real file container address if a function name starts with
GetFile?
- Otherwise, any other idea on how to proceed?

I might have misunderstood so don't hesitate to tell me if I'm totally
wrong!



On Sat, Aug 24, 2013 at 3:33 AM, Greg Clayton <gclayton at apple.com> wrote:

> Actually this one is not correct. The m_file_offset is the offset of the
> PECOFF file within the file itself. So m_file_offset should be zero for all
> files that aren't in containers (like universal files on Darwin that
> contain multiple architecture slices, or .o files within a BSD archive (.a
> file)).
>
>
> On Aug 23, 2013, at 10:10 AM, Virgile Bello <virgile.bello at gmail.com>
> wrote:
>
> > File offset is not set properly in PECOFF.
> >
> > http://llvm-reviews.chandlerc.com/D1488
> >
> > Files:
> >  source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
> >
> > Index: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
> > ===================================================================
> > --- source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
> > +++ source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
> > @@ -434,6 +434,8 @@
> >                     m_coff_header_opt.data_dirs[i].vmaddr =
> m_data.GetU32(offset_ptr);
> >                     m_coff_header_opt.data_dirs[i].vmsize =
> m_data.GetU32(offset_ptr);
> >                 }
> > +
> > +                m_file_offset = m_coff_header_opt.image_base;
> >             }
> >         }
> >     }
> > <D1488.1.patch>_______________________________________________
> > lldb-commits mailing list
> > lldb-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20130824/068e783a/attachment.html>