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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/strip-section-err.test:7
+
+# CHECK: error: Section .data cannot be removed because it is referenced by the relocation section .rela.text.
+ 
----------------
jhenderson wrote:
> grimar wrote:
> > jhenderson wrote:
> > > The relocations don't really reference the section, but rather a symbol in that section. I think the error message should reflect that somehow. It would also be great if the index of the relocation could be mentioned in the error message.
> > What do you think about the updated version (I am printing the relocation name and index)?
> Thanks, that looks much better, although I don't think there's much benefit in the type of relocation. I think we should probably be quoting section names too, but be consistent with what we do in other error messages.
Ok, I removed the type of relocation from the output.
That makes an interface to be slightly simpler.

Speaking about consistency of error messages, objdump is not very consistent.
It can quote/not quote the symbol names:

```
    return createStringError(llvm::errc::invalid_argument,
                             "Symbol %s cannot be removed because it is "
                             "referenced by the section %s[%d].",
                             Sym->Name.data(), this->Name.data(), this->Index);

```

```
      return createStringError(
          llvm::errc::invalid_argument,
          "not stripping symbol '%s' because it is named in a relocation.",
          Reloc.RelocSymbol->Name.data());
```

And seems never quotes the section names.


================
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());
+  }
----------------
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).


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

https://reviews.llvm.org/D58625