Page MenuHomePhabricator

[llvm-objcopy] Change SHT_NOBITS to SHT_PROBITS for some --set-section-flags
ClosedPublic

Authored by rupprecht on Mar 28 2019, 1:17 PM.

Details

Summary

Some flags accepted by --set-section-flags and --rename-section can change a SHT_NOBITS section to a SHT_PROGBITS section. Note that none of them can change a SHT_PROGBITS to SHT_NOBITS.

The full list (found via experimentation of individually setting each flag) that does this is: contents, load, noload, code, data, rom, and debug.

This was found by testing llvm-objcopy with the gnu binutils test suite, specifically this test case: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=binutils/testsuite/binutils-all/copy-1.d;h=f2b0d9e90df738c2891b4d5c7b62f62894b556ca;hb=HEAD

Diff Detail

Repository
rL LLVM

Event Timeline

rupprecht created this revision.Mar 28 2019, 1:17 PM

Note: I committed rL357199 as a much less interesting refactoring in preparation for this patch; if you have any comments on that patch, I can correct them here.

jhenderson added inline comments.Mar 29 2019, 2:43 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
107 ↗(On Diff #192707)

Won't this change the type of other non-NOBITS sections (e.g. SHT_RELA or SHT_DYNSYM)? Is that intended (I suspect not...)?

The intention makes sense. I just found this piece of code

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;hb=cc00a5d100973549bf5e4840937529633f4de1fa;f=bfd/elf.c#l3136

int
bfd_elf_get_default_section_type (flagword flags)
{
  if ((flags & (SEC_ALLOC | SEC_IS_COMMON)) != 0
      && (flags & (SEC_LOAD | SEC_HAS_CONTENTS)) == 0)
    return SHT_NOBITS;
  return SHT_PROGBITS;
}

Can the SectionFlag rules be simplified?

grimar added inline comments.Mar 29 2019, 4:48 AM
llvm/test/tools/llvm-objcopy/ELF/set-section-flags.test
52 ↗(On Diff #192707)

Would it be better to introduce a new section .foo with SHT_NOBITS?

grimar added inline comments.Mar 29 2019, 4:51 AM
llvm/test/tools/llvm-objcopy/ELF/set-section-flags.test
52 ↗(On Diff #192707)

I mean a new section '.bar' :) Sorry. I.e. To add a section instead of changing the existent one in a test.
(The same applies for the 'rename-section-flag.test', btw).

rupprecht marked 5 inline comments as done.
  • Only change the type if the input is SHT_NOBITS
  • Add test coverage

The intention makes sense. I just found this piece of code

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;hb=cc00a5d100973549bf5e4840937529633f4de1fa;f=bfd/elf.c#l3136

int
bfd_elf_get_default_section_type (flagword flags)
{
  if ((flags & (SEC_ALLOC | SEC_IS_COMMON)) != 0
      && (flags & (SEC_LOAD | SEC_HAS_CONTENTS)) == 0)
    return SHT_NOBITS;
  return SHT_PROGBITS;
}

Can the SectionFlag rules be simplified?

I don't (yet) see a good way to simplify it after looking at that snippet. I think there may be handling of the section flags elsewhere that isn't captured by that method.
For example, that only ever returns SHT_NOBITS or SHT_PROGBITS, but experimentation shows that the end result can be SHT_RELA.

llvm/test/tools/llvm-objcopy/ELF/set-section-flags.test
52 ↗(On Diff #192707)

Sure, reverted .foo and added .baz (also .rela.foo to test something else) instead of just changing the test case. Since the rename test is renaming it to .bar, the new section is .baz.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
107 ↗(On Diff #192707)

I did some experimentation with gnu objcopy.

  • For some sections (rela, symtab, init_array), it never touched the section type, *or* SHF flags (so we're actually already different there)
  • For some sections like .gnu.hash, it *always* changed it to progbits (even for flags like alloc that don't change nobits to progbits).

Fixed this to only change the type if the old type is NOBITS.

It looks fine to me, though I'd wait for somebodies else opinion too.
Few minor comments/suggestions are inline.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
88 ↗(On Diff #192881)

btw (not relative to this patch though), this is a bit strange name for this method.
It does not set anything, so seems it should be started with get*.

99 ↗(On Diff #192881)

Seems Flags would be a better name for argument here? Since in the current context, there are no old/new flags.
Also, this method changes not only flags but also a type, so perhaps it should be named setSectionFlagsAndType?

104 ↗(On Diff #192881)

What about doing the early return here? I.e.:

// Certain flags also promote SHT_NOBITS to SHT_PROGBITS. Don't change other
// section types (RELA, SYMTAB, etc.).
if (Sec.Type != SHT_NOBITS)
  return;

const SectionFlag NoBitsToProgBitsMask = ...
MaskRay added inline comments.Mar 30 2019, 8:54 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
107 ↗(On Diff #192707)

SHT_NULL may also change but it may not have much sense.

Other than that, SHT_NOBITS -> SHT_PROGBITS is the only possible change. I believe GNU objcopy's rule is similar to the following:

if (Sec.Type == SHT_NOTES && NewFlags & SectionFlags::SecAlloc &&
    NewFlags & !(SectionFlags::SecLoad))
  Sec.Type = SHT_PROGBITS;
MaskRay added inline comments.Mar 30 2019, 8:56 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
107 ↗(On Diff #192707)

Sorry, excessive !. It should be:

if (Sec.Type == SHT_NOTES && NewFlags & SectionFlags::SecAlloc &&
    NewFlags & SectionFlags::SecLoad)
  Sec.Type = SHT_PROGBITS;
MaskRay added inline comments.Mar 30 2019, 8:58 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
107 ↗(On Diff #192707)

Really sorry, not SHT_NOTES ...

if (Sec.Type == SHT_NOBITS && NewFlags & SectionFlags::SecAlloc &&
    NewFlags & SectionFlags::SecLoad)
  Sec.Type = SHT_PROGBITS;
rupprecht updated this revision to Diff 193187.Apr 1 2019, 3:20 PM
rupprecht marked 4 inline comments as done.
  • Fix some method/variable names
rupprecht added inline comments.Apr 1 2019, 3:20 PM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
107 ↗(On Diff #192707)

That's not the behavior I see. For example, when I run --set-section-flags=.baz=contents on the set-section-flags.test obj2yaml test file, GNU objcopy changes SHT_NOBITS to SHT_PROGBITS for that section, but the expression you give only changes it for alloc and load.

Perhaps you are getting SEC_ flags (which are bfd abstractions) mixed up with SHF_ flags (i.e. the actual ELF flags)?

104 ↗(On Diff #192881)

It'd be a mid-function return, not an early return, since it happens after some other important modifications to Section (SHF_ flags) have been made. IMO mid-function returns are harder to understand and cause pain in the long run. Otherwise I think that'd be a good idea.

grimar added a comment.Apr 2 2019, 5:27 AM

I have no other comments. Thanks!

This revision is now accepted and ready to land.Apr 2 2019, 5:32 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.Apr 3 2019, 3:07 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
107 ↗(On Diff #192707)

The rules are more complex than I thought, but I feel I understand its rules now. See https://reviews.llvm.org/D60189 for the simplification.