Atoms generated by a pass should be owned by the generated file and not the pass itself.

[Jean-Daniel]

Summary

Some Mach-O specific passes own a MachOFile member that is used to allocate the generated atoms. This is a fragile pattern as the generated atoms and references lifespan are tied to the pass lifespan and freed when the pass instance is destroyed.

While it is not an issue today as the PassManager is not destroy before we write the linked file, there is no guarantee it will always work.

Now that the File base class provide an allocator, we can use the processed file's allocator to generate new atoms.

Note: I have commit rights, so if that patch is good, I'll be glad to commit it myself ;-)

[kledzik]

I see what you are trying to do, but it leaves the pass-generated atoms in an odd state. All the other atoms (from .o files) still have their file() method returning their original file. But the pass-generated atoms now have their file() return the temp mergedFile. When inspecting atoms in the Writer, it is nice to be able to see where they came from. This patch looses that.

Perhaps we can change Pass::perform() to return a std::unique_ptr<File> which is the owning file of any atoms generated during the pass. And the PassManager owns those Files even it it deletes the pass objects.

Also, I see this same pattern in other Passes. It is not mach-o specific.

[Jean-Daniel]

OK, missed that side effect. Having the PassManager owning File will not solve the issue as the PassManager is not guarantee to live until we no longer need the mergedFile.

Anyway, after some though, this change is pointless as the actual pattern is consistent with the other linking steps (input graph, resolver, …). If somehow we need to be able to link a file in memory and keep the resulting representation, we will just make sure the PassManager and other objects live long enough.

lib/ReaderWriter/MachO/ArchHandler.h

virtual Reference::KindValue dataInCodeTransitionEnd(
                                                 const MachODefinedAtom &atom) {
     return 0;
   }
 
   /// Only relevant for 32-bit arm archs.
   virtual bool isThumbFunction(const DefinedAtom &atom) { return false; }
 
   /// Only relevant for 32-bit arm archs.
-  virtual const DefinedAtom *createShim(MachOFile &file, bool thumbToArm,
+  virtual const DefinedAtom *createShim(const File &file, bool thumbToArm,
                                         const DefinedAtom &) {
     llvm_unreachable("shims only support on arm");
   }
 
   /// Does a given unwind-cfi atom represent a CIE (as opposed to an FDE).
   static bool isDwarfCIE(bool isBig, const DefinedAtom *atom);
 
   struct ReferenceInfo {

lib/ReaderWriter/MachO/ArchHandler_arm.cpp

public:
   }
 
   Reference::KindValue dataInCodeTransitionEnd(
                                         const MachODefinedAtom &atom) override {
     return atom.isThumb() ? modeThumbCode : modeArmCode;
   }
 
   bool isThumbFunction(const DefinedAtom &atom) override;
-  const DefinedAtom *createShim(MachOFile &file, bool thumbToArm,
+  const DefinedAtom *createShim(const File &file, bool thumbToArm,
                                 const DefinedAtom &) override;
 
 private:
   friend class Thumb2ToArmShimAtom;
   friend class ArmToThumbShimAtom;
 
   static const Registry::KindStrings _sKindStrings[];
   static const StubInfo              _sStubInfoArmPIC;
if (ref->kindValue() == modeThumbCode)
       return true;
   }
   return false;
 }
 
 
 class Thumb2ToArmShimAtom : public SimpleDefinedAtom {
 public:
-  Thumb2ToArmShimAtom(MachOFile &file, StringRef targetName,
+  Thumb2ToArmShimAtom(const File &file, StringRef targetName,
                       const DefinedAtom &target)
       : SimpleDefinedAtom(file) {
     addReference(Reference::KindNamespace::mach_o, Reference::KindArch::ARM,
                  ArchHandler_arm::modeThumbCode, 0, this, 0);
     addReference(Reference::KindNamespace::mach_o, Reference::KindArch::ARM,
                  ArchHandler_arm::delta32, 8, &target, 0);
     std::string name = std::string(targetName) + "$shim";
     StringRef tmp(name);
public:
   }
 private:
   StringRef _name;
 };
 
 
 class ArmToThumbShimAtom : public SimpleDefinedAtom {
 public:
-  ArmToThumbShimAtom(MachOFile &file, StringRef targetName,
+  ArmToThumbShimAtom(const File &file, StringRef targetName,
                      const DefinedAtom &target)
       : SimpleDefinedAtom(file) {
     addReference(Reference::KindNamespace::mach_o, Reference::KindArch::ARM,
                  ArchHandler_arm::delta32, 12, &target, 0);
     std::string name = std::string(targetName) + "$shim";
     StringRef tmp(name);
     _name = tmp.copy(file.allocator());
   }
static const uint8_t bytes[] =
       0x00, 0x00, 0x00, 0x00 };     //  .long target - this
     assert(sizeof(bytes) == size());
     return llvm::makeArrayRef(bytes, sizeof(bytes));
   }
 private:
   StringRef _name;
 };
 
