Page MenuHomePhabricator

lldProject
ActivePublic

Details

Description

LLVM Linker

Recent Activity

Fri, Oct 16

aganea added inline comments to D70378: [LLD][COFF] Cover usage of LLD as a library.
Fri, Oct 16, 4:03 PM · Restricted Project, lld, Restricted Project
rnk added inline comments to D70378: [LLD][COFF] Cover usage of LLD as a library.
Fri, Oct 16, 3:24 PM · Restricted Project, lld, Restricted Project
ikudrin added a comment to D88877: [ELF] Drop --whole-archive before processing dependent libraries..

Hmm, that starts looking like a discussion for the mailing list, but I guess that all interested people are already here and the conversation will be better preserved for further references if being linked to the review.

Fri, Oct 16, 3:03 AM · Restricted Project, lld

Thu, Oct 15

bd1976llvm added a comment to D88877: [ELF] Drop --whole-archive before processing dependent libraries..

Firstly, is this a real problem?

The problem is real. We have a report from our customer, so the issue is already observed in the wild.

In the RFC thread http://lists.llvm.org/pipermail/llvm-dev/2019-March/131105.html Rui said:

Then they can specify --no-whole-archive on the end of the command line, no?

Unfortunately, sometimes that is not that simple as it sounds. In our case, the users who need autolinking are different from a unit that provides tools to adjust command lines. There is no option to add anything at the very end.

Thu, Oct 15, 10:10 AM · Restricted Project, lld
ikudrin added inline comments to D88877: [ELF] Drop --whole-archive before processing dependent libraries..
Thu, Oct 15, 12:50 AM · Restricted Project, lld
ikudrin updated the diff for D88877: [ELF] Drop --whole-archive before processing dependent libraries..
Thu, Oct 15, 12:49 AM · Restricted Project, lld

Wed, Oct 14

ikudrin added a comment to D88877: [ELF] Drop --whole-archive before processing dependent libraries..

Firstly, is this a real problem?

Wed, Oct 14, 11:14 PM · Restricted Project, lld
bd1976llvm added a comment to D88877: [ELF] Drop --whole-archive before processing dependent libraries..
  • Simplified the comment.

Even if that was not decided in the discussion, I am pretty sure that --whole-archive cannot be used with dependent libraries, at least as the feature is implemented currently. The whole idea of the feature is that you can place #pragma comment(lib,...) in each and every source file which depends on the library. This results in multiple references to the library from many input object files, and if --whole-archive is active, this leads the linker to fail with the "duplicate symbol" error.

Wed, Oct 14, 6:11 PM · Restricted Project, lld
avl updated the diff for D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

rebased, added command-line option --gc-debuginfo-no-odr.

Wed, Oct 14, 2:53 PM · debug-info, lld, Restricted Project
MaskRay added a comment to D88877: [ELF] Drop --whole-archive before processing dependent libraries..

I'm fine with bd1976llvm accepting when he is happy.

Wed, Oct 14, 9:20 AM · Restricted Project, lld
psmith added a comment to D88877: [ELF] Drop --whole-archive before processing dependent libraries..

I don't have any objections. My comment in the thread was more along the lines that dependent libraries could interact with whole-archive in hard to predict ways. I'm happy to have dependent libraries ignore whole-archive. I'm fine with MaskRay accepting when he is happy.

Wed, Oct 14, 9:04 AM · Restricted Project, lld
ikudrin added a comment to D88877: [ELF] Drop --whole-archive before processing dependent libraries..

Gently ping. Is there anything I can improve in the fix so that it is accepted?

Wed, Oct 14, 8:42 AM · Restricted Project, lld

Tue, Oct 13

aganea added a comment to D88348: [LLD][COFF] When using LLD-as-a-library, always prevent re-entrance on failures.
In D88348#2328425, @rnk wrote:

The code changes seem fine, but I haven't figured out why we need to sprinkle LLD_IN_TEST=1 for every LLD invocation we expect to error. What goes wrong exactly? I thought the point of your change is that, we *won't* re-run LLD when LLD_IN_TEST=2 after it reports an error.

Tue, Oct 13, 2:14 PM · Restricted Project, lld
rnk added a comment to D88348: [LLD][COFF] When using LLD-as-a-library, always prevent re-entrance on failures.

The code changes seem fine, but I haven't figured out why we need to sprinkle LLD_IN_TEST=1 for every LLD invocation we expect to error. What goes wrong exactly? I thought the point of your change is that, we *won't* re-run LLD when LLD_IN_TEST=2 after it reports an error.

Tue, Oct 13, 12:52 PM · Restricted Project, lld

Thu, Oct 8

aganea added a comment to D88348: [LLD][COFF] When using LLD-as-a-library, always prevent re-entrance on failures.

Ping! Any further comments?

