[llvm-readelf/obj] - Improve diagnostics when printing NT_FILE notes.

[grimar]

Summary

This changes the printNotesHelper to report warnings on its side when
there are errors when dumping notes.

With that we can provide more content when reporting warnings about broken notes.

[rupprecht]

Thanks!

llvm/tools/llvm-readobj/ELFDumper.cpp

5337

This is checking more than just the file mappings number, it also includes the page size (note the check above is for 2 bytes).
How about: "the note of size 0x1 is too short, expected at least 0x2"?

5348

I'd probably just say "unable to read file mappings".

If you prefer this, there's a typo: tripples -> triples.

5349

Maybe this could also say how many bytes it expected it to be? It should be (2 + 3 * FileCount) * Bytes IIRC.

And maybe say what FileCount is? That may give indication to a developer debugging a bad note whether the note is really too short or if the number of files is bogus.

5362

"file name for the file mapping" feels a little redundant -- maybe just "file name for the mapping"?

5575

Index should probably be incremented at the end, otherwise it reports the first note as index 1, not index 0.

[grimar]

• Addressed review comments.

llvm/tools/llvm-readobj/ELFDumper.cpp

5337

Done. Note: it is not 2 bytes, but 2 addresses. I.e 8 or 16 bytes.

5340

Side note: I think this doesn't work when N = 0 ("# of file mappings", see function comment) case.
Seems we should move this check below and exit a bit eariler when there are no mappings.

5349

Maybe this could also say how many bytes it expected it to be? It should be (2 + 3 * FileCount) * Bytes IIRC.

I think that (2 + 3 * FileCount) * Bytes is not the right value, because there should be at least N=FileCount NUL bytes in
filenames area it seems. I.e. here we have a check for (2 + 3 * FileCount) bytes, but only because we check
file names data in the code below and not right here.
So, I am not sure that we should mention an expected data size here?

5575

This is intentional. We enumerate program headers (few lines above) starting from 1 too.
And we do the same when printing warnings for relocations (in DumpStyle<ELFT>::forEachRelocationDo).

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

5575

Seems to me like this is inconsistent with our indexes elsewhere (where we index from 0). We should probably fix both cases here.

[grimar]

• Index notes from 0, not from 1.

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

5359

Do we want to index file mappings from 0 too?

[jhenderson]

LGTM!

llvm/test/tools/llvm-readobj/ELF/note-core-ntfile-bad.test

 ## Test that malformed NT_FILE sections in core files are gracefully ignored.
 
 ## llvm-mc doesn't support generating ET_CORE files; the 'Content' field in
 ## each of the following test cases were generated with the following steps:
 # $ llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu tmp.s -o tmp.o
 # $ bin/llvm-objcopy --dump-section=.note.foo=tmp.txt tmp.o /dev/null
 # $ xxd -p tmp.txt | tr -d '\n'; echo
 # using the assembly shown with each test case.
 
 # RUN: yaml2obj --docnum=1 %s -o %t1.o
 # RUN: llvm-readelf -n %t1.o 2>&1 | FileCheck -DFILE=%t1.o %s --check-prefix=ERR-HEADER-SHORT
-# ERR-HEADER-SHORT: warning: '[[FILE]]': malformed note: header too short
+# RUN: llvm-readobj -n %t1.o 2>&1 | FileCheck -DFILE=%t1.o %s --check-prefix=ERR-HEADER-SHORT
+
+# ERR-HEADER-SHORT: warning: '[[FILE]]': unable to read note with index 0 from the PT_NOTE segment with index 1: the note of size 0x8 is too short, expected at least 0x10
 
 # .section ".note.foo", "a"
 #       .align 4
 #       .long 5 /* namesz */
 #       .long end - begin /* descsz */
 #       .long 0x46494c45 /* type = NT_FILE */
 #       .asciz "CORE"
 #       .align 4
- Name:    .note.foo
     Content: 0500000008000000454C4946434F5245000000000000000000000000
 ProgramHeaders:
   - Type:     PT_NOTE
     FirstSec: .note.foo
     LastSec:  .note.foo
 
 # RUN: yaml2obj --docnum=2 %s -o %t2.o
 # RUN: llvm-readelf -n %t2.o 2>&1 | FileCheck -DFILE=%t2.o %s --check-prefix=ERR-NULL-TERM