-const DefinedAtom *ArchHandler_arm::createShim(MachOFile &file,
+const DefinedAtom *ArchHandler_arm::createShim(const File &file,
                                                bool thumbToArm,
                                                const DefinedAtom &target) {
   bool isStub = (target.contentType() == DefinedAtom::typeStub);
   StringRef targetName = isStub ? stubName(target) : target.name();
   if (thumbToArm)
     return new (file.allocator()) Thumb2ToArmShimAtom(file, targetName, target);
   else
     return new (file.allocator()) ArmToThumbShimAtom(file, targetName, target);

lib/ReaderWriter/MachO/CompactUnwindPass.cpp

 };
 
 /// Pass for instantiating and optimizing GOT slots.
 ///
 class CompactUnwindPass : public Pass {
 public:
   CompactUnwindPass(const MachOLinkingContext &context)
       : _context(context), _archHandler(_context.archHandler()),
-        _file("<mach-o Compact Unwind Pass>"),
         _isBig(MachOLinkingContext::isBigEndian(_context.arch())) {}
 
 private:
   void perform(std::unique_ptr<MutableFile> &mergedFile) override {
     DEBUG(llvm::dbgs() << "MachO Compact Unwind pass\n");
 
     std::map<const Atom *, CompactUnwindEntry> unwindLocs;
     std::map<const Atom *, const Atom *> dwarfFrames;
do {
 
       DEBUG(llvm::dbgs()
             << "    Page from " << pages.back().entries[0].rangeStart->name()
             << " to " << pages.back().entries.back().rangeStart->name() << " + "
             << llvm::format("0x%x", pages.back().entries.back().rangeLength)
             << " has " << entriesInPage << " entries\n");
     } while (pageStart < unwindInfos.size());
 
-    UnwindInfoAtom *unwind = new (_file.allocator())
-        UnwindInfoAtom(_archHandler, _file, _isBig, personalities,
+    UnwindInfoAtom *unwind = new (mergedFile->allocator())
+        UnwindInfoAtom(_archHandler, *mergedFile, _isBig, personalities,
                        commonEncodings, pages, numLSDAs);
     mergedFile->addAtom(*unwind);
 
     // Finally, remove all __compact_unwind atoms now that we've processed them.
     mergedFile->removeDefinedAtomsIf([](const DefinedAtom *atom) {
       return atom->contentType() == DefinedAtom::typeCompactUnwindInfo;
     });
   }
CompactUnwindEntry finalizeUnwindInfoEntryForAtom(
     if (entry.lsdaLocation)
       entry.encoding |= 1U << 30;
 
     return entry;
   }
 
   const MachOLinkingContext &_context;
   mach_o::ArchHandler &_archHandler;
-  MachOFile _file;
   bool _isBig;
 };
 
 void addCompactUnwindPass(PassManager &pm, const MachOLinkingContext &ctx) {
   assert(ctx.needsCompactUnwindPass());
   pm.add(std::unique_ptr<Pass>(new CompactUnwindPass(ctx)));
 }
 
 } // end namesapce mach_o
 } // end namesapce lld

