Page MenuHomePhabricator

[JITLoaderGDB] Read jit entry struct manually.
ClosedPublic

Authored by sivachandra on Mar 22 2016, 2:49 PM.

Details

Summary

Though r264012 was fancy enough to make reading the jit entry struct
work with templates, the packing and alignment attributes do not work on
Windows. So, this change makes it plain and simple with manual reading
of the jit entry struct.

Diff Detail

Event Timeline

sivachandra retitled this revision from to [JITLoaderGDB] Read jit entry struct manually..
sivachandra updated this object.
sivachandra added a reviewer: clayborg.
sivachandra added a subscriber: lldb-commits.
clayborg requested changes to this revision.Mar 22 2016, 4:41 PM
clayborg edited edge metadata.

One thing to note: don't use Process::DoReadMemory(), it doesn't cache data and it will always result in a read memory packet going to GDB server if you are using lldb-server and you don't want that. Use Process::ReadMemory(), it will read stuff 512 bytes at a time and caching will work.

The main issue with this patch is your stuff won't work if you debug a program with different endianess that the host system. You need to read all the memory you need in a single memory read and then use a DataExtractor to extract stuff. DataExtractor gets constructed with a variety of data and also takes a byte order and an address byte size. The byte order will allow the extractor to byte swap the data correctly and the address byte size will allow you to extract an pointer using DataExtractor::GetPointer(...). The code for extractor should be much simpler than what you wrote above. Something like:

const size_t addr_size = process->GetAddressByteSize();
const size_t data_byte_size = addr_size * 3 + sizeof(uint64_t);
DataBufferHeap data(data_byte_size, 0);
size_t bytes_read = process->ReadMemory(from_addr, data.GetBytes(), data.GetByteSize(), error);
if (bytes_read == data_byte_size)
{
    DataExtractor extractor (data.GetBytes(), data.GetByteSize(), process->GetByteOrder(), addr_size);
    lldb::offset_t offset = 0;
    entry->next_entry = extractor.GetPointer(&offset);
    entry->prev_entry = extractor.GetPointer(&offset);
    entry->symfile_addr = extractor.GetPointer(&offset);
    entry->symfile_size = extractor.GetU64(&offset);
}
from_addr += offset;

Let me know if you have any questions.

source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
132–161

There are many issues here: no byte swapping, multiple memory reads with no caching. See code in main comment for the simpler code to use.

This revision now requires changes to proceed.Mar 22 2016, 4:41 PM
sivachandra edited edge metadata.

Modify according to suggestions.

How does this look?

clayborg requested changes to this revision.Mar 23 2016, 9:53 AM
clayborg edited edge metadata.

See inlined comments.

source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
113–117

Unless this align is special and different from other align functions, we should use llvm::alignTo(...).

119

Why is "from_addr" passed by reference? No one is updating this address and it is const. This should be "const addr_t from_addr" or it should be non const and "from_addr" should be updated in the call to ReadJITEntry().

122–123

Is this truly for i386 only? Not for all 32 bit architectures?

128

Do you really want to adjust this here? Shouldn't this actually be:

if (from_addr % sizeof(ptr_t))
    return false;
This revision now requires changes to proceed.Mar 23 2016, 9:53 AM
sivachandra edited edge metadata.

Address comments.

sivachandra added inline comments.Mar 23 2016, 1:29 PM
source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
113–117

Removed.

119

Made it a value. Making it a reference was a slip.

122–123

Per my reading of the arm32, mip32 and x86 (i386) ABI specs, only x86 aligns 8 byte values at 4-byte boundaries. Others align 8 byte values are 8-byte boundaries.

For my testing, I have arm32 and i386 Android devices at hand which validate the changes.

You can correct me if you think I got it wrong or if I have to consider more archs.

128

Replaced this with an assert.

clayborg accepted this revision.Mar 23 2016, 1:33 PM
clayborg edited edge metadata.

Looks good. Thanks for all the fixes.

This revision is now accepted and ready to land.Mar 23 2016, 1:33 PM
sivachandra closed this revision.Mar 23 2016, 4:32 PM