Page MenuHomePhabricator

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

Authored by phosek on Dec 13 2018, 3:40 PM.

Details

Summary

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

Diff Detail

Event Timeline

phosek created this revision.Dec 13 2018, 3:40 PM

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

grimar added a subscriber: grimar.Dec 14 2018, 12:24 AM

-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 added a comment.Dec 14 2018, 1:47 PM

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

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

Can you raise one example in the description?

pcc added a subscriber: pcc.Dec 14 2018, 9:10 PM

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.

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 marked an inline comment as done.Dec 19 2018, 11:59 AM
phosek added inline comments.
lld/ELF/Writer.cpp
2231–2235

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

phosek edited the summary of this revision. (Show Details)Dec 19 2018, 12:03 PM

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.

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 added a comment.Dec 19 2018, 1:40 PM

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 added a comment.Jun 2 2020, 1:22 PM

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.

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.

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.

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

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

MaskRay added a comment.EditedJun 15 2020, 2:36 PM

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 updated this revision to Diff 271483.Jun 17 2020, 2:16 PM
phosek marked an inline comment as done.
phosek retitled this revision from [ELF] Support defining __start/__stop symbols as hidden to [ELF] Support for setting __start/__stop symbol visibility.
phosek edited the summary of this revision. (Show Details)
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2020, 2:16 PM

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.

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.

phosek updated this revision to Diff 271565.Jun 17 2020, 8:47 PM
phosek marked 2 inline comments as done.
phosek retitled this revision from [ELF] Support for setting __start/__stop symbol visibility to [ELF] Add -z start-stop-visibility= to set __start_/__stop_ symbol visibility.
MaskRay accepted this revision.EditedJun 17 2020, 10:10 PM

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.)

This revision is now accepted and ready to land.Jun 17 2020, 10:10 PM
phosek updated this revision to Diff 271585.Jun 17 2020, 11:29 PM

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.

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

(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 added a comment.EditedJun 18 2020, 12:50 PM

(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 updated this revision to Diff 272846.Jun 23 2020, 3:33 PM

(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 accepted this revision.Jun 23 2020, 3:45 PM

LGTM.

This revision was automatically updated to reflect the committed changes.