[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D42955#1027142, @clayborg wrote:

> Again, we need any objects coming out of the ObjectFile to have the correct 
> sections. Not sure how we would accomplish that with a solution where the 
> dSYM file uses it own useless sections.


I think we have different definitions of "correct" sections. I agree that the 
nulled out sections in the dSYM file are (nearly) useless, but I think they are 
also correct, because that's exactly what's in that object file (and I think a 
single ObjectFile instance should represent information from a single object 
file -- nothing more, nothing less). If we need  (and we do need) to have a 
view of combined information from multiple object files, then there should be a 
separate entity that does that (I think that's what the unified section list is 
supposed to be). Then, anything that needs to operate on the unified view 
should be taught to operate on the unified view and not on the individual 
object files.

Now, I have no idea how hard is it to achieve this, but the fact that this is 
already how things work for elf gives me hope that it will not be that hard.

Btw, do you know how much I can rely on the test suite to catch any issues 
here? I've tried simply commenting out the line where we add the section from 
the unified list into the dsym file, and all tests still passed. Is there any 
particular scenario I should look at in more detail?


https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In https://reviews.llvm.org/D42955#1026296, @labath wrote:

> In https://reviews.llvm.org/D42955#1026216, @clayborg wrote:
>
> > AS for the ELF example where only debug info is around, it might not be 
> > running into issues if it doesn't contain a symbol table. If it does 
> > contain a symbol table, hopefully it is using the unified section table, 
> > otherwise if might have symbol's whose addresses have sections from the 
> > stripped ELF file and it might not have the section contents that it needs 
> > to back it. In that case we might fail to disassemble the symbol's code.
>
>
> I made a trivial experiment:
>
>   g++ a.cc
>   objcopy a.out --only-keep-debug a.sym #a.sym contains a symtab
>   strip a.out # a.out does not contain a symtab
>   (lldb) target create "a.out"
>   Current executable set to 'a.out' (x86_64).
>   (lldb) b main
>   Breakpoint 1: no locations (pending).
>   WARNING:  Unable to resolve breakpoint to any actual locations.
>   (lldb) target symbols add a.sym
>   symbol file '/tmp/X/a.sym' has been added to '/tmp/X/a.out'
>   1 location added to breakpoint 1
>   #symbol resolution seems fine
>   (lldb) disassemble -n main
>   a.out`main:
>   a.out[0x69b] <+0>:  pushq  %rbp
>   a.out[0x69c] <+1>:  movq   %rsp, %rbp
>   a.out[0x69f] <+4>:  callq  0x690 ; foo()
>   a.out[0x6a4] <+9>:  addl   $0x2a, %eax
>   a.out[0x6a7] <+12>: popq   %rbp
>   a.out[0x6a8] <+13>: retq
>   # so does disassembling
>
>
> The part that is not clear to me is, if the dsym object file should absorb 
> the sections from the main object file, then what is the purpose of the 
> "unified section list" in the module? I can see why we need a unified list, 
> and I think it's a good idea, but having an ObjectFile containing the merged 
> list seems to defeat it. From a separation of concerns POV, it would be much 
> clearer if each ObjectFile contained only it's own sections, and then some 
> other entity (Module) held the combined data(sections) of the individual 
> object files. Then, most clients should operate on the unified view of the 
> module, but if anyone ever had a reason, he could always look directly at a 
> specific ObjectFile, and see what's contained there.


The issue is the object file must create symbols that uses the section from the 
unified section list. The easiest way to accomplish that was to have the object 
file just use the right section and not have to worry about any other code 
having to know anything about the relationship between a debuig info file and 
the main executable file.

> Among other things, this could be very useful for lldb-server. lldb-server 
> needs only lightweight access to the data in the object file -- it does not 
> need the Module class and everything it pulls in (SymbolVendor, SymbolFile, 
> ...). If we could make the ObjectFile class completely standalone, we can 
> avoid pulling all this stuff in and make the code more modular. It would also 
> help with the migration to llvm's Object parsing library, as it knows nothing 
> about unified sections. If we had a standalone ObjectFile implementation, 
> then it would be easy to swap it out for llvm's and still keep the "section 
> unification" code intact as it would be in a different layer.
> 
> So, could this be a direction we can move in? I can put this patch on ice and 
> try to make the ObjectFileMachO standalone instead. I don't think it should 
> be that hard, given that this is how things already work for elf. I'm hoping 
> it will be a matter of changing some consumers to use 
> module->GetSectionList() instead of objfile->GetSectionList()...

Again, we need any objects coming out of the ObjectFile to have the correct 
sections. Not sure how we would accomplish that with a solution where the dSYM 
file uses it own useless sections.


https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Pavel Labath via Phabricator via lldb-commits]

labath abandoned this revision.
labath added a comment.

I don't think this approach is good while ObjectFileMachO is messing with 
sections of other object files. At this point I think, I've actually learned 
enough about MachO to understand what the code is doing there, so I'm going to 
see if I can make it do what I want.


https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D42955#1026216, @clayborg wrote:

> AS for the ELF example where only debug info is around, it might not be 
> running into issues if it doesn't contain a symbol table. If it does contain 
> a symbol table, hopefully it is using the unified section table, otherwise if 
> might have symbol's whose addresses have sections from the stripped ELF file 
> and it might not have the section contents that it needs to back it. In that 
> case we might fail to disassemble the symbol's code.


I made a trivial experiment:

  g++ a.cc
  objcopy a.out --only-keep-debug a.sym #a.sym contains a symtab
  strip a.out # a.out does not contain a symtab
  (lldb) target create "a.out"
  Current executable set to 'a.out' (x86_64).
  (lldb) b main
  Breakpoint 1: no locations (pending).
  WARNING:  Unable to resolve breakpoint to any actual locations.
  (lldb) target symbols add a.sym
  symbol file '/tmp/X/a.sym' has been added to '/tmp/X/a.out'
  1 location added to breakpoint 1
  #symbol resolution seems fine
  (lldb) disassemble -n main
  a.out`main:
  a.out[0x69b] <+0>:  pushq  %rbp
  a.out[0x69c] <+1>:  movq   %rsp, %rbp
  a.out[0x69f] <+4>:  callq  0x690 ; foo()
  a.out[0x6a4] <+9>:  addl   $0x2a, %eax
  a.out[0x6a7] <+12>: popq   %rbp
  a.out[0x6a8] <+13>: retq
  # so does disassembling

The part that is not clear to me is, if the dsym object file should absorb the 
sections from the main object file, then what is the purpose of the "unified 
section list" in the module? I can see why we need a unified list, and I think 
it's a good idea, but having an ObjectFile containing the merged list seems to 
defeat it. From a separation of concerns POV, it would be much clearer if each 
ObjectFile contained only it's own sections, and then some other entity 
(Module) held the combined data(sections) of the individual object files. Then, 
most clients should operate on the unified view of the module, but if anyone 
ever had a reason, he could always look directly at a specific ObjectFile, and 
see what's contained there.

Among other things, this could be very useful for lldb-server. lldb-server 
needs only lightweight access to the data in the object file -- it does not 
need the Module class and everything it pulls in (SymbolVendor, SymbolFile, 
...). If we could make the ObjectFile class completely standalone, we can avoid 
pulling all this stuff in and make the code more modular. It would also help 
with the migration to llvm's Object parsing library, as it knows nothing about 
unified sections. If we had a standalone ObjectFile implementation, then it 
would be easy to swap it out for llvm's and still keep the "section 
unification" code intact as it would be in a different layer.

So, could this be a direction we can move in? I can put this patch on ice and 
try to make the ObjectFileMachO standalone instead. I don't think it should be 
that hard, given that this is how things already work for elf. I'm hoping it 
will be a matter of changing some consumers to use module->GetSectionList() 
instead of objfile->GetSectionList()...


https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In https://reviews.llvm.org/D42955#1026197, @labath wrote:

> In https://reviews.llvm.org/D42955#1026175, @clayborg wrote:
>
> > The dSYM file is a mach-o file that contains symbols only, It is because 
> > the dSYM file (stand alone debug info file) has all of the section 
> > definitions from the main executable, but no section content for everything 
> > except the DWARF debug info. The DWARF only exists in the dSYM file. The 
> > dSYM file also has all of the symbols as it gets made before the executable 
> > is stripped. Many symbols refer to a section by section index, so that is 
> > why we must have the section definitions in the dSYM file. So the dSYM 
> > wants the sections from the main executable so it can access __text and 
> > __data if needed since symbol refer to these and someone might ask a symbol 
> > from a dSYM file what its instructions are. So the dSYM uses the real 
> > sections from the main executable file instead of its own.
>
>
> I see, thanks for the explanation. The code makes more sense now.
>
> But now I have another question. Is this an intentional design decision that 
> we want to preserve; or just how things happen to work now, and should be 
> cleaned up in the future (to achieve the "ObjectFile contains only own 
> sections" invariant)?


Intentional, but it is up to the SymbolVendor subclass to do all of this. We 
want the data from the dSYM to be useful and since the dSYM really is a 
companion debug info file, it needs info from elsewhere to be complete.

> I'm asking this, because if it's the former then this patch probably needs to 
> be redone, as it tries to enforce that invariant (and then needs to jump 
> through hoops to enable the ObjectFileMachO use case). OTOH, if it's the 
> latter, then I think this is a step in the right direction, as it makes it 
> obvious that ObjectFileMachO is doing something that it shouldn't (and at the 
> same time it makes it harder for other ObjectFiles to do the same).

It should be doing it. Again, the symbol vendor is the one that is trying to 
make everything fit together and "just work".

> BTW, the same situation (symbol file containing empty section definitions for 
> code) can happen on elf targets as well (if the symbol file is produced 
> with `strip --only-keep-debug`), but everything seems to be working fine, 
> presumably because all consumers are accessing the sections through the 
> unified section list.

Exactly. I see any good debug info format not needing to duplicate things that 
might be in another file. The symbol vendor under the covers, creates a unified 
experience for all consumers of the Module that contains the symbol vendor, so 
it is something that we want to encourage and allow. So ObjectFileMachO isn't 
doing anything wrong. Feel free to update the design if needed?


https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D42955#1026175, @clayborg wrote:

> The dSYM file is a mach-o file that contains symbols only, It is because the 
> dSYM file (stand alone debug info file) has all of the section definitions 
> from the main executable, but no section content for everything except the 
> DWARF debug info. The DWARF only exists in the dSYM file. The dSYM file also 
> has all of the symbols as it gets made before the executable is stripped. 
> Many symbols refer to a section by section index, so that is why we must have 
> the section definitions in the dSYM file. So the dSYM wants the sections from 
> the main executable so it can access __text and __data if needed since symbol 
> refer to these and someone might ask a symbol from a dSYM file what its 
> instructions are. So the dSYM uses the real sections from the main executable 
> file instead of its own.


I see, thanks for the explanation. The code makes more sense now.

But now I have another question. Is this an intentional design decision that we 
want to preserve; or just how things happen to work now, and should be cleaned 
up in the future (to achieve the "ObjectFile contains only own sections" 
invariant)?

I'm asking this, because if it's the former then this patch probably needs to 
be redone, as it tries to enforce that invariant (and then needs to jump 
through hoops to enable the ObjectFileMachO use case). OTOH, if it's the 
latter, then I think this is a step in the right direction, as it makes it 
obvious that ObjectFileMachO is doing something that it shouldn't (and at the 
same time it makes it harder for other ObjectFiles to do the same).

