[llvm-objcopy] Allow 'protected' visibility to be set when using add-symbol

[chrisjackson]

Summary

Currently add-symbol allows "hidden" and "default" as valid flags, but not "protected". This diff enables symbols to be added with STV_PROTECTED visibility.

[jhenderson]

@jakehehrlich/@rupprecht, I looked at this offline with @chrisjackson, and I think it makes sense to add this functionality. I believe it's above what GNU does (at the moment), but I can't think of any reason why a user shouldn't be allowed to add STV_PROTECTED symbols, and we have a use-case for it internally.

[MaskRay]

It makes sense from the perspective of completeness but STV_PROTECTED is problematic and can hardly find a justified use case.

People use STV_PROTECTED when they want to avoid GOT/PLT costs in PIC code. However,

A protected STT_OBJECT prevents you from doing copy relocation. ld.bfd silently accepts this, which can cause ODR violations.
A protected STT_FUNC prevents you from making a canonical PLT (-fno-pic code in executable references an external function). If the linker fails to stop you, you'll get pointer equality problems. (It can be argued this case can be fixed by letting the compiler go through GOT when the address of a protected function is taken, but then the GOT indirection will likely break people's expectation. A symbol lookup change is also required on ld.so side.)

I think lld and gold reject both cases. So at the best, a protected symbol can be seen as a promise of the toolchain that the symbol will not be interposed.

Actually, in a DSO, a STV_PROTECTED use case can be easily replaced with the hidden+alias idiom:

__attribute__((visibility("hidden"))) void internal() {} // internal use
__attribute__((alias("internal"))) void external(); // external API

The reservation of STV_INTERNAL was requested by SGI when gABI was formulated. All other parties appear to treat STV_INTERNAL the same as STV_HIDDEN.

[chrisjackson]

In D65891#1620658, @MaskRay wrote:

It makes sense from the perspective of completeness but STV_PROTECTED is problematic and can hardly find a justified use case.

People use STV_PROTECTED when they want to avoid GOT/PLT costs in PIC code. However,

A protected STT_OBJECT prevents you from doing copy relocation. ld.bfd silently accepts this, which can cause ODR violations.
A protected STT_FUNC prevents you from making a canonical PLT (-fno-pic code in executable references an external function). If the linker fails to stop you, you'll get pointer equality problems. (It can be argued this case can be fixed by letting the compiler go through GOT when the address of a protected function is taken, but then the GOT indirection will likely break people's expectation. A symbol lookup change is also required on ld.so side.)

I think lld and gold reject both cases. So at the best, a protected symbol can be seen as a promise of the toolchain that the symbol will not be interposed.

Actually, in a DSO, a STV_PROTECTED use case can be easily replaced with the hidden+alias idiom:

__attribute__((visibility("hidden"))) void internal() {} // internal use
__attribute__((alias("internal"))) void external(); // external API

The reservation of STV_INTERNAL was requested by SGI when gABI was formulated. All other parties appear to treat STV_INTERNAL the same as STV_HIDDEN.

I agree with your points for standard ELF toolchains. We use STV_PROTECTED in our toolchain, as we have a custom DSO format. The use of protected is not unprecedented, see ARM reference for example.

llvm/trunk/docs/CommandGuide/llvm-objcopy.rst

  the symbol is added as an absolute symbol. The ``<flags>`` affect the symbol
  properties. Accepted values are:
 
  - `global` = the symbol will have global binding.
  - `local` = the symbol will have local binding.
  - `weak` = the symbol will have weak binding.
  - `default` = the symbol will have default visibility.
  - `hidden` = the symbol will have hidden visibility.
+ - `protected` = the symbol will have protected visibility.
  - `file` = the symbol will be an `STT_FILE` symbol.
  - `section` = the symbol will be an `STT_SECTION` symbol.
  - `object` = the symbol will be an `STT_OBJECT` symbol.
  - `function` = the symbol will be an `STT_FUNC` symbol.
  - `indirect-function` = the symbol will be an `STT_GNU_IFUNC` symbol.
 
  Additionally, the following flags are accepted but ignored: `debug`,
  `constructor`, `warning`, `indirect`, `synthetic`, `unique-object`, `before`.

llvm/trunk/test/tools/llvm-objcopy/ELF/add-symbol.test

 # RUN: yaml2obj %s -o %t
 # RUN: llvm-objcopy --add-symbol='abs1=1' \ 
 # RUN:              --add-symbol='abs2=2,LoCaL,HiDdEn,FuNcTiOn' \
 # RUN:              --add-symbol='abs3=3,global,default,object' \
+# RUN:              --add-symbol='abs4=4,global,protected,object' \
 # RUN:              --add-symbol='foo.cpp=0,file' \
 # RUN:              --add-symbol='=.text:0,section' \
 # RUN:              --add-symbol='data=.data:0x100,weak' \
 # RUN:              --add-symbol='ifunc=.text:0,indirect-function' \
 # RUN:              %t %t2
 # RUN: llvm-readelf -s %t2 | FileCheck %s
 
 # Checked ignored options
