[Lldb-commits] [PATCH] lldb incorrectly parses DWARF information from Go binary

[Stephen McDonald]

To answer clayborg's comment:
"The GO compiler should fix its mach-o sections to have unique addresses. I would really rather not parse an invalid MachO file and make work arounds for the incorrect file mach-o file."

This is only the second time I've looked at the format of a Mach-O file in detail, so I may have made a mistake in my argument below. Please let me know.

This part of the SymbolFileDWARF code is using a memory mapped object to reference the DWARF information. A memory mapped object duplicates the layout of of the information on the disk. So the memory offset information should be the memory offset of the data on the disk, not where it will be loaded in virtual memory. This is a bug in lldb. 

The Go compiler has the correct information for the offsets of the data on the disk.

As for the Go compiler setting the runtime load address of the section to 0, is that really an error? The DWARF information is not loaded into memory as part of normal program execution, right? Presumably that's why lldb memory maps the file. So is it really incorrect to say it's runtime load address in virtual memory is 0?

================
Comment at: include/lldb/Core/Section.h:205
@@ -203,1 +204,3 @@
+    // there is no parent then this method returns the offset from
+    // the beginning of the program in memory.
     lldb::addr_t
----------------
jasonmolenda wrote:
> Section.h uses the terminology "absolute file virtual address of this section if m_parent == NULL.  offset from parent file virtual address if m_parent != NULL".  I'd use similar wording here, maybe 
> 
> The absolute file virtual address of this section if it has no parent Section.  If there is a parent Section, this is the offset from the parent Section's file virtual address."
Thanks for the review! 
The problem is that there are two types of offsets. From Address.h:
"There are currently two types of addresses
/// for a section:
///     @li file addresses
///     @li load addresses
///
/// File addresses represent the virtual addresses that are in the "on
/// disk" object files...
/// Load addresses represent the virtual addresses where each section
/// ends up getting loaded at runtime."
Given this distinction, I find that the phrase "absolute file virtual address" confusing.
I do agree that my comment could be improved. How about 
GetOffset returns the difference between where the section is loaded in virtual memory at runtime and where the section's parent is loaded at runtime. 

================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:756
@@ +755,3 @@
+                    {
+                        offset = offset - parent_sp->GetFileOffset();
+                    }
----------------
jasonmolenda wrote:
> If we use Section::GetOffset() above, then this would be
> 
> offset = offset + parent_sp-GetOffset();
> 
> right?
As I noted above, GetOffset is for the offset in virtual memory where the code will be loaded. However this section of the code is using a memory mapped object. That means the memory corresponds directly to where the section information is located on the disk. For that you want the calculation I suggested.
It is really confusing.

http://reviews.llvm.org/D5568