[PATCH] D41046: [ELF] Make overlapping output sections an error

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Writer.cpp:1889-1890
+                                   Predicate ShouldSkip) {
+  auto FirstInteresting = partition(AllSections, ShouldSkip);
+  MutableArrayRef<OutputSection *> InterestingSections(AllSections);
+  InterestingSections = InterestingSections.slice(
----------------
ruiu wrote:
> Please do not use `auto` in lld unless the actual type is obvious in a very narrow context (e.g. RHS).
These variable names are a bit too lengthy and don't match other code in lld. Also this patch uses higher order functions than other code, so it feels different. I'd make this consistent with other code.

E.g. I'd write this part as

  std::vector<OutputSection *> Sections;
  for (OutputSection *Sec : AllSections)
    if (!ShouldSkip(Sec))
      Sections.push_back(Sec);


================
Comment at: ELF/Writer.cpp:1893
+      std::distance(AllSections.begin(), FirstInteresting));
+  // Instead of comparing every OutputSection with every other output section
+  // we sort the sections by address (file offset or load/virtual address). This
----------------
Insert a blank line before a multi-line comment.


================
Comment at: ELF/Writer.cpp:1904
+  for (size_t i = 0; i < InterestingSections.size(); ++i) {
+    auto *Sec = InterestingSections[i];
+    uint64_t Start = GetStart(Sec);
----------------
Please don't use `auto`


================
Comment at: ELF/Writer.cpp:1908
+      uint64_t OtherStart = GetStart(Other);
+      if (Start + Sec->Size > OtherStart && Start < OtherStart + Other->Size)
+        errorOrWarn("section " + Sec->Name + " " + Kind +
----------------
Isn't this condition enough?

  if (GetStart(Other) < GetStart(Sec) + Sec->Size)


================
Comment at: ELF/Writer.cpp:1929
+  std::vector<OutputSection *> Sections;
+  Sections.reserve(OutputSections.size());
+  // Skip all zero-size sections when checking for overlap to speed up
----------------
This is not a performance critical path, so we shouldn't optimize it by using `reserve`.


================
Comment at: ELF/Writer.cpp:1932-1933
+  // the checks a bit since they can never cause overlap.
+  copy_if(OutputSections, std::back_inserter(Sections),
+          [](const OutputSection *Sec) { return Sec->Size != 0; });
+
----------------
I'd write this with a plain for-loop for consistency with other code.

  for (OutputSection *Sec : OutputSections)
    if (Sec->Size > 0)
      Sections.push_back(Sec);


================
Comment at: ELF/Writer.cpp:1964
+                         SkipNonAllocSections);
+  // Finally, check that the load addresses don't overlap. This will usually be
+  // the same as the virtual addresses but can be different when using a linker
----------------
Insert a blank line.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D41046