Page MenuHomePhabricator

lldProject
ActivePublic

Watchers

  • This project does not have any watchers.

Details

Description

LLVM Linker

Recent Activity

Today

MaskRay added a comment to D77330: Consider increasing the default ARM32 page size to 64k..

I am still in favor of this change:

Tue, Apr 7, 2:10 PM · Restricted Project, lld
MaskRay added inline comments to D68065: Propeller: LLD Support for Basic Block Sections.
Tue, Apr 7, 12:31 PM · Restricted Project, lld
thieta added a comment to D77330: Consider increasing the default ARM32 page size to 64k..

A small ping here.

Tue, Apr 7, 8:07 AM · Restricted Project, lld
tmsriram added inline comments to D68065: Propeller: LLD Support for Basic Block Sections.
Tue, Apr 7, 7:34 AM · Restricted Project, lld
tmsriram closed D68065: Propeller: LLD Support for Basic Block Sections.
Tue, Apr 7, 7:02 AM · Restricted Project, lld
grimar added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

Few more comments. They are mostly to give a feedback about the code style, I have not look deep into how it works yet.

Tue, Apr 7, 3:45 AM · debug-info, lld, Restricted Project

Yesterday

MaskRay added inline comments to D68065: Propeller: LLD Support for Basic Block Sections.
Mon, Apr 6, 9:16 PM · Restricted Project, lld
ruiu accepted D68065: Propeller: LLD Support for Basic Block Sections.

I had a VC meeting wtih maskray to discuss concerns he has, and he is ok with this patch. I'm fine with this patch too. I'll give a final LGTM to unblock you. Please make the following changes and submit. Thanks!

Mon, Apr 6, 8:44 PM · Restricted Project, lld
Herald added a reviewer for D67876: Add support for using pass plugins from lld: MaskRay.
Mon, Apr 6, 4:55 PM · lld, Restricted Project
avl added inline comments to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..
Mon, Apr 6, 10:51 AM · debug-info, lld, Restricted Project
grimar added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

I've put a few first general comments and suggestions inline.

Mon, Apr 6, 4:50 AM · debug-info, lld, Restricted Project
avl added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

One drive-by request/question: have you done any measurements of the impact on debuggers? In other words, does it improve/make worse the time it takes them to load the debug data?

Mon, Apr 6, 4:49 AM · debug-info, lld, Restricted Project
avl added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

@dblaikie

This sort of complex design discussion probably merits some in person or video chat discussions or at least lengthy design discussion on llvm-dev.

Mon, Apr 6, 4:49 AM · debug-info, lld, Restricted Project
jhenderson updated subscribers of D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

One drive-by request/question: have you done any measurements of the impact on debuggers? In other words, does it improve/make worse the time it takes them to load the debug data?

Mon, Apr 6, 1:03 AM · debug-info, lld, Restricted Project

Sun, Apr 5

dblaikie added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

Rather than replying point-by-point, since I think the conversation has got a bit jumbled up - and maybe it's all best left to a separate thread than in this review... but:

Sun, Apr 5, 11:44 AM · debug-info, lld, Restricted Project

Sat, Apr 4

avl added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

To the best of my knowledge (as the person who implemented type unit support in LLVM... ) type units never reduce the size of object files - they only increase it.

Sat, Apr 4, 9:32 AM · debug-info, lld, Restricted Project
rprichard updated subscribers of D77330: Consider increasing the default ARM32 page size to 64k..

So it turns out that the ld shipped in the NDK actually did have the 64KB max-page-size patch, so not taking this patch would change current user behavior. Thanks to @MaskRay for sharing the exact binutils patch we were looking for to verify that it has it. For that reason, this patch should be fine for our 32-bit ARM NDK users. For the platform, we'll see if we should just set the max-page-size back to 4KB explicitly in the build rules, but that shouldn't affect this CL.

Sat, Apr 4, 1:25 AM · Restricted Project, lld

Fri, Apr 3

thieta added a comment to D77330: Consider increasing the default ARM32 page size to 64k..

So it sounds like I could go ahead and start changing the tests to match this? I can get to it during the weekend I hope. I anyone have a huge objection to this getting merged - let me know asap.

Fri, Apr 3, 10:42 PM · Restricted Project, lld
srhines added a comment to D77330: Consider increasing the default ARM32 page size to 64k..

I should have looked more closely at the AOSP sources, because we already hard-code the expected 4KB max-page-size. We do have some crufty flags that can be cleaned up, so I'll do that instead (https://android-review.googlesource.com/c/platform/build/soong/+/1278815). :) I'll also go ahead and create/test a CL to revert the Clang driver change forcing 4KB max-page-size next week.

