This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] - Check for invalidated relocations when removing a section.
ClosedPublic

Authored by grimar on Feb 25 2019, 7:38 AM.

Diff Detail

Event Timeline

grimar created this revision.Feb 25 2019, 7:38 AM
jhenderson added inline comments.Feb 25 2019, 7:55 AM
test/tools/llvm-objcopy/ELF/strip-section-err.test
7

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.

tools/llvm-objcopy/ELF/Object.cpp
1375

Nit: add a space between "e.g." and "remove".

1377

alive -> a live (here only, not in the first line).

1378

Same as above comment on e.g.

grimar updated this revision to Diff 188198.Feb 25 2019, 9:11 AM
grimar marked 5 inline comments as done.
  • Addressed review comments.
test/tools/llvm-objcopy/ELF/strip-section-err.test
7

What do you think about the updated version (I am printing the relocation name and index)?

jhenderson added inline comments.Feb 25 2019, 9:39 AM
test/tools/llvm-objcopy/ELF/strip-section-err.test
7

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.

tools/llvm-objcopy/ELF/Object.cpp
563

StringRef?

570

I + 1? Shouldn't this just be I? There's no null relocation to ignore...

Do you need the this->?

1377

You've lost the "a" here. Full text should be "if a live section critically"

1378

One more nit: e.g. removed -> e.g. the removed

grimar updated this revision to Diff 188321.Feb 26 2019, 1:17 AM
grimar marked 5 inline comments as done.
  • Addressed review comments.
test/tools/llvm-objcopy/ELF/strip-section-err.test
7

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.

tools/llvm-objcopy/ELF/Object.cpp
570

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).

grimar marked an inline comment as done.Feb 26 2019, 1:19 AM
grimar added inline comments.
tools/llvm-objcopy/ELF/Object.cpp
570

first error message in this file -> first error message in this method

jhenderson added inline comments.Feb 26 2019, 2:08 AM
test/tools/llvm-objcopy/ELF/strip-section-err.test
2

I'd make this comment a little more general for the behaviour, by removing references to the section name and describing what is the interesting characteristic of it. Something like "Check we cannot remove a section containing symbols referenced by relocations contained in the object." or something along those lines.

6

In addition to this test case, I've thought of another one: what happens if you remove both .data and .rela.text? That shouldn't produce an error, since the reference has also been removed.

7

Hmm... so the offset is where the relocation patches, and doesn't necessarily make it easy to locate the relocation either (plus technically it's not illegal in the ELF spec to have two relocations patching the same offset). Maybe it's worth some others chiming in? @rupprecht/@jakehehrlich?

By the way, I'd say "patching offset" not "at offset", since the relocation is sat at a completely unrelated offset in its own section so this is otherwise a bit ambiguous.

As for the quoting, that should really be tidied up. I'll file a bug.

grimar marked an inline comment as done.Feb 26 2019, 2:28 AM
grimar added inline comments.
test/tools/llvm-objcopy/ELF/strip-section-err.test
7

Hmm... so the offset is where the relocation patches, and doesn't necessarily make it easy to locate the relocation either

Two points:

  1. In compare with GNU objcopy which reports "bar2.o: symbol `foobar' required but not present" it a lot easier.
  2. We report a symbol name, section and relocation's offset together. This should be enough to find the place where things went wrong I think. For example, LLD might report a section name, offset and relocation name too:

(.nonalloc+0x1): has non-ABS relocation R_X86_64_PC32 against symbol '_start'
It works :)

grimar updated this revision to Diff 188347.Feb 26 2019, 5:20 AM
grimar marked 3 inline comments as done.
  • Addressed comments.
test/tools/llvm-objcopy/ELF/strip-section-err.test
2

Done.

6

Yes. Added such test.

jhenderson accepted this revision.Feb 26 2019, 5:41 AM

LGTM, but may be worth get another llvm-objcopy developer to give a thumbs up in case I missed anything.

test/tools/llvm-objcopy/ELF/strip-section-err.test
7

As for the quoting, that should really be tidied up. I'll file a bug.

https://bugs.llvm.org/show_bug.cgi?id=40859

9

"remove a section with relocation" -> "remove the relocation section"

I think that's a little clearer.

12

Maybe use a different file name for this test case, to make it easier to debug the test in the future.

This revision is now accepted and ready to land.Feb 26 2019, 5:41 AM
rupprecht accepted this revision.Feb 26 2019, 10:04 PM
rupprecht added inline comments.
test/tools/llvm-objcopy/ELF/strip-section-err.test
7

error: Section .data cannot be removed because of symbol 'foo' used by the relocation patching offset 0x1 from section .rela.text.

The amount of technical information is good, but I think the rest of it is too verbose... any way to make it a bit more compact would be nice. The LLD error looks a little better, maybe we should use something like that?

I don't want to bikeshed on the wording too much though.

grimar marked 3 inline comments as done.Feb 27 2019, 3:17 AM
grimar added inline comments.
test/tools/llvm-objcopy/ELF/strip-section-err.test
7

The amount of technical information is good, but I think the rest of it is too verbose... any way to make it a bit more compact would be nice. The LLD error looks a little better, maybe we should use something like that?

I'll think about how to reduce it. Will suggest a separate follow-up patch.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2019, 3:17 AM