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

[Rafael Avila de Espindola via llvm-commits]

George Rimar via Phabricator <reviews at reviews.llvm.org> writes:

> grimar added inline comments.
>
>
> ================
> Comment at: ELF/LinkerScript.h:113
> +  std::string CommandString;
> +  // The offset value within the section before execution of assignment.
> +  unsigned Offset;
> ----------------
> 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.
>
>
> ================
> Comment at: ELF/MapFile.cpp:154
> +      if (auto *Cmd = dyn_cast<ByteCommand>(Base)) {
> +        writeHeader(OS, OSec->Addr + Cmd->Offset, Cmd->Size, Cmd->Size);
> +        OS << Indent1 << Cmd->CommandString << '\n';
> ----------------
> ruiu wrote:
>> You are passing `Cmd->Size` as an alignment which seems wrong to me.
> Right, that was incorrect, changed to 1.
>
>
> ================
> Comment at: ELF/ScriptParser.cpp:118-119
> +  // Concatenates tokens in a [FirstTok, LastTok] into string with Prefix.
> +  std::string buildCommandString(StringRef Prefix, size_t FirstTok,
> +                                 size_t LastTok);
> +
> ----------------
> ruiu wrote:
>> FirstTok and LastTok are not tokens but positions, so you should avoid "Tok".
> Not sure what is wrong here honestly. 
> I think `first` and `last` can be both adjectives and nouns. I meant that they are adjectives describing the
> tokens and hence they are not positions here I think.
> I renamed to `First`/`Last` though as such naming is also OK for me.

I think Rui's objection is that one would expect a *Tok variable to be
a StringRef. What you have are token number/indexes. I am ok with
First/Last, but maybe FirstTokI/LastTokI would be best.

Cheers,
Rafael