BTW, the same situation (symbol file containing empty section definitions for 
code) can happen on elf targets as well (if the symbol file is produced 
with `strip --only-keep-debug`), but everything seems to be working fine, 
presumably because all consumers are accessing the sections through the unified 
section list.


https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In https://reviews.llvm.org/D42955#1026116, @labath wrote:

> I can try to split the patch up a bit if you think it will make things 
> easier.  I'm not sure how much I will be able to do that, as the patch is not 
> that big, it just needs to touch a bunch of files for the changed interfaces.
>
> In https://reviews.llvm.org/D42955#1026061, @clayborg wrote:
>
> > Looks fine, a bit hard to tell exactly what is going on so I will accept as 
> > long as the following things are still true:
> >
> > - each lldb_private::ObjectFile has its own section list that perfectly 
> > mirrors exactly what is in that object file and that file alone
>
>
> This part is not completely true. It is true for ObjectFileELF/COFF/JIT, and 
> is enforced by the fact that the ObjectFile::GetSections does not even have 
> access to the unified section list. However, it is *not* true for 
> ObjectFileMachO (but that is not because of this patch). This is the problem 
> I've had when trying to refactor this (and it's the reason the 
> ObjectFileMachO<->SymbolVendorMacOSX interface is a bit weird). It seems that 
> the ObjectFileMachO sometimes takes a section from the unified list and adds 
> it to it's own list (or at least it appears to be doing that). I don't know 
> enough about MachO to understand why is it doing that, or how to fix it. I've 
> highlighted the places in the code where I think this is happening. Any 
> suggestions would be welcome here.


