[PATCH] D38724: [ELF] - Do not collect SHT_REL[A] sections unconditionally when --gc-sections and --emit-relocs used together.

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 12 03:42:42 PDT 2017


grimar added inline comments.


================
Comment at: ELF/MarkLive.cpp:165-167
+// Some sections are reserved and should never be collected,
+// and should become a root node for futher garbage collection.
+// Function returns true if section given is one of reserved.
----------------
ruiu wrote:
> "reserved" doesn't make sense unless you describe what is considered "reserved". If you name a function "reserved" and write a comment saying that this function returns true if a given object is reserved, it doesn't explain the function. You need to explain to those who don't have a priori knowledge about the function.
> 
> If you revisit this code a few year later, you'd wonder why they are considered reserved and why they had to be treated in a different way than the other sections. Generally, your comments should answer these questions.
> 
> I'd explain: Some sections are used directly by the loader, so they should never be garbage-collected. This function returns true if a given section is such section.
Ok, thanks.


================
Comment at: ELF/MarkLive.cpp:219-221
+    // Not all sections are subject of garbage collection.
+    if (!canBeCollected(Sec))
       return;
----------------
ruiu wrote:
> Why do you have to check this? All non-collectible sections have already been marked alive, so you don't need to check for the same condition again in this function.
It is needed for REL[A] sections which are collectible. Initial condition exits
early for non-alloc sections, but REL[A] sections are not-alloc, but collectible,
so I want to let them go through.
Updated comment.


https://reviews.llvm.org/D38724





More information about the llvm-commits mailing list