ProgramHeaders:
   - Type: PT_LOAD
     Flags: [ PF_R, PF_W ]
     VAddr: 0x2000
     PAddr: 0x2000
     Align: 0x1000
     Sections:
       - Section: .data
 
-# CHECK:      0: 00000000 0 NOTYPE LOCAL DEFAULT UND 
+# CHECK:      0: 00000000 0 NOTYPE LOCAL DEFAULT UND
 # CHECK-NEXT: 1: 00000001 0 NOTYPE GLOBAL DEFAULT ABS abs1
 # CHECK-NEXT: 2: 00000002 0 FUNC LOCAL HIDDEN ABS abs2
 # CHECK-NEXT: 3: 00000003 0 OBJECT GLOBAL DEFAULT ABS abs3
-# CHECK-NEXT: 4: 00000000 0 FILE GLOBAL DEFAULT ABS foo.cpp
-# CHECK-NEXT: 5: 00001000 0 SECTION GLOBAL DEFAULT 1 
-# CHECK-NEXT: 6: 00002100 0 NOTYPE WEAK DEFAULT 2 data
-# CHECK-NEXT: 7: 00001000 0 IFUNC GLOBAL DEFAULT 1 ifunc
+# CHECK-NEXT: 4: 00000004 0 OBJECT GLOBAL PROTECTED ABS abs4
+# CHECK-NEXT: 5: 00000000 0 FILE GLOBAL DEFAULT ABS foo.cpp
+# CHECK-NEXT: 6: 00001000 0 SECTION GLOBAL DEFAULT 1
+# CHECK-NEXT: 7: 00002100 0 NOTYPE WEAK DEFAULT 2 data
+# CHECK-NEXT: 8: 00001000 0 IFUNC GLOBAL DEFAULT 1 ifunc
 
 # IGNORED:       1: 00000000 0 NOTYPE GLOBAL DEFAULT ABS dummy1
 # IGNORED-NEXT:  2: 00000000 0 NOTYPE GLOBAL DEFAULT ABS dummy2
 
 # ERR1: error: bad format for --add-symbol, missing '=' after 'test'
 # ERR2: error: bad format for --add-symbol, missing section name or symbol value
 # ERR3: error: unsupported flag for --add-symbol: 'cool'
 # ERR4: error: unsupported flags for --add-symbol: 'foo', 'bar'
 # ERR5: error: bad symbol value: 'xyz'
 

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

static Expected<NewSymbolInfo> parseNewSymbolInfo(StringRef FlagValue) {
   for (size_t I = 1, NumFlags = Flags.size(); I < NumFlags; ++I)
     static_cast<Functor>(
         StringSwitch<Functor>(Flags[I])
             .CaseLower("global", [&SI] { SI.Bind = ELF::STB_GLOBAL; })
             .CaseLower("local", [&SI] { SI.Bind = ELF::STB_LOCAL; })
             .CaseLower("weak", [&SI] { SI.Bind = ELF::STB_WEAK; })
             .CaseLower("default", [&SI] { SI.Visibility = ELF::STV_DEFAULT; })
             .CaseLower("hidden", [&SI] { SI.Visibility = ELF::STV_HIDDEN; })
+            .CaseLower("protected", [&SI] { SI.Visibility = ELF::STV_PROTECTED; })
             .CaseLower("file", [&SI] { SI.Type = ELF::STT_FILE; })
             .CaseLower("section", [&SI] { SI.Type = ELF::STT_SECTION; })
             .CaseLower("object", [&SI] { SI.Type = ELF::STT_OBJECT; })
             .CaseLower("function", [&SI] { SI.Type = ELF::STT_FUNC; })
             .CaseLower("indirect-function",
                        [&SI] { SI.Type = ELF::STT_GNU_IFUNC; })
             .CaseLower("debug", [] {})
             .CaseLower("constructor", [] {})

llvm/trunk/tools/llvm-objcopy/ObjcopyOpts.td

defm change_start : Eq<"change-start", "Add <incr> to the start address. Can be "
                        "cumulatively.">,
                     MetaVarName<"incr">;
 def adjust_start : JoinedOrSeparate<["--"], "adjust-start">,
                    Alias<change_start>,
                    HelpText<"Alias for --change-start">;
 
 defm add_symbol
     : Eq<"add-symbol", "Add new symbol <name> to .symtab. Accepted flags: "
-         "global, local, weak, default, hidden, file, section, object, "
+         "global, local, weak, default, hidden, protected, file, section, object, "
          "function, indirect-function. Accepted but ignored for "
          "compatibility: debug, constructor, warning, indirect, synthetic, "
          "unique-object, before.">,
       MetaVarName<"name=[section:]value[,flags]">;