[NFC][object] Change the input parameter of the method isDebugSection.

[Esme]

Summary

This is a NFC patch to change the input parameter of the method SectionRef::isDebugSection(), by replacing the StringRef SectionName with DataRefImpl Sec. This allows us to determine if a section is debug type in more ways than just by section name.

[qiucf]

You added implementation of XCOFFObjectFile::isDebugSection, which does not exist before your patch (besides, its implementation is not as 'trivial' as others). Are you sure this is an NFC?

[Esme]

In D102601#2762856, @qiucf wrote:

You added implementation of XCOFFObjectFile::isDebugSection, which does not exist before your patch (besides, its implementation is not as 'trivial' as others). Are you sure this is an NFC?

I understand your concern, hmm..., I will move this part to the child revision D102603 where this method is really used. Thanks!

[jhenderson]

llvm/lib/Object/MachOObjectFile.cpp

2050

I recognise that this is the common pattern used elsewhere in this file, but if I'm not mistaken, it can lead to an unhandled Error should getSectionName ever fail, which is an assertion when certain options are enabled.

At the very least, there needs to be a consumeError(SectionNameOrErr.takeError()) line after the if, with a TODO saying to report the error message properly. If you can prove that getSectionName(Sec) never returns an error, you can use cantFail and avoid the TODO instead.

Same applies to each of the other formats.

llvm/lib/Object/ObjectFile.cpp

96–97

Nit: clang-format as directed.

[Esme]

Address comments.

[jhenderson]

llvm/lib/Object/COFFObjectFile.cpp

335

You need to return after this consumeError, as you can't safely retrieve the name from a failed Expected. Same applies in the other file formats.

[Esme]

Address comment.

[jhenderson]

LGTM!

llvm/include/llvm/Object/COFF.h

protected:
   Expected<ArrayRef<uint8_t>>
   getSectionContents(DataRefImpl Sec) const override;
   uint64_t getSectionAlignment(DataRefImpl Sec) const override;
   bool isSectionCompressed(DataRefImpl Sec) const override;
   bool isSectionText(DataRefImpl Sec) const override;
   bool isSectionData(DataRefImpl Sec) const override;
   bool isSectionBSS(DataRefImpl Sec) const override;
   bool isSectionVirtual(DataRefImpl Sec) const override;
