[llvm] r230411 - Support SHF_MERGE sections in COMDATs.

Mikael Holmén mikael.holmen at ericsson.com
Thu Feb 26 02:22:37 PST 2015


Hi Rafael,

With this change our out-of-tree target breaks so I'm a little bit curious.

In our backend we override the text section

   // .text
   TextSection = getContext().getELFSection("code", ELF::SHT_LOUSER + 0,
                                            ELF::SHF_ALLOC | 
ELF::SHF_EXECINSTR);

so we get ".code" in the assembler output instead of ".text".

With the "old" implementation of TargetLoweringObjectFileELF::
SelectSectionForGlobal this worked fine, since it had

   if (Kind.isText()) return TextSection;

where it really used our overridden TextSection.

However, with this commit we now end up in

   return getContext().getELFSection(Name, getELFSectionType(Name, 
Kind), Flags,
                                     EntrySize, Group,
                                     EmitUniqueSection && 
!UniqueSectionNames);

with Name fetched from getSectionPrefixForGlobal, which just says 
".text", and we end up with ".text" in the assembler output instead of 
our wanted ".code".

Of course we can handle more cases in our backend in our own overridden 
version of SelectSectionForGlobal function, I'm just wondering if this 
was an expected outcome of the patch? Have we just been lucky it worked 
for us so far?

Best regards,
Mikael

