[ELF] Merge LinkerScript::addSymbol and declareSymbol

[MaskRay]

Summary

To factor out the common part.

[ruiu]

I'm not sure if this is an improvement. Sometimes, I actually like separating functions even if the separated functions have some amount of common code. It's like writing documents in a natural language -- sometimes you want to repeat the same thing in two different paragraphs to make it easier to understand. It feels to me that this is that kind of situation.

[MaskRay]

In D60030#1449524, @ruiu wrote:

Sometimes, I actually like separating functions even if the separated functions have some amount of common code.

I agree with this point: making duplication if it helps readability. And I noticed that rLLD357373 was the recent interpretation of this rule. I did think over before creating this, and i think it slightly improves readability.

IMHO, after merging, the common/different parts are emphasized and it stops a reader from asking "what the two functions have in common and what are their differences?" (I asked myself this question while reading it because the shared parts are not too trivial: if (!shouldDefineSym(Cmd)), uint8_t Visibility = Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT;, replaceSymbol<Defined> and the different choices of placeholder st_value).

ELF/LinkerScript.h

struct AddressState {
     OutputSection *OutSec = nullptr;
     MemoryRegion *MemRegion = nullptr;
     MemoryRegion *LMARegion = nullptr;
     uint64_t LMAOffset = 0;
   };
 
   llvm::DenseMap<StringRef, OutputSection *> NameToOutputSection;
 
-  void addSymbol(SymbolAssignment *Cmd);
   void assignSymbol(SymbolAssignment *Cmd, bool InSec);
   void setDot(Expr E, const Twine &Loc, bool InSec);
   void expandOutputSection(uint64_t Size);
   void expandMemoryRegions(uint64_t Size);
 
   std::vector<InputSection *>
   computeInputSections(const InputSectionDescription *);
 

ELF/LinkerScript.cpp

static bool shouldDefineSym(SymbolAssignment *Cmd) {
   // If a symbol was in PROVIDE(), we need to define it only
   // when it is a referenced undefined symbol.
   Symbol *B = Symtab->find(Cmd->Name);
   if (B && !B->isDefined())
     return true;
   return false;
 }
 
