[llvm-objcopy] Strip file symbols with --strip-unneeded

[evgeny777]

Summary

I don't get why such symbols are kept. Also it's always possible to use --keep-file-symbols if they shouldn't be stripped for whatever reason

[jhenderson]

Does this match GNU objcopy's behaviour? If GNU strips them, we should probably too. If not, we shouldn't.

[evgeny777]

Does this match GNU objcopy's behaviour? If GNU strips them, we should probably too. If not, we shouldn't.

Yes, GNU objcopy seems to remove STT_FILE symbols even in relocatable objects

[jhenderson]

This looks good to me, but it would be worth getting a second pair of eyes from one of the other contributors. Additionally, I'd like to have a better idea of why the decision was made to keep file symbols originally, since it was an explicit case before. I see you originally added it in https://reviews.llvm.org/rL353919.

[evgeny777]

I see you originally added it in https://reviews.llvm.org/rL353919.

It was in fact added in https://reviews.llvm.org/D46896. In r353919 I just moved the code to a separate function.

[jhenderson]

In D61641#1493743, @evgeny777 wrote:

I see you originally added it in https://reviews.llvm.org/rL353919.

It was in fact added in https://reviews.llvm.org/D46896. In r353919 I just moved the code to a separate function.

Oh, my bad. Sorry, I didn't look hard enough. Still worth seeing if anybody else has anything to chime in with before this goes in, so I'd give it a day before committing, if that's okay?

[evgeny777]

@paulsemel Any comments?

test/tools/llvm-objcopy/ELF/strip-unneeded.test

 # RUN: yaml2obj %s > %t
 # RUN: cp %t %t1
 # RUN: llvm-objcopy --strip-unneeded %t %t2
 # Verify that llvm-objcopy has not modified the input.
 # RUN: cmp %t %t1
 # RUN: llvm-readobj --symbols %t2 | FileCheck %s
 
+# Verify that --keep-file-symbols works together with --strip-unneeded
+# RUN: llvm-objcopy --keep-file-symbols --strip-unneeded %t %t2b
+# RUN: llvm-readobj --symbols %t2b | FileCheck %s --check-prefix=FILESYM
+
 # RUN: llvm-objcopy --strip-unneeded-symbol=bar \
 # RUN:              %t %t3
 # RUN: llvm-readobj --symbols %t3 | FileCheck %s --check-prefixes=STRIP-BAR,REMAIN
 
 # RUN: llvm-objcopy --regex --strip-unneeded-symbol='.*' %t %t4
 # RUN: cmp %t2 %t4
 
 # RUN: echo " bar # bar " > %t.list.txt
 # RUN: echo "foobar" >> %t.list.txt
 # RUN: echo "foobaz" >> %t.list.txt
 # RUN: echo " # comment " >> %t.list.txt
 # RUN: llvm-objcopy --strip-unneeded-symbols %t.list.txt %t %t5
-# RUN: cmp %t2 %t5
+# RUN: cmp %t2b %t5
 
 # RUN: echo " .* # * - remove all " > %t.list2.txt
 # RUN: llvm-objcopy --regex --strip-unneeded-symbols %t.list2.txt %t %t6
 # RUN: cmp %t2 %t6
 
 # Verify that llvm-strip modifies the symbol table the same way.
 # RUN: llvm-strip --strip-unneeded %t
 # RUN: cmp %t %t2

 #CHECK-NEXT:    Value: 0x1010
 #CHECK-NEXT:    Size: 8
 #CHECK-NEXT:    Binding: Local
 #CHECK-NEXT:    Type: Function
 #CHECK-NEXT:    Other: 0
 #CHECK-NEXT:    Section: .text
 #CHECK-NEXT:  }
 #CHECK-NEXT:  Symbol {
-#CHECK-NEXT:    Name: fileSymbol
-#CHECK-NEXT:    Value: 0x0
-#CHECK-NEXT:    Size: 0
-#CHECK-NEXT:    Binding: Local
-#CHECK-NEXT:    Type: File
-#CHECK-NEXT:    Other: 0
-#CHECK-NEXT:    Section: Undefined
-#CHECK-NEXT:  }
-#CHECK-NEXT:  Symbol {
 #CHECK-NEXT:    Name: sectionSymbol
 #CHECK-NEXT:    Value: 0x0
 #CHECK-NEXT:    Size: 0
 #CHECK-NEXT:    Binding: Local
 #CHECK-NEXT:    Type: Section
 #CHECK-NEXT:    Other: 0
 #CHECK-NEXT:    Section: Undefined
 #CHECK-NEXT:  }

 #CHECK-NEXT:    Size: 8
 #CHECK-NEXT:    Binding: Weak
 #CHECK-NEXT:    Type: Function
 #CHECK-NEXT:    Other: 0
 #CHECK-NEXT:    Section: .text
 #CHECK-NEXT:  }
 #CHECK-NEXT:]
 
+#FILESYM:       Name: fileSymbol
+#FILESYM-NEXT:  Value: 0x0
+
 #STRIP-BAR-NOT: Name: bar ({{.*}})
 #REMAIN: Name: foobar
 #REMAIN: Name: foobaz

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

static void replaceDebugSections(
   RemovePred = [shouldReplace, RemovePred](const SectionBase &Sec) {
     return shouldReplace(Sec) || RemovePred(Sec);
   };
 }
 
 static bool isUnneededSymbol(const Symbol &Sym) {
   return !Sym.Referenced &&
          (Sym.Binding == STB_LOCAL || Sym.getShndx() == SHN_UNDEF) &&
-         Sym.Type != STT_FILE && Sym.Type != STT_SECTION;
+         Sym.Type != STT_SECTION;
 }
 
 static Error updateAndRemoveSymbols(const CopyConfig &Config, Object &Obj) {
   // TODO: update or remove symbols only if there is an option that affects
   // them.
   if (!Obj.SymbolTable)
     return Error::success();