[PATCH] D42516: [llvm-objcopy] Add support for large indexes

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In https://reviews.llvm.org/D42516#1014046, @jakehehrlich wrote:

> 3. Check error for binary output


What error would you expect in this case? Why wouldn't it just work?

> 6. Check error for case when symbol has SHN_XINDEX index but not

I think you messed up the end of this sentence :)

How about tests for attempting to strip the SYMTAB_SHNDX table? I.e. We should refuse to do so, if there are too many sections. An interesting edge case is adding sections such that we now require the table, but also explicitly stripping the symtab_shndx table if it exists. I'm not clear on the behaviour here, but I guess we should throw away the old one and build one from scratch in this case.

Can't think of any other cases off the top of my head.



================
Comment at: test/tools/llvm-objcopy/many-sections.S:1
+// RUN: llvm-mc -filetype=obj -triple x86_64-linux %s -o %t
+// RUN: llvm-objcopy %t %t2
----------------
Lower-case .s is more common for test files.


================
Comment at: test/tools/llvm-objcopy/many-sections.S:47
+
+// SECS: 65542
+// SYMS: 65538
----------------
Could this be SECS: Index: 65542?

Similar for symbols.


================
Comment at: tools/llvm-objcopy/Object.cpp:661
                                      Twine TypeErrMsg) {
-  if (T *Sec = dyn_cast<T>(getSection(Index, IndexErrMsg)))
+  if (T *Sec = dyn_cast<T>(getSection(Index, IndexErrMsg))) {
     return Sec;
----------------
Unnecessary braces.


================
Comment at: tools/llvm-objcopy/Object.cpp:896-901
+  // """
+  // If the number of sections is greater than or equal to
+  // SHN_LORESERVE (0xff00), this member has the value zero and the actual
+  // number of section header table entries is contained in the sh_size field
+  // of the section header at index 0.
+  // """
----------------
Unfortunately, I think you've gone the wrong way round here! The quote refers to "this member" which only makes sense if it's talking about the Elf Header, so this and the other quote belong in writeEhdr.


================
Comment at: tools/llvm-objcopy/Object.cpp:1115
+  bool NeedsLargeIndexes = false;
+  for (auto &Sec : Obj.sections()) {
+    Sec.Index = Index++;
----------------
Could this loop be improved to not loop over every section? You only need to loop over sections from Index >= SHN_LORESERVE upwards, don't you? Or is this vector not ordered by section index?


================
Comment at: tools/llvm-objcopy/Object.cpp:1118
+    if (Sec.Index >= SHN_LORESERVE && Sec.HasSymbol)
+      NeedsLargeIndexes = true;
+  }
----------------
break here after this is set.


================
Comment at: tools/llvm-objcopy/Object.cpp:1124
+  if (NeedsLargeIndexes) {
+    // This means we definitly need to have a section index table but if
+    // we already have one then we should use it instead of making a new
----------------
definitly -> definitely


================
Comment at: tools/llvm-objcopy/Object.h:378
 public:
+  void setShndx(SectionIndexSection *Shndx) { SectionIndexes = Shndx; }
   void setStrTab(StringTableSection *StrTab) { SymbolNames = StrTab; }
----------------
jhenderson wrote:
> I'd rename this function "setShndxTable" since setShndx implies you're setting the section header index of the section.
Ping on this comment?


================
Comment at: tools/llvm-objcopy/Object.h:369
+  SectionIndexSection() {
+    Name = ".symtab_shndxr";
+    OriginalOffset = std::numeric_limits<uint64_t>::max();
----------------
The ELF gABI suggests that the name ".symtab_shndx" is used for the SHT_SYMTAB_SHNDX section.

An old commit from 2014 seems to have changed the name emitted by MC from this to ".symtab_shndxr". I'm not sure why the name changed there. Searching for that string online only yields that commit and references to the LLVM code-base, so it doesn't look like there's prior art for that name. Independently of this change, I think finding out why the name was changed (and change it back if appropriate) would be a sensible exercise.


================
Comment at: tools/llvm-objcopy/Object.h:394
+  void prepareForLayout();
+  const SectionBase *getShndx() const { return SectionIndexTable; }
   const SectionBase *getStrTab() const { return SymbolNames; }
----------------
getShndx() -> getShndxTable(), since getShndx() implies getting the section index for this section, which it doesn't (see also setShndx).


Repository:
  rL LLVM

https://reviews.llvm.org/D42516