[llvm-readelf] Print raw ELF note contents if we can't parse it

[arichardson]

Summary

Currently, if the note name is known, but the value isn't we don't print
the contents.

[MaskRay]

Since this extends llvm-readelf output, can you also send an email to [e-mail] and check what output they want for GNU readelf? For llvm-readobj output, we are not bound to GNU decisions. Though, if the GNU output is not bad, aiming for consistency can usually make our implementation simpler.

[jhenderson]

Is there a test case that shows we don't print the raw data when the decoding is known?

[arichardson]

In D74367#1868875, @jhenderson wrote:

Is there a test case that shows we don't print the raw data when the decoding is known?

Not yet, but I've added a test case that would previously not print the contents for a GNU note of an unknown type.

llvm/tools/llvm-readobj/ELFDumper.cpp

5129

It seems like GNU readelf does not print the contents in this case.

[arichardson]

Add a test showing the change in behaviour for GNU notes with unknown types

[jhenderson]

I assume that you've verified against GNU readelf all the behaviour that you're changing here?

llvm/test/tools/llvm-readobj/ELF/gnu-note-size.test

1

It's unclear to me what this test is testing, so I can't really say whether this change is good. Would you mind adding a comment at the top of the test, either in this patch or a different one, briefly explaining its purpose, please? I imagine you might need to dig into the old commit messages for this file to find out.

llvm/tools/llvm-readobj/ELFDumper.cpp

5129

Nit: missing trailing full stop.

[emaste]

Do you expect to pick this up again?

[arichardson]

Rebase

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/gnu-note-size.test

4–5

## for comments.

llvm/test/tools/llvm-readobj/ELF/note-amd.s

29 ↗

