[ELF] - Remove unnessesary checks from GC logic.

[grimar]

Summary

We set Live bit in constructor of InputSectionBase currently.
If it is set, then section should not be collected.

Previously GC code contained few excessive checks for that, thrown around in code.
And when I was need in D28612 to prevent GC for .rel[a] sections,
I had to modify that places. Though it is cleaner to remove them at all, because
we already have a Live bit set, what looks to be enough.

That should help to reduce amoount of changes for D28612.

[grimar]

• Minor improvement.

[davide]

Do we have a test to make sure sections containing debug info (marked as !SHF_ALLOC) are not reclaimed? I'm a little bit worried because this broke already in the past.

ELF/MarkLive.cpp

159–161

Why did you change this comment?
We still shouldn't reclaim !SHF_ALLOC sections as they can contain debug infos

[grimar]

ELF/MarkLive.cpp

159–161

Because method is called isReserved().
Previously it has logic that kept !SHF_ALLOC, though !SHF_ALLOC sections are not actually reserved.
But anyways now it does not have this logic anymore, there is no point to write this in comment here, it is not possible to support comments like this, when real logic is far away.

[davide]

ELF/MarkLive.cpp

159–161

Where we prevent !SHF_ALLOC sections from being reclaimed? The comment should go there, then,

[grimar]

ELF/MarkLive.cpp

159–161

We set Live bit for all !SHF_ALLOC sections in InputSectionBase constructor:

: InputSectionData(SectionKind, Name, Data,
                   !Config->GcSections || !(Flags & SHF_ALLOC)),

[davide]

ELF/MarkLive.cpp

159–161

Please move the comment there then.

[grimar]

In D29170#657588, @davide wrote:

Do we have a test to make sure sections containing debug info (marked as !SHF_ALLOC) are not reclaimed? I'm a little bit worried because this broke already in the past.

Yes, we do.
For example gc-sections-alloc.s has !SHF_ALLOC section that is kept, while allocatable section it refers to is collected.

[grimar]

• Addressed review comment.

[ruiu]

I don't know if this is making things better - it seems neutral to me or an unnecessary churn.

[davide]

In D29170#657896, @ruiu wrote:

I don't know if this is making things better - it seems neutral to me or an unnecessary churn.

In D29170#657896, @ruiu wrote:

I don't know if this is making things better - it seems neutral to me or an unnecessary churn.

Same. I'm actually a bit confused about this refactoring. First we had the logic in a single place and now we moved part of it to InputSection.{cpp,h} (i.e. the assertion became an if).

[grimar]

In D29170#657896, @ruiu wrote:

I don't know if this is making things better - it seems neutral to me or an unnecessary churn.

Interesting. Is not it the same what you mentioned during review of D28612 ?

"So, the right way of doing it is to set Live bit in the ctor if it should never be GC'ed."

Instead of checking the sections flags everywhere patch relies on Live bit now.

Same. I'm actually a bit confused about this refactoring. First we had the logic in a single place and now we moved part of it to InputSection.{cpp,h} (i.e. the assertion became an if).

D28612 adds SHT_RELA and SHT_REL sections that should be involved in GC (because .rela.text depends on .text and we do not want to keep REL[A] sections unconditionally).
REL[A] sections are not SHF_ALLOC, that means I need to set up GC logic for them.

With this patch only I need is to add that to constructor of InputSectionData which sets the Live bit now:

!Config->GcSections || !(Flags & SHF_ALLOC)), //Add here

Patch changes conditions of GC to use Live bit instead of rechecking the sections flags again.

Next part you are talking about is different:

void markLiveAt(uintX_t Offset) {
  if (this->Flags & SHF_ALLOC)
    LiveOffsets.insert(Offset);
}

It is a member of MergeInputSection, and MergeInputSection has multiple same checks in it's methods because its behavior is different for allocatable and nonallocatable
cases. So this part of patch localizes that check and isolates inside MergeInputSection instead of spreading it to GC code.

