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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:854-859
+  // """
+  // 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.
+  // """
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > I feel like having these quotes duplicated is not great. I wonder whether it's a sign that the null shdr should be constructed in the writeEhdr function, so that the decisions only need making the once? Thoughts?
> I'm a bit conflicted on this one. I like having the logic for writing the whole section header table in one place because it makes the logic of "do we write the section header table out or not" more succinct. I also however agree with your claim that quoting this twice is annoying.
> 
> I can think of two options here:
> 1) remove the comments in the second case and just leave a URL
> 2) move this to writeEhdr just like you said
I think I'd prefer a third option actually: remove these comments, and replace with a reference to the standard quote in writeEhdr(), i.e. "// See writeEhdr for why we do this" or similar.


================
Comment at: tools/llvm-objcopy/Object.cpp:705
+  // If we have a SectionIndexTable we need to initialize it before the symbol
+  // table because the symbol table will need it to properlly read in symbols.
+  if (Obj.SectionIndexTable)
----------------
properlly -> properly


================
Comment at: tools/llvm-objcopy/Object.cpp:1085
+  // We need to assign indexes before we perform layout because we need to know
+  // if we need Shndx. While we do that we can determine if we need shndx
+  uint64_t Index = 1;
----------------
This line doesn't read right, and I think you should avoid using "S/shndx" here. Refer to it as the symbol section index table or similar. It also looks like the last sentence is incomplete.


================
Comment at: tools/llvm-objcopy/Object.cpp:1090
+    Sec.Index = Index++;
+    if (Sec.Index > SHN_LORESERVE && Sec.HasSymbol)
+      NeedsShndx = true;
----------------
">=" here?


================
Comment at: tools/llvm-objcopy/Object.cpp:1112
+  // Make sure we add the names of all the sections. Importantly this must be
+  // done after we decide to add or remove SectionIndxes
   if (Obj.SectionNames != nullptr)
----------------
Typo in "SectionIndxes" and missing full-stop.


================
Comment at: tools/llvm-objcopy/Object.cpp:1118-1119
+
+  // Some sections don't yet have their final size due to symbols not keeping
+  // other sections updated.
   if (Obj.SymbolTable != nullptr)
----------------
It's not clear to me what this comment is saying. Why would symbols keep sections updated or not as the case may be?


Repository:
  rL LLVM

https://reviews.llvm.org/D42516