[ELF] Add -z start-stop-visibility= to set __start_/__stop_ symbol visibility

[phosek]

Summary

This matches the equivalent flag implemented in GNU linkers, see
https://sourceware.org/pipermail/binutils/2020-June/111685.html for
the associated discussion.

[phosek]

I'm open to other suggestions for the option name, other thing we considered is -start-stop-symbols-visibility=....

[grimar]

-z hide-start-stop maybe? (to shorten the name a bit)

lld/ELF/Writer.cpp

2231–2235

What is B stands for? I would expect to see V (Visibility).

[ruiu]

Adding a new option is a big deal. Could you elaborate on why you want to make these symbols hidden ones?

[MaskRay]

However, it's still useful to define them as hidden in many contexts.

Can you raise one example in the description?

[pcc]

I don't think you need to introduce an option for this. You can give the __start_ and __stop_ symbols hidden visibility in any object file, and the symbols in the final output file will be hidden because the linker will choose the most restrictive visibility that it sees for the visibility of the symbols in the final output file.

[phosek]

In D55682#1331628, @ruiu wrote:

Adding a new option is a big deal. Could you elaborate on why you want to make these symbols hidden ones?

The real question is concrete examples of when you'd ever not want them hidden? The normal use for these symbols is always local to the module, and you want to be able to have multiple modules using the same section names.

Consider a static library that is meant to be statically linked into shared objects as a private implementation detail. Say this library provides macros/templates to its users that emit metadata into a named section, and then library code that iterates over the section. Each instance of this library inside a given shared object must iterate over its own list, not the list of whichever user of the static library happens to be first in dynamic linking symbol resolution order.

It seems like the only reason these symbols are exported is because when support for generating these symbols has been first implemented in ld.bfd, it didn't even have a concept of visibility. Later, binutils tried to make these symbols hidden, but that broke clients that depend on the old behavior (according to Hyrum's law) which is documented in https://sourceware.org/bugzilla/show_bug.cgi?id=21964

In D55682#1332033, @pcc wrote:

I don't think you need to introduce an option for this. You can give the __start_ and __stop_ symbols hidden visibility in any object file, and the symbols in the final output file will be hidden because the linker will choose the most restrictive visibility that it sees for the visibility of the symbols in the final output file.

Yes that works, but it has to be done explicitly for each such symbol. We cannot change these symbols to be hidden by default for existing platforms as described above, but we would like to make them hidden by default in Fuchsia since we don't have any legacy code (yet) that would depend on the existing behavior. Introducing new option allows us to set it in the driver like we already do e.g. [for -z rodynamic](https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Fuchsia.cpp#L50) which is a similar case.

[phosek]

lld/ELF/Writer.cpp

2231–2235

Binding (that's what addOptionalRegular's argument is called as well), would you prefer V?

[phosek]

In D55682#1332019, @MaskRay wrote:

However, it's still useful to define them as hidden in many contexts.

Can you raise one example in the description?

I have updated the description to better explain the motivation and provide an example.

[MaskRay]

In D55682#1332033, @pcc wrote:

I don't think you need to introduce an option for this. You can give the __start_ and __stop_ symbols hidden visibility in any object file, and the symbols in the final output file will be hidden because the linker will choose the most restrictive visibility that it sees for the visibility of the symbols in the final output file.

Yes that works, but it has to be done explicitly for each such symbol. We cannot change these symbols to be hidden by default for existing platforms as described above, but we would like to make them hidden by default in Fuchsia since we don't have any legacy code (yet) that would depend on the existing behavior. Introducing new option allows us to set it in the driver like we already do e.g. [for -z rodynamic](https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Fuchsia.cpp#L50) which is a similar case.

Does that mean there is a visibility mismatch between source code and linker-defined symbol? e.g.

//extern char __start_f __attribute__((visibility("hidden")));
extern char __start_f; // Fuchsia wants to write code like this, but its visibility is set to hidden due to -z hidden-start-stop-symbols