[ruiu]

Interesting. Is not it the same what you mentioned during review of D28612 ?

"So, the right way of doing it is to set Live bit in the ctor if it should never be GC'ed."

Instead of checking the sections flags everywhere patch relies on Live bit now.

Well, it is not a good idea to follow someone's advice blindly. You wrote this, so what do you think of this patch? I think that is important. I think this is not something I expected, and I believe there's a better way, but that's not this.

Same. I'm actually a bit confused about this refactoring. First we had the logic in a single place and now we moved part of it to InputSection.{cpp,h} (i.e. the assertion became an if).

D28612 adds SHT_RELA and SHT_REL sections that should be involved in GC (because .rela.text depends on .text and we do not want to keep REL[A] sections unconditionally).
REL[A] sections are not SHF_ALLOC, that means I need to set up GC logic for them.

With this patch only I need is to add that to constructor of InputSectionData which sets the Live bit now:

!Config->GcSections || !(Flags & SHF_ALLOC)), //Add here

Patch changes conditions of GC to use Live bit instead of rechecking the sections flags again.

Next part you are talking about is different:

void markLiveAt(uintX_t Offset) {
  if (this->Flags & SHF_ALLOC)
    LiveOffsets.insert(Offset);
}

It is a member of MergeInputSection, and MergeInputSection has multiple same checks in it's methods because its behavior is different for allocatable and nonallocatable
cases. So this part of patch localizes that check and isolates inside MergeInputSection instead of spreading it to GC code.

[grimar]

In D29170#659009, @ruiu wrote:

Interesting. Is not it the same what you mentioned during review of D28612 ?

"So, the right way of doing it is to set Live bit in the ctor if it should never be GC'ed."

Instead of checking the sections flags everywhere patch relies on Live bit now.

Well, it is not a good idea to follow someone's advice blindly. You wrote this, so what do you think of this patch? I think that is important. I think this is not something I expected, and I believe there's a better way, but that's not this.

I did not follow your advice blindly, I tried that and found that it really reduces amount of changes I need to do for gc-support for emit relocs to single place.
Only because of that and after that I wrote that "it looks a good idea" and honestly I think this patch is very good, at least it IMO definetely makes code much better than it was before.
If it is not what you expected, then I looks I understood you wrong, but it does not change the situation and my opinion about that patch. May be your way you have in mind is better,
I do not know yet as I do not know what you expected :)

What is wrong with this patch ?

Rafael suggested to split D28612 to not support --gc-sections from start. I'll start from that then, though for GC case I believe we need some solution still, this or some another patch.

ELF/InputSection.h

 
 public:
   MergeInputSection(ObjectFile<ELFT> *F, const Elf_Shdr *Header,
                     StringRef Name);
   static bool classof(const InputSectionData *S);
   void splitIntoPieces();
 
   // Mark the piece at a given offset live. Used by GC.
+  // We always keep whole non-alloc sections (usually debug ones).
   void markLiveAt(uintX_t Offset) {
-    assert(this->Flags & llvm::ELF::SHF_ALLOC);
+    if (this->Flags & SHF_ALLOC)
       LiveOffsets.insert(Offset);
   }
 
   // Translate an offset in the input section to an offset
   // in the output section.
   uintX_t getOffset(uintX_t Offset) const;
 
   // Splittable sections are handled as a sequence of data
   // rather than a single large blob of data.

ELF/InputSection.cpp

 
 template <class ELFT>
 InputSectionBase<ELFT>::InputSectionBase(elf::ObjectFile<ELFT> *File,
                                          uintX_t Flags, uint32_t Type,
                                          uintX_t Entsize, uint32_t Link,
                                          uint32_t Info, uintX_t Addralign,
                                          ArrayRef<uint8_t> Data, StringRef Name,
                                          Kind SectionKind)