Thu, Oct 8, 3:49 PM · Restricted Project, lld
ikudrin added a comment to D88944: [ELF] Skip repeated libraries..

I've made some tests and speculatively estimate the saving up to several seconds for a relatively large library (about 100000 symbols) which is referenced several hundred times. For me, that is noticeable.

Thu, Oct 8, 6:59 AM · lld, Restricted Project

Wed, Oct 7

mstorsjo closed D88991: [LLD] Ignore ELF tests when ld.lld defaults to MinGW.
Wed, Oct 7, 11:40 PM · Restricted Project, lld
mati865 added a comment to D88991: [LLD] Ignore ELF tests when ld.lld defaults to MinGW.

I wonder if it'd be possible to add some custom substitution in the lit config, to have it replace ld.lld with ld.lld -m <some-elf> in this case...

Wed, Oct 7, 3:05 PM · Restricted Project, lld
MaskRay accepted D88991: [LLD] Ignore ELF tests when ld.lld defaults to MinGW.

Looks sensible to me, but I'd defer to @MaskRay to approve.

I wonder if it'd be possible to add some custom substitution in the lit config, to have it replace ld.lld with ld.lld -m <some-elf> in this case... Or does that fail if -m is explicitly specified but turns out to be the wrong architecture compared to what the test actually does?

Wed, Oct 7, 2:32 PM · Restricted Project, lld
mstorsjo added a comment to D88991: [LLD] Ignore ELF tests when ld.lld defaults to MinGW.

Looks sensible to me, but I'd defer to @MaskRay to approve.

Wed, Oct 7, 2:05 PM · Restricted Project, lld
mati865 requested review of D88991: [LLD] Ignore ELF tests when ld.lld defaults to MinGW.
Wed, Oct 7, 11:03 AM · Restricted Project, lld
MaskRay added a comment to D88944: [ELF] Skip repeated libraries..

I am skeptical about the change.

  • The duplicated libraries are usually indicator of user errors or build system brittleness.

The provided optimization is aimed to slightly improve the dependent libraries' case, where lots (hundreds) of references is very common. In a regular link, its impact is not expected to be noticeable.

  • The current way the state is introduced is incompatible with the direction on LLD-as-a-library (D88348) (This can be fixed).

Do you mean that addedLibraries is not reset on reenter? If so, the same is true for other members of LinkerDriver as well, so the patch does not make the situation worse.

  • The --verbose now diverges from GNU ld and gold. (This is usually a tie-breaker instead of a decisive reason.)

Well, we should not imitate GNU linkers in every nuance, especially in the internals, aren't we?

  • When duplicated libraries happen, the archive index can guarantee that the performance is not degraded.

The more repeated libraries referenced, the more time and memory the optimization saves. Even reading and basically parsing the archive files takes some time, and if there are hundreds of them, the improvement becomes noticeable. Of course, saving is not huge, but why not have it if the implementation is cheap?

Wed, Oct 7, 8:34 AM · lld, Restricted Project

Tue, Oct 6

ikudrin added a comment to D88944: [ELF] Skip repeated libraries..

I am skeptical about the change.

  • The duplicated libraries are usually indicator of user errors or build system brittleness.
Tue, Oct 6, 11:59 PM · lld, Restricted Project
ikudrin updated the diff for D88877: [ELF] Drop --whole-archive before processing dependent libraries..
  • Simplified the comment.
Tue, Oct 6, 11:26 PM · Restricted Project, lld
MaskRay requested changes to D88944: [ELF] Skip repeated libraries..

I am skeptical about the change.

Tue, Oct 6, 10:38 PM · lld, Restricted Project
MaskRay updated subscribers of D88877: [ELF] Drop --whole-archive before processing dependent libraries..

In the thread "[llvm-dev] RFC: ELF Autolinking", @psmith said:

Tue, Oct 6, 10:29 PM · Restricted Project, lld
ikudrin requested review of D88944: [ELF] Skip repeated libraries..
Tue, Oct 6, 10:04 PM · lld, Restricted Project
ikudrin updated the diff for D88877: [ELF] Drop --whole-archive before processing dependent libraries..
  • Fixed the comment.
Tue, Oct 6, 9:45 PM · Restricted Project, lld
ikudrin added inline comments to D88877: [ELF] Drop --whole-archive before processing dependent libraries..
Tue, Oct 6, 9:39 PM · Restricted Project, lld
MaskRay added inline comments to D88877: [ELF] Drop --whole-archive before processing dependent libraries..
Tue, Oct 6, 4:44 PM · Restricted Project, lld
ikudrin requested review of D88877: [ELF] Drop --whole-archive before processing dependent libraries..
Tue, Oct 6, 2:12 AM · Restricted Project, lld

Fri, Oct 2

aganea updated the diff for D88348: [LLD][COFF] When using LLD-as-a-library, always prevent re-entrance on failures.

