[Propeller] Use a bit-field struct for the metdata fields of BBEntry.

[rahmanl]

Summary

This patch encapsulates the encoding and decoding logic of basic block metadata into the Metadata struct, and also reduces the decoded size of SHT_LLVM_BB_ADDR_MAP section.

The patch would've looked more readable if we could use designated initializer, but that is a c++20 feature.

[rahmanl]

clang-format.

[rahmanl]

cleanup.

[rahmanl]

WIP.

[rahmanl]

Rebase and add comments.

[rahmanl]

clang-format.

[rahmanl]

clang-format.

[rahmanl]

Remove old accessors.

[rahmanl]

Use a bitwise equality operator, since memcmp won't work when padding is involved.

[rahmanl]

clang-format.

[rahmanl]

• cleanup.

[jhenderson]

llvm/include/llvm/Object/ELFTypes.h

853

Nit: use static_cast<unsigned>

855

Nit: blank line after method.

857

Could you add some simple unit tests for the encode/decode methods, please? They should be trivial to write, but I think would just help ensure there are no surprises. I think it would be sufficient to cover all 4 fields set individually, and also all fields set and no fields set.

863

FWIW, I'd find it more readable if you declared the struct, and then declared the member on a separate line i.e:

struct Metadata {
  ...
};

Metadata MD;

865–868

I'm not sure these should be moved. Typically, the constructor comes before other methods, from what I've seen, and it would reduce the size of the diff.

[rahmanl]

Add unittest for encoding and decoding.

[rahmanl]

clang-format.

[rahmanl]

• clang-format.

[jhenderson]

llvm/include/llvm/Object/ELFTypes.h

807–811

Nit: I'd add blank lines either side of this method.

813

Thinking about it, it's probably more appropriate for this to return a fixed width type (i.e. uint32_t) and similarly for the casts to cast to that.

Strictly speaking, I don't think there's any guarantee that unsigned is 4 bytes in size, plus it expresses the intent a little better in my opinion.

821

As above, uint32_t would be more appropriate here.

llvm/unittests/Object/ELFTypesTest.cpp

67

It might be interesting to show what happens when you provide decode with a value that includes bits that aren't related to any of the properties. For example, 0xffffffff.

74

What's the reason for these two being static? It's not modifiable, so I don't think it benefits anything, since it's already function-local.

75

These two loops should use size_t since that's what size returns.

[rahmanl]

• Add error handling for invalid encoding.
• Add llvm-readobj test.

[rahmanl]

llvm/unittests/Object/ELFTypesTest.cpp

67

Good point. I added a check to enforce 1-to-1 encoding.

[rahmanl]

@jhenderson Thanks for reviewing. Please let me know if I have addressed your comments.

[rahmanl]

• clang-format.

[rahmanl]

• Add error handling for invalid encoding.
• Add llvm-readobj test.

[rahmanl]

@jhenderson Another friendly ping.

[jhenderson]

Apologies for the delay: high priority "regular" work, combined with time off of various descriptions means my reviewing has mostly taken a back seat. LGTM.

llvm/lib/Object/ELF.cpp

747–748

Comment needs updating.

[rahmanl]

Update the comment.

[rahmanl]

Thank you for the review.

llvm/include/llvm/Object/ELFTypes.h