+    // We do not GC non-allocatable sections which typically contain debugging
+    // information, so set Live flag for them.
     : InputSectionData(SectionKind, Name, Data,
                        !Config->GcSections || !(Flags & SHF_ALLOC)),
       File(File), Flags(Flags), Entsize(Entsize), Type(Type), Link(Link),
       Info(Info), Repl(this) {
   NumRelocations = 0;
   AreRelocsRela = false;
 
   // The ELF spec states that a value of 0 means the section has

ELF/MarkLive.cpp

scanEhFrameSection(EhInputSection<ELFT> &EH,
   EH.split();
 
   if (EH.AreRelocsRela)
     scanEhFrameSection(EH, EH.relas(), Enqueue);
   else
     scanEhFrameSection(EH, EH.rels(), Enqueue);
 }
 
-// We do not garbage-collect two types of sections:
-// 1) Sections used by the loader (.init, .fini, .ctors, .dtors or .jcr)
-// 2) Non-allocatable sections which typically contain debugging information
+// We do not garbage-collect sections used by the loader (.init, .fini, .ctors,
+// .dtors or .jcr)
 template <class ELFT> static bool isReserved(InputSectionBase<ELFT> *Sec) {
   switch (Sec->Type) {
   case SHT_FINI_ARRAY:
   case SHT_INIT_ARRAY:
   case SHT_NOTE:
   case SHT_PREINIT_ARRAY:
     return true;
   default:
-    if (!(Sec->Flags & SHF_ALLOC))
-      return true;
-
     // We do not want to reclaim sections if they can be referred
     // by __start_* and __stop_* symbols.
     StringRef S = Sec->Name;
     if (isValidCIdentifier(S))
       return true;
 
     return S.startswith(".ctors") || S.startswith(".dtors") ||
            S.startswith(".init") || S.startswith(".fini") ||
template <class ELFT> void elf::markLive() {
   auto Enqueue = [&](ResolvedReloc<ELFT> R) {
     // Skip over discarded sections. This in theory shouldn't happen, because
     // the ELF spec doesn't allow a relocation to point to a deduplicated
     // COMDAT section directly. Unfortunately this happens in practice (e.g.
     // .eh_frame) so we need to add a check.
     if (!R.Sec || R.Sec == &InputSection<ELFT>::Discarded)
       return;
 
-    // We don't gc non alloc sections.
-    if (!(R.Sec->Flags & SHF_ALLOC))
-      return;
-
     // Usually, a whole section is marked as live or dead, but in mergeable
     // (splittable) sections, each piece of data has independent liveness bit.
     // So we explicitly tell it which offset is in use.
     if (auto *MS = dyn_cast<MergeInputSection<ELFT>>(R.Sec))
       MS->markLiveAt(R.Offset);
 
     if (R.Sec->Live)
       return;
template <class ELFT> void elf::markLive() {
   // Preserve special sections and those which are specified in linker
   // script KEEP command.
   for (InputSectionBase<ELFT> *Sec : Symtab<ELFT>::X->Sections) {
     // .eh_frame is always marked as live now, but also it can reference to
     // sections that contain personality. We preserve all non-text sections
     // referred by .eh_frame here.
     if (auto *EH = dyn_cast_or_null<EhInputSection<ELFT>>(Sec))
       scanEhFrameSection<ELFT>(*EH, Enqueue);
-    if (isReserved(Sec) || Script<ELFT>::X->shouldKeep(Sec))
+    if (Sec->Live || isReserved(Sec) || Script<ELFT>::X->shouldKeep(Sec))
       Enqueue({Sec, 0});
   }
 
   // Mark all reachable sections.
   while (!Q.empty())
     forEachSuccessor<ELFT>(*Q.pop_back_val(), Enqueue);
 }
 
 template void elf::markLive<ELF32LE>();
 template void elf::markLive<ELF32BE>();
 template void elf::markLive<ELF64LE>();
 template void elf::markLive<ELF64BE>();