[PATCH] D29170: [ELF] - Remove unnessesary checks from GC logic.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

> Interesting. Is not it the same what you mentioned during review of https://reviews.llvm.org/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).
> 
> https://reviews.llvm.org/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.




https://reviews.llvm.org/D29170