[PATCH] D45851: [llvm-objcopy] Fix sh_link for more types of sections

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Overall, I like the idea here, but there are some things that need fixing.



================
Comment at: tools/llvm-objcopy/Object.cpp:371
+bool Section::classof(const SectionBase *S) {
   return isa<DynamicSymbolTableSection>(S) || isa<DynamicSection>(S);
 }
----------------
This is wrong. There are plenty of other kinds of sections that use Section at the moment, e.g. SHT_HASH and dynamic string tables. I haven't looked to see if this will actually have a runtime effect however.


================
Comment at: tools/llvm-objcopy/Object.cpp:376-381
+  case ELF::SHN_UNDEF:
+  case ELF::SHN_LORESERVE:
+  case ELF::SHN_HIPROC:
+  case ELF::SHN_ABS:
+  case ELF::SHN_COMMON:
+  case ELF::SHN_HIRESERVE:
----------------
I don't think this statement should be here (possibly except for the SHN_UNDEF case) - the sh_link field is not a case of where the section index is confined (as it is 32-bits, rather than 16-bits), and as such, will just represent a raw section index. That index could represent the literal 0xffffffff'th index, for example.

>From the SCO page:
> Some section header table indexes are reserved in contexts where index size is restricted, for example, the st_shndx member of a symbol table entry and the e_shnum and e_shstrndx members of the ELF header. In such contexts, the reserved values do not represent actual sections in the object file. Also in such contexts, an escape value indicates that the actual section index is to be found elsewhere, in a larger field. 
There is no special case for sh_link here.



================
Comment at: tools/llvm-objcopy/Object.h:464
 
-  void initialize(SectionTableRef SecTable) override {};
+  void initialize(SectionTableRef SecTable) override{};
   void accept(SectionVisitor &) const override;
----------------
Nit: Is the missing space here definitely what clang-format does? It seems a little weird...


Repository:
  rL LLVM

https://reviews.llvm.org/D45851