__attribute__((section("f"))) char *foo() {
  return &__start_f;
}

Please correct me but if my understanding is correct, I think the explicit visibility attribute in source code would be clearer.

[ruiu]

Thank you for the explanation. Your explanation makes sense to me. Being said that, I don't think I'd add a new option to the linker for Fuchsia, as I guess that the situation in which you want to iterate over start/end symbols is not too frequent. Also if you iterate over a known list of start/end symbols, you can define the symbols as hidden ones using attribute, so there's already an easy workaround that works on many ELF linkers.

[phosek]

In D55682#1336709, @MaskRay wrote:

Does that mean there is a visibility mismatch between source code and linker-defined symbol? e.g.

//extern char __start_f __attribute__((visibility("hidden")));
extern char __start_f; // Fuchsia wants to write code like this, but its visibility is set to hidden due to -z hidden-start-stop-symbols

__attribute__((section("f"))) char *foo() {
  return &__start_f;
}

Please correct me but if my understanding is correct, I think the explicit visibility attribute in source code would be clearer.

In D55682#1336811, @ruiu wrote:

Thank you for the explanation. Your explanation makes sense to me. Being said that, I don't think I'd add a new option to the linker for Fuchsia, as I guess that the situation in which you want to iterate over start/end symbols is not too frequent. Also if you iterate over a known list of start/end symbols, you can define the symbols as hidden ones using attribute, so there's already an easy workaround that works on many ELF linkers.

The reason why is preferred over using explicit annotations in the source is because it matches the compiler behavior. We compile all our code with -fvisibility=hidden and we use explicit visibility annotations on symbols that should be exported. This is a standard way for controlling your ABI which is not specific to Fuchsia and is used by many other projects including LLVM. In that model, any symbol that does not have an explicit annotation is hidden. However, with __start/__stop symbols generated by lld, it's the opposite: they're exported by default and you need to use explicit annotation to make them hidden. That's confusing for developers and what we're trying to address. We would actually like to set -z hidden-start-stop-symbols in the Clang driver whenever -fvisibility=hidden is set to ensure that the compiler and linker use the same rules when it comes to symbol visibility.

[MaskRay]

In D55682#2069517, @phosek wrote:

In D55682#1336709, @MaskRay wrote:

Does that mean there is a visibility mismatch between source code and linker-defined symbol? e.g.

//extern char __start_f __attribute__((visibility("hidden")));
extern char __start_f; // Fuchsia wants to write code like this, but its visibility is set to hidden due to -z hidden-start-stop-symbols

__attribute__((section("f"))) char *foo() {
  return &__start_f;
}

Please correct me but if my understanding is correct, I think the explicit visibility attribute in source code would be clearer.

In D55682#1336811, @ruiu wrote:

Thank you for the explanation. Your explanation makes sense to me. Being said that, I don't think I'd add a new option to the linker for Fuchsia, as I guess that the situation in which you want to iterate over start/end symbols is not too frequent. Also if you iterate over a known list of start/end symbols, you can define the symbols as hidden ones using attribute, so there's already an easy workaround that works on many ELF linkers.

The reason why is preferred over using explicit annotations in the source is because it matches the compiler behavior. We compile all our code with -fvisibility=hidden and we use explicit visibility annotations on symbols that should be exported. This is a standard way for controlling your ABI which is not specific to Fuchsia and is used by many other projects including LLVM. In that model, any symbol that does not have an explicit annotation is hidden. However, with __start/__stop symbols generated by lld, it's the opposite: they're exported by default and you need to use explicit annotation to make them hidden. That's confusing for developers and what we're trying to address. We would actually like to set -z hidden-start-stop-symbols in the Clang driver whenever -fvisibility=hidden is set to ensure that the compiler and linker use the same rules when it comes to symbol visibility.

I am still on the fence, but my mind can be easily changed if binutils people are ok with a -z option: https://sourceware.org/pipermail/binutils/2020-June/111440.html

