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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:396
+  FlagWord = *Word++;
+  for (; Word != End; ++Word) {
+    GroupMembers.push_back(
----------------
alexshap wrote:
> 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 ?
I 100% agree with leaky abstraction complaint. That should be fixed somehow. There is also a bit of duplication for section contents but I think it only occurs with DynamicRelocationSection and Section right now. I also dislike how the RelocationSection hierarchy works.

Out of curiosity what's the issue with just adding a data argument to initialize(...)? I believe it would have the following benefits. If that's what you're proposing here then I 100% like that idea.

1) Many currently public members could be made private like addSymbol (which is a gross method anyway)
2) The initlize code could be made uniform
3) Most constructors could become default constructors instead of taking a data element which removes a lot of code
4) The logic for reading in a section would still be in the class

This doesn't solve part of the leaky abstraction part of this. Namely the symbol table would still need to be initialized first. For a long time I've been thinking about adding a sort of dependency system that requires certain sections to be initialized/finalized before other sections so that. There are a bunch of other finalize dependency issues that I haven't taken the proper time to solve. For instance a few finalize methods call the finalize method of other sections! That actually causes duplicate calls to finalize! It just happens for string tables which deliberately support this efficiently but that was a kludge that I knew I was adding and I'd like to fix it. My running idea on how to solve this 

What do you mean by "maintain the integrity of the references (link, sh_info, etc...)? Those have different meaning for every section type. I have a vauge idea that I need to flesh out more that involves using mixins/polcies for setStrTab/setSymTab. Also after initialize is brought fully into the class I don't know that they even need to exist which would have the effect of. I'd like to see what improving the initialize method buys us before committing to changes to Section (short of a name change to Section which it desperately needs).


Repository:
  rL LLVM

https://reviews.llvm.org/D43996