-// This function is called from processSectionCommands,
-// while we are fixing the output section layout.
-void LinkerScript::addSymbol(SymbolAssignment *Cmd) {
+static void addSymbol(SymbolAssignment *Cmd, bool Declare) {
   if (!shouldDefineSym(Cmd))
     return;
 
-  // Define a symbol.
   Symbol *Sym;
   uint8_t Visibility = Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT;
   std::tie(Sym, std::ignore) = Symtab->insert(Cmd->Name, Visibility,
                                               /*CanOmitFromDynSym*/ false,
                                               /*File*/ nullptr);
+  SectionBase *Sec = nullptr;
+  uint64_t SymValue = 0;
+  if (Declare) {
+    // Create a placeholder symbol if needed. Set Provide to false so that when
+    // the function is called again to define the symbol, it will not be skipped
+    // by shouldDefineSym().
+    Cmd->Provide = false;
+  } else {
+    // This is called from processSectionCommands, while we are fixing the
+    // output section layout.
     ExprValue Value = Cmd->Expression();
-  SectionBase *Sec = Value.isAbsolute() ? nullptr : Value.Sec;
+    Sec = Value.isAbsolute() ? nullptr : Value.Sec;
 
     // When this function is called, section addresses have not been
     // fixed yet. So, we may or may not know the value of the RHS
     // expression.
     //
     // For example, if an expression is `x = 42`, we know x is always 42.
     // However, if an expression is `x = .`, there's no way to know its
     // value at the moment.
     //
     // We want to set symbol values early if we can. This allows us to
     // use symbols as variables in linker scripts. Doing so allows us to
     // write expressions like this: `alignment = 16; . = ALIGN(., alignment)`.
-  uint64_t SymValue = Value.Sec ? 0 : Value.getValue();
-
-  replaceSymbol<Defined>(Sym, nullptr, Cmd->Name, STB_GLOBAL, Visibility,
-                         STT_NOTYPE, SymValue, 0, Sec);
-  Cmd->Sym = cast<Defined>(Sym);
+    if (!Value.Sec)
+      SymValue = Value.getValue();
   }
 
-// This function is called from LinkerScript::declareSymbols.
-// It creates a placeholder symbol if needed.
-static void declareSymbol(SymbolAssignment *Cmd) {
-  if (!shouldDefineSym(Cmd))
-    return;
-
-  // We can't calculate final value right now.
-  Symbol *Sym;
-  uint8_t Visibility = Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT;
-  std::tie(Sym, std::ignore) = Symtab->insert(Cmd->Name, Visibility,
-                                              /*CanOmitFromDynSym*/ false,
-                                              /*File*/ nullptr);
   replaceSymbol<Defined>(Sym, nullptr, Cmd->Name, STB_GLOBAL, Visibility,
-                         STT_NOTYPE, 0, 0, nullptr);
-  Cmd->Sym = cast<Defined>(Sym);
-  Cmd->Provide = false;
+                         STT_NOTYPE, SymValue, 0, Sec);
   Sym->ScriptDefined = true;
+  Cmd->Sym = cast<Defined>(Sym);
 }
 
 // This method is used to handle INSERT AFTER statement. Here we rebuild
 // the list of script commands to mix sections inserted into.
 void LinkerScript::processInsertCommands() {
   std::vector<BaseCommand *> V;
   auto Insert = [&](std::vector<BaseCommand *> &From) {
     V.insert(V.end(), From.begin(), From.end());

 
 // Symbols defined in script should not be inlined by LTO. At the same time
 // we don't know their final values until late stages of link. Here we scan
 // over symbol assignment commands and create placeholder symbols if needed.
 void LinkerScript::declareSymbols() {
   assert(!Ctx);
   for (BaseCommand *Base : SectionCommands) {
     if (auto *Cmd = dyn_cast<SymbolAssignment>(Base)) {
-      declareSymbol(Cmd);
+      addSymbol(Cmd, true);
       continue;
     }
 
     // If the output section directive has constraints,
     // we can't say for sure if it is going to be included or not.
     // Skip such sections for now. Improve the checks if we ever
     // need symbols from that sections to be declared early.
     auto *Sec = cast<OutputSection>(Base);
     if (Sec->Constraint != ConstraintKind::NoConstraint)
       continue;
     for (BaseCommand *Base2 : Sec->SectionCommands)
       if (auto *Cmd = dyn_cast<SymbolAssignment>(Base2))
-        declareSymbol(Cmd);
+        addSymbol(Cmd, true);
   }
 }
 
 // This function is called from assignAddresses, while we are
 // fixing the output section addresses. This function is supposed
 // to set the final value for a given symbol assignment.
 void LinkerScript::assignSymbol(SymbolAssignment *Cmd, bool InSec) {
   if (Cmd->Name == ".") {
void LinkerScript::processSectionCommands() {
   Ctx = Deleter.get();
   Ctx->OutSec = Aether;
 
   size_t I = 0;
   // Add input sections to output sections.
   for (BaseCommand *Base : SectionCommands) {
     // Handle symbol assignments outside of any output section.
     if (auto *Cmd = dyn_cast<SymbolAssignment>(Base)) {
-      addSymbol(Cmd);
+      addSymbol(Cmd, false);
       continue;
     }
 
     if (auto *Sec = dyn_cast<OutputSection>(Base)) {
       std::vector<InputSection *> V = createInputSectionList(*Sec);
 
       // The output section name `/DISCARD/' is special.
       // Any input section assigned to it is discarded.
if (auto *Sec = dyn_cast<OutputSection>(Base)) {
         Sec->SectionCommands.clear();
         continue;
       }
 
       // A directive may contain symbol definitions like this:
       // ".foo : { ...; bar = .; }". Handle them.
       for (BaseCommand *Base : Sec->SectionCommands)
         if (auto *OutCmd = dyn_cast<SymbolAssignment>(Base))
-          addSymbol(OutCmd);
+          addSymbol(OutCmd, false);
 
       // Handle subalign (e.g. ".foo : SUBALIGN(32) { ... }"). If subalign
       // is given, input sections are aligned to that value, whether the
       // given value is larger or smaller than the original section alignment.
       if (Sec->SubalignExpr) {
         uint32_t Subalign = Sec->SubalignExpr().getValue();
         for (InputSectionBase *S : V)
           S->Alignment = Subalign;