[mcgrathr]

Both GNU linkers now implement -z start-stop-visibility=... and binutils 2.35 will include that feature.

[MaskRay]

Please add a test, startstop-visibility.s (I would like start-stop-visibility.s, but some existing tests use the startstop- prefix).

Use -triple=x86_64 and use llvm-readelf -s to check visibility. Check both __start_foo and __stop_foo. Don't add another __start_bar.

Please mention that GNU ld and gold from binutils 2.35 onwards will support the option (
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=cae64165f47b64898c4f1982d294862cfae89a47 )

[phosek]

In D55682#2094089, @MaskRay wrote:

Please add a test, startstop-visibility.s (I would like start-stop-visibility.s, but some existing tests use the startstop- prefix).

Use -triple=x86_64 and use llvm-readelf -s to check visibility. Check both __start_foo and __stop_foo. Don't add another __start_bar.

Please mention that GNU ld and gold from binutils 2.35 onwards will support the option (
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=cae64165f47b64898c4f1982d294862cfae89a47 )

Done, the implementation should now match its GNU counterpart.

[MaskRay]

In D55682#2094089, @MaskRay wrote:

Please add a test, startstop-visibility.s (I would like start-stop-visibility.s, but some existing tests use the startstop- prefix).

Use -triple=x86_64 and use llvm-readelf -s to check visibility. Check both __start_foo and __stop_foo. Don't add another __start_bar.

Please mention that GNU ld and gold from binutils 2.35 will support the option (
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=cae64165f47b64898c4f1982d294862cfae89a47 )

Probably call out the option name in the subject, i.e. Add -z start-stop-visibility= to set __start_/__stop_ symbol visibility

lld/ELF/Driver.cpp

418

if (kv.second == "default")

The braces can be omitted.

lld/test/ELF/startstop-visibility.s

30

Use -o /dev/null if the output is not used.

[MaskRay]

LGTM. @pcc and @ruiu objected to this patch. Please give some time for them to make comments. Friday is a no meeting day for some of us, so waiting til next week will be nice.

(I guess @grimar may want you to use different output files for different RUN lines.)

[phosek]

In D55682#2099923, @MaskRay wrote:

LGTM. @pcc and @ruiu objected to this patch. Please give some time for them to make comments. Friday is a no meeting day for some of us, so waiting til next week will be nice.

SGTM thanks! I'm also fine waiting until the GNU change lands.

(I guess @grimar may want you to use different output files for different RUN lines.)

Using different output files is fine with me.

[phosek]

In D55682#2099972, @phosek wrote:

In D55682#2099923, @MaskRay wrote:

LGTM. @pcc and @ruiu objected to this patch. Please give some time for them to make comments. Friday is a no meeting day for some of us, so waiting til next week will be nice.

SGTM thanks! I'm also fine waiting until the GNU change lands.

Never mind, looks like it already landed in https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=cae64165f47b64898c4f1982d294862cfae89a47

[grimar]

In D55682#2099972, @phosek wrote:

In D55682#2099923, @MaskRay wrote:

(I guess @grimar may want you to use different output files for different RUN lines.)

Using different output files is fine with me.

Thanks! It really makes debugging failing test cases simpler when temporarily files are created.

[MaskRay]