The dSYM file is a mach-o file that contains symbols only, It is because the 
dSYM file (stand alone debug info file) has all of the section definitions from 
the main executable, but no section content for everything except the DWARF 
debug info. The DWARF only exists in the dSYM file. The dSYM file also has all 
of the symbols as it gets made before the executable is stripped. Many symbols 
refer to a section by section index, so that is why we must have the section 
definitions in the dSYM file. So the dSYM wants the sections from the main 
executable so it can access __text and __data if needed since symbol refer to 
these and someone might ask a symbol from a dSYM file what its instructions 
are. So the dSYM uses the real sections from the main executable file instead 
of its own.

> 
> 
>> - The lldb_private::Module hands out a unified section list that is 
>> populated by the symbol vendor where it uses one or more object files to 
>> create the unified section list
> 
> This part is true.




https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I can try to split the patch up a bit if you think it will make things easier.  
I'm not sure how much I will be able to do that, as the patch is not that big, 
it just needs to touch a bunch of files for the changed interfaces.

In https://reviews.llvm.org/D42955#1026061, @clayborg wrote:

> Looks fine, a bit hard to tell exactly what is going on so I will accept as 
> long as the following things are still true:
>
> - each lldb_private::ObjectFile has its own section list that perfectly 
> mirrors exactly what is in that object file and that file alone


