[DebugInfo] UnwindTable::create() should not add empty rows to CFI unwind table

[RamNalamothu]

Summary

UnwindTable::parseRows() may return successfully if the CFIProgram has either
no CFI instructions or only DW_CFA_nop instructions and the UnwindRow return
argument will be empty. But currently, the callers are not checking for this case
which is leading to incorrect dumps in the unwind tables in such cases i.e.

CFA=unspecified

[RamNalamothu]

Greg,

I found this case while working on https://reviews.llvm.org/D76877.

BTW, would appreciate if you could take a look at https://reviews.llvm.org/D76877 and https://reviews.llvm.org/D76878. Feel free to add others if they would be interested as well.

[clayborg]

LGTM! Just had one nit code edit suggestion.

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

234–242

We can return early if we check if empty first to avoid a function call here.

[RamNalamothu]

Made the suggested code changes.

[RamNalamothu]

Thank you.

[RamNalamothu]

Does this look good now for commit?

[RamNalamothu]

Handle the scenario when the CIE or FDE has only DW_CFA_nop instructions.

[clayborg]

Yes, lgtm!

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

 }
 
 raw_ostream &llvm::dwarf::operator<<(raw_ostream &OS, const UnwindTable &Rows) {
   Rows.dump(OS, nullptr, false, 0);
   return OS;
 }
 
 Expected<UnwindTable> UnwindTable::create(const FDE *Fde) {
-  UnwindTable UT;
-  UnwindRow Row;
-  Row.setAddress(Fde->getInitialLocation());
-  UT.EndAddress = Fde->getInitialLocation() + Fde->getAddressRange();
-
   const CIE *Cie = Fde->getLinkedCIE();
   if (Cie == nullptr)
     return createStringError(errc::invalid_argument,
                              "unable to get CIE for FDE at offset 0x%" PRIx64,
                              Fde->getOffset());
 
+  // Rows will be empty if there are no CFI instructions.
+  if (Cie->cfis().empty() && Fde->cfis().empty())
+    return UnwindTable();
+
+  UnwindTable UT;
+  UnwindRow Row;
+  Row.setAddress(Fde->getInitialLocation());
+  UT.EndAddress = Fde->getInitialLocation() + Fde->getAddressRange();
   if (Error CieError = UT.parseRows(Cie->cfis(), Row, nullptr))
     return std::move(CieError);
   // We need to save the initial locations of registers from the CIE parsing
   // in case we run into DW_CFA_restore or DW_CFA_restore_extended opcodes.
   const RegisterLocations InitialLocs = Row.getRegisterLocations();
   if (Error FdeError = UT.parseRows(Fde->cfis(), Row, &InitialLocs))
     return std::move(FdeError);
+  // May be all the CFI instructions were DW_CFA_nop amd Row becomes empty.
+  // Do not add that to the unwind table.
+  if (Row.getRegisterLocations().hasLocations() ||
+      Row.getCFAValue().getLocation() != UnwindLocation::Unspecified)
     UT.Rows.push_back(Row);
   return UT;
 }
 
 Expected<UnwindTable> UnwindTable::create(const CIE *Cie) {
+  // Rows will be empty if there are no CFI instructions.
+  if (Cie->cfis().empty())
+    return UnwindTable();
+
   UnwindTable UT;
   UnwindRow Row;
   if (Error CieError = UT.parseRows(Cie->cfis(), Row, nullptr))
     return std::move(CieError);
+  // May be all the CFI instructions were DW_CFA_nop amd Row becomes empty.
+  // Do not add that to the unwind table.
+  if (Row.getRegisterLocations().hasLocations() ||
+      Row.getCFAValue().getLocation() != UnwindLocation::Unspecified)
     UT.Rows.push_back(Row);
   return UT;
 }
 
 // See DWARF standard v3, section 7.23
 const uint8_t DWARF_CFI_PRIMARY_OPCODE_MASK = 0xc0;
 const uint8_t DWARF_CFI_PRIMARY_OPERAND_MASK = 0x3f;
 
 Error CFIProgram::parse(DWARFDataExtractor Data, uint64_t *Offset,
                         uint64_t EndOffset) {

llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp

TEST(DWARFDebugFrame, RegisterLocations) {
   EXPECT_TRUE(Locs.hasLocations());
   expectDumpResult(Locs, "reg12=[CFA+4]");
   Locs.removeRegisterLocation(12);
   EXPECT_FALSE(Locs.getRegisterLocation(12).hasValue());
   EXPECT_FALSE(Locs.hasLocations());
   expectDumpResult(Locs, "");
 }
 
+// Test that empty rows are not added to UnwindTable when
+// dwarf::CIE::CFIs or dwarf::FDE::CFIs is empty.
+TEST(DWARFDebugFrame, UnwindTableEmptyRows) {
+  dwarf::CIE TestCIE = createCIE(/*IsDWARF64=*/false,
+                                 /*Offset=*/0x0,
+                                 /*Length=*/0xff);
+
+  // Having an empty instructions list is fine.
+  EXPECT_THAT_ERROR(parseCFI(TestCIE, {}), Succeeded());
+  EXPECT_TRUE(TestCIE.cfis().empty());
+
+  // Verify dwarf::UnwindTable::create() won't result in errors and
+  // and empty rows are not added to CIE UnwindTable.
+  Expected<dwarf::UnwindTable> RowsOrErr = dwarf::UnwindTable::create(&TestCIE);
+  EXPECT_THAT_ERROR(RowsOrErr.takeError(), Succeeded());
+  const size_t ExpectedNumOfRows = 0;
+  EXPECT_EQ(RowsOrErr->size(), ExpectedNumOfRows);
+
+  dwarf::FDE TestFDE(/*IsDWARF64=*/true,
+                     /*Offset=*/0x3333abcdabcd,
+                     /*Length=*/0x4444abcdabcd,
+                     /*CIEPointer=*/0x1111abcdabcd,
+                     /*InitialLocation=*/0x1000,
+                     /*AddressRange=*/0x1000,
+                     /*Cie=*/&TestCIE,
+                     /*LSDAAddress=*/None,
+                     /*Arch=*/Triple::x86_64);
+
+  // Having an empty instructions list is fine.
+  EXPECT_THAT_ERROR(parseCFI(TestFDE, {}), Succeeded());
+  EXPECT_TRUE(TestFDE.cfis().empty());
+
+  // Verify dwarf::UnwindTable::create() won't result in errors and
+  // and empty rows are not added to FDE UnwindTable.
+  RowsOrErr = dwarf::UnwindTable::create(&TestFDE);
+  EXPECT_THAT_ERROR(RowsOrErr.takeError(), Succeeded());
+  EXPECT_EQ(RowsOrErr->size(), ExpectedNumOfRows);
+}
+
+// Test that empty rows are not added to UnwindTable when dwarf::CIE::CFIs
+// or dwarf::FDE::CFIs is not empty but has only DW_CFA_nop instructions.
+TEST(DWARFDebugFrame, UnwindTableEmptyRows_NOPs) {
+  dwarf::CIE TestCIE = createCIE(/*IsDWARF64=*/false,
+                                 /*Offset=*/0x0,
+                                 /*Length=*/0xff);
+
+  // Make a CIE that has only DW_CFA_nop instructions.
+  EXPECT_THAT_ERROR(parseCFI(TestCIE, {dwarf::DW_CFA_nop}), Succeeded());
+  EXPECT_TRUE(!TestCIE.cfis().empty());
+
+  // Verify dwarf::UnwindTable::create() won't result in errors and
+  // and empty rows are not added to CIE UnwindTable.
+  Expected<dwarf::UnwindTable> RowsOrErr = dwarf::UnwindTable::create(&TestCIE);
+  EXPECT_THAT_ERROR(RowsOrErr.takeError(), Succeeded());
+  const size_t ExpectedNumOfRows = 0;
+  EXPECT_EQ(RowsOrErr->size(), ExpectedNumOfRows);
+
+  dwarf::FDE TestFDE(/*IsDWARF64=*/true,
+                     /*Offset=*/0x3333abcdabcd,
+                     /*Length=*/0x4444abcdabcd,
+                     /*CIEPointer=*/0x1111abcdabcd,
+                     /*InitialLocation=*/0x1000,
+                     /*AddressRange=*/0x1000,
+                     /*Cie=*/&TestCIE,
+                     /*LSDAAddress=*/None,
+                     /*Arch=*/Triple::x86_64);
+
+  // Make an FDE that has only DW_CFA_nop instructions.
+  EXPECT_THAT_ERROR(parseCFI(TestFDE, {dwarf::DW_CFA_nop}), Succeeded());
+  EXPECT_TRUE(!TestFDE.cfis().empty());
+
+  // Verify dwarf::UnwindTable::create() won't result in errors and
+  // and empty rows are not added to FDE UnwindTable.
+  RowsOrErr = dwarf::UnwindTable::create(&TestFDE);
+  EXPECT_THAT_ERROR(RowsOrErr.takeError(), Succeeded());
+  EXPECT_EQ(RowsOrErr->size(), ExpectedNumOfRows);
+}
+
 TEST(DWARFDebugFrame, UnwindTableErrorNonAscendingFDERows) {
   dwarf::CIE TestCIE = createCIE(/*IsDWARF64=*/false,
                                  /*Offset=*/0x0,
                                  /*Length=*/0xff);
 
   dwarf::FDE TestFDE(/*IsDWARF64=*/true,
                      /*Offset=*/0x3333abcdabcd,
                      /*Length=*/0x4444abcdabcd,