template <class ELFT> struct Elf_Mips_ABIFlags {
   Elf_Word flags2;   // General flags
 };
 
 // Struct representing the BBAddrMap for one function.
 struct BBAddrMap {
   uint64_t Addr; // Function address
   // Struct representing the BBAddrMap information for one basic block.
   struct BBEntry {
+    struct Metadata {
+      bool HasReturn : 1;   // If this block ends with a return (or tail call).
+      bool HasTailCall : 1; // If this block ends with a tail call.
+      bool IsEHPad : 1;     // If this is an exception handling block.
+      bool CanFallThrough : 1; // If this block can fall through to its next.
+
+      bool operator==(const Metadata &Other) const {
+        return HasReturn == Other.HasReturn &&
+               HasTailCall == Other.HasTailCall && IsEHPad == Other.IsEHPad &&
+               CanFallThrough == Other.CanFallThrough;
+      }
+
+      // Encodes this struct as a uint32_t value.
+      uint32_t encode() const {
+        return static_cast<uint32_t>(HasReturn) |
+               (static_cast<uint32_t>(HasTailCall) << 1) |
+               (static_cast<uint32_t>(IsEHPad) << 2) |
+               (static_cast<uint32_t>(CanFallThrough) << 3);
+      }
+
+      // Decodes and returns a Metadata struct from a uint32_t value.
+      static Expected<Metadata> decode(uint32_t V) {
+        Metadata MD{/*HasReturn=*/static_cast<bool>(V & 1),
+                    /*HasTailCall=*/static_cast<bool>(V & (1 << 1)),
+                    /*IsEHPad=*/static_cast<bool>(V & (1 << 2)),
+                    /*CanFallThrough=*/static_cast<bool>(V & (1 << 3))};
+        if (MD.encode() != V)
+          return createStringError(
+              std::error_code(), "invalid encoding for BBEntry::Metadata: 0x%x",
+              V);
+        return MD;
+      }
+    };
+
     uint32_t ID;     // Unique ID of this basic block.
     uint32_t Offset; // Offset of basic block relative to function start.
     uint32_t Size;   // Size of the basic block.
+    Metadata MD;     // Metdata for this basic block.
 
-    // The following fields are decoded from the Metadata field. The encoding
-    // happens in AsmPrinter.cpp:getBBAddrMapMetadata.
-    bool HasReturn;      // If this block ends with a return (or tail call).
-    bool HasTailCall;    // If this block ends with a tail call.
-    bool IsEHPad;        // If this is an exception handling block.
-    bool CanFallThrough; // If this block can fall through to its next.
-
-    BBEntry(uint32_t ID, uint32_t Offset, uint32_t Size, uint32_t Metadata)
-        : ID(ID), Offset(Offset), Size(Size), HasReturn(Metadata & 1),
-          HasTailCall(Metadata & (1 << 1)), IsEHPad(Metadata & (1 << 2)),
-          CanFallThrough(Metadata & (1 << 3)){};
+    BBEntry(uint32_t ID, uint32_t Offset, uint32_t Size, Metadata MD)
+        : ID(ID), Offset(Offset), Size(Size), MD(MD){};
 
     bool operator==(const BBEntry &Other) const {
       return ID == Other.ID && Offset == Other.Offset && Size == Other.Size &&
-             HasReturn == Other.HasReturn && HasTailCall == Other.HasTailCall &&
-             IsEHPad == Other.IsEHPad && CanFallThrough == Other.CanFallThrough;
+             MD == Other.MD;
     }
 
-    bool hasReturn() const { return HasReturn; }
-    bool hasTailCall() const { return HasTailCall; }
-    bool isEHPad() const { return IsEHPad; }
-    bool canFallThrough() const { return CanFallThrough; }
+    bool hasReturn() const { return MD.HasReturn; }
+    bool hasTailCall() const { return MD.HasTailCall; }
+    bool isEHPad() const { return MD.IsEHPad; }
+    bool canFallThrough() const { return MD.CanFallThrough; }
   };
   std::vector<BBEntry> BBEntries; // Basic block entries for this function.
 
   // Equality operator for unit testing.
   bool operator==(const BBAddrMap &Other) const {
     return Addr == Other.Addr && std::equal(BBEntries.begin(), BBEntries.end(),
                                             Other.BBEntries.begin());
   }
 };
 
 } // end namespace object.
 } // end namespace llvm.
 
 #endif // LLVM_OBJECT_ELFTYPES_H

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

 #include "llvm/MC/MCSectionMachO.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/MCSymbolELF.h"
 #include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/MCValue.h"
 #include "llvm/MC/SectionKind.h"
+#include "llvm/Object/ELFTypes.h"
 #include "llvm/Pass.h"
 #include "llvm/Remarks/RemarkStreamer.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/MathExtras.h"
void AsmPrinter::emitFrameAlloc(const MachineInstr &MI) {
   MCSymbol *FrameAllocSym = MI.getOperand(0).getMCSymbol();
   int FrameOffset = MI.getOperand(1).getImm();
 
   // Emit a symbol assignment.
   OutStreamer->emitAssignment(FrameAllocSym,
                              MCConstantExpr::create(FrameOffset, OutContext));
 }
 
