[PATCH] D43996: [llvm-objcopy] Implement support for .group sections.

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:396
+  FlagWord = *Word++;
+  for (; Word != End; ++Word) {
+    GroupMembers.push_back(
----------------
alexshap wrote:
> jakehehrlich wrote:
> > I'm not in love with the existence of initlizeSymbolTable and initlizeRelocations but I think you'll have to do something similar here. Having owned data isn't really what we want. This might be an indication that we need to also pass section data to the initialize method.
> right,  Section is kind of breaking down the hierarchy (partially because of the confusing name) now.
@jakehehrlich , so I've looked at the code (without any of my changes),
to be honest at the moment I somehow don't like the suggestion and the way how the current abstractions are organized (i can update the patch following your suggestion but i think it would be better to start changing this code in a different direction, so probably it would be good to discuss the design first /come to an agreement -  probably this will significantly simplify the future work).

So let me try to explain my concerns ((1) and (2)) with it.

(1) "Leaky" abstractions. 
At the moment the responsibility of initialization is getting fragmented between the methods "initialize", initSymbolTable, initRelocations:
   if (Obj.SymbolTable) {
        Obj.SymbolTable->initialize(Obj.sections()); 
        initSymbolTable(Obj.SymbolTable);
    }

After SymbolTable->initialize(Obj.sections()); the object SymbolTable is still half-baked (which is weird on its own) and creates stronger coupling between these classes than it could be.
There is a similar issue here: 
    if (auto RelSec = dyn_cast<RelocationSection>(&Section)) {
        ....
        initRelocations(....)
    }

The abstractions of RelocationSection SymbolTableSection (and probably a few other classes) are "leaky".

(2) "Duplication". The field Contents is naturally required for setting up a more-or-less complex section, now several "leafs" of the hierarchy have it (Section, DynamicRelocationSection, + descendants of SectionWithStrTab) + duplication of setSymTab etc.

Proposed approach:
We probably need a more robust base class Section. Its behavior will be to pass-through the content (like what it is right now) + probably maintain the integrity of the references (link, sh_info etc) if any are present. The subclasses of Section can generate the new content (based on the old one (thus they should have (probably a read-only) access to it)) and will be responsible for maintaining the necessary state (specific to their needs). More robust initialize(...) method (with, probably, a different signature than it is right now) would enable the "complex" sections (like symbol table, relocations etc) to be responsible for its own initialization (thus avoid accumulating the complexity in ELFBuilder<ELFT>::readSectionHeaders). 
What are your thoughts on this direction ?


Repository:
  rL LLVM

https://reviews.llvm.org/D43996