Re: [Lldb-commits] [lldb] r324254 - Fix parsing of object files with "early" section headers

[Davide Italiano via lldb-commits]

On Mon, Feb 5, 2018 at 10:09 AM, Pavel Labath  wrote:
> Committed as r324256. Thanks for keeping me honest.
>
> Going back to this, I noticed that I did not check-in the final tiny
> binary that I intended to, which happened because I was rushing to get
> the build-id test fixed, so I could get back to what I originally
> planned to do for today, and the fact that I had to fix a completely
> unrelated bug to do that got me upset.
>
> I'll have to be more careful next time.
>

No worries, it happens to all of us :)

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

Re: [Lldb-commits] [lldb] r324254 - Fix parsing of object files with "early" section headers

[Pavel Labath via lldb-commits]

Committed as r324256. Thanks for keeping me honest.

Going back to this, I noticed that I did not check-in the final tiny
binary that I intended to, which happened because I was rushing to get
the build-id test fixed, so I could get back to what I originally
planned to do for today, and the fact that I had to fix a completely
unrelated bug to do that got me upset.

I'll have to be more careful next time.

On 5 February 2018 at 17:42, Davide Italiano  wrote:
> On Mon, Feb 5, 2018 at 9:25 AM, Pavel Labath via lldb-commits
>  wrote:
>> Author: labath
>> Date: Mon Feb  5 09:25:40 2018
>> New Revision: 324254
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=324254=rev
>> Log:
>> Fix parsing of object files with "early" section headers
>>
>> ObjectFileELF::GetModuleSpecifications contained a lot of tip-toing code
>> which was trying to avoid loading the full object file into memory. It
>> did this by trying to load data only up to the offset if was accessing.
>> However, in practice this was useless, as 99% of object files we
>> encounter have section headers at the end, so we would load the whole
>> file as soon as we start parsing the section headers.
>>
>> In fact, this would break as soon as we encounter a file which does
>> *not* have section headers at the end (yaml2obj produces these), as the
>> access to .strtab (which we need to get the section names) was not
>> guarded by this offset check.
>>
>> As this strategy was completely ineffective anyway, I do not attempt to
>> proliferate it further by guarding the .strtab accesses. Instead I just
>> lead the full file as soon as we are reasonably sure that we are indeed
>> processing an elf file.
>>
>> If we really care about the load size here, we would need to reimplement
>> this to just load the bits of the object file we need, instead of
>> loading everything from the start of the object file to the given
>> offset. However, given that the OS will do this for us for free when
>> using mmap, I think think this is really necessary.
>>
>> For testing this I check a (tiny) SO file instead of yaml2obj-ing it
>> because the fact that they come out first is an implementation detail of
>> yaml2obj that can change in the future.
>>
>
> I personally don't mind at all checking shared libraries directly if
> it saves us some time, but, FWIW, can you also add a comment to the
> test explaining how to generate the shared object?
> e.g. something like run `yaml2obj` on the following <> (with yaml2obj
> rev123456) [or, if you used a c file, just add the c file to a comment
> & the compiler/linker invocation).
> This worked us very well for `llvm-mc` tests in the past, if for any
> reason whatsoever we have to regenerate them.
>
> Thank you!
>
> --
> Davide
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [lldb] r324254 - Fix parsing of object files with "early" section headers

[Davide Italiano via lldb-commits]

On Mon, Feb 5, 2018 at 9:25 AM, Pavel Labath via lldb-commits
 wrote:
> Author: labath
> Date: Mon Feb  5 09:25:40 2018
> New Revision: 324254
>
> URL: http://llvm.org/viewvc/llvm-project?rev=324254=rev
> Log:
> Fix parsing of object files with "early" section headers
>
> ObjectFileELF::GetModuleSpecifications contained a lot of tip-toing code
> which was trying to avoid loading the full object file into memory. It
> did this by trying to load data only up to the offset if was accessing.
> However, in practice this was useless, as 99% of object files we
> encounter have section headers at the end, so we would load the whole
> file as soon as we start parsing the section headers.
>
> In fact, this would break as soon as we encounter a file which does
> *not* have section headers at the end (yaml2obj produces these), as the
> access to .strtab (which we need to get the section names) was not
> guarded by this offset check.
>
> As this strategy was completely ineffective anyway, I do not attempt to
> proliferate it further by guarding the .strtab accesses. Instead I just
> lead the full file as soon as we are reasonably sure that we are indeed
> processing an elf file.
>
> If we really care about the load size here, we would need to reimplement
> this to just load the bits of the object file we need, instead of
> loading everything from the start of the object file to the given
> offset. However, given that the OS will do this for us for free when
> using mmap, I think think this is really necessary.
>
> For testing this I check a (tiny) SO file instead of yaml2obj-ing it
> because the fact that they come out first is an implementation detail of
> yaml2obj that can change in the future.
>

