[llvm-objcopy] Fix --keep-global-symbol/--globalize-symbol for undefined symbols.

[rupprecht]

Summary

--keep-global-symbol and --globalize-symbol don't make sense for undefined symbols, so it should be ignored for those symbols. This matches GNU objcopy behavior.

[jhenderson]

My feeling is that a single separate test is not the right way to test this. I think it makes more sense for an undefined symbol to be added to globalize.test and --keep-global-symbols.test. What do you think?

test/tools/llvm-objcopy/globalize-undefined-sym.test

11 ↗

(On Diff #171201)

Are you sure this is Foo? That's a symbol that doesn't even exist. I assume you meant it to be Weak or Global?

[rupprecht]

test/tools/llvm-objcopy/globalize-undefined-sym.test

11 ↗

(On Diff #171201)

Foo is intentional, sorry for not explaining in a test comment why -- fixed.

[rupprecht]

• Update test

[jakehehrlich]

This seems correct to me. LGTM.

[rupprecht]

• Move test cases to already existing tests

[rupprecht]

• typo

[rupprecht]

In D53733#1278600, @jhenderson wrote:

My feeling is that a single separate test is not the right way to test this. I think it makes more sense for an undefined symbol to be added to globalize.test and --keep-global-symbols.test. What do you think?

Makes sense to me -- done.

[jhenderson]

LGTM.

llvm/trunk/test/tools/llvm-objcopy/globalize.test

 # RUN: yaml2obj %s > %t
-# RUN: llvm-objcopy --globalize-symbol Global --globalize-symbol Local --globalize-symbol Weak %t %t2
+# RUN: llvm-objcopy --globalize-symbol Global \
+# RUN:   --globalize-symbol Local \
+# RUN:   --globalize-symbol Weak \
+# RUN:   --globalize-symbol WeakUndef %t %t2
 # RUN: llvm-readobj -symbols %t2 | FileCheck %s
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
- Name:     Local
       Value:    0x1000
       Size:     8
   Weak:
     - Name:     Weak
       Type:     STT_FUNC
       Size:     8
       Section:  .text
       Value:    0x1008
+    - Name:     WeakUndef
   Global:
     - Name:     Global
       Type:     STT_FUNC
       Size:     8
       Section:  .text
       Value:    0x1010
 
 #CHECK: Symbols [

 #CHECK-NEXT:    Name: Weak
 #CHECK-NEXT:    Value: 0x1008
 #CHECK-NEXT:    Size: 8
 #CHECK-NEXT:    Binding: Global
 #CHECK-NEXT:    Type: Function
 #CHECK-NEXT:    Other: 0
 #CHECK-NEXT:    Section: .text
 #CHECK-NEXT:  }
+#CHECK-NEXT:  Symbol {
+#CHECK-NEXT:    Name: WeakUndef
+#CHECK-NEXT:    Value: 0x0
+#CHECK-NEXT:    Size: 0
+#CHECK-NEXT:    Binding: Weak
+#CHECK-NEXT:    Type: None
+#CHECK-NEXT:    Other: 0
+#CHECK-NEXT:    Section: Undefined
+#CHECK-NEXT:  }
 #CHECK-NEXT:]

llvm/trunk/test/tools/llvm-objcopy/keep-global-symbols.test

 # Global3: Global because of "--keep-global-symbols %t-globals1.txt".
 # Global4: Global because of "--keep-global-symbols %t-globals2.txt".
 # Global5: Local, it appears in %t-globals2.txt but only in comments and as
 #     part of another symbol
 # Global6: Local, it appears in %t-globals2.txt but only part of another symbol
 # "Global5 Global6": Global, because it appears in %t-globals2.txt, but we only
 #     trim leading and trailing whitespace. We don't just take the first chunk
 #     that looks like a symbol.
+# Global7: Global, because even though it doesn't appear as any -G flags, does
+#     not get demoted since it's undefined.
 
 # RUN: echo Global2 > %t-globals1.txt
 # RUN: echo "  Global3  " > %t-globals2.txt
 # RUN: echo "Global4 # Global5" >> %t-globals2.txt
 # RUN: echo "  Global5 Global6  " >> %t-globals2.txt
 # RUN: echo "Unknown" >> %t-globals2.txt
 # RUN: echo "   " >> %t-globals2.txt
 # RUN: echo "# File with no symbols" > %t-globals3.txt
Global:
     - Name:        Global4
       Section:     .text
     - Name:        Global5
       Section:     .text
     - Name:        Global6
       Section:     .text
     - Name:        "Global5 Global6"
       Section:     .text
+    - Name:        Global7
 
-# CHECK:      Symbol table '.symtab' contains 13 entries:
+# CHECK:      Symbol table '.symtab' contains 14 entries:
 # CHECK-NEXT:    Num: Value Size Type Bind Vis Ndx Name
 # CHECK-NEXT:      0: {{.*}}  LOCAL  {{.*}}
 # CHECK-NEXT:      1: {{.*}}  LOCAL  {{.*}} Local1
 # CHECK-NEXT:      2: {{.*}}  LOCAL  {{.*}} Global5
 # CHECK-NEXT:      3: {{.*}}  LOCAL  {{.*}} Global6
 # CHECK-NEXT:      4: {{.*}}  LOCAL  {{.*}} Weak3
 # CHECK-NEXT:      5: {{.*}}  GLOBAL {{.*}} Local2
 # CHECK-NEXT:      6: {{.*}}  GLOBAL {{.*}} Global1
 # CHECK-NEXT:      7: {{.*}}  GLOBAL {{.*}} Global2
 # CHECK-NEXT:      8: {{.*}}  GLOBAL {{.*}} Global3
 # CHECK-NEXT:      9: {{.*}}  GLOBAL {{.*}} Global4
 # CHECK-NEXT:     10: {{.*}}  GLOBAL {{.*}} Global5 Global6
-# CHECK-NEXT:     11: {{.*}}  WEAK   {{.*}} Weak1
-# CHECK-NEXT:     12: {{.*}}  GLOBAL {{.*}} Weak2
+# CHECK-NEXT:     11: {{.*}}  GLOBAL {{.*}} UND Global7
+# CHECK-NEXT:     12: {{.*}}  WEAK   {{.*}} Weak1
+# CHECK-NEXT:     13: {{.*}}  GLOBAL {{.*}} Weak2

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

Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
       // --globalize-symbol: promote a symbol to global
       // --keep-global-symbol: all symbols except for these should be made local
       //
       // If --globalize-symbol is specified for a given symbol, it will be
       // global in the output file even if it is not included via
       // --keep-global-symbol. Because of that, make sure to check
       // --globalize-symbol second.
       if (!Config.SymbolsToKeepGlobal.empty() &&
-          !is_contained(Config.SymbolsToKeepGlobal, Sym.Name))
+          !is_contained(Config.SymbolsToKeepGlobal, Sym.Name) &&
+          Sym.getShndx() != SHN_UNDEF)
         Sym.Binding = STB_LOCAL;
 
       if (!Config.SymbolsToGlobalize.empty() &&
-          is_contained(Config.SymbolsToGlobalize, Sym.Name))
+          is_contained(Config.SymbolsToGlobalize, Sym.Name) &&
+          Sym.getShndx() != SHN_UNDEF)
         Sym.Binding = STB_GLOBAL;
 
       if (!Config.SymbolsToWeaken.empty() &&
           is_contained(Config.SymbolsToWeaken, Sym.Name) &&
           Sym.Binding == STB_GLOBAL)
         Sym.Binding = STB_WEAK;
 
       if (Config.Weaken && Sym.Binding == STB_GLOBAL &&