Only emit files with subsections_via_symbols if all inputs had that set

[pete]

Summary

Hi Nick, Lang

The test case parse-data-relocs-x86_64.yaml wasn't generating the same output when round tripped through lld -r.

The reason was that we were always emitting the file with the MH_SUBSECTIONS_VIA_SYMBOLS attribute, even though the input file didn't have that set.

This patch checks the files for the emitted atoms and only if they all have that attribute set, we emit the output file with that set.

Thanks,
Pete

[pete]

Rebased to ToT.

I also moved where we cached the file flags. The flags still live in File.h, but now we set them in normalizedObjectToAtoms which is consistent with some of the other file properties.

[lhames]

LGTM. Thanks Pete!

lld/trunk/lib/ReaderWriter/MachO/File.h

public:
   void setArch(MachOLinkingContext::Arch arch) { _arch = arch; }
 
   MachOLinkingContext::OS OS() const { return _os; }
   void setOS(MachOLinkingContext::OS os) { _os = os; }
 
   uint32_t swiftVersion() const { return _swiftVersion; }
   void setSwiftVersion(uint32_t v) { _swiftVersion = v; }
 
+  bool subsectionsViaSymbols() const {
+    return _flags & llvm::MachO::MH_SUBSECTIONS_VIA_SYMBOLS;
+  }
+  void setFlags(normalized::FileFlags v) { _flags = v; }
+
   /// Methods for support type inquiry through isa, cast, and dyn_cast:
   static inline bool classof(const File *F) {
     return F->kind() == File::kindMachObject;
   }
 
 protected:
   std::error_code doParse() override {
     // Convert binary file to normalized mach-o.
private:
 
   std::unique_ptr<MemoryBuffer> _mb;
   MachOLinkingContext          *_ctx;
   SectionToAtoms                _sectionAtoms;
   NameToAtom                     _undefAtoms;
   MachOLinkingContext::Arch      _arch = MachOLinkingContext::arch_unknown;
   MachOLinkingContext::OS        _os = MachOLinkingContext::OS::unknown;
   uint32_t                       _swiftVersion = 0;
+  normalized::FileFlags          _flags;
 };
 
 class MachODylibFile : public SharedLibraryFile {
 public:
   MachODylibFile(std::unique_ptr<MemoryBuffer> mb, MachOLinkingContext *ctx)
       : SharedLibraryFile(mb->getBufferIdentifier()),
         _mb(std::move(mb)), _ctx(ctx) {}
 

lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp

 SegmentInfo::SegmentInfo(StringRef n)
  : name(n), address(0), size(0), access(0), normalizedSegmentIndex(0) {
 }
 
 class Util {
 public:
   Util(const MachOLinkingContext &ctxt)
       : _ctx(ctxt), _archHandler(ctxt.archHandler()), _entryAtom(nullptr),
-        _hasTLVDescriptors(false) {}
+        _hasTLVDescriptors(false), _subsectionsViaSymbols(true) {}
   ~Util();
 
-  void      assignAtomsToSections(const lld::File &atomFile);
+  void      processDefinedAtoms(const lld::File &atomFile);
+  void      processAtomAttributes(const DefinedAtom *atom);
+  void      assignAtomToSection(const DefinedAtom *atom);
   void      organizeSections();
   void      assignAddressesToSections(const NormalizedFile &file);
   uint32_t  fileFlags();
   void      copySegmentInfo(NormalizedFile &file);
   void      copySectionInfo(NormalizedFile &file);
   void      updateSectionInfo(NormalizedFile &file);
   void      buildAtomToAddressMap();
   std::error_code addSymbols(const lld::File &atomFile, NormalizedFile &file);
private:
   TypeToSection                 _sectionMap;
   std::vector<SectionInfo*>     _customSections;
   AtomToAddress                 _atomToAddress;
   DylibPathToInfo               _dylibInfo;
   const DefinedAtom            *_entryAtom;
   AtomToIndex                   _atomToSymbolIndex;
   std::vector<const Atom *>     _machHeaderAliasAtoms;
   bool                          _hasTLVDescriptors;
+  bool                          _subsectionsViaSymbols;
 };
 
 Util::~Util() {
   // The SectionInfo structs are BumpPtr allocated, but atomsAndOffsets needs
   // to be deleted.
   for (SectionInfo *si : _sectionInfos) {
     // clear() destroys vector elements, but does not deallocate.
     // Instead use swap() to deallocate vector buffer.
if (align > sect->alignment)
     sect->alignment = atomAlign.value;
   // Assign atom to this section with this offset.
   AtomInfo ai = {atom, offset};
   sect->atomsAndOffsets.push_back(ai);
   // Update section size to include this atom.
   sect->size = offset + atom->size();
 }
 
-void Util::assignAtomsToSections(const lld::File &atomFile) {
+void Util::processDefinedAtoms(const lld::File &atomFile) {
   for (const DefinedAtom *atom : atomFile.defined()) {
-    if (atom->contentType() == DefinedAtom::typeMachHeader)
-      _machHeaderAliasAtoms.push_back(atom);
-    else
-      appendAtom(sectionForAtom(atom), atom);
-  }
-}
+    processAtomAttributes(atom);
+    assignAtomToSection(atom);
+  }
+}
+
+void Util::processAtomAttributes(const DefinedAtom *atom) {
+  auto *machoFile = static_cast<const mach_o::MachOFile *>(&atom->file());
+  // If the file doesn't use subsections via symbols, then make sure we don't
+  // add that flag to the final output file if we have a relocatable file.
+  if (!machoFile->subsectionsViaSymbols())
+    _subsectionsViaSymbols = false;
+}
+
+void Util::assignAtomToSection(const DefinedAtom *atom) {
 
 SegmentInfo *Util::segmentForName(StringRef segName) {
   for (SegmentInfo *si : _segmentInfos) {
     if ( si->name.equals(segName) )
       return si;
   }
   auto *info = new (_allocator) SegmentInfo(segName);
   if (segName.equals("__TEXT"))
for (const AtomInfo &info : sect->atomsAndOffsets) {
       nFile.exportInfo.push_back(exprt);
     }
   }
 }
 
 uint32_t Util::fileFlags() {
   // FIXME: these need to determined at runtime.
   if (_ctx.outputMachOType() == MH_OBJECT) {
-    return MH_SUBSECTIONS_VIA_SYMBOLS;
+    return _subsectionsViaSymbols ? MH_SUBSECTIONS_VIA_SYMBOLS : 0;
   } else {
     uint32_t flags = MH_DYLDLINK;
     if (!_ctx.useFlatNamespace())
         flags |= MH_TWOLEVEL | MH_NOUNDEFS;
     if ((_ctx.outputMachOType() == MH_EXECUTE) && _ctx.PIE())
       flags |= MH_PIE;
     if (_hasTLVDescriptors)
       flags |= (MH_PIE | MH_HAS_TLV_DESCRIPTORS);
     return flags;
   }
 }
 
 } // end anonymous namespace
 
 namespace lld {
 namespace mach_o {
 namespace normalized {
 
 /// Convert a set of Atoms into a normalized mach-o file.
 ErrorOr<std::unique_ptr<NormalizedFile>>
 normalizedFromAtoms(const lld::File &atomFile,
                                            const MachOLinkingContext &context) {
   // The util object buffers info until the normalized file can be made.
   Util util(context);
-  util.assignAtomsToSections(atomFile);
+  util.processDefinedAtoms(atomFile);
   util.organizeSections();
 
   std::unique_ptr<NormalizedFile> f(new NormalizedFile());
   NormalizedFile &normFile = *f.get();
   normFile.arch = context.arch();
   normFile.fileType = context.outputMachOType();
   normFile.flags = util.fileFlags();
   normFile.stackSize = context.stackSize();

lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp

for (const DataInCode &entry : normalizedFile.dataInCode) {
 
     // Add reference that marks end of data-in-code.
     atom->addReference(offsetInAtom+entry.length,
                        handler->dataInCodeTransitionEnd(*atom), atom, 0,
                        handler->kindArch());
   }
 
   // Cache some attributes on the file for use later.
+  file->setFlags(normalizedFile.flags);
   file->setArch(normalizedFile.arch);
   file->setOS(normalizedFile.os);
 
   // Sort references in each atom to their canonical order.
   for (const DefinedAtom* defAtom : file->defined()) {
     reinterpret_cast<const SimpleDefinedAtom*>(defAtom)->sortReferences();
   }
   return std::error_code();

lld/trunk/test/mach-o/parse-data-relocs-x86_64.yaml

 # CHECK:  - name:            _bar
 # CHECK:    content:         [ C3 ]
 # CHECK:    dead-strip:      never
 # CHECK:  - ref-name:        L003
 # CHECK:    type:            unknown
 # CHECK:    content:         [ 00, 00, 00, 00, 00, 00, 00, 00 ]
 # CHECK:    section-choice:  custom-required
 # CHECK:    section-name:    __DATA/__custom
+# CHECK:    dead-strip:      never