[LLD][ELF] Add isDebug flag to SectionBase.

[avl]

Summary

Add isDebug flag to SectionBase to avoid multiplue string
comparisions by isDebugSection() function.

[MaskRay]

Most links don't use --strip-debug or --strip-all, so isDebugSection in LinkerDriver::link is rarely called.

Most links don't specify -r or --emit-relocs, so InputSection::copyRelocations is not called. In addition, isDebugInfo inside the function is called by edge cases which we have to suppress warnings for practical reasons, the cases are even fewer.

I am hesitant to accept this patch, unless you can make further justification.

[avl]

Most links don't use --strip-debug or --strip-all, so isDebugSection in LinkerDriver::link is rarely called.

Most links don't specify -r or --emit-relocs, so InputSection::copyRelocations is not called. In addition, isDebugInfo inside the function is called by edge cases which we have to suppress warnings for practical reasons, the cases are even fewer.

I am hesitant to accept this patch, unless you can make further justification.

I intend to use it for D74169(where checking for debug section is used more frequently). If not for D74169 then it would be suitable for any future checking of debug section(support of .debug_names?). This change costs nothing. It does not increase the size of SectionBase. Thus it seems to me it would be useful to do this patch even if it does not show noticeable performance impact right now.

[grimar]

In D78954#2006503, @avl wrote:

Most links don't use --strip-debug or --strip-all, so isDebugSection in LinkerDriver::link is rarely called.

Most links don't specify -r or --emit-relocs, so InputSection::copyRelocations is not called. In addition, isDebugInfo inside the function is called by edge cases which we have to suppress warnings for practical reasons, the cases are even fewer.

I am hesitant to accept this patch, unless you can make further justification.

This change costs nothing. It does not increase the size of SectionBase. Thus it seems to me it would be useful to do this patch even if it does not show noticeable performance impact right now.

I added a comment about this previously, I think it was not answered?
https://reviews.llvm.org/D74169#inline-709439

My assumption was that it is a premature unproved optimization (while you did not show the numbers, e.g. link time with/without it).
(Honestly I would be surprised to see a speed-up after doing this. At least while you have cases with 889% link time, my feeling is that something else is slow, but not this).

This change costs nothing.

This part is a bit wrong atm. It costs us:

1. A bit in SectionBase, which might be needed in the future.
2. It causes code readers to think that this is a useful optimization, though it is unproved yet. (General rule of LLD is do no work that can be avoided. It helps it to be that fast).
3. More code for perhaps no reason.

[avl]

I added a comment about this previously, I think it was not answered?
https://reviews.llvm.org/D74169#inline-709439

My assumption was that it is a premature unproved optimization (while you did not show the numbers, e.g. link time with/without it).
(Honestly I would be surprised to see a speed-up after doing this. At least while you have cases with 889% link time, my feeling is that something else is slow, but not this).

you are right. It does not have noticeable effect. would close that review.

lld/ELF/Driver.cpp

if (s->type == SHT_LLVM_SYMPART) {
       return true;
     }
 
     // We do not want to emit debug sections if --strip-all
     // or -strip-debug are given.
     if (config->strip == StripPolicy::None)
       return false;
 
-    if (isDebugSection(*s))
+    if (s->isDebug)
       return true;
     if (auto *isec = dyn_cast<InputSection>(s))
       if (InputSectionBase *rel = isec->getRelocatedSection())
-        if (isDebugSection(*rel))
+        if (rel->isDebug)
           return true;
 
     return false;
   });
 
   // Now that the number of partitions is fixed, save a pointer to the main
   // partition.
   mainPart = &partitions[0];

lld/ELF/InputSection.h

 struct Partition;
 class SyntheticSection;
 class MergeSyntheticSection;
 template <class ELFT> class ObjFile;
 class OutputSection;
 
 extern std::vector<Partition> partitions;
 
+inline bool isDebugSection(const StringRef &name) {
+  return name.startswith(".debug") || name.startswith(".zdebug");
+}
+
 // This is the base class of all sections that lld handles. Some are sections in
 // input files, some are sections in the produced output file and some exist
 // just as a convenience for implementing special ways of combining some
 // sections.
 class SectionBase {
 public:
   enum Kind { Regular, EHFrame, Merge, Synthetic, Output };
 
public:
   // The next two bit fields are only used by InputSectionBase, but we
   // put them here so the struct packs better.
 
   unsigned bss : 1;
 
   // Set for sections that should not be folded by ICF.
   unsigned keepUnique : 1;
 
+  // True if that section is a debug section.
+  unsigned isDebug : 1;
+
   // The 1-indexed partition that this section is assigned to by the garbage
   // collector, or 0 if this section is dead. Normally there is only one
   // partition, so this will either be 0 or 1.
   uint8_t partition;
   elf::Partition &getPartition() const;
 
   // These corresponds to the fields in Elf_Shdr.
   uint32_t alignment;
public:
   void markLive() { partition = 1; }
   void markDead() { partition = 0; }
 
 protected:
   SectionBase(Kind sectionKind, StringRef name, uint64_t flags,
               uint64_t entsize, uint64_t alignment, uint32_t type,
               uint32_t info, uint32_t link)
       : name(name), repl(this), sectionKind(sectionKind), bss(false),
-        keepUnique(false), partition(0), alignment(alignment), flags(flags),
-        entsize(entsize), type(type), link(link), info(info) {}
+        keepUnique(false), isDebug(isDebugSection(name)), partition(0),
+        alignment(alignment), flags(flags), entsize(entsize), type(type),
+        link(link), info(info) {}
 };
 
 // This corresponds to a section of an input file.
 class InputSectionBase : public SectionBase {
 public:
   template <class ELFT>
   InputSectionBase(ObjFile<ELFT> &file, const typename ELFT::Shdr &header,
                    StringRef name, Kind sectionKind);

 
 private:
   template <class ELFT, class RelTy>
   void copyRelocations(uint8_t *buf, llvm::ArrayRef<RelTy> rels);
 
   template <class ELFT> void copyShtGroup(uint8_t *buf);
 };
 
-inline bool isDebugSection(const InputSectionBase &sec) {
-  return sec.name.startswith(".debug") || sec.name.startswith(".zdebug");
-}
-
 // The list of all input sections.
 extern std::vector<InputSectionBase *> inputSections;
 
 } // namespace elf
 
 std::string toString(const elf::InputSectionBase *);
 } // namespace lld
 
 #endif

lld/ELF/InputSection.cpp

if (sym.type == STT_SECTION) {
       // avoid having to parse and recreate .eh_frame, we just replace any
       // relocation in it pointing to discarded sections with R_*_NONE, which
       // hopefully creates a frame that is ignored at runtime. Also, don't warn
       // on .gcc_except_table and debug sections.
       //
       // See the comment in maybeReportUndefined for PPC32 .got2 and PPC64 .toc
       auto *d = dyn_cast<Defined>(&sym);
       if (!d) {
-        if (!isDebugSection(*sec) && sec->name != ".eh_frame" &&
+        if (!sec->isDebug && sec->name != ".eh_frame" &&
             sec->name != ".gcc_except_table" && sec->name != ".got2" &&
             sec->name != ".toc") {
           uint32_t secIdx = cast<Undefined>(sym).discardedSecIdx;
           Elf_Shdr_Impl<ELFT> sec =
               CHECK(file->getObj().sections(), file)[secIdx];
           warn("relocation refers to a discarded section: " +
                CHECK(file->getObj().getSectionName(&sec), file) +
                "\n>>> referenced by " + getObjMsg(p->r_offset));