This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj, obj2yaml] - Add support for SHT_NOTE sections.
ClosedPublic

Authored by grimar on Oct 15 2019, 6:07 AM.

Details

Summary

SHT_NOTE is the section that consists of
namesz, descsz, type, name + padding, desc + padding data.
This patch teaches yaml2obj, obj2yaml to dump and parse them.

This patch implements the section how it is described here:
https://docs.oracle.com/cd/E23824_01/html/819-0690/chapter6-18048.html
Which says: "For 64–bit objects and 32–bit objects, each entry is an array of 4-byte words in
the format of the target processor"

The official specification is different
http://www.sco.com/developers/gabi/latest/ch5.pheader.html#note_section
And says: "n 64-bit objects (files with e_ident[EI_CLASS] equal to ELFCLASS64), each entry is an array
of 8-byte words in the format of the target processor. In 32-bit objects (files with e_ident[EI_CLASS]
equal to ELFCLASS32), each entry is an array of 4-byte words in the format of the target processor"

Since LLVM uses the first, 32-bit way, this patch follows it.

Diff Detail

Event Timeline

grimar created this revision.Oct 15 2019, 6:07 AM
MaskRay added a comment.EditedOct 16 2019, 1:41 AM

There is an official reference: http://www.sco.com/developers/gabi/latest/ch5.pheader.html#note_section

When operating systems implemented the 64-bit ELF ABI, many (including Linux, FreeBSD, and NetBSD) interpreted the gABI wording incorrectly and chose 32-bit struct members for Elf64_Nhdr. A few (including Solaris) are correct and use 64-bit members. This inconsistency should not affect this patch, but I just want to mention the problem.

I think all platforms that have interests in these binary utilities (Linux, *BSD, Fuchsia) use 32-bit members. So we are fine to use write32 etc. This may need a comment though.

@phosek to confirm n_namesz/n_descsz/n_type are 32-bit on 64-bit Fuchsia.

MaskRay added a subscriber: phosek.Oct 16 2019, 1:49 AM

Thanks!

When operating systems implemented the 64-bit ELF ABI, many (including Linux, FreeBSD, and NetBSD) interpreted the gABI wording incorrectly and chose 32-bit struct members for Elf64_Nhdr. A few (including Solaris) are correct and use 64-bit members. This inconsistency should not affect this patch, but I just want to mention the problem.

Interesting. I guess if we wanted to support it, we would probably be able to check OSABI:

--- !ELF
FileHeader:
...
OSABI = Solaris

For now I'd not do anything with that. I think LLVM tools like llvm-readobj implement only 32-bit case anyways:

template <class ELFT>
struct Elf_Nhdr_Impl {
  LLVM_ELF_IMPORT_TYPES_ELFT(ELFT)
  Elf_Word n_namesz;
  Elf_Word n_descsz;
  Elf_Word n_type;

  /// The alignment of the name and descriptor.
  ///
  /// Implementations differ from the specification here: in practice all
  /// variants align both the name and descriptor to 4-bytes.
  static const unsigned int Align = 4;

  /// Get the size of the note, including name, descriptor, and padding.
  size_t getSize() const {
    return sizeof(*this) + alignTo<Align>(n_namesz) + alignTo<Align>(n_descsz);
  }
};
grimar edited the summary of this revision. (Show Details)Oct 16 2019, 2:00 AM
grimar planned changes to this revision.Oct 16 2019, 2:15 AM

Planning an update.

grimar updated this revision to Diff 225186.EditedOct 16 2019, 3:09 AM
  • Implement NoteEntry::Desc as yaml::BinaryRef instead of StringRef. Description is not a null-terminated string, but a binary data. Previous implementation tried to use StringRef, but it was incorrect, it produced wrong output.
  • Fix the code and one more test case (seems I've forgot to do a final tests run before posting).
MaskRay added inline comments.Oct 17 2019, 4:08 AM
include/llvm/Support/YAMLTraits.h
652 ↗(On Diff #225186)

Interesting. Is there a test covering the fixed bug?

test/tools/obj2yaml/elf-sht-note.yaml
3 ↗(On Diff #225186)

Check we dump hex digit pairs if the note section is invalid.

Then, group invalid cases together, the first of which is when the size is less than 12 bytes.

It may be clearer if we place valid cases first.

62 ↗(On Diff #225186)

when the content is shorter than the computed size

test/tools/yaml2obj/elf-sht-note.yaml
26 ↗(On Diff #225186)

NUL terminator.

NUL != null.

56 ↗(On Diff #225186)

What is 'Tag'

grimar updated this revision to Diff 225487.Oct 17 2019, 11:44 AM
grimar marked 6 inline comments as done.
  • Addressed review comments, rebased.
grimar added inline comments.Oct 17 2019, 11:44 AM
include/llvm/Support/YAMLTraits.h
652 ↗(On Diff #225186)

I faced it running the tools\obj2yaml\missing_symtab.test initially.
It uses a precompiled binary.
I planned to convert it and add a separate test case for this place later,
but after your comment I decided to do it right now and this revealed
a different bug:

I am creating notes in dumpNoteSection().
I used toStringRef before this diff:

Entries.push_back({Note.getName(), toStringRef(Note.getDesc()), Note.getType()});

It was there because previously Desc was a StringRef, but now it is a yaml::BinaryRef,
so toStringRef became excessive and actually harmfull.

Problem of toStringRef is that BinaryRef constructor has a different behavior for
StringRef and ArrayRef<uint8_t>):

BinaryRef(ArrayRef<uint8_t> Data) : Data(Data), DataIsHexString(false) {}
BinaryRef(StringRef Data) : Data(arrayRefFromStringRef(Data)) {}

it wasn't correct to use StringRef version and I removed this change
from the patch after the fix, as it is not needed anymore.

I've added a one more test to elf-sht-note.yaml that covers this place.

I think I might be able to trigger this issue in a different way and going to investigate how
and suggest an independent patch for this place probably.
(I'll try to corrupt a section name in the same way, it should work I think).

grimar planned changes to this revision.Oct 17 2019, 12:09 PM

Going to add a one more test case for obj2yaml.

grimar updated this revision to Diff 225503.Oct 17 2019, 12:58 PM
  • Added few more test cases.
grimar marked an inline comment as done.Oct 18 2019, 1:58 AM
grimar added inline comments.
include/llvm/Support/YAMLTraits.h
652 ↗(On Diff #225186)

Patch: D69160

MaskRay accepted this revision.Oct 24 2019, 11:07 PM
This revision is now accepted and ready to land.Oct 24 2019, 11:07 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2019, 3:33 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
rupprecht added inline comments.Oct 25 2019, 2:14 PM
llvm/include/llvm/Support/YAMLTraits.h
652–653

Was this part of the change intentional? This looks like a rebase that accidentally reverted one of your previous patches.

grimar marked 2 inline comments as done.Oct 26 2019, 4:49 AM
grimar added inline comments.
llvm/include/llvm/Support/YAMLTraits.h
652–653

Thanks! It wasn't intentional. I've moved this and other my active patches to monorepo and I guess I've forgot to rebase it. I am going to recommit this part.

(I've re-checked that the existing test case for this, which is invalid-section-name.yaml fails for me now under windows in debug. I guess bots never build in debug probably, that is why it was not catched by them).