Address comments. Removed stdout/stderr buffering, and replace with env LLD_IN_TEST=1 set appropriately in tests, where needed.

Fri, Oct 2, 6:24 AM · Restricted Project, lld

Tue, Sep 29

ayrivera added inline comments to D70378: [LLD][COFF] Cover usage of LLD as a library.
Tue, Sep 29, 7:58 AM · Restricted Project, lld, Restricted Project

Mon, Sep 28

aganea added a comment to D88348: [LLD][COFF] When using LLD-as-a-library, always prevent re-entrance on failures.

I have seen this in a language server. It seems that CrashRecoveryContext does not (correctly) use sigaltstack(). With sigaltstack, we can recover after a stack overflow.

Good to know at least it could work on Linux. I wanted to activate coverage for this at some point in D74229 but then it wasn't working on Debug builds on Windows. I'll need to get back to it.

For the stack overflow you observed, how did that happen?

The stack overflow happens in the first place somewhere in the Windows CRT while releasing heap blocks. This is rare, as it depends on the random pattern that was in memory at the location of ObjFile::dwarf when fatal() was called by ELFFileBase::init().

Mon, Sep 28, 6:16 PM · Restricted Project, lld
aganea added inline comments to D70378: [LLD][COFF] Cover usage of LLD as a library.
Mon, Sep 28, 6:03 PM · Restricted Project, lld, Restricted Project
MaskRay added a comment to D88348: [LLD][COFF] When using LLD-as-a-library, always prevent re-entrance on failures.

Since catching stack overflows is not a reliable scenario when using CrashRecoveryContext

Mon, Sep 28, 5:57 PM · Restricted Project, lld
ayrivera added inline comments to D70378: [LLD][COFF] Cover usage of LLD as a library.
Mon, Sep 28, 5:52 PM · Restricted Project, lld, Restricted Project
MaskRay added inline comments to D70378: [LLD][COFF] Cover usage of LLD as a library.
Mon, Sep 28, 5:44 PM · Restricted Project, lld, Restricted Project
ayrivera added inline comments to D70378: [LLD][COFF] Cover usage of LLD as a library.
Mon, Sep 28, 5:22 PM · Restricted Project, lld, Restricted Project
rnk added a comment to D88348: [LLD][COFF] When using LLD-as-a-library, always prevent re-entrance on failures.

I think I'm OK with the functional change, but I think I would prefer not to keep the code that buffers stdout/stderr for the lit test. I think the complexity/size cost outweighs the test benefit.

Mon, Sep 28, 5:19 PM · Restricted Project, lld

Fri, Sep 25

aganea updated the diff for D88348: [LLD][COFF] When using LLD-as-a-library, always prevent re-entrance on failures.

Diff with context.

Fri, Sep 25, 6:00 PM · Restricted Project, lld
aganea requested review of D88348: [LLD][COFF] When using LLD-as-a-library, always prevent re-entrance on failures.
Fri, Sep 25, 5:56 PM · Restricted Project, lld

Thu, Sep 24

aganea closed D70378: [LLD][COFF] Cover usage of LLD as a library.
Thu, Sep 24, 12:09 PM · Restricted Project, lld, Restricted Project
avl added inline comments to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..
Thu, Sep 24, 1:26 AM · debug-info, lld, Restricted Project

Wed, Sep 23

rnk accepted D70378: [LLD][COFF] Cover usage of LLD as a library.

Looks good to me, I didn't review very in depth, but I see the test case that we need. :)

Wed, Sep 23, 4:17 PM · Restricted Project, lld, Restricted Project
ayermolo added inline comments to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..
Wed, Sep 23, 2:58 PM · debug-info, lld, Restricted Project

Tue, Sep 22

mgorny added a comment to D85278: [lld] Support building shared libLLD.so.

Ping.

Tue, Sep 22, 2:48 PM · lld

Mon, Sep 21

amccarth accepted D70378: [LLD][COFF] Cover usage of LLD as a library.

LGTM.

Mon, Sep 21, 8:37 AM · Restricted Project, lld, Restricted Project

Sun, Sep 20

MaskRay added a comment to D70378: [LLD][COFF] Cover usage of LLD as a library.

@rnk @amccarth Do you have more comments? ☺️

Sun, Sep 20, 10:38 AM · Restricted Project, lld, Restricted Project

Sep 18 2020

MaskRay added a comment to D87418: [LLD] Allow configuring default ld.lld backend.

Pushed this one now.

Just FTR, keep in mind that if LLD is configured this way, most existing ELF tests will fail, as they don't pass any explicit -m parameter. Not sure if it's worth adding such an option to all of them, or just require building with the option in the default state if one wants to work on LLD and actually run tests.

Sep 18 2020, 10:24 AM · Restricted Project, lld