[ELF] - Remove one of OutputSectionFactory::addInputSec().

[grimar]

Summary

Patch removes one of OutputSectionFactory::addInputSec methods.
That allows to simplify reporting of discarded sections and
should help to D37561.

[ruiu]

The new code is hard to understand due to a pointer to a pointer to something. I think you can do this without two levels of pointer indirection.

[grimar]

After playing with code I "re-invented" nicest way: just leave everything as is and make one of addInputSec
to be static helper. That way public addInputSec becomes the only way to add input section and it is able to report
discarded sections early enough, what solves initial problem.

[ruiu]

ELF/OutputSections.cpp

208–209 ↗

(On Diff #115017)

Instead of mutating a given pointer through a reference, this function should take OutputSection * and returns OutputSection *.

[grimar]

• Addressed review comments.

[grimar]

• Addressed comments.

ELF/OutputSections.cpp

208–209 ↗

(On Diff #115017)

Ok, but personally I find such constructions a bit odd.

[ruiu]

LGTM

ELF/OutputSections.cpp

208–209 ↗

(On Diff #115017)

Really? Then that's pretty different from my taste. Mutating an argument pointer through a reference is pretty tricky, and on the other hand, returning a pointer is very normal, straightforward and functional-ish.

[grimar]

ELF/OutputSections.cpp

208–209 ↗

(On Diff #115017)

Returning pointer itself - yes, but not in constructions when it is on both sides like
Sec = addSection(IS, OutsecName, Sec);
Mutating pointer is also not great though.

[ruiu]

ELF/OutputSections.cpp

208–209 ↗

(On Diff #115017)

IMO there's a big difference. The expression Sec = addSection(IS, OutsecName, Sec) is easier to understand because within the narrow local context, we know that Sec is a reference to a pointer, so what that expression does is obvious: Sec is updated. On the other hand, if you implicitly mutate Sec through a reference, that is not obvious at all.

You cannot assume that people who are reading this piece of code already know what exactly addSection does, so you want to make it easy to guess. One way of doing it is to make something explicitly. Generally, you want to avoid mutating variables implicitly through references unless there's other reason to do that.

In addition to that, the previous code

OutputSection *Out = nullptr;
addSection(IS, OutsecName, Out);

was ugly. I was tempted to remove Out variable, but I couldn't because it has to be an lvalue.

Overall, I strongly believe that changing this function to return a pointer instead of mutating an argument via a reference is pretty much reasonable change which improves code quality.

[grimar]

ELF/OutputSections.cpp

208–209 ↗

(On Diff #115017)

IMO there's a big difference. The expression Sec = addSection(IS, OutsecName, Sec) is easier to understand because within the narrow local context, we know that Sec is a reference to a pointer, so what that expression does is obvious: Sec is updated.

Issue is here for me. Such expression does not give guarantee that Sec is updated (I mean not variable itself, but instance). It can return new updated Sec but also may return the same one from right part unchanged. What is worse it may update Sec from argument and return different Sec at the same time.
In compare with simple assignment like Sec = addSection(IS, OutsecName) there is huge difference for me.
(I am not trying to say it is worse than mutating argument, just trying to explain why it confuses me when I see such things and why I don't prefer them).

lld/trunk/ELF/OutputSections.h

 // input section. Output section type is determined by various
 // factors, including input section's sh_flags, sh_type and
 // linker scripts.
 class OutputSectionFactory {
 public:
   OutputSectionFactory();
   ~OutputSectionFactory();
 
-  void addInputSec(InputSectionBase *IS, StringRef OutsecName);
   void addInputSec(InputSectionBase *IS, StringRef OutsecName,
-                   OutputSection *&Sec);
+                   OutputSection *OS = nullptr);
 
 private:
   llvm::SmallDenseMap<SectionKey, OutputSection *> Map;
 };
 
 uint64_t getHeaderSize();
 void reportDiscarded(InputSectionBase *IS);
 void sortByOrder(llvm::MutableArrayRef<InputSection *> In,
                  std::function<int(InputSectionBase *S)> Order);
 
 extern std::vector<OutputSection *> OutputSections;
 } // namespace elf
 } // namespace lld
 
 #endif

lld/trunk/ELF/OutputSections.cpp

 
 void elf::reportDiscarded(InputSectionBase *IS) {
   if (!Config->PrintGcSections)
     return;
   message("removing unused section from '" + IS->Name + "' in file '" +
           IS->File->getName() + "'");
 }
 
-void OutputSectionFactory::addInputSec(InputSectionBase *IS,
-                                       StringRef OutsecName) {
-  // Sections with the SHT_GROUP attribute reach here only when the - r option
-  // is given. Such sections define "section groups", and InputFiles.cpp has
-  // dedup'ed section groups by their signatures. For the -r, we want to pass
-  // through all SHT_GROUP sections without merging them because merging them
-  // creates broken section contents.
-  if (IS->Type == SHT_GROUP) {
-    OutputSection *Out = nullptr;
-    addInputSec(IS, OutsecName, Out);
-    return;
-  }
-
-  // Imagine .zed : { *(.foo) *(.bar) } script. Both foo and bar may have
-  // relocation sections .rela.foo and .rela.bar for example. Most tools do
-  // not allow multiple REL[A] sections for output section. Hence we
-  // should combine these relocation sections into single output.
-  // We skip synthetic sections because it can be .rela.dyn/.rela.plt or any
-  // other REL[A] sections created by linker itself.
-  if (!isa<SyntheticSection>(IS) &&
-      (IS->Type == SHT_REL || IS->Type == SHT_RELA)) {
-    auto *Sec = cast<InputSection>(IS);
-    OutputSection *Out = Sec->getRelocatedSection()->getOutputSection();
-    addInputSec(IS, OutsecName, Out->RelocationSection);
-    return;
-  }
-
-  SectionKey Key = createKey(IS, OutsecName);
-  OutputSection *&Sec = Map[Key];
-  addInputSec(IS, OutsecName, Sec);
-}
-
-void OutputSectionFactory::addInputSec(InputSectionBase *IS,
-                                       StringRef OutsecName,
-                                       OutputSection *&Sec) {
-  if (!IS->Live) {
-    reportDiscarded(IS);
-    return;
-  }
-
+static OutputSection *addSection(InputSectionBase *IS, StringRef OutsecName,
+                                 OutputSection *Sec) {
   if (Sec && Sec->Live) {
     if (getIncompatibleFlags(Sec->Flags) != getIncompatibleFlags(IS->Flags))
       error("incompatible section flags for " + Sec->Name + "\n>>> " +
             toString(IS) + ": 0x" + utohexstr(IS->Flags) +
             "\n>>> output section " + Sec->Name + ": 0x" +
             utohexstr(Sec->Flags));
     if (Sec->Type != IS->Type) {
       if (canMergeToProgbits(Sec->Type) && canMergeToProgbits(IS->Type))
if (!Sec) {
       Sec = Script->createOutputSection(OutsecName, "<internal>");
       Script->Opt.Commands.push_back(Sec);
     }
     Sec->Type = IS->Type;
     Sec->Flags = IS->Flags;
   }
 
   Sec->addSection(cast<InputSection>(IS));
+  return Sec;
+}
+
+void OutputSectionFactory::addInputSec(InputSectionBase *IS,
+                                       StringRef OutsecName,
+                                       OutputSection *OS) {
+  if (!IS->Live) {
+    reportDiscarded(IS);
+    return;
+  }
+
+  // If we have destination output section - use it directly.
+  if (OS) {
+    addSection(IS, OutsecName, OS);
+    return;
+  }
+
+  // Sections with the SHT_GROUP attribute reach here only when the - r option
+  // is given. Such sections define "section groups", and InputFiles.cpp has
+  // dedup'ed section groups by their signatures. For the -r, we want to pass
+  // through all SHT_GROUP sections without merging them because merging them
+  // creates broken section contents.
+  if (IS->Type == SHT_GROUP) {
+    addSection(IS, OutsecName, nullptr);
+    return;
+  }
+
+  // Imagine .zed : { *(.foo) *(.bar) } script. Both foo and bar may have
+  // relocation sections .rela.foo and .rela.bar for example. Most tools do
+  // not allow multiple REL[A] sections for output section. Hence we
+  // should combine these relocation sections into single output.
+  // We skip synthetic sections because it can be .rela.dyn/.rela.plt or any
+  // other REL[A] sections created by linker itself.
+  if (!isa<SyntheticSection>(IS) &&
+      (IS->Type == SHT_REL || IS->Type == SHT_RELA)) {
+    auto *Sec = cast<InputSection>(IS);
+    OutputSection *Out = Sec->getRelocatedSection()->getOutputSection();
+    Out->RelocationSection = addSection(IS, OutsecName, Out->RelocationSection);
+    return;
+  }
+
+  SectionKey Key = createKey(IS, OutsecName);
+  OutputSection *&Sec = Map[Key];
+  Sec = addSection(IS, OutsecName, Sec);
 }
 
 OutputSectionFactory::~OutputSectionFactory() {}
 
 SectionKey DenseMapInfo<SectionKey>::getEmptyKey() {
   return SectionKey{DenseMapInfo<StringRef>::getEmptyKey(), 0, 0};
 }