[PATCH] D58625: [llvm-objcopy] - Check for invalidated relocations when removing a section.

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:570
+        SymSecName.data(), R.RelocSymbol->Name.c_str(), RelName.str().c_str(),
+        I + 1, this->Name.data());
+  }
----------------
grimar wrote:
> jhenderson wrote:
> > `I + 1`? Shouldn't this just be `I`? There's no null relocation to ignore...
> > 
> > Do you need the `this->`?
> > I + 1? Shouldn't this just be I? There's no null relocation to ignore...
> 
> I did that because in real-life people usually count things starting from 1, not from 0...
> I thought that should work in this way here because I think I do not know any tool that would print
> indexes of relocations. For example, objdump output is:
> 
> ```
> RELOCATION RECORDS FOR [.text]:
> OFFSET           TYPE              VALUE 
> 0000000000000001 R_X86_64_PLT32    .data+0xfffffffffffffffd
> ```
> 
> and readelf's is:
> 
> ```
> Relocation section '.rela.text' at offset 0x90 contains 1 entries:
>   Offset          Info           Type           Sym. Value    Sym. Name + Addend
> 000000000001  000200000004 R_X86_64_PLT32    0000000000000000 .data - 3
> ```
> 
> Maybe we should print the Offset then, not an index? It seems to be more natural for relocations.
> I did that change, what do you think?
> 
> > Do you need the this->?
> No. but all other places in this file using `this->Name`, for example, see the first error message in this file.
> I would keep it for consistency. (Maybe it worth to remove them all at once in a follow-up, though).
first error message in this file -> first error message in this **method**


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58625/new/

https://reviews.llvm.org/D58625