This is an archive of the discontinued LLVM Phabricator instance.

[MachO] Stop generating *coal* sections.
ClosedPublic

Authored by ahatanak on Sep 25 2015, 7:33 PM.

Details

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.

Diff Detail

Event Timeline

ahatanak updated this revision to Diff 35791.Sep 25 2015, 7:33 PM
ahatanak retitled this revision from to [MachO] Stop generating *coal* sections..
ahatanak updated this object.
ahatanak added a subscriber: llvm-commits.
ahatanak updated this revision to Diff 35915.Sep 28 2015, 3:33 PM

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 added inline comments.
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 added inline comments.Oct 6 2015, 12:25 PM
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 edited edge metadata.Oct 6 2015, 12:40 PM
rafael added a subscriber: 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

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.

ahatanak added a subscriber: vsk.Oct 6 2015, 4:30 PM
vsk added a comment.Oct 6 2015, 4:49 PM

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 :).

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

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.

ahatanak updated this revision to Diff 36780.Oct 7 2015, 12:55 PM
ahatanak edited edge metadata.

Thanks, my patch looks much simpler now.

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

ppc64le darwin? :-)

ahatanak updated this revision to Diff 36786.Oct 7 2015, 1:18 PM
ahatanak marked an inline comment as done.

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

lib/MC/MCObjectFileInfo.cpp
126

OK, that check isn't needed.

grosbach edited edge metadata.Oct 7 2015, 1:38 PM

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());

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?

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 updated this revision to Diff 37373.Oct 14 2015, 12:32 PM
ahatanak edited edge metadata.

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 added inline comments.Oct 14 2015, 1:05 PM
lib/MC/MCParser/DarwinAsmParser.cpp
599

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 updated this revision to Diff 37397.Oct 14 2015, 3:14 PM

Make changes to improve warning message.

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 accepted this revision.Oct 14 2015, 3:18 PM
grosbach edited edge metadata.

LGTM. Thanks!

This revision is now accepted and ready to land.Oct 14 2015, 3:18 PM

Thanks, I'll commit this patch shortly.

This revision was automatically updated to reflect the committed changes.