lib/ReaderWriter/MachO/GOTPass.cpp

 };
 
 
 /// Pass for instantiating and optimizing GOT slots.
 /// 
 class GOTPass : public Pass {
 public:
   GOTPass(const MachOLinkingContext &context)
-    : _context(context), _archHandler(_context.archHandler()),
-      _file("<mach-o GOT Pass>") { } 
+    : _context(context), _archHandler(_context.archHandler()) { }
 
 private:
 
   void perform(std::unique_ptr<MutableFile> &mergedFile) override {
     // Scan all references in all atoms.
     for (const DefinedAtom *atom : mergedFile->defined()) {
       for (const Reference *ref : *atom) {
         // Look at instructions accessing the GOT.
         bool canBypassGOT;
         if (!_archHandler.isGOTAccess(*ref, canBypassGOT))
           continue;
         const Atom *target = ref->target();
         assert(target != nullptr);
 
         if (!shouldReplaceTargetWithGOTAtom(target, canBypassGOT)) {
           // Update reference kind to reflect that target is a direct accesss.
           _archHandler.updateReferenceToGOT(ref, false);
         } else {
           // Replace the target with a reference to a GOT entry.
-          const DefinedAtom *gotEntry = makeGOTEntry(target);
+          const DefinedAtom *gotEntry = makeGOTEntry(*mergedFile, target);
           const_cast<Reference *>(ref)->setTarget(gotEntry);
           // Update reference kind to reflect that target is now a GOT entry.
           _archHandler.updateReferenceToGOT(ref, true);
         }
       }
     }
 
     // Sort and add all created GOT Atoms to master file
if (defTarget != nullptr &&
       assert(defTarget->scope() != DefinedAtom::scopeTranslationUnit);
       return true;
     }
     // Target does not require indirection.  So, if instruction allows GOT to be
     // by-passed, do that optimization and don't create GOT entry.
     return !canBypassGOT;
   }
   
-  const DefinedAtom *makeGOTEntry(const Atom *target) {
+  const DefinedAtom *makeGOTEntry(const File &file, const Atom *target) {
     auto pos = _targetToGOT.find(target);
     if (pos == _targetToGOT.end()) {
-      GOTEntryAtom *gotEntry = new (_file.allocator()) 
-          GOTEntryAtom(_file, _context.is64Bit(), target->name());
+      GOTEntryAtom *gotEntry = new (file.allocator())
+          GOTEntryAtom(file, _context.is64Bit(), target->name());
       _targetToGOT[target] = gotEntry;
       const ArchHandler::ReferenceInfo &nlInfo = _archHandler.stubInfo().
                                                 nonLazyPointerReferenceToBinder;
       gotEntry->addReference(Reference::KindNamespace::mach_o, nlInfo.arch,
                              nlInfo.kind, 0, target, 0);
       return gotEntry;
     }
     return pos->second;
   }
 
   
   const MachOLinkingContext                       &_context;
   mach_o::ArchHandler                             &_archHandler;
-  MachOFile                                        _file;
   llvm::DenseMap<const Atom*, const GOTEntryAtom*> _targetToGOT;
 };
 
 
 
 void addGOTPass(PassManager &pm, const MachOLinkingContext &ctx) {
   assert(ctx.needsGOTPass());
   pm.add(std::unique_ptr<Pass>(new GOTPass(ctx)));
 }
   
 
 } // end namesapce mach_o
 } // end namesapce lld

