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

[Rafael Ávila de Espíndola via Phabricator via llvm-commits]

rafael added a comment.

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


https://reviews.llvm.org/D44004