-# ERR-NULL-TERM: warning: '[[FILE]]': malformed note: not NUL terminated
+# RUN: llvm-readobj -n %t2.o 2>&1 | FileCheck -DFILE=%t2.o %s --check-prefix=ERR-NULL-TERM
+
+# ERR-NULL-TERM: warning: '[[FILE]]': unable to read note with index 0 from the PT_NOTE segment with index 1: the note is not NUL terminated
 
 # .section ".note.foo", "a"
 #       .align 4
 #       .long 5 /* namesz */
 #       .long end - begin /* descsz */
 #       .long 0x46494c45 /* type = NT_FILE */
 #       .asciz "CORE"
 #       .align 4
- Name:    .note.foo
     Content: 050000002C000000454C4946434F5245000000000100000000000000001000000000000000100000000000000020000000000000003000000000000078787878
 ProgramHeaders:
   - Type:     PT_NOTE
     FirstSec: .note.foo
     LastSec:  .note.foo
 
 # RUN: yaml2obj --docnum=3 %s -o %t3.o
 # RUN: llvm-readelf -n %t3.o 2>&1 | FileCheck -DFILE=%t3.o %s --check-prefix=ERR-FILE-COUNT
-# ERR-FILE-COUNT: warning: '[[FILE]]': malformed note: too short for number of files
+# RUN: llvm-readobj -n %t3.o 2>&1 | FileCheck -DFILE=%t3.o %s --check-prefix=ERR-FILE-COUNT
+
+# ERR-FILE-COUNT: warning: '[[FILE]]': unable to read note with index 0 from the PT_NOTE segment with index 1: unable to read file mappings (found 2): the note of size 0x2c is too short
 
 # .section ".note.foo", "a"
 #       .align 4
 #       .long 5 /* namesz */
 #       .long end - begin /* descsz */
 #       .long 0x46494c45 /* type = NT_FILE */
 #       .asciz "CORE"
 #       .align 4
- Name:    .note.foo
     Content: 050000002C000000454C4946434F5245000000000200000000000000001000000000000000100000000000000020000000000000003000000000000078797A00
 ProgramHeaders:
   - Type:     PT_NOTE
     FirstSec: .note.foo
     LastSec:  .note.foo
 
 # RUN: yaml2obj --docnum=4 %s -o %t4.o
 # RUN: llvm-readelf -n %t4.o 2>&1 | FileCheck -DFILE=%t4.o %s --check-prefix=ERR-FILE-END-EARLY
-# ERR-FILE-END-EARLY: warning: '[[FILE]]': malformed note: too few filenames
+# RUN: llvm-readobj -n %t4.o 2>&1 | FileCheck -DFILE=%t4.o %s --check-prefix=ERR-FILE-END-EARLY
+
+# ERR-FILE-END-EARLY: warning: '[[FILE]]': unable to read note with index 0 from the PT_NOTE segment with index 1: unable to read the file name for the mapping with index 2: the note of size 0x44 is truncated
 
 # .section ".note.foo", "a"
 #       .align 4
 #       .long 5 /* namesz */
 #       .long end - begin /* descsz */
 #       .long 0x46494c45 /* type = NT_FILE */
 #       .asciz "CORE"
 #       .align 4

llvm/tools/llvm-readobj/ELFDumper.cpp