I personally don't mind at all checking shared libraries directly if
it saves us some time, but, FWIW, can you also add a comment to the
test explaining how to generate the shared object?
e.g. something like run `yaml2obj` on the following <> (with yaml2obj
rev123456) [or, if you used a c file, just add the c file to a comment
& the compiler/linker invocation).
This worked us very well for `llvm-mc` tests in the past, if for any
reason whatsoever we have to regenerate them.

Thank you!

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

[Lldb-commits] [lldb] r324254 - Fix parsing of object files with "early" section headers

[Pavel Labath via lldb-commits]

Author: labath
Date: Mon Feb  5 09:25:40 2018
New Revision: 324254

URL: http://llvm.org/viewvc/llvm-project?rev=324254=rev
Log:
Fix parsing of object files with "early" section headers

ObjectFileELF::GetModuleSpecifications contained a lot of tip-toing code
which was trying to avoid loading the full object file into memory. It
did this by trying to load data only up to the offset if was accessing.
However, in practice this was useless, as 99% of object files we
encounter have section headers at the end, so we would load the whole
file as soon as we start parsing the section headers.

In fact, this would break as soon as we encounter a file which does
*not* have section headers at the end (yaml2obj produces these), as the
access to .strtab (which we need to get the section names) was not
guarded by this offset check.

As this strategy was completely ineffective anyway, I do not attempt to
proliferate it further by guarding the .strtab accesses. Instead I just
lead the full file as soon as we are reasonably sure that we are indeed
processing an elf file.

If we really care about the load size here, we would need to reimplement
this to just load the bits of the object file we need, instead of
loading everything from the start of the object file to the given
offset. However, given that the OS will do this for us for free when
using mmap, I think think this is really necessary.

For testing this I check a (tiny) SO file instead of yaml2obj-ing it
because the fact that they come out first is an implementation detail of
yaml2obj that can change in the future.

Added:
lldb/trunk/unittests/ObjectFile/ELF/Inputs/early-section-headers.so
Modified:
lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
lldb/trunk/unittests/ObjectFile/ELF/CMakeLists.txt
lldb/trunk/unittests/ObjectFile/ELF/TestObjectFileELF.cpp

Modified: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp?rev=324254=324253=324254=diff
==
--- lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp (original)
+++ lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp Mon Feb  5 
09:25:40 2018
@@ -674,29 +674,16 @@ size_t ObjectFileELF::GetModuleSpecifica
   __FUNCTION__, file.GetPath().c_str());
   }
 
+  data_sp = MapFileData(file, -1, file_offset);
+  if (data_sp)
+data.SetData(data_sp);
   // In case there is header extension in the section #0, the header
   // we parsed above could have sentinel values for e_phnum, e_shnum,
   // and e_shstrndx.  In this case we need to reparse the header
   // with a bigger data source to get the actual values.
-  size_t section_header_end = header.e_shoff + header.e_shentsize;
-  if (header.HasHeaderExtension() &&
-section_header_end > data_sp->GetByteSize()) {
-data_sp = MapFileData(file, section_header_end, file_offset);
-if (data_sp) {
-  data.SetData(data_sp);
-  lldb::offset_t header_offset = data_offset;
-  header.Parse(data, _offset);
-}
-  }
-
-  // Try to get the UUID from the section list. Usually that's at the
-  // end, so map the file in if we don't have it already.
-  section_header_end =
-  header.e_shoff + header.e_shnum * header.e_shentsize;
-  if (section_header_end > data_sp->GetByteSize()) {
-data_sp = MapFileData(file, section_header_end, file_offset);
-if (data_sp)
-  data.SetData(data_sp);
+  if (header.HasHeaderExtension()) {
+lldb::offset_t header_offset = data_offset;
+header.Parse(data, _offset);
   }
 
   uint32_t gnu_debuglink_crc = 0;
@@ -733,39 +720,14 @@ size_t ObjectFileELF::GetModuleSpecifica
   // contents crc32 would be too much of luxury.  Thus we will need
   // to fallback to something simpler.
   if (header.e_type == llvm::ELF::ET_CORE) {
-size_t program_headers_end =
-header.e_phoff + header.e_phnum * header.e_phentsize;
-if (program_headers_end > data_sp->GetByteSize()) {
-  data_sp = MapFileData(file, program_headers_end, 
file_offset);
-  if (data_sp)
-data.SetData(data_sp);
-}
 ProgramHeaderColl program_headers;
 GetProgramHeaderInfo(program_headers, data, header);
 
-size_t segment_data_end = 0;
-for (ProgramHeaderCollConstIter I = program_headers.begin();
- I != program_headers.end(); ++I) {
-  segment_data_end = std::max(
-  I->p_offset + I->p_filesz,