[MachO] Stop generating *coal* sections.

[ahatanak]

Summary

Some background on why we don't have to use *coal* sections anymore:
Long ago when C++ was new and "weak" had not been standardized, an attempt was made in cctools to support C++ inlines that can be coalesced by putting them into their own section (TEXT/textcoal_nt instead of TEXT/text).

The current macho linker supports the weak-def bit on any symbol to allow it to be coalesced, but the compiler still puts weak-def functions/data into alternate section names, which the linker must map back to the base section name.

This patch makes changes that are necessary to prevent the compiler and asssembler from using the "coal" sections and have them use the base sections instead when the target architecture is not powerpc:

TEXT/textcoal_nt instead use TEXT/text
TEXT/const_coal instead use TEXT/const
DATA/datacoal_nt instead use DATA/data

If the target is powerpc, we continue to use the *coal* sections since anyone targeting powerpc is probably using an old linker that doesn't have support for the weak-def bits.

[ahatanak]

Fix a bug in the custom deleter passed to constructor of shared_ptr where the destructor of MCSectionMachO wasn't being called. Also, move the call to MachOUniquingMap.clear() up so that it's called before Allocator.Reset() is called.

[• rafael]

include/llvm/MC/MCContext.h

204 ↗

(On Diff #35915)

This is needed because we pool allocate sections, but not fragments and then have to manually call the section destructors.

Could you try pool allocating the fragments first? There are way more fragments than sections, so this would be a good change anyway.

[ahatanak]

include/llvm/MC/MCContext.h

204 ↗

(On Diff #35915)

I think I need more clarification to understand what you are suggesting.

Do you mean we don't need to use shared_ptr after we make changes to use BumpPtrAllocator for MCFragments?

[• rafael]

I think I need more clarification to understand what you are suggesting.

Do you mean we don't need to use shared_ptr after we make changes to use BumpPtrAllocator for MCFragments?

Correct. The reason we have

// Call the destructors so the fragments are freed
for (auto &I : ELFUniquingMap)
  I.second->~MCSectionELF();
for (auto &I : COFFUniquingMap)
  I.second->~MCSectionCOFF();
for (auto &I : MachOUniquingMap)
  I.second->~MCSectionMachO();

Is to avoid leaking the fragments.

Cheers,
Rafael

[ahatanak]

But you would still want to call the destructors (either explicitly or implicitly) of MCSection subclasses, right? If we don't run the destructors, we'll be leaking MCSection::SubsectionFragmentMap, even if we make changes to use BumpPtrAllocator for MCSection::Fragments.

If you are worried about the overhead of using shared_ptr, I think we can continue using raw pointers and add checks that prevent the destructor from being run twice, although I'm not sure that would be a good idea.

[vsk]

I think Rafael's suggestion is probably a good one, but is unrelated to Akira's problem. Comment inline --

include/llvm/MC/MCContext.h

204 ↗

(On Diff #35915)

@rafael - The "Fragments" list and the "SubsectionFragmentMap" SmallVector both need to be destroyed. Using a pool allocator for fragments is probably a good idea, but it won't solve Akira's problem since the SmallVector's contents would be leaked.

If there's a concern about the overhead of shared_ptr, we could consider adding a refcount field to MCSection proper. That at least eliminates extra allocations for shared_ptr control blocks.

Let me know what you think :).

[• rafael]

But you would still want to call the destructors (either explicitly or implicitly) of MCSection subclasses, right? If we don't run the destructors, we'll be leaking MCSection::SubsectionFragmentMap, even if we make changes to use BumpPtrAllocator for MCSection::Fragments.

You are right, sorry about that.

If you are worried about the overhead of using shared_ptr, I think we can continue using raw pointers and add checks that prevent the destructor from being run twice, although I'm not sure that would be a good idea.

It is just that changes like this really shouldn't need to worry about
memory management. If destructors must be called, we should probably
just use SpecificBumpPtrAllocator. What do you think of the attached
patch?

Cheers,
Rafael

• t.diff3 KBDownload

[ahatanak]

Yes, using SpecificBumpPtrAllocator solves all the problems without using shared_ptr. The patch looks fine, except that there were build errors because some of the destructors were declared private.

I'll update my patch after your patch is committed.

[• rafael]

r249589.

[ahatanak]

Thanks, my patch looks much simpler now.

[• rafael]

This is fine by me from a MC perspective.

If no one objects from the MachO side, go for it.

lib/MC/MCObjectFileInfo.cpp

126 ↗

(On Diff #36780)

ppc64le darwin? :-)

[ahatanak]

Sorry, I forgot to make changes in getMachOSectionRef to use getMachOSectionRef.

lib/MC/MCObjectFileInfo.cpp

126 ↗

(On Diff #36780)

OK, that check isn't needed.

[grosbach]

The idea seems fine. The implementation is off, though. Why are we re-mapping the section names after the fact instead of setting them correctly in the first place? The call to setMachOCoalSections() is already even being added in exactly the same function which would need to be modified for that.

Something like:

bool useCoalSections = ArchTy != Triple::ppc && ArchTy != Triple::ppc64;
const char *TextCoalSectionName = useCoalSections ? "textcoal_nt" : "text";
...

TextCoalSection                                                                
  = Ctx->getMachOSection("__TEXT", TextCoalSectionName,                            
                         MachO::S_COALESCED |                                  
                         MachO::S_ATTR_PURE_INSTRUCTIONS,                      
                         SectionKind::getText());

[ahatanak]

Perhaps I misunderstood the suggestion, but MachOUniquingMap will not have an entry that has key "TEXT,textcoal_n", is that right?

I think this change is fine for code-gen (if the code doesn't have any inline-asm), but would it also work when assembling a .s file or inline-asm statement? DarwinAsmParser calls MCContext::getMachOSection if it encounters section directive that switches to "TEXT,textcoal_n" in the .s file, but MCContext::getMachOSection wouldn't return section for "TEXT,text" if we didn't enter the entry for "TEXT,textcoal_n", would it?

[grosbach]

If an assembly file explicitly references a section with the old name, we should not be re-mapping that to the new name. People writing assembly expect the output to be exactly what they put in the .s file and we should honor that.

That said, a warning diagnostic saying that those sections are deprecated would be a reasonable follow-up patch.

[ahatanak]

Address Jim's review comments.

This patch makes changes to have the assembler issue a warning to the user if it encounters a directive to switch to a coal section, rather than switching to the corresponding non-coal section.

[grosbach]

lib/MC/MCParser/DarwinAsmParser.cpp

599 ↗

(On Diff #37373)

Warning phrasing should match the usual clang/assembler pattern. No initial capital letter, single sentence, with an additional note for the correction.

Something like:
foo.s:1:1: warning: section "__textcoal_nt" is deprecated
.section __TEXT,__textcoal_nt
^~~~~~~~~~~~~
foo.s:1:1: change section name to "__text"
.section __TEXT,__textcoal_nt
^~~~~~~~~~~~~

This usage is analogous to the warning given if the return type of main() in a C program is not "int."

[ahatanak]

Make changes to improve warning message.

[ahatanak]

With this patch, the warning message looks like this:

test/MC/MachO/coal-sections-x86_64.s:27:11: warning: section "__textcoal_nt" is deprecated

.section        __TEXT,__textcoal_nt,coalesced,pure_instructions
                    ^           ~~~~~~~~~~

test/MC/MachO/coal-sections-x86_64.s:27:11: note: change section name to "__text"

.section        __TEXT,__textcoal_nt,coalesced,pure_instructions
                    ^           ~~~~~~~~~~

[grosbach]

LGTM. Thanks!

[ahatanak]

Thanks, I'll commit this patch shortly.

llvm/trunk/lib/MC/MCObjectFileInfo.cpp

SixteenByteConstantSection // .literal16
       = Ctx->getMachOSection("__TEXT", "__literal16",
                              MachO::S_16BYTE_LITERALS,
                              SectionKind::getMergeableConst16());
 
   ReadOnlySection  // .const
     = Ctx->getMachOSection("__TEXT", "__const", 0,
                            SectionKind::getReadOnly());
 
+  // If the target is not powerpc, map the coal sections to the non-coal
+  // sections.
+  //
+  // "__TEXT/__textcoal_nt" => section "__TEXT/__text"
+  // "__TEXT/__const_coal"  => section "__TEXT/__const"
+  // "__DATA/__datacoal_nt" => section "__DATA/__data"
+  Triple::ArchType ArchTy = T.getArch();
+
+  if (ArchTy == Triple::ppc || ArchTy == Triple::ppc64) {
     TextCoalSection
       = Ctx->getMachOSection("__TEXT", "__textcoal_nt",
                              MachO::S_COALESCED |
                              MachO::S_ATTR_PURE_INSTRUCTIONS,
                              SectionKind::getText());
     ConstTextCoalSection
       = Ctx->getMachOSection("__TEXT", "__const_coal",
                              MachO::S_COALESCED,
                              SectionKind::getReadOnly());
-  ConstDataSection  // .const_data
-    = Ctx->getMachOSection("__DATA", "__const", 0,
-                           SectionKind::getReadOnlyWithRel());
     DataCoalSection
       = Ctx->getMachOSection("__DATA","__datacoal_nt",
                              MachO::S_COALESCED,
                              SectionKind::getDataRel());
+  } else {
+    TextCoalSection = TextSection;
+    ConstTextCoalSection = ReadOnlySection;
+    DataCoalSection = DataSection;
+  }
+
+  ConstDataSection  // .const_data
+    = Ctx->getMachOSection("__DATA", "__const", 0,
+                           SectionKind::getReadOnlyWithRel());
   DataCommonSection
     = Ctx->getMachOSection("__DATA","__common",
                            MachO::S_ZEROFILL,
                            SectionKind::getBSS());
   DataBSSSection
     = Ctx->getMachOSection("__DATA","__bss", MachO::S_ZEROFILL,
                            SectionKind::getBSS());
 

llvm/trunk/lib/MC/MCParser/DarwinAsmParser.cpp

 //===- DarwinAsmParser.cpp - Darwin (Mach-O) Assembly Parser --------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/MC/MCParser/MCAsmParserExtension.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCObjectFileInfo.h"
 #include "llvm/MC/MCParser/MCAsmLexer.h"
 #include "llvm/MC/MCParser/MCAsmParser.h"
 #include "llvm/MC/MCSectionMachO.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/SourceMgr.h"
bool DarwinAsmParser::parseDirectiveSection(StringRef, SMLoc) {
   bool TAAParsed;
   std::string ErrorStr =
     MCSectionMachO::ParseSectionSpecifier(SectionSpec, Segment, Section,
                                           TAA, TAAParsed, StubSize);
 
   if (!ErrorStr.empty())
     return Error(Loc, ErrorStr.c_str());
 
+  // Issue a warning if the target is not powerpc and Section is a *coal* section.
+  Triple TT = getParser().getContext().getObjectFileInfo()->getTargetTriple();
+  Triple::ArchType ArchTy = TT.getArch();
+
+  if (ArchTy != Triple::ppc && ArchTy != Triple::ppc64) {
+    StringRef NonCoalSection = StringSwitch<StringRef>(Section)
+                                   .Case("__textcoal_nt", "__text")
+                                   .Case("__const_coal", "__const")
+                                   .Case("__datacoal_nt", "__data")
+                                   .Default(Section);
+
+    if (!Section.equals(NonCoalSection)) {
+      StringRef SectionVal(Loc.getPointer());
+      size_t B = SectionVal.find(',') + 1, E = SectionVal.find(',', B);
+      SMLoc BLoc = SMLoc::getFromPointer(SectionVal.data() + B);
+      SMLoc ELoc = SMLoc::getFromPointer(SectionVal.data() + E);
+      getParser().Warning(Loc, "section \"" + Section + "\" is deprecated",
+                          SMRange(BLoc, ELoc));
+      getParser().Note(Loc, "change section name to \"" + NonCoalSection +
+                       "\"", SMRange(BLoc, ELoc));
+    }
+  }
+
   // FIXME: Arch specific.
   bool isText = Segment == "__TEXT";  // FIXME: Hack.
   getStreamer().SwitchSection(getContext().getMachOSection(
                                 Segment, Section, TAA, StubSize,
                                 isText ? SectionKind::getText()
                                 : SectionKind::getDataRel()));
   return false;
 }

llvm/trunk/test/CodeGen/PowerPC/coal-sections.ll

+; RUN: llc < %s -mtriple powerpc-apple-darwin | FileCheck %s
+
+; Check that *coal* sections are emitted.
+
+; CHECK: .section  __TEXT,__textcoal_nt,coalesced,pure_instructions
+; CHECK: .section  __TEXT,__textcoal_nt,coalesced,pure_instructions
+; CHECK-NEXT: .globl  _foo
+
+; CHECK: .section  __TEXT,__const_coal,coalesced
+; CHECK-NEXT: .globl  _a
+
+; CHECK: .section  __DATA,__datacoal_nt,coalesced
+; CHECK-NEXT: .globl  _b
+
+@a = weak_odr constant [4 x i32] [i32 1, i32 2, i32 3, i32 4], align 16
+@b = weak global i32 5, align 4
+@g = common global i32* null, align 8
+
+; Function Attrs: nounwind ssp uwtable
+define weak i32* @foo() {
+entry:
+  store i32* getelementptr inbounds ([4 x i32], [4 x i32]* @a, i64 0, i64 0), i32** @g, align 8
+  ret i32* @b
+}

llvm/trunk/test/CodeGen/X86/coal-sections.ll

+; RUN: llc < %s -mtriple x86_64-apple-darwin | FileCheck %s
+
+; Check that *coal* sections are not emitted.
+
+; CHECK: .section  __TEXT,__text,regular,pure_instructions{{$}}
+; CHECK-NEXT: .globl  _foo
+
+; CHECK: .section  __TEXT,__const{{$}}
+; CHECK-NEXT: .globl  _a
+
+; CHECK: .section  __DATA,__data{{$}}
+; CHECK-NEXT: .globl  _b
+
+@a = weak_odr constant [4 x i32] [i32 1, i32 2, i32 3, i32 4], align 16
+@b = weak global i32 5, align 4
+@g = common global i32* null, align 8
+
+; Function Attrs: nounwind ssp uwtable
+define weak i32* @foo() {
+entry:
+  store i32* getelementptr inbounds ([4 x i32], [4 x i32]* @a, i64 0, i64 0), i32** @g, align 8
+  ret i32* @b
+}

llvm/trunk/test/CodeGen/X86/global-sections.ll

 
 
 ; const int G2 __attribute__((weak)) = 42;
 @G2 = weak_odr unnamed_addr constant i32 42     
 
 
 ; TODO: linux drops this into .rodata, we drop it into ".gnu.linkonce.r.G2"
 
-; DARWIN: .section __TEXT,__const_coal,coalesced
+; DARWIN: .section __TEXT,__const{{$}}
 ; DARWIN: _G2:
 ; DARWIN:    .long 42
 
 
 ; int * const G3 = &G1;
 @G3 = unnamed_addr constant i32* @G1
 
 ; DARWIN: .section        __DATA,__const

 
 ; PR4584
 @"foo bar" = linkonce global i32 42
 
 ; LINUX: .type  "foo bar",@object
 ; LINUX: .weak  "foo bar"
 ; LINUX: "foo bar":
 
-; DARWIN: .section              __DATA,__datacoal_nt,coalesced
 ; DARWIN: .globl        "_foo bar"
 ; DARWIN:       .weak_definition "_foo bar"
 ; DARWIN: "_foo bar":
 
 ; PR4650
 @G6 = weak_odr unnamed_addr constant [1 x i8] c"\01"
 
 ; LINUX:   .type        G6,@object
 ; LINUX:   .weak        G6
 ; LINUX: G6:
 ; LINUX:   .byte        1
 ; LINUX:   .size        G6, 1
 
-; DARWIN:  .section __TEXT,__const_coal,coalesced
+; DARWIN:  .section __TEXT,__const{{$}}
 ; DARWIN:  .globl _G6
 ; DARWIN:  .weak_definition _G6
 ; DARWIN:_G6:
 ; DARWIN:  .byte 1
 
 
 @G7 = unnamed_addr constant [10 x i8] c"abcdefghi\00"
 

 ; LINUX:        .section        .rodata.str4.4,"aMS",@progbits,4
 ; LINUX:        .globl G9
 ; LINUX:G9
 
 
 @G10 = weak global [100 x i32] zeroinitializer, align 32 ; <[100 x i32]*> [#uses=0]
 
 
-; DARWIN:       .section        __DATA,__datacoal_nt,coalesced
+; DARWIN:       .section        __DATA,__data{{$}}
 ; DARWIN: .globl _G10
 ; DARWIN:       .weak_definition _G10
 ; DARWIN:       .align  5
 ; DARWIN: _G10:
 ; DARWIN:       .space  400
 
 ; LINUX:        .bss
 ; LINUX:        .weak   G10

llvm/trunk/test/MC/MachO/coal-sections-powerpc.s

+// RUN: llvm-mc -triple powerpc-apple-darwin -filetype=obj %s -o - | llvm-readobj -sections | FileCheck %s
+
+// CHECK: Section {
+// CHECK-NEXT: Index: 0
+
+// CHECK: Section {
+// CHECK-NEXT: Index: 1
+// CHECK-NEXT: Name: __textcoal_nt (
+
+// CHECK: Section {
+// CHECK-NEXT: Index: 2
+
+// CHECK: Section {
+// CHECK-NEXT: Index: 3
+// CHECK-NEXT: Name: __const_coal (
+
+// CHECK: Section {
+// CHECK-NEXT: Index: 4
+// CHECK-NEXT: Name: __datacoal_nt (
+
+  .section  __TEXT,__text,regular,pure_instructions
+  .machine ppc
+  .section  __TEXT,__textcoal_nt,coalesced,pure_instructions
+  .section  __TEXT,__symbol_stub1,symbol_stubs,pure_instructions,16
+  .section  __TEXT,__text,regular,pure_instructions
+  .section  __TEXT,__textcoal_nt,coalesced,pure_instructions
+  .globl  _foo
+  .weak_definition  _foo
+  .align  4
+_foo:
+  blr
+
+.subsections_via_symbols
+  .section  __TEXT,__const_coal,coalesced
+  .globl  _a                      ; @a
+  .weak_definition  _a
+  .align  4
+_a:
+  .long 1                       ; 0x1
+
+  .section  __DATA,__datacoal_nt,coalesced
+  .globl  _b                      ; @b
+  .weak_definition  _b
+  .align  2
+_b:
+  .long 5                       ; 0x5

llvm/trunk/test/MC/MachO/coal-sections-x86_64.s

+// RUN: llvm-mc -triple x86_64-apple-darwin -filetype=obj %s -o - 2>%t.err | llvm-readobj -sections | FileCheck %s
+// RUN: FileCheck --check-prefix=WARNING < %t.err %s
+
+// CHECK: Section {
+// CHECK-NEXT: Index: 0
+// CHECK-NEXT: Name: __text (
+
+// CHECK: Section {
+// CHECK-NEXT: Index: 1
+// CHECK-NEXT: Name: __textcoal_nt (
+
+// CHECK: Section {
+// CHECK-NEXT: Index: 2
+// CHECK-NEXT: Name: __const_coal (
+
+// CHECK: Section {
+// CHECK-NEXT: Index: 3
+// CHECK-NEXT: Name: __datacoal_nt (
+
+// WARNING: warning: section "__textcoal_nt" is deprecated
+// WARNING: note: change section name to "__text"
+// WARNING: warning: section "__const_coal" is deprecated
+// WARNING: note: change section name to "__const"
+// WARNING: warning: section "__datacoal_nt" is deprecated
+// WARNING: note: change section name to "__data"
+
+  .section  __TEXT,__textcoal_nt,coalesced,pure_instructions
+  .globl  _foo
+  .weak_definition  _foo
+  .align  4, 0x90
+_foo:
+  retq
+
+  .section  __TEXT,__const_coal,coalesced
+  .globl  _a                      ## @a
+  .weak_definition  _a
+  .align  4
+_a:
+  .long 1                       ## 0x1
+
+  .section  __DATA,__datacoal_nt,coalesced
+  .globl  _b                      ## @b
+  .weak_definition  _b
+  .align  2
+_b:
+  .long 5                       ## 0x5
+
+.subsections_via_symbols