This part is not completely true. It is true for ObjectFileELF/COFF/JIT, and is 
enforced by the fact that the ObjectFile::GetSections does not even have access 
to the unified section list. However, it is *not* true for ObjectFileMachO (but 
that is not because of this patch). This is the problem I've had when trying to 
refactor this (and it's the reason the ObjectFileMachO<->SymbolVendorMacOSX 
interface is a bit weird). It seems that the ObjectFileMachO sometimes takes a 
section from the unified list and adds it to it's own list (or at least it 
appears to be doing that). I don't know enough about MachO to understand why is 
it doing that, or how to fix it. I've highlighted the places in the code where 
I think this is happening. Any suggestions would be welcome here.

> - The lldb_private::Module hands out a unified section list that is populated 
> by the symbol vendor where it uses one or more object files to create the 
> unified section list

This part is true.




Comment at: source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1415-1416
 
   SectionSP unified_section_sp(
   unified_section_list.FindSectionByName(const_segname));
   if (is_dsym && unified_section_sp) {

Here we are grabbing a section from the unified section list.



Comment at: source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1562
   }
   m_sections_ap->AddSection(unified_section_sp);
 }

And here it gets added to the object file's section list.


https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

Looks fine, a bit hard to tell exactly what is going on so I will accept as 
long as the following things are still true:

- each lldb_private::ObjectFile has its own section list that perfectly mirrors 
exactly what is in that object file and that file alone
- The lldb_private::Module hands out a unified section list that is populated 
by the symbol vendor where it uses one or more object files to create the 
unified section list


https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Pavel Labath via Phabricator via lldb-commits]

labath requested review of this revision.
labath added a comment.

Explicitly requesting re-review to make sure this is on your radar.

We have a new patch that  would benefit from 
this, but if this is not ready to go in yet (it's a turned into a fairly big 
refactor), I'll probably add a workaround to lldb-test to make that patch 
testable.


https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 135975.
labath added a comment.