(On Diff #321370)

Maybe .note.unknown rather than invalid? Invalid to me suggests a recognised type, but broken data.

llvm/test/tools/llvm-readobj/ELF/note-amdgpu-invalid.s

90 ↗

(On Diff #321370)

Unless you're planning on fixing this immediately, it would be worth mentioning this test in the bug, or the TODO might get missed.

llvm/test/tools/llvm-readobj/ELF/note-amdgpu.test

106 ↗

(On Diff #321370)

As above: maybe .note.unknown for clarity?

llvm/test/tools/llvm-readobj/ELF/note-generic.s

4 ↗

(On Diff #321370)

This looks wrong?

Also, I'm not sure why this test is changing as part of this change?

[arichardson]

llvm/test/tools/llvm-readobj/ELF/note-generic.s

4 ↗

(On Diff #321370)

Yes, sorry about that - it's a debugging leftover to compare against GNU readelf and the changes should be part of D96010. Will fix.

[arichardson]

Address review feedback

[jhenderson]

Possibly this is a patch ordering thing, but some of the pre-merge bots are failing for this patch, I think. LGTM, apart than that.

[arichardson]

In D74367#2548036, @jhenderson wrote:

Possibly this is a patch ordering thing, but some of the pre-merge bots are failing for this patch, I think. LGTM, apart than that.

Yes, the problem was that I didn't mark D95953 as a parent of this change.

llvm/test/tools/llvm-readobj/ELF/gnu-note-size.test

 # RUN: yaml2obj %s > %t
 # RUN: llvm-readelf --notes %t | FileCheck %s --check-prefix=GNU
 # RUN: llvm-readobj --elf-output-style LLVM --notes %t | FileCheck %s --check-prefix=LLVM
 
 # GNU:        Owner                 Data size       Description
 # GNU-NEXT:   GNU                   0x00000004      NT_GNU_ABI_TAG (ABI version tag)
 # GNU-NEXT:     <corrupt GNU_ABI_TAG>
+# GNU-NEXT:     description data: 00 00 00 00
+# GNU-EMPTY:
 
 # LLVM:      Notes [
 # LLVM-NEXT:   NoteSection {
 # LLVM-NEXT:     Offset:
 # LLVM-NEXT:     Size: 0x14
 # LLVM-NEXT:     Note {
 # LLVM-NEXT:       Owner: GNU
 # LLVM-NEXT:       Data size: 0x4
 # LLVM-NEXT:       Type: NT_GNU_ABI_TAG (ABI version tag)
 # LLVM-NEXT:       ABI: <corrupt GNU_ABI_TAG>
+# LLVM-NEXT:       Description data (
+# LLVM-NEXT:         0000: 00000000 |....|
+# LLVM-NEXT:       )
 # LLVM-NEXT:     }
 # LLVM-NEXT:   }
 # LLVM-NEXT: ]
 
 --- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
   Machine:         EM_X86_64
 Sections:
   - Name:            .note.ABI-tag
     Type:            SHT_NOTE
     AddressAlign:    0x0000000000000004
     Content:         040000000400000001000000474E550000000000

llvm/tools/llvm-readobj/ELFDumper.cpp

static std::string getGNUBuildId(ArrayRef<uint8_t> Desc) {
   return OS.str();
 }
 
 static StringRef getGNUGoldVersion(ArrayRef<uint8_t> Desc) {
   return StringRef(reinterpret_cast<const char *>(Desc.data()), Desc.size());
 }
 
 template <typename ELFT>
-static void printGNUNote(raw_ostream &OS, uint32_t NoteType,
+static bool printGNUNote(raw_ostream &OS, uint32_t NoteType,
                          ArrayRef<uint8_t> Desc) {
   switch (NoteType) {
   default:
-    return;
+    return false;
   case ELF::NT_GNU_ABI_TAG: {
     const GNUAbiTag &AbiTag = getGNUAbiTag<ELFT>(Desc);
-    if (!AbiTag.IsValid)
-      OS << "    <corrupt GNU_ABI_TAG>";
-    else
-      OS << "    OS: " << AbiTag.OSName << ", ABI: " << AbiTag.ABI;
+    if (!AbiTag.IsValid) {
+      OS << "    <corrupt GNU_ABI_TAG>\n";
+      return false;
+    }
     break;
   }
   case ELF::NT_GNU_BUILD_ID: {
     OS << "    Build ID: " << getGNUBuildId(Desc);
     break;
   }
   case ELF::NT_GNU_GOLD_VERSION:
     OS << "    Version: " << getGNUGoldVersion(Desc);
     break;
   case ELF::NT_GNU_PROPERTY_TYPE_0:
     OS << "    Properties:";
     for (const auto &Property : getGNUPropertyList<ELFT>(Desc))
       OS << "    " << Property << "\n";
     break;
   }
   OS << '\n';
+  return true;
 }
 
 struct AMDNote {
   std::string Type;
   std::string Value;
 };
 
 template <typename ELFT>
if (Name == "GNU") {
         OS << NoteType << '\n';
       else
         OS << "Unknown note type: (" << format_hex(Type, 10) << ")\n";
     }
 
     // Print the description, or fallback to printing raw bytes for unknown
     // owners.
     if (Name == "GNU") {
-      printGNUNote<ELFT>(OS, Type, Descriptor);
+      if (printGNUNote<ELFT>(OS, Type, Descriptor))
+        return;
     } else if (Name == "AMD") {
       const AMDNote N = getAMDNote<ELFT>(Type, Descriptor);
-      if (!N.Type.empty())
+      if (!N.Type.empty()) {
         OS << "    " << N.Type << ":\n        " << N.Value << '\n';
+        return;
+      }
     } else if (Name == "AMDGPU") {
       const AMDGPUNote N = getAMDGPUNote<ELFT>(Type, Descriptor);
-      if (!N.Type.empty())
+      if (!N.Type.empty()) {
         OS << "    " << N.Type << ":\n        " << N.Value << '\n';
+        return;
+      }
     } 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);
         else
           reportWarning(Note.takeError(), this->FileName);
+        return;
       }
-    } else if (!Descriptor.empty()) {
+    }
+    if (!Descriptor.empty()) {
       OS << "   description data:";
       for (uint8_t B : Descriptor)
         OS << " " << format("%02x", B);
       OS << '\n';
     }
   };
 
   ArrayRef<Elf_Shdr> Sections = unwrapOrError(this->FileName, Obj->sections());
if (NameOrErr) {
       continue;
     }
     reportWarning(NameOrErr.takeError(), this->FileName);
     W.printNumber("Sym", "<?>", Sym);
   }
 }
 
 template <typename ELFT>