(It is also my personal preference: we can use # as the comment marker. It is simpler. I'll not enforce that, though. When you write comments which are not RUN or CHECK lines, you can write ## . It makes comments stand out and avoids caveats for while some FileCheck developers want to prevent)

[phosek]

In D55682#2101819, @MaskRay wrote:

(It is also my personal preference: we can use # as the comment marker. It is simpler. I'll not enforce that, though. When you write comments which are not RUN or CHECK lines, you can write ## . It makes comments stand out and avoids caveats for while some FileCheck developers want to prevent)

Done. I haven't heard any further comments so I'll go ahead and land this change.

[MaskRay]

LGTM.

lld/ELF/Config.h

struct Configuration {
   bool zNodelete;
   bool zNodlopen;
   bool zNow;
   bool zOrigin;
   bool zPacPlt;
   bool zRelro;
   bool zRodynamic;
   bool zShstk;
+  uint8_t zStartStopVisibility;
   bool zText;
   bool zRetpolineplt;
   bool zWxneeded;
   DiscardPolicy discard;
   GnuStackKind zGnustack;
   ICFLevel icf;
   OrphanHandlingPolicy orphanHandling;
   SortSectionPolicy sortSection;

lld/ELF/Driver.cpp

if (StringRef("noexecstack") == arg->getValue())
       return GnuStackKind::NoExec;
     if (StringRef("nognustack") == arg->getValue())
       return GnuStackKind::None;
   }
 
   return GnuStackKind::NoExec;
 }
 
+static uint8_t getZStartStopVisibility(opt::InputArgList &args) {
+  for (auto *arg : args.filtered_reverse(OPT_z)) {
+    std::pair<StringRef, StringRef> kv = StringRef(arg->getValue()).split('=');
+    if (kv.first == "start-stop-visibility") {
+      if (kv.second == "default")
+        return STV_DEFAULT;
+      else if (kv.second == "internal")
+        return STV_INTERNAL;
+      else if (kv.second == "hidden")
+        return STV_HIDDEN;
+      else if (kv.second == "protected")
+        return STV_PROTECTED;
+      error("unknown -z start-stop-visibility= value: " + StringRef(kv.second));
+    }
+  }
+  return STV_PROTECTED;
+}
+
 static bool isKnownZFlag(StringRef s) {
   return s == "combreloc" || s == "copyreloc" || s == "defs" ||
          s == "execstack" || s == "force-bti" || s == "force-ibt" ||
          s == "global" || s == "hazardplt" || s == "ifunc-noplt" ||
          s == "initfirst" || s == "interpose" ||
          s == "keep-text-section-prefix" || s == "lazy" || s == "muldefs" ||
          s == "separate-code" || s == "separate-loadable-segments" ||
          s == "nocombreloc" || s == "nocopyreloc" || s == "nodefaultlib" ||
          s == "nodelete" || s == "nodlopen" || s == "noexecstack" ||
          s == "nognustack" || s == "nokeep-text-section-prefix" ||
          s == "norelro" || s == "noseparate-code" || s == "notext" ||
          s == "now" || s == "origin" || s == "pac-plt" || s == "rel" ||
          s == "rela" || s == "relro" || s == "retpolineplt" ||
          s == "rodynamic" || s == "shstk" || s == "text" || s == "undefs" ||
          s == "wxneeded" || s.startswith("common-page-size=") ||
-         s.startswith("max-page-size=") || s.startswith("stack-size=");
+         s.startswith("max-page-size=") || s.startswith("stack-size=") ||
+         s.startswith("start-stop-visibility=");
 }
 
 // Report an error for an unknown -z option.
 static void checkZOptions(opt::InputArgList &args) {
   for (auto *arg : args.filtered(OPT_z))
     if (!isKnownZFlag(arg->getValue()))
       error("unknown -z value: " + StringRef(arg->getValue()));
 }
static void readConfigs(opt::InputArgList &args) {
   config->zOrigin = hasZOption(args, "origin");
   config->zPacPlt = hasZOption(args, "pac-plt");
   config->zRelro = getZFlag(args, "relro", "norelro", true);
   config->zRetpolineplt = hasZOption(args, "retpolineplt");
   config->zRodynamic = hasZOption(args, "rodynamic");
   config->zSeparate = getZSeparate(args);
   config->zShstk = hasZOption(args, "shstk");
   config->zStackSize = args::getZOptionValue(args, OPT_z, "stack-size", 0);
+  config->zStartStopVisibility = getZStartStopVisibility(args);
   config->zText = getZFlag(args, "text", "notext", true);
   config->zWxneeded = hasZOption(args, "wxneeded");
 
   // Parse LTO options.
   if (auto *arg = args.getLastArg(OPT_plugin_opt_mcpu_eq))
     parseClangOption(saver.save("-mcpu=" + StringRef(arg->getValue())),
                      arg->getSpelling());
 

lld/ELF/Writer.cpp

 // the leading '.'), linkers are expected to define __start_<secname> and
 // __stop_<secname> symbols. They are at beginning and end of the section,
 // respectively. This is not requested by the ELF standard, but GNU ld and
 // gold provide the feature, and used by many programs.
 template <class ELFT>
 void Writer<ELFT>::addStartStopSymbols(OutputSection *sec) {
   StringRef s = sec->name;
   if (!isValidCIdentifier(s))
     return;
-  addOptionalRegular(saver.save("__start_" + s), sec, 0, STV_PROTECTED);
-  addOptionalRegular(saver.save("__stop_" + s), sec, -1, STV_PROTECTED);
+  addOptionalRegular(saver.save("__start_" + s), sec, 0,
+                     config->zStartStopVisibility);
+  addOptionalRegular(saver.save("__stop_" + s), sec, -1,
+                     config->zStartStopVisibility);
 }
 
 static bool needsPtLoad(OutputSection *sec) {
   if (!(sec->flags & SHF_ALLOC) || sec->noload)
     return false;
 
   // Don't allocate VA space for TLS NOBITS sections. The PT_TLS PHDR is
   // responsible for allocating space for them, not the PT_LOAD that

lld/test/ELF/startstop-visibility.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+
+# RUN: ld.lld %t.o -o %t
+# RUN: llvm-readelf -s %t | FileCheck %s
+
+# CHECK: 0 NOTYPE GLOBAL PROTECTED 2 __start_aaa
+# CHECK: 0 NOTYPE GLOBAL PROTECTED 2 __stop_aaa
+
+# RUN: ld.lld -z start-stop-visibility=default %t.o -o %t1
+# RUN: llvm-readelf -s %t1 | FileCheck --check-prefix=CHECK-DEFAULT %s
+
+# CHECK-DEFAULT: 0 NOTYPE GLOBAL DEFAULT 2 __start_aaa
+# CHECK-DEFAULT: 0 NOTYPE GLOBAL DEFAULT 2 __stop_aaa
+
+# RUN: ld.lld -z start-stop-visibility=internal %t.o -o %t2
+# RUN: llvm-readelf -s %t2 | FileCheck --check-prefix=CHECK-INTERNAL %s
+
+# CHECK-INTERNAL: 0 NOTYPE LOCAL INTERNAL 2 __start_aaa
+# CHECK-INTERNAL: 0 NOTYPE LOCAL INTERNAL 2 __stop_aaa
+
+# RUN: ld.lld -z start-stop-visibility=hidden %t.o -o %t3
+# RUN: llvm-readelf -s %t3 | FileCheck --check-prefix=CHECK-HIDDEN %s
+
+# CHECK-HIDDEN: 0 NOTYPE LOCAL HIDDEN 2 __start_aaa
+# CHECK-HIDDEN: 0 NOTYPE LOCAL HIDDEN 2 __stop_aaa
+
+# RUN: ld.lld -z start-stop-visibility=protected %t.o -o %t4
+# RUN: llvm-readelf -s %t4 | FileCheck --check-prefix=CHECK-PROTECTED %s
+
+# CHECK-PROTECTED: 0 NOTYPE GLOBAL PROTECTED 2 __start_aaa
+# CHECK-PROTECTED: 0 NOTYPE GLOBAL PROTECTED 2 __stop_aaa
+
+# RUN: not ld.lld -z start-stop-visibility=aaa %t.o -o /dev/null
+# CHECK-ERROR: error: unknown -z start-stop-visibility= value: aaa
+
+.quad __start_aaa
+.quad __stop_aaa
+.section aaa,"ax"
+
+.global _start
+.text
+_start:
+  nop