[Lldb-commits] [PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Aleksei Sidorin via Phabricator via lldb-commits]

a_sidorin added a comment.

Hello Raphael,

Thank you for the explanation. I have +1 to Gabor's question to understand if 
this functionality can actually be added to the common ASTImporter.




Comment at: clang/include/clang/AST/ASTImporter.h:149
+/// decl on its own.
+virtual Expected ImportInternal(Decl *From);
+

I'd suggest to rename it to `ImportImpl`.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:590
+  new RedirectingImporter(ToContext, ToFileManager, FromContext,
+  FromFileManager, MinimalImport, LookupTabl));
+};

LookupTable?



Comment at: clang/unittests/AST/ASTImporterTest.cpp:578
+auto *ND = dyn_cast(FromD);
+if (!ND)
+  return ASTImporter::ImportInternal(FromD);

I think it's better to merge these conditions: `if (!ND || ND->getName() != 
"shouldNotBeImported")`



Comment at: clang/unittests/AST/ASTImporterTest.cpp:583
+for (Decl *D : getToContext().getTranslationUnitDecl()->decls()) {
+  if (auto ND = dyn_cast(D))
+if (ND->getName() == "realDecl")

`auto *` (pointer sign is needed).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59485/new/

https://reviews.llvm.org/D59485



___
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

[Robert O'Callahan via Phabricator via lldb-commits]

rocallahan marked 3 inline comments as done.
rocallahan added a comment.

> @rocallahan I find that people are discussing a generic approach in D59553 
> 

Thanks for the reference. It seems to me that there is no consensus there yet 
for any approach that would obsolete this change. Of the three approaches 
discussed there:

1. Having the linker rewrite DWARF: seems to have been decisively rejected.
2. Give each function standalone debuginfo in COMDAT: sounds to me like it 
would be very inefficient, both in the size of intermediate files and in the 
size of the final binary. DWARF type information currently shared between 
functions in the same CU would have to be duplicated, and eliminating that 
duplication would require the linker rewrite DWARF, which brings us back to 
point 1.
3. Optimize away unnecessary debuginfo using a post-link tool like `dwz`: 
Considerably less efficient than not writing out the bloated DWARF in the first 
place.




Comment at: lld/ELF/MarkLive.cpp:192
+  Sec->Live = true;
+  if (Sec->kind() != SectionBase::Kind::Regular &&
+  Sec->kind() != SectionBase::Kind::Merge)

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`?



Comment at: lld/ELF/MarkLive.cpp:195
+return;
+  if (!Sec->File || !ObjFile::classof(Sec->File))
+return;

MaskRay wrote:
> > `!ObjFile::classof(Sec->File)`
> 
> Can this happen?
I think not. Happy to remove this check if you don't want it.



Comment at: lld/ELF/MarkLive.cpp:199
+Sec->getFile()->HasLiveCodeOrData = true;
+  }
+}

MaskRay wrote:
> The brace is redundant.
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

[Robert O'Callahan via Phabricator via lldb-commits]

rocallahan added a comment.

> Do you have numbers with ld.bfd and gold?

I don't have numbers for that exact test with those linkers but they basically 
behave the same way as LLD.


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

[Robert O'Callahan via Phabricator via lldb-commits]

rocallahan added a comment.

To be clear, I think the best long-term solution is for LLD to rewrite the 
DWARF, but from my (admittedly limited) perspective that seems to be at best a 
distant prospect.


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

[Robert O'Callahan via Phabricator via lldb-commits]

rocallahan added a comment.

Another clarification:

> DWARF type information currently shared between functions in the same CU 
> would have to be duplicated.

I guess that's not necessary. The compiler could put all debuginfo that's 
shared between functions into a non-COMDAT section for the compilation unit. 
The downside of that is that that data is not removed by gc-sections ... 
although my patch here would remove it, if all functions in the CU are removed.

I guess this is what @echristo was getting at in their comment above:

In D54747#1319447 , @echristo wrote:

> There's a few options here, mostly using comdat groups for debug infomation 
> associated with sections that the debug information comes from. That said, it 
> still wouldn't encompass compile units that would have no code in them 
> because we didn't choose a single function from a .o file and so this patch 
> is still a strict improvement over that. In addition, it doesn't involve 
> future work to change how we emit debug info and doesn't depend on everyone 
> emitting debug info the same way.



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