[PATCH] D50494: [LLD] Reserve memory in implicitly set LMARegions

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/LinkerScript.cpp:723-724
+static bool areFlagsCompatible(MemoryRegion *M, OutputSection *Sec) {
+  return (M && (!M->Flags || (M->Flags & Sec->Flags)) &&
+          ((M->NegFlags & Sec->Flags) == 0));
+}
----------------
Remove extraneous parentheses. I.e.

  return M && (!M->Flags || (M->Flags & Sec->Flags)) &&
          (M->NegFlags & Sec->Flags) == 0;

But perhaps it is better to deconstruct it a bit by using more lines:

  if (!M)
    return false;
  if (M->Flags && !(M->Flags & Sec->Flags)
    return false;
  return (M->NegFlags & Sec->Flags) == 0;

Now, my question is, can really `M` be a nullptr?


================
Comment at: ELF/LinkerScript.cpp:781
+
+  // If this section has an LMARegion set, overwrite the previous LMARegion
+  if (Sec->LMARegion)
----------------
Add a full stop at end of a sentence.


================
Comment at: ELF/LinkerScript.cpp:784
+    return Sec->LMARegion;
+  // If this section has an LMAExpr set, find the memory region the address
+  // falls into
----------------
Add a blank line before a comment.


================
Comment at: ELF/LinkerScript.cpp:785
+  // If this section has an LMAExpr set, find the memory region the address
+  // falls into
+  if (Sec->LMAExpr) {
----------------
Ditto.


================
Comment at: ELF/LinkerScript.cpp:793
+  }
+  // If a memory region can be found that is compatible with the current
+  // section, and this region contains at least one section, then the LMA is
----------------
Blank line.


================
Comment at: ELF/LinkerScript.cpp:799
+  // This is only possible if the section has a memory region set and it is
+  // identical to the memory region of the previous section
+  if (Sec->MemRegion && Sec->MemRegion == Ctx->MemRegion &&
----------------
Ditto.


================
Comment at: ELF/LinkerScript.cpp:827
     Ctx->LMAOffset = MR->CurPos - Dot;
+  // If the LMAExpr is set but we could not find an LMARegion previously,
+  // this means we do not have any MEMORY commands and need to evaluate
----------------
If you add a comment to `else`, add {} and write the comment inside {}.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D50494