llvm dev - Sep 2020 - LLD: Can we make --warn-backrefs the default?

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
)
>On Mon, Aug 31, 2020 at 1:44 PM Fāng-ruì Sòng <maskray at google.com>
wrote:
>
>> On Mon, Aug 31, 2020 at 1:29 PM David Blaikie <dblaikie at
gmail.com> wrote:
>> >
>> >
>> >
>> > On Mon, Aug 31, 2020 at 1:24 PM Fāng-ruì Sòng <maskray at
google.com>
>> wrote:
>> >>
>> >> On Fri, Aug 28, 2020 at 11:16 AM David Blaikie <dblaikie at
gmail.com>
>> wrote:
>> >> >
>> >> > Would you like to conduct the conversation here, or on
the review
>> thread? (I lean towards having them here, but don't mind if folks
feel like
>> it keeps the noise down & want to more post a notice saying
"hey, here's
>> this thing, if you're interested, go discuss it over there" -
more an
>> optional opt-in rather than requiring people to opt-out via muting the
>> thread, etc)
>> >>
>> >> Yes, we can conduct the "should we enable --warn-backrefs
by default"
>> >> conversation here. Since the semantics --warn-backrefs of are
a bit
>> >> complex, we need a documentation.
https://reviews.llvm.org/D86762 is
>> >> put up to get wording suggestions. Explicitly adding the
people to the
>> >> CC list...
>> >>
>> >> FWIW for many code bases, --warn-backrefs should produce no
warnings
>> >> (error if --fatal-warnings). For some code bases, GNU ld may
error
>> >> "undefined reference".  --warn-backrefs can catch
such problems.
>> >
>> >
>> > One of the questions raised on the thread there was about
different
>> linker semantics. I assume the "--warn-backrefs by default"
we're
>> discussing is only related to the ld.lld frontend? Not the Windows
linker
>> lld behavior (or ld64 (old or new) lld behavior)?
>>
>> Yes, the ELF port (lld/ELF). No other port has implemented
>> --warn-backrefs. The intion of --warn-backrefs is to capture behavior
>> differences with GNU ld's ELF ports. If traditional ELF linkers
don't
>> behave like that, I can replace "traditional ELF linkers" in
>> https://reviews.llvm.org/D86762 with "GNU linkers".
>>
>> >>
>> >>
>> >>
>> >> > On Thu, Aug 27, 2020 at 10:15 PM Fangrui Song via
llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>> >> >>
>> >> >> Hi all, LLD's --warn-backrefs is a tool to
identify potential
>> >> >> incompatible archive selection semantics with
traditional Unix
>> linkers.
>> >> >> I have improved it (via D77522,D77630 and D77512) to
a state where a
>> >> >> --warn-backrefs diagnostic almost assuredly means
that the link will
>> >> >> fail with GNU ld, or the symbol will get different
resolution in GNU
>> ld and LLD.
>> >> >>
>> >> >> My conclusion is that --warn-backrefs is a very
useful layering
>> check tool.
>> >> >> I just wrote a documentation about the advantage (of
GNU ld's archive
>> >> >> selection semantics..... But we can do better with
--warn-backrefs!
>> >> >> GNU ld just reports "undefined reference"
with no actionable feedback
>> >> >> about the offending archive)
>> >> >>
>> >> >> https://reviews.llvm.org/D86762
>> >> >>
>> >> >> I am wondering whether in the next release we can
make
>> --warn-backrefs
>> >> >> the default.  I have added many known users to the
review.
>> >> >> (There is no need for --no-warn-backrefs because
>> --warn-backrefs-exclude='*' does the same job)
>> >> >> _______________________________________________
>> >> >> LLVM Developers mailing list
>> >> >> llvm-dev at lists.llvm.org
>> >> >>
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>

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. 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. It's also worth noting that even that would only detect the
layering problem if the program depends on the part of B that depends on C.

A better way to go about achieving layering checking would IMHO be to
implement a separate tool (not part of the linker) that is capable of
a complete layering check. Such a tool would only depend on symbol table
features common to all object formats, so it could probably be implemented
generically.

Peter

