[PATCH] D67761: [ELF] Error if the linked-to section of a SHF_LINK_ORDER section is discarded

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Ok. A suggestion about how to simplify it a little bit is below.



================
Comment at: ELF/Writer.cpp:1527
+              toString(link));
+    }
+    if (errorCount())
----------------
MaskRay wrote:
> grimar wrote:
> > Should we report this much earlier, on GC stage, in MarkLive.cpp for example?
> > 
> > I do not have a chance to check it today unfortunately, but looks it could be done somewhere here:
> > 
> > ```
> >   // Report garbage-collected sections.
> >   if (config->printGcSections)
> >     for (InputSectionBase *sec : inputSections)
> >       if (!sec->isLive())
> >         message("removing unused section " + toString(sec));
> > ```
> > 
> > ?
> markLive() is called before processSectionCommands(). Before processSectionCommands(), output sections are not allocated, and the `link->getParent()` check does not work.
> 
> We can change `link->getParent()` to `link->isLive()` if we really want to perform the check in markLive(). However, we have another rule: /DISCARD/ discards an input section as well as its dependentSections. If sh_link(a)=b && a has SHF_LINK_ORDER, we currently place a in b's dependentSections list. If we discard b, a will be discarded as well (see discard-section-metadata.s). Having the check in markLive() would issue a premature error.
> 
> For the reasons above, we cannot perform the check in markLive().
> 
> Actually, I'm thinking if we can/should drop the rule:
> 
> > If sh_link(a)=b && a has SHF_LINK_ORDER, we currently place a in b's dependentSections list
> 
> I need to check for tests and previous commits on this.
What about the following reordering of the code here then?

```
    // The ARM.exidx section use SHF_LINK_ORDER, but we have consolidated
    // this processing inside the ARMExidxsyntheticsection::finalizeContents().
    if (!config->relocatable && config->emachine == EM_ARM &&
        sec->type == SHT_ARM_EXIDX)
      continue;

    // Link order may be distributed across several InputSectionDescriptions
    // but sort must consider them all at once.
    std::vector<InputSection **> scriptSections;
    std::vector<InputSection *> sections;
    for (BaseCommand *base : sec->sectionCommands) {
      if (auto *isd = dyn_cast<InputSectionDescription>(base)) {
        for (InputSection *&isec : isd->sections) {
          scriptSections.push_back(&isec);
          sections.push_back(isec);

          InputSection *link = isec->getLinkOrderDep();
          if (!link->getParent())
            error(toString(isec) + ": sh_link points to discarded section " +
                  toString(link));
        }
      }
    }

    if (errorCount())
      continue;
```


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67761/new/

https://reviews.llvm.org/D67761