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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: test/tools/llvm-objcopy/test-groups.test:2
+# RUN: cp %p/Inputs/groups.o %t
+# RUN: llvm-objcopy -strip-dwo %t
+# RUN: llvm-readobj -section-data -sections %t | FileCheck %s
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > Why -strip-dwo? What do you expect to happen here.
> nit: Whatever this tests it can be tested by using -R directly. I'd prefer if -R was used since it's the most direct way of removing something. Other forms of stripping have subtle edge cases.
ok, i'm fine to add other tests (+ maybe remove this one), 
however it's pretty common in LLVM/Clang to add tests for the newly-discovered bugs to make sure it won't break in the future, that's why i thought it would be good to have a test with -strip-dwo which doesn't pass without this patch, but does pass with it.


================
Comment at: tools/llvm-objcopy/Object.h:261
 
-private:
+protected:
   ArrayRef<uint8_t> Contents;
----------------
jakehehrlich wrote:
> I don't think this should be protected. The point of Section it just writes it's data out and nothing else happens. It has no classof because no extra features of it are ever exposed. If you need access to the underlying data then you shouldn't inherit from this, you should be rolling your own section. It should really be called "PlainDataSection" or "RawDataSection" or something like that. I named it terribly.
yeah, let's leave it private and, probably, rename this class because it's really confusing now.
if i understand the intent behind it correctly it's more like an ImmutableSection whose content is not supposed to be modified. 


================
Comment at: tools/llvm-objcopy/Object.h:437
 
-class SectionWithStrTab : public Section {
-private:
-  const SectionBase *StrTab = nullptr;
+class SectionWithSymTab : public Section {
+  const SectionBase *SymTab = nullptr;
----------------
jakehehrlich wrote:
> I only see this used one place and this is the last major section type we might ever need to add. I think this should just all be rolled inside of a GroupSection.
if i'm not mistaken that's not correct: a bunch of other cases are not handled correctly now:
SHT_HASH, SHT_SUNW_move, SHT_SUNW_syminfo, SHT_SUNW_versym, maybe smth else.
So taking it into account i would prefer to leave it here for now. 


Repository:
  rL LLVM

https://reviews.llvm.org/D43996