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

Mon Jan 13 17:36:50 PST 2020

bd1976llvm added a comment.

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

> In D68101#1806213 <https://reviews.llvm.org/D68101#1806213>, @rjmccall wrote:
>
> > In D68101#1806135 <https://reviews.llvm.org/D68101#1806135>, @nickdesaulniers wrote:
> >
> > > In D68101#1802280 <https://reviews.llvm.org/D68101#1802280>, @rjmccall wrote:
> > >
> > > > I laid out a series of three options before:
> > > >
> > > > - Emit different object-file sections for different mergeability settings.
> > > > - Only mark an object-file section as mergeable if all the symbols in it would have the same mergeability settings.
> > > > - Stop implicitly using mergeability for "ordinary" sections (i.e. sections other than the string section).
> > >
> > >
> > > Of the above, I really think #2 is the only complete solution.
> >
> >
> > Can you explain why you think #1 is not "complete"?  All three seem to establish correctness; I can see how giving up on the optimization (#3) is not a great solution, but #1 doesn't have that problem and actually preserves it in more places.  To be clear, this is just taking advantage of the ability to have multiple sections with the same name in an ELF object file; they will still be assembled into a single section in the linked image.
> >
> > My understanding is that changing the flags on an MCSection retroactively is pretty counter to the architecture, so I'm not sure how we'd actually achieve #2.
>
>
> Ah, ok, I reread  https://reviews.llvm.org/D72194 and see that it's creating non-contiguous sections (option 1), with unique entity sizes.  That should be fine. Dissenting opinion retracted.  We should prefer https://reviews.llvm.org/D72194 with the commit message updated.

I have updated the commit message and patch on https://reviews.llvm.org/D72194.

I am abandoning this in favor of https://reviews.llvm.org/D72194.

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

https://reviews.llvm.org/D68101