> >On Mon, Aug 31, 2020 at 1:44 PM Fāng-ruì Sòng <maskray at
google.com> wrote:
> >
> >> On Mon, Aug 31, 2020 at 1:29 PM David Blaikie <dblaikie at
gmail.com>
> wrote:
> >> >
> >> >
> >> >
> >> > On Mon, Aug 31, 2020 at 1:24 PM Fāng-ruì Sòng <maskray at
google.com>
> >> wrote:
> >> >>
> >> >> On Fri, Aug 28, 2020 at 11:16 AM David Blaikie
<dblaikie at gmail.com>
> >> wrote:
> >> >> >
> >> >> > Would you like to conduct the conversation here, or
on the review
> >> thread? (I lean towards having them here, but don't mind if
folks feel
> like
> >> it keeps the noise down & want to more post a notice saying
"hey, here's
> >> this thing, if you're interested, go discuss it over
there" - more an
> >> optional opt-in rather than requiring people to opt-out via muting
the
> >> thread, etc)
> >> >>
> >> >> Yes, we can conduct the "should we enable
--warn-backrefs by default"
> >> >> conversation here. Since the semantics --warn-backrefs of
are a bit
> >> >> complex, we need a documentation.
https://reviews.llvm.org/D86762 is
> >> >> put up to get wording suggestions. Explicitly adding the
people to
> the
> >> >> CC list...
> >> >>
> >> >> FWIW for many code bases, --warn-backrefs should produce
no warnings
> >> >> (error if --fatal-warnings). For some code bases, GNU ld
may error
> >> >> "undefined reference".  --warn-backrefs can
catch such problems.
> >> >
> >> >
> >> > One of the questions raised on the thread there was about
different
> >> linker semantics. I assume the "--warn-backrefs by
default" we're
> >> discussing is only related to the ld.lld frontend? Not the Windows
> linker
> >> lld behavior (or ld64 (old or new) lld behavior)?
> >>
> >> Yes, the ELF port (lld/ELF). No other port has implemented
> >> --warn-backrefs. The intion of --warn-backrefs is to capture
behavior
> >> differences with GNU ld's ELF ports. If traditional ELF
linkers don't
> >> behave like that, I can replace "traditional ELF
linkers" in
> >> https://reviews.llvm.org/D86762 with "GNU linkers".
> >>
> >> >>
> >> >>
> >> >>
> >> >> > On Thu, Aug 27, 2020 at 10:15 PM Fangrui Song via
llvm-dev <
> >> llvm-dev at lists.llvm.org> wrote:
> >> >> >>
> >> >> >> Hi all, LLD's --warn-backrefs is a tool to
identify potential
> >> >> >> incompatible archive selection semantics with
traditional Unix
> >> linkers.
> >> >> >> I have improved it (via D77522,D77630 and
D77512) to a state
> where a
> >> >> >> --warn-backrefs diagnostic almost assuredly
means that the link
> will
> >> >> >> fail with GNU ld, or the symbol will get
different resolution in
> GNU
> >> ld and LLD.
> >> >> >>
> >> >> >> My conclusion is that --warn-backrefs is a very
useful layering
> >> check tool.
> >> >> >> I just wrote a documentation about the advantage
(of GNU ld's
> archive
> >> >> >> selection semantics..... But we can do better
with
> --warn-backrefs!
> >> >> >> GNU ld just reports "undefined
reference" with no actionable
> feedback
> >> >> >> about the offending archive)
> >> >> >>
> >> >> >> https://reviews.llvm.org/D86762
> >> >> >>
> >> >> >> I am wondering whether in the next release we
can make
> >> --warn-backrefs
> >> >> >> the default.  I have added many known users to
the review.
> >> >> >> (There is no need for --no-warn-backrefs because
> >> --warn-backrefs-exclude='*' does the same job)
> >> >> >> _______________________________________________
> >> >> >> LLVM Developers mailing list
> >> >> >> llvm-dev at lists.llvm.org
> >> >> >>
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >>
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

-- 
-- 
Peter
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200903/d3c1c657/attachment-0001.html>

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.

   If their application actually depends on A (thus they will get the
   dependency on C), their link may or may not succeed with GNU ld,
   depending on whether the build system picks
   -la -lb -lc -ld or -la -lc -lb -ld, and the diagnostic can be very
   confusing "undefined reference to" while C is actually linked.