On 02/25/2015 01:52 AM, Rafael Espindola wrote:
> Author: rafael
> Date: Tue Feb 24 18:52:15 2015
> New Revision: 230411
>
> URL: http://llvm.org/viewvc/llvm-project?rev=230411&view=rev
> Log:
> Support SHF_MERGE sections in COMDATs.
>
> This patch unifies the comdat and non-comdat code paths. By doing this
> it add missing features to the comdat side and removes the fixed
> section assumptions from the non-comdat side.
>
> In ELF there is no one true section for "4 byte mergeable" constants.
> We are better off computing the required properties of the section
> and asking the context for it.
>
> Modified:
>      llvm/trunk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
>      llvm/trunk/test/CodeGen/X86/global-sections-comdat.ll
>
> Modified: llvm/trunk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp?rev=230411&r1=230410&r2=230411&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp (original)
> +++ llvm/trunk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp Tue Feb 24 18:52:15 2015
> @@ -164,9 +164,7 @@ static unsigned getELFSectionType(String
>     return ELF::SHT_PROGBITS;
>   }
>
> -
> -static unsigned
> -getELFSectionFlags(SectionKind K, bool InCOMDAT) {
> +static unsigned getELFSectionFlags(SectionKind K) {
>     unsigned Flags = 0;
>
>     if (!K.isMetadata())
> @@ -181,10 +179,7 @@ getELFSectionFlags(SectionKind K, bool I
>     if (K.isThreadLocal())
>       Flags |= ELF::SHF_TLS;
>
> -  // FIXME: There is nothing in ELF preventing an SHF_MERGE from being
> -  // in a comdat. We just avoid it for now because we don't print
> -  // those .sections correctly.
> -  if (!InCOMDAT && (K.isMergeableCString() || K.isMergeableConst()))
> +  if (K.isMergeableCString() || K.isMergeableConst())
>       Flags |= ELF::SHF_MERGE;
>
>     if (K.isMergeableCString())
> @@ -214,7 +209,7 @@ const MCSection *TargetLoweringObjectFil
>     Kind = getELFKindForNamedSection(SectionName, Kind);
>
>     StringRef Group = "";
> -  unsigned Flags = getELFSectionFlags(Kind, GV->hasComdat());
> +  unsigned Flags = getELFSectionFlags(Kind);
>     if (const Comdat *C = getELFComdat(GV)) {
>       Group = C->getName();
>       Flags |= ELF::SHF_GROUP;
> @@ -249,97 +244,74 @@ static StringRef getSectionPrefixForGlob
>     return ".data.rel.ro";
>   }
>
> -static const MCSection *
> -getUniqueELFSection(MCContext &Ctx, const GlobalValue &GV, SectionKind Kind,
> -                    Mangler &Mang, const TargetMachine &TM, unsigned Flags) {
> -  StringRef Prefix = getSectionPrefixForGlobal(Kind);
> -
> -  SmallString<128> Name(Prefix);
> -  bool UniqueSectionNames = TM.getUniqueSectionNames();
> -  if (UniqueSectionNames) {
> -    Name.push_back('.');
> -    TM.getNameWithPrefix(Name, &GV, Mang, true);
> -  }
> -  StringRef Group = "";
> -  if (const Comdat *C = getELFComdat(&GV)) {
> -    Flags |= ELF::SHF_GROUP;
> -    Group = C->getName();
> -  }
> -
> -  return Ctx.getELFSection(Name, getELFSectionType(Name, Kind), Flags, 0, Group,
> -                           !UniqueSectionNames);
> -}
> -
>   const MCSection *TargetLoweringObjectFileELF::
>   SelectSectionForGlobal(const GlobalValue *GV, SectionKind Kind,
>                          Mangler &Mang, const TargetMachine &TM) const {
> -  unsigned Flags = getELFSectionFlags(Kind, GV->hasComdat());
> +  unsigned Flags = getELFSectionFlags(Kind);
>
>     // If we have -ffunction-section or -fdata-section then we should emit the
>     // global value to a uniqued section specifically for it.
> -  bool EmitUniquedSection = false;
> +  bool EmitUniqueSection = false;
>     if (!(Flags & ELF::SHF_MERGE) && !Kind.isCommon()) {
>       if (Kind.isText())
> -      EmitUniquedSection = TM.getFunctionSections();
> +      EmitUniqueSection = TM.getFunctionSections();
>       else
> -      EmitUniquedSection = TM.getDataSections();
> +      EmitUniqueSection = TM.getDataSections();
>     }
> +  EmitUniqueSection |= GV->hasComdat();
>
> -  if (EmitUniquedSection || GV->hasComdat())
> -    return getUniqueELFSection(getContext(), *GV, Kind, Mang, TM, Flags);
> +  unsigned EntrySize = 0;
> +  if (Kind.isMergeableCString()) {
> +    if (Kind.isMergeable2ByteCString()) {
> +      EntrySize = 2;
> +    } else if (Kind.isMergeable4ByteCString()) {
> +      EntrySize = 4;
> +    } else {
> +      EntrySize = 1;
> +      assert(Kind.isMergeable1ByteCString() && "unknown string width");
> +    }
> +  } else if (Kind.isMergeableConst()) {
> +    if (Kind.isMergeableConst4()) {
> +      EntrySize = 4;
> +    } else if (Kind.isMergeableConst8()) {
> +      EntrySize = 8;
> +    } else {
> +      assert(Kind.isMergeableConst16() && "unknown data width");
> +      EntrySize = 16;
> +    }
> +  }
>
> -  if (Kind.isText()) return TextSection;
> +  StringRef Group = "";
> +  if (const Comdat *C = getELFComdat(GV)) {
> +    Flags |= ELF::SHF_GROUP;
> +    Group = C->getName();
> +  }
>
> +  bool UniqueSectionNames = TM.getUniqueSectionNames();
> +  SmallString<128> Name;
>     if (Kind.isMergeableCString()) {
> -
>       // We also need alignment here.
>       // FIXME: this is getting the alignment of the character, not the
>       // alignment of the global!
>       unsigned Align =
>           TM.getDataLayout()->getPreferredAlignment(cast<GlobalVariable>(GV));
>
> -    unsigned EntrySize = 1;
> -    if (Kind.isMergeable2ByteCString())
> -      EntrySize = 2;
> -    else if (Kind.isMergeable4ByteCString())
> -      EntrySize = 4;
> -    else
> -      assert(Kind.isMergeable1ByteCString() && "unknown string width");
> -
>       std::string SizeSpec = ".rodata.str" + utostr(EntrySize) + ".";
> -    std::string Name = SizeSpec + utostr(Align);
> -    return getContext().getELFSection(
> -        Name, ELF::SHT_PROGBITS,
> -        ELF::SHF_ALLOC | ELF::SHF_MERGE | ELF::SHF_STRINGS, EntrySize, "");
> -  }
> -
> -  if (Kind.isMergeableConst()) {
> -    if (Kind.isMergeableConst4() && MergeableConst4Section)
> -      return MergeableConst4Section;
> -    if (Kind.isMergeableConst8() && MergeableConst8Section)
> -      return MergeableConst8Section;
> -    if (Kind.isMergeableConst16() && MergeableConst16Section)
> -      return MergeableConst16Section;
> -    return ReadOnlySection;  // .const
> -  }
> -
> -  if (Kind.isReadOnly())             return ReadOnlySection;
> -
> -  if (Kind.isThreadData())           return TLSDataSection;
> -  if (Kind.isThreadBSS())            return TLSBSSSection;
> -
> -  // Note: we claim that common symbols are put in BSSSection, but they are
> -  // really emitted with the magic .comm directive, which creates a symbol table
> -  // entry but not a section.
> -  if (Kind.isBSS() || Kind.isCommon()) return BSSSection;
> -
> -  if (Kind.isDataNoRel())            return DataSection;
> -  if (Kind.isDataRelLocal())         return DataRelLocalSection;
> -  if (Kind.isDataRel())              return DataRelSection;
> -  if (Kind.isReadOnlyWithRelLocal()) return DataRelROLocalSection;
> +    Name = SizeSpec + utostr(Align);
> +  } else if (Kind.isMergeableConst()) {
> +    Name = ".rodata.cst";
> +    Name += utostr(EntrySize);
> +  } else {
> +    Name = getSectionPrefixForGlobal(Kind);
> +  }
>
> -  assert(Kind.isReadOnlyWithRel() && "Unknown section kind");
> -  return DataRelROSection;
> +  if (EmitUniqueSection && UniqueSectionNames) {
> +    Name.push_back('.');
> +    TM.getNameWithPrefix(Name, GV, Mang, true);
> +  }
> +  return getContext().getELFSection(Name, getELFSectionType(Name, Kind), Flags,
> +                                    EntrySize, Group,
> +                                    EmitUniqueSection && !UniqueSectionNames);
>   }
>
>   const MCSection *TargetLoweringObjectFileELF::getSectionForJumpTable(
> @@ -351,8 +323,7 @@ const MCSection *TargetLoweringObjectFil
>     if (!EmitUniqueSection)
>       return ReadOnlySection;
>
> -  return getUniqueELFSection(getContext(), F, SectionKind::getReadOnly(), Mang,
> -                             TM, ELF::SHF_ALLOC);
> +  return SelectSectionForGlobal(&F, SectionKind::getReadOnly(), Mang, TM);
>   }
>
>   bool TargetLoweringObjectFileELF::shouldPutJumpTableInFunctionSection(
>
> Modified: llvm/trunk/test/CodeGen/X86/global-sections-comdat.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/global-sections-comdat.ll?rev=230411&r1=230410&r2=230411&view=diff
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/global-sections-comdat.ll (original)
> +++ llvm/trunk/test/CodeGen/X86/global-sections-comdat.ll Tue Feb 24 18:52:15 2015
> @@ -41,6 +41,6 @@ bb5:
>   $G16 = comdat any
>   @G16 = unnamed_addr constant i32 42, comdat
>
> -; LINUX: .section	.rodata.G16,"aG", at progbits,G16,comdat
> -; LINUX-SECTIONS: .section	.rodata.G16,"aG", at progbits,G16,comdat
> -; LINUX-SECTIONS-SHORT: .section	.rodata,"aG", at progbits,G16,comdat
> +; LINUX: .section	.rodata.cst4.G16,"aGM", at progbits,4,G16,comdat
> +; LINUX-SECTIONS: .section	.rodata.cst4.G16,"aGM", at progbits,4,G16,comdat
> +; LINUX-SECTIONS-SHORT: .section	.rodata.cst4,"aGM", at progbits,4,G16,comdat
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>



More information about the llvm-commits mailing list