[JITLoaderGDB] Read jit entry struct manually.

[sivachandra]

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.

[clayborg]

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.

[sivachandra]

Modify according to suggestions.

[sivachandra]

How does this look?

[clayborg]

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;

[sivachandra]

Address comments.

[sivachandra]

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]

Looks good. Thanks for all the fixes.

source/Plugins/JITLoader/GDB/JITLoaderGDB.h

private:
     SetJITBreakpoint(lldb_private::ModuleList &module_list);
 
     bool
     DidSetJITBreakpoint() const;
 
     bool
     ReadJITDescriptor(bool all_entries);
 
-    template <typename ptr_t, bool packed>
+    template <typename ptr_t>
     bool
     ReadJITDescriptorImpl(bool all_entries);
 
     static bool
     JITDebugBreakpointHit(void *baton,
                           lldb_private::StoppointCallbackContext *context,
                           lldb::user_id_t break_id,
                           lldb::user_id_t break_loc_id);

source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp

 //===-- JITLoaderGDB.cpp ----------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // C Includes
 
+#include "llvm/Support/MathExtras.h"
+
 #include "lldb/Breakpoint/Breakpoint.h"
+#include "lldb/Core/DataBufferHeap.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Log.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Core/StreamString.h"
 #include "lldb/Interpreter/OptionValueProperties.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
+#include "lldb/Utility/LLDBAssert.h"
 
 #include "JITLoaderGDB.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
+//------------------------------------------------------------------
+// Debug Interface Structures
+//------------------------------------------------------------------
+typedef enum
+{
+    JIT_NOACTION = 0,
+    JIT_REGISTER_FN,
+    JIT_UNREGISTER_FN
+} jit_actions_t;
+
+template <typename ptr_t>
+struct jit_code_entry
+{
+    ptr_t    next_entry; // pointer
+    ptr_t    prev_entry; // pointer
+    ptr_t    symfile_addr; // pointer
+    uint64_t symfile_size;
+};
+
+template <typename ptr_t>
+struct jit_descriptor
+{
+    uint32_t version;
+    uint32_t action_flag; // Values are jit_action_t
+    ptr_t    relevant_entry; // pointer
+    ptr_t    first_entry; // pointer
+};
+
 namespace {
 
     PropertyDefinition
     g_properties[] =
     {
         { "enable-jit-breakpoint", OptionValue::eTypeBoolean, true,  true , nullptr, nullptr, "Enable breakpoint on __jit_debug_register_code." },
         {  nullptr               , OptionValue::eTypeInvalid, false, 0,     nullptr, nullptr, nullptr }
     };
namespace {
 
     static const JITLoaderGDBPropertiesSP&
     GetGlobalPluginProperties()
     {
         static const auto g_settings_sp(std::make_shared<PluginProperties>());
         return g_settings_sp;
     }
 
-}  // anonymous namespace end
-
-//------------------------------------------------------------------
-// Debug Interface Structures
-//------------------------------------------------------------------
-typedef enum
+    template <typename ptr_t>
+    bool ReadJITEntry(const addr_t from_addr, Process *process, jit_code_entry<ptr_t> *entry)
     {
-    JIT_NOACTION = 0,
-    JIT_REGISTER_FN,
-    JIT_UNREGISTER_FN
-} jit_actions_t;
+        lldbassert(from_addr % sizeof(ptr_t) == 0);
 
-template <typename ptr_t, bool packed_size>
-struct jit_code_entry
-{
-    ptr_t    next_entry; // pointer
-    ptr_t    prev_entry; // pointer
-    ptr_t    symfile_addr; // pointer
-    uint64_t symfile_size __attribute__ ((aligned (8)));
-};
+        ArchSpec::Core core = process->GetTarget().GetArchitecture().GetCore();
+        bool i386_target = ArchSpec::kCore_x86_32_first <= core && core <= ArchSpec::kCore_x86_32_last;
+        uint8_t uint64_align_bytes =  i386_target ? 4 : 8;
+        const size_t data_byte_size = llvm::alignTo(sizeof(ptr_t) * 3, uint64_align_bytes) + sizeof(uint64_t);
 
-template <typename ptr_t>
-struct jit_code_entry<ptr_t, true>
-{
-    ptr_t    next_entry; // pointer
-    ptr_t    prev_entry; // pointer
-    ptr_t    symfile_addr; // pointer
-    uint64_t symfile_size __attribute__ ((packed));
-};
+        Error error;
+        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 || !error.Success())
+            return false;
 