-static void printGNUNoteLLVMStyle(uint32_t NoteType, ArrayRef<uint8_t> Desc,
+static bool printGNUNoteLLVMStyle(uint32_t NoteType, ArrayRef<uint8_t> Desc,
                                   ScopedPrinter &W) {
   switch (NoteType) {
   default:
-    return;
+    return false;
   case ELF::NT_GNU_ABI_TAG: {
     const GNUAbiTag &AbiTag = getGNUAbiTag<ELFT>(Desc);
     if (!AbiTag.IsValid) {
       W.printString("ABI", "<corrupt GNU_ABI_TAG>");
+      return false;
     } else {
       W.printString("OS", AbiTag.OSName);
       W.printString("ABI", AbiTag.ABI);
     }
     break;
   }
   case ELF::NT_GNU_BUILD_ID: {
     W.printString("Build ID", getGNUBuildId(Desc));
     break;
   }
   case ELF::NT_GNU_GOLD_VERSION:
     W.printString("Version", getGNUGoldVersion(Desc));
     break;
   case ELF::NT_GNU_PROPERTY_TYPE_0:
     ListScope D(W, "Property");
     for (const auto &Property : getGNUPropertyList<ELFT>(Desc))
       W.printString(Property);
     break;
   }
+  return true;
 }
 
 static void printCoreNoteLLVMStyle(const CoreNote &Note, ScopedPrinter &W) {
   W.printNumber("Page Size", Note.PageSize);
   for (const CoreFileMapping &Mapping : Note.Mappings) {
     ListScope D(W, "Mapping");
     W.printHex("Start", Mapping.Start);
     W.printHex("End", Mapping.End);
if (Name == "GNU") {
       else
         W.printString("Type",
                       "Unknown (" + to_string(format_hex(Type, 10)) + ")");
     }
 
     // Print the description, or fallback to printing raw bytes for unknown
     // owners.
     if (Name == "GNU") {
-      printGNUNoteLLVMStyle<ELFT>(Type, Descriptor, W);
+      if (printGNUNoteLLVMStyle<ELFT>(Type, Descriptor, W))
+        return;
     } else if (Name == "AMD") {
       const AMDNote N = getAMDNote<ELFT>(Type, Descriptor);
-      if (!N.Type.empty())
+      if (!N.Type.empty()) {
         W.printString(N.Type, N.Value);
+        return;
+      }
     } else if (Name == "AMDGPU") {
       const AMDGPUNote N = getAMDGPUNote<ELFT>(Type, Descriptor);
-      if (!N.Type.empty())
+      if (!N.Type.empty()) {
         W.printString(N.Type, N.Value);
+        return;
+      }
     } 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)
           printCoreNoteLLVMStyle(*Note, W);
         else
           reportWarning(Note.takeError(), this->FileName);
-      }
-    } else if (!Descriptor.empty()) {
+        return;
+      }
+    if (!Descriptor.empty()) {
       W.printBinaryBlock("Description data", Descriptor);
     }
   };
 
   ArrayRef<Elf_Shdr> Sections = unwrapOrError(this->FileName, Obj->sections());
   if (Obj->getHeader()->e_type != ELF::ET_CORE && !Sections.empty()) {
     for (const auto &S : Sections) {
       if (S.sh_type != SHT_NOTE)