llvm-objcopy: Set sh_link to 0 on unrecognized symtab-linked sections.

[pcc]

Summary

Per discussion on the generic-abi mailing list:
https://groups.google.com/forum/#!topic/generic-abi/MPr8TVtnVn4

An object file manipulation tool must either write out a symbol
table with the same number of entries as the original symbol table
and in the same order, or if this is impossible, refuse to operate
on the object file if it has unrecognized sections that are linked
to the symtab section. However, existing tools (namely GNU strip,
GNU objcopy and ld.{bfd,gold,lld} -r) do not comply with this at
present: they change symbol table indexes and set sh_link to 0 on
the unrecognized symtab-linked sections.

We intend to use the latter as a (temporary) signal that a tool has
operated on a proposed new symtab-linked section and invalidated the
symbol table indexes. However, llvm-objcopy currently keeps sh_link
pointing to the new symtab section. This patch changes llvm-objcopy
to set sh_link to 0 to match the behaviour of the other tools.

[alexander-shaposhnikov]

to me LG, but i would wait for @jhenderson .

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/Object.cpp

399 ↗

(On Diff #148698)

nit:

this->Link = LinkSection ? LinkSection->Index : 0;

[jhenderson]

LGTM with the suggested test change.

I'm not a massive fan of this behaviour, but it makes sense to follow GNU's lead on this until such point as there's a standards-defined way of doing it. I'm assuming that all tools out there already know how to consume such special sections.

llvm/test/tools/llvm-objcopy/symtab-link.test

2 ↗

(On Diff #148698)

Based on a comment I think I saw in the gABI thread, that suggests that it's important for llvm-strip to follow the same behaviour, would it be worth adding an equivalent case using llvm-strip? There are some other tests that show how we typically do this.

llvm/trunk/test/tools/llvm-objcopy/symtab-link.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy %t %t2
+# RUN: llvm-readobj -sections %t2 | FileCheck %s
+# RUN: cp %t %t3
+# RUN: llvm-strip --strip-debug %t3
+# RUN: llvm-readobj -sections %t3 | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .foo
+    Link:            .symtab
+    Type:            SHT_PROGBITS
+    Flags:           [ ]
+
+# CHECK:      Name: .foo
+# CHECK-NEXT: Type:
+# CHECK-NEXT: Flags [ (0x0)
+# CHECK-NEXT: ]
+# CHECK-NEXT: Address:
+# CHECK-NEXT: Offset:
+# CHECK-NEXT: Size:
+# CHECK-NEXT: Link: 0

llvm/trunk/tools/llvm-objcopy/Object.cpp

 }
 
 void GroupSection::markSymbols() {
   if (Sym)
     Sym->Referenced = true;
 }
 
 void Section::initialize(SectionTableRef SecTable) {
-  if (Link != ELF::SHN_UNDEF)
+  if (Link != ELF::SHN_UNDEF) {
     LinkSection =
         SecTable.getSection(Link, "Link field value " + Twine(Link) +
                                       " in section " + Name + " is invalid");
+    if (LinkSection->Type == ELF::SHT_SYMTAB)
+      LinkSection = nullptr;
+  }
 }
 
 void Section::finalize() {
-  if (LinkSection)
-    this->Link = LinkSection->Index;
+  this->Link = LinkSection ? LinkSection->Index : 0;
 }
 
 void GnuDebugLinkSection::init(StringRef File, StringRef Data) {
   FileName = sys::path::filename(File);
   // The format for the .gnu_debuglink starts with the file name and is
   // followed by a null terminator and then the CRC32 of the file. The CRC32
   // should be 4 byte aligned. So we add the FileName size, a 1 for the null
   // byte, and then finally push the size to alignment and add 4.