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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu 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.
----------------
"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.


================
Comment at: ELF/MarkLive.cpp:197
+// collection because, if we remove a text section, we also
+// remove its relocation section.
+static bool canBeCollected(InputSectionBase *Sec) {
----------------
we also want to remove ...


================
Comment at: ELF/MarkLive.cpp:219-221
+    // Not all sections are subject of garbage collection.
+    if (!canBeCollected(Sec))
       return;
----------------
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.


https://reviews.llvm.org/D38724