[PATCH] D44004: [ELF] - Show data and assignment commands in the map file.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM with these changes.



================
Comment at: ELF/LinkerScript.h:113
+  std::string CommandString;
+  // The offset value within the section before execution of assignment.
+  unsigned Offset;
----------------
grimar wrote:
> ruiu wrote:
> > ruiu wrote:
> > > Please insert a blank line before a comment.
> > What do you mean by "before execution of assignment"? Before you assign a command to an output section, it doesn't have any address.
> I mean that if we have a section description:
> 
> ```
> 0  .foo : {
> 1    *(.bar)
> 2    . += 0x123;
> 3  }
> ```
> 
> Then at line 2, we have an assignment to Dot.
> We handle commands one by one and offset before we process 
> ("before execution of assignment (//command//)" in my wording) the assignment will
> be equal to the size of .bar. That offset is saved to Offset member.
> After execution of assignment command, offset will be different and `Size` below
> stores the difference.
> 
> I rewrote the comment slightly.
Just "This is just an offset of this assignment command in the output section" should be more clear. "before processing the assignment" doesn't make much sense to me.


================
Comment at: ELF/LinkerScript.h:118
+
+  // The value by which the assignment changed the output section size.
+  unsigned Size;
----------------
"Size of this assignment command. This is usually 0, but if you move '.' or use a BYTE()-family command, this may be greater than 0."


================
Comment at: ELF/LinkerScript.h:198
 
+  // Keeps string representing the command. Used when printing map file.
+  std::string CommandString;
----------------
"Used for -Map" is perhaps better.


================
Comment at: ELF/ScriptParser.cpp:791
 
+std::string ScriptParser::buildCommandString(StringRef Prefix, size_t First,
+                                             size_t Last) {
----------------
I think it doesn't have to be a function; actually I believe it is easier to read if you inline it.


https://reviews.llvm.org/D44004