Fri, Apr 3, 8:34 PM · Restricted Project, lld
srhines updated subscribers of D77330: Consider increasing the default ARM32 page size to 64k..

So it turns out that the ld shipped in the NDK actually did have the 64KB max-page-size patch, so not taking this patch would change current user behavior. Thanks to @MaskRay for sharing the exact binutils patch we were looking for to verify that it has it. For that reason, this patch should be fine for our 32-bit ARM NDK users. For the platform, we'll see if we should just set the max-page-size back to 4KB explicitly in the build rules, but that shouldn't affect this CL.

Fri, Apr 3, 8:02 PM · Restricted Project, lld
avl added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

But good to check/ask - @avl does this patch already do the good things for duplicate (or dropped) function sections?
(eg: "inline void f1() { } void f2() { f1(); }" + "inline void f1() { } void f2(); int main() { f1(); f2(); }" compiled and linked at -O0 the DWARF would usually contain two descriptions of 'f1', but ideally with a DWARF aware linker it'd contain only one description of 'f1'. Similarly: "void f1() { } void f2() { }" + "void f2(); int main() { f2(); }" compiled and linked with -ffunction-sections -Wl,-gc-sections would normally contain a DWARF description of "f1" but no code for f1, and a DWARF aware linker would strip out the DWARF description of 'f1' here)

Fri, Apr 3, 2:38 PM · debug-info, lld, Restricted Project
clayborg added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..
In D74169#1960444, @avl wrote:

Not so much reason to support type units ("-fdebug-types-section/type units/.debug_types section/multiple .debug_info sections" all essentially describe the same thing). Since type units add object size overhead to reduced linked size in the face of a DWARF-agnostic/ignorant linker. If you have a DWARF aware linker, you'd probably avoid using type units so you could have smaller object files too.

It seems to me type units are useful for the current scheme. They increase the size of type reference(8 bytes vs 4 bytes) and introduce type unit header, but they avoid duplication. All references inside the object file point to the single type description - thus, object files have only one type's description instead of multiple copies. The effect of de-duplication is higher than the introduced increase.

Fri, Apr 3, 1:33 PM · debug-info, lld, Restricted Project
clayborg added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

Nice! Another thing I have been seeing a lot of is when functions are coalesced and many .o files have the same function that gets linked to the same location. We end up with many duplicate DWARF DIEs in the final executable since the linker will just link all of the DWARF for each function over and over (.debug_info and .debug_line content). I found this when making GSYM files from the DWARF and noticing that I had hundreds of duplicate entries that matched exactly. This would be a great follow up patch for --gc-debuginfo

Given this is built on top of the DWARFLinker used for llvm-dsymutil which, I believe, already strips out those duplicate descriptions - hopefully that's already included in the functionality under review? But good to check/ask - @avl does this patch already do the good things for duplicate (or dropped) function sections?

(eg: "inline void f1() { } void f2() { f1(); }" + "inline void f1() { } void f2(); int main() { f1(); f2(); }" compiled and linked at -O0 the DWARF would usually contain two descriptions of 'f1', but ideally with a DWARF aware linker it'd contain only one description of 'f1'. Similarly: "void f1() { } void f2() { }" + "void f2(); int main() { f2(); }" compiled and linked with -ffunction-sections -Wl,-gc-sections would normally contain a DWARF description of "f1" but no code for f1, and a DWARF aware linker would strip out the DWARF description of 'f1' here)

Fri, Apr 3, 1:33 PM · debug-info, lld, Restricted Project
dblaikie added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

(***) fdebug-types-section is not supported by dsymutil/DWARFLinker.
Thus presented results do not show real data.
But these numbers could be taken as an estimation for future results
when DWARF5/-fdebug-types-section would be supported by dsymutil/DWARFLinker.

Fri, Apr 3, 1:33 PM · debug-info, lld, Restricted Project
dblaikie added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

Nice! Another thing I have been seeing a lot of is when functions are coalesced and many .o files have the same function that gets linked to the same location. We end up with many duplicate DWARF DIEs in the final executable since the linker will just link all of the DWARF for each function over and over (.debug_info and .debug_line content). I found this when making GSYM files from the DWARF and noticing that I had hundreds of duplicate entries that matched exactly. This would be a great follow up patch for --gc-debuginfo

Fri, Apr 3, 1:33 PM · debug-info, lld, Restricted Project
clayborg added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

Nice! Another thing I have been seeing a lot of is when functions are coalesced and many .o files have the same function that gets linked to the same location. We end up with many duplicate DWARF DIEs in the final executable since the linker will just link all of the DWARF for each function over and over (.debug_info and .debug_line content). I found this when making GSYM files from the DWARF and noticing that I had hundreds of duplicate entries that matched exactly. This would be a great follow up patch for --gc-debuginfo

Fri, Apr 3, 12:57 PM · debug-info, lld, Restricted Project
dblaikie added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..
In D74169#1960444, @avl wrote:

Not so much reason to support type units ("-fdebug-types-section/type units/.debug_types section/multiple .debug_info sections" all essentially describe the same thing). Since type units add object size overhead to reduced linked size in the face of a DWARF-agnostic/ignorant linker. If you have a DWARF aware linker, you'd probably avoid using type units so you could have smaller object files too.

It seems to me type units are useful for the current scheme. They increase the size of type reference(8 bytes vs 4 bytes) and introduce type unit header, but they avoid duplication. All references inside the object file point to the single type description - thus, object files have only one type's description instead of multiple copies. The effect of de-duplication is higher than the introduced increase.

DWARF aware linker does not reduce the size of object files. It reduces the size of binary, whereas type units allow decreasing object file.

Fri, Apr 3, 12:57 PM · debug-info, lld, Restricted Project
avl added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

Not so much reason to support type units ("-fdebug-types-section/type units/.debug_types section/multiple .debug_info sections" all essentially describe the same thing). Since type units add object size overhead to reduced linked size in the face of a DWARF-agnostic/ignorant linker. If you have a DWARF aware linker, you'd probably avoid using type units so you could have smaller object files too.

It seems to me type units are useful for the current scheme. They increase the size of type reference(8 bytes vs 4 bytes) and introduce type unit header, but they avoid duplication. All references inside the object file point to the single type description - thus, object files have only one type's description instead of multiple copies. The effect of de-duplication is higher than the introduced increase.

Fri, Apr 3, 12:27 PM · debug-info, lld, Restricted Project
smeenai added a reviewer for D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info.: clayborg.
Fri, Apr 3, 11:53 AM · debug-info, lld, Restricted Project
avl added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

Size/Performance/Memory results:

Fri, Apr 3, 11:21 AM · debug-info, lld, Restricted Project
dblaikie added a comment to D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..
In D74169#1960135, @avl wrote:

This version is a full patch implementing "remove obsolete debug info from lld" functionality.
As it was requested, It is presented as a full patch, which could be applied to the current code
base and evaluated. Some parts of that patch are already presented for review - D77169, D76085.
I am going to add tests for this patch and to address comments. Performance results would be presented in a separate message.

Limitations: it does not support DWARF5, modules, -fdebug-types-section, type units, .debug_types, multiple .debug_info sections.

Fri, Apr 3, 11:20 AM · debug-info, lld, Restricted Project
avl updated the diff for D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info..

This version is a full patch implementing "remove obsolete debug info from lld" functionality.
As it was requested, It is presented as a full patch, which could be applied to the current code
base and evaluated. Some parts of that patch are already presented for review - D77169, D76085.
I am going to add tests for this patch and to address comments. Performance results would be presented in a separate message.

Fri, Apr 3, 10:47 AM · debug-info, lld, Restricted Project
MaskRay added a comment to D77330: Consider increasing the default ARM32 page size to 64k..

We (Android Google folks) have an internal thread going on about what implications this might have, but I can say that I am concerned about making this kind of change for ARM Android targets. Even though we manually set a 4KB max page size in our platform builds for 32-bit, we also have to consider the needs of NDK developers (i.e. those folks making native applications/games/etc.). Some of their build configurations might behave very differently with this change, so my current thinking is that 64KB should not be the default for ARM Android targets.

Can you say more about your concern? I was actually thinking the exact opposite -- that the special-case in the Driver to use 4K pages on Android AArch64 could very likely be removed, now that using the default 64k max-page-size will no longer cause the binary size to increase.

Fri, Apr 3, 10:15 AM · Restricted Project, lld
jyknight added a comment to D77330: Consider increasing the default ARM32 page size to 64k..

We (Android Google folks) have an internal thread going on about what implications this might have, but I can say that I am concerned about making this kind of change for ARM Android targets. Even though we manually set a 4KB max page size in our platform builds for 32-bit, we also have to consider the needs of NDK developers (i.e. those folks making native applications/games/etc.). Some of their build configurations might behave very differently with this change, so my current thinking is that 64KB should not be the default for ARM Android targets.

Fri, Apr 3, 9:40 AM · Restricted Project, lld
thieta added a comment to D77330: Consider increasing the default ARM32 page size to 64k..

@srhines Thanks for your reply! Would it be enough for you guys to expand this block in clang https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/ToolChains/Linux.cpp#L256 for Android's usage? Or would a change in lld make more sense?

Fri, Apr 3, 6:24 AM · Restricted Project, lld
srhines added a comment to D77330: Consider increasing the default ARM32 page size to 64k..

We (Android Google folks) have an internal thread going on about what implications this might have, but I can say that I am concerned about making this kind of change for ARM Android targets. Even though we manually set a 4KB max page size in our platform builds for 32-bit, we also have to consider the needs of NDK developers (i.e. those folks making native applications/games/etc.). Some of their build configurations might behave very differently with this change, so my current thinking is that 64KB should not be the default for ARM Android targets.

Fri, Apr 3, 1:34 AM · Restricted Project, lld
psmith added reviewers for D77330: Consider increasing the default ARM32 page size to 64k.: MaskRay, grimar, ruiu.

Some references:
The GNU ld change to 64k max page size was made in 2014 https://patches.linaro.org/patch/32565/ [RFC] ld/ARM: Increase maximum page size to 64kB
My understanding from conversations at Arm and Linaro was that this was made primarily for the benefit of people running AArch32 executables on a AArch64 kernel built for 64k page sizes. In practice I don't think there ended up being a widely used distribution that simultaneously used 64K page size and offered a 32-bit user space. Ubuntu's decision to use 4k citing compatibility with old Arm v7 binaries https://wiki.ubuntu.com/ARM64/performance

Fri, Apr 3, 1:02 AM · Restricted Project, lld
thieta added a comment to D77330: Consider increasing the default ARM32 page size to 64k..

Hello Nick,

Fri, Apr 3, 12:30 AM · Restricted Project, lld

Thu, Apr 2

tmsriram updated the diff for D68065: Propeller: LLD Support for Basic Block Sections.

Rebase and make recent suggested changes.

Thu, Apr 2, 5:22 PM · Restricted Project, lld
tmsriram added a comment to D68065: Propeller: LLD Support for Basic Block Sections.

I have a question about aaaaaaa.BB.foo style labels. Are they STB_LOCAL symbol? If so,

  • The assembler (MC) will generally convert relocations referencing STB_LOCAL to reference STT_SECTION instead.
  • The assembler will keep them in .symtab
  • The linker will retain such labels in the .symtab section in the executable/shared object unless --discard-locals is specified.
  • aaaa.BB.foo will just be marker symbols in executables/shared objects: "hey, these addresses are special and are referenced by some instructions."
  • The strip tool will drop .symtab . It seems that the executables/shared objects cannot be stripped.
Thu, Apr 2, 5:22 PM · Restricted Project, lld
MaskRay added inline comments to D68065: Propeller: LLD Support for Basic Block Sections.
Thu, Apr 2, 5:22 PM · Restricted Project, lld
tmsriram added inline comments to D68065: Propeller: LLD Support for Basic Block Sections.
Thu, Apr 2, 3:45 PM · Restricted Project, lld
tmsriram added inline comments to D68065: Propeller: LLD Support for Basic Block Sections.
Thu, Apr 2, 3:44 PM · Restricted Project, lld
MaskRay added inline comments to D68065: Propeller: LLD Support for Basic Block Sections.
Thu, Apr 2, 3:44 PM · Restricted Project, lld
MaskRay added a comment to D68065: Propeller: LLD Support for Basic Block Sections.

I have a question about aaaaaaa.BB.foo style labels. Are they STB_LOCAL symbol? If so,

Thu, Apr 2, 3:44 PM · Restricted Project, lld
MaskRay added inline comments to D68065: Propeller: LLD Support for Basic Block Sections.
Thu, Apr 2, 3:11 PM · Restricted Project, lld
MaskRay added inline comments to D68065: Propeller: LLD Support for Basic Block Sections.
Thu, Apr 2, 3:11 PM · Restricted Project, lld
nickdesaulniers added a comment to D77330: Consider increasing the default ARM32 page size to 64k..

From the mailing list:

Thu, Apr 2, 1:33 PM · Restricted Project, lld
Harbormaster failed remote builds in B51524: Diff 254588 for D77330: Consider increasing the default ARM32 page size to 64k.!
Thu, Apr 2, 1:01 PM · Restricted Project, lld
MaskRay edited reviewers for D77330: Consider increasing the default ARM32 page size to 64k., added: psmith; removed: peter.smith.
Thu, Apr 2, 1:00 PM · Restricted Project, lld