Page MenuHomePhabricator

[llvm-objcopy] Add switch to allow removing referenced sections
ClosedPublic

Authored by jhenderson on Apr 5 2019, 9:27 AM.

Details

Summary

llvm-objcopy currently emits an error if a section to be removed is referenced by another section. This is a reasonable thing to do, but is different to GNU objcopy. We should allow users who know what they are doing to have a way to produce the invalid ELF. This change adds a new switch --allow-broken-links to both llvm-strip and llvm-objcopy to do precisely that. The corresponding Link field is then set to 0 instead of an error being emitted.

I cannot use llvm-readelf/readobj to test the link fields because they emit an error if any sections, like the .dynsym, cannot be properly loaded.

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.Apr 5 2019, 9:27 AM
Herald added a project: Restricted Project. · View Herald Transcript

A couple of examples where this might be useful:

  1. Removing just the dynamic symbols. This would cause an error if there are any dynamic relocation sections. However, not all dynamic relocations require symbols, so potentially NO dynamic relocations need symbols. The .dynsym is then unnecessary.
  2. Removing the .dynstr. This would cause an error due to .dynamic referencing it. This section doesn't necessarily have tags that reference strings. Additionally, users might want to strip the information, but still provide some of the ELF file for example to aid debugging an issue.
jhenderson edited the summary of this revision. (Show Details)Apr 15 2019, 8:58 AM
grimar added inline comments.Apr 16 2019, 5:38 AM
test/tools/llvm-objcopy/ELF/reloc-error-remove-symtab.test
41 ↗(On Diff #193899)

Why not just SECTIONS: Link: 0?

tools/llvm-objcopy/ELF/Object.cpp
566 ↗(On Diff #193899)

It is a bit hard to follow ifs it seems, I would suggest adding bracers,
like you did for removeSectionReferences or
or may be even doing something like next for better readability:

if (ToRemove(Symbols)) {
  if (!AllowBrokenDependencies)
    return createStringError(
        llvm::errc::invalid_argument,
        "Symbol table %s cannot be removed because it is "
        "referenced by the relocation section %s.",
        Symbols->Name.data(), this->Name.data());

  Symbols = nullptr;
}
jhenderson marked 2 inline comments as done.Apr 16 2019, 6:03 AM
jhenderson added inline comments.
test/tools/llvm-objcopy/ELF/reloc-error-remove-symtab.test
41 ↗(On Diff #193899)

If the Link field is not 0 and I do SECTIONS: Link: 0, then it will find a different sh_link output later on with value 0, potentially. I could workaround this by doing CHECK-NEXT a few times on the unimportant fields, but it's not really ideal.

tools/llvm-objcopy/ELF/Object.cpp
566 ↗(On Diff #193899)

I like this suggestion, thanks.

jhenderson updated this revision to Diff 195381.EditedApr 16 2019, 7:41 AM

Re-arrange some if/else statements and improve/add some comments and the option help text.

grimar added inline comments.Apr 16 2019, 12:09 PM
tools/llvm-objcopy/ELF/Object.cpp
440 ↗(On Diff #195381)

Perhaps also be consistent here?

if (ToRemove(SymbolNames)) {
    if (!AllowBrokenDependencies)
      return createStringError(
          llvm::errc::invalid_argument,
          "String table %s cannot be removed because it is "
          "referenced by the symbol table %s",
          SymbolNames->Name.data(), this->Name.data());

    SymbolNames = nullptr;
}
tools/llvm-objcopy/ObjcopyOpts.td
13 ↗(On Diff #195381)

Not strong opinion here, but allow-broken-dependencies looks like a bit too long option name.
Will allow-broken-deps/allow-broken-links sound better?

jhenderson marked 2 inline comments as done.Apr 17 2019, 1:58 AM
jhenderson added inline comments.
tools/llvm-objcopy/ELF/Object.cpp
440 ↗(On Diff #195381)

Thanks, I missed this one.

tools/llvm-objcopy/ObjcopyOpts.td
13 ↗(On Diff #195381)

I don't like abbreviations in switches, because I don't know what "deps" stands for. However, allow-broken-links is reasonable. I was thinking this switch could also be used for other relationships via sh_info, but I don't know of any yet where this is needed.

grimar accepted this revision.Apr 17 2019, 2:36 AM

Ok, this looks fine I think. Maybe worth to hold this for a while time to check that there
are no objections about the new option name/description (or anything else).

tools/llvm-objcopy/ELF/Object.cpp
440 ↗(On Diff #195381)

nit: still not updated :)

This revision is now accepted and ready to land.Apr 17 2019, 2:36 AM
jhenderson marked an inline comment as done.Apr 17 2019, 2:37 AM
jhenderson added inline comments.
tools/llvm-objcopy/ELF/Object.cpp
440 ↗(On Diff #195381)

I know, I haven't updated the diff yet ;)

grimar added inline comments.Apr 17 2019, 2:41 AM
tools/llvm-objcopy/ObjcopyOpts.td
13 ↗(On Diff #195381)

I also did not find any valid use case.
I.e. looking at the table of how sh_info can be used (https://docs.oracle.com/cd/E19683-01/816-1386/chapter6-94076/index.html),
it does not seem that it is fine to allow broken dependencies there perhaps.

MaskRay added inline comments.Apr 17 2019, 2:52 AM
tools/llvm-objcopy/ELF/Object.cpp
432 ↗(On Diff #195381)

There is a -Wdangling-else warning.

jhenderson edited the summary of this revision. (Show Details)

Refactor an if block missed in previous change. Rename switch to --allow-broken-links.

lgtm, just a couple minor comments

test/tools/llvm-objcopy/ELF/dynsym-error-remove-strtab.test
9 ↗(On Diff #195381)

Isn't this kind of implicit? If there's an error, then llvm-objcopy will return a non-zero status and the test will fail

test/tools/llvm-objcopy/ELF/reloc-error-remove-symtab.test
40–41 ↗(On Diff #195381)

Maybe just: # SECTIONS: Link{{.*}}: 0?

tools/llvm-objcopy/ELF/Object.cpp
436 ↗(On Diff #195530)

We could make these error messages more helpful by saying something like:

String table cannot be removed ... Rerun with -R $name to also remove this section, or --allow-broken-links to suppress this error

tools/llvm-objcopy/ObjcopyOpts.td
12 ↗(On Diff #195530)

The examples given seem to apply to strip as well, so can you add the option there too? And at least one test that uses strip to make sure it's hooked up.

jhenderson marked 2 inline comments as done.Apr 17 2019, 3:41 AM
jhenderson added inline comments.
tools/llvm-objcopy/ELF/Object.cpp
432 ↗(On Diff #195381)

Is that with the latest version of this patch? There's no else anywhere around here now, so I assume it can't be.

tools/llvm-objcopy/ObjcopyOpts.td
13 ↗(On Diff #195381)

Theoretically it could affect platform/OS-specific sections (which would indicate their special handling with SHF_INFO_LINK), but I don't think llvm-objcopy correctly handles them anyway.

jhenderson marked 4 inline comments as done.Apr 17 2019, 3:45 AM
jhenderson added inline comments.
test/tools/llvm-objcopy/ELF/dynsym-error-remove-strtab.test
9 ↗(On Diff #195381)

I guess so. It's just a nice contrast to the first test case that's all, but I can remove it if you think it's unnecessary.

test/tools/llvm-objcopy/ELF/reloc-error-remove-symtab.test
40–41 ↗(On Diff #195381)

Same issue. It could match incorrectly against a later instance rather than the one we care about.

tools/llvm-objcopy/ELF/Object.cpp
436 ↗(On Diff #195530)

I don't disagree, but can this be a subsequent patch, as it affects an error message that isn't currently changing?

tools/llvm-objcopy/ObjcopyOpts.td
12 ↗(On Diff #195530)

Sounds reasonable.

rupprecht accepted this revision.Apr 17 2019, 4:11 AM

Sorry, I should have just accepted this before -- the option missing from strip is my only objection. The rest are just suggestions.

tools/llvm-objcopy/ELF/Object.cpp
436 ↗(On Diff #195530)

Yep, that's fine.

Rebase & address review comments:

  • Remove unnecessary empty output checks.
  • Add strip option and tests.
  • Reformat one bit.
  • Use distinct check prefixes for the error messages.
grimar added inline comments.Apr 17 2019, 8:58 AM
test/tools/llvm-objcopy/ELF/dynsym-error-remove-strtab.test
3 ↗(On Diff #195547)

behvior -> behavior

test/tools/llvm-objcopy/ELF/reloc-error-remove-symtab.test
4 ↗(On Diff #195547)

behvior -> behavior

test/tools/llvm-objcopy/ELF/remove-linked-section.test
4 ↗(On Diff #195547)

behvior -> behavior

5 ↗(On Diff #195547)

I am not familiar with llvm-strip.
Looking at its code:

if (!Config.StripDebug && !Config.StripUnneeded &&
    Config.DiscardMode == DiscardType::None && !Config.StripAllGNU && Config.SymbolsToRemove.empty())
  Config.StripAll = true;

Seems that -N fakesymbol would prevent stripping all. Should it work in the same way for -R?

This seems like a good idea and looks good from an architectural standpoint. Don't wait on me to land it (e.g. I'm not giving an LGTM because I haven't reviewed it properly but I've reviewed it enough to know that this is both a) a good idea and b) nothing bad is being introduced). I'm positive Jordan and George's review have already filled what I haven't done.

grimar added inline comments.Apr 18 2019, 12:50 AM
test/tools/llvm-objcopy/ELF/remove-linked-section.test
5 ↗(On Diff #195547)

To clarify: I do not really think using of --strip-debug to suppress the default --strip-all behavior is a stopper for landing this,
but I wonder if it worth to add a TODO about removing that in future or it is a normal thing? (it looks a bit hacky though).

i.e. for me as a person who knows nothing about llvm-strip is unclear if it should:

  • Have --strip-nothing flag for such situations? At least for use in test cases?
  • Do not force strip all if any -R is given. (With that you do would not need any hacks like that here)
jhenderson marked an inline comment as done.Apr 18 2019, 12:54 AM
jhenderson added inline comments.
test/tools/llvm-objcopy/ELF/remove-linked-section.test
5 ↗(On Diff #195547)

I was surprised by this as well, but testing GNU objcopy shows that we match their behaviour in this area. I think a --no-strip-all switch would be good, personally. It's in keeping with existing switches in other tools. I'll look at adding a TODO.

jhenderson edited the summary of this revision. (Show Details)

Add TODOs. Fix typos.

Thanks all, I'll get this landed now.

This revision was automatically updated to reflect the committed changes.