[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC
ruiu added a comment. Thank you for the patch. What you are doing in this patch is not too complicated and makes sense to me. That said, if actual size saving is not significant as you said in https://github.com/rust-lang/rust/issues/56068#issuecomment-440160568, it may not be worth doing. It seems to me that if debug info is already 2.4GB, shrinking it to 2GB doesn't make much difference. Do you have more convincing examples? Repository: rLLD LLVM Linker CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54747/new/ https://reviews.llvm.org/D54747 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC
ruiu added a comment. But 2GB is perhaps still too big and I guess a large part of it can be for dead sections. If we fix this, I'd like to fix it in a proper way so that we can completely eliminate debug info for dead sections. rocallahan, can I ask why Rust passes all object files to the linker and use `-gc-sections` to eliminate unused part of these files? One possible way to fix it is to pass `--start-lib` to the linker before any object file paths in the command line. That option gives archive file semantics to object files, so if you option, each object file is treated as if that were in an archive file containing that the single object file. (Note that `--start-lib` is a positional option just like `--start-group`, so all files between `--start-lib` and `--end-lib` get that special treament.) Repository: rLLD LLVM Linker CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54747/new/ https://reviews.llvm.org/D54747 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC
ruiu added a comment. It seems to me that just adding `--start-lib` to his command line can fix the issue, so I'm waiting for Robert's response. If it doesn't work for some reason, we can analyze why it doesn't work and then discuss what we can do for his problem. Repository: rLLD LLVM Linker CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54747/new/ https://reviews.llvm.org/D54747 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47791: Initial support for Hexagon target.
ruiu added inline comments.
Comment at: ELF/Arch/Hexagon.cpp:46
+
+ for (size_t Bit = 0; Bit != sizeof(uint32_t) * 8; ++Bit) {
+const bool ValBit = (Data >> Off) & 1;
sizeof(uint32_t) * 8 is always 32.
Comment at: ELF/Arch/Hexagon.cpp:77
+ }
+
+ default:
Remove extraneous blank line. If you take a look at other files in lld, you'd
notice that we normally don't add a blank line before "default:".
Comment at: test/ELF/Inputs/hexagon.s:1
+
+.global _start
Remove a leading empty line.
Comment at: test/ELF/hexagon.s:2
+# REQUIRES: hexagon
+# RUN: llvm-mc -filetype=obj -triple=hexagon-unknown-elf %s -o %t
+# RUN: llvm-mc -filetype=obj -triple=hexagon-unknown-elf %S/Inputs/hexagon.s
-o %t2
Remove extranesous space between "mc" and "-filetype"
Comment at: test/ELF/hexagon.s:9
+call #_start
+# CHECK:call 0x11000
Remove extraneous space characters.
Repository:
rLLD LLVM Linker
https://reviews.llvm.org/D47791
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47791: Initial support for Hexagon target.
ruiu added inline comments.
Comment at: ELF/Arch/Hexagon.cpp:47
+ for (size_t Bit = 0; Bit != 32; ++Bit) {
+const bool ValBit = (Data >> Off) & 1;
+const bool MaskBit = (Mask >> Bit) & 1;
We normally don't add `const` if the variable's scope is narrow.
Comment at: ELF/Arch/Hexagon.cpp:72
+ case R_HEX_B22_PCREL: {
+uint32_t EffectiveValue = (Val >> 2) & 0x3f;
+EffectiveValue = applyMask(0x01ff3ffe, EffectiveValue);
We usually use much shorter variable name for a variable whose scope is very
narrow. Just `V` would be enough for this.
Comment at: ELF/Arch/Hexagon.cpp:74
+EffectiveValue = applyMask(0x01ff3ffe, EffectiveValue);
+write32le(Loc, (read32le(Loc) | EffectiveValue));
+break;
I believe we have `or32le`.
Repository:
rLLD LLVM Linker
https://reviews.llvm.org/D47791
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC
ruiu added inline comments.
Comment at: lld/ELF/MarkLive.cpp:190
+template static void setSectionLive(InputSectionBase *Sec) {
+ Sec->Live = true;
Since this file is MarkLive, markSection is perhaps a better name.
Comment at: lld/ELF/MarkLive.cpp:192
+ Sec->Live = true;
+ if (Sec->kind() != SectionBase::Kind::Regular &&
+ Sec->kind() != SectionBase::Kind::Merge)
rocallahan wrote:
> MaskRay wrote:
> > This check can be changed to `!isa &&
> > !isa`. But do you just want to exclude `EhInputSection`?
> Shouldn't I also be excluding `SyntheticSection`?
This needs a comment.
Do you have to visit each file during the mark phase? Looks like you can mark
only sections first, and after marking all sections, you can scan all sections
to mark files. Looks like they can be two separate stages.
Repository:
rLLD LLVM Linker
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54747/new/
https://reviews.llvm.org/D54747
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC
ruiu added inline comments. Comment at: lld/ELF/MarkLive.cpp:192 + Sec->Live = true; + if (Sec->kind() != SectionBase::Kind::Regular && + Sec->kind() != SectionBase::Kind::Merge) rocallahan wrote: > ruiu wrote: > > rocallahan wrote: > > > MaskRay wrote: > > > > This check can be changed to `!isa && > > > > !isa`. But do you just want to exclude > > > > `EhInputSection`? > > > Shouldn't I also be excluding `SyntheticSection`? > > This needs a comment. > > > > Do you have to visit each file during the mark phase? Looks like you can > > mark only sections first, and after marking all sections, you can scan all > > sections to mark files. Looks like they can be two separate stages. > Maybe I'm wrong but I would have expected adding another pass over all > sections to be slower than what I'm doing here. > > Also we need to distinguish LSDA sections from other live sections since LSDA > does not count as "making the file live". So we'd have to add an LSDA flag to > `InputSection`. > > Are you sure you want me to make this change? > > Also I just noticed I've written LDSA in several places where it should be > LSDA! Ah OK, I thought that you set Sec->Live to true only in `setSectionLive()` but you manipulated that in EnqueueMaybeLDSA as well. Looks like we have too many callback functions in this function -- this file is organized that way because the callback functions were very simple. Now it's been growing organically and probably get to the point that we should just use the regular class-based abstraction. Let me do that first. Repository: rLLD LLVM Linker CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54747/new/ https://reviews.llvm.org/D54747 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC
ruiu added a comment. I committed https://reviews.llvm.org/D59800 which I believe makes your change easier to follow once rebased. Could you rebase it and upload a patch? Thanks! Repository: rLLD LLVM Linker CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54747/new/ https://reviews.llvm.org/D54747 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC
ruiu added a comment. Nice. One more thing you might want to try is to add `-O2` to the linker command line option. When that option is given, lld attempts to tail-merge strings in the string table. That's not very effective, but you might be able to shave off 0.2% or something like that. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54747/new/ https://reviews.llvm.org/D54747 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC
ruiu added a comment.
Overall looking good.
Comment at: lld/ELF/MarkLive.cpp:190
+ // case we still need to mark the file.
+ if (S && !IsLSDA && Sec->File)
+if (isa(Sec) || isa(Sec))
ruiu wrote:
> S is true only when it is an InputSection, so you have duplicate tests for
> this. It also looks like you don't have to care about whether a section is an
> input section or a merged section. Isn't this condition sufficient?
>
> if (Sec->File && !IsLSDA)
> Sec->getFile()->HasLiveCodeOrData = true;
Move this code after `Sec->Live = true` so that when we visit the same section
more than once, this piece of code runs only once.
Comment at: lld/ELF/MarkLive.cpp:190-191
+ // case we still need to mark the file.
+ if (S && !IsLSDA && Sec->File)
+if (isa(Sec) || isa(Sec))
+ Sec->getFile()->HasLiveCodeOrData = true;
S is true only when it is an InputSection, so you have duplicate tests for
this. It also looks like you don't have to care about whether a section is an
input section or a merged section. Isn't this condition sufficient?
if (Sec->File && !IsLSDA)
Sec->getFile()->HasLiveCodeOrData = true;
Comment at: lld/ELF/MarkLive.cpp:313
// remove its relocation section.
+ bool AnyDebugSections = false;
for (InputSectionBase *Sec : InputSections) {
Let's remove this optimization -- I don't think we need this.
Comment at: lld/ELF/MarkLive.cpp:321
bool IsLinkOrder = (Sec->Flags & SHF_LINK_ORDER);
bool IsRel = (Sec->Type == SHT_REL || Sec->Type == SHT_RELA);
Then you can simplify the condition to
if (!IsAlloc && !IsLinkOrder && !IsRel && !Sec->Debug)
Sec->Live = true;
Comment at: lld/ELF/MarkLive.cpp:330
+ if (AnyDebugSections)
+// Mark debug sections as live in any object file that has a live
Remove this variable and just visit all sections even if there's no debug info
(which shouldn't take too much time anyway.)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54747/new/
https://reviews.llvm.org/D54747
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC
ruiu added inline comments. Comment at: lld/ELF/MarkLive.cpp:190 + // case we still need to mark the file. + if (S && !IsLSDA && Sec->File) +if (isa(Sec) || isa(Sec)) ruiu wrote: > rocallahan wrote: > > rocallahan wrote: > > > ruiu wrote: > > > > ruiu wrote: > > > > > S is true only when it is an InputSection, so you have duplicate > > > > > tests for this. It also looks like you don't have to care about > > > > > whether a section is an input section or a merged section. Isn't this > > > > > condition sufficient? > > > > > > > > > > if (Sec->File && !IsLSDA) > > > > > Sec->getFile()->HasLiveCodeOrData = true; > > > > Move this code after `Sec->Live = true` so that when we visit the same > > > > section more than once, this piece of code runs only once. > > > We can't do that. The comment I added tries to explain why. Is it unclear? > > I want to skip `EhInputSection` and `SyntheticSection` here. So I guess the > > advice to use `isa` here was wrong and I should revert to checking `kind() > > == Regular || kind() == Merge`? > But you don't really care even if it is `EhInputSection` or > `SyntheticSection`, no? I mean an assigned value would not be used but > doesn't harm. Ah, OK, got it. Comment at: lld/ELF/MarkLive.cpp:190-191 + // case we still need to mark the file. + if (S && !IsLSDA && Sec->File) +if (isa(Sec) || isa(Sec)) + Sec->getFile()->HasLiveCodeOrData = true; rocallahan wrote: > rocallahan wrote: > > ruiu wrote: > > > ruiu wrote: > > > > S is true only when it is an InputSection, so you have duplicate tests > > > > for this. It also looks like you don't have to care about whether a > > > > section is an input section or a merged section. Isn't this condition > > > > sufficient? > > > > > > > > if (Sec->File && !IsLSDA) > > > > Sec->getFile()->HasLiveCodeOrData = true; > > > Move this code after `Sec->Live = true` so that when we visit the same > > > section more than once, this piece of code runs only once. > > We can't do that. The comment I added tries to explain why. Is it unclear? > I want to skip `EhInputSection` and `SyntheticSection` here. So I guess the > advice to use `isa` here was wrong and I should revert to checking `kind() == > Regular || kind() == Merge`? But you don't really care even if it is `EhInputSection` or `SyntheticSection`, no? I mean an assigned value would not be used but doesn't harm. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54747/new/ https://reviews.llvm.org/D54747 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC
ruiu added inline comments. Comment at: lld/ELF/MarkLive.cpp:190-191 + // case we still need to mark the file. + if (S && !IsLSDA && Sec->File) +if (isa(Sec) || isa(Sec)) + Sec->getFile()->HasLiveCodeOrData = true; rocallahan wrote: > ruiu wrote: > > ruiu wrote: > > > rocallahan wrote: > > > > rocallahan wrote: > > > > > ruiu wrote: > > > > > > ruiu wrote: > > > > > > > S is true only when it is an InputSection, so you have duplicate > > > > > > > tests for this. It also looks like you don't have to care about > > > > > > > whether a section is an input section or a merged section. Isn't > > > > > > > this condition sufficient? > > > > > > > > > > > > > > if (Sec->File && !IsLSDA) > > > > > > > Sec->getFile()->HasLiveCodeOrData = true; > > > > > > Move this code after `Sec->Live = true` so that when we visit the > > > > > > same section more than once, this piece of code runs only once. > > > > > We can't do that. The comment I added tries to explain why. Is it > > > > > unclear? > > > > I want to skip `EhInputSection` and `SyntheticSection` here. So I guess > > > > the advice to use `isa` here was wrong and I should revert to checking > > > > `kind() == Regular || kind() == Merge`? > > > But you don't really care even if it is `EhInputSection` or > > > `SyntheticSection`, no? I mean an assigned value would not be used but > > > doesn't harm. > > Ah, OK, got it. > I'm assuming that `.eh_frame` sections don't have associated debuginfo so > even if such a section is enqueued somehow, it should not cause debuginfo to > be included for that object file. Is it impossible for a `.eh_frame` section > to be enqueued here? Ditto for a `SyntheticSection`? > > To put it another way, the logic here is supposed to be: "debuginfo can only > be associated with Regular or Merge sections, so it only makes sense to mark > files as having 'live code or data' possibly associated with debuginfo for > those section types." No debug sections are attached to synthetic sections, but I don't think that logically matters. The logic we want to implement here is 1. if a file contains one or more debug info sections, and 2. if at least one of them are marked live, then 3. all debug sections should be marked live. If `Sec->Debug` is true, it is a debug section, and vice versa. If Sec->File is a non-null, it is a file that the section belongs to. So, in the above logic, there's no logic that depends on the type of the section. That's why I think it is better to not assert any expected type for a debug section. As long as `!IsLSDA && Sec->File`, that File should be marked live here. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54747/new/ https://reviews.llvm.org/D54747 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC
ruiu accepted this revision. ruiu added a comment. This revision is now accepted and ready to land. LGTM I'd write a comment explaining why we are handling debug sections in a special way, but that can be done later. Please submit. Thank you for doing this! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54747/new/ https://reviews.llvm.org/D54747 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC
ruiu added a comment. I will do that. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54747/new/ https://reviews.llvm.org/D54747 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC
This revision was automatically updated to reflect the committed changes.
Closed by commit rL358069: Discard debuginfo for object files empty after GC
(authored by ruiu, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D54747?vs=194255&id=194469#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54747/new/
https://reviews.llvm.org/D54747
Files:
lld/trunk/ELF/Driver.cpp
lld/trunk/ELF/InputFiles.h
lld/trunk/ELF/InputSection.cpp
lld/trunk/ELF/InputSection.h
lld/trunk/ELF/MarkLive.cpp
lld/trunk/test/ELF/linkerscript/comdat-gc.s
lld/trunk/test/ELF/linkerscript/debuginfo-gc.s
Index: lld/trunk/test/ELF/linkerscript/comdat-gc.s
===
--- lld/trunk/test/ELF/linkerscript/comdat-gc.s
+++ lld/trunk/test/ELF/linkerscript/comdat-gc.s
@@ -8,6 +8,9 @@
# GC1: Name: .debug_line
+# Add .ctors section so all debuginfo isn't GCed
+.section .ctors,"ax",@progbits
+
.file 1 "test/ELF/linkerscript/comdat_gc.s"
.section .text._Z3fooIiEvv,"axG",@progbits,_Z3fooIiEvv,comdat
.loc 1 14
Index: lld/trunk/test/ELF/linkerscript/debuginfo-gc.s
===
--- lld/trunk/test/ELF/linkerscript/debuginfo-gc.s
+++ lld/trunk/test/ELF/linkerscript/debuginfo-gc.s
@@ -0,0 +1,14 @@
+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/comdat-gc.s -o %t1
+# RUN: echo "SECTIONS { .text : { *(.text*) } }" > %t.script
+# RUN: ld.lld --gc-sections --script %t.script %t %t1 -o %t2
+# RUN: llvm-readobj -sections -symbols %t2 | FileCheck %s
+
+# CHECK-NOT: Name: .debug_line
+
+.file 1 "test/ELF/linkerscript/comdat_gc.s"
+.section .text._Z3fooIiEvv,"axG",@progbits,_Z3fooIiEvv,comdat
+.loc 1 14
+ ret
Index: lld/trunk/ELF/MarkLive.cpp
===
--- lld/trunk/ELF/MarkLive.cpp
+++ lld/trunk/ELF/MarkLive.cpp
@@ -47,7 +47,7 @@
void run();
private:
- void enqueue(InputSectionBase *Sec, uint64_t Offset);
+ void enqueue(InputSectionBase *Sec, uint64_t Offset, bool IsLSDA);
void markSymbol(Symbol *Sym);
template
@@ -97,7 +97,7 @@
Offset += getAddend(Sec, Rel);
if (!IsLSDA || !(RelSec->Flags & SHF_EXECINSTR))
- enqueue(RelSec, Offset);
+ enqueue(RelSec, Offset, IsLSDA);
return;
}
@@ -106,7 +106,7 @@
SS->getFile().IsNeeded = true;
for (InputSectionBase *Sec : CNamedSections.lookup(Sym.getName()))
-enqueue(Sec, 0);
+enqueue(Sec, 0, IsLSDA);
}
// The .eh_frame section is an unfortunate special case.
@@ -169,7 +169,8 @@
}
template
-void MarkLive::enqueue(InputSectionBase *Sec, uint64_t Offset) {
+void MarkLive::enqueue(InputSectionBase *Sec, uint64_t Offset,
+ bool IsLSDA) {
// Skip over discarded sections. This in theory shouldn't happen, because
// the ELF spec doesn't allow a relocation to point to a deduplicated
// COMDAT section directly. Unfortunately this happens in practice (e.g.
@@ -183,19 +184,26 @@
if (auto *MS = dyn_cast(Sec))
MS->getSectionPiece(Offset)->Live = true;
+ InputSection *S = dyn_cast(Sec);
+ // LSDA does not count as "live code or data" in the object file.
+ // The section may already have been marked live for LSDA in which
+ // case we still need to mark the file.
+ if (S && !IsLSDA && Sec->File)
+Sec->getFile()->HasLiveCodeOrData = true;
+
if (Sec->Live)
return;
- Sec->Live = true;
+ Sec->Live = true;
// Add input section to the queue.
- if (InputSection *S = dyn_cast(Sec))
+ if (S)
Queue.push_back(S);
}
template void MarkLive::markSymbol(Symbol *Sym) {
if (auto *D = dyn_cast_or_null(Sym))
if (auto *IS = dyn_cast_or_null(D->Section))
- enqueue(IS, D->Value);
+ enqueue(IS, D->Value, false);
}
// This is the main function of the garbage collector.
@@ -239,7 +247,7 @@
continue;
if (isReserved(Sec) || Script->shouldKeep(Sec)) {
- enqueue(Sec, 0);
+ enqueue(Sec, 0, false);
} else if (isValidCIdentifier(Sec->Name)) {
CNamedSections[Saver.save("__start_" + Sec->Name)].push_back(Sec);
CNamedSections[Saver.save("__stop_" + Sec->Name)].push_back(Sec);
@@ -259,7 +267,7 @@
}
for (InputSectionBase *IS : Sec.DependentSections)
- enqueue(IS, 0);
+ enqueue(IS, 0, false);
}
}
@@ -285,7 +293,7 @@
// The -gc-sections option works only for SHF_ALLOC sections
// (sections that are memory-mapped at runtime). So we can
// unconditionally make non-SHF_ALLOC sections alive except
- // SHF_LINK_ORDER and SHT_REL/SHT_RELA sections.
+ // SHF_LINK_ORDER and SHT_REL/SHT_RELA sections and .debug sections.
//
// Usually, SHF_ALLOC sections are not removed even if they are
// unreachable through relocations because reachability is not
@@ -306,13 +314,19
[Lldb-commits] [PATCH] D54747: Discard debuginfo for object files empty after GC
ruiu added a comment. Bob, thank you for reverting this. So, Robert, looks like this idea didn't work well. We need a different solution. And perhaps a better approach is to use --start-lib and --end-lib. You found that these options didn't work well for your input, but I don't fully understand why that's the case. Are you combining all input object files into a single object file using `-r` linker option? If you just pass all object files to the linker's command line, the linker can choose only the files that are actually used. I'd like to understand why that didn't work for Rust. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54747/new/ https://reviews.llvm.org/D54747 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
