[Lldb-commits] [PATCH] ObjectFileELF::GetSectionHeaderInfo sets arch_spec unnecessarily

[Todd Fiala]

Matthew,

Your patch looks good to me. I would like Greg and/or Ed to have a look at
it too. If they don’t object, I can submit it.

I ran tests on Linux and MacOSX with the patch applied (with a slight
indentation fix to the patch to fix some kind of strange spacing in my
Chrome browser with this line:

arch_spec.GetTriple().setVendorName(Host::GetVendorString().GetCString());

The tests came back clean.
​


On Thu, Jul 10, 2014 at 8:10 AM, Todd Fiala <tfiala at google.com> wrote:

> > Please note that I copied a lot of the PlatformLinux code into
> PlatformKalimba
>
> Oh shucks, I have a boatload of changes going into PlatformLinux to
> support remote debugging!  I've been stuck the last week dealing with
> testing, Windows-related builds/issues, and other minutiae but I am looking
> to getting back to this.  You can keep your eyes on the changes I check in
> for PlatofrmLinux to support llgs.  I'll be refactoring a bunch of code in
> PlatformDarwin into PlatformPOSIX, and changing a few more details in
> PlatformLinux.
>
> > When I invoke "target create kalimba.elf" I actually see ObjectFileELF::
> GetSectionHeaderInfo called *several times* whilst the target is created:
>
> Yeah I have a note to go back to figure out why that's the case.  When I
> added the note support, I saw it getting called several times when it seems
> like it should have been once.  The ObjectFileELF was getting created and
> destroyed multiple times, which I think was unintended.  I'll look closer
> into why that's happening.  Thanks for reminding me.
>
> > It would be great if we could prevent the arch_spec being modified
> again, after the headers are parsed. Either with my patch or something
> similar. Anyway, I'm off now, so I'll catch up sometime tomorrow.
>
> Okay, hopefully by then it will all be better :-)
>
> Thanks,
> Todd
>
>
> On Thu, Jul 10, 2014 at 7:07 AM, Matthew Gardiner <mg11 at csr.com> wrote:
>
>> Todd Fiala wrote:
>>
>>> I'll have a look later this morning. That code indeed is intended to
>>> only be run before they section header parsing. It may have evolved poorly
>>> since that was my initial attempt when I thought all of files had the
>>> header ABI set correctly.
>>>
>>> It is still useful as a fallback if the notes are not present.
>>>
>>> I hadn't yet seen a calling sequence where that static method was called
>>> after parsing the section headers but clearly it sounds like you are
>>> hitting it.
>>>
>>
>> Hi Todd,
>>
>> Yup, It would be cool if you could take a look.
>>
>> When I invoke "target create kalimba.elf" I actually see ObjectFileELF::GetSectionHeaderInfo
>> called *several times* whilst the target is created:
>>
>> The first time...
>>
>> #0 ObjectFileELF::GetSectionHeaderInfo (section_headers=std::vector of
>> length 0, capacity 0, object_data=...,
>> #1 ObjectFileELF::GetModuleSpecifications
>> #2  lldb_private::ObjectFile::GetModuleSpecifications (overloaded
>> function)
>> #3  lldb_private::ObjectFile::GetModuleSpecifications
>> #4  lldb_private::TargetList::CreateTarget
>>
>> It is subsequently called again like this:
>>
>> #0  ObjectFileELF::GetSectionHeaderInfo (section_headers=std::vector of
>> length 18,
>> #1  ObjectFileELF::ParseSectionHeaders (
>> #2  ObjectFileELF::GetArchitecture (
>> #3  lldb_private::Module::GetObjectFile (
>> #4  lldb_private::ModuleList::GetSharedModule (
>> #5  lldb_private::PlatformKalimba::ResolveExecutable (
>> #6  lldb_private::TargetList::CreateTarget (
>>
>> As you can see GSHI gets called again with a non-empty section list when
>> the platform tries to resolve the executable.
>>
>>
>> Please note that I copied a lot of the PlatformLinux code into
>> PlatformKalimba. So the behaviour I see is not specific to my kalimba
>> implementation: loading a linux ELF built for x86_64 execution does the
>> same thing:
>>
>> do:
>>
>> (lldb) target create ~/src/../simple/i64-hello.elf
>>
>> with a breakpoint here in ObjectFileELF::GSHI:
>>
>>     if (!section_headers.empty())
>> -->   return section_headers.size();
>>
>> you'll see this call stack at some stage:
>> #0  ObjectFileELF::GetSectionHeaderInfo (section_headers=std::vector of
>> length 35
>> #1  ObjectFileELF::ParseSectionHeaders (
>> #2  ObjectFileELF::GetArchitecture (
>> #3  lldb_private::Module::GetObjectFile (
>> #4  lldb_private::ModuleList::GetSharedModule (
>> #5  lldb_private::PlatformLinux::ResolveExecutable (
>> #6  lldb_private::TargetList::CreateTarget (
>>
>> It would be great if we could prevent the arch_spec being modified again,
>> after the headers are parsed. Either with my patch or something similar.
>> Anyway, I'm off now, so I'll catch up sometime tomorrow.
>>
>> thanks
>> Matt
>>
>>
>>
>> Member of the CSR plc group of companies. CSR plc registered in England
>> and Wales, registered number 4187346, registered office Churchill House,
>> Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
>> More information can be found at www.csr.com. Keep up to date with CSR
>> on our technical blog, www.csr.com/blog, CSR people blog,
>> www.csr.com/people, YouTube, www.youtube.com/user/CSRplc, Facebook,
>> www.facebook.com/pages/CSR/191038434253534, or follow us on Twitter at
>> www.twitter.com/CSR_plc.
>> New for 2014, you can now access the wide range of products powered by
>> aptX at www.aptx.com.
>>
>
>
>
> --
> Todd Fiala | Software Engineer |  tfiala at google.com |  650-943-3180
>



-- 
Todd Fiala | Software Engineer | tfiala at google.com | 650-943-3180
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140710/aa9c38ec/attachment.html>