[llvm-dev] LLD: Can we make --warn-backrefs the default?

[Fāng-ruì Sòng via llvm-dev]

On 2020-09-03, Fāng-ruì Sòng wrote:
>On 2020-09-03, Peter Collingbourne wrote:
>>On Tue, Sep 1, 2020 at 5:35 PM Fāng-ruì Sòng via llvm-dev <
>>llvm-dev at lists.llvm.org> wrote:
>>
>>>On 2020-09-01, Petr Hosek wrote:
>>>>I see the GNU ld behavior as a limitation, not as a feature, as Peter
>>>Smith
>>>>also pointed out in https://reviews.llvm.org/D86762. While it can be
>>>argued
>>>>that there are certain cases where it can help detect layering
>>>>violations as you mentioned in your change, I'm not sure how valuable that
>>>>is in practice. Every case I've encountered so far either in Chrome or in
>>>>Fuchsia was a valid use case, most commonly interceptors. The solution
>>>>has always been the same, wrap all libraries in --start-group/--stop-group
>>>>and it's what most projects do by default to avoid dealing with these
>>>>issues, see for example [Chromium](
>>>>
>>>https://source.chromium.org/chromium/chromium/src/+/master:build/toolchain/gcc_toolchain.gni;l=409
>>>).
>>>>In our case, compatibility with linkers on other platforms is more
>>>>important than compatibility with GNU ld, so I'd prefer to keep the
>>>current
>>>>behavior. Projects that care about compatibility with GNU ld can use
>>>>--warn-backrefs.
>>>
>>>I totally understand that some users may not want to deal with GNU ld
>>>compatibility:) I'll then question about Chromium's addition of -z defs:
>>>https://crrev.com/843583006 :)
>>>
>>>-z defs is like a layering checking tool for shared objects while
>>>--warn-backrefs is for archives. For performance, ABI concerns and ease
>>>of deployment, many projects tend to build their own components as
>>>archives instead of shared objects. In this sense --warn-backrefs will
>>>probably be more useful than -z defs.
>>>
>>>(
>>>TIL lorder and tsort were created to define an order of archives in
>>>early versions of Unix.
>>>https://www.gnu.org/software/coreutils/manual/html_node/tsort-background.html
>>>It seems that the article missed the point that proper library layering is
>>>still useful
>>>)
>>>
>>
>>I'm not a fan of this idea of reframing GNU ld behavior as a "layering
>>checking tool". It is an incomplete layering checking tool because it does
>>not detect the scenario where, for example, you have the intended
>>dependency graph:
>>
>>A -> B
>>A -> C
>>B -> D
>>C -> D
>>
>>(resulting in -la -lb -lc -ld) and you have an unexpected dependency B ->
>>C.
>
>Yes, the GNU ld layering checking behavior is incomplete (yet important
>and sufficient if we aim for compatibility).
>
>The build system pick two orders:
>-la -lb -lc -ld
>-la -lc -lb -ld
>
>>There is already a way to detect layering problems, that detects
>>practical layering problems and not just theoretical ones, which is to link
>>programs that use subsets of the libraries. For example, linking a program
>>that depends only on B would result in detecting the invalid B -> C
>>dependency.
>
>This is actually cumbersome and is explicitly described in https://reviews.llvm.org/D86762
>
>If B -> C is not specified,
>
>* If people write B_test ("linking a program that depends only on B"),
>  they will notice the dependency issue immediately via the "undefined symbol" diagnostic.
>* If such a B_test does not exist. The user may work on a large
>  application which depends on B (and transitively on D) but not on A.
>  OK, they will get an undefined symbol from C. However, it is not
>  appropriate for them to add a dependency on C because they don't use C
>  directly. See the "If adding the dependency does not form a cycle:"
>  paragraph in D86762.

Supplement: say the application is E, we have the following dependency
set and the link (lD -lB -lD) fail due to an undefined symbol
defined by C (because of the unspecified dependency from B to C):

E -> B
B -> D
C -> D

After adding E -> C, there is chance that --warn-backrefs will still
warn, because of a backward reference from B to C (-lD -lC -lB -lD). The
order topological order (-lD -lB -lC -lD) is good and accepted by GNU ld.
In the warning case, it will give the user actionable feedback on adding
the dependency at the appropriate level (B -> C).
My idea is that this is good enough and works well in practice
(compatible with GNU linkers).