[PATCH] D97661: [llvm-objcopy/strip] Fix off-by-one error in SYMTAB_SHNDX need check

[James Henderson via Phabricator via llvm-commits]

jhenderson updated this revision to Diff 327042.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97661/new/

https://reviews.llvm.org/D97661

Files:
  llvm/test/tools/llvm-objcopy/ELF/auto-remove-add-symtab-shndx.test
  llvm/test/tools/llvm-objcopy/ELF/auto-remove-shndx.test
  llvm/test/tools/llvm-objcopy/ELF/strict-no-add.test
  llvm/tools/llvm-objcopy/ELF/Object.cpp


Index: llvm/tools/llvm-objcopy/ELF/Object.cpp
===================================================================
--- llvm/tools/llvm-objcopy/ELF/Object.cpp
+++ llvm/tools/llvm-objcopy/ELF/Object.cpp
@@ -2450,8 +2450,10 @@
   bool NeedsLargeIndexes = false;
   if (Obj.sections().size() >= SHN_LORESERVE) {
     SectionTableRef Sections = Obj.sections();
+    // Sections doesn't include the null section header, so account for this
+    // when skipping the first N sections.
     NeedsLargeIndexes =
-        any_of(drop_begin(Sections, SHN_LORESERVE),
+        any_of(drop_begin(Sections, SHN_LORESERVE - 1),
                [](const SectionBase &Sec) { return Sec.HasSymbol; });
     // TODO: handle case where only one section needs the large index table but
     // only needs it because the large index table hasn't been removed yet.
Index: llvm/test/tools/llvm-objcopy/ELF/strict-no-add.test
===================================================================
--- llvm/test/tools/llvm-objcopy/ELF/strict-no-add.test
+++ /dev/null
@@ -1,10 +0,0 @@
-# This test makes sure that sections added at the end that don't have symbols
-# defined in them don't trigger the creation of a large index table.
-
-RUN: %python %p/../Inputs/ungzip.py %p/Inputs/many-sections.o.gz > %t.0
-RUN: echo 'foo' > %t
-RUN: llvm-objcopy -R 's65[3-9][0-9][0-9]' -R 's6529[0-9]' -R 's652[89][0-9]' %t.0 %t2
-RUN: llvm-objcopy --add-section=.s0=%t --add-section=.s1=%t --add-section=.s2=%t %t2 %t2
-RUN: llvm-readobj --sections %t2 | FileCheck --check-prefix=SECS %s
-
-SECS-NOT: Name: .symtab_shndx
Index: llvm/test/tools/llvm-objcopy/ELF/auto-remove-shndx.test
===================================================================
--- llvm/test/tools/llvm-objcopy/ELF/auto-remove-shndx.test
+++ /dev/null
@@ -1,5 +0,0 @@
-# RUN: %python %p/../Inputs/ungzip.py %p/Inputs/many-sections.o.gz > %t
-# RUN: llvm-objcopy -R 's65[3-9][0-9][0-9]' -R 's6529[0-9]' -R 's6528[1-9]' %t %t2
-# RUN: llvm-readobj --sections %t2 | FileCheck --check-prefix=SECS %s
-
-# SECS-NOT: Name: .symtab_shndx
Index: llvm/test/tools/llvm-objcopy/ELF/auto-remove-add-symtab-shndx.test
===================================================================
--- /dev/null
+++ llvm/test/tools/llvm-objcopy/ELF/auto-remove-add-symtab-shndx.test
@@ -0,0 +1,47 @@
+# This test makes sure that the .symtab_shndx section is automatically removed
+# when it is not needed and added when it is.
+
+RUN: %python %p/../Inputs/ungzip.py %p/Inputs/many-sections.o.gz > %t.0
+RUN: echo 'foo' > %t
+# many-sections.o contains sections from s1 to s65535, each with a symbol.
+# Remove enough so that the last section with a symbol has index
+# SHN_LORESERVE (i.e. s65280), and show the section hasn't been removed.
+RUN: llvm-objcopy -R 's65[3-9][0-9][0-9]' -R 's6529[0-9]' -R 's6528[1-9]' %t.0 %t2
+RUN: llvm-readobj --sections %t2 | FileCheck --check-prefix=SHNDX %s
+# Remove one more to cause removal of the section.
+RUN: llvm-objcopy -R s65280 %t2 %t3
+RUN: llvm-readobj --sections %t3 | FileCheck --check-prefix=NOSHNDX %s
+
+# Add additional sections. No .symtab_shndx should be added, as they contain no
+# symbols.
+RUN: llvm-objcopy --add-section=newsec1=%t --add-section=newsec2=%t --add-section=newsec3=%t %t3 %t4
+RUN: llvm-readobj --sections %t4 | FileCheck --check-prefix=NOSHNDX %s
+
+# Add a symbol to one of these sections and show that the .symtab_shndx
+# section has been created and that the new symbol has the right section index.
+RUN: llvm-objcopy --add-symbol=newsym=newsec1:0 %t4 %t5
+RUN: llvm-readobj --symbols --sections %t5 | \
+RUN:   FileCheck --check-prefixes=SHNDX,SHNDX-SYM -DSECTION=newsec1 %s
+
+# Show that removing and adding symbols, but not their sections, has the same
+# effect regarding removal/addition of the .symtab_shndx section.
+# FIXME: llvm-objcopy does not currently remove this section in these
+# circumstances. See https://bugs.llvm.org/show_bug.cgi?id=49364
+# Test is left here to document the current behaviour.
+RUN: llvm-objcopy -N 'sym65[3-9][0-9][0-9]' -N 'sym652[89][0-9]' -w %t.0 %t6
+RUN: llvm-readobj --sections %t6 | not FileCheck --check-prefix=NOSHNDX %s
+# FIXME: Remove these test lines once the bug has been fixed. They simply show
+# that the section is removed by a run without any further operations.
+RUN: llvm-objcopy %t6 %t6b
+RUN: llvm-readobj --sections %t6b | FileCheck --check-prefix=NOSHNDX %s
+
+# Add a symbol to ensure the section is reinstated.
+RUN: llvm-objcopy --add-symbol=newsym=s65280:0 -w %t6 %t7
+RUN: llvm-readobj --sections --symbols %t7 | \
+RUN:   FileCheck --check-prefixes=SHNDX,SHNDX-SYM -DSECTION=s65280 %s
+
+NOSHNDX-NOT:    Name: .symtab_shndx
+SHNDX:          Name: .symtab_shndx
+SHNDX-SYM:      Name: newsym
+SHNDX-SYM:      Section:
+SHNDX-SYM-SAME:           [[SECTION]]


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D97661.327042.patch
Type: text/x-patch
Size: 4817 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210301/7511192b/attachment.bin>