>It's also worth noting that even that would only detect the
>layering problem if the program depends on the part of B that depends on C.
>A better way to go about achieving layering checking would IMHO be to
>implement a separate tool (not part of the linker) that is capable of
>a complete layering check. Such a tool would only depend on symbol table
>features common to all object formats, so it could probably be implemented
>generically.
>
>Peter
A standalone tool will not achieve sufficient ergonomics.
It will read all input files and duplicate the symbol resolution work done by
the linker,
which can be slow. (ld.lld --warn-backrefs only imposes negligible
overhead when a lazy object is fetched.)
>
>> >On Mon, Aug 31, 2020 at 1:44 PM Fāng-ruì Sòng <maskray at
google.com> wrote:
>> >
>> >> On Mon, Aug 31, 2020 at 1:29 PM David Blaikie <dblaikie at
gmail.com>
>> wrote:
>> >> >
>> >> >
>> >> >
>> >> > On Mon, Aug 31, 2020 at 1:24 PM Fāng-ruì Sòng <maskray
at google.com>
>> >> wrote:
>> >> >>
>> >> >> On Fri, Aug 28, 2020 at 11:16 AM David Blaikie
<dblaikie at gmail.com>
>> >> wrote:
>> >> >> >
>> >> >> > Would you like to conduct the conversation here,
or on the review
>> >> thread? (I lean towards having them here, but don't mind
if folks feel
>> like
>> >> it keeps the noise down & want to more post a notice
saying "hey, here's
>> >> this thing, if you're interested, go discuss it over
there" - more an
>> >> optional opt-in rather than requiring people to opt-out via
muting the
>> >> thread, etc)
>> >> >>
>> >> >> Yes, we can conduct the "should we enable
--warn-backrefs by default"
>> >> >> conversation here. Since the semantics
--warn-backrefs of are a bit
>> >> >> complex, we need a documentation.
https://reviews.llvm.org/D86762 is
>> >> >> put up to get wording suggestions. Explicitly adding
the people to
>> the
>> >> >> CC list...
>> >> >>
>> >> >> FWIW for many code bases, --warn-backrefs should
produce no warnings
>> >> >> (error if --fatal-warnings). For some code bases, GNU
ld may error
>> >> >> "undefined reference".  --warn-backrefs can
catch such problems.
>> >> >
>> >> >
>> >> > One of the questions raised on the thread there was about
different
>> >> linker semantics. I assume the "--warn-backrefs by
default" we're
>> >> discussing is only related to the ld.lld frontend? Not the
Windows
>> linker
>> >> lld behavior (or ld64 (old or new) lld behavior)?
>> >>
>> >> Yes, the ELF port (lld/ELF). No other port has implemented
>> >> --warn-backrefs. The intion of --warn-backrefs is to capture
behavior
>> >> differences with GNU ld's ELF ports. If traditional ELF
linkers don't
>> >> behave like that, I can replace "traditional ELF
linkers" in
>> >> https://reviews.llvm.org/D86762 with "GNU linkers".
>> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >> > On Thu, Aug 27, 2020 at 10:15 PM Fangrui Song
via llvm-dev <
>> >> llvm-dev at lists.llvm.org> wrote:
>> >> >> >>
>> >> >> >> Hi all, LLD's --warn-backrefs is a tool
to identify potential
>> >> >> >> incompatible archive selection semantics
with traditional Unix
>> >> linkers.
>> >> >> >> I have improved it (via D77522,D77630 and
D77512) to a state
>> where a
>> >> >> >> --warn-backrefs diagnostic almost assuredly
means that the link
>> will
>> >> >> >> fail with GNU ld, or the symbol will get
different resolution in
>> GNU
>> >> ld and LLD.
>> >> >> >>
>> >> >> >> My conclusion is that --warn-backrefs is a
very useful layering
>> >> check tool.
>> >> >> >> I just wrote a documentation about the
advantage (of GNU ld's
>> archive
>> >> >> >> selection semantics..... But we can do
better with
>> --warn-backrefs!
>> >> >> >> GNU ld just reports "undefined
reference" with no actionable
>> feedback
>> >> >> >> about the offending archive)
>> >> >> >>
>> >> >> >> https://reviews.llvm.org/D86762
>> >> >> >>
>> >> >> >> I am wondering whether in the next release
we can make
>> >> --warn-backrefs
>> >> >> >> the default.  I have added many known users
to the review.
>> >> >> >> (There is no need for --no-warn-backrefs
because
>> >> --warn-backrefs-exclude='*' does the same job)
>> >> >> >>
_______________________________________________
>> >> >> >> LLVM Developers mailing list
>> >> >> >> llvm-dev at lists.llvm.org
>> >> >> >>
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>> >>
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>
>
>-- 
>-- 
>Peter