[ELF] Hint -z nostart-stop-gc for __start_ undefined references

[MaskRay]

Summary

Make users aware what to do with ld.lld 13.0.0 / GNU ld<2015-10 --gc-sections
behavior.

[MaskRay]

change path

[MaskRay]

Suggest KEEP

[MaskRay]

start_stop_gc => start-stop-gc

I think we can flesh out the content later.
Having the message in the 13.0.1 code may help.

[MaskRay]

I am happy to discuss the wording for start-stop-gc.rst. Having the code in 13.0.1 is more important, though, so I am pushing it.

[smeenai]

This won't diagnose the case where the actual contents of the C-named section get dropped, which can only be caught at runtime. For example, our application of encapsulation symbols for library merging was relying on C-named sections being retained; the behavior change in LLD 13 didn't result in any link-time errors, but it caused run-time crashes because all the JNI_OnLoads were dropped. (It was straightforward for us to add __attribute__((retain)) because of how our builds are set up, so it wasn't a problem for us.) This is still a good change; just pointing out that it doesn't capture all possible cases which might change program behavior.

[dblaikie]

Generally once a patch is sent for review it should remain in review until approved, rather than committed without approval - so this patch should probably be reverted until approved?

[smeenai]

In D114830#3168865, @dblaikie wrote:

Generally once a patch is sent for review it should remain in review until approved, rather than committed without approval - so this patch should probably be reverted until approved?

Hmm, is that standard convention? I've observed a bunch of cases where contributors put up a review as an FYI or to get some additional thoughts, but then commit if there's no feedback or objections, for patches where they would have otherwise been comfortable committing directly for post-commit review.

[dblaikie]

In D114830#3168885, @smeenai wrote:

In D114830#3168865, @dblaikie wrote:

Generally once a patch is sent for review it should remain in review until approved, rather than committed without approval - so this patch should probably be reverted until approved?

Hmm, is that standard convention?

Generally - not sure if it's well documented, but I can probably find a few instances of this being sited over the years by folks other than myself.

I've observed a bunch of cases where contributors put up a review as an FYI or to get some additional thoughts, but then commit if there's no feedback or objections, for patches where they would have otherwise been comfortable committing directly for post-commit review.

Yeah, there's some wriggle room, especially for code owners (like @MaskRay in this case) who might be seeking feedback but not feel like they're blocked on receiving that feedback. In that case I think the best/necessary thing to do is to document that ahead of time in the initial review - that the review is seeking feedback, but does not require it to proceed. That way it doesn't end up looking like the initial review required feedback, but the patch was then committed without feedback because the author wasn't willing to wait for folks to give that feedback. (that's the main reason for this rule: To ensure reviewers don't feel rushed/authors don't use short timelines as a way to push things through without the necessary review)

[peter.smith]

I'm happy to approve in post; my apologies for not getting to it in time. Regardless of the opinion on the default value of start-stop-gc this particular change adds a useful warning and documentation.

As an aside Arm's proprietary linker introduced a "weakly live" value for sections referenced by the equivalent of __start and __stop. The linker would follow relocations from a "weakly live" section to see if it could reach an existing "live" section from the same object file, where "live" is the result of classic garbage collection assuming --start-stop-gc . This heuristic kind of worked for what we needed it do, which was to permit all exception tables (reachable via start and stop) to be thrown away if no object called throw. We didn't have any real rigorous theory to back up the heuristic though other than it was conservative enough to keep what we need to keep and gave good enough GC results in most cases. I'd not recommend LLD follow that though as it was a heuristic tuned to a system library.

[dblaikie]

In D114830#3169336, @peter.smith wrote:

I'm happy to approve in post; my apologies for not getting to it in time. Regardless of the opinion on the default value of start-stop-gc this particular change adds a useful warning and documentation.

As an aside Arm's proprietary linker introduced a "weakly live" value for sections referenced by the equivalent of __start and __stop. The linker would follow relocations from a "weakly live" section to see if it could reach an existing "live" section from the same object file, where "live" is the result of classic garbage collection assuming --start-stop-gc . This heuristic kind of worked for what we needed it do, which was to permit all exception tables (reachable via start and stop) to be thrown away if no object called throw. We didn't have any real rigorous theory to back up the heuristic though other than it was conservative enough to keep what we need to keep and gave good enough GC results in most cases. I'd not recommend LLD follow that though as it was a heuristic tuned to a system library.

Ah, thanks for weighing in. Sounds good to me!

[MaskRay]

In D114830#3168833, @smeenai wrote:

This won't diagnose the case where the actual contents of the C-named section get dropped, which can only be caught at runtime.
For example, our application of encapsulation symbols for library merging was relying on C-named sections being retained; the behavior change in LLD 13 didn't result in any link-time errors, but it caused run-time crashes because all the JNI_OnLoads were dropped. (It was straightforward for us to add __attribute__((retain)) because of how our builds are set up, so it wasn't a problem for us.) This is still a good change; just pointing out that it doesn't capture all possible cases which might change program behavior.

This won't diagnose the case when the __start_ reference is weak.
Among the few Debian Code Search examples, I cannot find undefined weak examples.
I know that Clang InstrProfiling uses undefined weak. Such examples are just rare.
For such cases, we have to rely on the developer to understand the problem. (The section just doesn't exist, so presumably not too difficult to troubleshoot.)

[MaskRay]

In D114830#3168986, @dblaikie wrote:

In D114830#3168885, @smeenai wrote:

In D114830#3168865, @dblaikie wrote:

Generally once a patch is sent for review it should remain in review until approved, rather than committed without approval - so this patch should probably be reverted until approved?

Hmm, is that standard convention?

Generally - not sure if it's well documented, but I can probably find a few instances of this being sited over the years by folks other than myself.

I've observed a bunch of cases where contributors put up a review as an FYI or to get some additional thoughts, but then commit if there's no feedback or objections, for patches where they would have otherwise been comfortable committing directly for post-commit review.

Yeah, there's some wriggle room, especially for code owners (like @MaskRay in this case) who might be seeking feedback but not feel like they're blocked on receiving that feedback. In that case I think the best/necessary thing to do is to document that ahead of time in the initial review - that the review is seeking feedback, but does not require it to proceed. That way it doesn't end up looking like the initial review required feedback, but the patch was then committed without feedback because the author wasn't willing to wait for folks to give that feedback. (that's the main reason for this rule: To ensure reviewers don't feel rushed/authors don't use short timelines as a way to push things through without the necessary review)

Ah, ok. Rushed on this because the combination of (a) the the code change would be useful for 13.0.1 and the code was the final form and the documentation page can evolve separately as I mentioned..
(b) this was (clearly as I think) what the release manager and some folks wanted.
(I was on the plane for many hours today. Pushed this so that there was a commit id with which the release manager knew what to cherry pick)

In D114830#3169336, @peter.smith wrote:

I'm happy to approve in post; my apologies for not getting to it in time. Regardless of the opinion on the default value of start-stop-gc this particular change adds a useful warning and documentation.

Thanks!

lld/ELF/Relocations.cpp

if (const Symbol *corrected = getAlternativeSpelling<ELFT>(
         msg += "\n>>> defined in: " + toString(corrected->file);
     }
   }
 
   if (sym.getName().startswith("_ZTV"))
     msg +=
         "\n>>> the vtable symbol may be undefined because the class is missing "
         "its key function (see https://lld.llvm.org/missingkeyfunction)";
+  if (config->gcSections && config->zStartStopGC &&
+      sym.getName().startswith("__start_")) {
+    msg += "\n>>> the encapsulation symbol needs to be retained under "
+           "--gc-sections properly; consider -z nostart-stop-gc "
+           "(see https://lld.llvm.org/ELF/start-stop-gc)";
+  }
 
   if (undef.isWarning)
     warn(msg);
   else
     error(msg, ErrorTag::SymbolNotFound, {sym.getName()});
 }
 
 template <class ELFT> void elf::reportUndefinedSymbols() {

lld/docs/ELF/start-stop-gc.rst

+-z start-stop-gc
+================
+
+If your ``-Wl,--gc-sections`` build fail with a linker error like this:
+
+    error: undefined symbol: __start_meta
+    >>> referenced by {{.*}}
+    >>> the encapsulation symbol needs to be retained under --gc-sections properly; consider -z nostart-stop-gc (see https://lld.llvm.org/start-stop-gc) 
+
+it is likely your C identifier name sections are not properly annotated to
+suffice under ``--gc-sections``.
+
+``__start_meta`` and ``__stop_meta`` are sometimed called encapsulation
+symbols. In October 2015, GNU ld switched behavior and made a ``__start_meta``
+reference from a live section retain all ``meta`` input sections. This
+conservative behavior works for existing code which does not take GC into fair
+consideration, but unnecessarily increases sizes for modern metadata section
+usage which desires precise GC.
+
+GNU ld 2.37 added ``-z start-stop-gc`` to restore the traditional behavior
+ld.lld 13.0.0 defaults to ``-z start-stop-gc`` and supports ``-z nostart-stop-gc``
+to switch to the conservative behavior.
+
+The Apple ld64 linker has a similar ``section$start`` feature and always
+allowed GC (like ``-z start-stop-gc``).
+
+Annotate C identifier name sections
+-----------------------------------
+
+A C identifier name section (``meta``) sometimes depends on another section.
+Let that section reference ``meta`` via a relocation.
+
+.. code-block:: c
+
+  asm(".pushsection .init_array,\"aw\",@init_array\n" \
+      ".reloc ., R_AARCH64_NONE, meta\n"              \
+      ".popsection\n")
+
+If a relocation is inconvenient, consider using ``__attribute__((retain))``
+(GCC 11 with modern binutils, Clang 13).
+
+.. code-block:: c
+
+  #pragma GCC diagnostic push
+  #pragma GCC diagnostic ignored "-Wattributes"
+  __attribute__((retain,used,section("meta")))
+  static const char dummy[0];
+  #pragma GCC diagnostic pop
+
+GCC before 11 and Clang before 13 do not recognize ``__attribute__((retain))``,
+so ``-Wattributes`` may need to be ignored. On ELF targets,
+``__attribute__((used))`` prevents compiler discarding, but does not affect
+linker ``--gc-sections``.
+
+In a macro, you may use:
+
+.. code-block:: c
+
+  _Pragma("GCC diagnostic push")
+  _Pragma("GCC diagnostic ignored \"-Wattributes\"")
+  ...
+  _Pragma("GCC diagnostic pop")
+
+If you use the ``SECTIONS`` command in a linker script, use
+`the ``KEEP`` keyword <https://sourceware.org/binutils/docs/ld/Input-Section-Keep.html>`_, e.g.
+``meta : { KEEP(*(meta)) }``

lld/docs/index.rst

.. toctree::
    NewLLD
    WebAssembly
    windows_support
    missingkeyfunction
    error_handling_script
    Partitions
    ReleaseNotes
    ELF/linker_script
+   ELF/start-stop-gc
    ELF/warn_backrefs

lld/test/ELF/gc-sections-startstop-hint.s

+# REQUIRES: x86
+## Some projects may not work with GNU ld<2015-10 (ld.lld 13.0.0) --gc-sections behavior.
+## Give a hint.
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: ld.lld %t.o -o /dev/null
+# RUN: ld.lld %t.o --gc-sections -z nostart-stop-gc -o /dev/null
+# RUN: not ld.lld %t.o --gc-sections -o /dev/null 2>&1 | FileCheck %s
+
+# CHECK:      error: undefined symbol: __start_meta
+# CHECK-NEXT: >>> referenced by {{.*}}
+# CHECK-NEXT: >>> the encapsulation symbol needs to be retained under --gc-sections properly; consider -z nostart-stop-gc (see https://lld.llvm.org/ELF/start-stop-gc)
+
+.section .text,"ax",@progbits
+.global _start
+_start:
+  .quad __start_meta - .
+  .quad __stop_meta - .
+
+.section meta,"aw",@progbits
+.quad 0