lib/ReaderWriter/MachO/ShimPass.cpp

 namespace lld {
 namespace mach_o {
 
 class ShimPass : public Pass {
 public:
   ShimPass(const MachOLinkingContext &context)
     : _context(context)
     , _archHandler(_context.archHandler())
-    , _stubInfo(_archHandler.stubInfo())
-    , _file("<mach-o shim pass>") {
+    , _stubInfo(_archHandler.stubInfo()) {
   }
 
 
   void perform(std::unique_ptr<MutableFile> &mergedFile) override {
     // Scan all references in all atoms.
     for (const DefinedAtom *atom : mergedFile->defined()) {
       for (const Reference *ref : *atom) {
         // Look at non-call branches.
         if (!_archHandler.isNonCallBranch(*ref))
           continue;
         const Atom *target = ref->target();
         assert(target != nullptr);
         if (const lld::DefinedAtom *daTarget = dyn_cast<DefinedAtom>(target)) {
           bool atomIsThumb = _archHandler.isThumbFunction(*atom);
           bool targetIsThumb = _archHandler.isThumbFunction(*daTarget);
           if (atomIsThumb != targetIsThumb)
-            updateBranchToUseShim(atomIsThumb, *daTarget, ref);
+            updateBranchToUseShim(*mergedFile, atomIsThumb, *daTarget, ref);
         }
       }
     }
     // Exit early if no shims needed.
     if (_targetToShim.empty())
       return;
 
     // Sort shim atoms so the layout order is stable.
void perform(std::unique_ptr<MutableFile> &mergedFile) override {
     // Add all shims to master file.
     for (const DefinedAtom *shim : shims) {
       mergedFile->addAtom(*shim);
     }
   }
 
 private:
 
-  void updateBranchToUseShim(bool thumbToArm, const DefinedAtom& target,
-                             const Reference *ref) {
+  void updateBranchToUseShim(const File &file, bool thumbToArm,
+                             const DefinedAtom& target, const Reference *ref) {
     // Make file-format specific stub and other support atoms.
-    const DefinedAtom *shim = this->getShim(thumbToArm, target);
+    const DefinedAtom *shim = this->getShim(file, thumbToArm, target);
     assert(shim != nullptr);
     // Switch branch site to target shim atom.
     const_cast<Reference *>(ref)->setTarget(shim);
   }
 
-  const DefinedAtom* getShim(bool thumbToArm, const DefinedAtom& target) {
+  const DefinedAtom* getShim(const File &file, bool thumbToArm,
+                             const DefinedAtom& target) {
     auto pos = _targetToShim.find(&target);
     if ( pos != _targetToShim.end() ) {
       // Reuse an existing shim.
       assert(pos->second != nullptr);
       return pos->second;
     } else {
       // There is no existing shim, so create a new one.
-      const DefinedAtom *shim = _archHandler.createShim(_file, thumbToArm,
+      const DefinedAtom *shim = _archHandler.createShim(file, thumbToArm,
                                                         target);
        _targetToShim[&target] = shim;
        return shim;
     }
   }
 
   const MachOLinkingContext                      &_context;
   mach_o::ArchHandler                            &_archHandler;
   const ArchHandler::StubInfo                    &_stubInfo;
-  MachOFile                                       _file;
   llvm::DenseMap<const Atom*, const DefinedAtom*> _targetToShim;
 };
 
 
 
 void addShimPass(PassManager &pm, const MachOLinkingContext &ctx) {
   pm.add(std::unique_ptr<Pass>(new ShimPass(ctx)));
 }
 
 } // end namespace mach_o
 } // end namespace lld

lib/ReaderWriter/MachO/StubsPass.cpp

private:
   const ArchHandler::StubInfo   &_stubInfo;
 };
 
 
 class StubsPass : public Pass {
 public:
   StubsPass(const MachOLinkingContext &context)
     : _context(context), _archHandler(_context.archHandler()),
-    _stubInfo(_archHandler.stubInfo()), _file("<mach-o Stubs pass>") { }
+    _stubInfo(_archHandler.stubInfo()) { }
 
 
   void perform(std::unique_ptr<MutableFile> &mergedFile) override {
     // Skip this pass if output format uses text relocations instead of stubs.
     if (!this->noTextRelocs())
       return;
     
     // Scan all references in all atoms.
void perform(std::unique_ptr<MutableFile> &mergedFile) override {
     }
 
     // Exit early if no stubs needed.
     if (_targetToUses.empty())
       return;
 
     // First add help-common and GOT slots used by lazy binding.
     SimpleDefinedAtom *helperCommonAtom =
-        new (_file.allocator()) StubHelperCommonAtom(_file, _stubInfo);
+        new (mergedFile->allocator()) StubHelperCommonAtom(*mergedFile,
+                                                           _stubInfo);
     SimpleDefinedAtom *helperCacheNLPAtom =
-        new (_file.allocator()) NonLazyPointerAtom(_file, _context.is64Bit());
+        new (mergedFile->allocator()) NonLazyPointerAtom(*mergedFile,
+                                                         _context.is64Bit());
     SimpleDefinedAtom *helperBinderNLPAtom =
-        new (_file.allocator()) NonLazyPointerAtom(_file, _context.is64Bit());
+        new (mergedFile->allocator()) NonLazyPointerAtom(*mergedFile,
+                                                         _context.is64Bit());
     addReference(helperCommonAtom, _stubInfo.stubHelperCommonReferenceToCache,
                  helperCacheNLPAtom);
     addOptReference(
         helperCommonAtom, _stubInfo.stubHelperCommonReferenceToCache,
         _stubInfo.optStubHelperCommonReferenceToCache, helperCacheNLPAtom);
     addReference(helperCommonAtom, _stubInfo.stubHelperCommonReferenceToBinder,
                  helperBinderNLPAtom);
     addOptReference(
void perform(std::unique_ptr<MutableFile> &mergedFile) override {
     std::sort(targetsNeedingStubs.begin(), targetsNeedingStubs.end(),
               [](const Atom * left, const Atom * right) {
       return (left->name().compare(right->name()) < 0);
     });
 
     // Make and append stubs, lazy pointers, and helpers in alphabetical order.
     unsigned lazyOffset = 0;
     for (const Atom *target : targetsNeedingStubs) {
-      StubAtom *stub = new (_file.allocator()) StubAtom(_file, _stubInfo);
+      StubAtom *stub = new (mergedFile->allocator()) StubAtom(*mergedFile,
+                                                              _stubInfo);
       LazyPointerAtom *lp =
-          new (_file.allocator()) LazyPointerAtom(_file, _context.is64Bit());
+          new (mergedFile->allocator()) LazyPointerAtom(*mergedFile,
+                                                        _context.is64Bit());
       StubHelperAtom *helper =
-          new (_file.allocator()) StubHelperAtom(_file, _stubInfo);
+          new (mergedFile->allocator()) StubHelperAtom(*mergedFile,
+                                                       _stubInfo);
 
       addReference(stub, _stubInfo.stubReferenceToLP, lp);
       addOptReference(stub, _stubInfo.stubReferenceToLP,
                       _stubInfo.optStubReferenceToLP, lp);
       addReference(lp, _stubInfo.lazyPointerReferenceToHelper, helper);
       addReference(lp, _stubInfo.lazyPointerReferenceToFinal, target);
       addReference(helper, _stubInfo.stubHelperReferenceToImm, helper);
       addReferenceAddend(helper, _stubInfo.stubHelperReferenceToImm, helper,
private:
   }
 
   typedef llvm::DenseMap<const Atom*,
                          llvm::SmallVector<const Reference *, 8>> TargetToUses;
 
   const MachOLinkingContext                      &_context;
   mach_o::ArchHandler                            &_archHandler;
   const ArchHandler::StubInfo                    &_stubInfo;
-  MachOFile                                       _file;
   TargetToUses                                    _targetToUses;
 };
 
 
 
 void addStubsPass(PassManager &pm, const MachOLinkingContext &ctx) {
   pm.add(std::unique_ptr<Pass>(new StubsPass(ctx)));
 }
 
 } // end namespace mach_o
 } // end namespace lld