Remove the changes to source/Plugins/SymbolVendor/CMakeLists.txt that snuck in
(I was experimenting with enabling the plugin on non-mac systems, but realized
that's not possible right now).


https://reviews.llvm.org/D42955

Files:
  include/lldb/Core/Module.h
  include/lldb/Symbol/ObjectFile.h
  include/lldb/Symbol/SymbolVendor.h
  include/lldb/lldb-private-interfaces.h
  lit/Modules/Inputs/stripped.yaml
  lit/Modules/Inputs/unstripped.yaml
  lit/Modules/lit.local.cfg
  lit/Modules/unified-section-list.test
  lit/lit.cfg
  source/Core/Module.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.h
  source/Plugins/ObjectFile/JIT/ObjectFileJIT.cpp
  source/Plugins/ObjectFile/JIT/ObjectFileJIT.h
  source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
  source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwp.cpp
  source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
  source/Plugins/SymbolVendor/ELF/SymbolVendorELF.h
  source/Plugins/SymbolVendor/MacOSX/CMakeLists.txt
  source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
  source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.h
  source/Symbol/ObjectFile.cpp
  source/Symbol/SymbolFile.cpp
  source/Symbol/SymbolVendor.cpp
  tools/lldb-test/lldb-test.cpp

Index: tools/lldb-test/lldb-test.cpp
===
--- tools/lldb-test/lldb-test.cpp
+++ tools/lldb-test/lldb-test.cpp
@@ -72,7 +72,6 @@
 
   for (const auto  : opts::module::InputFilenames) {
 ModuleSpec Spec{FileSpec(File, false)};
-Spec.GetSymbolFileSpec().SetFile(File, false);
 
 auto ModulePtr = std::make_shared(Spec);
 SectionList *Sections = ModulePtr->GetSectionList();
Index: source/Symbol/SymbolVendor.cpp
===
--- source/Symbol/SymbolVendor.cpp
+++ source/Symbol/SymbolVendor.cpp
@@ -15,6 +15,7 @@
 // Project includes
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Section.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/SymbolFile.h"
@@ -31,15 +32,16 @@
 // also allow for finding separate debug information files.
 //--
 SymbolVendor *SymbolVendor::FindPlugin(const lldb::ModuleSP _sp,
+   SectionList _list,
lldb_private::Stream *feedback_strm) {
   std::unique_ptr instance_ap;
   SymbolVendorCreateInstance create_callback;
 
   for (size_t idx = 0;
(create_callback = PluginManager::GetSymbolVendorCreateCallbackAtIndex(
 idx)) != nullptr;
++idx) {
-instance_ap.reset(create_callback(module_sp, feedback_strm));
+instance_ap.reset(create_callback(module_sp, unified_list, feedback_strm));
 
 if (instance_ap.get()) {
   return instance_ap.release();
@@ -51,7 +53,9 @@
   if (instance_ap.get()) {
 ObjectFile *objfile = module_sp->GetObjectFile();
 if (objfile)
-  instance_ap->AddSymbolFileRepresentation(objfile->shared_from_this());
+  unified_list = *objfile->GetSectionList();
+
+instance_ap->AddSymbolFileRepresentation(objfile->shared_from_this());
   }
   return instance_ap.release();
 }
Index: source/Symbol/SymbolFile.cpp
===
--- source/Symbol/SymbolFile.cpp
+++ source/Symbol/SymbolFile.cpp
@@ -28,21 +28,6 @@
 SymbolFile *SymbolFile::FindPlugin(ObjectFile *obj_file) {
   std::unique_ptr best_symfile_ap;
   if (obj_file != nullptr) {
-
-// We need to test the abilities of this section list. So create what it
-// would
-// be with this new obj_file.
-lldb::ModuleSP module_sp(obj_file->GetModule());
-if (module_sp) {
-  // Default to the main module section list.
-  ObjectFile *module_obj_file = module_sp->GetObjectFile();
-  if (module_obj_file != obj_file) {
-// Make sure the main object file's sections are created
-module_obj_file->GetSectionList();
-obj_file->CreateSections(*module_sp->GetUnifiedSectionList());
-  }
-}
-
 // TODO: Load any plug-ins in the appropriate plug-in search paths and
 // iterate over all of them to find the best one for the job.
 
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -604,22 +604,6 @@
   }
 }
 
-SectionList *ObjectFile::GetSectionList(bool update_module_section_list) {
-  if (m_sections_ap.get() == nullptr) {
-if (update_module_section_list) {
-  

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: source/Core/Module.cpp:1283-1284
   if (!m_sections_ap) {
-ObjectFile *obj_file = GetObjectFile();
-if (obj_file != nullptr)
+if (ObjectFile *obj_file = GetObjectFile())
   obj_file->CreateSections(*GetUnifiedSectionList());
+if (SymbolVendor *vendor = GetSymbolVendor())

clayborg wrote:
> If we are going to let the symbol vendor do it's thing, then we should 
> probably not manually call ObjectFile::CreateSections. The idea behind 
> SymbolVendor is that _it_ knows how best to do things for one or more object 
> files. Can we move all logic for parsing sections into the 
> SymbolVendor::CreateSections()?
I like that idea. I'm going to take a look to see how that could be done.


https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Back from vacation, sorry for the delay. Only one question on who is actually 
responsible for creating the sections. I vote to let 
SymbolVendor::CreateSection() always do the work. Let me know what you think.




Comment at: source/Core/Module.cpp:1283-1284
   if (!m_sections_ap) {
-ObjectFile *obj_file = GetObjectFile();
-if (obj_file != nullptr)
+if (ObjectFile *obj_file = GetObjectFile())
   obj_file->CreateSections(*GetUnifiedSectionList());
+if (SymbolVendor *vendor = GetSymbolVendor())

If we are going to let the symbol vendor do it's thing, then we should probably 
not manually call ObjectFile::CreateSections. The idea behind SymbolVendor is 
that _it_ knows how best to do things for one or more object files. Can we move 
all logic for parsing sections into the SymbolVendor::CreateSections()?


https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Davide Italiano via Phabricator via lldb-commits]

davide accepted this revision.
davide added a comment.
This revision is now accepted and ready to land.

LGTM.




Comment at: source/Core/Module.cpp:1286
+if (SymbolVendor *vendor = GetSymbolVendor())
+  vendor->CreateSections(*GetUnifiedSectionList());
   }

labath wrote:
> clayborg wrote:
> > should we pass "obj_file" down into the SymbolVendor::CreateSections(...) 
> > call for the case where the symbol vendor is using the same ObjectFile that 
> > it can just return?
> I don't think an extra parameter is needed to achieve this, as this is 
> something that the symbol vendor should already know about based on how it 
> was constructed.
> 
> And after looking up the implementation, it seems this is already how it 
> works right now: SymbolVendorELF (the only non-trivial implementation of this 
> function) is only constructed if it manages to find debug info in an 
> alternative symbol file (there's an `if 
> (llvm::sys::fs::equivalent(candidate_file_spec.GetPath(), 
> module_file_spec.GetPath()))` check in 
> `Symbols::LocateExecutableSymbolFile`). If we fail to construct a 
> SymbolVendorELF then a default SymbolVendor instance is created in 
> `SymbolVendor::FindPlugin` (and that one has a no-op implementation of this 
> function).
> 
I agree with Pavel's reasoning here.


https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Greg, with my last comment in mind, how do you feel about this patch?


https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: source/Core/Module.cpp:1286
+if (SymbolVendor *vendor = GetSymbolVendor())
+  vendor->CreateSections(*GetUnifiedSectionList());
   }

clayborg wrote:
> should we pass "obj_file" down into the SymbolVendor::CreateSections(...) 
> call for the case where the symbol vendor is using the same ObjectFile that 
> it can just return?
I don't think an extra parameter is needed to achieve this, as this is 
something that the symbol vendor should already know about based on how it was 
constructed.

And after looking up the implementation, it seems this is already how it works 
right now: SymbolVendorELF (the only non-trivial implementation of this 
function) is only constructed if it manages to find debug info in an 
alternative symbol file (there's an `if 
(llvm::sys::fs::equivalent(candidate_file_spec.GetPath(), 
module_file_spec.GetPath()))` check in `Symbols::LocateExecutableSymbolFile`). 
If we fail to construct a SymbolVendorELF then a default SymbolVendor instance 
is created in `SymbolVendor::FindPlugin` (and that one has a no-op 
implementation of this function).



https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.
Herald added a subscriber: arichardson.



Comment at: source/Core/Module.cpp:1286
+if (SymbolVendor *vendor = GetSymbolVendor())
+  vendor->CreateSections(*GetUnifiedSectionList());
   }

should we pass "obj_file" down into the SymbolVendor::CreateSections(...) call 
for the case where the symbol vendor is using the same ObjectFile that it can 
just return?


https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: tools/lldb-test/lldb-test.cpp:75
 ModuleSpec Spec{FileSpec(File, false)};
-Spec.GetSymbolFileSpec().SetFile(File, false);
 

The expliciting symbol filespec setting was short-circuiting the regular search 
process. I removed it as there was no test depending on this yet.


https://reviews.llvm.org/D42955



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

[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: clayborg, jingham, davide.
Herald added a subscriber: emaste.

The result of Module::GetSectionList depended on whether the symbol
vendor has been loaded (which would augment the section list with the
extra sections that have been found by the vendor).

Although this bit was documented in the function header, this makes a
quirky api, as other Module functions have no requirement to load the
vendor explicitly -- instead they will do it on demand. In practice,
what this meant is that the user would nearly always get the augmented
section list (because by the time he requested it, it's likely something
would have loaded the vendor already), *except* if all that he was doing
was loading a module and immediately dumping out the sections like
lldb-test does.


https://reviews.llvm.org/D42955

Files:
  include/lldb/Symbol/SymbolVendor.h
  lit/Modules/Inputs/stripped.yaml
  lit/Modules/Inputs/unstripped.yaml
  lit/Modules/lit.local.cfg
  lit/Modules/unified-section-list.test
  lit/lit.cfg
  source/Core/Module.cpp
  source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
  source/Plugins/SymbolVendor/ELF/SymbolVendorELF.h
  tools/lldb-test/lldb-test.cpp

Index: tools/lldb-test/lldb-test.cpp
===
--- tools/lldb-test/lldb-test.cpp
+++ tools/lldb-test/lldb-test.cpp
@@ -72,7 +72,6 @@
 
   for (const auto  : opts::module::InputFilenames) {
 ModuleSpec Spec{FileSpec(File, false)};
-Spec.GetSymbolFileSpec().SetFile(File, false);
 
 auto ModulePtr = std::make_shared(Spec);
 SectionList *Sections = ModulePtr->GetSectionList();
Index: source/Plugins/SymbolVendor/ELF/SymbolVendorELF.h
===
--- source/Plugins/SymbolVendor/ELF/SymbolVendorELF.h
+++ source/Plugins/SymbolVendor/ELF/SymbolVendorELF.h
@@ -26,6 +26,8 @@
 
   ~SymbolVendorELF() override;
 
+  void CreateSections(lldb_private::SectionList ) override;
+
   //--
   // Static Functions
   //--
Index: source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
===
--- source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
+++ source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
@@ -120,47 +120,40 @@
 dsym_objfile_sp->SetType(ObjectFile::eTypeDebugInfo);
 
 SymbolVendorELF *symbol_vendor = new SymbolVendorELF(module_sp);
-if (symbol_vendor) {
-  // Get the module unified section list and add our debug sections to
-  // that.
-  SectionList *module_section_list = module_sp->GetSectionList();
-  SectionList *objfile_section_list = dsym_objfile_sp->GetSectionList();
-
-  static const SectionType g_sections[] = {
-  eSectionTypeDWARFDebugAbbrev,   eSectionTypeDWARFDebugAddr,
-  eSectionTypeDWARFDebugAranges,  eSectionTypeDWARFDebugCuIndex,
-  eSectionTypeDWARFDebugFrame,eSectionTypeDWARFDebugInfo,
-  eSectionTypeDWARFDebugLine, eSectionTypeDWARFDebugLoc,
-  eSectionTypeDWARFDebugMacInfo,  eSectionTypeDWARFDebugPubNames,
-  eSectionTypeDWARFDebugPubTypes, eSectionTypeDWARFDebugRanges,
-  eSectionTypeDWARFDebugStr,  eSectionTypeDWARFDebugStrOffsets,
-  eSectionTypeELFSymbolTable,
-  };
-  for (size_t idx = 0; idx < sizeof(g_sections) / sizeof(g_sections[0]);
-   ++idx) {
-SectionType section_type = g_sections[idx];
-SectionSP section_sp(
-objfile_section_list->FindSectionByType(section_type, true));
-if (section_sp) {
-  SectionSP module_section_sp(
-  module_section_list->FindSectionByType(section_type, true));
-  if (module_section_sp)
-module_section_list->ReplaceSection(module_section_sp->GetID(),
-section_sp);
-  else
-module_section_list->AddSection(section_sp);
-}
-  }
-
-  symbol_vendor->AddSymbolFileRepresentation(dsym_objfile_sp);
-  return symbol_vendor;
-}
+symbol_vendor->AddSymbolFileRepresentation(dsym_objfile_sp);
+return symbol_vendor;
   }
 }
   }
   return NULL;
 }
 
+void SymbolVendorELF::CreateSections(SectionList ) {
+  SectionList *ObjfileList = m_objfile_sp->GetSectionList();
+
+  static constexpr SectionType g_sections[] = {
+  eSectionTypeDWARFDebugAbbrev,   eSectionTypeDWARFDebugAddr,
+  eSectionTypeDWARFDebugAranges,  eSectionTypeDWARFDebugCuIndex,
+  eSectionTypeDWARFDebugFrame,eSectionTypeDWARFDebugInfo,
+  eSectionTypeDWARFDebugLine, eSectionTypeDWARFDebugLoc,
+  eSectionTypeDWARFDebugMacInfo,