This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Read modules from memory when a local copy is not available
ClosedPublic

Authored by wallace on Sep 6 2016, 4:44 PM.

Diff Detail

Event Timeline

wallace updated this revision to Diff 70490.Sep 6 2016, 4:44 PM
wallace retitled this revision from to [lldb] Read modules from memory when a local copy is not available.
wallace updated this object.
wallace added reviewers: zturner, sas.
wallace set the repository for this revision to rL LLVM.
wallace added a project: Restricted Project.
wallace added a reviewer: fjricci.
wallace added a subscriber: Restricted Project.
zturner edited edge metadata.Sep 6 2016, 4:46 PM

Can you rebase on top of tunk and re-upload? Also please use -U999999 when generating the diff (assuming you are using git) so we can see context.

This comment was removed by zturner.
wallace updated this revision to Diff 71265.Sep 13 2016, 4:40 PM
wallace updated this object.
wallace edited edge metadata.

rebase

zturner added inline comments.Sep 13 2016, 4:50 PM
source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
88

auto_ptr should never be used under any circumstances. Prefer std::unique_ptr any time you want to use an auto_ptr.

Also, can you do an early return here?

if (!data_sp || !ObjectFilePECOFF::MagicBytesMatch(data_sp))
  return nullptr;

To create the unique_ptr, the preferred syntax (similar for shared_ptr) is this:

auto objfile_ap = llvm::make_unique<ObjectFilePECOFF>(module_sp, data_sp, process_sp, header_addr);
423

Early return here.

return DataExtractor(buffer_sp, GetByteOrder(), getAddressByteSize());

427

See earlier comment about make_unique. Not critical, but a good habit to get into.

wallace updated this revision to Diff 71407.Sep 14 2016, 11:48 AM

fix pointers

wallace updated this object.Sep 14 2016, 11:56 AM
zturner accepted this revision.Sep 16 2016, 10:08 AM
zturner edited edge metadata.
zturner added inline comments.
source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
88

s/NULL/nullptr/

This revision is now accepted and ready to land.Sep 16 2016, 10:08 AM

I'm obliged, your grace

wallace updated this revision to Diff 71853.Sep 19 2016, 11:17 AM
wallace edited edge metadata.

s/NULL/nullptr/

zturner requested changes to this revision.Sep 20 2016, 1:19 PM
zturner edited edge metadata.

When I run ninja check-lldb with this patch, every single test crashes. It's crashing at ObjectFilePECOFF.cpp:583, which does this:

const char *symbol_name_cstr =
    symtab_data.PeekCStr(name_address - data_start);
symbol_name.assign(symbol_name_cstr);

the symbol name returned is null, which it's trying to assign to a std::string.

I confirmed that without your patch everything works.

The CMake command line I am using is

cmake -G Ninja -DLLDB_TEST_COMPILER=D:\src\llvmbuild\ninja_release\bin\clang.exe -DPYTHON_HOME=C:\Python35

Let me know if you have trouble reproducing this crash, but it will need to be fixed before this CL can go in.

This revision now requires changes to proceed.Sep 20 2016, 1:19 PM

I was using this patch as well https://reviews.llvm.org/D24530, which might be the reason why it fails on your side. I'll double check, but I'm pretty sure it's just that

This revision was automatically updated to reflect the committed changes.
wallace reopened this revision.Sep 20 2016, 3:43 PM
This revision now requires changes to proceed.Sep 20 2016, 3:43 PM
wallace updated this revision to Diff 71995.Sep 20 2016, 3:44 PM
wallace edited edge metadata.

rebase

wallace updated this revision to Diff 72596.Sep 26 2016, 8:00 PM
wallace edited edge metadata.

This time I'm calculating the address of the exports header correctly because it is
an RVA address.

wallace updated this revision to Diff 72597.Sep 26 2016, 8:06 PM

minor nits

I've run the tests and it's the same with and without this diff :)

These are the output from running the tests. Btw, this was failing before because it was reading the export table incorrectly, but I updated it and it should be correct now

zturner accepted this revision.Oct 17 2016, 1:04 PM
zturner edited edge metadata.

This looks fine now. Sorry for the delay, I had forgotten about it. I think you still don't have commit access right? Now would be a good time to request it, and this can be your first commit.

This revision is now accepted and ready to land.Oct 17 2016, 1:04 PM
This revision was automatically updated to reflect the committed changes.