static Expected<CoreNote> readCoreNote(DataExtractor Desc) {
   // 3. N (start, end, offset) triples
   // 4. N packed filenames (null delimited)
   // Each field is an Elf_Addr, except for filenames which are char* strings.
 
   CoreNote Ret;
   const int Bytes = Desc.getAddressSize();
 
   if (!Desc.isValidOffsetForAddress(2))
-    return createStringError(object_error::parse_failed,
-                             "malformed note: header too short");
+    return createError("the note of size 0x" + Twine::utohexstr(Desc.size()) +
+                       " is too short, expected at least 0x" +
+                       Twine::utohexstr(Bytes * 2));
   if (Desc.getData().back() != 0)
-    return createStringError(object_error::parse_failed,
-                             "malformed note: not NUL terminated");
+    return createError("the note is not NUL terminated");
 
   uint64_t DescOffset = 0;
   uint64_t FileCount = Desc.getAddress(&DescOffset);
   Ret.PageSize = Desc.getAddress(&DescOffset);
 
   if (!Desc.isValidOffsetForAddress(3 * FileCount * Bytes))
-    return createStringError(object_error::parse_failed,
-                             "malformed note: too short for number of files");
+    return createError("unable to read file mappings (found " +
+                       Twine(FileCount) + "): the note of size 0x" +
+                       Twine::utohexstr(Desc.size()) + " is too short");
 
   uint64_t FilenamesOffset = 0;
   DataExtractor Filenames(
       Desc.getData().drop_front(DescOffset + 3 * FileCount * Bytes),
       Desc.isLittleEndian(), Desc.getAddressSize());
 
   Ret.Mappings.resize(FileCount);
+  size_t I = 0;
   for (CoreFileMapping &Mapping : Ret.Mappings) {
+    ++I;
     if (!Filenames.isValidOffsetForDataOfSize(FilenamesOffset, 1))
-      return createStringError(object_error::parse_failed,
-                               "malformed note: too few filenames");
+      return createError(
+          "unable to read the file name for the mapping with index " +
+          Twine(I) + ": the note of size 0x" + Twine::utohexstr(Desc.size()) +
+          " is truncated");
     Mapping.Start = Desc.getAddress(&DescOffset);
     Mapping.End = Desc.getAddress(&DescOffset);
     Mapping.Offset = Desc.getAddress(&DescOffset);
     Mapping.Filename = Filenames.getCStrRef(&FilenamesOffset);
   }
 
   return Ret;
 }

 }
 
 template <class ELFT>
 static void printNotesHelper(
     const ELFDumper<ELFT> &Dumper,
     llvm::function_ref<void(Optional<StringRef>, typename ELFT::Off,
                             typename ELFT::Addr)>
         StartNotesFn,
-    llvm::function_ref<void(const typename ELFT::Note &)> ProcessNoteFn,
+    llvm::function_ref<Error(const typename ELFT::Note &)> ProcessNoteFn,
     llvm::function_ref<void()> FinishNotesFn) {
   const ELFFile<ELFT> &Obj = Dumper.getElfObject().getELFFile();
 
   ArrayRef<typename ELFT::Shdr> Sections = cantFail(Obj.sections());
   if (Obj.getHeader().e_type != ELF::ET_CORE && !Sections.empty()) {
     for (const typename ELFT::Shdr &S : Sections) {
       if (S.sh_type != SHT_NOTE)
         continue;
       StartNotesFn(expectedToOptional(Obj.getSectionName(S)), S.sh_offset,
                    S.sh_size);
       Error Err = Error::success();
-      for (const typename ELFT::Note Note : Obj.notes(S, Err))
-        ProcessNoteFn(Note);
+      size_t I = 0;
+      for (const typename ELFT::Note Note : Obj.notes(S, Err)) {
+        if (Error E = ProcessNoteFn(Note))
+          Dumper.reportUniqueWarning(
+              "unable to read note with index " + Twine(I) + " from the " +
+              describe(Obj, S) + ": " + toString(std::move(E)));
+        ++I;
+      }
       if (Err)
         Dumper.reportUniqueWarning("unable to read notes from the " +
                                    describe(Obj, S) + ": " +
                                    toString(std::move(Err)));
       FinishNotesFn();
     }
     return;
   }
 
   Expected<ArrayRef<typename ELFT::Phdr>> PhdrsOrErr = Obj.program_headers();
   if (!PhdrsOrErr) {
     Dumper.reportUniqueWarning(
         "unable to read program headers to locate the PT_NOTE segment: " +
         toString(PhdrsOrErr.takeError()));
     return;
   }
 
   size_t I = 0;
   for (const typename ELFT::Phdr &P : *PhdrsOrErr) {
     ++I;
     if (P.p_type != PT_NOTE)
       continue;
     StartNotesFn(/*SecName=*/None, P.p_offset, P.p_filesz);
     Error Err = Error::success();
-    for (const typename ELFT::Note Note : Obj.notes(P, Err))
-      ProcessNoteFn(Note);
+    size_t Index = 0;
+    for (const typename ELFT::Note Note : Obj.notes(P, Err)) {
+      if (Error E = ProcessNoteFn(Note))
+        Dumper.reportUniqueWarning("unable to read note with index " +
+                                   Twine(Index) +
+                                   " from the PT_NOTE segment with index " +
+                                   Twine(I) + ": " + toString(std::move(E)));
+      ++Index;
+    }
     if (Err)
       Dumper.reportUniqueWarning(
           "unable to read notes from the PT_NOTE segment with index " +
           Twine(I) + ": " + toString(std::move(Err)));
     FinishNotesFn();
   }
 }
 
 template <class ELFT> void GNUStyle<ELFT>::printNotes() {
   auto PrintHeader = [&](Optional<StringRef> SecName,
                          const typename ELFT::Off Offset,
                          const typename ELFT::Addr Size) {
     OS << "Displaying notes found ";
 
     if (SecName)
       OS << "in: " << *SecName << "\n";
     else
       OS << "at file offset " << format_hex(Offset, 10) << " with length "
          << format_hex(Size, 10) << ":\n";
 
     OS << "  Owner                Data size \tDescription\n";
   };
 
-  auto ProcessNote = [&](const Elf_Note &Note) {
+  auto ProcessNote = [&](const Elf_Note &Note) -> Error {
     StringRef Name = Note.getName();
     ArrayRef<uint8_t> Descriptor = Note.getDesc();
     Elf_Word Type = Note.getType();
 
     // Print the note owner/type.
     OS << "  " << left_justify(Name, 20) << ' '
        << format_hex(Descriptor.size(), 10) << '\t';
 
if (Name == "GNU") {
       const AMDGPUNote N = getAMDGPUNote<ELFT>(Type, Descriptor);
       if (!N.Type.empty())
         OS << "    " << N.Type << ":\n        " << N.Value << '\n';
     } else if (Name == "CORE") {
       if (Type == ELF::NT_FILE) {
         DataExtractor DescExtractor(Descriptor,
                                     ELFT::TargetEndianness == support::little,
                                     sizeof(Elf_Addr));
-        Expected<CoreNote> Note = readCoreNote(DescExtractor);
-        if (Note)
-          printCoreNote<ELFT>(OS, *Note);
+        if (Expected<CoreNote> NoteOrErr = readCoreNote(DescExtractor))
+          printCoreNote<ELFT>(OS, *NoteOrErr);
         else
-          reportWarning(Note.takeError(), this->FileName);
+          return NoteOrErr.takeError();
       }
     } else if (!Descriptor.empty()) {
       OS << "   description data:";
       for (uint8_t B : Descriptor)
         OS << " " << format("%02x", B);
       OS << '\n';
     }
+    return Error::success();
   };
 
   printNotesHelper(this->dumper(), PrintHeader, ProcessNote, []() {});
 }
 
 template <class ELFT> void GNUStyle<ELFT>::printELFLinkerOptions() {
   OS << "printELFLinkerOptions not implemented!\n";
 }
auto StartNotes = [&](Optional<StringRef> SecName,
     NoteScope = std::make_unique<DictScope>(W, "NoteSection");
     W.printString("Name", SecName ? *SecName : "<?>");
     W.printHex("Offset", Offset);
     W.printHex("Size", Size);
   };
 
   auto EndNotes = [&] { NoteScope.reset(); };
 
-  auto ProcessNote = [&](const Elf_Note &Note) {
+  auto ProcessNote = [&](const Elf_Note &Note) -> Error {
     DictScope D2(W, "Note");
     StringRef Name = Note.getName();
     ArrayRef<uint8_t> Descriptor = Note.getDesc();
     Elf_Word Type = Note.getType();
 
     // Print the note owner/type.
     W.printString("Owner", Name);
     W.printHex("Data size", Descriptor.size());
if (Name == "GNU") {
       const AMDGPUNote N = getAMDGPUNote<ELFT>(Type, Descriptor);
       if (!N.Type.empty())
         W.printString(N.Type, N.Value);
     } else if (Name == "CORE") {
       if (Type == ELF::NT_FILE) {
         DataExtractor DescExtractor(Descriptor,
                                     ELFT::TargetEndianness == support::little,
                                     sizeof(Elf_Addr));
-        Expected<CoreNote> Note = readCoreNote(DescExtractor);
-        if (Note)
+        if (Expected<CoreNote> Note = readCoreNote(DescExtractor))
           printCoreNoteLLVMStyle(*Note, W);
         else
-          reportWarning(Note.takeError(), this->FileName);
+          return Note.takeError();
       }
     } else if (!Descriptor.empty()) {
       W.printBinaryBlock("Description data", Descriptor);
     }
+    return Error::success();
   };
 
   printNotesHelper(this->dumper(), StartNotes, ProcessNote, EndNotes);
 }
 
 template <class ELFT> void LLVMStyle<ELFT>::printELFLinkerOptions() {
   ListScope L(W, "LinkerOptions");