Add DefinedAtom::sectionSize().

[ruiu]

Summary

Merge::mergeByLargestSection is defined in terms of section size,
but there was no way to get the section size for an atom. Previously
we followed layout-before/layout-after chains to collect all atoms
for a section, but that's not the right way to do it because there's
no guarantee that layout-before/layout-after exist. Even if they
exist, it's not guaranteed that they represent a section.

So this patch adds sectionSize() member function to DefinedAtom.

[• shankar.easwaran]

Iis this because of associated sections ? That you cannot get the section size ?

Can you please let us know why you can't get the section size from the layout after references / layout before references.

I am not in agreement for this as there is not a use of this on MachO or ELF.

[ruiu]

It's not related to associated sections.

As I wrote in the parch description, layout-before and layout-after are
mechanisms to layout atoms in some order, and it's not always guaranteed to
be the same as sections. The current implementation that to be removed by
this parch is a hack that depends on an incorrect assumption that
layout-after/layout-before can be used to walk all atoms in the same
section.
2014/06/06 22:47 "Shankar Easwaran" <[e-mail]>:

Iis this because of associated sections ? That you cannot get the section
size ?

Can you please let us know why you can't get the section size from the
layout after references / layout before references.

I am not in agreement for this as there is not a use of this on MachO or
ELF.

http://reviews.llvm.org/D4042

[ruiu]

Ping.

[atanasyan]

Is it possible to keep the sectionSize() method in the COFFDefinedAtom class only? In general every defined atom has non-zero section size. But this method returns a sensible result for a very specific set of atoms.

[shankarke]

We wont be able to keep the sectionSize in COFFDefinedAtom because the Native/YAML only understand DefinedAtoms. I was proposing a way to do this earlier by having atoms contain a set of key value pairs (key=>value) that can be used to set various values.

We could start a discussion on the thread, and arrive at a way to solve this if you think we should do that.

[atanasyan]

I like this idea because now I try to figure out how to propagate the --as-needed from the ELFFileNode::parse() to the point where we create the DT_NEEDED dynamic tag. Your idea might be a solution to this problem too.

[ruiu]

Adding generic key-value pair container to DefinedAtom class will probably end up with too limited or too generic code. If we limit the type of value to int64 (or some fixed-type size), it'd be pretty easy to dump it to YAML/Native, but it's restricted so it won't solve the problems we have. On the other hand, if we allow arbitrary type, it's hard to handle and needs custom dumper for Native anyway.

