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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich 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
----------------
alexshap wrote:
> 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.
That's fine. I just want a more specific raw test so that when I break this test later I know from reading the test what should be happening. Both is even better!


================
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;
----------------
alexshap wrote:
> 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. 
Nice catch! I'll make sure those section types are added once this lands. I should have caught SHT_HASH a long time ago. Right now it dosn't maintain the link. 


Repository:
  rL LLVM

https://reviews.llvm.org/D43996