-/// Returns the BB metadata to be emitted in the .llvm_bb_addr_map section for a
-/// given basic block. This can be used to capture more precise profile
-/// information. We use the last 4 bits (LSBs) to encode the following
-/// information:
-///  * (1): set if return block (ret or tail call).
-///  * (2): set if ends with a tail call.
-///  * (3): set if exception handling (EH) landing pad.
-///  * (4): set if the block can fall through to its next.
-/// The remaining bits are zero.
-static unsigned getBBAddrMapMetadata(const MachineBasicBlock &MBB) {
+/// Returns the BB metadata to be emitted in the SHT_LLVM_BB_ADDR_MAP section
+/// for a given basic block. This can be used to capture more precise profile
+/// information.
+static uint32_t getBBAddrMapMetadata(const MachineBasicBlock &MBB) {
   const TargetInstrInfo *TII = MBB.getParent()->getSubtarget().getInstrInfo();
-  return ((unsigned)MBB.isReturnBlock()) |
-         ((!MBB.empty() && TII->isTailCall(MBB.back())) << 1) |
-         (MBB.isEHPad() << 2) |
-         (const_cast<MachineBasicBlock &>(MBB).canFallThrough() << 3);
+  return object::BBAddrMap::BBEntry::Metadata{
+      MBB.isReturnBlock(), !MBB.empty() && TII->isTailCall(MBB.back()),
+      MBB.isEHPad(), const_cast<MachineBasicBlock &>(MBB).canFallThrough()}
+      .encode();
 }
 
 void AsmPrinter::emitBBAddrMapSection(const MachineFunction &MF) {
   MCSection *BBAddrMapSection =
       getObjFileLowering().getBBAddrMapSection(*MF.getSection());
   assert(BBAddrMapSection && ".llvm_bb_addr_map section is not initialized.");
 
   const MCSymbol *FunctionSymbol = getFunctionBegin();

llvm/lib/Object/ELF.cpp

ELFFile<ELFT>::decodeBBAddrMap(const Elf_Shdr &Sec,
   if (!ContentsOrErr)
     return ContentsOrErr.takeError();
   ArrayRef<uint8_t> Content = *ContentsOrErr;
   DataExtractor Data(Content, isLE(), ELFT::Is64Bits ? 8 : 4);
   std::vector<BBAddrMap> FunctionEntries;
 
   DataExtractor::Cursor Cur(0);
   Error ULEBSizeErr = Error::success();
+  Error MetadataDecodeErr = Error::success();
   // Helper to extract and decode the next ULEB128 value as uint32_t.
   // Returns zero and sets ULEBSizeErr if the ULEB128 value exceeds the uint32_t
   // limit.
   // Also returns zero if ULEBSizeErr is already in an error state.
   auto ReadULEB128AsUInt32 = [&Data, &Cur, &ULEBSizeErr]() -> uint32_t {
     // Bail out and do not extract data if ULEBSizeErr is already set.
     if (ULEBSizeErr)
       return 0;
     uint64_t Offset = Cur.tell();
     uint64_t Value = Data.getULEB128(Cur);
     if (Value > UINT32_MAX) {
       ULEBSizeErr = createError(
           "ULEB128 value at offset 0x" + Twine::utohexstr(Offset) +
           " exceeds UINT32_MAX (0x" + Twine::utohexstr(Value) + ")");
       return 0;
     }
     return static_cast<uint32_t>(Value);
   };
 
   uint8_t Version = 0;
-  while (!ULEBSizeErr && Cur && Cur.tell() < Content.size()) {
+  while (!ULEBSizeErr && !MetadataDecodeErr && Cur &&
+         Cur.tell() < Content.size()) {
     if (Sec.sh_type == ELF::SHT_LLVM_BB_ADDR_MAP) {
       Version = Data.getU8(Cur);
       if (!Cur)
         break;
       if (Version > 2)
         return createError("unsupported SHT_LLVM_BB_ADDR_MAP version: " +
                            Twine(static_cast<int>(Version)));
       Data.getU8(Cur); // Feature byte
if (IsRelocatable) {
                            describe(*this, Sec));
       }
       Address = FOTIterator->second;
     }
     uint32_t NumBlocks = ReadULEB128AsUInt32();
     std::vector<BBAddrMap::BBEntry> BBEntries;
     uint32_t PrevBBEndOffset = 0;
     for (uint32_t BlockIndex = 0;
-         !ULEBSizeErr && Cur && (BlockIndex < NumBlocks); ++BlockIndex) {
+         !MetadataDecodeErr && !ULEBSizeErr && Cur && (BlockIndex < NumBlocks);
+         ++BlockIndex) {
       uint32_t ID = Version >= 2 ? ReadULEB128AsUInt32() : BlockIndex;
       uint32_t Offset = ReadULEB128AsUInt32();
       uint32_t Size = ReadULEB128AsUInt32();
-      uint32_t Metadata = ReadULEB128AsUInt32();
+      uint32_t MD = ReadULEB128AsUInt32();
       if (Version >= 1) {
         // Offset is calculated relative to the end of the previous BB.
         Offset += PrevBBEndOffset;
         PrevBBEndOffset = Offset + Size;
       }
-      BBEntries.push_back({ID, Offset, Size, Metadata});
+      Expected<BBAddrMap::BBEntry::Metadata> MetadataOrErr =
+          BBAddrMap::BBEntry::Metadata::decode(MD);
+      if (!MetadataOrErr) {
+        MetadataDecodeErr = MetadataOrErr.takeError();
+        break;
+      }
+      BBEntries.push_back({ID, Offset, Size, *MetadataOrErr});
     }
     FunctionEntries.push_back({Address, std::move(BBEntries)});
   }
-  // Either Cur is in the error state, or ULEBSizeError is set (not both), but
-  // we join the two errors here to be safe.
-  if (!Cur || ULEBSizeErr)
-    return joinErrors(Cur.takeError(), std::move(ULEBSizeErr));
+  // Either Cur is in the error state, or we have an error in ULEBSizeErr or
+  // MetadataDecodeErr (but not both), but we join all errors here to be safe.
+  if (!Cur || ULEBSizeErr || MetadataDecodeErr)
+    return joinErrors(joinErrors(Cur.takeError(), std::move(ULEBSizeErr)),
+                      std::move(MetadataDecodeErr));
   return FunctionEntries;
 }
 
 template <class ELFT>
 Expected<
     MapVector<const typename ELFT::Shdr *, const typename ELFT::Shdr *>>
 ELFFile<ELFT>::getSectionAndRelocations(
     std::function<Expected<bool>(const Elf_Shdr &)> IsMatch) const {

llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test

 # RUN: yaml2obj --docnum=1 %s -DBITS=32 -o %t1.x32.o
 # RUN: llvm-readobj %t1.x32.o --bb-addr-map 2>&1 | FileCheck -DADDR=0x11111 %s -DFILE=%t1.x32.o --check-prefix=CHECK
 # RUN: llvm-readelf %t1.x32.o --bb-addr-map | FileCheck %s --check-prefix=GNU
 
 ## Check that a malformed section can be handled.
 # RUN: yaml2obj --docnum=1 %s -DBITS=32 -DSIZE=6 -o %t2.o
 # RUN: llvm-readobj %t2.o --bb-addr-map 2>&1 | FileCheck %s -DOFFSET=0x00000006 -DFILE=%t2.o --check-prefix=TRUNCATED
 
+## Check that invalid metadata can be handled.
+# RUN: yaml2obj --docnum=1 %s -DBITS=32 -DMETADATA=0xF000002 -o %t3.o
+# RUN: llvm-readobj %t3.o --bb-addr-map 2>&1 | FileCheck %s -DFILE=%t3.o --check-prefix=INVALIDMD
+
 # CHECK:      BBAddrMap [
 # CHECK-NEXT:   Function {
 # CHECK-NEXT:     At: [[ADDR]]
 # CHECK-NEXT: warning: '[[FILE]]': could not identify function symbol for address ([[ADDR]]) in SHT_LLVM_BB_ADDR_MAP section with index 3
 # CHECK-NEXT:     Name: <?>
 # CHECK-NEXT:     BB entries [
 # CHECK-NEXT:       {
 # CHECK-NEXT:         ID: 0

 # TRUNCATED-NEXT:         HasTailCall: Yes
 # TRUNCATED-NEXT:         IsEHPad: Yes
 # TRUNCATED-NEXT:         CanFallThrough: Yes
 # TRUNCATED-NEXT:       }
 # TRUNCATED-NEXT:     ]
 # TRUNCATED-NEXT:   }
 # TRUNCATED-NEXT: ]
 
+# INVALIDMD:      BBAddrMap [
+# INVALIDMD-NEXT:   warning: '[[FILE]]': unable to dump SHT_LLVM_BB_ADDR_MAP section with index 3: invalid encoding for BBEntry::Metadata: 0xf000002
+
 --- !ELF
 FileHeader:
   Class: ELFCLASS[[BITS]]
   Data:  ELFDATA2LSB
   Type:  ET_EXEC
 Sections:
   - Name:   .text
     Type:   SHT_PROGBITS
     Flags:  [SHF_ALLOC]
   - Name:   .text.bar
     Type:   SHT_PROGBITS
     Flags:  [SHF_ALLOC]
   - Name:   .llvm_bb_addr_map
     Type:   SHT_LLVM_BB_ADDR_MAP
     ShSize: [[SIZE=<none>]]
     Link:   .text
     Entries:
       - Version: 2
         Address: [[ADDR=0x11111]]
         BBEntries:
           - ID:            0
             AddressOffset: 0x0
             Size:          0x1
-            Metadata:      0xF0000002
+            Metadata:      [[METADATA=0x2]]
           - ID:            2
             AddressOffset: 0x3
             Size:          0x4
             Metadata:      0x5
       - Version: 2
         Address: 0x22222
         BBEntries:
           - ID:            4

llvm/unittests/Object/ELFObjectFileTest.cpp

Entries:
         Address: 0x44444
         BBEntries:
           - ID:            0
             AddressOffset: 0x0
             Size:          0x4
             Metadata:      0x8
 )");
 
-  BBAddrMap E1 = {0x11111, {{1, 0x0, 0x1, 0x2}}};
-  BBAddrMap E2 = {0x22222, {{2, 0x0, 0x2, 0x4}}};
-  BBAddrMap E3 = {0x33333, {{0, 0x0, 0x3, 0x6}}};
-  BBAddrMap E4 = {0x44444, {{0, 0x0, 0x4, 0x8}}};
+  BBAddrMap E1 = {0x11111, {{1, 0x0, 0x1, {false, true, false, false}}}};
+  BBAddrMap E2 = {0x22222, {{2, 0x0, 0x2, {false, false, true, false}}}};
+  BBAddrMap E3 = {0x33333, {{0, 0x0, 0x3, {false, true, true, false}}}};
+  BBAddrMap E4 = {0x44444, {{0, 0x0, 0x4, {false, false, false, true}}}};
 
   std::vector<BBAddrMap> Section0BBAddrMaps = {E4};
   std::vector<BBAddrMap> Section1BBAddrMaps = {E3};
   std::vector<BBAddrMap> Section2BBAddrMaps = {E1, E2};
   std::vector<BBAddrMap> AllBBAddrMaps = {E1, E2, E3, E4};
 
   auto DoCheckSucceeds = [&](StringRef YamlString,
                              std::optional<unsigned> TextSectionIndex,

llvm/unittests/Object/ELFTypesTest.cpp

 //===----------------------- ELFTypesTest.cpp -----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "llvm/Object/ELFTypes.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 #include <iostream>
 
 using namespace llvm;
 using namespace llvm::object;
 
 template <typename ELFT> using Elf_Note = typename ELFT::Note;
 
TEST(ELFTypesTest, NoteTest) {
 
   auto Note2 = TestData.getElfNote("", ELF::NT_AMDGPU_METADATA, RandomData);
   EXPECT_EQ(Note2.getName(), "");
 
   auto Note3 =
       TestData.getElfNote("AMD", ELF::NT_AMDGPU_METADATA, ArrayRef<uint8_t>());
   EXPECT_EQ(Note3.getDescAsStringRef(4), StringRef(""));
 }
+
+TEST(ELFTypesTest, BBEntryMetadataEncodingTest) {
+  const std::array<BBAddrMap::BBEntry::Metadata, 6> Decoded = {
+      {{false, false, false, false},
+       {true, false, false, false},
+       {false, true, false, false},
+       {false, false, true, false},
+       {false, false, false, true},
+       {true, true, true, true}}};
+  const std::array<uint32_t, 6> Encoded = {{0, 1, 2, 4, 8, 15}};
+  for (size_t i = 0; i < Decoded.size(); ++i)
+    EXPECT_EQ(Decoded[i].encode(), Encoded[i]);
+  for (size_t i = 0; i < Encoded.size(); ++i) {
+    Expected<BBAddrMap::BBEntry::Metadata> MetadataOrError =
+        BBAddrMap::BBEntry::Metadata::decode(Encoded[i]);
+    ASSERT_THAT_EXPECTED(MetadataOrError, Succeeded());
+    EXPECT_EQ(*MetadataOrError, Decoded[i]);
+  }
+}
+
+TEST(ELFTypesTest, BBEntryMetadataInvalidEncodingTest) {
+  const std::array<std::string, 2> Errors = {
+      "invalid encoding for BBEntry::Metadata: 0xffff",
+      "invalid encoding for BBEntry::Metadata: 0x100001"};
+  const std::array<uint32_t, 2> Values = {{0xFFFF, 0x100001}};
+  for (size_t i = 0; i < Values.size(); ++i) {
+    EXPECT_THAT_ERROR(
+        BBAddrMap::BBEntry::Metadata::decode(Values[i]).takeError(),
+        FailedWithMessage(Errors[i]));
+  }
+}