The thing I'd like to focus here is not the above one, but this. Merge::mergeByLargestSection is already there, and its definition is "pick the atom according to its source section size". However it currently lacks the way to get the section size for an atom. It's just wrong. Merge::mergeByLargestSection should go away (which we don't want) or we need a way to get a section size for an atom.

[atanasyan]

In general the patch is LGTM. The only thing concerns me is that we start to populate 'Core' classes by functions which really used by the single target.

[ruiu]

I basically agree, we have to be careful not to bloat Core with lots of target specific stuff. However something that is needed for linking stage itself needs to be in Core, and we already have quite a lot for ELF. I'd think this change is small and acceptable.

[shankarke]

I am not sure which instance in the Core Atom model that you are pointing at. The core atom model models generically for all flavors, I dont see something in the Atom model specifically designed for ELF.

That said, please check with Nick before you would like to commit this change.

[kledzik]

It is not ideal to add DefinedAtom::sectionSize() to every platform, when this is only needed in rare cases on one platform.

A more pay-to-play approach would be to require mergeByLargestSection atoms to have a (group?) reference to a new atom which has a special ContentType and is a placeholder for the section, and its size() method returns the size of the section. The resolver, when finding a mergeByLargestSection atom looks for the Reference to the magic "section" atom and gets its size for comparison. The PE/COFF Writer would then also need to ignore atoms of the special contentType atoms.

[ruiu]

DefinedAtom::sectionSize() is basically free if your atom does not have mergeByLargestSection attribute. It's a virtual function so it'll use one more slot for vftable, but that's it. There's no extra cost for each atomwhich does not use mergeByLargestSection. Making a virtual group atom only for size would work, but it's too complicated.

[kledzik]

Yes, adding a virtual method is "free" in that it does not increase the size of each object, but this patch alone does not solve the round-trip (yaml/native) that Shankar brought up. My suggestion to add an atom representing the section and a reference from the mergeByLargestSection atom to the section atom, does work with round tripping.

The extra section atom and reference are not just arbitrary. They actually correspond to this merge mode. The atom is saying "pick me if my section is biggest". The reference answers the question "what is your section?" and the section atom answers the question "what is your section's size".

But there may also be another approach to this. Rather than have the SymbolTable understand all the merge modes, maybe we can have it call out to the LinkingContext to pick between custom modes? Or have it support mergeByLargestSection by calling out to the LinkingContext for the section size of a particular atom?

[shankarke]

I would think adding a reference to another atom, and querying the atom for the size could be the approach we may want to take here.

The LinkingContext approach may work but it doesnot fit in the atom model properly IMHO.

include/lld/Core/DefinedAtom.h

public:
 
   /// \brief If sectionChoice() != sectionBasedOnContent, then this return the
   /// name of the section the atom should be placed into.
   virtual StringRef customSectionName() const = 0;
 
   /// \brief constraints on whether the linker may dead strip away this atom.
   virtual SectionPosition sectionPosition() const = 0;
 
+  /// \brief If merge() == mergeByLargestSection, this returns the
+  /// size of the section for this atom.
+  virtual uint64_t sectionSize() const { return 0; }
+
   /// \brief constraints on whether the linker may dead strip away this atom.
   virtual DeadStripKind deadStrip() const = 0;
 
   /// \brief Under which conditions should this atom be dynamically exported.
   virtual DynamicExport dynamicExport() const {
     return dynamicExportNormal;
   }
 

lib/Core/SymbolTable.cpp

 
 static MergeResolution mergeSelect(DefinedAtom::Merge first,
                                    DefinedAtom::Merge second) {
   assert(first != DefinedAtom::mergeByContent);
   assert(second != DefinedAtom::mergeByContent);
   return mergeCases[first][second];
 }
 
-static const DefinedAtom *followReference(const DefinedAtom *atom,
-                                          uint32_t kind) {
-  for (const Reference *r : *atom)
-    if (r->kindNamespace() == Reference::KindNamespace::all &&
-        r->kindArch() == Reference::KindArch::all &&
-        r->kindValue() == kind)
-      return cast<const DefinedAtom>(r->target());
-  return nullptr;
-}
-
-static uint64_t getSizeFollowReferences(const DefinedAtom *atom,
-                                        uint32_t kind) {
-  uint64_t size = 0;
-  for (;;) {
-    atom = followReference(atom, kind);
-    if (!atom)
-      return size;
-    size += atom->size();
-  }
-}
-
-// Returns the size of the section containing the given atom. Atoms in the same
-// section are connected by layout-before and layout-after edges, so this
-// function traverses them to get the total size of atoms in the same section.
-static uint64_t sectionSize(const DefinedAtom *atom) {
-  return atom->size()
-      + getSizeFollowReferences(atom, lld::Reference::kindLayoutBefore)
-      + getSizeFollowReferences(atom, lld::Reference::kindLayoutAfter);
-}
-
 bool SymbolTable::addByName(const Atom &newAtom) {
   StringRef name = newAtom.name();
   assert(!name.empty());
   const Atom *existing = findByName(name);
   if (existing == nullptr) {
     // Name is not in symbol table yet, add it associate with this atom.
     _nameTable[name] = &newAtom;
     return true;
switch (mergeSelect(((DefinedAtom*)existing)->merge(),
                         ((DefinedAtom*)&newAtom)->merge())) {
     case MCR_First:
       useNew = false;
       break;
     case MCR_Second:
       useNew = true;
       break;
     case MCR_Largest: {
-      uint64_t existingSize = sectionSize((DefinedAtom*)existing);
-      uint64_t newSize = sectionSize((DefinedAtom*)&newAtom);
+      uint64_t existingSize = ((DefinedAtom*)existing)->sectionSize();
+      uint64_t newSize = ((DefinedAtom*)&newAtom)->sectionSize();
       useNew = (newSize >= existingSize);
       break;
     }
     case MCR_SameSize: {
-      uint64_t existingSize = sectionSize((DefinedAtom*)existing);
-      uint64_t newSize = sectionSize((DefinedAtom*)&newAtom);
+      uint64_t existingSize = ((DefinedAtom*)existing)->sectionSize();
+      uint64_t newSize = ((DefinedAtom*)&newAtom)->sectionSize();
       if (existingSize == newSize) {
         useNew = true;
         break;
       }
       llvm::errs() << "Size mismatch: "
                    << existing->name() << " (" << existingSize << ") "
                    << newAtom.name() << " (" << newSize << ")\n";
       // fallthrough

lib/ReaderWriter/PECOFF/Atoms.h

private:
   std::vector<std::unique_ptr<COFFReference> > _references;
 };
 
 // A COFFDefinedAtom represents an atom read from a file and has contents.
 class COFFDefinedAtom : public COFFDefinedFileAtom {
 public:
   COFFDefinedAtom(const File &file, StringRef name, StringRef sectionName,
                   Scope scope, ContentType type, bool isComdat,
-                  ContentPermissions perms, Merge merge, ArrayRef<uint8_t> data,
-                  uint64_t ordinal)
+                  ContentPermissions perms, Merge merge, uint64_t sectionSize,
+                  ArrayRef<uint8_t> data, uint64_t ordinal)
       : COFFDefinedFileAtom(file, name, sectionName, scope, type, perms,
                             ordinal),
-        _isComdat(isComdat), _merge(merge), _dataref(data) {}
+    _isComdat(isComdat), _merge(merge), _sectionSize(sectionSize),
+    _dataref(data) {}
 
   Merge merge() const override { return _merge; }
   uint64_t size() const override { return _dataref.size(); }
   ArrayRef<uint8_t> rawContent() const override { return _dataref; }
 
+  uint64_t sectionSize() const override { return _sectionSize; }
+
   DeadStripKind deadStrip() const override {
     // Only COMDAT symbols would be dead-stripped.
     return _isComdat ? deadStripNormal : deadStripNever;
   }
 
 private:
   bool _isComdat;
   Merge _merge;
+  uint64_t _sectionSize;
   ArrayRef<uint8_t> _dataref;
 };
 
 // A COFFDefinedAtom represents an atom for BSS section.
 class COFFBSSAtom : public COFFDefinedFileAtom {
 public:
   COFFBSSAtom(const File &file, StringRef name, Scope scope,
               ContentPermissions perms, Merge merge, uint32_t size,

lib/ReaderWriter/PECOFF/ReaderCOFF.cpp

error_code FileCOFF::AtomizeDefinedSymbolsInSection(
   DefinedAtom::ContentPermissions perms = getPermissions(section);
   bool isComdat = (_comdatSections.count(section) == 1);
 
   // Create an atom for the entire section.
   if (symbols.empty()) {
     ArrayRef<uint8_t> data(secData.data(), secData.size());
     auto *atom = new (_alloc) COFFDefinedAtom(
         *this, "", sectionName, Atom::scopeTranslationUnit, type, isComdat,
-        perms, _merge[section], data, _ordinal++);
+        perms, _merge[section], section->SizeOfRawData, data, _ordinal++);
     atoms.push_back(atom);
     _definedAtomLocations[section][0].push_back(atom);
     return error_code();
   }
 
   // Create an unnamed atom if the first atom isn't at the start of the
   // section.
   if (symbols[0]->Value != 0) {
     uint64_t size = symbols[0]->Value;
     ArrayRef<uint8_t> data(secData.data(), size);
     auto *atom = new (_alloc) COFFDefinedAtom(
         *this, "", sectionName, Atom::scopeTranslationUnit, type, isComdat,
-        perms, _merge[section], data, _ordinal++);
+        perms, _merge[section], section->SizeOfRawData, data, _ordinal++);
     atoms.push_back(atom);
     _definedAtomLocations[section][0].push_back(atom);
   }
 
   for (auto si = symbols.begin(), se = symbols.end(); si != se; ++si) {
     const uint8_t *start = secData.data() + (*si)->Value;
     // if this is the last symbol, take up the remaining data.
     const uint8_t *end = (si + 1 == se) ? secData.data() + secData.size()
                                         : secData.data() + (*(si + 1))->Value;
     ArrayRef<uint8_t> data(start, end);
     auto *atom = new (_alloc) COFFDefinedAtom(
         *this, _symbolName[*si], sectionName, getScope(*si), type, isComdat,
-        perms, _merge[section], data, _ordinal++);
+        perms, _merge[section], section->SizeOfRawData, data, _ordinal++);
     atoms.push_back(atom);
     _symbolAtom[*si] = atom;
     _definedAtomLocations[section][(*si)->Value].push_back(atom);
   }
 
   // Finally, set alignment to the first atom so that the section contents
   // will be aligned as specified by the object section header.
   _definedAtomLocations[section][0][0]->setAlignment(getAlignment(section));
error_code FileCOFF::maybeCreateSXDataAtoms() {
   if (sxdata.empty())
     return error_code();
 
   std::vector<uint8_t> atomContent =
       *new (_alloc) std::vector<uint8_t>((size_t)sxdata.size());
   auto *atom = new (_alloc) COFFDefinedAtom(
       *this, "", ".sxdata", Atom::scopeTranslationUnit, DefinedAtom::typeData,
       false /*isComdat*/, DefinedAtom::permR__, DefinedAtom::mergeNo,
-      atomContent, _ordinal++);
+      0 /* sectionSize */, atomContent, _ordinal++);
 
   const ulittle32_t *symbolIndex =
       reinterpret_cast<const ulittle32_t *>(sxdata.data());
   int numSymbols = sxdata.size() / sizeof(uint32_t);
 
   for (int i = 0; i < numSymbols; ++i) {
     Atom *handlerFunc;
     if (error_code ec = getAtomBySymbolIndex(symbolIndex[i], handlerFunc))