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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

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

It depends on the endianness of the system we're writing to so it the BinaryWriter should throw an error if you try to write an allocated one. Later (in a seperate patch) we'll need to support large indexes for dynamic symbols which I plan on handling the general way we've handled dynamic stuff (by just copying). At that point the error will become impossible until we have section editing.

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

That was meant to be "Check error for case when symbol has SHN_XINDEX index but no symbol index table". There's an error in the code but it isn't checked that its actually thrown in that case at the moment.

> 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.

I think they should be allowed to strip it but if it winds up being needed we should add it in (which is what we do now). So there shouldn't be an error, it should just wind up having a SYMTAB_SHNDX table despite the fact that they stripped it. We should have a test for this case to make sure it's acceptable. We should also have a case for the edge case you mention.



================
Comment at: test/tools/llvm-objcopy/many-sections.S:47
+
+// SECS: 65542
+// SYMS: 65538
----------------
jhenderson wrote:
> Could this be SECS: Index: 65542?
> 
> Similar for symbols.
For sections I can do this but for symbols I can't. Also I'm glad you had me do this test. Somewhere in the output of llvm-readobj there is an extra occurence of "Name" than there should be. The largest section index is 65540 which means that there should be 65541 occurrences of "Name:" which means this test had an issue with it. It turns out that at the start of every llvm-readobj output there is a field "LoadName:" which triggers the count on this. So instead I'll use the Index number for sections and "Symbol {" count for symbols.


================
Comment at: tools/llvm-objcopy/Object.cpp:1115
+  bool NeedsLargeIndexes = false;
+  for (auto &Sec : Obj.sections()) {
+    Sec.Index = Index++;
----------------
jhenderson wrote:
> 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?
You have this right. It can also exit early if it finds one because we'll have to reassign indexes later anyway. Good catch.


Repository:
  rL LLVM

https://reviews.llvm.org/D42516