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

[Todd Fiala]

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.

On Thursday, July 10, 2014, Matthew Gardiner <mg11 at csr.com> wrote:

> Hi folks,
>
> In my local copy of lldb source I've added a case to the if/else-if of
> ObjectFileELF::RefineModuleDetailsFromNote to handle ELFs built for CSRs
> kalimba dsps. This parses the .note.ABI-tag I added and correctly sets up
> the triple as
>
> Vendor=CSR
> OS=Unknown
>
> This works really well for my kalimba ELFs so thanks to Todd, Ed, Greg and
> anyone else, for getting this code in. (When I eventually get my
> llvm::Triple modifications upstream, I'll try to send to lldb my kalimba
> changes).
>
> However, I have spotted a slight bug in ObjectFileELF::GetSectionHeaderInfo.
> The problem is in this stanza:
>
> // Only initialize the arch_spec to okay defaults...
> // We'll refine this with note data as we parse the notes.
> if (arch_spec.GetTriple ().getOS () == llvm::Triple::OSType::UnknownOS)
> {
>   arch_spec.SetArchitecture (eArchTypeELF..
>   arch_spec.GetTriple().setOSName (Host::GetOSString..
>   arch_spec.GetTriple().setVendorName(Host::GetVendorString..
>
> Initialising the arch_spec this way is fine, when the section headers have
> not been parsed. However, if the section headers have been parsed, then
> this is problematic since the arch_spec is modified and then function is
> exit due to the following statement:
>
> // We have already parsed the section headers
> if (!section_headers.empty())
>   return section_headers.size();
>
> I think if we return from GetSectionHeaderInfo immediately when the
> section_headers are not empty we solve this problem. I've attached a patch
> which does this.
>
> What do people think?
>
> thanks
> Matthew Gardiner
>
>
> Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
> ===================================================================
> --- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp     (revision 212686)
> +++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp     (working copy)
> @@ -1242,6 +1242,10 @@
>                                      uint32_t &gnu_debuglink_crc,
>                                      ArchSpec &arch_spec)
>  {
> +    // We have already parsed the section headers
> +    if (!section_headers.empty())
> +        return section_headers.size();
> +
>      // Only initialize the arch_spec to okay defaults if they're not
> already set.
>      // We'll refine this with note data as we parse the notes.
>      if (arch_spec.GetTriple ().getOS () == llvm::Triple::OSType::
> UnknownOS)
> @@ -1251,10 +1255,6 @@
>
> arch_spec.GetTriple().setVendorName(Host::GetVendorString().GetCString());
>      }
>
> -    // We have already parsed the section headers
> -    if (!section_headers.empty())
> -        return section_headers.size();
> -
>      // If there are no section headers we are done.
>      if (header.e_shnum == 0)
>          return 0;
>
>
>
>
>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140710/96e149f0/attachment.html>