> On Dec 10, 2018, at 3:11 PM, Leonard Mosescu <mose...@google.com> wrote:
> 
> I can see how this works for the PDB, no-module-binary case. What about the 
> PDB & module-binary case?

That would work fine because the symbol vendor will make an object file from 
the m_symfile_spec and use both the original binary + the symbol file to make 
symbols.

> Maybe I missed the rationale, but I think that modeling the general case: 
> module and symbols are separate files, is a better foundation for the 
> particular case where the symbols are embedded in the binary file.

Yeah, my case should handle the following cases:
- Minidumps Placeholder modules only, no real binaries, add symbols by 
downloading them and using "target symbols add ..."
- Download real binaries up front and load them into LLDB, then load the core 
file. For any binaries we do have, we should grab them from the global module 
cache (GetTarget().GetSharedModule(...) in the current code) and they will use 
real object files, not placeholder files. "target symbols add ..." still works 
in this case

But we can easily layer on top of your PlaceholderModule and the placeholder 
object file if needed. 

Greg

> 
> 
> On Mon, Dec 10, 2018 at 2:48 PM Greg Clayton via Phabricator 
> <revi...@reviews.llvm.org <mailto:revi...@reviews.llvm.org>> wrote:
> clayborg added a comment.
> 
> FYI: my approach to getting symbols to load was a bit different. I always let 
> a simple PlaceholderModule be created, but I played some tricks in the 
> GetObjectFile() method if someone had setting symbols for the module with 
> "target symbols add ..". I will attach my PlaceholderModule so you can see 
> what I had done. Since these modules always must be associated with a target, 
> I keep a weak pointer to the target in the constructor. Then later, if 
> someone does "target symbols add ..." the module will have Module:: 
> m_symfile_spec filled in, so I set the m_platform_file to be m_file, and then 
> move m_symfile_spec into m_file and then call Module::GetObjectFile(). This 
> is nice and clean because you don't have to make up fake symbols to fake 
> sections. When you create the placeholder module it needs the target:
> 
>   auto placeholder_module =
>       std::make_shared<PlaceholderModule>(module_spec, GetTarget());
> 
> Here is the copy of the PlaceholderModule that does what was discussed above:
> 
>   //------------------------------------------------------------------
>   /// A placeholder module used for minidumps, where the original
>   /// object files may not be available (so we can't parse the object
>   /// files to extract the set of sections/segments)
>   ///
>   /// This placeholder module has a single synthetic section (.module_image)
>   /// which represents the module memory range covering the whole module.
>   //------------------------------------------------------------------
>   class PlaceholderModule : public Module {
>   public:
>     PlaceholderModule(const ModuleSpec &module_spec, Target& target) :
>       Module(module_spec.GetFileSpec(), module_spec.GetArchitecture()),
>       m_target_wp(target.shared_from_this()),
>       m_base_of_image(LLDB_INVALID_ADDRESS), m_size_of_image(0) {
>       if (module_spec.GetUUID().IsValid())
>         SetUUID(module_spec.GetUUID());
>     }
> 
>     // Creates a synthetic module section covering the whole module image (and
>     // sets the section load address as well)
>     void CreateImageSection(const MinidumpModule *module) {
>       m_base_of_image = module->base_of_image;
>       m_size_of_image = module->size_of_image;
>       TargetSP target_sp = m_target_wp.lock();
>       if (!target_sp)
>         return;
>       const ConstString section_name(".module_image");
>       lldb::SectionSP section_sp(new Section(
>           shared_from_this(),     // Module to which this section belongs.
>           nullptr,                // ObjectFile
>           0,                      // Section ID.
>           section_name,           // Section name.
>           eSectionTypeContainer,  // Section type.
>           module->base_of_image,  // VM address.
>           module->size_of_image,  // VM size in bytes of this section.
>           0,                      // Offset of this section in the file.
>           module->size_of_image,  // Size of the section as found in the file.
>           12,                     // Alignment of the section (log2)
>           0,                      // Flags for this section.
>           1));                    // Number of host bytes per target byte
>       section_sp->SetPermissions(ePermissionsExecutable | 
> ePermissionsReadable);
>       GetSectionList()->AddSection(section_sp);
>       target_sp->GetSectionLoadList().SetSectionLoadAddress(
>           section_sp, module->base_of_image);
>     }
> 
>     ObjectFile *GetObjectFile() override {
>       // Since there is no object file for these place holder modules, check
>       // if the symbol file spec has been set, and if so, then transfer it 
> over
>       // to the file spec so the module can make a real object file out of it.
>       if (m_symfile_spec) {
>         // We need to load the sections once. We check of m_objfile_sp is 
> valid.
>         // If it is, we already have loaded the sections. If it isn't, we will
>         // load the sections.
>         const bool load_sections = !m_objfile_sp;
>         if (load_sections) {
>           m_platform_file = m_file;
>           m_file = m_symfile_spec;
>         }
>         ObjectFile *obj_file = Module::GetObjectFile();
>         if (load_sections && obj_file) {
>           TargetSP target_sp = m_target_wp.lock();
>           // The first time we create the object file from the external symbol
>           // file, we must load its sections and unload the ".module_image"
>           // section
>           bool changed;
>           SetLoadAddress(*target_sp, m_base_of_image, false, changed);
>         }
>         return obj_file;
>       }
>       return nullptr;
>     }
> 
>     SectionList *GetSectionList() override {
>       return Module::GetUnifiedSectionList();
>     }
>   protected:
>     // In case we need to load the sections in 
> PlaceholderModule::GetObjectFile()
>     // after a symbol file has been specified, we might need the target.
>     lldb::TargetWP m_target_wp;
>     lldb::addr_t m_base_of_image;
>     lldb::addr_t m_size_of_image;
>   };
> 
> 
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D55142/new/ <https://reviews.llvm.org/D55142/new/>
> 
> https://reviews.llvm.org/D55142 <https://reviews.llvm.org/D55142>
> 
> 
> 

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

Reply via email to