-  bool isDebugSection(StringRef SectionName) const override;
+  bool isDebugSection(DataRefImpl Sec) const override;
   relocation_iterator section_rel_begin(DataRefImpl Sec) const override;
   relocation_iterator section_rel_end(DataRefImpl Sec) const override;
 
   void moveRelocationNext(DataRefImpl &Rel) const override;
   uint64_t getRelocationOffset(DataRefImpl Rel) const override;
   symbol_iterator getRelocationSymbol(DataRefImpl Rel) const override;
   uint64_t getRelocationType(DataRefImpl Rel) const override;
   void getRelocationTypeName(DataRefImpl Rel,

llvm/include/llvm/Object/ELFObjectFile.h

protected:
   uint64_t getSectionAlignment(DataRefImpl Sec) const override;
   bool isSectionCompressed(DataRefImpl Sec) const override;
   bool isSectionText(DataRefImpl Sec) const override;
   bool isSectionData(DataRefImpl Sec) const override;
   bool isSectionBSS(DataRefImpl Sec) const override;
   bool isSectionVirtual(DataRefImpl Sec) const override;
   bool isBerkeleyText(DataRefImpl Sec) const override;
   bool isBerkeleyData(DataRefImpl Sec) const override;
-  bool isDebugSection(StringRef SectionName) const override;
+  bool isDebugSection(DataRefImpl Sec) const override;
   relocation_iterator section_rel_begin(DataRefImpl Sec) const override;
   relocation_iterator section_rel_end(DataRefImpl Sec) const override;
   std::vector<SectionRef> dynamic_relocation_sections() const override;
   Expected<section_iterator>
   getRelocatedSection(DataRefImpl Sec) const override;
 
   void moveRelocationNext(DataRefImpl &Rel) const override;
   uint64_t getRelocationOffset(DataRefImpl Rel) const override;

 template <class ELFT>
 bool ELFObjectFile<ELFT>::isBerkeleyData(DataRefImpl Sec) const {
   const Elf_Shdr *EShdr = getSection(Sec);
   return !isBerkeleyText(Sec) && EShdr->sh_type != ELF::SHT_NOBITS &&
          EShdr->sh_flags & ELF::SHF_ALLOC;
 }
 
 template <class ELFT>
-bool ELFObjectFile<ELFT>::isDebugSection(StringRef SectionName) const {
+bool ELFObjectFile<ELFT>::isDebugSection(DataRefImpl Sec) const {
+  Expected<StringRef> SectionNameOrErr = getSectionName(Sec);
+  if (!SectionNameOrErr) {
+    // TODO: Report the error message properly.
+    consumeError(SectionNameOrErr.takeError());
+    return false;
+  }
+  StringRef SectionName = SectionNameOrErr.get();
   return SectionName.startswith(".debug") ||
          SectionName.startswith(".zdebug") || SectionName == ".gdb_index";
 }
 
 template <class ELFT>
 relocation_iterator
 ELFObjectFile<ELFT>::section_rel_begin(DataRefImpl Sec) const {
   DataRefImpl RelData;

llvm/include/llvm/Object/MachO.h

public:
   Expected<SectionRef> getSection(unsigned SectionIndex) const;
   Expected<SectionRef> getSection(StringRef SectionName) const;
   bool isSectionCompressed(DataRefImpl Sec) const override;
   bool isSectionText(DataRefImpl Sec) const override;
   bool isSectionData(DataRefImpl Sec) const override;
   bool isSectionBSS(DataRefImpl Sec) const override;
   bool isSectionVirtual(DataRefImpl Sec) const override;
   bool isSectionBitcode(DataRefImpl Sec) const override;
-  bool isDebugSection(StringRef SectionName) const override;
+  bool isDebugSection(DataRefImpl Sec) const override;
 
   /// When dsymutil generates the companion file, it strips all unnecessary
   /// sections (e.g. everything in the _TEXT segment) by omitting their body
   /// and setting the offset in their corresponding load command to zero.
   ///
   /// While the load command itself is valid, reading the section corresponds
   /// to reading the number of bytes specified in the load command, starting
   /// from offset 0 (i.e. the Mach-O header at the beginning of the file).

llvm/include/llvm/Object/ObjectFile.h

public:
   /// contains either code or readonly data.
   bool isBerkeleyText() const;
   /// Whether this section will be placed in the data segment, according to the
   /// Berkeley size format. This is true if the section is allocatable and
   /// contains data (e.g. PROGBITS), but is not text.
   bool isBerkeleyData() const;
 
   /// Whether this section is a debug section.
-  bool isDebugSection(StringRef SectionName) const;
+  bool isDebugSection() const;
 
   bool containsSymbol(SymbolRef S) const;
 
   relocation_iterator relocation_begin() const;
   relocation_iterator relocation_end() const;
   iterator_range<relocation_iterator> relocations() const {
     return make_range(relocation_begin(), relocation_end());
   }
protected:
   virtual bool isSectionData(DataRefImpl Sec) const = 0;
   virtual bool isSectionBSS(DataRefImpl Sec) const = 0;
   // A section is 'virtual' if its contents aren't present in the object image.
   virtual bool isSectionVirtual(DataRefImpl Sec) const = 0;
   virtual bool isSectionBitcode(DataRefImpl Sec) const;
   virtual bool isSectionStripped(DataRefImpl Sec) const;
   virtual bool isBerkeleyText(DataRefImpl Sec) const;
   virtual bool isBerkeleyData(DataRefImpl Sec) const;
-  virtual bool isDebugSection(StringRef SectionName) const;
+  virtual bool isDebugSection(DataRefImpl Sec) const;
   virtual relocation_iterator section_rel_begin(DataRefImpl Sec) const = 0;
   virtual relocation_iterator section_rel_end(DataRefImpl Sec) const = 0;
   virtual Expected<section_iterator> getRelocatedSection(DataRefImpl Sec) const;
 
   // Same as above for RelocationRef.
   friend class RelocationRef;
   virtual void moveRelocationNext(DataRefImpl &Rel) const = 0;
   virtual uint64_t getRelocationOffset(DataRefImpl Rel) const = 0;

 inline bool SectionRef::isBerkeleyText() const {
   return OwningObject->isBerkeleyText(SectionPimpl);
 }
 
 inline bool SectionRef::isBerkeleyData() const {
   return OwningObject->isBerkeleyData(SectionPimpl);
 }
 
-inline bool SectionRef::isDebugSection(StringRef SectionName) const {
-  return OwningObject->isDebugSection(SectionName);
+inline bool SectionRef::isDebugSection() const {
+  return OwningObject->isDebugSection(SectionPimpl);
 }
 
 inline relocation_iterator SectionRef::relocation_begin() const {
   return OwningObject->section_rel_begin(SectionPimpl);
 }
 
 inline relocation_iterator SectionRef::relocation_end() const {
   return OwningObject->section_rel_end(SectionPimpl);

llvm/lib/Object/COFFObjectFile.cpp

bool COFFObjectFile::isSectionBSS(DataRefImpl Ref) const {
   const uint32_t BssFlags = COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA |
                             COFF::IMAGE_SCN_MEM_READ |
                             COFF::IMAGE_SCN_MEM_WRITE;
   return (Sec->Characteristics & BssFlags) == BssFlags;
 }
 
 // The .debug sections are the only debug sections for COFF
 // (\see MCObjectFileInfo.cpp).
-bool COFFObjectFile::isDebugSection(StringRef SectionName) const {
+bool COFFObjectFile::isDebugSection(DataRefImpl Ref) const {
+  Expected<StringRef> SectionNameOrErr = getSectionName(Ref);
+  if (!SectionNameOrErr) {
+    // TODO: Report the error message properly.
+    consumeError(SectionNameOrErr.takeError());
+    return false;
+  }
+  StringRef SectionName = SectionNameOrErr.get();
   return SectionName.startswith(".debug");
 }
 
 unsigned COFFObjectFile::getSectionID(SectionRef Sec) const {
   uintptr_t Offset =
       Sec.getRawDataRefImpl().p - reinterpret_cast<uintptr_t>(SectionTable);
   assert((Offset % sizeof(coff_section)) == 0);
   return (Offset / sizeof(coff_section)) + 1;

llvm/lib/Object/MachOObjectFile.cpp

 bool MachOObjectFile::isSectionBSS(DataRefImpl Sec) const {
   uint32_t Flags = getSectionFlags(*this, Sec);
   unsigned SectionType = Flags & MachO::SECTION_TYPE;
   return !(Flags & MachO::S_ATTR_PURE_INSTRUCTIONS) &&
          (SectionType == MachO::S_ZEROFILL ||
           SectionType == MachO::S_GB_ZEROFILL);
 }
 
-bool MachOObjectFile::isDebugSection(StringRef SectionName) const {
+bool MachOObjectFile::isDebugSection(DataRefImpl Sec) const {
+  Expected<StringRef> SectionNameOrErr = getSectionName(Sec);
+  if (!SectionNameOrErr) {
+    // TODO: Report the error message properly.
+    consumeError(SectionNameOrErr.takeError());
+    return false;
+  }
+  StringRef SectionName = SectionNameOrErr.get();
   return SectionName.startswith("__debug") ||
          SectionName.startswith("__zdebug") ||
          SectionName.startswith("__apple") || SectionName == "__gdb_index" ||
          SectionName == "__swift_ast";
 }
 
 unsigned MachOObjectFile::getSectionID(SectionRef Sec) const {
   return Sec.getRawDataRefImpl().d.a;
 }
 
 bool MachOObjectFile::isSectionVirtual(DataRefImpl Sec) const {
   uint32_t Flags = getSectionFlags(*this, Sec);
   unsigned SectionType = Flags & MachO::SECTION_TYPE;

llvm/lib/Object/ObjectFile.cpp

 
 bool ObjectFile::isBerkeleyText(DataRefImpl Sec) const {
   return isSectionText(Sec);
 }
 
 bool ObjectFile::isBerkeleyData(DataRefImpl Sec) const {
   return isSectionData(Sec);
 }
 
-bool ObjectFile::isDebugSection(StringRef SectionName) const {
-  return false;
-}
+bool ObjectFile::isDebugSection(DataRefImpl Sec) const { return false; }
 
 Expected<section_iterator>
 ObjectFile::getRelocatedSection(DataRefImpl Sec) const {
   return section_iterator(SectionRef(Sec, this));
 }
 
 Triple ObjectFile::makeTriple() const {
   Triple TheTriple;

llvm/tools/llvm-dwarfdump/SectionSizes.cpp

if (Expected<StringRef> NameOrErr = Section.getName())
       SectionName = *NameOrErr;
     else
       WithColor::defaultWarningHandler(
           createFileError(Filename, NameOrErr.takeError()));
 
     LLVM_DEBUG(dbgs() << SectionName.str() << ": " << Section.getSize()
                       << '\n');
 
-    if (!Section.isDebugSection(SectionName))
+    if (!Section.isDebugSection())
       continue;
 
     Sizes.TotalDebugSectionsSize += Section.getSize();
     Sizes.DebugSectionSizes[std::string(SectionName)] += Section.getSize();
   }
 }
 
 bool dwarfdump::collectObjectSectionSizes(ObjectFile &Obj,