[llvm-objcopy] Match GNU behaviour regarding file symbols

[kongyi]

Summary

GNU objcopy removes STT_FILE symbols for --strip-debug operations, and keeps them for --discard-all operation. Match their behaviour for llvm-objcopy.

[MaskRay]

llvm/test/tools/llvm-objcopy/ELF/keep-file-symbols.test

12

We need tests for --strip-debug --keep-file-symbols and --strip-unneeded --keep-file-symbols

110

Align Symbols [

Add a space after #

[kongyi]

llvm/test/tools/llvm-objcopy/ELF/keep-file-symbols.test

110

I don't think we should taint the blame history just for minor cosmetic reason. Besides, plenty of other tests does not have space after #.

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/keep-file-symbols.test

12

Yes, this test is about the "--keep-file-symbols" option. All calls should use it. To test your behaviour, you should add to the strip-debug.test, strip-unneeded.test and discard-all.test (although as @MaskRay says, you should also test things here with --keep-file-symbols).

You also need test cases for both llvm-objcopy and llvm-strip, since you've modified code that affects them each separately.

130

Nit: too many blank lines at end of file.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

424

No need for this code change: --keep-file-symbols is actioned above at line 410.

[jhenderson]

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

425

You still don't need && !Config.KeepFileSymbols. Line 410 of this file handles that case (if you delete it, you should see no changes in test results).

[jhenderson]

LGTM, with the suggested comment deletion. Please let @MaskRay comment too.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

423–425

Taking another look, I don't think you need the comment at all as the code clearly describes this behaviour (--strip-debug removing STT_FILE symbols), so I think you can delete it.

[MaskRay]

LGTM, with the comment deleted as requested.

[thakis]

Looks like this breaks tests everywhere, e.g http://45.33.8.238/linux/15764/step_12.txt

Please take a look and revert if it takes a while to investigate.

[kongyi]

In D76675#1993701, @thakis wrote:

Looks like this breaks tests everywhere, e.g http://45.33.8.238/linux/15764/step_12.txt

Please take a look and revert if it takes a while to investigate.

This is already reverted.

llvm/test/tools/llvm-objcopy/ELF/keep-file-symbols.test

 # RUN: yaml2obj %s -o %t
 # RUN: llvm-objcopy --strip-all --keep-file-symbols %t %t2
 # RUN: llvm-readobj --symbols %t2 | FileCheck %s --check-prefix=STRIPALL
 # RUN: llvm-objcopy --keep-file-symbols --strip-symbol foo %t %t2
 # RUN: llvm-readobj --symbols %t2 | FileCheck %s --check-prefix=STRIP
 
 # RUN: llvm-strip --keep-file-symbols %t -o %t3
 # RUN: llvm-readobj --symbols %t3 | FileCheck %s --check-prefix=STRIPALL
 # RUN: llvm-strip --keep-file-symbols --strip-symbol foo %t -o %t4
 # RUN: llvm-readobj --symbols %t4 | FileCheck %s --check-prefix=STRIP
 
+# RUN: llvm-objcopy --keep-file-symbols --strip-debug %t %t5
+# RUN: llvm-readobj --symbols %t5 | FileCheck %s --check-prefix=STRIPDEBUG
+# RUN: llvm-objcopy --keep-file-symbols --strip-unneeded %t %t6
+# RUN: llvm-readobj --symbols %t6 | FileCheck %s --check-prefix=STRIPDEBUG
+
+# RUN: llvm-strip --keep-file-symbols --strip-debug %t -o %t7
+# RUN: llvm-readobj --symbols %t7 | FileCheck %s --check-prefix=STRIPDEBUG
+# RUN: llvm-strip --keep-file-symbols --strip-unneeded %t -o %t8
+# RUN: llvm-readobj --symbols %t8 | FileCheck %s --check-prefix=STRIPDEBUG
+
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
+  - Name:            .debugfoo
+    Type:            SHT_PROGBITS
+    Content:         "00000000"
   - Name:            .text
     Type:            SHT_PROGBITS
     Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
     Address:         0x1000
     AddressAlign:    0x0000000000000010
     Size:            64
 Symbols:
   - Name:     foo
     Type:     STT_FILE
     Section:  .text
   - Name:     bar
     Type:     STT_FUNC
     Section:  .text
     Binding:  STB_GLOBAL
+  - Name:     debugfoo
+    Section:  .debugfoo
+    Binding:  STB_GLOBAL
 
 #STRIPALL: Symbols [
 #STRIPALL-NEXT:  Symbol {
 #STRIPALL-NEXT:    Name:
 #STRIPALL-NEXT:    Value: 0x0
 #STRIPALL-NEXT:    Size: 0
 #STRIPALL-NEXT:    Binding: Local
 #STRIPALL-NEXT:    Type: None

 #STRIP-NEXT:    Name: bar
 #STRIP-NEXT:    Value: 0x0
 #STRIP-NEXT:    Size: 0
 #STRIP-NEXT:    Binding: Global
 #STRIP-NEXT:    Type: Function
 #STRIP-NEXT:    Other: 0
 #STRIP-NEXT:    Section: .text
 #STRIP-NEXT:  }
+#STRIP-NEXT:  Symbol {
+#STRIP-NEXT:    Name: debugfoo
+#STRIP-NEXT:    Value: 0x0
+#STRIP-NEXT:    Size: 0
+#STRIP-NEXT:    Binding: Global
+#STRIP-NEXT:    Type: None
+#STRIP-NEXT:    Other: 0
+#STRIP-NEXT:    Section: .debugfoo
+#STRIP-NEXT:  }
 #STRIP-NEXT:]
+
+#STRIPDEBUG: Symbols [
+#STRIPDEBUG-NEXT:  Symbol {
+#STRIPDEBUG-NEXT:    Name:
+#STRIPDEBUG-NEXT:    Value: 0x0
+#STRIPDEBUG-NEXT:    Size: 0
+#STRIPDEBUG-NEXT:    Binding: Local
+#STRIPDEBUG-NEXT:    Type: None
+#STRIPDEBUG-NEXT:    Other: 0
+#STRIPDEBUG-NEXT:    Section: Undefined
+#STRIPDEBUG-NEXT:  }
+#STRIPDEBUG-NEXT:  Symbol {
+#STRIPDEBUG-NEXT:    Name: foo
+#STRIPDEBUG-NEXT:    Value: 0x0
+#STRIPDEBUG-NEXT:    Size: 0
+#STRIPDEBUG-NEXT:    Binding: Local
+#STRIPDEBUG-NEXT:    Type: File
+#STRIPDEBUG-NEXT:    Other: 0
+#STRIPDEBUG-NEXT:    Section: .text
+#STRIPDEBUG-NEXT:  }
+#STRIPDEBUG-NEXT:  Symbol {
+#STRIPDEBUG-NEXT:    Name: bar
+#STRIPDEBUG-NEXT:    Value: 0x0
+#STRIPDEBUG-NEXT:    Size: 0
+#STRIPDEBUG-NEXT:    Binding: Global
+#STRIPDEBUG-NEXT:    Type: Function
+#STRIPDEBUG-NEXT:    Other: 0
+#STRIPDEBUG-NEXT:    Section: .text
+#STRIPDEBUG-NEXT:  }
+#STRIPDEBUG-NEXT:]
 No newline at end of file

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

- Name:            .text
     Content:         "00000000"
 Symbols:
   - Name:    foo
     Section: .text
     Binding:  STB_GLOBAL
   - Name:    debugfoo
     Section: .debugfoo
     Binding:  STB_GLOBAL
+  - Name:    filesymbol
+    Type:    STT_FILE
 
 # CHECK: SectionHeaderCount: 5
 
 # CHECK: Name: .text
 # CHECK: Name: .symtab
 # CHECK: Name: .strtab
 # CHECK: Name: .shstrtab
 
-# Check that *only* foo is copied and not debugfoo
+# Check that *only* foo is copied and not debugfoo or filesymbol.
 # CHECK:      Symbols [
 # CHECK-NEXT:   Symbol {
 # CHECK-NEXT:     Name:
 # CHECK-NEXT:     Value:
 # CHECK-NEXT:     Size:
 # CHECK-NEXT:     Binding:
 # CHECK-NEXT:     Type:
 # CHECK-NEXT:     Other:

llvm/tools/llvm-objcopy/CopyConfig.cpp

if (InputArgs.hasArg(OBJCOPY_discard_all, OBJCOPY_discard_locals))
     Config.DiscardMode =
         InputArgs.hasFlag(OBJCOPY_discard_all, OBJCOPY_discard_locals)
             ? DiscardType::All
             : DiscardType::Locals;
   Config.OnlyKeepDebug = InputArgs.hasArg(OBJCOPY_only_keep_debug);
   Config.KeepFileSymbols = InputArgs.hasArg(OBJCOPY_keep_file_symbols);
   Config.DecompressDebugSections =
       InputArgs.hasArg(OBJCOPY_decompress_debug_sections);
-  if (Config.DiscardMode == DiscardType::All)
+  if (Config.DiscardMode == DiscardType::All) {
     Config.StripDebug = true;
+    Config.KeepFileSymbols = true;
+  }
   for (auto Arg : InputArgs.filtered(OBJCOPY_localize_symbol))
     if (Error E = Config.SymbolsToLocalize.addMatcher(NameOrPattern::create(
             Arg->getValue(), SymbolMatchStyle, ErrorCallback)))
       return std::move(E);
   for (auto Arg : InputArgs.filtered(OBJCOPY_localize_symbols))
     if (Error E = addSymbolsFromFile(Config.SymbolsToLocalize, DC.Alloc,
                                      Arg->getValue(), SymbolMatchStyle,
                                      ErrorCallback))
if (Error E = Config.SymbolsToKeep.addMatcher(NameOrPattern::create(
             Arg->getValue(), SymbolMatchStyle, ErrorCallback)))
       return std::move(E);
 
   if (!InputArgs.hasArg(STRIP_no_strip_all) && !Config.StripDebug &&
       !Config.StripUnneeded && Config.DiscardMode == DiscardType::None &&
       !Config.StripAllGNU && Config.SymbolsToRemove.empty())
     Config.StripAll = true;
 
-  if (Config.DiscardMode == DiscardType::All)
+  if (Config.DiscardMode == DiscardType::All) {
     Config.StripDebug = true;
+    Config.KeepFileSymbols = true;
+  }
 
   Config.DeterministicArchives =
       InputArgs.hasFlag(STRIP_enable_deterministic_archives,
                         STRIP_disable_deterministic_archives, /*default=*/true);
 
   Config.PreserveDates = InputArgs.hasArg(STRIP_preserve_dates);
   Config.InputFormat = FileFormat::Unspecified;
   Config.OutputFormat = FileFormat::Unspecified;

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

if ((Config.DiscardMode == DiscardType::All ||
           StringRef(Sym.Name).startswith(".L"))) &&
         Sym.Binding == STB_LOCAL && Sym.getShndx() != SHN_UNDEF &&
         Sym.Type != STT_FILE && Sym.Type != STT_SECTION)
       return true;
 
     if (Config.StripAll || Config.StripAllGNU)
       return true;
 
+    // StripDebug removes STT_FILE symbols, unless KeepFileSymbols is explicitly
+    // set to true. True case of Config.KeepFileSymbols is already handled above
+    // no need to check again.
+    if (Config.StripDebug && Sym.Type == STT_FILE)
+      return true;
+
     if (Config.SymbolsToRemove.matches(Sym.Name))
       return true;
 
     if ((Config.StripUnneeded ||
          Config.UnneededSymbolsToRemove.matches(Sym.Name)) &&
         (!Obj.isRelocatable() || isUnneededSymbol(Sym)))
       return true;