[Lldb-commits] [PATCH] D44041: Only replace object file sections when non-empty

[Francis Ricci via Phabricator via lldb-commits]

fjricci abandoned this revision.
fjricci added a comment.

Didn’t realize I still had open revisions, haven’t worked on lldb in quite some 
time


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

https://reviews.llvm.org/D44041



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D44041: Only replace object file sections when non-empty

[Francis Ricci via Phabricator via lldb-commits]

fjricci created this revision.
fjricci added reviewers: clayborg, zturner, labath.
Herald added subscribers: JDevlieghere, arichardson, emaste.

In order to allow some sections to exist either in split debug-info or in the 
main binary,
don't replace non-empty sections with empty sections.


https://reviews.llvm.org/D44041

Files:
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp


Index: source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
===
--- source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
+++ source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
@@ -141,7 +141,7 @@
 SectionType section_type = g_sections[idx];
 SectionSP section_sp(
 objfile_section_list->FindSectionByType(section_type, true));
-if (section_sp) {
+if (section_sp && section_sp->GetFileSize()) {
   SectionSP module_section_sp(
   module_section_list->FindSectionByType(section_type, true));
   if (module_section_sp)
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -2004,7 +2004,7 @@
 SectionType section_type = g_sections[idx];
 SectionSP section_sp(
 elf_section_list->FindSectionByType(section_type, true));
-if (section_sp) {
+if (section_sp && section_sp->GetFileSize()) {
   SectionSP module_section_sp(
   unified_section_list.FindSectionByType(section_type, true));
   if (module_section_sp)


Index: source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
===
--- source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
+++ source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
@@ -141,7 +141,7 @@
 SectionType section_type = g_sections[idx];
 SectionSP section_sp(
 objfile_section_list->FindSectionByType(section_type, true));
-if (section_sp) {
+if (section_sp && section_sp->GetFileSize()) {
   SectionSP module_section_sp(
   module_section_list->FindSectionByType(section_type, true));
   if (module_section_sp)
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -2004,7 +2004,7 @@
 SectionType section_type = g_sections[idx];
 SectionSP section_sp(
 elf_section_list->FindSectionByType(section_type, true));
-if (section_sp) {
+if (section_sp && section_sp->GetFileSize()) {
   SectionSP module_section_sp(
   unified_section_list.FindSectionByType(section_type, true));
   if (module_section_sp)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D44041: Only replace object file sections when non-empty

[Pavel Labath via Phabricator via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

This looks like a perfect case for lldb-test. You will run into a couple of 
problems which will prevent this from working out of the box. I tried to fix 
those in https://reviews.llvm.org/D42955, but that turned into a major 
refactor. Until that CL lands, we should be able to make a quick hack in 
lldb-test which will make this testable:
The thing you will need to modify is to explicitly call 
module->GetSymbolVendor() before dumping out the sections (so that the symbol 
vendor populates these), and avoid setting the SymbolFileSpec on the ModuleSpec 
(so that the symbol vendor will find the external symbol file.


https://reviews.llvm.org/D44041



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D44041: Only replace object file sections when non-empty

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Ok, as of r326805 it should be very easy to write a test for this. You can look 
at the test in that commit for inspiration.

Btw, do you happen to know why we have two copies of the section merging code? 
While working on that patch I noticed an issue that is caused (in part*) by the 
fact that we merge the sections three times. I've tried to remove the copy in 
ObjectFileELF, and everything seems to work fine, but our test support coverage 
for the debug-info-in-separate-file is not that awesome. If you have any other 
tests for this, I'd appreciate if you can run them before I check that in (I'll 
send you a patch soon).

(*) The problem is that we are replacing the sections by their IDs, but (ELF?) 
section IDs are unique only if they all come from a single object file. So when 
we start replacing them the second time round, we end up overwriting random 
sections. When we replace the sections only once, the code will be technically 
correct, but the replacement-by-id idea seems still a bit sketchy.


https://reviews.llvm.org/D44041



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits