[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

[ben via Phabricator via cfe-commits]

bd1976llvm added a comment.

In D68101#1766151 <https://reviews.llvm.org/D68101#1766151>, @nickdesaulniers wrote:

> In D68101#1765665 <https://reviews.llvm.org/D68101#1765665>, @rjmccall wrote:
>
> > Given all that, this patch seems far too aggressive.  While mergeable sections can be useful for optimizing arbitrary code that might not use a section, they are also extremely useful for optimizing the sorts of global tables that programmers frequently use explicit sections for.  It seems to me that the right fix is to find the place that ensures that we don't put mergeable and non-mergeable objects in the same section unit (or at least conservatively makes the section unit non-mergeable) and fix it to consider entry size as well.  That should be straightforward unless that place doesn't exist, in which case we have very serious problems.
>
>
> Disagree (but I spent all day thinking about this).  Going back to https://bugs.llvm.org/show_bug.cgi?id=31828, I think I solved the problem there incorrectly; we should be not marking the explicit section merge-able.


Thanks for the comments!

I am convinced that the current patch is too pessimistic, size optimizations can be functional requirements in memory constrained areas.

There are two frontend attributes involved:

pragma clang section ... - https://clang.llvm.org/docs/LanguageExtensions.html#specifying-section-names-for-global-objects-pragma-clang-section
_attribute_ ((section ("section-name"))) - https://clang.llvm.org/docs/AttributeReference.html#section-declspec-allocate

At the IR level you can query for explicit sections assignment with code like:

  static bool inExplicitSection(const GlobalObject *GO, SectionKind Kind) {
    if (GO->hasSection())
      return true;
  
    if (auto *GVar = dyn_cast<GlobalVariable>(GO)) {
      auto Attrs = GVar->getAttributes();
      if ((Attrs.hasAttribute("bss-section") && Kind.isBSS()) ||
          (Attrs.hasAttribute("data-section") && Kind.isData()) ||
          (Attrs.hasAttribute("rodata-section") && Kind.isReadOnly())) {
        return true;
      }
    }
  
    if (auto *F = dyn_cast<Function>(GO)) {
      if (F->hasFnAttribute("implicit-section-name"))
        return true;
    }
  
    return false;
  }

The section of a global is used for _attribute_ ((section ("section-name"))) but it is also set in LLVM e.g. by optimization passes.
Attributes "bss-section", "data-section", "rodata-section" and "implicit-section-name" are only used for "clang section" pragmas.

As I commented upthread I would like input from the authors of the "clang section" pragma to understand whether the semantics of that pragma would allow for generating multiple output sections (with the same name). Assuming that this pragma implies a single output section I have an new proposal for a less pessimistic fix:

1. Sections generated for "clang section" pragmas are never made mergeable.
2. It is an error if incompatible globals are assigned to a section with the same name.

This works as the user can easily fix any "_attribute_ ((section ("section-name")))" in their source code and internal uses in LLVM code can also make sure that incompatible globals are assigned to differently named sections. Thoughts? I'll update the patch shortly unless there are objections.


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

https://reviews.llvm.org/D68101