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

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 28 02:11:38 PST 2018

jhenderson added a comment.

Two comment wrapping issues are still outstanding (I've pinged them).

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

> It's not an ideal test but to test that

I think you forgot to end a sentence :) Also I don't see any comment about number 6?

Comment at: test/tools/llvm-objcopy/many-sections.test:41
+SECS: Name: .symtab_shndx
+SECS-NEXT: Flags [ (0x0)

Comment at: test/tools/llvm-objcopy/many-sections.test:46
+SECS-NEXT: Offset:
+SECS-NEXT: Link: 65285
I think checking the size here would be wise. It should be easy to do, since you know how many symbols there are. A comment describing the calculation would be helpful too, i.e. something like "# Size == 4 * symbol count".

Comment at: tools/llvm-objcopy/Object.cpp:875
+    // If the section name string table section index is greater than or equal
+    // to
+    // SHN_LORESERVE (0xff00), this member has the value SHN_XINDEX (0xffff) and
jhenderson wrote:
> Weird wrapping again?

Comment at: tools/llvm-objcopy/Object.cpp:1131
+  if (NeedsLargeIndexes) {
+    // This means we definitely need to have a section index table but if
+    // we already have one then we should use it instead of making a new
jhenderson wrote:
> Have you wrapped slightly early here? I think we can get this comment down to 2 lines.



More information about the llvm-commits mailing list