-template <typename ptr_t>
-struct jit_descriptor
-{
-    uint32_t version;
-    uint32_t action_flag; // Values are jit_action_t
-    ptr_t    relevant_entry; // pointer
-    ptr_t    first_entry; // pointer
-};
+        DataExtractor extractor (data.GetBytes(), data.GetByteSize(), process->GetByteOrder(), sizeof(ptr_t));
+        lldb::offset_t offset = 0;
+        entry->next_entry = extractor.GetPointer(&offset);
+        entry->prev_entry = extractor.GetPointer(&offset);
+        entry->symfile_addr = extractor.GetPointer(&offset);
+        offset = llvm::alignTo(offset, uint64_align_bytes);
+        entry->symfile_size = extractor.GetU64(&offset);
+
+        return true;
+    }
+
+}  // anonymous namespace end
 
 JITLoaderGDB::JITLoaderGDB (lldb_private::Process *process) :
     JITLoader(process),
     m_jit_objects(),
     m_jit_break_id(LLDB_INVALID_BREAK_ID),
     m_jit_descriptor_addr(LLDB_INVALID_ADDRESS)
 {
 }
 
 JITLoaderGDB::~JITLoaderGDB ()
 {
     if (LLDB_BREAK_ID_IS_VALID(m_jit_break_id))
         m_process->GetTarget().RemoveBreakpointByID (m_jit_break_id);
 }
 
 void
 JITLoaderGDB::DebuggerInitialize(Debugger &debugger)
 {
     if (!PluginManager::GetSettingForJITLoaderPlugin(debugger, PluginProperties::GetSettingName()))
     {
         const bool is_global_setting = true;
         PluginManager::CreateSettingForJITLoaderPlugin(debugger,
                                                        GetGlobalPluginProperties()->GetValueProperties(),
                                                        ConstString ("Properties for the JIT LoaderGDB plug-in."),
                                                        is_global_setting);
     }
 }
 
 void JITLoaderGDB::DidAttach()
for (uint32_t i = 0; i<num_sections; ++i)
             }
         }
     }
 }
 
 bool
 JITLoaderGDB::ReadJITDescriptor(bool all_entries)
 {
-    Target &target = m_process->GetTarget();
-    const ArchSpec &arch_spec = target.GetArchitecture();
-    if (arch_spec.GetAddressByteSize() == 8)
-        return ReadJITDescriptorImpl<uint64_t, false>(all_entries);
-    else
-    {
-        ArchSpec::Core core = arch_spec.GetCore();
-        if (ArchSpec::kCore_x86_32_first <= core && core <= ArchSpec::kCore_x86_32_last)
-            return ReadJITDescriptorImpl<uint32_t, true>(all_entries);
-        else
-            return ReadJITDescriptorImpl<uint32_t, false>(all_entries);
-    }
+    if (m_process->GetTarget().GetArchitecture().GetAddressByteSize() == 8)
+        return ReadJITDescriptorImpl<uint64_t>(all_entries);
+        return ReadJITDescriptorImpl<uint32_t>(all_entries);
 }
 
-template <typename ptr_t, bool packed>
+template <typename ptr_t>
 bool
 JITLoaderGDB::ReadJITDescriptorImpl(bool all_entries)
 {
     if (m_jit_descriptor_addr == LLDB_INVALID_ADDRESS)
         return false;
 
     Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_JIT_LOADER));
     Target &target = m_process->GetTarget();
JITLoaderGDB::ReadJITDescriptorImpl(bool all_entries)
     if (all_entries)
     {
         jit_action = JIT_REGISTER_FN;
         jit_relevant_entry = (addr_t)jit_desc.first_entry;
     }
 
     while (jit_relevant_entry != 0)
     {
-        jit_code_entry<ptr_t, packed> jit_entry;
-        const size_t jit_entry_size = sizeof(jit_entry);
-        bytes_read = m_process->DoReadMemory(jit_relevant_entry, &jit_entry, jit_entry_size, error);
-        if (bytes_read != jit_entry_size || !error.Success())
+        jit_code_entry<ptr_t> jit_entry;
+        if (!ReadJITEntry(jit_relevant_entry, m_process, &jit_entry))
         {
             if (log)
                 log->Printf(
                     "JITLoaderGDB::%s failed to read JIT entry at 0x%" PRIx64,
                     __FUNCTION__, jit_relevant_entry);
             return false;
         }